Re: [RFC PATCH 7/7] repack-promisor: record dropped objects in a drop log

On 16/07/26 18:58, Siddharth Shrimali wrote:
> After --drop-filtered removes promisor blobs, append a record of each
> dropped object to $GIT_DIR/objects/info/promisor-dropped. Each line
> records the object ID, a reflog-style timestamp (Unix seconds and
> timezone), the filter spec, and the promisor remote it was attested
> recoverable from like the following:
> 
>    <oid> <time> <tz> filter=<spec> remote=<name>
> 
> If a dropped object later becomes unrecoverable (for example, the
> branch holding it is deleted on the promisor remote), a lazy fetch
> fails with a generic error. This persistent record lets a later change
> explain that the object was dropped deliberately, when, under which
> filter, and from which remote it was expected to be recoverable.


I like the idea of better errors when a later lazy fetch fails.

An alternative would be to wait until we actually have that error-path
change in the same series, so we do not grow an on-disk format that
nothing reads yet. I think keeping the log in the RFC is fine though
if you find it useful while developing; I would not treat it as
required for the first mergeable version.



> 
> The remote field lists all configured promisor remotes rather than the
> specific one each dropped object is recoverable from. Determining the
> exact remote would require asking the remote whether it has the object.
> A "remote-object-info" command is being added to the "git cat-file
> --batch" protocol for this kind of query, but it is not available yet.
> A NEEDSWORK marks this for a follow-up.
> 
> The log is written only on a real run, i.e. --dry-run changes nothing.
> 
> Mentored-by: Christian Couder <[email protected]>
> Mentored-by: Siddharth Asthana <[email protected]>
> Signed-off-by: Siddharth Shrimali <[email protected]>
> ---
>   builtin/repack.c  |  4 +++
>   repack-promisor.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>   repack.h          |  4 +++
>   3 files changed, 99 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index aa3257a98a..49dcbbc567 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -702,6 +702,10 @@ int cmd_repack(int argc,
>   		write_midx_file(files->packed, NULL, NULL, flags);
>   	}
>   
> +	if (drop_filtered && !dry_run)
> +		append_drop_log(repo, &drop_oids,
> +			expand_list_objects_filter_spec(&po_args.filter_options));
> +
>   cleanup:
>   	string_list_clear(&keep_pack_list, 0);
>   	string_list_clear(&names, 1);
> diff --git a/repack-promisor.c b/repack-promisor.c
> index fabfdc168a..60913a5150 100644
> --- a/repack-promisor.c
> +++ b/repack-promisor.c
> @@ -7,6 +7,97 @@
>   #include "repository.h"
>   #include "run-command.h"
>   #include "oidset.h"
> +#include "date.h"
> +#include "promisor-remote.h"
> +#include "strbuf.h"
> +
> +/*
> + * Append the drop-log entries to the already-computed path.
> + * Returns -1 on any I/O failure so the caller can warn once.
> + * Keeping this in a separate helper avoids goto-based cleanup
> + * in append_drop_log();
> + */
> +static int write_to_drop_log(struct repository *repo,
> +			     const char *path,
> +			     const struct oidset *dropped,
> +			     const char *stamp,
> +			     const char *filter_spec,
> +			     const char *remotes)
> +{
> +	struct oidset_iter iter;
> +	const struct object_id *oid;
> +	FILE *fp;
> +
> +	if (safe_create_leading_directories(repo, (char *)path)) {
> +		warning(_("could not create leading directories for '%s'"), path);
> +		return -1;
> +	}
> +
> +	fp = fopen(path, "a");
> +	if (!fp) {
> +		warning_errno(_("could not open '%s'"), path);
> +		return -1;
> +	}
> +
> +	oidset_iter_init(dropped, &iter);
> +	while ((oid = oidset_iter_next(&iter))) {
> +		if (fprintf(fp, "%s %s filter=%s remote=%s\n",
> +				oid_to_hex(oid), stamp,
> +				filter_spec ? filter_spec : "",
> +				remotes) < 0) {
> +			warning(_("could not write to '%s'"), path);
> +			fclose(fp);
> +			return -1;
> +		}
> +	}
> +
> +	if (fclose(fp)) {
> +		warning_errno(_("could not close '%s'"), path);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +void append_drop_log(struct repository *repo,
> +		     const struct oidset *dropped,
> +		     const char *filter_spec)
> +{
> +	char *path;
> +	struct strbuf stamp = STRBUF_INIT;
> +	struct strbuf remotes = STRBUF_INIT;
> +	struct promisor_remote *pr;
> +
> +	if (!oidset_size(dropped))
> +		return;
> +
> +	datestamp(&stamp);
> +
> +	/*
> +	 * NEEDSWORK: we temporarily record all configured promisor remotes rather
> +	 * than the specific one a given object is recoverable from because there


Recording all promisor remotes for now looks OK to me with that
NEEDSWORK. I would not block this on remote-object-info.


> +	 * is currently no way to determine that locally. it would require
> +	 * asking the remote whether it has the object. A "remote-object-info"
> +	 * command is being added to the "git cat-file --batch" protocol for
> +	 * this kind of query. Once it is merged in the codebase, this should
> +	 * record the exact promisor remote that has each dropped object.
> +	 */
> +	for (pr = repo_promisor_remote_find(repo, NULL); pr; pr = pr->next) {
> +		if (remotes.len)
> +			strbuf_addch(&remotes, ',');
> +		strbuf_addstr(&remotes, pr->name);
> +	}
> +
> +	path = repo_git_path(repo, "objects/info/promisor-dropped");


If we keep it, it would be nice to document this path (for example in
gitrepository-layout) and to have a small test that a real drop appends
a line.


Thanks
Siddharth


> +
> +	if (write_to_drop_log(repo, path, dropped, stamp.buf,
> +			filter_spec, remotes.buf))
> +		warning(_("could not record all dropped objects in the drop log"));
> +
> +	strbuf_release(&stamp);
> +	strbuf_release(&remotes);
> +	free(path);
> +}
>   
>   struct write_oid_context {
>   	struct child_process *cmd;
> diff --git a/repack.h b/repack.h
> index 61e554e4ed..33309548ce 100644
> --- a/repack.h
> +++ b/repack.h
> @@ -171,6 +171,10 @@ int enumerate_promisor_blobs(struct repository *repo,
>   			     const struct list_objects_filter_options *filter,
>   			     struct oidset *to_drop);
>   
> +void append_drop_log(struct repository *repo,
> +		     const struct oidset *dropped,
> +		     const char *filter_spec);
> +
>   int write_cruft_pack(const struct write_pack_opts *opts,
>   		     const char *cruft_expiration,
>   		     unsigned long combine_cruft_below_size,