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

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

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

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

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.

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

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.