{"thread":{"id":"27985","subject":"BUG?: git diff with 3 args consults work tree when it shouldn't","startedAt":"2011-08-01T17:35:11Z","lastAt":"2011-08-04T18:58:45Z","messageCount":3,"participants":["Johan Herland","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"172552","messageId":"201108011935.11373.johan@herland.net","threadId":"27985","inReplyTo":null,"subject":"BUG?: git diff with 3 args consults work tree when it shouldn't","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-08-01T17:35:11Z","receivedAt":"2011-08-01T17:35:11Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nI'm viewing the combined diff of a merge commit where the correct \nresolution involved the removal of a conflicting file. In short, the \n(simplified) scenario is:\n\n    A\n   / \\\n  O   M\n   \\ /\n    B\n\nwhere commit O does not contain the file, branches A and B add \nconflicting versions of the file, and merge M removes the file (see \nbelow for a full command session illustrating the scenario).\n\nWhen I view the combined diff of the merge (using \"git diff M A B\" or \n\"git show M\" ), it seems the resulting diff depends on the \nexistence/contents of the file in my *current work tree*. I.e. if I \ncheckout O or M (where the file is missing) before producing the diff, \nthe result seems correct, but if I put a garbage version of the file in \nmy work tree (without adding or committing it at all), the garbage shows \nup in the diff, as follows:\n\n  $ git diff M A B\n  diff --cc file\n  index 34b6a0c,732c85a..0000000\n  --- a/file\n  +++ b/file\n  @@@ -1,1 -1,1 +1,1 @@@\n  - contents contributed by A\n   -contents contributed by B\n  ++garbage contents from my work tree\n\nAFAICS, when producing combined diffs, the diff code should not look up \nthe file in the current work tree, since the diff arguments refer to \nexisting commits, and do not indicate that the work tree should be \nconsulted at all.\n\n\n...Johan\n\n\nExample session illustrating the problem:\n\n$ git init repo\nInitialized empty Git repository in ./repo/.git/\n$ cd repo\n$ echo foo > foo\n$ git add foo\n$ git commit -m foo\n[master (root-commit) c6d62c2] foo\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 foo\n$ git checkout -b a\nSwitched to a new branch 'a'\n$ echo spam > file\n$ git add file\n$ git commit -m spam\n[a d2d3128] spam\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 file\n$ git checkout -b b master\nSwitched to a new branch 'b'\n$ echo eggs > file\n$ git add file\n$ git commit -m eggs\n[b 72e8252] eggs\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 file\n$ git checkout -b m\nSwitched to a new branch 'm'\n$ git merge a\nAuto-merging file\nCONFLICT (add/add): Merge conflict in file\nAutomatic merge failed; fix conflicts and then commit the result.\n$ git rm file\nfile: needs merge\nrm 'file'\n$ git commit -m merge\n[m c096356] merge\n$ git diff m a b\ndiff --cc file\nindex 34b6a0c,732c85a..0000000\ndeleted file mode 100644,100644\n--- a/file\n+++ /dev/null\n$ ### HERE IS WHERE IT GOES WRONG:\n$ echo xyzzy > file\n$ git diff m a b\ndiff --cc file\nindex 34b6a0c,732c85a..0000000\n--- a/file\n+++ b/file\n@@@ -1,1 -1,1 +1,1 @@@\n- spam\n -eggs\n++xyzzy\n\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"172844","messageId":"7vsjph6g4j.fsf@alter.siamese.dyndns.org","threadId":"27985","inReplyTo":"201108011935.11373.johan@herland.net","subject":"Re* BUG?: git diff with 3 args consults work tree when it shouldn't","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-04T02:54:20Z","receivedAt":"2011-08-04T02:54:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"What happens is that the more-than-two-ways combined diff is sometimes\nused to show the difference _to_ the working tree files.\n\nIn combine-diff.c::show_patch_diff(), we try to detect that case by\nchecking if sha1 recorded as \"the result\" is null, because diff-files will\ngive us 0{40} as the blob object name for files in the working tree.\n\nUnfortunately, that forgets another case where blob object is null, which\nis when the merge result tree _deleted_ the path.\n\nThis patch is not tested outside your test case. IOW, I know it will get\nthree-tree case correctly, but I didn't test it with cases where we should\nlook at the working tree files, so this may introduce regressions.\n\nSee 713a11f (combine-diff: diff-files fix., 2006-02-13) if you are\ninterested. I suspect that we may need to add a bit to the API to tell\nthis function if the end result is in the working tree.\n\n combine-diff.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex be67cfc..b4f1050 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -777,7 +777,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tstruct sline *sline; /* survived lines */\n \tint mode_differs = 0;\n \tint i, show_hunks;\n-\tint working_tree_file = is_null_sha1(elem->sha1);\n+\tint working_tree_file = is_null_sha1(elem->sha1) && elem->mode;\n \tmmfile_t result_file;\n \tstruct userdiff_driver *userdiff;\n \tstruct userdiff_driver *textconv = NULL;\n"},{"id":"172943","messageId":"7vwret3swq.fsf_-_@alter.siamese.dyndns.org","threadId":"27985","inReplyTo":"7vsjph6g4j.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff -c/--cc: do not mistake \"resolved as deletion\" as \"use working tree\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-04T18:58:45Z","receivedAt":"2011-08-04T18:58:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The combined diff machinery can be used to compare:\n\n - a merge commit with its parent commits;\n - a working-tree file with multiple stages in an unmerged index; or\n - a working-tree file with the HEAD and the index.\n\nThe internal function combine-diff.c:show_patch_diff() checked if it needs\nto read the \"result\" from the working tree by looking at the object name\nof the result --- if it is null_sha1, it read from the working tree.\n\nThis mistook a merge that records a deletion as the conflict resolution\nas if it is a cue to read from the working tree. Pass this information\nexplicitly from the caller instead.\n\nNoticed and reported by Johan Herland.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The patch is based on a bit older maintenance branch but will be queued\n   in 'pu'.\n\n combine-diff.c |   14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..360b816 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -682,7 +682,8 @@ static void dump_quoted_path(const char *head,\n }\n \n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n-\t\t\t    int dense, struct rev_info *rev)\n+\t\t\t    int dense, int working_tree_file,\n+\t\t\t    struct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n \tunsigned long result_size, cnt, lno;\n@@ -691,7 +692,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tstruct sline *sline; /* survived lines */\n \tint mode_differs = 0;\n \tint i, show_hunks;\n-\tint working_tree_file = is_null_sha1(elem->sha1);\n \tint abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;\n \tconst char *a_prefix, *b_prefix;\n \tmmfile_t result_file;\n@@ -954,6 +954,12 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \twrite_name_quoted(p->path, stdout, line_termination);\n }\n \n+/*\n+ * The result (p->elem) is from the working tree and their\n+ * parents are typically from multiple stages during a merge\n+ * (i.e. diff-files) or the state in HEAD and in the index\n+ * (i.e. diff-index).\n+ */\n void show_combined_diff(struct combine_diff_path *p,\n \t\t       int num_parent,\n \t\t       int dense,\n@@ -967,7 +973,7 @@ void show_combined_diff(struct combine_diff_path *p,\n \t\t\t\t  DIFF_FORMAT_NAME_STATUS))\n \t\tshow_raw_diff(p, num_parent, rev);\n \telse if (opt->output_format & DIFF_FORMAT_PATCH)\n-\t\tshow_patch_diff(p, num_parent, dense, rev);\n+\t\tshow_patch_diff(p, num_parent, dense, 1, rev);\n }\n \n void diff_tree_combined(const unsigned char *sha1,\n@@ -1035,7 +1041,7 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\tfor (p = paths; p; p = p->next) {\n \t\t\t\tif (p->len)\n \t\t\t\t\tshow_patch_diff(p, num_parent, dense,\n-\t\t\t\t\t\t\trev);\n+\t\t\t\t\t\t\t0, rev);\n \t\t\t}\n \t\t}\n \t}\n-- \n1.7.6.401.g6a319\n"}]}