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

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

[Ramkumar Ramachandra]

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

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

[Ramkumar Ramachandra]

'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(-)

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

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

[Ramkumar Ramachandra]

'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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Ramkumar Ramachandra]

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(-)

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

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

[Junio C Hamano]

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.

> 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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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.

>
> 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 () {

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

[Ramkumar Ramachandra]

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.

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

[Ramkumar Ramachandra]

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.

Thanks.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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.

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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).

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

[Ramkumar Ramachandra]

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.

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>   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.

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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.

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

[Ramkumar Ramachandra]

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.