threads / patch / 43871

patchpush: change submodule default to check

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

## tl;dr

5 messages between Aug 17, 2016 and Aug 24, 2016. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Stefan Beller· Aug 17, 2016, 20:48 UTC · lore

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(-)
Show changes to builtin/push.c +1 −1
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
Junio C Hamano· Aug 17, 2016, 21:05 UTC · re: Stefan Beller · lore

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

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.
Show 5 quoted lines
> 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? ;-)

Stefan Beller· Aug 17, 2016, 21:14 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 17, 2016 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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. ;)

Show 5 quoted lines
>
>> 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.
Show 10 quoted lines
>
>> 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

Stefan Beller· Aug 24, 2016, 16:46 UTC · lore

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

On Thu, Aug 18, 2016 at 7:09 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
Show 21 quoted lines
> 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

Junio C Hamano· Aug 24, 2016, 18:08 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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?

← back to recent threads