Hi Junio,
On Thu, 4 Feb 2021, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > int is_range_diff_range(const char *arg)
> > {
> > - return !!strstr(arg, "..");
> > + char *copy = xstrdup(arg); /* setup_revisions() modifies it */
> > + const char *argv[] = { "", copy, "--", NULL };
> > + int i, positive = 0, negative = 0;
> > + struct rev_info revs;
> > +
> > + init_revisions(&revs, NULL);
> > + if (setup_revisions(3, argv, &revs, 0) == 1) {
> > + for (i = 0; i < revs.pending.nr; i++)
> > + if (revs.pending.objects[i].item->flags & UNINTERESTING)
> > + negative++;
> > + else
> > + positive++;
> > + }
> > +
> > + free(copy);
> > + object_array_clear(&revs.pending);
> > + return negative > 0 && positive > 0;
> > }
>
> One thing that worries me with this code is that I do not see
> anybody that clears UNINTERESTING bit in the flags. In-core objects
> are singletons, so if a user fed the command two ranges,
>
> git range-diff A..B C..A
>
> and this code first handled "A..B", smudging the in-core instance of
> the commit object A with UNINTERESTING bit, that in-core instance
> will be reused when the second range argument "C..A" is given to
> this function again.
>
> At that point, has anybody cleared the UNINTERESTING bit in the
> flags word for the in-core commit A? I do not see it done in this
> function, but perhaps I am missing it done in the init/setup
> functions (I somehow doubt it, though)?
>
> Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the
> commits in revs.pending[] array before we clear it?
Yes, we should ;-)
> Depending on the shape of "arg" that is end-user supplied, we may have
> walked the history in handle_dotdot_1() to parse it (e.g. "A...B").
The code in `handle_dotdot_1()` that handles `...` calls
`get_merge_bases()`, which eventually calls `get_merge_bases_many_0()`
with the `cleanup` parameter set to `1`, i.e. it _does_ clean up the
flags.
Otherwise, `git log A...B` couldn't work, could it? ;-)
> Also we'd want to see what needs to be cleared in revs.cmdline
> that would have been populated by calls to add_rev_cmdline().
Is this cleared, like, ever? I don't see any code that handles `cmdline`
that way. Seems as we're willfully leaking this.
Ciao,
Dscho
> Other than that, I quite like the way the actual code turned out to
> be.
>
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 6eb344be0312..e217cecac9ed 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
> > test_cmp expect actual
> > '
> >
> > +test_expect_success 'A^! and A^-<n> (unmodified)' '
> > + git range-diff --no-color topic^! unmodified^-1 >actual &&
> > + cat >expect <<-EOF &&
> > + 1: $(test_oid t4) = 1: $(test_oid u4) s/12/B/
> > + EOF
> > + test_cmp expect actual
> > +'
> > +
> > test_expect_success 'trivial reordering' '
> > git range-diff --no-color master topic reordered >actual &&
> > cat >expect <<-EOF &&
>
