diff mbox series

[2/2] Rebase: Run post-checkout hook on checkout

Message ID 20181220214823.21378-2-orgads@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] t5403: Refactor | expand

Commit Message

Orgad Shaneh Dec. 20, 2018, 9:48 p.m. UTC 
From: Orgad Shaneh <orgads@gmail.com>

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
 builtin/rebase.c              |  8 +++++++-
 t/t5403-post-checkout-hook.sh | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Comments

Johannes Schindelin Dec. 21, 2018, 4:12 p.m. UTC | #1 
Hi Orgad,

On Thu, 20 Dec 2018, orgads@gmail.com wrote:

> From: Orgad Shaneh <orgads@gmail.com>
> 
> Signed-off-by: Orgad Shaneh <orgads@gmail.com>

Feel free to steal the PR description I added to your PR at
https://github.com/git-for-windows/git/pull/1992

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b5c99ec10c..78a09dcda2 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)
>  
>  #define RESET_HEAD_DETACH (1<<0)
>  #define RESET_HEAD_HARD (1<<1)
> +#define RESET_HEAD_RUN_HOOK (1<<2)
>  
>  static int reset_head(struct object_id *oid, const char *action,
>  		      const char *switch_to_branch, unsigned flags,
> @@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,
>  {
>  	unsigned detach_head = flags & RESET_HEAD_DETACH;
>  	unsigned reset_hard = flags & RESET_HEAD_HARD;
> +	unsigned run_hook = flags & RESET_HEAD_RUN_HOOK;
>  	struct object_id head_oid;
>  	struct tree_desc desc[2] = { { NULL }, { NULL } };
>  	struct lock_file lock = LOCK_INIT;
> @@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,
>  			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
>  					 UPDATE_REFS_MSG_ON_ERR);
>  	}
> +	if (run_hook)
> +		run_hook_le(NULL, "post-checkout",
> +			    oid_to_hex(orig ? orig : &null_oid),
> +			    oid_to_hex(oid), "1", NULL);

IIRC there was one `git checkout` in the scripted version that ran the
hook, and one did not. The latter did not run it because it did not
actually change the HEAD, but just switched branches.

We could imitate that here by extending the `if (run_hook)` to `if
(run_hook && !oideq(&head_oid, oid))`, I think. Do you agree?

The rest of the patch makes sense to me (and I merged the above-mentioned
PR already).

Thank you!
Johannes

>  
>  leave_reset_head:
>  	strbuf_release(&msg);
> @@ -1539,7 +1545,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	strbuf_addf(&msg, "%s: checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>  	if (reset_head(&options.onto->object.oid, "checkout", NULL,
> -		       RESET_HEAD_DETACH, NULL, msg.buf))
> +		       RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))
>  		die(_("Could not detach HEAD"));
>  	strbuf_release(&msg);
>  
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 7e941537f9..de9c7fb871 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -9,6 +9,8 @@ test_description='Test the post-checkout hook.'
>  test_expect_success setup '
>  	test_commit one &&
>      test_commit two &&
> +    test_commit rebase-on-me &&
> +    git reset --hard HEAD^ &&
>      test_commit three three &&
>      mv .git/hooks-disabled .git/hooks
>  '
> @@ -52,6 +54,22 @@ test_expect_success 'post-checkout receives the right args when not switching br
>  	test $old = $new && test $flag = 0
>  '
>  
> +test_expect_success 'post-checkout is triggered on rebase' '
> +	git checkout -b rebase-test master &&
> +	rm -f .git/post-checkout.args &&
> +	git rebase rebase-on-me &&
> +	read old new flag < .git/post-checkout.args &&
> +	test $old != $new && test $flag = 1
> +'
> +
> +test_expect_success 'post-checkout is triggered on rebase with fast-forward' '
> +	git checkout -b ff-rebase-test rebase-on-me^ &&
> +	rm -f .git/post-checkout.args &&
> +	git rebase rebase-on-me &&
> +	read old new flag < .git/post-checkout.args &&
> +	test $old != $new && test $flag = 1
> +'
> +
>  if test "$(git config --bool core.filemode)" = true; then
>  mkdir -p templates/hooks
>  cat >templates/hooks/post-checkout <<'EOF'
> -- 
> 2.20.1
> 
>
Orgad Shaneh Dec. 24, 2018, 9:24 p.m. UTC | #2 
Hi Johannes,

On Fri, Dec 21, 2018 at 6:12 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Orgad,
>
> On Thu, 20 Dec 2018, orgads@gmail.com wrote:
>
> > From: Orgad Shaneh <orgads@gmail.com>
> >
> > Signed-off-by: Orgad Shaneh <orgads@gmail.com>
>
> Feel free to steal the PR description I added to your PR at
> https://github.com/git-for-windows/git/pull/1992
>
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index b5c99ec10c..78a09dcda2 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)
> >
> >  #define RESET_HEAD_DETACH (1<<0)
> >  #define RESET_HEAD_HARD (1<<1)
> > +#define RESET_HEAD_RUN_HOOK (1<<2)
> >
> >  static int reset_head(struct object_id *oid, const char *action,
> >                     const char *switch_to_branch, unsigned flags,
> > @@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,
> >  {
> >       unsigned detach_head = flags & RESET_HEAD_DETACH;
> >       unsigned reset_hard = flags & RESET_HEAD_HARD;
> > +     unsigned run_hook = flags & RESET_HEAD_RUN_HOOK;
> >       struct object_id head_oid;
> >       struct tree_desc desc[2] = { { NULL }, { NULL } };
> >       struct lock_file lock = LOCK_INIT;
> > @@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,
> >                       ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
> >                                        UPDATE_REFS_MSG_ON_ERR);
> >       }
> > +     if (run_hook)
> > +             run_hook_le(NULL, "post-checkout",
> > +                         oid_to_hex(orig ? orig : &null_oid),
> > +                         oid_to_hex(oid), "1", NULL);
>
> IIRC there was one `git checkout` in the scripted version that ran the
> hook, and one did not. The latter did not run it because it did not
> actually change the HEAD, but just switched branches.
>
> We could imitate that here by extending the `if (run_hook)` to `if
> (run_hook && !oideq(&head_oid, oid))`, I think. Do you agree?

Not exactly. That's what I thought, but it looks like I was wrong.
Returning to the branch is done in sequencer.c. The 2 calls to reset_head
happen on fast-forward case.

On fast-forward there is a call to reset_head with "checkout",
followed by a call
with "Fast-forwarded". I'm not sure what exactly it does, but on my test it sent
a null oid.

I did however miss the `switch-to` case, which I now added too.

- Orgad
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b5c99ec10c..78a09dcda2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -530,6 +530,7 @@  static int run_specific_rebase(struct rebase_options *opts)
 
 #define RESET_HEAD_DETACH (1<<0)
 #define RESET_HEAD_HARD (1<<1)
+#define RESET_HEAD_RUN_HOOK (1<<2)
 
 static int reset_head(struct object_id *oid, const char *action,
 		      const char *switch_to_branch, unsigned flags,
@@ -537,6 +538,7 @@  static int reset_head(struct object_id *oid, const char *action,
 {
 	unsigned detach_head = flags & RESET_HEAD_DETACH;
 	unsigned reset_hard = flags & RESET_HEAD_HARD;
+	unsigned run_hook = flags & RESET_HEAD_RUN_HOOK;
 	struct object_id head_oid;
 	struct tree_desc desc[2] = { { NULL }, { NULL } };
 	struct lock_file lock = LOCK_INIT;
@@ -636,6 +638,10 @@  static int reset_head(struct object_id *oid, const char *action,
 			ret = update_ref(reflog_head, "HEAD", oid, NULL, 0,
 					 UPDATE_REFS_MSG_ON_ERR);
 	}
+	if (run_hook)
+		run_hook_le(NULL, "post-checkout",
+			    oid_to_hex(orig ? orig : &null_oid),
+			    oid_to_hex(oid), "1", NULL);
 
 leave_reset_head:
 	strbuf_release(&msg);
@@ -1539,7 +1545,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	if (reset_head(&options.onto->object.oid, "checkout", NULL,
-		       RESET_HEAD_DETACH, NULL, msg.buf))
+		       RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))
 		die(_("Could not detach HEAD"));
 	strbuf_release(&msg);
 
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 7e941537f9..de9c7fb871 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -9,6 +9,8 @@  test_description='Test the post-checkout hook.'
 test_expect_success setup '
 	test_commit one &&
     test_commit two &&
+    test_commit rebase-on-me &&
+    git reset --hard HEAD^ &&
     test_commit three three &&
     mv .git/hooks-disabled .git/hooks
 '
@@ -52,6 +54,22 @@  test_expect_success 'post-checkout receives the right args when not switching br
 	test $old = $new && test $flag = 0
 '
 
+test_expect_success 'post-checkout is triggered on rebase' '
+	git checkout -b rebase-test master &&
+	rm -f .git/post-checkout.args &&
+	git rebase rebase-on-me &&
+	read old new flag < .git/post-checkout.args &&
+	test $old != $new && test $flag = 1
+'
+
+test_expect_success 'post-checkout is triggered on rebase with fast-forward' '
+	git checkout -b ff-rebase-test rebase-on-me^ &&
+	rm -f .git/post-checkout.args &&
+	git rebase rebase-on-me &&
+	read old new flag < .git/post-checkout.args &&
+	test $old != $new && test $flag = 1
+'
+
 if test "$(git config --bool core.filemode)" = true; then
 mkdir -p templates/hooks
 cat >templates/hooks/post-checkout <<'EOF'