On Thu, Jan 24, 2019 at 12:18:41PM -0800, Junio C Hamano wrote: > 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. Yeah, I noticed that, too. I assumed it was preparing for a day when the logic for "are we collecting symbols" becomes more complex than just being equivalent to "o->color_moved". Under that rationale, I was OK leaving it. > - 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. Yeah, that is true. I think in the new world order proposed by this patch, we'd always assume that it's NULL coming in, possibly assign it, and re-NULL it going out. > The above observation does not necessarily indicate any bug; it just > shows that the code structure is messier than necessary. Yeah, I don't think it's a bug currently, although... > > 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 Hrm, I'm embarrassed to say I did not notice that it also uses the emitted_symbols system. I think we only do it there, though, in the sub-diff_options that word-diff uses, in which case we make a separate emitted_diff_symbols struct instead of re-using the one from the parent diff_options. So I think the general idea still holds, which is that whoever assigns the emitted_symbols flag is responsible for flushing it. For --color-moved, that happens in a single function, but for word-diff, it's split across the init/flush functions. -Peff
