On 1/5/21 10:05 PM, Junio C Hamano wrote:
> Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> writes:
>
>> 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?
You are correct that the A4 and A6 tags can be removed without affecting
the output. In fact, A4 is basically immediately deleted (in the second test).
I can remove that, if we want to stop testing the tag deletion logic here.
I suppose that is sufficiently validated elsewhere in the test suite.
There's a (weak IMO) argument to keep the A6 tag, since it might in principle
affect the output at later stages, since it is not deleted and calls are made
with --simplify-by-decoration. Of course, it isn't needed in order to be
displayed, so we're only testing that that the merge commit A6 shows up when
it *has* a tag, and --simplify-by-decoration is used. That failure mode
certainly seems perverse!
My guiding principle when I made this patch was to be as minimally invasive
as possible, while allowing modifications to this file to be pleasant---which
I must admit is my ulterior motive.
I can certainly remove these "extraneous" tags if desired.
Best,
Antonio
> That is ...
>
>>> . ./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?
>
>>> - # 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 ...
>>> + check_graph --all <<-\EOF
>>> + * A7
>>> + * A6
>>> + |\
>>> + | * C4
>>> + | * C3
>>> + * | A5
>>> + | |
>>> + | \
>>> + *-. | A4
>
> ... not anymore in the new version.
>
>>> + |\ \|
>>> + | | * C2
>>> + | | * C1
>>> + | * | B2
>>> + | * | B1
>>> + * | | A3
>>> + | |/
>>> + |/|
>>> + * | A2
>>> + |/
>>> + * A1
>>> + EOF
>>> +'
>>>
Attachment:
OpenPGP_0xB01C53D5DED4A4EE.asc
Description: application/pgp-keys
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
