From: Brian Lyles <brianmlyles@gmail.com>
Date: Fri, 23 Feb 2024 05:28:48 GMT
Subject: Re: [PATCH v2 4/8] sequencer: treat error reading HEAD as unborn branch
Message-ID: <17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
In-Reply-To: <08073d69-18f4-40c9-90a5-23db914c163e@gmail.com>

On Thu, Feb 22, 2024 at 10:34 AM <phillip.wood123@gmail.com> wrote:

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

Makes sense -- I'll update this in v3.

>> +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?

Yes, the previous test leaves us on that branch. In v3, I will update
this to instead just use `git checkout --detach`, as that does seem a
little less confusing than switching to some other branch that is only
relevant because it's not `unborn`. If there is a cleaner way to do
this, I'd be happy to switch to 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.

Thanks for pointing this out. Using git-switch(1) here instead of
git-checkout(1) allows us to drop the `rm -rf *` call form both the
existing 'cherry-pick on unborn branch' test as well as my new test. It
appears that the `git rm --cached -r .` call is still necessary in the
existing test.

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

This makes sense. In v3, I will update this new test as well as the
existing test to not use `--quiet`.

Combining the above suggestions, here's the version of the existing and
new tests that I intend to use in v3. Let me know if this isn't what you
had in mind!

    test_expect_success 'cherry-pick on unborn branch' '
    	git switch --orphan unborn &&
    	git rm --cached -r . &&
    	git cherry-pick initial &&
    	git diff initial &&
    	test_cmp_rev ! initial HEAD
    '
    
    test_expect_success 'cherry-pick on unborn branch with --allow-empty' '
    	git checkout --detach &&
    	git branch -D unborn &&
    	git switch --orphan unborn &&
    	git cherry-pick initial --allow-empty &&
    	git diff initial &&
    	test_cmp_rev ! initial HEAD
    '

-- 
Thank you,
Brian Lyles