threads / discuss / 10354

git stash apply usability issues

Subject: git stash apply usability issues

## tl;dr

8 messages between Oct 18, 2007 and Oct 19, 2007.

replies: 7people: 5as markdown or json

Johannes Sixt· Oct 18, 2007, 08:32 UTC · lore

(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
Steven Grimm· Oct 18, 2007, 14:12 UTC · re: Johannes Sixt · lore

Re: git stash apply usability issues

Johannes Sixt wrote:
Show 9 quoted lines
> (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
Shawn O. Pearce· Oct 19, 2007, 01:31 UTC · re: Johannes Sixt · lore

Re: git stash apply usability issues

Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 8 quoted lines
> (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.
Karl Hasselström· Oct 19, 2007, 13:27 UTC · re: Shawn O. Pearce · lore

Re: git stash apply usability issues

On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote:
> Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 12 quoted lines
> > (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
David Kastrup· Oct 19, 2007, 13:57 UTC · re: Karl Hasselström · lore

Re: git stash apply usability issues

Karl Hasselström <kha@treskal.com> writes:
Show 23 quoted lines
> 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
Shawn O. Pearce· Oct 19, 2007, 01:33 UTC · re: Johannes Sixt · lore

[PATCH] Avoid invoking diff drivers during git-stash

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
Johannes Sixt· Oct 19, 2007, 06:08 UTC · re: Shawn O. Pearce · lore

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

Shawn,
thanks for the fast response with a patch.
Shawn O. Pearce schrieb:
Show 9 quoted lines
>  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!
Show 6 quoted lines
> @@ -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
Shawn O. Pearce· Oct 19, 2007, 06:13 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 23 quoted lines
> 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.

← back to recent threads