[PATCH] stash: --keep option just saves

[PATCH] stash: --keep option just saves

[Nanako Shiraishi]

The "save" subcommand usually removes the changes it stashed from the
index and the work tree. Existing --keep-index option however keeps the
changes to the index. This new option keeps the changes made to both the
index and the work tree.

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
 Documentation/git-stash.txt |    7 +++++--
 git-stash.sh                |   35 +++++++++++++++++++++++++++++------
 t/t3903-stash.sh            |   22 ++++++++++++++++++++++
 3 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 051f94d..ddd68ef 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git stash' (show | drop | pop ) [<stash>]
 'git stash' apply [--index] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--keep-index] [<message>]]
+'git stash' [save [--keep | --keep-index] [<message>]]
 'git stash' clear
 'git stash' create
 
@@ -41,13 +41,16 @@ is also possible).
 OPTIONS
 -------
 
-save [--keep-index] [<message>]::
+save [--keep | --keep-index] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  This is the default action when no
 	subcommand is given. The <message> part is optional and gives
 	the description along with the stashed state.
 +
+If the `--keep` option is used, `git reset --hard` is omitted and your
+changes to the index and the work tree will be kept.
++
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
 
diff --git a/git-stash.sh b/git-stash.sh
index b9ace99..0d5efaa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -6,7 +6,7 @@ USAGE="list [<options>]
    or: $dashless (show | drop | pop ) [<stash>]
    or: $dashless apply [--index] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--keep-index] [<message>]]
+   or: $dashless [save [--keep | --keep-index] [<message>]]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -93,12 +93,31 @@ create_stash () {
 }
 
 save_stash () {
+	keep=
 	keep_index=
-	case "$1" in
-	--keep-index)
-		keep_index=t
+	while test $# != 0
+	do
+		case "$1" in
+		--keep-index)
+			keep_index=t
+			;;
+		--keep)
+			keep=t
+			;;
+		--)
+			shift
+			break
+			;;
+		*)
+			break
+			;;
+		esac
 		shift
-	esac
+	done
+	if test "$keep$keep_index" = tt
+	then
+		die "Cannot use --keep and --keep-index at the same time"
+	fi
 
 	stash_msg="$*"
 
@@ -120,8 +139,12 @@ save_stash () {
 		die "Cannot save the current status"
 	printf 'Saved working directory and index state "%s"\n' "$stash_msg"
 
-	git reset --hard
+	if test -n "$keep"
+	then
+		return
+	fi
 
+	git reset --hard
 	if test -n "$keep_index" && test -n $i_tree
 	then
 		git read-tree --reset -u $i_tree
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7484cbe..90f0902 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -177,4 +177,26 @@ test_expect_success 'stash branch' '
 	test 0 = $(git stash list | wc -l)
 '
 
+test_expect_success 'stash --keep-index' '
+	git stash clear &&
+	echo modified > file &&
+	git add file &&
+	echo changed > file &&
+	git stash save --keep-index test &&
+	git diff --exit-code &&
+	test modified = "$(cat file)" &&
+	git diff stash^ stash | grep "^+changed"
+'
+
+test_expect_success 'stash --keep' '
+	git stash clear &&
+	echo modified > file &&
+	git add file &&
+	echo changed > file &&
+	git stash save --keep test &&
+	test changed = "$(cat file)" &&
+	git diff --exit-code stash &&
+	test modified = "$(git show :file)"
+'
+
 test_done
-- 
1.6.2.rc0

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] stash: --keep option just saves

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> The "save" subcommand usually removes the changes it stashed from the
> index and the work tree. Existing --keep-index option however keeps the
> changes to the index. This new option keeps the changes made to both the
> index and the work tree.

I saw --no-reset mentioned earlier but probably this is a more consistent
name for the feature.

But I somewhat doubt if this line of change is a good idea to begin with.

The earlier --keep-index feature had a clear definition of what workflow
benefits from it, and also made it clear that the workflow it helped was a
good workflow.  You build what you would consider a good change in the
index bit by bit, but you would want a final test of the whole tree,
without the changes that you are still not sure and are not in the index.
Before --keep-index, we did not have a good way to do so.

This one, the "snapshot", and various other related topics, are quite
different.  The workflow the --keep (and for that matter, "snapshot")
would support I can think of does not sound a very good one we would want
to recommend (--untracked is a different issue; I haven't formed an
opinion).

You build on a branch, but you are forever in the state of indecision, and
instead of committing, you keep saying "save --keep" number of times to
leave a checkpoint on your stash.  After number of iterations, you may
have many stashes in "git stash list", but what you can do with them is
"git reset --hard && git stash apply stash@{$n}" to go back to any of the
state, but that is about it.

If the topic you are working on is that involved, and if you are afraid of
contaminating the branch you started off of (which is groundless given the
nature of git that is distributed --- the act of committing is explicitly
disconnected from the act of publishing), then you are much better off
forking the original branch to another branch on which you do your own
work, and make normal commits to checkpoint your states.  That way, you
can use the usual history rewriting tools such as "rebase --interactive"
and "merge --squash" to finish it off once you reached a good state.

All talks about using stash --no-rest and snapshot share this problem.  By
not making regular commits, you are denying yourself the rich set of tools
git offers you to use on regular commits and the ancestry chains between
them, and nobody explained what the benefits of not using normal commits
on normal branches are, nor how the inconveniences from this aversion to
branches forces you are justified by those unexplained benefits.

This topic won't go beyond 'next' during this round because it started way
after -rc0 was tagged.  It is not urgent to decide what will happen to the
recent "snapshot" related topics, and we have plenty of time to toss ideas
around, but currently I have to say that I am not very enthused about any
of the causes mentioned in various discussion threads.

Re: [PATCH] stash: --keep option just saves

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> This one, the "snapshot", and various other related topics, are quite
> different.  The workflow the --keep (and for that matter, "snapshot")
> would support I can think of does not sound a very good one we would want
> to recommend (--untracked is a different issue; I haven't formed an
> opinion).
>
> You build on a branch, but you are forever in the state of indecision, and
> instead of committing, you keep saying "save --keep" number of times to
> leave a checkpoint on your stash.  After number of iterations, you may
> have many stashes in "git stash list", but what you can do with them is
> "git reset --hard && git stash apply stash@{$n}" to go back to any of the
> state, but that is about it.

Yeah, but that's a pretty useful thing.

I often save checkpoints of my working state before starting a
tentative/intrusive series of edits -- if they are clearly just part
of a larger logical change, I may not want to make separate commits
[or perhaps more commonly, I'm not entirely sure what the final commit
will be like, and am still "exploring"].

Of course this can also be done in git by doing temporary commits
(to be changed later with --amend, or rolled back before making the
real commit), or whatever, but I think pretty much every usage of
git-stash can be done fairly easily via some other means in git;
git-stash is really just a convenience.

-miles

-- 
Politeness, n. The most acceptable hypocrisy.

Re: [PATCH] stash: --keep option just saves

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com> writes:

> But I somewhat doubt if this line of change is a good idea to begin with.
>
> All talks about using stash --no-rest and snapshot share this problem.  By
> not making regular commits, you are denying yourself the rich set of tools
> git offers you to use on regular commits and the ancestry chains between
> them, and nobody explained what the benefits of not using normal commits
> on normal branches are, nor how the inconveniences from this aversion to
> branches forces you are justified by those unexplained benefits.

I'm sorry. I didn't think about such a negative aspect of adding the feature.

I can imagine that, after using many 'stash save --keep', I'll naturally want to manipulate changes between kept stashes, like running 'git log' to view the difference between each step and cherry-picking a change between ones next to each other etc. Because a list of stashes doesn't support such operations, additional support for them is needed to make it useful, but I agree with you that such additions are not real features that are necessary, because git already supports these operations if I used commits on a normal branch instead. Use of 'stash save --keep' is making it necessary to implement duplicated features for no good reason.

It may also confuse users by unnecessarily adding another way to do the same thing. So I think you're right to point out that this is not a good thing to add.

> This topic won't go beyond 'next' during this round because it started way
> after -rc0 was tagged.  It is not urgent to decide what will happen to the
> recent "snapshot" related topics, and we have plenty of time to toss ideas
> around, but currently I have to say that I am not very enthused about any
> of the causes mentioned in various discussion threads.

You already applied my patch on pu branch. It's OK by me if you dropped it.

Thank you for your comments.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] stash: --keep option just saves

[Nanako Shiraishi]

Quoting Miles Bader <miles@gnu.org>:

> Junio C Hamano <gitster@pobox.com> writes:
>> This one, the "snapshot", and various other related topics, are quite
>> different.  The workflow the --keep (and for that matter, "snapshot")
>> would support I can think of does not sound a very good one we would want
>> to recommend (--untracked is a different issue; I haven't formed an
>> opinion).
>>
>> You build on a branch, but you are forever in the state of indecision, and
>> instead of committing, you keep saying "save --keep" number of times to
>> leave a checkpoint on your stash.  After number of iterations, you may
>> have many stashes in "git stash list", but what you can do with them is
>> "git reset --hard && git stash apply stash@{$n}" to go back to any of the
>> state, but that is about it.
>
> Yeah, but that's a pretty useful thing.
>
> I often save checkpoints of my working state before starting a
> tentative/intrusive series of edits -- if they are clearly just part
> of a larger logical change, I may not want to make separate commits
> [or perhaps more commonly, I'm not entirely sure what the final commit
> will be like, and am still "exploring"].

I suspect you either didn't read or didn't understand the rest of Junio's message in which he explained why it isn't a good use of git.

"I am still exploring" is exactly why git already offers cheap branches, distributed operation that makes committing and sharing separate, and rich set of commands to rewrite history, so that you don't have to worry about creating a sequence of commits that may not be ideal. "Just part of a larger logical change" is a sign that they belong to the same branch, and being able to rewrite your local history means the boundary between parts doesn't have to be perfect in your original set of commits.

If you created five "checkpoints" while working on such a "tentative/intrusive series of edits", you will have five commits on your tentative branch that shouldn't be separated in the way they are separated. Maybe they should be squashed to a single commit in the end, or maybe you want to split them into two commits. When you finished with them, you have a much better understanding of the issues to base your decision on how to split the changes, and that is when you use history rewriting tools to finalize what you have achieved.

An important thing to understand is that in a distributed system, commits don't have to be cast in iron when you make them, and that you don't have to be afraid of making "separate commits" only because they belong to one topic while you are exploring. You can combine them later, and you are in control when to declare they are finished and make them visible to others.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH] stash: --keep option just saves

[Miles Bader]

On Thu, Feb 12, 2009 at 5:17 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> I suspect you either didn't read or didn't understand the rest of Junio's message in which he explained why it isn't a good use of git.

See the final paragraph of my message.

-Miles

-- 
Do not taunt Happy Fun Ball.

Re: [PATCH] stash: --keep option just saves

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> I'm sorry. I didn't think about such a negative aspect of adding the
> feature.

No need to be sorry about anything, please.  Nobody has to apologise after
trying a constructive contribution to solve a perceived problem.

> I can imagine that, after using many 'stash save --keep', I'll naturally
> want to manipulate changes between kept stashes, like running 'git log'
> to view the difference between each step and cherry-picking a change
> between ones next to each other etc. Because a list of stashes doesn't
> support such operations, additional support for them is needed to make
> it useful, but I agree with you that such additions are not real
> features that are necessary, because git already supports these
> operations if I used commits on a normal branch instead. Use of 'stash
> save --keep' is making it necessary to implement duplicated features for
> no good reason.

That was exactly my point.  "save --keep" (or --no-reset) might look as if
it is a new convenience feature, but you will realize that it really is
not, once you think things through.  It is not new because there already
is a way meant to be used to snapshot before going forward (i.e. regular
commits on a private branch for experiment), and it is not convenient
because its results cannot be used as flexibly with existing tools as the
results by the other, existing approach.

It can be argued that it adds one huge convenience.  It is quicker to say
'git stash save --keep' than 'git commit -a -m "WIP as of $(date)"', and
in addition, you first have to do a "checkout -b wip" once, if you use the
approach with regular commits on a exprimental branch forked from the
target branch, instead of the approach with regular commits directly on
the target branch.

But if that convenience is the real motivation behind it, adding an option
"git commit --snap" that runs 'git commit -a -m "WIP as of $(date)"' would
be a far more superiour solution that does not have the problem of feature
duplication (and no, please do not suggest "git alias snap ..."; the point
is to make it easy to do this for *everybody* out of the box).

> It may also confuse users by unnecessarily adding another way to do the
> same thing.

I actually did not think of that issue, but you may be right.

>> This topic won't go beyond 'next' during this round because it started way
>> after -rc0 was tagged.  It is not urgent to decide what will happen to the
>> recent "snapshot" related topics, and we have plenty of time to toss ideas
>> around, but currently I have to say that I am not very enthused about any
>> of the causes mentioned in various discussion threads.
>
> You already applied my patch on pu branch. It's OK by me if you dropped it.

Rather, lets let a hundred flowers blossom and see what happens.  I will
promise it will not be followed by a purge ;-).

Re: [PATCH] stash: --keep option just saves

[Björn Steinbrink]

On 2009.02.12 13:04:48 -0800, Junio C Hamano wrote:
> It can be argued that it adds one huge convenience.  It is quicker to say
> 'git stash save --keep' than 'git commit -a -m "WIP as of $(date)"', and
> in addition, you first have to do a "checkout -b wip" once, if you use the
> approach with regular commits on a exprimental branch forked from the
> target branch, instead of the approach with regular commits directly on
> the target branch.

Another thing that came to my mind is that stash --keep would not affect
"git diff". I often look at the current diff to reassure myself of what
I'm actually doing, and when you do intermediate commits, you can no
longer use a plain "git diff", but need to use, for example, "git diff
master", assuming that your current branch is exclusively for this thing
you're working on. But the whole idea of taking snapshots feels more
like it is for a use case that assumes that you end up with a single
commit, that might in the end be a part of some bigger commit series. So
for the "git diff <whatever>" to be convenient, you might need a
(temporary) branch for just this one commit to be, or you need to tag
your starting point.

And (in part) you also lose the ability to dynamically mark parts of
your changes as "good" by staging and unstaging them. I use that quite
often to "trim" the diff output to the currently interesting part. When
I refactor some function, that might need a lot of callers to be
trivially changed, and I don't want to be bothered with those trivial
changes when I look at the diff to review my changes. So I temporarily
stage that trivial stuff, do a review of the "interesting" part, change
something here and there and might eventually unstage all the stuff
again. Often that's more comfortable than passing a list of interesting
files to each "git diff" command, and sometime that's the only way to
get things filtered down enough to make me happy, because the trivial
and the interesting changes are in the same file.

I think it could make sense to have that "snapshot" thing committing to
another branch (or some ref in /refs/snapshots/, or whatever). Say you
have topicA checked out, and work on some change "foo" that's likely
going to be a single commit, but you want to test several versions of
that thing, while always keeping your changes uncommitted as far as HEAD
is concerned. Then "git snapshot foo", could do something like:

export GIT_INDEX_FILE=.git/snap_index

parent_ref=$(git rev-parse --symbolic-full-name HEAD)
parent_ref=${parent_ref#refs/heads/}

snap_ref="refs/snapshots/$parent_ref/$1"

git read-tree HEAD
git add -u # Or maybe git add -A?

parent=$(git rev-parse --verify --quiet "$snap_ref" || echo HEAD)
commit=$(echo "Snapshot" | git commit-tree $(git write-tree) -p "$parent")

git update-ref "$snap_ref" "$commit"
rm "$GIT_INDEX_FILE"


So you get the benefits of having your snapshots stored much like
intermediate commits, but don't lose the "benefits" of having
uncommitted changes, like the plain "git diff" invocation instead of
"git diff <what's_my_base_today>".

Björn