t4215: don't put git commands upstream of pipe
diff mbox series

Message ID 8e950ddfba3fa0f6d0551a153228548da6af6117.1573520653.git.liu.denton@gmail.com
State New
Headers show
Series
  • t4215: don't put git commands upstream of pipe
Related show

Commit Message

Denton Liu Nov. 12, 2019, 1:08 a.m. UTC
When git commands are placed in the upstream of a pipe, their return
codes are lost. In this particular case, it is especially bad since we
are testing the intricacies of `git log --graph` behavior and if we hit
an unexpected failure or segfault, we want to know this.

Redirect the output of git commands upstream of pipe into a file and
have sed read from that file so that git failures are detected.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Thanks for the suggestion, Gábor. (Is that how I should refer to you? I
recently learned that some poeple write their names in ALL CAPS as a
convention.)

A little late to the party but since this cleanup hasn't been done yet,
let's do it now. We can apply this patch to the tip of
'jc/log-graph-simplify'.

 t/t4215-log-skewed-merges.sh | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Nov. 12, 2019, 6:57 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> A little late to the party but since this cleanup hasn't been done yet,
> let's do it now. We can apply this patch to the tip of
> 'jc/log-graph-simplify'.
> ...
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&

Obviously good but I wonder if

	show_graph () {
		git log --graph --pretty=tformat:%s >actual.raw &&
		sed "s/ *$//" actual.raw &&
		rm actual.raw
	}

would help to make it even more readable without too much repetition.

Will queue; thanks.

>  	test_cmp expect actual
>  '
>  
> @@ -68,7 +69,8 @@ test_expect_success 'log --graph with left-skewed merge' '
>  	git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
>  	git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
>  
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -99,7 +101,8 @@ test_expect_success 'log --graph with nested left-skewed merge' '
>  	git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
>  	git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
>  
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -139,7 +142,8 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
>  	git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
>  	git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
>  
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -175,7 +179,8 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
>  	git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
>  	git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
>  
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -210,7 +215,8 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
>  	git merge --no-ff 4_p -m 4_G &&
>  	git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
>  
> -	git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --date-order --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -250,7 +256,8 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
>  	git checkout 5_r &&
>  	git merge --no-ff 5_s -m 5_H &&
>  
> -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> +	git log --graph --pretty=tformat:%s >actual.raw &&
> +	sed "s/ *$//" actual.raw >actual &&
>  	test_cmp expect actual
>  '
SZEDER Gábor Nov. 12, 2019, 10:54 a.m. UTC | #2
On Tue, Nov 12, 2019 at 03:57:08PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > A little late to the party but since this cleanup hasn't been done yet,
> > let's do it now. We can apply this patch to the tip of
> > 'jc/log-graph-simplify'.
> > ...
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> 
> Obviously good but I wonder if
> 
> 	show_graph () {
> 		git log --graph --pretty=tformat:%s >actual.raw &&
> 		sed "s/ *$//" actual.raw &&
> 		rm actual.raw
> 	}
> 
> would help to make it even more readable without too much repetition.

I think it would indeed, but then let's go one step further, and add
that 'test_cmp expect actual' to the function, too, and call it
'check_graph'.

> Will queue; thanks.
> 
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -68,7 +69,8 @@ test_expect_success 'log --graph with left-skewed merge' '
> >  	git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
> >  	git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
> >  
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -99,7 +101,8 @@ test_expect_success 'log --graph with nested left-skewed merge' '
> >  	git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
> >  	git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
> >  
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -139,7 +142,8 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
> >  	git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
> >  	git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
> >  
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -175,7 +179,8 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
> >  	git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
> >  	git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
> >  
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -210,7 +215,8 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
> >  	git merge --no-ff 4_p -m 4_G &&
> >  	git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
> >  
> > -	git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --date-order --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '
> >  
> > @@ -250,7 +256,8 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
> >  	git checkout 5_r &&
> >  	git merge --no-ff 5_s -m 5_H &&
> >  
> > -	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > +	git log --graph --pretty=tformat:%s >actual.raw &&
> > +	sed "s/ *$//" actual.raw >actual &&
> >  	test_cmp expect actual
> >  '

Patch
diff mbox series

diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index d33c6438d8..0a2555fb28 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -31,7 +31,8 @@  test_expect_success 'log --graph with merge fusing with its left and right neigh
 	git checkout _p && git merge --no-ff _r -m G &&
 	git checkout @^^ && git merge --no-ff _p -m H &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -68,7 +69,8 @@  test_expect_success 'log --graph with left-skewed merge' '
 	git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
 	git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -99,7 +101,8 @@  test_expect_success 'log --graph with nested left-skewed merge' '
 	git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
 	git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -139,7 +142,8 @@  test_expect_success 'log --graph with nested left-skewed merge following normal
 	git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
 	git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -175,7 +179,8 @@  test_expect_success 'log --graph with nested right-skewed merge following left-s
 	git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
 	git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -210,7 +215,8 @@  test_expect_success 'log --graph with right-skewed merge following a left-skewed
 	git merge --no-ff 4_p -m 4_G &&
 	git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
 
-	git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --date-order --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '
 
@@ -250,7 +256,8 @@  test_expect_success 'log --graph with octopus merge with column joining its penu
 	git checkout 5_r &&
 	git merge --no-ff 5_s -m 5_H &&
 
-	git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+	git log --graph --pretty=tformat:%s >actual.raw &&
+	sed "s/ *$//" actual.raw >actual &&
 	test_cmp expect actual
 '