{"thread":{"id":"53153","subject":"[PATCH] commit-graph: fix buggy --expire-time option","startedAt":"2020-04-01T18:11:40Z","lastAt":"2020-04-02T16:36:21Z","messageCount":20,"participants":["Derrick Stolee via GitGitGadget","Derrick Stolee","Taylor Blau","Eric Sunshine","SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394507","messageId":"pull.596.git.1585764695643.gitgitgadget@gmail.com","threadId":"53153","inReplyTo":null,"subject":"[PATCH] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-01T18:11:35Z","receivedAt":"2020-04-01T18:11:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit-graph builtin has an --expire-time option that takes a\ndatetime using OPT_EXPIRY_DATE(). However, the implementation inside\nexpire_commit_graphs() was treating a non-zero value as a number of\nseconds to subtract from \"now\".\n\nUpdate t5323-split-commit-graph.sh to demonstrate the correct value\nof the --expire-time option by actually creating a crud .graph file\nwith mtime earlier than the expire time. Instead of using a super-\nearly time (1980) we need to use a recent time or else the old\nlogic actually passes by accident. This test will start passing\nagain on the old logic in 40 years or so.\n\nI noticed this when inspecting some Scalar repos that had an excess\nnumber of commit-graph files. In Scalar, we were using this second\ninterpretation by using \"--expire-time=3600\" to mean \"delete graphs\nolder than one hour ago\" to avoid deleting a commit-graph that a\nforeground process may be trying to load.\n\nAlso I noticed that the help text was copied from the --max-commits\noption. Fix that help text.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph: fix buggy --expire-time option\n \n This is embarassing. I should have noticed this when writing it the\n first time, or when integrating the feature into Scalar and VFS for Git.\n Sorry!\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/596\n\n builtin/commit-graph.c | 2 +-\n commit-graph.c | 2 +-\n t/t5324-split-commit-graph.sh | 4 +++-\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4a70b33fb5f..8000ff0d2ee 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n-\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n+\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n \t\tOPT_END(),\n \t};\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e294..0d0d37787a0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \ttimestamp_t expire_time = time(NULL);\n \n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n-\t\texpire_time -= ctx->split_opts->expire_time;\n+\t\texpire_time = ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n \t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n \t\tunlink(chain_file_name);\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 53b2e6b4555..4e4efcaff22 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n \t\tgit config core.commitGraph true &&\n \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n \t\ttest_commit 15 &&\n-\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n+\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n+\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&\n \t\ttest_line_count = 1 $graphdir/commit-graph-chain &&\n+\t\ttest_path_is_missing $graphdir/extra.graph &&\n \t\tls $graphdir/graph-*.graph >graph-files &&\n \t\ttest_line_count = 3 graph-files\n \t) &&\n\nbase-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n-- \ngitgitgadget\n"},{"id":"394508","messageId":"bbd2b8e4-47f5-a99e-fb14-b6c1ce2ad9a9@gmail.com","threadId":"53153","inReplyTo":"pull.596.git.1585764695643.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-01T18:17:50Z","receivedAt":"2020-04-01T18:17:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2020 2:11 PM, Derrick Stolee via GitGitGadget wrote:\n> Also I noticed that the help text was copied from the --max-commits\n> option. Fix that help text.\n...\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 4a70b33fb5f..8000ff0d2ee 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n> \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n> \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n> \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n> -\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n> +\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n\nand of course I messed this up even now. Should be:\n\n> +\t\t\tN_(\"do not expire files newer than a given date-time\")),\n\n-Stolee\n"},{"id":"394509","messageId":"20200401185629.GA13599@syl.local","threadId":"53153","inReplyTo":"bbd2b8e4-47f5-a99e-fb14-b6c1ce2ad9a9@gmail.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-01T18:56:29Z","receivedAt":"2020-04-01T18:56:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 01, 2020 at 02:17:50PM -0400, Derrick Stolee wrote:\n> On 4/1/2020 2:11 PM, Derrick Stolee via GitGitGadget wrote:\n> > Also I noticed that the help text was copied from the --max-commits\n> > option. Fix that help text.\n> ...\n> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > index 4a70b33fb5f..8000ff0d2ee 100644\n> > --- a/builtin/commit-graph.c\n> > +++ b/builtin/commit-graph.c\n> > @@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n> > \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n> > \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n> > \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n> > -\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n> > +\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n>\n> and of course I messed this up even now. Should be:\n>\n> > +\t\t\tN_(\"do not expire files newer than a given date-time\")),\n\nI wonder if the double-negative can be avoided. Perhaps this should be\ninstead:\n\n expire files older than the given date-time\n\nor similar.\n\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"394512","messageId":"20200401192715.GB13599@syl.local","threadId":"53153","inReplyTo":"pull.596.git.1585764695643.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-01T19:27:15Z","receivedAt":"2020-04-01T19:27:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The commit-graph builtin has an --expire-time option that takes a\n> datetime using OPT_EXPIRY_DATE(). However, the implementation inside\n> expire_commit_graphs() was treating a non-zero value as a number of\n> seconds to subtract from \"now\".\n\nAh, nice catch. The fix below seems straightforward. Since we're using\n'OPT_EXPIRY_DATE', parse-options accepts either, but gives us back a\ndate-time, so there's no need to subtract it from the current instant or\nanything like that.\n\nThis patch below looks fine (I left a minor note here, and in the\ndocumentation fixup that you sent shortly after this message, but\notherwise I think that this is good to go, at least from my point of\nview).\n\n> Update t5323-split-commit-graph.sh to demonstrate the correct value\n> of the --expire-time option by actually creating a crud .graph file\n> with mtime earlier than the expire time. Instead of using a super-\n> early time (1980) we need to use a recent time or else the old\n> logic actually passes by accident. This test will start passing\n> again on the old logic in 40 years or so.\n>\n> I noticed this when inspecting some Scalar repos that had an excess\n> number of commit-graph files. In Scalar, we were using this second\n> interpretation by using \"--expire-time=3600\" to mean \"delete graphs\n> older than one hour ago\" to avoid deleting a commit-graph that a\n> foreground process may be trying to load.\n>\n> Also I noticed that the help text was copied from the --max-commits\n> option. Fix that help text.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph: fix buggy --expire-time option\n>\n> This is embarassing. I should have noticed this when writing it the\n> first time, or when integrating the feature into Scalar and VFS for Git.\n> Sorry!\n>\n> Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/596\n>\n> builtin/commit-graph.c | 2 +-\n> commit-graph.c | 2 +-\n> t/t5324-split-commit-graph.sh | 4 +++-\n> 3 files changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 4a70b33fb5f..8000ff0d2ee 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n> \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n> \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n> \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n> -\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n> +\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n> \t\tOPT_END(),\n> \t};\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f013a84e294..0d0d37787a0 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n> \ttimestamp_t expire_time = time(NULL);\n>\n> \tif (ctx->split_opts && ctx->split_opts->expire_time)\n> -\t\texpire_time -= ctx->split_opts->expire_time;\n> +\t\texpire_time = ctx->split_opts->expire_time;\n> \tif (!ctx->split) {\n> \t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n> \t\tunlink(chain_file_name);\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 53b2e6b4555..4e4efcaff22 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n> \t\tgit config core.commitGraph true &&\n> \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n> \t\ttest_commit 15 &&\n> -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n> +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n> +\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&\n\nCould this be written instead as:\n\n touch -m -t $(date -r $test_tick +\"%Y%m%d%H%M.%S\") $graphdir/extra.graph &&\n test_tick &&\n git commit-graph write --reachable --split --size-multiple=10 --expire-time=now &&\n\nto avoid breaking in 40 years?\n\n> \t\ttest_line_count = 1 $graphdir/commit-graph-chain &&\n> +\t\ttest_path_is_missing $graphdir/extra.graph &&\n> \t\tls $graphdir/graph-*.graph >graph-files &&\n> \t\ttest_line_count = 3 graph-files\n> \t) &&\n>\n> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n> --\n> gitgitgadget\n\nThanks,\nTaylor\n"},{"id":"394513","messageId":"CAPig+cQc+Qr=BHKgWkQv8QrB47qG+-Y0+DQeKj5WD6VwzeAyHw@mail.gmail.com","threadId":"53153","inReplyTo":"20200401192715.GB13599@syl.local","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-01T19:36:42Z","receivedAt":"2020-04-01T19:36:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 1, 2020 at 3:27 PM Taylor Blau wrote:\n> On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:\n> > - git commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n> > + touch -m -t 201801010000.00 $graphdir/extra.graph &&\n> > + git commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&\n>\n> Could this be written instead as:\n>\n> touch -m -t $(date -r $test_tick +\"%Y%m%d%H%M.%S\") $graphdir/extra.graph &&\n> test_tick &&\n\nThis won't work with GNU 'date' command which doesn't understand\n\"-r\" (it understands only \"-r\").\n"},{"id":"394514","messageId":"20200401194706.GE2224@szeder.dev","threadId":"53153","inReplyTo":"pull.596.git.1585764695643.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-04-01T19:47:06Z","receivedAt":"2020-04-01T19:47:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The commit-graph builtin has an --expire-time option that takes a\n> datetime using OPT_EXPIRY_DATE(). However, the implementation inside\n> expire_commit_graphs() was treating a non-zero value as a number of\n> seconds to subtract from \"now\".\n> \n> Update t5323-split-commit-graph.sh to demonstrate the correct value\n> of the --expire-time option by actually creating a crud .graph file\n> with mtime earlier than the expire time. Instead of using a super-\n> early time (1980) we need to use a recent time or else the old\n> logic actually passes by accident. This test will start passing\n> again on the old logic in 40 years or so.\n> \n> I noticed this when inspecting some Scalar repos that had an excess\n> number of commit-graph files. In Scalar, we were using this second\n> interpretation by using \"--expire-time=3600\" to mean \"delete graphs\n> older than one hour ago\" to avoid deleting a commit-graph that a\n> foreground process may be trying to load.\n> \n> Also I noticed that the help text was copied from the --max-commits\n> option. Fix that help text.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph: fix buggy --expire-time option\n> \n> This is embarassing. I should have noticed this when writing it the\n> first time, or when integrating the feature into Scalar and VFS for Git.\n> Sorry!\n> \n> Thanks, -Stolee\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/596\n> \n> builtin/commit-graph.c | 2 +-\n> commit-graph.c | 2 +-\n> t/t5324-split-commit-graph.sh | 4 +++-\n> 3 files changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 4a70b33fb5f..8000ff0d2ee 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n> \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n> \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n> \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n> -\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n> +\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n> \t\tOPT_END(),\n> \t};\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index f013a84e294..0d0d37787a0 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n> \ttimestamp_t expire_time = time(NULL);\n> \n> \tif (ctx->split_opts && ctx->split_opts->expire_time)\n> -\t\texpire_time -= ctx->split_opts->expire_time;\n> +\t\texpire_time = ctx->split_opts->expire_time;\n> \tif (!ctx->split) {\n> \t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n> \t\tunlink(chain_file_name);\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 53b2e6b4555..4e4efcaff22 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n> \t\tgit config core.commitGraph true &&\n> \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n> \t\ttest_commit 15 &&\n> -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n> +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n\nYou can set a mtime relative to the current time with:\n\n test-tool chmtime =-3600 $graphdir/extra.graph\n\n\n> +\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&\n> \t\ttest_line_count = 1 $graphdir/commit-graph-chain &&\n> +\t\ttest_path_is_missing $graphdir/extra.graph &&\n> \t\tls $graphdir/graph-*.graph >graph-files &&\n> \t\ttest_line_count = 3 graph-files\n> \t) &&\n> \n> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\n> -- \n> gitgitgadget\n"},{"id":"394515","messageId":"xmqqo8sb56m2.fsf@gitster.c.googlers.com","threadId":"53153","inReplyTo":"pull.596.git.1585764695643.gitgitgadget@gmail.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-01T19:49:25Z","receivedAt":"2020-04-01T19:49:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> index 53b2e6b4555..4e4efcaff22 100755\n> --- a/t/t5324-split-commit-graph.sh\n> +++ b/t/t5324-split-commit-graph.sh\n> @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n> \t\tgit config core.commitGraph true &&\n> \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n> \t\ttest_commit 15 &&\n> -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n> +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n\nWe have \"test-tool chmtime\" since 17e48368 (Add test-chmtime: a\nutility to change mtime on files, 2007-02-24) and refrained from\nusing \"touch -t\" anywhere in our tests. Can we use it here, too?\n\nEspecially with its \"relative\" form, can't we make the test to\nstay correct not just for 40 years but forever ;-)?\n"},{"id":"394516","messageId":"20200401195708.GA3063415@coredump.intra.peff.net","threadId":"53153","inReplyTo":"xmqqo8sb56m2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-01T19:57:08Z","receivedAt":"2020-04-01T19:57:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:\n\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n> > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n> > index 53b2e6b4555..4e4efcaff22 100755\n> > --- a/t/t5324-split-commit-graph.sh\n> > +++ b/t/t5324-split-commit-graph.sh\n> > @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n> > \t\tgit config core.commitGraph true &&\n> > \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n> > \t\ttest_commit 15 &&\n> > -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n> > +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n> \n> We have \"test-tool chmtime\" since 17e48368 (Add test-chmtime: a\n> utility to change mtime on files, 2007-02-24) and refrained from\n> using \"touch -t\" anywhere in our tests. Can we use it here, too?\n\nThere are a couple new ones added last year in t5319. Nobody has\ncomplained yet, but I wonder if it's a matter of time.\n\n-Peff\n"},{"id":"394517","messageId":"3bacbd6b-a320-a17f-0d94-0322bffdec37@gmail.com","threadId":"53153","inReplyTo":"xmqqo8sb56m2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-01T20:14:44Z","receivedAt":"2020-04-01T20:14:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2020 3:49 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n>> index 53b2e6b4555..4e4efcaff22 100755\n>> --- a/t/t5324-split-commit-graph.sh\n>> +++ b/t/t5324-split-commit-graph.sh\n>> @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n>> \t\tgit config core.commitGraph true &&\n>> \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n>> \t\ttest_commit 15 &&\n>> -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n>> +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n> \n> We have \"test-tool chmtime\" since 17e48368 (Add test-chmtime: a\n> utility to change mtime on files, 2007-02-24) and refrained from\n> using \"touch -t\" anywhere in our tests. Can we use it here, too?\n\nI will definitely do the more portable thing here.\n \n> Especially with its \"relative\" form, can't we make the test to\n> stay correct not just for 40 years but forever ;-)?\n\nI should clarify: the test won't break in 40 years. It will\n\"fail to notice the bug\" that was previously present. But the\nrelative form is likely to continue catching this bug.\n\nThanks,\n-Stolee\n\n"},{"id":"394520","messageId":"xmqqk12z54k2.fsf@gitster.c.googlers.com","threadId":"53153","inReplyTo":"20200401195708.GA3063415@coredump.intra.peff.net","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-01T20:33:49Z","receivedAt":"2020-04-01T20:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:\n>\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>> \n>> > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n>> > index 53b2e6b4555..4e4efcaff22 100755\n>> > --- a/t/t5324-split-commit-graph.sh\n>> > +++ b/t/t5324-split-commit-graph.sh\n>> > @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n>> > \t\tgit config core.commitGraph true &&\n>> > \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n>> > \t\ttest_commit 15 &&\n>> > -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n>> > +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n>> \n>> We have \"test-tool chmtime\" since 17e48368 (Add test-chmtime: a\n>> utility to change mtime on files, 2007-02-24) and refrained from\n>> using \"touch -t\" anywhere in our tests. Can we use it here, too?\n>\n> There are a couple new ones added last year in t5319. Nobody has\n> complained yet, but I wonder if it's a matter of time.\n\nIndeed. We should fix them (#leftoverbits).\n"},{"id":"394522","messageId":"6028eb86-5172-a00e-860b-2d6f6d093f44@gmail.com","threadId":"53153","inReplyTo":"xmqqk12z54k2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-01T20:51:09Z","receivedAt":"2020-04-01T20:51:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2020 4:33 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:\n>>\n>>> \"Derrick Stolee via GitGitGadget\" writes:\n>>>\n>>>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\n>>>> index 53b2e6b4555..4e4efcaff22 100755\n>>>> --- a/t/t5324-split-commit-graph.sh\n>>>> +++ b/t/t5324-split-commit-graph.sh\n>>>> @@ -210,8 +210,10 @@ test_expect_success 'test merge stragety constants' '\n>>>> \t\tgit config core.commitGraph true &&\n>>>> \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n>>>> \t\ttest_commit 15 &&\n>>>> -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n>>>> +\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n>>>\n>>> We have \"test-tool chmtime\" since 17e48368 (Add test-chmtime: a\n>>> utility to change mtime on files, 2007-02-24) and refrained from\n>>> using \"touch -t\" anywhere in our tests. Can we use it here, too?\n>>\n>> There are a couple new ones added last year in t5319. Nobody has\n>> complained yet, but I wonder if it's a matter of time.\n> \n> Indeed. We should fix them (#leftoverbits).\n\nI'm adding a patch to fix that now.\n\n-Stolee\n\n"},{"id":"394523","messageId":"56a312695fe1efc3393f67894c95b5bd643c66d1.1585774844.git.gitgitgadget@gmail.com","threadId":"53153","inReplyTo":"pull.596.v2.git.1585774844.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-01T21:00:44Z","receivedAt":"2020-04-01T21:00:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe commit-graph builtin has an --expire-time option that takes a\ndatetime using OPT_EXPIRY_DATE(). However, the implementation inside\nexpire_commit_graphs() was treating a non-zero value as a number of\nseconds to subtract from \"now\".\n\nUpdate t5323-split-commit-graph.sh to demonstrate the correct value\nof the --expire-time option by actually creating a crud .graph file\nwith mtime earlier than the expire time. Instead of using a super-\nearly time (1980) we use an explicit, and recent, time. Using\ntest-tool chmtime to create two files on either end of an exact\nsecond, we create a test that catches this failure no matter the\ncurrent time. Using a fixed date is more portable than trying to\nformat a relative date string into the --expiry-date input.\n\nI noticed this when inspecting some Scalar repos that had an excess\nnumber of commit-graph files. In Scalar, we were using this second\ninterpretation by using \"--expire-time=3600\" to mean \"delete graphs\nolder than one hour ago\" to avoid deleting a commit-graph that a\nforeground process may be trying to load.\n\nAlso I noticed that the help text was copied from the --max-commits\noption. Fix that help text.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/commit-graph.c | 2 +-\n commit-graph.c | 2 +-\n t/t5324-split-commit-graph.sh | 8 +++++++-\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 4a70b33fb5f..5bfba972498 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -140,7 +140,7 @@ static int graph_write(int argc, const char **argv)\n \t\tOPT_INTEGER(0, \"size-multiple\", &split_opts.size_multiple,\n \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n-\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n+\t\t\tN_(\"only expire files older than a given date-time\")),\n \t\tOPT_END(),\n \t};\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex f013a84e294..0d0d37787a0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1707,7 +1707,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n \ttimestamp_t expire_time = time(NULL);\n \n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n-\t\texpire_time -= ctx->split_opts->expire_time;\n+\t\texpire_time = ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n \t\tchar *chain_file_name = get_chain_filename(ctx->odb);\n \t\tunlink(chain_file_name);\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 53b2e6b4555..b8b208fc3da 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -210,8 +210,14 @@ test_expect_success 'test merge stragety constants' '\n \t\tgit config core.commitGraph true &&\n \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n \t\ttest_commit 15 &&\n-\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n+\t\ttouch $graphdir/to-delete.graph $graphdir/to-keep.graph &&\n+\t\ttest-tool chmtime =1546362000 $graphdir/to-delete.graph &&\n+\t\ttest-tool chmtime =1546362001 $graphdir/to-keep.graph &&\n+\t\tgit commit-graph write --reachable --split --size-multiple=10 \\\n+\t\t\t--expire-time=\"2019-01-01 12:00 -05:00\" &&\n \t\ttest_line_count = 1 $graphdir/commit-graph-chain &&\n+\t\ttest_path_is_missing $graphdir/to-delete.graph &&\n+\t\ttest_path_is_file $graphdir/to-keep.graph &&\n \t\tls $graphdir/graph-*.graph >graph-files &&\n \t\ttest_line_count = 3 graph-files\n \t) &&\n-- \ngitgitgadget\n"},{"id":"394524","messageId":"24e26ecda632421e4c1a61ebabf6ea78301685b1.1585774844.git.gitgitgadget@gmail.com","threadId":"53153","inReplyTo":"pull.596.v2.git.1585774844.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-01T21:00:43Z","receivedAt":"2020-04-01T21:00:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe use of 'touch -m' to modify a file's mtime is slightly less\nportable than using our own 'test-tool chmtime'. The important\nthing is that these pack-files are ordered in a special way to\nensure the multi-pack-index selects some as the \"newer\" pack-files\nwhen resolving duplicate objects.\n\nReported-by: Jeff King \nSigned-off-by: Derrick Stolee \n---\n t/t5319-multi-pack-index.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43a7a66c9d1..6b20e60314d 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -521,10 +521,10 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n \t\tcd dup &&\n \t\trm -rf .git/objects/pack &&\n \t\tmv .git/objects/pack-backup .git/objects/pack &&\n-\t\ttouch -m -t 201901010000 .git/objects/pack/pack-D* &&\n-\t\ttouch -m -t 201901010001 .git/objects/pack/pack-C* &&\n-\t\ttouch -m -t 201901010002 .git/objects/pack/pack-B* &&\n-\t\ttouch -m -t 201901010003 .git/objects/pack/pack-A* &&\n+\t\ttest-tool chmtime =-5 .git/objects/pack/pack-D* &&\n+\t\ttest-tool chmtime =-4 .git/objects/pack/pack-C* &&\n+\t\ttest-tool chmtime =-3 .git/objects/pack/pack-B* &&\n+\t\ttest-tool chmtime =-2 .git/objects/pack/pack-A* &&\n \t\tls .git/objects/pack >expect &&\n \t\tMINSIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) &&\n \t\tgit multi-pack-index repack --batch-size=$MINSIZE &&\n-- \ngitgitgadget\n\n"},{"id":"394525","messageId":"pull.596.v2.git.1585774844.gitgitgadget@gmail.com","threadId":"53153","inReplyTo":"pull.596.git.1585764695643.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-01T21:00:42Z","receivedAt":"2020-04-01T21:00:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is embarassing. I should have noticed this when writing it the first\ntime, or when integrating the feature into Scalar and VFS for Git. Sorry!\n\nUpdate in V2:\n\n * Updated the helptext. Thanks, Taylor!\n * Added a patch for the \"test-tool chmtime\" in t5319\n * Used an absolute time with \"test-tool chmtime\" in t5324 instead of \"touch\n -m\"\n * Added a file that should be kept on the other side of the expire time to\n protect against off-by-one errors and future date errors.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n t5319: replace 'touch -m' with 'test-tool chmtime'\n commit-graph: fix buggy --expire-time option\n\n builtin/commit-graph.c | 2 +-\n commit-graph.c | 2 +-\n t/t5319-multi-pack-index.sh | 8 ++++----\n t/t5324-split-commit-graph.sh | 8 +++++++-\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\n\nbase-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-596%2Fderrickstolee%2Fcommit-graph-expire-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-596/derrickstolee/commit-graph-expire-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/596\n\nRange-diff vs v1:\n\n -: ----------- > 1: 24e26ecda63 t5319: replace 'touch -m' with 'test-tool chmtime'\n 1: 32c55c0fc21 ! 2: 56a312695fe commit-graph: fix buggy --expire-time option\n @@ Commit message\n Update t5323-split-commit-graph.sh to demonstrate the correct value\n of the --expire-time option by actually creating a crud .graph file\n with mtime earlier than the expire time. Instead of using a super-\n - early time (1980) we need to use a recent time or else the old\n - logic actually passes by accident. This test will start passing\n - again on the old logic in 40 years or so.\n + early time (1980) we use an explicit, and recent, time. Using\n + test-tool chmtime to create two files on either end of an exact\n + second, we create a test that catches this failure no matter the\n + current time. Using a fixed date is more portable than trying to\n + format a relative date string into the --expiry-date input.\n \n I noticed this when inspecting some Scalar repos that had an excess\n number of commit-graph files. In Scalar, we were using this second\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"maximum ratio between two levels of a split commit-graph\")),\n \t\tOPT_EXPIRY_DATE(0, \"expire-time\", &split_opts.expire_time,\n -\t\t\tN_(\"maximum number of commits in a non-base split commit-graph\")),\n -+\t\t\tN_(\"do not expire files newer than a number of seconds before now\")),\n ++\t\t\tN_(\"only expire files older than a given date-time\")),\n \t\tOPT_END(),\n \t};\n \n @@ t/t5324-split-commit-graph.sh: test_expect_success 'test merge stragety constant\n \t\ttest_line_count = 2 $graphdir/commit-graph-chain &&\n \t\ttest_commit 15 &&\n -\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=1980-01-01 &&\n -+\t\ttouch -m -t 201801010000.00 $graphdir/extra.graph &&\n -+\t\tgit commit-graph write --reachable --split --size-multiple=10 --expire-time=2019-01-01 &&\n ++\t\ttouch $graphdir/to-delete.graph $graphdir/to-keep.graph &&\n ++\t\ttest-tool chmtime =1546362000 $graphdir/to-delete.graph &&\n ++\t\ttest-tool chmtime =1546362001 $graphdir/to-keep.graph &&\n ++\t\tgit commit-graph write --reachable --split --size-multiple=10 \\\n ++\t\t\t--expire-time=\"2019-01-01 12:00 -05:00\" &&\n \t\ttest_line_count = 1 $graphdir/commit-graph-chain &&\n -+\t\ttest_path_is_missing $graphdir/extra.graph &&\n ++\t\ttest_path_is_missing $graphdir/to-delete.graph &&\n ++\t\ttest_path_is_file $graphdir/to-keep.graph &&\n \t\tls $graphdir/graph-*.graph >graph-files &&\n \t\ttest_line_count = 3 graph-files\n \t) &&\n\n-- \ngitgitgadget\n"},{"id":"394526","messageId":"xmqqbloa6hmw.fsf@gitster.c.googlers.com","threadId":"53153","inReplyTo":"pull.596.v2.git.1585774844.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] commit-graph: fix buggy --expire-time option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-01T21:05:59Z","receivedAt":"2020-04-01T21:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> * Added a file that should be kept on the other side of the expire time to\n> protect against off-by-one errors and future date errors.\n\nThanks for being extra careful. It is always a pleasure to read\npatches from you ;-)\n"},{"id":"394529","messageId":"xmqq369m6g9u.fsf@gitster.c.googlers.com","threadId":"53153","inReplyTo":"24e26ecda632421e4c1a61ebabf6ea78301685b1.1585774844.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-01T21:35:25Z","receivedAt":"2020-04-01T21:35:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The use of 'touch -m' to modify a file's mtime is slightly less\n> portable than using our own 'test-tool chmtime'.\n\nPortability aside, the relative form would also be resistant against\nskews between filesystem time and wallclock time and is preferrable\nwhen we can use it.\n\n> The important\n> thing is that these pack-files are ordered in a special way to\n> ensure the multi-pack-index selects some as the \"newer\" pack-files\n> when resolving duplicate objects.\n\nThis note is very much appreciated.\n\n> \t\trm -rf .git/objects/pack &&\n> \t\tmv .git/objects/pack-backup .git/objects/pack &&\n> -\t\ttouch -m -t 201901010000 .git/objects/pack/pack-D* &&\n> -\t\ttouch -m -t 201901010001 .git/objects/pack/pack-C* &&\n> -\t\ttouch -m -t 201901010002 .git/objects/pack/pack-B* &&\n> -\t\ttouch -m -t 201901010003 .git/objects/pack/pack-A* &&\n> +\t\ttest-tool chmtime =-5 .git/objects/pack/pack-D* &&\n> +\t\ttest-tool chmtime =-4 .git/objects/pack/pack-C* &&\n> +\t\ttest-tool chmtime =-3 .git/objects/pack/pack-B* &&\n> +\t\ttest-tool chmtime =-2 .git/objects/pack/pack-A* &&\n\nThe original wants D to be the oldest and A to be the newest, and\nthe updated would want the same ordering.\n\nWhen created, we know A gets created before B which gets created\nbefore C and so on, in the \"setup expire tests\" part. If each step\ntakes too much time (e.g. the VM is heavily loaded), wouldn't the\nadjustment above become insufficient?\n\nIn other words, would we want to flip the order these packs get\ncreated in the \"setup\" part, in addition to the use of chmtime\n(which reads the existing file timestamp using stat(2) and then\nupdates the file timestamp relative to the original timestamp)\nwe see here?\n\nAlso, in the best case (i.e. original timestamp of A/B/C/D are the\nsame), the above seems to assume that the filesystem has at least 1\nsecond file timestamp granularity. Do we want to make them at least\n2 seconds apart, or am I worried too much about ancient filesystems\nthat no longer metter?\n\nThanks.\n\n> \t\tls .git/objects/pack >expect &&\n> \t\tMINSIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) &&\n> \t\tgit multi-pack-index repack --batch-size=$MINSIZE &&\n"},{"id":"394534","messageId":"acf4b640-f66c-beed-adb9-30bc912d8723@gmail.com","threadId":"53153","inReplyTo":"xmqqbloa6hmw.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/2] commit-graph: fix buggy --expire-time option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-01T23:33:19Z","receivedAt":"2020-04-01T23:33:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2020 5:05 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> * Added a file that should be kept on the other side of the expire time to\n>> protect against off-by-one errors and future date errors.\n> \n> Thanks for being extra careful. It is always a pleasure to read\n> patches from you ;-)\n\nI appreciate that! Credit for this suggestion goes to Kevin Willford.\n\n-Stolee\n\n"},{"id":"394535","messageId":"8e6b1f75-534a-ad79-544b-126ea61b2058@gmail.com","threadId":"53153","inReplyTo":"xmqq369m6g9u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-02T00:06:47Z","receivedAt":"2020-04-02T00:06:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/1/2020 5:35 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> The use of 'touch -m' to modify a file's mtime is slightly less\n>> portable than using our own 'test-tool chmtime'.\n> \n> Portability aside, the relative form would also be resistant against\n> skews between filesystem time and wallclock time and is preferrable\n> when we can use it.\n> \n>> The important\n>> thing is that these pack-files are ordered in a special way to\n>> ensure the multi-pack-index selects some as the \"newer\" pack-files\n>> when resolving duplicate objects.\n> \n> This note is very much appreciated.\n> \n>> \t\trm -rf .git/objects/pack &&\n>> \t\tmv .git/objects/pack-backup .git/objects/pack &&\n>> -\t\ttouch -m -t 201901010000 .git/objects/pack/pack-D* &&\n>> -\t\ttouch -m -t 201901010001 .git/objects/pack/pack-C* &&\n>> -\t\ttouch -m -t 201901010002 .git/objects/pack/pack-B* &&\n>> -\t\ttouch -m -t 201901010003 .git/objects/pack/pack-A* &&\n>> +\t\ttest-tool chmtime =-5 .git/objects/pack/pack-D* &&\n>> +\t\ttest-tool chmtime =-4 .git/objects/pack/pack-C* &&\n>> +\t\ttest-tool chmtime =-3 .git/objects/pack/pack-B* &&\n>> +\t\ttest-tool chmtime =-2 .git/objects/pack/pack-A* &&\n> \n> The original wants D to be the oldest and A to be the newest, and\n> the updated would want the same ordering.\n> \n> When created, we know A gets created before B which gets created\n> before C and so on, in the \"setup expire tests\" part. If each step\n> takes too much time (e.g. the VM is heavily loaded), wouldn't the\n> adjustment above become insufficient?\n> \n> In other words, would we want to flip the order these packs get\n> created in the \"setup\" part, in addition to the use of chmtime\n> (which reads the existing file timestamp using stat(2) and then\n> updates the file timestamp relative to the original timestamp)\n> we see here?\n> \n> Also, in the best case (i.e. original timestamp of A/B/C/D are the\n> same), the above seems to assume that the filesystem has at least 1\n> second file timestamp granularity. Do we want to make them at least\n> 2 seconds apart, or am I worried too much about ancient filesystems\n> that no longer metter?\n\nThe old test relied on one-second granularity, so that hasn't changed.\nI could easily space it out a bit more without issue.\n\nYour concern about the original timestamps getting skewed shouldn't\nbe an issue because \"test-tool chmtime =-\" subtracts the\n from the current system time, not the file's mtime. This\nis subtle: without the \"=\" it would modify it from the file's mtime.\n\nSince we are assigning the offset values in the proper order (D to A)\nthere isn't an issue if time ticks forward between these steps.\n\nThanks,\n-Stolee\n"},{"id":"394566","messageId":"20200402125113.GA3091800@coredump.intra.peff.net","threadId":"53153","inReplyTo":"8e6b1f75-534a-ad79-544b-126ea61b2058@gmail.com","subject":"Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-02T12:51:13Z","receivedAt":"2020-04-02T12:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 01, 2020 at 08:06:47PM -0400, Derrick Stolee wrote:\n\n> > Also, in the best case (i.e. original timestamp of A/B/C/D are the\n> > same), the above seems to assume that the filesystem has at least 1\n> > second file timestamp granularity. Do we want to make them at least\n> > 2 seconds apart, or am I worried too much about ancient filesystems\n> > that no longer metter?\n> \n> The old test relied on one-second granularity, so that hasn't changed.\n> I could easily space it out a bit more without issue.\n> \n> Your concern about the original timestamps getting skewed shouldn't\n> be an issue because \"test-tool chmtime =-\" subtracts the\n> from the current system time, not the file's mtime. This\n> is subtle: without the \"=\" it would modify it from the file's mtime.\n> \n> Since we are assigning the offset values in the proper order (D to A)\n> there isn't an issue if time ticks forward between these steps.\n\nI wonder if that would run afoul of the same \"mtime and system clock are\nnot quite the same\" issue we saw recently in [1].\n\nI think it might not because we're only comparing mtimes set through the\nsame mechanism (find the system time, subtract from it, and assign to\nmtime). If system time is monotonically increasing at any rate, that\nwould produce the desired effect.\n\nThat said, it seems so easy to just give it a 10 second (or even 10\nminute) spread to avoid any possibility of confusion.\n\n-Peff\n\n[1] https://lore.kernel.org/git/pull.725.git.git.1584125875550.gitgitgadget@gmail.com/\n"},{"id":"394603","messageId":"xmqqo8s94zgf.fsf@gitster.c.googlers.com","threadId":"53153","inReplyTo":"20200402125113.GA3091800@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T16:36:16Z","receivedAt":"2020-04-02T16:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 01, 2020 at 08:06:47PM -0400, Derrick Stolee wrote:\n>\n> I wonder if that would run afoul of the same \"mtime and system clock are\n> not quite the same\" issue we saw recently in [1].\n\nI had the exact thing in mind, which was why I suggested \"chmtime\"\nin the first place (the second reason was that it was easy to do\nrelative time using the tool than \"date + touch\"), but I missed that\nthe tool has a mode that uses timestamp relative to system clock.\n\n> I think it might not because we're only comparing mtimes set\n> through the same mechanism (find the system time, subtract from\n> it, and assign to mtime). If system time is monotonically\n> increasing at any rate, that would produce the desired effect.\n\nBut in this particular case, I think, even though setting the\nfilesystem timestamp based on something that was obtained from the\nsystem clock feels as if we are asking for trouble, the values we\nlater read back from the filesystem for executing the codepath being\ntested (namely, expiration logic) get compared with another value\nthat is derived from from the system clock (i.e. expiration cutoff),\nso in that sense you might be able to argue that it is even more\ncorrect ;-)\n\n"}]}