threads / patch / 16884

patchnew: translate non word characters in patch name to '-'

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

## tl;dr

7 messages between Dec 27, 2008 and Jan 15, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Hannes Eder· Dec 27, 2008, 12:37 UTC · lore
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(-)
Show changes to stgit/commands/new.py +1 −1
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:
Karl Hasselström· Dec 28, 2008, 20:49 UTC · re: Hannes Eder · lore

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

On 2008-12-27 13:37:20 +0100, Hannes Eder wrote:
Show 11 quoted lines
> 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
Hannes Eder· Dec 29, 2008, 20:15 UTC · re: Karl Hasselström · lore

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

On 12/28/08, Karl Hasselström <kha@treskal.com> wrote:
Show 33 quoted lines
> 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 ;).

---
Show changes to 3 files +28 −1

stgit/commands/new.py, stgit/commands/rename.py, stgit/utils.py

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__):
Karl Hasselström· Dec 29, 2008, 21:21 UTC · re: Hannes Eder · lore

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

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
Hannes Eder· Dec 31, 2008, 08:07 UTC · re: Karl Hasselström · lore

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

On Mon, Dec 29, 2008 at 10:21 PM, Karl Hasselström <kha@treskal.com> wrote:
Show 17 quoted lines
> 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

Karl Hasselström· Dec 31, 2008, 10:38 UTC · re: Hannes Eder · lore

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

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
David Kågedal· Jan 15, 2009, 14:53 UTC · re: Hannes Eder · lore

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

"Hannes Eder" <hannes@hanneseder.net> writes:
Show 39 quoted lines
> 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

← back to recent threads