git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Nanako Shiraishi <nanako3@lavabit.com>
Cc: Pierre Habouzit <madcoder@debian.org>,
	Thomas Rast <trast@student.ethz.ch>,
	git@vger.kernel.org
Subject: Re: [PATCH] git-add -p: be able to undo a given hunk
Date: Fri, 24 Jul 2009 09:06:16 -0700	[thread overview]
Message-ID: <7v8wienk07.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: 20090724193207.6117@nanako3.lavabit.com

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> I fear tempting a new user who sees "undo" to say "yeah, I added the
>> change in this hunk to the index by mistake, please undo", which would
>> lose the work.  The confusion is easier to avoid if "add" only manipulates
>> the index without harming the work tree, and the user used a different
>> command, namely "checkout from the index", to get rid of the remaining
>> debug cruft, once s/he added all the necessary bits to the index perhaps
>> after a multi-stage commit session.
>
> I can see your argument that this might introduce more danger for
> newbies. As you said yourself number of times, nobody will stay being a
> newbie forever, and I don't think it is wise to reject a feature that is
> very handy for experts based solely on such a fear.

It is true that as new people learn they gain proficiency, and you are
also right to point out that I'd usually choose to optimize the interface
for making the life of experts easier rather than welding training wheels
to the system.

BUT

As new people gain proficiency in git, three things happen.

 (1) It becomes a lot less likely for them to make mistakes in choosing
     commands.  I mentioned that they may misunderstand what Pierre's
     "undo" would do, and that fear may be alleviated because of this.

 (2) It does _not_ become less likely for them to make typos when giving
     the command they chose.  The chance of saying 'u' when you mean 'y'
     does not decrease that much as you become more used to using git.
     Especially with interactive.singlekey, the consequence of such a typo
     is devastating.

 (3) They form a better mental model of how the world works.

     The high level view of the git workflow is for you to:

     (a) prepare good changes, together with some changes that are not
         quite ready, in your work tree; and

     (b) use "git add" to add only good changes suitable for the next
         commit to the index; and finally

     (c) make the next commit out of the index.  Repeat (b) and (c) as
         necessary to create multiple commits.

     If you botch the "git add" step during this process, because "git
     add" promises not to touch the work tree, you can safely reset the
     index entry to its previous state and redo the "git add" step,
     without having to fear that you may lose your work.

In your arsenal, you have "git add -p" to help you sift good pieces from
other parts in finer grained manner, instead of having to make an all or
nothing decision per file basis (i.e. "git add file").  But "git add -p"
(and "git add -i") is still about the "git add" step in the above high
level view.  You have a mixture of good and not so good changes in your
work tree, and you pick only good pieces to add to the index, _knowing_
that you can go back and redo this step safely exactly because your work
tree will stay the same even if you did make mistakes.

The proposed change breaks this expectation you would have naturally
gained during the course of becoming more and more proficient in using
git.

In other words, I do not think you can say that the change will not harm
the experts due to both the points 2 (experts can easily make typo) and 3
above (the change breaks the mental model of the world experts would have
formed).

Having said all that, it indeed would be useful to selectively revert
changes from the work tree files.

Even though you could add good bits interactively, making multiple
commits, and remove the remaining debugging cruft at the very end with
"git checkout $files" or "git reset --hard", if there are debugging crufts
for two or more phases of development, this alternative procedure would
not work well, compared to the workflow using Pierre's 'u', which allows
you to add necessary bits to commit one phase, remove the debugging bits
for that phase (but keeping other debugging bits for the remaining good
parts), then continue working and repeat committing and cleaning second
and subsequent phases.

It might be enough to change the command key to uppercase "U" to avoid
unintended mistakes, and document the fact prominently that this action is
an oddball exception to the principle of "git add", while describing why
it is an oddball, along the lines of the above discussion, if necessary.

  reply	other threads:[~2009-07-24 16:06 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-23  7:41 [PATCH] git-add -p: be able to undo a given hunk Pierre Habouzit
2009-07-23  8:41 ` Thomas Rast
2009-07-23  8:50   ` Pierre Habouzit
2009-07-24  9:15   ` [RFC PATCH] Implement unstage and reset modes for git-add--interactive Thomas Rast
2009-07-24 16:24     ` [RFC PATCH v2 1/3] Introduce git-unstage Thomas Rast
2009-07-24 17:59       ` Bert Wesarg
2009-07-24 18:02         ` Bert Wesarg
2009-07-24 18:23       ` Elijah Newren
2009-07-24 16:24     ` [RFC PATCH v2 2/3] Introduce git-discard Thomas Rast
2009-07-24 18:02       ` Elijah Newren
2009-07-24 18:12         ` Bert Wesarg
2009-07-24 18:24           ` Elijah Newren
2009-07-25 14:58       ` Pierre Habouzit
2009-07-24 16:24     ` [RFC PATCH v2 3/3] Implement unstage --patch and discard --patch Thomas Rast
2009-07-24 16:40       ` Matthias Kestenholz
2009-07-24 18:08       ` Bert Wesarg
2009-07-24 16:39     ` [RFC PATCH] Implement unstage and reset modes for git-add--interactive Junio C Hamano
2009-07-24 21:58       ` Nanako Shiraishi
2009-07-24 23:17         ` Thomas Rast
2009-07-24 23:25         ` Junio C Hamano
2009-07-25 21:29           ` [RFC PATCH v3 0/5] {checkout,reset,stash} --patch Thomas Rast
2009-07-25 21:29             ` [RFC PATCH v3 1/5] git-apply--interactive: Refactor patch mode code Thomas Rast
2009-07-25 21:29             ` [RFC PATCH v3 2/5] builtin-add: refactor the meat of interactive_add() Thomas Rast
2009-07-25 21:29             ` [RFC PATCH v3 3/5] Implement 'git reset --patch' Thomas Rast
2009-07-25 21:29             ` [RFC PATCH v3 4/5] Implement 'git checkout --patch' Thomas Rast
2009-07-25 21:29             ` [RFC PATCH v3 5/5] Implement 'git stash save --patch' Thomas Rast
2009-07-26  6:03               ` Sverre Rabbelier
2009-07-26  8:45                 ` Thomas Rast
2009-07-27 10:10             ` [RFC PATCH v3 0/5] {checkout,reset,stash} --patch Thomas Rast
2009-07-28 21:20               ` [PATCH v4 " Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 1/5] git-apply--interactive: Refactor patch mode code Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 2/5] builtin-add: refactor the meat of interactive_add() Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 3/5] Implement 'git reset --patch' Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 4/5] Implement 'git checkout --patch' Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 5/5] Implement 'git stash save --patch' Thomas Rast
2009-07-28 21:20                 ` [PATCH v4 6/5] DWIM 'git stash save -p' for 'git stash -p' Thomas Rast
2009-08-09  6:52                 ` [PATCH v4 0/5] {checkout,reset,stash} --patch Jeff King
2009-08-09  9:17                   ` Thomas Rast
2009-08-09 16:32                     ` [PATCH v4 0/5] " Nicolas Sebrecht
2009-08-09 16:44                       ` Thomas Rast
2009-08-09 21:28                         ` Nicolas Sebrecht
2009-08-09 21:42                           ` Thomas Rast
2009-08-09 22:26                             ` Nicolas Sebrecht
2009-08-10  9:36                               ` Thomas Rast
2009-08-13 12:29                                 ` [PATCH v5 0/6] " Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 1/6] git-apply--interactive: Refactor patch mode code Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 2/6] Add a small patch-mode testing library Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 3/6] builtin-add: refactor the meat of interactive_add() Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 4/6] Implement 'git reset --patch' Thomas Rast
2009-08-15 11:48                                     ` [PATCH v5.1 " Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 5/6] Implement 'git checkout --patch' Thomas Rast
2009-08-15 11:48                                     ` [PATCH v5.1 " Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 6/6] Implement 'git stash save --patch' Thomas Rast
2009-08-13 12:29                                   ` [PATCH v5 7/6] DWIM 'git stash save -p' for 'git stash -p' Thomas Rast
2009-08-14 20:57                                   ` [PATCH v5 0/6] Re: {checkout,reset,stash} --patch Nicolas Sebrecht
2009-08-15  6:51                                   ` [PATCH v5 0/6] " Jeff King
2009-08-15  7:57                                     ` Junio C Hamano
2009-08-15 10:14                                       ` Thomas Rast
2009-08-15 10:04                                     ` Thomas Rast
2009-08-18 16:48                                       ` Jeff King
2009-08-19  9:40                                         ` Thomas Rast
2009-08-19 10:11                                           ` Jeff King
2009-07-23 19:58 ` [PATCH] git-add -p: be able to undo a given hunk Junio C Hamano
2009-07-24 10:32   ` Nanako Shiraishi
2009-07-24 16:06     ` Junio C Hamano [this message]
2009-07-24 17:06       ` Jeff King
2009-07-25  0:54         ` Junio C Hamano
2009-07-25  9:35           ` Thomas Rast
2009-07-25 14:48         ` Pierre Habouzit
2009-07-25 14:52       ` Pierre Habouzit
2009-07-26 15:39         ` Jeff King
2009-07-27  8:26           ` Pierre Habouzit
2009-07-27 10:30             ` Jeff King
2009-07-27 10:06           ` Thomas Rast
2009-07-27 10:36             ` Jeff King
2009-07-24 14:58   ` Pierre Habouzit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7v8wienk07.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=madcoder@debian.org \
    --cc=nanako3@lavabit.com \
    --cc=trast@student.ethz.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).