Re: [RFC] git gc "--prune=now" semantics considered harmful

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

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Soes my use pattern of "git gc --prune=now" make sense? Maybe not. But 
> it's what I've gotten used to, and it's at least not entirely insane.

FWIW, my end-of-day ritual is to do repack -a -d -f with a large
window and a small depth, followed by prune, which boils down to
about the same.  So I'd hope that is not entirely insane.  I however
do not think I bother with an explicit --expire=now when running the
prune, though.

In any case, that makes two of us, and the suggested patch protects
only one of the two ;-)

> But at least once now, I've done that "git gc" at the end of the day, and 
> a new pull request comes in, so I do the "git pull" without even thinking 
> about the fact that "git gc" is still running.

*That* is something I don't do.  After all, I am fully aware that I
have started end-of-day ritual by that time, so I won't even look at
a new patch (or a pull request for that matter).

> So I actually would much prefer that foir git gc, "--prune=now" means
>
>  (a) "now"
>
>  (b) now at the _start_ of the "git gc" operation, not the time at
>      the _end_ of the operation when we've already spent a minute or
>      two doing repacking and are now doing the final pruning.
>
> anyway, with that explanation in mind, I'm appending a patch that is 
> pretty small and does that. It's a bit hacky, but I think it still makes 
> sense.
>
> Comments?

Closing the possiblity of racing a running "gc" and new object
creation like the above generally makes sense, I would think,
whether the creation is due to 'pull/fetch', 'add', or even 'push'.

I however have to wonder if there are opposite "oops" end-user
operation we also need to worry about, i.e. we are doing a large-ish
fetch, and get bored and run a gc fron another terminal.  Perhaps
*that* is a bit too stupid to worry about?  Auto-gc deliberately
does not use 'now' because it wants to leave a grace period to avoid
exactly that kind of race.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index c4777b244..98368c8b5 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -535,8 +535,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  	if (argc > 0)
>  		usage_with_options(builtin_gc_usage, builtin_gc_options);
>  
> -	if (prune_expire && parse_expiry_date(prune_expire, &dummy))
> -		die(_("failed to parse prune expiry value %s"), prune_expire);
> +	if (prune_expire) {
> +		if (!strcmp(prune_expire, "now"))
> +			prune_expire = show_date(time(NULL), 0, DATE_MODE(ISO8601));
> +		if (parse_expiry_date(prune_expire, &dummy))
> +			die(_("failed to parse prune expiry value %s"), prune_expire);
> +	}
>  
>  	if (aggressive) {
>  		argv_array_push(&repack, "-f");