* [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper
2021-10-14 23:37 [PATCH 0/3] commit-graph: make "verify" work with replace refs Ævar Arnfjörð Bjarmason
@ 2021-10-14 23:37 ` Ævar Arnfjörð Bjarmason
2021-10-15 16:03 ` Junio C Hamano
2021-10-14 23:37 ` [PATCH 2/3] commit-graph tests: fix another " Ævar Arnfjörð Bjarmason
2021-10-14 23:37 ` [PATCH 3/3] commit-graph: don't consider "replace" objects with "verify" Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 23:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Derrick Stolee,
Ævar Arnfjörð Bjarmason
The graph_git_two_modes() helper added in 177722b3442 (commit:
integrate commit graph with commit parsing, 2018-04-10) didn't
&&-chain its "git commit-graph" invocations, which as can be seen with
SANITIZE=leak will happily mark tests as passing if both of these
commands die, since test_cmp() will be comparing two empty files.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t5318-commit-graph.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 295c5bd94d2..88fbe004a38 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -70,8 +70,8 @@ test_expect_success 'create commits and repack' '
'
graph_git_two_modes() {
- git -c core.commitGraph=true $1 >output
- git -c core.commitGraph=false $1 >expect
+ git -c core.commitGraph=true $1 >output &&
+ git -c core.commitGraph=false $1 >expect &&
test_cmp expect output
}
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper
2021-10-14 23:37 ` [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper Ævar Arnfjörð Bjarmason
@ 2021-10-15 16:03 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2021-10-15 16:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Glen Choo, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The graph_git_two_modes() helper added in 177722b3442 (commit:
> integrate commit graph with commit parsing, 2018-04-10) didn't
> &&-chain its "git commit-graph" invocations, which as can be seen with
> SANITIZE=leak will happily mark tests as passing if both of these
> commands die, since test_cmp() will be comparing two empty files.
As the chains the four invocation of this helper with &&- correctly,
this does make a difference. Nicely spotted.
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> t/t5318-commit-graph.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 295c5bd94d2..88fbe004a38 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -70,8 +70,8 @@ test_expect_success 'create commits and repack' '
> '
>
> graph_git_two_modes() {
> - git -c core.commitGraph=true $1 >output
> - git -c core.commitGraph=false $1 >expect
> + git -c core.commitGraph=true $1 >output &&
> + git -c core.commitGraph=false $1 >expect &&
> test_cmp expect output
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] commit-graph tests: fix another graph_git_two_modes() helper
2021-10-14 23:37 [PATCH 0/3] commit-graph: make "verify" work with replace refs Ævar Arnfjörð Bjarmason
2021-10-14 23:37 ` [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper Ævar Arnfjörð Bjarmason
@ 2021-10-14 23:37 ` Ævar Arnfjörð Bjarmason
2021-10-15 16:15 ` Junio C Hamano
2021-10-14 23:37 ` [PATCH 3/3] commit-graph: don't consider "replace" objects with "verify" Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 23:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Derrick Stolee,
Ævar Arnfjörð Bjarmason
In 135a7123755 (commit-graph: add --split option to builtin,
2019-06-18) this function was copy/pasted to the split commit-graph
tests, as in the preceding commit we need to fix this to use
&&-chaining, so it won't be hiding errors.
Unlike its sister function in "t5318-commit-graph.sh", which we got
lucky with, this one was hiding a real test failure. A tests added in
c523035cbd8 (commit-graph: allow cross-alternate chains, 2019-06-18)
has never worked as intended. Unlike most other graph_git_behavior
uses in this file it clones the repository into a sub-directory, so
we'll need to refer to "commits/6" as "origin/commits/6".
It's not easy to simply move the "graph_git_behavior" to the test
above it, since it itself spawns a "test_expect_success". Let's
instead add support to "graph_git_behavior()" and
"graph_git_two_modes()" to pass a "-C" argument to git.
We also need to add a "test -d fork" here, because otherwise we'll
fail on e.g.:
GIT_SKIP_TESTS=t5324.13 ./t5324-split-commit-graph.sh
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t5324-split-commit-graph.sh | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 587226ed103..847b8097109 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -55,8 +55,8 @@ test_expect_success 'create commits and write commit-graph' '
'
graph_git_two_modes() {
- git -c core.commitGraph=true $1 >output
- git -c core.commitGraph=false $1 >expect
+ git ${2:+ -C "$2"} -c core.commitGraph=true $1 >output &&
+ git ${2:+ -C "$2"} -c core.commitGraph=false $1 >expect &&
test_cmp expect output
}
@@ -64,12 +64,13 @@ graph_git_behavior() {
MSG=$1
BRANCH=$2
COMPARE=$3
+ DIR=$4
test_expect_success "check normal git operations: $MSG" '
- graph_git_two_modes "log --oneline $BRANCH" &&
- graph_git_two_modes "log --topo-order $BRANCH" &&
- graph_git_two_modes "log --graph $COMPARE..$BRANCH" &&
- graph_git_two_modes "branch -vv" &&
- graph_git_two_modes "merge-base -a $BRANCH $COMPARE"
+ graph_git_two_modes "log --oneline $BRANCH" "$DIR" &&
+ graph_git_two_modes "log --topo-order $BRANCH" "$DIR" &&
+ graph_git_two_modes "log --graph $COMPARE..$BRANCH" "$DIR" &&
+ graph_git_two_modes "branch -vv" "$DIR" &&
+ graph_git_two_modes "merge-base -a $BRANCH $COMPARE" "$DIR"
'
}
@@ -187,7 +188,10 @@ test_expect_success 'create fork and chain across alternate' '
)
'
-graph_git_behavior 'alternate: commit 13 vs 6' commits/13 commits/6
+if test -d fork
+then
+ graph_git_behavior 'alternate: commit 13 vs 6' commits/13 origin/commits/6 "fork"
+fi
test_expect_success 'test merge stragety constants' '
git clone . merge-2 &&
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] commit-graph tests: fix another graph_git_two_modes() helper
2021-10-14 23:37 ` [PATCH 2/3] commit-graph tests: fix another " Ævar Arnfjörð Bjarmason
@ 2021-10-15 16:15 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2021-10-15 16:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Glen Choo, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> In 135a7123755 (commit-graph: add --split option to builtin,
> 2019-06-18) this function was copy/pasted to the split commit-graph
> tests, as in the preceding commit we need to fix this to use
> &&-chaining, so it won't be hiding errors.
>
> Unlike its sister function in "t5318-commit-graph.sh", which we got
> lucky with, this one was hiding a real test failure. A tests added in
> c523035cbd8 (commit-graph: allow cross-alternate chains, 2019-06-18)
> has never worked as intended. Unlike most other graph_git_behavior
> uses in this file it clones the repository into a sub-directory, so
> we'll need to refer to "commits/6" as "origin/commits/6".
Interesting. The original created "fork" prepared the alternates
structure in the preceding test, but tested the "behavour" of the
commands outside the "fork" it just prepared?
> graph_git_two_modes() {
> - git -c core.commitGraph=true $1 >output
> - git -c core.commitGraph=false $1 >expect
> + git ${2:+ -C "$2"} -c core.commitGraph=true $1 >output &&
> + git ${2:+ -C "$2"} -c core.commitGraph=false $1 >expect &&
OK, it was a bit curious to see :+ (instead of just +), but the
caller unconditionally passes "$DIR" (with double quotes), so it is
understandable. Much more concise than having the caller to repeat
${4+"$4"} where it says "$DIR".
> test_cmp expect output
> }
>
> @@ -64,12 +64,13 @@ graph_git_behavior() {
> MSG=$1
> BRANCH=$2
> COMPARE=$3
> + DIR=$4
> test_expect_success "check normal git operations: $MSG" '
> - graph_git_two_modes "log --oneline $BRANCH" &&
> - graph_git_two_modes "log --topo-order $BRANCH" &&
> - graph_git_two_modes "log --graph $COMPARE..$BRANCH" &&
> - graph_git_two_modes "branch -vv" &&
> - graph_git_two_modes "merge-base -a $BRANCH $COMPARE"
> + graph_git_two_modes "log --oneline $BRANCH" "$DIR" &&
> + graph_git_two_modes "log --topo-order $BRANCH" "$DIR" &&
> + graph_git_two_modes "log --graph $COMPARE..$BRANCH" "$DIR" &&
> + graph_git_two_modes "branch -vv" "$DIR" &&
> + graph_git_two_modes "merge-base -a $BRANCH $COMPARE" "$DIR"
> '
> }
>
> @@ -187,7 +188,10 @@ test_expect_success 'create fork and chain across alternate' '
> )
> '
>
> -graph_git_behavior 'alternate: commit 13 vs 6' commits/13 commits/6
> +if test -d fork
> +then
> + graph_git_behavior 'alternate: commit 13 vs 6' commits/13 origin/commits/6 "fork"
> +fi
>
> test_expect_success 'test merge stragety constants' '
> git clone . merge-2 &&
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] commit-graph: don't consider "replace" objects with "verify"
2021-10-14 23:37 [PATCH 0/3] commit-graph: make "verify" work with replace refs Ævar Arnfjörð Bjarmason
2021-10-14 23:37 ` [PATCH 1/3] commit-graph tests: fix error-hiding graph_git_two_modes() helper Ævar Arnfjörð Bjarmason
2021-10-14 23:37 ` [PATCH 2/3] commit-graph tests: fix another " Ævar Arnfjörð Bjarmason
@ 2021-10-14 23:37 ` Ævar Arnfjörð Bjarmason
2021-10-15 16:18 ` Junio C Hamano
2 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 23:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Glen Choo, Derrick Stolee,
Ævar Arnfjörð Bjarmason
Extend the code added in d6538246d3d (commit-graph: not compatible
with replace objects, 2018-08-20) which ignored replace objects in the
"write" command to ignore it in the "verify" command too.
We can just move this assignment to the cmd_commit_graph(), it
dispatches to "write" and "verify", and we're unlikely to ever get a
sub-command that would like to consider replace refs.
This will make tests added in eddc1f556cd (mktag tests: test
update-ref and reachable fsck, 2021-06-17) pass in combination with
the "GIT_TEST_COMMIT_GRAPH" mode added in 859fdc0c3cf (commit-graph:
define GIT_TEST_COMMIT_GRAPH, 2018-08-29), except that mode is
currently broken (but is being fixed concurrently). See the discussion
starting at [1].
1. https://lore.kernel.org/git/87wnmihswp.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 2 +-
t/t5318-commit-graph.sh | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3c3de3a156f..fb8e166a26f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -263,7 +263,6 @@ static int graph_write(int argc, const char **argv)
git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0))
flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
- read_replace_refs = 0;
odb = find_odb(the_repository, opts.obj_dir);
if (opts.reachable) {
@@ -318,6 +317,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
if (!argc)
goto usage;
+ read_replace_refs = 0;
save_commit_buffer = 0;
if (!strcmp(argv[0], "verify"))
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 88fbe004a38..84d122a7ae7 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -385,6 +385,7 @@ test_expect_success 'replace-objects invalidates commit-graph' '
git commit-graph write --reachable &&
test_path_is_file .git/objects/info/commit-graph &&
git replace HEAD~1 HEAD~2 &&
+ graph_git_two_modes "commit-graph verify" &&
git -c core.commitGraph=false log >expect &&
git -c core.commitGraph=true log >actual &&
test_cmp expect actual &&
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] commit-graph: don't consider "replace" objects with "verify"
2021-10-14 23:37 ` [PATCH 3/3] commit-graph: don't consider "replace" objects with "verify" Ævar Arnfjörð Bjarmason
@ 2021-10-15 16:18 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2021-10-15 16:18 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Glen Choo, Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Extend the code added in d6538246d3d (commit-graph: not compatible
> with replace objects, 2018-08-20) which ignored replace objects in the
> "write" command to ignore it in the "verify" command too.
>
> We can just move this assignment to the cmd_commit_graph(), it
> dispatches to "write" and "verify", and we're unlikely to ever get a
> sub-command that would like to consider replace refs.
>
> This will make tests added in eddc1f556cd (mktag tests: test
> update-ref and reachable fsck, 2021-06-17) pass in combination with
> the "GIT_TEST_COMMIT_GRAPH" mode added in 859fdc0c3cf (commit-graph:
> define GIT_TEST_COMMIT_GRAPH, 2018-08-29), except that mode is
> currently broken (but is being fixed concurrently). See the discussion
> starting at [1].
>
> 1. https://lore.kernel.org/git/87wnmihswp.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/commit-graph.c | 2 +-
> t/t5318-commit-graph.sh | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 3c3de3a156f..fb8e166a26f 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -263,7 +263,6 @@ static int graph_write(int argc, const char **argv)
> git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0))
> flags |= COMMIT_GRAPH_WRITE_BLOOM_FILTERS;
>
> - read_replace_refs = 0;
> odb = find_odb(the_repository, opts.obj_dir);
>
> if (opts.reachable) {
> @@ -318,6 +317,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
> if (!argc)
> goto usage;
>
> + read_replace_refs = 0;
> save_commit_buffer = 0;
>
> if (!strcmp(argv[0], "verify"))
OK. The only question I have is if this deserves some kind of a
warning. If the user has replacement defined and makes an explicit
request to work with the commit-graph, silently ignoring the request
instead of telling them why we are ignoring may lead to confusion.
Side note. It is a "question", not "objection".
Other than that, nicely done.
Thanks.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 88fbe004a38..84d122a7ae7 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -385,6 +385,7 @@ test_expect_success 'replace-objects invalidates commit-graph' '
> git commit-graph write --reachable &&
> test_path_is_file .git/objects/info/commit-graph &&
> git replace HEAD~1 HEAD~2 &&
> + graph_git_two_modes "commit-graph verify" &&
> git -c core.commitGraph=false log >expect &&
> git -c core.commitGraph=true log >actual &&
> test_cmp expect actual &&
^ permalink raw reply [flat|nested] 7+ messages in thread