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 <dstolee@xxxxxxxxxxxxx> >> >> 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 <peff@xxxxxxxx> >> Helped-by: Johannes Schindelin <johannes.schindelin@xxxxxx> >> Helped-by: Szeder Gábor <szeder.dev@xxxxxxxxx> >> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> --- >> 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
