git fails with control characters in trunk directory name

Using the mouse to paste a git url from a website into a terminal session in 
order to clone the repository, I recently managed to include invisible 
control characters into the git trunk directory name. 

Consequently, I faced all sort of strange behaviour like git pull not working 
(error 2), later on a kernel make which supposedly could not finding a rule 
to create the trunk directory and more such inconsistencies. 
I then reinstalled git, rcs and so on and also tried unsuccessfully several  
git versions. The next morning I looked into the .git/config file and 
recognized that the "url" key value within the [remote "origin"] section 
contained some control characters: ^J and \n, as fas as I remember.

While this was almost entirely my fault, git could possibly apply a filter, 
reject such a name or at least issue a warning. 

2009/5/11 Hugo Mildenberger [off-list ref]:

Using the mouse to paste a git url from a website into a terminal session in
order to clone the repository, I recently managed to include invisible
control characters into the git trunk directory name.

Git has no "trunk". Not in CVS/CVN sense, at least

Consequently, I faced all sort of strange behaviour like git pull not working
(error 2), later on a kernel make which supposedly could not finding a rule
to create the trunk directory and more such inconsistencies.
I then reinstalled git, rcs and so on and also tried unsuccessfully several
git versions. The next morning I looked into the .git/config file and
recognized that the "url" key value within the [remote "origin"] section
contained some control characters: ^J and \n, as fas as I remember.

What platform are you on?
Can you show your .git/config?

While this was almost entirely my fault, git could possibly apply a filter,
reject such a name or at least issue a warning.

Maybe. Or maybe it can just work (well, assuming the user meant to
use an url with character you considered "control").

Hello Alex,

2009/5/11 Hugo Mildenberger [off-list ref]:

quoted

Using the mouse to paste a git url from a website into a terminal session
in order to clone the repository, I recently managed to include invisible
control characters into the git trunk directory name.

Git has no "trunk". Not in CVS/CVN sense, at least

quoted

Consequently, I faced all sort of strange behaviour like git pull not
working (error 2), later on a kernel make which supposedly could not
finding a rule to create the trunk directory and more such
inconsistencies.
I then reinstalled git, rcs and so on and also tried unsuccessfully
several git versions. The next morning I looked into the .git/config file
and recognized that the "url" key value within the [remote "origin"]
section contained some control characters: ^J and \n, as fas as I
remember.

What platform are you on?
Can you show your .git/config?

quoted

While this was almost entirely my fault, git could possibly apply a
filter, reject such a name or at least issue a warning.

Maybe. Or maybe it can just work (well, assuming the user meant to
use an url with character you considered "control").

I said it actually did not work well, independent from how anyone prefers to 
classify characters. My platform is Gentoo-hardened with unicode support and 
an ext3 disk format .  With "trunk directory"  I meant the top level 
directory which is created when you run git clone on a remote url -- sorry 
for still not being a native git speaker. I don't have the original setup 
anymore. My _working_ ".git/config" is now:

[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = 
git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master

The remote url did contain something like "^J\n" at the end, as did the top 
level (the much derided "trunk") directory name on disk. I probably got there 
by puting the copied url within quotes on the command line, but today I'm  
unsure exactly how I arrived there. However, old fashioned as I am, I still 
consider e.g. a linefeed to be a "control character", and inspite of your 
flashing git punditry I still consider this to be an issue.

2009/5/12 Hugo Mildenberger [off-list ref]:

quoted

2009/5/11 Hugo Mildenberger [off-list ref]:

quoted

While this was almost entirely my fault, git could possibly apply a
filter, reject such a name or at least issue a warning.

Maybe. Or maybe it can just work (well, assuming the user meant to
use an url with character you considered "control").

I said it actually did not work well, independent from how anyone prefers to
classify characters. My platform is Gentoo-hardened with unicode support and
an ext3 disk format .  With "trunk directory"  I meant the top level
directory which is created when you run git clone on a remote url -- sorry
for still not being a native git speaker. I don't have the original setup
anymore. My _working_ ".git/config" is now:

Ok, let's assume you picked up '\r\n' from that web page

The remote url did contain something like "^J\n" at the end, as did the top
level (the much derided "trunk") directory name on disk. I probably got there
by puting the copied url within quotes on the command line, but today I'm
unsure exactly how I arrived there. However, old fashioned as I am, I still
consider e.g. a linefeed to be a "control character", and inspite of your
flashing git punditry I still consider this to be an issue.

Git tries not to enforce any specific rules where the program itself does
not need them. One can easily imagine an automatic system which
generates names of repositories from something like binary data
(something like BigTable comes to mind) and the less rules the
underlying levels impose on it, the simplier the upper levels will be.

OTOH, a warning about commonly used delimiters not fitting a name
context, maybe a good idea. Like "\r\n\t", backslash (came up recently
on this list). Such a check and associated warning may be useful for
repository names and branches.

Still, it's more of a policy issue and I would make it optional, even
if enabled by default. Maybe even by defining a regexp which the
repo name or branch name must (for hard error) or should (for a warning)
match.

For your specific case, you can take a look at builtin-clone.c,
just after the line containing "guess_dir_name(repo_name"...

OTOH, a warning about commonly used delimiters not fitting a name
context, maybe a good idea. Like "\r\n\t", backslash (came up recently
on this list). Such a check and associated warning may be useful for
repository names and branches.

Still, it's more of a policy issue and I would make it optional, even
if enabled by default. Maybe even by defining a regexp which the
repo name or branch name must (for hard error) or should (for a warning)
match.

For your specific case, you can take a look at builtin-clone.c,
just after the line containing "guess_dir_name(repo_name"...
--

But at least the git versions I tried (up to 1.6.3) really do have a problem 
when facing a trailing newline in repository names; so one should enforce a 
convention. Although the situation here is certainly a far-flung, uncommon 
one, it could also happen when git was called from scripts.

I looked into guess_dir_name(). A regex call would be easy to fit, but 
currently the git binary does not depend on libpcre. Is it generally 
considered to be acceptable to add such a dependency? 

While I like the idea to make use of a configurable regular expression, such 
an expression had to be a command line parameter with a reasonable default 
value, because .git/config still would not exist when the value would be 
needed. 

Last not least, I managed to reproduce the problem almost exactly:

1.) hm@localhost git 
clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"  
	(Note the trailing linefeed)

2.) hm@localhost ~/tmp/bluetooth-testing.git $ make
	Makefile:313: /home/hm/tmp/bluetooth-testing.git
	/scripts/Kbuild.include: No such file or directory
	make[1]: /home/hm/tmp/bluetooth-testing.git: No such file or directory
	make[1]: *** No rule to make target `/home/hm/tmp/bluetooth-testing.git'.
	Stop.
	make: *** No rule to make target `include/config/auto.conf', needed by
	`include/config/kernel.release'.  Stop.

3.) hm@localhost ~/tmp/bluetooth-testing.git $ git pull
	fatal: Error in line 2:

4.) ".git/config" now contains

url = 
git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git\n

I particulary liked the git message "fatal: Error in line 2:" ... 

grep says, it would stem from builtin-fmt-merge-msg.c/fmt_merge_msg():
270         /* get a line */
271         while (pos < in->len) {
272                 int len;
273                 char *newline, *p = in->buf + pos;
274
275                 newline = strchr(p, '\n');
276                 len = newline ? newline - p : strlen(p);
277                 pos += len + !!newline;
278                 i++;
279                 p[len] = 0;
280                 if (handle_line(p))
281                         die ("Error in line %d: %.*s", i, len, p);
282         }

2009/5/12 Hugo Mildenberger [off-list ref]:

quoted

OTOH, a warning about commonly used delimiters not fitting a name
context, maybe a good idea. Like "\r\n\t", backslash (came up recently
on this list). Such a check and associated warning may be useful for
repository names and branches.

Still, it's more of a policy issue and I would make it optional, even
if enabled by default. Maybe even by defining a regexp which the
repo name or branch name must (for hard error) or should (for a warning)
match.

For your specific case, you can take a look at builtin-clone.c,
just after the line containing "guess_dir_name(repo_name"...
--

But at least the git versions I tried (up to 1.6.3) really do have a problem
when facing a trailing newline in repository names; so one should enforce a
convention.

That's what I mean by saying: "... even if enabled by default".
I just want to disable newbie-helping annoyances on my systems.

I looked into guess_dir_name().

That's not the right place. The place I meant is right below the call
to this function (you have to parse the names given in the command-line
too).

The automatically generated (that's the case with guess_dir_name)
directory name certainly shouldn't contain any unexpected characters.

A regex call would be easy to fit, but
currently the git binary does not depend on libpcre. Is it generally
considered to be acceptable to add such a dependency?

No. And pcre is not the only regex lib in the world. And we prefer
shell patterns, if any at all.

While I like the idea to make use of a configurable regular expression, such
an expression had to be a command line parameter with a reasonable default
value, because .git/config still would not exist when the value would be
needed.

That's where _default_ policy plays its role. "Default" like in "it is compiled
into the git executable and needs no configuration present".

Last not least, I managed to reproduce the problem almost exactly:

1.) hm@localhost git
clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"
       (Note the trailing linefeed)

That's all the command printed? No "Initialized empty Git repository" line?

2.) hm@localhost ~/tmp/bluetooth-testing.git $ make

Hmm... At this point the clone may have worked (at least partially).
It named "bluetooth-testing.git", which it shouldn't (but explainable:
the repo url suffix is not .git anymore, but ".git\r\n"). But it looks like
the post-clone checkout failed (silently? which would be bad):

       Makefile:313: /home/hm/tmp/bluetooth-testing.git
       /scripts/Kbuild.include: No such file or directory
       make[1]: /home/hm/tmp/bluetooth-testing.git: No such file or directory
       make[1]: *** No rule to make target `/home/hm/tmp/bluetooth-testing.git'.
       Stop.
       make: *** No rule to make target `include/config/auto.conf', needed by
       `include/config/kernel.release'.  Stop.

Assuming the files must be present, of course.

3.) hm@localhost ~/tmp/bluetooth-testing.git $ git pull
       fatal: Error in line 2:

4.) ".git/config" now contains

url =
git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git\n

I particulary liked the git message "fatal: Error in line 2:" ...

Separate issue. Will look at it later.

quoted

I looked into guess_dir_name().

That's not the right place. The place I meant is right below the call
to this function (you have to parse the names given in the command-line
too).

The automatically generated (that's the case with guess_dir_name)
directory name certainly shouldn't contain any unexpected characters.

quoted

A regex call would be easy to fit, but
currently the git binary does not depend on libpcre. Is it generally
considered to be acceptable to add such a dependency?

No. And pcre is not the only regex lib in the world. And we prefer
shell patterns, if any at all.

quoted

While I like the idea to make use of a configurable regular expression, such
an expression had to be a command line parameter with a reasonable default
value, because .git/config still would not exist when the value would be
needed.

That's where _default_ policy plays its role. "Default" like in "it is compiled
into the git executable and needs no configuration present".

quoted

Last not least, I managed to reproduce the problem almost exactly:

1.) hm@localhost git
clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"
       (Note the trailing linefeed)

That's all the command printed? No "Initialized empty Git repository" line?

quoted

2.) hm@localhost ~/tmp/bluetooth-testing.git $ make

Hmm... At this point the clone may have worked (at least partially).
It named "bluetooth-testing.git", which it shouldn't (but explainable:
the repo url suffix is not .git anymore, but ".git\r\n"). But it looks like
the post-clone checkout failed (silently? which would be bad):

quoted

       Makefile:313: /home/hm/tmp/bluetooth-testing.git
       /scripts/Kbuild.include: No such file or directory
       make[1]: /home/hm/tmp/bluetooth-testing.git: No such file or directory
       make[1]: *** No rule to make target `/home/hm/tmp/bluetooth-testing.git'.
       Stop.
       make: *** No rule to make target `include/config/auto.conf', needed by
       `include/config/kernel.release'.  Stop.

Assuming the files must be present, of course.

quoted

3.) hm@localhost ~/tmp/bluetooth-testing.git $ git pull
       fatal: Error in line 2:

4.) ".git/config" now contains

url =
git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git\n

I particulary liked the git message "fatal: Error in line 2:" ...

Separate issue. Will look at it later.
--

You really want to use shell patterns to match against a string from within a binary? Although git 
already makes use of regexec from glibc or compat/regex directory in numerous places? 

Regarding the procedure above, sorry for keeping it much too short. Here comes a 
complete sequence (I will put some comments within square brackets):

hm@localhost /var/tmp $ git --version
git version 1.6.0.6

hm@localhost /var/tmp $ git clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"
Initialized empty Git repository in /mnt/hda1/tmp/bluetooth-testing.git
/.git/
remote: Counting objects: 1177836, done.
remote: Compressing objects: 100% (189467/189467), done.
remote: Total 1177836 (delta 982785), reused 1176855 (delta 981880)
Receiving objects: 100% (1177836/1177836), 288.16 MiB | 1288 KiB/s, done.
Resolving deltas: 100% (982785/982785), done.
Checking out files: 100% (27842/27842), done.

hm@localhost /var/tmp $ ls
bluetooth-testing.git?   [Note that question mark replacing \n in repository's name]

hm@localhost /var/tmp $ cd blue	
bluetooth-testing.git^J/ bluez-utils-2.25-r1.new/
[I typed cd blue<TAB> here, and there is the "^J" I remembered to have seen elsewhere] 

hm@localhost /var/tmp $ cd 'bluetooth-testing.git
'/ [bash puts the directory name automatically within single quotes in this case]

hm@localhost /var/tmp/bluetooth-testing.git $ make defconfig
*** Default configuration is based on 'i386_defconfig'
#
# configuration written to .config
#

hm@localhost /var/tmp/bluetooth-testing.git $ make
make[1]: /mnt/hda1/tmp/bluetooth-testing.git: No such file or directory
make[1]: *** No rule to make target `/mnt/hda1/tmp/bluetooth-testing.git'.  Stop.
make: *** No rule to make target `include/config/auto.conf', needed by `include/config/kernel.release'.  Stop.
[The problem is simply the odd directory name, while make tries to use the real one]

hm@localhost /var/tmp/bluetooth-testing.git $ git pull
fatal: Error in line 2:

2009/5/12 Hugo Mildenberger [off-list ref]:

quoted

quoted

Last not least, I managed to reproduce the problem almost exactly:

1.) hm@localhost git
clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"
       (Note the trailing linefeed)

That's all the command printed? No "Initialized empty Git repository" line?

hm@localhost /var/tmp $ git clone "git://git.kernel.org/pub/scm/linux/kernel/git/holtmann/bluetooth-testing.git
"
Initialized empty Git repository in /mnt/hda1/tmp/bluetooth-testing.git
/.git/
remote: Counting objects: 1177836, done.
remote: Compressing objects: 100% (189467/189467), done.
remote: Total 1177836 (delta 982785), reused 1176855 (delta 981880)
Receiving objects: 100% (1177836/1177836), 288.16 MiB | 1288 KiB/s, done.
Resolving deltas: 100% (982785/982785), done.
Checking out files: 100% (27842/27842), done.

hm@localhost /var/tmp $ ls
bluetooth-testing.git?   [Note that question mark replacing \n in repository's name]

Ok, clone works. Fully and correctly (even if a bit unexpected).

hm@localhost /var/tmp/bluetooth-testing.git $ make
make[1]: /mnt/hda1/tmp/bluetooth-testing.git: No such file or directory
make[1]: *** No rule to make target `/mnt/hda1/tmp/bluetooth-testing.git'.  Stop.

It is just linux build system which does not support compilation in oddly
named directories (which is reasonable, if you ask me).

hm@localhost /var/tmp/bluetooth-testing.git $ git pull
fatal: Error in line 2:

That's already fixed. Will send the fix in a minute.

The fmt-merge-msg does a strong syntax checking of its input and fails
with if it is incorrect. The LF character is the only character
important for fmt-merge-msg. As the url in FETCH_HEAD plays only
informational role, a quoted representation of the url should be good
and true enough.
The url often comes from either user-editable config or command line,
so it is reasonable to expect all kinds of characters in it, including
the characters which the format of FETCH_HEAD considers special (line
separator in this case).

Noticed and reported by Hugo Mildenberger.

Signed-off-by: Alex Riesen <redacted>
---

Alex Riesen, Tue, May 12, 2009 19:18:33 +0200:

2009/5/12 Hugo Mildenberger [off-list ref]:

quoted

hm@localhost /var/tmp/bluetooth-testing.git $ git pull
fatal: Error in line 2:

That's already fixed. Will send the fix in a minute.

Here

 builtin-fetch.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 3c998ea..eef7091 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -353,12 +353,17 @@ static int store_updated_refs(const char *url, const char *remote_name,
 						    kind);
 			note_len += sprintf(note + note_len, "'%s' of ", what);
 		}
-		note_len += sprintf(note + note_len, "%.*s", url_len, url);
-		fprintf(fp, "%s\t%s\t%s\n",
+		fprintf(fp, "%s\t%s\t%s",
 			sha1_to_hex(commit ? commit->object.sha1 :
 				    rm->old_sha1),
 			rm->merge ? "" : "not-for-merge",
 			note);
+		for (i = 0; i < url_len; ++i)
+			if ('\n' == url[i])
+				fputs("\\n", fp);
+			else
+				fputc(url[i], fp);
+		fputc('\n', fp);
 
 		if (ref)
 			rc |= update_local_ref(ref, what, note);

-- 
1.6.3.28.ga852b

Hugo Mildenberger, Tue, May 12, 2009 18:59:58 +0200:

quoted

quoted

I looked into guess_dir_name().

That's not the right place. The place I meant is right below the call
to this function (you have to parse the names given in the command-line
too).

The automatically generated (that's the case with guess_dir_name)
directory name certainly shouldn't contain any unexpected characters.

quoted

A regex call would be easy to fit, but
currently the git binary does not depend on libpcre. Is it generally
considered to be acceptable to add such a dependency?

No. And pcre is not the only regex lib in the world. And we prefer
shell patterns, if any at all.

You really want to use shell patterns to match against a string from within a binary?

Is that a problem? Especially if the matching expression is just
something like a character class?

Although git already makes use of regexec from glibc or compat/regex
directory in numerous places? 

You said pcre, and I wasn't able to look at the source at the time I
answered your mail to check if git relies on POSIX regexp. I did
(and remembered git grep), so no need for pcre and regular exceptions
are just a line away.


P.S. Could you please quote more appropriately? Your discussion is a
little hard to follow.

The fmt-merge-msg does a strong syntax checking of its input and fails
with if it is incorrect. The LF character is the only character
important for fmt-merge-msg. As the url in FETCH_HEAD plays only
informational role, a quoted representation of the url should be good
and true enough.
The url often comes from either user-editable config or command line,
so it is reasonable to expect all kinds of characters in it, including
the characters which the format of FETCH_HEAD considers special (line
separator in this case).

Noticed and reported by Hugo Mildenberger.

Signed-off-by: Alex Riesen <redacted>
---

Alex Riesen, Tue, May 12, 2009 19:24:52 +0200:

Alex Riesen, Tue, May 12, 2009 19:18:33 +0200:

quoted

2009/5/12 Hugo Mildenberger [off-list ref]:

quoted

hm@localhost /var/tmp/bluetooth-testing.git $ git pull
fatal: Error in line 2:

That's already fixed. Will send the fix in a minute.

Here

That one has a bug which breaks t5515: printing an unterminated
string ('note').

 builtin-fetch.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 3c998ea..ec75df0 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c

@@ -353,12 +353,18 @@ static int store_updated_refs(const char *url, const char *remote_name,
 						    kind);
 			note_len += sprintf(note + note_len, "'%s' of ", what);
 		}
-		note_len += sprintf(note + note_len, "%.*s", url_len, url);
-		fprintf(fp, "%s\t%s\t%s\n",
+		note[note_len] = '\0';
+		fprintf(fp, "%s\t%s\t%s",
 			sha1_to_hex(commit ? commit->object.sha1 :
 				    rm->old_sha1),
 			rm->merge ? "" : "not-for-merge",
 			note);
+		for (i = 0; i < url_len; ++i)
+			if ('\n' == url[i])
+				fputs("\\n", fp);
+			else
+				fputc(url[i], fp);
+		fputc('\n', fp);
 
 		if (ref)
 			rc |= update_local_ref(ref, what, note);

-- 
1.6.3.28.ga852b

Strip trailing spaces off guessed target directory in builtin clone,
and replace 'control' characters with an ASCII space.

User still can have any name by specifying it explicitely after url.

Signed-off-by: Alex Riesen <redacted>
---

This should take care of accidental pastings inside shell quotes.
At least for the local part of the operation.
Now I'm looking at the code and think I should have stripped the
heading whitespace as well. It is much less likely to happen, though.

 builtin-clone.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 880373f..bdbe931 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c

@@ -104,11 +104,12 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;
+	char *dir;
 
 	/*
-	 * Strip trailing slashes and /.git
+	 * Strip trailing spaces, slashes and /.git
 	 */
-	while (repo < end && is_dir_sep(end[-1]))
+	while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1])))
 		end--;
 	if (end - repo > 5 && is_dir_sep(end[-5]) &&
 	    !strncmp(end - 4, ".git", 4)) {

@@ -140,10 +141,21 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	if (is_bare) {
 		struct strbuf result = STRBUF_INIT;
 		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
-		return strbuf_detach(&result, 0);
-	}
-
-	return xstrndup(start, end - start);
+		dir = strbuf_detach(&result, 0);
+	} else
+		dir = xstrndup(start, end - start);
+	/* replace all 'control' characters with ascii space */
+	for (start = dir; *start; ++start)
+		if (*(const unsigned char *)start < 32u)
+			dir[start - dir] = '\x20';
+	/* remove trailing spaces */
+	if (dir < start)
+		for (end = start; dir < --end; )
+			if (!isspace(*end))
+				break;
+			else
+				dir[end - dir] = '\0';
+	return dir;
 }
 
 static void strip_trailing_slashes(char *dir)

-- 
1.6.3.28.ga852b