Re: [PATCH v3 4/5] stash: introduce new format create

[Thomas Gummerer]

On 02/13, Jeff King wrote:
> On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:
> 
> > Yeah, I think your patch is actually fixing that case. But your search
> > is only part of the story. You found somebody using "-m" explicitly, but
> > what about somebody blindly calling:
> > 
> >   git stash create $*
> > 
> > That's now surprising to somebody who puts "-m" in their message.
> > 
> > > I *think* this regression is acceptable, but I'm happy to introduce
> > > another verb if people think otherwise.
> > 
> > Despite what I wrote above, I'm still inclined to say that this isn't an
> > important regression. I'd be surprised if "stash create" is used
> > independently much at all.
> 
> Just thinking on this more...do we really care about "fixing" the
> interface of "stash create"? This is really just about refactoring what
> underlies the new "push", right?
>
> So we could just do:
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index 6d629fc43..ee37db135 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -711,7 +711,7 @@ clear)
>   ;;
>  create)
>   shift
> - create_stash "$@" && echo "$w_commit"
> + create_stash -m "$*" && echo "$w_commit"
>   ;;
>  store)
>   shift
> 
> on top of your patch and keep the external interface the same.
> 
> It might be nice to clean up the interface for "create" to match other
> ones, but from this discussion I think it is mostly a historical wart
> for scripting, and we are OK to just leave its slightly-broken interface
> in place forever.

Yeah tbh I don't personally care too much about modernizing the
interface to git stash create.  What you have above makes a lot of
sense to me.

I also just noticed that git stash create -m message is not the worst
regression I introduced when trying to modernize this.  In that case
only the -m would go missing, but that's probably not the end of the
world.  The worse thing to do would be something like
git stash create -u untracked, where the intended message was "-u
untracked", but instead there is no message, but all untracked files
are now included in the stash as well.

In that light what you have above makes even more sense to me.
Thanks!

> I could go either way.
> 
> -Peff

-- 
Thomas

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:

> Yeah, I think your patch is actually fixing that case. But your search
> is only part of the story. You found somebody using "-m" explicitly, but
> what about somebody blindly calling:
> 
>   git stash create $*
> 
> That's now surprising to somebody who puts "-m" in their message.
> 
> > I *think* this regression is acceptable, but I'm happy to introduce
> > another verb if people think otherwise.
> 
> Despite what I wrote above, I'm still inclined to say that this isn't an
> important regression. I'd be surprised if "stash create" is used
> independently much at all.

Just thinking on this more...do we really care about "fixing" the
interface of "stash create"? This is really just about refactoring what
underlies the new "push", right?

So we could just do:

diff --git a/git-stash.sh b/git-stash.sh
index 6d629fc43..ee37db135 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -711,7 +711,7 @@ clear)
;;
 create)
shift
-   create_stash "$@" && echo "$w_commit"
+   create_stash -m "$*" && echo "$w_commit"
;;
 store)
shift

on top of your patch and keep the external interface the same.

It might be nice to clean up the interface for "create" to match other
ones, but from this discussion I think it is mostly a historical wart
for scripting, and we are OK to just leave its slightly-broken interface
in place forever.

I could go either way.

-Peff

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Sat, Feb 11, 2017 at 02:51:27PM +, Thomas Gummerer wrote:

> > How do we tell the difference between new-style invocations, and
> > old-style ones that look new-style? IOW, I think:
> > 
> >   git stash create -m works
> > 
> > currently treats "-m works" as the full message, and it would now become
> > just "works".
> > 
> > That may be an acceptable loss for the benefit we are getting. The
> > alternative is to make yet another verb for create, as we did with
> > save/push). I have a feeling that hardly anybody uses "create", though,
> > and it's mostly an implementation detail. So given the obscure nature,
> > maybe it's an acceptable level of regression. I dunno.
> 
> Right.  So I did a quick search on google and github for this, and
> there seems one place where git stash create -m is used [1].  From a
> quick look it does however not seem like the -m in the stash message
> is of any significance there, but rather that the intention was to use
> a flag that doesn't exist.

Yeah, I think your patch is actually fixing that case. But your search
is only part of the story. You found somebody using "-m" explicitly, but
what about somebody blindly calling:

  git stash create $*

That's now surprising to somebody who puts "-m" in their message.

> I *think* this regression is acceptable, but I'm happy to introduce
> another verb if people think otherwise.

Despite what I wrote above, I'm still inclined to say that this isn't an
important regression. I'd be surprised if "stash create" is used
independently much at all.

-Peff

Re: [PATCH v3 4/5] stash: introduce new format create

[Thomas Gummerer]

On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:41PM +, Thomas Gummerer wrote:
> 
> > git stash create currently supports a positional argument for adding a
> > message.  This is not quite in line with how git commands usually take
> > comments (using a -m flag).
> > 
> > Add a new syntax for adding a message to git stash create using a -m
> > flag.  This is with the goal of deprecating the old style git stash
> > create with positional arguments.
> > 
> > This also adds a -u argument, for untracked files.  This is already used
> > internally as another positional argument, but can now be used from the
> > command line.
> 
> How do we tell the difference between new-style invocations, and
> old-style ones that look new-style? IOW, I think:
> 
>   git stash create -m works
> 
> currently treats "-m works" as the full message, and it would now become
> just "works".
> 
> That may be an acceptable loss for the benefit we are getting. The
> alternative is to make yet another verb for create, as we did with
> save/push). I have a feeling that hardly anybody uses "create", though,
> and it's mostly an implementation detail. So given the obscure nature,
> maybe it's an acceptable level of regression. I dunno.

Right.  So I did a quick search on google and github for this, and
there seems one place where git stash create -m is used [1].  From a
quick look it does however not seem like the -m in the stash message
is of any significance there, but rather that the intention was to use
a flag that doesn't exist.

I *think* this regression is acceptable, but I'm happy to introduce
another verb if people think otherwise.

> But either way, it should probably be in the commit message in case
> somebody does have to track it down later.

I'll add something there, thanks!

> -Peff

[1]: 
https://github.com/Andersbakken/nrdp-scripts/blob/1052fc6781c36c4b227c7dd4838a501341b0862a/bin/git-rstash#L81

Re: [PATCH v3 4/5] stash: introduce new format create

[Jeff King]

On Sun, Feb 05, 2017 at 08:26:41PM +, Thomas Gummerer wrote:

> git stash create currently supports a positional argument for adding a
> message.  This is not quite in line with how git commands usually take
> comments (using a -m flag).
> 
> Add a new syntax for adding a message to git stash create using a -m
> flag.  This is with the goal of deprecating the old style git stash
> create with positional arguments.
> 
> This also adds a -u argument, for untracked files.  This is already used
> internally as another positional argument, but can now be used from the
> command line.

How do we tell the difference between new-style invocations, and
old-style ones that look new-style? IOW, I think:

  git stash create -m works

currently treats "-m works" as the full message, and it would now become
just "works".

That may be an acceptable loss for the benefit we are getting. The
alternative is to make yet another verb for create, as we did with
save/push). I have a feeling that hardly anybody uses "create", though,
and it's mostly an implementation detail. So given the obscure nature,
maybe it's an acceptable level of regression. I dunno.

But either way, it should probably be in the commit message in case
somebody does have to track it down later.

-Peff

[PATCH v3 4/5] stash: introduce new format create

[Thomas Gummerer]

git stash create currently supports a positional argument for adding a
message.  This is not quite in line with how git commands usually take
comments (using a -m flag).

Add a new syntax for adding a message to git stash create using a -m
flag.  This is with the goal of deprecating the old style git stash
create with positional arguments.

This also adds a -u argument, for untracked files.  This is already used
internally as another positional argument, but can now be used from the
command line.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  1 +
 git-stash.sh| 50 +
 t/t3903-stash.sh|  9 
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..a138ed6a24 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [-u|--include-untracked] [-a|--all] []]
 'git stash' clear
 'git stash' create []
+'git stash' create [-m ] [-u|--include-untracked ]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index bf7863a846..33b2d0384c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -56,8 +56,50 @@ clear_stash () {
 }
 
 create_stash () {
-   stash_msg="$1"
-   untracked="$2"
+   stash_msg=
+   untracked=
+   new_style=
+   while test $# != 0
+   do
+   case "$1" in
+   -m|--message)
+   shift
+   stash_msg=${1?"-m needs an argument"}
+   new_style=t
+   ;;
+   -u|--include-untracked)
+   shift
+   untracked="$1"
+   new_style=t
+   ;;
+   *)
+   if test -n "$new_style"
+   then
+   echo "invalid argument"
+   option="$1"
+   # TRANSLATORS: $option is an invalid option, 
like
+   # `--blah-blah'. The 7 spaces at the beginning 
of the
+   # second line correspond to "error: ". So you 
should line
+   # up the second line with however many 
characters the
+   # translation of "error: " takes in your 
language. E.g. in
+   # English this is:
+   #
+   #$ git stash save --blah-blah 2>&1 | head 
-n 2
+   #error: unknown option for 'stash save': 
--blah-blah
+   #   To provide a message, use git stash 
save -- '--blah-blah'
+   eval_gettextln "error: unknown option for 
'stash create': \$option"
+   usage
+   fi
+   break
+   ;;
+   esac
+   shift
+   done
+
+   if test -z "$new_style"
+   then
+   stash_msg="$*"
+   fi
 
git update-index -q --refresh
if no_changes
@@ -265,7 +307,7 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
 
-   create_stash "$stash_msg" $untracked
+   create_stash -m "$stash_msg" -u "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state 
\$stash_msg")"
@@ -667,7 +709,7 @@ clear)
;;
 create)
shift
-   create_stash "$*" && echo "$w_commit"
+   create_stash "$@" && echo "$w_commit"
;;
 store)
shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 21cb70dc74..b859e51086 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,13 @@ test_expect_success 'create with multiple arguments for 
the message' '
test_cmp expect actual
 '
 
+test_expect_success 'new style stash create stores correct message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create -m "create test message new style") &&
+   echo "On master: create test message new style" >expect &&
+   git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.12.0.rc0.208.g81c5d00b20.dirty