Re: [PATCH v3 1/8] t2407: test branches currently using apply backend

From: Derrick Stolee <derrickstolee@github.com>
To: Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v3 1/8] t2407: test branches currently using apply backend
Date: Wed, 29 Jun 2022 08:54:56 -0400	[thread overview]
Message-ID: <4016eee6-0759-dece-cd7b-0ed8454faf63@github.com> (raw)
In-Reply-To: <xmqqtu84iklu.fsf@gitster.g>

On 6/28/22 4:44 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>> -test_expect_success 'refuse to overwrite: worktree in rebase' '
>> +test_expect_success 'refuse to overwrite: worktree in rebase (apply)' '
>> +	test_when_finished rm -rf .git/worktrees/wt-*/rebase-apply &&
>> +
>> +	mkdir -p .git/worktrees/wt-3/rebase-apply &&
>> +	echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-apply/head-name &&
>> +	echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-apply/onto &&
>> +
>> +	test_must_fail git branch -f fake-1 HEAD 2>err &&
>> +	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
>> +'
>> +
>> +test_expect_success 'refuse to overwrite: worktree in rebase (merge)' '
>>  	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
>>  
>>  	mkdir -p .git/worktrees/wt-3/rebase-merge &&
> 
> This is not the first offender, since the other existing one is
> doing the same, but it is a bit sad that this makes it worse to
> expose and depend on the details of the way the rebase happens to be
> currently implemented.

This is true that it is based on how  it's currently implemented. I
was confused that none of these rebase storage mechanisms are
documented anywhere (so I couldn't extend that documentation with the
new update-refs file added in this series). But perhaps that's by
design: this is intentionally an internal implementation detail.

I do wonder where that leaves third-party implementations like libgit2
to handle any changes we make in this space, or how much we care about
supporting someone rebasing in a worktree with an older Git client
while also checking out a branch in another worktree using a newer one.

> Perhaps a more kosher way to do this is to find an commit that
> surely would not allow fake-1 branch to be cleanly rebased onto and
> actually start (and cause it to stop) a rebase.

This, and the equivalent "pause in the middle of a bisect" would be
ways to hide these internal details.

While I was thinking that this provides a low-weight mechanism for
testing the implementation of branch_checked_out(), it is only testing
the implementation but not the _intention_. If 'git rebase' changed
its backend, then these tests would not start failing as we need them
to.

> I notice that the original offence was committed fairly recently, by
> d2ba271a (branch: check for bisects and rebases, 2022-06-14) that we
> can easily eject out of the 'next' branch when we rewind and rebuild
> it, if we wanted to.

I can either re-roll that series or create a new forward-fix that
includes the functionality of this test. Both are the same amount of
work for me, so let me know which you prefer.

Thanks,
-Stolee