On Wed, Oct 11, 2023 at 12:38:03PM -0700, Junio C Hamano wrote:
> 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
