Re: [PATCH] push: add recurseSubmodules config option

[Mike Crowe <mac@mcrowe.com>]

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.