Re: [PATCH] merge-tree: load default git config

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 11, 2023, 22:07 UTC
Message-ID: <645d672447ebb_13d3fe294f@chronos.notmuch>
In-Reply-To: <015dcb79-9630-e188-65cf-23b005184db1@github.com>

Derrick Stolee wrote:

Show 22 quoted lines

> On 5/10/2023 4:30 PM, Felipe Contreras wrote:
> > Derrick Stolee via GitGitGadget wrote:
> >> From: Derrick Stolee <derrickstolee@github.com>
> >>
> >> The 'git merge-tree' command handles creating root trees for merges
> >> without using the worktree. This is a critical operation in many Git
> >> hosts, as they typically store bare repositories.
> >>
> >> This builtin does not load the default Git config, which can have
> >> several important ramifications.
> > 
> > For the record, I had already sent a better version of this patch almost 2
> > years ago [1], not just for `git merge-tree`, but other commands as well.
> > 
> > The obvious fix was completely ignored by the maintainer.
> > 
> > The reason why it should be git_xmerge_config and not git_default_config, is
> > that merge.conflictstyle would not be parsed if you call git_default_config.
> 
> As mentioned by Elijah in a different thread, the merge machinery loads
> the merge config as needed. I confirmed by creating this test, which I
> may submit as an independent patch:

I wrote my patches before Elijah wrote the real merge implementation, and in his function he does `init_merge_options()`, which eventually calls `git_config(git_xmerge_config, NULL)`.

But if `git_config()` is already called, you shouldn't need to add yet another `git_config()` call.

The problem is that he added `init_merge_options()` *after* the `get_merge_parent()` calls, that's why the configuration is ignored.

If we move `init_merge_options()` to the right place, the problem is fixed:

--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -424,6 +424,8 @@ static int real_merge(struct merge_tree_options *o,
 	struct merge_result result = { 0 };
 	int show_messages = o->show_messages;
 
+	init_merge_options(&opt, the_repository);
+
 	parent1 = get_merge_parent(branch1);
 	if (!parent1)
 		help_unknown_ref(branch1, "merge-tree",
@@ -434,8 +436,6 @@ static int real_merge(struct merge_tree_options *o,
 		help_unknown_ref(branch2, "merge-tree",
 				 _("not something we can merge"));
 
-	init_merge_options(&opt, the_repository);
-
 	opt.show_rename_progress = 0;
 
 	opt.branch1 = branch1;

I ran your test case, and it passes.

I sent a patch for that here:

https://lore.kernel.org/git/20230511215608.1297686-1-felipe.contreras@gmail.com/

This is a more proper fix because a) it doesn't add any new line of code, b) it
doesn't add a new include, and c) it doesn't call `git_config()` twice.

Cheers.

-- 
Felipe Contreras