git stash: status from current dir not top dir?

git stash: status from current dir not top dir?

[Piotr Krukowiecki]

Hi,

git status shows paths relative to current directory, so it's
possible to copy&paste them directly, even if you're in a subdirectory. 

But "git stash apply" shows status from root of git repository.
This is misleading because you can't copy and paste the paths.

I wonder if it's possible/better to change it so "git stash apply" shows
status relative to directory from which it was executed?

OTOH "git commit" also shows paths relative to root. So maybe git status
should be changed (hopefully no, because the relative paths are quite
useful IMO).

This patch tries to fix git-stash.sh to show status relative to 
current directory. I can resend the patch with better commit message.

Example:

$ mkdir d && cd d && echo a > ../topfile && echo b > subfile && git add ..
$ git commit -m 1
[master (root-commit) 6935958] 1
 2 files changed, 2 insertions(+), 0 deletions(-)
 create mode 100644 d/subfile
 create mode 100644 topfile
$ echo x > ../topfile ; echo y > subfile 
$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   subfile
#	modified:   ../topfile
#
no changes added to commit (use "git add" and/or "git commit -a")
$ git stash
Saved working directory and index state WIP on master: 6935958 1
HEAD is now at 6935958 1
$ git stash pop
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   d/subfile
#	modified:   topfile
#
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (3c7c82dba01feed37c725a795116354f6e229d76)
$ git checkout -- topfile
error: pathspec 'd/topfile' did not match any file(s) known to git.


With the patch:

$ git stash pop
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   subfile
#	modified:   ../topfile
#
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (4a0f7bb0a3e2902ba9046686d457b1b8f1ade04c)

---8<---
From: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Date: Fri, 11 Mar 2011 20:50:49 +0100
Subject: [PATCH] git stash: show status relative to currect directory


Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 git-stash.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7561b37..586c12f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -12,6 +12,7 @@ USAGE="list [<options>]
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
+START_DIR=`pwd`
 . git-sh-setup
 require_work_tree
 cd_to_toplevel
@@ -394,7 +395,7 @@ apply_stash () {
 		then
 			squelch='>/dev/null 2>&1'
 		fi
-		eval "git status $squelch" || :
+		(cd "$START_DIR" && eval "git status $squelch") || :
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
-- 
1.7.1

Re: git stash: status from current dir not top dir?

[Jeff King]

On Fri, Mar 11, 2011 at 09:49:13PM +0100, Piotr Krukowiecki wrote:

> But "git stash apply" shows status from root of git repository.
> This is misleading because you can't copy and paste the paths.

Yeah, I am inclined to call it a bug. git-status will show the status of
the whole tree from wherever you are, and people who want full paths
will have status.relativePaths turned off, anyway. So I think your
proposed semantics are more natural.

> This patch tries to fix git-stash.sh to show status relative to 
> current directory. I can resend the patch with better commit message.

Yes, please. There is lots of nice discussion in your email but none of
it in the commit message. :)

> diff --git a/git-stash.sh b/git-stash.sh
> index 7561b37..586c12f 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -12,6 +12,7 @@ USAGE="list [<options>]
>  
>  SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
> +START_DIR=`pwd`
>  . git-sh-setup
>  require_work_tree
>  cd_to_toplevel
> @@ -394,7 +395,7 @@ apply_stash () {
>  		then
>  			squelch='>/dev/null 2>&1'
>  		fi
> -		eval "git status $squelch" || :
> +		(cd "$START_DIR" && eval "git status $squelch") || :
>  	else
>  		# Merge conflict; keep the exit status from merge-recursive
>  		status=$?

This fix looks reasonable to me. The other option would be to avoid
cd_to_toplevel at the beginning (which I am not sure why we really need
in the first place, but presumably some code paths rely on it), but it's
probably not worth the risk of introducing new confusing bugs.

-Peff

Re: git stash: status from current dir not top dir?

[Piotr Krukowiecki]

Thanks for review,
attaching patch with updated commit message.

W dniu 11.03.2011 23:32, Jeff King pisze:
> On Fri, Mar 11, 2011 at 09:49:13PM +0100, Piotr Krukowiecki wrote:
> 
>> But "git stash apply" shows status from root of git repository.
>> This is misleading because you can't copy and paste the paths.
> 
> Yeah, I am inclined to call it a bug. git-status will show the status of
> the whole tree from wherever you are, and people who want full paths
> will have status.relativePaths turned off, anyway. So I think your
> proposed semantics are more natural.

I think it's better this way too, one thing that made me doubt if this is 
intended or not is fact that git-commit also shows paths relative to root
dir...
 
>> This patch tries to fix git-stash.sh to show status relative to 
>> current directory. I can resend the patch with better commit message.
> 
> Yes, please. There is lots of nice discussion in your email but none of
> it in the commit message. :)

Didn't want to duplicate ;)

> 
>> diff --git a/git-stash.sh b/git-stash.sh
>> index 7561b37..586c12f 100755
>> --- a/git-stash.sh
>> +++ b/git-stash.sh
>> @@ -12,6 +12,7 @@ USAGE="list [<options>]
>>  
>>  SUBDIRECTORY_OK=Yes
>>  OPTIONS_SPEC=
>> +START_DIR=`pwd`
>>  . git-sh-setup
>>  require_work_tree
>>  cd_to_toplevel
>> @@ -394,7 +395,7 @@ apply_stash () {
>>  		then
>>  			squelch='>/dev/null 2>&1'
>>  		fi
>> -		eval "git status $squelch" || :
>> +		(cd "$START_DIR" && eval "git status $squelch") || :
>>  	else
>>  		# Merge conflict; keep the exit status from merge-recursive
>>  		status=$?
> 
> This fix looks reasonable to me. The other option would be to avoid
> cd_to_toplevel at the beginning (which I am not sure why we really need
> in the first place, but presumably some code paths rely on it), but it's
> probably not worth the risk of introducing new confusing bugs.
> 
> -Peff
---8<---
From: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Date: Fri, 11 Mar 2011 20:50:49 +0100
Subject: [PATCH] git stash: show status relative to current directory

git status shows modified paths relative to current directory, so it's
possible to copy&paste them directly, even if you're in a subdirectory.

But "git stash apply" always shows status from root of git repository.
This is misleading because you can't use the paths without modifications.

This is caused by changing directory to root of repository at the
beginning of git stash.

This patch makes git stash show status relative to current directory.
Instead of removing the "cd to toplevel", which would affect whole
script and might have other side-effects, the fix is to change directory
temporarily back to original dir just before displaying status.

Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 git-stash.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 7561b37..b59c201 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -12,6 +12,7 @@ USAGE="list [<options>]
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
+START_DIR=`pwd`
 . git-sh-setup
 require_work_tree
 cd_to_toplevel
@@ -394,7 +395,7 @@ apply_stash () {
 		then
 			squelch='>/dev/null 2>&1'
 		fi
-		eval "git status $squelch" || :
+		(cd "$START_DIR" && eval "git status $squelch") || :
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
-- 
1.7.4.1.228.g9e388

Re: git stash: status from current dir not top dir?

[Junio C Hamano]

Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:

> Subject: [PATCH] git stash: show status relative to current directory
>
> git status shows modified paths relative to current directory, so it's
> possible to copy&paste them directly, even if you're in a subdirectory.
>
> But "git stash apply" always shows status from root of git repository.
> This is misleading because you can't use the paths without modifications.
>
> This is caused by changing directory to root of repository at the
> beginning of git stash.
>
> This patch makes git stash show status relative to current directory.
> Instead of removing the "cd to toplevel", which would affect whole
> script and might have other side-effects, the fix is to change directory
> temporarily back to original dir just before displaying status.
>
> Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
> ---

Sensible. Thanks.

Don't we want to protect this output with some tests?

Re: git stash: status from current dir not top dir?

[Piotr Krukowiecki]

W dniu 14.03.2011 08:29, Junio C Hamano pisze:
> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
> 
>> Subject: [PATCH] git stash: show status relative to current directory
>>
>> git status shows modified paths relative to current directory, so it's
>> possible to copy&paste them directly, even if you're in a subdirectory.
>>
>> But "git stash apply" always shows status from root of git repository.
>> This is misleading because you can't use the paths without modifications.
>>
>> This is caused by changing directory to root of repository at the
>> beginning of git stash.
>>
>> This patch makes git stash show status relative to current directory.
>> Instead of removing the "cd to toplevel", which would affect whole
>> script and might have other side-effects, the fix is to change directory
>> temporarily back to original dir just before displaying status.
>>
>> Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
>> ---
> 
> Sensible. Thanks.
> 
> Don't we want to protect this output with some tests?

Right. Wrote a test but it fails mysteriously. Looks like a debug output
is added when test is run as "sh t3903-stash.sh" (the "Merging Version" etc).
No such output when "git apply" is run by hand.

Not sure what to do with it?

With --verbose I see:

[...]
[master b27a2bc] subdir
 Author: A U Thor <author@example.com>
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 subdir/subfile1
Saved working directory and index state WIP on master: b27a2bc subdir
HEAD is now at b27a2bc subdir
--- ../output	2011-03-14 19:39:42.473685001 +0000
+++ ../expect	2011-03-14 19:39:42.489685001 +0000
@@ -1,9 +1,3 @@
-Merging Version stash was based on with Stashed changes
-Merging:
-virtual Version stash was based on
-virtual Stashed changes
-found 1 common ancestor(s):
-virtual 13419d0b4f5b097f61dde4c911de99a154f8286f
 # On branch master
 # Changes not staged for commit:
 #   (use "git add <file>..." to update what will be committed)
not ok - 41 stash apply shows status same as git status (relative to current directory)


---8<---
From: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Date: Mon, 14 Mar 2011 20:19:36 +0100
Subject: [PATCH] Add test: git stash shows status relative to current dir


Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 t/t3903-stash.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 6fd560c..3682f1c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -556,4 +556,19 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
 	git rev-parse stash@{0} --
 '
 
+test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
+	git stash clear &&
+	echo 1 > subdir/subfile1 &&
+	echo 2 > subdir/subfile2 &&
+	git add subdir/subfile1 &&
+	git commit -m subdir &&
+	cd subdir &&
+	echo x > subfile1 &&
+	echo x > ../file &&
+	git stash &&
+	git stash apply > ../output &&
+	git status > ../expect &&
+	test_cmp ../output ../expect
+'
+
 test_done
-- 
1.7.4.1.228.g9e388



-- 
Piotr Krukowiecki

Re: git stash: status from current dir not top dir?

[Piotr Krukowiecki]

W dniu 14.03.2011 20:45, Piotr Krukowiecki pisze:
> Right. Wrote a test but it fails mysteriously. Looks like a debug output
> is added when test is run as "sh t3903-stash.sh" (the "Merging Version" etc).
> No such output when "git apply" is run by hand.

To recap the problem was that "git stash apply" run during tests printed some
debug messages as shown below. In result I could not compare its output with
expected "git status" output.


> Not sure what to do with it?

I found out it was caused by 
    GIT_MERGE_VERBOSITY=5
in test-lib.sh

It was introduced in 8d0fc48f2730 with comment:
    Its really nice to be able to run a test with -v and automatically
    see the "debugging" dump from merge-recursive, especially if we
    are actually trying to debug merge-recursive.

Now I don't know how should I handle this:

    1. unset it just before "git stash apply" in my test
       A safe, local change

    2. remove it from test-lib.sh
       The variable changed git behaviour - might impact tests, should it be
       set by default?

    3. add new option in test-lib.sh to set it (--merge-verbosity?)
       Also looks safe, but still some tests would fail with it
       (which would be mentioned in the option documentation)

    4. change test-lib.sh to set it only when --verbose/--debug is passed
       This seems to be the intention, but test results would be different
       with those options (some tests would fail)!

First three choices look more or less sensible.


> With --verbose I see:
> 
> [...]
> [master b27a2bc] subdir
>  Author: A U Thor <author@example.com>
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 subdir/subfile1
> Saved working directory and index state WIP on master: b27a2bc subdir
> HEAD is now at b27a2bc subdir
> --- ../output	2011-03-14 19:39:42.473685001 +0000
> +++ ../expect	2011-03-14 19:39:42.489685001 +0000
> @@ -1,9 +1,3 @@
> -Merging Version stash was based on with Stashed changes
> -Merging:
> -virtual Version stash was based on
> -virtual Stashed changes
> -found 1 common ancestor(s):
> -virtual 13419d0b4f5b097f61dde4c911de99a154f8286f
>  # On branch master
>  # Changes not staged for commit:
>  #   (use "git add <file>..." to update what will be committed)
> not ok - 41 stash apply shows status same as git status (relative to current directory)
> 
> 
> ---8<---
> From: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
> Date: Mon, 14 Mar 2011 20:19:36 +0100
> Subject: [PATCH] Add test: git stash shows status relative to current dir
> 
> 
> Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
> ---
>  t/t3903-stash.sh |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 6fd560c..3682f1c 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -556,4 +556,19 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
>  	git rev-parse stash@{0} --
>  '
>  
> +test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
> +	git stash clear &&
> +	echo 1 > subdir/subfile1 &&
> +	echo 2 > subdir/subfile2 &&
> +	git add subdir/subfile1 &&
> +	git commit -m subdir &&
> +	cd subdir &&
> +	echo x > subfile1 &&
> +	echo x > ../file &&
> +	git stash &&
> +	git stash apply > ../output &&
> +	git status > ../expect &&
> +	test_cmp ../output ../expect
> +'
> +
>  test_done


-- 
Piotr Krukowiecki

Re: git stash: status from current dir not top dir?

[Junio C Hamano]

Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:

> Now I don't know how should I handle this:
>
>     1. unset it just before "git stash apply" in my test
>        A safe, local change

This is the preferred method; in addition to "a safe local", more
importantly, at that point you are testing what you _want_ to be testing,
namely, how the output appears to the _real_ end users who do not use
verbose message. So for that purpose, mucking locally with MERGE_VERBOSITY
is perfectly acceptable.

You would not just "unset it just before", but "unset around it" in a
subshell like this:

	git stash &&
	(
		sane_unset GIT_MERGE_VERBOSITY &&
	        git stash apply
	) >../actual &&
	git status >../expect &&
        test_cmp ../expect ../actual

so that if somebody adds new tests later in the script, they are not
affected by this change.

Write your test_cmp always to compare expected with actual, not the other
way around, so that the diff output you see when the test is run under -v
option shows the changes from what is expected.

Thanks.

Re: git stash: status from current dir not top dir?

[Piotr Krukowiecki]

W dniu 17.03.2011 20:30, Junio C Hamano pisze:
> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
> 
>> Now I don't know how should I handle this:
>>
>>     1. unset it just before "git stash apply" in my test
>>        A safe, local change
> 
> This is the preferred method; in addition to "a safe local", more
> importantly, at that point you are testing what you _want_ to be testing,
> namely, how the output appears to the _real_ end users who do not use
> verbose message. So for that purpose, mucking locally with MERGE_VERBOSITY
> is perfectly acceptable.
> 
> You would not just "unset it just before", but "unset around it" in a
> subshell like this:
> 
> 	git stash &&
> 	(
> 		sane_unset GIT_MERGE_VERBOSITY &&
> 	        git stash apply
> 	) >../actual &&
> 	git status >../expect &&
>         test_cmp ../expect ../actual
> 
> so that if somebody adds new tests later in the script, they are not
> affected by this change.
> 
> Write your test_cmp always to compare expected with actual, not the other
> way around, so that the diff output you see when the test is run under -v
> option shows the changes from what is expected.
> 
> Thanks.

Thanks, updated according to your suggestions.

I've also added a check to see if the output contains a relative path
(so we really test not only that git-stash shows the same status
as git-status, but that the paths are relative).
    
I'm not resending the original patch for git-stash.sh - I don't know if
it's expected to always send full set of patches?


---8<---
From: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Date: Mon, 14 Mar 2011 20:19:36 +0100
Subject: [PATCH 2/2] Add test: git stash shows status relative to current dir


Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 t/t3903-stash.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 6fd560c..13f9ae8 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -556,4 +556,23 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
 	git rev-parse stash@{0} --
 '
 
+test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
+	git stash clear &&
+	echo 1 > subdir/subfile1 &&
+	echo 2 > subdir/subfile2 &&
+	git add subdir/subfile1 &&
+	git commit -m subdir &&
+	cd subdir &&
+	echo x > subfile1 &&
+	echo x > ../file &&
+	git stash &&
+	(
+		sane_unset GIT_MERGE_VERBOSITY &&
+		git stash apply
+	) > ../actual &&
+	git status > ../expect &&
+	test_cmp ../expect ../actual &&
+	grep "[.][.]/actual" ../actual 
+'
+
 test_done
-- 
1.7.4.1.296.gca6da


-- 
Piotr Krukowiecki