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

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

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.

quoted

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.