Re: [RFC PATCH 0/6] Fix commit-graph/graft/replace/shallow combo

[Derrick Stolee <stolee@gmail.com>]

On 5/31/2018 2:33 PM, Stefan Beller wrote:
> On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee <dstolee@microsoft.com> wrote:
>> The commit-graph file stores a condensed version of the commit history.
>> This helps speed up several operations involving commit walks. This
>> feature does not work well if those commits "change" using features like
>> commit grafts, replace objects, or shallow clones.
>>
>> Since the commit-graph feature is optional, hidden behind the
>> 'core.commitGraph' config option, and requires manual maintenance (until
>> ds/commit-graph-fsck' is merged), these issues only arise for expert
>> users who decided to opt-in.
>>
>> This RFC is a first attempt at rectify the issues that come up when
>> these features interact. I have not succeeded entirely, as I will
>> discuss below.
>>
>> The first two "DO NOT MERGE" commits are not intended to be part of the
>> main product, but are ways to help the full test suite run while
>> computing and consuming commit-graph files. This is acheived by calling
>> write_commit_graph_reachable() from `git fetch` and `git commit`. I
>> believe this is the only dependence on ds/commit-graph-fsck. The other
>> commits should only depend on ds/commit-graph-lockfile-fix.
> I applied these patches on top of ds/commit-graph-fsck
> (it would have been nice if you mentioned that it applies there)
> Running the test suite with all patches applied results in:
>
> ./t0410-partial-clone.sh                    (Wstat: 256 Tests: 15 Failed: 2)
>    Failed tests:  5, 8
> ./t5307-pack-missing-commit.sh              (Wstat: 256 Tests: 5 Failed: 2)
>    Failed tests:  3-4
> ./t5500-fetch-pack.sh                       (Wstat: 256 Tests: 353 Failed: 1)
>    Failed test:  348
> ./t6011-rev-list-with-bad-commit.sh         (Wstat: 256 Tests: 6 Failed: 1)
>    Failed test:  4
> ./t6024-recursive-merge.sh                  (Wstat: 256 Tests: 6 Failed: 1)
>    Failed test:  4
>
> which you identified as 4x noise and t5500 as not understood.
>
> $ GIT_TRACE=1 ./t5500-fetch-pack.sh -d -i -v -x
> [...]
> expecting success:
> git -C shallow12 fetch --shallow-exclude one origin &&
> git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
> test_write_lines three two >expected &&
> test_cmp expected actual
>
> ++ git -C shallow12 fetch --shallow-exclude one origin
> trace: built-in: git fetch --shallow-exclude one origin
> trace: run_command: unset GIT_PREFIX; 'git-upload-pack
> '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\'''
> trace: run_command: git --shallow-file  pack-objects --revs --thin
> --stdout --shallow --progress --delta-base-offset --include-tag
> trace: built-in: git pack-objects --revs --thin --stdout --shallow
> --progress --delta-base-offset --include-tag
> remote: Counting objects: 4, done.
> remote: Compressing objects: 100% (3/3), done.
> remote: Total 4 (delta 0), reused 0 (delta 0)
> trace: run_command: git --shallow-file  unpack-objects --pack_header=2,4
> trace: built-in: git unpack-objects --pack_header=2,4
> Unpacking objects: 100% (4/4), done.
> trace: run_command: git rev-list --objects --stdin --not --all --quiet
> trace: built-in: git rev-list --objects --stdin --not --all --quiet
> trace: run_command: unset GIT_PREFIX; 'git-upload-pack
> '\''/u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.'\'''
> trace: run_command: git pack-objects --revs --thin --stdout --progress
> --delta-base-offset
> trace: built-in: git pack-objects --revs --thin --stdout --progress
> --delta-base-offset
> remote: Total 0 (delta 0), reused 0 (delta 0)
> trace: run_command: git unpack-objects --pack_header=2,0
> trace: built-in: git unpack-objects --pack_header=2,0
> trace: run_command: git rev-list --objects --stdin --not --all --quiet
> trace: built-in: git rev-list --objects --stdin --not --all --quiet
>  From file:///u/git/t/trash directory.t5500-fetch-pack/shallow-exclude/.
>   * [new tag]         one        -> one
>   * [new tag]         two        -> two
> run_processes_parallel: preparing to run up to 1 tasks
> run_processes_parallel: done
> trace: run_command: git gc --auto
> trace: built-in: git gc --auto
> ++ git -C shallow12 log --pretty=tformat:%s origin/master
> trace: built-in: git log '--pretty=tformat:%s' origin/master
> ++ test_write_lines three two
> ++ printf '%s\n' three two
> ++ test_cmp expected actual
> ++ diff -u expected actual
> --- expected 2018-05-31 18:14:25.944540810 +0000
> +++ actual 2018-05-31 18:14:25.944540810 +0000
> @@ -1,2 +1,3 @@
>   three
>   two
> +one
> error: last command exited with $?=1
> not ok 348 - fetch exclude tag one
> #
> # git -C shallow12 fetch --shallow-exclude one origin &&
> # git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
> # test_write_lines three two >expected &&
> # test_cmp expected actual
> #
>
>
>> After these changes, there is one test case that still fails, and I
>> cannot understand why:
>>
>> t5500-fetch-pack.sh                     Failed test:  348
>>
>> This test fails when performing a `git fetch --shallow-exclude`. When I
>> halt the test using `t5500-fetch-pack.sh -d -i` and navigate to the
>> directory to replay the fetch it performs as expected.
> What is "as expected" ?
>
> When I insert a test_pause before that first line, such that:
>
> test_expect_success 'fetch exclude tag one' '
>      test_pause &&
>      git -C shallow12 fetch --shallow-exclude one origin &&
>      git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
>      test_write_lines three two >expected &&
>      test_cmp expected actual
> '
>
> and then run
>
>    rm "shallow-exclude/.git/objects/info/commit-graph
>
> the test works after exiting the test_pause.
>
> Note how the shallow-exclude is the *remote* of the fetch.
> So I think you also want to introduce the destruction
> of commit graphs in upload-pack.c which is the remote counter part to fetch.

Thanks for the details above. I agree that the solution is probably to 
disable the commit-graph during certain upload-pack parameters. 
Something is interfering there. I don't think the "destroy" pattern is 
right here.

> Why do you think these other tests are noice?

Take t5307-pack-missing-commit.sh for a typical example: a repo is 
constructed, a pack is made, and then that pack is manipulated to 
"remove" a commit. That missing commit is "detected" by running 
rev-list. Except if we are running the commit-graph on every 'git 
commit' call, the rev-list now succeeds since we are not looking at the 
packfile for the commit details any more!

This is what I mean by the tests being "noise". Adding the "DO NOT 
MERGE" commits only interrupted the test mechanism for unrelated behavior.

Thanks,

-Stolee