BUG?: git diff with 3 args consults work tree when it shouldn't

BUG?: git diff with 3 args consults work tree when it shouldn't

[Johan Herland]

Hi,

I'm viewing the combined diff of a merge commit where the correct 
resolution involved the removal of a conflicting file. In short, the 
(simplified) scenario is:

    A
   / \
  O   M
   \ /
    B

where commit O does not contain the file, branches A and B add 
conflicting versions of the file, and merge M removes the file (see 
below for a full command session illustrating the scenario).

When I view the combined diff of the merge (using "git diff M A B" or 
"git show M" ), it seems the resulting diff depends on the 
existence/contents of the file in my *current work tree*. I.e. if I 
checkout O or M (where the file is missing) before producing the diff, 
the result seems correct, but if I put a garbage version of the file in 
my work tree (without adding or committing it at all), the garbage shows 
up in the diff, as follows:

  $ git diff M A B
  diff --cc file
  index 34b6a0c,732c85a..0000000
  --- a/file
  +++ b/file
  @@@ -1,1 -1,1 +1,1 @@@
  - contents contributed by A
   -contents contributed by B
  ++garbage contents from my work tree

AFAICS, when producing combined diffs, the diff code should not look up 
the file in the current work tree, since the diff arguments refer to 
existing commits, and do not indicate that the work tree should be 
consulted at all.


...Johan


Example session illustrating the problem:

$ git init repo
Initialized empty Git repository in ./repo/.git/
$ cd repo
$ echo foo > foo
$ git add foo
$ git commit -m foo
[master (root-commit) c6d62c2] foo
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo
$ git checkout -b a
Switched to a new branch 'a'
$ echo spam > file
$ git add file
$ git commit -m spam
[a d2d3128] spam
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file
$ git checkout -b b master
Switched to a new branch 'b'
$ echo eggs > file
$ git add file
$ git commit -m eggs
[b 72e8252] eggs
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file
$ git checkout -b m
Switched to a new branch 'm'
$ git merge a
Auto-merging file
CONFLICT (add/add): Merge conflict in file
Automatic merge failed; fix conflicts and then commit the result.
$ git rm file
file: needs merge
rm 'file'
$ git commit -m merge
[m c096356] merge
$ git diff m a b
diff --cc file
index 34b6a0c,732c85a..0000000
deleted file mode 100644,100644
--- a/file
+++ /dev/null
$ ### HERE IS WHERE IT GOES WRONG:
$ echo xyzzy > file
$ git diff m a b
diff --cc file
index 34b6a0c,732c85a..0000000
--- a/file
+++ b/file
@@@ -1,1 -1,1 +1,1 @@@
- spam
 -eggs
++xyzzy



-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re* BUG?: git diff with 3 args consults work tree when it shouldn't

[Junio C Hamano]

What happens is that the more-than-two-ways combined diff is sometimes
used to show the difference _to_ the working tree files.

In combine-diff.c::show_patch_diff(), we try to detect that case by
checking if sha1 recorded as "the result" is null, because diff-files will
give us 0{40} as the blob object name for files in the working tree.

Unfortunately, that forgets another case where blob object is null, which
is when the merge result tree _deleted_ the path.

This patch is not tested outside your test case. IOW, I know it will get
three-tree case correctly, but I didn't test it with cases where we should
look at the working tree files, so this may introduce regressions.

See 713a11f (combine-diff: diff-files fix., 2006-02-13) if you are
interested. I suspect that we may need to add a bit to the API to tell
this function if the end result is in the working tree.

 combine-diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index be67cfc..b4f1050 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -777,7 +777,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	struct sline *sline; /* survived lines */
 	int mode_differs = 0;
 	int i, show_hunks;
-	int working_tree_file = is_null_sha1(elem->sha1);
+	int working_tree_file = is_null_sha1(elem->sha1) && elem->mode;
 	mmfile_t result_file;
 	struct userdiff_driver *userdiff;
 	struct userdiff_driver *textconv = NULL;

[PATCH] diff -c/--cc: do not mistake "resolved as deletion" as "use working tree"

[Junio C Hamano]

The combined diff machinery can be used to compare:

 - a merge commit with its parent commits;
 - a working-tree file with multiple stages in an unmerged index; or
 - a working-tree file with the HEAD and the index.

The internal function combine-diff.c:show_patch_diff() checked if it needs
to read the "result" from the working tree by looking at the object name
of the result --- if it is null_sha1, it read from the working tree.

This mistook a merge that records a deletion as the conflict resolution
as if it is a cue to read from the working tree. Pass this information
explicitly from the caller instead.

Noticed and reported by Johan Herland.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The patch is based on a bit older maintenance branch but will be queued
   in 'pu'.

 combine-diff.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 655fa89..360b816 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -682,7 +682,8 @@ static void dump_quoted_path(const char *head,
 }
 
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
-			    int dense, struct rev_info *rev)
+			    int dense, int working_tree_file,
+			    struct rev_info *rev)
 {
 	struct diff_options *opt = &rev->diffopt;
 	unsigned long result_size, cnt, lno;
@@ -691,7 +692,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	struct sline *sline; /* survived lines */
 	int mode_differs = 0;
 	int i, show_hunks;
-	int working_tree_file = is_null_sha1(elem->sha1);
 	int abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
 	const char *a_prefix, *b_prefix;
 	mmfile_t result_file;
@@ -954,6 +954,12 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 	write_name_quoted(p->path, stdout, line_termination);
 }
 
+/*
+ * The result (p->elem) is from the working tree and their
+ * parents are typically from multiple stages during a merge
+ * (i.e. diff-files) or the state in HEAD and in the index
+ * (i.e. diff-index).
+ */
 void show_combined_diff(struct combine_diff_path *p,
 		       int num_parent,
 		       int dense,
@@ -967,7 +973,7 @@ void show_combined_diff(struct combine_diff_path *p,
 				  DIFF_FORMAT_NAME_STATUS))
 		show_raw_diff(p, num_parent, rev);
 	else if (opt->output_format & DIFF_FORMAT_PATCH)
-		show_patch_diff(p, num_parent, dense, rev);
+		show_patch_diff(p, num_parent, dense, 1, rev);
 }
 
 void diff_tree_combined(const unsigned char *sha1,
@@ -1035,7 +1041,7 @@ void diff_tree_combined(const unsigned char *sha1,
 			for (p = paths; p; p = p->next) {
 				if (p->len)
 					show_patch_diff(p, num_parent, dense,
-							rev);
+							0, rev);
 			}
 		}
 	}
-- 
1.7.6.401.g6a319