[GSoC,2/5] t3430: use lib-log-graph functions

Commit Message

Abhishek Kumar Feb. 16, 2020, 1:47 p.m. UTC

Helped-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Abhishek Kumar <[e-mail]>
---
 t/t3430-rebase-merges.sh | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

Comments

Junio C Hamano Feb. 19, 2020, 5:23 p.m. UTC | #1

Abhishek Kumar <[e-mail]> writes:

> Helped-by: Johannes Schindelin <[e-mail]>
> Signed-off-by: Abhishek Kumar <[e-mail]>
> ---
>  t/t3430-rebase-merges.sh | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index e72ca348ea..74c61fa787 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -20,13 +20,7 @@ Initial setup:
>  '
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-rebase.sh
> -
> -test_cmp_graph () {
> -	cat >expect &&
> -	git log --graph --boundary --format=%s "$@" >output &&
> -	sed "s/ *$//" <output >output.trimmed &&
> -	test_cmp expect output.trimmed
> -}
> +. "$TEST_DIRECTORY"/lib-log-graph.sh
>  
>  test_expect_success 'setup' '
>  	write_script replace-editor.sh <<-\EOF &&
> @@ -84,7 +78,7 @@ test_expect_success 'create completely different structure' '
>  	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
>  	test_tick &&
>  	git rebase -i -r A master &&
> -	test_cmp_graph <<-\EOF
> +	test_cmp_graph --pretty=tformat:%s --boundary <<-\EOF

The original used a more readble short-hand "--format=%s"; was there
a strong reason why we wanted to use "--pretty=tformat:%s"?

The same comment applies to all the following hunks.

I actually have to wonder if this is a good change at all.  Surely
you lost one local and specialized test helper and replaced its use
with a more flexible one from the lib-log-graph file, but because
the one from the lib-log-graph is more flexible, you now need to
tell it what options the tests want to give to the "git log"
command, the same thing over and over, which would make it much more
error prone, no?

It would have been more acceptable if we kept test_cmp_graph a local
and specialized test helper defined in this file, but changed its
implementation (i.e. the 4 lines we see above) to call to a more
generic helper function defined in lib-log-graph file, i.e.

	test_cmp_graph () {
		test_cmp_graph_from_lib --boundary --format=%s "$@"
	}

but then the more flexible helper defined in lib-log-graph file
cannot squat on the short-and-sweet name "test_cmp_graph" that is
already used in the test scripts without unnecessary churn.

I dunno.

Patch

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index e72ca348ea..74c61fa787 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -20,13 +20,7 @@  Initial setup:
 '
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
-
-test_cmp_graph () {
-	cat >expect &&
-	git log --graph --boundary --format=%s "$@" >output &&
-	sed "s/ *$//" <output >output.trimmed &&
-	test_cmp expect output.trimmed
-}
+. "$TEST_DIRECTORY"/lib-log-graph.sh
 
 test_expect_success 'setup' '
 	write_script replace-editor.sh <<-\EOF &&
@@ -84,7 +78,7 @@  test_expect_success 'create completely different structure' '
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
 	git rebase -i -r A master &&
-	test_cmp_graph <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary <<-\EOF
 	*   Merge the topic branch '\''onebranch'\''
 	|\
 	| * D
@@ -201,7 +195,7 @@  test_expect_success 'with a branch tip that was cherry-picked already' '
 	git checkout already-upstream &&
 	test_tick &&
 	git rebase -i -r upstream-with-a2 &&
-	test_cmp_graph upstream-with-a2.. <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary upstream-with-a2.. <<-\EOF
 	*   Merge branch A
 	|\
 	| * A1
@@ -219,7 +213,7 @@  test_expect_success 'do not rebase cousins unless asked for' '
 	test_cmp_rev HEAD $before &&
 	test_tick &&
 	git rebase --rebase-merges=rebase-cousins HEAD^ &&
-	test_cmp_graph HEAD^.. <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary HEAD^.. <<-\EOF
 	*   Merge the topic branch '\''onebranch'\''
 	|\
 	| * D
@@ -311,7 +305,7 @@  test_expect_success 'root commits' '
 	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
 	test $(git rev-parse second-root:second-root.t) = \
 		$(git rev-parse HEAD^:second-root.t) &&
-	test_cmp_graph HEAD <<-\EOF &&
+	test_cmp_graph --pretty=tformat:%s --boundary HEAD <<-\EOF &&
 	*   Merge the 3rd root
 	|\
 	| * third-root
@@ -347,7 +341,7 @@  test_expect_success 'A root commit can be a cousin, treat it that way' '
 	test_tick &&
 	git rebase -f -r HEAD^ &&
 	test_cmp_rev ! HEAD^2 khnum &&
-	test_cmp_graph HEAD^.. <<-\EOF &&
+	test_cmp_graph --pretty=tformat:%s --boundary HEAD^.. <<-\EOF &&
 	*   Merge branch '\''khnum'\'' into asherah
 	|\
 	| * yama
@@ -355,7 +349,7 @@  test_expect_success 'A root commit can be a cousin, treat it that way' '
 	EOF
 	test_tick &&
 	git rebase --rebase-merges=rebase-cousins HEAD^ &&
-	test_cmp_graph HEAD^.. <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary HEAD^.. <<-\EOF
 	*   Merge branch '\''khnum'\'' into asherah
 	|\
 	| * yama
@@ -402,7 +396,7 @@  test_expect_success 'octopus merges' '
 	git rebase -i --force-rebase -r HEAD^^ &&
 	test "Hank" = "$(git show -s --format=%an HEAD)" &&
 	test "$before" != $(git rev-parse HEAD) &&
-	test_cmp_graph HEAD^^.. <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary HEAD^^.. <<-\EOF
 	*-.   Tüntenfüsch
 	|\ \
 	| | * three
@@ -478,7 +472,7 @@  test_expect_success '--rebase-merges with message matched with onto label' '
 	git checkout -b onto-label E &&
 	git merge -m onto G &&
 	git rebase --rebase-merges --force-rebase E &&
-	test_cmp_graph <<-\EOF
+	test_cmp_graph --pretty=tformat:%s --boundary <<-\EOF
 	*   onto
 	|\
 	| * G