[PATCH] git-stash: add flag to skip "git reset --hard"

[PATCH] git-stash: add flag to skip "git reset --hard"

[Tom Anderson]

Add the "--no-reset" (same as "-r") flag to git-stash.sh.  A common
workflow is to use "git-stash save" and "git-stash apply" to save your
work without modifying your changed files.  This forces users to
reload their changed files in text editors like emacs, and touches the
modify time on all changed files, potentially making incremental
builds slower if many files in the build depend on your changed files.

Add documentation in Documentation/git-stash.txt and tests in
t/t3903-stash.sh.

Signed-off-by: Thomas Anderson <thomasanderson@google.com>
---
  Documentation/git-stash.txt |  9 ++++++---
  git-stash.sh                | 15 ++++++++++-----
  t/t3903-stash.sh            | 27 +++++++++++++++++++++++++++
  3 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 92df596..ba5ecf2 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,8 +13,8 @@ SYNOPSIS
  'git stash' drop [-q|--quiet] [<stash>]
  'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
  'git stash' branch <branchname> [<stash>]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-	     [-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' [save [-p|--patch] [-r|--no-reset] [-k|--[no-]keep-index]
+	     [-q|--quiet] [-u|--include-untracked] [-a|--all] [<message>]]
  'git stash' clear
  'git stash' create [<message>]
  'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -44,7 +44,7 @@ is also possible).
  OPTIONS
  -------
  -save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] 
[-a|--all] [-q|--quiet] [<message>]::
+save [-p|--patch] [-r|--no-reset] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
   	Save your local modifications to a new 'stash', and run `git reset
  	--hard` to revert them.  The <message> part is optional and gives
@@ -61,6 +61,9 @@ stashed and then cleaned up with `git clean`, leaving 
the working directory
  in a very clean state. If the `--all` option is used instead then the
  ignored files are stashed and cleaned in addition to the untracked files.
  +
+If the `--no-reset` option is used, `git reset --hard` is skipped and the
+`--[no-]keep-index`, `--include-untracked`, and `--all` flags are ignored.
++
  With `--patch`, you can interactively select hunks from the diff
  between HEAD and the working tree to be stashed.  The stash entry is
  constructed such that its index state is the same as the index state
diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..aebebb0 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,8 +7,9 @@ USAGE="list [<options>]
     or: $dashless drop [-q|--quiet] [<stash>]
     or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
     or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-		       [-u|--include-untracked] [-a|--all] [<message>]]
+   or: $dashless [save [--patch] [-r|--no-reset] [-k|--[no-]keep-index]
+		       [-q|--quiet] [-u|--include-untracked] [-a|--all]
+		       [<message>]]
     or: $dashless clear"
   SUBDIRECTORY_OK=Yes
@@ -194,9 +195,13 @@ save_stash () {
  	keep_index=
  	patch_mode=
  	untracked=
+	no_reset=
  	while test $# != 0
  	do
  		case "$1" in
+		-r|--no-reset)
+			no_reset=t
+			;;
  		-k|--keep-index)
  			keep_index=t
  			;;
@@ -268,7 +273,7 @@ save_stash () {
  	die "$(gettext "Cannot save the current status")"
  	say Saved working directory and index state "$stash_msg"
  -	if test -z "$patch_mode"
+	if test -z "$patch_mode" -a -z "$no_reset"
  	then
  		git reset --hard ${GIT_QUIET:+-q}
  		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
@@ -276,12 +281,12 @@ save_stash () {
  		then
  			git clean --force --quiet -d $CLEAN_X_OPTION
  		fi
-
  		if test "$keep_index" = "t" && test -n $i_tree
  		then
  			git read-tree --reset -u $i_tree
  		fi
-	else
+	elif -n "$patch_mode"
+	then
  		git apply -R < "$TMP-patch" ||
  		die "$(gettext "Cannot remove worktree changes")"
  diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..3eba19a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -248,6 +248,33 @@ test_expect_success 'stash --no-keep-index' '
  	test bar,bar2 = $(cat file),$(cat file2)
  '
  +test_expect_success 'stash -r' '
+	git reset &&
+	echo foo420 > file &&
+	echo bar420 > file2 &&
+	git add file2 &&
+	git stash -r &&
+	test foo420,bar420 = $(cat file),$(cat file2)
+'
+
+test_expect_success 'stash --no-reset' '
+	git reset &&
+	echo bar33 > file &&
+	echo bar44 > file2 &&
+	git add file2 &&
+	git stash --no-reset &&
+	test bar33,bar44 = $(cat file),$(cat file2)
+'
+
+test_expect_success 'stash -r with ignored options -k -u -a' '
+	git reset &&
+	echo foo55 > file &&
+	echo bar66 > file2 &&
+	git add file2 &&
+	git stash -r &&
+	test foo55,bar66 = $(cat file),$(cat file2)
+'
+
  test_expect_success 'stash --invalid-option' '
  	echo bar5 > file &&
  	echo bar6 > file2 &&
-- 
2.8.0.rc2

Re: [PATCH] git-stash: add flag to skip "git reset --hard"

[Junio C Hamano]

Tom Anderson <thomasanderson@google.com> writes:

> @@ -61,6 +61,9 @@ stashed and then cleaned up with `git clean`,
> leaving the working directory
>  in a very clean state. If the `--all` option is used instead then the
>  ignored files are stashed and cleaned in addition to the untracked files.
>  +
> +If the `--no-reset` option is used, `git reset --hard` is skipped and the
> +`--[no-]keep-index`, `--include-untracked`, and `--all` flags are ignored.
> ++

I am afraid that a reader who does not read git-stash.sh script
would not know what you are talking about.  They do not know (or
particularly care) where "git reset --hard", how often and for what
purpose.  They can tell that this option affects only "save",
because that is where it is described, but they would not know what
it means to "skip reset --hard", other than that they cannot use the
three features listed there.

It is unclear what problem you are trying to solve from this text,
and the log message's mention of mtime and rebuilding makes it
sound like an X-Y problem.

It could very well be that what you are trying to implement makes
perfect sense and the new option is named with a stress on a wrong
aspect (i.e. named after what it uses to achieve things, rather than
saying what you are trying to achieve).

The workhorse used in "git stash save" (which is what you are
touching) is "git stash create", and that is only responsible for
recording a new stash entry without touch the working tree.  Is that
what you are after, perhaps?

Re: [PATCH] git-stash: add flag to skip "git reset --hard"

[Tom Anderson]

On 05/02/2016 11:42 AM, Junio C Hamano wrote:
> Tom Anderson <thomasanderson@google.com> writes:
>
>> @@ -61,6 +61,9 @@ stashed and then cleaned up with `git clean`,
>> leaving the working directory
>>   in a very clean state. If the `--all` option is used instead then the
>>   ignored files are stashed and cleaned in addition to the untracked files.
>>   +
>> +If the `--no-reset` option is used, `git reset --hard` is skipped and the
>> +`--[no-]keep-index`, `--include-untracked`, and `--all` flags are ignored.
>> ++
> I am afraid that a reader who does not read git-stash.sh script
> would not know what you are talking about.  They do not know (or
> particularly care) where "git reset --hard", how often and for what
> purpose.  They can tell that this option affects only "save",
> because that is where it is described, but they would not know what
> it means to "skip reset --hard", other than that they cannot use the
> three features listed there.
The man page for git-stash unfortunately already makes a reference to
git reset --hard:

        save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked]
        [-a|--all] [-q|--quiet] [<message>]
            Save your local modifications to a new stash, and run git reset
            --hard to revert them
>
> It is unclear what problem you are trying to solve from this text,
> and the log message's mention of mtime and rebuilding makes it
> sound like an X-Y problem.
>
> It could very well be that what you are trying to implement makes
> perfect sense and the new option is named with a stress on a wrong
> aspect (i.e. named after what it uses to achieve things, rather than
> saying what you are trying to achieve).
Perhaps it would be better to rename --no-reset to something more
appropriate?
> The workhorse used in "git stash save" (which is what you are
> touching) is "git stash create", and that is only responsible for
> recording a new stash entry without touch the working tree.  Is that
> what you are after, perhaps?
>
Yes, but I like my stashes to be saved in the ref namespace. Maybe an
alternative if it's a bad idea to add this new flag to "git stash save" is
to add a new flag to "git stash create"?  Or make a new git-stash command
entirely?

Re: [PATCH] git-stash: add flag to skip "git reset --hard"

[Junio C Hamano]

Tom Anderson <thomasanderson@google.com> writes:

> Yes, but I like my stashes to be saved in the ref namespace.

Isn't that something you can do so yourself with store_stash?

Re: [PATCH] git-stash: add flag to skip "git reset --hard"

[Tom Anderson]

What I want can be achieved using "git stash store `git stash create`"

On 05/02/2016 12:58 PM, Junio C Hamano wrote:
> Tom Anderson <thomasanderson@google.com> writes:
>
>> Yes, but I like my stashes to be saved in the ref namespace.
> Isn't that something you can do so yourself with store_stash?

Re: [PATCH] git-stash: add flag to skip "git reset --hard"

[Junio C Hamano]

Tom Anderson <thomasanderson@google.com> writes:

> On 05/02/2016 12:58 PM, Junio C Hamano wrote:
>> Tom Anderson <thomasanderson@google.com> writes:
>>
>>> Yes, but I like my stashes to be saved in the ref namespace.
>> Isn't that something you can do so yourself with store_stash?
>
> What I want can be achieved using "git stash store `git stash create`"

OK.

I am not strongly opposed to either a new option to create
(i.e. "create --store" stores a new stash entry) or a new
subcommand.

It just felt that a new option to "save" that deliberately breaks
the basic premise of the command, i.e. "my working tree is a mess,
and I want to revert it to the pristine state quickly to work on
something else that is urgent, but I want to store the mess away
instead of discarding, so that I can come back to it later", was
totally out of place.  I.e.

	work work work to create a mess
	git stash save
        work on a quick and urgent stuff, knowing that you are
        building on a solidly committed state
        git commit
        git stash pop
        continue building on the mess

is what "save" is about.

As your workflow is quite different (without "reset --hard", you no
longer can rely on the resulting state to be pristine, suitable to
work on something totally unrelated), it would have needed way way
more explanation in the description section to describe this quite
different mode of operation, as it is quite incompatible to the way
traditional "stash save" users would want to use the command for if
you do not revert the working tree to the pristine state.  Instead
you would do something else (which was not described--if the answer
is "keep working, leading to the creation of the next commit", it is
unclear what the resulting stash entry would be used for, as it
obviously won't apply to that state, as the stashed change is
already contained in that next commit).

"git stash save --keep" is bad enough already; it does not give the
user pristine state wrt the current HEAD--what it does is a pristine
state wrt the next HEAD that _would_ have resulted if the stuff
you've been working on and decided are good by doing "git add" is
all committed.  In hindsight, it may have deserved a separate
command to make it easier to explain to the end users.

Let's not make it worse.

This is a tangent, but I am not sure where your aversion to "reset
--hard" comes from.  If something was changed, mtime changes, and
otherwise your build would break.  It's not like "reset --hard"
touches mtime for every path in the working tree including unchanged
ones.