[PATCH 2/6] diff: clear emitted_symbols flag after use

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->color_moved
     is in effect, so we point o->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->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->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->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->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 <peff@peff.net>
---
 diff.c                     |  4 +-
 t/t4066-diff-emit-delay.sh | 79 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100755 t/t4066-diff-emit-delay.sh