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

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

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

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

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

[Derrick Stolee via GitGitGadget]

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

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

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

[Stefan Beller]

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?


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

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

[Derrick Stolee]

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:
>> 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.
>
>> +               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

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

[Eric Sunshine]

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.

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

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

[Derrick Stolee]

On 8/28/2018 5:59 PM, Eric Sunshine wrote:
> 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.


> 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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Derrick Stolee]

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.

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Derrick Stolee]

On 10/8/2018 10:58 AM, Ævar Arnfjörð Bjarmason wrote:
> 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.

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

[Junio C Hamano]

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

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

[Stefan Beller]

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)

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

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

[Derrick Stolee]

On 8/28/2018 4:37 PM, Stefan Beller wrote:
> 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

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

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

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

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

[Derrick Stolee via GitGitGadget]

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

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

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

[Duy Nguyen]

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.

> 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

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

[Derrick Stolee]

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.

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

[Duy Nguyen]

On Tue, Sep 04, 2018 at 01:12:55PM -0400, Derrick Stolee wrote:
> 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

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