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

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

[Hannes Eder]

This allows following usage:

$ stg new full/path/file-fix-foobar
Now at patch "full-path-file-fix-foobar"

Signed-off-by: Hannes Eder <hannes@hanneseder.net>
---

I ran into as a '/' in a patch messed up stgit.

I find this useful as 'stg uncommit' does the same translation.

 stgit/commands/new.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/stgit/commands/new.py b/stgit/commands/new.py
index 151cfe9..ab09476 100644
--- a/stgit/commands/new.py
+++ b/stgit/commands/new.py
@@ -58,7 +58,7 @@ def func(parser, options, args):
     if len(args) == 0:
         name = None
     elif len(args) == 1:
-        name = args[0]
+        name = utils.patch_name_from_msg(args[0])
         if stack.patches.exists(name):
             raise common.CmdException('%s: patch already exists' % name)
     else:

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

[Karl Hasselström]

On 2008-12-27 13:37:20 +0100, Hannes Eder wrote:

> This allows following usage:
>
> $ stg new full/path/file-fix-foobar
> Now at patch "full-path-file-fix-foobar"
>
> Signed-off-by: Hannes Eder <hannes@hanneseder.net>
> ---
>
> I ran into as a '/' in a patch messed up stgit.
>
> I find this useful as 'stg uncommit' does the same translation.

Clearly, bad path names shouldn't mess anything up -- see

  https://gna.org/bugs/?10919

But I would prefer "stg new" to quit with an error message when given
an illegal patch name, not silently mangle it. (Of course, the
commands that generate patch names from commit messages -- such as
"stg new" when not given an explicit patch name -- should mangle the
commit message as much as necessary. But when the user gives us a
patch name, we should either use that as is or fail with an
informative message.)

I think the right thing to do would be to construct a function that
validates patch names (I don't think we have one right now), and then
call it whenever we need to check if a patch name is OK. Such as when
the user gives us the name of a new patch. And when we've
auto-generated a patch name from a commit message, we should probably
assert that that the check function is OK with the name.

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

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

[Hannes Eder]

On 12/28/08, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-12-27 13:37:20 +0100, Hannes Eder wrote:
>
>  > This allows following usage:
>  >
>  > $ stg new full/path/file-fix-foobar
>  > Now at patch "full-path-file-fix-foobar"
>  >
>  > Signed-off-by: Hannes Eder <hannes@hanneseder.net>
>  > ---
>  >
>  > I ran into as a '/' in a patch messed up stgit.
>  >
>  > I find this useful as 'stg uncommit' does the same translation.
>
>
> Clearly, bad path names shouldn't mess anything up -- see
>
>   https://gna.org/bugs/?10919
>
>  But I would prefer "stg new" to quit with an error message when given
>  an illegal patch name, not silently mangle it. (Of course, the
>  commands that generate patch names from commit messages -- such as
>  "stg new" when not given an explicit patch name -- should mangle the
>  commit message as much as necessary. But when the user gives us a
>  patch name, we should either use that as is or fail with an
>  informative message.)
>
>  I think the right thing to do would be to construct a function that
>  validates patch names (I don't think we have one right now), and then
>  call it whenever we need to check if a patch name is OK. Such as when
>  the user gives us the name of a new patch. And when we've
>  auto-generated a patch name from a commit message, we should probably
>  assert that that the check function is OK with the name.

What about instead of 'fail with an informative message', just issue a
warning that
the name has been mangled. I use pathnames in patch names frequently,
so this would be very handy.
I guess some with with more python skills needs to clean that up, this
is the first stuff I do in python ;).

---

diff --git a/stgit/commands/new.py b/stgit/commands/new.py
index 151cfe9..ed5c9ce 100644
--- a/stgit/commands/new.py
+++ b/stgit/commands/new.py
@@ -58,7 +58,7 @@ def func(parser, options, args):
     if len(args) == 0:
         name = None
     elif len(args) == 1:
-        name = args[0]
+        name = utils.sanitize_patch_name(args[0])
         if stack.patches.exists(name):
             raise common.CmdException('%s: patch already exists' % name)
     else:
diff --git a/stgit/commands/rename.py b/stgit/commands/rename.py
index 8a593ac..455b67e 100644
--- a/stgit/commands/rename.py
+++ b/stgit/commands/rename.py
@@ -50,6 +50,8 @@ def func(parser, options, args):
         old, [new] = crt, args
     else:
         parser.error('incorrect number of arguments')
+
+    new = utils.sanitize_patch_name(new)

     out.start('Renaming patch "%s" to "%s"' % (old, new))
     crt_series.rename_patch(old, new)
diff --git a/stgit/utils.py b/stgit/utils.py
index 81035a5..8ffe5a3 100644
--- a/stgit/utils.py
+++ b/stgit/utils.py
@@ -231,6 +231,31 @@ def make_patch_name(msg, unacceptable,
default_name = 'patch'):
         patchname = default_name
     return find_patch_name(patchname, unacceptable)

+class PatchNameException(StgException):
+    pass
+
+def sanitize_patch_name(patchname):
+    # '..' is for patch ranges only, so it is not allowed here
+    if patchname.find('..') != -1:
+         raise PatchNameException('Patch name must not contain "..".')
+
+    # if patch name contains non word characters, replace them but warn user
+    # about that
+    if re.search('\W', patchname):
+        out.warn('replacing non word characters in patch name');
+        patchname = re.sub('[\W]+', '-', patchname).strip('-')
+
+    # limit patch name length
+    name_len = config.get('stgit.namelength')
+    if not name_len:
+        name_len = 30
+
+    if len(patchname) > name_len:
+        out.warn('truncating the patch name to %d characters' % name_len)
+        patchname = patchname[:name_len]
+
+    return patchname
+
 # any and all functions are builtin in Python 2.5 and higher, but not
 # in 2.4.
 if not 'any' in dir(__builtins__):

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

[Karl Hasselström]

On 2008-12-29 21:15:44 +0100, Hannes Eder wrote:

> What about instead of 'fail with an informative message', just issue
> a warning that the name has been mangled. I use pathnames in patch
> names frequently, so this would be very handy.

Hmm, I'm still skeptical. Programs that try to be too clever all too
often end up just being annoying and unpredictable.

> I guess some with with more python skills needs to clean that up,
> this is the first stuff I do in python ;).

The code looks OK -- but as I said, I don't agree with what it's
trying to do.

There's a small inconsistency: you fail if the name contains "..", but
correct single bad characters.

And as I recall, stgit.namelength is about the automatically generated
patch names -- there's no limit for the names provided by the user.

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

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

[Hannes Eder]

On Mon, Dec 29, 2008 at 10:21 PM, Karl Hasselström <kha@treskal.com> wrote:
> On 2008-12-29 21:15:44 +0100, Hannes Eder wrote:
>
>> What about instead of 'fail with an informative message', just issue
>> a warning that the name has been mangled. I use pathnames in patch
>> names frequently, so this would be very handy.
>
> Hmm, I'm still skeptical. Programs that try to be too clever all too
> often end up just being annoying and unpredictable.
>
>> I guess some with with more python skills needs to clean that up,
>> this is the first stuff I do in python ;).
>
> The code looks OK -- but as I said, I don't agree with what it's
> trying to do.
>
> 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>].

> And as I recall, stgit.namelength is about the automatically generated
> patch names -- there's no limit for the names provided by the user.

Ok.

Maybe we should start defining what a 'valid' patch name has to look
like, i.e. define

def is_patch_name_valid(patchname)

Best,
Hannes

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

[Karl Hasselström]

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

> 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

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

[David Kågedal]

"Hannes Eder" <hannes@hanneseder.net> writes:

> On 12/28/08, Karl Hasselström <kha@treskal.com> wrote:
>> On 2008-12-27 13:37:20 +0100, Hannes Eder wrote:
>>
>>  > This allows following usage:
>>  >
>>  > $ stg new full/path/file-fix-foobar
>>  > Now at patch "full-path-file-fix-foobar"
>>  >
>>  > Signed-off-by: Hannes Eder <hannes@hanneseder.net>
>>  > ---
>>  >
>>  > I ran into as a '/' in a patch messed up stgit.
>>  >
>>  > I find this useful as 'stg uncommit' does the same translation.
>>
>>
>> Clearly, bad path names shouldn't mess anything up -- see
>>
>>   https://gna.org/bugs/?10919
>>
>>  But I would prefer "stg new" to quit with an error message when given
>>  an illegal patch name, not silently mangle it. (Of course, the
>>  commands that generate patch names from commit messages -- such as
>>  "stg new" when not given an explicit patch name -- should mangle the
>>  commit message as much as necessary. But when the user gives us a
>>  patch name, we should either use that as is or fail with an
>>  informative message.)
>>
>>  I think the right thing to do would be to construct a function that
>>  validates patch names (I don't think we have one right now), and then
>>  call it whenever we need to check if a patch name is OK. Such as when
>>  the user gives us the name of a new patch. And when we've
>>  auto-generated a patch name from a commit message, we should probably
>>  assert that that the check function is OK with the name.
>
> What about instead of 'fail with an informative message', just issue a
> warning that
> the name has been mangled. I use pathnames in patch names frequently,
> so this would be very handy.

No, I agree with Karl. For example, a tool (such as the Emacs
frontend) might want to do a "stg new foobar" and then do something
with the patch it now knows is called "foobar". And if it was called
something else the tool will fail.

-- 
David Kågedal