[PATCH] push: change submodule default to check

[PATCH] push: change submodule default to check

[Stefan Beller]

When working with submodules, it is easy to forget to push the submodules.
Change the default to 'check' if any existing submodule is present on at
least one remote of the submodule remotes.

This doesn't affect you if you do not work with submodules.
If working with submodules, there are more reports of missing submodules
rather than the desire to push the superproject without the submodules
to be pushed out. Flipping the default to check for submodules is safer
than the current default of ignoring submodules while pushing.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
 Probably too late for the 2.10 release as I'd propose to keep it in master for
 quite a while before actually doing a release with this.
 
 Thanks,
 Stefan

 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..479150a 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 = RECURSE_SUBMODULES_CHECK;
 static enum transport_family family;
 
 static struct push_cas_option cas;
-- 
2.9.2.730.g525ad04.dirty

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> If working with submodules, there are more reports of missing submodules
> rather than the desire to push the superproject without the submodules
> to be pushed out.

I do not know how you are counting the "more reports" part of that
assertion, but it is very likely that it is biased by the current
default.  If you flip the default, you would see more reports that
say "I know I wasn't ready to push the submodule part out; don't bug
me".

IOW, those who want to have something different always sound louder,
because people who are satisified tend to stay silent.

> Flipping the default to check for submodules is safer
> than the current default of ignoring submodules while pushing.

That part of the assertion, on the other hand, is justifiable.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  
>  Probably too late for the 2.10 release as I'd propose to keep it in master for
>  quite a while before actually doing a release with this.

I think you meant 'next' not 'master'.  We have a few "let's keep it
in 'next' to see if people scream" topics there already--the more
the merrier? ;-)

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

[Stefan Beller]

On Wed, Aug 17, 2016 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> If working with submodules, there are more reports of missing submodules
>> rather than the desire to push the superproject without the submodules
>> to be pushed out.
>
> I do not know how you are counting the "more reports" part of that
> assertion, but it is very likely that it is biased by the current
> default.  If you flip the default, you would see more reports that
> say "I know I wasn't ready to push the submodule part out; don't bug
> me".
>
> IOW, those who want to have something different always sound louder,
> because people who are satisified tend to stay silent.

Yeah I thought about this mistake and how to get real numbers, but
I was just misled by some people on #git IRC waving hands. ;)

>
>> Flipping the default to check for submodules is safer
>> than the current default of ignoring submodules while pushing.
>
> That part of the assertion, on the other hand, is justifiable.

ok.

>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>  Probably too late for the 2.10 release as I'd propose to keep it in master for
>>  quite a while before actually doing a release with this.
>
> I think you meant 'next' not 'master'.  We have a few "let's keep it
> in 'next' to see if people scream" topics there already--the more
> the merrier? ;-)

Well we put it into next, but that are not as many people as those
running master I would think, so I would want to maximize both times
in next as well as in master, e.g. if you put it into next today (unreasonable,
but let's assume), then it could make it into master next week, and then be
released as part of 2.10 IIUC the release schedule.

I would say that is too fast. rather I'd see this patch transition from next to
master just after a release, such that it lives in master for a full
release cycle
before being actually in a release.

So far for my line of thinking.

Thanks,
Stefan

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

[Stefan Beller]

On Thu, Aug 18, 2016 at 7:09 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Wed, Aug 17, 2016 at 02:14:11PM -0700, Stefan Beller wrote:
>> On Wed, Aug 17, 2016 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Stefan Beller <sbeller@google.com> writes:
> [...]
>> >> Flipping the default to check for submodules is safer
>> >> than the current default of ignoring submodules while pushing.
>> >
>> > That part of the assertion, on the other hand, is justifiable.
>>
>> ok.
>
> I also think that this is a good reason to flip the default. IMO more
> people will be annoyed by not being able to checkout a certain version
> if someone forgets to push a submodule then people deliberately pushing
> something with a submodule hash that is not on any remote.
>
> At the same time I am wondering whether it makes sense to keep this for
> a bigger version change (like 3.0) or so? Since that is were people will
> expect such changes. Not sure when 3.0 is planned though.
>
> Cheers Heiko

I guess we can postpone it until 3.0, though I currently think it is not a big
issue as it helps avoiding "bugs in your workflow".

On the other hand if you really want to push out the superproject without
the submodules, you need to adapt your behavior (i.e. set an option or
give a command line flag), and such breaking things we should delay
until 3.0.

I think I'll resend it with a proper commit message, such that we can just pick
it up when 3.0 comes around.

Thanks,
Stefan

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I guess we can postpone it until 3.0, though I currently think it is not a big
> issue as it helps avoiding "bugs in your workflow".
>
> On the other hand if you really want to push out the superproject without
> the submodules, you need to adapt your behavior (i.e. set an option or
> give a command line flag), and such breaking things we should delay
> until 3.0.
>
> I think I'll resend it with a proper commit message, such that we can just pick
> it up when 3.0 comes around.

A change that needs to wait until a major version bump implies that
it is possibly compatibility breaking.  So "resend IT", implying one
single step, sounds like a bad sign that the users won't have any
transition period.  Shouldn't we do the usual two-step deprecation
process, i.e. warn when an unconfigured user pushes a superproject
that may be ahead of a submodule about upcoming planned default
change with the first patch, and then flip the default in the second
patch while dropping the warning?

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

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.