git stash apply usability issues

git stash apply usability issues

[Johannes Sixt]

(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

Re: git stash apply usability issues

[Steven Grimm]

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

Re: git stash apply usability issues

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> 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.

[PATCH] Avoid invoking diff drivers during git-stash

[Shawn O. Pearce]

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 <spearce@spearce.org>
---

 Johannes Sixt <j.sixt@viscovery.net> 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

Re: [PATCH] Avoid invoking diff drivers during git-stash

[Johannes Sixt]

Shawn,

thanks for the fast response with a patch.

Shawn O. Pearce schrieb:
>  Johannes Sixt <j.sixt@viscovery.net> 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!

> @@ -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().

-- Hannes

Re: [PATCH] Avoid invoking diff drivers during git-stash

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> Shawn O. Pearce schrieb:
> > Johannes Sixt <j.sixt@viscovery.net> 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!
> 
> >@@ -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.

Re: git stash apply usability issues

[Karl Hasselström]

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

> Johannes Sixt <j.sixt@viscovery.net> 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.

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

Re: git stash apply usability issues

[David Kastrup]

Karl Hasselström <kha@treskal.com> writes:

> On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote:
>
>> Johannes Sixt <j.sixt@viscovery.net> 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.
>
> 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