Re: [PATCH] push: add recurseSubmodules config option
- From
Mike Crowe <mac@mcrowe.com>
- Date
- Nov 30, 2015, 19:00 UTC
- Message-ID
- <20151130190021.GA29232@mcrowe.com>
- In-Reply-To
- <xmqqa8pv8hkx.fsf@gitster.mtv.corp.google.com>
On Monday 30 November 2015 at 10:31:26 -0800, Junio C Hamano wrote:
Show 24 quoted lines
> 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.
Show 15 quoted lines
> > @@ -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.