Download raw body.
make gotadmin cleanup pack the repository before cleaning
Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Jan 21, 2025 at 11:02:43AM +0100, Stefan Sperling wrote:
> > Our cleanup implementation is only safe to use after everything
> > referenced has been packed into a single pack file. Otherwise, the
> > algorithm we use for checking pack redundancy might remove small
> > packs which contain objects that other objects depend on.
> >
> > The easy fix for this issue is to have 'gotadmin cleanup' create the
> > required pack file before cleaning up, making cleanup safe by default.
> > This happens to be what 'git gc' does as well.
This will be good so I can remove gotadmin pack from my mirrors'
gotsync script
> Two fixes on top:
>
> 1) pack progress output should go to stdout, not stderr.
>
> 2) cleanup -n output was inaccurate since pack generation was skipped.
> Instead, create a pack file, figure out the dry-run numbers to show,
> and then delete the generated pack again. Not ideal, but this is the
> easiest way to keep cleanup -n working.
>
> ok?
The diff reads good and I think the simple solution is the way to go;
regress is also happy.
ok for me with a couple small nits inline; I've included a diff below
that you can apply on top and commit if in agreement
> M gotadmin/gotadmin.c | 23+ 12-
> M gotd/repo_read.c | 1+ 1-
> M include/got_repository_admin.h | 10+ 7-
> M lib/got_lib_pack_create.h | 1+ 1-
> M lib/pack_create.c | 11+ 10-
> M lib/pack_create_io.c | 2+ 2-
> M lib/pack_create_privsep.c | 3+ 3-
> M lib/repository_admin.c | 148+ 55-
> M lib/send.c | 1+ 1-
> M regress/cmdline/cleanup.sh | 6+ 6-
> M regress/cmdline/common.sh | 1+ 1-
>
> 11 files changed, 207 insertions(+), 99 deletions(-)
>
> commit - 85db99906136b77c3a31a58c53ffc746b1568353
> commit + 1bd17f431a34f45c0d71b44e2026bcca9685d023
> blob - 5a8c2bc8ec21628c9260de98ee4077f5f878ff15
> blob + a634d86f4c245b3288c0f76c18f1ccf81d285d62
> --- gotadmin/gotadmin.c
> +++ gotadmin/gotadmin.c
> @@ -520,7 +520,7 @@ print_load_info(FILE *out, int print_colored, int prin
> static const struct got_error *
> pack_progress(void *arg, int ncolored, int nfound, int ntrees,
> off_t packfile_size, int ncommits, int nobj_total, int nobj_deltify,
> - int nobj_written)
> + int nobj_written, int pack_done)
> {
> struct got_pack_progress_arg *a = arg;
> char scaled_size[FMT_SCALED_STRSIZE];
> @@ -621,17 +621,18 @@ pack_progress(void *arg, int ncolored, int nfound, int
> if (print_written)
> fprintf(a->out, "; writing pack: %*s %d%%",
> FMT_SCALED_STRSIZE - 2, scaled_size, p_written);
> - if (print_searching || print_total || print_deltify ||
> - print_written) {
> + if (print_searching || print_total || print_deltify || print_written) {
> a->printed_something = 1;
> fflush(a->out);
> }
> + if (pack_done)
> + fprintf(a->out, "\n");
> return NULL;
> }
>
> static const struct got_error *
> pack_index_progress(void *arg, off_t packfile_size, int nobj_total,
> - int nobj_indexed, int nobj_loose, int nobj_resolved)
> + int nobj_indexed, int nobj_loose, int nobj_resolved, int indexing_done)
> {
> struct got_pack_progress_arg *a = arg;
> char scaled_size[FMT_SCALED_STRSIZE];
> @@ -678,7 +679,9 @@ pack_index_progress(void *arg, off_t packfile_size, in
> printf("; indexing %d%%", p_indexed);
> if (print_resolved)
> printf("; resolving deltas %d%%", p_resolved);
> - if (print_size || print_indexed || print_resolved)
> + if (indexing_done)
> + printf("\n");
> + if (print_size || print_indexed || print_resolved || indexing_done)
> fflush(stdout);
>
> return NULL;
> @@ -719,7 +722,7 @@ cmd_pack(int argc, char *argv[])
> struct got_repository *repo = NULL;
> int ch, i, loose_obj_only = 1, force_refdelta = 0, verbosity = 0;
> struct got_object_id *pack_hash = NULL;
> - char *id_str = NULL;
> + char *id_str = NULL, *idxpath = NULL;
> struct got_pack_progress_arg ppa;
> FILE *packfile = NULL;
> struct got_pathlist_head exclude_args;
> @@ -844,7 +847,7 @@ cmd_pack(int argc, char *argv[])
> if (verbosity >= 0)
> printf("\nWrote %s.pack\n", id_str);
>
> - error = got_repo_index_pack(packfile, pack_hash, repo,
> + error = got_repo_index_pack(&idxpath, packfile, pack_hash, repo,
> pack_index_progress, &ppa, check_cancelled, NULL);
> if (error)
> goto done;
> @@ -865,6 +868,7 @@ done:
> free(id_str);
> free(pack_hash);
> free(repo_path);
> + free(idxpath);
> return error;
> }
>
> @@ -883,7 +887,7 @@ cmd_indexpack(int argc, char *argv[])
> struct got_repository *repo = NULL;
> int ch;
> struct got_object_id *pack_hash = NULL;
> - char *packfile_path = NULL;
> + char *packfile_path = NULL, *idxpath = NULL;
> char *id_str = NULL;
> struct got_pack_progress_arg ppa;
> FILE *packfile = NULL;
> @@ -939,7 +943,7 @@ cmd_indexpack(int argc, char *argv[])
> if (error)
> goto done;
>
> - error = got_repo_index_pack(packfile, pack_hash, repo,
> + error = got_repo_index_pack(&idxpath, packfile, pack_hash, repo,
> pack_index_progress, &ppa, check_cancelled, NULL);
> if (error)
> goto done;
> @@ -955,6 +959,7 @@ done:
> }
> free(id_str);
> free(pack_hash);
> + free(idxpath);
> return error;
> }
>
> @@ -1266,6 +1271,7 @@ cmd_cleanup(int argc, char *argv[])
> int ch, dry_run = 0, verbosity = 0;
> int ncommits = 0, nloose = 0, npacked = 0;
> int remove_lonely_packidx = 0, ignore_mtime = 0;
> + struct got_pack_progress_arg ppa;
> struct got_cleanup_progress_arg cpa;
> struct got_lonely_packidx_progress_arg lpa;
> off_t loose_before, loose_after;
> @@ -1279,7 +1285,7 @@ cmd_cleanup(int argc, char *argv[])
> int *pack_fds = NULL;
>
> #ifndef PROFILE
> - if (pledge("stdio rpath wpath cpath flock proc exec sendfd unveil",
> + if (pledge("stdio rpath wpath cpath fattr flock proc exec sendfd unveil",
> NULL) == -1)
> err(1, "pledge");
> #endif
> @@ -1353,11 +1359,16 @@ cmd_cleanup(int argc, char *argv[])
> cpa.dry_run = dry_run;
> cpa.verbosity = verbosity;
>
> + memset(&ppa, 0, sizeof(ppa));
> + ppa.out = stdout;
> + ppa.verbosity = verbosity;
> +
> error = got_repo_cleanup(repo, &loose_before, &loose_after,
> &pack_before, &pack_after, &ncommits, &nloose, &npacked,
> dry_run, ignore_mtime, cleanup_progress, &cpa,
> + pack_progress, &ppa, pack_index_progress, &ppa,
> check_cancelled, NULL);
> - if (cpa.printed_something)
> + if (ppa.printed_something || cpa.printed_something)
> printf("\n");
> if (error)
> goto done;
> @@ -1562,7 +1573,7 @@ load_progress(void *arg, off_t packfile_size, int nobj
> int nobj_indexed, int nobj_loose, int nobj_resolved)
> {
> return pack_index_progress(arg, packfile_size, nobj_total,
> - nobj_indexed, nobj_loose, nobj_resolved);
> + nobj_indexed, nobj_loose, nobj_resolved, 0);
> }
>
> static int
> blob - 62e95bc3c4eee3e045d642b2b1a6496f839d03ce
> blob + 654887bcb69f15932fb19f6820fe2a8c5ffb698b
> --- gotd/repo_read.c
> +++ gotd/repo_read.c
> @@ -510,7 +510,7 @@ struct repo_read_pack_progress_arg {
> static const struct got_error *
> pack_progress(void *arg, int ncolored, int nfound, int ntrees,
> off_t packfile_size, int ncommits, int nobj_total, int nobj_deltify,
> - int nobj_written)
> + int nobj_written, int pack_done)
> {
> struct repo_read_pack_progress_arg *a = arg;
> struct gotd_imsg_packfile_progress iprog;
> blob - a1bb191618ec66c76deecf76d52349088c755a06
> blob + 41b7e6bd3ec9d8b9aef83734da5dfe17ea860f28
> --- include/got_repository_admin.h
> +++ include/got_repository_admin.h
> @@ -17,7 +17,7 @@
> /* A callback function which gets invoked with progress information to print. */
> typedef const struct got_error *(*got_pack_progress_cb)(void *arg,
> int ncolored, int nfound, int ntrees, off_t packfile_size, int ncommits,
> - int nobj_total, int obj_deltify, int nobj_written);
> + int nobj_total, int obj_deltify, int nobj_written, int pack_done);
>
> /*
> * Attempt to pack objects reachable via 'include_refs' into a new packfile.
> @@ -49,12 +49,12 @@ got_repo_find_pack(FILE **packfile, struct got_object_
> /* A callback function which gets invoked with progress information to print. */
> typedef const struct got_error *(*got_pack_index_progress_cb)(void *arg,
> off_t packfile_size, int nobj_total, int nobj_indexed,
> - int nobj_loose, int nobj_resolved);
> + int nobj_loose, int nobj_resolved, int indexing_done);
>
> /* (Re-)Index the pack file identified by the given hash. */
> const struct got_error *
> -got_repo_index_pack(FILE *packfile, struct got_object_id *pack_hash,
> - struct got_repository *repo,
> +got_repo_index_pack(char **idxpath, FILE *packfile,
> + struct got_object_id *pack_hash, struct got_repository *repo,
> got_pack_index_progress_cb progress_cb, void *progress_arg,
> got_cancel_cb cancel_cb, void *cancel_arg);
>
> @@ -73,9 +73,10 @@ typedef const struct got_error *(*got_cleanup_progress
> int ncommits, int nloose, int npurged, int nredundant);
>
> /*
> - * Walk objects reachable via references to determine whether any loose
> - * objects can be removed from disk. Do remove such objects from disk
> - * unless the dry_run parameter is set.
> + * Walk objects reachable via references and, unless the dry-run parameter
> + * is set, pack all referenced objects into a single pack.
> + * Then determine whether any loose objects can be removed from disk.
> + * Do remove such objects from disk unless the dry_run parameter is set.
> * Do not remove objects with a modification timestamp above an
> * implementation-defined timestamp threshold, unless ignore_mtime is set.
> * Remove packfiles which objects are either unreachable or provided
> @@ -91,6 +92,8 @@ got_repo_cleanup(struct got_repository *repo,
> int *ncommits, int *nloose,
> int *npacked, int dry_run, int ignore_mtime,
> got_cleanup_progress_cb progress_cb, void *progress_arg,
> + got_pack_progress_cb pack_progress_cb, void *pack_progress_arg,
> + got_pack_index_progress_cb index_progress_cb, void *index_progress_arg,
> got_cancel_cb cancel_cb, void *cancel_arg);
>
> /* A callback function which gets invoked with cleanup information to print. */
> blob - 8bf80e3bec6bdb3edf09b007a268b9fb8eb96d0b
> blob + 265d2bf372d90b0b2c3c124b8d9885056f6f1250
> --- lib/got_lib_pack_create.h
> +++ lib/got_lib_pack_create.h
> @@ -106,7 +106,7 @@ const struct got_error *
> got_pack_report_progress(got_pack_progress_cb progress_cb, void *progress_arg,
> struct got_ratelimit *rl, int ncolored, int nfound, int ntrees,
> off_t packfile_size, int ncommits, int nobj_total, int obj_deltify,
> - int nobj_written);
> + int nobj_written, int pack_done);
>
> const struct got_error *
> got_pack_load_packed_object_ids(int *found_all_objects,
> blob - c6787c3643ca1ced91d4ef5d2530f153f72bb6e0
> blob + 7cbe1bb9a78f7c06fed0b49bd3658873b9882862
> --- lib/pack_create.c
> +++ lib/pack_create.c
> @@ -445,7 +445,7 @@ const struct got_error *
> got_pack_report_progress(got_pack_progress_cb progress_cb, void *progress_arg,
> struct got_ratelimit *rl, int ncolored, int nfound, int ntrees,
> off_t packfile_size, int ncommits, int nobj_total, int obj_deltify,
> - int nobj_written)
> + int nobj_written, int pack_done)
> {
> const struct got_error *err;
> int elapsed;
> @@ -458,7 +458,8 @@ got_pack_report_progress(got_pack_progress_cb progress
> return err;
>
> return progress_cb(progress_arg, ncolored, nfound, ntrees,
> - packfile_size, ncommits, nobj_total, obj_deltify, nobj_written);
> + packfile_size, ncommits, nobj_total, obj_deltify, nobj_written,
> + pack_done);
> }
>
> const struct got_error *
> @@ -566,7 +567,7 @@ pick_deltas(struct got_pack_meta **meta, int nmeta, in
> }
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> ncolored, nfound, ntrees, 0L, ncommits, nreused + nmeta,
> - nreused + i, 0);
> + nreused + i, 0, 0);
> if (err)
> goto done;
> m = meta[i];
> @@ -750,7 +751,7 @@ got_pack_add_object(int want_meta, struct got_object_i
>
> (*nfound)++;
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> - *ncolored, *nfound, *ntrees, 0L, 0, 0, 0, 0);
> + *ncolored, *nfound, *ntrees, 0L, 0, 0, 0, 0, 0);
> if (err) {
> clear_meta(m);
> free(m);
> @@ -781,7 +782,7 @@ got_pack_load_tree_entries(struct got_object_id_queue
>
> (*ntrees)++;
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> - *ncolored, *nfound, *ntrees, 0L, 0, 0, 0, 0);
> + *ncolored, *nfound, *ntrees, 0L, 0, 0, 0, 0, 0);
> if (err)
> return err;
>
> @@ -1764,7 +1765,7 @@ genpack(struct got_object_id *pack_hash, int packfd,
> for (i = 0; i < ndeltify; i++) {
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> ncolored, nfound, ntrees, packfile_size, nours,
> - ndeltify + nreuse, ndeltify + nreuse, i);
> + ndeltify + nreuse, ndeltify + nreuse, i, 0);
> if (err)
> goto done;
> m = deltify[i];
> @@ -1793,7 +1794,7 @@ genpack(struct got_object_id *pack_hash, int packfd,
> for (i = 0; i < nreuse; i++) {
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> ncolored, nfound, ntrees, packfile_size, nours,
> - ndeltify + nreuse, ndeltify + nreuse, ndeltify + i);
> + ndeltify + nreuse, ndeltify + nreuse, ndeltify + i, 0);
> if (err)
> goto done;
> m = reuse[i];
> @@ -1813,7 +1814,7 @@ genpack(struct got_object_id *pack_hash, int packfd,
> if (progress_cb) {
> err = progress_cb(progress_arg, ncolored, nfound, ntrees,
> packfile_size, nours, ndeltify + nreuse,
> - ndeltify + nreuse, ndeltify + nreuse);
> + ndeltify + nreuse, ndeltify + nreuse, 1);
> if (err)
> goto done;
> }
> @@ -1875,7 +1876,7 @@ got_pack_create(struct got_object_id *packhash, int pa
>
> if (progress_cb) {
> err = progress_cb(progress_arg, ncolored, nfound, ntrees,
> - 0L, nours, got_object_idset_num_elements(idset), 0, 0);
> + 0L, nours, got_object_idset_num_elements(idset), 0, 0, 0);
> if (err)
> goto done;
> }
> @@ -1946,7 +1947,7 @@ got_pack_create(struct got_object_id *packhash, int pa
> err = progress_cb(progress_arg, ncolored, nfound, ntrees,
> 1 /* packfile_size */, nours,
> got_object_idset_num_elements(idset),
> - deltify.nmeta + reuse.nmeta, 0);
> + deltify.nmeta + reuse.nmeta, 0, 0);
> if (err)
> goto done;
> }
> blob - 5aa56e154c58be00385c847ac667f05de6d70d6e
> blob + 0d826389e2105b3dffcab6046a757b0fdb83a8ad
> --- lib/pack_create_io.c
> +++ lib/pack_create_io.c
> @@ -156,7 +156,7 @@ search_delta_for_object(struct got_object_id *id, void
>
> err = got_pack_report_progress(a->progress_cb, a->progress_arg,
> a->rl, a->ncolored, a->nfound, a->ntrees, 0L, a->ncommits,
> - got_object_idset_num_elements(a->idset), a->v->nmeta, 0);
> + got_object_idset_num_elements(a->idset), a->v->nmeta, 0, 0);
> if (err)
> goto done;
> }
> @@ -312,7 +312,7 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> }
>
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> - *ncolored, 0, 0, 0L, 0, 0, 0, 0);
> + *ncolored, 0, 0, 0L, 0, 0, 0, 0, 0);
> if (err)
> break;
>
> blob - 609522bd15cada03e452f340e81a8ffce06233e2
> blob + ea20b9c4b08199a59e5e822d3319990fd6f175fc
> --- lib/pack_create_privsep.c
> +++ lib/pack_create_privsep.c
> @@ -190,7 +190,7 @@ got_pack_search_deltas(struct got_packidx **packidx, s
>
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> ncolored, nfound, ntrees, 0L, ncommits,
> - got_object_idset_num_elements(idset), v->nmeta, 0);
> + got_object_idset_num_elements(idset), v->nmeta, 0, 0);
> if (err)
> break;
> }
> @@ -263,7 +263,7 @@ recv_painted_commit(void *arg, struct got_object_id *i
> }
>
> return got_pack_report_progress(a->progress_cb, a->progress_arg, a->rl,
> - *a->ncolored, 0, 0, 0L, 0, 0, 0, 0);
> + *a->ncolored, 0, 0, 0L, 0, 0, 0, 0, 0);
> }
>
> static const struct got_error *
> @@ -447,7 +447,7 @@ got_pack_paint_commits(int *ncolored, struct got_objec
> }
>
> err = got_pack_report_progress(progress_cb, progress_arg, rl,
> - *ncolored, 0, 0, 0L, 0, 0, 0, 0);
> + *ncolored, 0, 0, 0L, 0, 0, 0, 0, 0);
> if (err)
> break;
>
> blob - ba9e6ae53cbffd025f1157b73322e282138bf3d3
> blob + 3659093bee84b25db368dbe3c0e93a76adf6ed2d
> --- lib/repository_admin.c
> +++ lib/repository_admin.c
> @@ -142,6 +142,79 @@ done:
> return err;
> }
>
> +static const struct got_error *
> +create_temp_packfile(int *packfd, char **tmpfile_path,
> + struct got_repository *repo)
> +{
> + const struct got_error *err = NULL;
> + char *path;
> +
> + *packfd = -1;
> +
> + if (asprintf(&path, "%s/%s/packing.pack",
> + got_repo_get_path_git_dir(repo), GOT_OBJECTS_PACK_DIR) == -1)
> + return got_error_from_errno("asprintf");
> +
> + err = got_opentemp_named_fd(tmpfile_path, packfd, path, "");
> + if (err)
> + goto done;
> +
> + if (fchmod(*packfd, GOT_DEFAULT_PACK_MODE) == -1)
> + err = got_error_from_errno2("fchmod", *tmpfile_path);
> +done:
path is leaked in this routine:
free(path);
> + if (err) {
> + close(*packfd);
We need to ensure *packfd is not -1 before passing to close(2)
as got_opentemp_named_fd() does not return a valid fd on error:
if (*packfd != -1)
close(*packfd);
> + *packfd = -1;
> + free(*tmpfile_path);
> + *tmpfile_path = NULL;
> + }
> + return err;
> +}
> +
> +static const struct got_error *
> +install_packfile(FILE **packfile, int *packfd, char **packfile_path,
> + char **tmpfile_path, struct got_object_id *pack_hash,
> + struct got_repository *repo)
> +{
> + const struct got_error *err;
> + char *hash_str;
> +
> + err = got_object_id_str(&hash_str, pack_hash);
> + if (err)
> + return err;
> +
> + if (asprintf(packfile_path, "%s/%s/pack-%s.pack",
> + got_repo_get_path_git_dir(repo), GOT_OBJECTS_PACK_DIR,
> + hash_str) == -1) {
> + err = got_error_from_errno("asprintf");
> + goto done;
> + }
> +
> + if (lseek(*packfd, 0L, SEEK_SET) == -1) {
> + err = got_error_from_errno("lseek");
> + goto done;
> + }
> +
> + if (rename(*tmpfile_path, *packfile_path) == -1) {
> + err = got_error_from_errno3("rename", *tmpfile_path,
> + *packfile_path);
> + goto done;
> + }
> +
> + free(*tmpfile_path);
> + *tmpfile_path = NULL;
> +
> + *packfile = fdopen(*packfd, "w");
> + if (*packfile == NULL) {
> + err = got_error_from_errno2("fdopen", *packfile_path);
> + goto done;
> + }
> + *packfd = -1;
> +done:
> + free(hash_str);
> + return err;
> +}
> +
> const struct got_error *
> got_repo_pack_objects(FILE **packfile, struct got_object_id **pack_hash,
> struct got_reflist_head *include_refs,
> @@ -153,8 +226,7 @@ got_repo_pack_objects(FILE **packfile, struct got_obje
> const struct got_error *err = NULL;
> struct got_object_id **ours = NULL, **theirs = NULL;
> int nours = 0, ntheirs = 0, packfd = -1, i;
> - char *tmpfile_path = NULL, *path = NULL, *packfile_path = NULL;
> - char *hash_str = NULL;
> + char *tmpfile_path = NULL, *packfile_path = NULL;
> FILE *delta_cache = NULL;
> struct got_ratelimit rl;
>
> @@ -163,20 +235,10 @@ got_repo_pack_objects(FILE **packfile, struct got_obje
>
> got_ratelimit_init(&rl, 0, 500);
>
> - if (asprintf(&path, "%s/%s/packing.pack",
> - got_repo_get_path_git_dir(repo), GOT_OBJECTS_PACK_DIR) == -1) {
> - err = got_error_from_errno("asprintf");
> - goto done;
> - }
> - err = got_opentemp_named_fd(&tmpfile_path, &packfd, path, "");
> + err = create_temp_packfile(&packfd, &tmpfile_path, repo);
> if (err)
> - goto done;
> + return err;
>
> - if (fchmod(packfd, GOT_DEFAULT_PACK_MODE) == -1) {
> - err = got_error_from_errno2("fchmod", tmpfile_path);
> - goto done;
> - }
> -
> delta_cache = got_opentemp();
> if (delta_cache == NULL) {
> err = got_error_from_errno("got_opentemp");
> @@ -215,34 +277,8 @@ got_repo_pack_objects(FILE **packfile, struct got_obje
> if (err)
> goto done;
>
> - err = got_object_id_str(&hash_str, *pack_hash);
> - if (err)
> - goto done;
> - if (asprintf(&packfile_path, "%s/%s/pack-%s.pack",
> - got_repo_get_path_git_dir(repo), GOT_OBJECTS_PACK_DIR,
> - hash_str) == -1) {
> - err = got_error_from_errno("asprintf");
> - goto done;
> - }
> -
> - if (lseek(packfd, 0L, SEEK_SET) == -1) {
> - err = got_error_from_errno("lseek");
> - goto done;
> - }
> - if (rename(tmpfile_path, packfile_path) == -1) {
> - err = got_error_from_errno3("rename", tmpfile_path,
> - packfile_path);
> - goto done;
> - }
> - free(tmpfile_path);
> - tmpfile_path = NULL;
> -
> - *packfile = fdopen(packfd, "w");
> - if (*packfile == NULL) {
> - err = got_error_from_errno2("fdopen", tmpfile_path);
> - goto done;
> - }
> - packfd = -1;
> + err = install_packfile(packfile, &packfd, &packfile_path,
> + &tmpfile_path, *pack_hash, repo);
> done:
> for (i = 0; i < nours; i++)
> free(ours[i]);
> @@ -258,8 +294,6 @@ done:
> err = got_error_from_errno2("unlink", tmpfile_path);
> free(tmpfile_path);
> free(packfile_path);
> - free(hash_str);
> - free(path);
> if (err) {
> free(*pack_hash);
> *pack_hash = NULL;
> @@ -271,8 +305,8 @@ done:
> }
>
> const struct got_error *
> -got_repo_index_pack(FILE *packfile, struct got_object_id *pack_hash,
> - struct got_repository *repo,
> +got_repo_index_pack(char **idxpath, FILE *packfile,
> + struct got_object_id *pack_hash, struct got_repository *repo,
> got_pack_index_progress_cb progress_cb, void *progress_arg,
> got_cancel_cb cancel_cb, void *cancel_arg)
> {
> @@ -282,14 +316,16 @@ got_repo_index_pack(FILE *packfile, struct got_object_
> int npackfd = -1, idxfd = -1, nidxfd = -1;
> int tmpfds[3];
> int idxstatus, done = 0;
> + int nobj_total = 0, nobj_indexed = 0, nobj_loose = 0, nobj_resolved = 0;
> const struct got_error *err;
> struct imsgbuf idxibuf;
> pid_t idxpid;
> char *tmpidxpath = NULL;
> - char *packfile_path = NULL, *idxpath = NULL, *id_str = NULL;
> + char *packfile_path = NULL, *id_str = NULL;
> const char *repo_path = got_repo_get_path_git_dir(repo);
> struct stat sb;
>
> + *idxpath = NULL;
> memset(&idxibuf, 0, sizeof(idxibuf));
>
> for (i = 0; i < nitems(tmpfds); i++)
> @@ -338,7 +374,7 @@ got_repo_index_pack(FILE *packfile, struct got_object_
> goto done;
> }
>
> - if (asprintf(&idxpath, "%s/%s/pack-%s.idx",
> + if (asprintf(idxpath, "%s/%s/pack-%s.idx",
> repo_path, GOT_OBJECTS_PACK_DIR, id_str) == -1) {
> err = got_error_from_errno("asprintf");
> goto done;
> @@ -386,8 +422,6 @@ got_repo_index_pack(FILE *packfile, struct got_object_
> }
> done = 0;
> while (!done) {
> - int nobj_total, nobj_indexed, nobj_loose, nobj_resolved;
> -
> if (cancel_cb) {
> err = cancel_cb(cancel_arg);
> if (err)
> @@ -402,11 +436,18 @@ got_repo_index_pack(FILE *packfile, struct got_object_
> if (nobj_indexed != 0) {
> err = progress_cb(progress_arg, sb.st_size,
> nobj_total, nobj_indexed, nobj_loose,
> - nobj_resolved);
> + nobj_resolved, 0);
> if (err)
> break;
> }
> }
> + if (done) {
> + err = progress_cb(progress_arg, sb.st_size,
> + nobj_total, nobj_indexed, nobj_loose,
> + nobj_resolved, done);
> + if (err)
> + goto done;
> + }
> if (close(imsg_idxfds[0]) == -1) {
> err = got_error_from_errno("close");
> goto done;
> @@ -416,8 +457,8 @@ got_repo_index_pack(FILE *packfile, struct got_object_
> goto done;
> }
>
> - if (rename(tmpidxpath, idxpath) == -1) {
> - err = got_error_from_errno3("rename", tmpidxpath, idxpath);
> + if (rename(tmpidxpath, *idxpath) == -1) {
> + err = got_error_from_errno3("rename", tmpidxpath, *idxpath);
> goto done;
> }
> free(tmpidxpath);
> @@ -437,7 +478,6 @@ done:
> err = got_error_from_errno("close");
> }
> free(tmpidxpath);
> - free(idxpath);
> free(packfile_path);
> return err;
> }
> @@ -1420,6 +1460,8 @@ got_repo_cleanup(struct got_repository *repo,
> off_t *pack_before, off_t *pack_after,
> int *ncommits, int *nloose, int *npacked, int dry_run, int ignore_mtime,
> got_cleanup_progress_cb progress_cb, void *progress_arg,
> + got_pack_progress_cb pack_progress_cb, void *pack_progress_arg,
> + got_pack_index_progress_cb index_progress_cb, void *index_progress_arg,
> got_cancel_cb cancel_cb, void *cancel_arg)
> {
> const struct got_error *unlock_err, *err = NULL;
> @@ -1430,11 +1472,15 @@ got_repo_cleanup(struct got_repository *repo,
> struct got_reflist_entry *re;
> struct got_object_id **referenced_ids;
> int i, nreferenced;
> - int npurged = 0;
> + int npurged = 0, packfd = -1;
> + char *tmpfile_path = NULL, *packfile_path = NULL, *idxpath = NULL;
> + FILE *delta_cache = NULL, *packfile = NULL;
> + struct got_object_id pack_hash;
> time_t max_mtime = 0;
>
> TAILQ_INIT(&refs);
> got_ratelimit_init(&rl, 0, 500);
> + memset(&pack_hash, 0, sizeof(pack_hash));
>
> *loose_before = 0;
> *loose_after = 0;
> @@ -1448,6 +1494,16 @@ got_repo_cleanup(struct got_repository *repo,
> if (err)
> return err;
>
> + err = create_temp_packfile(&packfd, &tmpfile_path, repo);
> + if (err)
> + goto done;
> +
> + delta_cache = got_opentemp();
> + if (delta_cache == NULL) {
> + err = got_error_from_errno("got_opentemp");
> + goto done;
> + }
> +
> traversed_ids = got_object_idset_alloc();
> if (traversed_ids == NULL) {
> err = got_error_from_errno("got_object_idset_alloc");
> @@ -1486,6 +1542,28 @@ got_repo_cleanup(struct got_repository *repo,
> goto done;
> }
>
> + err = got_pack_create(&pack_hash, packfd, delta_cache,
> + NULL, 0, referenced_ids, nreferenced, repo, 0,
> + 0, 0, pack_progress_cb, pack_progress_arg,
> + &rl, cancel_cb, cancel_arg);
> + if (err)
> + goto done;
> +
> + err = install_packfile(&packfile, &packfd, &packfile_path,
> + &tmpfile_path, &pack_hash, repo);
> + if (err)
> + goto done;
> +
> + err = got_repo_index_pack(&idxpath, packfile, &pack_hash, repo,
> + index_progress_cb, index_progress_arg,
> + cancel_cb, cancel_arg);
> + if (err)
> + goto done;
> +
> + err = got_repo_list_packidx(&repo->packidx_paths, repo);
> + if (err)
> + goto done;
> +
> err = repo_purge_unreferenced_loose_objects(repo, traversed_ids,
> loose_before, loose_after, *ncommits, nloose, npacked, &npurged,
> dry_run, ignore_mtime, max_mtime, &rl, progress_cb, progress_arg,
> @@ -1500,6 +1578,12 @@ got_repo_cleanup(struct got_repository *repo,
> if (err)
> goto done;
>
> + if (dry_run) {
> + if (idxpath && unlink(idxpath) == -1)
> + err = got_error_from_errno2("unlink", idxpath);
> + if (packfile_path && unlink(packfile_path) == -1 && err == NULL)
> + err = got_error_from_errno2("unlink", packfile_path);
> + }
> done:
> if (lk) {
> unlock_err = got_lockfile_unlock(lk, got_repo_get_fd(repo));
> @@ -1508,6 +1592,15 @@ got_repo_cleanup(struct got_repository *repo,
> }
> if (traversed_ids)
> got_object_idset_free(traversed_ids);
> + if (packfd != -1 && close(packfd) == -1 && err == NULL)
> + err = got_error_from_errno2("close", packfile_path);
packfile_path may be NULL in the above got_error_from_errno2() call
because we can goto done before the call to install_packfile(),
which is where packfile_path is allocated and packfd is set to -1.
I think it's safe to assume that if packfile_path is NULL but packfd is
not -1, then tmpfile_path is still valid as tmpfile_path is not released
until after packfile_path is allocated in install_packfile():
if (packfd != -1 && close(packfd) == -1 && err == NULL)
err = got_error_from_errno2("close",
packfile_path ? packfile_path : tmpfile_path) :
There is also another instance of packfile_path being passed as a
potential NULL pointer to got_error_from_errno2() that already
exists in HEAD in got_repo_pack_objects(), which I've included
in the below diff
> + if (delta_cache && fclose(delta_cache) == EOF && err == NULL)
> + err = got_error_from_errno("fclose");
> + if (tmpfile_path && unlink(tmpfile_path) == -1 && err == NULL)
> + err = got_error_from_errno2("unlink", tmpfile_path);
> + free(tmpfile_path);
> + free(packfile_path);
> + free(idxpath);
> return err;
> }
>
> blob - ed9e497c5a0fdf9632b1c65ac9a22bdfa1d64a61
> blob + 90efc87ae3b506ea594098e26225608af3b9d3f6
> --- lib/send.c
> +++ lib/send.c
> @@ -125,7 +125,7 @@ struct pack_progress_arg {
> static const struct got_error *
> pack_progress(void *arg, int ncolored, int nfound, int ntrees,
> off_t packfile_size, int ncommits, int nobj_total, int nobj_deltify,
> - int nobj_written)
> + int nobj_written, int pack_done)
> {
> const struct got_error *err;
> struct pack_progress_arg *a = arg;
> blob - 00f32bb02afd89bd6cb1db91f4b9385e33bd5617
> blob + f4099794bb7ebc53f96a1addc9b3d92c43e8c93d
> --- regress/cmdline/cleanup.sh
> +++ regress/cmdline/cleanup.sh
> @@ -90,7 +90,7 @@ test_cleanup_unreferenced_loose_objects() {
> return 1
> fi
>
> - # cleanup should remove loose objects that belonged to the branch
> + # cleanup should remove all loose objects
> gotadmin cleanup -a -q -r $testroot/repo > $testroot/stdout
> ret=$?
> if [ $ret -ne 0 ]; then
> @@ -109,7 +109,7 @@ test_cleanup_unreferenced_loose_objects() {
>
> nloose2=`gotadmin info -r $testroot/repo | grep '^loose objects:' | \
> cut -d ':' -f 2 | tr -d ' '`
> - if [ "$nloose2" != "$nloose0" ]; then
> + if [ "$nloose2" != "0" ]; then
> echo "unexpected number of loose objects: $nloose2" >&2
> test_done "$testroot" "1"
> return 1
> @@ -304,8 +304,8 @@ test_cleanup_redundant_pack_files() {
> gotadmin cleanup -a -q -r "$testroot/repo"
>
> n=$(gotadmin info -r "$testroot/repo" | awk '/^pack files/{print $3}')
> - if [ "$n" -ne 3 ]; then
> - echo "expected 3 pack files left, $n found instead" >&2
> + if [ "$n" -ne 2 ]; then
> + echo "expected 2 pack files left, $n found instead" >&2
> test_done "$testroot" 1
> return 1
> fi
> @@ -326,8 +326,8 @@ test_cleanup_redundant_pack_files() {
>
> gotadmin cleanup -a -q -r "$testroot/repo"
> n=$(gotadmin info -r "$testroot/repo" | awk '/^pack files/{print $3}')
> - if [ "$n" -ne 3 ]; then
> - echo "expected 3 pack files left, $n found instead" >&2
> + if [ "$n" -ne 1 ]; then
> + echo "expected 1 pack files left, $n found instead" >&2
> test_done "$testroot" 1
> return 1
> fi
> blob - 7877214cb032a74a7787849d3f0254ee23d84e2c
> blob + a22cf957b235a6c58aa4c61d12180d3ffe2e7e3f
> --- regress/cmdline/common.sh
> +++ regress/cmdline/common.sh
> @@ -156,7 +156,7 @@ git_fsck()
> local testroot="$1"
> local repo="$2"
>
> - git -C $repo fsck --strict \
> + git -C $repo fsck --strict --no-reflogs \
> > $testroot/fsck.stdout 2> $testroot/fsck.stderr
> ret=$?
> if [ $ret -ne 0 ]; then
diff of the abovementioned changes:
M lib/repository_admin.c | 7+ 3-
1 file changed, 7 insertions(+), 3 deletions(-)
path + /home/mark/src/got
commit - 739a1bba295e9f4a4183e31cf8ccb487b363863d
blob - 3659093bee84b25db368dbe3c0e93a76adf6ed2d
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -162,8 +162,10 @@ create_temp_packfile(int *packfd, char **tmpfile_path,
if (fchmod(*packfd, GOT_DEFAULT_PACK_MODE) == -1)
err = got_error_from_errno2("fchmod", *tmpfile_path);
done:
+ free(path);
if (err) {
- close(*packfd);
+ if (*packfd != -1)
+ close(*packfd);
*packfd = -1;
free(*tmpfile_path);
*tmpfile_path = NULL;
@@ -287,7 +289,8 @@ done:
free(theirs[i]);
free(theirs);
if (packfd != -1 && close(packfd) == -1 && err == NULL)
- err = got_error_from_errno2("close", packfile_path);
+ err = got_error_from_errno2("close",
+ packfile_path ? packfile_path : tmpfile_path);
if (delta_cache && fclose(delta_cache) == EOF && err == NULL)
err = got_error_from_errno("fclose");
if (tmpfile_path && unlink(tmpfile_path) == -1 && err == NULL)
@@ -1593,7 +1596,8 @@ got_repo_cleanup(struct got_repository *repo,
if (traversed_ids)
got_object_idset_free(traversed_ids);
if (packfd != -1 && close(packfd) == -1 && err == NULL)
- err = got_error_from_errno2("close", packfile_path);
+ err = got_error_from_errno2("close",
+ packfile_path ? packfile_path : tmpfile_path);
if (delta_cache && fclose(delta_cache) == EOF && err == NULL)
err = got_error_from_errno("fclose");
if (tmpfile_path && unlink(tmpfile_path) == -1 && err == NULL)
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
make gotadmin cleanup pack the repository before cleaning