* Slow pushes on 'pu' - even when up-to-date.. @ 2016-10-03 21:11 Linus Torvalds 2016-10-03 21:17 ` Stefan Beller 2016-10-04 11:18 ` Heiko Voigt 0 siblings, 2 replies; 31+ messages in thread From: Linus Torvalds @ 2016-10-03 21:11 UTC (permalink / raw) To: Junio C Hamano, Stefan Beller; +Cc: Git Mailing List This seems to be because I'm now on 'pu' as of a day or two ago in order to test the abbrev logic, but lookie here: time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux .. shows all the branches and tags .. real 0m0.655s user 0m0.011s sys 0m0.004s so the remote is fast to connect to, and with network connection overhead and everything, it's just over half a second. But then: time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux and it just sits there, and it's at 100% CPU the whole time, until it says: Everything up-to-date real 1m7.307s user 1m2.761s sys 0m0.475s Whaa? It took a *minute* of CPU time to decide that everything was up-to-date? That's just not right. The branch is entirely up-to-date: git rev-parse HEAD af79ad2b1f337a00aa150b993635b10bc68dc842 git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux master af79ad2b1f337a00aa150b993635b10bc68dc842 refs/heads/master so there should be no need for any history walking. But it sure is doing *something*. A minute of CPU time on my machine is actually a pretty damn big deal. Looking at the trace, there's no IO - there's no back-and-forth about "I have this, do you have it?" or anything like that. The system call trace is just a lot of allocations, which I think means that "git push" is walking a lot of objects but not doing anything useful. I bisected it to commit 60cd66f "push: change submodule default to check", which makes little sense since I have no submodules, but there you go.. Apparently RECURSE_SUBMODULES_CHECK is just terminally broken. Linus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-03 21:11 Slow pushes on 'pu' - even when up-to-date Linus Torvalds @ 2016-10-03 21:17 ` Stefan Beller 2016-10-03 21:23 ` Stefan Beller 2016-10-04 11:18 ` Heiko Voigt 1 sibling, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-03 21:17 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List On Mon, Oct 3, 2016 at 2:11 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > This seems to be because I'm now on 'pu' as of a day or two ago in > order to test the abbrev logic, but lookie here: > > time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux > .. shows all the branches and tags .. > real 0m0.655s > user 0m0.011s > sys 0m0.004s > > so the remote is fast to connect to, and with network connection > overhead and everything, it's just over half a second. But then: > > time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux > > and it just sits there, and it's at 100% CPU the whole time, until it says: > > Everything up-to-date > > real 1m7.307s > user 1m2.761s > sys 0m0.475s > > Whaa? It took a *minute* of CPU time to decide that everything was up-to-date? > > That's just not right. The branch is entirely up-to-date: > > git rev-parse HEAD > af79ad2b1f337a00aa150b993635b10bc68dc842 > > git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux master > af79ad2b1f337a00aa150b993635b10bc68dc842 refs/heads/master > > so there should be no need for any history walking. But it sure is > doing *something*. A minute of CPU time on my machine is actually a > pretty damn big deal. > > Looking at the trace, there's no IO - there's no back-and-forth about > "I have this, do you have it?" or anything like that. The system call > trace is just a lot of allocations, which I think means that "git > push" is walking a lot of objects but not doing anything useful. > > I bisected it to commit 60cd66f "push: change submodule default to > check", which makes little sense since I have no submodules, but there > you go.. Apparently RECURSE_SUBMODULES_CHECK is just terminally > broken. Sorry for breaking you, too. Junio complained about that when I was proposing the topic; but now the strategy seems to just wait until Heiko fixed the RECURSE_SUBMODULES_CHECK to be less broken. * sb/push-make-submodule-check-the-default (2016-08-24) 1 commit - push: change submodule default to check Turn the default of "push.recurseSubmodules" to "check". Will hold to wait for hv/submodule-not-yet-pushed-fix This reveals that the "check" mode is too inefficient to use in real projects, even in ones as small as git itself. cf. <xmqqh9aaot49.fsf@gitster.mtv.corp.google.com> Which itself is * hv/submodule-not-yet-pushed-fix (2016-09-14) 2 commits - serialize collection of refs that contain submodule changes - serialize collection of changed submodules The code in "git push" to compute if any commit being pushed in the superproject binds a commit in a submodule that hasn't been pushed out was overly inefficient, making it unusable even for a small project that does not have any submodule but have a reasonable number of refs. The last two in the original series seem to break a few tests when queued to 'pu', and dropped for now. Waiting for a reroll. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-03 21:17 ` Stefan Beller @ 2016-10-03 21:23 ` Stefan Beller 2016-10-03 22:06 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-03 21:23 UTC (permalink / raw) To: Linus Torvalds, Heiko Voigt; +Cc: Junio C Hamano, Git Mailing List +cc Heiko On Mon, Oct 3, 2016 at 2:17 PM, Stefan Beller <sbeller@google.com> wrote: > > * sb/push-make-submodule-check-the-default (2016-08-24) 1 commit > - push: change submodule default to check > > Turn the default of "push.recurseSubmodules" to "check". > > Will hold to wait for hv/submodule-not-yet-pushed-fix > > This reveals that the "check" mode is too inefficient to use in > real projects, even in ones as small as git itself. > cf. <xmqqh9aaot49.fsf@gitster.mtv.corp.google.com> So maybe we should eject this series from pu as long as hv/submodule-not-yet-pushed-fix is ejected to enable you running pu happily. Thanks, Stefan ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-03 21:23 ` Stefan Beller @ 2016-10-03 22:06 ` Junio C Hamano 0 siblings, 0 replies; 31+ messages in thread From: Junio C Hamano @ 2016-10-03 22:06 UTC (permalink / raw) To: Stefan Beller; +Cc: Linus Torvalds, Heiko Voigt, Git Mailing List Stefan Beller <sbeller@google.com> writes: > On Mon, Oct 3, 2016 at 2:17 PM, Stefan Beller <sbeller@google.com> wrote: >> >> * sb/push-make-submodule-check-the-default (2016-08-24) 1 commit >> - push: change submodule default to check >> >> Turn the default of "push.recurseSubmodules" to "check". >> >> Will hold to wait for hv/submodule-not-yet-pushed-fix >> >> This reveals that the "check" mode is too inefficient to use in >> real projects, even in ones as small as git itself. >> cf. <xmqqh9aaot49.fsf@gitster.mtv.corp.google.com> > > So maybe we should eject this series from pu as long as > hv/submodule-not-yet-pushed-fix is ejected to enable you > running pu happily. I am planning to merge lt/abbrev-auto to 'next' together with Peff's ambiguous-short-object-names series in today's pushout. Just FYI, there is another integration branch called 'jch' that typically has several topics more than 'next' but does not merge things I haven't looked at (or things I have looked at and decided not ready). That is what I use for my daily work. You can grab it out of "git log --oneline --first-parent master..pu", or from my broken-out repository (git://github.com/gitster/git/). ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-03 21:11 Slow pushes on 'pu' - even when up-to-date Linus Torvalds 2016-10-03 21:17 ` Stefan Beller @ 2016-10-04 11:18 ` Heiko Voigt 2016-10-04 11:44 ` Jeff King 1 sibling, 1 reply; 31+ messages in thread From: Heiko Voigt @ 2016-10-04 11:18 UTC (permalink / raw) To: Linus Torvalds; +Cc: Junio C Hamano, Stefan Beller, Git Mailing List Hi, On Mon, Oct 03, 2016 at 02:11:36PM -0700, Linus Torvalds wrote: > This seems to be because I'm now on 'pu' as of a day or two ago in > order to test the abbrev logic, but lookie here: > > time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux > .. shows all the branches and tags .. > real 0m0.655s > user 0m0.011s > sys 0m0.004s > > so the remote is fast to connect to, and with network connection > overhead and everything, it's just over half a second. But then: > > time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux The reason behind this is when pushing to an address we do not easily have the remote refs to compare available. When pushing an existing ref it would be easy and could get a shortcut but it gets more complicated for new refs. Currently we fall back to walking the whole history since that is "the most correct way" we have. But obviously it is not a practical solution in any way. I mentioned this fact when discussing the current state and my patches to make this check less painful. So we still need to think about a solution for this check when passing an address. IMO: It's definitely not ready to be switched on as default, unless we find something a lot cheaper for the above case. My idea of a solution goes like this: * collect all SHA1's of the remotes refs * check if we have them locally * if not we abort and tell the user to fetch them somehow into local refs or disable the check * when we have them locally we proceed passing those SHA1's as bases instead of --remotes=<name> Cheers Heiko ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-04 11:18 ` Heiko Voigt @ 2016-10-04 11:44 ` Jeff King 2016-10-04 12:04 ` Heiko Voigt 2016-10-04 16:19 ` Junio C Hamano 0 siblings, 2 replies; 31+ messages in thread From: Jeff King @ 2016-10-04 11:44 UTC (permalink / raw) To: Heiko Voigt Cc: Linus Torvalds, Junio C Hamano, Stefan Beller, Git Mailing List On Tue, Oct 04, 2016 at 01:18:45PM +0200, Heiko Voigt wrote: > On Mon, Oct 03, 2016 at 02:11:36PM -0700, Linus Torvalds wrote: > > This seems to be because I'm now on 'pu' as of a day or two ago in > > order to test the abbrev logic, but lookie here: > > > > time git ls-remote ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux > > .. shows all the branches and tags .. > > real 0m0.655s > > user 0m0.011s > > sys 0m0.004s > > > > so the remote is fast to connect to, and with network connection > > overhead and everything, it's just over half a second. But then: > > > > time git push ra.kernel.org:/pub/scm/linux/kernel/git/torvalds/linux > > The reason behind this is when pushing to an address we do not easily > have the remote refs to compare available. When pushing an existing ref > it would be easy and could get a shortcut but it gets more complicated > for new refs. Currently we fall back to walking the whole history since > that is "the most correct way" we have. But obviously it is not a > practical solution in any way. > > I mentioned this fact when discussing the current state and my patches > to make this check less painful. So we still need to think about a > solution for this check when passing an address. > > IMO: It's definitely not ready to be switched on as default, unless we > find something a lot cheaper for the above case. > > My idea of a solution goes like this: > * collect all SHA1's of the remotes refs > * check if we have them locally > * if not we abort and tell the user to fetch them somehow into local > refs or disable the check > * when we have them locally we proceed passing those SHA1's as bases > instead of --remotes=<name> As I argued in [1], I think it's not just "this must be cheaper" but "this must not be enabled if submodules are not in use at all". Most repositories don't have submodules enabled at all, so anything that cause any extra traversal, even of a portion of the history, is going to be a net negative for a lot of people. I think the only sane default is going to be some kind of heuristic that says "submodules are probably in use". Something like "is there a .gitmodules file" is not perfect (you can have gitlink entries without it), but it's a really cheap constant-time check. -Peff [1] Quoted in http://public-inbox.org/git/xmqqh9aaot49.fsf@gitster.mtv.corp.google.com/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-04 11:44 ` Jeff King @ 2016-10-04 12:04 ` Heiko Voigt 2016-10-04 12:07 ` Jeff King 2016-10-04 16:19 ` Junio C Hamano 1 sibling, 1 reply; 31+ messages in thread From: Heiko Voigt @ 2016-10-04 12:04 UTC (permalink / raw) To: Jeff King; +Cc: Linus Torvalds, Junio C Hamano, Stefan Beller, Git Mailing List On Tue, Oct 04, 2016 at 07:44:28AM -0400, Jeff King wrote: > > My idea of a solution goes like this: > > * collect all SHA1's of the remotes refs > > * check if we have them locally > > * if not we abort and tell the user to fetch them somehow into local > > refs or disable the check > > * when we have them locally we proceed passing those SHA1's as bases > > instead of --remotes=<name> > > As I argued in [1], I think it's not just "this must be cheaper" but > "this must not be enabled if submodules are not in use at all". Most > repositories don't have submodules enabled at all, so anything that > cause any extra traversal, even of a portion of the history, is going to > be a net negative for a lot of people. > > I think the only sane default is going to be some kind of heuristic that > says "submodules are probably in use". Something like "is there a > .gitmodules file" is not perfect (you can have gitlink entries without > it), but it's a really cheap constant-time check. I agree. We are adding convenience for submodules, so we can also say a checked out ".gitmodules" file is a must to have convenience. I am not sure if I agree on another layer of options for this as suggested in your post. More options mean more implementation complexity and more confusion on the users side. How about we choose our defaults based on the existence of a checked out .gitmodules file? So the default would only be --recurse-submodules=check if there is a .gitmodules file in the worktree. All other users need to either pass or explicitly configure it. Cheers Heiko > [1] Quoted in > http://public-inbox.org/git/xmqqh9aaot49.fsf@gitster.mtv.corp.google.com/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-04 12:04 ` Heiko Voigt @ 2016-10-04 12:07 ` Jeff King 0 siblings, 0 replies; 31+ messages in thread From: Jeff King @ 2016-10-04 12:07 UTC (permalink / raw) To: Heiko Voigt Cc: Linus Torvalds, Junio C Hamano, Stefan Beller, Git Mailing List On Tue, Oct 04, 2016 at 02:04:21PM +0200, Heiko Voigt wrote: > > I think the only sane default is going to be some kind of heuristic that > > says "submodules are probably in use". Something like "is there a > > .gitmodules file" is not perfect (you can have gitlink entries without > > it), but it's a really cheap constant-time check. > > I agree. We are adding convenience for submodules, so we can also say a > checked out ".gitmodules" file is a must to have convenience. > > I am not sure if I agree on another layer of options for this as > suggested in your post. More options mean more implementation > complexity and more confusion on the users side. > > How about we choose our defaults based on the existence of a checked out > .gitmodules file? So the default would only be --recurse-submodules=check > if there is a .gitmodules file in the worktree. All other users need to > either pass or explicitly configure it. That's OK with me. Though you may end up in the long run wanting some name for the default behavior (e.g., if people configure something else and then want to override back to "auto" in some instances), but that can probably come later. -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-04 11:44 ` Jeff King 2016-10-04 12:04 ` Heiko Voigt @ 2016-10-04 16:19 ` Junio C Hamano 2016-10-04 16:21 ` Jeff King 1 sibling, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2016-10-04 16:19 UTC (permalink / raw) To: Jeff King; +Cc: Heiko Voigt, Linus Torvalds, Stefan Beller, Git Mailing List Jeff King <peff@peff.net> writes: > As I argued in [1], I think it's not just "this must be cheaper" but > "this must not be enabled if submodules are not in use at all". Most > repositories don't have submodules enabled at all, so anything that > cause any extra traversal, even of a portion of the history, is going to > be a net negative for a lot of people. > > I think the only sane default is going to be some kind of heuristic that > says "submodules are probably in use". Why should we even have a default different from today's? If most repositories don't have submodules enabled at all, we can just let those working with submodules enabled to toggle their configuration and that is an very easy to understand solution, no? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Slow pushes on 'pu' - even when up-to-date.. 2016-10-04 16:19 ` Junio C Hamano @ 2016-10-04 16:21 ` Jeff King 2016-10-04 16:40 ` [PATCH] push: change submodule default to check Stefan Beller 0 siblings, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-04 16:21 UTC (permalink / raw) To: Junio C Hamano Cc: Heiko Voigt, Linus Torvalds, Stefan Beller, Git Mailing List On Tue, Oct 04, 2016 at 09:19:01AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > As I argued in [1], I think it's not just "this must be cheaper" but > > "this must not be enabled if submodules are not in use at all". Most > > repositories don't have submodules enabled at all, so anything that > > cause any extra traversal, even of a portion of the history, is going to > > be a net negative for a lot of people. > > > > I think the only sane default is going to be some kind of heuristic that > > says "submodules are probably in use". > > Why should we even have a default different from today's? If most > repositories don't have submodules enabled at all, we can just let > those working with submodules enabled to toggle their configuration > and that is an very easy to understand solution, no? You will not see any complaint from me on that. I was taking for granted that the current default is inconvenient to submodule users, but I don't have any experience myself. -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] push: change submodule default to check 2016-10-04 16:21 ` Jeff King @ 2016-10-04 16:40 ` Stefan Beller 2016-10-04 17:34 ` Jeff King 2016-10-04 18:00 ` [PATCH] push: change submodule default to check Junio C Hamano 0 siblings, 2 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-04 16:40 UTC (permalink / raw) To: peff, gitster; +Cc: git, hvoigt, torvalds, Stefan Beller When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake. Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing. However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .gitmodules file. [1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/ Signed-off-by: Stefan Beller <sbeller@google.com> --- On Tue, Oct 4, 2016 at 9:21 AM, Jeff King <peff@peff.net> wrote: > On Tue, Oct 04, 2016 at 09:19:01AM -0700, Junio C Hamano wrote: >> >> Why should we even have a default different from today's? If most >> repositories don't have submodules enabled at all, we can just let >> those working with submodules enabled to toggle their configuration >> and that is an very easy to understand solution, no? > > You will not see any complaint from me on that. I was taking for granted > that the current default is inconvenient to submodule users, but I don't > have any experience myself. > And there I was trying to help submodule users not shoot in their foot. I think it is one of the problems that causes serious problems, that is easy to fix from the side of Git. This patch replaces sb/push-make-submodule-check-the-default and should be cheap enough for non-submodule users to accept, but still helping submodule users as it seems to be an ok-ish heuristic. (It is possible to use submodules and currently have no .gitmodules file present, because you're in a weird state; then the heuristic fails. By weird state I mean e.g. a bare repository, or you just checked out an ancient version that has no submodules yet, or you deleted it locally for whatever reason.) So how about this patch? Thanks, Stefan builtin/push.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index 3bb9d6b..d7d664a 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -22,7 +22,7 @@ static int deleterefs; static const char *receivepack; static int verbosity; static int progress = -1; -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static int recurse_submodules; static enum transport_family family; static struct push_cas_option cas; @@ -31,6 +31,14 @@ static const char **refspec; static int refspec_nr; static int refspec_alloc; +static void preset_submodule_default(void) +{ + if (file_exists(".gitmodules")) + recurse_submodules = RECURSE_SUBMODULES_CHECK; + else + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +} + static void add_refspec(const char *ref) { refspec_nr++; @@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) }; packet_trace_identity("push"); + preset_submodule_default(); git_config(git_push_config, &flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); set_push_cert_flags(&flags, push_cert); -- 2.10.0.129.g35f6318 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 16:40 ` [PATCH] push: change submodule default to check Stefan Beller @ 2016-10-04 17:34 ` Jeff King 2016-10-04 17:48 ` Stefan Beller 2016-10-04 18:00 ` [PATCH] push: change submodule default to check Junio C Hamano 1 sibling, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-04 17:34 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, hvoigt, torvalds On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote: > >> Why should we even have a default different from today's? If most > >> repositories don't have submodules enabled at all, we can just let > >> those working with submodules enabled to toggle their configuration > >> and that is an very easy to understand solution, no? > > > > You will not see any complaint from me on that. I was taking for granted > > that the current default is inconvenient to submodule users, but I don't > > have any experience myself. > > > > And there I was trying to help submodule users not shoot in their foot. Sorry if my reply came off as snarky. I really did mean it literally. I do not know if the end goal is good or not, so all of my discussion was just assuming it was. So in that vein... > diff --git a/builtin/push.c b/builtin/push.c > index 3bb9d6b..d7d664a 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -22,7 +22,7 @@ static int deleterefs; > static const char *receivepack; > static int verbosity; > static int progress = -1; > -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > +static int recurse_submodules; > static enum transport_family family; > > static struct push_cas_option cas; > @@ -31,6 +31,14 @@ static const char **refspec; > static int refspec_nr; > static int refspec_alloc; > > +static void preset_submodule_default(void) > +{ > + if (file_exists(".gitmodules")) > + recurse_submodules = RECURSE_SUBMODULES_CHECK; > + else > + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > +} This does seem like a reasonable heuristic. I wonder if you want to confirm that we actually have a worktree (and are in it) before looking at file_exists(). It's unlikely that looking at ".gitmodules" in a bare repo would trigger in practice, but it does not hurt to be careful. -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 17:34 ` Jeff King @ 2016-10-04 17:48 ` Stefan Beller 2016-10-04 17:54 ` Jeff King 0 siblings, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-04 17:48 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 4, 2016 at 10:34 AM, Jeff King <peff@peff.net> wrote: > On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote: > >> >> Why should we even have a default different from today's? If most >> >> repositories don't have submodules enabled at all, we can just let >> >> those working with submodules enabled to toggle their configuration >> >> and that is an very easy to understand solution, no? >> > >> > You will not see any complaint from me on that. I was taking for granted >> > that the current default is inconvenient to submodule users, but I don't >> > have any experience myself. >> > >> >> And there I was trying to help submodule users not shoot in their foot. > > Sorry if my reply came off as snarky. Yeah, sorry about starting being snarky here. >> >> +static void preset_submodule_default(void) >> +{ >> + if (file_exists(".gitmodules")) >> + recurse_submodules = RECURSE_SUBMODULES_CHECK; >> + else >> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; >> +} > > This does seem like a reasonable heuristic. I wonder if you want to > confirm that we actually have a worktree (and are in it) before looking > at file_exists(). It's unlikely that looking at ".gitmodules" in a bare > repo would trigger in practice, but it does not hurt to be careful. In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ? I considered it a non issue, as I don't think many people push from bare repositories. So if we were to check in the HEAD tree instead of the file system, why would we apply different rules for bare and non bare repositories? We probably would not want to do that, so is it reasonable to check for the .gitmodules in the HEAD tree in general in the non bare case? I dunno, it sounds like an equally cheap heuristic covering most cases. Here is another thought: .gitmodules may not exist (either in working dir or in HEADs git tree), so maybe the "correct" heuristic is to check for directories in $GIT_DIR/modules/ That is "more correct", because it is inconceivable to change the submodule pointers without having the submodules checked out. (Who would do that? Why?) > > -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 17:48 ` Stefan Beller @ 2016-10-04 17:54 ` Jeff King 2016-10-04 18:04 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-04 17:54 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 04, 2016 at 10:48:51AM -0700, Stefan Beller wrote: > > This does seem like a reasonable heuristic. I wonder if you want to > > confirm that we actually have a worktree (and are in it) before looking > > at file_exists(). It's unlikely that looking at ".gitmodules" in a bare > > repo would trigger in practice, but it does not hurt to be careful. > > In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ? Yeah, I think that is the closest equivalent. > I considered it a non issue, as I don't think many people push from > bare repositories. I'd also agree, and I have no problem if there simply _isn't_ an auto heuristic for bare repos. Mostly I just thought blindly calling file_exists() was ugly (especially after all the recent "whoops, we look at .git/config in the wrong directory" fixes I've been doing lately). > Here is another thought: > .gitmodules may not exist (either in working dir or in HEADs git > tree), so maybe the > "correct" heuristic is to check for directories in $GIT_DIR/modules/ > That is "more correct", because it is inconceivable to change the submodule > pointers without having the submodules checked out. (Who would do that? Why?) Actually, I like that a bit better. It would not cover the case where you have not actually checked out any of the submodules (or at least not called "submodule init", I guess?). But arguably that is a sign that the auto-recurse behavior should not be kicking in anyway. Bearing in mind that I am not too familiar with what's normal in the submodule world, and so might be spouting nonsense. :) -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 17:54 ` Jeff King @ 2016-10-04 18:04 ` Junio C Hamano 2016-10-04 18:08 ` Stefan Beller 0 siblings, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2016-10-04 18:04 UTC (permalink / raw) To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Linus Torvalds Jeff King <peff@peff.net> writes: > Actually, I like that a bit better. It would not cover the case where > you have not actually checked out any of the submodules (or at least not > called "submodule init", I guess?). But arguably that is a sign that the > auto-recurse behavior should not be kicking in anyway. Yeah, the "no init, no recursion" line of thought is very sensible. I like it. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 18:04 ` Junio C Hamano @ 2016-10-04 18:08 ` Stefan Beller 2016-10-04 18:28 ` Jeff King 0 siblings, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-04 18:08 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote: > Jeff King <peff@peff.net> writes: > >> Actually, I like that a bit better. It would not cover the case where >> you have not actually checked out any of the submodules (or at least not >> called "submodule init", I guess?). But arguably that is a sign that the >> auto-recurse behavior should not be kicking in anyway. > > Yeah, the "no init, no recursion" line of thought is very sensible. > I like it. > Bear in mind that "submodule init" only fuzzes around with .git/config. It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done with the update command. So if we also want to cover the case of init'd submodules, but not cloned/checked out submodules, we'd rather want to consult .git/config whether there is any submodule.* option set, though that seems more expensive than just checking for files inside the modules directory. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 18:08 ` Stefan Beller @ 2016-10-04 18:28 ` Jeff King 2016-10-04 19:29 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Stefan Beller 0 siblings, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-04 18:28 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 04, 2016 at 11:08:33AM -0700, Stefan Beller wrote: > On Tue, Oct 4, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote: > > Jeff King <peff@peff.net> writes: > > > >> Actually, I like that a bit better. It would not cover the case where > >> you have not actually checked out any of the submodules (or at least not > >> called "submodule init", I guess?). But arguably that is a sign that the > >> auto-recurse behavior should not be kicking in anyway. > > > > Yeah, the "no init, no recursion" line of thought is very sensible. > > I like it. > > Bear in mind that "submodule init" only fuzzes around with .git/config. > It doesn't touch .git/modules (i.e. cloning/fetching), that is to be done > with the update command. > > So if we also want to cover the case of init'd submodules, but not > cloned/checked out submodules, we'd rather want to consult .git/config > whether there is any submodule.* option set, though that seems more > expensive than just checking for files inside the modules directory. Consulting .git/config is fine, I think. It's not like we don't read it (sometimes multiple times!) during the normal course of the program anyway. It's just a question of whether it makes more sense for the heuristic to kick in after "init", or only after "update". I don't know enough to have an opinion. -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCHv2 1/2] push: change submodule default to check when submodules exist 2016-10-04 18:28 ` Jeff King @ 2016-10-04 19:29 ` Stefan Beller 2016-10-04 19:29 ` [PATCH 2/2] builtin/push: move flags do_push Stefan Beller 2016-10-04 19:39 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Jeff King 0 siblings, 2 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-04 19:29 UTC (permalink / raw) To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake. Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing. However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .git/modules directory. That directory doesn't exist when no submodules are used and is only created and populated when submodules are cloned/added. When the submodule directory doesn't exist, a user may have changed the gitlinks via plumbing commands. Currently the default is to not check. RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently, though it may change in the future. When no submodules exist such a check is pointless as it would fail anyway, so let's just turn it off. [1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/ Signed-off-by: Stefan Beller <sbeller@google.com> --- Jeff wrote: > Consulting .git/config is fine, I think. It's not like we don't read it > (sometimes multiple times!) during the normal course of the program > anyway. It's just a question of whether it makes more sense for the > heuristic to kick in after "init", or only after "update". I don't know > enough to have an opinion. I think there is no difference in practice, however the "after update" is way easier to implement and hence more maintainable (one lstat instead of fiddeling with the config; that can go wrong easily). Thanks, Stefan builtin/push.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index 3bb9d6b..06fd3bd 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -3,6 +3,7 @@ */ #include "cache.h" #include "refs.h" +#include "dir.h" #include "run-command.h" #include "builtin.h" #include "remote.h" @@ -22,7 +23,7 @@ static int deleterefs; static const char *receivepack; static int verbosity; static int progress = -1; -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static int recurse_submodules; static enum transport_family family; static struct push_cas_option cas; @@ -31,6 +32,19 @@ static const char **refspec; static int refspec_nr; static int refspec_alloc; +static void preset_submodule_default(void) +{ + struct strbuf sb = STRBUF_INIT; + strbuf_addf(&sb, "%s/modules", get_git_dir()); + + if (file_exists(sb.buf)) + recurse_submodules = RECURSE_SUBMODULES_CHECK; + else + recurse_submodules = RECURSE_SUBMODULES_OFF; + + strbuf_release(&sb); +} + static void add_refspec(const char *ref) { refspec_nr++; @@ -552,6 +566,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) }; packet_trace_identity("push"); + preset_submodule_default(); git_config(git_push_config, &flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); set_push_cert_flags(&flags, push_cert); -- 2.10.0.129.g35f6318 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/2] builtin/push: move flags do_push 2016-10-04 19:29 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Stefan Beller @ 2016-10-04 19:29 ` Stefan Beller 2016-10-04 19:39 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Jeff King 1 sibling, 0 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-04 19:29 UTC (permalink / raw) To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller In do_push we set the flags for other options, so let's make the code more consistent and also apply the flags for recursing into submodules there. Signed-off-by: Stefan Beller <sbeller@google.com> --- No functional change intended, just a cleanup while we're at it. Feel free to drop if it's too much churn. builtin/push.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 06fd3bd..6690301 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -388,6 +388,11 @@ static int do_push(const char *repo, int flags, " git push <name>\n")); } + if (recurse_submodules == RECURSE_SUBMODULES_CHECK) + flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK; + else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) + flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND; + if (remote->mirror) flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE); @@ -576,11 +581,6 @@ int cmd_push(int argc, const char **argv, const char *prefix) if (deleterefs && argc < 2) die(_("--delete doesn't make sense without any refs")); - if (recurse_submodules == RECURSE_SUBMODULES_CHECK) - flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK; - else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) - flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND; - if (tags) add_refspec("refs/tags/*"); -- 2.10.0.129.g35f6318 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist 2016-10-04 19:29 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Stefan Beller 2016-10-04 19:29 ` [PATCH 2/2] builtin/push: move flags do_push Stefan Beller @ 2016-10-04 19:39 ` Jeff King 2016-10-04 19:51 ` Stefan Beller 1 sibling, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-04 19:39 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, hvoigt, torvalds On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote: > Jeff wrote: > > Consulting .git/config is fine, I think. It's not like we don't read it > > (sometimes multiple times!) during the normal course of the program > > anyway. It's just a question of whether it makes more sense for the > > heuristic to kick in after "init", or only after "update". I don't know > > enough to have an opinion. > > I think there is no difference in practice, however the "after update" > is way easier to implement and hence more maintainable (one lstat instead of > fiddeling with the config; that can go wrong easily). Hmm, I would have thought it is the opposite; can't submodules exist in the working tree in their own ".git" directory? I know that's the "old" way of doing it, but I didn't know if it was totally deprecated. Anyway, the config version is probably just: int config_check_submodule(const char *var, const char *value, void *data) { if (starts_with(var, "submodule.") && ends_with(var, ".path")) *(int *)data = 1; return 0; } ... int have_submodule = 0; git_config(config_check_submodule, &have_submodule); But I don't care too much either way. that's just for reference. > @@ -31,6 +32,19 @@ static const char **refspec; > static int refspec_nr; > static int refspec_alloc; > > +static void preset_submodule_default(void) > +{ > + struct strbuf sb = STRBUF_INIT; > + strbuf_addf(&sb, "%s/modules", get_git_dir()); > + > + if (file_exists(sb.buf)) Maybe just: if (file_exists(git_path("modules")) ? -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist 2016-10-04 19:39 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Jeff King @ 2016-10-04 19:51 ` Stefan Beller 2016-10-04 21:03 ` [PATCHv3 " Stefan Beller 0 siblings, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-04 19:51 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 4, 2016 at 12:39 PM, Jeff King <peff@peff.net> wrote: > On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote: > >> Jeff wrote: >> > Consulting .git/config is fine, I think. It's not like we don't read it >> > (sometimes multiple times!) during the normal course of the program >> > anyway. It's just a question of whether it makes more sense for the >> > heuristic to kick in after "init", or only after "update". I don't know >> > enough to have an opinion. >> >> I think there is no difference in practice, however the "after update" >> is way easier to implement and hence more maintainable (one lstat instead of >> fiddeling with the config; that can go wrong easily). > > Hmm, I would have thought it is the opposite; can't submodules exist in > the working tree in their own ".git" directory? I know that's the "old" > way of doing it, but I didn't know if it was totally deprecated. Oo, right. :( The proposed patch would not change the current behavior for a layout of .git directories inside the submodule working trees, actually. It would however also not have the desired effect of enabling the check for push. However these not-yet-deprecated layouts are likely in use by people who know what they are doing, so maybe we can punt on that. > > Anyway, the config version is probably just: > > int config_check_submodule(const char *var, const char *value, void *data) > { > if (starts_with(var, "submodule.") && ends_with(var, ".path")) s/.path/.url/ but I get the point. I do dislike this solution for another reasons though: In the future when worktree supports submodules we either have the url per worktree,so we'd need to process all working tree configs as well or we do it the proper way, which is replacing the submodule.$name.url variable with 2 options, one is purely used to configure the URL and the other is purely used to indicate the existence of a submodule. Piling on the mixed use case of urls today feels sad. > *(int *)data = 1; > return 0; > } > > ... > int have_submodule = 0; > git_config(config_check_submodule, &have_submodule); > > But I don't care too much either way. that's just for reference. > >> @@ -31,6 +32,19 @@ static const char **refspec; >> static int refspec_nr; >> static int refspec_alloc; >> >> +static void preset_submodule_default(void) >> +{ >> + struct strbuf sb = STRBUF_INIT; >> + strbuf_addf(&sb, "%s/modules", get_git_dir()); >> + >> + if (file_exists(sb.buf)) > > Maybe just: > > if (file_exists(git_path("modules")) Sounds good. So I'll see if I can get the version running you propose here, otherwise I'll resend with these changes. > > ? > > -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-04 19:51 ` Stefan Beller @ 2016-10-04 21:03 ` Stefan Beller 2016-10-05 13:53 ` Heiko Voigt 2016-10-05 15:47 ` Jeff King 0 siblings, 2 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-04 21:03 UTC (permalink / raw) To: gitster; +Cc: git, peff, hvoigt, torvalds, Stefan Beller When working with submodules, it is easy to forget to push the submodules. The setting 'check', which checks if any existing submodule is present on at least one remote of the submodule remotes, is designed to prevent this mistake. Flipping the default to check for submodules is safer than the current default of ignoring submodules while pushing. However checking for submodules requires additional work[1], which annoys users that do not use submodules, so we turn on the check for submodules based on a cheap heuristic, the existence of the .git/modules directory. That directory doesn't exist when no submodules are used and is only created and populated when submodules are cloned/added. When the submodule directory doesn't exist, a user may have changed the gitlinks via plumbing commands. Currently the default is to not check. RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently, though it may change in the future. When no submodules exist such a check is pointless as it would fail anyway, so let's just turn it off. [1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/ Signed-off-by: Stefan Beller <sbeller@google.com> --- Jeff, thanks for the suggestions, both git_path(..) as well as checking the config, this seems quite readable to me: builtin/push.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index 3bb9d6b..65bb1df 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -3,6 +3,7 @@ */ #include "cache.h" #include "refs.h" +#include "dir.h" #include "run-command.h" #include "builtin.h" #include "remote.h" @@ -22,6 +23,7 @@ static int deleterefs; static const char *receivepack; static int verbosity; static int progress = -1; +static int has_submodules; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; static enum transport_family family; @@ -31,6 +33,14 @@ static const char **refspec; static int refspec_nr; static int refspec_alloc; +static void preset_submodule_default(void) +{ + if (has_submodules || file_exists(git_path("modules"))) + recurse_submodules = RECURSE_SUBMODULES_CHECK; + else + recurse_submodules = RECURSE_SUBMODULES_OFF; +} + static void add_refspec(const char *ref) { refspec_nr++; @@ -495,7 +505,8 @@ static int git_push_config(const char *k, const char *v, void *cb) const char *value; if (!git_config_get_value("push.recursesubmodules", &value)) recurse_submodules = parse_push_recurse_submodules_arg(k, value); - } + } else if (starts_with(k, "submodule.") && ends_with(k, ".url")) + has_submodules = 1; return git_default_config(k, v, NULL); } @@ -552,6 +563,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) }; packet_trace_identity("push"); + preset_submodule_default(); git_config(git_push_config, &flags); argc = parse_options(argc, argv, prefix, options, push_usage, 0); set_push_cert_flags(&flags, push_cert); -- 2.10.0.129.g35f6318 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-04 21:03 ` [PATCHv3 " Stefan Beller @ 2016-10-05 13:53 ` Heiko Voigt 2016-10-06 9:23 ` Heiko Voigt 2016-10-05 15:47 ` Jeff King 1 sibling, 1 reply; 31+ messages in thread From: Heiko Voigt @ 2016-10-05 13:53 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, peff, torvalds On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > Jeff, > thanks for the suggestions, both git_path(..) as well as checking the config, > this seems quite readable to me: When reading the discussion I thought the same: What about the "old-style" repositories. I like this one. Checking both locations is nice. Cheers Heiko ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-05 13:53 ` Heiko Voigt @ 2016-10-06 9:23 ` Heiko Voigt 2016-10-06 17:20 ` Stefan Beller 0 siblings, 1 reply; 31+ messages in thread From: Heiko Voigt @ 2016-10-06 9:23 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, peff, torvalds On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote: > On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > > Jeff, > > thanks for the suggestions, both git_path(..) as well as checking the config, > > this seems quite readable to me: > > When reading the discussion I thought the same: What about the > "old-style" repositories. I like this one. Checking both locations > is nice. BTW, since it seems we all agree on the direction. Should we add some tests? Cheers Heiko ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-06 9:23 ` Heiko Voigt @ 2016-10-06 17:20 ` Stefan Beller 2016-10-07 12:41 ` Heiko Voigt 0 siblings, 1 reply; 31+ messages in thread From: Stefan Beller @ 2016-10-06 17:20 UTC (permalink / raw) To: Heiko Voigt Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Linus Torvalds On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote: >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: >> > Jeff, >> > thanks for the suggestions, both git_path(..) as well as checking the config, >> > this seems quite readable to me: >> >> When reading the discussion I thought the same: What about the >> "old-style" repositories. I like this one. Checking both locations >> is nice. > > BTW, since it seems we all agree on the direction. Should we add some > tests? > Good call. What do we want to test for? * Correctness in case of submodules? (just push and get rejected) I think that is easy to do. * Performance with [no, lots of] submodules? That seems harder to me. I'll add a test for the correctness part and resend. Thanks, Stefan ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-06 17:20 ` Stefan Beller @ 2016-10-07 12:41 ` Heiko Voigt 0 siblings, 0 replies; 31+ messages in thread From: Heiko Voigt @ 2016-10-07 12:41 UTC (permalink / raw) To: Stefan Beller Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Linus Torvalds On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote: > On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote: > > On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote: > >> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > >> > Jeff, > >> > thanks for the suggestions, both git_path(..) as well as checking the config, > >> > this seems quite readable to me: > >> > >> When reading the discussion I thought the same: What about the > >> "old-style" repositories. I like this one. Checking both locations > >> is nice. > > > > BTW, since it seems we all agree on the direction. Should we add some > > tests? > > > > Good call. What do we want to test for? > * Correctness in case of submodules? (just push and get rejected) > I think that is easy to do. > * Performance with [no, lots of] submodules? That seems harder to me. > > I'll add a test for the correctness part and resend. Well I though about the following tests: * Without submodules: Make sure submodule processing is disabled by default * With new-style submodules: Make sure submodules are processed by default * With old-style submodules: Make sure submodules are processed by default But I have to admit that I did not think about the "how can we do that". But performance seems to be the only thing that is exposing the processing when we have no submodules, so it seems we can only easily do the tests with submodules. Cheers Heiko ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-04 21:03 ` [PATCHv3 " Stefan Beller 2016-10-05 13:53 ` Heiko Voigt @ 2016-10-05 15:47 ` Jeff King 2016-10-05 15:54 ` Stefan Beller 1 sibling, 1 reply; 31+ messages in thread From: Jeff King @ 2016-10-05 15:47 UTC (permalink / raw) To: Stefan Beller; +Cc: gitster, git, hvoigt, torvalds On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > thanks for the suggestions, both git_path(..) as well as checking the config, > this seems quite readable to me: > > builtin/push.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) Yeah, this seems like a good compromise to me. I did have one other thought, but I don't think it's worth pursuing now. We care about finding gitlinks in objects we're pushing; is there any process which is already looking at those objects? Genreally, the "pack-objects" doing the push has to. Not to actually push the objects, which often are sent blindly off disk, but to determine the set of reachable objects in the first place. So in theory we could prepare the list of objects to pack, and as a side effect it could say "and here are gitlinks referenced by those objects". But that doesn't work if bitmaps are in effect, because then we don't access the objects directly at all. I think you could solve that by extending the bitmap format to include a bit for gitlinks that are reachable (but not necessarily included in the pack). So I don't think that's worth thinking too much about now, but it might be an interesting optimization down the road. -Peff ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist 2016-10-05 15:47 ` Jeff King @ 2016-10-05 15:54 ` Stefan Beller 0 siblings, 0 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-05 15:54 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Wed, Oct 5, 2016 at 8:47 AM, Jeff King <peff@peff.net> wrote: > On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote: > >> thanks for the suggestions, both git_path(..) as well as checking the config, >> this seems quite readable to me: >> >> builtin/push.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) > > Yeah, this seems like a good compromise to me. > > I did have one other thought, but I don't think it's worth pursuing now. > We care about finding gitlinks in objects we're pushing; is there any > process which is already looking at those objects? Genreally, the > "pack-objects" doing the push has to. Not to actually push the objects, > which often are sent blindly off disk, but to determine the set of > reachable objects in the first place. So in theory we could prepare the > list of objects to pack, and as a side effect it could say "and here are > gitlinks referenced by those objects". But that doesn't work if bitmaps > are in effect, because then we don't access the objects directly at all. > I think you could solve that by extending the bitmap format to include a > bit for gitlinks that are reachable (but not necessarily included in the > pack). > > So I don't think that's worth thinking too much about now, but it might > be an interesting optimization down the road. > > -Peff That sounds like what we'd want down the road. In my imagination the submodules of the future may live completely inside the superproject, i.e. they do not necessarily have their own URL. They can be obtained via fetching the superproject, that automagically also transmits the objects of the submodules. Thanks for the hint w.r.t. the bimaps! Stefan ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 16:40 ` [PATCH] push: change submodule default to check Stefan Beller 2016-10-04 17:34 ` Jeff King @ 2016-10-04 18:00 ` Junio C Hamano 2016-10-04 18:02 ` Junio C Hamano 2016-10-04 18:05 ` Stefan Beller 1 sibling, 2 replies; 31+ messages in thread From: Junio C Hamano @ 2016-10-04 18:00 UTC (permalink / raw) To: Stefan Beller; +Cc: peff, git, hvoigt, torvalds Stefan Beller <sbeller@google.com> writes: > +static void preset_submodule_default(void) > +{ > + if (file_exists(".gitmodules")) Don't we need to see if we are in a bare repository? > + recurse_submodules = RECURSE_SUBMODULES_CHECK; > + else > + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; Hmph, why "_DEFAULT" not "_OFF"? > +} > + > static void add_refspec(const char *ref) > { > refspec_nr++; > @@ -552,6 +560,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) > }; > > packet_trace_identity("push"); > + preset_submodule_default(); > git_config(git_push_config, &flags); > argc = parse_options(argc, argv, prefix, options, push_usage, 0); > set_push_cert_flags(&flags, push_cert); ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 18:00 ` [PATCH] push: change submodule default to check Junio C Hamano @ 2016-10-04 18:02 ` Junio C Hamano 2016-10-04 18:05 ` Stefan Beller 1 sibling, 0 replies; 31+ messages in thread From: Junio C Hamano @ 2016-10-04 18:02 UTC (permalink / raw) To: Stefan Beller; +Cc: peff, git, hvoigt, torvalds Junio C Hamano <gitster@pobox.com> writes: > Stefan Beller <sbeller@google.com> writes: > >> +static void preset_submodule_default(void) >> +{ >> + if (file_exists(".gitmodules")) > > Don't we need to see if we are in a bare repository? > >> + recurse_submodules = RECURSE_SUBMODULES_CHECK; >> + else >> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > > Hmph, why "_DEFAULT" not "_OFF"? ... because you wanted to keep the same behaviour, i.e. keep recurse_submodules set to _DEFAULT just like the compiled-in initialization does. Perhaps we can lose "else" clause altogether? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] push: change submodule default to check 2016-10-04 18:00 ` [PATCH] push: change submodule default to check Junio C Hamano 2016-10-04 18:02 ` Junio C Hamano @ 2016-10-04 18:05 ` Stefan Beller 1 sibling, 0 replies; 31+ messages in thread From: Stefan Beller @ 2016-10-04 18:05 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, git@vger.kernel.org, Heiko Voigt, Linus Torvalds On Tue, Oct 4, 2016 at 11:00 AM, Junio C Hamano <gitster@pobox.com> wrote: > Stefan Beller <sbeller@google.com> writes: > >> +static void preset_submodule_default(void) >> +{ >> + if (file_exists(".gitmodules")) > > Don't we need to see if we are in a bare repository? See discussion with Jeff; instead of checking the file, we rather want to check if $GIT_DIR/modules/ is populated, as that is version agnostic ("Was a submodule initialized and fetched at any time in the life time of this repository?"), as well as bare/non-bare agnostic. > >> + recurse_submodules = RECURSE_SUBMODULES_CHECK; >> + else >> + recurse_submodules = RECURSE_SUBMODULES_DEFAULT; > > Hmph, why "_DEFAULT" not "_OFF"? You answered yourself below, and that was indeed my thought. I was also wondering whether to remove the else, but then I thought that we'd rather do not want to rely on compiled-in at all, and have one init function which sets out the new world order. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2016-10-07 12:42 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-03 21:11 Slow pushes on 'pu' - even when up-to-date Linus Torvalds 2016-10-03 21:17 ` Stefan Beller 2016-10-03 21:23 ` Stefan Beller 2016-10-03 22:06 ` Junio C Hamano 2016-10-04 11:18 ` Heiko Voigt 2016-10-04 11:44 ` Jeff King 2016-10-04 12:04 ` Heiko Voigt 2016-10-04 12:07 ` Jeff King 2016-10-04 16:19 ` Junio C Hamano 2016-10-04 16:21 ` Jeff King 2016-10-04 16:40 ` [PATCH] push: change submodule default to check Stefan Beller 2016-10-04 17:34 ` Jeff King 2016-10-04 17:48 ` Stefan Beller 2016-10-04 17:54 ` Jeff King 2016-10-04 18:04 ` Junio C Hamano 2016-10-04 18:08 ` Stefan Beller 2016-10-04 18:28 ` Jeff King 2016-10-04 19:29 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Stefan Beller 2016-10-04 19:29 ` [PATCH 2/2] builtin/push: move flags do_push Stefan Beller 2016-10-04 19:39 ` [PATCHv2 1/2] push: change submodule default to check when submodules exist Jeff King 2016-10-04 19:51 ` Stefan Beller 2016-10-04 21:03 ` [PATCHv3 " Stefan Beller 2016-10-05 13:53 ` Heiko Voigt 2016-10-06 9:23 ` Heiko Voigt 2016-10-06 17:20 ` Stefan Beller 2016-10-07 12:41 ` Heiko Voigt 2016-10-05 15:47 ` Jeff King 2016-10-05 15:54 ` Stefan Beller 2016-10-04 18:00 ` [PATCH] push: change submodule default to check Junio C Hamano 2016-10-04 18:02 ` Junio C Hamano 2016-10-04 18:05 ` Stefan Beller
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).