Cogito limitation in tag names?

From cg-tag in 0.16:

(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
die "name contains invalid characters"

WHY? I can see rejecting control characters, but the above will reject pretty much anything written in a non-English language, which is rude to say the least...

-hpa

The egrep pattern used by cg-tag is too restrictive. While it will prevent control characters from being specified as a tag name, it will also reject nearly anything written in a non-English language, as noted by -hpa

---

 cg-tag |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

187670279068e7177149765a7f736cc565a1fe19

index da4f2d5..73616b8 100755
@@ -54,7 +54,7 @@ id=$(cg-object-id -n "$id") || exit 1
 type=$(git-cat-file -t "$id")
 id=${id% *}
 
-(echo $name | egrep -qv '[^a-zA-Z0-9_.@!:-]') || \
+git-check-ref-format $name || \
 	die "name contains invalid characters"
 
 mkdir -p $_git/refs/tags

0.99.9.GIT

Yes. i've just finished preparing a new patch with this exact change. But your patch is much better.

- Martin -

Due to a shortage of devoted followers, the production of great leaders has 
been discontinued.

Dear diary, on Tue, Dec 13, 2005 at 12:13:12PM CET, I got a letter where Junio C Hamano <[email protected]> said that...

Thank you both for the patch, but I'd be much more comfortable if at least quotes (both ' and "), backslashes, ? and * would be prohibited in the names as well. Any chance of also implementing this policy upstream? Taken to the extreme, using such a names for tags might be perceived as a possible security vulnerability wrt. the less shell-savy users. ;-)

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

I second that, and thanks for pointing it out. Any objections?

Probably it can safely be updated to use check-ref-format, and I suspect that the only reason why it does not do so is because check-ref-format came later.

Junio C Hamano, Tue, Dec 13, 2005 19:41:27 +0100:

Just as a warning, perhaps? It's not like git is anywhere limited in this respect...

Yeah, after thinking about it a bit more, I changed my mind.

The wildcard letters like ? and * I understand and sympathetic about somewhat. Something like this:

name="*.sh" ;# this also comes from the end user echo $name

ends up showing every shell script in the current directory, and not literal '*.sh'.

However, I do not think covering five characters '"\?* gives us anything, and sends a strong message that we do not know our shell programming to whoever is reading our code. For one thing, the user can still say "foo[a-z]bar" to confuse you, so you also need to forbid [].

The thing is, if you start to care about single and double quotes, then what you are doing carelessly is not something simple like this:

name='frotz'\''nitfol"filfre\xyzzy' ;# this comes from the end user. echo $name ;# and this prints just fine.

For quotes to matter, you must be doing an "eval" carelessly, and "eval" and careless should never go together.

        # do not try this in your repository without echo
	name="foo; echo rm -fr ."
        eval "git-rev-parse $name" 

You end up needing to forbid a lot more than the quoting and wildcard, if you want to keep your shell scripts loose and lazy; which may be a worthy goal in itself but pretty much defeats the initial discussion of "why do we allow only these characters in tags".

So in short, I am somewhat negative about the idea of adding more "forbidden letters". Let's make sure our scripts are careful where safety matters.

Note that this does not forbid Porcelains to enforce additional restrictions on their own.

So why don't we do this?

-- >8 --
Subject: [PATCH] Forbid pattern maching characters in refnames.

by marking '?', '*', and '[' as bad_ref_char().

---

 Documentation/git-check-ref-format.txt |    8 +++++---
 refs.c                                 |    4 +++-
 2 files changed, 8 insertions(+), 4 deletions(-)

d04ec25a77249095b6d2af5a08fe131351f2d86d

index 636e951..f7f84c6 100644
@@ -26,13 +26,15 @@ imposes the following rules on how refs 
 
 . It cannot have ASCII control character (i.e. bytes whose
   values are lower than \040, or \177 `DEL`), space, tilde `~`,
-  caret `{caret}`, or colon `:` anywhere;
+  caret `{caret}`, colon `:`, question-mark `?`, asterisk `*`,
+  or open bracket `[` anywhere;
 
 . It cannot end with a slash `/`.
 
 These rules makes it easy for shell script based tools to parse
-refnames, and also avoids ambiguities in certain refname
-expressions (see gitlink:git-rev-parse[1]).  Namely:
+refnames, pathname expansion by the shell when a refname is used
+unquoted (by mistake), and also avoids ambiguities in certain
+refname expressions (see gitlink:git-rev-parse[1]).  Namely:
 
 . double-dot `..` are often used as in `ref1..ref2`, and in some
   context this notation means `{caret}ref1 ref2` (i.e. not in

index b8fcb98..0d63c1f 100644
@@ -313,7 +313,9 @@ int write_ref_sha1(const char *ref, int 
 static inline int bad_ref_char(int ch)
 {
 	return (((unsigned) ch) <= ' ' ||
-		ch == '~' || ch == '^' || ch == ':');
+		ch == '~' || ch == '^' || ch == ':' ||
+		/* 2.13 Pattern Matching Notation */
+		ch == '?' || ch == '*' || ch == '[');
 }
 
 int check_ref_format(const char *ref)

0.99.9.GIT

Dear diary, on Fri, Dec 16, 2005 at 03:17:19AM CET, I got a letter where Junio C Hamano <[email protected]> said that...

I'm all for it, and now I also agree that forbidding \'" is pointless.

Thanks,

				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.