threads / patch / 34148

v3, 5 partsstash: introduce 'git stash store'

Subject: [PATCH v3 4/5] stash: introduce 'git stash store'

## tl;dr

10 messages between Jun 15, 2013 and Jun 18, 2013. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · lore

[PATCH v3 0/5] Write a good 'stash store' for autostash

The interface actually makes sense in this iteration, thanks to Junio. Also fix minor nits pointed out by Phil Hord.

Thanks.
Ramkumar Ramachandra (5):
  stash doc: add a warning about using create
  stash doc: document short form -p in synopsis
  stash: simplify option parser for create
  stash: introduce 'git stash store'
  rebase: use 'git stash store' to simplify logic
 Documentation/git-stash.txt | 13 ++++++++++--
 git-rebase.sh               |  7 ++----
 git-stash.sh                | 52 ++++++++++++++++++++++++++++++++++++---------
 t/t3903-stash.sh            | 19 +++++++++++++++++
 4 files changed, 74 insertions(+), 17 deletions(-)
-- 
1.8.3.1.441.gd7d6b72.dirty
Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · re: Ramkumar Ramachandra · lore

[PATCH v3 1/5] stash doc: add a warning about using create

Add a note saying that the user probably wants "save" in the create description. While at it, document that it can optionally take a message in the synopsis.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-stash.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to Documentation/git-stash.txt +3 −1
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 711ffe1..cb0c1a6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' clear
-'git stash' create
+'git stash' create [<message>]
 
 DESCRIPTION
 -----------
@@ -151,6 +151,8 @@ create::
 
 	Create a stash (which is a regular commit object) and return its
 	object name, without storing it anywhere in the ref namespace.
+	This is intended to be useful for scripts.  It is probably not
+	the command you want to use; see "save" above.
 
 
 DISCUSSION
-- 
1.8.3.1.441.gd7d6b72.dirty
Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · re: Ramkumar Ramachandra · lore

[PATCH v3 2/5] stash doc: document short form -p in synopsis

'git stash save' can take -p, the short form of --patch, as an option. Document this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-stash.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/git-stash.txt +1 −1
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index cb0c1a6..632d4fb 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' clear
 'git stash' create [<message>]
-- 
1.8.3.1.441.gd7d6b72.dirty
Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · re: Ramkumar Ramachandra · lore

[PATCH v3 3/5] stash: simplify option parser for create

The option parser for create unnecessarily checks "$1" inside a case statement that matches "$1" in the first place.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-stash.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
Show changes to git-stash.sh +1 −4
diff --git a/git-stash.sh b/git-stash.sh
index bbefdf6..64800b8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -546,10 +546,7 @@ clear)
 	clear_stash "$@"
 	;;
 create)
-	if test $# -gt 0 && test "$1" = create
-	then
-		shift
-	fi
+	shift
 	create_stash "$*" && echo "$w_commit"
 	;;
 drop)
-- 
1.8.3.1.441.gd7d6b72.dirty
Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · re: Ramkumar Ramachandra · lore

save_stash() contains the logic for doing two potentially independent operations; the first is preparing the stash merge commit, and the second is updating the stash ref/ reflog accordingly. While the first operation is abstracted out into a create_stash() for callers to access via 'git stash create', the second one is not. Fix this by factoring out the logic for storing the stash into a store_stash() that callers can access via 'git stash store'.

Like create, store is not intended for end user interactive use, but for callers in other scripts. We can simplify the logic in the rebase.autostash feature using this new subcommand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-stash.txt |  7 +++++++
 git-stash.sh                | 47 +++++++++++++++++++++++++++++++++++++++------
 t/t3903-stash.sh            | 19 ++++++++++++++++++
 3 files changed, 67 insertions(+), 6 deletions(-)
Show changes to 3 files +67 −6

Documentation/git-stash.txt, git-stash.sh, t/t3903-stash.sh

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 632d4fb..db7e803 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' clear
 'git stash' create [<message>]
+'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
 -----------
@@ -154,6 +155,12 @@ create::
 	This is intended to be useful for scripts.  It is probably not
 	the command you want to use; see "save" above.
 
+store::
+
+	Store a given stash created via 'git stash create' (which is a
+	dangling merge commit) in the stash ref, updating the stash
+	reflog.  This is intended to be useful for scripts.  It is
+	probably not the command you want to use; see "save" above.
 
 DISCUSSION
 ----------
diff --git a/git-stash.sh b/git-stash.sh
index 64800b8..62e8cb6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -156,6 +156,41 @@ create_stash () {
 	die "$(gettext "Cannot record working tree state")"
 }
 
+store_stash () {
+	while test $# != 0
+	do
+		case "$1" in
+		-m|--message)
+			shift
+			stash_msg="$1"
+			;;
+		-q|--quiet)
+			quiet=t
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+	test $# == 1 ||
+	die "$(eval_gettext "\"$dashless store\" requires one <commit> argument")"
+
+	w_commit="$1"
+	if test -z "$stash_msg"
+	then
+		stash_msg="Created via \"git stash store\"."
+	fi
+
+	# Make sure the reflog for stash is kept.
+	: >>"$GIT_DIR/logs/$ref_stash"
+	git update-ref -m "$stash_msg" $ref_stash $w_commit
+	ret=$?
+	test $ret != 0 && test -z $quiet &&
+	die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
+	return $ret
+}
+
 save_stash () {
 	keep_index=
 	patch_mode=
@@ -227,12 +262,8 @@ save_stash () {
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
 	create_stash "$stash_msg" $untracked
-
-	# Make sure the reflog for stash is kept.
-	: >>"$GIT_DIR/logs/$ref_stash"
-
-	git update-ref -m "$stash_msg" $ref_stash $w_commit ||
-		die "$(gettext "Cannot save the current status")"
+	store_stash -m "$stash_msg" -q $w_commit ||
+	die "$(gettext "Cannot save the current status")"
 	say Saved working directory and index state "$stash_msg"
 
 	if test -z "$patch_mode"
@@ -549,6 +580,10 @@ create)
 	shift
 	create_stash "$*" && echo "$w_commit"
 	;;
+store)
+	shift
+	store_stash "$@"
+	;;
 drop)
 	shift
 	drop_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..75189ec 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -637,4 +637,23 @@ test_expect_success 'stash where working directory contains "HEAD" file' '
 	test_cmp output expect
 '
 
+test_expect_success 'store called with invalid commit' '
+	test_must_fail git stash store foo
+'
+
+test_expect_success 'store updates stash ref and reflog' '
+	git stash clear &&
+	git reset --hard &&
+	echo quux >bazzy &&
+	git add bazzy &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	! grep quux bazzy &&
+	git stash store -m quuxery $STASH_ID &&
+	test $(cat .git/refs/stash) = $STASH_ID &&
+	grep $STASH_ID .git/logs/refs/stash &&
+	git stash pop &&
+	grep quux bazzy
+'
+
 test_done
-- 
1.8.3.1.441.gd7d6b72.dirty
Junio C Hamano· Jun 18, 2013, 14:31 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v3 4/5] stash: introduce 'git stash store'

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 7 quoted lines
> Junio C Hamano wrote:
>>> +     test $# == 1 ||
>>
>> This is broken under POSIX shells.  No need to resend (will locally
>> patch up).
>
> Ugh, my C habit.  Thanks for catching it.
You're welcome---that is what the reviews are for.
Ramkumar Ramachandra· Jun 15, 2013, 13:13 UTC · re: Ramkumar Ramachandra · lore

[PATCH v3 5/5] rebase: use 'git stash store' to simplify logic

rebase has no reason to know about the implementation of the stash. In the case when applying the autostash results in conflicts, replace the relevant code in finish_rebase () to simply call 'git stash store'.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to git-rebase.sh +2 −5
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..17be392 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -153,11 +153,8 @@ finish_rebase () {
 		then
 			echo "$(gettext 'Applied autostash.')"
 		else
-			ref_stash=refs/stash &&
-			>>"$GIT_DIR/logs/$ref_stash" &&
-			git update-ref -m "autostash" $ref_stash $stash_sha1 ||
-			die "$(eval_gettext 'Cannot store $stash_sha1')"
-
+			git stash store -m "autostash" -q $stash_sha1 ||
+			die "$(eval_gettext "Cannot store \$stash_sha1")"
 			gettext 'Applying autostash resulted in conflicts.
 Your changes are safe in the stash.
 You can run "git stash pop" or "git stash drop" it at any time.
-- 
1.8.3.1.441.gd7d6b72.dirty
Junio C Hamano· Jun 16, 2013, 01:22 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v3 0/5] Write a good 'stash store' for autostash

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 6 quoted lines
> Ramkumar Ramachandra (5):
>   stash doc: add a warning about using create
>   stash doc: document short form -p in synopsis
>   stash: simplify option parser for create
>   stash: introduce 'git stash store'
>   rebase: use 'git stash store' to simplify logic

Looked sensible overall. I briefly debated myself if "git stash store" even needs its own error message that needs to be squelched with -q (as there is no reason for the end user to invoke it), but I think it does not matter that much, and we can remove that part fairly easily before it hits 'next'.

Will queue (but I am not on my main machine today, so not very soon).

Thanks.

← back to recent threads