Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base

From: Denton Liu <liu.denton@gmail.com>
Date: Sep 18, 2020, 10:48 UTC
Message-ID: <20200918104833.GB1874074@generichostname>
In-Reply-To: <xmqq363gs1mt.fsf@gitster.c.googlers.com>

On Thu, Sep 17, 2020 at 11:23:54AM -0700, Junio C Hamano wrote:

Show 46 quoted lines

> Denton Liu <liu.denton@gmail.com> 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