Patrick Steinhardt <ps@xxxxxx> writes:
> We're about to add another task for git-maintenance(1) that prunes stale
> rerere entries via `git rerere gc`. The condition of when to run this
> subcommand will be configurable so that the subcommand is only executed
> when a certain number of stale rerere entries exists. This requires us
> to know about the number of stale rerere entries in the first place,
> which is non-trivial to figure out.
>
> Refactor `rerere_gc()` and `prune_one()` so that garbage collection is
> split into three phases:
>
> 1. We collect any stale rerere entries and directories that are about
> to become empty.
>
> 2. Prune all stale rerere entries.
>
> 3. Remove all directories that should have become empty in (2).
>
> By splitting out the collection of stale entries we can trivially expose
> this function to external callers and thus reuse it in later steps.
>
> This refactoring is not expected to result in a user-visible change in
> behaviour.
I have no objection against the goal of allowing "git maintenance"
drive "git rerere gc", and as the primary author of this code path I
do not see anything wrong, in the "correctness" sense, in the
updated code.
I however am not sure if "count what we would prune, and remove only
when there are too many" would work well for this subsystem, because
I expect that the cost to enumerate existing rerere entries and
check each of them for staleness would be the dominant part,
relative to actual "rm -fr <rerere-id>", of the cost you are paying
when you run "git rerere gc".
And if my suspicion is correct, all this change does to the plain
vanilla user of "git rerere gc" is to have them pay the extra cost
of allocating and deallocating the list of names of paths in string
lists.
We need to see some performance measurement to show that the we pay
for collection and counting is a lot smaller compared to the whole
pruning operation to justify the "auto" thing.
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> rerere.c | 92 ++++++++++++++++++++++++++++++++++++++++++++--------------------
> rerere.h | 14 ++++++++++
> 2 files changed, 78 insertions(+), 28 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 740e8ad1a0b..eb06e5f8bea 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -1202,8 +1202,8 @@ static void unlink_rr_item(struct rerere_id *id)
> strbuf_release(&buf);
> }
>
> -static void prune_one(struct rerere_id *id,
> - timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
> +static int is_stale(struct rerere_id *id,
> + timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
> {
> timestamp_t then;
> timestamp_t cutoff;
> @@ -1214,11 +1214,11 @@ static void prune_one(struct rerere_id *id,
> else {
> then = rerere_created_at(id);
> if (!then)
> - return;
> + return 0;
> cutoff = cutoff_noresolve;
> }
> - if (then < cutoff)
> - unlink_rr_item(id);
> +
> + return then < cutoff;
> }
>
> /* Does the basename in "path" look plausibly like an rr-cache entry? */
> @@ -1229,29 +1229,35 @@ static int is_rr_cache_dirname(const char *path)
> return !parse_oid_hex(path, &oid, &end) && !*end;
> }
>
> -void rerere_gc(struct repository *r, struct string_list *rr)
> +int rerere_collect_stale_entries(struct repository *r,
> + struct string_list *prunable_dirs,
> + struct rerere_id **prunable_entries,
> + size_t *prunable_entries_nr)
> {
> - struct string_list to_remove = STRING_LIST_INIT_DUP;
> - DIR *dir;
> - struct dirent *e;
> - int i;
> timestamp_t now = time(NULL);
> timestamp_t cutoff_noresolve = now - 15 * 86400;
> timestamp_t cutoff_resolve = now - 60 * 86400;
> struct strbuf buf = STRBUF_INIT;
> + size_t prunable_entries_alloc;
> + struct dirent *e;
> + DIR *dir = NULL;
> + int ret;
>
> - if (setup_rerere(r, rr, 0) < 0)
> - return;
> + *prunable_entries = NULL;
> + *prunable_entries_nr = 0;
> + prunable_entries_alloc = 0;
>
> - repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
> + repo_config_get_expiry_in_days(r, "gc.rerereresolved",
> &cutoff_resolve, now);
> - repo_config_get_expiry_in_days(the_repository, "gc.rerereunresolved",
> + repo_config_get_expiry_in_days(r, "gc.rerereunresolved",
> &cutoff_noresolve, now);
> - git_config(git_default_config, NULL);
> - dir = opendir(repo_git_path_replace(the_repository, &buf, "rr-cache"));
> - if (!dir)
> - die_errno(_("unable to open rr-cache directory"));
> - /* Collect stale conflict IDs ... */
> +
> + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache"));
> + if (!dir) {
> + ret = error_errno(_("unable to open rr-cache directory"));
> + goto out;
> + }
> +
> while ((e = readdir_skip_dot_and_dotdot(dir))) {
> struct rerere_dir *rr_dir;
> struct rerere_id id;
> @@ -1266,23 +1272,53 @@ void rerere_gc(struct repository *r, struct string_list *rr)
> for (id.variant = 0, id.collection = rr_dir;
> id.variant < id.collection->status_nr;
> id.variant++) {
> - prune_one(&id, cutoff_resolve, cutoff_noresolve);
> - if (id.collection->status[id.variant])
> + if (is_stale(&id, cutoff_resolve, cutoff_noresolve)) {
> + ALLOC_GROW(*prunable_entries, *prunable_entries_nr + 1,
> + prunable_entries_alloc);
> + (*prunable_entries)[(*prunable_entries_nr)++] = id;
> + } else {
> now_empty = 0;
> + }
> }
> if (now_empty)
> - string_list_append(&to_remove, e->d_name);
> + string_list_append(prunable_dirs, e->d_name);
> }
> - closedir(dir);
>
> - /* ... and then remove the empty directories */
> - for (i = 0; i < to_remove.nr; i++)
> - rmdir(repo_git_path_replace(the_repository, &buf,
> - "rr-cache/%s", to_remove.items[i].string));
> + ret = 0;
> +
> +out:
> + strbuf_release(&buf);
> + if (dir)
> + closedir(dir);
> + return ret;
> +}
> +
> +void rerere_gc(struct repository *r, struct string_list *rr)
> +{
> + struct string_list prunable_dirs = STRING_LIST_INIT_DUP;
> + struct rerere_id *prunable_entries;
> + struct strbuf buf = STRBUF_INIT;
> + size_t prunable_entries_nr;
> +
> + if (setup_rerere(r, rr, 0) < 0)
> + return;
> +
> + git_config(git_default_config, NULL);
> +
> + if (rerere_collect_stale_entries(r, &prunable_dirs, &prunable_entries,
> + &prunable_entries_nr) < 0)
> + exit(127);
> +
> + for (size_t i = 0; i < prunable_entries_nr; i++)
> + unlink_rr_item(&prunable_entries[i]);
> + for (size_t i = 0; i < prunable_dirs.nr; i++)
> + rmdir(repo_git_path_replace(r, &buf, "rr-cache/%s",
> + prunable_dirs.items[i].string));
>
> - string_list_clear(&to_remove, 0);
> + string_list_clear(&prunable_dirs, 0);
> rollback_lock_file(&write_lock);
> strbuf_release(&buf);
> + free(prunable_entries);
> }
>
> /*
> diff --git a/rerere.h b/rerere.h
> index d4b5f7c9320..fd5a2388b06 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -37,6 +37,20 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
> int rerere_forget(struct repository *, struct pathspec *);
> int rerere_remaining(struct repository *, struct string_list *);
> void rerere_clear(struct repository *, struct string_list *);
> +
> +/*
> + * Collect prunable rerere entries that would be garbage collected via
> + * `rerere_gc()`. Whether or not an entry is prunable depends on both
> + * "gc.rerereResolved" and "gc.rerereUnresolved".
> + *
> + * Returns 0 on success, a negative error code in case entries could not be
> + * collected.
> + */
> +int rerere_collect_stale_entries(struct repository *r,
> + struct string_list *prunable_dirs,
> + struct rerere_id **prunable_entries,
> + size_t *prunable_entries_nr);
> +
> void rerere_gc(struct repository *, struct string_list *);
>
> #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
