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

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> 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:

When running the tree diff, we'd need the object anyway, and the
result of the look-up made here is cached, right?

That is why I expected it would just be an insertion before the
existing code, like the other side.

But the existing "if we got either ^A B or B ^A, treat it as A..B"
logic is just like "if we got '--merge-base A B', treat it as
something else" we are adding, and they (and any future such special
syntax) should not interact with each other.  So in that sense, the
code structure you have in the originally posted patch (not the code
snippet in your message I am responding to) that does

    ...
    if (using merge-base feature) {
	do the merge base thing to populate oid[]
    } else if (user used A..B) {
	ensure "^A B" and "B ^A" both have A in oid[0] and B in oid[1]
    }
    ...
    call diff-tree between oid[0] and oid[1]

makes a lot more sense than anything else we discussed so far.

I wonder if turning the builtin/diff-tree.c to match that structure
make the result easier to understand (and I'll be perfectly happy if
the answer to this question turns out to be "no, the result of the
posted patch is the easiest to follow").

Thanks.