On Thu, Jan 24, 2019 at 12:18 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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.
I do not recall if we discussed this or if I just had talking voices in my
head, but this was done deliberately to separate the implementation
of the buffering feature and its user. I thought once we had the buffering
in place, we might add options that also require buffering;
hence the explicit "move coloring implies using a buffer".
Having the buffer as a local static might be an iffy implementation
that should be fixed for clarify.
>
> - 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.
Ah I see where you are coming from, as the code was written
I imagined:
if (o->color_moved)
o->emitted_symbols = &esm;
if (o->distim_gostaks)
o->emitted_symbols = &esm;
if (o->emitted_symbols) {
if (o->color_moved)
handle_color_moved(o);
if (o->distim_gostaks)
handle_distimming(o);
... flush symbols...
... free &cleanup ...
}
> - 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.
I see. So either we'd want to have
if (o->emitted_symbols)
BUG("should not be already set");
or as Peff points out, make it non-static.
> The above observation does not necessarily indicate any bug; it just
> shows that the code structure is messier than necessary.
Makes sense. I should not have anticipated any new feature to be
added, yet.
>
> > 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
Yes, this was done as to make the buffering complete.
We would not have to buffer any word diff as the color move doesn't yet
work on that.
See the NEEDSWORK in diff_words_flush, where we could implement
a word based move coloring.
