commit-graph: fix buggy --expire-time option
PATCH 1/1 commit-graph: fix buggy --expire-time option
From: Derrick Stolee <[email protected]>
The commit-graph builtin has an --expire-time option that takes a datetime using OPT_EXPIRY_DATE(). However, the implementation inside expire_commit_graphs() was treating a non-zero value as a number of seconds to subtract from "now".
Update t5323-split-commit-graph.sh to demonstrate the correct value of the --expire-time option by actually creating a crud .graph file with mtime earlier than the expire time. Instead of using a super- early time (1980) we need to use a recent time or else the old logic actually passes by accident. This test will start passing again on the old logic in 40 years or so.
I noticed this when inspecting some Scalar repos that had an excess number of commit-graph files. In Scalar, we were using this second interpretation by using "--expire-time=3600" to mean "delete graphs older than one hour ago" to avoid deleting a commit-graph that a foreground process may be trying to load.
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
---
commit-graph: fix buggy --expire-time option
This is embarassing. I should have noticed this when writing it the first time, or when integrating the feature into Scalar and VFS for Git. Sorry!
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/596
builtin/commit-graph.c | 2 +- commit-graph.c | 2 +- t/t5324-split-commit-graph.sh | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-)
builtin/commit-graph.c +1 -1
@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, -N_ ("maximum number of commits in a non-base split commit-graph" )), +N_ ("do not expire files newer than a number of seconds before now" )), OPT_END(), };
commit-graph.c +1 -1
@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)timestamp_t expire_time =time (NULL );if (ctx->split_opts && ctx->split_opts->expire_time) - expire_time -= ctx->split_opts->expire_time; + expire_time = ctx->split_opts->expire_time;if (!ctx->split) {char *chain_file_name =get_chain_filename (ctx->odb);unlink (chain_file_name);
t/t5324-split-commit-graph.sh +3 -1
@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph && + git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 && test_line_count = 1 $graphdir/commit-graph-chain && + test_path_is_missing $graphdir/extra.graph && ls $graphdir/graph-*.graph >graph-files && test_line_count = 3 graph-files ) &&
base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Signature · 1 line
gitgitgadget
Quoted 2 lines · Derrick Stolee via GitGitGadget
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
...
builtin/commit-graph.c static int graph_write(int argc, const char **argv) · line 143
Quoted 10 lines
builtin/commit-graph.c +1
-1@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, - N_("maximum number of commits in a non-base split commit-graph")), + N_("do not expire files newer than a number of seconds before now")),
and of course I messed this up even now. Should be:
builtin/commit-graph.c static int graph_write(int argc, const char **argv) · line 144
Quoted 1 line
builtin/commit-graph.c +1
-0+ N_("do not expire files newer than a given date-time")),
-Stolee
Quoted 17 lines · Derrick Stolee
Quoted 2 lines · Derrick Stolee via GitGitGadget
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
...
builtin/commit-graph.c static int graph_write(int argc, const char **argv) · line 143
Quoted 10 lines
builtin/commit-graph.c +1
-1@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, - N_("maximum number of commits in a non-base split commit-graph")), + N_("do not expire files newer than a number of seconds before now")),and of course I messed this up even now. Should be:
Quoted 1 line
builtin/commit-graph.c static int graph_write(int argc, const char **argv) · line 144
Quoted 1 line
builtin/commit-graph.c +1
-0+ N_("do not expire files newer than a given date-time")),
I wonder if the double-negative can be avoided. Perhaps this should be instead:
expire files older than the given date-time
or similar.
Quoted 1 line
-Stolee
Thanks,
Taylor
Quoted 6 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The commit-graph builtin has an --expire-time option that takes a datetime using
OPT_EXPIRY_DATE(). However, the implementation insideexpire_commit_graphs()was treating a non-zero value as a number of seconds to subtract from "now".
Ah, nice catch. The fix below seems straightforward. Since we're using 'OPT_EXPIRY_DATE', parse-options accepts either, but gives us back a date-time, so there's no need to subtract it from the current instant or anything like that.
This patch below looks fine (I left a minor note here, and in the documentation fixup that you sent shortly after this message, but otherwise I think that this is good to go, at least from my point of view).
Quoted 66 lines
Update
t5323-split-commit-graph.shto demonstrate the correct value of the --expire-time option by actually creating a crud .graph file with mtime earlier than the expire time. Instead of using a super- early time (1980) we need to use a recent time or else the old logic actually passes by accident. This test will start passing again on the old logic in 40 years or so.I noticed this when inspecting some Scalar repos that had an excess number of commit-graph files. In Scalar, we were using this second interpretation by using "--expire-time=3600" to mean "delete graphs older than one hour ago" to avoid deleting a commit-graph that a foreground process may be trying to load.
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
Signed-off-by Derrick Stolee <[email protected]>---
commit-graph: fix buggy --expire-time optionThis is embarassing. I should have noticed this when writing it the first time, or when integrating the feature into Scalar and VFS for Git. Sorry!
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/596builtin/commit-graph.c | 2 +- commit-graph.c | 2 +- t/t5324-split-commit-graph.sh | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-)builtin/commit-graph.c +1
-1@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, -N_ ("maximum number of commits in a non-base split commit-graph" )), +N_ ("do not expire files newer than a number of seconds before now" )), OPT_END(), };commit-graph.c +1
-1@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)timestamp_t expire_time =time (NULL );if (ctx->split_opts && ctx->split_opts->expire_time) - expire_time -= ctx->split_opts->expire_time; + expire_time = ctx->split_opts->expire_time;if (!ctx->split) {char *chain_file_name =get_chain_filename (ctx->odb);unlink (chain_file_name);t/t5324-split-commit-graph.sh +0
-0
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 214
Quoted 6 lines
t/t5324-split-commit-graph.sh +2
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph && + git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&
Could this be written instead as:
touch -m -t $(date -r $test_tick +"%Y%m%d%H%M.%S") $graphdir/extra.graph && test_tick && git commit-graph write --reachable --split --size-multiple=10 --expire-time=now &&
to avoid breaking in 40 years?
Quoted 9 lines
t/t5324-split-commit-graph.sh +1
-0test_line_count = 1 $graphdir/commit-graph-chain && + test_path_is_missing $graphdir/extra.graph && ls $graphdir/graph-*.graph >graph-files && test_line_count = 3 graph-files ) &&base-commit:
274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925--
gitgitgadget
Thanks,
Taylor
Quoted 9 lines · Taylor Blau
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 214
Quoted 3 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +2
-1- git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph && + git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&Could this be written instead as:
touch -m -t $(date -r $test_tick +"%Y%m%d%H%M.%S") $graphdir/extra.graph && test_tick &&
This won't work with GNU 'date' command which doesn't understand "-r<number>" (it understands only "-r<reference-file>").
Quoted 73 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The commit-graph builtin has an --expire-time option that takes a datetime using
OPT_EXPIRY_DATE(). However, the implementation insideexpire_commit_graphs()was treating a non-zero value as a number of seconds to subtract from "now".Update
t5323-split-commit-graph.shto demonstrate the correct value of the --expire-time option by actually creating a crud .graph file with mtime earlier than the expire time. Instead of using a super- early time (1980) we need to use a recent time or else the old logic actually passes by accident. This test will start passing again on the old logic in 40 years or so.I noticed this when inspecting some Scalar repos that had an excess number of commit-graph files. In Scalar, we were using this second interpretation by using "--expire-time=3600" to mean "delete graphs older than one hour ago" to avoid deleting a commit-graph that a foreground process may be trying to load.
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
Signed-off-by Derrick Stolee <[email protected]>---
commit-graph: fix buggy --expire-time optionThis is embarassing. I should have noticed this when writing it the first time, or when integrating the feature into Scalar and VFS for Git. Sorry!
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/596builtin/commit-graph.c | 2 +- commit-graph.c | 2 +- t/t5324-split-commit-graph.sh | 4 +++- 3 files changed, 5 insertions(+), 3 deletions(-)builtin/commit-graph.c +1
-1@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, -N_ ("maximum number of commits in a non-base split commit-graph" )), +N_ ("do not expire files newer than a number of seconds before now" )), OPT_END(), };commit-graph.c +1
-1@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)timestamp_t expire_time =time (NULL );if (ctx->split_opts && ctx->split_opts->expire_time) - expire_time -= ctx->split_opts->expire_time; + expire_time = ctx->split_opts->expire_time;if (!ctx->split) {char *chain_file_name =get_chain_filename (ctx->odb);unlink (chain_file_name);t/t5324-split-commit-graph.sh +0
-0
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 5 lines
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&
You can set a mtime relative to the current time with:
test-tool chmtime =-3600 $graphdir/extra.graph
Quoted 10 lines
t/t5324-split-commit-graph.sh +2
-0+ git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 && test_line_count = 1 $graphdir/commit-graph-chain && + test_path_is_missing $graphdir/extra.graph && ls $graphdir/graph-*.graph >graph-files && test_line_count = 3 graph-files ) &&base-commit:
274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925Signature · 1 line
gitgitgadget
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 10 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&
We have "test-tool chmtime" since 17e48368 (Add test-chmtime: a utility to change mtime on files, 2007-02-24) and refrained from using "touch -t" anywhere in our tests. Can we use it here, too?
Especially with its "relative" form, can't we make the test to stay correct not just for 40 years but forever ;-)?
Quoted 16 lines · Junio C Hamano
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 10 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&We have "test-tool chmtime" since
17e48368(Add test-chmtime: a utility to change mtime on files, 2007-02-24) and refrained from using "touch -t" anywhere in our tests. Can we use it here, too?
There are a couple new ones added last year in t5319. Nobody has complained yet, but I wonder if it's a matter of time.
-Peff
Quoted 19 lines · Jeff King
Quoted 16 lines · Junio C Hamano
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 10 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&We have "test-tool chmtime" since
17e48368(Add test-chmtime: a utility to change mtime on files, 2007-02-24) and refrained from using "touch -t" anywhere in our tests. Can we use it here, too?
Quoted 2 lines
There are a couple new ones added last year in t5319. Nobody has complained yet, but I wonder if it's a matter of time.
Indeed. We should fix them (#leftoverbits).
Quoted 21 lines · Junio C Hamano
Quoted 19 lines · Jeff King
Quoted 16 lines · Junio C Hamano
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 10 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&We have "test-tool chmtime" since
17e48368(Add test-chmtime: a utility to change mtime on files, 2007-02-24) and refrained from using "touch -t" anywhere in our tests. Can we use it here, too?
Quoted 4 lines
Quoted 2 lines
There are a couple new ones added last year in t5319. Nobody has complained yet, but I wonder if it's a matter of time.
Indeed. We should fix them (#leftoverbits).
I'm adding a patch to fix that now.
-Stolee
Quoted 16 lines · Junio C Hamano
t/t5324-split-commit-graph.sh test_expect_success 'test merge stragety constants' ' · line 213
Quoted 10 lines · Derrick Stolee via GitGitGadget
t/t5324-split-commit-graph.sh +1
-1@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch -m -t 201801010000.00 $graphdir/extra.graph &&We have "test-tool chmtime" since
17e48368(Add test-chmtime: a utility to change mtime on files, 2007-02-24) and refrained from using "touch -t" anywhere in our tests. Can we use it here, too?
I will definitely do the more portable thing here.
Quoted 2 lines
Especially with its "relative" form, can't we make the test to stay correct not just for 40 years but forever ;-)?
I should clarify: the test won't break in 40 years. It will "fail to notice the bug" that was previously present. But the relative form is likely to continue catching this bug.
Thanks,
-Stolee
v2 cover commit-graph: fix buggy --expire-time option
In reply to Derrick Stolee via GitGitGadgetThis is embarassing. I should have noticed this when writing it the first time, or when integrating the feature into Scalar and VFS for Git. Sorry!
Update in V2:
* Updated the helptext. Thanks, Taylor!
* Added a patch for the "test-tool chmtime" in t5319
* Used an absolute time with "test-tool chmtime" in t5324 instead of "touch -m"
* Added a file that should be kept on the other side of the expire time to protect against off-by-one errors and future date errors.
Thanks, -Stolee
Derrick Stolee (2):
t5319: replace 'touch -m' with 'test-tool chmtime'
commit-graph: fix buggy --expire-time option
builtin/commit-graph.c | 2 +- commit-graph.c | 2 +- t/t5319-multi-pack-index.sh | 8 ++++---- t/t5324-split-commit-graph.sh | 8 +++++++- 4 files changed, 13 insertions(+), 7 deletions(-)
base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/596
Range-diff vs v1:
-: ----------- > 1: 24e26ecda63 t5319: replace 'touch -m' with 'test-tool chmtime' 1: 32c55c0fc21 ! 2: 56a312695fe commit-graph: fix buggy --expire-time option
@@ Commit message
Update t5323-split-commit-graph.sh to demonstrate the correct value of the --expire-time option by actually creating a crud .graph file with mtime earlier than the expire time. Instead of using a super-
- early time (1980) we need to use a recent time or else the old
- logic actually passes by accident. This test will start passing
- again on the old logic in 40 years or so.
+ early time (1980) we use an explicit, and recent, time. Using
+ test-tool chmtime to create two files on either end of an exact
+ second, we create a test that catches this failure no matter the
+ current time. Using a fixed date is more portable than trying to
+ format a relative date string into the --expiry-date input.
I noticed this when inspecting some Scalar repos that had an excess number of commit-graph files. In Scalar, we were using this second
@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
N_("maximum ratio between two levels of a split commit-graph")),
OPT_EXPIRY_DATE(0, "expire-time", &split_opts.expire_time,
- N_("maximum number of commits in a non-base split commit-graph")),
-+ N_("do not expire files newer than a number of seconds before now")),
++ N_("only expire files older than a given date-time")),
OPT_END(),
};
@@ t/t5324-split-commit-graph.sh: test_expect_success 'test merge stragety constant
test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 &&
- git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&
-+ touch -m -t 201801010000.00 $graphdir/extra.graph &&
-+ git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&
++ touch $graphdir/to-delete.graph $graphdir/to-keep.graph &&
++ test-tool chmtime =1546362000 $graphdir/to-delete.graph &&
++ test-tool chmtime =1546362001 $graphdir/to-keep.graph &&
++ git commit-graph write --reachable --split --size-multiple=10 \
++ --expire-time="2019-01-01 12:00 -05:00" &&
test_line_count = 1 $graphdir/commit-graph-chain &&
-+ test_path_is_missing $graphdir/extra.graph &&
++ test_path_is_missing $graphdir/to-delete.graph &&
++ test_path_is_file $graphdir/to-keep.graph &&
ls $graphdir/graph-*.graph >graph-files &&
test_line_count = 3 graph-files
) &&
Signature · 1 line
gitgitgadget
PATCH v2 1/2 t5319: replace 'touch -m' with 'test-tool chmtime'
From: Derrick Stolee <[email protected]>
The use of 'touch -m' to modify a file's mtime is slightly less portable than using our own 'test-tool chmtime'. The important thing is that these pack-files are ordered in a special way to ensure the multi-pack-index selects some as the "newer" pack-files when resolving duplicate objects.
--- t/t5319-multi-pack-index.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
t/t5319-multi-pack-index.sh +4 -4
@@ -521,10 +521,10 @@ test_expect_success 'repack with minimum size does not alter existing packs' ' cd dup && rm -rf .git/objects/pack && mv .git/objects/pack-backup .git/objects/pack && - touch -m -t 201901010000 .git/objects/pack/pack-D* && - touch -m -t 201901010001 .git/objects/pack/pack-C* && - touch -m -t 201901010002 .git/objects/pack/pack-B* && - touch -m -t 201901010003 .git/objects/pack/pack-A* && + test-tool chmtime =-5 .git/objects/pack/pack-D* && + test-tool chmtime =-4 .git/objects/pack/pack-C* && + test-tool chmtime =-3 .git/objects/pack/pack-B* && + test-tool chmtime =-2 .git/objects/pack/pack-A* && ls .git/objects/pack >expect && MINSIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) && git multi-pack-index repack --batch-size=$MINSIZE &&
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'
Quoted 4 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The use of 'touch -m' to modify a file's mtime is slightly less portable than using our own 'test-tool chmtime'.
Portability aside, the relative form would also be resistant against skews between filesystem time and wallclock time and is preferrable when we can use it.
Quoted 4 lines
The important
thing is that these pack-files are ordered in a special way to ensure the multi-pack-index selects some as the "newer" pack-files when resolving duplicate objects.
This note is very much appreciated.
t/t5319-multi-pack-index.sh test_expect_success 'repack with minimum size does not alter existing packs' ' · line 527
Quoted 10 lines
t/t5319-multi-pack-index.sh +4
-4rm -rf .git/objects/pack && mv .git/objects/pack-backup .git/objects/pack && - touch -m -t 201901010000 .git/objects/pack/pack-D* && - touch -m -t 201901010001 .git/objects/pack/pack-C* && - touch -m -t 201901010002 .git/objects/pack/pack-B* && - touch -m -t 201901010003 .git/objects/pack/pack-A* && + test-tool chmtime =-5 .git/objects/pack/pack-D* && + test-tool chmtime =-4 .git/objects/pack/pack-C* && + test-tool chmtime =-3 .git/objects/pack/pack-B* && + test-tool chmtime =-2 .git/objects/pack/pack-A* &&
The original wants D to be the oldest and A to be the newest, and the updated would want the same ordering.
When created, we know A gets created before B which gets created before C and so on, in the "setup expire tests" part. If each step takes too much time (e.g. the VM is heavily loaded), wouldn't the adjustment above become insufficient?
In other words, would we want to flip the order these packs get created in the "setup" part, in addition to the use of chmtime (which reads the existing file timestamp using stat(2) and then updates the file timestamp relative to the original timestamp) we see here?
Also, in the best case (i.e. original timestamp of A/B/C/D are the same), the above seems to assume that the filesystem has at least 1 second file timestamp granularity. Do we want to make them at least 2 seconds apart, or am I worried too much about ancient filesystems that no longer metter?
Thanks.
Quoted 3 lines
t/t5319-multi-pack-index.sh +0
-0ls .git/objects/pack >expect && MINSIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) && git multi-pack-index repack --batch-size=$MINSIZE &&
Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'
Quoted 43 lines · Junio C Hamano
Quoted 4 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
The use of 'touch -m' to modify a file's mtime is slightly less portable than using our own 'test-tool chmtime'.
Portability aside, the relative form would also be resistant against skews between filesystem time and wallclock time and is preferrable when we can use it.
Quoted 4 lines
The important
thing is that these pack-files are ordered in a special way to ensure the multi-pack-index selects some as the "newer" pack-files when resolving duplicate objects.This note is very much appreciated.
t/t5319-multi-pack-index.sh test_expect_success 'repack with minimum size does not alter existing packs' ' · line 527
Quoted 10 lines
t/t5319-multi-pack-index.sh +4
-4rm -rf .git/objects/pack && mv .git/objects/pack-backup .git/objects/pack && - touch -m -t 201901010000 .git/objects/pack/pack-D* && - touch -m -t 201901010001 .git/objects/pack/pack-C* && - touch -m -t 201901010002 .git/objects/pack/pack-B* && - touch -m -t 201901010003 .git/objects/pack/pack-A* && + test-tool chmtime =-5 .git/objects/pack/pack-D* && + test-tool chmtime =-4 .git/objects/pack/pack-C* && + test-tool chmtime =-3 .git/objects/pack/pack-B* && + test-tool chmtime =-2 .git/objects/pack/pack-A* &&The original wants D to be the oldest and A to be the newest, and the updated would want the same ordering.
When created, we know A gets created before B which gets created before C and so on, in the "setup expire tests" part. If each step takes too much time (e.g. the VM is heavily loaded), wouldn't the adjustment above become insufficient?
In other words, would we want to flip the order these packs get created in the "setup" part, in addition to the use of chmtime (which reads the existing file timestamp using stat(2) and then updates the file timestamp relative to the original timestamp) we see here?
Quoted 5 lines
Also, in the best case (i.e. original timestamp of A/B/C/D are the same), the above seems to assume that the filesystem has at least 1 second file timestamp granularity. Do we want to make them at least 2 seconds apart, or am I worried too much about ancient filesystems that no longer metter?
The old test relied on one-second granularity, so that hasn't changed. I could easily space it out a bit more without issue.
Your concern about the original timestamps getting skewed shouldn't be an issue because "test-tool chmtime =-<seconds>" subtracts the <seconds> from the current system time, not the file's mtime. This is subtle: without the "=" it would modify it from the file's mtime.
Since we are assigning the offset values in the proper order (D to A) there isn't an issue if time ticks forward between these steps.
Thanks,
-Stolee
Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'
Quoted 9 lines · Derrick Stolee
Quoted 5 lines
Also, in the best case (i.e. original timestamp of A/B/C/D are the same), the above seems to assume that the filesystem has at least 1 second file timestamp granularity. Do we want to make them at least 2 seconds apart, or am I worried too much about ancient filesystems that no longer metter?
The old test relied on one-second granularity, so that hasn't changed. I could easily space it out a bit more without issue.
Quoted 7 lines
Your concern about the original timestamps getting skewed shouldn't be an issue because "test-tool chmtime =-<seconds>" subtracts the <seconds> from the current system time, not the file's mtime. This is subtle: without the "=" it would modify it from the file's mtime.
Since we are assigning the offset values in the proper order (D to A) there isn't an issue if time ticks forward between these steps.
I wonder if that would run afoul of the same "mtime and system clock are not quite the same" issue we saw recently in [1].
I think it might not because we're only comparing mtimes set through the same mechanism (find the system time, subtract from it, and assign to mtime). If system time is monotonically increasing at any rate, that would produce the desired effect.
That said, it seems so easy to just give it a 10 second (or even 10 minute) spread to avoid any possibility of confusion.
-Peff
Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'
Quoted 4 lines · Jeff King
On Wed, Apr 01, 2020 at 08:06:47PM -0400, Derrick Stolee wrote:
I wonder if that would run afoul of the same "mtime and system clock are not quite the same" issue we saw recently in [1].
I had the exact thing in mind, which was why I suggested "chmtime" in the first place (the second reason was that it was easy to do relative time using the tool than "date + touch"), but I missed that the tool has a mode that uses timestamp relative to system clock.
Quoted 4 lines
I think it might not because we're only comparing mtimes set through the same mechanism (find the system time, subtract from it, and assign to mtime). If system time is monotonically increasing at any rate, that would produce the desired effect.
But in this particular case, I think, even though setting the filesystem timestamp based on something that was obtained from the system clock feels as if we are asking for trouble, the values we later read back from the filesystem for executing the codepath being tested (namely, expiration logic) get compared with another value that is derived from from the system clock (i.e. expiration cutoff), so in that sense you might be able to argue that it is even more correct ;-)
PATCH v2 2/2 commit-graph: fix buggy --expire-time option
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
The commit-graph builtin has an --expire-time option that takes a datetime using OPT_EXPIRY_DATE(). However, the implementation inside expire_commit_graphs() was treating a non-zero value as a number of seconds to subtract from "now".
Update t5323-split-commit-graph.sh to demonstrate the correct value of the --expire-time option by actually creating a crud .graph file with mtime earlier than the expire time. Instead of using a super- early time (1980) we use an explicit, and recent, time. Using test-tool chmtime to create two files on either end of an exact second, we create a test that catches this failure no matter the current time. Using a fixed date is more portable than trying to format a relative date string into the --expiry-date input.
I noticed this when inspecting some Scalar repos that had an excess number of commit-graph files. In Scalar, we were using this second interpretation by using "--expire-time=3600" to mean "delete graphs older than one hour ago" to avoid deleting a commit-graph that a foreground process may be trying to load.
Also I noticed that the help text was copied from the --max-commits option. Fix that help text.
--- builtin/commit-graph.c | 2 +- commit-graph.c | 2 +- t/t5324-split-commit-graph.sh | 8 +++++++- 3 files changed, 9 insertions(+), 3 deletions(-)
builtin/commit-graph.c +1 -1
@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)OPT_INTEGER (0 ,"size-multiple" , &split_opts.size_multiple,N_ ("maximum ratio between two levels of a split commit-graph" )),OPT_EXPIRY_DATE (0 ,"expire-time" , &split_opts.expire_time, -N_ ("maximum number of commits in a non-base split commit-graph" )), +N_ ("only expire files older than a given date-time" )), OPT_END(), };
commit-graph.c +1 -1
@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)timestamp_t expire_time =time (NULL );if (ctx->split_opts && ctx->split_opts->expire_time) - expire_time -= ctx->split_opts->expire_time; + expire_time = ctx->split_opts->expire_time;if (!ctx->split) {char *chain_file_name =get_chain_filename (ctx->odb);unlink (chain_file_name);
t/t5324-split-commit-graph.sh +7 -1
@@ -210,8 +210,14 @@ test_expect_success 'test merge stragety constants' ' git config core.commitGraph true && test_line_count = 2 $graphdir/commit-graph-chain && test_commit 15 && - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 && + touch $graphdir/to-delete.graph $graphdir/to-keep.graph && + test-tool chmtime =1546362000 $graphdir/to-delete.graph && + test-tool chmtime =1546362001 $graphdir/to-keep.graph && + git commit-graph write --reachable --split --size-multiple=10 \ + --expire-time="2019-01-01 12:00 -05:00" && test_line_count = 1 $graphdir/commit-graph-chain && + test_path_is_missing $graphdir/to-delete.graph && + test_path_is_file $graphdir/to-keep.graph && ls $graphdir/graph-*.graph >graph-files && test_line_count = 3 graph-files ) &&
Signature · 1 line
gitgitgadget
Quoted 2 lines · Derrick Stolee via GitGitGadget
* Added a file that should be kept on the other side of the expire time to protect against off-by-one errors and future date errors.
Thanks for being extra careful. It is always a pleasure to read patches from you ;-)
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Derrick Stolee via GitGitGadget
* Added a file that should be kept on the other side of the expire time to protect against off-by-one errors and future date errors.
Thanks for being extra careful. It is always a pleasure to read patches from you ;-)
I appreciate that! Credit for this suggestion goes to Kevin Willford.
-Stolee