{"thread":{"id":"48579","subject":"[RFC] git gc \"--prune=now\" semantics considered harmful","startedAt":"2018-05-26T21:49:36Z","lastAt":"2018-06-01T11:07:44Z","messageCount":5,"participants":["Linus Torvalds","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348610","messageId":"alpine.LFD.2.21.999.1805261449160.6211@i7.lan","threadId":"48579","inReplyTo":null,"subject":"[RFC] git gc \"--prune=now\" semantics considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-05-26T21:49:25Z","receivedAt":"2018-05-26T21:49:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nSo this is a RFC patch, I'm not sure how much people really care, but I \nfind the current behavior of \"git gc --prune=now\" to be unnecessarily \ndangerous.\n\nThere's two issues with it:\n\n (a) parse_expiry_date() considers \"now\" to be special, and it actually\n     doesn't mean \"now\" at all, it means \"everything\".\n\n (b) the date parsing isn't actually done \"now\", it's done *after* gc has \n     already run, and we run \"git prune --expire\". So even if (a) wasn't \n     true, \"--prune=now\" wouldn't actually mean \"now\" when the user \n    expects it to happen, but \"after doing repacking\".\n\nI actually think that the \"parse_expiry_date()\" behavior makes sense \nwithin the context of \"git prune --expire\", so I'm not really complaining \nabout (a) per se. I just think that what makes sense within the context of \n\"git prune\" does *not* necessarily make sense within the context of \"git \ngc\".\n\nWhy do I care? I end up doing lots of random things in my local \nrepository, and I also end up wanting to keep my repository fairly clean, \nso I tend to do \"git gc --prune=now\" to just make sure everything is \npacked and I've gotten rid of all the temporary stuff that so often \nhappens when doing lots of git merges (which is what I do). \n\nYou won't see those temporary objects for the usual trivial merges, but \nwhenever you have a real recursive merge with automated conflict \nresolution, there will be things like those temporary merge-only objects \nfor the 3-way base merge state. \n\nSoes my use pattern of \"git gc --prune=now\" make sense? Maybe not. But \nit's what I've gotten used to, and it's at least not entirely insane.\n\nBut at least once now, I've done that \"git gc\" at the end of the day, and \na new pull request comes in, so I do the \"git pull\" without even thinking \nabout the fact that \"git gc\" is still running.\n\nAnd then the \"--prune=now\" behavior is actually really pretty dangerous. \nBecause it will prune *all* unreachable objects, even if they are only \n*currently* unreachable because they are in the process of being unpacked \nby the concurrent \"git fetch\" (and I didn't check - I might just have been \nunlocky, bit I think \"git prune\" ignores FETCH_HEAD).\n\nSo I actually would much prefer that foir git gc, \"--prune=now\" means\n\n (a) \"now\"\n\n (b) now at the _start_ of the \"git gc\" operation, not the time at\n     the _end_ of the operation when we've already spent a minute or\n     two doing repacking and are now doing the final pruning.\n\nanyway, with that explanation in mind, I'm appending a patch that is \npretty small and does that. It's a bit hacky, but I think it still makes \nsense.\n\nComments?\n\nNote that this really isn't likely very noticeable on most projects. When \nI do \"git gc\" on a fairly well-packed repo of git itself, it takes under \n4s for me. So the window for that whole \"do git pull at the same time\" is \nsimply not much of an issue.\n\nFor the kernel, \"git gc\" takes a minute and a half on the same machine \n(again, this is already a packed repo, it can be worse). So there's a much \nbigger window there to do something stupid,\n\n             Linus\n\n---\n builtin/gc.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c4777b244..98368c8b5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -535,8 +535,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (argc > 0)\n \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n \n-\tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n-\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n+\tif (prune_expire) {\n+\t\tif (!strcmp(prune_expire, \"now\"))\n+\t\t\tprune_expire = show_date(time(NULL), 0, DATE_MODE(ISO8601));\n+\t\tif (parse_expiry_date(prune_expire, &dummy))\n+\t\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n+\t}\n \n \tif (aggressive) {\n \t\targv_array_push(&repack, \"-f\");\n"},{"id":"348611","messageId":"xmqqd0xim1tp.fsf@gitster-ct.c.googlers.com","threadId":"48579","inReplyTo":"alpine.LFD.2.21.999.1805261449160.6211@i7.lan","subject":"Re: [RFC] git gc \"--prune=now\" semantics considered harmful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-26T23:31:14Z","receivedAt":"2018-05-26T23:31:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Soes my use pattern of \"git gc --prune=now\" make sense? Maybe not. But \n> it's what I've gotten used to, and it's at least not entirely insane.\n\nFWIW, my end-of-day ritual is to do repack -a -d -f with a large\nwindow and a small depth, followed by prune, which boils down to\nabout the same.  So I'd hope that is not entirely insane.  I however\ndo not think I bother with an explicit --expire=now when running the\nprune, though.\n\nIn any case, that makes two of us, and the suggested patch protects\nonly one of the two ;-)\n\n> But at least once now, I've done that \"git gc\" at the end of the day, and \n> a new pull request comes in, so I do the \"git pull\" without even thinking \n> about the fact that \"git gc\" is still running.\n\n*That* is something I don't do.  After all, I am fully aware that I\nhave started end-of-day ritual by that time, so I won't even look at\na new patch (or a pull request for that matter).\n\n> So I actually would much prefer that foir git gc, \"--prune=now\" means\n>\n>  (a) \"now\"\n>\n>  (b) now at the _start_ of the \"git gc\" operation, not the time at\n>      the _end_ of the operation when we've already spent a minute or\n>      two doing repacking and are now doing the final pruning.\n>\n> anyway, with that explanation in mind, I'm appending a patch that is \n> pretty small and does that. It's a bit hacky, but I think it still makes \n> sense.\n>\n> Comments?\n\nClosing the possiblity of racing a running \"gc\" and new object\ncreation like the above generally makes sense, I would think,\nwhether the creation is due to 'pull/fetch', 'add', or even 'push'.\n\nI however have to wonder if there are opposite \"oops\" end-user\noperation we also need to worry about, i.e. we are doing a large-ish\nfetch, and get bored and run a gc fron another terminal.  Perhaps\n*that* is a bit too stupid to worry about?  Auto-gc deliberately\ndoes not use 'now' because it wants to leave a grace period to avoid\nexactly that kind of race.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c4777b244..98368c8b5 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -535,8 +535,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \tif (argc > 0)\n>  \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n>  \n> -\tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n> -\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n> +\tif (prune_expire) {\n> +\t\tif (!strcmp(prune_expire, \"now\"))\n> +\t\t\tprune_expire = show_date(time(NULL), 0, DATE_MODE(ISO8601));\n> +\t\tif (parse_expiry_date(prune_expire, &dummy))\n> +\t\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n> +\t}\n>  \n>  \tif (aggressive) {\n>  \t\targv_array_push(&repack, \"-f\");\n"},{"id":"348614","messageId":"CA+55aFycPQ9PTbAGXqL7i9QQPsx-AT8R3S5gTvC0Ue=ot2J9FQ@mail.gmail.com","threadId":"48579","inReplyTo":"xmqqd0xim1tp.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC] git gc \"--prune=now\" semantics considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-05-27T01:27:45Z","receivedAt":"2018-05-27T01:28:01Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sat, May 26, 2018 at 4:31 PM Junio C Hamano <gitster@pobox.com> wrote:\n\n> *That* is something I don't do.  After all, I am fully aware that I\n> have started end-of-day ritual by that time, so I won't even look at\n> a new patch (or a pull request for that matter).\n\nSounds like you're more organized about the end-of-day ritual than I am.\nFor me the gc is not quite so structured.\n\n> I however have to wonder if there are opposite \"oops\" end-user\n> operation we also need to worry about, i.e. we are doing a large-ish\n> fetch, and get bored and run a gc fron another terminal.  Perhaps\n> *that* is a bit too stupid to worry about?  Auto-gc deliberately\n> does not use 'now' because it wants to leave a grace period to avoid\n> exactly that kind of race.\n\nFor me, a \"pull\" never takes that long.  Sure, any manual merging and the\nwriting of the commit message might take a while, but it's \"foreground\"\nactivity for me, I'd not start a gc in the middle of it.\n\nSo at least to me, doing \"git fsck --full\" and \"git gc --prune=now\" are\nsomewhat special because they take a while and tend to be background things\nthat I \"start and forget\" about (the same way I sometimes start and forget\na kernel build).\n\nWhich is why that current \"git gc --prune=now\" behavior seems a bit\ndangerous.\n\n                Linus\n"},{"id":"348979","messageId":"20180601070456.GB15578@sigill.intra.peff.net","threadId":"48579","inReplyTo":"xmqqd0xim1tp.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC] git gc \"--prune=now\" semantics considered harmful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-01T07:04:56Z","receivedAt":"2018-06-01T07:05:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 27, 2018 at 08:31:14AM +0900, Junio C Hamano wrote:\n\n> > So I actually would much prefer that foir git gc, \"--prune=now\" means\n> >\n> >  (a) \"now\"\n> >\n> >  (b) now at the _start_ of the \"git gc\" operation, not the time at\n> >      the _end_ of the operation when we've already spent a minute or\n> >      two doing repacking and are now doing the final pruning.\n> >\n> > anyway, with that explanation in mind, I'm appending a patch that is \n> > pretty small and does that. It's a bit hacky, but I think it still makes \n> > sense.\n> >\n> > Comments?\n> \n> Closing the possiblity of racing a running \"gc\" and new object\n> creation like the above generally makes sense, I would think,\n> whether the creation is due to 'pull/fetch', 'add', or even 'push'.\n\nI think Linus's suggestion is an obvious improvement. It does shorten\nthe window for confusing things to happen, and I think it makes things\nmuch easier to reason about if all parts of the gc are using the same\ntimestamp.\n\nRegarding the implementation:\n\n> > -\tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n> > -\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n> > +\tif (prune_expire) {\n> > +\t\tif (!strcmp(prune_expire, \"now\"))\n> > +\t\t\tprune_expire = show_date(time(NULL), 0, DATE_MODE(ISO8601));\n> > +\t\tif (parse_expiry_date(prune_expire, &dummy))\n> > +\t\t\tdie(_(\"failed to parse prune expiry value %s\"), prune_expire);\n> > +\t}\n\nWe'd also accept relative times like \"5.minutes.ago\" (in fact, the\ndefault is a relative 2.weeks.ago, though it's long enough that the\ndifference between \"2 weeks\" and \"2 weeks plus 5 minutes\" may not matter\nmuch). So we probably ought to just normalize _everything_ without even\nbothering to match \"now\". It's a noop for non-relative times, but that's\nOK.\n\n> I however have to wonder if there are opposite \"oops\" end-user\n> operation we also need to worry about, i.e. we are doing a large-ish\n> fetch, and get bored and run a gc fron another terminal.  Perhaps\n> *that* is a bit too stupid to worry about?  Auto-gc deliberately\n> does not use 'now' because it wants to leave a grace period to avoid\n> exactly that kind of race.\n\nThere are still possibilities for a race, even with the grace period.\nYou can have an unreferenced 2-week-old object sitting on disk, and\nsomebody can choose to reference it at the same time as we are pruning\nit. My freshness patches from a few years ago made things a bit better:\n\n  - when we optimize out the write of an existing object, we now at\n    least update its timestamp\n\n  - we consider non-fresh objects reachable from fresh ones to also be\n    fresh\n\nBut fundamentally none of this is atomic. You can have an old tree, and\nwhile you're pruning somebody writes a new commit referencing it and\nsticks that in a ref. It's more common if your grace period is \"now\",\nbut it can still happen with any grace period.\n\n-Peff\n"},{"id":"348997","messageId":"CA+55aFz5cwZa_PWMV2aGv0TGBpBz8OUB-dPvdwv4ouQs1t1mzA@mail.gmail.com","threadId":"48579","inReplyTo":"20180601070456.GB15578@sigill.intra.peff.net","subject":"Re: [RFC] git gc \"--prune=now\" semantics considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-06-01T11:07:28Z","receivedAt":"2018-06-01T11:07:44Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Jun 1, 2018 at 2:04 AM Jeff King <peff@peff.net> wrote:\n>\n> We'd also accept relative times like \"5.minutes.ago\" (in fact, the\n> default is a relative 2.weeks.ago, though it's long enough that the\n> difference between \"2 weeks\" and \"2 weeks plus 5 minutes\" may not matter\n> much). So we probably ought to just normalize _everything_ without even\n> bothering to match \"now\". It's a noop for non-relative times, but that's\n> OK.\n\nHeh. That's what I tried to do at first.\n\nIt's surprisingly hard.\n\nYou can't normalize it as a date, because we have a few times that\naren't expressible as dates because they are just the maximum value\n(ie \"all\").\n\nAnd then I tried to just express it just as a standard numerical\nvalue, which we accept on input. But we *only* accept that if it's\nmore than eight digits.\n\nAnd regardless, you need to special-case \"now\", since\nparse_expiry_date() special cases it.\n\nOr you'd need to just make another version of parse_expiry_date() entirely.\n\nEnd result: I only special-cased \"now\".\n\n> There are still possibilities for a race, even with the grace period.\n\nAgreed. But you *really* have to work at it.\n\n              Linus\n"}]}