Re: [PATCH 1/3] Clean up t6016-rev-list-graph-simplify-history

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

Antonio Russo <antonio.e.russo@gmail.com> writes:

> Subject: Re: [PATCH 1/3] Clean up t6016-rev-list-graph-simplify-history

Hmph, didn't anybody give you guidance on the subjects the last
round?

If this is a second round (I do not recall---I read too many
patches), the subject should begin with

	[PATCH v2 1/3]

and then "<area>: one line summary".  For test scripts, the script
number is enough to identify the area the patch affects, e.g.

	[PATCH v2 1/3] t6016: simplify the way expected history is drawn

> Simplifies the logic used to test rev-list, making adding new tests
> easier.  Uses a heredoc and sed expansion of the expected output,
> instead of shell substitutions and manually escaped echo's.

Justify why this change is a good thing upfront, by (1) giving a
short summary of what is done in the current code and (2) saying
what is suboptimal in it.

    Many tests in this script prepare each line of the expected
    --graph output separately with "echo", using bare object names
    of commits shown in the graph (captured in shell variables), run
    "rev-list --graph" with some other parameters and compares the
    result.  The expected shape of the graph is hard to see and the
    resulting code is repetitious.

And then outline your solution, giving orders to the codebase to
"become like so", in the next paragraph.

    In order to add new tests easier, introduce a helper function
    that takes extra parameters given to the "rev-list --graph" as
    its arguments, and the expected output from its standard input
    stream.  By allowing tagnames to be used in the expected output,
    eliminate the need to capture commit object names in shell
    variables.

cf. Documentation/SubmittingPatches::imperative-mood

> Reviewed-by: Junio C Hamano <gitster@pobox.com>

I didn't review this version (in other words, I think you changed
the patch after I reviewed)---so do not write this line.  It is
misleading.

>  . ./test-lib.sh
>
> +check_graph () {
> +	sed -f expand_tag_to_oid.sed >expect &&
> +	git rev-list --graph "$@" >actual &&
> +	sed 's/ *$//' actual >actual.sanitized &&
> +	test_cmp expect actual.sanitized
> +}

OK, so we prepare the mapping from tagname to objectname somewhere,
feed an expected output that uses tagname to this helper function
and munge it into the file expect, and then compare it with the
actual output.  

Why do we drop the trailing whitespace in the output before
comparing?  Is it because it does not matter?  Is it because it is
cumbersome to spell in the expected output in the source?

Not complaining.  But if we truly aim to make writing new tests
easier, we should tell those who may write new tests what they are
expected to be doing when using this helper function in a comment
before it to help them.  Some points I can think of off the top of
my head are:

 * The file expend_tag_to_oid.sed is created in a single "setup"
   test;  when adding a new test, the shape of the history and new
   tags used in it must be prepared before the "for-each-ref" that
   produces the file in the "setup" test.

 * The arguments to the helper are given to "git rev-list --graph"
   to be compared with the expected graph fed from the standard
   input.

 * The expected graph can use tags and branches instead of object
   names (and it is encouraged to do so) for readability.  You do
   not mimick trailing whitespaces on the lines [*1*].


>  test_expect_success '--graph --all' '
> -	rm -f expected &&
> -	echo "* $A7" >> expected &&
> -	echo "*   $A6" >> expected &&
> -	echo "|\\  " >> expected &&
> -	echo "| * $C4" >> expected &&
> -	echo "| * $C3" >> expected &&
> -	echo "* | $A5" >> expected &&
> -	echo "| |   " >> expected &&
> -	echo "|  \\  " >> expected &&
> -	echo "*-. | $A4" >> expected &&
> -	echo "|\\ \\| " >> expected &&
> -	echo "| | * $C2" >> expected &&
> -	echo "| | * $C1" >> expected &&
> -	echo "| * | $B2" >> expected &&
> -	echo "| * | $B1" >> expected &&
> -	echo "* | | $A3" >> expected &&
> -	echo "| |/  " >> expected &&
> -	echo "|/|   " >> expected &&
> -	echo "* | $A2" >> expected &&
> -	echo "|/  " >> expected &&
> -	echo "* $A1" >> expected &&
> -	git rev-list --graph --all > actual &&
> -	test_cmp expected actual
> -	'
> +	check_graph --all <<-\EOF
> +	* A7
> +	*   A6
> +	|\
> +	| * C4
> +	| * C3
> +	* | A5
> +	| |
> +	|  \
> +	*-. | A4
> +	|\ \|
> +	| | * C2
> +	| | * C1
> +	| * | B2
> +	| * | B1
> +	* | | A3
> +	| |/
> +	|/|
> +	* | A2
> +	|/
> +	* A1
> +	EOF
> +'

I do agree that the resulting test is easier to understand.

Thanks.

[Footnote *1*]

This is a tangent, but I wonder if somebody looked into how feasible
it is to eliminate the trailing whitespace in the graph output.  It
is OK to give whitespaces in anticipation of writing the commit
object names, but on a line in the output that exists only to show
connecting lines, where we know we will not write anything after the
graph, there is no excuse to end the graph part with a trailing
whitespace.