git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Horst H. von Brand]

What I did:

- New file images/coins.asy ~~-> 'git add images/coins.asy'
- Started adding new stuff to fg.tex
- Noticed a old bug in fg.tex, fixed that one
- Did 'git -pm "Some message"' and selected just the bugfix

But git created a commit _including_ the new file. Tried to go back:

- 'git reset HEAD^'

Now the new file isn't staged anymore


What I expected to happen:

- Only the explicitly selected chunks commited
- No "losing staged changes"
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239
Casilla 110-V, Valparaiso, Chile 2340000       Fax:  +56 32 2797513

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Frans Klaver]

On Fri, 05 Oct 2012 16:20:45 +0200, Horst H. von Brand  
<vonbrand@inf.utfsm.cl> wrote:

> What I did:
>
> - New file images/coins.asy ~~-> 'git add images/coins.asy'
> - Started adding new stuff to fg.tex
> - Noticed a old bug in fg.tex, fixed that one
> - Did 'git -pm "Some message"' and selected just the bugfix
>
> But git created a commit _including_ the new file. Tried to go back:

Exactly what's supposed to happen. "git add" tells git you want to add the  
file to the index. The index is what you're going to commit later on. So  
what you did there was

- Tell git to add images/coins.asy to the next commit
- hack hack hack
- fix old_bug
- Add old_bug chunks of code to next commit && create commit

>
> - 'git reset HEAD^'
>
> Now the new file isn't staged anymore
>
>
> What I expected to happen:
>
> - Only the explicitly selected chunks commited
> - No "losing staged changes"

As explained above, you didn't lose staged changes, you staged more  
changes and committed. Then you use git reset to go back to the state of  
HEAD^, where the file wasn't tracked and therefore not staged either.

So you're back at square one[1], commit the bug fix, then add the bugfixes  
in a commit and stage the new file for inclusion in your next commit.

Hope this helps,
Frans

[1] Arguably two, since you still have changes lying around.

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

"Frans Klaver" <fransklaver@gmail.com> writes:

> On Fri, 05 Oct 2012 16:20:45 +0200, Horst H. von Brand
> <vonbrand@inf.utfsm.cl> wrote:
>
>> What I did:
>>
>> - New file images/coins.asy ~~-> 'git add images/coins.asy'
>> - Started adding new stuff to fg.tex
>> - Noticed a old bug in fg.tex, fixed that one
>> - Did 'git -pm "Some message"' and selected just the bugfix
>>
>> But git created a commit _including_ the new file. Tried to go back:
>
> Exactly what's supposed to happen. "git add" tells git you want to add
> the file to the index. The index is what you're going to commit later
> on.

Assuming that the last step of what Horst did was "git commit -pm",
I think Git is wrong in this case.  When you tell "git commit" what
to commit, unless you give "-i" (aka "also") option, the command
makes a commit to record changes only from what you tell "git
commit" to commit, regardless of what you earlier did to the index.

And choosing what to add via the interactive interface is in the
same spirit as telling what to commit to "git commit", so it should
behave the same.

This is one of the times I wish I said "No, you cannot have a pony".
The change was done without thinking things through, and reviewers
including me did not realize this particular downside.  My accepting
this misfeature (or a poorly implemented feature that has a
potential to be useful) was essentially me saying:

    When making a commit that does not match my working tree state,
    I always check with "diff --cached" to make sure what I think I
    am committing matches what I am committing, so I won't use such
    a lazy option myself.  I am not excited to think things through
    to see what possible pitfalls the feature may have for you; I'll
    let you guys hang yourself with that long rope.

And we are seeing a backfire from that "not bothering to think
things thorough".

I think the right thing to do is to fix "git commit -p" so that it
starts from the HEAD (on a temporary index), just like how partial
commits are made with "git commit file1 file2".   Or just forbid it
when the index does not match HEAD.

Cf. 

  http://thread.gmane.org/gmane.comp.version-control.git/173033/focus=173246

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Fri, Oct 05, 2012 at 03:29:10PM -0700, Junio C Hamano wrote:

> Assuming that the last step of what Horst did was "git commit -pm",
> I think Git is wrong in this case.  When you tell "git commit" what
> to commit, unless you give "-i" (aka "also") option, the command
> makes a commit to record changes only from what you tell "git
> commit" to commit, regardless of what you earlier did to the index.

Yeah. Defaulting to "-o" would match the rest of git-commit's behavior
much better.

> This is one of the times I wish I said "No, you cannot have a pony".
> The change was done without thinking things through, and reviewers
> including me did not realize this particular downside.
> [...]
> Cf. 
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/173033/focus=173246

Actually, I am not sure that thread or feature is to blame. Certainly it
would have been an opportune time to notice the problem. But this issue
goes back much further for "git commit --interactive", which has always
assumed "-i" rather than "-o". This even predates the switch from shell
to C; you can see the same behavior from 6cbf07e (git-commit: add a
--interactive option, 2007-03-05).

I guess you could argue that "--interactive" and "--patch" should have
different defaults, but I'm not sure I agree. They should both match
what "git commit foo" does by default.

> I think the right thing to do is to fix "git commit -p" so that it
> starts from the HEAD (on a temporary index), just like how partial
> commits are made with "git commit file1 file2".   Or just forbid it
> when the index does not match HEAD.

Agreed. I am inclined to call this a bugfix, though it does worry me
slightly that we would be changing a behavior that has existed for so
many years.

We should probably also support explicit "-i -p" and "-o -p" options, as
well (the former would give people who really want the existing behavior
a way to get it). And the same for "--interactive". I can't say I'm
excited about making all that work, though. Like you, I think it is more
sane to use existing tools to inspect and tweak the index to your
liking, and then commit.

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Actually, I am not sure that thread or feature is to blame. Certainly it
> would have been an opportune time to notice the problem. But this issue
> goes back much further for "git commit --interactive", which has always
> assumed "-i" rather than "-o". This even predates the switch from shell
> to C; you can see the same behavior from 6cbf07e (git-commit: add a
> --interactive option, 2007-03-05).

Yes.  That was after we started defaulting to "only" (not "also")
semantics when the command is run with paths, and it should also
have raised a red flag to reviewers.

In the case of "add/commit --interactive", it is much more clear
what state the index is in when the command gave interactive control
to the user.  The short-cut "add/commit -p" interface, however, does
not give you an access to its s)tatus subcommand, making the user
experience somewhat different.

That makes the problem much more severe for "-p" compared to
"--interactive", but the fundamental UI consistency it introduces is
the same as the issue under discussion in this thread.

>> I think the right thing to do is to fix "git commit -p" so that it
>> starts from the HEAD (on a temporary index), just like how partial
>> commits are made with "git commit file1 file2".   Or just forbid it
>> when the index does not match HEAD.
>
> Agreed. I am inclined to call this a bugfix, though it does worry me
> slightly that we would be changing a behavior that has existed for so
> many years.

I agree it will be a bugfix, but I am afraid that the fix may have
to be much more involved than "start from a temporary index that
matches HEAD when we are doing the '--only' semantics".

Suppose you have two paths E and F, both of which have differences
between HEAD and the index, and the index and the working tree file
(i.e. you earlier edited E and F, did "git add E F" and further
edited them).

You say "git commit -p F".

What should happen?  It is clear that the resulting commit should
record no change since its parent commit at path E (that is what
"only" semantics mean).

What state should the "add -p" interaction start from for path F?
Should you be picking from a patch between the state you previously
"git add"ed to the index and the working tree, or should the entire
difference between HEAD and the working tree eligible to be picked
or deferred during the "add -p" session?  Starting from a temporary
index that matches HEAD essentially means that you lose the earlier
"git add F" [*1*].

Another case to consider is to start from the same condition, and
instead to say "git commit -p" without any pathspec.  What should
happen?

Just doing "use a temporary index that is initialized to HEAD" may
be an expedient thing to do, but I suspect that I will be saying the
same "I should have said 'You cannot have a pony' back then" again
in a not so distant future if we did so without thinking these
things through.

As I do not see any practical value in "commit -p", I do not think
it is worth my time thinking these things through thoroughly myself.

Unless somebody who cares about "commit -p" does so to come up with
reasonable semantics, and updates the code to match that desired
behaviour, the responsible thing to do is to error out "-p" when
your index is different from HEAD, I think.


[Footnote]

*1* A not-so-deep thinking of the above might lead to "start from
the index that match HEAD, except for paths specified on the
pathspec given to the -p option".  But I do not think it is
satisfactory, either.  With "add -i" (or "commit --interactive"),
you have an option to selectively discard parts of your previous,
overzealous "git add F" with its r)evert action, but because "commit
-p" does not give an option to switch to "reset -p", you can only
add hunks, People who did "git add E F" earlier and then wants to
amend that earlier add with "git commit -p F", but it does not allow
them to fully amend their earlier action. That is the one of the
reasons why I think "commit -p" is a mistaken "we can save one
command invocation" false economy that adds confusion without adding
much value to the UI.

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Fri, Oct 05, 2012 at 11:26:47PM -0700, Junio C Hamano wrote:

> In the case of "add/commit --interactive", it is much more clear
> what state the index is in when the command gave interactive control
> to the user.  The short-cut "add/commit -p" interface, however, does
> not give you an access to its s)tatus subcommand, making the user
> experience somewhat different.
> 
> That makes the problem much more severe for "-p" compared to
> "--interactive", but the fundamental UI consistency it introduces is
> the same as the issue under discussion in this thread.

Agreed.

> Suppose you have two paths E and F, both of which have differences
> between HEAD and the index, and the index and the working tree file
> (i.e. you earlier edited E and F, did "git add E F" and further
> edited them).
> 
> You say "git commit -p F".
> 
> What should happen?  It is clear that the resulting commit should
> record no change since its parent commit at path E (that is what
> "only" semantics mean).
> 
> What state should the "add -p" interaction start from for path F?
> Should you be picking from a patch between the state you previously
> "git add"ed to the index and the working tree, or should the entire
> difference between HEAD and the working tree eligible to be picked
> or deferred during the "add -p" session?  Starting from a temporary
> index that matches HEAD essentially means that you lose the earlier
> "git add F" [*1*].
> 
> Another case to consider is to start from the same condition, and
> instead to say "git commit -p" without any pathspec.  What should
> happen?

Hmm. Good questions. In the former case, I would have said you should
definitely omit E and then start from the staged point of F, as that is
almost certainly what the user meant. But that is utterly inconsistent
with what we are discussing for the no-pathspec case.

I have a gut feeling that what I would expect for "-p" is roughly:

  1. Feed add--interactive the current index state.

  2. Feed add--interactive the set of pathspecs on the command line to
     limit its work.

  3. For any path that is updated by the interactive session, keep the
     result.

  4. For other paths, revert to HEAD.

I think that would "do what I mean" most of the time. But it is a
horrible set of rules to try to explain to someone (and it is off the
top of my head; I wouldn't be surprised if you can come up with a
situation where those rules do not behave well).

> Just doing "use a temporary index that is initialized to HEAD" may
> be an expedient thing to do, but I suspect that I will be saying the
> same "I should have said 'You cannot have a pony' back then" again
> in a not so distant future if we did so without thinking these
> things through.
> 
> As I do not see any practical value in "commit -p", I do not think
> it is worth my time thinking these things through thoroughly myself.
> 
> Unless somebody who cares about "commit -p" does so to come up with
> reasonable semantics, and updates the code to match that desired
> behaviour, the responsible thing to do is to error out "-p" when
> your index is different from HEAD, I think.

Yeah. I did not agree with your conclusion here when we started the
conversation, but I am starting to now. I am not opposed at all to
somebody working out the semantics, but I do not really care to work on
it myself. In the meantime, I would rather not do any halfway fixes
that will just make things worse.

Another option is to leave it with "-i" semantics in the meantime, which
are at least easy to explain: it is simply a shorthand for running "git
add -p && git commit". That may be inconsistent with other aspects of
commit, but people have (apparently) been happy with it, and there has
not been a rash of complaints.

As a non-user of "commit -p" myself, I don't have a strong opinion
either way.

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Another option is to leave it with "-i" semantics in the meantime, which
> are at least easy to explain: it is simply a shorthand for running "git
> add -p && git commit". That may be inconsistent with other aspects of
> commit, but people have (apparently) been happy with it, and there has
> not been a rash of complaints.

Yeah, that would be the safest and possibly the sanest way forward.
Did the documentation update patch by Conrad on the other subthread
look sane to you?  I haven't read it very carefully yet.

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Sat, Oct 06, 2012 at 11:22:50AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Another option is to leave it with "-i" semantics in the meantime, which
> > are at least easy to explain: it is simply a shorthand for running "git
> > add -p && git commit". That may be inconsistent with other aspects of
> > commit, but people have (apparently) been happy with it, and there has
> > not been a rash of complaints.
> 
> Yeah, that would be the safest and possibly the sanest way forward.
> Did the documentation update patch by Conrad on the other subthread
> look sane to you?  I haven't read it very carefully yet.

I didn't notice any documentation patch, and I can't find one looking
through the archive. Do you have a link?

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Conrad Irwin]

I think I messed up sending somehow:

On Fri, Oct 5, 2012 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Suppose you have two paths E and F, both of which have differences
> between HEAD and the index, and the index and the working tree file
> (i.e. you earlier edited E and F, did "git add E F" and further
> edited them).
>
> You say "git commit -p F".
[...]
>
> What state should the "add -p" interaction start from for path F?
> Should you be picking from a patch between the state you previously
> "git add"ed to the index and the working tree, or should the entire
> difference between HEAD and the working tree eligible to be picked
> or deferred during the "add -p" session?  Starting from a temporary
> index that matches HEAD essentially means that you lose the earlier
> "git add F" [*1*].

Two questions are easier answered:

What should git commit --only --patch F do?
=> It should start you from the state of HEAD.

What should git commit --include --patch F do?
=> It should start you from the state of the index.

The question that's harder to ponder, is "what should the default be".
Historically it's been '--include', but that was for the sake of easy
implementation (6cbf07efc5702351897dee4742525c9b9f7828ac). Using '--only' seems
good for consistency with other forms of git commit and the current
documentation; inventing a third way (i.e. depending on which paths are
specified) seems worst of all.

The big UI problem with --only is not figuring out what should go in the commit,
but rather ensuring that the index is in the expected state after the commit
(it's the problems solved by 2888605c649ccd423232161186d72c0e6c458a48 but for
hunks instead of files). If file F has hunks (H, J, K) then I stage hunk J with
git add --interactive; then I commit hunks H & K with git commit --interactive,
the resulting index should contain H, J, K. Unfortunately, git add --interactive
allows me to edit hunks, and so if I instead commit H & J2 (where J2 is an
edited version of J) then the index would contain (H, J) and the commit (H, J2);
the working tree would contain H, J, K still.

This gets a bit mind-bending to resolve; the first solution I came up with
"don't touch the index if the index differs from HEAD" will give unexpected
results in the case that extra non-conflicting chunks are added to the commit.
The next idea is to do a three-way merge between the new commit and the index
with the old HEAD as the base, and resolve conflicts in favour of the index. I
think that works, but it sounds pretty horrific to implement and still leaves
you in a pretty confusing state (though no more confusing than using edit in git
add --interactive normally is).

The other cases to consider are files that aren't in HEAD. At the moment git add
 --patch and git commit --patch cannot include new files, though that's fixable
by treating new files as 1 hunk instead of 0.

All in all, I think supporting --only --interactive is well beyond what I'm
capable of doing, and probably pushing the limits of what's sane. (it would be
nice for warm fuzzy completeness reasons though).

On Fri, Oct 5, 2012 at 3:57 PM, Jeff King <peff@peff.net> wrote:
> We should probably also support explicit "-i -p" and "-o -p" options, as
> well (the former would give people who really want the existing behavior
> a way to get it). And the same for "--interactive". I can't say I'm
> excited about making all that work, though. Like you, I think it is more
> sane to use existing tools to inspect and tweak the index to your
> liking, and then commit.

You made the same thinko as me :). --include isn't defined to mean "include the
index as well", but rather "include these files when committing the index".
Flipping that around makes a lot of sense and then --include can be used
semantically with --patch, --interactive or even --all. (patch attached).

>
> Unless somebody who cares about "commit -p" does so to come up with
> reasonable semantics, and updates the code to match that desired
> behaviour, the responsible thing to do is to error out "-p" when
> your index is different from HEAD, I think.

That would be a shame; instead we should just document that "--interactive" and
"--patch" add to the existing index like they always have. If we still worry
about users shooting themselves in the foot, then we can require "--include" to
use --interactive or --patch on a dirty index. (not done)

Conrad

--------8<------

Flip the meaning of 'git commit --include' from 'include these files' to
'include the index' to reduce the number of concepts in the manpage.

Clarify that --interactive/--patch add to the existing index to avoid
confusion like [1].

[1] http://thread.gmane.org/gmane.comp.version-control.git/207108

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-commit.txt | 20 +++++++++++---------
 builtin/commit.c             | 10 ++++++----
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9594ac8..a2d4a6d 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -41,9 +41,9 @@ The content to be added can be specified in several ways:
    actual commit;

 5. by using the --interactive or --patch switches with the 'commit' command
-   to decide one by one which files or hunks should be part of the commit,
-   before finalizing the operation. See the ``Interactive Mode'' section of
-   linkgit:git-add[1] to learn how to operate these modes.
+   to add files or hunks to the current index before committing. See the
+   ``Interactive Mode'' section of linkgit:git-add[1] to learn how to
+   operate these modes.

 The `--dry-run` option can be used to obtain a
 summary of what is included by any of the above for the next
@@ -63,10 +63,14 @@ OPTIONS

 -p::
 --patch::
-       Use the interactive patch selection interface to chose
-       which changes to commit. See linkgit:git-add[1] for
+       Use the interactive patch selection interface to add hunks
+       to the index before committing. See linkgit:git-add[1] for
        details.

+--interactive::
+       Use the ``Interactive mode'' of linkgit:git-add[1] to edit
+       the index before committing.
+
 -C <commit>::
 --reuse-message=<commit>::
        Take an existing commit object, and reuse the log message
@@ -215,10 +219,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)

 -i::
 --include::
-       Before making a commit out of staged contents so far,
-       stage the contents of paths given on the command line
-       as well.  This is usually not what you want unless you
-       are concluding a conflicted merge.
+       In addition to the paths specified on the command line,
+       include the current contents of the index in the commit.

 -o::
 --only::
diff --git a/builtin/commit.c b/builtin/commit.c
index a17a5df..14afa58 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1034,10 +1034,12 @@ static int parse_and_validate_options(int
argc, const char *argv[],
        if (patch_interactive)
                interactive = 1;

-       if (!!also + !!only + !!all + !!interactive > 1)
-               die(_("Only one of
--include/--only/--all/--interactive/--patch can be used."));
-       if (argc == 0 && (also || (only && !amend)))
-               die(_("No paths with --include/--only does not make sense."));
+       if (only && all)
+               die(_("--only with --all does not make sense."));
+       if (only && interactive)
+               die(_("--only with --interactive/--patch is not supported."));
+       if (argc == 0 && (only && !amend))
+               die(_("No paths with --only does not make sense."));
        if (argc == 0 && only && amend)
                only_include_assumed = _("Clever... amending the last
one with dirty index.");
        if (argc > 0 && !also && !only)

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Sat, Oct 06, 2012 at 11:32:51AM -0700, Conrad Irwin wrote:

> I think I messed up sending somehow:

Thanks for resending.

> > What state should the "add -p" interaction start from for path F?
> > Should you be picking from a patch between the state you previously
> > "git add"ed to the index and the working tree, or should the entire
> > difference between HEAD and the working tree eligible to be picked
> > or deferred during the "add -p" session?  Starting from a temporary
> > index that matches HEAD essentially means that you lose the earlier
> > "git add F" [*1*].
> 
> Two questions are easier answered:
> 
> What should git commit --only --patch F do?
> => It should start you from the state of HEAD.

Are you sure?  Does "--only" mean "only the changes I am about to mark"
or "only the paths I am about to tell you about"? Without partial hunk
selection (i.e., "commit -p"), they were the same; a path you mention is
a path which will be either be staged in its entirety or not. Specifying
(or omitting) the path was sufficient to say what you wanted. But with
"-p", I can see three useful possibilities:

  1. Do not include F in the commit, even if changes are staged in the
     index (i.e., take HEAD exactly).

  2. Include F in the commit, and stage partial changes on top of what is
     already staged.

  3. Include F in the commit, and stage partial changes on top of HEAD.

In cases 2 and 3, we are still taking "only the path" F. But we are
not taking "only what is about to be staged" in 2. And I can see both
being useful (2 because it is more convenient not to re-approve staged
changes, and 3 because there is no way to unstage changes via "-p").

> What should git commit --include --patch F do?
> => It should start you from the state of the index.

This one is much easier. The distinction between cases 2 and 3 above
does not exist here, because we always start from the current index
state.

So there are two questions:

  1. How does --only interact with partial staging (whether paths are
     specified or not)?

  2. What should the default for "-p" be, between "--only" and
     "--include"?

I think the answer to the second is "--only"; but a prerequisite to that
is making "--only" work at all (it currently just barfs). And a
prerequisite to that is figuring out what the right semantics are.

> The question that's harder to ponder, is "what should the default be".

Interestingly, I came to the exact opposite conclusion of which question
is harder. :)

> The big UI problem with --only is not figuring out what should go in the commit,
> but rather ensuring that the index is in the expected state after the commit
> (it's the problems solved by 2888605c649ccd423232161186d72c0e6c458a48 but for
> hunks instead of files). If file F has hunks (H, J, K) then I stage hunk J with
> git add --interactive; then I commit hunks H & K with git commit --interactive,
> the resulting index should contain H, J, K. Unfortunately, git add --interactive
> allows me to edit hunks, and so if I instead commit H & J2 (where J2 is an
> edited version of J) then the index would contain (H, J) and the commit (H, J2);
> the working tree would contain H, J, K still.

Yeah, that's a gross-ness I hadn't even considered.

> All in all, I think supporting --only --interactive is well beyond what I'm
> capable of doing, and probably pushing the limits of what's sane. (it would be
> nice for warm fuzzy completeness reasons though).

Yes. The more we talk about it, the more turned off I am by the idea.
Above I posed my questions as "what _should_ we do when...". And I still
think we _should_ default to --only with interactive, if we can find
sane semantics. But until we can find them, it obviously does not make
sense to enable it, and the whole discussion is stalled. And we must
come up with an interim solution that is the least bad.

Which is obviously one of:

  1. Keep defaulting to "--include", as that is what we have been doing.

  2. Forbid the cases where it would matter (i.e., when the index and
     HEAD differ).

The former is more convenient, but the latter is safer against future
breakage. I'm OK either way, but option (1) clearly needs a
documentation update.

> On Fri, Oct 5, 2012 at 3:57 PM, Jeff King <peff@peff.net> wrote:
> > We should probably also support explicit "-i -p" and "-o -p" options, as
> > well (the former would give people who really want the existing behavior
> > a way to get it). And the same for "--interactive". I can't say I'm
> > excited about making all that work, though. Like you, I think it is more
> > sane to use existing tools to inspect and tweak the index to your
> > liking, and then commit.
> 
> You made the same thinko as me :). --include isn't defined to mean "include the
> index as well", but rather "include these files when committing the index".
> Flipping that around makes a lot of sense and then --include can be used
> semantically with --patch, --interactive or even --all. (patch attached).

But of course we're not specifying paths. So to me it is "include the
changes I am about to stage via -p", as opposed to "--only use the
changes I am about to stage via -p". I think the current behavior is
morally equivalent to how --include works with paths (which includes the
paths along with the current index, rather than only committing the
paths).

Or am I missing something about the distinction you're making? It seems
to me that the end behavior of thinking about it either way would be the
same.

> --------8<------
> 
> Flip the meaning of 'git commit --include' from 'include these files' to
> 'include the index' to reduce the number of concepts in the manpage.
> 
> Clarify that --interactive/--patch add to the existing index to avoid
> confusion like [1].
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/207108

The documentation updates look like an improvement to me. Do we also
need a note about "-p" under "-o" where it says "This is the default
mode..."?

> diff --git a/builtin/commit.c b/builtin/commit.c
> index a17a5df..14afa58 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1034,10 +1034,12 @@ static int parse_and_validate_options(int
> argc, const char *argv[],
>         if (patch_interactive)
>                 interactive = 1;
> 
> -       if (!!also + !!only + !!all + !!interactive > 1)
> -               die(_("Only one of
> --include/--only/--all/--interactive/--patch can be used."));
> -       if (argc == 0 && (also || (only && !amend)))
> -               die(_("No paths with --include/--only does not make sense."));
> +       if (only && all)
> +               die(_("--only with --all does not make sense."));
> +       if (only && interactive)
> +               die(_("--only with --interactive/--patch is not supported."));

We used to complain if (argc == 0 && also), but that seems to be lost
here. Wouldn't the new condition be (argc == 0 && also && !interactive)?

We also stopped complaining about "also && all", "all && interactive",
and "also && only", all of which are nonsensical.

> +       if (argc == 0 && (only && !amend))
> +               die(_("No paths with --only does not make sense."));
>         if (argc == 0 && only && amend)
>                 only_include_assumed = _("Clever... amending the last

It might be more readable to collapse these two conditionals to:

  if (argc == 0 && only) {
  	if (!amend)
		die("...does not make sense");
	only_include_assumed = "Clever..."
  }

I wonder if it is even worth loosening this, though. The point, as I
understand it, would be to allow "-i -p". But it doesn't actually do
anything, does it (except, I suppose for allowing one to future-proof
their script against a later change of the default to --only).

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yes. The more we talk about it, the more turned off I am by the idea.
> Above I posed my questions as "what _should_ we do when...". And I still
> think we _should_ default to --only with interactive, if we can find
> sane semantics. But until we can find them, it obviously does not make
> sense to enable it, and the whole discussion is stalled. And we must
> come up with an interim solution that is the least bad.
>
> Which is obviously one of:
>
>   1. Keep defaulting to "--include", as that is what we have been doing.
>
>   2. Forbid the cases where it would matter (i.e., when the index and
>      HEAD differ).
>
> The former is more convenient, but the latter is safer against
> future breakage. I'm OK either way, but option (1) clearly needs a
> documentation update.

Yeah, I agree with the reasoning.  This is an unessential feature
that is with the problem for a long time, so let's go the route #1
first before we do anything else.

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Sun, Oct 07, 2012 at 01:51:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yes. The more we talk about it, the more turned off I am by the idea.
> > Above I posed my questions as "what _should_ we do when...". And I still
> > think we _should_ default to --only with interactive, if we can find
> > sane semantics. But until we can find them, it obviously does not make
> > sense to enable it, and the whole discussion is stalled. And we must
> > come up with an interim solution that is the least bad.
> >
> > Which is obviously one of:
> >
> >   1. Keep defaulting to "--include", as that is what we have been doing.
> >
> >   2. Forbid the cases where it would matter (i.e., when the index and
> >      HEAD differ).
> >
> > The former is more convenient, but the latter is safer against
> > future breakage. I'm OK either way, but option (1) clearly needs a
> > documentation update.
> 
> Yeah, I agree with the reasoning.  This is an unessential feature
> that is with the problem for a long time, so let's go the route #1
> first before we do anything else.

OK. I think Conrad's patch takes us most of the way there. I had a few
minor comments, but I think another round should do it. Conrad?

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Oct 07, 2012 at 01:51:21PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Which is obviously one of:
>> >
>> >   1. Keep defaulting to "--include", as that is what we have been doing.
>> >
>> >   2. Forbid the cases where it would matter (i.e., when the index and
>> >      HEAD differ).
>> >
>> > The former is more convenient, but the latter is safer against
>> > future breakage. I'm OK either way, but option (1) clearly needs a
>> > documentation update.
>> 
>> Yeah, I agree with the reasoning.  This is an unessential feature
>> that is with the problem for a long time, so let's go the route #1
>> first before we do anything else.
>
> OK. I think Conrad's patch takes us most of the way there. I had a few
> minor comments, but I think another round should do it. Conrad?

I'd rather want to see a patch that _only_ documents the current
behaviour to unconfuse people first.  I definitely do not want any
patch that changes the command line parsing or any other behaviour
change with problems that have to take time from reviewers to point
them out mixed in it.

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Jeff King]

On Sun, Oct 07, 2012 at 03:23:31PM -0700, Junio C Hamano wrote:

> >> Yeah, I agree with the reasoning.  This is an unessential feature
> >> that is with the problem for a long time, so let's go the route #1
> >> first before we do anything else.
> >
> > OK. I think Conrad's patch takes us most of the way there. I had a few
> > minor comments, but I think another round should do it. Conrad?
> 
> I'd rather want to see a patch that _only_ documents the current
> behaviour to unconfuse people first.  I definitely do not want any
> patch that changes the command line parsing or any other behaviour
> change with problems that have to take time from reviewers to point
> them out mixed in it.

Sorry, I should have been more clear. I want to see a re-roll of only
the documentation bits of Conrad's patch, for which I had only minor
comments. The code part had major problems. :)

-Peff

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Conrad Irwin]

On Sat, Oct 6, 2012 at 12:07 PM, Jeff King <peff@peff.net> wrote:
> Are you sure?  Does "--only" mean "only the changes I am about to mark"
> or "only the paths I am about to tell you about"? Without partial hunk
> selection (i.e., "commit -p"), they were the same; a path you mention is
> a path which will be either be staged in its entirety or not. Specifying
> (or omitting) the path was sufficient to say what you wanted. But with
> "-p", I can see three useful possibilities:
>
>   1. Do not include F in the commit, even if changes are staged in the
>      index (i.e., take HEAD exactly).
>
>   2. Include F in the commit, and stage partial changes on top of what is
>      already staged.
>
>   3. Include F in the commit, and stage partial changes on top of HEAD.
>
> In cases 2 and 3, we are still taking "only the path" F. But we are
> not taking "only what is about to be staged" in 2. And I can see both
> being useful (2 because it is more convenient not to re-approve staged
> changes, and 3 because there is no way to unstage changes via "-p").

I think I didn't consider 2. as a viable alternative because
re-approving hunks is not a problem (there are typically very few
hunks per file, and you'll recognise them if you've already staged
them) but not being able to unstage is a big problem (as it restricts
what commits I can make with --patch without changing my index).

>
> But of course we're not specifying paths. So to me it is "include the
> changes I am about to stage via -p", as opposed to "--only use the
> changes I am about to stage via -p". I think the current behavior is
> morally equivalent to how --include works with paths (which includes the
> paths along with the current index, rather than only committing the
> paths).
>
> Or am I missing something about the distinction you're making? It seems
> to me that the end behavior of thinking about it either way would be the
> same.

The way I was thinking about it was to treat the index and the command
line as two orthogonal parts of the commit. --include and --only
control the inclusion/exclusion of the index; while the command line
arguments control which (currently unstaged) things are included. This
led me to the conclusion that "git commit --include" is equivalent to
"git commit", "git commit --include --all" is the same as "git commit
--all" which is why I tried to change the validation logic. (You are
correct that "--include --only" and "--interactive --all" still make
no sense).

Here's a re-roll of the patch with --only docs tweaked.

Conrad

----8<----

Clarify that --interactive/--patch add to the existing index to avoid
confusion like [1].

Make explicit that --only does not work with --interactive/--patch and
clean up wording around --only --amend.

[1] http://thread.gmane.org/gmane.comp.version-control.git/207108

Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
 Documentation/git-commit.txt | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9594ac8..680d2bf 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -41,9 +41,9 @@ The content to be added can be specified in several ways:
    actual commit;

 5. by using the --interactive or --patch switches with the 'commit' command
-   to decide one by one which files or hunks should be part of the commit,
-   before finalizing the operation. See the ``Interactive Mode'' section of
-   linkgit:git-add[1] to learn how to operate these modes.
+   to add files or hunks to the current index before committing. See the
+   ``Interactive Mode'' section of linkgit:git-add[1] to learn how to
+   operate these modes.

 The `--dry-run` option can be used to obtain a
 summary of what is included by any of the above for the next
@@ -63,10 +63,14 @@ OPTIONS

 -p::
 --patch::
-	Use the interactive patch selection interface to chose
-	which changes to commit. See linkgit:git-add[1] for
+	Use the interactive patch selection interface to add hunks
+	to the index before committing. See linkgit:git-add[1] for
 	details.

+--interactive::
+	Use the ``Interactive mode'' of linkgit:git-add[1] to edit
+	the index before committing.
+
 -C <commit>::
 --reuse-message=<commit>::
 	Take an existing commit object, and reuse the log message
@@ -215,22 +219,17 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)

 -i::
 --include::
-	Before making a commit out of staged contents so far,
-	stage the contents of paths given on the command line
-	as well.  This is usually not what you want unless you
-	are concluding a conflicted merge.
+	In addition to the paths specified on the command line,
+	include the current contents of the index in the commit.

 -o::
 --only::
-	Make a commit only from the paths specified on the
-	command line, disregarding any contents that have been
-	staged so far. This is the default mode of operation of
-	'git commit' if any paths are given on the command line,
-	in which case this option can be omitted.
-	If this option is specified together with '--amend', then
-	no paths need to be specified, which can be used to amend
-	the last commit without committing changes that have
-	already been staged.
+	Only commit changes to the paths specified on the command line,
+	do not include the current contents of the index. This is
+	the default mode of operation when paths are specified.
+	If this option is specified with --amend it can be used
+	to reword the last commit without changing its contents.
+	This mode cannot be used with --patch or --interactive.

 -u[<mode>]::
 --untracked-files[=<mode>]::
-- 
1.7.12.289.g0ce9864

Re: git 1.8.0.rc0.18.gf84667d trouble with "git commit -p file"

[Junio C Hamano]

Conrad Irwin <conrad.irwin@gmail.com> writes:

>  -i::
>  --include::
> -	Before making a commit out of staged contents so far,
> -	stage the contents of paths given on the command line
> -	as well.  This is usually not what you want unless you
> -	are concluding a conflicted merge.
> +	In addition to the paths specified on the command line,
> +	include the current contents of the index in the commit.

"commit" is about committing what is in the index.  include has
always meant "in addition, include the contents of listed paths
in the resulting commit".

The updated text looks totally the other way around.

>  -o::
>  --only::
> -	Make a commit only from the paths specified on the
> -	command line, disregarding any contents that have been
> -	staged so far. This is the default mode of operation of
> -	'git commit' if any paths are given on the command line,
> -	in which case this option can be omitted.
> -	If this option is specified together with '--amend', then
> -	no paths need to be specified, which can be used to amend
> -	the last commit without committing changes that have
> -	already been staged.
> +	Only commit changes to the paths specified on the command line,
> +	do not include the current contents of the index. This is
> +	the default mode of operation when paths are specified.
> +	If this option is specified with --amend it can be used
> +	to reword the last commit without changing its contents.
> +	This mode cannot be used with --patch or --interactive.

The new text on this one does look cleaner and easier to read, at
least to me, but "do not include the current contents" sounds as if
you are recording a tree that only has Makefile and losing all the
other files when you say "git commit Makefile".

    Disregard what has been added to the index since HEAD, and only
    commit changes to the given paths.

might be an improvement, but I dunno.