I think I messed up sending somehow:
On Fri, Oct 5, 2012 at 11:26 PM, Junio C Hamano [off-list ref] 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 [off-list ref] 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 <redacted>
---
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)
On Sat, Oct 06, 2012 at 11:32:51AM -0700, Conrad Irwin wrote:
I think I messed up sending somehow:
Thanks for resending.
quoted
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 [off-list ref] wrote:
quoted
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..."?
quoted hunk
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