Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

PATCH Superseded Derrick Stolee via GitGitGadget 7 participants 18 messages Aug 28, 2018 – Oct 9, 2018

Share & tools

Derrick Stolee via GitGitGadget

author

2018-08-28 20:33 UTC #0 lore raw

cover 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 [1].

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

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2018-08-28 20:33 UTC #1 lore raw

PATCH 1/1 commit-graph: define GIT_TEST_COMMIT_GRAPH

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.

Signed-off-by Derrick Stolee <[email protected]>

---
 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(-)

builtin/commit.c +4 -0

index 158e3f843a..a25e652102 100644
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "mailmap.h"
 #include "help.h"
+#include "commit-graph.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),

@@ -1651,6 +1652,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "new_index file. Check that disk is not full and quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
+	if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
+		write_commit_graph_reachable(get_object_directory(), 0);
+
 	rerere(0);
 	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);

commit-graph.c +3 -2

index 4bd1a4abbf..5cae83fe03 100644
@@ -237,8 +237,9 @@ static int prepare_commit_graph(struct repository *r)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
 
-	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
-	    !config_value)
+	if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
+	    (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value))
 		/*
 		 * This repository is not configured to use commit graphs, so
 		 * do not load one. (But report commit_graph_attempted anyway

commit-graph.h +2 -0

index 13d736cdde..cf9141f356 100644
@@ -5,6 +5,8 @@
 #include "repository.h"
 #include "string-list.h"
 
+#define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+
 struct commit;
 
 char *get_commit_graph_filename(const char *obj_dir);

t/README +4 -0

index 8373a27fea..2b90c433f5 100644
@@ -315,6 +315,10 @@ packs on demand. This normally only happens when the object size is
 over 2GB. This variable forces the code path on any object larger than
 <n> bytes.
 
+GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to
+be written after every 'git commit' command, and overrides the
+'core.commitGraph' setting to true.
+
 Naming Tests
 ------------

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

index 4984ca583d..73d5284a91 100755
@@ -181,7 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
-	git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
+	GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out
 '

t/t5307-pack-missing-commit.sh +8 -2

index ae52a1882d..15df19a040 100755
@@ -24,11 +24,17 @@ test_expect_success 'check corruption' '
 '
 
 test_expect_success 'rev-list notices corruption (1)' '
-	test_must_fail git rev-list HEAD
+	(
+		GIT_TEST_COMMIT_GRAPH=0 &&
+		test_must_fail git rev-list HEAD
+	)
 '
 
 test_expect_success 'rev-list notices corruption (2)' '
-	test_must_fail git rev-list --objects HEAD
+	(
+		GIT_TEST_COMMIT_GRAPH=0 &&
+		test_must_fail git rev-list --objects HEAD
+	)
 '
 
 test_expect_success 'pack-objects notices corruption' '

t/t6011-rev-list-with-bad-commit.sh +6 -4

index e51eb41f4b..d774c69871 100755
@@ -41,10 +41,12 @@ test_expect_success 'corrupt second commit object' \
    test_must_fail git fsck --full
    '
 
-test_expect_success 'rev-list should fail' \
-   '
-   test_must_fail git rev-list --all > /dev/null
-   '
+test_expect_success 'rev-list should fail' '
+	(
+		GIT_TEST_COMMIT_GRAPH=0 &&
+		test_must_fail git rev-list --all > /dev/null
+	)
+'
 
 test_expect_success 'git repack _MUST_ fail' \
    '

t/t6024-recursive-merge.sh +6 -3

index 3f59e58dfb..04b30c5b76 100755
@@ -60,9 +60,12 @@ git update-index a1 &&
 GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 '
 
-test_expect_success "combined merge conflicts" "
-	test_must_fail git merge -m final G
-"
+test_expect_success 'combined merge conflicts' '
+	(
+		GIT_TEST_COMMIT_GRAPH=0 &&
+		test_must_fail git merge -m final G
+	)
+'
 
 cat > expect << EOF
 <<<<<<< HEAD

Signature · 1 line

gitgitgadget

Stefan Beller

2018-08-28 20:42 UTC #2 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 16 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?

Quoted 5 lines

t/t0410-partial-clone.sh +0 -0
index 4984ca583d..73d5284a91 100755

t/t0410-partial-clone.sh test_expect_success 'rev-list stops traversal at missing and promised commit' '

Quoted 9 lines

t/t0410-partial-clone.sh +3 -1

@@ -181,7 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
        git -C repo config core.repositoryformatversion 1 &&
        git -C repo config extensions.partialclone "arbitrary string" &&
-       git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
+       GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
 
 
+               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.

Derrick Stolee

2018-08-28 21:31 UTC #3 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 17 lines · Stefan Beller

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 5 lines

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?

Excluding these specific _steps_, but yes.

Quoted 5 lines

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.

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

-Stolee

Eric Sunshine

2018-08-28 21:59 UTC #4 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 10 lines · Derrick Stolee

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.

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 ... &&

Derrick Stolee

2018-08-29 12:14 UTC #5 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 4 lines · Eric Sunshine

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 8 lines

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.

In my testing this changed the behavior from fail to pass when passing GIT_TEST_COMMIT_GRAPH=1 from the command.

Quoted 5 lines

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 ... &&

Thanks for this! This is clearly the better solution.

-Stolee

Ævar Arnfjörð Bjarmason

2018-10-08 13:43 UTC #6 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

In reply to Derrick Stolee via GitGitGadget

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..).

Derrick Stolee

2018-10-08 14:45 UTC #7 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

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.

Thanks,
-Stolee

Ævar Arnfjörð Bjarmason

2018-10-08 14:58 UTC #8 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 45 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..).

Quoted 7 lines

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.

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

Derrick Stolee

2018-10-08 15:01 UTC #9 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

Quoted 52 lines · Ævar Arnfjörð Bjarmason

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 4 lines

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.

Thanks for testing with the optional features! It's good to keep them exercised.

Junio C Hamano

2018-10-09 05:53 UTC #10 lore raw

Re: [PATCH 1/1] commit-graph: define GIT_TEST_COMMIT_GRAPH

In reply to Derrick Stolee

Quoted 4 lines · Derrick Stolee

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.

A separate "ping" would have helped me. Will merge it down to 'next'.

Stefan Beller

2018-08-28 20:38 UTC #11 lore raw

In reply to Derrick Stolee via GitGitGadget

Quoted 8 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 5 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 40 lines

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

Derrick Stolee

2018-08-28 21:32 UTC #12 lore raw

Quoted 9 lines · Stefan Beller

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 6 lines

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)

I just added you because you've been on a lot of commit-graph things lately. But yes, I forgot to add Duy to the CC list. Added to this message.

Thanks,

-Stolee

Derrick Stolee via GitGitGadget

author

2018-08-29 12:49 UTC #13 lore raw

v2 cover Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

In reply to Derrick Stolee via GitGitGadget

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.

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

[3]
https://public-inbox.org/git/CAPig+cSjanDi=jV75PdzYpAjwVgd4Suh3UyvY+Vy7yeHAuY8RA@mail.gmail.com/

Based-On: ds/commit-graph-with-grafts Cc: [email protected]:
[email protected]: [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      | 4 ++--
 t/t6011-rev-list-with-bad-commit.sh | 7 +++----
 t/t6024-recursive-merge.sh          | 6 +++---
 8 files changed, 22 insertions(+), 12 deletions(-)

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

Range-diff vs v1:

1: 85d02ac8d8 ! 1: 4ff6695c7e commit-graph: define GIT_TEST_COMMIT_GRAPH

     @@ -23,6 +23,7 @@
          merge-base algorithm picking one of two ambiguous merge-bases, and
          the commit-graph feature changes which merge-base is picked.
      
     +    Helped-by: Eric Sunshine <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
      diff --git a/builtin/commit.c b/builtin/commit.c
     @@ -112,18 +113,12 @@
       
       test_expect_success 'rev-list notices corruption (1)' '
      -	test_must_fail git rev-list HEAD
     -+	(
     -+		GIT_TEST_COMMIT_GRAPH=0 &&
     -+		test_must_fail git rev-list HEAD
     -+	)
     ++	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list HEAD
       '
       
       test_expect_success 'rev-list notices corruption (2)' '
      -	test_must_fail git rev-list --objects HEAD
     -+	(
     -+		GIT_TEST_COMMIT_GRAPH=0 &&
     -+		test_must_fail git rev-list --objects HEAD
     -+	)
     ++	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --objects HEAD
       '
       
       test_expect_success 'pack-objects notices corruption' '
     @@ -140,10 +135,7 @@
      -   test_must_fail git rev-list --all > /dev/null
      -   '
      +test_expect_success 'rev-list should fail' '
     -+	(
     -+		GIT_TEST_COMMIT_GRAPH=0 &&
     -+		test_must_fail git rev-list --all > /dev/null
     -+	)
     ++	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null
      +'
       
       test_expect_success 'git repack _MUST_ fail' \
     @@ -160,10 +152,7 @@
      -	test_must_fail git merge -m final G
      -"
      +test_expect_success 'combined merge conflicts' '
     -+	(
     -+		GIT_TEST_COMMIT_GRAPH=0 &&
     -+		test_must_fail git merge -m final G
     -+	)
     ++	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge -m final G
      +'
       
       cat > expect << EOF

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2018-08-29 12:49 UTC #14 lore raw

PATCH v2 1/1 commit-graph: define GIT_TEST_COMMIT_GRAPH

From: Derrick Stolee <[email protected]>

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.

Helped-by Eric Sunshine <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 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      | 4 ++--
 t/t6011-rev-list-with-bad-commit.sh | 7 +++----
 t/t6024-recursive-merge.sh          | 6 +++---
 8 files changed, 22 insertions(+), 12 deletions(-)

builtin/commit.c +4 -0

index 158e3f843a..a25e652102 100644
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "mailmap.h"
 #include "help.h"
+#include "commit-graph.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),

@@ -1651,6 +1652,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "new_index file. Check that disk is not full and quota is\n"
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
+	if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
+		write_commit_graph_reachable(get_object_directory(), 0);
+
 	rerere(0);
 	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);

commit-graph.c +3 -2

index 4bd1a4abbf..5cae83fe03 100644
@@ -237,8 +237,9 @@ static int prepare_commit_graph(struct repository *r)
 		return !!r->objects->commit_graph;
 	r->objects->commit_graph_attempted = 1;
 
-	if (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
-	    !config_value)
+	if (!git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
+	    (repo_config_get_bool(r, "core.commitgraph", &config_value) ||
+	    !config_value))
 		/*
 		 * This repository is not configured to use commit graphs, so
 		 * do not load one. (But report commit_graph_attempted anyway

commit-graph.h +2 -0

index 13d736cdde..cf9141f356 100644
@@ -5,6 +5,8 @@
 #include "repository.h"
 #include "string-list.h"
 
+#define GIT_TEST_COMMIT_GRAPH "GIT_TEST_COMMIT_GRAPH"
+
 struct commit;
 
 char *get_commit_graph_filename(const char *obj_dir);

t/README +4 -0

index 8373a27fea..2b90c433f5 100644
@@ -315,6 +315,10 @@ packs on demand. This normally only happens when the object size is
 over 2GB. This variable forces the code path on any object larger than
 <n> bytes.
 
+GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to
+be written after every 'git commit' command, and overrides the
+'core.commitGraph' setting to true.
+
 Naming Tests
 ------------

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

index 4984ca583d..73d5284a91 100755
@@ -181,7 +181,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "arbitrary string" &&
-	git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
+	GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out
 '

t/t5307-pack-missing-commit.sh +2 -2

index ae52a1882d..dacb440b27 100755
@@ -24,11 +24,11 @@ test_expect_success 'check corruption' '
 '
 
 test_expect_success 'rev-list notices corruption (1)' '
-	test_must_fail git rev-list HEAD
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list HEAD
 '
 
 test_expect_success 'rev-list notices corruption (2)' '
-	test_must_fail git rev-list --objects HEAD
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --objects HEAD
 '
 
 test_expect_success 'pack-objects notices corruption' '

t/t6011-rev-list-with-bad-commit.sh +3 -4

index e51eb41f4b..545b461e51 100755
@@ -41,10 +41,9 @@ test_expect_success 'corrupt second commit object' \
    test_must_fail git fsck --full
    '
 
-test_expect_success 'rev-list should fail' \
-   '
-   test_must_fail git rev-list --all > /dev/null
-   '
+test_expect_success 'rev-list should fail' '
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null
+'
 
 test_expect_success 'git repack _MUST_ fail' \
    '

t/t6024-recursive-merge.sh +3 -3

index 3f59e58dfb..27c7de90ce 100755
@@ -60,9 +60,9 @@ git update-index a1 &&
 GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
 '
 
-test_expect_success "combined merge conflicts" "
-	test_must_fail git merge -m final G
-"
+test_expect_success 'combined merge conflicts' '
+	test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git merge -m final G
+'
 
 cat > expect << EOF
 <<<<<<< HEAD

Signature · 1 line

gitgitgadget

Duy Nguyen

2018-09-04 16:49 UTC #15 lore raw

In reply to Derrick Stolee via GitGitGadget

Quoted 8 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 6 lines

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.

Signature · 1 line

Duy

Derrick Stolee

2018-09-04 17:12 UTC #16 lore raw

Quoted 9 lines · Duy Nguyen

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 7 lines

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.

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.

Duy Nguyen

2018-09-04 17:18 UTC #17 lore raw

Quoted 14 lines · Derrick Stolee

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 8 lines

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.

OK I think there's a misunderstanding. You would not need a separate test run for this, instead the second (existing) run just activates both split index and commit-graph together. The run may be a bit slower (doubtful since commit-graph is all about speed) but it would not cost much more resources. Such a change would look like this

ci/run-build-and-tests.sh +1 -0

index 2a5bff4a1c..185305e3e2 100755
@@ -15,6 +15,7 @@ then
 	export GIT_TEST_FULL_IN_PACK_ARRAY=true
 	export GIT_TEST_OE_SIZE=10
 	export GIT_TEST_OE_DELTA_SIZE=5
+	export GIT_TEST_COMMIT_GRAPH=yes
 	make --quiet test
 fi

--
Duy