{"thread":{"id":"44206","subject":"Slow pushes on 'pu' - even when up-to-date..","startedAt":"2016-10-03T21:11:46Z","lastAt":"2016-10-07T12:42:11Z","messageCount":31,"participants":["Linus Torvalds","Stefan Beller","Junio C Hamano","Heiko Voigt","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"303139","messageId":"CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com","threadId":"44206","inReplyTo":null,"subject":"Slow pushes on 'pu' - even when up-to-date..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-10-03T21:11:36Z","receivedAt":"2016-10-03T21:11:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"This seems to be because I'm now on 'pu' as of a day or two ago in\norder to test the abbrev logic, but lookie here:\n\n time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n .. shows all the branches and tags ..\n real 0m0.655s\n user 0m0.011s\n sys 0m0.004s\n\nso the remote is fast to connect to, and with network connection\noverhead and everything, it's just over half a second. But then:\n\n time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n\nand it just sits there, and it's at 100% CPU the whole time, until it says:\n\n Everything up-to-date\n\n real 1m7.307s\n user 1m2.761s\n sys 0m0.475s\n\nWhaa? It took a *minute* of CPU time to decide that everything was up-to-date?\n\nThat's just not right. The branch is entirely up-to-date:\n\n git rev-parse HEAD\n af79ad2b1f337a00aa150b993635b10bc68dc842\n\n git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux master\n af79ad2b1f337a00aa150b993635b10bc68dc842 refs/heads/master\n\nso there should be no need for any history walking. But it sure is\ndoing *something*. A minute of CPU time on my machine is actually a\npretty damn big deal.\n\nLooking at the trace, there's no IO - there's no back-and-forth about\n\"I have this, do you have it?\" or anything like that. The system call\ntrace is just a lot of allocations, which I think means that \"git\npush\" is walking a lot of objects but not doing anything useful.\n\nI bisected it to commit 60cd66f \"push: change submodule default to\ncheck\", which makes little sense since I have no submodules, but there\nyou go.. Apparently RECURSE_SUBMODULES_CHECK is just terminally\nbroken.\n\n Linus\n"},{"id":"303140","messageId":"CAGZ79kYXx+a+U3o6FPBy2mHOy4BxxuF97t6iyedPLk0Qw1Jx1A@mail.gmail.com","threadId":"44206","inReplyTo":"CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-03T21:17:49Z","receivedAt":"2016-10-03T21:17:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 3, 2016 at 2:11 PM, Linus Torvalds\n wrote:\n> This seems to be because I'm now on 'pu' as of a day or two ago in\n> order to test the abbrev logic, but lookie here:\n>\n> time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n> .. shows all the branches and tags ..\n> real 0m0.655s\n> user 0m0.011s\n> sys 0m0.004s\n>\n> so the remote is fast to connect to, and with network connection\n> overhead and everything, it's just over half a second. But then:\n>\n> time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n>\n> and it just sits there, and it's at 100% CPU the whole time, until it says:\n>\n> Everything up-to-date\n>\n> real 1m7.307s\n> user 1m2.761s\n> sys 0m0.475s\n>\n> Whaa? It took a *minute* of CPU time to decide that everything was up-to-date?\n>\n> That's just not right. The branch is entirely up-to-date:\n>\n> git rev-parse HEAD\n> af79ad2b1f337a00aa150b993635b10bc68dc842\n>\n> git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux master\n> af79ad2b1f337a00aa150b993635b10bc68dc842 refs/heads/master\n>\n> so there should be no need for any history walking. But it sure is\n> doing *something*. A minute of CPU time on my machine is actually a\n> pretty damn big deal.\n>\n> Looking at the trace, there's no IO - there's no back-and-forth about\n> \"I have this, do you have it?\" or anything like that. The system call\n> trace is just a lot of allocations, which I think means that \"git\n> push\" is walking a lot of objects but not doing anything useful.\n>\n> I bisected it to commit 60cd66f \"push: change submodule default to\n> check\", which makes little sense since I have no submodules, but there\n> you go.. Apparently RECURSE_SUBMODULES_CHECK is just terminally\n> broken.\n\nSorry for breaking you, too.\n\nJunio complained about that when I was proposing the topic; but now\nthe strategy seems to just wait until Heiko fixed the\nRECURSE_SUBMODULES_CHECK to be less broken.\n\n* sb/push-make-submodule-check-the-default (2016-08-24) 1 commit\n - push: change submodule default to check\n\n Turn the default of \"push.recurseSubmodules\" to \"check\".\n\n Will hold to wait for hv/submodule-not-yet-pushed-fix\n\n This reveals that the \"check\" mode is too inefficient to use in\n real projects, even in ones as small as git itself.\n cf. \n\nWhich itself is\n\n* hv/submodule-not-yet-pushed-fix (2016-09-14) 2 commits\n - serialize collection of refs that contain submodule changes\n - serialize collection of changed submodules\n\n The code in \"git push\" to compute if any commit being pushed in the\n superproject binds a commit in a submodule that hasn't been pushed\n out was overly inefficient, making it unusable even for a small\n project that does not have any submodule but have a reasonable\n number of refs.\n\n The last two in the original series seem to break a few tests when\n queued to 'pu', and dropped for now.\n\n Waiting for a reroll.\n"},{"id":"303141","messageId":"CAGZ79kY6FpTD1VJQ=+wJ0FrXe9LjJ=NBwLsOku0R4FerAmQGJQ@mail.gmail.com","threadId":"44206","inReplyTo":"CAGZ79kYXx+a+U3o6FPBy2mHOy4BxxuF97t6iyedPLk0Qw1Jx1A@mail.gmail.com","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-03T21:23:16Z","receivedAt":"2016-10-03T21:23:30Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+cc Heiko\n\nOn Mon, Oct 3, 2016 at 2:17 PM, Stefan Beller wrote:\n>\n> * sb/push-make-submodule-check-the-default (2016-08-24) 1 commit\n> - push: change submodule default to check\n>\n> Turn the default of \"push.recurseSubmodules\" to \"check\".\n>\n> Will hold to wait for hv/submodule-not-yet-pushed-fix\n>\n> This reveals that the \"check\" mode is too inefficient to use in\n> real projects, even in ones as small as git itself.\n> cf. \n\nSo maybe we should eject this series from pu as long as\nhv/submodule-not-yet-pushed-fix is ejected to enable you\nrunning pu happily.\n\nThanks,\nStefan\n"},{"id":"303150","messageId":"xmqqd1jh5bq2.fsf@gitster.mtv.corp.google.com","threadId":"44206","inReplyTo":"CAGZ79kY6FpTD1VJQ=+wJ0FrXe9LjJ=NBwLsOku0R4FerAmQGJQ@mail.gmail.com","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T22:06:29Z","receivedAt":"2016-10-03T22:06:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Oct 3, 2016 at 2:17 PM, Stefan Beller wrote:\n>>\n>> * sb/push-make-submodule-check-the-default (2016-08-24) 1 commit\n>> - push: change submodule default to check\n>>\n>> Turn the default of \"push.recurseSubmodules\" to \"check\".\n>>\n>> Will hold to wait for hv/submodule-not-yet-pushed-fix\n>>\n>> This reveals that the \"check\" mode is too inefficient to use in\n>> real projects, even in ones as small as git itself.\n>> cf. \n>\n> So maybe we should eject this series from pu as long as\n> hv/submodule-not-yet-pushed-fix is ejected to enable you\n> running pu happily.\n\nI am planning to merge lt/abbrev-auto to 'next' together with Peff's\nambiguous-short-object-names series in today's pushout.\n\nJust FYI, there is another integration branch called 'jch' that\ntypically has several topics more than 'next' but does not merge\nthings I haven't looked at (or things I have looked at and decided\nnot ready). That is what I use for my daily work. You can grab it\nout of \"git log --oneline --first-parent master..pu\", or from my\nbroken-out repository (git://github.com/gitster/git/).\n\n\n\n\n\n"},{"id":"303200","messageId":"20161004111845.GA20309@book.hvoigt.net","threadId":"44206","inReplyTo":"CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-04T11:18:45Z","receivedAt":"2016-10-04T11:18:57Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Mon, Oct 03, 2016 at 02:11:36PM -0700, Linus Torvalds wrote:\n> This seems to be because I'm now on 'pu' as of a day or two ago in\n> order to test the abbrev logic, but lookie here:\n> \n> time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n> .. shows all the branches and tags ..\n> real 0m0.655s\n> user 0m0.011s\n> sys 0m0.004s\n> \n> so the remote is fast to connect to, and with network connection\n> overhead and everything, it's just over half a second. But then:\n> \n> time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n\nThe reason behind this is when pushing to an address we do not easily\nhave the remote refs to compare available. When pushing an existing ref\nit would be easy and could get a shortcut but it gets more complicated\nfor new refs. Currently we fall back to walking the whole history since\nthat is \"the most correct way\" we have. But obviously it is not a\npractical solution in any way.\n\nI mentioned this fact when discussing the current state and my patches\nto make this check less painful. So we still need to think about a\nsolution for this check when passing an address.\n\nIMO: It's definitely not ready to be switched on as default, unless we\nfind something a lot cheaper for the above case.\n\nMy idea of a solution goes like this:\n * collect all SHA1's of the remotes refs\n * check if we have them locally\n * if not we abort and tell the user to fetch them somehow into local\n refs or disable the check\n * when we have them locally we proceed passing those SHA1's as bases\n instead of --remotes=\n\nCheers Heiko\n"},{"id":"303204","messageId":"20161004114428.4wyq54afd4td3epp@sigill.intra.peff.net","threadId":"44206","inReplyTo":"20161004111845.GA20309@book.hvoigt.net","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T11:44:28Z","receivedAt":"2016-10-04T11:44:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 01:18:45PM +0200, Heiko Voigt wrote:\n\n> On Mon, Oct 03, 2016 at 02:11:36PM -0700, Linus Torvalds wrote:\n> > This seems to be because I'm now on 'pu' as of a day or two ago in\n> > order to test the abbrev logic, but lookie here:\n> > \n> > time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n> > .. shows all the branches and tags ..\n> > real 0m0.655s\n> > user 0m0.011s\n> > sys 0m0.004s\n> > \n> > so the remote is fast to connect to, and with network connection\n> > overhead and everything, it's just over half a second. But then:\n> > \n> > time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux\n> \n> The reason behind this is when pushing to an address we do not easily\n> have the remote refs to compare available. When pushing an existing ref\n> it would be easy and could get a shortcut but it gets more complicated\n> for new refs. Currently we fall back to walking the whole history since\n> that is \"the most correct way\" we have. But obviously it is not a\n> practical solution in any way.\n> \n> I mentioned this fact when discussing the current state and my patches\n> to make this check less painful. So we still need to think about a\n> solution for this check when passing an address.\n> \n> IMO: It's definitely not ready to be switched on as default, unless we\n> find something a lot cheaper for the above case.\n> \n> My idea of a solution goes like this:\n> * collect all SHA1's of the remotes refs\n> * check if we have them locally\n> * if not we abort and tell the user to fetch them somehow into local\n> refs or disable the check\n> * when we have them locally we proceed passing those SHA1's as bases\n> instead of --remotes=\n\nAs I argued in [1], I think it's not just \"this must be cheaper\" but\n\"this must not be enabled if submodules are not in use at all\". Most\nrepositories don't have submodules enabled at all, so anything that\ncause any extra traversal, even of a portion of the history, is going to\nbe a net negative for a lot of people.\n\nI think the only sane default is going to be some kind of heuristic that\nsays \"submodules are probably in use\". Something like \"is there a\n.gitmodules file\" is not perfect (you can have gitlink entries without\nit), but it's a really cheap constant-time check.\n\n-Peff\n\n[1] Quoted in\n http://public-inbox.org/git/xmqqh9aaot49.fsf@gitster.mtv.corp.google.com/\n"},{"id":"303205","messageId":"20161004120421.GA20701@book.hvoigt.net","threadId":"44206","inReplyTo":"20161004114428.4wyq54afd4td3epp@sigill.intra.peff.net","subject":"Re: Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-04T12:04:21Z","receivedAt":"2016-10-04T12:04:34Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Oct 04, 2016 at 07:44:28AM -0400, Jeff King wrote:\n> > My idea of a solution goes like this:\n> > * collect all SHA1's of the remotes refs\n> > * check if we have them locally\n> > * if not we abort and tell the user to fetch them somehow into local\n> > refs or disable the check\n> > * when we have them locally we proceed passing those SHA1's as bases\n> > instead of --remotes=\n> \n> As I argued in [1], I think it's not just \"this must be cheaper\" but\n> \"this must not be enabled if submodules are not in use at all\". Most\n> repositories don't have submodules enabled at all, so anything that\n> cause any extra traversal, even of a portion of the history, is going to\n> be a net negative for a lot of people.\n> \n> I think the only sane default is going to be some kind of heuristic that\n> says \"submodules are probably in use\". Something like \"is there a\n> .gitmodules file\" is not perfect (you can have gitlink entries without\n> it), but it's a really cheap constant-time check.\n\nI agree. We are adding convenience for submodules, so we can also say a\nchecked out \".gitmodules\" file is a must to have convenience.\n\nI am not sure if I agree on another layer of options for this as\nsuggested in your post. More options mean more implementation\ncomplexity and more confusion on the users side.\n\nHow about we choose our defaults based on the existence of a checked out\n.gitmodules file? So the default would only be --recurse-submodules=check\nif there is a .gitmodules file in the worktree. All other users need to\neither pass or explicitly configure it.\n\nCheers Heiko\n\n> [1] Quoted in\n> http://public-inbox.org/git/xmqqh9aaot49.fsf@gitster.mtv.corp.google.com/\n"},{"id":"303206","messageId":"20161004120735.pbhejoqpwiccemga@sigill.intra.peff.net","threadId":"44206","inReplyTo":"20161004120421.GA20701@book.hvoigt.net","subject":"Re: Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T12:07:35Z","receivedAt":"2016-10-04T12:07:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:04:21PM +0200, Heiko Voigt wrote:\n\n> > I think the only sane default is going to be some kind of heuristic that\n> > says \"submodules are probably in use\". Something like \"is there a\n> > .gitmodules file\" is not perfect (you can have gitlink entries without\n> > it), but it's a really cheap constant-time check.\n> \n> I agree. We are adding convenience for submodules, so we can also say a\n> checked out \".gitmodules\" file is a must to have convenience.\n> \n> I am not sure if I agree on another layer of options for this as\n> suggested in your post. More options mean more implementation\n> complexity and more confusion on the users side.\n> \n> How about we choose our defaults based on the existence of a checked out\n> .gitmodules file? So the default would only be --recurse-submodules=check\n> if there is a .gitmodules file in the worktree. All other users need to\n> either pass or explicitly configure it.\n\nThat's OK with me. Though you may end up in the long run wanting some\nname for the default behavior (e.g., if people configure something else\nand then want to override back to \"auto\" in some instances), but that\ncan probably come later.\n\n-Peff\n"},{"id":"303253","messageId":"xmqq8tu4w0i2.fsf@gitster.mtv.corp.google.com","threadId":"44206","inReplyTo":"20161004114428.4wyq54afd4td3epp@sigill.intra.peff.net","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T16:19:01Z","receivedAt":"2016-10-04T16:19:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As I argued in [1], I think it's not just \"this must be cheaper\" but\n> \"this must not be enabled if submodules are not in use at all\". Most\n> repositories don't have submodules enabled at all, so anything that\n> cause any extra traversal, even of a portion of the history, is going to\n> be a net negative for a lot of people.\n>\n> I think the only sane default is going to be some kind of heuristic that\n> says \"submodules are probably in use\".\n\nWhy should we even have a default different from today's? If most\nrepositories don't have submodules enabled at all, we can just let\nthose working with submodules enabled to toggle their configuration\nand that is an very easy to understand solution, no?\n"},{"id":"303254","messageId":"20161004162102.rwofudnx3g3fsyul@sigill.intra.peff.net","threadId":"44206","inReplyTo":"xmqq8tu4w0i2.fsf@gitster.mtv.corp.google.com","subject":"Re: Slow pushes on 'pu' - even when up-to-date..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T16:21:03Z","receivedAt":"2016-10-04T16:21:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 09:19:01AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > As I argued in [1], I think it's not just \"this must be cheaper\" but\n> > \"this must not be enabled if submodules are not in use at all\". Most\n> > repositories don't have submodules enabled at all, so anything that\n> > cause any extra traversal, even of a portion of the history, is going to\n> > be a net negative for a lot of people.\n> >\n> > I think the only sane default is going to be some kind of heuristic that\n> > says \"submodules are probably in use\".\n> \n> Why should we even have a default different from today's? If most\n> repositories don't have submodules enabled at all, we can just let\n> those working with submodules enabled to toggle their configuration\n> and that is an very easy to understand solution, no?\n\nYou will not see any complaint from me on that. I was taking for granted\nthat the current default is inconvenient to submodule users, but I don't\nhave any experience myself.\n\n-Peff\n"},{"id":"303255","messageId":"20161004164036.6584-1-sbeller@google.com","threadId":"44206","inReplyTo":"20161004162102.rwofudnx3g3fsyul@sigill.intra.peff.net","subject":"[PATCH] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T16:40:36Z","receivedAt":"2016-10-04T16:40:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When working with submodules, it is easy to forget to push the submodules.\nThe setting 'check', which checks if any existing submodule is present on\nat least one remote of the submodule remotes, is designed to prevent this\nmistake.\n\nFlipping the default to check for submodules is safer than the current\ndefault of ignoring submodules while pushing.\n\nHowever checking for submodules requires additional work[1], which annoys\nusers that do not use submodules, so we turn on the check for submodules\nbased on a cheap heuristic, the existence of the .gitmodules file.\n\n[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/\n\nSigned-off-by: Stefan Beller \n---\n\nOn Tue, Oct 4, 2016 at 9:21 AM, Jeff King wrote:\n> On Tue, Oct 04, 2016 at 09:19:01AM -0700, Junio C Hamano wrote:\n>>\n>> Why should we even have a default different from today's? If most\n>> repositories don't have submodules enabled at all, we can just let\n>> those working with submodules enabled to toggle their configuration\n>> and that is an very easy to understand solution, no?\n>\n> You will not see any complaint from me on that. I was taking for granted\n> that the current default is inconvenient to submodule users, but I don't\n> have any experience myself.\n>\n\nAnd there I was trying to help submodule users not shoot in their foot.\n\nI think it is one of the problems that causes serious problems, that is easy\nto fix from the side of Git. This patch replaces sb/push-make-submodule-check-the-default\nand should be cheap enough for non-submodule users to accept, but still helping\nsubmodule users as it seems to be an ok-ish heuristic. (It is possible to use\nsubmodules and currently have no .gitmodules file present, because you're in\na weird state; then the heuristic fails. By weird state I mean e.g. a bare\nrepository, or you just checked out an ancient version that has no submodules\nyet, or you deleted it locally for whatever reason.)\n\nSo how about this patch?\n\nThanks,\nStefan\n\n builtin/push.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 3bb9d6b..d7d664a 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -22,7 +22,7 @@ static int deleterefs;\n static const char *receivepack;\n static int verbosity;\n static int progress = -1;\n-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules;\n static enum transport_family family;\n \n static struct push_cas_option cas;\n@@ -31,6 +31,14 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static void preset_submodule_default(void)\n+{\n+\tif (file_exists(\".gitmodules\"))\n+\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n+\telse\n+\t\trecurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+}\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t};\n \n \tpacket_trace_identity(\"push\");\n+\tpreset_submodule_default();\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \tset_push_cert_flags(&flags, push_cert);\n-- \n2.10.0.129.g35f6318\n\n"},{"id":"303265","messageId":"20161004173430.eax4ptohyonc5bw2@sigill.intra.peff.net","threadId":"44206","inReplyTo":"20161004164036.6584-1-sbeller@google.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T17:34:30Z","receivedAt":"2016-10-04T17:34:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:\n\n> >> Why should we even have a default different from today's? If most\n> >> repositories don't have submodules enabled at all, we can just let\n> >> those working with submodules enabled to toggle their configuration\n> >> and that is an very easy to understand solution, no?\n> >\n> > You will not see any complaint from me on that. I was taking for granted\n> > that the current default is inconvenient to submodule users, but I don't\n> > have any experience myself.\n> >\n> \n> And there I was trying to help submodule users not shoot in their foot.\n\nSorry if my reply came off as snarky. I really did mean it literally. I\ndo not know if the end goal is good or not, so all of my discussion was\njust assuming it was.\n\nSo in that vein...\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 3bb9d6b..d7d664a 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -22,7 +22,7 @@ static int deleterefs;\n> static const char *receivepack;\n> static int verbosity;\n> static int progress = -1;\n> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules;\n> static enum transport_family family;\n> \n> static struct push_cas_option cas;\n> @@ -31,6 +31,14 @@ static const char **refspec;\n> static int refspec_nr;\n> static int refspec_alloc;\n> \n> +static void preset_submodule_default(void)\n> +{\n> +\tif (file_exists(\".gitmodules\"))\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n> +\telse\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +}\n\nThis does seem like a reasonable heuristic. I wonder if you want to\nconfirm that we actually have a worktree (and are in it) before looking\nat file_exists(). It's unlikely that looking at \".gitmodules\" in a bare\nrepo would trigger in practice, but it does not hurt to be careful.\n\n-Peff\n"},{"id":"303269","messageId":"CAGZ79kaKOjqLBsNVSmudzLUCkOJf_CsFGE8OZZHsTmuXCfiVeg@mail.gmail.com","threadId":"44206","inReplyTo":"20161004173430.eax4ptohyonc5bw2@sigill.intra.peff.net","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T17:48:51Z","receivedAt":"2016-10-04T17:49:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 10:34 AM, Jeff King wrote:\n> On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:\n>\n>> >> Why should we even have a default different from today's? If most\n>> >> repositories don't have submodules enabled at all, we can just let\n>> >> those working with submodules enabled to toggle their configuration\n>> >> and that is an very easy to understand solution, no?\n>> >\n>> > You will not see any complaint from me on that. I was taking for granted\n>> > that the current default is inconvenient to submodule users, but I don't\n>> > have any experience myself.\n>> >\n>>\n>> And there I was trying to help submodule users not shoot in their foot.\n>\n> Sorry if my reply came off as snarky.\n\nYeah, sorry about starting being snarky here.\n\n>>\n>> +static void preset_submodule_default(void)\n>> +{\n>> + if (file_exists(\".gitmodules\"))\n>> + recurse_submodules = RECURSE_SUBMODULES_CHECK;\n>> + else\n>> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n>> +}\n>\n> This does seem like a reasonable heuristic. I wonder if you want to\n> confirm that we actually have a worktree (and are in it) before looking\n> at file_exists(). It's unlikely that looking at \".gitmodules\" in a bare\n> repo would trigger in practice, but it does not hurt to be careful.\n\nIn a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?\n\nI considered it a non issue, as I don't think many people push from\nbare repositories.\nSo if we were to check in the HEAD tree instead of the file system, why would we\napply different rules for bare and non bare repositories? We probably\nwould not want\nto do that, so is it reasonable to check for the .gitmodules in the HEAD tree in\ngeneral in the non bare case? I dunno, it sounds like an equally cheap heuristic\ncovering most cases.\n\nHere is another thought:\n.gitmodules may not exist (either in working dir or in HEADs git\ntree), so maybe the\n\"correct\" heuristic is to check for directories in $GIT_DIR/modules/\nThat is \"more correct\", because it is inconceivable to change the submodule\npointers without having the submodules checked out. (Who would do that? Why?)\n\n>\n> -Peff\n"},{"id":"303270","messageId":"20161004175449.gn5cw6wcbvloqkzj@sigill.intra.peff.net","threadId":"44206","inReplyTo":"CAGZ79kaKOjqLBsNVSmudzLUCkOJf_CsFGE8OZZHsTmuXCfiVeg@mail.gmail.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T17:54:49Z","receivedAt":"2016-10-04T17:54:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 10:48:51AM -0700, Stefan Beller wrote:\n\n> > This does seem like a reasonable heuristic. I wonder if you want to\n> > confirm that we actually have a worktree (and are in it) before looking\n> > at file_exists(). It's unlikely that looking at \".gitmodules\" in a bare\n> > repo would trigger in practice, but it does not hurt to be careful.\n> \n> In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?\n\nYeah, I think that is the closest equivalent.\n\n> I considered it a non issue, as I don't think many people push from\n> bare repositories.\n\nI'd also agree, and I have no problem if there simply _isn't_ an auto\nheuristic for bare repos. Mostly I just thought blindly calling\nfile_exists() was ugly (especially after all the recent \"whoops, we look\nat .git/config in the wrong directory\" fixes I've been doing lately).\n\n> Here is another thought:\n> .gitmodules may not exist (either in working dir or in HEADs git\n> tree), so maybe the\n> \"correct\" heuristic is to check for directories in $GIT_DIR/modules/\n> That is \"more correct\", because it is inconceivable to change the submodule\n> pointers without having the submodules checked out. (Who would do that? Why?)\n\nActually, I like that a bit better. It would not cover the case where\nyou have not actually checked out any of the submodules (or at least not\ncalled \"submodule init\", I guess?). But arguably that is a sign that the\nauto-recurse behavior should not be kicking in anyway.\n\nBearing in mind that I am not too familiar with what's normal in the\nsubmodule world, and so might be spouting nonsense. :)\n\n-Peff\n"},{"id":"303273","messageId":"xmqqfuocuh92.fsf@gitster.mtv.corp.google.com","threadId":"44206","inReplyTo":"20161004164036.6584-1-sbeller@google.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T18:00:09Z","receivedAt":"2016-10-04T18:00:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +static void preset_submodule_default(void)\n> +{\n> +\tif (file_exists(\".gitmodules\"))\n\nDon't we need to see if we are in a bare repository?\n\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n> +\telse\n> +\t\trecurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n\nHmph, why \"_DEFAULT\" not \"_OFF\"?\n\n> +}\n> +\n> static void add_refspec(const char *ref)\n> {\n> \trefspec_nr++;\n> @@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> \t};\n> \n> \tpacket_trace_identity(\"push\");\n> +\tpreset_submodule_default();\n> \tgit_config(git_push_config, &flags);\n> \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n> \tset_push_cert_flags(&flags, push_cert);\n"},{"id":"303275","messageId":"xmqqbmz0uh54.fsf@gitster.mtv.corp.google.com","threadId":"44206","inReplyTo":"xmqqfuocuh92.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T18:02:31Z","receivedAt":"2016-10-04T18:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> +static void preset_submodule_default(void)\n>> +{\n>> +\tif (file_exists(\".gitmodules\"))\n>\n> Don't we need to see if we are in a bare repository?\n>\n>> +\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n>> +\telse\n>> +\t\trecurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n>\n> Hmph, why \"_DEFAULT\" not \"_OFF\"?\n\n... because you wanted to keep the same behaviour, i.e. keep\nrecurse_submodules set to _DEFAULT just like the compiled-in\ninitialization does.\n\nPerhaps we can lose \"else\" clause altogether?\n"},{"id":"303276","messageId":"xmqq7f9ouh1t.fsf@gitster.mtv.corp.google.com","threadId":"44206","inReplyTo":"20161004175449.gn5cw6wcbvloqkzj@sigill.intra.peff.net","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T18:04:30Z","receivedAt":"2016-10-04T18:04:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, I like that a bit better. It would not cover the case where\n> you have not actually checked out any of the submodules (or at least not\n> called \"submodule init\", I guess?). But arguably that is a sign that the\n> auto-recurse behavior should not be kicking in anyway.\n\nYeah, the \"no init, no recursion\" line of thought is very sensible.\nI like it.\n\n"},{"id":"303277","messageId":"CAGZ79kb=q9vCMxKTCfuGGDZdHMdAmh5RP0KN=vCbSS=oerPH3Q@mail.gmail.com","threadId":"44206","inReplyTo":"xmqqfuocuh92.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T18:05:26Z","receivedAt":"2016-10-04T18:05:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 11:00 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +static void preset_submodule_default(void)\n>> +{\n>> + if (file_exists(\".gitmodules\"))\n>\n> Don't we need to see if we are in a bare repository?\n\nSee discussion with Jeff; instead of checking the file, we rather want to check\nif $GIT_DIR/modules/ is populated, as that is version agnostic\n(\"Was a submodule initialized and fetched at any time in the\nlife time of this repository?\"), as well as bare/non-bare agnostic.\n\n>\n>> + recurse_submodules = RECURSE_SUBMODULES_CHECK;\n>> + else\n>> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n>\n> Hmph, why \"_DEFAULT\" not \"_OFF\"?\n\nYou answered yourself below, and that was indeed my thought. I was\nalso wondering\nwhether to remove the else, but then I thought that we'd rather do not\nwant to rely on\ncompiled-in at all, and have one init function which sets out the new\nworld order.\n"},{"id":"303278","messageId":"CAGZ79kYQsXxzXihtzC70Oj29RAe=8+nuJeUWNmbF17C75xbEag@mail.gmail.com","threadId":"44206","inReplyTo":"xmqq7f9ouh1t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T18:08:33Z","receivedAt":"2016-10-04T18:08:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Actually, I like that a bit better. It would not cover the case where\n>> you have not actually checked out any of the submodules (or at least not\n>> called \"submodule init\", I guess?). But arguably that is a sign that the\n>> auto-recurse behavior should not be kicking in anyway.\n>\n> Yeah, the \"no init, no recursion\" line of thought is very sensible.\n> I like it.\n>\n\nBear in mind that \"submodule init\" only fuzzes around with .git/config.\nIt doesn't touch .git/modules (i.e. cloning/fetching), that is to be done\nwith the update command.\n\nSo if we also want to cover the case of init'd submodules, but not\ncloned/checked out submodules, we'd rather want to consult .git/config\nwhether there is any submodule.* option set, though that seems more\nexpensive than just checking for files inside the modules directory.\n"},{"id":"303279","messageId":"20161004182801.j3fdpewybatmibpo@sigill.intra.peff.net","threadId":"44206","inReplyTo":"CAGZ79kYQsXxzXihtzC70Oj29RAe=8+nuJeUWNmbF17C75xbEag@mail.gmail.com","subject":"Re: [PATCH] push: change submodule default to check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T18:28:01Z","receivedAt":"2016-10-04T18:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 11:08:33AM -0700, Stefan Beller wrote:\n\n> On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> >> Actually, I like that a bit better. It would not cover the case where\n> >> you have not actually checked out any of the submodules (or at least not\n> >> called \"submodule init\", I guess?). But arguably that is a sign that the\n> >> auto-recurse behavior should not be kicking in anyway.\n> >\n> > Yeah, the \"no init, no recursion\" line of thought is very sensible.\n> > I like it.\n> \n> Bear in mind that \"submodule init\" only fuzzes around with .git/config.\n> It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done\n> with the update command.\n> \n> So if we also want to cover the case of init'd submodules, but not\n> cloned/checked out submodules, we'd rather want to consult .git/config\n> whether there is any submodule.* option set, though that seems more\n> expensive than just checking for files inside the modules directory.\n\nConsulting .git/config is fine, I think. It's not like we don't read it\n(sometimes multiple times!) during the normal course of the program\nanyway. It's just a question of whether it makes more sense for the\nheuristic to kick in after \"init\", or only after \"update\". I don't know\nenough to have an opinion.\n\n-Peff\n"},{"id":"303286","messageId":"20161004192910.30649-2-sbeller@google.com","threadId":"44206","inReplyTo":"20161004192910.30649-1-sbeller@google.com","subject":"[PATCH 2/2] builtin/push: move flags do_push","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T19:29:10Z","receivedAt":"2016-10-04T19:29:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In do_push we set the flags for other options, so let's make the code\nmore consistent and also apply the flags for recursing into submodules\nthere. \n\nSigned-off-by: Stefan Beller \n---\n\nNo functional change intended, just a cleanup while we're at it.\nFeel free to drop if it's too much churn.\n\n builtin/push.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 06fd3bd..6690301 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -388,6 +388,11 @@ static int do_push(const char *repo, int flags,\n \t\t \" git push \\n\"));\n \t}\n \n+\tif (recurse_submodules == RECURSE_SUBMODULES_CHECK)\n+\t\tflags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;\n+\telse if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n+\t\tflags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;\n+\n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n \n@@ -576,11 +581,6 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (deleterefs && argc < 2)\n \t\tdie(_(\"--delete doesn't make sense without any refs\"));\n \n-\tif (recurse_submodules == RECURSE_SUBMODULES_CHECK)\n-\t\tflags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;\n-\telse if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n-\t\tflags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;\n-\n \tif (tags)\n \t\tadd_refspec(\"refs/tags/*\");\n \n-- \n2.10.0.129.g35f6318\n\n"},{"id":"303287","messageId":"20161004192910.30649-1-sbeller@google.com","threadId":"44206","inReplyTo":"20161004182801.j3fdpewybatmibpo@sigill.intra.peff.net","subject":"[PATCHv2 1/2] push: change submodule default to check when submodules exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T19:29:09Z","receivedAt":"2016-10-04T19:29:33Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When working with submodules, it is easy to forget to push the submodules.\nThe setting 'check', which checks if any existing submodule is present on\nat least one remote of the submodule remotes, is designed to prevent this\nmistake.\n\nFlipping the default to check for submodules is safer than the current\ndefault of ignoring submodules while pushing.\n\nHowever checking for submodules requires additional work[1], which annoys\nusers that do not use submodules, so we turn on the check for submodules\nbased on a cheap heuristic, the existence of the .git/modules directory.\nThat directory doesn't exist when no submodules are used and is only\ncreated and populated when submodules are cloned/added.\n\nWhen the submodule directory doesn't exist, a user may have changed the\ngitlinks via plumbing commands. Currently the default is to not check.\nRECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,\nthough it may change in the future. When no submodules exist such a check\nis pointless as it would fail anyway, so let's just turn it off.\n\n[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/\n\nSigned-off-by: Stefan Beller \n---\n\nJeff wrote:\n> Consulting .git/config is fine, I think. It's not like we don't read it\n> (sometimes multiple times!) during the normal course of the program\n> anyway. It's just a question of whether it makes more sense for the\n> heuristic to kick in after \"init\", or only after \"update\". I don't know\n> enough to have an opinion.\n\nI think there is no difference in practice, however the \"after update\"\nis way easier to implement and hence more maintainable (one lstat instead of\nfiddeling with the config; that can go wrong easily). \n\nThanks,\nStefan\n\n builtin/push.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 3bb9d6b..06fd3bd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -3,6 +3,7 @@\n */\n #include \"cache.h\"\n #include \"refs.h\"\n+#include \"dir.h\"\n #include \"run-command.h\"\n #include \"builtin.h\"\n #include \"remote.h\"\n@@ -22,7 +23,7 @@ static int deleterefs;\n static const char *receivepack;\n static int verbosity;\n static int progress = -1;\n-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules;\n static enum transport_family family;\n \n static struct push_cas_option cas;\n@@ -31,6 +32,19 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static void preset_submodule_default(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstrbuf_addf(&sb, \"%s/modules\", get_git_dir());\n+\n+\tif (file_exists(sb.buf))\n+\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n+\telse\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\n+\tstrbuf_release(&sb);\n+}\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -552,6 +566,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t};\n \n \tpacket_trace_identity(\"push\");\n+\tpreset_submodule_default();\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \tset_push_cert_flags(&flags, push_cert);\n-- \n2.10.0.129.g35f6318\n\n"},{"id":"303290","messageId":"20161004193926.32w7yivkakqoadm2@sigill.intra.peff.net","threadId":"44206","inReplyTo":"20161004192910.30649-1-sbeller@google.com","subject":"Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-04T19:39:26Z","receivedAt":"2016-10-04T19:39:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote:\n\n> Jeff wrote:\n> > Consulting .git/config is fine, I think. It's not like we don't read it\n> > (sometimes multiple times!) during the normal course of the program\n> > anyway. It's just a question of whether it makes more sense for the\n> > heuristic to kick in after \"init\", or only after \"update\". I don't know\n> > enough to have an opinion.\n> \n> I think there is no difference in practice, however the \"after update\"\n> is way easier to implement and hence more maintainable (one lstat instead of\n> fiddeling with the config; that can go wrong easily). \n\nHmm, I would have thought it is the opposite; can't submodules exist in\nthe working tree in their own \".git\" directory? I know that's the \"old\"\nway of doing it, but I didn't know if it was totally deprecated.\n\nAnyway, the config version is probably just:\n\n int config_check_submodule(const char *var, const char *value, void *data)\n {\n\tif (starts_with(var, \"submodule.\") && ends_with(var, \".path\"))\n\t\t*(int *)data = 1;\n\treturn 0;\n }\n\n ...\n int have_submodule = 0;\n git_config(config_check_submodule, &have_submodule);\n\nBut I don't care too much either way. that's just for reference.\n\n> @@ -31,6 +32,19 @@ static const char **refspec;\n> static int refspec_nr;\n> static int refspec_alloc;\n> \n> +static void preset_submodule_default(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstrbuf_addf(&sb, \"%s/modules\", get_git_dir());\n> +\n> +\tif (file_exists(sb.buf))\n\nMaybe just:\n\n if (file_exists(git_path(\"modules\"))\n\n?\n\n-Peff\n"},{"id":"303291","messageId":"CAGZ79kbt+SZoogKTV_-rVfOOFzf6xrhWytrBo2H3r6NQw34WTw@mail.gmail.com","threadId":"44206","inReplyTo":"20161004193926.32w7yivkakqoadm2@sigill.intra.peff.net","subject":"Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T19:51:26Z","receivedAt":"2016-10-04T19:51:32Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 4, 2016 at 12:39 PM, Jeff King wrote:\n> On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote:\n>\n>> Jeff wrote:\n>> > Consulting .git/config is fine, I think. It's not like we don't read it\n>> > (sometimes multiple times!) during the normal course of the program\n>> > anyway. It's just a question of whether it makes more sense for the\n>> > heuristic to kick in after \"init\", or only after \"update\". I don't know\n>> > enough to have an opinion.\n>>\n>> I think there is no difference in practice, however the \"after update\"\n>> is way easier to implement and hence more maintainable (one lstat instead of\n>> fiddeling with the config; that can go wrong easily).\n>\n> Hmm, I would have thought it is the opposite; can't submodules exist in\n> the working tree in their own \".git\" directory? I know that's the \"old\"\n> way of doing it, but I didn't know if it was totally deprecated.\n\nOo, right. :(\n\nThe proposed patch would not change the current behavior for a layout of\n.git directories inside the submodule working trees, actually.\nIt would however also not have the desired effect of enabling the check for\npush.\n\nHowever these not-yet-deprecated layouts are likely in use by people\nwho know what they are doing, so maybe we can punt on that.\n\n>\n> Anyway, the config version is probably just:\n>\n> int config_check_submodule(const char *var, const char *value, void *data)\n> {\n> if (starts_with(var, \"submodule.\") && ends_with(var, \".path\"))\n\ns/.path/.url/ but I get the point. I do dislike this solution for\nanother reasons though:\n\nIn the future when worktree supports submodules we either\nhave the url per worktree,so we'd need to process all working tree\nconfigs as well\nor we do it the proper way, which is replacing the submodule.$name.url variable\nwith 2 options, one is purely used to configure the URL and the other is purely\nused to indicate the existence of a submodule. Piling on the mixed use case of\nurls today feels sad.\n\n\n> *(int *)data = 1;\n> return 0;\n> }\n>\n> ...\n> int have_submodule = 0;\n> git_config(config_check_submodule, &have_submodule);\n>\n> But I don't care too much either way. that's just for reference.\n>\n>> @@ -31,6 +32,19 @@ static const char **refspec;\n>> static int refspec_nr;\n>> static int refspec_alloc;\n>>\n>> +static void preset_submodule_default(void)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + strbuf_addf(&sb, \"%s/modules\", get_git_dir());\n>> +\n>> + if (file_exists(sb.buf))\n>\n> Maybe just:\n>\n> if (file_exists(git_path(\"modules\"))\n\nSounds good.\n\nSo I'll see if I can get the version running you propose here, otherwise\nI'll resend with these changes.\n\n\n>\n> ?\n>\n> -Peff\n"},{"id":"303307","messageId":"20161004210359.15266-1-sbeller@google.com","threadId":"44206","inReplyTo":"CAGZ79kbt+SZoogKTV_-rVfOOFzf6xrhWytrBo2H3r6NQw34WTw@mail.gmail.com","subject":"[PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-04T21:03:58Z","receivedAt":"2016-10-04T21:04:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When working with submodules, it is easy to forget to push the submodules.\nThe setting 'check', which checks if any existing submodule is present on\nat least one remote of the submodule remotes, is designed to prevent this\nmistake.\n\nFlipping the default to check for submodules is safer than the current\ndefault of ignoring submodules while pushing.\n\nHowever checking for submodules requires additional work[1], which annoys\nusers that do not use submodules, so we turn on the check for submodules\nbased on a cheap heuristic, the existence of the .git/modules directory.\nThat directory doesn't exist when no submodules are used and is only\ncreated and populated when submodules are cloned/added.\n\nWhen the submodule directory doesn't exist, a user may have changed the\ngitlinks via plumbing commands. Currently the default is to not check.\nRECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,\nthough it may change in the future. When no submodules exist such a check\nis pointless as it would fail anyway, so let's just turn it off.\n\n[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/\n\nSigned-off-by: Stefan Beller \n---\n\nJeff,\nthanks for the suggestions, both git_path(..) as well as checking the config,\nthis seems quite readable to me:\n\n builtin/push.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 3bb9d6b..65bb1df 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -3,6 +3,7 @@\n */\n #include \"cache.h\"\n #include \"refs.h\"\n+#include \"dir.h\"\n #include \"run-command.h\"\n #include \"builtin.h\"\n #include \"remote.h\"\n@@ -22,6 +23,7 @@ static int deleterefs;\n static const char *receivepack;\n static int verbosity;\n static int progress = -1;\n+static int has_submodules;\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static enum transport_family family;\n \n@@ -31,6 +33,14 @@ static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n \n+static void preset_submodule_default(void)\n+{\n+\tif (has_submodules || file_exists(git_path(\"modules\")))\n+\t\trecurse_submodules = RECURSE_SUBMODULES_CHECK;\n+\telse\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+}\n+\n static void add_refspec(const char *ref)\n {\n \trefspec_nr++;\n@@ -495,7 +505,8 @@ static int git_push_config(const char *k, const char *v, void *cb)\n \t\tconst char *value;\n \t\tif (!git_config_get_value(\"push.recursesubmodules\", &value))\n \t\t\trecurse_submodules = parse_push_recurse_submodules_arg(k, value);\n-\t}\n+\t} else if (starts_with(k, \"submodule.\") && ends_with(k, \".url\"))\n+\t\thas_submodules = 1;\n \n \treturn git_default_config(k, v, NULL);\n }\n@@ -552,6 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t};\n \n \tpacket_trace_identity(\"push\");\n+\tpreset_submodule_default();\n \tgit_config(git_push_config, &flags);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\n \tset_push_cert_flags(&flags, push_cert);\n-- \n2.10.0.129.g35f6318\n\n"},{"id":"303363","messageId":"20161005135325.GC30930@book.hvoigt.net","threadId":"44206","inReplyTo":"20161004210359.15266-1-sbeller@google.com","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-05T13:53:25Z","receivedAt":"2016-10-05T13:53:35Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n> Jeff,\n> thanks for the suggestions, both git_path(..) as well as checking the config,\n> this seems quite readable to me:\n\nWhen reading the discussion I thought the same: What about the\n\"old-style\" repositories. I like this one. Checking both locations\nis nice.\n\nCheers Heiko\n"},{"id":"303376","messageId":"20161005154715.qmmcwpkt2yudbc2d@sigill.intra.peff.net","threadId":"44206","inReplyTo":"20161004210359.15266-1-sbeller@google.com","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T15:47:15Z","receivedAt":"2016-10-05T15:47:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n\n> thanks for the suggestions, both git_path(..) as well as checking the config,\n> this seems quite readable to me:\n> \n> builtin/push.c | 14 +++++++++++++-\n> 1 file changed, 13 insertions(+), 1 deletion(-)\n\nYeah, this seems like a good compromise to me.\n\nI did have one other thought, but I don't think it's worth pursuing now.\nWe care about finding gitlinks in objects we're pushing; is there any\nprocess which is already looking at those objects? Genreally, the\n\"pack-objects\" doing the push has to. Not to actually push the objects,\nwhich often are sent blindly off disk, but to determine the set of\nreachable objects in the first place. So in theory we could prepare the\nlist of objects to pack, and as a side effect it could say \"and here are\ngitlinks referenced by those objects\". But that doesn't work if bitmaps\nare in effect, because then we don't access the objects directly at all.\nI think you could solve that by extending the bitmap format to include a\nbit for gitlinks that are reachable (but not necessarily included in the\npack).\n\nSo I don't think that's worth thinking too much about now, but it might\nbe an interesting optimization down the road.\n\n-Peff\n"},{"id":"303377","messageId":"CAGZ79kaWsS489YRHY3Djjb2pPoVJa5F0MaFhwhne5izg4oG55g@mail.gmail.com","threadId":"44206","inReplyTo":"20161005154715.qmmcwpkt2yudbc2d@sigill.intra.peff.net","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-05T15:54:19Z","receivedAt":"2016-10-05T15:54:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 5, 2016 at 8:47 AM, Jeff King wrote:\n> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n>\n>> thanks for the suggestions, both git_path(..) as well as checking the config,\n>> this seems quite readable to me:\n>>\n>> builtin/push.c | 14 +++++++++++++-\n>> 1 file changed, 13 insertions(+), 1 deletion(-)\n>\n> Yeah, this seems like a good compromise to me.\n>\n> I did have one other thought, but I don't think it's worth pursuing now.\n> We care about finding gitlinks in objects we're pushing; is there any\n> process which is already looking at those objects? Genreally, the\n> \"pack-objects\" doing the push has to. Not to actually push the objects,\n> which often are sent blindly off disk, but to determine the set of\n> reachable objects in the first place. So in theory we could prepare the\n> list of objects to pack, and as a side effect it could say \"and here are\n> gitlinks referenced by those objects\". But that doesn't work if bitmaps\n> are in effect, because then we don't access the objects directly at all.\n> I think you could solve that by extending the bitmap format to include a\n> bit for gitlinks that are reachable (but not necessarily included in the\n> pack).\n>\n> So I don't think that's worth thinking too much about now, but it might\n> be an interesting optimization down the road.\n>\n> -Peff\n\nThat sounds like what we'd want down the road. In my imagination the\nsubmodules of the future may live completely inside the superproject,\ni.e. they do not necessarily have their own URL. They can be obtained\nvia fetching the superproject, that automagically also transmits the objects\nof the submodules.\n\nThanks for the hint w.r.t. the bimaps!\nStefan\n"},{"id":"303469","messageId":"20161006092332.GB38550@book.hvoigt.net","threadId":"44206","inReplyTo":"20161005135325.GC30930@book.hvoigt.net","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-06T09:23:32Z","receivedAt":"2016-10-06T09:23:43Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:\n> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n> > Jeff,\n> > thanks for the suggestions, both git_path(..) as well as checking the config,\n> > this seems quite readable to me:\n> \n> When reading the discussion I thought the same: What about the\n> \"old-style\" repositories. I like this one. Checking both locations\n> is nice.\n\nBTW, since it seems we all agree on the direction. Should we add some\ntests?\n\nCheers Heiko\n"},{"id":"303504","messageId":"CAGZ79ka=bVX51wrSiz4u7xOy+54t+v7a-iU5dHtQoNnB7-NAkw@mail.gmail.com","threadId":"44206","inReplyTo":"20161006092332.GB38550@book.hvoigt.net","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-06T17:20:16Z","receivedAt":"2016-10-06T17:20:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt wrote:\n> On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:\n>> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n>> > Jeff,\n>> > thanks for the suggestions, both git_path(..) as well as checking the config,\n>> > this seems quite readable to me:\n>>\n>> When reading the discussion I thought the same: What about the\n>> \"old-style\" repositories. I like this one. Checking both locations\n>> is nice.\n>\n> BTW, since it seems we all agree on the direction. Should we add some\n> tests?\n>\n\nGood call. What do we want to test for?\n* Correctness in case of submodules? (just push and get rejected)\n I think that is easy to do.\n* Performance with [no, lots of] submodules? That seems harder to me.\n\nI'll add a test for the correctness part and resend.\n\nThanks,\nStefan\n"},{"id":"303556","messageId":"20161007124157.GA61543@book.hvoigt.net","threadId":"44206","inReplyTo":"CAGZ79ka=bVX51wrSiz4u7xOy+54t+v7a-iU5dHtQoNnB7-NAkw@mail.gmail.com","subject":"Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-07T12:41:58Z","receivedAt":"2016-10-07T12:42:11Z","isPatch":false,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote:\n> On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt wrote:\n> > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:\n> >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:\n> >> > Jeff,\n> >> > thanks for the suggestions, both git_path(..) as well as checking the config,\n> >> > this seems quite readable to me:\n> >>\n> >> When reading the discussion I thought the same: What about the\n> >> \"old-style\" repositories. I like this one. Checking both locations\n> >> is nice.\n> >\n> > BTW, since it seems we all agree on the direction. Should we add some\n> > tests?\n> >\n> \n> Good call. What do we want to test for?\n> * Correctness in case of submodules? (just push and get rejected)\n> I think that is easy to do.\n> * Performance with [no, lots of] submodules? That seems harder to me.\n> \n> I'll add a test for the correctness part and resend.\n\nWell I though about the following tests:\n\n * Without submodules: Make sure submodule processing is disabled by\n default\n * With new-style submodules: Make sure submodules are processed by\n default\n * With old-style submodules: Make sure submodules are processed by\n default\n\nBut I have to admit that I did not think about the \"how can we do that\".\nBut performance seems to be the only thing that is exposing the\nprocessing when we have no submodules, so it seems we can only easily do\nthe tests with submodules.\n\nCheers Heiko\n"}]}