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

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2018, 23:31 UTC
Message-ID: <xmqqd0xim1tp.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <alpine.LFD.2.21.999.1805261449160.6211@i7.lan>

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).

Show 13 quoted lines

> 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.

Show 19 quoted lines

> 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");