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

From: Jeff King <peff@peff.net>
Date: Oct 4, 2016, 17:34 UTC
Message-ID: <20161004173430.eax4ptohyonc5bw2@sigill.intra.peff.net>
In-Reply-To: <20161004164036.6584-1-sbeller@google.com>

On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:

Show 11 quoted lines

> >> Why should we even have a default different from today's?  If most
> >> repositories don't have submodules enabled at all, we can just let
> >> those working with submodules enabled to toggle their configuration
> >> and that is an very easy to understand solution, no?
> >
> > You will not see any complaint from me on that. I was taking for granted
> > that the current default is inconvenient to submodule users, but I don't
> > have any experience myself.
> >
> 
> And there I was trying to help submodule users not shoot in their foot.

Sorry if my reply came off as snarky. I really did mean it literally. I do not know if the end goal is good or not, so all of my discussion was just assuming it was.

So in that vein...

Show 24 quoted lines

> diff --git a/builtin/push.c b/builtin/push.c
> index 3bb9d6b..d7d664a 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;
>  static enum transport_family family;
>  
>  static struct push_cas_option cas;
> @@ -31,6 +31,14 @@ static const char **refspec;
>  static int refspec_nr;
>  static int refspec_alloc;
>  
> +static void preset_submodule_default(void)
> +{
> +	if (file_exists(".gitmodules"))
> +		recurse_submodules = RECURSE_SUBMODULES_CHECK;
> +	else
> +		recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
> +}

This does seem like a reasonable heuristic. I wonder if you want to confirm that we actually have a worktree (and are in it) before looking at file_exists(). It's unlikely that looking at ".gitmodules" in a bare repo would trigger in practice, but it does not hurt to be careful.

-Peff