Slow pushes on 'pu' - even when up-to-date..

Slow pushes on 'pu' - even when up-to-date..

[Linus Torvalds]

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

Re: Slow pushes on 'pu' - even when up-to-date..

[Stefan Beller]

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.

Re: Slow pushes on 'pu' - even when up-to-date..

[Stefan Beller]

+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

Re: Slow pushes on 'pu' - even when up-to-date..

[Junio C Hamano]

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/).

Re: Slow pushes on 'pu' - even when up-to-date..

[Heiko Voigt]

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

Re: Slow pushes on 'pu' - even when up-to-date..

[Jeff King]

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/

Re: Re: Slow pushes on 'pu' - even when up-to-date..

[Heiko Voigt]

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/

Re: Re: Slow pushes on 'pu' - even when up-to-date..

[Jeff King]

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

Re: Slow pushes on 'pu' - even when up-to-date..

[Junio C Hamano]

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?

Re: Slow pushes on 'pu' - even when up-to-date..

[Jeff King]

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

[PATCH] push: change submodule default to check

[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

Re: [PATCH] push: change submodule default to check

[Jeff King]

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

Re: [PATCH] push: change submodule default to check

[Stefan Beller]

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

Re: [PATCH] push: change submodule default to check

[Jeff King]

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

Re: [PATCH] push: change submodule default to check

[Junio C Hamano]

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.

Re: [PATCH] push: change submodule default to check

[Stefan Beller]

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.

Re: [PATCH] push: change submodule default to check

[Jeff King]

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

[PATCHv2 1/2] push: change submodule default to check when submodules exist

[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

[PATCH 2/2] builtin/push: move flags do_push

[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

Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist

[Jeff King]

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

Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

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

[PATCHv3 1/2] push: change submodule default to check when submodules exist

[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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Heiko Voigt]

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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Jeff King]

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

Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist

[Stefan Beller]

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

Re: [PATCH] push: change submodule default to check

[Junio C Hamano]

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);

Re: [PATCH] push: change submodule default to check

[Junio C Hamano]

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?

Re: [PATCH] push: change submodule default to check

[Stefan Beller]

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.