[PATCH v2 0/2] 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!

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