t5403: Refactor

PATCH In review [email protected] 2 participants 6 messages Dec 20, 2018 – Dec 24, 2018 1 message not in the archive

lore

Share & tools

lore

[email protected]

author

2018-12-20 21:48 UTC #0 lore raw

PATCH 1/2 t5403: Refactor

From: Orgad Shaneh <[email protected]>

* Replace multiple clones and commits by test_commits.
* Replace 3 invocations of awk by read.

Signed-off-by Orgad Shaneh <[email protected]>

---
 t/t5403-post-checkout-hook.sh | 55 ++++++++++++-----------------------
 1 file changed, 18 insertions(+), 37 deletions(-)

t/t5403-post-checkout-hook.sh +18 ~~-37~~

index fc898c9eac..7e941537f9 100755
@@ -7,67 +7,48 @@ test_description='Test the post-checkout hook.'
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo Data for commit0. >a &&
-	echo Data for commit0. >b &&
-	git update-index --add a &&
-	git update-index --add b &&
-	tree0=$(git write-tree) &&
-	commit0=$(echo setup | git commit-tree $tree0) &&
-	git update-ref refs/heads/master $commit0 &&
-	git clone ./. clone1 &&
-	git clone ./. clone2 &&
-	GIT_DIR=clone2/.git git branch new2 &&
-	echo Data for commit1. >clone2/b &&
-	GIT_DIR=clone2/.git git add clone2/b &&
-	GIT_DIR=clone2/.git git commit -m new2
+	test_commit one &&
+    test_commit two &&
+    test_commit three three &&
+    mv .git/hooks-disabled .git/hooks
 '
 
-for clone in 1 2; do
-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
+cat >.git/hooks/post-checkout <<'EOF'
 #!/bin/sh
-echo $@ > $GIT_DIR/post-checkout.args
+echo $@ > .git/post-checkout.args
 EOF
-    chmod u+x clone${clone}/.git/hooks/post-checkout
-done
+chmod u+x .git/hooks/post-checkout
 
 test_expect_success 'post-checkout runs as expected ' '
-	GIT_DIR=clone1/.git git checkout master &&
-	test -e clone1/.git/post-checkout.args
+	git checkout master &&
+	test -e .git/post-checkout.args
 '
 
 test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
-	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+	read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout runs as expected ' '
-	GIT_DIR=clone1/.git git checkout master &&
-	test -e clone1/.git/post-checkout.args
+	git checkout master &&
+	test -e .git/post-checkout.args
 '
 
 test_expect_success 'post-checkout args are correct with git checkout -b ' '
-	GIT_DIR=clone1/.git git checkout -b new1 &&
-	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+	git checkout -b new1 &&
+    read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args with HEAD changed ' '
-	GIT_DIR=clone2/.git git checkout new2 &&
-	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+	git checkout two &&
+    read old new flag < .git/post-checkout.args &&
 	test $old != $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args when not switching branches ' '
-	GIT_DIR=clone2/.git git checkout master b &&
-	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+	git checkout master -- three &&
+    read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 0
 '

Signature · 1 line

2.20.1

[email protected]

author

2018-12-20 21:48 UTC #1 lore raw

PATCH 2/2 Rebase: Run post-checkout hook on checkout

From: Orgad Shaneh <[email protected]>

Signed-off-by Orgad Shaneh <[email protected]>

---
 builtin/rebase.c              |  8 +++++++-
 t/t5403-post-checkout-hook.sh | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

builtin/rebase.c +7 -1

index b5c99ec10c..78a09dcda2 100644
@@ -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);

t/t5403-post-checkout-hook.sh +18 -0

index 7e941537f9..de9c7fb871 100755
@@ -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'

Signature · 1 line

2.20.1

Johannes Schindelin

2018-12-21 16:12 UTC #2 lore raw

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

Hi Orgad,

Quoted 3 lines · On Thu, 20 Dec 2018, [email protected] wrote:

From: Orgad Shaneh <[email protected]>
Signed-off-by Orgad Shaneh <[email protected]>

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

Quoted 21 lines

builtin/rebase.c +2 -0

index b5c99ec10c..78a09dcda2 100644
@@ -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;

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 644

Quoted 7 lines

builtin/rebase.c +4 -0

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

Quoted 52 lines

builtin/rebase.c +1 -1

 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);

t/t5403-post-checkout-hook.sh +18 -0

index 7e941537f9..de9c7fb871 100755
@@ -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'

Signature · 1 line

2.20.1

Orgad Shaneh

author

2018-12-24 21:24 UTC #3 lore raw

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

Hi Johannes,

Quoted 45 lines · Johannes Schindelin

Hi Orgad,

Quoted 3 lines · On Thu, 20 Dec 2018, [email protected] wrote:

From: Orgad Shaneh <[email protected]>
Signed-off-by Orgad Shaneh <[email protected]>

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

Quoted 21 lines

builtin/rebase.c +2 -0

index b5c99ec10c..78a09dcda2 100644
@@ -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;

builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 644

Quoted 7 lines

builtin/rebase.c +4 -0

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

Quoted 2 lines

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

Johannes Schindelin

2018-12-21 16:06 UTC #4 lore raw

In reply to [email protected]

Hi Orgad,

Quoted 14 lines · On Thu, 20 Dec 2018, [email protected] wrote:

t/t5403-post-checkout-hook.sh +0 -6

index fc898c9eac..7e941537f9 100755
@@ -7,67 +7,48 @@ test_description='Test the post-checkout hook.'
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo Data for commit0. >a &&
-	echo Data for commit0. >b &&
-	git update-index --add a &&
-	git update-index --add b &&
-	tree0=$(git write-tree) &&
-	commit0=$(echo setup | git commit-tree $tree0) &&

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 12

Quoted 10 lines

t/t5403-post-checkout-hook.sh +3 -7

-	git update-ref refs/heads/master $commit0 &&
-	git clone ./. clone1 &&
-	git clone ./. clone2 &&
-	GIT_DIR=clone2/.git git branch new2 &&
-	echo Data for commit1. >clone2/b &&
-	GIT_DIR=clone2/.git git add clone2/b &&
-	GIT_DIR=clone2/.git git commit -m new2
+	test_commit one &&
+    test_commit two &&
+    test_commit three three &&

A very nice simplification (but please use tabs to indent).

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 18

Quoted 9 lines

t/t5403-post-checkout-hook.sh +3 -3

+    mv .git/hooks-disabled .git/hooks
 '
 
-for clone in 1 2; do
-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
+cat >.git/hooks/post-checkout <<'EOF'
 #!/bin/sh
-echo $@ > $GIT_DIR/post-checkout.args
+echo $@ > .git/post-checkout.args

While at it, you could lose the space after the redirector that we seem to no longer prefer:

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 19

Quoted 1 line

t/t5403-post-checkout-hook.sh +1 -0
+echo $@ >.git/post-checkout.args

And since we are already cleaning up, we could easily move use write_script instead, *and* move it into the setup test case (which makes it easier to use something like

	sh t5403-post-checkout-hook.sh --run=1,13

The rest looks good (modulo indentation issues). I would have preferred the separate concerns to be addressed in individual commits (one commit to replace the awk calls, one to avoid the clones, one to simplify by using test_commit, etc), as that would have been easier to review. But others might disagree (Junio recently made the case of smooshing separate concerns into single commits, even squashing two of my patches into one against my wish), so... I guess you don't have to change this.

Thank you,
Johannes

Quoted 61 lines

t/t5403-post-checkout-hook.sh +12 ~~-21~~

 EOF
-    chmod u+x clone${clone}/.git/hooks/post-checkout
-done
+chmod u+x .git/hooks/post-checkout
 
 test_expect_success 'post-checkout runs as expected ' '
-	GIT_DIR=clone1/.git git checkout master &&
-	test -e clone1/.git/post-checkout.args
+	git checkout master &&
+	test -e .git/post-checkout.args
 '
 
 test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
-	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+	read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout runs as expected ' '
-	GIT_DIR=clone1/.git git checkout master &&
-	test -e clone1/.git/post-checkout.args
+	git checkout master &&
+	test -e .git/post-checkout.args
 '
 
 test_expect_success 'post-checkout args are correct with git checkout -b ' '
-	GIT_DIR=clone1/.git git checkout -b new1 &&
-	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+	git checkout -b new1 &&
+    read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args with HEAD changed ' '
-	GIT_DIR=clone2/.git git checkout new2 &&
-	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+	git checkout two &&
+    read old new flag < .git/post-checkout.args &&
 	test $old != $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args when not switching branches ' '
-	GIT_DIR=clone2/.git git checkout master b &&
-	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
-	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
-	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+	git checkout master -- three &&
+    read old new flag < .git/post-checkout.args &&
 	test $old = $new && test $flag = 0
 '

Signature · 2 lines

-- 
2.20.1

A message that is not in the archive (sent off-list or not yet received): CAGHpTB+9L55Gvezhb7x6Kb49WS_nfzmKdVvpH3_=6GM7y8YQ_g@mail.gmail.com

Orgad Shaneh

author

2018-12-24 20:54 UTC #6 lore raw

Hi Johannes,

Thanks for reviewing this.

Quoted 23 lines · Johannes Schindelin

Hi Orgad,

Quoted 18 lines · On Thu, 20 Dec 2018, [email protected] wrote:

t/t5403-post-checkout-hook.sh +0 ~~-10~~

index fc898c9eac..7e941537f9 100755
@@ -7,67 +7,48 @@ test_description='Test the post-checkout hook.'
 . ./test-lib.sh
 
 test_expect_success setup '
-     echo Data for commit0. >a &&
-     echo Data for commit0. >b &&
-     git update-index --add a &&
-     git update-index --add b &&
-     tree0=$(git write-tree) &&
-     commit0=$(echo setup | git commit-tree $tree0) &&
-     git update-ref refs/heads/master $commit0 &&
-     git clone ./. clone1 &&
-     git clone ./. clone2 &&
-     GIT_DIR=clone2/.git git branch new2 &&

Quoted 8 lines

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 12
Quoted 6 lines
t/t5403-post-checkout-hook.sh +3 -3
-     echo Data for commit1. >clone2/b &&
-     GIT_DIR=clone2/.git git add clone2/b &&
-     GIT_DIR=clone2/.git git commit -m new2
+     test_commit one &&
+    test_commit two &&
+    test_commit three three &&
A very nice simplification (but please use tabs to indent).

Thanks. I already did. I sent these patches twice - first revision had spaces, and the second one had tabs.

Quoted 13 lines

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 18
Quoted 9 lines
t/t5403-post-checkout-hook.sh +3 -3
+    mv .git/hooks-disabled .git/hooks
 '
 
-for clone in 1 2; do
-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'
+cat >.git/hooks/post-checkout <<'EOF'
 #!/bin/sh
-echo $@ > $GIT_DIR/post-checkout.args
+echo $@ > .git/post-checkout.args
While at it, you could lose the space after the redirector that we seem to no longer prefer:

Quoted 7 lines

t/t5403-post-checkout-hook.sh test_description='Test the post-checkout hook.' · line 19
Quoted 1 line
t/t5403-post-checkout-hook.sh +1 -0
+echo $@ >.git/post-checkout.args
And since we are already cleaning up, we could easily move use write_script instead, *and* move it into the setup test case (which makes it easier to use something like
        sh t5403-post-checkout-hook.sh --run=1,13

Done.

Quoted 7 lines

The rest looks good (modulo indentation issues). I would have preferred the separate concerns to be addressed in individual commits (one commit to replace the awk calls, one to avoid the clones, one to simplify by using test_commit, etc), as that would have been easier to review. But others might disagree (Junio recently made the case of smooshing separate concerns into single commits, even squashing two of my patches into one against my wish), so... I guess you don't have to change this.

I also like commit granularity, but since I'm not familiar with the workflow in the mailing list, how to amend and resend commits etc. I try to keep the number of commits low :)

Thanks,
- Orgad