{"thread":{"id":"51791","subject":"[PATCH 0/1] Write commit-graph on fetch","startedAt":"2019-09-03T02:22:04Z","lastAt":"2019-09-09T17:53:27Z","messageCount":18,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"381703","messageId":"pull.328.git.gitgitgadget@gmail.com","threadId":"51791","inReplyTo":null,"subject":"[PATCH 0/1] Write commit-graph on fetch","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-03T02:22:01Z","receivedAt":"2019-09-03T02:22:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Instead of waiting for a non-trivial GC command to write the commit-graph,\nwrite one during 'git fetch'. By using the equivalent method for 'git\ncommit-graph write --split --reachable', we create a commit-graph chain that\nincludes all reachable commits. Most of the time, these writes only add the\nnewly-downloaded commits in a small file at the top of the commit-graph\nchain. Commits that are no longer reachable still exist in the commit-graph,\nbut will be cleaned up by a later GC command that forces the commit-graph to\nbe rewritten completely.\n\nA version of this patch [1] used to be a part of\nds/commit-graph-incremental, but was removed to focus on the incremental\ncommit-graph file format. Now that the incremental file format has been\nshipped in v2.23.0 and some config things have adjusted in\nds/feature-macros, I'm reintroducing the idea.\n\nÆvar had mentioned wanting to do something with \"incremental maintenance\nduring GC\" [2]. I haven't seen any patches towards that aim (please point me\nin that direction if they have been submitted). I still think it is worth\nallowing a write at fetch time, as some users have GC disabled. I know for\nsure that users who only interact with their Git repos via Visual Studio\nTeam Explorer have all Git commands running with GC disabled, and likely\nother desktop GUI clients have it disabled to avoid blocking processes.\n\nAside: VFS for Git users have GC disabled, but the commit-graph is being\nwritten in the background by a monitoring process. We shipped the\nincremental commit-graph writes in a recent version and reduced our writes\nfrom ~60 seconds each to less than a second on average. Very rarely, the\nlayers of the commit-graph chain collapse and return to the old values. This\nfeature has been performing well with no known issues.\n\nThanks, -Stolee\n\n[1] \nhttps://public-inbox.org/git/3c52385e5696887c40cab4a6b9b7923d60a0567c.1557330827.git.gitgitgadget@gmail.com/\n\n[2] \nhttps://public-inbox.org/git/b1de6af2-c015-098e-a656-e1b68056e037@gmail.com/\n\nDerrick Stolee (1):\n  fetch: add fetch.writeCommitGraph config setting\n\n Documentation/config/feature.txt |  8 ++++++++\n Documentation/config/fetch.txt   | 10 ++++++++++\n builtin/fetch.c                  | 15 +++++++++++++++\n repo-settings.c                  |  4 ++++\n repository.h                     |  1 +\n t/t5510-fetch.sh                 | 13 +++++++++++++\n 6 files changed, 51 insertions(+)\n\n\nbase-commit: aaf633c2ad10b47af7623c130ddfe7231658c7e4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-328%2Fderrickstolee%2Ffetch-write-commit-graph-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-328/derrickstolee/fetch-write-commit-graph-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/328\n-- \ngitgitgadget\n"},{"id":"381704","messageId":"49f877c85ca2be5bb76d9082ee4aa26e26111a14.1567477320.git.gitgitgadget@gmail.com","threadId":"51791","inReplyTo":"pull.328.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-09-03T02:22:02Z","receivedAt":"2019-09-03T02:22:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe commit-graph feature is now on by default, and is being\nwritten during 'git gc' by default. Typically, Git only writes\na commit-graph when a 'git gc --auto' command passes the gc.auto\nsetting to actualy do work. This means that a commit-graph will\ntypically fall behind the commits that are being used every day.\n\nTo stay updated with the latest commits, add a step to 'git\nfetch' to write a commit-graph after fetching new objects. The\nfetch.writeCommitGraph config setting enables writing a split\ncommit-graph, so on average the cost of writing this file is\nvery small. Occasionally, the commit-graph chain will collapse\nto a single level, and this could be slow for very large repos.\n\nFor additional use, adjust the default to be true when\nfeature.experimental is enabled.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n Documentation/config/feature.txt |  8 ++++++++\n Documentation/config/fetch.txt   | 10 ++++++++++\n builtin/fetch.c                  | 15 +++++++++++++++\n repo-settings.c                  |  4 ++++\n repository.h                     |  1 +\n t/t5510-fetch.sh                 | 13 +++++++++++++\n 6 files changed, 51 insertions(+)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex 545522f306..875f8c8a66 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -17,6 +17,14 @@ which can improve `git push` performance in repos with many files.\n +\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 feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex d402110638..e8cb20547c 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -69,3 +69,13 @@ fetch.showForcedUpdates::\n \tSet to false to enable `--no-show-forced-updates` in\n \tlinkgit:git-fetch[1] and linkgit:git-pull[1] commands.\n \tDefaults to true.\n+\n+fetch.writeCommitGraph::\n+\tSet to true to write a commit-graph after every `git fetch` command\n+\tthat downloads a pack-file from a remote. Using the `--split` option,\n+\tmost executions will create a very small commit-graph file on top of\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.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 53ce99d2bb..d36a403859 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -23,6 +23,7 @@\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n #include \"commit-reach.h\"\n+#include \"commit-graph.h\"\n \n #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n \n@@ -1715,6 +1716,20 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tstring_list_clear(&list, 0);\n \n+\tprepare_repo_settings(the_repository);\n+\tif (the_repository->settings.fetch_write_commit_graph) {\n+\t\tint commit_graph_flags = COMMIT_GRAPH_SPLIT;\n+\t\tstruct split_commit_graph_opts split_opts;\n+\t\tmemset(&split_opts, 0, sizeof(struct split_commit_graph_opts));\n+\n+\t\tif (progress)\n+\t\t\tcommit_graph_flags |= COMMIT_GRAPH_PROGRESS;\n+\n+\t\twrite_commit_graph_reachable(get_object_directory(),\n+\t\t\t\t\t     commit_graph_flags,\n+\t\t\t\t\t     &split_opts);\n+\t}\n+\n \tclose_object_store(the_repository->objects);\n \n \tif (enable_auto_gc) {\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 3779b85c17..05546db98e 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -49,10 +49,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+\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.pack_use_sparse, 1);\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 \t/* Hack for test programs like test-dump-untracked-cache */\n \tif (ignore_untracked_cache_config)\ndiff --git a/repository.h b/repository.h\nindex 4da275e73f..fe0b5f5dc6 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -30,6 +30,7 @@ struct repo_settings {\n \n \tint core_commit_graph;\n \tint gc_write_commit_graph;\n+\tint fetch_write_commit_graph;\n \n \tint index_version;\n \tenum untracked_cache_setting core_untracked_cache;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 139f7106f7..91ede622f0 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -570,6 +570,19 @@ test_expect_success 'LHS of refspec follows ref disambiguation rules' '\n \t)\n '\n \n+test_expect_success 'fetch.writeCommitGraph' '\n+\tgit clone three write &&\n+\t(\n+\t\tcd three &&\n+\t\ttest_commit new\n+\t) &&\n+\t(\n+\t\tcd write &&\n+\t\tgit -c fetch.writeCommitGraph fetch origin &&\n+\t\ttest_path_is_file .git/objects/info/commit-graphs/commit-graph-chain\n+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \ngitgitgadget\n"},{"id":"381757","messageId":"xmqqef0xtd3p.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"49f877c85ca2be5bb76d9082ee4aa26e26111a14.1567477320.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-03T19:05:14Z","receivedAt":"2019-09-03T19:05:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 53ce99d2bb..d36a403859 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -23,6 +23,7 @@\n>  #include \"packfile.h\"\n>  #include \"list-objects-filter-options.h\"\n>  #include \"commit-reach.h\"\n> +#include \"commit-graph.h\"\n>  \n>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n>  \n> @@ -1715,6 +1716,20 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \n>  \tstring_list_clear(&list, 0);\n>  \n> +\tprepare_repo_settings(the_repository);\n> +\tif (the_repository->settings.fetch_write_commit_graph) {\n> +\t\tint commit_graph_flags = COMMIT_GRAPH_SPLIT;\n> +\t\tstruct split_commit_graph_opts split_opts;\n> +\t\tmemset(&split_opts, 0, sizeof(struct split_commit_graph_opts));\n> +\n> +\t\tif (progress)\n> +\t\t\tcommit_graph_flags |= COMMIT_GRAPH_PROGRESS;\n> +\n> +\t\twrite_commit_graph_reachable(get_object_directory(),\n> +\t\t\t\t\t     commit_graph_flags,\n> +\t\t\t\t\t     &split_opts);\n> +\t}\n\nAs a low-impact change this is good.  \n\nFor longer term, it feels a bit unfortunate that this is still a\nseparate phase of the program, though.  We know what new refs we\nadded, we know what new objects we received, and we even scanned\neach and every one of them while running the index-pack step to\nstore the .pack and compute the .idx file, i.e. it feels that we\nhave most of the information already in-core to extend the commit\ngraph for new parts of the history we just received.\n\nThanks.\n\n"},{"id":"381779","messageId":"d550ac23-cb65-b547-d9dc-1428ee2e9420@gmail.com","threadId":"51791","inReplyTo":"xmqqef0xtd3p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-03T23:36:48Z","receivedAt":"2019-09-03T23:36:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/3/2019 3:05 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 53ce99d2bb..d36a403859 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -23,6 +23,7 @@\n>>  #include \"packfile.h\"\n>>  #include \"list-objects-filter-options.h\"\n>>  #include \"commit-reach.h\"\n>> +#include \"commit-graph.h\"\n>>  \n>>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)\n>>  \n>> @@ -1715,6 +1716,20 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>>  \n>>  \tstring_list_clear(&list, 0);\n>>  \n>> +\tprepare_repo_settings(the_repository);\n>> +\tif (the_repository->settings.fetch_write_commit_graph) {\n>> +\t\tint commit_graph_flags = COMMIT_GRAPH_SPLIT;\n>> +\t\tstruct split_commit_graph_opts split_opts;\n>> +\t\tmemset(&split_opts, 0, sizeof(struct split_commit_graph_opts));\n>> +\n>> +\t\tif (progress)\n>> +\t\t\tcommit_graph_flags |= COMMIT_GRAPH_PROGRESS;\n>> +\n>> +\t\twrite_commit_graph_reachable(get_object_directory(),\n>> +\t\t\t\t\t     commit_graph_flags,\n>> +\t\t\t\t\t     &split_opts);\n>> +\t}\n> \n> As a low-impact change this is good.  \n> \n> For longer term, it feels a bit unfortunate that this is still a\n> separate phase of the program, though.  We know what new refs we\n> added, we know what new objects we received, and we even scanned\n> each and every one of them while running the index-pack step to\n> store the .pack and compute the .idx file, i.e. it feels that we\n> have most of the information already in-core to extend the commit\n> graph for new parts of the history we just received.\n\nYou're right that we could isolate the new write to the refs we\njust received. We could use the more cumbersome write_commit_graph()\nmethod with a list of commit oids as starting points. I'm happy to\nmake that change if we see a lot of value there.\n\nHowever, the current patch also gives us a way to add local refs\nto the commit graph and \"catch up\" to the work the user had done.\nWith this in mind, I do think the simpler code has another functional\nadvantage.\n\nThanks,\n-Stolee\n\n"},{"id":"381787","messageId":"20190904030829.GB28836@sigill.intra.peff.net","threadId":"51791","inReplyTo":"49f877c85ca2be5bb76d9082ee4aa26e26111a14.1567477320.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-04T03:08:30Z","receivedAt":"2019-09-04T03:08:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2019 at 07:22:02PM -0700, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> The commit-graph feature is now on by default, and is being\n> written during 'git gc' by default. Typically, Git only writes\n> a commit-graph when a 'git gc --auto' command passes the gc.auto\n> setting to actualy do work. This means that a commit-graph will\n> typically fall behind the commits that are being used every day.\n> \n> To stay updated with the latest commits, add a step to 'git\n> fetch' to write a commit-graph after fetching new objects. The\n> fetch.writeCommitGraph config setting enables writing a split\n> commit-graph, so on average the cost of writing this file is\n> very small. Occasionally, the commit-graph chain will collapse\n> to a single level, and this could be slow for very large repos.\n> \n> For additional use, adjust the default to be true when\n> feature.experimental is enabled.\n\nSeems like a good time to do it. We'd eventually want a similar option\nfor updating it on the receiving side of a push, too. I don't insist\nthat come at the same time, but we should at least have a plan about how\nit will look to the user.\n\nDo we want to to have fetch.writeCommitGraph, receive.writeCommitGraph,\nand then a master transfer.writeCommitGraph?\n\n-Peff\n"},{"id":"381915","messageId":"xmqqa7bipjil.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"20190904030829.GB28836@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T20:37:06Z","receivedAt":"2019-09-05T20:37:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Do we want to to have fetch.writeCommitGraph, receive.writeCommitGraph,\n> and then a master transfer.writeCommitGraph?\n\nIf anything, it may be good for consistency.\n\nI am not sure if it is a good idea to trigger writing the commit\ngraph when accepting a push, though.  It tends to be a lot finer\ngrained than fetching, right?\n"},{"id":"381967","messageId":"a697c4de-e269-c330-ece2-8ac7007e6c55@gmail.com","threadId":"51791","inReplyTo":"xmqqa7bipjil.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T17:00:40Z","receivedAt":"2019-09-06T17:00:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/5/2019 4:37 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> Do we want to to have fetch.writeCommitGraph, receive.writeCommitGraph,\n>> and then a master transfer.writeCommitGraph?\n> \n> If anything, it may be good for consistency.\n> \n> I am not sure if it is a good idea to trigger writing the commit\n> graph when accepting a push, though.  It tends to be a lot finer\n> grained than fetching, right?\n\nAnd I expect a push to include many fewer commits than a fetch.\nIn a server environment, I would expect to have a separate\nmaintenance task responsible for updating the commit-graph after\nreceiving new data, but not in an in-line fashion with the push.\n\nThink about the situation of many pushes that happen in a short\nburst: one write after all of the pushes would have close to the\nsame performance benefits as writing with every push, but does\na lot less work.\n\nThanks,\n-Stolee\n"},{"id":"381979","messageId":"20190906175628.GH23181@sigill.intra.peff.net","threadId":"51791","inReplyTo":"a697c4de-e269-c330-ece2-8ac7007e6c55@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:56:28Z","receivedAt":"2019-09-06T17:56:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 01:00:40PM -0400, Derrick Stolee wrote:\n\n> On 9/5/2019 4:37 PM, Junio C Hamano wrote:\n> > Jeff King <peff@peff.net> writes:\n> > \n> >> Do we want to to have fetch.writeCommitGraph, receive.writeCommitGraph,\n> >> and then a master transfer.writeCommitGraph?\n> > \n> > If anything, it may be good for consistency.\n> > \n> > I am not sure if it is a good idea to trigger writing the commit\n> > graph when accepting a push, though.  It tends to be a lot finer\n> > grained than fetching, right?\n> \n> And I expect a push to include many fewer commits than a fetch.\n> In a server environment, I would expect to have a separate\n> maintenance task responsible for updating the commit-graph after\n> receiving new data, but not in an in-line fashion with the push.\n\nThat's probably how we'll end up doing it at GitHub, because we run a\nbig server farm that has a job-queueing system for periodic maintenance,\netc.\n\nBut keep in mind the \"little guy\" who is hosting a few repositories for\nthemselves or their company. They'd presumably want the benefits here,\ntoo, right? We already have options to trigger auto-gc and\nupdate-server-info for them, so why not this maintenance, too?\n\n> Think about the situation of many pushes that happen in a short\n> burst: one write after all of the pushes would have close to the\n> same performance benefits as writing with every push, but does\n> a lot less work.\n\nSure, but wouldn't that similarly apply to fetching? What is it that\nmakes bursts of pushes more likely than bursts of fetches?\n\n-Peff\n"},{"id":"381980","messageId":"xmqq1rwtnv03.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"20190906175628.GH23181@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T18:24:12Z","receivedAt":"2019-09-06T18:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Sure, but wouldn't that similarly apply to fetching? What is it that\n> makes bursts of pushes more likely than bursts of fetches?\n\nBecause people tend to use a repository as a gathering point?  You\nmay periodically fetch from and push to a repository, and you may\neven do so at the same interval, but simply because there are more\n\"other\" people than you alone as a single developer in the project,\nyour fetch tends to grab work from more people than yoru push that\npublish your work alone?\n"},{"id":"381982","messageId":"20190906191622.GA7096@sigill.intra.peff.net","threadId":"51791","inReplyTo":"xmqq1rwtnv03.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T19:16:22Z","receivedAt":"2019-09-06T19:16:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 11:24:12AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Sure, but wouldn't that similarly apply to fetching? What is it that\n> > makes bursts of pushes more likely than bursts of fetches?\n> \n> Because people tend to use a repository as a gathering point?  You\n> may periodically fetch from and push to a repository, and you may\n> even do so at the same interval, but simply because there are more\n> \"other\" people than you alone as a single developer in the project,\n> your fetch tends to grab work from more people than yoru push that\n> publish your work alone?\n\nI suppose so. But I think the \"stock git without any other job\ninfrastructure\" case would still benefit. How do we know when the burst\nis done? We'd effectively be relying on auto-gc to do that, but \"enough\npacks to merit gc\" and \"burst is done, now is a good time to update the\ncommit graph\" are two different metrics.\n\n-Peff\n"},{"id":"381985","messageId":"xmqqpnkdma0g.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"20190906191622.GA7096@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T20:42:55Z","receivedAt":"2019-09-06T20:43:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suppose so. But I think the \"stock git without any other job\n> infrastructure\" case would still benefit.\n\nOh, no question about it.\n\nI did question if an automatic writing would benefit the side that\nreceives a push when you brought up the usual \"fetch.* and receive.*\nfor separate configuration, transfer.* when want to rule them both\"\nconvention, which is a good idea if only for consistency, but the\nquestion was if it helps the receiving end of a push to the same\ndegree as it would help the repository that fetches.\n"},{"id":"381986","messageId":"25ed1562-dc74-1ebc-46ff-d6cd643504a4@gmail.com","threadId":"51791","inReplyTo":"xmqqpnkdma0g.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T21:04:17Z","receivedAt":"2019-09-06T21:04:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2019 4:42 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> I suppose so. But I think the \"stock git without any other job\n>> infrastructure\" case would still benefit.\n> \n> Oh, no question about it.\n> \n> I did question if an automatic writing would benefit the side that\n> receives a push when you brought up the usual \"fetch.* and receive.*\n> for separate configuration, transfer.* when want to rule them both\"\n> convention, which is a good idea if only for consistency, but the\n> question was if it helps the receiving end of a push to the same\n> degree as it would help the repository that fetches.\n\nI think the \"stock git without any other job infrastructure\" is\na very important scenario. Putting the simplest version of\n\"commit-graph writes in-line with every push\" seems to be ripe\nfor failure under load. I'd rather think deeply about what is\nbest for this scenario.\n\nSpit-balling: what if we used the same mechanism as running GC\nin the background to launch 'git commit-graph write' commands?\nAnd we could have the command give up (without failure) if the\ncommit-graph.lock file is already created, so concurrent pushes\nwould not fight each other.\n\nThanks,\n-Stolee\n"},{"id":"381987","messageId":"xmqqlfv1m72p.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"d550ac23-cb65-b547-d9dc-1428ee2e9420@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T21:46:22Z","receivedAt":"2019-09-06T21:46:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>>> +\t\twrite_commit_graph_reachable(get_object_directory(),\n>>> +\t\t\t\t\t     commit_graph_flags,\n>>> +\t\t\t\t\t     &split_opts);\n>>> +\t}\n>> \n>> As a low-impact change this is good.  \n>> \n>> For longer term, it feels a bit unfortunate that this is still a\n>> separate phase of the program, though.  We know what new refs we\n>> added, we know what new objects we received, and we even scanned\n>> each and every one of them while running the index-pack step to\n>> store the .pack and compute the .idx file, i.e. it feels that we\n>> have most of the information already in-core to extend the commit\n>> graph for new parts of the history we just received.\n>\n> You're right that we could isolate the new write to the refs we\n> just received. We could use the more cumbersome write_commit_graph()\n> method with a list of commit oids as starting points. I'm happy to\n> make that change if we see a lot of value there.\n\nWell, that is not the kind of information reuse I am talking about.\n\nI was wondering if \"index-pack\" has enough information in-core after\nit receives and processes the incoming pack data, scanned each and\nevery object in it, in order to write out the commit graph _without_\nhaving to do a lot of duplicate computation and enumeration of the\nobjects done in the current commit-graph.c::write_commit_graph(), so\nthat it can learn a \"--write-commit-graph\" option that performs much\nbetter than running \"git fetch && git commit-graph write\".\n\nPerhaps that would require too much refactoring of both index-pack\nand commit-graph code and infeasible in short to medium term and\nthat is why I said \"for longer term, it feels a bit unfortunate\".\n\nThanks.\n"},{"id":"381988","messageId":"xmqqh85pm6jg.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"25ed1562-dc74-1ebc-46ff-d6cd643504a4@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T21:57:55Z","receivedAt":"2019-09-06T21:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 9/6/2019 4:42 PM, Junio C Hamano wrote:\n>> Jeff King <peff@peff.net> writes:\n>> \n>>> I suppose so. But I think the \"stock git without any other job\n>>> infrastructure\" case would still benefit.\n>> \n>> Oh, no question about it.\n>> \n>> I did question if an automatic writing would benefit the side that\n>> receives a push when you brought up the usual \"fetch.* and receive.*\n>> for separate configuration, transfer.* when want to rule them both\"\n>> convention, which is a good idea if only for consistency, but the\n>> question was if it helps the receiving end of a push to the same\n>> degree as it would help the repository that fetches.\n>\n> I think the \"stock git without any other job infrastructure\" is\n> a very important scenario. Putting the simplest version of\n> \"commit-graph writes in-line with every push\" seems to be ripe\n> for failure under load. I'd rather think deeply about what is\n> best for this scenario.\n\nAs to what to do on the push side, I suppose we can afford to let it\nsimmer in the back of our heads while moving this topic forward.\nWhether we'd later decide to have receive.writeCommitGraph (in which\ncase we would add transfer.writeCommitGraph, too) or not, this\nchange on the fetch side can independently be used, right?\n\nThanks.\n\n"},{"id":"381993","messageId":"20190907044650.GB24463@sigill.intra.peff.net","threadId":"51791","inReplyTo":"25ed1562-dc74-1ebc-46ff-d6cd643504a4@gmail.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T04:46:50Z","receivedAt":"2019-09-07T04:46:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 05:04:17PM -0400, Derrick Stolee wrote:\n\n> On 9/6/2019 4:42 PM, Junio C Hamano wrote:\n> > Jeff King <peff@peff.net> writes:\n> > \n> >> I suppose so. But I think the \"stock git without any other job\n> >> infrastructure\" case would still benefit.\n> > \n> > Oh, no question about it.\n> > \n> > I did question if an automatic writing would benefit the side that\n> > receives a push when you brought up the usual \"fetch.* and receive.*\n> > for separate configuration, transfer.* when want to rule them both\"\n> > convention, which is a good idea if only for consistency, but the\n> > question was if it helps the receiving end of a push to the same\n> > degree as it would help the repository that fetches.\n> \n> I think the \"stock git without any other job infrastructure\" is\n> a very important scenario. Putting the simplest version of\n> \"commit-graph writes in-line with every push\" seems to be ripe\n> for failure under load. I'd rather think deeply about what is\n> best for this scenario.\n\nIf it's going to be a problem under load, that makes me worry about the\nsame thing for fetches. Whether you see a lot of either depends on your\nworkflow. But as long as neither option is enabled by default (or at\nleast if it becomes common knowledge to _disable_ them if you have a\nhigh rate), it may be OK.\n\nI do agree that the normal \"busy\" repos you and I have both seen in\nenterprise settings (where people are literally pushing multiple times\nper second at peak) would want some kind of throttling. But I think\nthere's a long tail of \"push once a week\" repos.\n\n> Spit-balling: what if we used the same mechanism as running GC\n> in the background to launch 'git commit-graph write' commands?\n> And we could have the command give up (without failure) if the\n> commit-graph.lock file is already created, so concurrent pushes\n> would not fight each other.\n\nI have mixed feelings. It's nice not to stand in the critical path of\nthe user, but background tasks have a way of finding funny corner cases\n(e.g., packfiles moving around, or the issues we've had with deciding\nwhen to shut down auto-gc for a grace period due to warnings/errors).\n\nI think since this is generally additive (adding new commit-graph\nfiles), it might be less finicky, though.\n\n-Peff\n"},{"id":"381994","messageId":"20190907044717.GC24463@sigill.intra.peff.net","threadId":"51791","inReplyTo":"xmqqh85pm6jg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T04:47:18Z","receivedAt":"2019-09-07T04:47:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 02:57:55PM -0700, Junio C Hamano wrote:\n\n> > I think the \"stock git without any other job infrastructure\" is\n> > a very important scenario. Putting the simplest version of\n> > \"commit-graph writes in-line with every push\" seems to be ripe\n> > for failure under load. I'd rather think deeply about what is\n> > best for this scenario.\n> \n> As to what to do on the push side, I suppose we can afford to let it\n> simmer in the back of our heads while moving this topic forward.\n> Whether we'd later decide to have receive.writeCommitGraph (in which\n> case we would add transfer.writeCommitGraph, too) or not, this\n> change on the fetch side can independently be used, right?\n\nYeah, I'm OK with proceeding without the receive-pack side for now.\n\n-Peff\n"},{"id":"381995","messageId":"20190907045101.GD24463@sigill.intra.peff.net","threadId":"51791","inReplyTo":"xmqqlfv1m72p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T04:51:01Z","receivedAt":"2019-09-07T04:51:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 02:46:22PM -0700, Junio C Hamano wrote:\n\n> > You're right that we could isolate the new write to the refs we\n> > just received. We could use the more cumbersome write_commit_graph()\n> > method with a list of commit oids as starting points. I'm happy to\n> > make that change if we see a lot of value there.\n> \n> Well, that is not the kind of information reuse I am talking about.\n> \n> I was wondering if \"index-pack\" has enough information in-core after\n> it receives and processes the incoming pack data, scanned each and\n> every object in it, in order to write out the commit graph _without_\n> having to do a lot of duplicate computation and enumeration of the\n> objects done in the current commit-graph.c::write_commit_graph(), so\n> that it can learn a \"--write-commit-graph\" option that performs much\n> better than running \"git fetch && git commit-graph write\".\n> \n> Perhaps that would require too much refactoring of both index-pack\n> and commit-graph code and infeasible in short to medium term and\n> that is why I said \"for longer term, it feels a bit unfortunate\".\n\nI think the basic metadata should be easy. We have each commit expanded\nin memory at some point, so parsing it and filing away its parents,\ntrees, etc isn't too hard.\n\nGeneration numbers are a little trickier, though, because they imply an\nactual topological traversal. It might actually be easier to couple this\nwith the connectivity check we do after index-pack finishes (though I've\noften wondered if we could drop that check in favor of making index-pack\nsmarter about finding the boundaries).\n\n-Peff\n"},{"id":"382078","messageId":"xmqqmufdl5kc.fsf@gitster-ct.c.googlers.com","threadId":"51791","inReplyTo":"20190907045101.GD24463@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] fetch: add fetch.writeCommitGraph config setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T17:53:23Z","receivedAt":"2019-09-09T17:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Generation numbers are a little trickier, though, because they imply an\n> actual topological traversal. It might actually be easier to couple this\n> with the connectivity check we do after index-pack finishes (though I've\n> often wondered if we could drop that check in favor of making index-pack\n> smarter about finding the boundaries).\n\nCurrently after scanning the incoming objects with the fsck\nmachinery, we count the number of objects that are pointed at by\nthese objects in the pack and are not in the pack in the\nbuiltin/index-pack.c::check_object() function; at that point, we no\nlonger know who points at the object in question, which is needed if\nwe want to compute the boundary, so we need a bit of work here.\n\nWith boundary information, we could be smarter about lazy fetching,\nI presume?\n"}]}