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

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

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

> +check_graph () {
> +	cat >expected &&

Not a new issue, but we may want to fix this to align to majority of
tests by calling it "expect".

> +	git rev-list --graph "$@" | sed "$(cat sedscript)" > actual &&

Style. No SP between > (or < for that matter) and the filename.

The "sed" utility can be told to read its script from a file with
its "-f" option.

Correctness.  Never run "git" command that is the target being
tested on the left side of a pipe.  It will hide the exit status.

> +	test_cmp expected actual
> +}

>  test_expect_success 'set up rev-list --graph test' '
>  	# 3 commits on branch A
>  	test_commit A1 foo.txt &&
> @@ -43,76 +49,62 @@ test_expect_success 'set up rev-list --graph test' '
>
>  	test_commit A7 bar.txt &&
>
> -	# Store commit names in variables for later use
> -	A1=$(git rev-parse --verify A1) &&
> -	A2=$(git rev-parse --verify A2) &&
> -	A3=$(git rev-parse --verify A3) &&
> -	A4=$(git rev-parse --verify A4) &&
> -	A5=$(git rev-parse --verify A5) &&
> -	A6=$(git rev-parse --verify A6) &&
> -	A7=$(git rev-parse --verify A7) &&
> -	B1=$(git rev-parse --verify B1) &&
> -	B2=$(git rev-parse --verify B2) &&
> -	C1=$(git rev-parse --verify C1) &&
> -	C2=$(git rev-parse --verify C2) &&
> -	C3=$(git rev-parse --verify C3) &&
> -	C4=$(git rev-parse --verify C4)
> -	'
> +	echo "s/ *$//;" > sedscript &&
> +	( for tag in $(git tag -l) ; do echo "s/$(git rev-parse --verify $tag)/$tag/;" >> sedscript ; done )

Avoid unreadable one-liner with needless subshell.

I suspect that this is a task for-each-ref was designed for,
something along the lines of...

	git for-each-ref --format='s|%(objectname)|%(refname:short)|' \
		refs/tags/ >>sedScript

> +'
>
>  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

Much nicer to see.

Having said all that, I am not sure if this change of design is
sound.

The original approach would have worked even if two or more of these
tags pointed at the same object.  Your version will pick one of
them.  If two tags, say A5 and C8, pointed at the same commit, and
the illustration given to check_graph helper from its standard
output labeled a commit as C8, wouldn't the actual output converted
to show A5 with your sedScript approach?

I think it is salvageable by changing the direction you munge.
Instead of munging the rev-list output, you can store it as-is in
"actual", and instead pass the illustration that comes from the
standard input of the check_graph helper through sed to expand the
symbolic names to actual object names before comparing. i.e.

	check_graph () {
		sed -f expand_tag_to_objects.sed >expect &&
		git rev-list --graph "$@" >actual &&
		test_cmp expect actual
	}

Note that I renamed the overly generic "sedscript" to a name that
reflects the purpose of the file (and the contents being of a
certain type is conveyed by .sed suffix, just like you'd use
suffixes like .c, .txt).  A good discipline to learn, I would say.

Thanks.