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

From: Jeff King <peff@peff.net>
Date: Jan 24, 2019, 19:11 UTC
Message-ID: <20190124191124.GB29828@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kbHLvN252v-gNbcpsyGg8pZ9GPBtyZquX50HwhtYep5oA@mail.gmail.com>

On Thu, Jan 24, 2019 at 10:55:10AM -0800, Stefan Beller wrote:

Show 16 quoted lines

> >      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),
> 
> Thanks for your analysis. As always a pleasant read.
> I understand and agree with what is written up to here remembering
> the code vaguely.
> 
> > which
> >      is wrong.
> 
> I disagree with this sentiment. If we remember to flush the queued output
> this is merely an inefficiency due to implementation details, but not wrong.
> 
> We could argue that it is wrong to have o->emitted_symbols set, as
> we know we don't need it for producing a diffstat only.

It's wrong in the sense that we finish printing that merge commit without having shown its diff. If it were the final commit, we would not ever print it at all!

So if you are arguing that it would be OK to queue it as long as we flushed it before deciding we were done with the diff, then I agree. But doing that correctly would actually be non-trivial, because the combined-diff code does not use the emitted_symbols queue for its diff (so the stat and the patch would appear out of order).

I also wondered why diffstats go to o->emitted_symbols at all. We do not do any analysis of them with --color-moved, I don't think. But I can also see that having emitted_symbols hold everything makes sense from a maintainability standpoint; future features may want to see more of what we're emitting.

Show 9 quoted lines

> >   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.
> 
> Right, that is how the queueing can produce errors. I wonder if the
> test that is included in this patch would work on top of
> e6e045f803 ("diff.c: buffer all output if asked to", 2017-06-29)
> as that commit specifically wanted to make sure these errors
> would be caught.

I suspect that would not work with "--cc", because combine-diff outputs directly stdout. That's something that we might want to improve in the long run (since obviously it cannot use --color-moved at this point).

Show 8 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.
> 
> This would also cause the inefficiency I mentioned after (2) to disappear,
> as the merge commits diffstat would be just printed to stdout?

Yes, it avoids the overhead of even storing them in the emitted struct at all.

> Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks!

I did quite a bit of head-scratching figuring out this bug, but at the end of it I now understand the flow of the color-moved code quite a bit better. :)

-Peff