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

On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

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.

quoted

+       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