threads / patch / 12641

patchgc: call "prune --expire 2.weeks.ago"

Subject: [PATCH] gc: call "prune --expire 2.weeks.ago"

## tl;dr

41 messages between Mar 11, 2008 and Mar 13, 2008. Diffs are folded; open one to read it.

replies: 40people: 11as markdown or json

Johannes Schindelin· Mar 11, 2008, 20:58 UTC · lore

If "--prune" is passed to gc, it still just calls "git prune". Otherwise, "prune --expire 2.weeks.ago" is called, where the grace period is overrideable by the config variable gc.pruneExpire.

While adding a test to t5304-prune.sh (since it really tests the implicit call to "prune"), the original test for "prune --expire" is moved there from t1410-reflog.sh, where it did not belong.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	I am really tempted to reduce the grace period further, but
	I'd like to hear opinions first.  Is 3.days.ago too short?
 Documentation/config.txt |    5 +++++
 Documentation/git-gc.txt |   16 +++++++++++-----
 builtin-gc.c             |   19 +++++++++++++++++--
 t/t1410-reflog.sh        |   18 ------------------
 t/t5304-prune.sh         |   36 ++++++++++++++++++++++++++++++++++++
 5 files changed, 69 insertions(+), 25 deletions(-)
Show changes to 5 files +69 −25

Documentation/config.txt, Documentation/git-gc.txt, builtin-gc.c, t/t1410-reflog.sh, t/t5304-prune.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 14df635..adde89a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -590,6 +590,11 @@ gc.packrefs::
 	at some stage, and setting this to `false` will continue to
 	prevent `git pack-refs` from being run from `git gc`.
 
+gc.pruneexpire::
+	When `git gc` is run without `--prune`, it will still call
+	`prune`, but with `--expire 2.weeks.ago`.  Override the value
+	with this config variable.
+
 gc.reflogexpire::
 	`git reflog expire` removes reflog entries older than
 	this time; defaults to 90 days.
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2e7be91..2042d9f 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -28,13 +28,19 @@ OPTIONS
 --prune::
 	Usually `git-gc` packs refs, expires old reflog entries,
 	packs loose objects,
-	and removes old 'rerere' records.  Removal
+	and removes old 'rerere' records.  Unilateral removal
 	of unreferenced loose objects is an unsafe operation
 	while other git operations are in progress, so it is not
-	done by default.  Pass this option if you want it, and only
-	when you know nobody else is creating new objects in the
-	repository at the same time (e.g. never use this option
-	in a cron script).
+	done by default.
++
+Instead, `git-prune` is called with an option telling it to expire
+only unreferenced loose objects that are at least 2 weeks old.  Set
+the config variable `gc.pruneexpire` to override this grace period.
++
+Pass `--prune` to expire all unreferenced loose objects, but only
+when you know nobody else is creating new objects in the
+repository at the same time (e.g. never use this option
+in a cron script).
 
 --aggressive::
 	Usually 'git-gc' runs very quickly while providing good disk
diff --git a/builtin-gc.c b/builtin-gc.c
index 7cad366..8d07350 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -26,12 +26,13 @@ static int pack_refs = 1;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 20;
+static char *prune_expire = "2.weeks.ago";
 
 #define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
 static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", NULL};
+static const char *argv_prune[] = {"prune", NULL, NULL, NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
 static int gc_config(const char *var, const char *value)
@@ -55,6 +56,14 @@ static int gc_config(const char *var, const char *value)
 		gc_auto_pack_limit = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.pruneexpire")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (!approxidate(value))
+			return error("Invalid gc.pruneExpire: '%s'", value);
+		prune_expire = xstrdup(value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -235,7 +244,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_repack[0]);
 
-	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+	if (!prune) {
+		argv_prune[1] = "--expire";
+		argv_prune[2] = prune_expire;
+		argv_prune[3] = NULL;
+	}
+
+	if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_prune[0]);
 
 	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 24476be..73f830d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -202,22 +202,4 @@ test_expect_success 'delete' '
 
 '
 
-test_expect_success 'prune --expire' '
-
-	before=$(git count-objects | sed "s/ .*//") &&
-	BLOB=$(echo aleph | 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 &&
-	git reset --hard &&
-	git prune --expire=1.hour.ago &&
-	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
-	test -f $BLOB_FILE &&
-	test-chmtime -86500 $BLOB_FILE &&
-	git prune --expire 1.day &&
-	test $before = $(git count-objects | sed "s/ .*//") &&
-	! test -f $BLOB_FILE
-
-'
-
 test_done
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6560af7..2a88b3f 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -29,4 +29,40 @@ test_expect_success 'prune stale packs' '
 
 '
 
+test_expect_success 'prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph | 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 &&
+	git prune --expire=1.hour.ago &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -86500 $BLOB_FILE &&
+	git prune --expire 1.day &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: implicit prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph_0 | git hash-object -w --stdin) &&
+echo blob: $BLOB &&
+	BLOB_FILE=.git/objects/$(echo $BLOB | sed "s/^../&\//") &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14-30)) $BLOB_FILE &&
+	git gc &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14+1)) $BLOB_FILE &&
+	git gc &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
 test_done
-- 
1.5.4.4.646.ge37ad
Junio C Hamano· Mar 12, 2008, 02:13 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> If "--prune" is passed to gc, it still just calls "git prune".
> Otherwise, "prune --expire 2.weeks.ago" is called, where the grace
> period is overrideable by the config variable gc.pruneExpire.
"What it does."
> While adding a test to t5304-prune.sh (since it really tests the
> implicit call to "prune"), the original test for "prune --expire"
> is moved there from t1410-reflog.sh, where it did not belong.
"What the fallouts from this change were."
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Can we also have "why this is a good idea", "what problem this solves"?
Nicolas Pitre· Mar 12, 2008, 02:37 UTC · re: Junio C Hamano · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Tue, 11 Mar 2008, Junio C Hamano wrote:
Show 17 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > If "--prune" is passed to gc, it still just calls "git prune".
> > Otherwise, "prune --expire 2.weeks.ago" is called, where the grace
> > period is overrideable by the config variable gc.pruneExpire.
> 
> "What it does."
> 
> > While adding a test to t5304-prune.sh (since it really tests the
> > implicit call to "prune"), the original test for "prune --expire"
> > is moved there from t1410-reflog.sh, where it did not belong.
> 
> "What the fallouts from this change were."
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Can we also have "why this is a good idea", "what problem this solves"?

FWIW, my agreeing with the "why this is a good idea" can be translated into:

Acked-by: Nicolas Pitre <nico@cam.org>
Nicolas
Junio C Hamano· Mar 12, 2008, 06:49 UTC · re: Nicolas Pitre · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Nicolas Pitre <nico@cam.org> writes:
Show 6 quoted lines
>> Can we also have "why this is a good idea", "what problem this solves"?
>
> FWIW, my agreeing with the "why this is a good idea" can be translated 
> into:
>
> Acked-by: Nicolas Pitre <nico@cam.org>
Hmmm.  Is it _that_ obvious?

At least it would be easier to readers if we had something like this in the documentation (and/or the commit message):

    "git gc" used to never prune unreachable objects without being
    explicitly told to, with its --prune option.  This left cruft to
    accumulate; the user eventually has to run "git prune" manually.
    It is safe to prune old objects that are unreachable from refs nor
    reflogs.  "git gc" is updated to run "git prune --expire 2.weeks.ago"
    so that users has to run "git prune" by hand much less often.

Is it too much to ask for regulars to set the example of justifying why each of the change is a good idea?

Johannes Schindelin· Mar 12, 2008, 10:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Hi,
On Tue, 11 Mar 2008, Junio C Hamano wrote:
Show 25 quoted lines
> Nicolas Pitre <nico@cam.org> writes:
> 
> >> Can we also have "why this is a good idea", "what problem this 
> >> solves"?
> >
> > FWIW, my agreeing with the "why this is a good idea" can be translated 
> > into:
> >
> > Acked-by: Nicolas Pitre <nico@cam.org>
> 
> Hmmm.  Is it _that_ obvious?
> 
> At least it would be easier to readers if we had something like this in 
> the documentation (and/or the commit message):
> 
>     "git gc" used to never prune unreachable objects without being
>     explicitly told to, with its --prune option.  This left cruft to
>     accumulate; the user eventually has to run "git prune" manually.
> 
>     It is safe to prune old objects that are unreachable from refs nor
>     reflogs.  "git gc" is updated to run "git prune --expire 2.weeks.ago"
>     so that users has to run "git prune" by hand much less often.
> 
> Is it too much to ask for regulars to set the example of justifying why 
> each of the change is a good idea?
I would have written something like
	Earlier, git-gc would not prune loose objects without being called 
	with --prune.  However, users were actively warned that it is not 
	a safe operation, so most users never called it.
	This makes the operation reasonably safe (unless you have critical 
	git operations running for over two weeks), by pruning only those 
	objects that are old (in the sense of git operations, which 
	typically take no more than a few seconds).

However, I think that it should have been obvious to those who know the internals of git-gc, and it is completely uninteresting to those that are just users. All they will realise (or not) is that "git gc --auto" now less often complains about too many loose objects (hopefully).

The real question I asked was: is 2 weeks a sensible default? As I said, I was almost tempted to reduce it to 3 days.

Hmm?

Ciao, Dscho

Nicolas Pitre· Mar 12, 2008, 15:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Wed, 12 Mar 2008, Johannes Schindelin wrote:
> The real question I asked was: is 2 weeks a sensible default?  As I said, 
> I was almost tempted to reduce it to 3 days.
3 days would make me nervous, even if it should be plenty safe.
2 weeks is OTOH maybe a bit too conservative.
What about one week instead?
Nicolas
Pieter de Bie· Mar 12, 2008, 15:53 UTC · re: Nicolas Pitre · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Mar 12, 2008, at 4:45 PM, Nicolas Pitre wrote:
>
> 2 weeks is OTOH maybe a bit too conservative.
>
> What about one week instead?
I'd really like it to be at least 2 weeks
- Pieter
Johannes Schindelin· Mar 12, 2008, 16:05 UTC · re: Pieter de Bie · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Hi,
On Wed, 12 Mar 2008, Pieter de Bie wrote:
Show 7 quoted lines
> On Mar 12, 2008, at 4:45 PM, Nicolas Pitre wrote:
> 
> > 2 weeks is OTOH maybe a bit too conservative.
> >
> > What about one week instead?
> 
> I'd really like it to be at least 2 weeks
Could you back that up with an explanation, as to why?

Thanks, Dscho

Jeff King· Mar 12, 2008, 17:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Wed, Mar 12, 2008 at 05:05:51PM +0100, Johannes Schindelin wrote:
> > I'd really like it to be at least 2 weeks
> 
> Could you back that up with an explanation, as to why?

I assume it's "because I wouldn't want to lose work I had done within the last two weeks." Yes, I know that this expiration is actually after the reflog has already expired, but there is a loophole there: branches that have been deleted have their reflogs deleted (some have argued that this doesn't matter, since the HEAD reflog will still mention the commits. In most cases, this is true, though there are still a few exceptions).

I think being conservative here is a good idea. The big reason for this is to fix the spurious "gc --auto" runs caused by needing to prune. So there is no downside to increasing the time limit unless you think people will generate enough objects in that limit to cause the problem again. In which case they will continue to complain to the list, and we can drop the time.

-Peff
Pieter de Bie· Mar 12, 2008, 22:50 UTC · re: Jeff King · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Mar 12, 2008, at 6:01 PM, Jeff King wrote:
Show 10 quoted lines
> On Wed, Mar 12, 2008 at 05:05:51PM +0100, Johannes Schindelin wrote:
>
>>> I'd really like it to be at least 2 weeks
>>
>> Could you back that up with an explanation, as to why?
>
> I assume it's "because I wouldn't want to lose work I had done within
> the last two weeks." Yes, I know that this expiration is actually  
> after
> the reflog has already expired

Ah, I hadn't realised that. Then I don't really care, one week sounds fine too. 2 weeks just seemed a bit short, as the default reflog is 30 days. But if it's 2 weeks after the 30 days, that should be more than enough

- Pieter
Junio C Hamano· Mar 12, 2008, 23:20 UTC · re: Pieter de Bie · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Pieter de Bie <pdebie@ai.rug.nl> writes:
Show 17 quoted lines
> On Mar 12, 2008, at 6:01 PM, Jeff King wrote:
>
>> On Wed, Mar 12, 2008 at 05:05:51PM +0100, Johannes Schindelin wrote:
>>
>>>> I'd really like it to be at least 2 weeks
>>>
>>> Could you back that up with an explanation, as to why?
>>
>> I assume it's "because I wouldn't want to lose work I had done within
>> the last two weeks." Yes, I know that this expiration is actually
>> after
>> the reflog has already expired
>
> Ah, I hadn't realised that. Then I don't really care, one week sounds
> fine too. 2 weeks just seemed a bit short, as the default reflog is 30
> days. But if it's 2 weeks after the 30 days, that should be more than
> enough
Wasn't the default 90 days?

In any case, after 90 days, my reading of the code is that these loose ones become unreachable, and when we look at their timestamps, we notice that they are already more than 2 weeks old, and they will immediately be removed.

So it won't be "2 weeks after X", either.
Johannes Schindelin· Mar 12, 2008, 23:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Hi,
On Wed, 12 Mar 2008, Junio C Hamano wrote:
> In any case, after 90 days, my reading of the code is that these loose 
> ones become unreachable, and when we look at their timestamps, we notice 
> that they are already more than 2 weeks old, and they will immediately 
> be removed.
Yes.

In any case, most likely these objects would have been packed anyway during that period, so they would have been pruned by the repack called by git-gc.

My patch really, really is about _unreferenced_ loose objects, i.e. something you get by multiple "git add"s without commits. And by merge conflicts.

IOW by objects that were never referenced by commit objects.

Ciao, Dscho

Junio C Hamano· Mar 12, 2008, 23:41 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
> Yes.
>
> In any case, most likely these objects would have been packed anyway 
> during that period, so they would have been pruned by the repack called by 
> git-gc.
Yes, that is the most probable way for these unused cruft to get removed.
Show 5 quoted lines
> My patch really, really is about _unreferenced_ loose objects, i.e. 
> something you get by multiple "git add"s without commits.  And by merge 
> conflicts.
>
> IOW by objects that were never referenced by commit objects.
Geert Bosch· Mar 12, 2008, 16:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Mar 12, 2008, at 06:57, Johannes Schindelin wrote:
Show 5 quoted lines
> The real question I asked was: is 2 weeks a sensible default?  As I  
> said,
> I was almost tempted to reduce it to 3 days.
>
> Hmm?

Two weeks is a sensible default. Many people don't use their SCM every day (really!). Say you'd work on something friday, mess up and go home, it would be quite bad on monday morning to find that gc kicks in and removes some objects you're trying to recover.

The only point is to reduce build-up over long periods, so two weeks seems a perfectly fine cut-off.

Nicolas Pitre· Mar 12, 2008, 15:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On Tue, 11 Mar 2008, Junio C Hamano wrote:
Show 10 quoted lines
> Nicolas Pitre <nico@cam.org> writes:
> 
> >> Can we also have "why this is a good idea", "what problem this solves"?
> >
> > FWIW, my agreeing with the "why this is a good idea" can be translated 
> > into:
> >
> > Acked-by: Nicolas Pitre <nico@cam.org>
> 
> Hmmm.  Is it _that_ obvious?
To the average user, maybe not.  But My ack is orthogonal to that issue.
Nicolas
Marko Kreen· Mar 12, 2008, 15:32 UTC · re: Nicolas Pitre · lore

Re: [PATCH] gc: call "prune --expire 2.weeks.ago"

On 3/12/08, Nicolas Pitre <nico@cam.org> wrote:
Show 12 quoted lines
> On Tue, 11 Mar 2008, Junio C Hamano wrote:
> > Nicolas Pitre <nico@cam.org> writes:
>  > >> Can we also have "why this is a good idea", "what problem this solves"?
>  > >
>  > > FWIW, my agreeing with the "why this is a good idea" can be translated
>  > > into:
>  > >
>  > > Acked-by: Nicolas Pitre <nico@cam.org>
>  >
>  > Hmmm.  Is it _that_ obvious?
>
> To the average user, maybe not.  But My ack is orthogonal to that issue.

Well, I'm a newbie user and now I'm trained to always do "gc --prune", because "gc" itself does not make tree "really" clean.

But this will quite likely bit me in the long run.

So from my newbie perspective, anything that decreases number of mandatory arguments to commands is good. *cough* commit -a *cough*

But the difference from 'commit -a' is that its not a style issue - "gc" without --prune will keep stuff around indefinitely, which makes occasional --prune usage mandatory. As its annoying to memorize when it was last ran, its easier to use it always.

So from my newbie perpective, +1 for making plain "gc" work.
-- 
marko
Johannes Schindelin· Mar 12, 2008, 17:35 UTC · re: Nicolas Pitre · lore

[PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

The only reason we did not call "prune" in git-gc was that it is an inherently dangerous operation: if there is a commit going on, you will prune loose objects that were just created, and are, in fact, needed by the commit object just about to be created.

Since it is dangerous, we told users so. That led to many users not even daring to run it when it was actually safe. Besides, they are users, and should not have to remember such details as when to call git-gc with --prune, or to call git-prune directly.

Of course, the consequence was that "git gc --auto" gets triggered much more often than we would like, since unreferenced loose objects (such as left-overs from a rebase or a reset --hard) were never pruned.

Alas, git-prune recently learnt the option --expire <minimum-age>, which makes it a much safer operation. This allows us to call prune from git-gc, with a grace period of 2 weeks for the unreferenced loose objects (this value was determined in a discussion on the git list as a safe one).

If you want to override this grace period, just set the config variable gc.pruneExpire to a different value; an example would be

	[gc]
		pruneExpire = 6.months.ago
if you feel really paranoid.

Note that this new behaviour does not affect git-gc when you pass the option --prune; in that case, prune will clean up the loose objects with no grace period at all.

While adding a test to t5304-prune.sh (since it really tests the implicit call to "prune"), also the original test for "prune --expire" was moved there from t1410-reflog.sh, where it did not belong.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Nicolas Pitre <nico@cam.org>
---
	Since my original suggestion of 2 weeks was more or less agreed 
	upon, I only reworked the commit message, and added the Ack of 
	Nico.
	Junio, is this message good enough?
 Documentation/config.txt |    5 +++++
 Documentation/git-gc.txt |   16 +++++++++++-----
 builtin-gc.c             |   19 +++++++++++++++++--
 t/t1410-reflog.sh        |   18 ------------------
 t/t5304-prune.sh         |   36 ++++++++++++++++++++++++++++++++++++
 5 files changed, 69 insertions(+), 25 deletions(-)
Show changes to 5 files +69 −25

Documentation/config.txt, Documentation/git-gc.txt, builtin-gc.c, t/t1410-reflog.sh, t/t5304-prune.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f64b269..db5b2dc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -590,6 +590,11 @@ gc.packrefs::
 	at some stage, and setting this to `false` will continue to
 	prevent `git pack-refs` from being run from `git gc`.
 
+gc.pruneexpire::
+	When `git gc` is run without `--prune`, it will still call
+	`prune`, but with `--expire 2.weeks.ago`.  Override the value
+	with this config variable.
+
 gc.reflogexpire::
 	`git reflog expire` removes reflog entries older than
 	this time; defaults to 90 days.
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2e7be91..2042d9f 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -28,13 +28,19 @@ OPTIONS
 --prune::
 	Usually `git-gc` packs refs, expires old reflog entries,
 	packs loose objects,
-	and removes old 'rerere' records.  Removal
+	and removes old 'rerere' records.  Unilateral removal
 	of unreferenced loose objects is an unsafe operation
 	while other git operations are in progress, so it is not
-	done by default.  Pass this option if you want it, and only
-	when you know nobody else is creating new objects in the
-	repository at the same time (e.g. never use this option
-	in a cron script).
+	done by default.
++
+Instead, `git-prune` is called with an option telling it to expire
+only unreferenced loose objects that are at least 2 weeks old.  Set
+the config variable `gc.pruneexpire` to override this grace period.
++
+Pass `--prune` to expire all unreferenced loose objects, but only
+when you know nobody else is creating new objects in the
+repository at the same time (e.g. never use this option
+in a cron script).
 
 --aggressive::
 	Usually 'git-gc' runs very quickly while providing good disk
diff --git a/builtin-gc.c b/builtin-gc.c
index 7cad366..8d07350 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -26,12 +26,13 @@ static int pack_refs = 1;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 20;
+static char *prune_expire = "2.weeks.ago";
 
 #define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
 static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", NULL};
+static const char *argv_prune[] = {"prune", NULL, NULL, NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
 static int gc_config(const char *var, const char *value)
@@ -55,6 +56,14 @@ static int gc_config(const char *var, const char *value)
 		gc_auto_pack_limit = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.pruneexpire")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (!approxidate(value))
+			return error("Invalid gc.pruneExpire: '%s'", value);
+		prune_expire = xstrdup(value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -235,7 +244,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_repack[0]);
 
-	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+	if (!prune) {
+		argv_prune[1] = "--expire";
+		argv_prune[2] = prune_expire;
+		argv_prune[3] = NULL;
+	}
+
+	if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_prune[0]);
 
 	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 24476be..73f830d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -202,22 +202,4 @@ test_expect_success 'delete' '
 
 '
 
-test_expect_success 'prune --expire' '
-
-	before=$(git count-objects | sed "s/ .*//") &&
-	BLOB=$(echo aleph | 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 &&
-	git reset --hard &&
-	git prune --expire=1.hour.ago &&
-	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
-	test -f $BLOB_FILE &&
-	test-chmtime -86500 $BLOB_FILE &&
-	git prune --expire 1.day &&
-	test $before = $(git count-objects | sed "s/ .*//") &&
-	! test -f $BLOB_FILE
-
-'
-
 test_done
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6560af7..2a88b3f 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -29,4 +29,40 @@ test_expect_success 'prune stale packs' '
 
 '
 
+test_expect_success 'prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph | 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 &&
+	git prune --expire=1.hour.ago &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -86500 $BLOB_FILE &&
+	git prune --expire 1.day &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: implicit prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph_0 | git hash-object -w --stdin) &&
+echo blob: $BLOB &&
+	BLOB_FILE=.git/objects/$(echo $BLOB | sed "s/^../&\//") &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14-30)) $BLOB_FILE &&
+	git gc &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14+1)) $BLOB_FILE &&
+	git gc &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
 test_done
-- 
1.5.4.4.694.g43223
Brandon Casey· Mar 12, 2008, 17:56 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin wrote:
Show 11 quoted lines
> If you want to override this grace period, just set the config variable
> gc.pruneExpire to a different value; an example would be
> 
> 	[gc]
> 		pruneExpire = 6.months.ago
> 
> if you feel really paranoid.
> 
> Note that this new behaviour does not affect git-gc when you pass the
> option --prune; in that case, prune will clean up the loose objects with no
> grace period at all.

Hmm. Perhaps 'git-gc' should always call 'prune' with the '--expire' argument for simplicity of the 'git-gc' interface and --prune should become a noop?

Is 'git-gc --prune' still useful to end users when those in-the-know can use git-prune when they really want all loose unreferenced objects to be removed?

Also, what about clones created with --shared or --reference? Should there be a way to disable this functionality? gc.pruneExpire never

-brandon
Jakub Narebski· Mar 12, 2008, 18:35 UTC · re: Brandon Casey · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Brandon Casey <casey@nrlssc.navy.mil> writes:
> Is 'git-gc --prune' still useful to end users when those in-the-know can use
> git-prune when they really want all loose unreferenced objects to be removed?
It is one command less to remember (just like with "git tag --verify"
and "git verify-tag"), so I'm all for "git gc --prune" to remain.
 
> Also, what about clones created with --shared or --reference? Should there be
> a way to disable this functionality? gc.pruneExpire never
That would be nice.
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Johannes Schindelin· Mar 12, 2008, 19:07 UTC · re: Jakub Narebski · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Jakub Narebski wrote:
Show 8 quoted lines
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
> > Is 'git-gc --prune' still useful to end users when those in-the-know 
> > can use git-prune when they really want all loose unreferenced objects 
> > to be removed?
> 
> It is one command less to remember (just like with "git tag --verify" 
> and "git verify-tag"), so I'm all for "git gc --prune" to remain.
I don't care one way or the other.
> > Also, what about clones created with --shared or --reference? Should 
> > there be a way to disable this functionality? gc.pruneExpire never
> 
> That would be nice.

Okay, so I just remove the !approxidate() check. Then, "gc.pruneExpire = never" should work as you expect it to.

Ciao, Dscho

Junio C Hamano· Mar 12, 2008, 19:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Okay, so I just remove the !approxidate() check.  Then, "gc.pruneExpire = 
> never" should work as you expect it to.
Huh?  date.c::special[] has "never" defined for this exact reason.
Johannes Schindelin· Mar 12, 2008, 19:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Junio C Hamano wrote:
Show 6 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Okay, so I just remove the !approxidate() check.  Then, "gc.pruneExpire = 
> > never" should work as you expect it to.
> 
> Huh?  date.c::special[] has "never" defined for this exact reason.

Oops. I thought that approxidate() returns 0 on error, but apparently this is not so. Instead, it returns "now"!

So first of all, my patch is incorrect, and second: invalid dates cannot be caught reliably.

Darn.

Ciao, Dscho "who'll think about a way around that"

Johannes Schindelin· Mar 12, 2008, 19:53 UTC · re: Junio C Hamano · lore

[PATCH v3] gc: call "prune --expire 2.weeks.ago" by default

The only reason we did not call "prune" in git-gc was that it is an inherently dangerous operation: if there is a commit going on, you will prune loose objects that were just created, and are, in fact, needed by the commit object just about to be created.

Since it is dangerous, we told users so. That led to many users not even daring to run it when it was actually safe. Besides, they are users, and should not have to remember such details as when to call git-gc with --prune, or to call git-prune directly.

Of course, the consequence was that "git gc --auto" gets triggered much more often than we would like, since unreferenced loose objects (such as left-overs from a rebase or a reset --hard) were never pruned.

Alas, git-prune recently learnt the option --expire <minimum-age>, which makes it a much safer operation. This allows us to call prune from git-gc, with a grace period of 2 weeks for the unreferenced loose objects (this value was determined in a discussion on the git list as a safe one).

If you want to override this grace period, just set the config variable gc.pruneExpire to a different value; an example would be

	[gc]
		pruneExpire = 6.months.ago
if you feel really paranoid.

Note that this new behaviour does not affect git-gc when you pass the option --prune; in that case, prune will clean up the loose objects with no grace period at all.

While adding a test to t5304-prune.sh (since it really tests the implicit call to "prune"), also the original test for "prune --expire" was moved there from t1410-reflog.sh, where it did not belong.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	This checks for an invalid gc.pruneExpire by assuming that an 
	invalid date string is not "now", but parses to the same value
	(or actually newer, since between the two calls to approxidate(), 
	there is a slight chance of a wrapover to the next second).
	So yes, gc.pruneExpire = never should work now.
 Documentation/config.txt |    5 +++++
 Documentation/git-gc.txt |   16 +++++++++++-----
 builtin-gc.c             |   20 ++++++++++++++++++--
 t/t1410-reflog.sh        |   18 ------------------
 t/t5304-prune.sh         |   44 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 25 deletions(-)
Show changes to 5 files +78 −25

Documentation/config.txt, Documentation/git-gc.txt, builtin-gc.c, t/t1410-reflog.sh, t/t5304-prune.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f64b269..db5b2dc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -590,6 +590,11 @@ gc.packrefs::
 	at some stage, and setting this to `false` will continue to
 	prevent `git pack-refs` from being run from `git gc`.
 
+gc.pruneexpire::
+	When `git gc` is run without `--prune`, it will still call
+	`prune`, but with `--expire 2.weeks.ago`.  Override the value
+	with this config variable.
+
 gc.reflogexpire::
 	`git reflog expire` removes reflog entries older than
 	this time; defaults to 90 days.
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2e7be91..2042d9f 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -28,13 +28,19 @@ OPTIONS
 --prune::
 	Usually `git-gc` packs refs, expires old reflog entries,
 	packs loose objects,
-	and removes old 'rerere' records.  Removal
+	and removes old 'rerere' records.  Unilateral removal
 	of unreferenced loose objects is an unsafe operation
 	while other git operations are in progress, so it is not
-	done by default.  Pass this option if you want it, and only
-	when you know nobody else is creating new objects in the
-	repository at the same time (e.g. never use this option
-	in a cron script).
+	done by default.
++
+Instead, `git-prune` is called with an option telling it to expire
+only unreferenced loose objects that are at least 2 weeks old.  Set
+the config variable `gc.pruneexpire` to override this grace period.
++
+Pass `--prune` to expire all unreferenced loose objects, but only
+when you know nobody else is creating new objects in the
+repository at the same time (e.g. never use this option
+in a cron script).
 
 --aggressive::
 	Usually 'git-gc' runs very quickly while providing good disk
diff --git a/builtin-gc.c b/builtin-gc.c
index 7cad366..9663fae 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -26,12 +26,13 @@ static int pack_refs = 1;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 20;
+static char *prune_expire = "2.weeks.ago";
 
 #define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
 static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", NULL};
+static const char *argv_prune[] = {"prune", NULL, NULL, NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
 static int gc_config(const char *var, const char *value)
@@ -55,6 +56,15 @@ static int gc_config(const char *var, const char *value)
 		gc_auto_pack_limit = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.pruneexpire")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (strcmp(value, "now") &&
+				approxidate(value) - approxidate("now") >= 0)
+			return error("Invalid gc.pruneExpire: '%s'", value);
+		prune_expire = xstrdup(value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -235,7 +245,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_repack[0]);
 
-	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+	if (!prune) {
+		argv_prune[1] = "--expire";
+		argv_prune[2] = prune_expire;
+		argv_prune[3] = NULL;
+	}
+
+	if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_prune[0]);
 
 	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 24476be..73f830d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -202,22 +202,4 @@ test_expect_success 'delete' '
 
 '
 
-test_expect_success 'prune --expire' '
-
-	before=$(git count-objects | sed "s/ .*//") &&
-	BLOB=$(echo aleph | 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 &&
-	git reset --hard &&
-	git prune --expire=1.hour.ago &&
-	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
-	test -f $BLOB_FILE &&
-	test-chmtime -86500 $BLOB_FILE &&
-	git prune --expire 1.day &&
-	test $before = $(git count-objects | sed "s/ .*//") &&
-	! test -f $BLOB_FILE
-
-'
-
 test_done
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6560af7..3b6b01d 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -29,4 +29,48 @@ test_expect_success 'prune stale packs' '
 
 '
 
+test_expect_success 'prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph | 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 &&
+	git prune --expire=1.hour.ago &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -86500 $BLOB_FILE &&
+	git prune --expire 1.day &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: implicit prune --expire' '
+
+	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*14-30)) $BLOB_FILE &&
+	git gc &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14+1)) $BLOB_FILE &&
+	git gc &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' '
+
+	git config gc.pruneExpire invalid &&
+	test_must_fail git gc &&
+	git config gc.pruneExpire now &&
+	git gc
+
+'
+
 test_done
-- 
1.5.4.4.694.g43223
Johannes Schindelin· Mar 12, 2008, 19:55 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3] gc: call "prune --expire 2.weeks.ago" by default

... and here is the interdiff:
 builtin-gc.c     |    3 ++-
 t/t5304-prune.sh |   10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)
Show changes to 2 files +11 −2

builtin-gc.c, t/t5304-prune.sh

diff --git a/builtin-gc.c b/builtin-gc.c
index 8d07350..9663fae 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -59,7 +59,8 @@ static int gc_config(const char *var, const char *value)
 	if (!strcmp(var, "gc.pruneexpire")) {
 		if (!value)
 			return config_error_nonbool(var);
-		if (!approxidate(value))
+		if (strcmp(value, "now") &&
+				approxidate(value) - approxidate("now") >= 0)
 			return error("Invalid gc.pruneExpire: '%s'", value);
 		prune_expire = xstrdup(value);
 		return 0;
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 2a88b3f..3b6b01d 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -50,7 +50,6 @@ test_expect_success 'gc: implicit prune --expire' '
 
 	before=$(git count-objects | sed "s/ .*//") &&
 	BLOB=$(echo aleph_0 | git hash-object -w --stdin) &&
-echo blob: $BLOB &&
 	BLOB_FILE=.git/objects/$(echo $BLOB | sed "s/^../&\//") &&
 	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
 	test -f $BLOB_FILE &&
@@ -65,4 +64,13 @@ echo blob: $BLOB &&
 
 '
 
+test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' '
+
+	git config gc.pruneExpire invalid &&
+	test_must_fail git gc &&
+	git config gc.pruneExpire now &&
+	git gc
+
+'
+
 test_done
Brandon Casey· Mar 12, 2008, 19:55 UTC · re: Jakub Narebski · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Jakub Narebski wrote:
Show 7 quoted lines
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> Is 'git-gc --prune' still useful to end users when those in-the-know can use
>> git-prune when they really want all loose unreferenced objects to be removed?
> 
> It is one command less to remember (just like with "git tag --verify"
> and "git verify-tag"), so I'm all for "git gc --prune" to remain.

I think the issue here is that the prune behavior of git-gc is more complex when both 'git-gc' does prune and 'git-gc --prune' does prune, but only one of them is controlled by gc.pruneExpire. From a high level perspective, this is non-obvious and so has to be explicitly outlined in the help text _and_ users have to remember it. The git-gc command and the documentation and suggested work flows can all be simplified by just always doing a safe prune.

I think git-prune is seldomly used by normal users for the reasons Dscho described, and I think once the behavior implemented by his patch becomes standard it will never be used by normal users (except the ones who always use --prune for the reasons Geert Bosch described, and they'll probably want the new behavior). So I think git-prune will sink a little lower into plumbing and common users won't need to know anything about pruning, and only sophisticated users will need to know git-prune.

-brandon
Johannes Schindelin· Mar 12, 2008, 19:59 UTC · re: Brandon Casey · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Brandon Casey wrote:
Show 8 quoted lines
> I think git-prune is seldomly used by normal users for the reasons Dscho 
> described, and I think once the behavior implemented by his patch 
> becomes standard it will never be used by normal users (except the ones 
> who always use --prune for the reasons Geert Bosch described, and 
> they'll probably want the new behavior). So I think git-prune will sink 
> a little lower into plumbing and common users won't need to know 
> anything about pruning, and only sophisticated users will need to know 
> git-prune.

But because we are nice people, we will deprecate --prune before we remove it, should we go that route at all.

Ciao, Dscho

Brandon Casey· Mar 12, 2008, 20:25 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin wrote:
Show 15 quoted lines
> Hi,
> 
> On Wed, 12 Mar 2008, Brandon Casey wrote:
> 
>> I think git-prune is seldomly used by normal users for the reasons Dscho 
>> described, and I think once the behavior implemented by his patch 
>> becomes standard it will never be used by normal users (except the ones 
>> who always use --prune for the reasons Geert Bosch described, and 
>> they'll probably want the new behavior). So I think git-prune will sink 
>> a little lower into plumbing and common users won't need to know 
>> anything about pruning, and only sophisticated users will need to know 
>> git-prune.
> 
> But because we are nice people, we will deprecate --prune before we remove 
> it, should we go that route at all.

I am not suggesting that git-gc stop parsing --prune and instead start erroring out on it. If it is ok to change the behavior of git-gc, it seems like it is ok to change the behavior of 'git-gc --prune' in the same way, especially if the change makes it less destructive.

I would suggest not having the somewhat ambiguous state of 'git-gc' prunes one way and 'git-gc --prune' prunes in another more dangerous way. And only one is controlled by a config option named gc.pruneExpire (_and_ it's not the obvious usage of git-gc which actually has the word 'prune' on the command line).

So I hope making --prune a noop fits your definition of nice deprecation.
-brandon
Junio C Hamano· Mar 12, 2008, 20:35 UTC · re: Brandon Casey · lore

Re: [PATCH v2] gc: call "prune --expire 2.weeks.ago" by default

Brandon Casey <casey@nrlssc.navy.mil> writes:
> I am not suggesting that git-gc stop parsing --prune and instead start
> erroring out on it. If it is ok to change the behavior of git-gc, it
> seems like it is ok to change the behavior of 'git-gc --prune' in the
> same way, especially if the change makes it less destructive.

Yeah, gc.pruneexpire does sound like it applies to both implicit ones and explicit ones, doesn't it? I think your suggestion makes sense.

Johannes Schindelin· Mar 12, 2008, 20:55 UTC · re: Junio C Hamano · lore

[PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

The only reason we did not call "prune" in git-gc was that it is an inherently dangerous operation: if there is a commit going on, you will prune loose objects that were just created, and are, in fact, needed by the commit object just about to be created.

Since it is dangerous, we told users so. That led to many users not even daring to run it when it was actually safe. Besides, they are users, and should not have to remember such details as when to call git-gc with --prune, or to call git-prune directly.

Of course, the consequence was that "git gc --auto" gets triggered much more often than we would like, since unreferenced loose objects (such as left-overs from a rebase or a reset --hard) were never pruned.

Alas, git-prune recently learnt the option --expire <minimum-age>, which makes it a much safer operation. This allows us to call prune from git-gc, with a grace period of 2 weeks for the unreferenced loose objects (this value was determined in a discussion on the git list as a safe one).

If you want to override this grace period, just set the config variable gc.pruneExpire to a different value; an example would be

	[gc]
		pruneExpire = 6.months.ago
or even "never", if you feel really paranoid.
Note that this new behaviour makes "--prune" be a no-op.

While adding a test to t5304-prune.sh (since it really tests the implicit call to "prune"), also the original test for "prune --expire" was moved there from t1410-reflog.sh, where it did not belong.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Wed, 12 Mar 2008, Junio C Hamano wrote:
	> Brandon Casey <casey@nrlssc.navy.mil> writes:
	> 
	> > I am not suggesting that git-gc stop parsing --prune and 
	> > instead start erroring out on it. If it is ok to change the behavior 
	> > of git-gc, it seems like it is ok to change the behavior of 'git-gc 
	> > --prune' in the same way, especially if the change makes it less 
	> > destructive.
	> 
	> Yeah, gc.pruneexpire does sound like it applies to both implicit 
	> ones and explicit ones, doesn't it?  I think your suggestion makes 
	> sense.
	Okay, I give up my resistance.
	Changes since v3: --prune is not even described in the man page 
	anymore, since it now is a no-op.
	Interdiff will follow in its own mail...
 Documentation/config.txt |    4 ++++
 Documentation/git-gc.txt |   17 +++++------------
 builtin-gc.c             |   15 +++++++++++++--
 t/t1410-reflog.sh        |   18 ------------------
 t/t5304-prune.sh         |   44 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 32 deletions(-)
Show changes to 5 files +66 −31

Documentation/config.txt, Documentation/git-gc.txt, builtin-gc.c, t/t1410-reflog.sh, t/t5304-prune.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f64b269..a2f1df2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -590,6 +590,10 @@ gc.packrefs::
 	at some stage, and setting this to `false` will continue to
 	prevent `git pack-refs` from being run from `git gc`.
 
+gc.pruneexpire::
+	When `git gc` is run, it will call `prune --expire 2.weeks.ago`.
+	Override the grace period with this config variable.
+
 gc.reflogexpire::
 	`git reflog expire` removes reflog entries older than
 	this time; defaults to 90 days.
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2e7be91..229a7c9 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -8,7 +8,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 
 SYNOPSIS
 --------
-'git-gc' [--prune] [--aggressive] [--auto] [--quiet]
+'git-gc' [--aggressive] [--auto] [--quiet]
 
 DESCRIPTION
 -----------
@@ -25,17 +25,6 @@ operating performance. Some git commands may automatically run
 OPTIONS
 -------
 
---prune::
-	Usually `git-gc` packs refs, expires old reflog entries,
-	packs loose objects,
-	and removes old 'rerere' records.  Removal
-	of unreferenced loose objects is an unsafe operation
-	while other git operations are in progress, so it is not
-	done by default.  Pass this option if you want it, and only
-	when you know nobody else is creating new objects in the
-	repository at the same time (e.g. never use this option
-	in a cron script).
-
 --aggressive::
 	Usually 'git-gc' runs very quickly while providing good disk
 	space utilization and performance.  This option will cause
@@ -104,6 +93,10 @@ the value, the more time is spent optimizing the delta compression.  See
 the documentation for the --window' option in linkgit:git-repack[1] for
 more details.  This defaults to 10.
 
+The optional configuration variable 'gc.pruneExpire' controls how old
+the unreferenced loose objects have to be before they are pruned.  The
+default is "2 weeks ago".
+
 See Also
 --------
 linkgit:git-prune[1]
diff --git a/builtin-gc.c b/builtin-gc.c
index 7cad366..8bd54d8 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -26,12 +26,13 @@ static int pack_refs = 1;
 static int aggressive_window = 250;
 static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 20;
+static char *prune_expire = "2.weeks.ago";
 
 #define MAX_ADD 10
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
 static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", NULL};
+static const char *argv_prune[] = {"prune", "--expire", NULL, NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
 static int gc_config(const char *var, const char *value)
@@ -55,6 +56,15 @@ static int gc_config(const char *var, const char *value)
 		gc_auto_pack_limit = git_config_int(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "gc.pruneexpire")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (strcmp(value, "now") &&
+				approxidate(value) - approxidate("now") >= 0)
+			return error("Invalid gc.pruneExpire: '%s'", value);
+		prune_expire = xstrdup(value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
@@ -235,7 +245,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_repack[0]);
 
-	if (prune && run_command_v_opt(argv_prune, RUN_GIT_CMD))
+	argv_prune[2] = prune_expire;
+	if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_prune[0]);
 
 	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 24476be..73f830d 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -202,22 +202,4 @@ test_expect_success 'delete' '
 
 '
 
-test_expect_success 'prune --expire' '
-
-	before=$(git count-objects | sed "s/ .*//") &&
-	BLOB=$(echo aleph | 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 &&
-	git reset --hard &&
-	git prune --expire=1.hour.ago &&
-	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
-	test -f $BLOB_FILE &&
-	test-chmtime -86500 $BLOB_FILE &&
-	git prune --expire 1.day &&
-	test $before = $(git count-objects | sed "s/ .*//") &&
-	! test -f $BLOB_FILE
-
-'
-
 test_done
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6560af7..3b6b01d 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -29,4 +29,48 @@ test_expect_success 'prune stale packs' '
 
 '
 
+test_expect_success 'prune --expire' '
+
+	before=$(git count-objects | sed "s/ .*//") &&
+	BLOB=$(echo aleph | 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 &&
+	git prune --expire=1.hour.ago &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -86500 $BLOB_FILE &&
+	git prune --expire 1.day &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: implicit prune --expire' '
+
+	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*14-30)) $BLOB_FILE &&
+	git gc &&
+	test $((1 + $before)) = $(git count-objects | sed "s/ .*//") &&
+	test -f $BLOB_FILE &&
+	test-chmtime -$((86400*14+1)) $BLOB_FILE &&
+	git gc &&
+	test $before = $(git count-objects | sed "s/ .*//") &&
+	! test -f $BLOB_FILE
+
+'
+
+test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' '
+
+	git config gc.pruneExpire invalid &&
+	test_must_fail git gc &&
+	git config gc.pruneExpire now &&
+	git gc
+
+'
+
 test_done
-- 
1.5.4.4.694.g43223
Johannes Schindelin· Mar 12, 2008, 20:56 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

And here is the interdiff:
 Documentation/config.txt |    5 ++---
 Documentation/git-gc.txt |   23 +++++------------------
 builtin-gc.c             |    9 ++-------
 3 files changed, 9 insertions(+), 28 deletions(-)
Show changes to 3 files +9 −27

Documentation/config.txt, Documentation/git-gc.txt, builtin-gc.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index db5b2dc..a2f1df2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -591,9 +591,8 @@ gc.packrefs::
 	prevent `git pack-refs` from being run from `git gc`.
 
 gc.pruneexpire::
-	When `git gc` is run without `--prune`, it will still call
-	`prune`, but with `--expire 2.weeks.ago`.  Override the value
-	with this config variable.
+	When `git gc` is run, it will call `prune --expire 2.weeks.ago`.
+	Override the grace period with this config variable.
 
 gc.reflogexpire::
 	`git reflog expire` removes reflog entries older than
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 2042d9f..229a7c9 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -8,7 +8,7 @@ git-gc - Cleanup unnecessary files and optimize the local repository
 
 SYNOPSIS
 --------
-'git-gc' [--prune] [--aggressive] [--auto] [--quiet]
+'git-gc' [--aggressive] [--auto] [--quiet]
 
 DESCRIPTION
 -----------
@@ -25,23 +25,6 @@ operating performance. Some git commands may automatically run
 OPTIONS
 -------
 
---prune::
-	Usually `git-gc` packs refs, expires old reflog entries,
-	packs loose objects,
-	and removes old 'rerere' records.  Unilateral removal
-	of unreferenced loose objects is an unsafe operation
-	while other git operations are in progress, so it is not
-	done by default.
-+
-Instead, `git-prune` is called with an option telling it to expire
-only unreferenced loose objects that are at least 2 weeks old.  Set
-the config variable `gc.pruneexpire` to override this grace period.
-+
-Pass `--prune` to expire all unreferenced loose objects, but only
-when you know nobody else is creating new objects in the
-repository at the same time (e.g. never use this option
-in a cron script).
-
 --aggressive::
 	Usually 'git-gc' runs very quickly while providing good disk
 	space utilization and performance.  This option will cause
@@ -110,6 +93,10 @@ the value, the more time is spent optimizing the delta compression.  See
 the documentation for the --window' option in linkgit:git-repack[1] for
 more details.  This defaults to 10.
 
+The optional configuration variable 'gc.pruneExpire' controls how old
+the unreferenced loose objects have to be before they are pruned.  The
+default is "2 weeks ago".
+
 See Also
 --------
 linkgit:git-prune[1]
diff --git a/builtin-gc.c b/builtin-gc.c
index 9663fae..8bd54d8 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -32,7 +32,7 @@ static char *prune_expire = "2.weeks.ago";
 static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
 static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
 static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", NULL, NULL, NULL};
+static const char *argv_prune[] = {"prune", "--expire", NULL, NULL};
 static const char *argv_rerere[] = {"rerere", "gc", NULL};
 
 static int gc_config(const char *var, const char *value)
@@ -245,12 +245,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_repack[0]);
 
-	if (!prune) {
-		argv_prune[1] = "--expire";
-		argv_prune[2] = prune_expire;
-		argv_prune[3] = NULL;
-	}
-
+	argv_prune[2] = prune_expire;
 	if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
 		return error(FAILED_RUN, argv_prune[0]);
 
Junio C Hamano· Mar 12, 2008, 21:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ---prune::

I am fairly paranoid about end users wondering about what is described in ancient documentation and complaining that we do not talk about it anymore. I am tempted to suggest:

	This is a no-op but you may see it mentioned in older docs and
	scripts.  Older git-gc never ran 'prune' without being told, and
	this option was a way to tell it to.

but this would lead to littering the documentation with too much historical information in the long run. I dunno. I am inclined to favor the removal as your patch did, but somebody else may have clever ideas.

Show 6 quoted lines
> +	if (!strcmp(var, "gc.pruneexpire")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		if (strcmp(value, "now") &&
> +				approxidate(value) - approxidate("now") >= 0)
> +			return error("Invalid gc.pruneExpire: '%s'", value);

Yuck; approxidate() returns ulong. Can subtracting a ulong from another ever go negative?

Besides, because there is no guarantee of the order of evaluation between these two approxidate() calls, you may get +1 or -1 on the second boundary.

I think the reason why you did not catch it in your test is because your tests are half complete; they test only what you wanted to catch (misconfigured case) and do not test the other half (properly working case).

Nicolas Pitre· Mar 12, 2008, 22:40 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

On Wed, 12 Mar 2008, Junio C Hamano wrote:
Show 15 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ---prune::
> 
> I am fairly paranoid about end users wondering about what is described in
> ancient documentation and complaining that we do not talk about it
> anymore.  I am tempted to suggest:
> 
> 	This is a no-op but you may see it mentioned in older docs and
> 	scripts.  Older git-gc never ran 'prune' without being told, and
> 	this option was a way to tell it to.
> 
> but this would lead to littering the documentation with too much
> historical information in the long run.  I dunno.  I am inclined to favor
> the removal as your patch did, but somebody else may have clever ideas.
Historical notes of that sort belong in RelNotes.
Nicolas
Johannes Schindelin· Mar 12, 2008, 22:50 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ---prune::
> 
> I am fairly paranoid about end users wondering about what is described in
> ancient documentation and complaining that we do not talk about it
> anymore.  I am tempted to suggest:
> 
> 	This is a no-op but you may see it mentioned in older docs and
> 	scripts.  Older git-gc never ran 'prune' without being told, and
> 	this option was a way to tell it to.
> 
> but this would lead to littering the documentation with too much 
> historical information in the long run.  I dunno.  I am inclined to 
> favor the removal as your patch did, but somebody else may have clever 
> ideas.

Well, if you want to, I am quite willing to adapt the patch. (I care enough about the real issue, namely that "prune" should be called automatically.)

Show 17 quoted lines
> > +	if (!strcmp(var, "gc.pruneexpire")) {
> > +		if (!value)
> > +			return config_error_nonbool(var);
> > +		if (strcmp(value, "now") &&
> > +				approxidate(value) - approxidate("now") >= 0)
> > +			return error("Invalid gc.pruneExpire: '%s'", value);
> 
> Yuck; approxidate() returns ulong.  Can subtracting a ulong from another
> ever go negative?
> 
> Besides, because there is no guarantee of the order of evaluation between
> these two approxidate() calls, you may get +1 or -1 on the second boundary.
> 
> I think the reason why you did not catch it in your test is because your
> tests are half complete; they test only what you wanted to catch
> (misconfigured case) and do not test the other half (properly working
> case).

Yes, probably. Of course, comparing a difference to 0 is absolutely moronic.

I should have written
				approxidate(value) >= approxidate("now"))
in the first place.

So, could you tell me, please, if I should resend the patch with your --prune documentation, or without?

Ciao, Dscho

Junio C Hamano· Mar 12, 2008, 23:13 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
>> Yuck; approxidate() returns ulong.  Can subtracting a ulong from another
>> ever go negative?
>> 
>> Besides, because there is no guarantee of the order of evaluation between
>> these two approxidate() calls, you may get +1 or -1 on the second boundary.
>> 
>> I think the reason why you did not catch it in your test is because your
>> tests are half complete; they test only what you wanted to catch
>> (misconfigured case) and do not test the other half (properly working
>> case).
>
> Yes, probably.  Of course, comparing a difference to 0 is absolutely 
> moronic.
>
> I should have written
>
> 				approxidate(value) >= approxidate("now"))
>
> in the first place.
Eh, sorry, but why?
> So, could you tell me, please, if I should resend the patch with your 
> --prune documentation, or without?

I like Nico's suggestion to put that "historical notes" in RelNotes, so the documentation part is fine as is.

Johannes Schindelin· Mar 12, 2008, 23:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Junio C Hamano wrote:
Show 24 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Yuck; approxidate() returns ulong.  Can subtracting a ulong from 
> >> another ever go negative?
> >> 
> >> Besides, because there is no guarantee of the order of evaluation 
> >> between these two approxidate() calls, you may get +1 or -1 on the 
> >> second boundary.
> >> 
> >> I think the reason why you did not catch it in your test is because 
> >> your tests are half complete; they test only what you wanted to catch 
> >> (misconfigured case) and do not test the other half (properly working 
> >> case).
> >
> > Yes, probably.  Of course, comparing a difference to 0 is absolutely 
> > moronic.
> >
> > I should have written
> >
> > 				approxidate(value) >= approxidate("now"))
> >
> > in the first place.
> 
> Eh, sorry, but why?

The thing is: I want to prevent invalid dates in gc.pruneExpire from going unnoticed, _especially_ since they would default to "now". IOW if you said something like "one.weak.ago", it would actually have the same effect as "now" and offer _no_ grace period.

But like you said, comparing the difference of two unsigned longs to >= 0 might be quite stupid. Instead, I compare them _directly_.

Since I compare the value to "now" first, and only if it is not, compare the approxidate() of the value to the current time stamp, I can verify that no invalid date was specified.

Unfortunately, this check includes future dates. Fortunately, they do not make sense at all.

To make my reasoning clear, how about this comment above that if() clause?
		/*
		 * In case of an invalid date, approxidate() returns the
		 * same as approxidate("now").  Since the millisecond
		 * boundary could have been crossed between the two calls
		 * to approxidate(), we compare not only for equality,
		 * but also if the former is greater than the latter.
		 *
		 * Note: this assumes that future dates are invalid, which
		 * makes sense, really.
		 */
Hmm?

Ciao, Dscho

Junio C Hamano· Mar 12, 2008, 23:39 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 31 quoted lines
>> Eh, sorry, but why?
>
> The thing is: I want to prevent invalid dates in gc.pruneExpire from going 
> unnoticed, _especially_ since they would default to "now".  IOW if you 
> said something like "one.weak.ago", it would actually have the same effect 
> as "now" and offer _no_ grace period.
>
> But like you said, comparing the difference of two unsigned longs to >= 0 
> might be quite stupid.  Instead, I compare them _directly_.
>
> Since I compare the value to "now" first, and only if it is not, compare 
> the approxidate() of the value to the current time stamp, I can verify 
> that no invalid date was specified.
>
> Unfortunately, this check includes future dates.  Fortunately, they do not 
> make sense at all.
>
> To make my reasoning clear, how about this comment above that if() clause?
>
> 		/*
> 		 * In case of an invalid date, approxidate() returns the
> 		 * same as approxidate("now").  Since the millisecond
> 		 * boundary could have been crossed between the two calls
> 		 * to approxidate(), we compare not only for equality,
> 		 * but also if the former is greater than the latter.
> 		 *
> 		 * Note: this assumes that future dates are invalid, which
> 		 * makes sense, really.
> 		 */
>
> Hmm?
Ah,...

But C language rules haven't changed in such a way that it guarantees B to be evaluated before A when you write "A >= B", have it?

So at least I think you would need something like this if you go that route:

  		if (strcmp(value, "now")) {
                	unsigned long now = approxidate("now");
                	if (approxidate(value) >= now)
				return error("Invalid %s: '%s'", var, value);
			...
		}

Also the resolution of approxidate() is in seconds so millisecond boundary does not matter, but that issue is, eh, secondary ;-).

I have to wonder if approxidate_with_error() function that takes a pointer to receive an error condition may be a better way to solve this cleanly.

Johannes Schindelin· Mar 12, 2008, 23:43 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Wed, 12 Mar 2008, Junio C Hamano wrote:
> I have to wonder if approxidate_with_error() function that takes a 
> pointer to receive an error condition may be a better way to solve this 
> cleanly.

Right. But that will have to wait for at least tomorrow, if it waits for me.

Ciao, Dscho

Wincent Colaiuta· Mar 13, 2008, 09:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

El 13/3/2008, a las 0:39, Junio C Hamano escribió:
Show 15 quoted lines
> Ah,...
>
> But C language rules haven't changed in such a way that it  
> guarantees B to
> be evaluated before A when you write "A >= B", have it?
>
> So at least I think you would need something like this if you go that
> route:
>
>  		if (strcmp(value, "now")) {
>                	unsigned long now = approxidate("now");
>                	if (approxidate(value) >= now)
> 				return error("Invalid %s: '%s'", var, value);
> 			...
> 		}

Are you sure that that alternative provides any guarantees about evaluation order either? (I'm not a compiler expert, nor am I consulting a copy of the standard; but I don't think it does.)

In order to enforce evaluation in the required order I think it might have to be something like this:

	if (strcmp(value, "now")) {
		unsigned long now;
		if ((now = approxidate("now")) &&
		    approxidate(value) >= now)
			return error("Invalid %s: '%s'", var, value);
		...
	}

ie. the && operator guarantees left to right evaluation order, and since approxidate always returns a non-zero ulong the second expression (after the &&) will always be evaluated.

Cheers, Wincent

Johannes Sixt· Mar 13, 2008, 10:17 UTC · re: Wincent Colaiuta · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Wincent Colaiuta schrieb:
Show 12 quoted lines
> El 13/3/2008, a las 0:39, Junio C Hamano escribió:
>>          if (strcmp(value, "now")) {
>>                    unsigned long now = approxidate("now");
>>                    if (approxidate(value) >= now)
>>                 return error("Invalid %s: '%s'", var, value);
>>             ...
>>         }
> 
> 
> Are you sure that that alternative provides any guarantees about
> evaluation order either? (I'm not a compiler expert, nor am I consulting
> a copy of the standard; but I don't think it does.)

There is a sequence point at the semicolon. This means that all observable side effects of approxidate("now") are visible when approxidate(value) is evaluated (and no observable side effect of the latter is visible when the former is evaluated), which doesn't leave much choice for the compiler. So, yes, this does guarantee the intended evaluation order.

-- Hannes
Wincent Colaiuta· Mar 13, 2008, 09:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

El 12/3/2008, a las 21:55, Johannes Schindelin escribió:
Show 7 quoted lines
> Alas, git-prune recently learnt the option --expire <minimum-age>,  
> which
> makes it a much safer operation.  This allows us to call prune from  
> git-gc,
> with a grace period of 2 weeks for the unreferenced loose objects  
> (this
> value was determined in a discussion on the git list as a safe one).

Just a really minor quibble: I don't think you mean "alas" here; "alas" basically means "unfortunately" or "regrettably".

Cheers, Wincent

Johannes Schindelin· Mar 13, 2008, 11:11 UTC · re: Wincent Colaiuta · lore

Re: [PATCH v4] gc: call "prune --expire 2.weeks.ago" by default

Hi,
On Thu, 13 Mar 2008, Wincent Colaiuta wrote:
Show 10 quoted lines
> El 12/3/2008, a las 21:55, Johannes Schindelin escribió:
> 
> >Alas, git-prune recently learnt the option --expire <minimum-age>, 
> >which makes it a much safer operation.  This allows us to call prune 
> >from git-gc, with a grace period of 2 weeks for the unreferenced loose 
> >objects (this value was determined in a discussion on the git list as a 
> >safe one).
> 
> Just a really minor quibble: I don't think you mean "alas" here; "alas" 
> basically means "unfortunately" or "regrettably".

No, I really meant "alas" here, since it is more of a sigh. The sigh is because I had expected somebody else to care more than me about it, and I had done the --expire patch already.

Ciao, Dscho

← back to recent threads