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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.

Wow, that was nasty.  

I did not like the complexity of that "emitted symbols" conversion
we had to do recently and never trusted the code.  There still is
something funny in diff_flush_patch_all_file_pairs() even after this
patch, though.

 - We first check o->color_moved and unconditionally point
   o->emitted_symbols to &esm.

 - In an if() block we enter when o->emitted_symbols is set, there
   is a check to see if o->color_moved is set.  This makes sense
   only if we are trying to be prepared to handle a case where we
   are not the one that assigned a non-NULL to o->emitted_symbols
   due to o->color_moved.  So it certainly is possible that
   o->emitted_symbols is set before we enter this function.

 - But then, it means that o->emitted_symbols we may have had
   non-NULL when the function is called may be overwritten if
   o->color_moved is set.

The above observation does not necessarily indicate any bug; it just
shows that the code structure is messier than necessary.

> 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.

Perhaps.  I see word-diff codepath gives an allocated buffer to
o->emitted_symbols, so assigning NULL without freeing would mean a
leak, but I guess this helper function is not designed to be called