From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 10 Mar 2026 20:01:53 GMT
Subject: Re: [PATCH] merge-file: fix BUG when --object-id is used in a worktree
Message-ID: <xmqqh5qntpvy.fsf@gitster.g>
In-Reply-To: <abATPiRUczb8fe4t@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Mar 10, 2026 at 11:46:01AM +0000, Mathias Rav wrote:
>
> Which commit is this patch based on? It doesn't apply in its current
> form on top of "master" since at least 8600b4ec9e (merge-file: honor
> merge.conflictStyle outside of a repository, 2026-02-07). Please rebase
> the patch.

This applies cleanly relative to v2.53.0.  Generally, it is a
recommended practice to fork from the latest stable release in many
projects, so I do not mind it too much.

>> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
>> index 46775d0c79..a8768c6e0c 100644
>> --- a/builtin/merge-file.c
>> +++ b/builtin/merge-file.c
>> @@ -110,7 +110,7 @@ int cmd_merge_file(int argc,
>>  			return error_errno("failed to redirect stderr to /dev/null");
>>  	}
>>  
>> -	if (object_id)
>> +	if (object_id && !repo)
>>  		setup_git_directory();

Good.  I suspect we would want to check !repo first, though.  The
idea here is

 (1) We know we need to support running "git merge-file" outside a
     repository, so the git potty may have called us with !repo (aka
     RUN_SETUP_GENTLY);

 (2) But we do need to make sure we have a repository in some case.
     We happen to have only one case, i.e., when "--object-id"
     option is in use, but that condition may grow in the future.

So in the longer run, we'd better prepared for the "has the user
gave us the object_id option?" part to grow over time, which would
mean

	if (!repo &&
	    (object_id || another_option || yet_another ...))
		setup_git_directory();

would be easier to handle.

>>  	for (i = 0; i < 3; i++) {
>
> Okay, makes sense. Makes me wonder whether we have other cases of the
> same error class.

While I agree that the second call to setup_git_directory() that
triggered this patch is pointless, I also wish that the function
were more robust.  I wonder if there is a clean way to make it
idempotent.

>> diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
>> index 06ab4d7aed..60cc43775f 100755
>> --- a/t/t6403-merge-file.sh
>> +++ b/t/t6403-merge-file.sh
>> @@ -506,6 +506,15 @@ test_expect_success '--object-id fails without repository' '
>>  	grep "not a git repository" err
>>  '
>>  
>> +test_expect_success 'run inside worktree with --object-id' '
>> +	empty="$(test_oid empty_blob)" &&
>> +	git worktree add work &&
>> +	(cd work && git merge-file --object-id $empty $empty $empty) >actual &&
>
> This can be written without a subshell by saying `git -C work
> merge-file`.

Yup, that would make the test even better.