Re: [PATCH v3] submodule: Improve documentation of update subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2015, 23:31 UTC
Message-ID: <xmqqlhjsxira.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1424371972-13393-1-git-send-email-sojkam1@fel.cvut.cz>

Michal Sojka <sojkam1@fel.cvut.cz> writes:

> This patch fixes all these problems. Now, submodule.$name.update is
> fully documented in git-submodule.txt and the other files just refer to

"Fix all these problems by documenting submodule.*.update in git-submodule.txt and make everybody else refer to it" in imperative mood, as if you are giving an order to the source to "be this way". It would be sweeter and shorter that way.

> it. This is based on discussion between Junio C Hamano, Jens Lehmann and
> myself.

It's customary to just mention them on Helped-by: around here, I think.

Show 13 quoted lines

> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 8e6af65..72c6fb2 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -154,14 +154,36 @@ If `--force` is specified, the submodule's work tree will be removed even if
>  it contains local modifications.
>  
>  update::
> +	Update the registered submodules to match what the superproject
> +	expects by cloning missing submodules and updating the working
> +	tree of the submodules. The "updating" can be done in several
> +	ways depending on command line options and the value of
> +	`submodule.<name>.update` in .git/config:

No quoting around .git/config? Actually, it is probably better not to spell out that path. "... and the value of the `...` configuration variable" would be better.

> +	checkout;; the new commit recorded in the superproject will be
> +	    checked out in the submodule on a detached HEAD. This is

Drop "new". It does not add anything to the description, and you may even be checking out an old commit in the superproject.

> @@ -238,10 +261,11 @@ OPTIONS

Totally offtopic, but we may want a custom xfuncname for our AsciiDoc documentation; we would want to see "--force::" not "OPTIONS" on the above line, I would think.

Show 12 quoted lines

>  	When running add, allow adding an otherwise ignored submodule path.
>  	When running deinit the submodule work trees will be removed even if
>  	they contain local changes.
> -	When running update, throw away local changes in submodules when
> -	switching to a different commit; and always run a checkout operation
> -	in the submodule, even if the commit listed in the index of the
> -	containing repository matches the commit checked out in the submodule.
> +	When running update and the checkout method is used, throw away
> +	local changes in submodules when switching to a different
> +	commit; and always run a checkout operation in the submodule,
> +	even if the commit listed in the index of the containing
> +	repository matches the commit checked out in the submodule.

This makes a reader wonder what --force would do when --merge or --rebase is given from the command line (or specifiedy in the configuration). The original (unfortunately) did not have that problem because it did not single out the --checkout mode.

The use of the phrase "the checkout method" is iffy, as nobody defines what it is (I just said "--checkout mode" to mean the same thing, but I do not think anybody defines it). See below.

Show 6 quoted lines

> @@ -302,7 +326,7 @@ the submodule itself.
>  	Checkout the commit recorded in the superproject on a detached HEAD
>  	in the submodule. This is the default behavior, the main use of
>  	this option is to override `submodule.$name.update` when set to
> -	`merge`, `rebase` or `none`.
> +	other value than `checkout`.

"... when set to a value other than `checkout`", would read better, I would think.

Show 14 quoted lines

> diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
> index f6c0dfd..a51183c 100644
> --- a/Documentation/gitmodules.txt
> +++ b/Documentation/gitmodules.txt
> @@ -38,18 +38,12 @@ submodule.<name>.url::
>  In addition, there are a number of optional keys:
>  
>  submodule.<name>.update::
> +	Defines what to do when the submodule is updated by the
> +	superproject. This is only used by `git submodule init` to
> +	initialize the variable of the same name in .git/config.
> +	Allowed values here are 'checkout', 'rebase', 'merge' or
> +	'none'. See description of 'update' command in
> +	linkgit:git-submodule[1] for their meaning.

Whatever word we decide to use, this may be a good place to introduce it, perhaps like this (if we were to go with 'update method'):

    submodule.<name>.update::

	Define the default update method for the named submodule,
	how the submodule is updated by "git submodule update"
	command in the superproject.

The enumeration of the allowed values is correct, I think, but we might want to be very clear that we do not copy the !command form and that is on purpose.