Re: [PATCH v2 1/2] t5510-fetch.sh: demonstrate fetch.writeCommitGraph bug

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 23, 2019, 20:46 UTC
Message-ID: <a9ba6007-aeb9-96b4-9436-8e956798be59@gmail.com>
In-Reply-To: <20191023141813.GA26017@szeder.dev>

On 10/23/2019 10:18 AM, SZEDER Gábor wrote:

Show 35 quoted lines

> On Wed, Oct 23, 2019 at 01:01:34PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> While dogfooding, Johannes found a bug in the fetch.writeCommitGraph
>> config behavior. His example initially happened during a clone with
>> --recurse-submodules, we found that this happens with the first fetch
>> after cloning a repository that contains a submodule:
>>
>> 	$ git clone <url> test
>> 	$ cd test
>> 	$ git -c fetch.writeCommitGraph=true fetch origin
>> 	Computing commit graph generation numbers: 100% (12/12), done.
>> 	BUG: commit-graph.c:886: missing parent <hash1> for commit <hash2>
>> 	Aborted (core dumped)
>>
>> In the repo I had cloned, there were really 60 commits to scan, but
>> only 12 were in the list to write when calling
>> compute_generation_numbers(). A commit in the list expects to see a
>> parent, but that parent is not in the list.
>>
>> A follow-up will fix the bug, but first we create a test that
>> demonstrates the problem.
>>
>> I used "test_expect_failure" for the entire test instead of
>> "test_must_fail" only on the command that I expect to fail. This is
>> because the BUG() returns an exit code so test_must_fail complains.
> 
> I don't think this paragraph is necessary; using 'test_expect_failure'
> is the way to demonstrate a known breakage.
> 
> 'test_must_fail' should only be used when the failure is the desired
> behavior of a git command.  (I used the word "desired" here, because
> you just used the word "expect" above in the sense that "I expect it
> to fail, because I know it's buggy, and I want to demonstrate that
> bug")

I guess that I prefer pointing out which line of the test fails, and making that part of the test (that must otherwise pass). However, you are right that test_expect_failure does a good job of communicating that the test script shows an existing bug. Those are not always cleaned up immediately, but at least we can find them easily.

Show 24 quoted lines

>> Helped-by: Jeff King <peff@peff.net>
>> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> Helped-by: Szeder Gábor <szeder.dev@gmail.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  t/t5510-fetch.sh | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index ecabbe1616..e8ae3af0b6 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -583,6 +583,23 @@ test_expect_success 'fetch.writeCommitGraph' '
>>  	)
>>  '
>>  
>> +test_expect_failure 'fetch.writeCommitGraph with submodules' '
>> +	pwd="$(pwd)" &&
>> +	git clone dups super &&
>> +	(
>> +		cd super &&
>> +		git submodule add "file://$pwd/three" &&
> 
> Nits: couldn't the URL simply be file://$TRASH_DIRECTORY/three ?

True, that would be better. Thanks!

Show 6 quoted lines

>> +		git commit -m "add submodule"
>> +	) &&
>> +	git clone "super" writeError &&
> 
> There is a write error now, because we have a bug, but after the next
> patch the bug will be fixed and we won't have a write error anymore.

Good point.

Thanks! -Stolee