diff: clear emitted_symbols flag after use

peff · gitster · commit 48edf3a02add · 2019-01-24T11:59:07.000-08:00
There's an odd bug when "log --color-moved" is used with the combination
of "--cc --stat -p": the stat for merge commits is erroneously shown
with the diff of the _next_ commit.

The included test demonstrates the issue. Our history looks something
like this:

  A-B-M--D
   \ /
    C

When we run "git log --cc --stat -p --color-moved" starting at D, we get
this sequence of events:

  1. The diff for D is using -p, so diff_flush() calls into
     diff_flush_patch_all_file_pairs(). There we see that o-&gt;color_moved
     is in effect, so we point o-&gt;emitted_symbols to a static local
     struct, causing diff_flush_patch() to queue the symbols instead of
     actually writing them out.

     We then do our move detection, emit the symbols, and clear the
     struct. But we leave o-&gt;emitted_symbols pointing to our struct.

  2. Next we compute the diff for M. This is a merge, so we use the
     combined diff code. In find_paths_generic(), we compute the
     pairwise diff between each commit and its parent. Normally this is
     done with DIFF_FORMAT_NO_OUTPUT, since we're just looking for
     intersecting paths. But since "--stat --cc" shows the first-parent
     stat, and since we're computing that diff anyway, we enable
     DIFF_FORMAT_DIFFSTAT for the first parent. This outputs the stat
     information immediately, saving us from running a separate
     first-parent diff later.

     But where does that output go? Normally it goes directly to stdout,
     but because o-&gt;emitted_symbols is set, we queue it. As a result, we
     don't actually print the diffstat for the merge commit (yet), which
     is wrong.

  3. Next we compute the diff for C. We're actually showing a patch
     again, so we end up in diff_flush_patch_all_file_pairs(), but this
     time we have the queued stat from step 2 waiting in our struct.

     We add new elements to it for C's diff, and then flush the whole
     thing. And we see the diffstat from M as part of C's diff, which is
     wrong.

So triggering the bug really does require the combination of all of
those options.

To fix it, we can simply restore o-&gt;emitted_symbols to NULL after
flushing it, so that it does not affect anything outside of
diff_flush_patch_all_file_pairs(). This intuitively makes sense, since
nobody outside of that function is going to bother flushing it, so we
would not want them to write to it either.

In fact, we could take this a step further and turn the local "esm"
struct into a non-static variable that goes away after the function
ends. However, since it contains a dynamically sized array, we benefit
from amortizing the cost of allocations over many calls. So we'll leave
it as static to retain that benefit.

But let's push the zero-ing of esm.nr into the conditional for "if
(o-&gt;emitted_symbols)" to make it clear that we do not expect esm to hold
any values if we did not just try to use it. With the code as it is
written now, if we did encounter such a case (which I think would be a
bug), we'd silently leak those values without even bothering to display
them. With this change, we'd at least eventually show them, and somebody
would notice.

Signed-off-by: Jeff King &lt;peff@peff.net&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/diff.c b/diff.c
@@ -5885,8 +5885,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 
 		for (i = 0; i < esm.nr; i++)
 			free((void *)esm.buf[i].line);
+		esm.nr = 0;
+
+		o->emitted_symbols = NULL;
 	}
-	esm.nr = 0;
 }
 
 void diff_flush(struct diff_options *options)
diff --git a/t/t4066-diff-emit-delay.sh b/t/t4066-diff-emit-delay.sh
@@ -0,0 +1,79 @@
+#!/bin/sh
+
+test_description='test combined/stat/moved interaction'
+. ./test-lib.sh
+
+# This test covers a weird 3-way interaction between "--cc -p", which will run
+# the combined diff code, along with "--stat", which will be computed as a
+# first-parent stat during the combined diff, and "--color-moved", which
+# enables the emitted_symbols list to store the diff in memory.
+
+test_expect_success 'set up history with a merge' '
+	test_commit A &&
+	test_commit B &&
+	git checkout -b side HEAD^ &&
+	test_commit C &&
+	git merge -m M master &&
+	test_commit D
+'
+
+test_expect_success 'log --cc -p --stat --color-moved' '
+	cat >expect <<-\EOF &&
+	commit D
+	---
+	 D.t | 1 +
+	 1 file changed, 1 insertion(+)
+
+	diff --git a/D.t b/D.t
+	new file mode 100644
+	index 0000000..1784810
+	--- /dev/null
+	+++ b/D.t
+	@@ -0,0 +1 @@
+	+D
+	commit M
+
+	 B.t | 1 +
+	 1 file changed, 1 insertion(+)
+	commit C
+	---
+	 C.t | 1 +
+	 1 file changed, 1 insertion(+)
+
+	diff --git a/C.t b/C.t
+	new file mode 100644
+	index 0000000..3cc58df
+	--- /dev/null
+	+++ b/C.t
+	@@ -0,0 +1 @@
+	+C
+	commit B
+	---
+	 B.t | 1 +
+	 1 file changed, 1 insertion(+)
+
+	diff --git a/B.t b/B.t
+	new file mode 100644
+	index 0000000..223b783
+	--- /dev/null
+	+++ b/B.t
+	@@ -0,0 +1 @@
+	+B
+	commit A
+	---
+	 A.t | 1 +
+	 1 file changed, 1 insertion(+)
+
+	diff --git a/A.t b/A.t
+	new file mode 100644
+	index 0000000..f70f10e
+	--- /dev/null
+	+++ b/A.t
+	@@ -0,0 +1 @@
+	+A
+	EOF
+	git log --format="commit %s" --cc -p --stat --color-moved >actual &&
+	test_cmp expect actual
+'
+
+test_done

Original file line number	Diff line number	Diff line change
`@@ -5885,8 +5885,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)`
`5885`	`5885`
`5886`	`5886`	`for (i = 0; i < esm.nr; i++)`
`5887`	`5887`	`free((void *)esm.buf[i].line);`
	`5888`	`+ esm.nr = 0;`
	`5889`	`+`
	`5890`	`+ o->emitted_symbols = NULL;`
`5888`	`5891`	`}`
`5889`		`- esm.nr = 0;`
`5890`	`5892`	`}`
`5891`	`5893`
`5892`	`5894`	`void diff_flush(struct diff_options *options)`