"git reflog expire" blindly trusting timestamps in reflogs
[Changed subject]
On Sat, Oct 09 2021, René Scharfe wrote:
> Turn off automatic background maintenance for perf tests by default to
> avoid interference with performance measurements.
Turning off background GC during the perf tests seems like a good idea in general, so I think this patch should go in. Even with the WIP (haven't picked it up again in a while) test mode I menitoned in[1] it still wouldn't make any sense to run detached background GC in t/perf.
Because first of all we take the repo as-is (hardlinks and all), so changing it is a bug in itself.
But...
Show 7 quoted lines
> Do that by using the
> new file t/perf/config and using it as the system config file for perf
> tests. Future tests intended to measure gc performance can override
> the setting locally or call "git gc" explicitly.
>
> This fixes a breakage in p2000 caused by gc automatically emptying the
> reflog due its fake dates from 2005 being older than 90 days.
I think the behavior of git-reflog is probably just broken here and needs fixing in general.
I.e. we don't actually change the ctime the relevant reflog files, so surely this is pointing to deeper edge cases where e.g. someone might import old history, only to see it wiped away due to the commits being "old", but the mtime is just seconds/minutes old...
But I never had the time/chance to dig into that, maybe there's a good reason for it.
Of course we can't rely on *just* the mtime since the whole point is to expire older entries, we need to walk the file and trust the timestamps therein. See the show_one_reflog_ent().
But my recollection & reading of [2] is that we nuke the whole file. Surely we should have a "uh oh?" safeguard when we encounter a file whose tip timestamp_t is from 2005, it's 2021, and our lstat() mtime says it was changed seconds ago.
I.e. shouldn't reflog expiry at least have the invariant that nothing should expire in a given file if the tip commit is old enough to be expired, *AND* the mtime of the file is more recent than the expiry epoch, because such a scenario points to either git's own test suite (and we should just fake up the time there), or that we're about to corrupt some user's repository because they're either doing similar import work, or their clock was screwy?
Show 31 quoted lines
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> t/perf/config | 2 ++
> t/perf/perf-lib.sh | 4 ++++
> 2 files changed, 6 insertions(+)
> create mode 100644 t/perf/config
>
> diff --git a/t/perf/config b/t/perf/config
> new file mode 100644
> index 0000000000..b92768b039
> --- /dev/null
> +++ b/t/perf/config
> @@ -0,0 +1,2 @@
> +[gc]
> + auto = 0
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index f74cfd35d6..4c4c568a37 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -27,6 +27,10 @@ TEST_NO_MALLOC_CHECK=t
>
> . ../test-lib.sh
>
> +unset GIT_CONFIG_NOSYSTEM
> +GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
> +export GIT_CONFIG_SYSTEM
> +
> if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
> then
> error "Do not use GIT_TEST_INSTALLED with the perf tests.
1. https://lore.kernel.org/git/8735pfjg47.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/b25ac1cc-8e77-17e6-602a-b289c1e1cfeb@web.de/
René Scharfe
