Re: [PATCH v3 4/7] sequencer: treat error reading HEAD as unborn branch

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2024, 00:07 UTC
Message-ID: <xmqq4jddwrzr.fsf@gitster.g>
In-Reply-To: <20240310184602.539656-5-brianmlyles@gmail.com>

Brian Lyles <brianmlyles@gmail.com> writes:

> When using git-cherry-pick(1) with `--allow-empty` while on an unborn
> branch, an error is thrown. This is inconsistent with the same
> cherry-pick when `--allow-empty` is not specified.

When cherry-picking on top of an unborn branch without the option, how does the code figure out that we are on an unborn branch? We must be doing the detection, as we'd need to at least know the fact that we are on an unborn in order to make the resulting commit a root commit and also in order to apply the cherry-picked changes against an empty tree.

> Treat a failure reading HEAD as an unborn branch in
> `is_index_unchanged`. This is consistent with other sequencer logic such
> as `do_pick_commit`. When on an unborn branch, use the `empty_tree` as
> the tree to compare against.

It is not a good code hygiene to assume that a failure to read HEAD always means we are on an unborn branch, even if that is the most likely cause of the failure. We may instead want to positively determine that we are on an unborn state, by seeing if the HEAD is a symbolic ref that points at a ref in refs/heads/* hierarchy, and that ref does not exist.

But if the existing sequencer code is littered with such loose logic everywhere, perhaps imitating the looseness for now with a NEEDSWORK comment to later clean them all up would be the least we could do right now. If we want to be more ambitious, a few clean-up patches that refactors existing code paths that detect that we are on an unborn branch into a call to a single helper function, and then tightens its implementation, before doing this step would be even better (but I offhand do not know how bad the current code is, so I would understand it if it may turn out to be a lot more work than you would want to invest in).