Re: [PATCH] Allow git-mergetool to handle paths with a leading space

Rogan Dawes [off-list ref] writes:

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

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:

Rogan Dawes [off-list ref] writes:

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.

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

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.

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