From: phillip.wood123@gmail.com
Date: Thu, 22 Feb 2024 16:34:48 GMT
Subject: Re: [PATCH v2 4/8] sequencer: treat error reading HEAD as unborn branch
Message-ID: <08073d69-18f4-40c9-90a5-23db914c163e@gmail.com>
In-Reply-To: <20240210074859.552497-5-brianmlyles@gmail.com>

Hi Brian

On 10/02/2024 07:43, Brian Lyles wrote:
> 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.
> 
> 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.
> 
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> 
> This is another new commit that was not present in v1.
> 
> See this comment[1] from Phillip for context.
> 
> [1]: https://lore.kernel.org/git/b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com/

Thanks for fixing this and adding a test, I've left a few small comments 
below.

>   static int is_index_unchanged(struct repository *r)
>   {
> -	struct object_id head_oid, *cache_tree_oid;
> +	struct object_id head_oid, *cache_tree_oid, head_tree_oid;

I think we can make `head_tree_oid` a pointer like cache_tree_oid and 
avoid de-referencing `the_hash_algo->empty_tree` and the return value of 
`get_commit_tree_oid()`. I think the only reason to copy it would be if 
the underlying object had a shorter lifetime than `head_tree_oid` but I 
don't think that's the case.

> +test_expect_success 'cherry-pick on unborn branch with --allow-empty' '
> +	git checkout main &&

I'm a bit confused by this - are we already on the branch "unborn" and 
so need to move away from it to delete it?

> +	git branch -D unborn &&
> +	git checkout --orphan unborn &&
> +	git rm --cached -r . &&
> +	rm -rf * &&

"git switch --orphan" leaves us with an empty index and working copy 
without having to remove the files ourselves.

> +	git cherry-pick initial --allow-empty &&
> +	git diff --quiet initial &&

I'd drop "--quiet" here as it makes debugging easier if we can see the 
diff if the test fails.

> +	test_cmp_rev ! initial HEAD
> +'

Best Wishes

Phillip