diff mbox series

[1/2] t5403: Refactor

Message ID 20181220214823.21378-1-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>

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

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
 t/t5403-post-checkout-hook.sh | 55 ++++++++++++-----------------------
 1 file changed, 18 insertions(+), 37 deletions(-)

Comments

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

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

> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index fc898c9eac..7e941537f9 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -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 &&

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

> +    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:

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

>  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
>  '
>  
> -- 
> 2.20.1
> 
>
Orgad Shaneh Dec. 24, 2018, 8:54 p.m. UTC | #2 
Hi Johannes,

Thanks for reviewing this.

On Fri, Dec 21, 2018 at 6:06 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Orgad,
>
> On Thu, 20 Dec 2018, orgads@gmail.com wrote:
>
> > diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> > index fc898c9eac..7e941537f9 100755
> > --- a/t/t5403-post-checkout-hook.sh
> > +++ b/t/t5403-post-checkout-hook.sh
> > @@ -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 &&
>
> 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.

> > +    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:
>
> > +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.

> 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
diff mbox series

Patch

diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index fc898c9eac..7e941537f9 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -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
 '