Re: [PATCH] mergetool: respect autocrlf by using checkout-index

[Charles Bailey <charles@hashpling.org>]

On Fri, Jan 23, 2009 at 09:20:10AM -0800, Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
> > Previously, git mergetool used cat-file which does not perform git to
> > worktree conversion. This changes mergetool to use git checkout-index
> > instead which means that the temporary files used for mergetool use the
> > correct line endings for the platform.
> >
> > Signed-off-by: Charles Bailey <charles@hashpling.org>
> 
> Sounds like the right thing to do and from a cursory review it looks Ok to
> me.
> 
> But I do not use mergetool myself, so an Ack from Ted and a Thanks from
> whoever reported the breakage would be encouraging ;-).

Yes, please!

I had wondered why I hadn't really noticed about this 'issue' before
as I've used git mergetool on windows with autocrlf set to true quite
a bit. I think that if your mergetool handles LF endings it doesn't
really matter as it's only the temporary files that are affected and
if the mergetool generates LF output files in response to LF input
files then this is resolved to the correct format at the time it is
added to the index in any case.

> > +checkout_staged_file () {
> > +    tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^	]*\)	')
> > +
> > +    if test $? -eq 0 -a -n "$tmpfile" ; then
> > +	mv -- "$tmpfile" "$3"
> 
> The original redirects into the final destination but this moves.  This
> will lose the perm bits of the original and obey the perm bits
> checkout-index gives you.  It will also behave differently when the path
> is a symlink.  These two differences _may_ well be improvements and/or
> bugfixes, but if that is the case please describe them as such.

I hadn't actually thought about the perms thing that much but now that
I do...

This code, and the code that it replaces, only affects the temporary
files that form the basis for the merge. The result/destination file
is as generated by the merge (or rebase).

The redirect version (as is) will not set permissions from the index -
effectively losing information, the new version should (I think - I'm
not an expert in checkout-index) get the 'correct' repository
permissions. I would say that this is, if anything, an improvement.
The ultimate effect really depends on the mergetool and whether the
source file permissions affect the permissions that it sets on the
target file. In the vast majority of cases I would think that it
doesn't have any effect.

Note that symlinks in the repository are not resolved by this code
path so they aren't affected.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/