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

From: Junio C Hamano <[email-masked]>
Date: Mon, 03 Feb 2014 14:23:15 -0800

Jens Lehmann <[email-masked]> writes:

> This commit adds the functions and files needed for configuration,

Please just say "Add the functions and files needed for ...".

> +++ 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

That single quote does not seem to be closing any matching quote.

The phrase "according to" feels a bit too fuzzy.  Merging the commit
to what is checked out is one possible implementation of "according to".
Applying the diff between the commit and what is checked out to work
tree is another.  Resetting the work tree files to exactly match the
commit would be yet another.

I think "update the work trees to the commit" (i.e. lose the
"according") would be the closest to what you are trying to say
here.

> +	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.

It is unclear what happens if their update configuration is set to
something other than 'checkout'.

> diff --git a/submodule.c b/submodule.c
> index 613857e..b3eb28d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -382,6 +384,48 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
> ...
> +int option_parse_update_submodules(const struct option *opt,
> +				   const char *arg, int unset)
> +{
> +	if (unset) {
> +		*(int *)opt->value = RECURSE_SUBMODULES_OFF;
> +	} else {
> +		if (arg)
> +			*(int *)opt->value = parse_update_recurse_submodules_arg(opt->long_name, arg);
> +		else
> +			*(int *)opt->value = RECURSE_SUBMODULES_ON;
> +	}

You can easily unnest to lose {}

    if (unset)
            value = off;
    else if (arg)
            value = parse...;
    else
            value = on;

Also I suspect that git_config_maybe_bool() natively knows how to
handle arg==NULL, so

    if (unset)
	value = off;
    else
	value = parse...;

is sufficient?