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

> --drop-filtered removes local promisor blobs. That is only safe when the
> repository is not mid-operation and when the blobs are not actively in
> use, so add two guards, both skipped for bare repositories which have
> neither a worktree nor an index.
> 
> First, refuse to run while a merge, rebase, am, cherry-pick, revert, or
> bisect is in progress. During these operations the working tree and
> index are in an intermediate state, and rewriting packs and deleting
> objects underneath a half-finished operation is unsafe.
> 
> Second, refuse to drop a blob that the current index references. Such a

> blob is needed by the working tree, so dropping it would only cause the
> next command that touches the worktree to lazy-fetch it straight back,
> reclaiming nothing. The offending path is reported so the user can see
> why the drop was refused.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>   builtin/repack.c                | 47 +++++++++++++++++++++++++++++++++
>   t/t7706-repack-drop-filtered.sh | 36 +++++++++++++++++++++++++
>   2 files changed, 83 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 9a15ab1f2a..2339bcaac4 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -17,6 +17,8 @@
>   #include "list-objects-filter-options.h"
>   #include "oidset.h"
>   #include "hex.h"
> +#include "wt-status.h"
> +#include "read-cache-ll.h"
>   
>   #define ALL_INTO_ONE 1
>   #define LOOSEN_UNREACHABLE 2
> @@ -309,6 +311,28 @@ int cmd_repack(int argc,
>   		if (!repo_has_promisor_remote(repo))
>   			die(_("--drop-filtered requires a promisor remote"));
>   
> +		/*
> +		 * refuse to drop objects while another operation is in
> +		 * progress. the working tree and index are in an
> +		 * intermediate state, and rewriting packs in a half-finished
> +		 * merge/rebase/cherry-pick/revert/bisect is unsafe
> +		 * bare repositories have no such state, so the check
> +		 * is skipped there
> +		 */
> +		if (!is_bare_repository(repo)) {
> +			struct wt_status_state state = { 0 };
> +
> +			wt_status_get_state(repo, &state, 0);
> +			if (state.merge_in_progress || state.revert_in_progress ||
> +			    state.rebase_in_progress ||state.bisect_in_progress ||
> +			    state.cherry_pick_in_progress ||state.am_in_progress||
> +			    state.rebase_interactive_in_progress) {
> +				wt_status_state_free_buffers(&state);
> +				die(_("--drop-filtered cannot be used while another operation is in progress"));
> +			}
> +			wt_status_state_free_buffers(&state);
> +		}
> +
>   		write_bitmaps = 0;
>   
>   		/*
> @@ -324,6 +348,29 @@ int cmd_repack(int argc,
>   		if (ret)
>   			goto cleanup;
>   
> +		/*
> +		 * refuse to drop blobs that the current index references.
> +		 * dropping such a blob would cause the very next command
> +		 * that touches the worktree to lazy-fetch it straight back, so
> +		 * the drop would reclaim nothing. 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));
> +			}
> +		}
> +
>   		if (dry_run) {
>   			struct oidset_iter iter;
>   			const struct object_id *oid;
> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
> index b3e493e851..dabed97541 100755
> --- a/t/t7706-repack-drop-filtered.sh
> +++ b/t/t7706-repack-drop-filtered.sh
> @@ -140,4 +140,40 @@ test_expect_success '--drop-filtered removes the promisor blob locally' '
>   	grep -q "$SMALL" present
>   '
>   
> +test_expect_success '--drop-filtered refuses when a merge is in progress' '
> +	test_when_finished "git -C repo merge --abort || :" &&
> +
> +	# creat a conflicting merge so wt_status reports it
> +	git -C repo checkout -B mergebase base &&
> +	echo one >repo/conflict.txt &&
> +	git -C repo add conflict.txt &&
> +	git -C repo commit -m one &&
> +
> +	git -C repo checkout -B mergeother base &&
> +	echo two >repo/conflict.txt &&
> +	git -C repo add conflict.txt &&
> +	git -C repo commit -m two &&
> +
> +	test_must_fail git -C repo merge mergebase &&
> +
> +	test_must_fail git -C repo -c repack.writeBitmaps=false \
> +		repack --drop-filtered --filter=blob:limit=1k --dry-run -a 2>err &&
> +	test_grep "in progress" err
> +'
> +
> +
> +test_expect_success '--drop-filtered refuses to drop an index-referenced blob' '
> +	# create a large blob, add it to the index and make it a promisor object
> +	# so the index references it and enumeration picks it up
> +	test-tool genrandom idx 4096 >repo/tracked-big.bin &&
> +	git -C repo add tracked-big.bin &&
> +	OID=$(git -C repo rev-parse :tracked-big.bin) &&
> +	printf "%s\n" "$OID" | pack_as_from_promisor >/dev/null &&
> +	delete_object repo "$OID" &&
> +
> +	test_must_fail git -C repo -c repack.writeBitmaps=false \
> +		repack --drop-filtered --filter=blob:limit=1k --dry-run -a 2>err &&
> +	test_grep "referenced by the current index" err
> +'
> +
>   test_done