Rebase: Run post-checkout hook on checkout

Subject: Re: [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 | 80 +++++++++++++----------------------
 1 file changed, 29 insertions(+), 51 deletions(-)

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

index fc898c9eac..9f9a5163c5 100755
@@ -7,77 +7,55 @@ 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
-'
-
-for clone in 1 2; do
-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'

t/t5403-post-checkout-hook.sh +2 -4

 mkdir -p templates/hooks
-cat >templates/hooks/post-checkout <<'EOF'
-#!/bin/sh
-echo $@ > $GIT_DIR/post-checkout.args
+write_script templates/hooks/post-checkout <<-\EOF
+echo $@ >$GIT_DIR/post-checkout.args
 EOF
-chmod +x templates/hooks/post-checkout
 
 test_expect_success 'post-checkout hook is triggered by clone' '
 	git clone --template=templates . clone3 &&

From: Orgad Shaneh <[email protected]>

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

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

---
 builtin/rebase.c              | 11 +++++++++--
 t/t5403-post-checkout-hook.sh | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

builtin/rebase.c +0 -0

index b5c99ec10c..7f7a2c738e 100644

builtin/rebase.c +1 -0

@@ -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 +2 -1

@@ -1465,7 +1471,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 					    options.switch_to);
 				if (reset_head(&oid, "checkout",
-					       options.head_name, 0,
+					       options.head_name,
+					       RESET_HEAD_RUN_HOOK,
 					       NULL, buf.buf) < 0) {
 					ret = !!error(_("could not switch to "
 							"%s"),

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

index 9f9a5163c5..5b4e582caa 100755
@@ -13,6 +13,8 @@ test_expect_success setup '
 	EOF
 	test_commit one &&
 	test_commit two &&
+	test_commit rebase-on-me &&
+	git reset --hard HEAD^ &&
 	test_commit three three
 '
 

Quoted 1 line · [email protected] writes:

Subject: Re: [PATCH 1/2] t5403: Refactor

Quoted 1 line

if test "$(git config --bool core.filemode)" = true; then

This is a tangent but this conditional came from an ancient d42ec126 ("disable post-checkout test on Cygwin", 2009-03-17) that says

disable post-checkout test on Cygwin

It is broken because of the tricks we have to play with lstat to get the bearable perfomance out of the call. Sadly, it disables access to Cygwin's executable attribute, which Windows filesystems do not have at all.

I wonder if this is still relevant these days (Cc'ed Ramsay for input).

Quoted 1 line · [email protected] writes:

From: Orgad Shaneh <[email protected]>

Quoted 1 line

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

There is no explanation here?

Is this a regression fix (i.e. scripted version of "rebase" used to run the hook)? Or a new feature (i.e. no earlier version of "rebase" run the hook but you think it ought to run it)?

Quoted 2 lines

+#define RESET_HEAD_RUN_HOOK (1<<2)

Would it be plausible that the only possible hook that can be run by reset_head() function will always be post-checkout and nothing else, I wonder? Shouldn't this bit be called *_RUN_POST_CHECKOUT to make sure it is specific enough?

t/t5403-post-checkout-hook.sh test_expect_success 'post-checkout receives the right args when not switching br · line 58

Quoted 4 lines

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

+test_expect_success 'post-checkout is triggered on rebase' '
+     git checkout -b rebase-test master &&
+     rm -f .git/post-checkout.args &&

Read the title of this whole test script file; it should verify what is in the file before removing it.

Review

Quoted 3 lines

(quoted diff) +2 -0

+     git rebase rebase-on-me &&
+     read old new flag < .git/post-checkout.args &&

No SP between "<" and ".git/post-checkout.args".

t/t5403-post-checkout-hook.sh test_expect_success 'post-checkout receives the right args when not switching br · line 63

Quoted 3 lines

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

+     test $old != $new && test $flag = 1 &&
+     rm -f .git/post-checkout.args
+'

Regarding the clean-up of this file, see my review on the previous one.

Quoted 1 line

+test_expect_success 'post-checkout is triggered on rebase with fast-forward' '

The same comment as above applies to this.

Quoted 1 line · [email protected] writes:

Subject: Re: [PATCH 1/2] t5403: Refactor

Hmph. "Refactor" alone leaves readers wondering "refactor what?", "refactor for what?" and "refactor how?". Given that the overfall goal this change seeks seems to be to simplify it by losing about 20 lines, how about justifying it like so?

Subject: t5403: simplify by using a single repository

There is no strong reason to use separate clones to run these tests; just use a single test repository prepared with more modern test_commit shell helper function.

While at it, replace three "awk '{print $N}'" on the same file with shell built-in "read" into three variables.

Quoted 1 line

+ mv .git/hooks-disabled .git/hooks &&

I am not sure why you want to do this---it sends a wrong signal to readers saying that you want to use whatever hook that are in the moved-away .git/hooks-disabled/ directory. I am guessing that the only reason why you do this is because there must be .git/hooks directory in order for write_script below to work, so a more readable approach would be to "mkdir .git/hooks" instead, no?

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

Quoted 2 lines

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

+     write_script .git/hooks/post-checkout <<-\EOF &&
+     echo $@ >.git/post-checkout.args

A dollar-at inside a shell script that is not in a pair of dq always makes readers wonder if the author forgot dq around it or omitted eq around it deliberately; avoid it.

In this case, use "$@" (i.e. within dq) would be more friendly to readers.

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

Quoted 4 lines

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

+     EOF
+     test_commit one &&
+     test_commit two &&
+     test_commit three three

Makes readers wonder why the last one duplicates. Is this because you somehow do not want to use three.t as the pathname in a later test? "test_commit X" that creates test file X.t is a quite well established convention (see "git grep '\.t\>' t/"), by the way.

Quoted 1 line

+ test -e .git/post-checkout.args &&

Use "test -f", as you do know you'd be creating a file ("-e" succeeds as long as it _exists_, and does not care if it is a file or directory or whatever).

Quoted 1 line

+ read old new flag <.git/post-checkout.args &&

This indeed is much nicer.

Review

Quoted 2 lines

(quoted diff) +2 -0

+     test $old = $new && test $flag = 1 &&
+     rm -f .git/post-checkout.args

The last one is not a test but a clean-up. If any of the earlier step failed (e.g. $old and $new were different), the output file would be left behind, resulting in confusing the next test.

Instead, do it like so:

        test_expect_success 'title of the test' '
                test_when_finished "rm -f .git/post-checkout.args" &&
                git checkout master &&
                test -f .git/post-checkout.args &&
                read old new flag <.git/post-checkout.args &&
                test $old = $new &&
                test $flag = 1
        '

That is, use test_when_finished() before the step that creates the file that may be left behind to arrange that it will be cleaned at the end.

This comment on clean-up applies to _all_ tests in this patch that has "rm -f .git/post-checkout.args" at the end.

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

Quoted 7 lines

test_expect_success 'post-checkout runs as expected ' '

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

-     GIT_DIR=clone1/.git git checkout master &&
-     test -e clone1/.git/post-checkout.args
+     git checkout master &&
+     test -e .git/post-checkout.args &&
+     rm -f .git/post-checkout.args
 '

Now that the script got so simplified, this one looks even more redundant, given that the previous one already checked the same "checkout 'master' when already at the tip of 'master'" situation.

Do we still need this one, in other words?

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

Quoted 11 lines

test_expect_success 'post-checkout args are correct with git checkout -b ' '

t/t5403-post-checkout-hook.sh +4 -5

-     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) &&
-     test $old = $new && test $flag = 1
+     git checkout -b new1 &&
+     read old new flag <.git/post-checkout.args &&
+     test $old = $new && test $flag = 1 &&
+     rm -f .git/post-checkout.args
 '

This one forgets "did the hook run and create the file" before "read", unlike the previous tests. It is not strictly necessary as "read" will fail if the file is not there, but it'd be better to be consistent.

Quoted 1 line

if test "$(git config --bool core.filemode)" = true; then

This is a tangent but this conditional came from an ancient d42ec126 ("disable post-checkout test on Cygwin", 2009-03-17) that says

disable post-checkout test on Cygwin

It is broken because of the tricks we have to play with lstat to get the bearable perfomance out of the call. Sadly, it disables access to Cygwin's executable attribute, which Windows filesystems do not have at all.

I wonder if this is still relevant these days (Cc'ed Ramsay for input). Windows port should be running enabled hooks (and X_OK is made pretty much no-op in compat/mingw.c IIUC), so the above conditional is overly broad anyway, even if Cygwin still has issues with the executable bit.