From: Omar Polo <op@omarpolo.com>
Subject: Re: ignore lonely packs at a lower level
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Sat, 10 Aug 2024 11:40:56 +0200

On 2024/08/09 18:05:48 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Aug 09, 2024 at 03:58:18PM +0200, Omar Polo wrote:
> > instead of having to deal with these in gotwebd, deal with the lonely
> > pack error at a lowel level, i.e. in got_repo_search_packidx and match
> > routines.
> > 
> > This also makes the repo_purge_redundant_packfile skip over these lonely
> > packs, but maybe the -p could be an implicit default behaviour of
> > cleanup.  But not in this diff :)
> 
> The problem with making -p implicit is that we cannot tell why the
> pack went missing. Maybe someone ran a bad script which moved the
> pack file away for some reason? In this case the dangling index provides
> a hint that a pack file should have existed in the repository at some point.
> 
> On the other hand, removing index files is not fatal because index files
> can always be regenerated from the pack, so perhaps we should just remove
> them, and update our documentation accordingly.

This is a valid concern and I guess also why this error is exposed also
to consumers that care about packfiles at all.

However, given that git itself seems to generate these, I'm tempted to
ignore them (when feasible) and remove them automatically at cleanup
time.

> > thoughts?
> 
> Will check the diff later, out of time for now.

here's the same diff again but with a missing `err = NULL' and without
the last hunk.  This makes the search_packidx, match_packed_object and
get_packfile_info routine implicitly skip over lonely packidxs instead
of erroring while the rest (including gotadmin cleanup) will still get
the lonely pack error.

This means when we only need to open objects (got log, tog, gotwebd) we
won't get this error, while code that tries to open specific pack{,idx}
get it.

I guess that this is safer while we figure out what to do with cleanup
with or without -p.

In any case, I'd like to commit at least the gotwebd bits that have
proven to cause issues.

diff /home/op/w/got
commit - faf51db5e8152629d9c4aa4672b3f26e6acecf10
path + /home/op/w/got
blob - ea44d67c0df8c731d5a45b797add4cbee60f171c
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -197,7 +197,7 @@ gotweb_process_request(struct request *c)
 	if (qs->action != INDEX) {
 		error = gotweb_load_got_path(&repo_dir, qs->path, c);
 		c->t->repo_dir = repo_dir;
-		if (error && error->code != GOT_ERR_LONELY_PACKIDX)
+		if (error)
 			goto err;
 	}
 
@@ -825,7 +825,7 @@ gotweb_render_index(struct template *tp)
 
 		error = gotweb_load_got_path(&repo_dir, sd_dent[d_i]->d_name,
 		    c);
-		if (error && error->code != GOT_ERR_LONELY_PACKIDX) {
+		if (error) {
 			if (error->code != GOT_ERR_NOT_GIT_REPO)
 				log_warnx("%s: %s: %s", __func__,
 				    sd_dent[d_i]->d_name, error->msg);
blob - f6c9b881e64ee0270d643a9dc809378060318b2b
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -1404,8 +1404,13 @@ got_repo_search_packidx(struct got_packidx **packidx, 
 
 		err = got_packidx_open(packidx, got_repo_get_fd(repo),
 		    path_packidx, 0, repo->algo);
-		if (err)
+		if (err) {
+			if (err->code == GOT_ERR_LONELY_PACKIDX) {
+				err = NULL;
+				continue;
+			}
 			goto done;
+		}
 
 		err = add_packidx_bloom_filter(repo, *packidx, path_packidx);
 		if (err)
@@ -1849,8 +1854,13 @@ retry:
 
 		err = got_packidx_open(&packidx, got_repo_get_fd(repo),
 		    path_packidx, 0, repo->algo);
-		if (err)
+		if (err) {
+			if (err->code == GOT_ERR_LONELY_PACKIDX) {
+				err = NULL;
+				continue;
+			}
 			break;
+		}
 
 		got_object_id_queue_free(&matched_ids);
 
@@ -2598,8 +2608,13 @@ got_repo_get_packfile_info(int *npackfiles, int *nobje
 		err = got_packidx_open(&packidx, got_repo_get_fd(repo),
 		    path_packidx, 0, repo->algo);
 		free(path_packidx);
-		if (err)
+		if (err) {
+			if (err->code == GOT_ERR_LONELY_PACKIDX) {
+				err = NULL;
+				continue;
+			}
 			goto done;
+		}
 
 		if (fstat(packidx->fd, &sb) == -1)
 			goto done;