threads / patch / 33806

patch, 6 partsGet stash to help rebase.autostash

Subject: [PATCH 0/6] Get stash to help rebase.autostash

## tl;dr

21 messages between May 13, 2013 and May 13, 2013. Diffs are folded; open one to read it.

replies: 20people: 2as markdown or json

Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · lore
Hi,

This topic is based on the rebase.autostash topic. It cleans up a few things, introduces 'git stash store', and patches rebase to use it. Should be simple enough.

Thanks.
Ramkumar Ramachandra (6):
  Documentation/stash: correct synopsis for create
  Documentation/stash: document short form -p in synopsis
  stash: simplify option parser for create
  stash: introduce 'git stash store'
  stash: tweak error message in store_stash ()
  rebase: use 'git stash store' to simplify logic
 Documentation/git-stash.txt | 10 ++++++++--
 git-rebase.sh               |  6 +-----
 git-stash.sh                | 32 +++++++++++++++++++++-----------
 t/t3903-stash.sh            | 20 ++++++++++++++++++++
 4 files changed, 50 insertions(+), 18 deletions(-)
-- 
1.8.3.rc1.57.g4ac1522
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/6] Documentation/stash: correct synopsis for create

'git stash create' can optionally take one or two arguments. 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 711ffe1..35a0134 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> [<include-untracked-p>]]
 
 DESCRIPTION
 -----------
-- 
1.8.3.rc1.57.g4ac1522
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/6] Documentation/stash: document short form -p in synopsis

'git stash save' can take -p, the short form of --patch, as an optional. 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 35a0134..05e462b 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> [<include-untracked-p>]]
-- 
1.8.3.rc1.57.g4ac1522
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/6] 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. Also, use "$@", not "$*", as our caller is expecting "$1" "$2", not "$1c$2" (where c is the first character of IFS).

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-stash.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to git-stash.sh +2 −5
diff --git a/git-stash.sh b/git-stash.sh
index bbefdf6..0ede313 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -546,11 +546,8 @@ clear)
 	clear_stash "$@"
 	;;
 create)
-	if test $# -gt 0 && test "$1" = create
-	then
-		shift
-	fi
-	create_stash "$*" && echo "$w_commit"
+	shift
+	create_stash "$@" && echo "$w_commit"
 	;;
 drop)
 	shift
-- 
1.8.3.rc1.57.g4ac1522
Junio C Hamano· May 13, 2013, 14:34 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> The option parser for create unnecessarily checks "$1" inside a case
> statement that matches "$1" in the first place.  Also, use "$@", not
> "$*", as our caller is expecting "$1" "$2", not "$1c$2" (where c is the
> first character of IFS).

The first part of the patch may be OK but the rest unfortunately is wrong.

The semi-user facing "git stash create" never was meant to take anything but a message sentence and "$*" is the proper way to say "everything is meant for a single message (just like echo)". Changing it to "$@" will change the semantics in a big way.

Show 23 quoted lines
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-stash.sh | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index bbefdf6..0ede313 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -546,11 +546,8 @@ clear)
>  	clear_stash "$@"
>  	;;
>  create)
> -	if test $# -gt 0 && test "$1" = create
> -	then
> -		shift
> -	fi
> -	create_stash "$*" && echo "$w_commit"
> +	shift
> +	create_stash "$@" && echo "$w_commit"
>  	;;
>  drop)
>  	shift
Ramkumar Ramachandra· May 13, 2013, 14:59 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> The semi-user facing "git stash create" never was meant to take
> anything but a message sentence and "$*" is the proper way to say
> "everything is meant for a single message (just like echo)".
> Changing it to "$@" will change the semantics in a big way.

Ah, I see. As an interactive caller, it is impossible to set $untracked (I thought this was a mistake, but you're indicating that it's intentional). Okay, I'll fix the patch and documentation.

Thanks.
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

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

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 |  6 ++++++
 git-stash.sh                | 25 +++++++++++++++++++------
 t/t3903-stash.sh            | 20 ++++++++++++++++++++
 3 files changed, 45 insertions(+), 6 deletions(-)
Show changes to 3 files +45 −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 05e462b..e58ab74 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> [<include-untracked-p>]]
+'git stash' store <commit> <message>
 
 DESCRIPTION
 -----------
@@ -152,6 +153,11 @@ create::
 	Create a stash (which is a regular commit object) and return its
 	object name, without storing it anywhere in the ref namespace.
 
+store::
+
+	Store a given stash created via 'git stash create' (which is a
+	dangling merge commit) in the stash ref, updating the stash
+	reflog.
 
 DISCUSSION
 ----------
diff --git a/git-stash.sh b/git-stash.sh
index 0ede313..1d483f5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -156,6 +156,20 @@ create_stash () {
 	die "$(gettext "Cannot record working tree state")"
 }
 
+store_stash () {
+	if test $# != 2
+	then
+		die "$(gettext "git stash store requires two arguments")"
+	fi
+	w_commit="$1"
+	stash_msg="$2"
+
+	# 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")"
+}
+
 save_stash () {
 	keep_index=
 	patch_mode=
@@ -227,12 +241,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 $w_commit "$stash_msg"
 	say Saved working directory and index state "$stash_msg"
 
 	if test -z "$patch_mode"
@@ -549,6 +558,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..2ff3afd 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -637,4 +637,24 @@ test_expect_success 'stash where working directory contains "HEAD" file' '
 	test_cmp output expect
 '
 
+test_expect_success 'store - argument 1 not a commit' '
+	test_must_fail git stash store foo bar
+'
+
+test_expect_success 'store - updates stash ref and reflog' '
+	git stash clear &&
+	git reset --hard &&
+	echo quux >file &&
+	git add file &&
+	STASH_ID=$(git stash create) &&
+	git reset --hard &&
+	! grep quux file &&
+	git stash store ${STASH_ID} quuxery &&
+	test $(cat .git/refs/stash) = ${STASH_ID} &&
+	grep ${STASH_ID} .git/logs/refs/stash &&
+	git stash pop &&
+	grep quux file &&
+	false
+'
+
 test_done
-- 
1.8.3.rc1.57.g4ac1522
Junio C Hamano· May 13, 2013, 14:40 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 9 quoted lines
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 05e462b..e58ab74 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> [<include-untracked-p>]]
> +'git stash' store <commit> <message>
Two points:
 - We should not advertise "store" (and "create" for that matter) in
   the end-user facing documentation.  IIRC, "git stash -h"
   deliberately omits 'create'; having it in the documentation is
   unavoidable, but it was a mistake that it was not marked with
   "this is most likely not what you want to use; see 'save'".
   It may even be better with a leading underscore or two in the
   name that clearly marks it as "not meant for direct end-user
   consumption".
 - The error message store_stash() gives should not be hardcoded in
   that function.
   Save-stash may want to keep saying 'the current status' as it
   said before, but a caller like your rebase-autostash will not be
   saving the "current" status and would want to have a different
   message.
Other than that, the overfall structure of the patch looks OK to me.
Ramkumar Ramachandra· May 13, 2013, 15:02 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 7 quoted lines
>  - The error message store_stash() gives should not be hardcoded in
>    that function.
>
>    Save-stash may want to keep saying 'the current status' as it
>    said before, but a caller like your rebase-autostash will not be
>    saving the "current" status and would want to have a different
>    message.
Makes sense.
Thanks.
Junio C Hamano· May 13, 2013, 15:49 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 10 quoted lines
> Junio C Hamano wrote:
>>  - The error message store_stash() gives should not be hardcoded in
>>    that function.
>>
>>    Save-stash may want to keep saying 'the current status' as it
>>    said before, but a caller like your rebase-autostash will not be
>>    saving the "current" status and would want to have a different
>>    message.
>
> Makes sense.

I think it is fine to have a "default" message to be used when store_stash shell function is not given an optional error message.

The command line for it may become:
	git stash store [-m <message>] [-e <error message>] $stash_sha1
or something like that.
Ramkumar Ramachandra· May 13, 2013, 17:15 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
>         git stash store [-m <message>] [-e <error message>] $stash_sha1
1. The message is NOT optional.  If the user says 'git stash store
b8bb3fe9', what "default message" can we possibly put in?  There is
absolutely no context: no branch name, nothing.  So, the best we can
do is "generic WIP".  What is the point of putting in such a useless
message?
2. We have already determined that the command is NOT for end user
interactive use.  So, why do we need a default error message at all?
The last statement is an update-ref, and you can infer whether it
succeeded or failed from the exit status.
3. Why are we designing a command-line interface?  git stash store
"$stash_sha1" "$message" is sufficient for scripts, and there is
absolutely no point in parsing '-m', '-e', or any such thing.
Junio C Hamano· May 13, 2013, 17:29 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 6 quoted lines
> Junio C Hamano wrote:
>>         git stash store [-m <message>] [-e <error message>] $stash_sha1
> ...
> 3. Why are we designing a command-line interface?  git stash store
> "$stash_sha1" "$message" is sufficient for scripts, and there is
> absolutely no point in parsing '-m', '-e', or any such thing.

"git stash store $stash_sha1 $message [ $error_message ]" is adequate an internal API _for now_.

I however suspect that you would regret later when you need more customization. It already happened once for "git merge" when it was an internal API for "git pull" and it was painful to support saner interface and the traditional one at the same time [*1*].

[Footnote]

*1* And no, don't even try to rewrite "git merge" call inside "git pull" to use the modern style with "-m <message>"; you will likely break it (I've tried once and decided it was not worth the hassle).

Ramkumar Ramachandra· May 13, 2013, 18:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> I however suspect that you would regret later when you need more
> customization.  It already happened once for "git merge" when it was
> an internal API for "git pull" and it was painful to support saner
> interface and the traditional one at the same time [*1*].
Oh god.
  git-merge --stat --progress "$merge_name" HEAD 04c5b83c46760573

We made a design mistake at the command-level in merge. This is at a subcommand-level.

1. Will git stash store ever be more than a one-liner?  Can you think
of how this function could be larger?
2. Will git stash store ever become an interactive command?  Isn't the
whole point of interactive stash something that operates on a
worktree?  Why will I ever want to operate on a commit with stash,
interactively?

While it is absolutely necessary to avoid calamities like the merge invocation in git-pull.sh, we shouldn't be over-engineering either.

Junio C Hamano· May 13, 2013, 18:35 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 7 quoted lines
>   git-merge --stat --progress "$merge_name" HEAD 04c5b83c46760573
>
> We made a design mistake at the command-level in merge.  This is at a
> subcommand-level.
>
> 1. Will git stash store ever be more than a one-liner?  Can you think
> of how this function could be larger?
That is not a valid question.  When Linus added the original
    git merge 'message' HEAD $other_branch

there wasn't any merge strategy switch and all the other frills even envisioned yet. With what we know about Git today, even you could have answered "Will git merge be more involved compared to the original?" 8 years ago, but without that, not even Linus nor I did anticipate that the command line interface would become a pain point when enhancing it.

Ramkumar Ramachandra· May 13, 2013, 18:45 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> That is not a valid question.

I was just asking to see if you could think of something. I just did: named stashes (each one has a different ref/ reflog) for internal use.

Sure, we'll go with the -m -e approach.
Junio C Hamano· May 13, 2013, 19:16 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 7 quoted lines
> Junio C Hamano wrote:
>> That is not a valid question.
>
> I was just asking to see if you could think of something.  I just did:
> named stashes (each one has a different ref/ reflog) for internal use.
>
> Sure, we'll go with the -m -e approach.

The whole point of my response is that it is not a valid approach to decide to add (or not to add) a reasonable enhancement mechanism built in from the beginning by asking "what future enhancement do you foresee today?". It is unclear if you got that point.

As long as the end result is to have something that does not paint us into a corner from which it will be painful to escape, I'll be happy anyway.

Ramkumar Ramachandra· May 13, 2013, 19:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> The whole point of my response is that it is not a valid approach to
> decide to add (or not to add) a reasonable enhancement mechanism
> built in from the beginning by asking "what future enhancement do
> you foresee today?".  It is unclear if you got that point.

Right, got it. The fact that we weren't able to foresee the merge UI problem tells us that we're capable of repeating the mistake no matter how much we think we've thought about it.

Ramkumar Ramachandra· May 13, 2013, 18:15 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> *1* And no, don't even try to rewrite "git merge" call inside "git
> pull" to use the modern style with "-m <message>"; you will likely
> break it (I've tried once and decided it was not worth the hassle).

This falls in my basket of "nice theoretical exercise": a lot of work for no tangible benefit ;)

Footnote: I'm not saying that code is not important; you've seen me
arguing for beautiful implementations several times before, against
all odds.
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 5/6] stash: tweak error message in store_stash ()

When the update-ref invocation fails, store_stash currently prints:
    Cannot save the current status
This is not very useful for diagnosing the problem.  Instead, print:
    Cannot store 688268c4254ca5dc6e2effa83bae4f0dbbe75e5b
so we can inspect the object and analyze why the update-ref failed.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-stash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git-stash.sh +1 −1
diff --git a/git-stash.sh b/git-stash.sh
index 1d483f5..24d72fc 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -167,7 +167,7 @@ store_stash () {
 	# 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")"
+	die "$(gettext "Cannot store $w_commit")"
 }
 
 save_stash () {
-- 
1.8.3.rc1.57.g4ac1522
Junio C Hamano· May 13, 2013, 14:42 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/6] stash: tweak error message in store_stash ()

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 9 quoted lines
> When the update-ref invocation fails, store_stash currently prints:
>
>     Cannot save the current status
>
> This is not very useful for diagnosing the problem.  Instead, print:
>
>     Cannot store 688268c4254ca5dc6e2effa83bae4f0dbbe75e5b
>
> so we can inspect the object and analyze why the update-ref failed.

This would break the error message for save_stash with your current patch series, wouldn't it? I think this patch is a solution to a wrong problem. As I already said in 4/6, store_stash should allow its caller to supply a customised error message.

Show 19 quoted lines
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-stash.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 1d483f5..24d72fc 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -167,7 +167,7 @@ store_stash () {
>  	# 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")"
> +	die "$(gettext "Cannot store $w_commit")"
>  }
>  
>  save_stash () {
Ramkumar Ramachandra· May 13, 2013, 12:45 UTC · re: Ramkumar Ramachandra · lore

[PATCH 6/6] 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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Show changes to git-rebase.sh +1 −5
diff --git a/git-rebase.sh b/git-rebase.sh
index f4a3a26..7c82f95 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -153,11 +153,7 @@ finish_rebase () {
 		then
 			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 $stash_sha1 "autostash" &&
 			gettext 'Applying autostash resulted in conflicts.
 Your changes are safe in the stash.
 You can run "git stash pop" or "git stash drop" at any time.
-- 
1.8.3.rc1.57.g4ac1522

← back to recent threads