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

From: Brian Lyles <brianmlyles@gmail.com>
Date: Mar 12, 2024, 02:04 UTC
Message-ID: <17bbe214bab8dc40.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
In-Reply-To: <xmqqh6hcu2tg.fsf@gitster.g>

Hi Junio,

On Mon, Mar 11, 2024 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 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.
> 
> I suspect that you are almost there.
> 
> +	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
> +		/*
> +		 * Treat an error reading HEAD as an unborn branch.
> +		 */
> 
> After we see this error, if we make a call to resolve_ref_unsafe()
> with RESOLVE_REF_NO_RECURSE, the call should return the branch that
> we are on but is not yet born, and &oid will get the null_oid.  I am
> not sure if there is a way to combine the two calls into one, but
> because the failure case (i.e. doing anything on an unborn branch)
> is a rare case that happens only once before actually giving birth
> to a new branch, it probably is not worth spending extra brain
> cycles on it and just use a simple and stupid "when resolving fails,
> see if we are in a rare salvageable case with extra code" approach.

If I'm understanding you correctly, it sounds like you're hinting at something like this:

	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
		/*
		 * Check to see if this is an unborn branch
		 */
		if (resolve_ref_unsafe("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL)
			&& is_null_oid(&head_oid)) {
			head_tree_oid = the_hash_algo->empty_tree;
		} else {
			return error(_("could not resolve HEAD commit"));
		}
	} else {
		...
	}

This does in fact seem to have the same effect as the original patch in this case. If this is in line with what you are looking for, I can include this in v4. The commit message would be adjusted slightly to be:

	sequencer: handle unborn branch with `--allow-empty`

	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.

	Detect unborn branches in `is_index_unchanged`. When on an unborn
	branch, use the `empty_tree` as the tree to compare against.

...

Do these changes look good?

-- 
Thank you,
Brian Lyles