{"thread":{"id":"53806","subject":"[PATCH] experimental: default to fetch.writeCommitGraph=false","startedAt":"2020-07-07T06:20:43Z","lastAt":"2020-07-08T05:47:30Z","messageCount":7,"participants":["Jonathan Nieder","Derrick Stolee","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401078","messageId":"20200707062039.GC784740@google.com","threadId":"53806","inReplyTo":null,"subject":"[PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-07T06:20:39Z","receivedAt":"2020-07-07T06:20:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The fetch.writeCommitGraph feature makes fetches write out a commit\ngraph file for the newly downloaded pack on fetch. This improves the\nperformance of various commands that would perform a revision walk and\neventually ought to be the default for everyone. To prepare for that\nfuture, it's enabled by default for users that set\nfeature.experimental=true to experience such future defaults.\n\nAlas, for --unshallow fetches from a shallow clone it runs into a\nsnag: by the time Git has fetched the new objects and is writing a\ncommit graph, it has performed a revision walk and r->parsed_objects\ncontains information about the shallow boundary from *before* the\nfetch. The commit graph writing code is careful to avoid writing a\ncommit graph file in shallow repositories, but the new state is not\nshallow, and the result is that from that point on, commands like \"git\nlog\" make use of a newly written commit graph file representing a\nfictional history with the old shallow boundary.\n\nWe could fix this by making the commit graph writing code more careful\nto avoid writing a commit graph that could have used any grafts or\nshallow state, but it is possible that there are other pieces of\nmutated state that fetch's commit graph writing code may be relying\non. So disable it in the feature.experimental configuration.\n\nGoogle developers have been running in this configuration (by setting\nfetch.writeCommitGraph=false in the system config) to work around this\nbug since it was discovered in April. Once the fix lands, we'll\nenable fetch.writeCommitGraph=true again to give it some early testing\nbefore rolling out to a wider audience.\n\nIn other words:\n\n- this patch only affects behavior with feature.experimental=true\n\n- it makes feature.experimental match the configuration Google has\n been using for the last few months, meaning it would leave users in\n a better tested state than without it\n\n- this should improve testing for other features guarded by\n feature.experimental, by making feature.experimental safer to use\n\nReported-by: Jay Conrod \nHelped-by: Taylor Blau \nSigned-off-by: Jonathan Nieder \n---\nI realize this is late to send. That said, as described above, I\nthink it's a good way to buy time by minimizing user exposure to\nfetch.writeCommitGraph=true until a fix for it is well cooked.\n\nIn other words, I'd like to see this patch in Git 2.28-rc0.\n\nThanks of all kinds welcome, as always. Previous discussion:\nhttps://lore.kernel.org/git/20200603034213.GB253041@google.com/\n\n Documentation/config/feature.txt | 8 --------\n Documentation/config/fetch.txt | 3 +--\n repo-settings.c | 8 ++++----\n 3 files changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 28c33602d52..c0cbf2bb1cd 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -15,14 +15,6 @@ feature.experimental::\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n +\n-* `fetch.writeCommitGraph=true` writes a commit-graph after every `git fetch`\n-command that downloads a pack-file from a remote. Using the `--split` option,\n-most executions will create a very small commit-graph file on top of the\n-existing commit-graph file(s). Occasionally, these files will merge and the\n-write may take longer. Having an updated commit-graph file helps performance\n-of many Git commands, including `git merge-base`, `git push -f`, and\n-`git log --graph`.\n-+\n * `protocol.version=2` speeds up fetches from repositories with many refs by\n allowing the client to specify which refs to list before the server lists\n them.\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex b1a9b1461d3..b20394038d1 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -90,5 +90,4 @@ fetch.writeCommitGraph::\n \tthe existing commit-graph file(s). Occasionally, these files will\n \tmerge and the write may take longer. Having an updated commit-graph\n \tfile helps performance of many Git commands, including `git merge-base`,\n-\t`git push -f`, and `git log --graph`. Defaults to false, unless\n-\t`feature.experimental` is true.\n+\t`git push -f`, and `git log --graph`. Defaults to false.\ndiff --git a/repo-settings.c b/repo-settings.c\nindex dc6817daa95..0918408b344 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -51,14 +51,14 @@ void prepare_repo_settings(struct repository *r)\n \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n \t}\n+\n \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n \t\tr->settings.fetch_write_commit_graph = value;\n-\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n-\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n-\t}\n \tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n \n+\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n+\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n+\n \t/* Hack for test programs like test-dump-untracked-cache */\n \tif (ignore_untracked_cache_config)\n \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401095","messageId":"0b7435e3-95fc-8085-9e26-6d451809faa8@gmail.com","threadId":"53806","inReplyTo":"20200707062039.GC784740@google.com","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-07T13:24:12Z","receivedAt":"2020-07-07T13:24:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/7/2020 2:20 AM, Jonathan Nieder wrote:\n> The fetch.writeCommitGraph feature makes fetches write out a commit\n> graph file for the newly downloaded pack on fetch. This improves the\n> performance of various commands that would perform a revision walk and\n> eventually ought to be the default for everyone. To prepare for that\n> future, it's enabled by default for users that set\n> feature.experimental=true to experience such future defaults.\n> \n> Alas, for --unshallow fetches from a shallow clone it runs into a\n> snag: by the time Git has fetched the new objects and is writing a\n> commit graph, it has performed a revision walk and r->parsed_objects\n> contains information about the shallow boundary from *before* the\n> fetch. The commit graph writing code is careful to avoid writing a\n> commit graph file in shallow repositories, but the new state is not\n> shallow, and the result is that from that point on, commands like \"git\n> log\" make use of a newly written commit graph file representing a\n> fictional history with the old shallow boundary.\n> \n> We could fix this by making the commit graph writing code more careful\n> to avoid writing a commit graph that could have used any grafts or\n> shallow state, but it is possible that there are other pieces of\n> mutated state that fetch's commit graph writing code may be relying\n> on. So disable it in the feature.experimental configuration.\n> \n> Google developers have been running in this configuration (by setting\n> fetch.writeCommitGraph=false in the system config) to work around this\n> bug since it was discovered in April. Once the fix lands, we'll\n> enable fetch.writeCommitGraph=true again to give it some early testing\n> before rolling out to a wider audience.\n> \n> In other words:\n> \n> - this patch only affects behavior with feature.experimental=true\n> \n> - it makes feature.experimental match the configuration Google has\n> been using for the last few months, meaning it would leave users in\n> a better tested state than without it\n> \n> - this should improve testing for other features guarded by\n> feature.experimental, by making feature.experimental safer to use\n> \n> Reported-by: Jay Conrod \n> Helped-by: Taylor Blau \n> Signed-off-by: Jonathan Nieder \n> ---\n> I realize this is late to send. That said, as described above, I\n> think it's a good way to buy time by minimizing user exposure to\n> fetch.writeCommitGraph=true until a fix for it is well cooked.\n\nWhile it would certainly be better to fix the commit_graph_compatible()\nmethod, I understand that that is a more complicated problem.\n\n> In other words, I'd like to see this patch in Git 2.28-rc0.\n\nThis patch is 100% correct.\n\nNormally, I would say \"this is experimental, and a user can always\ndisable fetch.writeCommitGraph manually if they are running into this.\"\nThis is especially true because unshallowing a repo is (probably)\na rare operation. At least, I expect that very few users actually do\nit, and those who do are expert users.\n\nBut, it is best to reduce user pain, even in rare cases like this.\n\nFurther, I hope to submit new maintenance tasks soon which can\nreplace fetch.writeCommitGraph.\n\nThanks,\n-Stolee\n"},{"id":"401122","messageId":"xmqq8sfv745r.fsf@gitster.c.googlers.com","threadId":"53806","inReplyTo":"20200707062039.GC784740@google.com","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T15:09:36Z","receivedAt":"2020-07-07T15:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The fetch.writeCommitGraph feature makes fetches write out a commit\n\n> In other words:\n>\n> - this patch only affects behavior with feature.experimental=true\n>\n> - it makes feature.experimental match the configuration Google has\n> been using for the last few months, meaning it would leave users in\n> a better tested state than without it\n>\n> - this should improve testing for other features guarded by\n> feature.experimental, by making feature.experimental safer to use\n\n\nIn other words, fetch.writeCommitGraph in its current form is too\nbroken to be recommended even for brave souls with \"experimental\"\nbit on.\n\nI wonder if we perhaps wnat to add to the documentation for\nwriteCommitGraph configuration that its use is currently not\nrecommended in a shallow clone or something (I know it is not\na problem just to use it with shallow but the breakage needs\nto involve unshallowing, but by definition those who do not\nuse shallow would not hit the unshallowing bug, so...).\n\n> Reported-by: Jay Conrod \n> Helped-by: Taylor Blau \n> Signed-off-by: Jonathan Nieder \n> ---\n> I realize this is late to send. That said, as described above, I\n> think it's a good way to buy time by minimizing user exposure to\n> fetch.writeCommitGraph=true until a fix for it is well cooked.\n>\n> In other words, I'd like to see this patch in Git 2.28-rc0.\n\nYes, I do, too.\n\nThanks.\n\n> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> index b1a9b1461d3..b20394038d1 100644\n> --- a/Documentation/config/fetch.txt\n> +++ b/Documentation/config/fetch.txt\n> @@ -90,5 +90,4 @@ fetch.writeCommitGraph::\n> \tthe existing commit-graph file(s). Occasionally, these files will\n> \tmerge and the write may take longer. Having an updated commit-graph\n> \tfile helps performance of many Git commands, including `git merge-base`,\n> -\t`git push -f`, and `git log --graph`. Defaults to false, unless\n> -\t`feature.experimental` is true.\n> +\t`git push -f`, and `git log --graph`. Defaults to false.\n> diff --git a/repo-settings.c b/repo-settings.c\n> index dc6817daa95..0918408b344 100644\n> --- a/repo-settings.c\n> +++ b/repo-settings.c\n> @@ -51,14 +51,14 @@ void prepare_repo_settings(struct repository *r)\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n> \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n> \t}\n> +\n> \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n> \t\tr->settings.fetch_write_commit_graph = value;\n> -\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n> -\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n> -\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n> -\t}\n> \tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n> \n> +\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n> +\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n> +\n> \t/* Hack for test programs like test-dump-untracked-cache */\n> \tif (ignore_untracked_cache_config)\n> \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n"},{"id":"401125","messageId":"20200707151735.GA27992@syl.lan","threadId":"53806","inReplyTo":"xmqq8sfv745r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-07T15:17:35Z","receivedAt":"2020-07-07T15:17:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Junio,\n\nOn Tue, Jul 07, 2020 at 08:09:36AM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n> > The fetch.writeCommitGraph feature makes fetches write out a commit\n>\n> > In other words:\n> >\n> > - this patch only affects behavior with feature.experimental=true\n> >\n> > - it makes feature.experimental match the configuration Google has\n> > been using for the last few months, meaning it would leave users in\n> > a better tested state than without it\n> >\n> > - this should improve testing for other features guarded by\n> > feature.experimental, by making feature.experimental safer to use\n>\n>\n> In other words, fetch.writeCommitGraph in its current form is too\n> broken to be recommended even for brave souls with \"experimental\"\n> bit on.\n>\n> I wonder if we perhaps wnat to add to the documentation for\n> writeCommitGraph configuration that its use is currently not\n> recommended in a shallow clone or something (I know it is not\n> a problem just to use it with shallow but the breakage needs\n> to involve unshallowing, but by definition those who do not\n> use shallow would not hit the unshallowing bug, so...).\n\nI think this is a good direction if you don't want to take the patch I\nsent in [1] for v2.28.0. If you do, though, I don't think that this\nwould be necessary.\n\n> > Reported-by: Jay Conrod \n> > Helped-by: Taylor Blau \n> > Signed-off-by: Jonathan Nieder \n> > ---\n> > I realize this is late to send. That said, as described above, I\n> > think it's a good way to buy time by minimizing user exposure to\n> > fetch.writeCommitGraph=true until a fix for it is well cooked.\n> >\n> > In other words, I'd like to see this patch in Git 2.28-rc0.\n>\n> Yes, I do, too.\n>\n> Thanks.\n>\n> > diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> > index b1a9b1461d3..b20394038d1 100644\n> > --- a/Documentation/config/fetch.txt\n> > +++ b/Documentation/config/fetch.txt\n> > @@ -90,5 +90,4 @@ fetch.writeCommitGraph::\n> > \tthe existing commit-graph file(s). Occasionally, these files will\n> > \tmerge and the write may take longer. Having an updated commit-graph\n> > \tfile helps performance of many Git commands, including `git merge-base`,\n> > -\t`git push -f`, and `git log --graph`. Defaults to false, unless\n> > -\t`feature.experimental` is true.\n> > +\t`git push -f`, and `git log --graph`. Defaults to false.\n> > diff --git a/repo-settings.c b/repo-settings.c\n> > index dc6817daa95..0918408b344 100644\n> > --- a/repo-settings.c\n> > +++ b/repo-settings.c\n> > @@ -51,14 +51,14 @@ void prepare_repo_settings(struct repository *r)\n> > \t\tUPDATE_DEFAULT_BOOL(r->settings.index_version, 4);\n> > \t\tUPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);\n> > \t}\n> > +\n> > \tif (!repo_config_get_bool(r, \"fetch.writecommitgraph\", &value))\n> > \t\tr->settings.fetch_write_commit_graph = value;\n> > -\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value) {\n> > -\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n> > -\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);\n> > -\t}\n> > \tUPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);\n> >\n> > +\tif (!repo_config_get_bool(r, \"feature.experimental\", &value) && value)\n> > +\t\tUPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);\n> > +\n> > \t/* Hack for test programs like test-dump-untracked-cache */\n> > \tif (ignore_untracked_cache_config)\n> > \t\tr->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/20200707144338.GA26342@syl.lan/T/#t\n"},{"id":"401133","messageId":"xmqqeepn5kxz.fsf@gitster.c.googlers.com","threadId":"53806","inReplyTo":"20200707151735.GA27992@syl.lan","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T16:50:00Z","receivedAt":"2020-07-07T16:50:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> I wonder if we perhaps wnat to add to the documentation for\n>> writeCommitGraph configuration that its use is currently not\n>> recommended in a shallow clone or something (I know it is not\n>> a problem just to use it with shallow but the breakage needs\n>> to involve unshallowing, but by definition those who do not\n>> use shallow would not hit the unshallowing bug, so...).\n>\n> I think this is a good direction if you don't want to take the patch I\n> sent in [1] for v2.28.0. If you do, though, I don't think that this\n> would be necessary.\n\nGood timing. I didn't know a \"fix\" was already being worked on ([1]\nis the patch from this morning, right? I haven't seen it except for\nits subject).\n\nWe could obviously do both excluding it from the usual experimental\nset and applying your fix, so that those who are really curious can\nhelp us make sure your fix would be all that is needed. Let's see\nwhat Jonathan says...\n\nThanks.\n"},{"id":"401134","messageId":"20200707165342.GB36941@syl.lan","threadId":"53806","inReplyTo":"xmqqeepn5kxz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-07T16:53:42Z","receivedAt":"2020-07-07T16:53:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 07, 2020 at 09:50:00AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> I wonder if we perhaps wnat to add to the documentation for\n> >> writeCommitGraph configuration that its use is currently not\n> >> recommended in a shallow clone or something (I know it is not\n> >> a problem just to use it with shallow but the breakage needs\n> >> to involve unshallowing, but by definition those who do not\n> >> use shallow would not hit the unshallowing bug, so...).\n> >\n> > I think this is a good direction if you don't want to take the patch I\n> > sent in [1] for v2.28.0. If you do, though, I don't think that this\n> > would be necessary.\n>\n> Good timing. I didn't know a \"fix\" was already being worked on ([1]\n> is the patch from this morning, right? I haven't seen it except for\n> its subject).\n\n[1] is the fix. Jonathan wrote it a month or so ago, I just added a test\non top. (Independently, I tested it with the reproduction in the\noriginal bug report, and it worked properly).\n\n> We could obviously do both excluding it from the usual experimental\n> set and applying your fix, so that those who are really curious can\n> help us make sure your fix would be all that is needed. Let's see\n> what Jonathan says...\n\nEither of those sound good to me.\n\n> Thanks.\n\nThanks,\nTaylor\n"},{"id":"401180","messageId":"20200708054725.GB118756@google.com","threadId":"53806","inReplyTo":"xmqqeepn5kxz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] experimental: default to fetch.writeCommitGraph=false","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-08T05:47:25Z","receivedAt":"2020-07-08T05:47:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> We could obviously do both excluding it from the usual experimental\n> set and applying your fix, so that those who are really curious can\n> help us make sure your fix would be all that is needed. Let's see\n> what Jonathan says...\n\nYes, that would be my preference.\n\nThat is, both:\n\n* applying the fix to provide a good experience to users interested in\n fetch.writeCommitGraph\n\n* disabling fetch.writeCommitGraph in the experimental set, since it\n has not had much production exposure yet. The experimental set is\n relatively young, so I want to ensure people's initial experiences\n with it are positive so that they stick with it if they're\n interested in experimental features (or in other words, I think\n there's still a place for features that are not yet proven enough\n to go in the experimental set).\n\nRegardless of what is put in the experimental set, at $DAYJOB we will\nrun with the fix applied and with fetch.writeCommitGraph=true starting\nnext week. I'd encourage anyone else with a similarly controlled\nsetup to try the same.\n\nThanks,\nJonathan\n"}]}