On Thu, Sep 17, 2020 at 11:23:54AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> writes:
>
> > + if (read_stdin && merge_base)
> > + die(_("--stdin and --merge-base are mutually exclusive"));
> > +
> > + if (merge_base) {
> > + struct object_id oid;
> > +
> > + if (opt->pending.nr != 2)
> > + die(_("--merge-base only works with two commits"));
> > +
> > + diff_get_merge_base(opt, &oid);
> > + opt->pending.objects[0].item = lookup_object(the_repository, &oid);
> > + }
> > +
>
> This looks quite straight-forward.
>
> > - /*
> > - * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> > - * swap them.
> > - */
> > - if (ent1->item->flags & UNINTERESTING)
> > - swap = 1;
> > - oid[swap] = &ent0->item->oid;
> > - oid[1 - swap] = &ent1->item->oid;
> > + if (merge_base) {
> > + diff_get_merge_base(revs, &mb_oid);
> > + oid[0] = &mb_oid;
> > + oid[1] = &revs->pending.objects[1].item->oid;
> > + } else {
> > + int swap = 0;
> > +
> > + /*
> > + * We saw two trees, ent0 and ent1. If ent1 is uninteresting,
> > + * swap them.
> > + */
> > + if (ent1->item->flags & UNINTERESTING)
> > + swap = 1;
> > + oid[swap] = &ent0->item->oid;
> > + oid[1 - swap] = &ent1->item->oid;
> > + }
>
> It is not entirely clear why the original has to become an [else]
> clause here, unlike the change we saw earlier in cmd_diff_tree().
> It feels quite inconsistent.
Since we're only interested in the oids, I thought that it would be
possible to save a lookup_object() and just use the oids directly. If
it's clearer, this can be written as something like this but the lookup
feels unnecessary:
/*
* We saw two trees, ent0 and ent1. If ent1 is uninteresting,
* swap them.
*/
if (ent1->item->flags & UNINTERESTING)
swap = 1;
if (merge_base) {
struct object_id mb_oid;
if (swap)
BUG("swap is unexpectedly set");
if (diff_get_merge_base(revs, &mb_oid))
exit(128);
ent0->item = lookup_object(the_repository, &mb_oid);
}
oid[swap] = &ent0->item->oid;
oid[1 - swap] = &ent1->item->oid;
Thanks,
Denton
