Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 4, 2021, 21:42 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2102042027570.54@tvgsbejvaqbjf.bet>
- In-Reply-To
- <xmqq7dnn7jjz.fsf@gitster.c.googlers.com>
Hi Junio,
On Thu, 4 Feb 2021, Junio C Hamano wrote:
Show 43 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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
Show 23 quoted lines
> 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 && >