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

[Junio C Hamano <gitster@pobox.com>]

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

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