Re: [RFC WIP PATCH] merge: implement -s theirs -X N

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 19, 2018, 04:29 UTC
Message-ID: <xmqqy3hjes3n.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <87sh7sdtc1.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Questions:
>
>  1. Should I be calling read-tree here with run_command_v_opt(), or is
>     there some internal API I should be using?

The internal is unpack_trees(), which is usabe as a library-ish API reasonably cleanly and easily. For a project large enough where the perforce can become an issue, the overhead to spawn read-tree as an external process would be dwarfed by the cost of real processing of merging multiple trees into an in-core index, but it involves two extra writing and reading the index file back and forth compared to the approach to use unpack_trees() to do everything in core. As long as the "now make sure that the contents of the index file is that of the tree of the N-th parent" code is cleanly isolated in the implementation, it is probably better to refrain from premature optimization.

Show 5 quoted lines

>  2. Currently merge-ours is just a no-op since we take the current HEAD,
>     but maybe it would be cleaner to implement it in terms of this
>     thing, also to get immediate test coverage for all the -s ours
>     stuff. We'd be reading the tree redundantly into the index, but
>     maybe it's worth it for the coverage...

I doubt that it would be a sensible approach. If anything, even if we lived in an alternate universe where "-s ours" weren't invented and "-s become-nth-tree -W $N" feature gets first introduced, I would imagine that we would introduce a code to special case "-s ours" (aka "take the current HEAD") as an obvious optimization for the common and trivial case, essentially splitting the "unification" you are suggesting here.

>  3. Is there a better name for this than -s theirs? Maybe `-s nth -X N`?

I tend to agree that "-s thiers -X N" is horrible; "-s ours -X N" would slightly be a better choice as it does not have to introduce a new option. "-s nth -X N" does not sound all that bad.

"Does this feature make sense?" was not among the questions listed, and I am not ready to answer to it yet anyway, so ...