# git stash apply usability issues 8 messages from 2007-10-18 to 2007-10-19. Participants: Johannes Sixt, Steven Grimm, Shawn O. Pearce, Karl Hasselström, David Kastrup. Thread: https://gitlist.dev/t/10354 ## Johannes Sixt, 2007-10-18 08:32 Subject: git stash apply usability issues Message-ID: <47171A21.9030003@viscovery.net> URL: https://gitlist.dev/e/47171A21.9030003%40viscovery.net ``` (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, 2007-10-18 14:12 Subject: Re: git stash apply usability issues Message-ID: <471769D3.4080908@midwinter.com> URL: https://gitlist.dev/e/471769D3.4080908%40midwinter.com In-Reply-To: <47171A21.9030003@viscovery.net> ``` 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 ``` ## Shawn O. Pearce, 2007-10-19 01:31 Subject: Re: git stash apply usability issues Message-ID: <20071019013156.GU14735@spearce.org> URL: https://gitlist.dev/e/20071019013156.GU14735%40spearce.org In-Reply-To: <47171A21.9030003@viscovery.net> ``` 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? 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. ``` ## Shawn O. Pearce, 2007-10-19 01:33 Subject: [PATCH] Avoid invoking diff drivers during git-stash Message-ID: <20071019013350.GA14020@spearce.org> URL: https://gitlist.dev/e/20071019013350.GA14020%40spearce.org In-Reply-To: <47171A21.9030003@viscovery.net> ``` 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 --- Johannes Sixt 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, 2007-10-19 06:08 Subject: Re: [PATCH] Avoid invoking diff drivers during git-stash Message-ID: <471849D7.1020303@viscovery.net> URL: https://gitlist.dev/e/471849D7.1020303%40viscovery.net In-Reply-To: <20071019013350.GA14020@spearce.org> ``` Shawn, thanks for the fast response with a patch. Shawn O. Pearce schrieb: > Johannes Sixt 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 ``` ## Shawn O. Pearce, 2007-10-19 06:13 Subject: Re: [PATCH] Avoid invoking diff drivers during git-stash Message-ID: <20071019061351.GL14735@spearce.org> URL: https://gitlist.dev/e/20071019061351.GL14735%40spearce.org In-Reply-To: <471849D7.1020303@viscovery.net> ``` Johannes Sixt wrote: > Shawn O. Pearce schrieb: > > Johannes Sixt 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. ``` ## Karl Hasselström, 2007-10-19 13:27 Subject: Re: git stash apply usability issues Message-ID: <20071019132753.GA23765@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20071019132753.GA23765%40diana.vm.bytemark.co.uk In-Reply-To: <20071019013156.GU14735@spearce.org> ``` On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote: > 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? > > 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, 2007-10-19 13:57 Subject: Re: git stash apply usability issues Message-ID: <86y7dz9ph8.fsf@lola.quinscape.zz> URL: https://gitlist.dev/e/86y7dz9ph8.fsf%40lola.quinscape.zz In-Reply-To: <20071019132753.GA23765@diana.vm.bytemark.co.uk> ``` Karl Hasselström writes: > On 2007-10-18 21:31:56 -0400, Shawn O. Pearce wrote: > >> 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? >> >> 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 ```