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

[Denton Liu <liu.denton@gmail.com>]

Hi Junio,

On Mon, Sep 21, 2020 at 02:09:24PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> >  	case 2:
> >  		tree1 = opt->pending.objects[0].item;
> >  		tree2 = opt->pending.objects[1].item;
> > -		if (tree2->flags & UNINTERESTING) {
> > +		if (merge_base) {
> > +			struct object_id oid;
> > +
> > +			diff_get_merge_base(opt, &oid);
> > +			tree1 = lookup_object(the_repository, &oid);
> > +		} else if (tree2->flags & UNINTERESTING) {
> >  			SWAP(tree2, tree1);
> >  		}
> >  		diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
> 
> OK.  Handling this in that "case 2" does make sense.
> 
> However.
> 
> The above code as-is will allow something like
> 
>     git diff --merge-base A..B
> 
> and it will be taken the same as
> 
>     git diff --merge-base A B

This does not happen because at the top of diff_get_merge_base(), we
have

	for (i = 0; i < revs->pending.nr; i++) {
		struct object *obj = revs->pending.objects[i].item;
		if (obj->flags)
			die(_("--merge-base does not work with ranges"));
		if (obj->type != OBJ_COMMIT)
			die(_("--merge-base only works with commits"));
	}

which ensures that we don't accept any ranges at all. This is why I
considered the SWAP and merge_base cases to be mutually exclusive.

> Another possibility is to error out when "--merge-base A..B" is
> given, which might be simpler.  Then the code would look more like
> 
> 
> 	tree1 = ...
> 	tree2 = ...
> 
> 	if (merge_base) {
> 		if ((tree1->flags | tree2->flags) & UNINTERESTING)
> 			die(_("use of --merge-base with A..B forbidden"));
> 		... get merge base and assign it to tree1 ...
> 	} else if (tree2->flags & UNINTERESTING) {
> 		SWAP();
> 	}

This is the route I picked, although the logic for this is in
diff_get_merge_base().

> While we are at it, what happens when "--merge-base A...B" is given?
> 
> In the original code without "--merge-base", "git diff-tree A...B"
> places the merge base between A and B in pending.objects[0] and B in
> pending.objects[1], I think.  "git diff-tree --merge-base A...B"
> would further compute the merge base between these two objects, but
> luckily $(git merge-base $(merge-base A B) B) is the same as $(git
> merge-base A B), so you won't get an incorrect answer from such a
> request.  Is this something we want to diagnose as an error?  I am
> inclined to say we should allow it (and if it hurts the user can
> stop doing so) as there is no harm done.

I think that we should error out for all ranges because this option
semantically only really makes sense on two endpoints, not a range of
commits. Since the check is cheap to protect users from themselves, we
might as well actually do it.

Worst case, if someone has a legimitate use case for --merge-base and
ranges, we can allow it in the future, which would be easier than
removing this feature.

Thanks,
Denton