Re: [GSoC PATCH v5 6/6] builtin/repack: add guards for --drop-filtered

[Samuel Bronson <naesten@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> @@ -332,6 +361,29 @@ int cmd_repack(int argc,
>  		if (ret)
>  			goto cleanup;
>  
> +		/*
> +		 * Refuse to drop blobs that the current index references.
> +		 * Such a blob would only be lazily re-fetched by the next
> +		 * command that touches the worktree, so dropping it reclaims
> +		 * nothing. This guard just avoids that churn. Bare
> +		 * repositories have no index, so the check is skipped there.
> +		 */
> +		if (!is_bare_repository(repo) && oidset_size(&drop_oids)) {
> +			struct index_state *istate = repo->index;
> +			unsigned int i;
> +
> +			if (repo_read_index(repo) < 0)
> +				die(_("could not read the index"));
> +
> +			for (i = 0; i < istate->cache_nr; i++) {
> +				const struct cache_entry *ce = istate->cache[i];
> +
> +				if (oidset_contains(&drop_oids, &ce->oid))
> +					die(_("cannot drop '%s' (%s): it is referenced by the current index"),
> +						ce->name, oid_to_hex(&ce->oid));

The good news: I've tried this whole feature on a real repository and it
*seems* to work quite well on the whole.

I was able to greatly reduce the size of my llvm-project clone's object
databasev not originally cloned with --filter, using it.

The bad news: dying at this time is *not* convenient, especially after
we've finished that *entire* enumerate_promisor_blobs(), (which is kind
of slow for a step with no progress output, btw).

While I do want to keep the index blobs, I do *not* want to cancel the
whole operation over them.

The following seems much more convenient:

-- >8 --
Subject: [RFC] builtin/repack: just don't --drop-filtered index blobs

Instead of dying when we would drop a blob referenced by the index, just
... don't drop it. (Retain the explanatory message as a warning.)

This allows `git repack -a --filter=blob:limit=0 --drop-filtered` to
work in non-bare repositories that have non-trivial files around.

Not done:

  - Fixing the tests to match

  - Allowing `--filter=blob:none`

Signed-off-by: Samuel Bronson <naesten@gmail.com>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: convenience.patch --]
[-- Type: text/x-patch, Size: 580 bytes --]

diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..ab4471f740 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -378,8 +378,8 @@ int cmd_repack(int argc,
 			for (i = 0; i < istate->cache_nr; i++) {
 				const struct cache_entry *ce = istate->cache[i];
 
-				if (oidset_contains(&drop_oids, &ce->oid))
-					die(_("cannot drop '%s' (%s): it is referenced by the current index"),
+				if (oidset_remove(&drop_oids, &ce->oid))
+					warning(_("cannot drop '%s' (%s): it is referenced by the current index"),
 						ce->name, oid_to_hex(&ce->oid));
 			}
 		}