git stash apply usability issues

(1) Looking at git-stash.sh I see a few uses of 'git diff' in apply_stash(). 
Shouldn't these use one of git-diff-{tree,index,files)? The reason is that 
porcelain 'git diff' invokes custom diff drivers (that in my case run a UI 
program), whereas the plumbing does not.

Is there a particular reason to use porcelain 'git diff'?

(2) when 'git stash apply' runs merge-recursive, it treats the current state 
as 'ours' and the stash as 'theirs'. IMHO it should be the other way round: 
I have stashed away changes to a binary file. Then committed a different 
modification to it, and now want to apply the stash. This results in a 
conflict that leaves the current state in the working tree, but I had 
preferred that the stashed binary file were in the working tree now.

What do other git-stash users think about changing the order?

-- Hannes

Johannes Sixt wrote:

(2) when 'git stash apply' runs merge-recursive, it treats the current 
state as 'ours' and the stash as 'theirs'. IMHO it should be the other 
way round: I have stashed away changes to a binary file. Then 
committed a different modification to it, and now want to apply the 
stash. This results in a conflict that leaves the current state in the 
working tree, but I had preferred that the stashed binary file were in 
the working tree now.

What do other git-stash users think about changing the order?

Seems right to me. I'd expect to get the stashed version in the working 
tree in that case.

-Steve

Johannes Sixt [off-list ref] wrote:

(2) when 'git stash apply' runs merge-recursive, it treats the current 
state as 'ours' and the stash as 'theirs'. IMHO it should be the other way 
round: I have stashed away changes to a binary file. Then committed a 
different modification to it, and now want to apply the stash. This results 
in a conflict that leaves the current state in the working tree, but I had 
preferred that the stashed binary file were in the working tree now.

What do other git-stash users think about changing the order?

The current order is the same order that git-rebase uses.  I'm not
saying its correct, just that its the same as rebase.  I think rebase
is also backwards and if we change git-stash we should also change
git-rebase at the same time (though probably not in the same commit).

-- 
Shawn.

git-stash needs to restrict itself to plumbing when running automated
diffs as part of its operation as the user may have configured a
custom diff driver that opens an interactive UI for certain/all
files.  Doing that during scripted actions is very unfriendly to
the end-user and may cause git-stash to fail to work.

Reported by Johannes Sixt

Signed-off-by: Shawn O. Pearce <redacted>
---

 Johannes Sixt [off-list ref] wrote:
 > (1) Looking at git-stash.sh I see a few uses of 'git diff' in
 > apply_stash(). Shouldn't these use one of git-diff-{tree,index,files)? The
 > reason is that porcelain 'git diff' invokes custom diff drivers (that in my   
 > case run a UI program), whereas the plumbing does not.
 >
 > Is there a particular reason to use porcelain 'git diff'?

 Does this fix the problem?

 git-stash.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7ba6162..def3163 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -110,7 +110,7 @@ show_stash () {
 
 	w_commit=$(git rev-parse --verify "$s") &&
 	b_commit=$(git rev-parse --verify "$s^") &&
-	git diff $flags $b_commit $w_commit
+	git diff-tree $flags $b_commit $w_commit
 }
 
 apply_stash () {

@@ -139,7 +139,7 @@ apply_stash () {
 	unstashed_index_tree=
 	if test -n "$unstash_index" && test "$b_tree" != "$i_tree"
 	then
-		git diff --binary $s^2^..$s^2 | git apply --cached
+		git diff-tree --binary $s^2^..$s^2 | git apply --cached
 		test $? -ne 0 &&
 			die 'Conflicts in index. Try without --index.'
 		unstashed_index_tree=$(git-write-tree) ||

@@ -162,7 +162,7 @@ apply_stash () {
 			git read-tree "$unstashed_index_tree"
 		else
 			a="$TMP-added" &&
-			git diff --cached --name-only --diff-filter=A $c_tree >"$a" &&
+			git diff-index --cached --name-only --diff-filter=A $c_tree >"$a" &&
 			git read-tree --reset $c_tree &&
 			git update-index --add --stdin <"$a" ||
 				die "Cannot unstage modified files"

-- 
1.5.3.4.1249.g895be

Shawn,

thanks for the fast response with a patch.

Shawn O. Pearce schrieb:

 Johannes Sixt [off-list ref] wrote:
 > (1) Looking at git-stash.sh I see a few uses of 'git diff' in
 > apply_stash(). Shouldn't these use one of git-diff-{tree,index,files)? The
 > reason is that porcelain 'git diff' invokes custom diff drivers (that in my   
 > case run a UI program), whereas the plumbing does not.
 >
 > Is there a particular reason to use porcelain 'git diff'?

 Does this fix the problem?

It does!

However, this porcelain 'git diff' should actually remain because it's part 
of show_stash().

-- Hannes

Johannes Sixt [off-list ref] wrote:

Shawn O. Pearce schrieb:

quoted

Johannes Sixt [off-list ref] wrote:

quoted

(1) Looking at git-stash.sh I see a few uses of 'git diff' in
apply_stash(). Shouldn't these use one of git-diff-{tree,index,files)? 

The

quoted

reason is that porcelain 'git diff' invokes custom diff drivers (that 

in my   > case run a UI program), whereas the plumbing does not.

quoted

Is there a particular reason to use porcelain 'git diff'?

Does this fix the problem?

It does!

quoted

@@ -110,7 +110,7 @@ show_stash () {

	w_commit=$(git rev-parse --verify "$s") &&
	b_commit=$(git rev-parse --verify "$s^") &&
-	git diff $flags $b_commit $w_commit
+	git diff-tree $flags $b_commit $w_commit

However, this porcelain 'git diff' should actually remain because it's part 
of show_stash().

Heh.  Damn.  I was just starting to prepare my evening push and
this patch is in maint, which I just merged to master, and I just
rebased all of my pu topic branches over that.  Junio's Meta toolkit
doesn't have an "unrebase" so I can go back and amend that damn
commit before pushing.

I'm feeling lazy and don't want to create an unRB right now.
I'll probably just throw another commit into maint to fix the
above hunk.  Thanks for catching it.

-- 
Shawn.

On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote:

Johannes Sixt [off-list ref] wrote:

quoted

(2) when 'git stash apply' runs merge-recursive, it treats the
current state as 'ours' and the stash as 'theirs'. IMHO it should
be the other way round: I have stashed away changes to a binary
file. Then committed a different modification to it, and now want
to apply the stash. This results in a conflict that leaves the
current state in the working tree, but I had preferred that the
stashed binary file were in the working tree now.

What do other git-stash users think about changing the order?

The current order is the same order that git-rebase uses. I'm not
saying its correct, just that its the same as rebase.

FWIW, StGit push works the same way. The idea being that the current
HEAD is our current state ("ours"), and the patch we're pushing is
some change we want to apply ("theirs"). I always felt that this was a
very natural order of things. But I guess the philosophy in the
"stash" case is subtly different, so maybe the change is warranted
there.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Karl Hasselström [off-list ref] writes:

On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote:

quoted

Johannes Sixt [off-list ref] wrote:

quoted

quoted

(2) when 'git stash apply' runs merge-recursive, it treats the
current state as 'ours' and the stash as 'theirs'. IMHO it should
be the other way round: I have stashed away changes to a binary
file. Then committed a different modification to it, and now want
to apply the stash. This results in a conflict that leaves the
current state in the working tree, but I had preferred that the
stashed binary file were in the working tree now.

What do other git-stash users think about changing the order?

The current order is the same order that git-rebase uses. I'm not
saying its correct, just that its the same as rebase.

FWIW, StGit push works the same way. The idea being that the current
HEAD is our current state ("ours"), and the patch we're pushing is
some change we want to apply ("theirs"). I always felt that this was a
very natural order of things. But I guess the philosophy in the
"stash" case is subtly different, so maybe the change is warranted
there.

Well, maybe one should then just name this "current" and "separate"
instead of "ours" and "theirs".

-- 
David Kastrup