Re: [PATCH v6] merge-tree: add -X strategy option

From: Jeff King <peff@peff.net>
Date: Oct 11, 2023, 21:43 UTC
Message-ID: <20231011214340.GA518221@coredump.intra.peff.net>
In-Reply-To: <xmqqlec9kl0k.fsf@gitster.g>

On Wed, Oct 11, 2023 at 12:38:03PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> If we have no plan and intention to extend "merge-tree" even more in
> the future, then option 1 would be the approach with least patch
> noise, and as your "something like this" shows, it is a nice and
> clean solution.  I very much like it.
> 
> But as the renovated "merge-tree" is a relatively young thing in our
> toolbox, I suspect that more and more work may want to go into it.
> And the other "official copy_merge_options()" approach would be a
> more healthy solution in the longer run, I would think.  If we were
> to go that route, we should also give an interface to free the
> resources held by the copy.

I am happy with either, as they both resolve the "merge-tree knows intimate details about merge_options" issue. The patch I showed would require manually passing more details down to real_merge(), which is I guess what you are getting at with the "more work may want to go into it".

> It is not that much code on top of the commit that is already queued
> in 'next', I suspect.  Perhaps something like this?

This looks OK, though...

> +void clear_merge_options(struct merge_options *opt UNUSED)
> +{
> +	; /* no-op as our copy is shallow right now */
> +}

Clearing is generally not just about copies, but any use of the struct. so this invites the question of whether the original non-copy struct should have a call to clear_merge_options() in cmd_merge_tree(). And ditto for every other user.

I do not mind adding such calls, but of course we know that they are currently noops (and we don't have any particular plan to change that).

-Peff