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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2020, 17:23 UTC
Message-ID: <xmqqy2syfq48.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200216134750.18947-2-abhishekkumar8222@gmail.com>

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

Show 31 quoted lines

> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  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.