Re: [STGIT][PATCH] new: translate non word characters in patch name to '-'

From: Karl Hasselström <kha@treskal.com>
Date: Dec 31, 2008, 10:38 UTC
Message-ID: <20081231103827.GA29195@diana.vm.bytemark.co.uk>
In-Reply-To: <154e089b0812310007j6642af70v4bfd64b784da8b4c@mail.gmail.com>

On 2008-12-31 09:07:51 +0100, Hannes Eder wrote:

Show 8 quoted lines

> On Mon, Dec 29, 2008 at 10:21 PM, Karl Hasselström <kha@treskal.com> wrote:
>
> > There's a small inconsistency: you fail if the name contains "..",
> > but correct single bad characters.
>
> ".." is used to denote patch name ranges [<patch1>..<patch2>] for
> commands like "stg pop", "stg push", and so forth, therefore I think
> it is wise to exclude ".." from single patch names [<patch3>].

Yes. But what I meant was that it's a tad inconsistent to fail on some illegal patch names, and correct others. You should either always fail (my preference), or always correct.

> Maybe we should start defining what a 'valid' patch name has to look
> like, i.e. define
>
> def is_patch_name_valid(patchname)

Yes, exactly. This function could be called

  1. when validating user input in e.g. "stg new";

  2. in an assert at the end of the function that constructs a patch
     name from the commit message; and

  3. in an assert just before we try to actually create a patch with a
     given name.

(2) and (3) aren't really necessary, of course; they're just there to catch bugs.

If you define such a function, be liberal in forbidding stuff. It's easy to relax the rules later, but hard to tighten them since we have to deal with existing repositories with illegal patch names. It's probably a good idea to look at what git allows in its ref names, and additionally forbid "/" and anything else you can think of.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle