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

DORMANTno replies

10 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

Hi,

So, I've taken Junio's suggestion and designed a proper command-line
interface for 'git stash store' in this iteration:

  git stash store [-m <message>] [-e <error>] <commit>

The error string will be passed through eval_gettext before it is
printed.  Otherwise, the idea is the same: to clean up the logic
surrounding autostash.

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 | 12 ++++++++--
 git-rebase.sh               |  6 +----
 git-stash.sh                | 53 ++++++++++++++++++++++++++++++++++++---------
 t/t3903-stash.sh            | 19 ++++++++++++++++
 4 files changed, 73 insertions(+), 17 deletions(-)

-- 
1.8.3.1.383.g0d5ad6b

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

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 <redacted>
---
 Documentation/git-stash.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 711ffe1..83bb3db 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,7 @@ 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 probably not what you want to use; see "save" above.
 
 
 DISCUSSION
-- 
1.8.3.1.383.g0d5ad6b

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

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

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 Documentation/git-stash.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 83bb3db..08cdf2f 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.383.g0d5ad6b

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

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 <redacted>
---
 git-rebase.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..bf37259 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -153,11 +153,7 @@ 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" -e "Cannot store $stash_sha1." $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.383.g0d5ad6b

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

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 <redacted>
---
 Documentation/git-stash.txt |  7 +++++++
 git-stash.sh                | 48 +++++++++++++++++++++++++++++++++++++++------
 t/t3903-stash.sh            | 19 ++++++++++++++++++
 3 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 08cdf2f..ec3a9a3 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>] [-e|--error <message>] <commit>
 
 DESCRIPTION
 -----------
@@ -153,6 +154,12 @@ create::
 	object name, without storing it anywhere in the ref namespace.
 	This is probably not what 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 probably not what you want to use; see
+	"save" above.
 
 DISCUSSION
 ----------
diff --git a/git-stash.sh b/git-stash.sh
index 64800b8..7985473 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -156,6 +156,43 @@ create_stash () {
 	die "$(gettext "Cannot record working tree state")"
 }
 
+store_stash () {
+	while test $# != 0
+	do
+		case "$1" in
+		-m|--message)
+			shift
+			stash_msg="$1"
+			;;
+		-e|--error)
+			shift
+			error_msg="$1"
+			;;
+		*)
+			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
+	if test -z "$error_msg"
+	then
+		error_msg="Cannot update $ref_stash with $w_commit."
+	fi
+
+	# Make sure the reflog for stash is kept.
+	: >>"$GIT_DIR/logs/$ref_stash"
+	git update-ref -m "$stash_msg" $ref_stash $w_commit ||
+	die "$(eval_gettext "$error_msg")"
+}
+
 save_stash () {
 	keep_index=
 	patch_mode=
@@ -227,12 +264,7 @@ 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" -e "Cannot save the current status." $w_commit
 	say Saved working directory and index state "$stash_msg"
 
 	if test -z "$patch_mode"
@@ -549,6 +581,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.383.g0d5ad6b

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

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

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 git-stash.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
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.383.g0d5ad6b

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

From: Phil Hord <hidden>
Date: 2016-06-15 22:57:44

On Fri, Jun 14, 2013 at 6:32 AM, Ramkumar Ramachandra
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
 Documentation/git-stash.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 711ffe1..83bb3db 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,7 @@ 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 probably not what you want to use; see "save" above.
Thanks, this is helpful.

Maybe you can also hint why this command exists.

  +       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.383.g0d5ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: Phil Hord <hidden>
Date: 2016-06-15 22:57:44

On Fri, Jun 14, 2013 at 6:32 AM, Ramkumar Ramachandra
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
 Documentation/git-stash.txt |  7 +++++++
 git-stash.sh                | 48 +++++++++++++++++++++++++++++++++++++++------
 t/t3903-stash.sh            | 19 ++++++++++++++++++
 3 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 08cdf2f..ec3a9a3 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>] [-e|--error <message>] <commit>

 DESCRIPTION
 -----------
@@ -153,6 +154,12 @@ create::
        object name, without storing it anywhere in the ref namespace.
        This is probably not what 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 probably not what you want to use; see
+       "save" above.
Here, again, I think you should explain more about this command.  Your
commit message explained it pretty well.

  + Like create, store is intended to be useful for scripts.
quoted hunk
 DISCUSSION
 ----------
diff --git a/git-stash.sh b/git-stash.sh
index 64800b8..7985473 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -156,6 +156,43 @@ create_stash () {
        die "$(gettext "Cannot record working tree state")"
 }

+store_stash () {
+       while test $# != 0
+       do
+               case "$1" in
+               -m|--message)
+                       shift
+                       stash_msg="$1"
+                       ;;
+               -e|--error)
+                       shift
+                       error_msg="$1"
+                       ;;
+               *)
+                       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
+       if test -z "$error_msg"
+       then
+               error_msg="Cannot update $ref_stash with $w_commit."
+       fi
+
+       # Make sure the reflog for stash is kept.
+       : >>"$GIT_DIR/logs/$ref_stash"
+       git update-ref -m "$stash_msg" $ref_stash $w_commit ||
+       die "$(eval_gettext "$error_msg")"
+}
+
 save_stash () {
        keep_index=
        patch_mode=
@@ -227,12 +264,7 @@ 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" -e "Cannot save the current status." $w_commit
nit: this adds a period to the end of the message which it did not
have previously.
quoted hunk
        say Saved working directory and index state "$stash_msg"

        if test -z "$patch_mode"
@@ -549,6 +581,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.383.g0d5ad6b
Looks clean to me, but I didn't test it.

Phil

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

From: Phil Hord <hidden>
Date: 2016-06-15 22:57:44

On Fri, Jun 14, 2013 at 6:32 AM, Ramkumar Ramachandra
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
 git-rebase.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..bf37259 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -153,11 +153,7 @@ 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" -e "Cannot store $stash_sha1." $stash_sha1
nit: adds a period where there was not one previously.

Maybe this doesn't matter so much since this code is new anyway.  But
showing a period after sha1 seems wrong, too. Or maybe I am confused
again.  Does eval_gettext routinely add a period to the end of
translated strings?
                        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.383.g0d5ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:44

Phil Hord wrote:
nit: adds a period where there was not one previously.
Stripped periods in both, thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help