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

From: Jeff King <peff@peff.net>
Date: Jan 24, 2019, 21:15 UTC
Message-ID: <20190124211505.GA16114@sigill.intra.peff.net>
In-Reply-To: <xmqqy379hkri.fsf@gitster-ct.c.googlers.com>

On Thu, Jan 24, 2019 at 12:18:41PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

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

Show 9 quoted lines

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