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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 21, 2022, 21:19 UTC
Message-ID: <xmqqczkkg51h.fsf@gitster.g>
In-Reply-To: <220121.86iludl4d9.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 8 quoted lines

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

Yes, I concur.

Show 14 quoted lines

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

OK, that might have done a wrong thing (instead of dying) for a strange settings like that, but the change was never about tightening and detecting such a strangeness but only about plugging leaks, so reverting that narrow part of the patch is the right thing to do.

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

Yes, except for the update in the test. I do not think we want to promise what should happen to the _values_ of these refs after the operation at all. If it only says "checkout should not exit with non-zero status", I would be OK. Promising anything more than that, I do not think it is a good idea.

For now, I plan to do the "revert the check-and-BUG and nothing else" change.

Thanks.