{"thread":{"id":"44162","subject":"[PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","startedAt":"2016-09-26T19:36:22Z","lastAt":"2016-09-26T22:18:12Z","messageCount":6,"participants":["Stefan Beller","Junio C Hamano","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302623","messageId":"20160926193553.32631-1-sbeller@google.com","threadId":"44162","inReplyTo":null,"subject":"[PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-26T19:35:53Z","receivedAt":"2016-09-26T19:36:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Currently the section about recursing into submodules is repeated in\ngit-pull word for word as it is in fetch-options.\n\nDon't repeat ourselves here and include the --recurse-submodules via\nfetch options.\n\nAs a bonus expose the --jobs parameter in git-pull as well as that is\ndeclared as a OPT_PASSTHRU for fetch internally already.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/fetch-options.txt | 2 ++\n Documentation/git-pull.txt      | 9 ---------\n 2 files changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt\nindex 9eab1f5..352b640 100644\n--- a/Documentation/fetch-options.txt\n+++ b/Documentation/fetch-options.txt\n@@ -89,6 +89,7 @@ ifndef::git-pull[]\n \toption alone does not subject tags to pruning, even if --prune\n \tis used (though tags may be pruned anyway if they are also the\n \tdestination of an explicit refspec; see `--prune`).\n+endif::git-pull[]\n \n --recurse-submodules[=yes|on-demand|no]::\n \tThis option controls if and under what conditions new commits of\n@@ -108,6 +109,7 @@ ifndef::git-pull[]\n \tsubmodules will be faster. By default submodules will be fetched\n \tone at a time.\n \n+ifndef::git-pull[]\n --no-recurse-submodules::\n \tDisable recursive fetching of submodules (this has the same effect as\n \tusing the `--recurse-submodules=no` option).\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex d033b25..e4cd56a 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -84,15 +84,6 @@ OPTIONS\n --verbose::\n \tPass --verbose to git-fetch and git-merge.\n \n---[no-]recurse-submodules[=yes|on-demand|no]::\n-\tThis option controls if new commits of all populated submodules should\n-\tbe fetched too (see linkgit:git-config[1] and linkgit:gitmodules[5]).\n-\tThat might be necessary to get the data needed for merging submodule\n-\tcommits, a feature Git learned in 1.7.3. Notice that the result of a\n-\tmerge will not be checked out in the submodule, \"git submodule update\"\n-\thas to be called afterwards to bring the work tree up to date with the\n-\tmerge result.\n-\n Options related to merging\n ~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n2.10.0.129.g35f6318\n\n"},{"id":"302633","messageId":"xmqq7f9yqu9w.fsf@gitster.mtv.corp.google.com","threadId":"44162","inReplyTo":"20160926193553.32631-1-sbeller@google.com","subject":"Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T20:29:47Z","receivedAt":"2016-09-26T20:29:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Currently the section about recursing into submodules is repeated in\n> git-pull word for word as it is in fetch-options.\n>\n> Don't repeat ourselves here and include the --recurse-submodules via\n> fetch options.\n>\n> As a bonus expose the --jobs parameter in git-pull as well as that is\n> declared as a OPT_PASSTHRU for fetch internally already.\n\nThe above may not be technically wrong, but smells like an\nunder-researched description.\n\nIOW, \"Why did the commit that introduced the option described it\nthis way in the first place?  Was there a specific reason why it had\nto be that way, and that reason is no longer with us now, which\nmakes this change safe?\" is a very natural question somebody who\nsees this patch, and your description does not answer it.\n\nIt seems that the option to recurse into submodules was added by\nJens's 7dce19d3 (\"fetch/pull: Add the --recurse-submodules option\",\n2010-11-12) to both fetch and pull at the same time.  I suspected\nperhaps we hid it from pull initially while describing it for fetch,\nbut that does not seem to be the case, and back at that version,\npull and fetch shared the description without duplicating.\n\nSo where did we go wrong?  Was there a good reason why we have two\ninstances of these option descriptions, and if so, are we sure that\nthat reason is no longer applicable to today's system that we can\nsafely share the description?  The proposed log message is a place\nto answer these questions.\n\nBy the way, 7dce19d3 is interesting in another way and worth\nstudying in that it adds --submodule-prefix ;-) It may be something\nwe want to consider consolidating with what Brandon has been working\non.\n\nBy the way^2, the \"unconditionally\" on the title conveyed less\ninformation than their bits weigh.  Unless a reader knows\nfetch-options are shared between fetch and pull, s/he would not know\nyou meant by \"unconditionally\" to show these in both fetch and pull.\n\n    Documentation: share more descriptions for options between fetch and pull\n\nperhaps?\n\nThanks.\n"},{"id":"302634","messageId":"CAGZ79kbeq+Wznm=ChWO0tU5A_haPQ6DvKNHbK+8Y3es5OVcRag@mail.gmail.com","threadId":"44162","inReplyTo":"xmqq7f9yqu9w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-26T20:44:19Z","receivedAt":"2016-09-26T20:44:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 26, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Currently the section about recursing into submodules is repeated in\n>> git-pull word for word as it is in fetch-options.\n>>\n>> Don't repeat ourselves here and include the --recurse-submodules via\n>> fetch options.\n>>\n>> As a bonus expose the --jobs parameter in git-pull as well as that is\n>> declared as a OPT_PASSTHRU for fetch internally already.\n>\n> The above may not be technically wrong, but smells like an\n> under-researched description.\n\nI knew this question was coming, but the research was not as easy to\nfollow as it seemed convoluted to me.\n\nAfter a bit more research, I think 8f0700dd33f (fetch/pull: Add the\n'on-demand' value to the --recurse-submodules option) is the culprit,\nwhere this patch should have been squashed into, as that made the\nboth locations word for word equal.\n\n>\n> So where did we go wrong?  Was there a good reason why we have two\n> instances of these option descriptions, and if so, are we sure that\n> that reason is no longer applicable to today's system that we can\n> safely share the description?  The proposed log message is a place\n> to answer these questions.\n\nThe commit above is where we went wrong; It doesn't seem like a good\nreason for not including this is given in there.\n\n>\n> By the way, 7dce19d3 is interesting in another way and worth\n> studying in that it adds --submodule-prefix ;-) It may be something\n> we want to consider consolidating with what Brandon has been working\n> on.\n\nThat's why Brandon is cc'd now. :)\n\n>\n> By the way^2, the \"unconditionally\" on the title conveyed less\n> information than their bits weigh.  Unless a reader knows\n> fetch-options are shared between fetch and pull, s/he would not know\n> you meant by \"unconditionally\" to show these in both fetch and pull.\n>\n>     Documentation: share more descriptions for options between fetch and pull\n>\n> perhaps?\n\nThat's way better.\nI'll resend shortly.\n\nThanks,\nStefan\n"},{"id":"302636","messageId":"xmqq37kmqt4a.fsf@gitster.mtv.corp.google.com","threadId":"44162","inReplyTo":"CAGZ79kbeq+Wznm=ChWO0tU5A_haPQ6DvKNHbK+8Y3es5OVcRag@mail.gmail.com","subject":"Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-26T20:54:45Z","receivedAt":"2016-09-26T20:54:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> After a bit more research, I think 8f0700dd33f (fetch/pull: Add the\n> 'on-demand' value to the --recurse-submodules option) is the culprit,\n> where this patch should have been squashed into, as that made the\n> both locations word for word equal.\n\nHmph, my digging points to elsewhere.  7811d960 (\"pull: Document the\n\"--[no-]recurse-submodules\" options\", 2011-02-07) which is older\nthan 8f0700dd (\"fetch/pull: Add the 'on-demand' value to the\n--recurse-submodules option\", 2011-03-06) seems to be the real\nchange that pulled the description of recurse-submodules made in\nfetch-options into \"show this only when we are not describing pull\".\n\nUnfortunately it is not clear why we actively wanted to be sketchier\nwhen showing \"git help fetch\"; otherwise the change would have been\nmade to the existing description there without adding a new entry to\n\"git-pull.txt\".\n\n"},{"id":"302638","messageId":"CAGZ79kZmy=sx5Awh4W5gq=sW6E7HTRVOiZak834brftENZSyJw@mail.gmail.com","threadId":"44162","inReplyTo":"xmqq37kmqt4a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-26T21:14:25Z","receivedAt":"2016-09-26T21:14:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 26, 2016 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> After a bit more research, I think 8f0700dd33f (fetch/pull: Add the\n>> 'on-demand' value to the --recurse-submodules option) is the culprit,\n>> where this patch should have been squashed into, as that made the\n>> both locations word for word equal.\n>\n> Hmph, my digging points to elsewhere.  7811d960 (\"pull: Document the\n> \"--[no-]recurse-submodules\" options\", 2011-02-07)\n\nThat commit seems like it want to intentionally keep it different for\nfetch and pull\n(otherwise the fetch-options.txt would have been reworded there).\n\nRereading the actual option descriptions, I realize they are different.\n(Initially I used a diff tool to see if there is aminor difference, and I was\nsurprised they were word for word identical; It must have been a mistake\non copying one of the option texts)\n\nThe git-pull part actually conveys pull specific information, so let's drop\nthis patch entirely.\n\n> which is older\n> than 8f0700dd (\"fetch/pull: Add the 'on-demand' value to the\n> --recurse-submodules option\", 2011-03-06) seems to be the real\n> change that pulled the description of recurse-submodules made in\n> fetch-options into \"show this only when we are not describing pull\".\n>\n> Unfortunately it is not clear why we actively wanted to be sketchier\n> when showing \"git help fetch\"; otherwise the change would have been\n> made to the existing description there without adding a new entry to\n> \"git-pull.txt\".\n>\n"},{"id":"302646","messageId":"20160926221803.GA10222@google.com","threadId":"44162","inReplyTo":"CAGZ79kbeq+Wznm=ChWO0tU5A_haPQ6DvKNHbK+8Y3es5OVcRag@mail.gmail.com","subject":"Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-26T22:18:03Z","receivedAt":"2016-09-26T22:18:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"> > By the way, 7dce19d3 is interesting in another way and worth\n> > studying in that it adds --submodule-prefix ;-) It may be something\n> > we want to consider consolidating with what Brandon has been working\n> > on.\n> \n> That's why Brandon is cc'd now. :)\n\nInteresting.  Once we get something we agree on for adding the\n--submodule-prefix option to the top level we'll definitely have to\nupdate this section of code with the change.\n\n-- \nBrandon Williams\n"}]}