Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

The commit-graph (and multi-pack-index) features are optional data structures that can make Git operations faster. Since they are optional, we do not enable them in most Git tests. The commit-graph is tested in t5318-commit-graph.sh (and t6600-test-reach.sh in ds/reachable), but that one script cannot cover the data shapes present in the rest of the test suite.

This patch introduces a new test environment variable, GIT_TEST_COMMIT_GRAPH . Similar to GIT_TEST_SPLIT_INDEX, it enables the commit-graph and writes it with every git commit command.

Thanks, Duy, for pointing out this direction

A few tests needed to be modified. These are the same tests that were mentioned in my previous example patch [2].

When this merges down, I'll create a CI build in VSTS that runs the test suite with this option enabled.

Thanks, -Stolee

[1]
https://public-inbox.org/git/CACsJy8CKnXVJYkKM_W=N=Vq-TVXf+YCqZP_uP7B-dN_6xddB=g@mail.gmail.com/
Re: [PATCH 0/9] multi-pack-index cleanups (Discussing test environment variables)

[2]
https://public-inbox.org/git/[email protected]/
[PATCH] DO-NOT-MERGE: write and read commit-graph always

Based-On: ds/commit-graph-with-grafts Cc: [email protected]

Derrick Stolee (1):
commit-graph: define GIT_TEST_COMMIT_GRAPH

 builtin/commit.c                    |  4 ++++
 commit-graph.c                      |  5 +++--
 commit-graph.h                      |  2 ++
 t/README                            |  4 ++++
 t/t0410-partial-clone.sh            |  2 +-
 t/t5307-pack-missing-commit.sh      | 10 ++++++++--
 t/t6011-rev-list-with-bad-commit.sh | 10 ++++++----
 t/t6024-recursive-merge.sh          |  9 ++++++---
 8 files changed, 34 insertions(+), 12 deletions(-)

base-commit: 829a321569d8e8f2c582aef9f0c990df976ab842
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-26%2Fderrickstolee%2Fshallow%2Ftest-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-26/derrickstolee/shallow/test-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/26
--
gitgitgadget

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

t/t0410-partial-clone.sh +0 -0

index 4984ca583d..73d5284a91 100755

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

Quoted 3 lines

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

So the plan is to turn on the commit graph for the whole test suite excluding these selected tests?

t/t6024-recursive-merge.sh git update-index a1 && · line 66

Quoted 3 lines

t/t6024-recursive-merge.sh +2 -0

+               GIT_TEST_COMMIT_GRAPH=0 &&
+               test_must_fail git merge -m final G

This could go on the same line without the && in between, setting the variable as a prefix.

Quoted 7 lines · Derrick Stolee via GitGitGadget

The commit-graph (and multi-pack-index) features are optional data structures that can make Git operations faster. Since they are optional, we do not enable them in most Git tests. The commit-graph is tested in t5318-commit-graph.sh (and t6600-test-reach.sh in ds/reachable), but that one script cannot cover the data shapes present in the rest of the test suite.

Quoted 4 lines

This patch introduces a new test environment variable, GIT_TEST_COMMIT_GRAPH . Similar to GIT_TEST_SPLIT_INDEX, it enables the commit-graph and writes it with every git commit command.
Thanks, Duy, for pointing out this direction

Did you mean to cc Duy (instead of me)? (I'll happily review the patch, too... just asking)

Quoted 6 lines · Stefan Beller

t/t6024-recursive-merge.sh git update-index a1 && · line 66

Quoted 2 lines · Derrick Stolee via GitGitGadget

t/t6024-recursive-merge.sh +2 -0

+               GIT_TEST_COMMIT_GRAPH=0 &&
+               test_must_fail git merge -m final G

This could go on the same line without the && in between, setting the variable as a prefix.

It cannot! The Linux build I ran complained that you can't put environment variables through test_must_fail.

Quoted 3 lines · Derrick Stolee

Quoted 2 lines · Stefan Beller

On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget <[email protected]> wrote:

Quoted 6 lines

Quoted 4 lines

t/t6024-recursive-merge.sh git update-index a1 && · line 66

Quoted 2 lines

t/t6024-recursive-merge.sh +2 -0

+               GIT_TEST_COMMIT_GRAPH=0 &&
+               test_must_fail git merge -m final G

This could go on the same line without the && in between, setting the variable as a prefix.

It cannot! The Linux build I ran complained that you can't put environment variables through test_must_fail.

Is GIT_TEST_COMMIT_GRAPH exported? If not, it won't have an impact on git-merge anyhow.

As for the special case of one-shot environment variable and test_must_fail(), you'll find "env" used as a workaround in a number of tests:

     test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge ... &&

The commit-graph (and multi-pack-index) features are optional data structures that can make Git operations faster. Since they are optional, we do not enable them in most Git tests. The commit-graph is tested in t5318-commit-graph.sh (and t6600-test-reach.sh in ds/reachable), but that one script cannot cover the data shapes present in the rest of the test suite.

This patch introduces a new test environment variable, GIT_TEST_COMMIT_GRAPH . Similar to GIT_TEST_SPLIT_INDEX, it enables the commit-graph and writes it with every git commit command. Thanks, Duy, for pointing out this direction [1].

A few tests needed to be modified. These are the same tests that were mentioned in my previous example patch [2]. Thanks, Eric, for providing the correct way to override the settings [3].

When this merges down, I'll create a CI build in VSTS that runs the test suite with this option enabled.

Quoted 7 lines · Derrick Stolee via GitGitGadget

The commit-graph (and multi-pack-index) features are optional data structures that can make Git operations faster. Since they are optional, we do not enable them in most Git tests. The commit-graph is tested in t5318-commit-graph.sh (and t6600-test-reach.sh in ds/reachable), but that one script cannot cover the data shapes present in the rest of the test suite.

Quoted 4 lines

This patch introduces a new test environment variable, GIT_TEST_COMMIT_GRAPH . Similar to GIT_TEST_SPLIT_INDEX, it enables the commit-graph and writes it with every git commit command. Thanks, Duy, for pointing out this direction [1].

Any reason to not add this new flag in ci/run-build-and-tests.sh (which is used by Travis)? I see your note about VSTS but I don't see why it has to be exclusive to VSTS.

Quoted 13 lines · Duy Nguyen

Quoted 11 lines · Derrick Stolee via GitGitGadget

The commit-graph (and multi-pack-index) features are optional data structures that can make Git operations faster. Since they are optional, we do not enable them in most Git tests. The commit-graph is tested in t5318-commit-graph.sh (and t6600-test-reach.sh in ds/reachable), but that one script cannot cover the data shapes present in the rest of the test suite.

This patch introduces a new test environment variable, GIT_TEST_COMMIT_GRAPH . Similar to GIT_TEST_SPLIT_INDEX, it enables the commit-graph and writes it with every git commit command. Thanks, Duy, for pointing out this direction [1].

Quoted 3 lines

Any reason to not add this new flag in ci/run-build-and-tests.sh (which is used by Travis)? I see your note about VSTS but I don't see why it has to be exclusive to VSTS.

I only wanted to volunteer resources that I know are available. I am looking into an additional test run in Travis CI builds, but didn't want to presume the extra resources would be available.

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

There is one test in t6024-recursive-merge.sh that relies on the merge-base algorithm picking one of two ambiguous merge-bases, and the commit-graph feature changes which merge-base is picked.

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

Quoted 8 lines

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

There is one test in t6024-recursive-merge.sh that relies on the merge-base algorithm picking one of two ambiguous merge-bases, and the commit-graph feature changes which merge-base is picked.

The test feature itself seems fine, but this consistently fails ever since it got introduced (a reset --hard on the commit merged to msater in git.git):

     GIT_TEST_COMMIT_GRAPH=true prove -j$(parallel --number-of-cores) t5500-fetch-pack.sh t6001-rev-list-graft.sh t6050-replace.sh
     Test Summary Report
     -------------------
     t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 6)
       Failed tests:  3, 5, 7, 9, 11, 13
       Non-zero exit status: 1
     t6050-replace.sh       (Wstat: 256 Tests: 35 Failed: 9)
       Failed tests:  12-16, 24-25, 30, 35
       Non-zero exit status: 1
     t5500-fetch-pack.sh    (Wstat: 256 Tests: 357 Failed: 1)
       Failed test:  351
       Non-zero exit status: 1

This is on Linux/Debian 4.17.0-1-amd64. Can you reproduce this? If not I can provide more info (-x output etc..).

Quoted 42 lines · Ævar Arnfjörð Bjarmason

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

Quoted 8 lines

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

There is one test in t6024-recursive-merge.sh that relies on the merge-base algorithm picking one of two ambiguous merge-bases, and the commit-graph feature changes which merge-base is picked.

The test feature itself seems fine, but this consistently fails ever since it got introduced (a reset --hard on the commit merged to msater in git.git):

     GIT_TEST_COMMIT_GRAPH=true prove -j$(parallel --number-of-cores) t5500-fetch-pack.sh t6001-rev-list-graft.sh t6050-replace.sh
     Test Summary Report
     -------------------
     t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 6)
       Failed tests:  3, 5, 7, 9, 11, 13
       Non-zero exit status: 1
     t6050-replace.sh       (Wstat: 256 Tests: 35 Failed: 9)
       Failed tests:  12-16, 24-25, 30, 35
       Non-zero exit status: 1
     t5500-fetch-pack.sh    (Wstat: 256 Tests: 357 Failed: 1)
       Failed test:  351
       Non-zero exit status: 1

Quoted 2 lines

This is on Linux/Debian 4.17.0-1-amd64. Can you reproduce this? If not I can provide more info (-x output etc..).

I see these failures, too, but I believe they are due to ds/commit-graph-with-grafts not being merged to 'next' yet. The purpose of that branch is to fix these test breaks. The environment variable got merged a lot faster.

I just built & tested the 'jch' branch at 515d82d9 with GIT_TEST_COMMIT_GRAPH=1 and they all passed.

Quoted 50 lines · Derrick Stolee

Quoted 42 lines · Ævar Arnfjörð Bjarmason

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The commit-graph feature is tested in isolation by t5318-commit-graph.sh and t6600-test-reach.sh, but there are many more interesting scenarios involving commit walks. Many of these scenarios are covered by the existing test suite, but we need to maintain coverage when the optional commit-graph structure is not present.

To allow running the full test suite with the commit-graph present, add a new test environment variable, GIT_TEST_COMMIT_GRAPH. Similar to GIT_TEST_SPLIT_INDEX, this variable makes every Git command try to load the commit-graph when parsing commits, and writes the commit-graph file after every 'git commit' command.

Quoted 8 lines

There are a few tests that rely on commits not existing in pack-files to trigger important events, so manually set GIT_TEST_COMMIT_GRAPH to false for the necessary commands.

There is one test in t6024-recursive-merge.sh that relies on the merge-base algorithm picking one of two ambiguous merge-bases, and the commit-graph feature changes which merge-base is picked.

The test feature itself seems fine, but this consistently fails ever since it got introduced (a reset --hard on the commit merged to msater in git.git):

      GIT_TEST_COMMIT_GRAPH=true prove -j$(parallel --number-of-cores) t5500-fetch-pack.sh t6001-rev-list-graft.sh t6050-replace.sh
      Test Summary Report
      -------------------
      t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 6)
        Failed tests:  3, 5, 7, 9, 11, 13
        Non-zero exit status: 1
      t6050-replace.sh       (Wstat: 256 Tests: 35 Failed: 9)
        Failed tests:  12-16, 24-25, 30, 35
        Non-zero exit status: 1
      t5500-fetch-pack.sh    (Wstat: 256 Tests: 357 Failed: 1)
        Failed test:  351
        Non-zero exit status: 1

Quoted 2 lines

This is on Linux/Debian 4.17.0-1-amd64. Can you reproduce this? If not I can provide more info (-x output etc..).

I see these failures, too, but I believe they are due to ds/commit-graph-with-grafts not being merged to 'next' yet. The purpose of that branch is to fix these test breaks. The environment variable got merged a lot faster.

Quoted 2 lines

I just built & tested the 'jch' branch at 515d82d9 with GIT_TEST_COMMIT_GRAPH=1 and they all passed.

I should have tested "pu" first. These failures are indeed fixed there. Thanks, and sorry about the noise.

I see these failures, too, but I believe they are due to ds/commit-graph-with-grafts not being merged to 'next' yet. The purpose of that branch is to fix these test breaks. The environment variable got merged a lot faster.