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

2013-03-07 Thread Greg Price
On Wed, Mar 06, 2013 at 09:10:57AM -0800, Junio C Hamano wrote:
 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???

I meant the behavior without -A or -u.  This could be fixed directly
by s/the default behavior will/with neither -A nor -u we/.  But we can
also keep revising -- there's definitely more room to make this
clearer!  I suggest new versions at the bottom of this message.

I do think that it's important that the reader be able to see clearly
what the option does as a contrast with what the command does without
the option.  Normally in a man page this is how the option is
described in the first place -- for example, the next entry in this
very man page says Record *only* the fact that the path will be added
later, rather than Record the fact that ...  These two descriptions
suffer from not doing that, so that it's not clear which parts of the
behavior they describe are just what 'add' does with no options and
which parts are actually due to the option.


 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 [...]

Seems like a reasonable heuristic.


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

These are good -- I especially like that they're shorter.  I do think
they're still likely to be confusing.  The lead sentences are hard to
tell apart from each other or one's mental model of what 'add' alone
does, though the contrasts that follow them help.  I also think the
lead sentence for '--all' isn't really correct -- we update the index
not only for the working tree files that match pathspec, but also
where there is no working tree file, only an index entry.  (So the
sentence actually describes what 'add' with neither option does.)

Maybe it's worth taking a step back.  The overall taxonomy is
 * 'add' alone considers matching filenames in the working tree
 * 'add -u' considers matching filenames in the index
 * 'add -A' considers matching filenames in both the index and the
   working tree
and in each case we make the index match the working tree on those
files.  Or, put another way,
 * 'add' alone modifies and adds files
 * 'add -u' modifies and removes files
 * 'add -A' modifies, adds, and removes files

Here's a crack at making those distinctions clear.  I've also tried to
make the descriptions as parallel as possible, as what they're saying
is very similar.

-u::
--update::
Update the index just where it already has an entry matching
pathspec.  This removes as well as modifies index entries to
match the working tree, but adds no new files.

-A::
--all::
Update the index not only where the working tree has a file
matching pathspec but also where the index already has an
entry.  This adds, modifies, and removes index entries to
match the working tree.

These are the shortest in the discussion so far, and I think they're
also the clearest.

Then follow both with the If no pathspec paragraph.  I just
noticed that the paragraph actually needs a small modification to fit
'-A', too.  New patch below.

Greg


From: Greg Price pr...@mit.edu
Date: Thu, 7 Mar 2013 02:08:21 -0800
Subject: [PATCH] add: Clarify documentation of -A and -u

The documentation of '-A' and '-u' is very confusing for someone who
doesn't already know what they do.  Describe them with fewer words and
clearer parallelism to each other and to the behavior of plain 'add'.

Also mention the default pathspec for '-A' as well as '-u', because
it applies to both.

Signed-off-by: Greg Price pr...@mit.edu
---
 Documentation/git-add.txt | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 388a225..b0944e5 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -100,12 +100,9 @@ apply to the index. See EDITING PATCHES below.
 
 -u::
 --update::
-   

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

2013-03-06 Thread Junio C Hamano
Greg Price pr...@mit.edu writes:

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

 @@ -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::
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html