Re: [PATCH] checkout: fix BUG() case in 9081a421a6

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jan 20 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Fix a regression in my 9081a421a6d (checkout: fix "branch info" memory
>> leaks, 2021-11-16) where I'd assumed that the old_branch_info.path
>> would have to start with refs/heads/*, but as has been reported[1]
>> that's not the case.
>>
>> As a test case[2] to reproduce this shows the second "git checkout"
>> here runs into the BUG() in the pre-image. The test being added is
>> amended from[2] and will pass both with this change, and before
>> 9081a421a6. I.e. our behavior now is again the same as before that
>> commit.
>
>> +test_expect_success REFFILES 'checkout a branch without refs/heads/* prefix' '
>> +	git clone --no-tags . repo-odd-prefix &&
>> +	(
>> +		cd repo-odd-prefix &&
>> +
>> +		cp .git/refs/remotes/origin/HEAD .git/refs/heads/a-branch &&
>
> I am not sure if this is a sensible test case to begin with.
>
> It sets up recursive symbolic ref in this way:
>
> 	HEAD points at refs/heads/a-branch
> 	refs/heads/a-branch points at refs/remotes/origin/HEAD
> 	refs/remotes/origin/HEAD points at refs/remotes/origin/branch1
>
> The checked out branch (i.e. what HEAD points at) is nominally a
> local branch, but it totally violates the spirit of the safety valve
> that says "HEAD" MUST point at a local branch or otherwise it is
> detached.  Creating a commit while "a-branch" is checked out would
> not affect *ANY* local branch state and instead makes an update to
> the remote tracking branch that does not reflect *any* past states
> at the remote repository.  Even worse, a "git fetch" that updates
> the remote tracking branches will make the HEAD, the index and the
> working tree into an inconsistent state.
>
> Simply put, I think the BUG() is catching a case where we should
> have been diagnosing as a broken repository.

Yes, I agree that we should be spotting this . Todd/Petr: If you're able
to describe it I'm curious about the original case you encountered that
the test case is derived from.

> So from my point of view, BUG() is indeed inappropriate because what
> the conditional statement noticed was a broken repository, and not a
> programming bug.
>
> What we should never do is to promise this "only kosher in letter
> but not in spirit" violation of "HEAD must point at a local branch"
> rule will be supported.
>
> So, unless I hear more convincing arguments (and Todd's example or
> anything similar that makes "git commit" from that state update a
> ref outside local branches is *not*), I am hesitant to call the new
> behaviour and 9081a421a6d a regression.

Well, the user is doing odd things with git, but we should reserve BUG()
for things that aren't rechable. Any time a user is able to arrange our
tooling in such a way as to call BUG() is a ... bug.

> What did the code before that BUG() do when faced with this nonsense
> configuration?  If forbidding outright broke a sensible workflow
> that happened to have been "working", I am OK to demote it to
> warning() and restore the previous behaviour temporarily, whatever
> it was (I think it was just old_branch_info.name was left unset
> because we were not on local branch, but I don't know if the missing
> .name was making any irrecoverable damage).  But the longer term
> direction should be that we treat the "update HEAD ends up updating
> some ref outside refs/heads/" a longstanding bug that needs to be
> fixed.

The behavior with my patch here is exactly the same as before. I.e. it
was rather straightforward, the xstrdup() is new, but before we'd just
take the un-skipped string that didn't start with refs/heads/ as-is.

I agree that it's better to look at this more deeply, but given the rc2
being out, and this surely being something we want in the final I'd
think we'd want to keep this patch as-is.

I.e. this is all a bit odd, but it was odd in exactly this way in v2.34.0 too.

We can add a warning() or die(), but a change that does that & convinces
you it's the right thing thing will require more careful consideration,
testing etc.

So I really think we should narrow our focus and keep this as-is.