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

From: Derrick Stolee <derrickstolee@github.com>
Date: May 11, 2023, 15:00 UTC
Message-ID: <2ce5dd62-3b76-c14f-35e2-5cb2ccda42a1@github.com>
In-Reply-To: <CABPp-BECZgACeEUqG3pajJpHAaY=-orNwwOUEX5qqzAKVRMFdQ@mail.gmail.com>

On 5/11/2023 2:34 AM, Elijah Newren wrote:

Show 15 quoted lines

> On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> 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.
>>
>> In particular, one config that is loaded by default is
>> core.useReplaceRefs. This is typically disabled in Git hosts due to
>> the ability to spoof commits in strange ways.

Show 23 quoted lines

>> +       git_config(git_default_config, NULL);
>> +
> 
> Always nice when it's a simple fix.  :-)
> 
> I am curious though...
> 
> init_merge_options() in merge-recursive.c (which is also used by
> merge-ort) calls merge_recursive_config().  merge_recursive_config()
> does a bunch of config parsing, regardless of whatever config parsing
> is done beforehand by the caller of init_merge_options().  This makes
> me wonder if the config which handles replace refs should be included
> in merge_recursive_config() as well.  Doing so would have the added
> benefit of making sure all the builtins calling the merge logic behave
> similarly.  And if we copy/move the replace-refs-handling config
> logic, does that replace the fix in this patch, or just supplement it?
> 
> To be honest, I've mostly ignored the config side of things while
> working on the merge machinery, so I didn't even know (or at least
> remember) the above details until I went digging just now.  I don't
> know if the way init_merge_options()/merge_recursive_config() is how
> we should do things, or just vestiges of how it's evolved from 15
> years ago.

...

> Looks good.  I am curious for other's thoughts on whether it may make
> sense to add parsing of core.useReplaceRefs within
> merge_recursive_config().

In terms of a "real" fix to this kind of problem, I'm thinking that we actually need to be sure we've parsed things like core.useReplaceRefs when loading the object database for the first time.

Here, I'm suggesting the simplest fix before we can go about a more rigorous change to prevent this from happening again.

The custom ahead-behind builtin that we have in our fork once also had this same problem, and the fix was exactly like this. The impact was less severe (mostly, things slowed down because the commit-graph was disabled, but also the numbers could be different from expected).

Thanks, -Stolee