Re: [PATCH v3 06/20] commit-graph: add 'verify' subcommand

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <stolee@gmail.com> writes:
> On 5/27/2018 6:55 PM, Jakub Narebski wrote:
>> Derrick Stolee <dstolee@microsoft.com> writes:
[...]
>>> +static int verify_commit_graph_error;
>>> +
>>> +static void graph_report(const char *fmt, ...)
>>> +{
>>> +	va_list ap;
>>> +	struct strbuf sb = STRBUF_INIT;
>>> +	verify_commit_graph_error = 1;
>>> +
>>> +	va_start(ap, fmt);
>>> +	strbuf_vaddf(&sb, fmt, ap);
>>> +
>>> +	fprintf(stderr, "%s\n", sb.buf);
>>> +	strbuf_release(&sb);
>>> +	va_end(ap);
>>
>> Why do you use strbuf_vaddf + fprintf instead of straighforward
>> vfprintf (or function instead of variable-level macro)?
>>
>> Is it because of [string] safety?
>
> It's because I've never used this variable-parameter thing before and
> found a different example.
>
> I'll use vfprintf() in v4, as it is simpler.

All right, if it is not dangerous, then simpler is better.

Sidenote: such error messaging is often handled by variadic macros,
e.g.:

  #define eprintf(...) fprintf(stderr, __VA_ARGS__)

[...]
>>> diff --git a/commit-graph.h b/commit-graph.h
>>> index 96cccb10f3..71a39c5a57 100644
>>> --- a/commit-graph.h
>>> +++ b/commit-graph.h
>>> @@ -53,4 +53,6 @@ void write_commit_graph(const char *obj_dir,
>>>   			int nr_commits,
>>>   			int append);
>>>   +int verify_commit_graph(struct commit_graph *g);
>>> +
>> Why does this need to be exported?  I think it is not used outside of
>> commit-graph.c, isn't it?
>
> Used by builtin/commit-graph.c

Ah, true.

[...]
>>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>>> index 77d85aefe7..6ca451dfd2 100755
>>> --- a/t/t5318-commit-graph.sh
>>> +++ b/t/t5318-commit-graph.sh
>>> @@ -11,6 +11,11 @@ test_expect_success 'setup full repo' '
>>>   	objdir=".git/objects"
>>>   '
>>> +test_expect_success 'verify graph with no graph file' '
>>> +	cd "$TRASH_DIRECTORY/full" &&
>>> +	git commit-graph verify
>>> +'
>>> +
>>>   test_expect_success 'write graph with no packs' '
>>>   	cd "$TRASH_DIRECTORY/full" &&
>>>   	git commit-graph write --object-dir . &&
>>> @@ -230,4 +235,9 @@ test_expect_success 'perform fast-forward merge in full repo' '
>>>   	test_cmp expect output
>>>   '
>>>   +test_expect_success 'git commit-graph verify' '
>>> +	cd "$TRASH_DIRECTORY/full" &&
>>> +	git commit-graph verify >output
>>> +'
>> Those are tests with nearly the same code, but they are (by their
>> descriptions) testing different things.  This means that they rely on
>> side effects of earlier tests.
>>
>> This is suboptimal, as it means that it would be impossible or very
>> difficult to run individual tests (e.g. with GIT_SKIP_TESTS environment
>> variable, or with an individual test suite --run option), unless you
>> know which tests setup the repository state for later tests.
>>
>> It also means that running only failed tests with prove
>> --state=failed,save or equivalently with
>>
>>    $ make DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS='--state=failed,save' test
>>
>> wouldn't work correctly.
>>
>> As Johannes Schindelin (alias Dscho) said in latest Git Rev News
>> interview: https://git.github.io/rev_news/2018/05/16/edition-39/
>>
>> JS> We have a test suite where debugging a regression may mean that you
>> JS> have to run 98 test cases before the failing one every single time in
>> JS> the edit/compile/debug cycle, because the 99th test case may depend on
>> JS> a side effect of at least one of the preceding test cases. Git’s test
>> JS> suite is so not [21st century best practices][1].
>> JS>
>> JS> [1]: https://www.slideshare.net/BuckHodges/lessons-learned-doing-devops-at-scale-at-microsoft
>>
>>
>> I think can be solved quite efficiently by creating and using shell
>> function, or two shell functions, which would either:
>>
>>   * rename commit-graph file to some other temporary name if it exists,
>>     and move it back after the test.
>>   * create commit-graph file if it does not exist.
>>
>> For example (untested):
>>
>>    prepare_no_commit_graph() {
>>    	mv .git/info/commit-graph .git/info/commit-graph.away &&
>>    	test_when_finished "mv .git/info/commit-graph.away .git/info/commit-graph"
>>    }
>>
>>    prepare_commit_graph() {
>>    	if ! test -f ".git/info/commit-graph"
>>    	then
>>    		git commit-graph write
>>    	fi
>>    }
>>
>> Or something like that.
>
> Do we have a way to run individual steps of the test suite? I am
> unfamiliar with that process.

The t/README describes three such ways in "Skipping Tests" section:

- GIT_SKIP_TESTS environment variable, which can either can match the
  "t[0-9]{4}" part to skip the whole test, or t[0-9]{4} followed by
  ".$number" to say which particular test to skip

- For an individual test suite --run could be used to specify that
  only some tests should be run or that some tests should be
  excluded from a run (the latter with '!' prefix).

- 'prove' harness can also run individual tests; one of more useful
  options is --state, which for example would allow to run only failed
  tests with --state=failed,save ... if the tests were independent.

>
> Adding the complexity of storing a copy of the commit-graph file for
> re-use in a later test is wasted energy right now, because we need to
> run the steps of the test that create the repo shape with the commits
> laid out as set earlier in the test. This shape changes as we test
> different states of the commit-graph (exists and contains all commits,
> exists and doesn't contain all commits, etc.)

I think we can solve most of the problem by separating validation tests
(which all or almost all use the same commit-graph file) and other test;
putting them in different test scripts.  This means that the more
complicated case would be limited to the subset of tests.

Anyway, if the setup stages are clearly separated and clearly marked as
such, we would be able to at least manually skip tests, or manually run
only a subset of tests.

Test independence is certainly something nice to have, but as the git
testsuite is not in best shape wrt this, it is not a requirement.

Best,
-- 
Jakub Narębski