Re: [PATCH 03/11] reset_head(): don't run checkout hook if there is an error

From

Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Date

Oct 12, 2021, 08:48 UTC

Message-ID

<877deilixj.fsf@evledraar.gmail.com>

In-Reply-To

<b298f8cb-f55f-86f0-52fc-8d9db0047525@gmail.com>

On Mon, Oct 04 2021, Phillip Wood wrote:

> Hi Junio
>
> On 01/10/2021 21:52, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> The hook should only be run if the worktree and refs were successfully
>>> updated.
>> OK.  This is a behaviour change visible to end-users, and deserves a
>> mention in the release notes.
>>   - When "git rebase" attempted to check out a branch (or detached
>
> It only affects "git rebase --apply" as the "merge" backend forks "git
> checkout" which does not run the hook if it cannot update the worktree 
> or refs.
>
>>     the HEAD) to work on, we used to always call the "post-checkout"
>>     hook, even if the checkout failed to update the ref.  The hook is
>>     no longer called if the checkout fails.
>> or something.
>> Again, can the bug this step fixes be protected with a new test in
>> t/ please?
>
> I'll try and come up with something - it should be possible to arrange
> an untracked file to make unpack_trees() to fail, I'm not sure how
> we'd make update_ref() fail

    git_env_bool("GIT_TEST_FAIL_UPDATE_REF", 1) or
    git_env_bool("GIT_TEST_FAIL_UPDATE_REF_FOR_POST_CHECKOUT", 1)

*tests it out a bit*

I tried this and it "worked", as in it'll fail :)

diff --git a/refs.c b/refs.c
index 97a9501c06f..25e2b0ab49a 100644
--- a/refs.c
+++ b/refs.c
@@ -2016,6 +2016,9 @@ int refs_create_symref(struct ref_store *refs,
        char *msg;
        int retval;
 
+       if (git_env_bool("GIT_TEST_FAIL_REFS_CREATE_SYMREF", 0))
+               return -1;
+
        msg = normalize_reflog_message(logmsg);
        retval = refs->be->create_symref(refs, ref_target, refs_heads_master,
                                         msg);

That seems to me to also be a worthwhile thing to do/test for other
reasons, i.e. my HEAD won't update, but now my index is in some state
where it points to the new branch, but presumably we'd want to revert
that in case of the failure (probably a much more general issue than
you're poking at here...).

>>>
>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> ---
>>>   reset.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/reset.c b/reset.c
>>> index fc4dae3fd2d..5abb1a5683e 100644
>>> --- a/reset.c
>>> +++ b/reset.c
>>> @@ -125,7 +125,7 @@ reset_head_refs:
>>>   			ret = create_symref("HEAD", switch_to_branch,
>>>   					    reflog_head);
>>>   	}
>>> -	if (run_hook)
>>> +	if (!ret && run_hook)
>>>   		run_hook_le(NULL, "post-checkout",
>>>   			    oid_to_hex(orig ? orig : null_oid()),
>>>   			    oid_to_hex(oid), "1", NULL);