I think I messed up sending somehow: On Fri, Oct 5, 2012 at 11:26 PM, Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxxx> --- 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) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html