Re: [PATCH v2 03/14] rebase: pass correct arguments to post-checkout hook

[Junio C Hamano <gitster@pobox.com>]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If a rebase started with "rebase [--apply|--merge] <upstream> <branch>"
> detects that <upstream> is an ancestor of <branch> then it fast-forwards
> and checks out <branch>. Unfortunately in that case it passed the null
> oid as the first argument to the post-checkout hook rather than the oid
> of HEAD.
>
> A side effect of this change is that the call to update_ref() which
> updates HEAD now always receives the old value of HEAD. This provides
> protection against another process updating HEAD during the checkout.

Nicely described.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  reset.c                       | 18 +++++++++---------
>  t/t5403-post-checkout-hook.sh | 13 +++++++++++++
>  2 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/reset.c b/reset.c
> index f214df3d96c..315fef91d33 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -18,7 +18,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>  	unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
>  	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
>  	unsigned update_orig_head = flags & RESET_ORIG_HEAD;
> -	struct object_id head_oid;
> +	struct object_id *head = NULL, head_oid;
>  	struct tree_desc desc[2] = { { NULL }, { NULL } };
>  	struct lock_file lock = LOCK_INIT;
>  	struct unpack_trees_options unpack_tree_opts = { 0 };
> @@ -26,8 +26,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>  	const char *reflog_action;
>  	struct strbuf msg = STRBUF_INIT;
>  	size_t prefix_len;
> -	struct object_id *orig = NULL, oid_orig,
> -		*old_orig = NULL, oid_old_orig;
> +	struct object_id *old_orig = NULL, oid_old_orig;
>  	int ret = 0, nr = 0;
>  
>  	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> @@ -38,7 +37,9 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>  		goto leave_reset_head;
>  	}
>  
> -	if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
> +	if (!get_oid("HEAD", &head_oid)) {
> +		head = &head_oid;
> +	} else if (!oid || !reset_hard) {
>  		ret = error(_("could not determine HEAD revision"));
>  		goto leave_reset_head;
>  	}

Here, the original grabbed head_oid only when oid is not known or we
are not doing a hard reset.  Now we grab head_oid when available,
regardless of the other conditions.  But the logic to error out is
the same.  If oid is not given or not doing hard reset, it is an
error if we cannot figure out what object HEAD points at (is that
"unborn branch"?).

OK.


> @@ -98,13 +99,12 @@ reset_head_refs:
>  	if (update_orig_head) {
>  		if (!get_oid("ORIG_HEAD", &oid_old_orig))
>  			old_orig = &oid_old_orig;
> -		if (!get_oid("HEAD", &oid_orig)) {
> -			orig = &oid_orig;
> +		if (head) {
>  			if (!reflog_orig_head) {
>  				strbuf_addstr(&msg, "updating ORIG_HEAD");
>  				reflog_orig_head = msg.buf;
>  			}
> -			update_ref(reflog_orig_head, "ORIG_HEAD", orig,
> +			update_ref(reflog_orig_head, "ORIG_HEAD", head,
>  				   old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
>  		} else if (old_orig)
>  			delete_ref(NULL, "ORIG_HEAD", old_orig, 0);

The variable 'orig' was initialized to NULL and then got assigned
the value of HEAD here, which is used in update_ref().  The new code
does the same thing with the 'head' variable that we have already
prepared earlier.  So there is no change here locally, but this
affects correctness later ...

> @@ -116,7 +116,7 @@ reset_head_refs:
>  		reflog_head = msg.buf;
>  	}
>  	if (!switch_to_branch)
> -		ret = update_ref(reflog_head, "HEAD", oid, orig,
> +		ret = update_ref(reflog_head, "HEAD", oid, head,
>  				 detach_head ? REF_NO_DEREF : 0,
>  				 UPDATE_REFS_MSG_ON_ERR);

... here.  In the original code, there is nothing that populates the
'orig' variable, if "update_orig_head" is not true, so we used to
pass NULL here.  Now as long as we know what HEAD is, we pass use it.



> @@ -128,7 +128,7 @@ reset_head_refs:
>  	}
>  	if (run_hook)
>  		run_hook_le(NULL, "post-checkout",
> -			    oid_to_hex(orig ? orig : null_oid()),
> +			    oid_to_hex(head ? head : null_oid()),

And the same fix is in effect here.

Nice.


> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 272b02687ba..17ab518f268 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -72,6 +72,19 @@ test_rebase () {
>  		test_cmp_rev rebase-on-me $new &&
>  		test $flag = 1
>  	'
> +
> +	test_expect_success "rebase $args fast-forward branch checkout runs post-checkout hook" '
> +		test_when_finished "test_might_fail git rebase --abort" &&
> +		test_when_finished "rm -f .git/post-checkout.args" &&
> +		git update-ref refs/heads/rebase-fast-forward three &&
> +		git checkout two  &&
> +		rm -f .git/post-checkout.args &&
> +		git rebase $args HEAD rebase-fast-forward  &&
> +		read old new flag <.git/post-checkout.args &&
> +		test_cmp_rev two $old &&
> +		test_cmp_rev three $new &&
> +		test $flag = 1
> +	'
>  }
>  
>  test_rebase --apply &&