On 5/31/2018 2:33 PM, Stefan Beller wrote:
On Thu, May 31, 2018 at 10:40 AM, Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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
