[PATCH v5 0/6] {checkout,reset,stash} --patch

From: Thomas Rast <trast@student.ethz.ch>
Date: Aug 13, 2009, 12:29 UTC
Message-ID: <cover.1250164190.git.trast@student.ethz.ch>
In-Reply-To: <200908101136.34660.trast@student.ethz.ch>

Junio C Hamano wrote:

> * tr/reset-checkout-patch (Tue Jul 28 23:20:12 2009 +0200) 8 commits

[...]

> Progress?

Slow, as always.  There are three groups of changes:

1. This iteration goes the "complicated" way to mitigate Jeff's concern:

Jeff King wrote:

> Shouldn't the diff [in checkout -p] be reversed? That is, I think
> what users would like to see is "bring this hunk over from the index
> to the working tree". But we have the opposite (a hunk that is in
> the working tree that we would like to undo).

That is, the rules are now as follows:

add -p forward application reset -p [HEAD] exact opposite of add -p: reverse application reset -p other forward application to index (**) checkout -p "opposite of editing": reverse application checkout -p HEAD "opposite of editing and staging": reverse application checkout -p other forward application to WT and index (**) stash -p "stash these edits": reverse application to WT, "forward to stash"

Those marked (**) are the only ones that changed semantics compared to v4. However, I adjusted the messages to look different:

add -p Stage this hunk? reset -p [HEAD] Reset this hunk? (**) reset -p other Apply this hunk to index? (**) checkout -p Discard this hunk from worktree? (**) checkout -p HEAD Discard this hunk from index and worktree? (**) checkout -p other Apply this hunk to index and worktree? (**) stash -p Stash this hunk?

Again, (**) are the changed ones from v4. The help message also shows the "to/from ..." extra in the help for y/n.

I think this should now make 'reset -p' and 'checkout -p' fairly intuitive, while at the same time making the '... other' forms easier to wrap one's head around. Of course, as stated earlier in the thread, the downside with this approach is that the direction suddenly changes when you give it an 'other'.

These changes affect all 'Implement foo --patch' patches, and the git-apply--interactive refactoring.

2. git checkout -p HEAD fixed

Nicolas Sebrecht wrote:

Show 8 quoted lines

> 
>   % git checkout -p HEAD
> 
> and
> 
>   % git checkout -p HEAD -- file
> 
> behave differently here in my test above.

This sadly was a rather trivial thinko on my part in the C glue for 'checkout -p', which I fixed. I also changed the tests to cover various ways of limiting paths.

3. Tests rewritten

I added a new 2/6 refactors the many occurences of

	test "$(cat file)" = expected_worktree &&
	test "$(git show :file)" = expected_index

to a few library functions, and rewritten all three tests to use them. Due to the bug discussed in (2.) above, the tests also cover pathspecs for all new commands. Due to my own concern because this was broken at some point during development, all commands also check if relative paths inside a directory work.

3/6 (which was 2/5) and 7/6 (was 6/5) are unchanged, and apart from the fix for (2.) which was a one-liner, so is all the C code. 7/6 is, as before, based on a merge with js/stash-dwim.

Thomas Rast (7):
  git-apply--interactive: Refactor patch mode code
  Add a small patch-mode testing library
  builtin-add: refactor the meat of interactive_add()
  Implement 'git reset --patch'
  Implement 'git checkout --patch'
  Implement 'git stash save --patch'
  DWIM 'git stash save -p' for 'git stash -p'