patchAllow git-mergetool to handle paths with a leading space

Signed-off-by: Rogan Dawes <rogan@dawes.za.net>

--- I am working on a project which has the root directory constructed with a leading space. i.e. ./ dir/. "read" skips the leading space char, and ends up with an incorrect filename, which can then not be found. Setting IFS=\n solves this problem.

Thanks to Mikachu and Tv on #git for assistance and suggestions.

Unfortunately, Thunderbird is not very easy to convince not to mangle patches, so I am attaching the patch as well. Sorry.

Rogan Dawes <rogan@dawes.za.net> writes:

Does the project have a file that has letter 'n' (en) in its name? Have you tested your patch while having a conflict in that file?

Junio C Hamano wrote:

Yes, it works correctly.

$ git mergetool merge tool candidates: kdiff3 tkdiff xxdiff meld gvimdiff vimdiff opendiff emerge vimdiff Merging the files: webgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java webgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BlindSqlInjection.java webgoat/main/project/JavaSource/org/owasp/webgoat/lessons/Challenge2Screen.java

Normal merge conflict for ' 
webgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java':
   {local}: modified
   {remote}: modified
Hit return to start merge resolution tool (kdiff3):
merge of 
webgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java 
failed

$

My copy and paste does not show the spaces properly, since everything gets wrapped. but the "Normal merge conflict for ' webgoat/" line would have taken the ' to the next line if the space was not there. :-)

Note that the lines after "Merging the files" don't include the spaces, but that is just cosmetic, IMO.

Rogan

Rogan Dawes <rogan@dawes.za.net> writes:

I am curious and puzzled...

        $ echo 'ann1' | (IFS=\n read i; echo "<$i>")
        <ann1>
        $ echo 'ann1' | (IFS=\n read i j; echo "<$i>")
	<a>
        $ echo 'n1' | (IFS=\n read i j; echo "<$i>")
	<>

Ok, "\n" is a funny way to say IFS does not matter as long as it is set to a non whitespace letter.

It is VERY misleading as it looks as if the issue is fixed by setting IFS to a single LF alone (excluding SP and HT from the usual set), but that is not the patch is doing. It is setting it to a single 'n'.

I think you still will lose backslash by using read, but I guess you would not care about that too much.

If you really cared, you would do something like this, but you would also need similar surgery in merge_file function itself that parses text form of ls-files output that tries to verify and extract paths, which I did not bother to touch in this demonstration patch.

 git-mergetool.sh |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

Signed-off-by: Rogan Dawes <rogan@dawes.za.net>
---
Junio C Hamano wrote:

Yes, you are right. Maybe setting IFS to the empty string is better?

$ printf " ann 1\n ann 2\n" | while IFS="" read i j k ; do echo "<$i> <$j> <$k>"; done < ann 1> <> <> < ann 2> <> <> $

Admittedly, there are still problems with my version, as you say, backslashes and newlines will not be handled correctly. My Perl-fu is weak, however, and my revised solution (IFS="") works for me :-)

Rogan

P.S. Quoting "$files" stops the spaces from being eaten in the preceding echo line.

Rogan Dawes <rogan@dawes.za.net> writes:

Yes. POSIX XCU "2.6.5 Field Splitting" is quite clear about this. Empty IFS tells read not to split the fields, and such an IFS does not have IFS white space in it so the SP at the beginning of the input will not be stripped.

My limited test indicates that bash, dash and ksh do behave as specified, so I think it is a reasonably safe thing to do. Although I usually would say "I do not care about Solaris default /bin/sh", I know even that shell at least gets this right.

I think the patch is Ok as long as the user does not care any character outside the portable filename character set (POSIX XBD 3.276) other than SP. Because the rest of the mergetool is written in such a way that it does not handle many characters outside the portable filename character set well anyway, I think the filename limitation that still remains after your patch may be acceptable.

I did not however apply your patch to my tree tonight, as Ted as the original and the primary author of the script may have better ideas, and/or future directions for the patch. He can simply just Ack your patch if he chooses to.

I'd however write that overly long pipeline indented a bit more sanely, like this, though:

	git ls-files -u |
        sed -e 's/^[^	]*	//' |
        sort -u |
        while IFS="" read i
	do
        	printf "\n"
                merge_file "$i" </dev/tty >/dev/tty
                ...

In the longer term, I'd probably suggest redoing the entire command in a NUL safe scripting language (or even C, especially if pressed by mingw or msys folks) to eliminate the issue altogether, though. But that would definitely be a post 1.5.4 item.

[Reference]

http://www.opengroup.org/onlinepubs/000095399/toc.htm

On Mon, Jan 07, 2008 at 01:09:45AM -0800, Junio C Hamano wrote:

Acked-by: "Theodore Ts'o" <tytso@mit.edu>

Yeah, we eventually need to redo "git mergetool" in C. One of the reasons for that is at the moment it can't take advantage of the rename detection machinery, so in the case where there is a merge conflict involving a file that has been renamed in one of the two branches, "git mergetool" doesn't do the right thing.

Even if it would be possible to expose the rename machinery in a form that would be convenient for a shell script, it's past time to rewrite it in C, I think....

						- Ted