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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2016, 18:38 UTC
Message-ID: <xmqqshtuovvx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160824173017.24782-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 21 quoted lines

> 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.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Slightly reworded commit message than in v1,
> (https://public-inbox.org/git/20160817204848.8983-1-sbeller@google.com/)
> The patch itself is however the same.
>
> I just push it out now with a new commit message, such that we can easier
> pick it up later for Git 3.0, when changes that change default make more sense.
>
> As said in an earlier message, you could however also argue that this is
> fixing a bug in your workflow, so it might be worth fixing before 3.0
> as well. I dunno.

With this change suddenly landing on the version of Git a user just updated, the only possible negative effect would be that somebody who did not configure push.recurseSubmodules suddenly starts getting an error. What would the error message say?

What I want you to think about and explain in the justification is if it gives the user enough information to decide the best course of action to adjust to the new world order, when the user's expectation has been that the superproject gets pushed independent from its submodules. If the existing error message gives enough information, explains why Git suddenly started refusing the push is generally a good thing for the user, tells some minority users (in whose workflow it is perfectly safe to push out the toplevel independently from the submodule) how to turn it back to the old behaviour clearly enough, then this single-liner may be good enough. Otherwise we may need a new logic to allow us to see if recurse_submodules that is set to RECURSE_SUBMODULES_CHECK is because the user explicitly configured, or because the user did not have any configuration, and issue a different message in the latter case.

Show 16 quoted lines

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