Re: [PATCH v3 0/3] Git commit --patch (again)

[Jeff King <peff@peff.net>]

On Fri, May 06, 2011 at 10:59:58PM -0700, conrad.irwin@gmail.com wrote:

> I've rebased my support for git commit -p onto the current master
> branch. I've posted it to the list twice before [1][2].

Thanks for reposting. I had been meaning to look at this again, but
hadn't gotten around to it. So thanks for being persistent. :)

>   Use a temporary index for git commit --interactive

I think the intent of this one is good. In reviewing your initial
series, I had wondered about consistency with respect to the atomicity
of "git commit -p" versus "git add -p" (i.e., what state is the index
left in when you abort). But reading through the discussion again, I
think we should worry more about consistency between "git commit -i" and
"git commit --interactive". That is, both should produce no changes to
the index when the commit is aborted. So I think your patch is a step in
the right direction.

That still leaves an inconsistency in "git add -p" versus "git commit
-p" (e.g., if you abort "git add -p" with "^C"). But if we care, the
right solution is probably to make "git add -p" atomic. That can be a
separate topic, though, and I'm not sure anyone really cares enough to
work on it.

I have one final question. If I do abort a commit, is there any way to
recover the state that was in the temporary index? That is, if I abort
"git commit -i" by using an empty commit message, it is easy enough to
use shell history to repeat the command (possibly with a different set
of files). But if I spend some time selecting (and possibly editing)
hunks, and then decide to abort the commit, is there any way to recover
the intermediate index state?

>From my reading of the code, it looks like "no". We will rollback the
lockfile which contains the new index when aborting the commit.

I'm not sure if it is worth caring about. If you are really interested
in index state, you are probably better off using "git add -p" and "git
commit" separately. And even if we kept the index file around, it
requires a fairly savvy plumbing user to be able to pick changes out of
it.

>   Allow git commit --interactive with paths

Hmm. Test t7501.8 explicitly tests that this isn't allowed. But the test
is poorly written, and falsely returns success even with your patch.

The original test should have looked like this:

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 7f7f7c7..8090b3c 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -45,7 +45,8 @@ test_expect_success \
 test_expect_success PERL \
 	"using paths with --interactive" \
 	"echo bong-o-bong >file &&
-	! (echo 7 | git commit -m foo --interactive file)"
+	! ({ echo 2; echo 1; echo; echo 7; } |
+	git commit -m foo --interactive file)"
 
 test_expect_success \
 	"using invalid commit with -C" \

which does properly fail with your change. Your commit should tweak that
test (speaking of which, it would be nice for patch 1 to have a test,
too).

Other than that, the code in all 3 looks fine to me.

-Peff