Re: [PATCH] gc: do not warn about too many loose objects

2018-07-18 Thread Jeff King
On Wed, Jul 18, 2018 at 03:11:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > > Yeah, I agree that deferring repeated gc's based on time is going to run > > into pathological corner cases. OTOH, what we've found at GitHub is that > > "gc --auto" is quite insufficient for scheduling maintenance

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-18 Thread Ævar Arnfjörð Bjarmason
On Tue, Jul 17 2018, Jeff King wrote: > On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> That doesn't make sense to me. Just because git itself is happily >> ignoring the exit code I don't think that should mean there shouldn't be >> a meaningful exit code. Why

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Jeff King
On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote: > That doesn't make sense to me. Just because git itself is happily > ignoring the exit code I don't think that should mean there shouldn't be > a meaningful exit code. Why don't you just ignore the exit code in the > repo

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Junio C Hamano
Jonathan Nieder writes: > That doesn't really solve the problem: > > 1. "gc --auto" would produce noise *every time it runs* until gc.log > is removed, for example via expiry > > 2. "gc --auto" would not do any garbage collection until gc.log is > removed, for example by expiry > > 3.

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Duy Nguyen
On Tue, Jul 17, 2018 at 3:53 AM Jonathan Nieder wrote: > Subject: gc: do not return error for prior errors in daemonized mode > > Some build machines started failing to fetch updated source using > "repo sync", with error > > error: The last gc run reported the following. Please correct the

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Ævar Arnfjörð Bjarmason
On Tue, Jul 17 2018, Jonathan Nieder wrote: > Hi Ævar, > > Ævar Arnfjörð Bjarmason wrote: >> On Tue, Jul 17 2018, Jonathan Nieder wrote: > >>> That suggests a possible improvement. If all callers should be >>> ignoring the exit code, can we make the exit code in daemonize mode >>>

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Jonathan Nieder
Hi Ævar, Ævar Arnfjörð Bjarmason wrote: > On Tue, Jul 17 2018, Jonathan Nieder wrote: >> That suggests a possible improvement. If all callers should be >> ignoring the exit code, can we make the exit code in daemonize mode >> unconditionally zero in this kind of case? > > That doesn't make

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-17 Thread Ævar Arnfjörð Bjarmason
On Tue, Jul 17 2018, Jonathan Nieder wrote: > Hi, > > Jeff King wrote: >> On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote: > >>> The calling command in the motivating example is Android's "repo" tool: >>> >>> bare_git.gc('--auto') >>> >>> from

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Hi, Jeff King wrote: > On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote: >> The calling command in the motivating example is Android's "repo" tool: >> >> bare_git.gc('--auto') >> >> from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I >> think it's

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote: > The calling command in the motivating example is Android's "repo" tool: > > bare_git.gc('--auto') > > from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I > think it's reasonable that it expects a

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
On Mon, Jul 16, 2018 at 3:55 PM, Jeff King wrote: > On Mon, Jul 16, 2018 at 03:07:34PM -0700, Elijah Newren wrote: > >> > If we were to delete those objects, wouldn't it be exactly the same as >> > running "git prune"? Or setting your gc.pruneExpire to "now" or even "5 >> > minutes"? Or are you

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > On Mon, Jul 16, 2018 at 03:03:06PM -0700, Jonathan Nieder wrote: >> Oh, good point. In non-daemon mode, we don't let "gc --auto" failure >> cause the invoking command to fail, but in daemon mode we do. That >> should be a straightforward fix; patch coming in a moment. > > OK,

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 03:07:34PM -0700, Elijah Newren wrote: > > If we were to delete those objects, wouldn't it be exactly the same as > > running "git prune"? Or setting your gc.pruneExpire to "now" or even "5 > > minutes"? Or are you concerned with taking other objects along for the > >

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 03:03:06PM -0700, Jonathan Nieder wrote: > Jeff King wrote: > > > I don't think any command should report failure of its _own_ operation > > if "gc --auto" failed. And grepping around the source code shows that we > > typically ignore it. > > Oh, good point. In

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
On Mon, Jul 16, 2018 at 2:21 PM, Jeff King wrote: > On Mon, Jul 16, 2018 at 02:09:43PM -0700, Elijah Newren wrote: >> The point of gc is to: expire reflogs, repack referenced objects, and >> delete loose objects that (1) are no longer referenced and (2) are >> "old enough". >> >> The "old

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > I don't think any command should report failure of its _own_ operation > if "gc --auto" failed. And grepping around the source code shows that we > typically ignore it. Oh, good point. In non-daemon mode, we don't let "gc --auto" failure cause the invoking command to fail,

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 02:40:03PM -0700, Jonathan Nieder wrote: > > The key thing is that the presence of the warning/log still suppress > > the actual gc for the gc.logExpiry period. > > Ah, now I think I see the source of the misunderstanding. > > When I initially read the docs, I also

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > On Mon, Jul 16, 2018 at 01:37:53PM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> With the current code, that produces a bunch of annoying warnings for >>> every commit ("I couldn't gc because the last gc reported a warning"). >>> But after Jonathan's patch, every single

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Elijah Newren wrote: > I totally agree with your general plan to put unreferenced loose > objects into a pack. However, I don't think these objects should be > part of that pack; they should just be deleted instead. This might be the wrong thread to discuss it, but did you follow the

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 02:09:43PM -0700, Elijah Newren wrote: > >> Um, except it doesn't actually do that. The testcase I provided shows > >> that it leaves around 1 objects that are totally deletable and > >> were only previously referenced by reflog entries -- entries that gc > >> removed

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 01:56:46PM -0700, Jonathan Nieder wrote: > I don't believe we are very good at transitively propagating freshness > today, by the way. Perhaps I don't understand what you mean by transitive here. But we should be traversing from any fresh object and marking any non-fresh

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
On Mon, Jul 16, 2018 at 1:38 PM, Jeff King wrote: > On Mon, Jul 16, 2018 at 01:16:45PM -0700, Elijah Newren wrote: > >> >> The basic problem here, at least for us, is that gc has enough >> >> information to know it could expunge some objects, but because of how >> >> it is structured in terms of

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 01:37:53PM -0700, Jonathan Nieder wrote: > >and I think the > > right solution is to pack the unreachable objects instead of exploding > > them. > > That seems like a huge widening in scope relative to what this >

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > On Mon, Jul 16, 2018 at 01:21:40PM -0700, Elijah Newren wrote: >> Jonathan Nieder wrote: >>> My understanding is that exploding the objects is intentional behavior, >>> to avoid a race where objects are newly referenced while they are being >>> pruned. [...] >> Ah, that's good

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 01:16:45PM -0700, Elijah Newren wrote: > >> The basic problem here, at least for us, is that gc has enough > >> information to know it could expunge some objects, but because of how > >> it is structured in terms of several substeps (reflog expiration, > >> repack, prune),

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > On Mon, Jul 16, 2018 at 12:54:31PM -0700, Jonathan Nieder wrote: >> Even restricting attention to the warning, not the exit code, I think >> you are saying the current behavior is acceptable. I do not believe >> it is. > > I didn't say that at all. The current situation sucks,

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 01:21:40PM -0700, Elijah Newren wrote: > > My understanding is that exploding the objects is intentional behavior, > > to avoid a race where objects are newly referenced while they are being > > pruned. > > > > I am not a fan of that behavior. It's still racy. But when

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 12:54:31PM -0700, Jonathan Nieder wrote: > Even restricting attention to the warning, not the exit code, I think > you are saying the current behavior is acceptable. I do not believe > it is. I didn't say that at all. The current situation sucks, and I think the right

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
Hi, On Mon, Jul 16, 2018 at 12:19 PM, Jonathan Nieder wrote: > Elijah Newren wrote: > >> The basic problem here, at least for us, is that gc has enough >> information to know it could expunge some objects, but because of how >> it is structured in terms of several substeps (reflog expiration, >>

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
On Mon, Jul 16, 2018 at 12:52 PM, Jeff King wrote: > On Mon, Jul 16, 2018 at 12:15:05PM -0700, Elijah Newren wrote: > >> The basic problem here, at least for us, is that gc has enough >> information to know it could expunge some objects, but because of how >> it is structured in terms of several

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Jeff King wrote: > On Mon, Jul 16, 2018 at 12:09:49PM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> Er, the action is to run "git prune", like the warning says. :) >> >> I don't think we want to recommend that, especially when "git gc --auto" >> does the right thing automatically. > > But

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 12:15:05PM -0700, Elijah Newren wrote: > The basic problem here, at least for us, is that gc has enough > information to know it could expunge some objects, but because of how > it is structured in terms of several substeps (reflog expiration, > repack, prune), the

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 12:09:49PM -0700, Jonathan Nieder wrote: > >>> So while I completely agree that it's not a great thing to encourage > >>> users to blindly run "git prune", I think it _is_ actionable. > >> > >> To flesh this out a little more: what user action do you suggest? Could > >>

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Hi, Elijah Newren wrote: > The basic problem here, at least for us, is that gc has enough > information to know it could expunge some objects, but because of how > it is structured in terms of several substeps (reflog expiration, > repack, prune), the information is lost between the steps and it

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Elijah Newren
On Mon, Jul 16, 2018 at 10:27 AM, Jonathan Tan wrote: > In a087cc9819 ("git-gc --auto: protect ourselves from accumulated > cruft", 2007-09-17), the user was warned if there were too many > unreachable loose objects. This made sense at the time, because gc > couldn't prune them safely. But

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Hi, Jeff King wrote: > On Mon, Jul 16, 2018 at 11:22:07AM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> So while I completely agree that it's not a great thing to encourage >>> users to blindly run "git prune", I think it _is_ actionable. >> >> To flesh this out a little more: what user

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 11:22:07AM -0700, Jonathan Nieder wrote: > > I'm not sure if this tells the whole story. You may still run into a > > case where auto-gc runs over and over during the grace period, because > > you have a bunch of objects which are not reachable and not yet ready to > > be

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jonathan Nieder
Hi, Jeff King wrote: > On Mon, Jul 16, 2018 at 10:27:17AM -0700, Jonathan Tan wrote: >> In a087cc9819 ("git-gc --auto: protect ourselves from accumulated >> cruft", 2007-09-17), the user was warned if there were too many >> unreachable loose objects. This made sense at the time, because gc >>

Re: [PATCH] gc: do not warn about too many loose objects

2018-07-16 Thread Jeff King
On Mon, Jul 16, 2018 at 10:27:17AM -0700, Jonathan Tan wrote: > In a087cc9819 ("git-gc --auto: protect ourselves from accumulated > cruft", 2007-09-17), the user was warned if there were too many > unreachable loose objects. This made sense at the time, because gc > couldn't prune them safely.