Re: [PATCH] repack: do not rebuild packs on --dry-run

[Junio C Hamano <gitster@pobox.com>]

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

> "git repack --drop-filtered --dry-run" is documented to list the
> objects that would be dropped "without rebuilding any pack or
> deleting anything", but it does both.
>
> This is a bug in cmd_repack(): after printing the candidates, the
> --dry-run block falls through into the regular repack code.
> repack_promisor_objects() writes a new promisor pack, and with -d,
> existing_packs_remove_redundant() deletes the old packs. The command
> still exits successfully, so the user is not told that the repository
> was modified.

Very interesting finding.  I am curious if this was the case from
the beginning, or we broke --dry-run unknowingly as a side effect of
some unrelated changes.  If it is not too much, can you bisect and
document where we broke it in the log message?

> The existing guard only skips the implied "delete_redundant = 1", so
> it does not stop an explicit -d, nor the new pack from being written.
>
> Fix it by returning right after the candidates are listed, and add a
> test that checks the pack directory is unchanged with and without -d.
>
> Reported-by: Coy Geek <coygeek@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
> Bug report:
> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/
>
>  builtin/repack.c                |  8 ++++++++
>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c4360382c1..c048053912 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -391,6 +391,14 @@ int cmd_repack(int argc,
>  			oidset_iter_init(&drop_oids, &iter);
>  			while ((oid = oidset_iter_next(&iter)))
>  				printf("%s\n", oid_to_hex(oid));
> +
> +			/*
> +			 * add an exit here, so that dry run does not
> +			 * go on to rebuild any pack or delete anything, even
> +			 * if the user explicitly asked for -d
> +			 */
> +			ret = 0;
> +			goto cleanup;
>  		}
>  	}
>  
> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
> index cb36115834..a1c475e4ec 100755
> --- a/t/t7706-repack-drop-filtered.sh
> +++ b/t/t7706-repack-drop-filtered.sh
> @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '
>  	git -C repo cat-file -e "$BIG"
>  '
>  
> +test_expect_success '--dry-run leaves the pack directory untouched' '
> +	BIG=$(cat big_oid) &&
> +	packdir=repo/.git/objects/pack &&
> +
> +	for opt in "" -d
> +	do
> +		ls $packdir >before &&
> +
> +		git -C repo -c repack.writeBitmaps=false \
> +			repack --drop-filtered --filter=blob:limit=1k \
> +			--dry-run -a $opt >out &&
> +
> +		ls $packdir >after &&
> +		test_cmp before after &&
> +		test_grep "$BIG" out &&
> +		git -C repo cat-file -e "$BIG" || return 1
> +	done
> +'
> +
>  test_expect_success '--drop-filtered removes the promisor blob locally' '
>  	BIG=$(cat big_oid) &&
>  	SMALL=$(cat small_oid) &&