Re: [WIP/PATCH 1/9] submodule: prepare for recursive checkout of submodules

[Jonathan Nieder <jrnieder@gmail.com>]

Jens Lehmann wrote:

> This commit adds the functions and files needed for configuration,
> documentation, setting the default behavior and determining if a
> submodule path should be updated automatically.

Yay!

[...]
>  Documentation/recurse-submodules-update.txt |  8 +++++
>  submodule.c                                 | 50 +++++++++++++++++++++++++++++
>  submodule.h                                 |  6 ++++
>  3 files changed, 64 insertions(+)
>  create mode 100644 Documentation/recurse-submodules-update.txt

I like the shared documentation snippet.

Ok, naive questions and overly pedantic nitpicking follow.  Patch with
a couple of suggested changes at the end.

[...]
> --- /dev/null
> +++ b/Documentation/recurse-submodules-update.txt
> @@ -0,0 +1,8 @@
> +--[no-]recurse-submodules::
> +	Using --recurse-submodules will update the work tree of all
> +	initialized submodules according to the commit recorded in the
> +	superproject if their update configuration is set to checkout'. If
> +	local modifications in a submodule would be overwritten the checkout
> +	will fail unless forced. Without this option or with
> +	--no-recurse-submodules is, the work trees of submodules will not be
> +	updated, only the hash recorded in the superproject will be updated.

Tweaks:

 * Spelling out "--no-recurse-submodules, --recurse-submodules" (imitating
   e.g. --decorate in git-log(1))

 * Shortening, using imperative mood
 
 * Skipping description of safety check, since it matches how checkout
   works in general

That would make

	--no-recurse-submodules::
	--recurse-submodules::
		Perform the checkout in submodules, too.  This only affects
		submodules with update strategy `checkout` (which is the
		default update strategy; see `submodule.<name>.update` in
		link:gitmodules[5]).
	+
	The default behavior is to update submodule entries in the superproject
	index and to leave the inside of submodules alone.  That behavior can also
	be requested explicitly with --no-recurse-submodules.

Ideas for further work:

 * The safety check probably deserves a new section where it could be
   described in detail alongside a description of the corresponding check
   for plain checkout.  Then the description of the -f option could
   point to that section.

 * What happens when update = merge, rebase, or !command?  I think
   skipping them for now like suggested above is fine, but:

   - It would be even better to error out when there are changes to carry
     over with update = merge or rebase

   - Better still to perform the rebase when update = rebase

   - I have no idea what update = merge should do for non-fast-forward
     moves

> --- a/submodule.c
> +++ b/submodule.c
> @@ -16,6 +16,8 @@ static struct string_list config_name_for_path;
>  static struct string_list config_fetch_recurse_submodules_for_name;
>  static struct string_list config_ignore_for_name;
>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
> +static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
> +static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;

Confusingly, config_update_recurse_submodules is set using the
--recurse-submodules-default option, not configuration.  There's
precedent for that in fetch.recurseSubmodules handling, but perhaps
a comment would help --- something like

	/*
	 * When no --recurse-submodules option was passed, should git fetch
	 * from submodules where submodule.<name>.fetchRecurseSubmodules
	 * doesn't indicate what to do?
	 *
	 * Controlled by fetch.recurseSubmodules.  The default is determined by
	 * the --recurse-submodules-default option, which propagates
	 * --recurse-submodules from the parent git process when recursing.
	 */
	static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;

	/*
	 * When no --recurse-submodules option was passed, should git update
	 * the index and worktree within submodules (and in turn their
	 * submodules, etc)?
	 *
	 * Controlled by the --recurse-submodules-default option, which
	 * propagates --recurse-submodules from the parent git process
	 * when recursing.
	 */
	static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;

[...]
> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
>  	}
>  }
> 
> +int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
> +{
> +	switch (git_config_maybe_bool(opt, arg)) {
> +	case 1:
> +		return RECURSE_SUBMODULES_ON;
> +	case 0:
> +		return RECURSE_SUBMODULES_OFF;
> +	default:
> +		if (!strcmp(arg, "checkout"))
> +			return RECURSE_SUBMODULES_ON;

Hm, is this arg == checkout case futureproofing for when
--recurse-submodules learns to handle submodules without
'update = checkout', too?

Is it safe to leave it out for now?

[...]
> +int submodule_needs_update(const char *path)

Return value convention: 1 means "do update"; 0 means "don't update".

Some day later I suppose 2 or -1 could mean "error out".  Ok.

Naming nit: needs_update sounds like it's checking if there was a
change at that path.  How about something like submodule_should_update(),
!submodule_ignore_for_update(), or update_should_recurse_into_submodule()?

[...]
> @@ -589,6 +633,12 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>  	return ret;
>  }
> 
> +void set_config_update_recurse_submodules(int default_value, int option_value)
> +{
> +	config_update_recurse_submodules = default_value;
> +	option_update_recurse_submodules = option_value;
> +}

Could option_parse_update_submodules set
option_update_recurse_submodules directly?  Alternatively, could this
function examine option_value so that submodule.c would only need one
variable?

	if (option_value == RECURSE_SUBMODULES_DEFAULT)
		update_recurse_submodules = default_value;
	else
		update_recurse_submodules = option_value;

If .gitmodules some day grows a submodule.<name>.checkoutRecurseSubmodules
option then it would be convenient to have the option that overrides and
the default tracked separately.  Is that the idea here?

I might try writing a dummy command to test this basic --recurse-submodules
option handling as a separate patch.

Thanks,
Jonathan

diff --git i/Documentation/recurse-submodules-update.txt w/Documentation/recurse-submodules-update.txt
index e57d452..eae376d 100644
--- i/Documentation/recurse-submodules-update.txt
+++ w/Documentation/recurse-submodules-update.txt
@@ -1,8 +1,10 @@
---[no-]recurse-submodules::
-	Using --recurse-submodules will update the work tree of all
-	initialized submodules according to the commit recorded in the
-	superproject if their update configuration is set to checkout'. If
-	local modifications in a submodule would be overwritten the checkout
-	will fail unless forced. Without this option or with
-	--no-recurse-submodules is, the work trees of submodules will not be
-	updated, only the hash recorded in the superproject will be updated.
+--no-recurse-submodules::
+--recurse-submodules::
+	Perform the checkout in submodules, too.  This only affects
+	submodules with update strategy `checkout` (which is the
+	default update strategy; see `submodule.<name>.update` in
+	linkgit:gitmodules[5]).
++
+The default behavior is to update submodule entries in the superproject
+index and to leave the inside of submodules alone.  That behavior can
+also be requested explicitly with `--no-recurse-submodules`.
diff --git i/submodule.c w/submodule.c
index b3eb28d..f88bf70 100644
--- i/submodule.c
+++ w/submodule.c
@@ -12,11 +12,30 @@
 #include "argv-array.h"
 #include "blob.h"
 
+/*
+ * When no --recurse-submodules option was passed, should git fetch
+ * from submodules where submodule.<name>.fetchRecurseSubmodules doesn't
+ * indicate what to do?
+ *
+ * Controlled by fetch.recurseSubmodules.  The default is determined by
+ * the --recurse-submodules-default option, which propagates
+ * --recurse-submodules from the parent git process when recursing.
+ */
+static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+
+/*
+ * When no --recurse-submodules option was passed, should git update the
+ * index and worktree within submodules (and in turn their submodules,
+ * etc)?
+ *
+ * Controlled by the --recurse-submodules-default option, which propagates
+ * --recurse-submodules from the parent git process when recursing.
+ */
+static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
+
 static struct string_list config_name_for_path;
 static struct string_list config_fetch_recurse_submodules_for_name;
 static struct string_list config_ignore_for_name;
-static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
-static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 static int option_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static struct string_list changed_submodule_paths;
 static int initialized_fetch_ref_tips;
@@ -392,8 +411,6 @@ int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
 	case 0:
 		return RECURSE_SUBMODULES_OFF;
 	default:
-		if (!strcmp(arg, "checkout"))
-			return RECURSE_SUBMODULES_ON;
 		die("bad %s argument: %s", opt, arg);
 	}
 }