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

>
> 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.