Re: [PATCH] add: Clarify documentation of -A and -u

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2013, 17:10 UTC
Message-ID: <7vhakoqwri.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130306072610.GB15534@biohazard-cafe.mit.edu>

Greg Price <price@MIT.EDU> writes:

Show 15 quoted lines

> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 388a225..f89d920 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -105,7 +105,9 @@ apply to the index. See EDITING PATCHES below.
>  	will never stage new files, but that it will stage modified
>  	new contents of tracked files and that it will remove files
>  	from the index if the corresponding files in the working tree
> -	have been removed.
> +	have been removed.  By contrast `-A` will add new files as
> +	well as updating and removing existing ones, and the default
> +	behavior will add new files and will not remove existing ones.
>  +
>  If no <pathspec> is given, the current version of Git defaults to
>  "."; in other words, update all tracked files in the current directory

I do not know if mentioning what -A does in the description for -u (and vice versa) makes it easier to understand or more confusing (not rhetorical: I suspect some may find it easier and others not).

But "and the default behaviour will..." here _is_ confusing. After reading this patch three times, I still cannot tell what "default" you are trying to describe. Is it "-u" without arguments? Is it "add" without "-u" nor "-A"? Is it something else???

Exactly the same comment applies to the other hunk.

Having said that, I agree that the current description for "-u" is way suboptimal. It begins like this:

    -u::
    --update::
            Only match <pathspec> against already tracked files in
            the index rather than the working tree. That means that it
            will ...

Whenever you see an incomprehensible technobabble followed by "That means", "In other words", etc., you (not limited to Greg-you but figuratively "everybody") should see if it makes it easier to read to remove everything up to that "That means" and adding a bit more to the remainder. In this particular case, the technobabble is not even correct---"match"ing is only a small part of what "add -u" does to find what to "add".

I would suggest rewriting the first part of "-u" perhaps like this:

	-u::
        --update::
		Update modified and/or removed paths in the index
		that match <pathspec> with the current state of the
		working tree files.  No new path is added because
		this considers only the paths that are already in
		the index.

The text for "-A" may look like this:

	-A::
        --all::
		Update the index to record the current state of the
		working tree files that match <pathspec>.  Note that
		new paths will be added to the index, in addition to
		modified and/or removed paths.

I agree with your patch that "If no <pathspec> is given" should be repeated for both "-u" and "-A".

Show 22 quoted lines

> @@ -114,10 +116,17 @@ of Git, hence the form without <pathspec> should not be used.
>  
>  -A::
>  --all::
> -	Like `-u`, but match <pathspec> against files in the
> -	working tree in addition to the index. That means that it
> -	will find new files as well as staging modified content and
> -	removing files that are no longer in the working tree.
> +	Update the index regarding all files that match <pathspec>,
> +	either in the index or the working tree.  That is, remove
> +	files that are only in the index, add files that are only in
> +	the working tree, and update files that differ between the
> +	two.  By contrast `-u` only removes and updates, and the
> +	default behavior only adds and updates.
> ++
> +If no <pathspec> is given, the current version of Git defaults to
> +"."; in other words, update all tracked files in the current directory
> +and its subdirectories. This default will change in a future version
> +of Git, hence the form without <pathspec> should not be used.
>  
>  -N::
>  --intent-to-add::