Re: [PATCH] t6016: move to lib-log-graph.sh framework

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2021, 05:05 UTC
Message-ID: <xmqq1reywt7x.fsf@gitster.c.googlers.com>
In-Reply-To: <X/K1BgP8tpsgNe2x@Abhishek-Arch>

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:

Show 25 quoted lines

> On Sun, Jan 03, 2021 at 07:30:35PM -0700, Antonio Russo wrote:
>> t6016 manually reconstructs git log --graph output by using the reported
>> commit hashes from `git rev-parse`.  Each tag is converted into an
>> environment variable manually, and then `echo`-ed to an expected output
>> file, which is in turn compared to the actual output.
>> 
>> The expected output is difficult to read and write, because, e.g.,
>> each line of output must be prefaced with echo, quoted, and properly
>> escaped.  Additionally, the test is sensitive to trailing whitespace,
>> which may potentially be removed from graph log output in the future.
>> 
>> In order to reduce duplication, ease troubleshooting of failed tests by
>> improving readability, and ease the addition of more tests to this file,
>> port the operations to `lib-log-graph.sh`, which is already used in
>> several other tests, e.g., t4215.  Give all merges a simple commit
>> message, and use a common `check_graph` macro taking a heredoc of the
>> expected output which does not required extensive escaping.
>> 
>
> Glad to see others using `lib-log-graph.sh` as well!
>
> The changes look alright to me but maybe you could have split the two
> changes into two different commits: Using tags directly instead of
> environment variables and using `check_graph` instead of manually
> `echo`-ing to an expected output and comparing with the actual output.

Perhaps. Also I am wondering if the tests still need to create tags after this change---isn't the output all matched by the commit title now?

That is ...

Show 7 quoted lines

>>  . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-log-graph.sh
>> +
>> +check_graph () {
>> +	cat >expect &&
>> +	lib_test_cmp_graph --format=%s "$@"
>> +}

... this only shows the title, and ...

>> -	git merge B C &&
>> +	git merge B C -m A4 &&

... that is why we need to have the title here.

>>  	git tag A4 &&

Now, does this A4 used anywhere?

Show 5 quoted lines

>> -	# 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) &&

It certainly used to be needed here, but ...

Show 10 quoted lines

>> +	check_graph --all <<-\EOF
>> +	* A7
>> +	*   A6
>> +	|\
>> +	| * C4
>> +	| * C3
>> +	* | A5
>> +	| |
>> +	|  \
>> +	*-. | A4

... not anymore in the new version.

Show 14 quoted lines

>> +	|\ \|
>> +	| | * C2
>> +	| | * C1
>> +	| * | B2
>> +	| * | B1
>> +	* | | A3
>> +	| |/
>> +	|/|
>> +	* | A2
>> +	|/
>> +	* A1
>> +	EOF
>> +'
>>