threads / discuss / 17284
Subject: git diff, git mergetool and CRLF conversion
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
Charles BaileyRe: git diff, git mergetool and CRLF conversion
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/
Theodore TsoRe: git diff, git mergetool and CRLF conversion
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
Charles Bailey[PATCH] mergetool: respect autocrlf by using checkout-index
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
Junio C HamanoRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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.
Charles BaileyRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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
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
Charles BaileyRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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/
Junio C HamanoRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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."
Junio C HamanoRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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");
Junio C HamanoRe: [PATCH] mergetool: respect autocrlf by using checkout-index
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
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