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