git svn clone/fetch hits issues with gc --auto

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

Quoted 8 lines · Martin Langhoff

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

GC can be annoying when that happens... For git-svn, perhaps this can be appropriate to at least allow the import to continue:

perl/Git/SVN.pm +1 -1

index 76b2965905..9b0caa3d47 100644
@@ -999,7 +999,7 @@ sub restore_commit_header_env {
 }
 
 sub gc {
-	command_noisy('gc', '--auto');
+	eval { command_noisy('gc', '--auto') };
 };
 
 sub do_git_commit {

But yeah, somebody else who works on git regularly could probably stop repack from writing thousands of loose objects (and instead write a self-contained pack with those objects, instead). I haven't followed git closely lately, myself.

Forwarding to Jonathan, as I think this is an interesting supporting vote for the topic that we were stuck on.

Quoted 33 lines · Eric Wong

Quoted 8 lines · Martin Langhoff

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

GC can be annoying when that happens... For git-svn, perhaps this can be appropriate to at least allow the import to continue:

perl/Git/SVN.pm +1 -1

index 76b2965905..9b0caa3d47 100644
@@ -999,7 +999,7 @@ sub restore_commit_header_env {
 }
 
 sub gc {
-     command_noisy('gc', '--auto');
+     eval { command_noisy('gc', '--auto') };
 };
 
 sub do_git_commit {

But yeah, somebody else who works on git regularly could probably stop repack from writing thousands of loose objects (and instead write a self-contained pack with those objects, instead). I haven't followed git closely lately, myself.

Looking around, Jonathan Tan's "[PATCH] gc: do not warn about too many loose objects" makes sense to me.

- remove unactionable warning
- as the warning is gone, no gc.log is produced
- subsequent gc runs don't exit due to gc.log

My very humble +1 on that.

As for downsides... if we have truly tons of _recent_ loose objects, it'll ... take disk space? I'm fine with that.

For more aggressive gc options, thoughts:

- Do we always consider git gc --prune=now "safe" in a "won't delete
stuff the user is likely to want" sense? For example -- are the references from reflogs enough safety?

- Even if we don't, for some commands it should be safe to run git gc
--prune=now at the end of the process, for example an import that generates a new git repo (git svn clone).

m

Quoted 39 lines · Junio C Hamano

Forwarding to Jonathan, as I think this is an interesting supporting vote for the topic that we were stuck on.

Quoted 33 lines · Eric Wong

Quoted 8 lines · Martin Langhoff

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

GC can be annoying when that happens... For git-svn, perhaps this can be appropriate to at least allow the import to continue:

perl/Git/SVN.pm +1 -1

index 76b2965905..9b0caa3d47 100644
@@ -999,7 +999,7 @@ sub restore_commit_header_env {
 }
 
 sub gc {
-     command_noisy('gc', '--auto');
+     eval { command_noisy('gc', '--auto') };
 };
 
 sub do_git_commit {

But yeah, somebody else who works on git regularly could probably stop repack from writing thousands of loose objects (and instead write a self-contained pack with those objects, instead). I haven't followed git closely lately, myself.

As Jeff's
https://public-inbox.org/git/[email protected]/
and my https://public-inbox.org/git/[email protected]/
note it's a bit more complex than that.

- The warning is actionable, you can decide to up your expiration policy.

- We use this warning as a proxy for "let's not run for a day"

- This conflation of the user-visible warning and the policy is an emergent effect of how the different gc pieces interact, which as I note in the linked thread(s) sucks.

So it's creating a lot of garbage during its cloning process that can just be immediately thrown away? What is it doing? Using the object store as a scratch pad for its own temporary state?

Quoted 4 lines · Ævar Arnfjörð Bjarmason

As Jeff's
https://public-inbox.org/git/[email protected]/
and my https://public-inbox.org/git/[email protected]/
note it's a bit more complex than that.

Ok, my bad for not reading the whole thread :-) thanks for the kind explanation.

Quoted 2 lines

- The warning is actionable, you can decide to up your expiration policy.

A newbie-ish user shouldn't need to know git's internal store model _and the nuances of its special cases_ got get through.

Quoted 2 lines

- We use this warning as a proxy for "let's not run for a day"

Oh, so _that's_ the trick with creating gc.log? I then understand the idea of changing to exit 0.

But it's far from clear, and a clear _flag_, and not spitting again the same warning, or differently-worded warning would be better.

"We won't try running gc, a recent run was deemed pointless until some time passes. Nothing to worry about."

Quoted 3 lines

- This conflation of the user-visible warning and the policy is an emergent effect of how the different gc pieces interact, which as I note in the linked thread(s) sucks.

It sure does, and that aspect should be easy to fix...(?)

Quoted 3 lines

So it's creating a lot of garbage during its cloning process that can just be immediately thrown away? What is it doing? Using the object store as a scratch pad for its own temporary state?

Yeah, thats suspicious and I don't know why. I've worked on other importers and while those needed 'gc' to generate packs, they didn't generate garbage objects. After gc, the repo was "clean".

I'm just saying it's hard in this case to remove one piece without the whole Jenga tower collapsing, and it's probably a good idea in some of these cases to pester the user about what he wants, but probably not via gc --auto emitting the same warning every time, e.g. in one of these threads I suggested maybe "git status" should emit this.

Quoted 5 lines · Ævar Arnfjörð Bjarmason

I'm just saying it's hard in this case to remove one piece without the whole Jenga tower collapsing, and it's probably a good idea in some of these cases to pester the user about what he wants, but probably not via gc --auto emitting the same warning every time, e.g. in one of these threads I suggested maybe "git status" should emit this.

I have to say, I don't have a lot of sympathy for this.

I've been running with the patches I sent before for a while now, and the behavior that they create is great. I think we can make further refinements on top. To put it another way, I haven't actually experienced any bad knock-on effects, and I think other feature requests can be addressed separately.

Quoted 9 lines

Quoted 5 lines · Ævar Arnfjörð Bjarmason

I'm just saying it's hard in this case to remove one piece without the whole Jenga tower collapsing, and it's probably a good idea in some of these cases to pester the user about what he wants, but probably not via gc --auto emitting the same warning every time, e.g. in one of these threads I suggested maybe "git status" should emit this.

Quoted 7 lines

I have to say, I don't have a lot of sympathy for this.

I've been running with the patches I sent before for a while now, and the behavior that they create is great. I think we can make further refinements on top. To put it another way, I haven't actually experienced any bad knock-on effects, and I think other feature requests can be addressed separately.

I think there may be some miscommunication here. The Jenga tower above is referring (I think) to Jonathan Tan's original patch to drop the warning entirely, which does have some unwanted side effects.

Your patches are much less controversial, I think, and are in next and marked as "will merge to master" in the last "what's cooking".

Add an --auto-exit-code variable and a corresponding 'gc.autoExitCode' configuration option to optionally bring back the 'git gc --auto' exit code behavior as it existed between 2.6.3..2.19.0 (inclusive).

This was changed in 3029970275 ("gc: do not return error for prior errors in daemonized mode", 2018-07-16). The motivation for that patch was to appease 3rd party tools whose treatment of the 'git gc --auto' exit code is different from that of git core where it has always been ignored.

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Add an --auto-exit-code variable and a corresponding 'gc.autoExitCode' configuration option to optionally bring back the 'git gc --auto' exit code behavior as it existed between 2.6.3..2.19.0 (inclusive).

This was changed in 3029970275 ("gc: do not return error for prior errors in daemonized mode", 2018-07-16). The motivation for that patch was to appease 3rd party tools whose treatment of the 'git gc --auto' exit code is different from that of git core where it has always been ignored.

OK. I wouldn't want to use this myself, but I think you've made clear why you find it useful. So I don't mind making it an optional behavior (and it probably beats you trying to poke at the logfile yourself).

I'm not sure if the config is going to actually help that much, though. The callers within Git will generally ignore the exit code anyway. So for those cases, setting it will at best do nothing, and at worst it may confuse the few stragglers (e.g., the git-svn one under recent discussion).

Callers who _are_ prepared to act on the exit code probably ought to just use --auto-exit-code in their invocation.

That said, I'm not entirely opposed to the matching config. There's enough history here that somebody might want a sledgehammer setting to go back to the old behavior.

Quoted 6 lines

Callers who _are_ prepared to act on the exit code probably ought to just use --auto-exit-code in their invocation.

That said, I'm not entirely opposed to the matching config. There's enough history here that somebody might want a sledgehammer setting to go back to the old behavior.

If it's not a config option then as git is upgraded I'll need to change my across-server invocation to be some variant of checking git version, then etiher using the --auto-exit-code option or not (which'll error on older gits). Easier to be able to just drop in a config setting before the upgrade.

Add an --auto-exit-code variable and a corresponding 'gc.autoExitCode' configuration option to optionally bring back the 'git gc --auto' exit code behavior as it existed between 2.6.3..2.19.0 (inclusive).

Hi,

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Add an --auto-exit-code variable and a corresponding 'gc.autoExitCode' configuration option to optionally bring back the 'git gc --auto' exit code behavior as it existed between 2.6.3..2.19.0 (inclusive).

Hm. Can you tell me more about the use case where this would be helpful to you? That would help us come up with a better name for it.

Right. I know. What I mean is now I can (and do) use it to run 'git gc --auto' across our server fleet and see whether I have any of #3, or whether it's all #1 or #2. If there's nothing to do in #1 that's fine, and it just so happens that I'll run gc due to #2 that's also fine, but I'd like to see if gc really is stuck.

This of course relies on them having other users / scripts doing normal git commands which would trigger previous 'git gc --auto' runs.

I.e. with your change that command:

    git gc --auto

Would change to something like:

    git gc --auto && ! test -e .git/gc.log

Which, as noted is a bit of a nasty breaker of the encapsulation

Perhaps this reporting could also print the message from a previous run, so you could write:

	git gc --detached-status || exit
	git gc --auto; # perhaps also passing --detach

(Names still open for bikeshedding.)

Quoted 6 lines · Jonathan Nieder

Perhaps this reporting could also print the message from a previous run, so you could write:

	git gc --detached-status || exit
	git gc --auto; # perhaps also passing --detach

Quoted 1 line

(Names still open for bikeshedding.)

When the command is given --detached-exit-code/status option, what does it do? Does it perform the "did an earlier run left gc.log?" and report the result and nothing else? In other words, is it a pure replacement for "test -e .git/gc.log"?

Or does it do some of
the "auto-gc" prep logic like guestimating loose object count and have that also in its exit status (e.g. "from the gc.log left behind, we know that we failed to reduce loose object count down sufficiently after finding there are more than 6700 earlier, but now we do not have that many loose object, so there is nothing to complain about the presence of gc.log")?

I am bad at naming myself, but worse at guessing what others meant with a new thing that was given a new name whose name is fuzzy, so... ;-)

Quoted 8 lines · Junio C Hamano

Quoted 6 lines · Jonathan Nieder

Perhaps this reporting could also print the message from a previous run, so you could write:

	git gc --detached-status || exit
	git gc --auto; # perhaps also passing --detach

Quoted 6 lines

Quoted 1 line

(Names still open for bikeshedding.)

When the command is given --detached-exit-code/status option, what does it do? Does it perform the "did an earlier run left gc.log?" and report the result and nothing else? In other words, is it a pure replacement for "test -e .git/gc.log"?

My intent was the latter. In other words, in the idiom

do_something_async &
... a lot of time passes ...
wait

it is something like the replacement for "wait".

More precisely,

	git gc --detached-status || exit

would mean something like

if test -e .git/gc.log # Error from previous gc --detach?
then
cat >&2 .git/gc.log # Report the error.
exit 1
fi

Quoted 7 lines

Or does it do some of
the "auto-gc" prep logic like guestimating loose object count and have that also in its exit status (e.g. "from the gc.log left behind, we know that we failed to reduce loose object count down sufficiently after finding there are more than 6700 earlier, but now we do not have that many loose object, so there is nothing to complain about the presence of gc.log")?

Depending on the use case, a user might want to avoid losing information about the results of a previous "git gc --detach" run, even if they no longer apply. For example, a user might want to collect the error message for monitoring or later log analysis, to track down intermittent gc errors that go away on their own.

A separate possible use case might be a

	git gc --needs-auto-gc

command that detects whether an auto gc is needed. With that, a caller that only wants to learn about errors if auto gc is needed could run

	if git gc --needs-auto-gc
	then
		git gc --detached-status || exit
	fi

Quoted 3 lines

I am bad at naming myself, but worse at guessing what others meant with a new thing that was given a new name whose name is fuzzy, so... ;-)

No problem. I'm mostly trying to tease out more details about the use case.

Which is what I'm doing by running "gc --auto" across a set of servers and looking at the exit code. If it's been failing I get an error, if there's no need to gc nothing happens, and if it hasn't been failing and it just so happens that it's time to GC then fine, now was as good a time as any.

So if we assume that for the sake of argument there's no point in a --detached-status either. My only reason for ever caring about that status is when I run "gc --auto" and it says it can't fork() itself so it fails. Since I'm using "gc --auto" I have zero reason to even ask that question unless I'm OK with kicking off a gc run as a side-effect, so why split up the two? It just introduces a race condition for no benefit.

Hi,

Quoted 5 lines · Ævar Arnfjörð Bjarmason

I'm just saying it's hard in this case to remove one piece without the whole Jenga tower collapsing, and it's probably a good idea in some of these cases to pester the user about what he wants, but probably not via gc --auto emitting the same warning every time, e.g. in one of these threads I suggested maybe "git status" should emit this.

I have to say, I don't have a lot of sympathy for this.

I've been running with the patches I sent before for a while now, and the behavior that they create is great. I think we can make further refinements on top. To put it another way, I haven't actually experienced any bad knock-on effects, and I think other feature requests can be addressed separately.

I do have sympathy for some wishes for changes to "git gc --auto" behavior (I think it should be synchronous regardless of config and the asynchrony should move to being requested explicitly through a command line option by the callers within Git) but I don't understand why this holds up a change that IMHO is wholly positive for users.

To put it another way, I am getting the feeling that the objections to that series were theoretical, while the practical benefits of the patch are pretty immediate and real. I'm happy to help anyone who wants to polish it but time has shown no one is working on that, so...

Quoted 11 lines · Martin Langhoff

Looking around, Jonathan Tan's "[PATCH] gc: do not warn about too many loose objects" makes sense to me.

- remove unactionable warning
- as the warning is gone, no gc.log is produced
- subsequent gc runs don't exit due to gc.log

My very humble +1 on that.

As for downsides... if we have truly tons of _recent_ loose objects, it'll ... take disk space? I'm fine with that.

As Jeff's
https://public-inbox.org/git/[email protected]/
and my https://public-inbox.org/git/[email protected]/
note it's a bit more complex than that.

I.e.:

- The warning is actionable, you can decide to up your expiration policy.

- We use this warning as a proxy for "let's not run for a day", otherwise we'll just grind on gc --auto trying to consolidate possibly many hundreds of K of loose objects only to find none of them can be pruned because the run into the expiry policy. With the warning we retry that once per day, which sucks less.

- This conflation of the user-visible warning and the policy is an emergent effect of how the different gc pieces interact, which as I note in the linked thread(s) sucks.

But we can't just yank one piece away (as Jonathan's patch does) without throwing the baby out with the bathwater.

It will mean that e.g. if you have 10k loose objects in your git.git, and created them just now, that every time you run anything that runs "gc --auto" we'll fork to the background, peg a core at 100% CPU for 2-3 minutes or whatever it is, only do get nowhere and do the same thing again in ~3 minutes when you run your next command.

- I think you may be underestimating some of the cases where this ends up taking a huge amount of disk space (and now we'll issue at least *some*) warning. See my
https://public-inbox.org/git/[email protected]/
where a repo's .git went from 2.5G to 30G due to being stuck in this mode.

Quoted 5 lines

For more aggressive gc options, thoughts:

- Do we always consider git gc --prune=now "safe" in a "won't delete
stuff the user is likely to want" sense? For example -- are the references from reflogs enough safety?

The --prune=now command is not generally safe for the reasons noted in the "NOTES" section in "git help gc".

Quoted 3 lines

- Even if we don't, for some commands it should be safe to run git gc
--prune=now at the end of the process, for example an import that generates a new git repo (git svn clone).

Yeah I don't see a problem with that, I didn't know about this interesting use-case, i.e. that "git svn clone" will create a lot of loose objects.

As seen in my
https://public-inbox.org/git/[email protected]/ I'm working on making "gc --auto" run at the end of clone for unrelated reasons, i.e. so we generate the commit-graph, seems like "git svn clone" could do something similar.

So it's creating a lot of garbage during its cloning process that can just be immediately thrown away? What is it doing? Using the object store as a scratch pad for its own temporary state?

Quoted 41 lines

m

Quoted 39 lines · Junio C Hamano

Forwarding to Jonathan, as I think this is an interesting supporting vote for the topic that we were stuck on.

Quoted 33 lines · Eric Wong

Quoted 8 lines · Martin Langhoff

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

GC can be annoying when that happens... For git-svn, perhaps this can be appropriate to at least allow the import to continue:

perl/Git/SVN.pm +1 -1

index 76b2965905..9b0caa3d47 100644
@@ -999,7 +999,7 @@ sub restore_commit_header_env {
 }
 
 sub gc {
-     command_noisy('gc', '--auto');
+     eval { command_noisy('gc', '--auto') };
 };
 
 sub do_git_commit {

But yeah, somebody else who works on git regularly could probably stop repack from writing thousands of loose objects (and instead write a self-contained pack with those objects, instead). I haven't followed git closely lately, myself.

- We use this warning as a proxy for "let's not run for a day", otherwise we'll just grind on gc --auto trying to consolidate possibly many hundreds of K of loose objects only to find none of them can be pruned because the run into the expiry policy. With the warning we retry that once per day, which sucks less.

- This conflation of the user-visible warning and the policy is an emergent effect of how the different gc pieces interact, which as I note in the linked thread(s) sucks.

But we can't just yank one piece away (as Jonathan's patch does) without throwing the baby out with the bathwater.

It will mean that e.g. if you have 10k loose objects in your git.git, and created them just now, that every time you run anything that runs "gc --auto" we'll fork to the background, peg a core at 100% CPU for 2-3 minutes or whatever it is, only do get nowhere and do the same thing again in ~3 minutes when you run your next command.

Quoted 10 lines · Ævar Arnfjörð Bjarmason

- We use this warning as a proxy for "let's not run for a day", otherwise we'll just grind on gc --auto trying to consolidate possibly many hundreds of K of loose objects only to find none of them can be pruned because the run into the expiry policy. With the warning we retry that once per day, which sucks less.

- This conflation of the user-visible warning and the policy is an emergent effect of how the different gc pieces interact, which as I note in the linked thread(s) sucks.

Quoted 8 lines

But we can't just yank one piece away (as Jonathan's patch does) without throwing the baby out with the bathwater.

It will mean that e.g. if you have 10k loose objects in your git.git, and created them just now, that every time you run anything that runs "gc --auto" we'll fork to the background, peg a core at 100% CPU for 2-3 minutes or whatever it is, only do get nowhere and do the same thing again in ~3 minutes when you run your next command.

We probably can keep the "let's not run for a day" safety while pretending that "git gc -auto" succeeded for callers like "git svn" so that these callers do not hae to do "eval { ... }" to hide our exit code, no?

I think that is what Jonathan's patch (jn/gc-auto) does.

From: Jonathan Nieder <[email protected]>
Date: Mon, 16 Jul 2018 23:57:40 -0700
Subject: [PATCH] gc: do not return error for prior errors in daemonized mode

builtin/gc.c +18 -1

index 95c8afd07b..ce8a663a01 100644
@@ -438,9 +438,15 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	return NULL;
 }
 
-static void report_last_gc_error(void)
+/*
+ * Returns 0 if there was no previous error and gc can proceed, 1 if
+ * gc should not proceed due to an error in the last run. Prints a
+ * message and returns -1 if an error occured while reading gc.log
+ */
+static int report_last_gc_error(void)
 {
 	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
...
 	if (len < 0)
+		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+	else if (len > 0) {
+		/*
+		 * A previous gc failed.  Report the error, and don't
+		 * bother with an automatic gc run since it is likely
+		 * to fail in the same way.
+		 */
+		warning(_("The last gc run reported the following. "
 			       "Please correct the root cause\n"
 			       "and remove %s.\n"
 			       "Automatic cleanup will not be performed "
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
+		ret = 1;
+	}
 	strbuf_release(&sb);
 done:
 	free(gc_log_path);
+	return ret;
 }

I.e. report_last_gc_error() returns 1 when finds that the previous attempt to "gc --auto" failed. And then

builtin/gc.c +7 -1

@@ -561,7 +576,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			report_last_gc_error(); /* dies on error */
+			int ret = report_last_gc_error();
+			if (ret < 0)
+				/* an I/O error occured, already reported */
+				exit(128);
+			if (ret == 1)
+				/* Last gc --auto failed. Skip this one. */
+				return 0;
 

... it exits with 0 without bothering to rerun "gc".

So it won't get stuck for 3 minutes; the repository after "gc --auto" punts will stay to be suboptimal for a day, and the user kill not get an "actionable" error notice (due to this hiding of previous error), hence cannot make changes that may help like shortening expiry period, though.

We probably can keep the "let's not run for a day" safety while pretending that "git gc -auto" succeeded for callers like "git svn" so that these callers do not hae to do "eval { ... }" to hide our exit code, no?

I think that is what Jonathan's patch (jn/gc-auto) does.

Hi folks,

Long time no see! Importing a 3GB (~25K revs, tons of files) SVN repo I hit the gc error:

warning: There are too many unreachable loose objects; run 'git prune' to remove them.
gc --auto: command returned error: 255

I don't seem to be the only one --
https://stackoverflow.com/questions/35738680/avoiding-warning-there-are-too-many-unreachable-loose-objects-during-git-svn

Looking at code history, it dropped the ability to pass options to git repack when it was converted it to using git gc.

Experimentally I find that tweaking it to run git gc --auto --prune=5.minutes.ago works well, while --prune=now breaks it. Attempts to commit fail with missing objects.

- Why does --prune=now break it? Perhaps "gc" runs in the background, and races with the commit being prepared?

- Would it be safe, sane to apply --prune=some.value on _clone_?

- During _fetch_, --prune=some.value seems risky. In a checkout being actively used for development or merging it'd risk pruning objects users expect to be there for recovery. Would there be a safe, sane way?

- Is there a safer, saner value than 5 minutes?