From: Omar Polo <op@omarpolo.com>
Subject: cleanup: always remove redundant packs
To: gameoftrees@openbsd.org
Date: Sun, 22 Oct 2023 20:40:13 +0200

stsp reported that `gotadmin pack -a && gotadmin cleanup' wasn't working
in one case since rsync was constantly messing with mtime.

looking closely, I don't think we have to treat "recentish" pack files
in a special way.  If they're redundant, they can go, regardless of when
they were created.

I've taken a quick look at git' builtin/repack.c and it doesn't seem to
care about the {c,m}time of pack files.  From another quick look, gotd
should be fine too.

I probably got over-cautious and copied the loose object time check,
with for packs is not needed.  If a pack is redundant, all of its
contents are already available in the repository so there's nothing to
loose by removing it.  Loose object instead may be installed and only
later being pointed by a reference, so we still need the creation check.

ok?

diff /home/op/w/got
commit - 87bd0c08f248a23b64471e77d2b081d5b04b4fa5
path + /home/op/w/got
blob - dde785d1cee7ca7d6591b4aeae5dc65472294bc7
file + gotadmin/gotadmin.1
--- gotadmin/gotadmin.1
+++ gotadmin/gotadmin.1
@@ -315,7 +315,7 @@ The options for
 are as follows:
 .Bl -tag -width Ds
 .It Fl a
-Delete all redundant loose and packed objects.
+Delete all loose objects.
 By default, objects which are newer than an implementation-defined
 modification timestamp are kept on disk to prevent race conditions
 with other commands that add new objects to the repository while
blob - ba7276ad6779e4b6a924ef8284b01c4bbdb79216
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -1191,8 +1191,7 @@ done:
 
 static const struct got_error *
 purge_redundant_pack(struct got_repository *repo, const char *packidx_path,
-    int dry_run, int ignore_mtime, time_t max_mtime,
-    int *remove, off_t *size_before, off_t *size_after)
+    int dry_run, int *remove, off_t *size_before, off_t *size_after)
 {
 	static const char *ext[] = {".idx", ".pack", ".rev", ".bitmap",
 	    ".promisor", ".mtimes"};
@@ -1204,20 +1203,6 @@ purge_redundant_pack(struct got_repository *repo, cons
 		return got_error(GOT_ERR_NO_SPACE);
 
 	/*
-	 * Do not delete pack files which are younger than our maximum
-	 * modification time threshold.  This prevents a race where a
-	 * new pack file which is being added to the repository
-	 * concurrently would be deleted.
-	 */
-	if (fstatat(got_repo_get_fd(repo), path, &sb, 0) == -1) {
-		if (errno == ENOENT)
-			return NULL;
-		return got_error_from_errno2("fstatat", path);
-	}
-	if (!ignore_mtime && sb.st_mtime > max_mtime)
-		*remove = 0;
-
-	/*
 	 * For compatibility with Git, if a matching .keep file exist
 	 * don't delete the packfile.
 	 */
@@ -1323,8 +1308,8 @@ pack_info_cmp(const void *a, const void *b)
 static const struct got_error *
 repo_purge_redundant_packfiles(struct got_repository *repo,
     struct got_object_idset *traversed_ids,
-    off_t *size_before, off_t *size_after, int dry_run, int ignore_mtime,
-    time_t max_mtime, int nloose, int ncommits, int npurged,
+    off_t *size_before, off_t *size_after, int dry_run,
+    int nloose, int ncommits, int npurged,
     struct got_ratelimit *rl,
     got_cleanup_progress_cb progress_cb, void *progress_arg,
     got_cancel_cb cancel_cb, void *cancel_arg)
@@ -1378,7 +1363,7 @@ repo_purge_redundant_packfiles(struct got_repository *
 		if (err)
 			goto done;
 		err = purge_redundant_pack(repo, sorted[i].path, dry_run,
-		    ignore_mtime, max_mtime, &remove, size_before, size_after);
+		    &remove, size_before, size_after);
 		if (err)
 			goto done;
 		if (!remove)
@@ -1483,9 +1468,8 @@ got_repo_cleanup(struct got_repository *repo,
 		goto done;
 
 	err = repo_purge_redundant_packfiles(repo, traversed_ids,
-	    pack_before, pack_after, dry_run, ignore_mtime, max_mtime,
-	    *nloose, *ncommits, npurged, &rl, progress_cb, progress_arg,
-	    cancel_cb, cancel_arg);
+	    pack_before, pack_after, dry_run, *nloose, *ncommits,
+	    npurged, &rl, progress_cb, progress_arg, cancel_cb, cancel_arg);
 	if (err)
 		goto done;