git diff, git mergetool and CRLF conversion

git diff, git mergetool and CRLF conversion

[Hannu Koivisto]

Hi,

Suppose I have core.autocrlf set to true and and due to that a
version controlled file in a working tree with CRLF line endings.
If I modify such a file and then say "git diff", I get a patch with
LF line endings.

Also, if get a merge conflict with a file to which CRLF conversion
is applied and run e.g. "git mergetool -t emerge", the temporary
files representing stage2 and stage3 versions seem to have LF line
endings.

Is this intended behaviour?  I'm using 1.6.1 on Cygwin.

-- 
Hannu

Re: git diff, git mergetool and CRLF conversion

[Charles Bailey]

On Wed, Jan 21, 2009 at 06:55:34PM +0200, Hannu Koivisto wrote:
> Hi,
> 
> Suppose I have core.autocrlf set to true and and due to that a
> version controlled file in a working tree with CRLF line endings.
> If I modify such a file and then say "git diff", I get a patch with
> LF line endings.
> 
> Also, if get a merge conflict with a file to which CRLF conversion
> is applied and run e.g. "git mergetool -t emerge", the temporary
> files representing stage2 and stage3 versions seem to have LF line
> endings.
> 
> Is this intended behaviour?  I'm using 1.6.1 on Cygwin.

Speaking for mergetool, I believe that it's simply because mergetool
uses git cat-file which just outputs the raw contents of a blob and
doesn't do any line ending conversion.

IMHO, I think that it should probably perform the 'convert to working
tree format' change when preparing the temporary files. I'm not sure
how best to do that, but perhaps it should be using git checkout-index
with the --temp option instead of cat-file.

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

Re: git diff, git mergetool and CRLF conversion

[Theodore Tso]

On Wed, Jan 21, 2009 at 05:23:51PM +0000, Charles Bailey wrote:
> > Is this intended behaviour?  I'm using 1.6.1 on Cygwin.
> 
> Speaking for mergetool, I believe that it's simply because mergetool
> uses git cat-file which just outputs the raw contents of a blob and
> doesn't do any line ending conversion.
> 
> IMHO, I think that it should probably perform the 'convert to working
> tree format' change when preparing the temporary files. I'm not sure
> how best to do that, but perhaps it should be using git checkout-index
> with the --temp option instead of cat-file.

Yes, I agree, that would probably be better.

						- Ted

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

[Charles Bailey]

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>
---
 git-mergetool.sh     |   14 +++++++++++---
 t/t7610-mergetool.sh |   15 +++++++++++++--
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 00e1337..a4855d9 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -127,6 +127,14 @@ check_unchanged () {
     fi
 }
 
+checkout_staged_file () {
+    tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^	]*\)	')
+
+    if test $? -eq 0 -a -n "$tmpfile" ; then
+	mv -- "$tmpfile" "$3"
+    fi
+}
+
 merge_file () {
     MERGED="$1"
 
@@ -153,9 +161,9 @@ merge_file () {
     local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
     remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
 
-    base_present   && git cat-file blob ":1:$prefix$MERGED" >"$BASE" 2>/dev/null
-    local_present  && git cat-file blob ":2:$prefix$MERGED" >"$LOCAL" 2>/dev/null
-    remote_present && git cat-file blob ":3:$prefix$MERGED" >"$REMOTE" 2>/dev/null
+    base_present   && checkout_staged_file 1 "$prefix$MERGED" "$BASE"
+    local_present  && checkout_staged_file 2 "$prefix$MERGED" "$LOCAL"
+    remote_present && checkout_staged_file 3 "$prefix$MERGED" "$REMOTE"
 
     if test -z "$local_mode" -o -z "$remote_mode"; then
 	echo "Deleted merge conflict for '$MERGED':"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 09fa5f1..edb6a57 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -34,13 +34,24 @@ test_expect_success 'custom mergetool' '
     git config merge.tool mytool &&
     git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
     git config mergetool.mytool.trustExitCode true &&
-	git checkout branch1 &&
+    git checkout branch1 &&
     test_must_fail git merge master >/dev/null 2>&1 &&
     ( yes "" | git mergetool file1>/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2>/dev/null 2>&1 ) &&
     test "$(cat file1)" = "master updated" &&
     test "$(cat file2)" = "master new" &&
-	git commit -m "branch1 resolved with mergetool"
+    git commit -m "branch1 resolved with mergetool"
+'
+
+test_expect_success 'mergetool crlf' '
+    git config core.autocrlf true &&
+    git reset --hard HEAD^
+    test_must_fail git merge master >/dev/null 2>&1 &&
+    ( yes "" | git mergetool file1>/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool file2>/dev/null 2>&1 ) &&
+    test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
+    test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
+    git commit -m "branch1 resolved with mergetool - autocrlf"
 '
 
 test_done
-- 
1.6.1.235.gc9d403

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

[Junio C Hamano]

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

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

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

[Charles Bailey]

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/

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

[Hannu Koivisto]

Junio C Hamano <gitster@pobox.com> writes:

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

I apologize for not being able to test this earlier and I'm
certainly thankful for the patch, although admittedly I reported
the issue mainly to help improve git, not because mergetool is part
of my normal use of git (that may change when I find time to study it
and see if I can easily add ediff support to it in addition to
emerge).

Now that I tried the patch, I observed that while the stage2 and
stage3 temporary files have CRLF line endings, the merge result
buffer/file has LF line endings.  I'm again using Cygwin git,
mergetool -t emerge and native Windows Emacs.  So when I quit the
mergetool, I get

...
Hit return to start merge resolution tool (emerge):
warning: LF will be replaced by CRLF in kala.txt
warning: LF will be replaced by CRLF in kala.txt

and indeed hexdump proves that the file in my worktree now has LF
line endings even though it had CRLF line endings before invoking
mergetool.

I wonder why I didn't notice this the first time.  I can certainly
reproduce it now without Charles' patch as well so I suppose this
is a separate issue and the patch does what it is supposed to do.

-- 
Hannu

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

[Charles Bailey]

On Mon, Jan 26, 2009 at 06:15:01PM +0200, Hannu Koivisto wrote:
>
> Now that I tried the patch, I observed that while the stage2 and
> stage3 temporary files have CRLF line endings, the merge result
> buffer/file has LF line endings.  I'm again using Cygwin git,
> mergetool -t emerge and native Windows Emacs.  So when I quit the
> mergetool, I get
> 
> ...
> Hit return to start merge resolution tool (emerge):
> warning: LF will be replaced by CRLF in kala.txt
> warning: LF will be replaced by CRLF in kala.txt
> 
> and indeed hexdump proves that the file in my worktree now has LF
> line endings even though it had CRLF line endings before invoking
> mergetool.
> 
> I wonder why I didn't notice this the first time.  I can certainly
> reproduce it now without Charles' patch as well so I suppose this
> is a separate issue and the patch does what it is supposed to do.

mergetool doesn't touch the destination file, it just asks the merge
tool to overwrite it.

I suspect that the LF endings in the file is due to the fact that in
builtin-merge-file.c, the file is opened (fopen) in binary mode
("wb"), but xdl_merge terminates all lines with a raw '\n'.

The obvious fix would be to change fopen in builtin-file-merge.c to
use "w" instead, but this doesn't work in a number of scenarios. In
particular, it is wrong for repositories on windows with core.autocrlf
set to false, and would not fix non-windows repositories with
core.autocrlf set to true.

Currently, I've no idea as to what the solution should be.

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

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> I suspect that the LF endings in the file is due to the fact that in
> builtin-merge-file.c, the file is opened (fopen) in binary mode
> ("wb"), but xdl_merge terminates all lines with a raw '\n'.
>
> The obvious fix would be to change fopen in builtin-file-merge.c to
> use "w" instead, but this doesn't work in a number of scenarios. In
> particular, it is wrong for repositories on windows with core.autocrlf
> set to false, and would not fix non-windows repositories with
> core.autocrlf set to true.
>
> Currently, I've no idea as to what the solution should be.

"git file-merge" is designed to be a replacement for stock RCS merge, and
unfortunately it does not call convert_to_working_tree(), nor has any way
to know for which path it should take the attributes to apply to affect
what convert_to_working_tree() should do even if it were to call it.

I think we would need a new option to the command that says "pretend this
is about merging this path, and use the gitattributes specified for it
when writing out the result."

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Charles Bailey <charles@hashpling.org> writes:
>
>> I suspect that the LF endings in the file is due to the fact that in
>> builtin-merge-file.c, the file is opened (fopen) in binary mode
>> ("wb"), but xdl_merge terminates all lines with a raw '\n'.
>>
>> The obvious fix would be to change fopen in builtin-file-merge.c to
>> use "w" instead, but this doesn't work in a number of scenarios. In
>> particular, it is wrong for repositories on windows with core.autocrlf
>> set to false, and would not fix non-windows repositories with
>> core.autocrlf set to true.
>>
>> Currently, I've no idea as to what the solution should be.
>
> "git file-merge" is designed to be a replacement for stock RCS merge, and
> unfortunately it does not call convert_to_working_tree(), nor has any way
> to know for which path it should take the attributes to apply to affect
> what convert_to_working_tree() should do even if it were to call it.
>
> I think we would need a new option to the command that says "pretend this
> is about merging this path, and use the gitattributes specified for it
> when writing out the result."

Perhaps something along this line to teach

    $ git merge-file --attribute-path=frotz.c file1 orig_file file2

to merge what happened since orig_file to file2 into file1, and deposit
the result after converting it appropriately for path "frotz.c" obeying
core.autocrlf and gitattribute rules.

I see rerere.c::merge() has the exact same issue, but its breakage is half
hidden by its use of fopen(path, "w").  It should explicitly use
convert_to_working_tree() like this patch does, and write the results out
in binary mode.

 builtin-merge-file.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git i/builtin-merge-file.c w/builtin-merge-file.c
index 96edb97..61d1092 100644
--- i/builtin-merge-file.c
+++ w/builtin-merge-file.c
@@ -5,7 +5,7 @@
 #include "parse-options.h"
 
 static const char *const merge_file_usage[] = {
-	"git merge-file [options] [-L name1 [-L orig [-L name2]]] file1 orig_file file2",
+	"git merge-file [options] [-L name1 [-L orig [-L name2]]] [--attribute-path path] file1 orig_file file2",
 	NULL
 };
 
@@ -30,10 +30,13 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	int merge_level = XDL_MERGE_ZEALOUS_ALNUM;
 	int merge_style = 0, quiet = 0;
 	int nongit;
+	char *attribute_path = NULL;
 
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, "send results to standard output"),
 		OPT_SET_INT(0, "diff3", &merge_style, "use a diff3 based merge", XDL_MERGE_DIFF3),
+		OPT_STRING('a', "attribute-path", &attribute_path, "path",
+			   "apply work-tree conversion for the path"),
 		OPT__QUIET(&quiet),
 		OPT_CALLBACK('L', NULL, names, "name",
 			     "set labels for file1/orig_file/file2", &label_cb),
@@ -73,6 +76,19 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < 3; i++)
 		free(mmfs[i].ptr);
 
+	if (ret >= 0 && attribute_path) {
+		struct strbuf buf = STRBUF_INIT;
+		ret = convert_to_working_tree(attribute_path,
+					      result.ptr, result.size,
+					      &buf);
+		free(result.ptr);
+		if (!ret) {
+			size_t len;
+			result.ptr = strbuf_detach(&buf, &len);
+			result.size = len;
+		}
+	}
+
 	if (ret >= 0) {
 		const char *filename = argv[0];
 		FILE *f = to_stdout ? stdout : fopen(filename, "wb");

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps something along this line to teach
>
>     $ git merge-file --attribute-path=frotz.c file1 orig_file file2
>
> to merge what happened since orig_file to file2 into file1, and deposit
> the result after converting it appropriately for path "frotz.c" obeying
> core.autocrlf and gitattribute rules.
>
> I see rerere.c::merge() has the exact same issue, but its breakage is half
> hidden by its use of fopen(path, "w").  It should explicitly use
> convert_to_working_tree() like this patch does, and write the results out
> in binary mode.

Second try.  I forgot how convert_* worked X-<.

 builtin-merge-file.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git i/builtin-merge-file.c w/builtin-merge-file.c
index 96edb97..edee815 100644
--- i/builtin-merge-file.c
+++ w/builtin-merge-file.c
@@ -5,7 +5,7 @@
 #include "parse-options.h"
 
 static const char *const merge_file_usage[] = {
-	"git merge-file [options] [-L name1 [-L orig [-L name2]]] file1 orig_file file2",
+	"git merge-file [options] [-L name1 [-L orig [-L name2]]] [--attribute-path path] file1 orig_file file2",
 	NULL
 };
 
@@ -30,10 +30,13 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	int merge_level = XDL_MERGE_ZEALOUS_ALNUM;
 	int merge_style = 0, quiet = 0;
 	int nongit;
+	char *attribute_path = NULL;
 
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, "send results to standard output"),
 		OPT_SET_INT(0, "diff3", &merge_style, "use a diff3 based merge", XDL_MERGE_DIFF3),
+		OPT_STRING('a', "attribute-path", &attribute_path, "path",
+			   "apply work-tree conversion for the path"),
 		OPT__QUIET(&quiet),
 		OPT_CALLBACK('L', NULL, names, "name",
 			     "set labels for file1/orig_file/file2", &label_cb),
@@ -73,6 +76,21 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < 3; i++)
 		free(mmfs[i].ptr);
 
+	if (ret >= 0 && attribute_path) {
+		struct strbuf buf = STRBUF_INIT;
+		int st;
+		st = convert_to_working_tree(attribute_path,
+					     result.ptr, result.size,
+					     &buf);
+		if (st) {
+			size_t len;
+
+			free(result.ptr);
+			result.ptr = strbuf_detach(&buf, &len);
+			result.size = len;
+		}
+	}
+
 	if (ret >= 0) {
 		const char *filename = argv[0];
 		FILE *f = to_stdout ? stdout : fopen(filename, "wb");

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

[Hannu Koivisto]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Perhaps something along this line to teach
>>
>>     $ git merge-file --attribute-path=frotz.c file1 orig_file file2
>>
>> to merge what happened since orig_file to file2 into file1, and deposit
>> the result after converting it appropriately for path "frotz.c" obeying
>> core.autocrlf and gitattribute rules.
>>
>> I see rerere.c::merge() has the exact same issue, but its breakage is half
>> hidden by its use of fopen(path, "w").  It should explicitly use
>> convert_to_working_tree() like this patch does, and write the results out
>> in binary mode.
>
> Second try.  I forgot how convert_* worked X-<.

Argh, it seems I have wasted your time.  That patch may do
something useful but in this case with _or without_ it all the
files seem to be correct in the filesystem before I save the merge
result in emerge.  I.e. it seems that for some reason Emacs detects
the coding system of the result file incorrectly.  I'll investigate
that at some point especially if ediff suffers from the same
problem.

-- 
Hannu