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

[Junio C Hamano <gitster@pobox.com>]

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

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

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