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

From: Junio C Hamano <[email-masked]>
Date: Thu, 17 Sep 2020 11:23:54 -0700

Denton Liu <[email-masked]> 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.

Thanks.