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")
> 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 ?
> + 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
>
