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

Commit Message

Derrick Stolee via GitGitGadget Oct. 23, 2019, 1:01 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Helped-by: Jeff King <[e-mail]>
Helped-by: Johannes Schindelin <[e-mail]>
Helped-by: Szeder Gábor <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 t/t5510-fetch.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

SZEDER Gábor Oct. 23, 2019, 2:18 p.m. UTC | #1

On Wed, Oct 23, 2019 at 01:01:34PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>
> 
> 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")

> Helped-by: Jeff King <[e-mail]>
> Helped-by: Johannes Schindelin <[e-mail]>
> Helped-by: Szeder Gábor <[e-mail]>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  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 ?

> +		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.

> +	(
> +		cd writeError &&
> +		test_path_is_missing .git/objects/info/commit-graphs/commit-graph-chain &&
> +		git -c fetch.writeCommitGraph=true fetch origin &&
> +		test_path_is_file .git/objects/info/commit-graphs/commit-graph-chain
> +	)
> +'
> +
>  # configured prune tests
>  
>  set_config_tristate () {
> -- 
> gitgitgadget
>

Derrick Stolee Oct. 23, 2019, 8:46 p.m. UTC | #2

On 10/23/2019 10:18 AM, SZEDER Gábor wrote:
> On Wed, Oct 23, 2019 at 01:01:34PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <[e-mail]>
>>
>> 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.

>> Helped-by: Jeff King <[e-mail]>
>> Helped-by: Johannes Schindelin <[e-mail]>
>> Helped-by: Szeder Gábor <[e-mail]>
>> Signed-off-by: Derrick Stolee <[e-mail]>
>> ---
>>  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!

 
>> +		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

SZEDER Gábor Oct. 24, 2019, 12:18 p.m. UTC | #3

On Wed, Oct 23, 2019 at 01:01:34PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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" &&
> +		git commit -m "add submodule"
> +	) &&
> +	git clone "super" writeError &&
> +	(
> +		cd writeError &&
> +		test_path_is_missing .git/objects/info/commit-graphs/commit-graph-chain &&
> +		git -c fetch.writeCommitGraph=true fetch origin &&
> +		test_path_is_file .git/objects/info/commit-graphs/commit-graph-chain

With the fix applied this test fails when run with
GIT_TEST_COMMIT_GRAPH=1:

  + cd writeError
  + test_path_is_missing .git/objects/info/commit-graphs/commit-graph-chain
  + test -e .git/objects/info/commit-graphs/commit-graph-chain
  + git -c fetch.writeCommitGraph=true fetch origin
  + test_path_is_file .git/objects/info/commit-graphs/commit-graph-chain
  + test -f .git/objects/info/commit-graphs/commit-graph-chain
  + echo File .git/objects/info/commit-graphs/commit-graph-chain doesn't exist.
  File .git/objects/info/commit-graphs/commit-graph-chain doesn't exist.
  + false
  error: last command exited with $?=1

I think GIT_TEST_COMMIT_GRAPH should be unset for both
fetch.writeCommitGraph tests.

> +	)
> +'
> +
>  # configured prune tests
>  
>  set_config_tristate () {
> -- 
> gitgitgadget
>

Patch

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" &&
+		git commit -m "add submodule"
+	) &&
+	git clone "super" writeError &&
+	(
+		cd writeError &&
+		test_path_is_missing .git/objects/info/commit-graphs/commit-graph-chain &&
+		git -c fetch.writeCommitGraph=true fetch origin &&
+		test_path_is_file .git/objects/info/commit-graphs/commit-graph-chain
+	)
+'
+
 # configured prune tests
 
 set_config_tristate () {