Re: [PATCH] gc: make --prune useful again by accepting an optional parameter

[Thomas Rast <trast@student.ethz.ch>]

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

Johannes Schindelin wrote:
> With this patch, "git gc --no-prune" will not prune any loose (and
> dangling) object, and "git gc --prune=5.minutes.ago" will prune
> all loose objects older than 5 minutes.

Prompted by discussion on IRC, I just spent some time staring at the
code, and for the code part and FWIW,

  Acked-by: Thomas Rast <trast@student.ethz.ch>

Meaning that for all I can see, it handles every combination of
config, --prune and --no-prune correctly.

I have some non-code nits however:

> +--prune=<date>::
> +	Prune loose objects older than date (default is 2 weeks ago).
> +	This option is on by default.

This isn't really true; to the user, it defaults to the value of
'gc.pruneExpire'.  That this is done by defaulting the state of
--prune to 2 weeks ago is an implementation detail.

> +		{ OPTION_STRING, 0, "prune", &prune_expire, "date",
> +			"prune unreferenced objects (deprecated)",
> +			PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },

You still call the option "deprecated" :-)

> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
[...]
> +test_expect_success 'gc --no-prune && gc --prune=<date>' '
> +
> +	before=$(git count-objects | sed "s/ .*//") &&
> +	BLOB=$(echo aleph_0 | git hash-object -w --stdin) &&
> +	BLOB_FILE=.git/objects/$(echo $BLOB | sed "s/^../&\//") &&
> +	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
> +	test -f $BLOB_FILE &&
> +	test-chmtime =-$((86400*5001)) $BLOB_FILE &&
> +	git gc --no-prune &&
> +	test 1 = $(git count-objects | sed "s/ .*//") &&
> +	test -f $BLOB_FILE &&
> +	git gc --prune=5002.days.ago &&
> +	test 1 = $(git count-objects | sed "s/ .*//") &&
> +	test -f $BLOB_FILE &&
> +	git gc --prune=5000.days.ago &&
> +	test 0 = $(git count-objects | sed "s/ .*//") &&
> +	test ! -f $BLOB_FILE
> +
> +'
> +

I'm not a huge fan of such contiguous blocks, as a failure is harder
to pin to any of the several tests inside, especially with 'test' not
showing any output when it fails.

Also, it does test the interaction between having gc.pruneExpire set,
but passing --no-prune: the tests above leave gc.pruneExpire at
"2.days.ago".  This wasn't immediately obvious however, and you should
probably set the config again just in case someone decides to insert a
test with a different value.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]