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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2020, 16:52 UTC
Message-ID: <xmqqy2l7m3hk.fsf@gitster.c.googlers.com>
In-Reply-To: <20200918104833.GB1874074@generichostname>

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.