git fails with control characters in trunk directory name

git fails with control characters in trunk directory name

[Hugo Mildenberger]

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. 

Re: git fails with control characters in trunk directory name

[Alex Riesen]

2009/5/11 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> 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").

Re: git fails with control characters in trunk directory name

[Hugo Mildenberger]

Hello Alex,

> 2009/5/11 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> > 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").

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.

Re: git fails with control characters in trunk directory name

[Alex Riesen]

2009/5/12 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
>> 2009/5/11 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
>> > 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"...

Re: git fails with control characters in trunk directory name

[Hugo Mildenberger]

> 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         }

Re: git fails with control characters in trunk directory name

[Alex Riesen]

2009/5/12 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
>> 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.

Re: git fails with control characters in trunk directory name

[Hugo Mildenberger]

> > 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.
> --

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:

Re: git fails with control characters in trunk directory name

[Alex Riesen]

2009/5/12 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
>> > 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.

[PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

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 <raa.lkml@gmail.com>
---

Alex Riesen, Tue, May 12, 2009 19:18:33 +0200:
> 2009/5/12 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> > 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

Re: git fails with control characters in trunk directory name

[Alex Riesen]

Hugo Mildenberger, Tue, May 12, 2009 18:59:58 +0200:
> > > 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.
> 
> 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.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> +		for (i = 0; i < url_len; ++i)
> +			if ('\n' == url[i])
> +				fputs("\\n", fp);
> +			else
> +				fputc(url[i], fp);
> +		fputc('\n', fp);

This ad-hoc quoting feels _very_ wrong.  Who is on the reading side and
how does it unquote?  If it uses quote.c infrastructure, we should be
quoting using the function from the same library shouldn't we?

If it is just informational use only, then it might make more sense to
drop this ugly "quoted \n" silently.  I dunno.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

2009/5/13 Junio C Hamano <gitster@pobox.com>:
> Alex Riesen <raa.lkml@gmail.com> writes:
>
>> +             for (i = 0; i < url_len; ++i)
>> +                     if ('\n' == url[i])
>> +                             fputs("\\n", fp);
>> +                     else
>> +                             fputc(url[i], fp);
>> +             fputc('\n', fp);
>
> This ad-hoc quoting feels _very_ wrong.  Who is on the reading side and
> how does it unquote?

git fmt-merge-msg. It does not unquote. The url is purely informational here.
OTOH, the \n shouldn't be in url text at all, so treat it as slightly
less annoying
warning.

> If it is just informational use only, then it might make more sense to
> drop this ugly "quoted \n" silently.  I dunno.

That'd mean to loose the information completely. Which is just as bad
as putting the LF in the url in the first place.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

2009/5/13 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> Am Mittwoch, 13. Mai 2009 schrieb Alex Riesen <raa.lkml@gmail.com>
>>
>> That'd mean to loose the information completely. Which is just as bad
>> as putting the LF in the url in the first place.
>
> This stray linefeed is not information, but pure contamination. Thus it
> would be much better to simply strip it off.

Not at the place where the patch changes it. In git clone, maybe (the many
times mentioned guessing function). But then, we have to provide an option
to leave the names alone, verbatim (which, I think, the non-quessing form
already provides. No additional coding necessary).

> And besides from the fact that git apply rejects this patch (fatal: corrupt
> patch at line 6),

This is your local problem. Maybe you copied the patch through clipboard
and it kept the presentation in KMail, or KMail generally corrupts
mails, I don't
know. Just save the mail as is and do "git am -3" on it.

> I think it would also not handle the equally wrong repository directory name on disk,

You think wrong. It does handle it perfectly.

And it is not wrong. It is just how it is. Don't spend too much time in MacOS
and that overgrown DOS. They'll wreck your brain.

> which then possibly leads to subsequent make failures (as it actually happend in
> the case I described earlier here.)

The make(1) works in such directories just fine. Fix yours (thinking about
it now - you wont find a make which does not work in such directories).

> Why not just return to your original idea, which proposed testing the
> repository name against a regular expression describing a forbidden
> set of characters (which is "\n", currently) and then terminate with a
> clear message?

The idea was not abandoned nor followed upon. It's just I don't need that
warning and would switch it off immediately anyway (if it was implemented).
I just spent a minute thinking about it (that's where regexp idea came from).
I'm not going to work on it (at least, not right now). I'm even sure you wont
be working on it too, now when you have learned what the problem is and
how to work around it. And that's while all you needed is to put a two-three
lines in that guessing function...

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Hugo Mildenberger]

Am Mittwoch, 13. Mai 2009 schrieb Alex Riesen <raa.lkml@gmail.com>
> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> >
> >> +             for (i = 0; i < url_len; ++i)
> >> +                     if ('\n' == url[i])
> >> +                             fputs("\\n", fp);
> >> +                     else
> >> +                             fputc(url[i], fp);
> >> +             fputc('\n', fp);
> >
> > This ad-hoc quoting feels _very_ wrong.  Who is on the reading side and
> > how does it unquote?
> 
> git fmt-merge-msg. It does not unquote. The url is purely informational here.
> OTOH, the \n shouldn't be in url text at all, so treat it as slightly
> less annoying
> warning.
> 
> > If it is just informational use only, then it might make more sense to
> > drop this ugly "quoted \n" silently.  I dunno.
> 
> That'd mean to loose the information completely. Which is just as bad
> as putting the LF in the url in the first place.
> --

This stray linefeed is not information, but pure contamination. Thus it 
would be much better to simply strip it off. And besides from the fact that 
git apply rejects this patch (fatal: corrupt patch at line 6), I think it would 
also not handle the equally wrong repository directory name on disk, which 
then possibly leads to subsequent make failures (as it actually happend in 
the case I described earlier here.) Why not just return to your original idea, 
which proposed testing the repository name against a regular expression 
describing a forbidden set of characters (which is "\n", currently) and then 
terminate with a clear message?

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Hugo Mildenberger]

Am Mittwoch, 13. Mai 2009 schrieb Alex Riesen:
> 2009/5/13 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> > Am Mittwoch, 13. Mai 2009 schrieb Alex Riesen <raa.lkml@gmail.com>
> >>
> >> That'd mean to loose the information completely. Which is just as bad
> >> as putting the LF in the url in the first place.
> >
> > This stray linefeed is not information, but pure contamination. Thus it
> > would be much better to simply strip it off.
> 
> Not at the place where the patch changes it. In git clone, maybe (the many
> times mentioned guessing function). But then, we have to provide an option
> to leave the names alone, verbatim (which, I think, the non-quessing form
> already provides. No additional coding necessary).
> 
> > And besides from the fact that git apply rejects this patch (fatal: corrupt
> > patch at line 6),
> 
> This is your local problem. Maybe you copied the patch through clipboard
> and it kept the presentation in KMail, or KMail generally corrupts
> mails, I don't
> know. Just save the mail as is and do "git am -3" on it.

My apologies, the problem actually was copy & paste from kmail.

> > I think it would also not handle the equally wrong repository directory name on disk,
> 
> You think wrong. It does handle it perfectly.

I'm thinking right. The stray linefeed is still there:

ls -l 'bluetooth-testing.git
'/ -d
drwxr-xr-x 23 hm hm 4096 13. Mai 15:06 bluetooth-testing.git?/


> > which then possibly leads to subsequent make failures (as it actually happend in
> > the case I described earlier here.)
> 
> The make(1) works in such directories just fine. Fix yours (thinking about
> it now - you wont find a make which does not work in such directories).

You aren't saying which make you are using, but GNU make version 3.81 or 
the linux kernel make system still can't cope with a linefeed contaminated directory 
name:

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.

> > Why not just return to your original idea, which proposed testing the
> > repository name against a regular expression describing a forbidden
> > set of characters (which is "\n", currently) and then terminate with a
> > clear message?
> 
> The idea was not abandoned nor followed upon. It's just I don't need that
> warning and would switch it off immediately anyway (if it was implemented).
> I just spent a minute thinking about it (that's where regexp idea came from).
> I'm not going to work on it (at least, not right now). I'm even sure you wont
> be working on it too, now when you have learned what the problem is and
> how to work around it. And that's while all you needed is to put a two-three
> lines in that guessing function...

I reported this problem here after I already knew the cause and how to avoid
the problem, because I thought it might be useful fix it generally, especially 
useful for those who run git within a brushwood of scripts, which I do not. 
If you personally would need this or not is not of so much interest here. But 
I was interested in discussing a clean way to implement it.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1523 bytes --]

On Wed, 13 May 2009, Alex Riesen wrote:

> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> >
> >> +             for (i = 0; i < url_len; ++i)
> >> +                     if ('\n' == url[i])
> >> +                             fputs("\\n", fp);
> >> +                     else
> >> +                             fputc(url[i], fp);
> >> +             fputc('\n', fp);
> >
> > This ad-hoc quoting feels _very_ wrong.  Who is on the reading side and
> > how does it unquote?
> 
> git fmt-merge-msg. It does not unquote. The url is purely informational here.
> OTOH, the \n shouldn't be in url text at all, so treat it as slightly
> less annoying
> warning.
> 
> > If it is just informational use only, then it might make more sense to
> > drop this ugly "quoted \n" silently.  I dunno.
> 
> That'd mean to loose the information completely. Which is just as bad
> as putting the LF in the url in the first place.

Looking back at the original message, it looks like the user included a 
newline in an argument to clone, and the fetch must have stripped it out 
(or ignored it in some other fashion), because data was retrieved from a 
repository that doesn't have a newline in its name. Most likely, the 
newline should just be prohibited in the URL in the config file in the 
first place, and we shouldn't be able to get to the point of writing a 
FETCH_HEAD with that value.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

2009/5/13 Daniel Barkalow <barkalow@iabervon.org>:
> On Wed, 13 May 2009, Alex Riesen wrote:
>> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
>>
>> > If it is just informational use only, then it might make more sense to
>> > drop this ugly "quoted \n" silently.  I dunno.
>>
>> That'd mean to loose the information completely. Which is just as bad
>> as putting the LF in the url in the first place.
>
> Looking back at the original message, it looks like the user included a
> newline in an argument to clone, and the fetch must have stripped it out
> (or ignored it in some other fashion), because data was retrieved from a
> repository that doesn't have a newline in its name.

_That_ looks like a bug to me. We should have asked exactly the name
we were given, or notify the user about what we have used for the url
if we have ignored user's input and decided to use something else.
Patches follow.

> ... Most likely, the
> newline should just be prohibited in the URL in the config file in the
> first place, and we shouldn't be able to get to the point of writing a
> FETCH_HEAD with that value.

What I cannot understand is what's wrong with url containing an LF?
Especially if we can handle it, all the tools can handle it, and putting
LF in a filename is a long-standing UNIX tradition.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

Strip trailing spaces off guessed target directory, and replace every
'control' character in the new directory name with ASCII space (\x20).

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

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Alex Riesen, Wed, May 13, 2009 18:09:42 +0200:
> 2009/5/13 Daniel Barkalow <barkalow@iabervon.org>:
> > On Wed, 13 May 2009, Alex Riesen wrote:
> >> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
> >>
> >> > If it is just informational use only, then it might make more sense to
> >> > drop this ugly "quoted \n" silently.  I dunno.
> >>
> >> That'd mean to loose the information completely. Which is just as bad
> >> as putting the LF in the url in the first place.
> >
> > Looking back at the original message, it looks like the user included a
> > newline in an argument to clone, and the fetch must have stripped it out
> > (or ignored it in some other fashion), because data was retrieved from a
> > repository that doesn't have a newline in its name.
> 
> _That_ looks like a bug to me. We should have asked exactly the name
> we were given, or notify the user about what we have used for the url
> if we have ignored user's input and decided to use something else.
> Patches follow.
> 

Now I just have to figure out why the fetch works. Something seem to
strip spaces off urls...

 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

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2022 bytes --]

On Wed, 13 May 2009, Alex Riesen wrote:

> 2009/5/13 Daniel Barkalow <barkalow@iabervon.org>:
> > On Wed, 13 May 2009, Alex Riesen wrote:
> >> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
> >>
> >> > If it is just informational use only, then it might make more sense to
> >> > drop this ugly "quoted \n" silently.  I dunno.
> >>
> >> That'd mean to loose the information completely. Which is just as bad
> >> as putting the LF in the url in the first place.
> >
> > Looking back at the original message, it looks like the user included a
> > newline in an argument to clone, and the fetch must have stripped it out
> > (or ignored it in some other fashion), because data was retrieved from a
> > repository that doesn't have a newline in its name.
> 
> _That_ looks like a bug to me. We should have asked exactly the name
> we were given, or notify the user about what we have used for the url
> if we have ignored user's input and decided to use something else.
> Patches follow.
> 
> > ... Most likely, the
> > newline should just be prohibited in the URL in the config file in the
> > first place, and we shouldn't be able to get to the point of writing a
> > FETCH_HEAD with that value.
> 
> What I cannot understand is what's wrong with url containing an LF?
> Especially if we can handle it, all the tools can handle it, and putting
> LF in a filename is a long-standing UNIX tradition.

The RFC for URLs (which we don't actually follow particularly much) 
doesn't permit whitespace. Protocols that use URLs directly generally use 
newlines as control characters. For example, an HTTP request for:

http://host/path/with
newline/in/it

would parse as a request for "http://host/path/with" with an invalid 
header line. Normally, to refer to a resource whose name contains a 
newline, the URL contains "%0A" instead.

Of course, our "url" config can contain some things that aren't URLs, but 
this particular case was of the form that's supposed to be a standard URL.

	-Daniel
*This .sig left intentionally blank*

[PATCH 1/2] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

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 <raa.lkml@gmail.com>
---

Alex Riesen, Tue, May 12, 2009 19:24:52 +0200:
> Alex Riesen, Tue, May 12, 2009 19:18:33 +0200:
> > 2009/5/12 Hugo Mildenberger <Hugo.Mildenberger@namir.de>:
> > > 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

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Alex Riesen]

Daniel Barkalow, Wed, May 13, 2009 19:12:26 +0200:
> Of course, our "url" config can contain some things that aren't URLs, but 
> this particular case was of the form that's supposed to be a standard URL.

This particular case handled the _url_ just fine. The problem was in the
automatically generated name for local directory.

Re: [PATCH] Quote LF in urls git fetch saves in FETCH_HEAD

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Wed, 13 May 2009, Alex Riesen wrote:
>
>> 2009/5/13 Junio C Hamano <gitster@pobox.com>:
>> > Alex Riesen <raa.lkml@gmail.com> writes:
>> >
>> >> +             for (i = 0; i < url_len; ++i)
>> >> +                     if ('\n' == url[i])
>> >> +                             fputs("\\n", fp);
>> >> +                     else
>> >> +                             fputc(url[i], fp);
>> >> +             fputc('\n', fp);
>> >
>> > This ad-hoc quoting feels _very_ wrong.  Who is on the reading side and
>> > how does it unquote?
>> 
>> git fmt-merge-msg. It does not unquote. The url is purely informational here.
>> OTOH, the \n shouldn't be in url text at all, so treat it as slightly
>> less annoying
>> warning.
>> 
>> > If it is just informational use only, then it might make more sense to
>> > drop this ugly "quoted \n" silently.  I dunno.
>> 
>> That'd mean to loose the information completely. Which is just as bad
>> as putting the LF in the url in the first place.
>
> Looking back at the original message, it looks like the user included a 
> newline in an argument to clone, and the fetch must have stripped it out 
> (or ignored it in some other fashion), because data was retrieved from a 
> repository that doesn't have a newline in its name. Most likely, the 
> newline should just be prohibited in the URL in the config file in the 
> first place, and we shouldn't be able to get to the point of writing a 
> FETCH_HEAD with that value.

Let me attempt to summarize the situation.

 . FETCH_HEAD is a LF terminated sequence of records; each record
   describes the commit object name, if it is meant to be merged, what URL
   and what ref it came from;

 . "git pull" reads from FETCH_HEAD to decide which commit objects to pass
   to underlying "git merge", and "git fmt-merge-msg" reads from
   FETCH_HEAD to decide what message to produce; and

 . "git fetch" allows a URL with an LF in it and fetches from such a URL
   just fine, but "git pull" barfed because "git fmt-patch" noticed.

Because we copy the LF in the URL straight to FETCH_HEAD, it breaks the
file format.  Alex's proposal is to quote it as "\\n" to avoid it (I
suggested stripping it).  Either change will _fix_ the situation in the
sense that the file format will now be correct, "git pull" will extract
the right set of commits (as it does not look at the URL at all when
deciding which records are used for merge), and "git fmt-merge-msg" will
generate correct set of branches and list of incoming commits without
parse errors, even though the user won't be able to cut and paste the URL
it records in its output either way (if stripped, it will be "information
loss", but even if it were quoted, it won't be usable because nobody
unquotes it).

Given all of the above, I think:

 (1) Alex's patch is a good minimum fix to the issue [*1*].  If unfixed,
     people will be able to fetch from but won't be able to pull from a
     URL with a LF in it.

 (2) Even though it seems that we do _support_ fetching from such a URL,
     it is not a good thing.  People may do all sorts of crazy things, and
     this may be one of these crazy things that we _could_ break the
     backward compatibility to avoid innocent mistakes, by forbidding LF
     in URLs.  Nobody could have been using such a URL in real settings
     because of the issue Alex's patch addresses anyway.

 (3) But forbidding or warning will be done by new code.  Is the cost to
     do such a check (implementation and maintenance of the new code)
     justifiable?  Where do we check and when?

So for now, I'd say I take Alex's patch as-is, and do nothing else.

If somebody has a compelling example that this kind of mistake is common
and is hard to figure out why, we can explicitly forbid certain byte
values in the repository URL as a separate step.


[Footnote]

*1* The output is purely informational, and especially its URL field is
not meant to be used as cut-and-paste source by general public anyway
(e.g. you may pull using git over SSH but general public may not have an
SSH access to the repository).  I actually was tempted, when I did the
initial version of fmt-merge-msg, to shorten an overlong URL with
ellipses, even though I didn't.

[PATCH 2/2] Improve the naming of guessed target repository for git clone

[Alex Riesen]

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 <raa.lkml@gmail.com>
---

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

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> 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 <raa.lkml@gmail.com>
> ---
>
> 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.

> @@ -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);
> +		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';

What's this strange mixture of 32u and '\x20'?

> +	/* remove trailing spaces */
> +	if (dir < start)
> +		for (end = start; dir < --end; )
> +			if (!isspace(*end))
> +				break;
> +			else
> +				dir[end - dir] = '\0';
> +	return dir;
>  }

Honestly, I regret having asked if there was a 2/2 ;-)

What's the point of this change, now that you have a fix in 1/2?  Who are
you helping with this patch?

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Alex Riesen]

2009/5/14 Junio C Hamano <gitster@pobox.com>:
> Alex Riesen <raa.lkml@gmail.com> writes:
>> +     /* replace all 'control' characters with ascii space */
>> +     for (start = dir; *start; ++start)
>> +             if (*(const unsigned char *)start < 32u)
>> +                     dir[start - dir] = '\x20';
>
> What's this strange mixture of 32u and '\x20'?
>

Not sure myself. I probably wanted visibility, and somehow ended
up using different presentations.

>> +     /* remove trailing spaces */
>> +     if (dir < start)
>> +             for (end = start; dir < --end; )
>> +                     if (!isspace(*end))
>> +                             break;
>> +                     else
>> +                             dir[end - dir] = '\0';
>> +     return dir;
>>  }
>
> Honestly, I regret having asked if there was a 2/2 ;-)
>
> What's the point of this change, now that you have a fix in 1/2?  Who are
> you helping with this patch?
>

Without this the _automatically_ generated names for cloned repositories
have all the whitespace around them. As you cannot sanely depend on
automatically
generated names, I thought that making them simpler will make sense.

But I should complete the patch: remove heading whitespace, and replace
multiple spaces and control characters with one space.

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

>> What's the point of this change, now that you have a fix in 1/2?  Who are
>> you helping with this patch?
>
> Without this the _automatically_ generated names for cloned repositories
> have all the whitespace around them.

Even if it has whitespace around its name, that's what you got from the
upstream (a valid source of clone), and wasn't it you who said something
about UNIX tradition of allowing LF and others in the filename?

If clone reports "ok we created this new repository" so that the caller
can capture it, then the whole process should be able to cope with
automatically generated names with or without the patch, shouldn't it?

Or are you trying to help a human user who gives a pathname ridden with
excess whitespaces to "git clone", and that pathname _happens_ to work as
a valid clone source, creating a new repository whose name is ridden with
excess whitespaces the same way as the input pathname?  After all, the
user deliberately gave them to us, and the repository we cloned from had
these excesses in its name (iow, without the excess whitespaces the clone
itself wouldn't have worked).  In such a case, is it really helping him to
remove these whitespaces as excesses?

I do not understand the point of this patch and that is why I asked who
you are trying to help.

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

Strip leading and trailing spaces off guessed target directory, and
replace sequences of whitespace and 'control' characters with one
space character.

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

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

2009/5/14 Alex Riesen <raa.lkml@gmail.com>:
> But I should complete the patch: remove heading whitespace, and replace
> multiple spaces and control characters with one space.

Done.

 builtin-clone.c |   34 +++++++++++++++++++++++++++++-----
 1 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 880373f..d068b7e 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,33 @@ 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);
+		dir = strbuf_detach(&result, 0);
+	} else
+		dir = xstrndup(start, end - start);
+	/*
+	 * Replace sequences of 'control' characters and whitespace
+	 * with one ascii space, remove leading and trailing spaces.
+	 */
+	if (*dir) {
+		char *out = dir;
+		int prev_space = 1 /* strip leading whitespace */;
+		for (end = dir; *end; ++end) {
+			char ch = *end;
+			if ((unsigned char)ch < '\x20')
+				ch = '\x20';
+			if (isspace(ch)) {
+				if (prev_space)
+					continue;
+				prev_space = 1;
+			} else
+				prev_space = 0;
+			*out++ = ch;
+		}
+		*out = '\0';
+		if (out > dir && prev_space)
+			out[-1] = '\0';
 	}
-
-	return xstrndup(start, end - start);
+	return dir;
 }

 static void strip_trailing_slashes(char *dir)
-- 
1.6.3.1.37.g5d96e

[-- Attachment #2: 0002-Improve-the-naming-of-guessed-target-repository-for-.patch --]
[-- Type: application/octet-stream, Size: 2269 bytes --]

From 6d6075130afbdd35260e3e33c099f57d9faebe98 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Wed, 13 May 2009 18:32:06 +0200
Subject: [PATCH 2/2] Improve the naming of guessed target repository for git clone

Strip leading and trailing spaces off guessed target directory, and
replace sequences of whitespace and 'control' characters with one
space character.

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

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-clone.c |   34 +++++++++++++++++++++++++++++-----
 1 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 880373f..d068b7e 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,33 @@ 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);
+		dir = strbuf_detach(&result, 0);
+	} else
+		dir = xstrndup(start, end - start);
+	/*
+	 * Replace sequences of 'control' characters and whitespace
+	 * with one ascii space, remove leading and trailing spaces.
+	 */
+	if (*dir) {
+		char *out = dir;
+		int prev_space = 1 /* strip leading whitespace */;
+		for (end = dir; *end; ++end) {
+			char ch = *end;
+			if ((unsigned char)ch < '\x20')
+				ch = '\x20';
+			if (isspace(ch)) {
+				if (prev_space)
+					continue;
+				prev_space = 1;
+			} else
+				prev_space = 0;
+			*out++ = ch;
+		}
+		*out = '\0';
+		if (out > dir && prev_space)
+			out[-1] = '\0';
 	}
-
-	return xstrndup(start, end - start);
+	return dir;
 }
 
 static void strip_trailing_slashes(char *dir)
-- 
1.6.3.1.37.g5d96e

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Alex Riesen]

2009/5/14 Junio C Hamano <gitster@pobox.com>:
> Alex Riesen <raa.lkml@gmail.com> writes:
>
>>> What's the point of this change, now that you have a fix in 1/2?  Who are
>>> you helping with this patch?
>>
>> Without this the _automatically_ generated names for cloned repositories
>> have all the whitespace around them.
>
> Even if it has whitespace around its name, that's what you got from the
> upstream (a valid source of clone), and wasn't it you who said something
> about UNIX tradition of allowing LF and others in the filename?

Yes, when user explicitely asked a program about that. This here
(clone with only URL as argument) is not the case, I think.

> If clone reports "ok we created this new repository" so that the caller
> can capture it, then the whole process should be able to cope with
> automatically generated names with or without the patch, shouldn't it?

No, don't think so. You're not always able to capture the output of git clone
(Windows again), and BTW - init-db output is not designed to be captured
unambiguously.

> Or are you trying to help a human user who gives a pathname ridden with
> excess whitespaces to "git clone", and that pathname _happens_ to work as
> a valid clone source, creating a new repository whose name is ridden with
> excess whitespaces the same way as the input pathname?

Not really. I just try to make the _generated_ output, which the user cannot
predict anyway (nor does the user care much about it) to be less
problematic. Yes, I did say that LF-anything in UNIX filenames is a normal
thing. That does not mean that such names are so very convenient to use.
They do cause problems, even if just through scrambling terminal output.
They are "inconvenient". If our users don't expect precise output anyway,
we can be a little more adhering to usual practices in choosing names.

> ... After all, the
> user deliberately gave them to us, and the repository we cloned from had
> these excesses in its name (iow, without the excess whitespaces the clone
> itself wouldn't have worked).  In such a case, is it really helping him to
> remove these whitespaces as excesses?

I think yes. Otherwise the strict form of git clone could have been used,
which involves absolutely no guessing and mangling.

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Hugo Mildenberger]

Am Donnerstag, 14. Mai 2009 schrieb Alex Riesen:
> 2009/5/14 Junio C Hamano <gitster@pobox.com>:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> >
> >>> What's the point of this change, now that you have a fix in 1/2?  Who are
> >>> you helping with this patch?
> >>
> >> Without this the _automatically_ generated names for cloned repositories
> >> have all the whitespace around them.
> >
> > Even if it has whitespace around its name, that's what you got from the
> > upstream (a valid source of clone), and wasn't it you who said something
> > about UNIX tradition of allowing LF and others in the filename?
> 
> Yes, when user explicitely asked a program about that. This here
> (clone with only URL as argument) is not the case, I think.
> 
> > If clone reports "ok we created this new repository" so that the caller
> > can capture it, then the whole process should be able to cope with
> > automatically generated names with or without the patch, shouldn't it?
> 
> No, don't think so. You're not always able to capture the output of git clone
> (Windows again), and BTW - init-db output is not designed to be captured
> unambiguously.
> 
> > Or are you trying to help a human user who gives a pathname ridden with
> > excess whitespaces to "git clone", and that pathname _happens_ to work as
> > a valid clone source, creating a new repository whose name is ridden with
> > excess whitespaces the same way as the input pathname?
> 
> Not really. I just try to make the _generated_ output, which the user cannot
> predict anyway (nor does the user care much about it) to be less
> problematic. Yes, I did say that LF-anything in UNIX filenames is a normal
> thing. That does not mean that such names are so very convenient to use.
> They do cause problems, even if just through scrambling terminal output.
> They are "inconvenient". If our users don't expect precise output anyway,
> we can be a little more adhering to usual practices in choosing names.
> 
> > ... After all, the
> > user deliberately gave them to us, and the repository we cloned from had
> > these excesses in its name (iow, without the excess whitespaces the clone
> > itself wouldn't have worked).  In such a case, is it really helping him to
> > remove these whitespaces as excesses?
> 
> I think yes. Otherwise the strict form of git clone could have been used,
> which involves absolutely no guessing and mangling.
> 
This discussion began with the problem I encountered during a git clone 
operation applied to an url which accidentally included a linefeed. It partly 
went through although the remote side did not include that character. 
After Alex Riesen fixed the git part by escaping linefeeds, the linefeed 
left over in the top level directory name still caused a linux kernel 
make to fail (which really is not problem of git or GNU make itself). 
Elsewhere in this thread, Daniel Barkalow proposed to apply the HTTP 
RFC to the whole url. If accepted,  this would cope with the problem in a 
standard way. 

Re: [PATCH 2/2] Improve the naming of guessed target repository for git clone

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Strip leading and trailing spaces off guessed target directory, and
> replace sequences of whitespace and 'control' characters with one
> space character.
>
> User still can have any name by specifying it explicitely after url.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

Thanks, will queue.