Re: [PATCH v2 2/4] stash: introduce push verb

[Thomas Gummerer]

On 01/30, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Introduce a new git stash push verb in addition to git stash save.  The
> > push verb is used to transition from the current command line arguments
> > to a more conventional way, in which the message is specified after a -m
> > parameter instead of being a positional argument.
> 
> I think the canonical way to express that is "... the message is
> given as an argument to the -m option" (i.e. some options take an
> argument, some others do not, and the "-m" takes one).
> 
> > This allows introducing a new filename argument to stash single files.
> 
> I do not want them to be "a filename argument", and I do not think
> you meant them as such, either.  
> 
> This allows us to have pathspecs at the end of the command line
> arguments like other Git commands do, so that the user can say
> which subset of paths to stash (and leave others behind).

Yeah, this is much better, thanks.

> > +save_stash () {
> > +   push_options=
> > +   while test $# != 0
> > +   do
> > +   case "$1" in
> > +   -k|--keep-index)
> > +...
> > +   esac
> > +   shift
> > +   done
> 
> It is a bit unfortunate that we need to duplicate the above
> case/esac here.  I do not know if doing it this way:
> 
>   case "$1" in
>   --)
>   shift
>   break 
>   ;;
>   --help)
>   show_help
>   ;;
>   -*)
>   # pass all options through to push_stash
>   push_options="$push_options $1"
>   ;;
>   *)
>   break
> ;;
>   esac
> 
> and letting push_stash complain for an unknown option is easier to
> maintain.

I think this will work out nicely.  Will try to implement it this way.

> You are reversing the order of the options in the loop.  Don't.

-- 
Thomas

Re: [PATCH v2 2/4] stash: introduce push verb

[Junio C Hamano]

Thomas Gummerer  writes:

> Introduce a new git stash push verb in addition to git stash save.  The
> push verb is used to transition from the current command line arguments
> to a more conventional way, in which the message is specified after a -m
> parameter instead of being a positional argument.

I think the canonical way to express that is "... the message is
given as an argument to the -m option" (i.e. some options take an
argument, some others do not, and the "-m" takes one).

> This allows introducing a new filename argument to stash single files.

I do not want them to be "a filename argument", and I do not think
you meant them as such, either.  

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say
which subset of paths to stash (and leave others behind).

> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + -k|--keep-index)
> +...
> + esac
> + shift
> + done

It is a bit unfortunate that we need to duplicate the above
case/esac here.  I do not know if doing it this way:

case "$1" in
--)
shift
break 
;;
--help)
show_help
;;
-*)
# pass all options through to push_stash
push_options="$push_options $1"
;;
*)
break
;;
esac

and letting push_stash complain for an unknown option is easier to
maintain.

You are reversing the order of the options in the loop.  Don't.

[PATCH v2 2/4] stash: introduce push verb

[Thomas Gummerer]

Introduce a new git stash push verb in addition to git stash save.  The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is specified after a -m
parameter instead of being a positional argument.

This allows introducing a new filename argument to stash single files.
Using that as a positional argument is much more consistent with the
rest of git, than using the positional argument for the message.

Signed-off-by: Thomas Gummerer 
---
 git-stash.sh | 76 +---
 t/t3903-stash.sh |  9 +++
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8528708f61 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -189,10 +189,11 @@ store_stash () {
return $ret
 }
 
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+   stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +217,10 @@ save_stash () {
-a|--all)
untracked=all
;;
+   -m|--message)
+   shift
+   stash_msg=$1
+   ;;
--help)
show_help
;;
@@ -251,8 +256,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or 
--all at the same time")"
fi
 
-   stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +294,69 @@ save_stash () {
fi
 }
 
+save_stash () {
+   push_options=
+   while test $# != 0
+   do
+   case "$1" in
+   -k|--keep-index)
+   push_options="-k $push_options"
+   ;;
+   --no-keep-index)
+   push_options="--no-keep-index $push_options"
+   ;;
+   -p|--patch)
+   push_options="-p $push_options"
+   ;;
+   -q|--quiet)
+   push_options="-q $push_options"
+   ;;
+   -u|--include-untracked)
+   push_options="-u $push_options"
+   ;;
+   -a|--all)
+   push_options="-a $push_options"
+   ;;
+   --help)
+   show_help
+   ;;
+   --)
+   shift
+   break
+   ;;
+   -*)
+   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 save': 
\$option
+   To provide a message, use git stash save -- '\$option'"
+   usage
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   stash_msg="$*"
+
+   if test -z "$stash_msg"
+   then
+   push_stash $push_options
+   else
+   push_stash $push_options -m "$stash_msg"
+   fi
+}
+
 have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +683,10 @@ save)
shift
save_stash "$@"
;;
+push)
+   shift
+   push_stash "$@"
+   ;;
 apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..0171b824c9 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial 
renames' '
test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+   >foo &&
+   git add foo &&
+   git stash push -m "test message" &&
+   echo "stash@{0}: On master: test message" >expect &&
+   git stash list | head -n 1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.297.g9a2118ac0b.dirty