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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2014, 22:23 UTC
Message-ID: <xmqqha8fdeek.fsf@gitster.dls.corp.google.com>
In-Reply-To: <52EFF290.5090501@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

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

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

Show 6 quoted lines

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

Show 17 quoted lines

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