{"thread":{"id":"48890","subject":"[PATCH] gc: do not warn about too many loose objects","startedAt":"2018-07-16T17:27:23Z","lastAt":"2018-09-18T17:30:22Z","messageCount":57,"participants":["Jonathan Tan","Jeff King","Jonathan Nieder","Elijah Newren","Ævar Arnfjörð Bjarmason","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352635","messageId":"20180716172717.237373-1-jonathantanmy@google.com","threadId":"48890","inReplyTo":null,"subject":"[PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-16T17:27:17Z","receivedAt":"2018-07-16T17:27:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a087cc9819 (\"git-gc --auto: protect ourselves from accumulated\ncruft\", 2007-09-17), the user was warned if there were too many\nunreachable loose objects. This made sense at the time, because gc\ncouldn't prune them safely. But subsequently, git prune learned the\nability to not prune recently created loose objects, making pruning able\nto be done more safely, and gc was made to automatically prune old\nunreachable loose objects in 25ee9731c1 (\"gc: call \"prune --expire\n2.weeks.ago\" by default\", 2008-03-12).\n\nThis makes the warning unactionable by the user, as any loose objects\nleft are not deleted yet because of safety, and \"git prune\" is not a\ncommand that the user is recommended to run directly anyway.\n\nTherefore, remove this warning.\n\nSigned-off-by: Jonathan Tan \n---\nThis was noticed when a daemonized gc run wrote this warning to the log\nfile, and returned 0; but a subsequent run merely read the log file, saw\nthat it is non-empty and returned -1 (which is inconsistent in that such\na run should return 0, as it did the first time).\n---\n builtin/gc.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..fc3b553651 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -641,10 +641,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (pack_garbage.nr > 0)\n \t\tclean_pack_garbage();\n \n-\tif (auto_gc && too_many_loose_objects())\n-\t\twarning(_(\"There are too many unreachable loose objects; \"\n-\t\t\t\"run 'git prune' to remove them.\"));\n-\n \tif (!daemonized)\n \t\tunlink(git_path(\"gc.log\"));\n \n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352637","messageId":"20180716175103.GB18636@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716172717.237373-1-jonathantanmy@google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T17:51:03Z","receivedAt":"2018-07-16T17:51:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 10:27:17AM -0700, Jonathan Tan wrote:\n\n> In a087cc9819 (\"git-gc --auto: protect ourselves from accumulated\n> cruft\", 2007-09-17), the user was warned if there were too many\n> unreachable loose objects. This made sense at the time, because gc\n> couldn't prune them safely. But subsequently, git prune learned the\n> ability to not prune recently created loose objects, making pruning able\n> to be done more safely, and gc was made to automatically prune old\n> unreachable loose objects in 25ee9731c1 (\"gc: call \"prune --expire\n> 2.weeks.ago\" by default\", 2008-03-12).\n> \n> This makes the warning unactionable by the user, as any loose objects\n> left are not deleted yet because of safety, and \"git prune\" is not a\n> command that the user is recommended to run directly anyway.\n\nI'm not sure if this tells the whole story. You may still run into a\ncase where auto-gc runs over and over during the grace period, because\nyou have a bunch of objects which are not reachable and not yet ready to\nbe expired. So a gc cannot make forward progress until the 2-week\nexpiration, and the way to break the cycle is to run an immediate\n\"prune\".\n\nSo while I completely agree that it's not a great thing to encourage\nusers to blindly run \"git prune\", I think it _is_ actionable.\n\nBut...\n\n> This was noticed when a daemonized gc run wrote this warning to the log\n> file, and returned 0; but a subsequent run merely read the log file, saw\n> that it is non-empty and returned -1 (which is inconsistent in that such\n> a run should return 0, as it did the first time).\n\nYes, this got much worse with daemonized gc. The warning blocks further\nruns. And then even after the 2-week period, you still cannot make\nfurther progress until the user steps in! I think we dealt with that in\na831c06a2b (gc: ignore old gc.log files, 2017-02-10). So now we won't\nrun gc for a day, but eventually we may make further progress.\n\nSo the warning _is_ still doing something useful there (it's blocking\nimmediate auto-gc and kicking in the 1-day threshold).\n\nI agree that the \"-1\" return is a little funny. Perhaps on the reading\nside we should detect that the log contains only a \"warning\" line and\nadjust our exit code accordingly.\n\nUltimately, I think Git should avoid keeping unreachable objects as\nloose in the first place, which would make this whole problem go away.\nThere's some discussion in this thread:\n\n https://public-inbox.org/git/87inc89j38.fsf@evledraar.gmail.com/\n\n-Peff\n"},{"id":"352642","messageId":"20180716182207.GA11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716175103.GB18636@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T18:22:07Z","receivedAt":"2018-07-16T18:22:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, Jul 16, 2018 at 10:27:17AM -0700, Jonathan Tan wrote:\n\n>> In a087cc9819 (\"git-gc --auto: protect ourselves from accumulated\n>> cruft\", 2007-09-17), the user was warned if there were too many\n>> unreachable loose objects. This made sense at the time, because gc\n>> couldn't prune them safely. But subsequently, git prune learned the\n>> ability to not prune recently created loose objects, making pruning able\n>> to be done more safely, and gc was made to automatically prune old\n>> unreachable loose objects in 25ee9731c1 (\"gc: call \"prune --expire\n>> 2.weeks.ago\" by default\", 2008-03-12).\n>>\n>> This makes the warning unactionable by the user, as any loose objects\n>> left are not deleted yet because of safety, and \"git prune\" is not a\n>> command that the user is recommended to run directly anyway.\n>\n> I'm not sure if this tells the whole story. You may still run into a\n> case where auto-gc runs over and over during the grace period, because\n> you have a bunch of objects which are not reachable and not yet ready to\n> be expired. So a gc cannot make forward progress until the 2-week\n> expiration, and the way to break the cycle is to run an immediate\n> \"prune\".\n>\n> So while I completely agree that it's not a great thing to encourage\n> users to blindly run \"git prune\", I think it _is_ actionable.\n\nTo flesh this out a little more: what user action do you suggest? Could\nwe carry out that action automatically?\n\nI mentally make a distinction between this warning from \"git gc\n--auto\" and the warning from commands like \"git gui\". The former was\nsaying \"Please run 'git prune', because I'm not going to do it\", and\nas Jonathan noticed, that's not true any more. The latter says\n\n\tThis repository currently has approximately %i loose objects.\n\n\tTo maintain optimal performance it is strongly recommended\n\tthat you compress the database.\n\n\tCompress the database now?\n\nwhich is relevant to the current operation (slowly reading the\nrepository) and easy to act on (choose \"yes\").\n\n[...]\n> I agree that the \"-1\" return is a little funny. Perhaps on the reading\n> side we should detect that the log contains only a \"warning\" line and\n> adjust our exit code accordingly.\n\nYes, this is a real problem, and it would affect any other warning\n(or even GIT_TRACE=1 output) produced by gc --auto as well. I think we\nshould consider it a serious bug, separate from the symptom Jonathan is\nfixing.\n\nUnfortunately I don't have a great idea about how to fix it. Should\nwe avoid writing warnings to gc.log in daemon mode? That would\nprevent the user from ever seeing the warnings, but because of the\nnature of a warning, that might be reasonable. Should we put warnings\nin a separate log file with different semantics? Should we change the\nformat of gc.log to allow more structured information (include 'gc'\nexit code) and perform a two-stage migration to the new format (first\nlearn to read the new format, then switch to writing it)?\n\nThanks,\nJonathan\n"},{"id":"352651","messageId":"20180716185255.GC22298@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716182207.GA11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T18:52:56Z","receivedAt":"2018-07-16T18:52:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 11:22:07AM -0700, Jonathan Nieder wrote:\n\n> > I'm not sure if this tells the whole story. You may still run into a\n> > case where auto-gc runs over and over during the grace period, because\n> > you have a bunch of objects which are not reachable and not yet ready to\n> > be expired. So a gc cannot make forward progress until the 2-week\n> > expiration, and the way to break the cycle is to run an immediate\n> > \"prune\".\n> >\n> > So while I completely agree that it's not a great thing to encourage\n> > users to blindly run \"git prune\", I think it _is_ actionable.\n> \n> To flesh this out a little more: what user action do you suggest? Could\n> we carry out that action automatically?\n\nEr, the action is to run \"git prune\", like the warning says. :)\n\nAnd no, I do not think we should run it automatically. It deletes\nobjects without respect to the grace period, which may not be what the\nuser wants. (And yes, the existing warning is terrible because it does\nnot at all make clear that following its advice may be dangerous).\n\n> I mentally make a distinction between this warning from \"git gc\n> --auto\" and the warning from commands like \"git gui\". The former was\n> saying \"Please run 'git prune', because I'm not going to do it\", and\n> as Jonathan noticed, that's not true any more. The latter says\n> \n> \tThis repository currently has approximately %i loose objects.\n> \n> \tTo maintain optimal performance it is strongly recommended\n> \tthat you compress the database.\n> \n> \tCompress the database now?\n> \n> which is relevant to the current operation (slowly reading the\n> repository) and easy to act on (choose \"yes\").\n\nI don't think those warnings are the same. The warning from \"git gui\"\nis: you may benefit from running gc.\n\nThe warning that is deleted by this patch is: you _just_ ran gc, and hey\nwe even did it automatically for you, but we're still in a funky state\nafterwards. You might need to clean up this state.\n\n(As an aside, I think the git-gui warning pre-dates \"gc --auto\", and\nprobably should just run that rather than implementing its own\nheuristic).\n\n> > I agree that the \"-1\" return is a little funny. Perhaps on the reading\n> > side we should detect that the log contains only a \"warning\" line and\n> > adjust our exit code accordingly.\n> \n> Yes, this is a real problem, and it would affect any other warning\n> (or even GIT_TRACE=1 output) produced by gc --auto as well. I think we\n> should consider it a serious bug, separate from the symptom Jonathan is\n> fixing.\n> \n> Unfortunately I don't have a great idea about how to fix it. Should\n> we avoid writing warnings to gc.log in daemon mode? That would\n> prevent the user from ever seeing the warnings, but because of the\n> nature of a warning, that might be reasonable.\n\nIf you do that without anything further, then it will break the\nprotection against repeatedly running auto-gc, as I described in the\nprevious email.\n\n> Should we put warnings\n> in a separate log file with different semantics? Should we change the\n> format of gc.log to allow more structured information (include 'gc'\n> exit code) and perform a two-stage migration to the new format (first\n> learn to read the new format, then switch to writing it)?\n\nAny of those would work similarly to the \"just detect warnings\" I\nsuggested earlier, with respect to keeping the \"1 day\" expiration\nintact, so I'd be OK with them. In theory they'd be more robust than\nscraping the \"warning:\" prefix. But in practice, I think you have to\nresort to scraping anyway, if you are interested in treating warnings\nfrom sub-processes the same way.\n\n-Peff\n"},{"id":"352655","messageId":"20180716190949.GB11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716185255.GC22298@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T19:09:49Z","receivedAt":"2018-07-16T19:09:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, Jul 16, 2018 at 11:22:07AM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> So while I completely agree that it's not a great thing to encourage\n>>> users to blindly run \"git prune\", I think it _is_ actionable.\n>>\n>> To flesh this out a little more: what user action do you suggest? Could\n>> we carry out that action automatically?\n>\n> Er, the action is to run \"git prune\", like the warning says. :)\n\nI don't think we want to recommend that, especially when \"git gc --auto\"\ndoes the right thing automatically.\n\nCan you convince me I'm wrong?\n\n[...]\n>> I mentally make a distinction between this warning from \"git gc\n>> --auto\" and the warning from commands like \"git gui\".\n[...]\n> I don't think those warnings are the same. The warning from \"git gui\"\n> is: you may benefit from running gc.\n>\n> The warning that is deleted by this patch is: you _just_ ran gc, and hey\n> we even did it automatically for you, but we're still in a funky state\n> afterwards. You might need to clean up this state.\n\nThis sounds awful. It sounds to me like you're saying \"git gc --auto\"\nis saying \"I just did the wrong thing, and here is how you can clean\nup after me\". That's not how I want a program to behave.\n\nBut there's a simpler explanation for what \"git gc --auto\" was trying\nto say, which Jonathan described.\n\n[...]\n>> Yes, this is a real problem, and it would affect any other warning\n>> (or even GIT_TRACE=1 output) produced by gc --auto as well. I think we\n>> should consider it a serious bug, separate from the symptom Jonathan is\n>> fixing.\n>>\n>> Unfortunately I don't have a great idea about how to fix it. Should\n>> we avoid writing warnings to gc.log in daemon mode? That would\n>> prevent the user from ever seeing the warnings, but because of the\n>> nature of a warning, that might be reasonable.\n>\n> If you do that without anything further, then it will break the\n> protection against repeatedly running auto-gc, as I described in the\n> previous email.\n\nDo you mean ratelimiting for the message, or do you actually mean\nrepeatedly running auto-gc itself?\n\nIf we suppress warnings, there would still be a gc.log while \"git gc\n--auto\" is running, just as though there had been no warnings at all.\nI believe this is close to the intended behavior; it's the same as\nwhat you'd get without daemon mode, except that you lose the warning.\n\n[...]\n>> Should we put warnings\n>> in a separate log file with different semantics? Should we change the\n>> format of gc.log to allow more structured information (include 'gc'\n>> exit code) and perform a two-stage migration to the new format (first\n>> learn to read the new format, then switch to writing it)?\n>\n> Any of those would work similarly to the \"just detect warnings\" I\n> suggested earlier, with respect to keeping the \"1 day\" expiration\n> intact, so I'd be OK with them. In theory they'd be more robust than\n> scraping the \"warning:\" prefix. But in practice, I think you have to\n> resort to scraping anyway, if you are interested in treating warnings\n> from sub-processes the same way.\n\nCan you say more about this for me? I am not understanding what\nyou're saying necessitates scraping the output. I would strongly\nprefer to avoid scraping the output.\n\nThanks,\nJonathan\n"},{"id":"352658","messageId":"20180716191505.857-1-newren@gmail.com","threadId":"48890","inReplyTo":"20180716172717.237373-1-jonathantanmy@google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T19:15:05Z","receivedAt":"2018-07-16T19:15:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nOn Mon, Jul 16, 2018 at 10:27 AM, Jonathan Tan wrote:\n> In a087cc9819 (\"git-gc --auto: protect ourselves from accumulated\n> cruft\", 2007-09-17), the user was warned if there were too many\n> unreachable loose objects. This made sense at the time, because gc\n> couldn't prune them safely. But subsequently, git prune learned the\n> ability to not prune recently created loose objects, making pruning able\n> to be done more safely, and gc was made to automatically prune old\n> unreachable loose objects in 25ee9731c1 (\"gc: call \"prune --expire\n> 2.weeks.ago\" by default\", 2008-03-12).\n...\n>\n> ---\n> This was noticed when a daemonized gc run wrote this warning to the log\n> file, and returned 0; but a subsequent run merely read the log file, saw\n> that it is non-empty and returned -1 (which is inconsistent in that such\n> a run should return 0, as it did the first time).\n\nYeah, we've hit this several times too. I even created a testcase and a\nworkaround, though I never got it into proper submission form.\n\nThe basic problem here, at least for us, is that gc has enough\ninformation to know it could expunge some objects, but because of how\nit is structured in terms of several substeps (reflog expiration,\nrepack, prune), the information is lost between the steps and it\ninstead writes them out as unreachable objects. If we could prune (or\navoid exploding) loose objects that are only reachable from reflog\nentries that we are expiring, then the problem goes away for us. (I\ntotally understand that other repos may have enough unreachable\nobjects for other reasons that Peff's suggestion to just pack up\nunreachable objects is still a really good idea. But on its own, it\nseems like a waste since it's packing stuff that we know we could just\nexpunge.)\n\nAnyway, my very rough testcase is below. The workaround is the\ngit_actual_garbage_collect() function (minus the call to\nwait_for_background_gc_to_finish).\n\nElijah\n\n---\n\n\nwait_for_background_gc_to_finish() {\n while ( ps -ef | grep -v grep | grep --quiet git.gc.--auto ); do\n sleep 1;\n done\n}\n\ngit_standard_garbage_collect() {\n # Current git gc sprays unreachable objects back in loose form; this is\n # fine in many cases, but is annoying when done with objects which\n # newly become unreachable because of something else git-gc does and\n # git-gc doesn't clean them up.\n git gc --auto\n wait_for_background_gc_to_finish\n}\n\ngit_actual_garbage_collect() {\n GITDIR=$(git rev-parse --git-dir)\n\n # Record all revisions stored in reflog before and after gc\n git rev-list --no-walk --reflog >$GITDIR/gc.original-refs\n git gc --auto\n wait_for_background_gc_to_finish\n git rev-list --no-walk --reflog >$GITDIR/gc.final-refs\n\n # Find out which reflog entries were removed\n DELETED_REFS=$(comm -23 <(sort $GITDIR/gc.original-refs) <(sort $GITDIR/gc.final-refs))\n\n # Get the list of objects which used to be reachable, but were made\n # unreachable due to gc's reflog expiration. To get these, I need\n # the intersection of things reachable from $DELETED_REFS and things\n # which are unreachable now.\n git rev-list --objects $DELETED_REFS --not --all --reflog | awk '{print $1}' >$GITDIR/gc.previously-reachable-objects\n git prune --expire=now --dry-run | awk '{print $1}' > $GITDIR/gc.unreachable-objects\n\n # Delete all the previously-reachable-objects made unreachable by the\n # reflog expiration done by git gc\n comm -12 <(sort $GITDIR/gc.unreachable-objects) <(sort $GITDIR/gc.previously-reachable-objects) | sed -e \"s#^\\(..\\)#$GITDIR/objects/\\1/#\" | xargs rm\n}\n\n\ntest -d testcase && { echo \"testcase exists; exiting\"; exit 1; }\ngit init testcase/\ncd testcase\n\n# Create a basic commit\necho hello >world\ngit add world\ngit commit -q -m \"Initial\"\n\n# Create a commit with lots of files\nfor i in {0000..9999}; do echo $i >$i; done\ngit add [0-9]*\ngit commit --quiet -m \"Lots of numbers\"\n\n# Pack it all up\ngit gc --quiet\n\n# Stop referencing the garbage\ngit reset --quiet --hard HEAD~1\n\n# Pretend we did all the above stuff 30 days ago\nfor rlog in $(find .git/logs -type f); do\n # Subtract 3E6 (just over 30 days) from every date (assuming dates have\n # exactly 10 digits, which just happens to be valid...right now at least)\n perl -i -ne '/(.*?)(\\b[0-9]{10}\\b)(.*)/ && print $1,$2-3000000,$3,\"\\n\"' $rlog\ndone\n\n# HOWEVER: note that the pack is new; if we make the pack old, the old objects\n# will get pruned for us. But it is quite common to have new packfiles with\n# old-and-soon-to-be-unreferenced-objects because frequent gc's mean moving\n# the objects to new packfiles often, and eventually the reflog is expired.\n# If you want to test them being part of an old packfile, uncomment this:\n# find .git/objects/pack -type f | xargs touch -t 200001010101\n\n# Create 50 packfiles in the current repo so that 'git gc --auto' will\n# trigger `git repack -A -d -l` instead of just `git repack -d -l`\nfor i in {01..50}; do\n git fast-export master | sed -e s/Initial/Initi$i/ | git -c fastimport.unpacklimit=0 fast-import --quiet |& grep -v \"Not updating refs/heads/master\"\ndone\n\necho \"*** Before gc, reflog refers to garbage-collectible commits: ***\"\ngit rev-list --no-walk --all --reflog\ncat .git/logs/refs/heads/master\necho \"*** Before gc, everything is packed with no loose objects: ***\"\ngit count-objects -v\n\ngit_standard_garbage_collect # Just `git gc --auto`\n#git_actual_garbage_collect # What I really want from `git gc --auto`\n\necho -e \"\\n\\n*** After gc, commit garbage collected and objects made loose: ***\"\ngit rev-list --no-walk --all --reflog\ncat .git/logs/refs/heads/master\ngit count-objects -v\n\necho -e \"\\n\\n*** Now we can trigger the 'too many unreachable' error: ***\"\ngit gc --auto\n"},{"id":"352659","messageId":"20180716191923.GC11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716191505.857-1-newren@gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T19:19:23Z","receivedAt":"2018-07-16T19:19:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nElijah Newren wrote:\n\n> The basic problem here, at least for us, is that gc has enough\n> information to know it could expunge some objects, but because of how\n> it is structured in terms of several substeps (reflog expiration,\n> repack, prune), the information is lost between the steps and it\n> instead writes them out as unreachable objects. If we could prune (or\n> avoid exploding) loose objects that are only reachable from reflog\n> entries that we are expiring, then the problem goes away for us.\n\nMy understanding is that exploding the objects is intentional behavior,\nto avoid a race where objects are newly referenced while they are being\npruned.\n\nI am not a fan of that behavior. It's still racy. But when we've\nbrought it up in the past, the consensus seems to have been that it's\nbetter than nothing. Documentation/technical/hash-function-transition.txt\nsection \"Loose objects and unreachable objects\" describes a way to\neliminate the race.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"352662","messageId":"20180716194136.GA25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716190949.GB11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T19:41:36Z","receivedAt":"2018-07-16T19:41:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 12:09:49PM -0700, Jonathan Nieder wrote:\n\n> >>> So while I completely agree that it's not a great thing to encourage\n> >>> users to blindly run \"git prune\", I think it _is_ actionable.\n> >>\n> >> To flesh this out a little more: what user action do you suggest? Could\n> >> we carry out that action automatically?\n> >\n> > Er, the action is to run \"git prune\", like the warning says. :)\n> \n> I don't think we want to recommend that, especially when \"git gc --auto\"\n> does the right thing automatically.\n\nBut that's the point. This warning is written literally after running\n\"git gc --auto\" _didn't_ do the right thing. Yes, it would be nicer if\nit could do the right thing. But it doesn't yet know how to.\n\nSee the thread I linked earlier on putting unreachable objects into a\npack, which I think is the real solution.\n\n> > The warning that is deleted by this patch is: you _just_ ran gc, and hey\n> > we even did it automatically for you, but we're still in a funky state\n> > afterwards. You might need to clean up this state.\n> \n> This sounds awful. It sounds to me like you're saying \"git gc --auto\"\n> is saying \"I just did the wrong thing, and here is how you can clean\n> up after me\". That's not how I want a program to behave.\n\nSure, it would be nice if it did the right thing. Nobody has written\nthat yet. Until they do, we have to deal with the fallout.\n\n> > If you do that without anything further, then it will break the\n> > protection against repeatedly running auto-gc, as I described in the\n> > previous email.\n> \n> Do you mean ratelimiting for the message, or do you actually mean\n> repeatedly running auto-gc itself?\n> \n> If we suppress warnings, there would still be a gc.log while \"git gc\n> --auto\" is running, just as though there had been no warnings at all.\n> I believe this is close to the intended behavior; it's the same as\n> what you'd get without daemon mode, except that you lose the warning.\n\nI mean that if you do not write a persistent log, then \"gc --auto\" will\ndo an unproductive gc every time it is invoked. I.e., it will see \"oh,\nthere are too many loose objects\", and then waste a bunch of CPU every\ntime you commit.\n\n> > Any of those would work similarly to the \"just detect warnings\" I\n> > suggested earlier, with respect to keeping the \"1 day\" expiration\n> > intact, so I'd be OK with them. In theory they'd be more robust than\n> > scraping the \"warning:\" prefix. But in practice, I think you have to\n> > resort to scraping anyway, if you are interested in treating warnings\n> > from sub-processes the same way.\n> \n> Can you say more about this for me? I am not understanding what\n> you're saying necessitates scraping the output. I would strongly\n> prefer to avoid scraping the output.\n\nA daemonized git-gc runs a bunch of sub-commands (e.g., \"git prune\")\nwith their stderr redirected into the logfile. If you want to have\nwarnings go somewhere else, then either:\n\n - you need some way to tell those sub-commands to send the warnings\n elsewhere (i.e., _not_ stderr)\n\n or\n\n - you have to post-process the output they send to separate warnings\n from other errors. Right now that means scraping. If you are\n proposing a system of machine-readable output, it would need to work\n not just for git-gc, but for every sub-command it runs.\n\n-Peff\n"},{"id":"352663","messageId":"20180716195226.GB25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716191505.857-1-newren@gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T19:52:26Z","receivedAt":"2018-07-16T19:52:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 12:15:05PM -0700, Elijah Newren wrote:\n\n> The basic problem here, at least for us, is that gc has enough\n> information to know it could expunge some objects, but because of how\n> it is structured in terms of several substeps (reflog expiration,\n> repack, prune), the information is lost between the steps and it\n> instead writes them out as unreachable objects. If we could prune (or\n> avoid exploding) loose objects that are only reachable from reflog\n> entries that we are expiring, then the problem goes away for us. (I\n> totally understand that other repos may have enough unreachable\n> objects for other reasons that Peff's suggestion to just pack up\n> unreachable objects is still a really good idea. But on its own, it\n> seems like a waste since it's packing stuff that we know we could just\n> expunge.)\n\nNo, we should have expunged everything that could be during the \"repack\"\nand \"prune\" steps. We feed the expiration time to repack, so that it\nknows to drop objects entirely instead of exploding them loose.\n\nSo the leftovers really are objects that cannot be deleted yet. You\ncould literally just do:\n\n find .git/objects/?? -type f |\n perl -lne 's{../.{38}$} and print \"$1$2\"' |\n git pack-objects .git/objects/pack/cruft-pack\n\nBut:\n\n - that will explode them out only to repack them, which is inefficient\n (if they're already packed, you can probably reuse deltas, not to\n mention the I/O savings)\n\n - there's the question of how to handle timestamps. Some of those\n objects may have been _about_ to expire, but now you've just put\n them in a brand-new pack that adds another 2 weeks to their life\n\n - the find above is sloppy, and will race with somebody adding new\n objects to the repo\n\nSo probably you want to have pack-objects write out the list of objects\nit _would_ explode, rather than exploding them. And then before\ngit-repack deletes the old packs, put those into a new cruft pack. That\n_just_ leaves the timestamp issue (which is discussed at length in the\nthread I linked earlier).\n\n> git_actual_garbage_collect() {\n> GITDIR=$(git rev-parse --git-dir)\n> \n> # Record all revisions stored in reflog before and after gc\n> git rev-list --no-walk --reflog >$GITDIR/gc.original-refs\n> git gc --auto\n> wait_for_background_gc_to_finish\n> git rev-list --no-walk --reflog >$GITDIR/gc.final-refs\n> \n> # Find out which reflog entries were removed\n> DELETED_REFS=$(comm -23 <(sort $GITDIR/gc.original-refs) <(sort $GITDIR/gc.final-refs))\n\nThis is too detailed, I think. There are other reasons to have\nunreachable objects than expired reflogs. I think you really just want\nto consider all unreachable objects (like the pack-objects thing I\nmentioned above).\n\n-Peff\n"},{"id":"352664","messageId":"20180716195431.GD11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716194136.GA25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T19:54:31Z","receivedAt":"2018-07-16T19:54:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Jul 16, 2018 at 12:09:49PM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> Er, the action is to run \"git prune\", like the warning says. :)\n>>\n>> I don't think we want to recommend that, especially when \"git gc --auto\"\n>> does the right thing automatically.\n>\n> But that's the point. This warning is written literally after running\n> \"git gc --auto\" _didn't_ do the right thing. Yes, it would be nicer if\n> it could do the right thing. But it doesn't yet know how to.\n\nI think we fundamentally disagree, and I would like your help getting\npast this impasse.\n\nEven restricting attention to the warning, not the exit code, I think\nyou are saying the current behavior is acceptable. I do not believe\nit is. I think you are saying Jonathan's patch makes the behavior\nworse, and I'm not seeing it. Can you describe an example user\ninteraction where the current behavior would be better than the\nbehavior after Jonathan's patch? That might help make this discussion\nmore concrete.\n\n[...]\n> See the thread I linked earlier on putting unreachable objects into a\n> pack, which I think is the real solution.\n\nHave you looked over the discussion in \"Loose objects and unreachable\nobjects\" in Documentation/technical/hash-function-transition.txt? Do\nyou have thoughts on it (preferrably in a separate thread)?\n\n[...]\n>> This sounds awful. It sounds to me like you're saying \"git gc --auto\"\n>> is saying \"I just did the wrong thing, and here is how you can clean\n>> up after me\". That's not how I want a program to behave.\n>\n> Sure, it would be nice if it did the right thing. Nobody has written\n> that yet. Until they do, we have to deal with the fallout.\n\n\"git gc --auto\" is already doing the right thing.\n\n[...]\n> I mean that if you do not write a persistent log, then \"gc --auto\" will\n> do an unproductive gc every time it is invoked. I.e., it will see \"oh,\n> there are too many loose objects\", and then waste a bunch of CPU every\n> time you commit.\n\nIf so, then this would already be the behavior in non daemon mode.\nAre you saying this accidental effect of daemon mode is in fact\nintentional?\n\n[...]\n>>> But in practice, I think you have to\n>>> resort to scraping anyway, if you are interested in treating warnings\n>>> from sub-processes the same way.\n>>\n>> Can you say more about this for me? I am not understanding what\n>> you're saying necessitates scraping the output. I would strongly\n>> prefer to avoid scraping the output.\n>\n> A daemonized git-gc runs a bunch of sub-commands (e.g., \"git prune\")\n> with their stderr redirected into the logfile. If you want to have\n> warnings go somewhere else, then either:\n>\n> - you need some way to tell those sub-commands to send the warnings\n> elsewhere (i.e., _not_ stderr)\n>\n> or\n>\n> - you have to post-process the output they send to separate warnings\n> from other errors. Right now that means scraping. If you are\n> proposing a system of machine-readable output, it would need to work\n> not just for git-gc, but for every sub-command it runs.\n\n or\n\n - you can simply record and trust the exit code\n\nA simple way to do that without changing formats is to truncate the\nfile when exiting with status 0.\n\nThanks,\nJonathan\n"},{"id":"352667","messageId":"CABPp-BGdzyhPkFYyocqArtMX8=cDKFuV88q3mboeaTDjt275Tw@mail.gmail.com","threadId":"48890","inReplyTo":"20180716195226.GB25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T20:16:45Z","receivedAt":"2018-07-16T20:16:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 16, 2018 at 12:52 PM, Jeff King wrote:\n> On Mon, Jul 16, 2018 at 12:15:05PM -0700, Elijah Newren wrote:\n>\n>> The basic problem here, at least for us, is that gc has enough\n>> information to know it could expunge some objects, but because of how\n>> it is structured in terms of several substeps (reflog expiration,\n>> repack, prune), the information is lost between the steps and it\n>> instead writes them out as unreachable objects. If we could prune (or\n>> avoid exploding) loose objects that are only reachable from reflog\n>> entries that we are expiring, then the problem goes away for us. (I\n>> totally understand that other repos may have enough unreachable\n>> objects for other reasons that Peff's suggestion to just pack up\n>> unreachable objects is still a really good idea. But on its own, it\n>> seems like a waste since it's packing stuff that we know we could just\n>> expunge.)\n>\n> No, we should have expunged everything that could be during the \"repack\"\n> and \"prune\" steps. We feed the expiration time to repack, so that it\n> knows to drop objects entirely instead of exploding them loose.\n\nUm, except it doesn't actually do that. The testcase I provided shows\nthat it leaves around 10000 objects that are totally deletable and\nwere only previously referenced by reflog entries -- entries that gc\nremoved without removing the corresponding objects.\n\n\nI will note that my testcase was slightly out-of-date; with current\ngit it needs a call to 'wait_for_background_gc_to_finish' right before\nthe 'git gc --quiet' to avoid erroring out.\n\n> You\n> could literally just do:\n>\n> find .git/objects/?? -type f |\n> perl -lne 's{../.{38}$} and print \"$1$2\"' |\n> git pack-objects .git/objects/pack/cruft-pack\n>\n> But:\n>\n> - that will explode them out only to repack them, which is inefficient\n> (if they're already packed, you can probably reuse deltas, not to\n> mention the I/O savings)\n>\n> - there's the question of how to handle timestamps. Some of those\n> objects may have been _about_ to expire, but now you've just put\n> them in a brand-new pack that adds another 2 weeks to their life\n>\n> - the find above is sloppy, and will race with somebody adding new\n> objects to the repo\n>\n> So probably you want to have pack-objects write out the list of objects\n> it _would_ explode, rather than exploding them. And then before\n> git-repack deletes the old packs, put those into a new cruft pack. That\n> _just_ leaves the timestamp issue (which is discussed at length in the\n> thread I linked earlier).\n>\n>> git_actual_garbage_collect() {\n>> GITDIR=$(git rev-parse --git-dir)\n>>\n>> # Record all revisions stored in reflog before and after gc\n>> git rev-list --no-walk --reflog >$GITDIR/gc.original-refs\n>> git gc --auto\n>> wait_for_background_gc_to_finish\n>> git rev-list --no-walk --reflog >$GITDIR/gc.final-refs\n>>\n>> # Find out which reflog entries were removed\n>> DELETED_REFS=$(comm -23 <(sort $GITDIR/gc.original-refs) <(sort $GITDIR/gc.final-refs))\n>\n> This is too detailed, I think. There are other reasons to have\n> unreachable objects than expired reflogs. I think you really just want\n> to consider all unreachable objects (like the pack-objects thing I\n> mentioned above).\n\nYes, like I said, coarse workaround and I never had time to create a\nreal fix. But I thought the testcase might be useful as a\ndemonstration of how git gc leaves around loose objects that were\npreviously reference by reflogs that gc itself pruned.\n"},{"id":"352668","messageId":"CABPp-BGN4NeH2QAP0WMyuOdr2GYq+hpt9584wdn2jHHinEAgDg@mail.gmail.com","threadId":"48890","inReplyTo":"20180716191923.GC11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T20:21:40Z","receivedAt":"2018-07-16T20:21:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Mon, Jul 16, 2018 at 12:19 PM, Jonathan Nieder wrote:\n> Elijah Newren wrote:\n>\n>> The basic problem here, at least for us, is that gc has enough\n>> information to know it could expunge some objects, but because of how\n>> it is structured in terms of several substeps (reflog expiration,\n>> repack, prune), the information is lost between the steps and it\n>> instead writes them out as unreachable objects. If we could prune (or\n>> avoid exploding) loose objects that are only reachable from reflog\n>> entries that we are expiring, then the problem goes away for us.\n>\n> My understanding is that exploding the objects is intentional behavior,\n> to avoid a race where objects are newly referenced while they are being\n> pruned.\n>\n> I am not a fan of that behavior. It's still racy. But when we've\n> brought it up in the past, the consensus seems to have been that it's\n> better than nothing. Documentation/technical/hash-function-transition.txt\n> section \"Loose objects and unreachable objects\" describes a way to\n> eliminate the race.\n\nAh, that's good to know and at least makes sense. It seems somewhat\nodd, though; loose objects that are two weeks old are just as\nsusceptible to being referenced anew by new commits, so the default of\nrunning 'git prune --expire=2.weeks.ago' as gc currently does would\nalso be unsafe, wouldn't it? Why is that any more or less unsafe than\npruning objects only referenced by reflog entries that are more than\n90 days old?\n\nElijah\n"},{"id":"352669","messageId":"20180716202915.GC25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716195431.GD11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T20:29:16Z","receivedAt":"2018-07-16T20:29:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 12:54:31PM -0700, Jonathan Nieder wrote:\n\n> Even restricting attention to the warning, not the exit code, I think\n> you are saying the current behavior is acceptable. I do not believe\n> it is.\n\nI didn't say that at all. The current situation sucks, and I think the\nright solution is to pack the unreachable objects instead of exploding\nthem.\n\n> I think you are saying Jonathan's patch makes the behavior\n> worse, and I'm not seeing it. Can you describe an example user\n> interaction where the current behavior would be better than the\n> behavior after Jonathan's patch? That might help make this discussion\n> more concrete.\n\nIt makes it worse because there is nothing to throttle a \"gc --auto\"\nthat is making no progress.\n\nTry this:\n\n-- >8 --\n#!/bin/sh\n\nrm -rf repo\n\n# new mostly-empty repo\ngit init repo\ncd repo\ngit commit --allow-empty -m base\n\n# make a bunch of unreachable blobs\nfor i in $(seq 7000); do\n echo \"blob\"\n echo \"data < [...]\n> > See the thread I linked earlier on putting unreachable objects into a\n> > pack, which I think is the real solution.\n> \n> Have you looked over the discussion in \"Loose objects and unreachable\n> objects\" in Documentation/technical/hash-function-transition.txt? Do\n> you have thoughts on it (preferrably in a separate thread)?\n\nIt seems to propose putting the unreachable objects into a pack. Which\nyes, I absolutely agree with (as I thought I'd been saying in every\nsingle email in this thread).\n\n> > I mean that if you do not write a persistent log, then \"gc --auto\" will\n> > do an unproductive gc every time it is invoked. I.e., it will see \"oh,\n> > there are too many loose objects\", and then waste a bunch of CPU every\n> > time you commit.\n> \n> If so, then this would already be the behavior in non daemon mode.\n> Are you saying this accidental effect of daemon mode is in fact\n> intentional?\n\nI'm not sure if I'd call it intentional, since I don't recall ever\nseeing this side effect discussed. But since daemonizing is the default,\nI think that's the case people have focused on when they hit annoying\ncases. E.g., a831c06a2b (gc: ignore old gc.log files, 2017-02-10).\n\nI'd consider the fact that \"gc --auto\" with gc.autoDetach=false will\nrepeatedly do useless work to be a minor bug. But I think Jonathan's\npatch makes it even worse because we do not even tell people that\nuseless work is being done (while making them wait for each gc to\nfinish!). Even if we were going to remove this message to help the\ndaemonized case, I think we'd want to retain it for the non-daemon case.\n\n> > A daemonized git-gc runs a bunch of sub-commands (e.g., \"git prune\")\n> > with their stderr redirected into the logfile. If you want to have\n> > warnings go somewhere else, then either:\n> >\n> > - you need some way to tell those sub-commands to send the warnings\n> > elsewhere (i.e., _not_ stderr)\n> >\n> > or\n> >\n> > - you have to post-process the output they send to separate warnings\n> > from other errors. Right now that means scraping. If you are\n> > proposing a system of machine-readable output, it would need to work\n> > not just for git-gc, but for every sub-command it runs.\n> \n> or\n> \n> - you can simply record and trust the exit code\n> \n> A simple way to do that without changing formats is to truncate the\n> file when exiting with status 0.\n\nThat's a different behavior than what we do now (and what was suggested\nearlier), in that it assumes that anything written to stderr is OK for\ngc to hide from the user if the process exits with code zero.\n\nThat's probably OK in most cases, though I wonder if there are corner\ncases. For example, if you have a corrupt ref, we used to say \"error:\nrefs/heads/foo does not point to a valid object!\" but otherwise ignore\nit. These days git-gc sets REF_PARANOIA=1, so we'll actually barf on a\ncorrupt ref. But I wonder if there are other cases lurking.\n\n-Peff\n"},{"id":"352670","messageId":"20180716203539.GD25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"CABPp-BGN4NeH2QAP0WMyuOdr2GYq+hpt9584wdn2jHHinEAgDg@mail.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T20:35:39Z","receivedAt":"2018-07-16T20:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 01:21:40PM -0700, Elijah Newren wrote:\n\n> > My understanding is that exploding the objects is intentional behavior,\n> > to avoid a race where objects are newly referenced while they are being\n> > pruned.\n> >\n> > I am not a fan of that behavior. It's still racy. But when we've\n> > brought it up in the past, the consensus seems to have been that it's\n> > better than nothing. Documentation/technical/hash-function-transition.txt\n> > section \"Loose objects and unreachable objects\" describes a way to\n> > eliminate the race.\n> \n> Ah, that's good to know and at least makes sense. It seems somewhat\n> odd, though; loose objects that are two weeks old are just as\n> susceptible to being referenced anew by new commits, so the default of\n> running 'git prune --expire=2.weeks.ago' as gc currently does would\n> also be unsafe, wouldn't it? Why is that any more or less unsafe than\n> pruning objects only referenced by reflog entries that are more than\n> 90 days old?\n\nThe 2-week safety isn't primarily about things which just became\nunreferenced. It's about things which are in the act of being\nreferenced.\n\nImagine a \"git commit\" racing with a \"git prune\". The commit has to\ncreate an object, and then it will update a ref to point to it. But\nbetween those two actions, prune may racily delete the object!\nThe mtime grace period is what makes that work.\n\nUsing 2 weeks is sort of ridiculous for that. But it also helps with\nmanual recovery (e.g., imagine a blob added to the index but never\ncommitted; 3 days later you may want to try to recover your old work).\n\nAnd you're correct that a new git-commit may still reference an old\nobject (e.g., a blob that's 5 seconds shy of being 2 weeks old that\nyou're including in a new commit). That's why we retain non-fresh\nobjects that are referenced from fresh ones (so as long as you made the\nnew commit recently, it transitively infers freshness on the old blob),\nand why we fresh mtimes when we elide a write for an existing object.\n\nThat's _still_ not race-proof, because none of these operations is\natomic. git-prune can decide the blob is unfresh at the exact moment\nyou're creating the commit object.\n\n-Peff\n"},{"id":"352671","messageId":"20180716203753.GE11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716202915.GC25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T20:37:53Z","receivedAt":"2018-07-16T20:37:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Jul 16, 2018 at 12:54:31PM -0700, Jonathan Nieder wrote:\n\n>> Even restricting attention to the warning, not the exit code, I think\n>> you are saying the current behavior is acceptable. I do not believe\n>> it is.\n>\n> I didn't say that at all. The current situation sucks,\n\nThanks for clarifying. That helps.\n\n> and I think the\n> right solution is to pack the unreachable objects instead of exploding\n> them.\n\nThat seems like a huge widening in scope relative to what this\noriginal patch tackled. I'm not aware of a way to do this without\nbreaking compatibility with the current (broken) race prevention. If\nyou're saying that breaking compatibility in that way is okay with\nyou, then great!\n\n[...]\n>> I think you are saying Jonathan's patch makes the behavior\n>> worse, and I'm not seeing it. Can you describe an example user\n>> interaction where the current behavior would be better than the\n>> behavior after Jonathan's patch? That might help make this discussion\n>> more concrete.\n>\n> It makes it worse because there is nothing to throttle a \"gc --auto\"\n> that is making no progress.\n[...]\n> With the current code, that produces a bunch of annoying warnings for\n> every commit (\"I couldn't gc because the last gc reported a warning\").\n> But after Jonathan's patch, every single commit will do a full gc of the\n> repository. In this tiny repository that's relatively quick, but in a\n> real repo it's a ton of CPU and I/O, all for nothing.\n\nI see. Do I understand correctly that if we find a way to print the\nwarning but not error out, that would be good enough for you?\n\n[...]\n>> Have you looked over the discussion in \"Loose objects and unreachable\n>> objects\" in Documentation/technical/hash-function-transition.txt? Do\n>> you have thoughts on it (preferrably in a separate thread)?\n>\n> It seems to propose putting the unreachable objects into a pack. Which\n> yes, I absolutely agree with (as I thought I'd been saying in every\n> single email in this thread).\n\nI figured you were proposing something like\nhttps://public-inbox.org/git/20180113100734.GA30698@sigill.intra.peff.net/,\nwhich is still racy (because it does not handle the freshening in a safe\nway).\n\n[...]\n> Even if we were going to remove this message to help the\n> daemonized case, I think we'd want to retain it for the non-daemon case.\n\nInteresting. That should be doable, e.g. following the approach\ndescribed below.\n\n[...]\n>> A simple way to do that without changing formats is to truncate the\n>> file when exiting with status 0.\n>\n> That's a different behavior than what we do now (and what was suggested\n> earlier), in that it assumes that anything written to stderr is OK for\n> gc to hide from the user if the process exits with code zero.\n>\n> That's probably OK in most cases, though I wonder if there are corner\n> cases. For example, if you have a corrupt ref, we used to say \"error:\n> refs/heads/foo does not point to a valid object!\" but otherwise ignore\n> it. These days git-gc sets REF_PARANOIA=1, so we'll actually barf on a\n> corrupt ref. But I wonder if there are other cases lurking.\n\nWhat decides it for me is that the user did not invoke \"git gc --auto\"\nexplicitly, so anything \"git gc --auto\" prints is tangential to what\nthe user was trying to do. If the gc failed, that is worth telling\nthem, but if e.g. it encountered a disk I/O error reading and\nsucceeded on retry (to make up a fake example), then that's likely\nworth logging to syslog but it's not something the user asked to be\ndirectly informed about.\n\nThanks,\nJonathan\n"},{"id":"352672","messageId":"20180716203847.GE25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"CABPp-BGdzyhPkFYyocqArtMX8=cDKFuV88q3mboeaTDjt275Tw@mail.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T20:38:47Z","receivedAt":"2018-07-16T20:38:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 01:16:45PM -0700, Elijah Newren wrote:\n\n> >> The basic problem here, at least for us, is that gc has enough\n> >> information to know it could expunge some objects, but because of how\n> >> it is structured in terms of several substeps (reflog expiration,\n> >> repack, prune), the information is lost between the steps and it\n> >> instead writes them out as unreachable objects. If we could prune (or\n> >> avoid exploding) loose objects that are only reachable from reflog\n> >> entries that we are expiring, then the problem goes away for us. (I\n> >> totally understand that other repos may have enough unreachable\n> >> objects for other reasons that Peff's suggestion to just pack up\n> >> unreachable objects is still a really good idea. But on its own, it\n> >> seems like a waste since it's packing stuff that we know we could just\n> >> expunge.)\n> >\n> > No, we should have expunged everything that could be during the \"repack\"\n> > and \"prune\" steps. We feed the expiration time to repack, so that it\n> > knows to drop objects entirely instead of exploding them loose.\n> \n> Um, except it doesn't actually do that. The testcase I provided shows\n> that it leaves around 10000 objects that are totally deletable and\n> were only previously referenced by reflog entries -- entries that gc\n> removed without removing the corresponding objects.\n\nWhat's your definition of \"totally deletable\"?\n\nIf the pack they were in is less than 2 weeks old, then they are\nunreachable but \"fresh\", and are intentionally retained. If it was older\nthan 2 weeks, they should have been deleted.\n\nIf you don't like that policy, you can set gc.pruneExpire to something\nlower (including \"now\", but watch out, as that can race with other\noperations!). But I don't think that's an implementation short-coming.\nIt's intentional to keep those objects. It's just that the format\nthey're in is inefficient and confuses later gc runs.\n\n-Peff\n"},{"id":"352678","messageId":"20180716205646.GF11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716203539.GD25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T20:56:46Z","receivedAt":"2018-07-16T20:56:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Jul 16, 2018 at 01:21:40PM -0700, Elijah Newren wrote:\n>> Jonathan Nieder wrote:\n\n>>> My understanding is that exploding the objects is intentional behavior,\n>>> to avoid a race where objects are newly referenced while they are being\n>>> pruned.\n[...]\n>> Ah, that's good to know and at least makes sense. It seems somewhat\n>> odd, though; loose objects that are two weeks old are just as\n>> susceptible to being referenced anew by new commits, so the default of\n>> running 'git prune --expire=2.weeks.ago' as gc currently does would\n>> also be unsafe, wouldn't it? Why is that any more or less unsafe than\n>> pruning objects only referenced by reflog entries that are more than\n>> 90 days old?\n\nPart of the answer is that this safety feature applies even when\nreflogs are not in use. Another part is that as you say, you can\nstart referencing an object at the same time as the reflog entry\npointing to it is expiring.\n\n[...]\n> That's why we retain non-fresh\n> objects that are referenced from fresh ones (so as long as you made the\n> new commit recently, it transitively infers freshness on the old blob),\n> and why we fresh mtimes when we elide a write for an existing object.\n>\n> That's _still_ not race-proof, because none of these operations is\n> atomic.\n\nIndeed. One of the advantages of using a packfile for unreachable\nobjects is that metadata associated with that packfile can be updated\natomically.\n\nI don't believe we are very good at transitively propagating freshness\ntoday, by the way.\n\nThanks,\nJonathan\n"},{"id":"352681","messageId":"20180716210938.GF25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716203753.GE11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:09:38Z","receivedAt":"2018-07-16T21:09:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 01:37:53PM -0700, Jonathan Nieder wrote:\n\n> > and I think the\n> > right solution is to pack the unreachable objects instead of exploding\n> > them.\n> \n> That seems like a huge widening in scope relative to what this\n> original patch tackled.\n\nI'm not at all saying that Jonathan needs to work on that. It's a big\ntopic that we've been putting off for years. I _am_ saying that I don't\nthink his patch as-is should be merged, as it makes the daemonized\nbehavior here worse.\n\n> I'm not aware of a way to do this without\n> breaking compatibility with the current (broken) race prevention. If\n> you're saying that breaking compatibility in that way is okay with\n> you, then great!\n\nIt depends what you're trying to fix. If you're trying to fix auto-gc\noutputting an annoying message (or repeatedly doing useless invocations\nof pack-objects), you don't have to break compatibility at all. You just\nhave to put the objects into a pack instead of exploding them loose.\nWorst case if you use an old implementation to run \"git gc\", it may\naccidentally freshen a cruft pack's timestamp.\n\nIf you want to improve the raciness, then yes, I think you need stronger\nrules around marking packs as cruft/garbage, and making operations that\nwant to use those objects copy them out to a non-cruft location (like\nyou describe in the hash transition document). There you risk corruption\nif an old implementation fails to make the non-cruft copy. But that's\nreally no worse than we are today.\n\nSo I don't even really see it as a backwards-compatibility break. But\neven if it were, I'd probably still be fine with it. This is a\nlocal-repo thing, and in the absolute worst case we could bump\ncore.repositoryformatversion (though again, I don't even think that\nwould be necessary since it would degrade to behaving just as the\ncurrent code does).\n\n> > With the current code, that produces a bunch of annoying warnings for\n> > every commit (\"I couldn't gc because the last gc reported a warning\").\n> > But after Jonathan's patch, every single commit will do a full gc of the\n> > repository. In this tiny repository that's relatively quick, but in a\n> > real repo it's a ton of CPU and I/O, all for nothing.\n> \n> I see. Do I understand correctly that if we find a way to print the\n> warning but not error out, that would be good enough for you?\n\nYes. I thought that's what I proposed originally. ;)\n\nThe key thing is that the presence of the warning/log still suppress\nthe actual gc for the gc.logExpiry period.\n\n> >> Have you looked over the discussion in \"Loose objects and unreachable\n> >> objects\" in Documentation/technical/hash-function-transition.txt? Do\n> >> you have thoughts on it (preferrably in a separate thread)?\n> >\n> > It seems to propose putting the unreachable objects into a pack. Which\n> > yes, I absolutely agree with (as I thought I'd been saying in every\n> > single email in this thread).\n> \n> I figured you were proposing something like\n> https://public-inbox.org/git/20180113100734.GA30698@sigill.intra.peff.net/,\n> which is still racy (because it does not handle the freshening in a safe\n> way).\n\nThat message is just me telling somebody that their idea _won't_ work. ;)\n\nBut I assume you mean the general idea of putting things in a cruft\npack[1]. Yes, I was only suggesting going that far as a solution to the\nauto-gc woes. Solving races is another issue entirely (though obviously\nit may make sense to build on the single-pack work, if it exists).\n\n[1] The best message discussion on that is I think:\n\n https://public-inbox.org/git/20170610080626.sjujpmgkli4muh7h@sigill.intra.peff.net/\n\n which I think I linked earlier, so possibly that is even what you\n meant. :)\n\n> What decides it for me is that the user did not invoke \"git gc --auto\"\n> explicitly, so anything \"git gc --auto\" prints is tangential to what\n> the user was trying to do. If the gc failed, that is worth telling\n> them, but if e.g. it encountered a disk I/O error reading and\n> succeeded on retry (to make up a fake example), then that's likely\n> worth logging to syslog but it's not something the user asked to be\n> directly informed about.\n\nI'm not sure I agree. If a repack discovered that you had a bit\ncorruption on disk, but you happened to have another copy of the object\navailable that made the repack succeed, I'd like to know that I'm\ngetting bit corruptions, and earlier is better. I think we need to\nsurface that information to the user eventually, and I don't think we\ncan count on syslog being universally available.\n\nBy definition we're daemonized here, so we can't count on even having\naccess to the user's terminal. But it would make more sense to me if the\nlogic were:\n\n - at the end of a \"gc --auto\" run, gc should write a machine-readable\n indication of its exit code (either as a final line in the log file,\n or to a gc.exitcode file). The warnings/errors remain intermingled\n in the logfile.\n\n - on the _next_ run, if:\n\n - the exit code was non-zero, dump the log output to stderr and exit\n with the same code. This is the same as the current behavior (with\n the exception that we propagate the code instead of returning -1,\n which is a minor bonus).\n\n - the exit code was zero, report to the user synchronously that the\n last run found some oddities, and dump the log. Proceed with the\n rest of auto-gc (either exiting 0 if nothing to be done, or doing\n useful work).\n\n Optionally, delete the log after showing it so the user is not\n spammed with the warning on every commit. Though for the warning\n under discussion, this does mean that if they don't take any\n action, they'll end up running a useless gc every _other_ commit.\n So I'd probably keep the log and let it expire naturally after the\n 1-day gc.logExpiry period.\n\n-Peff\n"},{"id":"352682","messageId":"CABPp-BEpCF9FE7eJwZWjY+bMsjDQnnDaSrHO+e3DtDDsR-=7Hg@mail.gmail.com","threadId":"48890","inReplyTo":"20180716203847.GE25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T21:09:43Z","receivedAt":"2018-07-16T21:09:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 16, 2018 at 1:38 PM, Jeff King wrote:\n> On Mon, Jul 16, 2018 at 01:16:45PM -0700, Elijah Newren wrote:\n>\n>> >> The basic problem here, at least for us, is that gc has enough\n>> >> information to know it could expunge some objects, but because of how\n>> >> it is structured in terms of several substeps (reflog expiration,\n>> >> repack, prune), the information is lost between the steps and it\n>> >> instead writes them out as unreachable objects. If we could prune (or\n>> >> avoid exploding) loose objects that are only reachable from reflog\n>> >> entries that we are expiring, then the problem goes away for us. (I\n>> >> totally understand that other repos may have enough unreachable\n>> >> objects for other reasons that Peff's suggestion to just pack up\n>> >> unreachable objects is still a really good idea. But on its own, it\n>> >> seems like a waste since it's packing stuff that we know we could just\n>> >> expunge.)\n>> >\n>> > No, we should have expunged everything that could be during the \"repack\"\n>> > and \"prune\" steps. We feed the expiration time to repack, so that it\n>> > knows to drop objects entirely instead of exploding them loose.\n>>\n>> Um, except it doesn't actually do that. The testcase I provided shows\n>> that it leaves around 10000 objects that are totally deletable and\n>> were only previously referenced by reflog entries -- entries that gc\n>> removed without removing the corresponding objects.\n>\n> What's your definition of \"totally deletable\"?\n\nThe point of gc is to: expire reflogs, repack referenced objects, and\ndelete loose objects that (1) are no longer referenced and (2) are\n\"old enough\".\n\nThe \"old enough\" bit is the special part here. Before the gc, those\nobjects were referenced (only by old reflog entries) and were found in\na pack. git currently writes those objects out to disk and gives them\nthe age of the packfile they are contained in, which I think is the\nsource of the bug. We have a reference for those objects from the\nreflogs, know they aren't referenced anywhere else, so those objects\nto me are the age of those reflog entries: 90 days. As such, they are\n\"old enough\" and should be deleted.\n\nWith big repos, it's easy to get into situations where there are well\nmore than 10000 objects satisfying these conditions. In fact, it's\nnot all that rare that the repo has far more loose objects after a git\ngc than before.\n\nI never got around to fixing it properly, though, because 'git prune'\nis such a handy workaround that for now. Having people nuke all their\nloose objects is a bit risky, but the only other workaround people had\nwas to re-clone (waiting the requisite 20+ minutes for the repo to\ndownload) and throw away their old clone. (Though some people even\nwent that route, IIRC.)\n\n> If the pack they were in is less than 2 weeks old, then they are\n> unreachable but \"fresh\", and are intentionally retained. If it was older\n> than 2 weeks, they should have been deleted.\n\nThat's what's currently implemented, yes, but as above I think that\nlogic is bad.\n\n> If you don't like that policy, you can set gc.pruneExpire to something\n> lower (including \"now\", but watch out, as that can race with other\n> operations!). But I don't think that's an implementation short-coming.\n> It's intentional to keep those objects. It's just that the format\n> they're in is inefficient and confuses later gc runs.\n\nI'm aware of pruneExpire, but I think it's the wrong way to handle this.\n\nI totally agree with your general plan to put unreferenced loose\nobjects into a pack. However, I don't think these objects should be\npart of that pack; they should just be deleted instead.\n"},{"id":"352684","messageId":"20180716211234.GG25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716205646.GF11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:12:35Z","receivedAt":"2018-07-16T21:12:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 01:56:46PM -0700, Jonathan Nieder wrote:\n\n> I don't believe we are very good at transitively propagating freshness\n> today, by the way.\n\nPerhaps I don't understand what you mean by transitive here. But we\nshould be traversing from any fresh object and marking any non-fresh\nones that are reachable from it to be saved. If you know of a case where\nwe're not, there's a bug, and I'd be happy to look into it.\n\nThe only problem I know about is the utter lack of atomicity.\n\n-Peff\n"},{"id":"352685","messageId":"20180716212159.GH25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"CABPp-BEpCF9FE7eJwZWjY+bMsjDQnnDaSrHO+e3DtDDsR-=7Hg@mail.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:21:59Z","receivedAt":"2018-07-16T21:22:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 02:09:43PM -0700, Elijah Newren wrote:\n\n> >> Um, except it doesn't actually do that. The testcase I provided shows\n> >> that it leaves around 10000 objects that are totally deletable and\n> >> were only previously referenced by reflog entries -- entries that gc\n> >> removed without removing the corresponding objects.\n> >\n> > What's your definition of \"totally deletable\"?\n> \n> The point of gc is to: expire reflogs, repack referenced objects, and\n> delete loose objects that (1) are no longer referenced and (2) are\n> \"old enough\".\n> \n> The \"old enough\" bit is the special part here. Before the gc, those\n> objects were referenced (only by old reflog entries) and were found in\n> a pack. git currently writes those objects out to disk and gives them\n> the age of the packfile they are contained in, which I think is the\n> source of the bug. We have a reference for those objects from the\n> reflogs, know they aren't referenced anywhere else, so those objects\n> to me are the age of those reflog entries: 90 days. As such, they are\n> \"old enough\" and should be deleted.\n\nOK, I see what you're saying, but...\n\n> I never got around to fixing it properly, though, because 'git prune'\n> is such a handy workaround that for now. Having people nuke all their\n> loose objects is a bit risky, but the only other workaround people had\n> was to re-clone (waiting the requisite 20+ minutes for the repo to\n> download) and throw away their old clone. (Though some people even\n> went that route, IIRC.)\n\nIf we were to delete those objects, wouldn't it be exactly the same as\nrunning \"git prune\"? Or setting your gc.pruneExpire to \"now\" or even \"5\nminutes\"? Or are you concerned with taking other objects along for the\nride that weren't part of old reflogs? I think that's a valid concern,\nbut it's also an issue for objects which were previously referenced in\na reflog, but are part of another current operation.\n\nAlso, what do you do if there weren't reflogs in the repo? Or the\nreflogs were deleted (e.g., because branch deletion drops the associated\nreflog entirely)?\n\n> With big repos, it's easy to get into situations where there are well\n> more than 10000 objects satisfying these conditions. In fact, it's\n> not all that rare that the repo has far more loose objects after a git\n> gc than before.\n\nYes, this is definitely a wart and I think is worth addressing.\n\n> I totally agree with your general plan to put unreferenced loose\n> objects into a pack. However, I don't think these objects should be\n> part of that pack; they should just be deleted instead.\n\nI assume by \"these objects\" you mean ones which used to be reachable\nfrom a reflog, but that reflog entry just expired. I think you'd be\nsacrificing some race-safety in that case.\n\nIf the objects went into a pack under a race-proof scheme, would that\nactually bother you? Is it the 10,000 objects that's a problem, or is it\nthe horrible I/O from exploding them coupled with the annoying auto-gc\nbehavior?\n\n-Peff\n"},{"id":"352688","messageId":"20180716213104.GG11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"CABPp-BEpCF9FE7eJwZWjY+bMsjDQnnDaSrHO+e3DtDDsR-=7Hg@mail.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T21:31:04Z","receivedAt":"2018-07-16T21:31:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Elijah Newren wrote:\n\n> I totally agree with your general plan to put unreferenced loose\n> objects into a pack. However, I don't think these objects should be\n> part of that pack; they should just be deleted instead.\n\nThis might be the wrong thread to discuss it, but did you follow the\nreference/prune race that Peff mentioned? The simplest cure I'm aware\nof to it does involve writing those objects to a pack. The idea is to\nenforce a straightforward contract:\n\nThere are two kinds of packs: GC and UNREACHABLE_GARBAGE.\n\nEvery object in a GC pack has a minimum lifetime of (let's say\n\"1 days\") from the time they are read. If you start making use of an\nobject from a GC pack (e.g. by creating a new object referencing it),\nyou have three days to ensure it's referenced.\n\nEach UNREACHABLE_GARBAGE pack has a (let's say \"3 days\") from\nthe time it is created. Objects in an UNREACHABLE_GARBAGE have no\nminimum ttl from the time they are read. If you want to start making\nuse of an object from an UNREACHABLE_GARBAGE pack (e.g. by creating a\nnew object referencing it), then copy it and everything it references\nto a GC pack.\n\nTo avoid a proliferation of UNREACHABLE_GARBAGE packs, there's a rule\nfor coalescing them, but that's not relevant here.\n\nIt is perfectly possible for an object in a GC pack to reference an\nobject in an UNREACHABLE_GARBAGE pack via writes racing with gc, but\nthat's fine --- on the next gc run, the unreachable garbage objects\nget copied to a GC pack.\n\nWe've been using this on a JGit DfsRepository based server for > 2\nyears now and it's been working well. More details are in the \"Loose\nobjects and unreachable objects\" section in Documentation/technical/\nmentioned before.\n\nThanks,\nJonathan\n"},{"id":"352694","messageId":"20180716214003.GH11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716210938.GF25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T21:40:03Z","receivedAt":"2018-07-16T21:40:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Jul 16, 2018 at 01:37:53PM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> With the current code, that produces a bunch of annoying warnings for\n>>> every commit (\"I couldn't gc because the last gc reported a warning\").\n>>> But after Jonathan's patch, every single commit will do a full gc of the\n>>> repository. In this tiny repository that's relatively quick, but in a\n>>> real repo it's a ton of CPU and I/O, all for nothing.\n>>\n>> I see. Do I understand correctly that if we find a way to print the\n>> warning but not error out, that would be good enough for you?\n>\n> Yes. I thought that's what I proposed originally. ;)\n>\n> The key thing is that the presence of the warning/log still suppress\n> the actual gc for the gc.logExpiry period.\n\nAh, now I think I see the source of the misunderstanding.\n\nWhen I initially read the docs, I also assumed that\n\n\tIf the file gc.log exists, then git gc --auto won’t run unless\n\tthat file is more than gc.logExpiry old.\n\nmeant \"git gc --auto\" would be skipped (and thus silently succeed) when\nthe file is less than gc.logExpiry old. But that assumption was wrong:\nit errors out!\n\nThis makes scripting difficult, since arbitrary commands that\nincidentally run \"git gc --auto\" will fail in this state, until gc.log\nexpires (but at that point they'll fail again anyway).\n\nFor comparison, in non-daemon mode, there is nothing enforcing the\nkind of ratelimiting you are talking about. It is an accident of\nhistory. If we want this kind of ratelimiting, I'd rather we build it\ndeliberately instead of relying on this accident.\n\nJonathan\n"},{"id":"352695","messageId":"20180716214539.GL25189@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716214003.GH11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:45:39Z","receivedAt":"2018-07-16T21:45:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 02:40:03PM -0700, Jonathan Nieder wrote:\n\n> > The key thing is that the presence of the warning/log still suppress\n> > the actual gc for the gc.logExpiry period.\n> \n> Ah, now I think I see the source of the misunderstanding.\n> \n> When I initially read the docs, I also assumed that\n> \n> \tIf the file gc.log exists, then git gc --auto won’t run unless\n> \tthat file is more than gc.logExpiry old.\n> \n> meant \"git gc --auto\" would be skipped (and thus silently succeed) when\n> the file is less than gc.logExpiry old. But that assumption was wrong:\n> it errors out!\n\nRight. That's the mysterious \"-1\" error code, and I agree that part is\nconfusing.\n\n> This makes scripting difficult, since arbitrary commands that\n> incidentally run \"git gc --auto\" will fail in this state, until gc.log\n> expires (but at that point they'll fail again anyway).\n\nI don't think any command should report failure of its _own_ operation\nif \"gc --auto\" failed. And grepping around the source code shows that we\ntypically ignore it.\n\n> For comparison, in non-daemon mode, there is nothing enforcing the\n> kind of ratelimiting you are talking about. It is an accident of\n> history. If we want this kind of ratelimiting, I'd rather we build it\n> deliberately instead of relying on this accident.\n\nWhat I was trying to say earlier is that we _did_ build this\nrate-limiting, and I think it is a bug that the non-daemon case does not\nrate-limit (but nobody noticed, because the default is daemonizing).\n\nSo the fix is not \"rip out the rate-limiting in daemon mode\", but rather\n\"extend it to the non-daemon case\".\n\n-Peff\n"},{"id":"352699","messageId":"20180716220306.GI11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716214539.GL25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T22:03:06Z","receivedAt":"2018-07-16T22:03:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I don't think any command should report failure of its _own_ operation\n> if \"gc --auto\" failed. And grepping around the source code shows that we\n> typically ignore it.\n\nOh, good point. In non-daemon mode, we don't let \"gc --auto\" failure\ncause the invoking command to fail, but in daemon mode we do. That\nshould be a straightforward fix; patch coming in a moment.\n\n> On Mon, Jul 16, 2018 at 02:40:03PM -0700, Jonathan Nieder wrote:\n\n>> For comparison, in non-daemon mode, there is nothing enforcing the\n>> kind of ratelimiting you are talking about. It is an accident of\n>> history. If we want this kind of ratelimiting, I'd rather we build it\n>> deliberately instead of relying on this accident.\n>\n> What I was trying to say earlier is that we _did_ build this\n> rate-limiting, and I think it is a bug that the non-daemon case does not\n> rate-limit (but nobody noticed, because the default is daemonizing).\n>\n> So the fix is not \"rip out the rate-limiting in daemon mode\", but rather\n> \"extend it to the non-daemon case\".\n\nCan you point me to some discussion about building that rate-limiting?\nThe commit message for v2.12.2~17^2 (gc: ignore old gc.log files,\n2017-02-10) definitely doesn't describe that as its intent.\n\nThis is the kind of review that Dscho often complains about, where\nsomeone tries to fix something small but significant to users and gets\ntold to build something larger that was not their itch instead.\n\nThe comments about the \"Why is 'git commit' so slow?\" experience and\nhow having the warning helps with that are well taken. I think we\nshould be able to find a way to keep the warning in a v2 of this\npatch. But the rest about rate-limiting and putting unreachable\nobjects in packs etc as a blocker for this are demoralizing, since\nthey gives the feeling that even if I handle the cases that are\nhandled today well, it will never be enough for the project unless I\nsolve the larger problems that were already there.\n\nJonathan\n"},{"id":"352700","messageId":"CABPp-BHFinoE1=1bOhiwOrYpLB+kB3yAKbNg77K9kqKDH_1JLA@mail.gmail.com","threadId":"48890","inReplyTo":"20180716212159.GH25189@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T22:07:34Z","receivedAt":"2018-07-16T22:07:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 16, 2018 at 2:21 PM, Jeff King wrote:\n> On Mon, Jul 16, 2018 at 02:09:43PM -0700, Elijah Newren wrote:\n\n>> The point of gc is to: expire reflogs, repack referenced objects, and\n>> delete loose objects that (1) are no longer referenced and (2) are\n>> \"old enough\".\n>>\n>> The \"old enough\" bit is the special part here. Before the gc, those\n>> objects were referenced (only by old reflog entries) and were found in\n>> a pack. git currently writes those objects out to disk and gives them\n>> the age of the packfile they are contained in, which I think is the\n>> source of the bug. We have a reference for those objects from the\n>> reflogs, know they aren't referenced anywhere else, so those objects\n>> to me are the age of those reflog entries: 90 days. As such, they are\n>> \"old enough\" and should be deleted.\n>\n> OK, I see what you're saying, but...\n>\n>> I never got around to fixing it properly, though, because 'git prune'\n>> is such a handy workaround that for now. Having people nuke all their\n>> loose objects is a bit risky, but the only other workaround people had\n>> was to re-clone (waiting the requisite 20+ minutes for the repo to\n>> download) and throw away their old clone. (Though some people even\n>> went that route, IIRC.)\n>\n> If we were to delete those objects, wouldn't it be exactly the same as\n> running \"git prune\"? Or setting your gc.pruneExpire to \"now\" or even \"5\n> minutes\"? Or are you concerned with taking other objects along for the\n> ride that weren't part of old reflogs? I think that's a valid concern,\n\nYes, I was worried about taking other objects along for the ride that\nweren't part of old reflogs.\n\n> but it's also an issue for objects which were previously referenced in\n> a reflog, but are part of another current operation.\n\nI'm not certain what you're referring to here.\n\n> Also, what do you do if there weren't reflogs in the repo? Or the\n> reflogs were deleted (e.g., because branch deletion drops the associated\n> reflog entirely)?\n\nYes, there are issues this rule won't help with, but in my experience\nit was a primary (if not sole) actual cause in practice. (I believe I\neven said elsewhere in this thread that I knew there were unreachable\nobjects for other reasons and they might also become large in number).\nAt $DAYJOB we've had multiple people including myself hit the \"too\nmany unreachable loose objects\" nasty loop issue (some of us multiple\ndifferent times), and as far as I can tell, most (perhaps even all) of\nthem would have been avoided by just \"properly\" deleting garbage as\nper my object-age-is-reflog-age-if-not-otherwise-referenced rule.\n\n>> With big repos, it's easy to get into situations where there are well\n>> more than 10000 objects satisfying these conditions. In fact, it's\n>> not all that rare that the repo has far more loose objects after a git\n>> gc than before.\n>\n> Yes, this is definitely a wart and I think is worth addressing.\n>\n>> I totally agree with your general plan to put unreferenced loose\n>> objects into a pack. However, I don't think these objects should be\n>> part of that pack; they should just be deleted instead.\n>\n> I assume by \"these objects\" you mean ones which used to be reachable\n> from a reflog, but that reflog entry just expired. I think you'd be\n> sacrificing some race-safety in that case.\n\nIs that inherently any more race unsafe than 'git prune\n--expire=2.weeks.ago'? I thought it'd be racy in the same ways, and\nthus a tradeoff folks are already accepting (at least implicitly) when\nrunning git-gc. Since these objects are actually 90 days old rather\nthan a mere two weeks, it actually seemed more safe to me. But maybe\nI'm overlooking something with the pack->loose transition that makes\nit more racy?\n\n> If the objects went into a pack under a race-proof scheme, would that\n> actually bother you? Is it the 10,000 objects that's a problem, or is it\n> the horrible I/O from exploding them coupled with the annoying auto-gc\n> behavior?\n\nYeah, good point. It's mostly the annoying auto-gc behavior and the\nhorrible I/O of future git operations from having lots of loose\nobjects. They've already been paying the cost of storing those\nobjects in packed form for 90 days; a few more won't hurt much. I'd\nbe slightly annoyed knowing that we're storing garbage we don't need\nto be, but I don't think it's a real issue.\n"},{"id":"352706","messageId":"20180716224337.GB12482@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716220306.GI11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T22:43:37Z","receivedAt":"2018-07-16T22:43:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 03:03:06PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > I don't think any command should report failure of its _own_ operation\n> > if \"gc --auto\" failed. And grepping around the source code shows that we\n> > typically ignore it.\n> \n> Oh, good point. In non-daemon mode, we don't let \"gc --auto\" failure\n> cause the invoking command to fail, but in daemon mode we do. That\n> should be a straightforward fix; patch coming in a moment.\n\nOK, that definitely sounds like a bug. I'm still confused how that could\nhappen, though, since from the caller's perspective they ignore git-gc's\nexit code either way. I guess I'll see in your patch. :)\n\n> > What I was trying to say earlier is that we _did_ build this\n> > rate-limiting, and I think it is a bug that the non-daemon case does not\n> > rate-limit (but nobody noticed, because the default is daemonizing).\n> >\n> > So the fix is not \"rip out the rate-limiting in daemon mode\", but rather\n> > \"extend it to the non-daemon case\".\n> \n> Can you point me to some discussion about building that rate-limiting?\n> The commit message for v2.12.2~17^2 (gc: ignore old gc.log files,\n> 2017-02-10) definitely doesn't describe that as its intent.\n\nI think that commit is a loosening of the rate-limiting (because we'd\nrefuse to progress for something that was actually time-based). But the\noriginal stopping comes from this discussion, I think:\n\n https://public-inbox.org/git/xmqqlhijznpm.fsf@gitster.dls.corp.google.com/\n\n(I didn't read the whole thread, but that was what I hit by blaming the\nlog code and then tracing that back to the list).\n\n> This is the kind of review that Dscho often complains about, where\n> someone tries to fix something small but significant to users and gets\n> told to build something larger that was not their itch instead.\n\nI don't know how to say more emphatically that I am not asking anyone to\nbuild something larger (like cruft packfiles). I'm just trying to bring\nup an impact that the author didn't consider (and that IMHO would be a\nregression). Isn't that what reviews are for?\n\nI only mention packfiles because as the discussion turns to \"well, all\nof these solutions are mediocre hacks\" (because they absolutely are),\nit's important to realize that there _is_ a right solution, and we even\nalready know about it. Even if we don't work on it now, knowing that\nit's there makes it easier to decide about the various hacks.\n\n> The comments about the \"Why is 'git commit' so slow?\" experience and\n> how having the warning helps with that are well taken. I think we\n> should be able to find a way to keep the warning in a v2 of this\n> patch. But the rest about rate-limiting and putting unreachable\n> objects in packs etc as a blocker for this are demoralizing, since\n> they gives the feeling that even if I handle the cases that are\n> handled today well, it will never be enough for the project unless I\n> solve the larger problems that were already there.\n\nI really don't know why we are having such trouble communicating. I've\ntried to make it clear several times that the pack thing is not\nsomething I expect your or Jonathan Tan to work on, but obviously I\nfailed. I'd be _delighted_ if you wanted to work on it, but AFAICT this\npatch is purely motivated by:\n\n 1. there's a funny exit code thing going on (0 on the first run, -1 on\n the second)\n\n 2. the warning is not actionable by users\n\nI disagree with the second, and I think we've discussed easy solutions\nfor the first.\n\n-Peff\n"},{"id":"352708","messageId":"20180716225520.GC12482@sigill.intra.peff.net","threadId":"48890","inReplyTo":"CABPp-BHFinoE1=1bOhiwOrYpLB+kB3yAKbNg77K9kqKDH_1JLA@mail.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T22:55:20Z","receivedAt":"2018-07-16T22:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 03:07:34PM -0700, Elijah Newren wrote:\n\n> > If we were to delete those objects, wouldn't it be exactly the same as\n> > running \"git prune\"? Or setting your gc.pruneExpire to \"now\" or even \"5\n> > minutes\"? Or are you concerned with taking other objects along for the\n> > ride that weren't part of old reflogs? I think that's a valid concern,\n> \n> Yes, I was worried about taking other objects along for the ride that\n> weren't part of old reflogs.\n> \n> > but it's also an issue for objects which were previously referenced in\n> > a reflog, but are part of another current operation.\n> \n> I'm not certain what you're referring to here.\n\nI mean that an ongoing operation could refer to a blob that just\nrecently became unreachable via reflog pruning. And then we would delete\nit, leaving the repository corrupt.\n\nOne of the protections we have against that is that if I ask to write\nblob XYZ and we find that we already have the object, Git will freshen\nthe mtime of the loose object or pack that contains it, protecting it\nfrom pruning. But with your suggestion, we'd delete it immediately,\nregardless of the mtime of the containing pack.\n\nAnother way to think of it is this: a reflog mentioning an object does\nnot mean it is the exclusive user of that object. So when we expire it,\nthat does not mean that it is OK to delete it immediately; there may be\nother users.\n\n> > Also, what do you do if there weren't reflogs in the repo? Or the\n> > reflogs were deleted (e.g., because branch deletion drops the associated\n> > reflog entirely)?\n> \n> Yes, there are issues this rule won't help with, but in my experience\n> it was a primary (if not sole) actual cause in practice. (I believe I\n> even said elsewhere in this thread that I knew there were unreachable\n> objects for other reasons and they might also become large in number).\n> At $DAYJOB we've had multiple people including myself hit the \"too\n> many unreachable loose objects\" nasty loop issue (some of us multiple\n> different times), and as far as I can tell, most (perhaps even all) of\n> them would have been avoided by just \"properly\" deleting garbage as\n> per my object-age-is-reflog-age-if-not-otherwise-referenced rule.\n\nI agree with you that this is a frequent cause, and probably even the\nprimary one. But my concern is that your loosening increases the risk of\ncorruption for other cases.\n\n> > I assume by \"these objects\" you mean ones which used to be reachable\n> > from a reflog, but that reflog entry just expired. I think you'd be\n> > sacrificing some race-safety in that case.\n> \n> Is that inherently any more race unsafe than 'git prune\n> --expire=2.weeks.ago'? I thought it'd be racy in the same ways, and\n> thus a tradeoff folks are already accepting (at least implicitly) when\n> running git-gc. Since these objects are actually 90 days old rather\n> than a mere two weeks, it actually seemed more safe to me. But maybe\n> I'm overlooking something with the pack->loose transition that makes\n> it more racy?\n\nI think it's worse in terms of race-safety because you're losing one of\nthe signals that users of the objects can provide to git-prune to tell\nit the object is useful: updating the mtime. So yes, you think of the\nobjects as \"90 days old\", but we don't know if there are other users.\nHas somebody else been accessing them in the meantime?\n\nTo be honest, I'm not sure how meaningful that distinction is in\npractice. The current scheme is still racy, even if the windows are\nshorter in some circumstances. But it seems like cruft packfiles are\na similar amount of work to your scheme, cover more cases, and are\nslightly safer. And possibly even give us a long-term route to true\nrace-free pruning (via the \"garbage pack\" mark that Jonathan mentioned).\n\nAssuming you buy into the idea that objects in a cruft-pack are not\nhurting anything aside from a little wasted storage for up to 2 weeks\n(which it sounds like you're at least partially on board with ;) ).\n\n-Peff\n"},{"id":"352709","messageId":"20180716225639.GK11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716224337.GB12482@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-16T22:56:39Z","receivedAt":"2018-07-16T22:56:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Jul 16, 2018 at 03:03:06PM -0700, Jonathan Nieder wrote:\n\n>> Oh, good point. In non-daemon mode, we don't let \"gc --auto\" failure\n>> cause the invoking command to fail, but in daemon mode we do. That\n>> should be a straightforward fix; patch coming in a moment.\n>\n> OK, that definitely sounds like a bug. I'm still confused how that could\n> happen, though, since from the caller's perspective they ignore git-gc's\n> exit code either way. I guess I'll see in your patch. :)\n\nAlas, I just misremembered. What I was remembering is that gc --auto\ndoes\n\n\tif (auto_gc && too_many_loose_objects())\n\t\twarning(_(\"There are too many unreachable loose objects; \"\n\t\t\t\"run 'git prune' to remove them.\"));\n\treturn 0;\n\nwhich means that too_many_loose_objects is not an error in undaemonized\nmode, while it is in daemonized mode. But we've already discussed that.\n\nThe calling command in the motivating example is Android's \"repo\" tool:\n\n bare_git.gc('--auto')\n\nfrom https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I\nthink it's reasonable that it expects a status code of 0 in the normal\ncase. So life is less simple than I hoped.\n\n[...]\n>> Can you point me to some discussion about building that rate-limiting?\n>> The commit message for v2.12.2~17^2 (gc: ignore old gc.log files,\n>> 2017-02-10) definitely doesn't describe that as its intent.\n>\n> I think that commit is a loosening of the rate-limiting (because we'd\n> refuse to progress for something that was actually time-based). But the\n> original stopping comes from this discussion, I think:\n>\n> https://public-inbox.org/git/xmqqlhijznpm.fsf@gitster.dls.corp.google.com/\n\nInteresting! It looks like that thread anticipated the problems we've\nseen here. Three years without having to have fixed it is a good run,\nI suppose.\n\nThe discussion of stopping there appears to be primarily about\nstopping in the error case, not rate-limiting in the success or\nwarning case.\n\nHere's a patch for the 'return -1' thing.\n\n-- >8 --\nSubject: gc: exit with status 128 on failure\n\nA value of -1 returned from cmd_gc gets propagated to exit(),\nresulting in an exit status of 255. Use die instead for a clearer\nerror message and a controlled exit.\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/gc.c | 36 ++++++++++++++++--------------------\n t/t6500-gc.sh | 2 +-\n 2 files changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..2bebc52bda 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -438,10 +438,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n-static int report_last_gc_error(void)\n+static void report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = 0;\n+\tssize_t ret;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n \n@@ -449,16 +449,17 @@ static int report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"Can't stat %s\"), gc_log_path);\n-\t\tgoto done;\n+\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t}\n \n \tif (st.st_mtime < gc_log_expire_time)\n \t\tgoto done;\n \n \tret = strbuf_read_file(&sb, gc_log_path, 0);\n+\tif (ret < 0)\n+\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n \tif (ret > 0)\n-\t\tret = error(_(\"The last gc run reported the following. \"\n+\t\tdie(_(\"The last gc run reported the following. \"\n \t\t\t \"Please correct the root cause\\n\"\n \t\t\t \"and remove %s.\\n\"\n \t\t\t \"Automatic cleanup will not be performed \"\n@@ -468,20 +469,18 @@ static int report_last_gc_error(void)\n \tstrbuf_release(&sb);\n done:\n \tfree(gc_log_path);\n-\treturn ret;\n }\n \n-static int gc_before_repack(void)\n+static void gc_before_repack(void)\n {\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n+\t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, reflog.argv[0]);\n+\t\tdie(FAILED_RUN, reflog.argv[0]);\n \n \tpack_refs = 0;\n \tprune_reflogs = 0;\n-\treturn 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n@@ -562,13 +561,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\tif (report_last_gc_error())\n-\t\t\t\treturn -1;\n+\t\t\treport_last_gc_error(); /* dies on error */\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\n-\t\t\tif (gc_before_repack())\n-\t\t\t\treturn -1;\n+\t\t\tgc_before_repack(); /* dies on failure */\n \t\t\tdelete_tempfile(&pidfile);\n \n \t\t\t/*\n@@ -608,12 +605,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tatexit(process_log_file_at_exit);\n \t}\n \n-\tif (gc_before_repack())\n-\t\treturn -1;\n+\tgc_before_repack();\n \n \tif (!repository_format_precious_objects) {\n \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n-\t\t\treturn error(FAILED_RUN, repack.argv[0]);\n+\t\t\tdie(FAILED_RUN, repack.argv[0]);\n \n \t\tif (prune_expire) {\n \t\t\targv_array_push(&prune, prune_expire);\n@@ -623,18 +619,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\t\targv_array_push(&prune,\n \t\t\t\t\t\t\"--exclude-promisor-objects\");\n \t\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\n-\t\t\t\treturn error(FAILED_RUN, prune.argv[0]);\n+\t\t\t\tdie(FAILED_RUN, prune.argv[0]);\n \t\t}\n \t}\n \n \tif (prune_worktrees_expire) {\n \t\targv_array_push(&prune_worktrees, prune_worktrees_expire);\n \t\tif (run_command_v_opt(prune_worktrees.argv, RUN_GIT_CMD))\n-\t\t\treturn error(FAILED_RUN, prune_worktrees.argv[0]);\n+\t\t\tdie(FAILED_RUN, prune_worktrees.argv[0]);\n \t}\n \n \tif (run_command_v_opt(rerere.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, rerere.argv[0]);\n+\t\tdie(FAILED_RUN, rerere.argv[0]);\n \n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 818435f04e..c474a94a9f 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -117,7 +117,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_config gc.autodetach true &&\n \techo fleem >.git/gc.log &&\n \ttest_must_fail git gc --auto 2>err &&\n-\ttest_i18ngrep \"^error:\" err &&\n+\ttest_i18ngrep \"^fatal:\" err &&\n \ttest_config gc.logexpiry 5.days &&\n \ttest-tool chmtime =-345600 .git/gc.log &&\n \ttest_must_fail git gc --auto &&\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352723","messageId":"CABPp-BFnMLAzsHO6ksA26sOMwSCy_-LvBfe=izxFxrFVteuTVA@mail.gmail.com","threadId":"48890","inReplyTo":"20180716225520.GC12482@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-16T23:06:42Z","receivedAt":"2018-07-16T23:06:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 16, 2018 at 3:55 PM, Jeff King wrote:\n> On Mon, Jul 16, 2018 at 03:07:34PM -0700, Elijah Newren wrote:\n>\n>> > If we were to delete those objects, wouldn't it be exactly the same as\n>> > running \"git prune\"? Or setting your gc.pruneExpire to \"now\" or even \"5\n>> > minutes\"? Or are you concerned with taking other objects along for the\n>> > ride that weren't part of old reflogs? I think that's a valid concern,\n>>\n>> Yes, I was worried about taking other objects along for the ride that\n>> weren't part of old reflogs.\n>>\n>> > but it's also an issue for objects which were previously referenced in\n>> > a reflog, but are part of another current operation.\n>>\n>> I'm not certain what you're referring to here.\n>\n> I mean that an ongoing operation could refer to a blob that just\n> recently became unreachable via reflog pruning. And then we would delete\n> it, leaving the repository corrupt.\n>\n> One of the protections we have against that is that if I ask to write\n> blob XYZ and we find that we already have the object, Git will freshen\n> the mtime of the loose object or pack that contains it, protecting it\n> from pruning. But with your suggestion, we'd delete it immediately,\n> regardless of the mtime of the containing pack.\n>\n> Another way to think of it is this: a reflog mentioning an object does\n> not mean it is the exclusive user of that object. So when we expire it,\n> that does not mean that it is OK to delete it immediately; there may be\n> other users.\n>\n>> > Also, what do you do if there weren't reflogs in the repo? Or the\n>> > reflogs were deleted (e.g., because branch deletion drops the associated\n>> > reflog entirely)?\n>>\n>> Yes, there are issues this rule won't help with, but in my experience\n>> it was a primary (if not sole) actual cause in practice. (I believe I\n>> even said elsewhere in this thread that I knew there were unreachable\n>> objects for other reasons and they might also become large in number).\n>> At $DAYJOB we've had multiple people including myself hit the \"too\n>> many unreachable loose objects\" nasty loop issue (some of us multiple\n>> different times), and as far as I can tell, most (perhaps even all) of\n>> them would have been avoided by just \"properly\" deleting garbage as\n>> per my object-age-is-reflog-age-if-not-otherwise-referenced rule.\n>\n> I agree with you that this is a frequent cause, and probably even the\n> primary one. But my concern is that your loosening increases the risk of\n> corruption for other cases.\n>\n>> > I assume by \"these objects\" you mean ones which used to be reachable\n>> > from a reflog, but that reflog entry just expired. I think you'd be\n>> > sacrificing some race-safety in that case.\n>>\n>> Is that inherently any more race unsafe than 'git prune\n>> --expire=2.weeks.ago'? I thought it'd be racy in the same ways, and\n>> thus a tradeoff folks are already accepting (at least implicitly) when\n>> running git-gc. Since these objects are actually 90 days old rather\n>> than a mere two weeks, it actually seemed more safe to me. But maybe\n>> I'm overlooking something with the pack->loose transition that makes\n>> it more racy?\n>\n> I think it's worse in terms of race-safety because you're losing one of\n> the signals that users of the objects can provide to git-prune to tell\n> it the object is useful: updating the mtime. So yes, you think of the\n> objects as \"90 days old\", but we don't know if there are other users.\n> Has somebody else been accessing them in the meantime?\n>\n> To be honest, I'm not sure how meaningful that distinction is in\n> practice. The current scheme is still racy, even if the windows are\n> shorter in some circumstances. But it seems like cruft packfiles are\n> a similar amount of work to your scheme, cover more cases, and are\n> slightly safer. And possibly even give us a long-term route to true\n> race-free pruning (via the \"garbage pack\" mark that Jonathan mentioned).\n>\n> Assuming you buy into the idea that objects in a cruft-pack are not\n> hurting anything aside from a little wasted storage for up to 2 weeks\n> (which it sounds like you're at least partially on board with ;) ).\n\nAh, I see now. Thanks (to you and Jonathan) for the thorough\nexplanations. I'm totally on board now.\n"},{"id":"352726","messageId":"20180716232603.GB13570@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180716225639.GK11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T23:26:03Z","receivedAt":"2018-07-16T23:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote:\n\n> The calling command in the motivating example is Android's \"repo\" tool:\n> \n> bare_git.gc('--auto')\n> \n> from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I\n> think it's reasonable that it expects a status code of 0 in the normal\n> case. So life is less simple than I hoped.\n\nIMHO it should ignore the return code, since that's what Git does\nitself. And at any rate, you'd miss errors from daemonized gc's (at\nleast until the _next_ one runs and propagates the error code).\n\n> Interesting! It looks like that thread anticipated the problems we've\n> seen here. Three years without having to have fixed it is a good run,\n> I suppose.\n> \n> The discussion of stopping there appears to be primarily about\n> stopping in the error case, not rate-limiting in the success or\n> warning case.\n\nI think the two are essentially the same. The point is that we cannot\nmake further progress by re-running the gc again, so we should not.\n\nAmusingly, the warning we're talking about is the exact reason that the\nlogging in that thread was added. 329e6e8794 (gc: save log from\ndaemonized gc --auto and print it next time, 2015-09-19) says:\n\n The latest in this set is, as the result of daemonizing, stderr is\n closed and all warnings are lost. This warning at the end of cmd_gc()\n is particularly important because it tells the user how to avoid \"gc\n --auto\" running repeatedly. Because stderr is closed, the user does\n not know, naturally they complain about 'gc --auto' wasting CPU.\n\n> -- >8 --\n> Subject: gc: exit with status 128 on failure\n> \n> A value of -1 returned from cmd_gc gets propagated to exit(),\n> resulting in an exit status of 255. Use die instead for a clearer\n> error message and a controlled exit.\n\nI agree it's better to not pass -1 to exit(). But I thought the original\nmotivation was the fact that we were returning non-zero in this case at\nall? (And I thought you and I both agreed with that motivation).\n\nSo is this meant to be a preparatory fix until somebody is interested in\nfixing that?\n\n> -static int gc_before_repack(void)\n> +static void gc_before_repack(void)\n> {\n> \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> -\t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n> +\t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n> \n> \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n> -\t\treturn error(FAILED_RUN, reflog.argv[0]);\n> +\t\tdie(FAILED_RUN, reflog.argv[0]);\n\nDscho is going to yell at you about replacing error returns with die(). ;)\n\nI wonder if it would be simpler to just reinterpret the \"-1\" into \"128\"\nin cmd_gc(). I thought we had talked about having run_builtin() do that\nat one point, but I don't think we ever did. I dunno. I'm fine with it\neither way.\n\n-Peff\n"},{"id":"352739","messageId":"20180717015339.GL11513@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716232603.GB13570@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T01:53:39Z","receivedAt":"2018-07-17T01:53:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote:\n\n>> The calling command in the motivating example is Android's \"repo\" tool:\n>> \n>> bare_git.gc('--auto')\n>> \n>> from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I\n>> think it's reasonable that it expects a status code of 0 in the normal\n>> case. So life is less simple than I hoped.\n>\n> IMHO it should ignore the return code, since that's what Git does\n> itself. And at any rate, you'd miss errors from daemonized gc's (at\n> least until the _next_ one runs and propagates the error code).\n\nThat suggests a possible improvement. If all callers should be\nignoring the exit code, can we make the exit code in daemonize mode\nunconditionally zero in this kind of case?\n\nThat doesn't really solve the problem:\n\n 1. \"gc --auto\" would produce noise *every time it runs* until gc.log\n is removed, for example via expiry\n\n 2. \"gc --auto\" would not do any garbage collection until gc.log is\n removed, for example by expiry\n\n 3. \"gc --auto\" would still not ratelimit itself, for example when\n there are a large number of loose unreachable objects that is not\n large enough to hit the loose object threshold.\n\nbut maybe it's better than the present state.\n\nTo solve (1) and (2), we could introduce a gc.warnings file that\nbehaves like gc.log except that it only gets printed once and then\nself-destructs, allowing gc --auto to proceed. To solve (3), we could\nintroduce a gc.lastrun file that is touched whenever \"gc --auto\" runs\nsuccessfully and make \"gc --auto\" a no-op for a while after each run.\n\n-- >8 --\nSubject: gc: do not return error for prior errors in daemonized mode\n\nSome build machines started failing to fetch updated source using\n\"repo sync\", with error\n\n error: The last gc run reported the following. Please correct the root cause\n and remove /build/.repo/projects/tools/git.git/gc.log.\n Automatic cleanup will not be performed until the file is removed.\n\n warning: There are too many unreachable loose objects; run 'git prune' to remove them.\n\nThe cause takes some time to describe.\n\nIn v2.0.0-rc0~145^2 (gc: config option for running --auto in\nbackground, 2014-02-08), \"git gc --auto\" learned to run in the\nbackground instead of blocking the invoking command. In this mode, it\nclosed stderr to avoid interleaving output with any subsequent\ncommands, causing warnings like the above to be swallowed; v2.6.3~24^2\n(gc: save log from daemonized gc --auto and print it next time,\n2015-09-19) addressed this by storing any diagnostic output in\n.git/gc.log and allowing the next \"git gc --auto\" run to print it.\n\nTo avoid wasteful repeated fruitless gcs, when gc.log is present, the\nsubsequent \"gc --auto\" would die after print its contents. Most git\ncommands, such as \"git fetch\", ignore the exit status from \"git gc\n--auto\" so all is well at this point: the user gets to see the error\nmessage, and the fetch succeeds, without a wasteful additional attempt\nat an automatic gc.\n\nExternal tools like repo[1], though, do care about the exit status\nfrom \"git gc --auto\". In non-daemonized mode, the exit status is\nstraightforward: if there is an error, it is nonzero, but after a\nwarning like the above, the status is zero. The daemonized mode, as a\nside effect of the other properties provided, offers a very strange\nexit code convention:\n\n - if no housekeeping was required, the exit status is 0\n\n - the first real run, after forking into the background, returns exit\n status 0 unconditionally. The parent process has no way to know\n whether gc will succeed.\n\n - if there is any diagnostic output in gc.log, subsequent runs return\n a nonzero exit status to indicate that gc was not triggered.\n\nThere's nothing for the calling program to act on on the basis of that\nerror. Use status 0 consistently instead, to indicate that we decided\nnot to run a gc (just like if no housekeeping was required). This\nway, repo and similar tools can get the benefit of the same behavior\nas tools like \"git fetch\" that ignore the exit status from gc --auto.\n\nOnce the period of time described by gc.pruneExpire elapses, the\nunreachable loose objects will be removed by \"git gc --auto\"\nautomatically.\n\n[1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/\n\nReported-by: Andrii Dehtiarov \nHelped-by: Jeff King \nSigned-off-by: Jonathan Nieder \n---\n Documentation/config.txt | 3 ++-\n builtin/gc.c | 16 +++++++++++-----\n t/t6500-gc.sh | 6 +++---\n 3 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1cc18a828c..5eaa4aaa7d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n \n gc.logExpiry::\n-\tIf the file gc.log exists, then `git gc --auto` won't run\n+\tIf the file gc.log exists, then `git gc --auto` will print\n+\tits content and exit with status zero instead of running\n \tunless that file is more than 'gc.logExpiry' old. Default is\n \t\"1.day\". See `gc.pruneExpire` for more ways to specify its\n \tvalue.\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2bebc52bda..484ab21b8c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -438,7 +438,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n-static void report_last_gc_error(void)\n+static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tssize_t ret;\n@@ -449,7 +449,7 @@ static void report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\treturn error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t}\n \n \tif (st.st_mtime < gc_log_expire_time)\n@@ -457,9 +457,9 @@ static void report_last_gc_error(void)\n \n \tret = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (ret < 0)\n-\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\treturn error_errno(_(\"cannot read '%s'\"), gc_log_path);\n \tif (ret > 0)\n-\t\tdie(_(\"The last gc run reported the following. \"\n+\t\treturn error(_(\"The last gc run reported the following. \"\n \t\t\t \"Please correct the root cause\\n\"\n \t\t\t \"and remove %s.\\n\"\n \t\t\t \"Automatic cleanup will not be performed \"\n@@ -469,6 +469,7 @@ static void report_last_gc_error(void)\n \tstrbuf_release(&sb);\n done:\n \tfree(gc_log_path);\n+\treturn 0;\n }\n \n static void gc_before_repack(void)\n@@ -561,7 +562,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\treport_last_gc_error(); /* dies on error */\n+\t\t\tif (report_last_gc_error())\n+\t\t\t\t/*\n+\t\t\t\t * A previous gc failed. We've reported the\n+\t\t\t\t * error, so there's nothing left to do.\n+\t\t\t\t */\n+\t\t\t\treturn 0;\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex c474a94a9f..3e62df616c 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_config gc.autopacklimit 1 &&\n \ttest_config gc.autodetach true &&\n \techo fleem >.git/gc.log &&\n-\ttest_must_fail git gc --auto 2>err &&\n-\ttest_i18ngrep \"^fatal:\" err &&\n+\tgit gc --auto 2>err &&\n+\ttest_i18ngrep \"^error:\" err &&\n \ttest_config gc.logexpiry 5.days &&\n \ttest-tool chmtime =-345600 .git/gc.log &&\n-\ttest_must_fail git gc --auto &&\n+\tgit gc --auto &&\n \ttest_config gc.logexpiry 2.days &&\n \trun_and_wait_for_auto_gc &&\n \tls .git/objects/pack/pack-*.pack >packs &&\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352740","messageId":"20180717065151.GA177907@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180716172717.237373-1-jonathantanmy@google.com","subject":"[PATCH v2 0/3] gc --auto: do not return error for prior errors in daemonized mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T06:51:51Z","receivedAt":"2018-07-17T06:52:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> In a087cc9819 (\"git-gc --auto: protect ourselves from accumulated\n> cruft\", 2007-09-17), the user was warned if there were too many\n> unreachable loose objects. This made sense at the time, because gc\n> couldn't prune them safely. But subsequently, git prune learned the\n> ability to not prune recently created loose objects, making pruning able\n> to be done more safely, and gc was made to automatically prune old\n> unreachable loose objects in 25ee9731c1 (\"gc: call \"prune --expire\n> 2.weeks.ago\" by default\", 2008-03-12).\n>\n> This makes the warning unactionable by the user, as any loose objects\n> left are not deleted yet because of safety, and \"git prune\" is not a\n> command that the user is recommended to run directly anyway.\n\nI agree that given the better alternatives we have now, \"git prune\" is\nnot such a great option these days. E.g. should it say\n\n\tstruct strbuf now = STRBUF_INIT;\n\tdate_stamp(&now);\n\t...\n\n\t\"run 'git gc --prune=%s' to remove them\", now.buf);\n\n?\n\n> This was noticed when a daemonized gc run wrote this warning to the log\n> file, and returned 0; but a subsequent run merely read the log file, saw\n> that it is non-empty and returned -1 (which is inconsistent in that such\n> a run should return 0, as it did the first time).\n\nHere's a series to address that motivating case. Thanks for the\ncareful analysis and to Peff for the patient explanations.\n\nSincerely,\nJonathan Nieder (3):\n gc: improve handling of errors reading gc.log\n gc: exit with status 128 on failure\n gc: do not return error for prior errors in daemonized mode\n\n Documentation/config.txt | 3 ++-\n builtin/gc.c | 53 ++++++++++++++++++++++++++--------------\n t/t6500-gc.sh | 6 ++---\n 3 files changed, 40 insertions(+), 22 deletions(-)\n"},{"id":"352741","messageId":"20180717065321.GB177907@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180717065151.GA177907@aiede.svl.corp.google.com","subject":"[PATCH 1/3] gc: improve handling of errors reading gc.log","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T06:53:21Z","receivedAt":"2018-07-17T06:53:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A collection of minor error handling fixes:\n\n- use an error message in lower case, following the usual style\n- quote filenames in error messages to make them easier to read and to\n decrease translation load by matching other 'stat' error messages\n- check for and report errors from 'read', too\n- avoid being confused by a gc.log larger than INT_MAX bytes\n\nNoticed by code inspection.\n\nSigned-off-by: Jonathan Nieder \n---\nSplit out from\nhttps://public-inbox.org/git/20180716225639.GK11513@aiede.svl.corp.google.com/.\n\n builtin/gc.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ccfb1ceaeb..d69fc4c0b0 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -442,6 +442,7 @@ static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n+\tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n \n@@ -449,15 +450,17 @@ static int report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"Can't stat %s\"), gc_log_path);\n+\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t\tgoto done;\n \t}\n \n \tif (st.st_mtime < gc_log_expire_time)\n \t\tgoto done;\n \n-\tret = strbuf_read_file(&sb, gc_log_path, 0);\n-\tif (ret > 0)\n+\tlen = strbuf_read_file(&sb, gc_log_path, 0);\n+\tif (len < 0)\n+\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\telse if (len > 0)\n \t\tret = error(_(\"The last gc run reported the following. \"\n \t\t\t \"Please correct the root cause\\n\"\n \t\t\t \"and remove %s.\\n\"\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352742","messageId":"20180717065416.GC177907@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180717065151.GA177907@aiede.svl.corp.google.com","subject":"[PATCH 2/3] gc: exit with status 128 on failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T06:54:16Z","receivedAt":"2018-07-17T06:54:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A value of -1 returned from cmd_gc gets propagated to exit(),\nresulting in an exit status of 255. Use die instead for a clearer\nerror message and a controlled exit.\n\nSigned-off-by: Jonathan Nieder \n---\nAs in\nhttps://public-inbox.org/git/20180716225639.GK11513@aiede.svl.corp.google.com/.\nThe only change is splitting out patch 1/3.\n\n builtin/gc.c | 35 ++++++++++++++---------------------\n t/t6500-gc.sh | 2 +-\n 2 files changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex d69fc4c0b0..95c8afd07b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -438,10 +438,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n-static int report_last_gc_error(void)\n+static void report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = 0;\n \tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n@@ -450,8 +449,7 @@ static int report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n-\t\tgoto done;\n+\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t}\n \n \tif (st.st_mtime < gc_log_expire_time)\n@@ -459,9 +457,9 @@ static int report_last_gc_error(void)\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n \telse if (len > 0)\n-\t\tret = error(_(\"The last gc run reported the following. \"\n+\t\tdie(_(\"The last gc run reported the following. \"\n \t\t\t \"Please correct the root cause\\n\"\n \t\t\t \"and remove %s.\\n\"\n \t\t\t \"Automatic cleanup will not be performed \"\n@@ -471,20 +469,18 @@ static int report_last_gc_error(void)\n \tstrbuf_release(&sb);\n done:\n \tfree(gc_log_path);\n-\treturn ret;\n }\n \n-static int gc_before_repack(void)\n+static void gc_before_repack(void)\n {\n \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n+\t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n \n \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, reflog.argv[0]);\n+\t\tdie(FAILED_RUN, reflog.argv[0]);\n \n \tpack_refs = 0;\n \tprune_reflogs = 0;\n-\treturn 0;\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n@@ -565,13 +561,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\tif (report_last_gc_error())\n-\t\t\t\treturn -1;\n+\t\t\treport_last_gc_error(); /* dies on error */\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\n-\t\t\tif (gc_before_repack())\n-\t\t\t\treturn -1;\n+\t\t\tgc_before_repack(); /* dies on failure */\n \t\t\tdelete_tempfile(&pidfile);\n \n \t\t\t/*\n@@ -611,12 +605,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tatexit(process_log_file_at_exit);\n \t}\n \n-\tif (gc_before_repack())\n-\t\treturn -1;\n+\tgc_before_repack();\n \n \tif (!repository_format_precious_objects) {\n \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n-\t\t\treturn error(FAILED_RUN, repack.argv[0]);\n+\t\t\tdie(FAILED_RUN, repack.argv[0]);\n \n \t\tif (prune_expire) {\n \t\t\targv_array_push(&prune, prune_expire);\n@@ -626,18 +619,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\t\targv_array_push(&prune,\n \t\t\t\t\t\t\"--exclude-promisor-objects\");\n \t\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\n-\t\t\t\treturn error(FAILED_RUN, prune.argv[0]);\n+\t\t\t\tdie(FAILED_RUN, prune.argv[0]);\n \t\t}\n \t}\n \n \tif (prune_worktrees_expire) {\n \t\targv_array_push(&prune_worktrees, prune_worktrees_expire);\n \t\tif (run_command_v_opt(prune_worktrees.argv, RUN_GIT_CMD))\n-\t\t\treturn error(FAILED_RUN, prune_worktrees.argv[0]);\n+\t\t\tdie(FAILED_RUN, prune_worktrees.argv[0]);\n \t}\n \n \tif (run_command_v_opt(rerere.argv, RUN_GIT_CMD))\n-\t\treturn error(FAILED_RUN, rerere.argv[0]);\n+\t\tdie(FAILED_RUN, rerere.argv[0]);\n \n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 818435f04e..c474a94a9f 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -117,7 +117,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_config gc.autodetach true &&\n \techo fleem >.git/gc.log &&\n \ttest_must_fail git gc --auto 2>err &&\n-\ttest_i18ngrep \"^error:\" err &&\n+\ttest_i18ngrep \"^fatal:\" err &&\n \ttest_config gc.logexpiry 5.days &&\n \ttest-tool chmtime =-345600 .git/gc.log &&\n \ttest_must_fail git gc --auto &&\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352743","messageId":"20180717065740.GD177907@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180717065151.GA177907@aiede.svl.corp.google.com","subject":"[PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T06:57:40Z","receivedAt":"2018-07-17T06:57:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Some build machines started consistently failing to fetch updated\nsource using \"repo sync\", with error\n\n error: The last gc run reported the following. Please correct the root cause\n and remove /build/.repo/projects/tools/git.git/gc.log.\n Automatic cleanup will not be performed until the file is removed.\n\n warning: There are too many unreachable loose objects; run 'git prune' to remove them.\n\nThe cause takes some time to describe.\n\nIn v2.0.0-rc0~145^2 (gc: config option for running --auto in\nbackground, 2014-02-08), \"git gc --auto\" learned to run in the\nbackground instead of blocking the invoking command. In this mode, it\nclosed stderr to avoid interleaving output with any subsequent\ncommands, causing warnings like the above to be swallowed; v2.6.3~24^2\n(gc: save log from daemonized gc --auto and print it next time,\n2015-09-19) addressed that by storing any diagnostic output in\n.git/gc.log and allowing the next \"git gc --auto\" run to print it.\n\nTo avoid wasteful repeated fruitless gcs, when gc.log is present, the\nsubsequent \"gc --auto\" would die after printing its contents. Most\ngit commands, such as \"git fetch\", ignore the exit status from \"git gc\n--auto\" so all is well at this point: the user gets to see the error\nmessage, and the fetch succeeds, without a wasteful additional attempt\nat an automatic gc.\n\nExternal tools like repo[1], though, do care about the exit status\nfrom \"git gc --auto\". In non-daemonized mode, the exit status is\nstraightforward: if there is an error, it is nonzero, but after a\nwarning like the above, the status is zero. The daemonized mode, as a\nside effect of the other properties provided, offers a very strange\nexit code convention:\n\n - if no housekeeping was required, the exit status is 0\n\n - the first real run, after forking into the background, returns exit\n status 0 unconditionally. The parent process has no way to know\n whether gc will succeed.\n\n - if there is any diagnostic output in gc.log, subsequent runs return\n a nonzero exit status to indicate that gc was not triggered.\n\nThere's nothing for the calling program to act on on the basis of that\nerror. Use status 0 consistently instead, to indicate that we decided\nnot to run a gc (just like if no housekeeping was required). This\nway, repo and similar tools can get the benefit of the same behavior\nas tools like \"git fetch\" that ignore the exit status from gc --auto.\n\nOnce the period of time described by gc.pruneExpire elapses, the\nunreachable loose objects will be removed by \"git gc --auto\"\nautomatically.\n\n[1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/\n\nReported-by: Andrii Dehtiarov \nHelped-by: Jeff King \nSigned-off-by: Jonathan Nieder \n---\nChanges from v1:\n- minor commit message tweaks\n- using warning() instead of error()\n- avoiding leaking gc_log_path in report_last_gc_error\n- only changing the exit status when reporting a prior error, not a\n new one\n\nThoughts of all kinds welcome, as always.\n\nSincerely,\nJonathan\n\n Documentation/config.txt | 3 ++-\n builtin/gc.c | 33 +++++++++++++++++++++++++++------\n t/t6500-gc.sh | 6 +++---\n 3 files changed, 32 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1cc18a828c..5eaa4aaa7d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n \n gc.logExpiry::\n-\tIf the file gc.log exists, then `git gc --auto` won't run\n+\tIf the file gc.log exists, then `git gc --auto` will print\n+\tits content and exit with status zero instead of running\n \tunless that file is more than 'gc.logExpiry' old. Default is\n \t\"1.day\". See `gc.pruneExpire` for more ways to specify its\n \tvalue.\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 95c8afd07b..ce8a663a01 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -438,9 +438,15 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \treturn NULL;\n }\n \n-static void report_last_gc_error(void)\n+/*\n+ * Returns 0 if there was no previous error and gc can proceed, 1 if\n+ * gc should not proceed due to an error in the last run. Prints a\n+ * message and returns -1 if an error occured while reading gc.log\n+ */\n+static int report_last_gc_error(void)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tint ret = 0;\n \tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n@@ -449,7 +455,8 @@ static void report_last_gc_error(void)\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tgoto done;\n \t}\n \n \tif (st.st_mtime < gc_log_expire_time)\n@@ -457,18 +464,26 @@ static void report_last_gc_error(void)\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n-\telse if (len > 0)\n-\t\tdie(_(\"The last gc run reported the following. \"\n+\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\telse if (len > 0) {\n+\t\t/*\n+\t\t * A previous gc failed. Report the error, and don't\n+\t\t * bother with an automatic gc run since it is likely\n+\t\t * to fail in the same way.\n+\t\t */\n+\t\twarning(_(\"The last gc run reported the following. \"\n \t\t\t \"Please correct the root cause\\n\"\n \t\t\t \"and remove %s.\\n\"\n \t\t\t \"Automatic cleanup will not be performed \"\n \t\t\t \"until the file is removed.\\n\\n\"\n \t\t\t \"%s\"),\n \t\t\t gc_log_path, sb.buf);\n+\t\tret = 1;\n+\t}\n \tstrbuf_release(&sb);\n done:\n \tfree(gc_log_path);\n+\treturn ret;\n }\n \n static void gc_before_repack(void)\n@@ -561,7 +576,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\treport_last_gc_error(); /* dies on error */\n+\t\t\tint ret = report_last_gc_error();\n+\t\t\tif (ret < 0)\n+\t\t\t\t/* an I/O error occured, already reported */\n+\t\t\t\texit(128);\n+\t\t\tif (ret == 1)\n+\t\t\t\t/* Last gc --auto failed. Skip this one. */\n+\t\t\t\treturn 0;\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex c474a94a9f..a222efdbe1 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_config gc.autopacklimit 1 &&\n \ttest_config gc.autodetach true &&\n \techo fleem >.git/gc.log &&\n-\ttest_must_fail git gc --auto 2>err &&\n-\ttest_i18ngrep \"^fatal:\" err &&\n+\tgit gc --auto 2>err &&\n+\ttest_i18ngrep \"^warning:\" err &&\n \ttest_config gc.logexpiry 5.days &&\n \ttest-tool chmtime =-345600 .git/gc.log &&\n-\ttest_must_fail git gc --auto &&\n+\tgit gc --auto &&\n \ttest_config gc.logexpiry 2.days &&\n \trun_and_wait_for_auto_gc &&\n \tls .git/objects/pack/pack-*.pack >packs &&\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352752","messageId":"87d0vmck55.fsf@evledraar.gmail.com","threadId":"48890","inReplyTo":"20180717015339.GL11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-17T08:59:50Z","receivedAt":"2018-07-17T08:59:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 17 2018, Jonathan Nieder wrote:\n\n> Hi,\n>\n> Jeff King wrote:\n>> On Mon, Jul 16, 2018 at 03:56:39PM -0700, Jonathan Nieder wrote:\n>\n>>> The calling command in the motivating example is Android's \"repo\" tool:\n>>>\n>>> bare_git.gc('--auto')\n>>>\n>>> from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I\n>>> think it's reasonable that it expects a status code of 0 in the normal\n>>> case. So life is less simple than I hoped.\n>>\n>> IMHO it should ignore the return code, since that's what Git does\n>> itself. And at any rate, you'd miss errors from daemonized gc's (at\n>> least until the _next_ one runs and propagates the error code).\n\nI've read this whole thread, and as Peff noted I've barked up this\nparticular tree before[1] without coming up with a solution myself.\n\nSo please don't take the following as critique of any way of moving\nforward, I'm just trying to poke holes in what you're doing to make sure\nwe don't have regressions to the currently (sucky) logic.\n\n> That suggests a possible improvement. If all callers should be\n> ignoring the exit code, can we make the exit code in daemonize mode\n> unconditionally zero in this kind of case?\n\nThat doesn't make sense to me. Just because git itself is happily\nignoring the exit code I don't think that should mean there shouldn't be\na meaningful exit code. Why don't you just ignore the exit code in the\nrepo tool as well?\n\nNow e.g. automation I have to see if git-gc ---auto is having issues\ncan't just be 'git gc --auto || echo fail @ {hostname}' across a fleet\nof servers, but will need to start caring if stderr was emitted to.\n\nI don't care if we e.g. have a 'git gc --auto --exit-code' similar to\nwhat git-diff does, but it doesn't make sense to me that we *know* we\ncan't background the gc due to a previous error and then always return\n0. Having to parse STDERR to see if it *really* failed is un-unixy,\nlet's use exit codes. That's what they're for.\n\n> That doesn't really solve the problem:\n>\n> 1. \"gc --auto\" would produce noise *every time it runs* until gc.log\n> is removed, for example via expiry\n>\n> 2. \"gc --auto\" would not do any garbage collection until gc.log is\n> removed, for example by expiry\n>\n> 3. \"gc --auto\" would still not ratelimit itself, for example when\n> there are a large number of loose unreachable objects that is not\n> large enough to hit the loose object threshold.\n>\n> but maybe it's better than the present state.\n>\n> To solve (1) and (2), we could introduce a gc.warnings file that\n> behaves like gc.log except that it only gets printed once and then\n> self-destructs, allowing gc --auto to proceed.\n\nI think you're conflating two things here in a way that (if I'm reading\nthis correctly) produces a pretty bad regression for users.\n\n a) If we have something in the gc.log we keep yelling at users until we\n reach the gc.logExpiry, this was the subject of my thread back in\n January. This sucks, and should be fixed somehow.\n\n Maybe we should only emit the warning once, e.g. creating a\n .git/gc.log.wasemitted marker and as long as its ctime is later than\n the mtime on .git/gc.log we don't emit it again).\n\n But that also creates the UX problem that it's easy to miss this\n message in the middle of some huge \"fetch\" output. Perhaps we should\n just start emitting this as part of \"git status\" or something (or\n solve the root cause, as Peff notes...).\n\n b) We *also* use this presence of a gc.log as a marker for \"we failed\n too recently, let's not try again until after a day\".\n\nThe semantics of b) are very useful, and just because they're tied up\nwith a) right now, let's not throw out b) just because we're trying to\nsolve a).\n\nWe have dev machines with limited I/O & CPU/memory that occasionally\nbreak the gc.auto limit, I really don't want those churning a background\n\"git gc\" on every fetch/commit etc. until we're finally below the\ngc.auto limit (possibly days later), which would be a side-effect of\nprinting the .git/gc.log once and then removing it.\n\n> To solve (3), we could\n> introduce a gc.lastrun file that is touched whenever \"gc --auto\" runs\n> successfully and make \"gc --auto\" a no-op for a while after each run.\n\nThis would work around my concern with b) above in most cases by\nintroducing a purely time-based rate limit, but I'm uneasy about this\nchange in git-gc semantics.\n\nRight now one thing we do right is we always try to look at the actual\nstate of the repo with too_many_packs() and too_many_loose_objects().\n\nWe don't assume the state of your repo hasn't drastically changed\nrecently. That means that we do the right thing and gc when the repo\nneeds it, not just because we GC'd recently enough.\n\nWith these proposed semantics we'd skip a needed GC (potentially for\ndays, depending on the default) just because we recently ran one.\n\nIn many environments, such as on busy servers, we could have tens of\nthousands of packs by this point, since this facility would (presumably)\nbypass both gc.autoPackLimit and gc.auto in favor of only running gc at\na maximum of every N minutes, similarly in a local checkout I could have\na crapload of loose objects because I ran a big rebase or a\nfilter-branch on one of my branches.\n\n1. https://public-inbox.org/git/87inc89j38.fsf@evledraar.gmail.com/\n\n> -- >8 --\n> Subject: gc: do not return error for prior errors in daemonized mode\n>\n> Some build machines started failing to fetch updated source using\n> \"repo sync\", with error\n>\n> error: The last gc run reported the following. Please correct the root cause\n> and remove /build/.repo/projects/tools/git.git/gc.log.\n> Automatic cleanup will not be performed until the file is removed.\n>\n> warning: There are too many unreachable loose objects; run 'git prune' to remove them.\n>\n> The cause takes some time to describe.\n>\n> In v2.0.0-rc0~145^2 (gc: config option for running --auto in\n> background, 2014-02-08), \"git gc --auto\" learned to run in the\n> background instead of blocking the invoking command. In this mode, it\n> closed stderr to avoid interleaving output with any subsequent\n> commands, causing warnings like the above to be swallowed; v2.6.3~24^2\n> (gc: save log from daemonized gc --auto and print it next time,\n> 2015-09-19) addressed this by storing any diagnostic output in\n> .git/gc.log and allowing the next \"git gc --auto\" run to print it.\n>\n> To avoid wasteful repeated fruitless gcs, when gc.log is present, the\n> subsequent \"gc --auto\" would die after print its contents. Most git\n> commands, such as \"git fetch\", ignore the exit status from \"git gc\n> --auto\" so all is well at this point: the user gets to see the error\n> message, and the fetch succeeds, without a wasteful additional attempt\n> at an automatic gc.\n>\n> External tools like repo[1], though, do care about the exit status\n> from \"git gc --auto\". In non-daemonized mode, the exit status is\n> straightforward: if there is an error, it is nonzero, but after a\n> warning like the above, the status is zero. The daemonized mode, as a\n> side effect of the other properties provided, offers a very strange\n> exit code convention:\n>\n> - if no housekeeping was required, the exit status is 0\n>\n> - the first real run, after forking into the background, returns exit\n> status 0 unconditionally. The parent process has no way to know\n> whether gc will succeed.\n>\n> - if there is any diagnostic output in gc.log, subsequent runs return\n> a nonzero exit status to indicate that gc was not triggered.\n>\n> There's nothing for the calling program to act on on the basis of that\n> error. Use status 0 consistently instead, to indicate that we decided\n> not to run a gc (just like if no housekeeping was required). This\n> way, repo and similar tools can get the benefit of the same behavior\n> as tools like \"git fetch\" that ignore the exit status from gc --auto.\n>\n> Once the period of time described by gc.pruneExpire elapses, the\n> unreachable loose objects will be removed by \"git gc --auto\"\n> automatically.\n>\n> [1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/\n>\n> Reported-by: Andrii Dehtiarov \n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Nieder \n> ---\n> Documentation/config.txt | 3 ++-\n> builtin/gc.c | 16 +++++++++++-----\n> t/t6500-gc.sh | 6 +++---\n> 3 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1cc18a828c..5eaa4aaa7d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n> gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n>\n> gc.logExpiry::\n> -\tIf the file gc.log exists, then `git gc --auto` won't run\n> +\tIf the file gc.log exists, then `git gc --auto` will print\n> +\tits content and exit with status zero instead of running\n> \tunless that file is more than 'gc.logExpiry' old. Default is\n> \t\"1.day\". See `gc.pruneExpire` for more ways to specify its\n> \tvalue.\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 2bebc52bda..484ab21b8c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -438,7 +438,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> \treturn NULL;\n> }\n>\n> -static void report_last_gc_error(void)\n> +static int report_last_gc_error(void)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tssize_t ret;\n> @@ -449,7 +449,7 @@ static void report_last_gc_error(void)\n> \t\tif (errno == ENOENT)\n> \t\t\tgoto done;\n>\n> -\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> +\t\treturn error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> \t}\n>\n> \tif (st.st_mtime < gc_log_expire_time)\n> @@ -457,9 +457,9 @@ static void report_last_gc_error(void)\n>\n> \tret = strbuf_read_file(&sb, gc_log_path, 0);\n> \tif (ret < 0)\n> -\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n> +\t\treturn error_errno(_(\"cannot read '%s'\"), gc_log_path);\n> \tif (ret > 0)\n> -\t\tdie(_(\"The last gc run reported the following. \"\n> +\t\treturn error(_(\"The last gc run reported the following. \"\n> \t\t\t \"Please correct the root cause\\n\"\n> \t\t\t \"and remove %s.\\n\"\n> \t\t\t \"Automatic cleanup will not be performed \"\n> @@ -469,6 +469,7 @@ static void report_last_gc_error(void)\n> \tstrbuf_release(&sb);\n> done:\n> \tfree(gc_log_path);\n> +\treturn 0;\n> }\n>\n> static void gc_before_repack(void)\n> @@ -561,7 +562,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n> \t\t}\n> \t\tif (detach_auto) {\n> -\t\t\treport_last_gc_error(); /* dies on error */\n> +\t\t\tif (report_last_gc_error())\n> +\t\t\t\t/*\n> +\t\t\t\t * A previous gc failed. We've reported the\n> +\t\t\t\t * error, so there's nothing left to do.\n> +\t\t\t\t */\n> +\t\t\t\treturn 0;\n>\n> \t\t\tif (lock_repo_for_gc(force, &pid))\n> \t\t\t\treturn 0;\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index c474a94a9f..3e62df616c 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n> \ttest_config gc.autopacklimit 1 &&\n> \ttest_config gc.autodetach true &&\n> \techo fleem >.git/gc.log &&\n> -\ttest_must_fail git gc --auto 2>err &&\n> -\ttest_i18ngrep \"^fatal:\" err &&\n> +\tgit gc --auto 2>err &&\n> +\ttest_i18ngrep \"^error:\" err &&\n> \ttest_config gc.logexpiry 5.days &&\n> \ttest-tool chmtime =-345600 .git/gc.log &&\n> -\ttest_must_fail git gc --auto &&\n> +\tgit gc --auto &&\n> \ttest_config gc.logexpiry 2.days &&\n> \trun_and_wait_for_auto_gc &&\n> \tls .git/objects/pack/pack-*.pack >packs &&\n"},{"id":"352787","messageId":"20180717140336.GA229988@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"87d0vmck55.fsf@evledraar.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-17T14:03:36Z","receivedAt":"2018-07-17T14:03:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ævar,\n\nÆvar Arnfjörð Bjarmason wrote:\n> On Tue, Jul 17 2018, Jonathan Nieder wrote:\n\n>> That suggests a possible improvement. If all callers should be\n>> ignoring the exit code, can we make the exit code in daemonize mode\n>> unconditionally zero in this kind of case?\n>\n> That doesn't make sense to me. Just because git itself is happily\n> ignoring the exit code I don't think that should mean there shouldn't be\n> a meaningful exit code. Why don't you just ignore the exit code in the\n> repo tool as well?\n\nI don't maintain the repo tool. I am just trying to improve git to\nmake some users' lives better.\n\nrepo worked fine for years, until gc's daemonized mode broke it. I\ndon't think it makes sense for us to declare that it has been holding\ngit gc wrong for all that time before then.\n\n> Now e.g. automation I have to see if git-gc ---auto is having issues\n> can't just be 'git gc --auto || echo fail @ {hostname}' across a fleet\n> of servers, but will need to start caring if stderr was emitted to.\n\nThanks for bringing this up. The thing is, the current exit code is\nnot useful for that purpose. It doesn't report a failure until the\n*next* `git gc --auto` run, when it is too late to be useful.\n\nSee the commit message on the proposed patch\nhttps://public-inbox.org/git/20180717065740.GD177907@aiede.svl.corp.google.com/\nfor more on that subject.\n\n> I don't care if we e.g. have a 'git gc --auto --exit-code' similar to\n> what git-diff does, but it doesn't make sense to me that we *know* we\n> can't background the gc due to a previous error and then always return\n> 0. Having to parse STDERR to see if it *really* failed is un-unixy,\n> let's use exit codes. That's what they're for.\n\nDo you know of anyone trying to use the exit code from gc --auto in\nthis way?\n\nIt sounds to me like for what you're proposing, a separate\n\n\tgit gc --auto --last-run-failed\n\ncommand that a tool could use to check the outcome from the previous\ngc --auto run without triggering a new one would be best.\n\n[...]\n> I think you're conflating two things here in a way that (if I'm reading\n> this correctly) produces a pretty bad regression for users.\n\nThe patch doesn't touch the \"ratelimiting\" behavior at all, so I'm not\nsure what I'm doing to regress users.\n\n[...]\n>> To solve (3), we could\n>> introduce a gc.lastrun file that is touched whenever \"gc --auto\" runs\n>> successfully and make \"gc --auto\" a no-op for a while after each run.\n>\n> This would work around my concern with b) above in most cases by\n> introducing a purely time-based rate limit, but I'm uneasy about this\n> change in git-gc semantics.\n[..]\n> With these proposed semantics we'd skip a needed GC (potentially for\n> days, depending on the default) just because we recently ran one.\n>\n> In many environments, such as on busy servers, we could have tens of\n> thousands of packs by this point, since this facility would (presumably)\n> bypass both gc.autoPackLimit and gc.auto in favor of only running gc at\n> a maximum of every N minutes, similarly in a local checkout I could have\n> a crapload of loose objects because I ran a big rebase or a\n> filter-branch on one of my branches.\n\nI think we all agree that getting rid of the hack that 'explodes'\nunreachable objects into loose objects is the best way forward, long\nterm.\n\nEven in that future, some ratelimiting may be useful, though. I also\nsuspect that we're never going to achieve a perfect set of defaults\nthat works well for both humans and servers, though we can try.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"352788","messageId":"878t69dgvx.fsf@evledraar.gmail.com","threadId":"48890","inReplyTo":"20180717140336.GA229988@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-17T15:24:50Z","receivedAt":"2018-07-17T15:24:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 17 2018, Jonathan Nieder wrote:\n\n> Hi Ævar,\n>\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Tue, Jul 17 2018, Jonathan Nieder wrote:\n>\n>>> That suggests a possible improvement. If all callers should be\n>>> ignoring the exit code, can we make the exit code in daemonize mode\n>>> unconditionally zero in this kind of case?\n>>\n>> That doesn't make sense to me. Just because git itself is happily\n>> ignoring the exit code I don't think that should mean there shouldn't be\n>> a meaningful exit code. Why don't you just ignore the exit code in the\n>> repo tool as well?\n>\n> I don't maintain the repo tool. I am just trying to improve git to\n> make some users' lives better.\n>\n> repo worked fine for years, until gc's daemonized mode broke it. I\n> don't think it makes sense for us to declare that it has been holding\n> git gc wrong for all that time before then.\n\nBefore the sequence of commits noted at the bottom of my\nhttps://public-inbox.org/git/87inc89j38.fsf@evledraar.gmail.com/ plus\na831c06a2b (\"gc: ignore old gc.log files\", 2017-02-10) we wouldn't do\nthat, that's true.\n\nWe'd just happily run GC again pointlessly even though it wasn't going\nto accomplish anything, in cases where you really did have ~>6700 loose\nobjects that weren't going to be pruned.\n\nI don't think it makes sense to revert those patches and go back to the\nmuch more naïve (and user waiting there twiddling his thumbs while it\nruns) method, but I also don't think making no distinction between the\nfollowing states:\n\n 1. gc --auto has nothing to do\n 2. gc --auto has something to do, will fork and try to do it\n 3. gc --auto has something to do, but notices that gc has been failing\n before and can't do anything now.\n\nI think #3 should exit with non-zero.\n\nYes, before this whole backgrounding etc. we wouldn't have exited with\nnon-zero either, we'd just print a thing to STDERR.\n\nBut with backgrounding we implicitly introduced a new state, which is we\nwon't do *any* maintenance at all, including consolidating packfiles\n(very important for servers), so I think it makes sense to exit with\nnon-zero since gc can't run at all.\n\n>> Now e.g. automation I have to see if git-gc ---auto is having issues\n>> can't just be 'git gc --auto || echo fail @ {hostname}' across a fleet\n>> of servers, but will need to start caring if stderr was emitted to.\n>\n> Thanks for bringing this up. The thing is, the current exit code is\n> not useful for that purpose. It doesn't report a failure until the\n> *next* `git gc --auto` run, when it is too late to be useful.\n>\n> See the commit message on the proposed patch\n> https://public-inbox.org/git/20180717065740.GD177907@aiede.svl.corp.google.com/\n> for more on that subject.\n\nRight. I know. What I mean is now I can (and do) use it to run 'git gc\n--auto' across our server fleet and see whether I have any of #3, or\nwhether it's all #1 or #2. If there's nothing to do in #1 that's fine,\nand it just so happens that I'll run gc due to #2 that's also fine, but\nI'd like to see if gc really is stuck.\n\nThis of course relies on them having other users / scripts doing normal\ngit commands which would trigger previous 'git gc --auto' runs.\n\n>> I don't care if we e.g. have a 'git gc --auto --exit-code' similar to\n>> what git-diff does, but it doesn't make sense to me that we *know* we\n>> can't background the gc due to a previous error and then always return\n>> 0. Having to parse STDERR to see if it *really* failed is un-unixy,\n>> let's use exit codes. That's what they're for.\n>\n> Do you know of anyone trying to use the exit code from gc --auto in\n> this way?\n>\n> It sounds to me like for what you're proposing, a separate\n>\n> \tgit gc --auto --last-run-failed\n>\n> command that a tool could use to check the outcome from the previous\n> gc --auto run without triggering a new one would be best.\n\nYeah this is admittedly a bit of a poweruser thing I'm doing, so I don't\nmind if it's hidden behind something like that in principle, but it\nseems wrong to exit with zero in this (#3) case:\n\n $ git gc --auto; echo $?\n Auto packing the repository in background for optimum performance.\n See \"git help gc\" for manual housekeeping.\n error: The last gc run reported the following. Please correct the root cause\n and remove .git/gc.log.\n Automatic cleanup will not be performed until the file is removed.\n\n warning: There are too many unreachable loose objects; run 'git prune' to remove them.\n\n 255\n\nAs a previous (good) patch in this series notes that shouldn't be 255,\nlet's fix that, but I don't see how emitting an \"error\" and \"warning\"\nsaying we're broken and can't gc at all here in conjunction with exiting\nwith zero makes sense.\n\n> [...]\n>> I think you're conflating two things here in a way that (if I'm reading\n>> this correctly) produces a pretty bad regression for users.\n>\n> The patch doesn't touch the \"ratelimiting\" behavior at all, so I'm not\n> sure what I'm doing to regress users.\n\nSorry about being unclear again, this is not a comment on this patch,\nbut your paragraph beginning with \"To solve[...]\", i.e. \"this patch\ndoesn't do X, but in the future maybe we'll...\". I'm pointing out\npotential caveats with that, I realize you have not posted an\nimplementation of that yet.\n\n> [...]\n>>> To solve (3), we could\n>>> introduce a gc.lastrun file that is touched whenever \"gc --auto\" runs\n>>> successfully and make \"gc --auto\" a no-op for a while after each run.\n>>\n>> This would work around my concern with b) above in most cases by\n>> introducing a purely time-based rate limit, but I'm uneasy about this\n>> change in git-gc semantics.\n> [..]\n>> With these proposed semantics we'd skip a needed GC (potentially for\n>> days, depending on the default) just because we recently ran one.\n>>\n>> In many environments, such as on busy servers, we could have tens of\n>> thousands of packs by this point, since this facility would (presumably)\n>> bypass both gc.autoPackLimit and gc.auto in favor of only running gc at\n>> a maximum of every N minutes, similarly in a local checkout I could have\n>> a crapload of loose objects because I ran a big rebase or a\n>> filter-branch on one of my branches.\n>\n> I think we all agree that getting rid of the hack that 'explodes'\n> unreachable objects into loose objects is the best way forward, long\n> term.\n>\n> Even in that future, some ratelimiting may be useful, though. I also\n> suspect that we're never going to achieve a perfect set of defaults\n> that works well for both humans and servers, though we can try.\n\nHaving read this whole thing over again, and with some time to reflect\non it, I think the best way forward for now, in lieu of the bigger\nsolution of consolidating loose objects into a pack, is to split up this\nspecial case of warning due to too many loose objects at the end, and\nany other errors we may print during GC.\n\nAs noted above if our policy for gc.auto is legitimately exceeded, we\ndeal with that badly by stopping all gc, includin gc that would just run\ndue to too_many_packs().\n\nInstead we should note that we had an error due to too many loose\nobjects last time, but still try to run if the too_many_packs()\ncondition is satisfied.\n\nNow we're throwing the baby out with the bathwater and not consolidating\npacks (for a default period of 2 weeks!) just because some operation\nproduced a lot of loose objects, and not writing bitmaps, commit graph\netc.\n\nIt would also be nice to expose via an exit code \"do we need to gc?\" and\n\"is gc failing here?\" separately from the (currently working) \"run gc or\nreport last error + code\" mode we have with \"gc --auto\".\n"},{"id":"352789","messageId":"CACsJy8C=+0n_5Uz3ub1t-1fJtu4HJQ3_ro1R+aEmpUz+1Ge6RA@mail.gmail.com","threadId":"48890","inReplyTo":"20180717015339.GL11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-17T15:59:57Z","receivedAt":"2018-07-17T16:00:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 17, 2018 at 3:53 AM Jonathan Nieder wrote:\n> Subject: gc: do not return error for prior errors in daemonized mode\n>\n> Some build machines started failing to fetch updated source using\n> \"repo sync\", with error\n>\n> error: The last gc run reported the following. Please correct the root cause\n> and remove /build/.repo/projects/tools/git.git/gc.log.\n> Automatic cleanup will not be performed until the file is removed.\n>\n> warning: There are too many unreachable loose objects; run 'git prune' to remove them.\n>\n> The cause takes some time to describe.\n>\n> In v2.0.0-rc0~145^2 (gc: config option for running --auto in\n> background, 2014-02-08), \"git gc --auto\" learned to run in the\n> background instead of blocking the invoking command. In this mode, it\n> closed stderr to avoid interleaving output with any subsequent\n> commands, causing warnings like the above to be swallowed; v2.6.3~24^2\n> (gc: save log from daemonized gc --auto and print it next time,\n> 2015-09-19) addressed this by storing any diagnostic output in\n> .git/gc.log and allowing the next \"git gc --auto\" run to print it.\n>\n> To avoid wasteful repeated fruitless gcs, when gc.log is present, the\n> subsequent \"gc --auto\" would die after print its contents. Most git\n> commands, such as \"git fetch\", ignore the exit status from \"git gc\n> --auto\" so all is well at this point: the user gets to see the error\n> message, and the fetch succeeds, without a wasteful additional attempt\n> at an automatic gc.\n>\n> External tools like repo[1], though, do care about the exit status\n> from \"git gc --auto\". In non-daemonized mode, the exit status is\n> straightforward: if there is an error, it is nonzero, but after a\n> warning like the above, the status is zero. The daemonized mode, as a\n> side effect of the other properties provided, offers a very strange\n> exit code convention:\n>\n> - if no housekeeping was required, the exit status is 0\n>\n> - the first real run, after forking into the background, returns exit\n> status 0 unconditionally. The parent process has no way to know\n> whether gc will succeed.\n>\n> - if there is any diagnostic output in gc.log, subsequent runs return\n> a nonzero exit status to indicate that gc was not triggered.\n>\n> There's nothing for the calling program to act on on the basis of that\n> error. Use status 0 consistently instead, to indicate that we decided\n> not to run a gc (just like if no housekeeping was required). This\n> way, repo and similar tools can get the benefit of the same behavior\n> as tools like \"git fetch\" that ignore the exit status from gc --auto.\n\nThis background gc thing is pretty much designed for interactive use.\nWhen it's scripted, you probably should avoid it. Then you can fork a\nnew process by yourself and have much better control if you still want\n\"background\" gc. So an alternative here is to turn on or off\ngc.autodetach based on interactiveness (i.e. having tty). We can add a\nnew (and default) value \"auto\" to gc.autodetach for this purpose.\n\nIn automated scripts, it will always run in non-damonized mode. You\nwill get non-zero exit code when real errors occur. You don't worry\nabout hanging processes. Rate limit is also thrown out in this mode if\nI'm not mistaken, but it could be added back and more tailored for\nserver needs.\n\n> Once the period of time described by gc.pruneExpire elapses, the\n> unreachable loose objects will be removed by \"git gc --auto\"\n> automatically.\n>\n> [1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/\n>\n> Reported-by: Andrii Dehtiarov \n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Nieder \n> ---\n> Documentation/config.txt | 3 ++-\n> builtin/gc.c | 16 +++++++++++-----\n> t/t6500-gc.sh | 6 +++---\n> 3 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1cc18a828c..5eaa4aaa7d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n> gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n>\n> gc.logExpiry::\n> - If the file gc.log exists, then `git gc --auto` won't run\n> + If the file gc.log exists, then `git gc --auto` will print\n> + its content and exit with status zero instead of running\n> unless that file is more than 'gc.logExpiry' old. Default is\n> \"1.day\". See `gc.pruneExpire` for more ways to specify its\n> value.\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 2bebc52bda..484ab21b8c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -438,7 +438,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> return NULL;\n> }\n>\n> -static void report_last_gc_error(void)\n> +static int report_last_gc_error(void)\n> {\n> struct strbuf sb = STRBUF_INIT;\n> ssize_t ret;\n> @@ -449,7 +449,7 @@ static void report_last_gc_error(void)\n> if (errno == ENOENT)\n> goto done;\n>\n> - die_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> + return error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> }\n>\n> if (st.st_mtime < gc_log_expire_time)\n> @@ -457,9 +457,9 @@ static void report_last_gc_error(void)\n>\n> ret = strbuf_read_file(&sb, gc_log_path, 0);\n> if (ret < 0)\n> - die_errno(_(\"cannot read '%s'\"), gc_log_path);\n> + return error_errno(_(\"cannot read '%s'\"), gc_log_path);\n> if (ret > 0)\n> - die(_(\"The last gc run reported the following. \"\n> + return error(_(\"The last gc run reported the following. \"\n> \"Please correct the root cause\\n\"\n> \"and remove %s.\\n\"\n> \"Automatic cleanup will not be performed \"\n> @@ -469,6 +469,7 @@ static void report_last_gc_error(void)\n> strbuf_release(&sb);\n> done:\n> free(gc_log_path);\n> + return 0;\n> }\n>\n> static void gc_before_repack(void)\n> @@ -561,7 +562,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> fprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n> }\n> if (detach_auto) {\n> - report_last_gc_error(); /* dies on error */\n> + if (report_last_gc_error())\n> + /*\n> + * A previous gc failed. We've reported the\n> + * error, so there's nothing left to do.\n> + */\n> + return 0;\n>\n> if (lock_repo_for_gc(force, &pid))\n> return 0;\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index c474a94a9f..3e62df616c 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n> test_config gc.autopacklimit 1 &&\n> test_config gc.autodetach true &&\n> echo fleem >.git/gc.log &&\n> - test_must_fail git gc --auto 2>err &&\n> - test_i18ngrep \"^fatal:\" err &&\n> + git gc --auto 2>err &&\n> + test_i18ngrep \"^error:\" err &&\n> test_config gc.logexpiry 5.days &&\n> test-tool chmtime =-345600 .git/gc.log &&\n> - test_must_fail git gc --auto &&\n> + git gc --auto &&\n> test_config gc.logexpiry 2.days &&\n> run_and_wait_for_auto_gc &&\n> ls .git/objects/pack/pack-*.pack >packs &&\n> --\n> 2.18.0.233.g985f88cf7e\n>\n\n\n-- \nDuy\n"},{"id":"352811","messageId":"xmqq36whpwd7.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180717015339.GL11513@aiede.svl.corp.google.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T18:09:40Z","receivedAt":"2018-07-17T18:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> That doesn't really solve the problem:\n>\n> 1. \"gc --auto\" would produce noise *every time it runs* until gc.log\n> is removed, for example via expiry\n>\n> 2. \"gc --auto\" would not do any garbage collection until gc.log is\n> removed, for example by expiry\n>\n> 3. \"gc --auto\" would still not ratelimit itself, for example when\n> there are a large number of loose unreachable objects that is not\n> large enough to hit the loose object threshold.\n>\n> but maybe it's better than the present state.\n>\n> To solve (1) and (2), we could introduce a gc.warnings file that\n> behaves like gc.log except that it only gets printed once and then\n> self-destructs, allowing gc --auto to proceed. \n\nThat makes it not rate-limit, no?\n\n> To solve (3), we could\n> introduce a gc.lastrun file that is touched whenever \"gc --auto\" runs\n> successfully and make \"gc --auto\" a no-op for a while after each run.\n\nDoes absolute time-interval make sense here? Some repositories may\nbe busily churning new objects and for them 5-minute interval may be\ntoo infrequent, while other repositories may be relatively slow and\nonce a day may be sufficient for them. I wonder if we can somehow\nauto-tune this.\n\n> To avoid wasteful repeated fruitless gcs, when gc.log is present, the\n> subsequent \"gc --auto\" would die after print its contents. Most git\n\ns/print/&ing/\n\n> commands, such as \"git fetch\", ignore the exit status from \"git gc\n> --auto\" so all is well at this point: the user gets to see the error\n> message, and the fetch succeeds, without a wasteful additional attempt\n> at an automatic gc.\n\nThe above, by saying \"Most git commands\", leaves readers want to\nknow \"then what are minority git commands that do not correctly do\nso?\" If you are not going to answer that question, it probably is\nbetter not to even say \"Most\".\n\n> External tools like repo[1], though, do care about the exit status\n> from \"git gc --auto\". In non-daemonized mode, the exit status is\n> straightforward: if there is an error, it is nonzero, but after a\n> warning like the above, the status is zero. The daemonized mode, as a\n> side effect of the other properties provided, offers a very strange\n> exit code convention:\n>\n> - if no housekeeping was required, the exit status is 0\n\nOK.\n\n> - the first real run, after forking into the background, returns exit\n> status 0 unconditionally. The parent process has no way to know\n> whether gc will succeed.\n\nUnderstandable; allowing the parent to exit before we know the\noutcome of the gc is the whole point of backgrounding.\n\n> - if there is any diagnostic output in gc.log, subsequent runs return\n> a nonzero exit status to indicate that gc was not triggered.\n\nThis is unfortunate.\n\n> There's nothing for the calling program to act on on the basis of that\n> error. Use status 0 consistently instead, to indicate that we decided\n> not to run a gc (just like if no housekeeping was required). \n\ns/.$/ in the last case./ And I fully agree with the reasoning.\n\n> This\n> way, repo and similar tools can get the benefit of the same behavior\n> as tools like \"git fetch\" that ignore the exit status from gc --auto.\n>\n> Once the period of time described by gc.pruneExpire elapses, the\n> unreachable loose objects will be removed by \"git gc --auto\"\n> automatically.\n>\n> [1] https://gerrit-review.googlesource.com/c/git-repo/+/10598/\n>\n> Reported-by: Andrii Dehtiarov \n> Helped-by: Jeff King \n> Signed-off-by: Jonathan Nieder \n> ---\n> Documentation/config.txt | 3 ++-\n> builtin/gc.c | 16 +++++++++++-----\n> t/t6500-gc.sh | 6 +++---\n> 3 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1cc18a828c..5eaa4aaa7d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n> gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n> \n> gc.logExpiry::\n> -\tIf the file gc.log exists, then `git gc --auto` won't run\n> +\tIf the file gc.log exists, then `git gc --auto` will print\n> +\tits content and exit with status zero instead of running\n> \tunless that file is more than 'gc.logExpiry' old. Default is\n> \t\"1.day\". See `gc.pruneExpire` for more ways to specify its\n> \tvalue.\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 2bebc52bda..484ab21b8c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -438,7 +438,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> \treturn NULL;\n> }\n> \n> -static void report_last_gc_error(void)\n> +static int report_last_gc_error(void)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tssize_t ret;\n> @@ -449,7 +449,7 @@ static void report_last_gc_error(void)\n> \t\tif (errno == ENOENT)\n> \t\t\tgoto done;\n> \n> -\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> +\t\treturn error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> \t}\n> \n> \tif (st.st_mtime < gc_log_expire_time)\n> @@ -457,9 +457,9 @@ static void report_last_gc_error(void)\n> \n> \tret = strbuf_read_file(&sb, gc_log_path, 0);\n> \tif (ret < 0)\n> -\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n> +\t\treturn error_errno(_(\"cannot read '%s'\"), gc_log_path);\n> \tif (ret > 0)\n> -\t\tdie(_(\"The last gc run reported the following. \"\n> +\t\treturn error(_(\"The last gc run reported the following. \"\n> \t\t\t \"Please correct the root cause\\n\"\n> \t\t\t \"and remove %s.\\n\"\n> \t\t\t \"Automatic cleanup will not be performed \"\n> @@ -469,6 +469,7 @@ static void report_last_gc_error(void)\n> \tstrbuf_release(&sb);\n> done:\n> \tfree(gc_log_path);\n> +\treturn 0;\n> }\n> \n> static void gc_before_repack(void)\n> @@ -561,7 +562,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n> \t\t}\n> \t\tif (detach_auto) {\n> -\t\t\treport_last_gc_error(); /* dies on error */\n> +\t\t\tif (report_last_gc_error())\n> +\t\t\t\t/*\n> +\t\t\t\t * A previous gc failed. We've reported the\n> +\t\t\t\t * error, so there's nothing left to do.\n> +\t\t\t\t */\n> +\t\t\t\treturn 0;\n> \n> \t\t\tif (lock_repo_for_gc(force, &pid))\n> \t\t\t\treturn 0;\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index c474a94a9f..3e62df616c 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n> \ttest_config gc.autopacklimit 1 &&\n> \ttest_config gc.autodetach true &&\n> \techo fleem >.git/gc.log &&\n> -\ttest_must_fail git gc --auto 2>err &&\n> -\ttest_i18ngrep \"^fatal:\" err &&\n> +\tgit gc --auto 2>err &&\n> +\ttest_i18ngrep \"^error:\" err &&\n> \ttest_config gc.logexpiry 5.days &&\n> \ttest-tool chmtime =-345600 .git/gc.log &&\n> -\ttest_must_fail git gc --auto &&\n> +\tgit gc --auto &&\n> \ttest_config gc.logexpiry 2.days &&\n> \trun_and_wait_for_auto_gc &&\n> \tls .git/objects/pack/pack-*.pack >packs &&\n"},{"id":"352812","messageId":"xmqqy3e9ohcf.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180717065321.GB177907@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/3] gc: improve handling of errors reading gc.log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T18:19:28Z","receivedAt":"2018-07-17T18:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> - avoid being confused by a gc.log larger than INT_MAX bytes\n\n;-)\n\n>\n> Noticed by code inspection.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n\nLooks good.\n"},{"id":"352813","messageId":"xmqqtvoxoh7i.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180717065416.GC177907@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/3] gc: exit with status 128 on failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-17T18:22:25Z","receivedAt":"2018-07-17T18:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> A value of -1 returned from cmd_gc gets propagated to exit(),\n> resulting in an exit status of 255. Use die instead for a clearer\n> error message and a controlled exit.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> As in\n> https://public-inbox.org/git/20180716225639.GK11513@aiede.svl.corp.google.com/.\n> The only change is splitting out patch 1/3.\n>\n> builtin/gc.c | 35 ++++++++++++++---------------------\n> t/t6500-gc.sh | 2 +-\n> 2 files changed, 15 insertions(+), 22 deletions(-)\n\nI double checked \"return' in cmd_gc() and this patch catches them\nall. Good clean-up.\n\nThanks.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index d69fc4c0b0..95c8afd07b 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -438,10 +438,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n> \treturn NULL;\n> }\n> \n> -static int report_last_gc_error(void)\n> +static void report_last_gc_error(void)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> -\tint ret = 0;\n> \tssize_t len;\n> \tstruct stat st;\n> \tchar *gc_log_path = git_pathdup(\"gc.log\");\n> @@ -450,8 +449,7 @@ static int report_last_gc_error(void)\n> \t\tif (errno == ENOENT)\n> \t\t\tgoto done;\n> \n> -\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> -\t\tgoto done;\n> +\t\tdie_errno(_(\"cannot stat '%s'\"), gc_log_path);\n> \t}\n> \n> \tif (st.st_mtime < gc_log_expire_time)\n> @@ -459,9 +457,9 @@ static int report_last_gc_error(void)\n> \n> \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n> \tif (len < 0)\n> -\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n> +\t\tdie_errno(_(\"cannot read '%s'\"), gc_log_path);\n> \telse if (len > 0)\n> -\t\tret = error(_(\"The last gc run reported the following. \"\n> +\t\tdie(_(\"The last gc run reported the following. \"\n> \t\t\t \"Please correct the root cause\\n\"\n> \t\t\t \"and remove %s.\\n\"\n> \t\t\t \"Automatic cleanup will not be performed \"\n> @@ -471,20 +469,18 @@ static int report_last_gc_error(void)\n> \tstrbuf_release(&sb);\n> done:\n> \tfree(gc_log_path);\n> -\treturn ret;\n> }\n> \n> -static int gc_before_repack(void)\n> +static void gc_before_repack(void)\n> {\n> \tif (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))\n> -\t\treturn error(FAILED_RUN, pack_refs_cmd.argv[0]);\n> +\t\tdie(FAILED_RUN, pack_refs_cmd.argv[0]);\n> \n> \tif (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))\n> -\t\treturn error(FAILED_RUN, reflog.argv[0]);\n> +\t\tdie(FAILED_RUN, reflog.argv[0]);\n> \n> \tpack_refs = 0;\n> \tprune_reflogs = 0;\n> -\treturn 0;\n> }\n> \n> int cmd_gc(int argc, const char **argv, const char *prefix)\n> @@ -565,13 +561,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n> \t\t}\n> \t\tif (detach_auto) {\n> -\t\t\tif (report_last_gc_error())\n> -\t\t\t\treturn -1;\n> +\t\t\treport_last_gc_error(); /* dies on error */\n> \n> \t\t\tif (lock_repo_for_gc(force, &pid))\n> \t\t\t\treturn 0;\n> -\t\t\tif (gc_before_repack())\n> -\t\t\t\treturn -1;\n> +\t\t\tgc_before_repack(); /* dies on failure */\n> \t\t\tdelete_tempfile(&pidfile);\n> \n> \t\t\t/*\n> @@ -611,12 +605,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\tatexit(process_log_file_at_exit);\n> \t}\n> \n> -\tif (gc_before_repack())\n> -\t\treturn -1;\n> +\tgc_before_repack();\n> \n> \tif (!repository_format_precious_objects) {\n> \t\tif (run_command_v_opt(repack.argv, RUN_GIT_CMD))\n> -\t\t\treturn error(FAILED_RUN, repack.argv[0]);\n> +\t\t\tdie(FAILED_RUN, repack.argv[0]);\n> \n> \t\tif (prune_expire) {\n> \t\t\targv_array_push(&prune, prune_expire);\n> @@ -626,18 +619,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\t\t\targv_array_push(&prune,\n> \t\t\t\t\t\t\"--exclude-promisor-objects\");\n> \t\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\n> -\t\t\t\treturn error(FAILED_RUN, prune.argv[0]);\n> +\t\t\t\tdie(FAILED_RUN, prune.argv[0]);\n> \t\t}\n> \t}\n> \n> \tif (prune_worktrees_expire) {\n> \t\targv_array_push(&prune_worktrees, prune_worktrees_expire);\n> \t\tif (run_command_v_opt(prune_worktrees.argv, RUN_GIT_CMD))\n> -\t\t\treturn error(FAILED_RUN, prune_worktrees.argv[0]);\n> +\t\t\tdie(FAILED_RUN, prune_worktrees.argv[0]);\n> \t}\n> \n> \tif (run_command_v_opt(rerere.argv, RUN_GIT_CMD))\n> -\t\treturn error(FAILED_RUN, rerere.argv[0]);\n> +\t\tdie(FAILED_RUN, rerere.argv[0]);\n> \n> \treport_garbage = report_pack_garbage;\n> \treprepare_packed_git(the_repository);\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 818435f04e..c474a94a9f 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -117,7 +117,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n> \ttest_config gc.autodetach true &&\n> \techo fleem >.git/gc.log &&\n> \ttest_must_fail git gc --auto 2>err &&\n> -\ttest_i18ngrep \"^error:\" err &&\n> +\ttest_i18ngrep \"^fatal:\" err &&\n> \ttest_config gc.logexpiry 5.days &&\n> \ttest-tool chmtime =-345600 .git/gc.log &&\n> \ttest_must_fail git gc --auto &&\n"},{"id":"352825","messageId":"20180717195817.GB26218@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180717065321.GB177907@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/3] gc: improve handling of errors reading gc.log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T19:58:17Z","receivedAt":"2018-07-17T19:58:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 11:53:21PM -0700, Jonathan Nieder wrote:\n\n> A collection of minor error handling fixes:\n> \n> - use an error message in lower case, following the usual style\n> - quote filenames in error messages to make them easier to read and to\n> decrease translation load by matching other 'stat' error messages\n> - check for and report errors from 'read', too\n> - avoid being confused by a gc.log larger than INT_MAX bytes\n> \n> Noticed by code inspection.\n> \n> Signed-off-by: Jonathan Nieder \n\nThanks, this all looks obviously good.\n\n-Peff\n"},{"id":"352826","messageId":"20180717195946.GC26218@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180717065416.GC177907@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/3] gc: exit with status 128 on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T19:59:47Z","receivedAt":"2018-07-17T19:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 11:54:16PM -0700, Jonathan Nieder wrote:\n\n> A value of -1 returned from cmd_gc gets propagated to exit(),\n> resulting in an exit status of 255. Use die instead for a clearer\n> error message and a controlled exit.\n\nThis feels a little funny because we know we're going to turn some of\nthese back in the next patch. That said, I'm OK with it, since this\nversion is already written.\n\n-Peff\n"},{"id":"352827","messageId":"20180717201348.GD26218@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180717065740.GD177907@aiede.svl.corp.google.com","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T20:13:49Z","receivedAt":"2018-07-17T20:13:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 11:57:40PM -0700, Jonathan Nieder wrote:\n\n> External tools like repo[1], though, do care about the exit status\n> from \"git gc --auto\". In non-daemonized mode, the exit status is\n> straightforward: if there is an error, it is nonzero, but after a\n> warning like the above, the status is zero. The daemonized mode, as a\n> side effect of the other properties provided, offers a very strange\n> exit code convention:\n> \n> - if no housekeeping was required, the exit status is 0\n> \n> - the first real run, after forking into the background, returns exit\n> status 0 unconditionally. The parent process has no way to know\n> whether gc will succeed.\n> \n> - if there is any diagnostic output in gc.log, subsequent runs return\n> a nonzero exit status to indicate that gc was not triggered.\n> \n> There's nothing for the calling program to act on on the basis of that\n> error. Use status 0 consistently instead, to indicate that we decided\n> not to run a gc (just like if no housekeeping was required). This\n> way, repo and similar tools can get the benefit of the same behavior\n> as tools like \"git fetch\" that ignore the exit status from gc --auto.\n\nI think this is a good change.\n\nIn theory it might be useful to propagate the original exit code (_not_\n\"did we see a warning or an error\", but the true original exit code. But\nas you note, it's not deterministic anyway (we'd miss that exit code on\nthe first run, or even any simultaneous runs that exit early due to lock\ncontention). So it's clear that callers can't really do anything robust\nbased on the exit code of a daemonized \"gc --auto\".\n\nI still think that \"repo\" should probably stop respecting the exit code.\nBut that's no excuse for Git not to have a sensible exit code in the\nfirst place.\n\nThe patch itself looks good overall. A few comments (none of which I\nthink even requires a fix):\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1cc18a828c..5eaa4aaa7d 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1649,7 +1649,8 @@ will be repacked. After this the number of packs should go below\n> gc.autoPackLimit and gc.bigPackThreshold should be respected again.\n> \n> gc.logExpiry::\n> -\tIf the file gc.log exists, then `git gc --auto` won't run\n> +\tIf the file gc.log exists, then `git gc --auto` will print\n> +\tits content and exit with status zero instead of running\n> \tunless that file is more than 'gc.logExpiry' old. Default is\n> \t\"1.day\". See `gc.pruneExpire` for more ways to specify its\n> \tvalue.\n\nYeah, this is definitely worth documenting. I was surprised there's no\ndiscussion of daemonization at all in git-gc(1). I don't think adding it\nis a blocker for this series, though.\n\n> static void gc_before_repack(void)\n> @@ -561,7 +576,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n> \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n> \t\t}\n> \t\tif (detach_auto) {\n> -\t\t\treport_last_gc_error(); /* dies on error */\n> +\t\t\tint ret = report_last_gc_error();\n> +\t\t\tif (ret < 0)\n> +\t\t\t\t/* an I/O error occured, already reported */\n> +\t\t\t\texit(128);\n\nWe have a few exit(128)'s sprinkled throughout the code-base, and I\nalways wonder if they will one day go stale if we change the code that\ndie() uses. But it probably doesn't matter, and anyway I don't think\nthere is a better way to do this currently.\n\nI would have written \"return 128\" since the other case arm also returns,\nbut I really cannot think of a reason to prefer one over the other.\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index c474a94a9f..a222efdbe1 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -116,11 +116,11 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n> \ttest_config gc.autopacklimit 1 &&\n> \ttest_config gc.autodetach true &&\n> \techo fleem >.git/gc.log &&\n> -\ttest_must_fail git gc --auto 2>err &&\n> -\ttest_i18ngrep \"^fatal:\" err &&\n> +\tgit gc --auto 2>err &&\n> +\ttest_i18ngrep \"^warning:\" err &&\n> \ttest_config gc.logexpiry 5.days &&\n> \ttest-tool chmtime =-345600 .git/gc.log &&\n> -\ttest_must_fail git gc --auto &&\n> +\tgit gc --auto &&\n\nNice. At first I thought this was changing an existing test to cover the\nnew case (which I usually frown on), but it is just that your patch is\nintentionally changing the case covered here. So this is the right thing\nto do.\n\n-Peff\n"},{"id":"352828","messageId":"20180717202700.GE26218@sigill.intra.peff.net","threadId":"48890","inReplyTo":"87d0vmck55.fsf@evledraar.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-17T20:27:00Z","receivedAt":"2018-07-17T20:27:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> That doesn't make sense to me. Just because git itself is happily\n> ignoring the exit code I don't think that should mean there shouldn't be\n> a meaningful exit code. Why don't you just ignore the exit code in the\n> repo tool as well?\n> \n> Now e.g. automation I have to see if git-gc ---auto is having issues\n> can't just be 'git gc --auto || echo fail @ {hostname}' across a fleet\n> of servers, but will need to start caring if stderr was emitted to.\n\nIf you're daemonizing gc, though, there are a number of cases where the\nexit code is not propagated. If you really care about the outcome,\nyou're probably better off either:\n\n - doing synchronous gc's, which will still return a meaningful code\n after Jonathan's patches\n\n - inspecting the log yourself. I know that comes close to the un-unixy\n stderr thing. But in this case, the presence of a non-empty log is\n literally the on-disk bit for \"the previous run failed\". And doing\n so catches all of the daemonized cases, even the \"first one\" that\n you'd miss by paying attention to the immediate exit code.\n\n This will treat the zero-exit-code \"warning\" case as an error. I'm\n not against propagating the true original error code, if somebody\n wants to work on it. But I think Jonathan's patches are a strict\n improvement over the current situation, and a patch to propagate\n could come on top.\n\n> I think you're conflating two things here in a way that (if I'm reading\n> this correctly) produces a pretty bad regression for users.\n> \n> a) If we have something in the gc.log we keep yelling at users until we\n> reach the gc.logExpiry, this was the subject of my thread back in\n> January. This sucks, and should be fixed somehow.\n> \n> Maybe we should only emit the warning once, e.g. creating a\n> .git/gc.log.wasemitted marker and as long as its ctime is later than\n> the mtime on .git/gc.log we don't emit it again).\n> \n> But that also creates the UX problem that it's easy to miss this\n> message in the middle of some huge \"fetch\" output. Perhaps we should\n> just start emitting this as part of \"git status\" or something (or\n> solve the root cause, as Peff notes...).\n\nI kind of like that \"git status\" suggestion. Of course many users run\n\"git status\" more often than \"git commit\", so it may actually spam them\nmore!\n\n> b) We *also* use this presence of a gc.log as a marker for \"we failed\n> too recently, let's not try again until after a day\".\n> \n> The semantics of b) are very useful, and just because they're tied up\n> with a) right now, let's not throw out b) just because we're trying to\n> solve a).\n\nYeah. In general my concern was the handling of (b), which I think this\nlast crop of patches is fine with. As far as showing the message\nrepeatedly or not, I don't have a strong opinion (it could even be\nconfigurable, once it's split from the \"marker\" behavior).\n\n> Right now one thing we do right is we always try to look at the actual\n> state of the repo with too_many_packs() and too_many_loose_objects().\n> \n> We don't assume the state of your repo hasn't drastically changed\n> recently. That means that we do the right thing and gc when the repo\n> needs it, not just because we GC'd recently enough.\n> \n> With these proposed semantics we'd skip a needed GC (potentially for\n> days, depending on the default) just because we recently ran one.\n\nYeah, I agree that deferring repeated gc's based on time is going to run\ninto pathological corner cases. OTOH, what we've found at GitHub is that\n\"gc --auto\" is quite insufficient for scheduling maintenance anyway\n(e.g., if a machine gets pushes to 100 separate repositories in quick\nsuccession, you probably want to queue and throttle any associated gc).\n\nBut there are probably more mid-size sites that are big enough to have\ncorner cases, but not so big that \"gc --auto\" is hopeless. ;)\n\n-Peff\n"},{"id":"352863","messageId":"874lgwd6yd.fsf@evledraar.gmail.com","threadId":"48890","inReplyTo":"20180717202700.GE26218@sigill.intra.peff.net","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-18T13:11:38Z","receivedAt":"2018-07-18T13:11:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 17 2018, Jeff King wrote:\n\n> On Tue, Jul 17, 2018 at 10:59:50AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> That doesn't make sense to me. Just because git itself is happily\n>> ignoring the exit code I don't think that should mean there shouldn't be\n>> a meaningful exit code. Why don't you just ignore the exit code in the\n>> repo tool as well?\n>>\n>> Now e.g. automation I have to see if git-gc ---auto is having issues\n>> can't just be 'git gc --auto || echo fail @ {hostname}' across a fleet\n>> of servers, but will need to start caring if stderr was emitted to.\n>\n> If you're daemonizing gc, though, there are a number of cases where the\n> exit code is not propagated. If you really care about the outcome,\n> you're probably better off either:\n\nIn theory a lot of the other stuff can fail, but in practice I've only\nseen it get stuck due to running out of disk space (easily detected in\nother ways), and because of having too many loose objects (e.g. due to,\nbut not limited to\nhttps://public-inbox.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/).\n\n> - doing synchronous gc's, which will still return a meaningful code\n> after Jonathan's patches\n\n(Reply to this and \"we've found at GitHub...\" later in your mail)\n\nI'm targeting clients (dev machines, laptops, staging boxes) which have\nclones of repos, some put in place by automation, some manually cloned\nby users in ~.\n\nSo it's much easier to rely on shipping a /etc/gitconfig than setting\ngc.auto=0 and having some system-wide job that goes and hunts for local\ngit repos to manually gc.\n\nIt's also a big advantage that it's driven by user activity, because\nit's an implicit guard of only_do_this_if_the_user_is_active_here()\nbefore \"git-gc\" on a fleet of machines, which when some of those get\ntheir disk space from shared NFS resources cuts down on overall I/O.\n\n> - inspecting the log yourself. I know that comes close to the un-unixy\n> stderr thing. But in this case, the presence of a non-empty log is\n> literally the on-disk bit for \"the previous run failed\". And doing\n> so catches all of the daemonized cases, even the \"first one\" that\n> you'd miss by paying attention to the immediate exit code.\n>\n> This will treat the zero-exit-code \"warning\" case as an error. I'm\n> not against propagating the true original error code, if somebody\n> wants to work on it. But I think Jonathan's patches are a strict\n> improvement over the current situation, and a patch to propagate\n> could come on top.\n\nYeah, I can check gc.log, if others are of a different opinion about\nthis #3 case at the end of the day I don't mind if it returns 0. It just\ndoesn't make any conceptual sense to me.\n\n>> I think you're conflating two things here in a way that (if I'm reading\n>> this correctly) produces a pretty bad regression for users.\n>>\n>> a) If we have something in the gc.log we keep yelling at users until we\n>> reach the gc.logExpiry, this was the subject of my thread back in\n>> January. This sucks, and should be fixed somehow.\n>>\n>> Maybe we should only emit the warning once, e.g. creating a\n>> .git/gc.log.wasemitted marker and as long as its ctime is later than\n>> the mtime on .git/gc.log we don't emit it again).\n>>\n>> But that also creates the UX problem that it's easy to miss this\n>> message in the middle of some huge \"fetch\" output. Perhaps we should\n>> just start emitting this as part of \"git status\" or something (or\n>> solve the root cause, as Peff notes...).\n>\n> I kind of like that \"git status\" suggestion. Of course many users run\n> \"git status\" more often than \"git commit\", so it may actually spam them\n> more!\n\nMaybe piggy-packing on the advice facility ...\n\n>> b) We *also* use this presence of a gc.log as a marker for \"we failed\n>> too recently, let's not try again until after a day\".\n>>\n>> The semantics of b) are very useful, and just because they're tied up\n>> with a) right now, let's not throw out b) just because we're trying to\n>> solve a).\n>\n> Yeah. In general my concern was the handling of (b), which I think this\n> last crop of patches is fine with. As far as showing the message\n> repeatedly or not, I don't have a strong opinion (it could even be\n> configurable, once it's split from the \"marker\" behavior).\n>\n>> Right now one thing we do right is we always try to look at the actual\n>> state of the repo with too_many_packs() and too_many_loose_objects().\n>>\n>> We don't assume the state of your repo hasn't drastically changed\n>> recently. That means that we do the right thing and gc when the repo\n>> needs it, not just because we GC'd recently enough.\n>>\n>> With these proposed semantics we'd skip a needed GC (potentially for\n>> days, depending on the default) just because we recently ran one.\n>\n> Yeah, I agree that deferring repeated gc's based on time is going to run\n> into pathological corner cases. OTOH, what we've found at GitHub is that\n> \"gc --auto\" is quite insufficient for scheduling maintenance anyway\n> (e.g., if a machine gets pushes to 100 separate repositories in quick\n> succession, you probably want to queue and throttle any associated gc).\n\nI'm sure you have better solutions for this at GitHub, but as an aside\nit might be interesting to add some sort of gc flock + retry setting for\nthis use-case, i.e. even if you had 100 concurrent gc's due to\ntoo_many_*(), they'd wait + retry until they could get the flock on a\ngiven file.\n\nThen again this is probably too specific, and could be done with a\npre-auto-gc hook too..\n\n> But there are probably more mid-size sites that are big enough to have\n> corner cases, but not so big that \"gc --auto\" is hopeless. ;)\n\nFWIW I don't deal with gc on the server at all these days (that's our\ninternal GitLab's instance problem to solve). I just mentioned the edge\ncase of a growing number of pack files on the server upthread as\nsomething that we'd be shipping with git if we had time-based backoff\nsemantics.\n"},{"id":"352909","messageId":"xmqqk1psldkx.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180717201348.GD26218@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T16:21:18Z","receivedAt":"2018-07-18T16:21:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> There's nothing for the calling program to act on on the basis of that\n>> error. Use status 0 consistently instead, to indicate that we decided\n>> not to run a gc (just like if no housekeeping was required). This\n>> way, repo and similar tools can get the benefit of the same behavior\n>> as tools like \"git fetch\" that ignore the exit status from gc --auto.\n>\n> I think this is a good change.\n>\n> In theory it might be useful to propagate the original exit code (_not_\n> \"did we see a warning or an error\", but the true original exit code. But\n> as you note, it's not deterministic anyway (we'd miss that exit code on\n> the first run, or even any simultaneous runs that exit early due to lock\n> contention). So it's clear that callers can't really do anything robust\n> based on the exit code of a daemonized \"gc --auto\".\n>\n> I still think that \"repo\" should probably stop respecting the exit code.\n> But that's no excuse for Git not to have a sensible exit code in the\n> first place.\n\nI am not yet convinced that this last step to exit with 0 is a good\nchange, even though I can understand that it would be more\nconvenient, as there currently is no easy way for the calling script\nto tell two error cases apart.\n\nI think the \"sensible exit code\" you mention would be something like\n\"1 for hard error, 2 for 'I am punting as I see there were previous\nerrors---you may want to examine your repository'\". \n\nIf we did that from day one and documented that behaviour, nobody\nwould have complained, but adopting that suddenly is of course a\nbreaking change.\n\nPerhaps we should exit with 2 (not 0) in that \"previous error\" case\nby default, and then have a configuration knob to turn that 2 into 0\nfor those who cannot easily modify the calling script? That way, we\nby default will *not* break those who have been paying attention to\nzero-ness of the exit status, we allow those who want to treat this\n\"prior error\" case as if there were no error with just a knob, and\nthen those who are willing to update their script can tell two cases\nby the exit status and act differently.\n\n\n\n"},{"id":"352922","messageId":"20180718172225.GA2677@sigill.intra.peff.net","threadId":"48890","inReplyTo":"xmqqk1psldkx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T17:22:25Z","receivedAt":"2018-07-18T17:22:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 09:21:18AM -0700, Junio C Hamano wrote:\n\n> > I still think that \"repo\" should probably stop respecting the exit code.\n> > But that's no excuse for Git not to have a sensible exit code in the\n> > first place.\n> \n> I am not yet convinced that this last step to exit with 0 is a good\n> change, even though I can understand that it would be more\n> convenient, as there currently is no easy way for the calling script\n> to tell two error cases apart.\n> \n> I think the \"sensible exit code\" you mention would be something like\n> \"1 for hard error, 2 for 'I am punting as I see there were previous\n> errors---you may want to examine your repository'\". \n> \n> If we did that from day one and documented that behaviour, nobody\n> would have complained, but adopting that suddenly is of course a\n> breaking change.\n> \n> Perhaps we should exit with 2 (not 0) in that \"previous error\" case\n> by default, and then have a configuration knob to turn that 2 into 0\n> for those who cannot easily modify the calling script? That way, we\n> by default will *not* break those who have been paying attention to\n> zero-ness of the exit status, we allow those who want to treat this\n> \"prior error\" case as if there were no error with just a knob, and\n> then those who are willing to update their script can tell two cases\n> by the exit status and act differently.\n\nI think we have been exiting non-zero with \"previous errors\" for some\ntime with the daemonizing code. It was just spelled \"-1\" instead of \"2\".\nSo just jumping right there does not mean any regression from the\ncurrent state, I don't think (but it also does not fix existing scripts\nlike \"repo\" that check the code).\n\nI agree the config you suggest would give people the tools to make that\ncase work. But it somehow rubs me the wrong way. Can you imagine the\nperspective of a user who is told \"oh, your script breaks? Just try\nsetting this option to ignore error codes in this one particular\nsituation\". It feels like a weird hack, because it is.\n\nIt's also still inconsistent in the daemonize case. The run that yields\nthe error won't return a non-zero exit. But the next run will exit with\n\"2\".\n\n-Peff\n"},{"id":"352924","messageId":"20180718172913.GB2677@sigill.intra.peff.net","threadId":"48890","inReplyTo":"874lgwd6yd.fsf@evledraar.gmail.com","subject":"Re: [PATCH] gc: do not warn about too many loose objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T17:29:13Z","receivedAt":"2018-07-18T17:29:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 03:11:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Yeah, I agree that deferring repeated gc's based on time is going to run\n> > into pathological corner cases. OTOH, what we've found at GitHub is that\n> > \"gc --auto\" is quite insufficient for scheduling maintenance anyway\n> > (e.g., if a machine gets pushes to 100 separate repositories in quick\n> > succession, you probably want to queue and throttle any associated gc).\n> \n> I'm sure you have better solutions for this at GitHub, but as an aside\n> it might be interesting to add some sort of gc flock + retry setting for\n> this use-case, i.e. even if you had 100 concurrent gc's due to\n> too_many_*(), they'd wait + retry until they could get the flock on a\n> given file.\n> \n> Then again this is probably too specific, and could be done with a\n> pre-auto-gc hook too..\n\nYeah, I think any multi-repo solution is getting way too specific for\nGit, and the best thing we can do is provide a hook. I agree you could\nprobably do this today with a pre-auto-gc hook (if it skips gc it would\njust queue itself and return non-zero). Or even just make a mark in a\ndatabase that says \"there was some activity here\".\n\nSince we have so much other infrastructure sitting between the user and\nGit anyway, we do that marking at a separate layer which is already\ntalking to a database. ;)\n\nAnyway, I do agree with your general notion that this isn't the right\napproach for many situations, and auto-gc is a better solution for many\ncases.\n\n-Peff\n"},{"id":"352938","messageId":"xmqqlga8jtka.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180718172225.GA2677@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T18:19:01Z","receivedAt":"2018-07-18T18:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Perhaps we should exit with 2 (not 0) in that \"previous error\" case\n>> by default, and then have a configuration knob to turn that 2 into 0\n>> for those who cannot easily modify the calling script? That way, we\n>> by default will *not* break those who have been paying attention to\n>> zero-ness of the exit status, we allow those who want to treat this\n>> \"prior error\" case as if there were no error with just a knob, and\n>> then those who are willing to update their script can tell two cases\n>> by the exit status and act differently.\n>\n> I think we have been exiting non-zero with \"previous errors\" for some\n> time with the daemonizing code. It was just spelled \"-1\" instead of \"2\".\n> So just jumping right there does not mean any regression from the\n> current state, I don't think (but it also does not fix existing scripts\n> like \"repo\" that check the code).\n\nCorrect. That is why I suggested that default setting. \n\nHowever.\n\nIf we change it so that by default we exit with 0 but allow those\nwho care to see 2 with a knob, that would be a regression to those\nwho actually were acting on the non-zero exit, but they no longer\nhave to scrape the log/warning output if they choose to set the\nknob, so the \"convenience\" factor might argue in favor of the \"by\ndefault we return 0 but you can choose to get the 'sensible exit\ncodes'\" approach. I do not have a strong opinion either way wrt\nwhich one should be the default.\n\n> I agree the config you suggest would give people the tools to make that\n> case work. But it somehow rubs me the wrong way. Can you imagine the\n> perspective of a user who is told \"oh, your script breaks? Just try\n> setting this option to ignore error codes in this one particular\n> situation\". It feels like a weird hack, because it is.\n\nWell, the message is \"your script is broken (and has always been\nwhen 'gc --auto' saw that the previous round failed), but now we\nhave two ways to help you correct. An easier but broken way is to\nturn this knob to turn 2 to 0 to hide the issue under the rug; or\nyou can notice the difference between $?==1 and $?==2 and act\ndifferently\". \"turn 2 to 0\" is a weird hack, but they are also\ngiven the option of doing the right thing, so...\n\n> It's also still inconsistent in the daemonize case. The run that yields\n> the error won't return a non-zero exit. But the next run will exit with\n> \"2\".\n\nI do not see this particular \"inconsistency\" a problem. The run\nthat first discovers the problem by definition cannot return with\nnon-zero; not waiting until finding out the outcome is the whole\npoint of running in the background and giving control back early.\n\n"},{"id":"352945","messageId":"20180718190645.GA7778@sigill.intra.peff.net","threadId":"48890","inReplyTo":"xmqqlga8jtka.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-18T19:06:46Z","receivedAt":"2018-07-18T19:07:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 18, 2018 at 11:19:01AM -0700, Junio C Hamano wrote:\n\n> > It's also still inconsistent in the daemonize case. The run that yields\n> > the error won't return a non-zero exit. But the next run will exit with\n> > \"2\".\n> \n> I do not see this particular \"inconsistency\" a problem. The run\n> that first discovers the problem by definition cannot return with\n> non-zero; not waiting until finding out the outcome is the whole\n> point of running in the background and giving control back early.\n\nI guess I just see it as encouraging a non-robust flow. I can see\nsomebody thinking they should care about \"gc --auto\" errors, because\nthey can turn up latent repository corruption (because most operations\ntry to only look at the objects they need to, and gc inherently\nconsiders every object). But doing so would give a very delayed\nnotification for a repository that is handled infrequently. So finding\nout about a corruption that we detected might takes weeks or months.\n\nI dunno. If your primary motivation is not finding latent problems, but\nloudly complaining when gc does not make forward progress, I suppose it\nmakes more sense.\n\n-Peff\n"},{"id":"352972","messageId":"xmqq8t68jp3s.fsf@gitster-ct.c.googlers.com","threadId":"48890","inReplyTo":"20180718190645.GA7778@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] gc: do not return error for prior errors in daemonized mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T19:55:19Z","receivedAt":"2018-07-18T19:55:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 18, 2018 at 11:19:01AM -0700, Junio C Hamano wrote:\n>\n>> > It's also still inconsistent in the daemonize case. The run that yields\n>> > the error won't return a non-zero exit. But the next run will exit with\n>> > \"2\".\n>> \n>> I do not see this particular \"inconsistency\" a problem. The run\n>> that first discovers the problem by definition cannot return with\n>> non-zero; not waiting until finding out the outcome is the whole\n>> point of running in the background and giving control back early.\n>\n> I guess I just see it as encouraging a non-robust flow. I can see\n> somebody thinking they should care about \"gc --auto\" errors, because\n> they can turn up latent repository corruption (because most operations\n> try to only look at the objects they need to, and gc inherently\n> considers every object). But doing so would give a very delayed\n> notification for a repository that is handled infrequently. So finding\n> out about a corruption that we detected might takes weeks or months.\n>\n> I dunno. If your primary motivation is not finding latent problems, but\n> loudly complaining when gc does not make forward progress, I suppose it\n> makes more sense.\n\nI am not sure either. If your primary motivation is to protect\nyourself from ignoring gc failures that could be an indication of\npossible repository corruption, you wouldn't be running it\nbackgrounded in the first place, I woudl guess. And if you are\nbackgrounding, with \"exit with 2, not hide it with 0\" approach, you\nwould have a better chance to notice, no?\n\n"},{"id":"358311","messageId":"20180917183336.GA4630@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180717195946.GC26218@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] gc: exit with status 128 on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-17T18:33:37Z","receivedAt":"2018-09-17T18:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 17, 2018 at 03:59:47PM -0400, Jeff King wrote:\n\n> On Mon, Jul 16, 2018 at 11:54:16PM -0700, Jonathan Nieder wrote:\n> \n> > A value of -1 returned from cmd_gc gets propagated to exit(),\n> > resulting in an exit status of 255. Use die instead for a clearer\n> > error message and a controlled exit.\n> \n> This feels a little funny because we know we're going to turn some of\n> these back in the next patch. That said, I'm OK with it, since this\n> version is already written.\n\nThere's discussion elsewhere[1] of applying just up to patch 2.\n\nDo we still want to convert these cases to die() as their end-state?\nAnother alternative would be to just convert a \"-1\" return to 128 or\nsimilar at the level of cmd_gc(), which avoids the 255 weirdness.\n\nDoing so also keeps the error messages the same as (as \"error\" and not\n\"fatal\"). I'm not sure which we prefer.\n\nIt also makes the code more flexible and lib-ifiable (since the caller\ncan decide how to handle the errors). That probably doesn't matter much\nsince this is all static-local to builtin/gc.c, though I suppose in\ntheory we could eventually do parts of \"gc --auto\" without spawning a\nseparate process.\n\n-Peff\n\n[1] https://public-inbox.org/git/20180917182210.GB3894@sigill.intra.peff.net/\n"},{"id":"358313","messageId":"20180917184012.GD140909@aiede.svl.corp.google.com","threadId":"48890","inReplyTo":"20180917183336.GA4630@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] gc: exit with status 128 on failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-17T18:40:12Z","receivedAt":"2018-09-17T18:40:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Tue, Jul 17, 2018 at 03:59:47PM -0400, Jeff King wrote:\n>> On Mon, Jul 16, 2018 at 11:54:16PM -0700, Jonathan Nieder wrote:\n\n>>> A value of -1 returned from cmd_gc gets propagated to exit(),\n>>> resulting in an exit status of 255. Use die instead for a clearer\n>>> error message and a controlled exit.\n>>\n>> This feels a little funny because we know we're going to turn some of\n>> these back in the next patch. That said, I'm OK with it, since this\n>> version is already written.\n>\n> There's discussion elsewhere[1] of applying just up to patch 2.\n>\n> Do we still want to convert these cases to die() as their end-state?\n\nIMHO yes, we do. die() is the function that you can use to exit with\na fatal error.\n\nIf we want to get rid of die(), that would be a tree-wide effort, not\nsomething that should hold up this patch.\n\n[...]\n> It also makes the code more flexible and lib-ifiable (since the caller\n> can decide how to handle the errors). That probably doesn't matter much\n> since this is all static-local to builtin/gc.c,\n\nExactly. I'm a strong believer in http://wiki.c2.com/?YouArentGonnaNeedIt.\n\nThanks,\nJonathan\n"},{"id":"358403","messageId":"20180918173017.GD15470@sigill.intra.peff.net","threadId":"48890","inReplyTo":"20180917184012.GD140909@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/3] gc: exit with status 128 on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-18T17:30:18Z","receivedAt":"2018-09-18T17:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2018 at 11:40:12AM -0700, Jonathan Nieder wrote:\n\n> > There's discussion elsewhere[1] of applying just up to patch 2.\n> >\n> > Do we still want to convert these cases to die() as their end-state?\n> \n> IMHO yes, we do. die() is the function that you can use to exit with\n> a fatal error.\n> \n> If we want to get rid of die(), that would be a tree-wide effort, not\n> something that should hold up this patch.\n\nBut that was sort of my question. I think there are people who _do_ want\nto get rid of most die() calls (like Dscho), and there is a tree-wide\neffort that is happening slowly to lib-ify. Your patch goes in the\nopposite direction.\n\nThat said, I think there are actually two cases in your patch.\n\nThe calls to \"return error()\" or even just \"return -1\" in cmd_gc() seem\nlike obvious candidates for die(). We're at the top of the stack, and\nanybody lib-ifying at that level is going to need to extract bits into\nreusable functions anyway.\n\nI more wondered about helpers like report_last_gc_error() and\ngc_before_repack().\n\n> > It also makes the code more flexible and lib-ifiable (since the caller\n> > can decide how to handle the errors). That probably doesn't matter much\n> > since this is all static-local to builtin/gc.c,\n> \n> Exactly. I'm a strong believer in http://wiki.c2.com/?YouArentGonnaNeedIt.\n\nI only half-agree that this is YAGNI. If it were \"let's punt on making\nthis code friendlier to lib-ification\", I'd agree more completely. But\nit's actually taking an active step in the opposite direction.\n\nI dunno. It's probably not worth spending too much more time discussing,\nand I'm OK either way. I mostly just wanted to raise the issue since\ndropping patch 3 changes the balance to me.\n\n-Peff\n"}]}