[PATCH] push: add recurseSubmodules config option

[PATCH] push: add recurseSubmodules config option

[Mike Crowe]

The --recurse-submodules command line parameter has existed for some
time but it has no config file equivalent.

Following the style of the corresponding parameter for git fetch, let's
invent push.recurseSubmodules to provide a default for this
parameter. This also requires the addition of --recurse-submodules=no to
allow the configuration to be overridden on the command line when
required.

The most straightforward way to implement this appears to be to make
push use code in submodule-config in a similar way to fetch.

Signed-off-by: Mike Crowe <mac@mcrowe.com>
---
 Documentation/config.txt       |  13 +++++
 Documentation/git-push.txt     |   4 +-
 builtin/push.c                 |  37 ++++++++-----
 submodule-config.c             |  20 +++++++
 submodule-config.h             |   1 +
 submodule.h                    |   1 +
 t/t5531-deep-submodule-push.sh | 123 ++++++++++++++++++++++++++++++++++++++++-
 7 files changed, 182 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..0546da5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2226,6 +2226,19 @@ push.gpgSign::
 	override a value from a lower-priority config file. An explicit
 	command-line flag always overrides this config option.
 
+push.recurseSubmodules::
+	Make sure all submodule commits used by the revisions to be pushed
+	are available on a remote-tracking branch. If the value is 'check'
+	then Git will verify that all submodule commits that changed in the
+	revisions to be pushed are available on at least one remote of the
+	submodule. If any commits are missing the push will be aborted and
+	exit with non-zero status. If the value is 'on-demand' then all
+	submodules that changed in the revisions to be pushed will be
+	pushed. If on-demand was not able to push all necessary revisions
+	it will also be aborted and exit with non-zero status. You may
+	override this configuration at time of push by specifying
+	'--recurse-submodules=check|on-demand|no'.
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 85a4d7d..fb0e9b7 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -257,7 +257,7 @@ origin +master` to force a push to the `master` branch). See the
 	is specified. This flag forces progress status even if the
 	standard error stream is not directed to a terminal.
 
---recurse-submodules=check|on-demand::
+--recurse-submodules=check|on-demand|no::
 	Make sure all submodule commits used by the revisions to be
 	pushed are available on a remote-tracking branch. If 'check' is
 	used Git will verify that all submodule commits that changed in
@@ -267,6 +267,8 @@ origin +master` to force a push to the `master` branch). See the
 	all submodules that changed in the revisions to be pushed will
 	be pushed. If on-demand was not able to push all necessary
 	revisions it will also be aborted and exit with non-zero status.
+	A value of 'no' is used to override the push.recurseSubmodules
+	variable when no submodule recursion is required.
 
 --[no-]verify::
 	Toggle the pre-push hook (see linkgit:githooks[5]).  The
diff --git a/builtin/push.c b/builtin/push.c
index 3bda430..dfced74 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -9,6 +9,7 @@
 #include "transport.h"
 #include "parse-options.h"
 #include "submodule.h"
+#include "submodule-config.h"
 #include "send-pack.h"
 
 static const char * const push_usage[] = {
@@ -20,7 +21,7 @@ static int thin = 1;
 static int deleterefs;
 static const char *receivepack;
 static int verbosity;
-static int progress = -1;
+static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 
 static struct push_cas_option cas;
 
@@ -452,22 +453,15 @@ static int do_push(const char *repo, int flags)
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
 {
-	int *flags = opt->value;
+	int *recurse_submodules = opt->value;
 
-	if (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |
-		      TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
+	if (*recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
 		die("%s can only be used once.", opt->long_name);
 
-	if (arg) {
-		if (!strcmp(arg, "check"))
-			*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
-		else if (!strcmp(arg, "on-demand"))
-			*flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
-		else
-			die("bad %s argument: %s", opt->long_name, arg);
-	} else
-		die("option %s needs an argument (check|on-demand)",
-				opt->long_name);
+	if (arg)
+		*recurse_submodules = parse_push_recurse_submodules_arg(opt->long_name, arg);
+	else
+		die("%s missing parameter", opt->long_name);
 
 	return 0;
 }
@@ -522,6 +516,10 @@ static int git_push_config(const char *k, const char *v, void *cb)
 					return error("Invalid value for '%s'", k);
 			}
 		}
+	} else if (!strcmp(k, "push.recursesubmodules")) {
+		const char *value;
+		if (!git_config_get_value("push.recursesubmodules", &value))
+			recurse_submodules = parse_push_recurse_submodules_arg(k, value);
 	}
 
 	return git_default_config(k, v, NULL);
@@ -532,6 +530,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	int flags = 0;
 	int tags = 0;
 	int push_cert = -1;
+	int recurse_submodules_from_cmdline = RECURSE_SUBMODULES_DEFAULT;
 	int rc;
 	const char *repo = NULL;	/* default repository */
 	struct option options[] = {
@@ -549,7 +548,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
 		  N_("require old value of ref to be at this value"),
 		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
-		{ OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check|on-demand",
+		{ OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules_from_cmdline, N_("check|on-demand|no"),
 			N_("control recursive pushing of submodules"),
 			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
 		OPT_BOOL( 0 , "thin", &thin, N_("use thin pack")),
@@ -580,6 +579,14 @@ 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_from_cmdline != RECURSE_SUBMODULES_DEFAULT)
+		recurse_submodules = recurse_submodules_from_cmdline;
+
+	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/*");
 
diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..33d8790 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -228,6 +228,26 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
 	return parse_fetch_recurse(opt, arg, 1);
 }
 
+static int parse_push_recurse(const char *opt, const char *arg,
+			       int die_on_error)
+{
+    if (!strcmp(arg, "on-demand"))
+	return RECURSE_SUBMODULES_ON_DEMAND;
+    else if (!strcmp(arg, "check"))
+	return RECURSE_SUBMODULES_CHECK;
+    else if (!strcmp(arg, "no"))
+	return RECURSE_SUBMODULES_OFF;
+    else if (die_on_error)
+	die("bad %s argument: %s", opt, arg);
+    else
+	return RECURSE_SUBMODULES_ERROR;
+}
+
+int parse_push_recurse_submodules_arg(const char *opt, const char *arg)
+{
+	return parse_push_recurse(opt, arg, 1);
+}
+
 static void warn_multiple_config(const unsigned char *commit_sha1,
 				 const char *name, const char *option)
 {
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..9bfa65a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -19,6 +19,7 @@ struct submodule {
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
+int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
 int parse_submodule_config_option(const char *var, const char *value);
 const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
 		const char *name);
diff --git a/submodule.h b/submodule.h
index 5507c3d..ddff512 100644
--- a/submodule.h
+++ b/submodule.h
@@ -5,6 +5,7 @@ struct diff_options;
 struct argv_array;
 
 enum {
+	RECURSE_SUBMODULES_CHECK = -4,
 	RECURSE_SUBMODULES_ERROR = -3,
 	RECURSE_SUBMODULES_NONE = -2,
 	RECURSE_SUBMODULES_ON_DEMAND = -1,
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 6507487..d2fb072 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -64,7 +64,15 @@ test_expect_success 'push fails if submodule commit not on remote' '
 		cd work &&
 		git add gar/bage &&
 		git commit -m "Third commit for gar/bage" &&
-		test_must_fail git push --recurse-submodules=check ../pub.git master
+		# the push should fail with --recurse-submodules=check
+		# on the command line...
+		test_must_fail git push --recurse-submodules=check ../pub.git master &&
+
+		# ...or if specified in the configuration..
+		git config push.recurseSubmodules check &&
+		test_must_fail git push ../pub.git master &&
+
+		git config --unset push.recurseSubmodules
 	)
 '
 
@@ -79,6 +87,119 @@ test_expect_success 'push succeeds after commit was pushed to remote' '
 	)
 '
 
+test_expect_success 'push succeeds if submodule commit not on remote but using on-demand on command line' '
+	(
+		cd work/gar/bage &&
+		>recurse-on-demand-on-command-line &&
+		git add recurse-on-demand-on-command-line &&
+		git commit -m "Recurse on-demand on command line junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Recurse on-demand on command line for gar/bage" &&
+		git push --recurse-submodules=on-demand ../pub.git master &&
+		# Check that the supermodule commit got there
+		git fetch ../pub.git &&
+		git diff --quiet FETCH_HEAD master
+		# Check that the submodule commit got there too
+		cd gar/bage &&
+		git diff --quiet origin/master master
+	)
+'
+
+test_expect_success 'push succeeds if submodule commit not on remote but using on-demand from config' '
+	(
+		cd work/gar/bage &&
+		>recurse-on-demand-from-config &&
+		git add recurse-on-demand-from-config &&
+		git commit -m "Recurse on-demand from config junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Recurse on-demand on command line for gar/bage" &&
+		git config push.recurseSubmodules on-demand &&
+		git push ../pub.git master &&
+		git config --unset push.recurseSubmodules &&
+		# Check that the supermodule commit got there
+		git fetch ../pub.git &&
+		git diff --quiet FETCH_HEAD master
+		# Check that the submodule commit got there too
+		cd gar/bage &&
+		git diff --quiet origin/master master
+	)
+'
+
+test_expect_success 'push fails if submodule commit not on remote using check from cmdline overriding config' '
+	(
+		cd work/gar/bage &&
+		>recurse-check-on-command-line-overriding-config &&
+		git add recurse-check-on-command-line-overriding-config &&
+		git commit -m "Recurse on command-line overridiing config junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Recurse on command-line overriding config for gar/bage" &&
+		git config push.recurseSubmodules on-demand &&
+		test_must_fail git push --recurse-submodules=check ../pub.git master &&
+		git config --unset push.recurseSubmodules &&
+		# Check that the supermodule commit did not get there
+		git fetch ../pub.git &&
+		git diff --quiet FETCH_HEAD master^
+		# Check that the submodule commit did not get there
+		cd gar/bage &&
+		git diff --quiet origin/master master^
+	)
+'
+
+test_expect_success 'push succeeds if submodule commit not on remote using on-demand from cmdline overriding config' '
+	(
+		cd work/gar/bage &&
+		>recurse-on-demand-on-command-line-overriding-config &&
+		git add recurse-on-demand-on-command-line-overriding-config &&
+		git commit -m "Recurse on-demand on command-line overriding config junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Recurse on-demand on command-line overriding config for gar/bage" &&
+		git config push.recurseSubmodules check &&
+		git push --recurse-submodules=on-demand ../pub.git master &&
+		git config --unset push.recurseSubmodules &&
+		# Check that the supermodule commit got there
+		git fetch ../pub.git &&
+		git diff --quiet FETCH_HEAD master
+		# Check that the submodule commit got there
+		cd gar/bage &&
+		git diff --quiet origin/master master
+	)
+'
+
+test_expect_success 'push succeeds if submodule commit disabling recursion from cmdline overriding config' '
+	(
+		cd work/gar/bage &&
+		>recurse-disable-on-command-line-overriding-config &&
+		git add recurse-disable-on-command-line-overriding-config &&
+		git commit -m "Recurse disable on command-line overriding config junk"
+	) &&
+	(
+		cd work &&
+		git add gar/bage &&
+		git commit -m "Recurse disable on command-line overriding config for gar/bage" &&
+		git config push.recurseSubmodules check &&
+		git push --recurse-submodules=no ../pub.git master &&
+		git config --unset push.recurseSubmodules &&
+		# Check that the supermodule commit got there
+		git fetch ../pub.git &&
+		git diff --quiet FETCH_HEAD master &&
+		# But that the submodule commit did not
+		cd gar/bage &&
+		git diff --quiet origin/master master^
+	)
+'
+
 test_expect_success 'push fails when commit on multiple branches if one branch has no remote' '
 	(
 		cd work/gar/bage &&
-- 
2.1.4

Re: [PATCH] push: add recurseSubmodules config option

[Stefan Beller]

On Mon, Nov 16, 2015 at 5:24 AM, Mike Crowe <mac@mcrowe.com> wrote:
> The --recurse-submodules command line parameter has existed for some
> time but it has no config file equivalent.
>
> Following the style of the corresponding parameter for git fetch, let's
> invent push.recurseSubmodules to provide a default for this
> parameter. This also requires the addition of --recurse-submodules=no to
> allow the configuration to be overridden on the command line when
> required.
>
> The most straightforward way to implement this appears to be to make
> push use code in submodule-config in a similar way to fetch.
>
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---

The code itself looks good to me, one nit in the tests though.

> @@ -79,6 +87,119 @@ test_expect_success 'push succeeds after commit was pushed to remote' '
>         )
>  '
>
> +test_expect_success 'push succeeds if submodule commit not on remote but using on-demand on command line' '
> +       (
> +               cd work/gar/bage &&
> +               >recurse-on-demand-on-command-line &&
> +               git add recurse-on-demand-on-command-line &&
> +               git commit -m "Recurse on-demand on command line junk"
> +       ) &&
> +       (
> +               cd work &&
> +               git add gar/bage &&
> +               git commit -m "Recurse on-demand on command line for gar/bage" &&
> +               git push --recurse-submodules=on-demand ../pub.git master &&
> +               # Check that the supermodule commit got there
> +               git fetch ../pub.git &&
> +               git diff --quiet FETCH_HEAD master

Missing && chain here.

> +               # Check that the submodule commit got there too
> +               cd gar/bage &&
> +               git diff --quiet origin/master master
> +       )
> +'
> +

Re: [PATCH] push: add recurseSubmodules config option

[Mike Crowe]

On Monday 16 November 2015 at 10:15:24 -0800, Stefan Beller wrote:
> The code itself looks good to me, one nit in the tests though.
> 
> > @@ -79,6 +87,119 @@ test_expect_success 'push succeeds after commit was pushed to remote' '
> >         )
> >  '
> >
> > +test_expect_success 'push succeeds if submodule commit not on remote but using on-demand on command line' '
> > +       (
> > +               cd work/gar/bage &&
> > +               >recurse-on-demand-on-command-line &&
> > +               git add recurse-on-demand-on-command-line &&
> > +               git commit -m "Recurse on-demand on command line junk"
> > +       ) &&
> > +       (
> > +               cd work &&
> > +               git add gar/bage &&
> > +               git commit -m "Recurse on-demand on command line for gar/bage" &&
> > +               git push --recurse-submodules=on-demand ../pub.git master &&
> > +               # Check that the supermodule commit got there
> > +               git fetch ../pub.git &&
> > +               git diff --quiet FETCH_HEAD master
> 
> Missing && chain here.

Oh, well spotted! I'll provide an updated version.

Thanks.

Mike.

Re: [PATCH] push: add recurseSubmodules config option

[Jens Lehmann]

Am 16.11.2015 um 19:31 schrieb Mike Crowe:
> On Monday 16 November 2015 at 10:15:24 -0800, Stefan Beller wrote:
>> The code itself looks good to me, one nit in the tests though.
>>
>>> @@ -79,6 +87,119 @@ test_expect_success 'push succeeds after commit was pushed to remote' '
>>>          )
>>>   '
>>>
>>> +test_expect_success 'push succeeds if submodule commit not on remote but using on-demand on command line' '
>>> +       (
>>> +               cd work/gar/bage &&
>>> +               >recurse-on-demand-on-command-line &&
>>> +               git add recurse-on-demand-on-command-line &&
>>> +               git commit -m "Recurse on-demand on command line junk"
>>> +       ) &&
>>> +       (
>>> +               cd work &&
>>> +               git add gar/bage &&
>>> +               git commit -m "Recurse on-demand on command line for gar/bage" &&
>>> +               git push --recurse-submodules=on-demand ../pub.git master &&
>>> +               # Check that the supermodule commit got there
>>> +               git fetch ../pub.git &&
>>> +               git diff --quiet FETCH_HEAD master
>>
>> Missing && chain here.
>
> Oh, well spotted! I'll provide an updated version.

Looking good for me too!

Cool, another issue from my Wiki that's being worked on!

Re: [PATCH] push: add recurseSubmodules config option

[Eric Sunshine]

On Mon, Nov 16, 2015 at 8:24 AM, Mike Crowe <mac@mcrowe.com> wrote:
> The --recurse-submodules command line parameter has existed for some
> time but it has no config file equivalent.
>
> Following the style of the corresponding parameter for git fetch, let's
> invent push.recurseSubmodules to provide a default for this
> parameter. This also requires the addition of --recurse-submodules=no to
> allow the configuration to be overridden on the command line when
> required.
>
> The most straightforward way to implement this appears to be to make
> push use code in submodule-config in a similar way to fetch.
>
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -2226,6 +2226,19 @@ push.gpgSign::
> +push.recurseSubmodules::
> +       Make sure all submodule commits used by the revisions to be pushed
> +       are available on a remote-tracking branch. If the value is 'check'
> +       then Git will verify that all submodule commits that changed in the
> +       revisions to be pushed are available on at least one remote of the
> +       submodule. If any commits are missing the push will be aborted and
> +       exit with non-zero status. If the value is 'on-demand' then all
> +       submodules that changed in the revisions to be pushed will be
> +       pushed. If on-demand was not able to push all necessary revisions
> +       it will also be aborted and exit with non-zero status. You may
> +       override this configuration at time of push by specifying
> +       '--recurse-submodules=check|on-demand|no'.

Does this configuration variable also support 'no' as a value? If so,
then it probably ought to be documented. If not, shouldn't it do so to
allow a configuration file to override a 'check' or 'on-demand' value
specified in a more global git configuration file?

>  rebase.stat::
>         Whether to show a diffstat of what changed upstream since the last
>         rebase. False by default.
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> @@ -257,7 +257,7 @@ origin +master` to force a push to the `master` branch). See the
> ---recurse-submodules=check|on-demand::
> +--recurse-submodules=check|on-demand|no::
>         Make sure all submodule commits used by the revisions to be
>         pushed are available on a remote-tracking branch. If 'check' is
>         used Git will verify that all submodule commits that changed in
> @@ -267,6 +267,8 @@ origin +master` to force a push to the `master` branch). See the
>         all submodules that changed in the revisions to be pushed will
>         be pushed. If on-demand was not able to push all necessary
>         revisions it will also be aborted and exit with non-zero status.
> +       A value of 'no' is used to override the push.recurseSubmodules
> +       variable when no submodule recursion is required.

Does this deserve a --no-recurse-submodules alias for consistency with
how other options are turned off?

>  --[no-]verify::
>         Toggle the pre-push hook (see linkgit:githooks[5]).  The
> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
> @@ -64,7 +64,15 @@ test_expect_success 'push fails if submodule commit not on remote' '
>                 cd work &&
>                 git add gar/bage &&
>                 git commit -m "Third commit for gar/bage" &&
> -               test_must_fail git push --recurse-submodules=check ../pub.git master
> +               # the push should fail with --recurse-submodules=check
> +               # on the command line...
> +               test_must_fail git push --recurse-submodules=check ../pub.git master &&
> +
> +               # ...or if specified in the configuration..
> +               git config push.recurseSubmodules check &&
> +               test_must_fail git push ../pub.git master &&
> +
> +               git config --unset push.recurseSubmodules

If something above this line fails, then 'git config --unset' will not
be invoked, so the expected cleanup won't happen. Typically, to ensure
cleanup, you'd use test_config(), however that function doesn't work
in subshells. Probably the easiest fix, in this case, is to set the
config variable as a one-shot and drop 'git config' and 'git config
--unset' altogether:

    test_must_fail git -c push.recurseSubmodules check \
        push ../pub.git master

>         )
>  '
>
> @@ -79,6 +87,119 @@ test_expect_success 'push succeeds after commit was pushed to remote' '
> +test_expect_success 'push succeeds if submodule commit not on remote but using on-demand from config' '
> +       (
> +               cd work/gar/bage &&
> +               >recurse-on-demand-from-config &&
> +               git add recurse-on-demand-from-config &&
> +               git commit -m "Recurse on-demand from config junk"
> +       ) &&
> +       (
> +               cd work &&
> +               git add gar/bage &&
> +               git commit -m "Recurse on-demand on command line for gar/bage" &&
> +               git config push.recurseSubmodules on-demand &&
> +               git push ../pub.git master &&
> +               git config --unset push.recurseSubmodules &&

Ditto regarding 'git config --unset' cleanup not being run there is a
failure above this. Same for following tests.

> +               # Check that the supermodule commit got there
> +               git fetch ../pub.git &&
> +               git diff --quiet FETCH_HEAD master
> +               # Check that the submodule commit got there too
> +               cd gar/bage &&
> +               git diff --quiet origin/master master
> +       )
> +'

Re: [PATCH] push: add recurseSubmodules config option

[Junio C Hamano]

Mike Crowe <mac@mcrowe.com> writes:

> diff --git a/builtin/push.c b/builtin/push.c
> index 3bda430..dfced74 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -9,6 +9,7 @@
>  #include "transport.h"
>  #include "parse-options.h"
>  #include "submodule.h"
> +#include "submodule-config.h"
>  #include "send-pack.h"
>  
>  static const char * const push_usage[] = {
> @@ -20,7 +21,7 @@ static int thin = 1;
>  static int deleterefs;
>  static const char *receivepack;
>  static int verbosity;
> -static int progress = -1;
> +static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;

One variable per line, please.  Especially when the two variables do
not have anything to do with each other, and do not have any logical
similarity between them.

> @@ -452,22 +453,15 @@ static int do_push(const char *repo, int flags)
>  static int option_parse_recurse_submodules(const struct option *opt,
>  				   const char *arg, int unset)
>  {
> -	int *flags = opt->value;
> +	int *recurse_submodules = opt->value;
>  
> -	if (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |
> -		      TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
> +	if (*recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
>  		die("%s can only be used once.", opt->long_name);

The usual convention thoughout Git user experience is "the last one
wins" (both in the configuration and in the command line options).
Is there a good reason to deviate from that here?

Re: [PATCH] push: add recurseSubmodules config option

[Mike Crowe]

On Monday 30 November 2015 at 10:31:26 -0800, Junio C Hamano wrote:
> Mike Crowe <mac@mcrowe.com> writes:
> 
> > diff --git a/builtin/push.c b/builtin/push.c
> > index 3bda430..dfced74 100644
> > --- a/builtin/push.c
> > +++ b/builtin/push.c
> > @@ -9,6 +9,7 @@
> >  #include "transport.h"
> >  #include "parse-options.h"
> >  #include "submodule.h"
> > +#include "submodule-config.h"
> >  #include "send-pack.h"
> >  
> >  static const char * const push_usage[] = {
> > @@ -20,7 +21,7 @@ static int thin = 1;
> >  static int deleterefs;
> >  static const char *receivepack;
> >  static int verbosity;
> > -static int progress = -1;
> > +static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> 
> One variable per line, please.  Especially when the two variables do
> not have anything to do with each other, and do not have any logical
> similarity between them.

I wouldn't normally have done that either, but I was mirroring the
equivalent code in fetch.c. I will change it.

> > @@ -452,22 +453,15 @@ static int do_push(const char *repo, int flags)
> >  static int option_parse_recurse_submodules(const struct option *opt,
> >  				   const char *arg, int unset)
> >  {
> > -	int *flags = opt->value;
> > +	int *recurse_submodules = opt->value;
> >  
> > -	if (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |
> > -		      TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
> > +	if (*recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
> >  		die("%s can only be used once.", opt->long_name);
> 
> The usual convention thoughout Git user experience is "the last one
> wins" (both in the configuration and in the command line options).
> Is there a good reason to deviate from that here?

I was aiming to retain the existing behaviour, which was to complain if
conflicting options were supplied on the command line. Making the last one
win would have been rather simpler. I can change this too, unless someone
knows why complaining about conflicting options would be useful.

Note that I previously sent an updated patch as
<1447758356-7727-1-git-send-email-mac@mcrowe.com> but I believe that your
criticisms still apply.

Thanks for the review.

Mike.