threads / patch / 49244

patchDefine GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

Subject: [PATCH 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

## tl;dr

18 messages between Aug 28, 2018 and Oct 9, 2018. Diffs are folded; open one to read it.

replies: 17people: 6as markdown or json

Derrick Stolee via GitGitGadget· Aug 28, 2018, 20:33 UTC · lore

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/20180718152244.45513-1-dstolee@microsoft.com/ [PATCH] DO-NOT-MERGE: write and read commit-graph always

Based-On: ds/commit-graph-with-grafts Cc: jnareb@gmail.com
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 via GitGitGadget· Aug 28, 2018, 20:33 UTC · re: Derrick Stolee via GitGitGadget · lore

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

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 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(-)
Show changes to 8 files +34 −12

builtin/commit.c, commit-graph.c, commit-graph.h, t/README, t/t0410-partial-clone.sh, t/t5307-pack-missing-commit.sh, t/t6011-rev-list-with-bad-commit.sh, t/t6024-recursive-merge.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index 158e3f843a..a25e652102 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/commit-graph.c b/commit-graph.c
index 4bd1a4abbf..5cae83fe03 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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
diff --git a/commit-graph.h b/commit-graph.h
index 13d736cdde..cf9141f356 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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);
diff --git a/t/README b/t/README
index 8373a27fea..2b90c433f5 100644
--- a/t/README
+++ b/t/README
@@ -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
 ------------
 
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 4984ca583d..73d5284a91 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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
 '
diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
index ae52a1882d..15df19a040 100755
--- a/t/t5307-pack-missing-commit.sh
+++ b/t/t5307-pack-missing-commit.sh
@@ -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' '
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index e51eb41f4b..d774c69871 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -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' \
    '
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 3f59e58dfb..04b30c5b76 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -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
-- 
gitgitgadget
Stefan Beller· Aug 28, 2018, 20:41 UTC · re: Derrick Stolee via GitGitGadget · lore

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

On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 19 quoted lines
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

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

Show 10 quoted lines
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 4984ca583d..73d5284a91 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -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· Aug 28, 2018, 21:31 UTC · re: Stefan Beller · lore

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

On 8/28/2018 4:41 PM, Stefan Beller wrote:
Show 22 quoted lines
> On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
> 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.
Show 5 quoted lines
>
>> +               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· Aug 28, 2018, 21:59 UTC · re: Derrick Stolee · lore

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

On Tue, Aug 28, 2018 at 5:31 PM Derrick Stolee <stolee@gmail.com> wrote:
Show 10 quoted lines
> On 8/28/2018 4:41 PM, Stefan Beller wrote:
> > On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +               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· Aug 29, 2018, 12:14 UTC · re: Eric Sunshine · lore

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

On 8/28/2018 5:59 PM, Eric Sunshine wrote:
Show 12 quoted lines
> On Tue, Aug 28, 2018 at 5:31 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 8/28/2018 4:41 PM, Stefan Beller wrote:
>>> On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>>>> +               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.

Show 5 quoted 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· Oct 8, 2018, 13:43 UTC · re: Derrick Stolee via GitGitGadget · lore

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

On Tue, Aug 28 2018, Derrick Stolee via GitGitGadget wrote:
Show 23 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.
>

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· Oct 8, 2018, 14:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 10/8/2018 9:43 AM, Ævar Arnfjörð Bjarmason wrote:
Show 44 quoted lines
> On Tue, Aug 28 2018, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
>>
> 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..).

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· Oct 8, 2018, 14:58 UTC · re: Derrick Stolee · lore

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

On Mon, Oct 08 2018, Derrick Stolee wrote:
Show 52 quoted lines
> On 10/8/2018 9:43 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Aug 28 2018, Derrick Stolee via GitGitGadget wrote:
>>
>>> From: Derrick Stolee <dstolee@microsoft.com>
>>>
>>> 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.
>>>
>> 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..).
> 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· Oct 8, 2018, 15:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 10/8/2018 10:58 AM, Ævar Arnfjörð Bjarmason wrote:
Show 56 quoted lines
> On Mon, Oct 08 2018, Derrick Stolee wrote:
>
>> On 10/8/2018 9:43 AM, Ævar Arnfjörð Bjarmason wrote:
>>> On Tue, Aug 28 2018, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> From: Derrick Stolee <dstolee@microsoft.com>
>>>>
>>>> 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.
>>>>
>>> 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..).
>> 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.

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

Junio C Hamano· Oct 9, 2018, 05:53 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <stolee@gmail.com> writes:
> 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· Aug 28, 2018, 20:37 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines
>
> 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

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

Show 40 quoted 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/20180718152244.45513-1-dstolee@microsoft.com/
> [PATCH] DO-NOT-MERGE: write and read commit-graph always
>
> Based-On: ds/commit-graph-with-grafts Cc: jnareb@gmail.com
>
> 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· Aug 28, 2018, 21:32 UTC · re: Stefan Beller · lore

Re: [PATCH 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

On 8/28/2018 4:37 PM, Stefan Beller wrote:
Show 15 quoted lines
> On Tue, Aug 28, 2018 at 1:33 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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
> 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· Aug 29, 2018, 12:49 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 0/1] 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]. 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/20180718152244.45513-1-dstolee@microsoft.com/ [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: jnareb@gmail.comCc: 
sbeller@google.comCc: sunshine@sunshineco.com
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 <sunshine@sunshineco.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
      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
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Aug 29, 2018, 12:49 UTC · re: Derrick Stolee via GitGitGadget · lore

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

From: Derrick Stolee <dstolee@microsoft.com>

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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(-)
Show changes to 8 files +22 −12

builtin/commit.c, commit-graph.c, commit-graph.h, t/README, t/t0410-partial-clone.sh, t/t5307-pack-missing-commit.sh, t/t6011-rev-list-with-bad-commit.sh, t/t6024-recursive-merge.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index 158e3f843a..a25e652102 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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);
diff --git a/commit-graph.c b/commit-graph.c
index 4bd1a4abbf..5cae83fe03 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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
diff --git a/commit-graph.h b/commit-graph.h
index 13d736cdde..cf9141f356 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -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);
diff --git a/t/README b/t/README
index 8373a27fea..2b90c433f5 100644
--- a/t/README
+++ b/t/README
@@ -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
 ------------
 
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 4984ca583d..73d5284a91 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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
 '
diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh
index ae52a1882d..dacb440b27 100755
--- a/t/t5307-pack-missing-commit.sh
+++ b/t/t5307-pack-missing-commit.sh
@@ -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' '
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index e51eb41f4b..545b461e51 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -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' \
    '
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 3f59e58dfb..27c7de90ce 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -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
-- 
gitgitgadget
Duy Nguyen· Sep 4, 2018, 16:49 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v2 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

On Wed, Aug 29, 2018 at 2:49 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines
>
> 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].

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.

Show 6 quoted 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.
-- 
Duy
Derrick Stolee· Sep 4, 2018, 17:12 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

On 9/4/2018 12:49 PM, Duy Nguyen wrote:
Show 16 quoted lines
> On Wed, Aug 29, 2018 at 2:49 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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].
> 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· Sep 4, 2018, 17:18 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 0/1] Define GIT_TEST_COMMIT_GRAPH for commit-graph test coverage

On Tue, Sep 04, 2018 at 01:12:55PM -0400, Derrick Stolee wrote:
Show 22 quoted lines
> On 9/4/2018 12:49 PM, Duy Nguyen wrote:
> > On Wed, Aug 29, 2018 at 2:49 PM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> 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].
> > 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

Show changes to ci/run-build-and-tests.sh +1 −1
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2a5bff4a1c..185305e3e2 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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

← back to recent threads