Jeff King <peff@xxxxxxxx> 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
