[PATCH] commit-graph: fix buggy --expire-time option

[PATCH] commit-graph: fix buggy --expire-time option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
    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(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4a70b33fb5f..8000ff0d2ee 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -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(),
 	};
 
diff --git a/commit-graph.c b/commit-graph.c
index f013a84e294..0d0d37787a0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 53b2e6b4555..4e4efcaff22 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Derrick Stolee]

On 4/1/2020 2:11 PM, Derrick Stolee via GitGitGadget wrote:
> Also I noticed that the help text was copied from the --max-commits
> option. Fix that help text.
...
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4a70b33fb5f..8000ff0d2ee 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -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:

> +			N_("do not expire files newer than a given date-time")),

-Stolee

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Taylor Blau]

On Wed, Apr 01, 2020 at 02:17:50PM -0400, Derrick Stolee wrote:
> On 4/1/2020 2:11 PM, Derrick Stolee via GitGitGadget wrote:
> > Also I noticed that the help text was copied from the --max-commits
> > option. Fix that help text.
> ...
> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> > index 4a70b33fb5f..8000ff0d2ee 100644
> > --- a/builtin/commit-graph.c
> > +++ b/builtin/commit-graph.c
> > @@ -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:
>
> > +			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.

> -Stolee

Thanks,
Taylor

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Taylor Blau]

On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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".

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

> 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.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     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(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4a70b33fb5f..8000ff0d2ee 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -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(),
>  	};
>
> diff --git a/commit-graph.c b/commit-graph.c
> index f013a84e294..0d0d37787a0 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -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);
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 53b2e6b4555..4e4efcaff22 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -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?

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

Thanks,
Taylor

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Eric Sunshine]

On Wed, Apr 1, 2020 at 3:27 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:
> > -             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>").

Re: [PATCH] commit-graph: fix buggy --expire-time option

[SZEDER Gábor]

On Wed, Apr 01, 2020 at 06:11:35PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     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(-)
> 
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4a70b33fb5f..8000ff0d2ee 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -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(),
>  	};
>  
> diff --git a/commit-graph.c b/commit-graph.c
> index f013a84e294..0d0d37787a0 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -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);
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 53b2e6b4555..4e4efcaff22 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -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


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

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 53b2e6b4555..4e4efcaff22 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -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 ;-)?

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Jeff King]

On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> > index 53b2e6b4555..4e4efcaff22 100755
> > --- a/t/t5324-split-commit-graph.sh
> > +++ b/t/t5324-split-commit-graph.sh
> > @@ -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

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Derrick Stolee]

On 4/1/2020 3:49 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
>> index 53b2e6b4555..4e4efcaff22 100755
>> --- a/t/t5324-split-commit-graph.sh
>> +++ b/t/t5324-split-commit-graph.sh
>> @@ -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.
 
> 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

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:
>
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>> > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
>> > index 53b2e6b4555..4e4efcaff22 100755
>> > --- a/t/t5324-split-commit-graph.sh
>> > +++ b/t/t5324-split-commit-graph.sh
>> > @@ -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.

Indeed.  We should fix them (#leftoverbits).

Re: [PATCH] commit-graph: fix buggy --expire-time option

[Derrick Stolee]

On 4/1/2020 4:33 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Wed, Apr 01, 2020 at 12:49:25PM -0700, Junio C Hamano wrote:
>>
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
>>>> index 53b2e6b4555..4e4efcaff22 100755
>>>> --- a/t/t5324-split-commit-graph.sh
>>>> +++ b/t/t5324-split-commit-graph.sh
>>>> @@ -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.
> 
> Indeed.  We should fix them (#leftoverbits).

I'm adding a patch to fix that now.

-Stolee

[PATCH v2 0/2] commit-graph: fix buggy --expire-time option

[Derrick Stolee via GitGitGadget]

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!

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

-- 
gitgitgadget

[PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t5319-multi-pack-index.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 43a7a66c9d1..6b20e60314d 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -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 &&
-- 
gitgitgadget

[PATCH v2 2/2] commit-graph: fix buggy --expire-time option

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/commit-graph.c        | 2 +-
 commit-graph.c                | 2 +-
 t/t5324-split-commit-graph.sh | 8 +++++++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 4a70b33fb5f..5bfba972498 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -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(),
 	};
 
diff --git a/commit-graph.c b/commit-graph.c
index f013a84e294..0d0d37787a0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -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);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 53b2e6b4555..b8b208fc3da 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -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
 	) &&
-- 
gitgitgadget

Re: [PATCH v2 0/2] commit-graph: fix buggy --expire-time option

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  * 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 ;-)

Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

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

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.

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

Re: [PATCH v2 0/2] commit-graph: fix buggy --expire-time option

[Derrick Stolee]

On 4/1/2020 5:05 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>  * 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

Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'

[Derrick Stolee]

On 4/1/2020 5:35 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
> 
>> 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.
> 
>>  		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* &&
> 
> 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?

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'

[Jeff King]

On Wed, Apr 01, 2020 at 08:06:47PM -0400, Derrick Stolee wrote:

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

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

[1] https://lore.kernel.org/git/pull.725.git.git.1584125875550.gitgitgadget@gmail.com/

Re: [PATCH v2 1/2] t5319: replace 'touch -m' with 'test-tool chmtime'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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