* [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
@ 2017-01-29 20:16 ` Thomas Gummerer
2017-01-30 21:13 ` Junio C Hamano
2017-01-29 20:16 ` [PATCH v2 2/4] stash: introduce push verb Thomas Gummerer
` (3 subsequent siblings)
4 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 20:16 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
Don't mention git reset --hard in the documentation for git stash save.
It's an implementation detail that doesn't matter to the end user and
thus shouldn't be exposed to them. In addition it's not quite true for
git stash -p, and will not be true when a filename argument to limit the
stash to a few files is introduced.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..0fc23c25ee 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -47,8 +47,9 @@ OPTIONS
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
- Save your local modifications to a new 'stash', and run `git reset
- --hard` to revert them. The <message> part is optional and gives
+ Save your local modifications to a new 'stash' and roll them
+ back both in the working tree and in the index.
+ The <message> part is optional and gives
the description along with the stashed state. For quickly making
a snapshot, you can omit _both_ "save" and <message>, but giving
only <message> does not trigger this action to prevent a misspelled
--
2.11.0.297.g9a2118ac0b.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
2017-01-29 20:16 ` [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-01-30 21:13 ` Junio C Hamano
2017-02-05 12:13 ` Thomas Gummerer
0 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2017-01-30 21:13 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Don't mention git reset --hard in the documentation for git stash save.
> It's an implementation detail that doesn't matter to the end user and
> thus shouldn't be exposed to them.
Everybody understands what "reset --hard" does; it can be an
easier-to-read "short-hand" description to say "reset --hard"
instead of giving a lengthy description of what happens.
Because of that, I do not necessarily agree with the above
justification. I'll read the remainder of the series before
commenting further on the above.
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..0fc23c25ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>
> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them. The <message> part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back both in the working tree and in the index.
"... roll them back both ..." is unclear where they are rolled back
to.
Perhaps "roll them back ... to HEAD" or something?
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
2017-01-30 21:13 ` Junio C Hamano
@ 2017-02-05 12:13 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 12:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Don't mention git reset --hard in the documentation for git stash save.
> > It's an implementation detail that doesn't matter to the end user and
> > thus shouldn't be exposed to them.
>
> Everybody understands what "reset --hard" does; it can be an
> easier-to-read "short-hand" description to say "reset --hard"
> instead of giving a lengthy description of what happens.
While this is definitely true for experienced git users, it might not
be for some people relatively new to git, which are probably the ones
that need the description most.
> Because of that, I do not necessarily agree with the above
> justification. I'll read the remainder of the series before
> commenting further on the above.
>
> > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> > index 2e9cef06e6..0fc23c25ee 100644
> > --- a/Documentation/git-stash.txt
> > +++ b/Documentation/git-stash.txt
> > @@ -47,8 +47,9 @@ OPTIONS
> >
> > save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
> >
> > - Save your local modifications to a new 'stash', and run `git reset
> > - --hard` to revert them. The <message> part is optional and gives
> > + Save your local modifications to a new 'stash' and roll them
> > + back both in the working tree and in the index.
>
> "... roll them back both ..." is unclear where they are rolled back
> to.
>
> Perhaps "roll them back ... to HEAD" or something?
Yeah that makes sense, thanks.
--
Thomas
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 2/4] stash: introduce push verb
2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
2017-01-29 20:16 ` [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-01-29 20:16 ` Thomas Gummerer
2017-01-30 21:12 ` Junio C Hamano
[not found] ` <xmqqy3xsux4g.fsf@gitster.mtv.corp.google.com>
2017-01-29 20:16 ` [PATCH v2 3/4] introduce new format for git stash create Thomas Gummerer
` (2 subsequent siblings)
4 siblings, 2 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 20:16 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
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 <t.gummerer@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v2 2/4] stash: introduce push verb
2017-01-29 20:16 ` [PATCH v2 2/4] stash: introduce push verb Thomas Gummerer
@ 2017-01-30 21:12 ` Junio C Hamano
[not found] ` <xmqqy3xsux4g.fsf@gitster.mtv.corp.google.com>
1 sibling, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2017-01-30 21:12 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
Thomas Gummerer <t.gummerer@gmail.com> 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.
^ permalink raw reply [flat|nested] 57+ messages in thread[parent not found: <xmqqy3xsux4g.fsf@gitster.mtv.corp.google.com>]
* Re: [PATCH v2 2/4] stash: introduce push verb
[not found] ` <xmqqy3xsux4g.fsf@gitster.mtv.corp.google.com>
@ 2017-02-04 12:19 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-04 12:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A . Holm, Jakub Narębski
On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 3/4] introduce new format for git stash create
2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
2017-01-29 20:16 ` [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
2017-01-29 20:16 ` [PATCH v2 2/4] stash: introduce push verb Thomas Gummerer
@ 2017-01-29 20:16 ` Thomas Gummerer
2017-01-30 21:10 ` Junio C Hamano
[not found] ` <xmqqtw8guwfm.fsf@gitster.mtv.corp.google.com>
2017-01-29 20:16 ` [PATCH v2 4/4] stash: support filename argument Thomas Gummerer
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
4 siblings, 2 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 20:16 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
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 <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 1 +
git-stash.sh | 50 +++++++++++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 18 ++++++++++++++++
3 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 0fc23c25ee..0bce33e3fc 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' create [-m <message>] [-u|--include-untracked <untracked|all>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 8528708f61..5f08b43967 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"
+ 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")"
@@ -697,7 +739,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 0171b824c9..34e9610bb6 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
'
+test_expect_success 'deprecated version of stash create stores correct message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create "create test message") &&
+ echo "On master: create test message" >expect &&
+ git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+ 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.11.0.297.g9a2118ac0b.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v2 3/4] introduce new format for git stash create
2017-01-29 20:16 ` [PATCH v2 3/4] introduce new format for git stash create Thomas Gummerer
@ 2017-01-30 21:10 ` Junio C Hamano
[not found] ` <xmqqtw8guwfm.fsf@gitster.mtv.corp.google.com>
1 sibling, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2017-01-30 21:10 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
Thomas Gummerer <t.gummerer@gmail.com> writes:
> 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"
${1?"-m needs an argument"}
to error check "git stash create -m<Enter>"?
> + if test -z "$new_style"
> + then
> + stash_msg="$*"
> + fi
This breaks external users who do "git stash create" in the old
fashioned way, I think, but can be easily fixed with something like:
stash_msg=$1 untracked=$2
If the existing tests did not catch this, I guess there is a
coverage gap we may want to fill. Perhaps add a new test to 3903
that runs "git stash create message untracked" and makes sure it
still works?
^ permalink raw reply [flat|nested] 57+ messages in thread[parent not found: <xmqqtw8guwfm.fsf@gitster.mtv.corp.google.com>]
* Re: [PATCH v2 3/4] introduce new format for git stash create
[not found] ` <xmqqtw8guwfm.fsf@gitster.mtv.corp.google.com>
@ 2017-02-04 13:18 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-04 13:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A . Holm, Jakub Narębski
On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > 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"
>
> ${1?"-m needs an argument"}
>
> to error check "git stash create -m<Enter>"?
>
> > + if test -z "$new_style"
> > + then
> > + stash_msg="$*"
> > + fi
>
> This breaks external users who do "git stash create" in the old
> fashioned way, I think, but can be easily fixed with something like:
>
> stash_msg=$1 untracked=$2
>
> If the existing tests did not catch this, I guess there is a
> coverage gap we may want to fill. Perhaps add a new test to 3903
> that runs "git stash create message untracked" and makes sure it
> still works?
No I don't think this breaks. It was never possible to add an
untracked argument to git stash create. The difference is in a part
of this patch that is snipped out in your reply:
@@ -697,7 +739,7 @@ clear)
;;
create)
shift
- create_stash "$*" && echo "$w_commit"
+ create_stash "$@" && echo "$w_commit"
;;
store)
shift
If I understand this piece correctly (I'm not very proficient in
shell, but my testing seems to agree with me), previously we used $*,
which transformed all arguments to git stash create into one argument
in create_stash. This needed to change to $@, as otherwise we can't
pull the arguments apart for the new calling style. The two argument
version of create_stash was only used internally in the save_stash
function.
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 0171b824c9..34e9610bb6 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
> > test_cmp expect actual
> > '
> >
> > +test_expect_success 'deprecated version of stash create stores correct message' '
> > + >foo &&
> > + git add foo &&
> > + STASH_ID=$(git stash create "create test message") &&
> > + echo "On master: create test message" >expect &&
> > + git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> > + 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
--
Thomas
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v2 4/4] stash: support filename argument
2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
` (2 preceding siblings ...)
2017-01-29 20:16 ` [PATCH v2 3/4] introduce new format for git stash create Thomas Gummerer
@ 2017-01-29 20:16 ` Thomas Gummerer
2017-01-30 21:11 ` Junio C Hamano
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
4 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 20:16 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone. Unfortunately
git currently offers no such option. git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.
Add an optional filename argument to git stash push, which allows for
stashing a single (or multiple) files.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 9 +++++++++
git-stash.sh | 30 +++++++++++++++++++++++-------
t/t3903-stash.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 0bce33e3fc..8306bac397 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
'git stash' branch <branchname> [<stash>]
'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
+ [--] [<paths>...]
'git stash' clear
'git stash' create [<message>]
'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
+ [-- <paths>...]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<paths>...]::
Save your local modifications to a new 'stash' and roll them
back both in the working tree and in the index.
@@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
only <message> does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
+
+If the paths argument is given in 'git stash push', only these files
+are put in the new 'stash'. In addition only the indicated files are
+changed in the working tree to match the index.
++
If the `--keep-index` option is used, all changes already added to the
index are left intact.
+
diff --git a/git-stash.sh b/git-stash.sh
index 5f08b43967..0072a38b4c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt
+ git ls-files -o -z $excl_opt -- $1
}
clear_stash () {
@@ -59,6 +59,7 @@ create_stash () {
stash_msg=
untracked=
new_style=
+ files=
while test $# != 0
do
case "$1" in
@@ -72,6 +73,12 @@ create_stash () {
untracked="$1"
new_style=t
;;
+ --)
+ shift
+ files="$@"
+ new_style=t
+ break
+ ;;
*)
if test -n "$new_style"
then
@@ -134,7 +141,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files | (
+ untracked_files $files | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -157,7 +164,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- $files >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -171,7 +178,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- &&
+ git add--interactive --patch=stash -- $files &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -293,6 +300,8 @@ push_stash () {
shift
done
+ files="$*"
+
if test -n "$patch_mode" && test -n "$untracked"
then
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
@@ -307,18 +316,25 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash -m "$stash_msg" -u "$untracked"
+ create_stash -m "$stash_msg" -u "$untracked" -- $files
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")"
if test -z "$patch_mode"
then
- git reset --hard ${GIT_QUIET:+-q}
+ if test -n "$files"
+ then
+ git ls-files -z -- "$@" | xargs -0 git reset --
+ git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
+ git ls-files -z --others -- "$@" | xargs -0 git clean --force --
+ else
+ git reset --hard ${GIT_QUIET:+-q}
+ fi
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION
+ git clean --force --quiet -d $CLEAN_X_OPTION -- $files
fi
if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 34e9610bb6..ca4c44aa9c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,46 @@ test_expect_success 'new style stash create stores correct message' '
test_cmp expect actual
'
+test_expect_success 'stash -- <filename> stashes and restores the file' '
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ git stash push -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+ >foo &&
+ >bar &&
+ >extra &&
+ git add foo bar extra &&
+ git stash push -- foo bar &&
+ test_path_is_missing bar &&
+ test_path_is_missing foo &&
+ test_path_is_file extra &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >untracked &&
+ git add foo* &&
+ git stash push -- foo* &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_missing foo &&
+ test_path_is_file untracked &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file untracked
+'
+
test_done
--
2.11.0.297.g9a2118ac0b.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v2 4/4] stash: support filename argument
2017-01-29 20:16 ` [PATCH v2 4/4] stash: support filename argument Thomas Gummerer
@ 2017-01-30 21:11 ` Junio C Hamano
2017-02-05 11:02 ` Thomas Gummerer
0 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2017-01-30 21:11 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Add an optional filename argument to git stash push, which allows for
> stashing a single (or multiple) files.
You can give pathspec with one or more elements, so "an optional
argument" sounds too limiting.
Allow 'git stash push' to take pathspec to specify which paths
to stash.
Also retitle
stash: teach 'push' (and 'create') to honor pathspec
or something.
> @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> only <message> does not trigger this action to prevent a misspelled
> subcommand from making an unwanted stash.
> +
> +If the paths argument is given in 'git stash push', only these files
> +are put in the new 'stash'. In addition only the indicated files are
> +changed in the working tree to match the index.
Actually the stash contains "all paths". You could say that you are
placing _modifications_ to these paths in stash, even though that is
not how Git's world model works (i.e. everything is a snapshot, and
modifications are merely difference between two successive
snapshots). A technically correct version may be:
When pathspec is given to 'git stash push', the new stash
records the modified states only for the files that match
the pathspec. The index entries and working tree files are
then rolled back to the state in HEAD only for these files,
too, leaving files that do not match the pathspec intact.
> diff --git a/git-stash.sh b/git-stash.sh
> index 5f08b43967..0072a38b4c 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -41,7 +41,7 @@ no_changes () {
> untracked_files () {
> excl_opt=--exclude-standard
> test "$untracked" = "all" && excl_opt=
> - git ls-files -o -z $excl_opt
> + git ls-files -o -z $excl_opt -- $1
Hmph, why "$1" is spelled without dq, implying that it is split at
$IFS boundary? This line alone makes me suspect that this is not
prepared to deal correctly with $IFS. Let's read on...
> @@ -59,6 +59,7 @@ create_stash () {
> stash_msg=
> untracked=
> new_style=
> + files=
> while test $# != 0
> do
> case "$1" in
> @@ -72,6 +73,12 @@ create_stash () {
> untracked="$1"
> new_style=t
> ;;
> + --)
> + shift
> + files="$@"
Isn't this the same as writing files="$*", i.e. concatenate the
multiple arguments with the first whitespace in $IFS in between?
> @@ -134,7 +141,7 @@ create_stash () {
> # Untracked files are stored by themselves in a parentless commit, for
> # ease of unpacking later.
> u_commit=$(
> - untracked_files | (
> + untracked_files $files | (
... and this lets it split at $IFS again when passing it down to the
helper. But the helper looks only at $1 so the second and subsequent
ones will be ignored altogether.
This cannot be correct, and any hunk in the remainder of the patch
that mentions $files will be incorrect for the same reason.
Is it possible to carry what the caller (and the end user) gave you
in "$@" without molesting it at all? That would mean you do not
need to introduce $files variable at all, and then the places that
do things like this:
> - create_stash -m "$stash_msg" -u "$untracked"
> + create_stash -m "$stash_msg" -u "$untracked" -- $files
can instead do
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
That would allow you to work correctly with pathspec with $IFS
whitespaces in them.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v2 4/4] stash: support filename argument
2017-01-30 21:11 ` Junio C Hamano
@ 2017-02-05 11:02 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 11:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Add an optional filename argument to git stash push, which allows for
> > stashing a single (or multiple) files.
>
> You can give pathspec with one or more elements, so "an optional
> argument" sounds too limiting.
>
> Allow 'git stash push' to take pathspec to specify which paths
> to stash.
>
> Also retitle
>
> stash: teach 'push' (and 'create') to honor pathspec
>
> or something.
>
> > @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> > only <message> does not trigger this action to prevent a misspelled
> > subcommand from making an unwanted stash.
> > +
> > +If the paths argument is given in 'git stash push', only these files
> > +are put in the new 'stash'. In addition only the indicated files are
> > +changed in the working tree to match the index.
>
> Actually the stash contains "all paths". You could say that you are
> placing _modifications_ to these paths in stash, even though that is
> not how Git's world model works (i.e. everything is a snapshot, and
> modifications are merely difference between two successive
> snapshots). A technically correct version may be:
>
> When pathspec is given to 'git stash push', the new stash
> records the modified states only for the files that match
> the pathspec. The index entries and working tree files are
> then rolled back to the state in HEAD only for these files,
> too, leaving files that do not match the pathspec intact.
>
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 5f08b43967..0072a38b4c 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -41,7 +41,7 @@ no_changes () {
> > untracked_files () {
> > excl_opt=--exclude-standard
> > test "$untracked" = "all" && excl_opt=
> > - git ls-files -o -z $excl_opt
> > + git ls-files -o -z $excl_opt -- $1
>
> Hmph, why "$1" is spelled without dq, implying that it is split at
> $IFS boundary? This line alone makes me suspect that this is not
> prepared to deal correctly with $IFS. Let's read on...
>
> > @@ -59,6 +59,7 @@ create_stash () {
> > stash_msg=
> > untracked=
> > new_style=
> > + files=
> > while test $# != 0
> > do
> > case "$1" in
> > @@ -72,6 +73,12 @@ create_stash () {
> > untracked="$1"
> > new_style=t
> > ;;
> > + --)
> > + shift
> > + files="$@"
>
> Isn't this the same as writing files="$*", i.e. concatenate the
> multiple arguments with the first whitespace in $IFS in between?
>
> > @@ -134,7 +141,7 @@ create_stash () {
> > # Untracked files are stored by themselves in a parentless commit, for
> > # ease of unpacking later.
> > u_commit=$(
> > - untracked_files | (
> > + untracked_files $files | (
>
> ... and this lets it split at $IFS again when passing it down to the
> helper. But the helper looks only at $1 so the second and subsequent
> ones will be ignored altogether.
>
> This cannot be correct, and any hunk in the remainder of the patch
> that mentions $files will be incorrect for the same reason.
>
> Is it possible to carry what the caller (and the end user) gave you
> in "$@" without molesting it at all? That would mean you do not
> need to introduce $files variable at all, and then the places that
> do things like this:
>
> > - create_stash -m "$stash_msg" -u "$untracked"
> > + create_stash -m "$stash_msg" -u "$untracked" -- $files
>
> can instead do
>
> create_stash -m "$stash_msg" -u "$untracked" -- "$@"
>
> That would allow you to work correctly with pathspec with $IFS
> whitespaces in them.
Thanks for taking the time for this explanation! It cleared quite a
few things up in my head. I'll make these fixes in the re-roll.
--
Thomas
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 0/5] stash: support pathspec argument
2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
` (3 preceding siblings ...)
2017-01-29 20:16 ` [PATCH v2 4/4] stash: support filename argument Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
2017-02-05 20:26 ` [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
` (5 more replies)
4 siblings, 6 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
Thanks Junio for the review in the previous round.
Changes since v2:
- $IFS should now be supported by using "$@" everywhere instead of using
a $files variable.
- Added a new patch showing the old behaviour of git stash create is
preserved.
- Rephrased the documentation
- Simplified the option parsing in save_stash, by leaving the
actual parsing to push_stash instead.
Interdiff below:
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 8306bac397..be55e456fa 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,7 +17,7 @@ SYNOPSIS
[-u|--include-untracked] [-a|--all] [<message>]]
'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m|--message <message>]]
- [--] [<paths>...]
+ [--] [<pathspec>...]
'git stash' clear
'git stash' create [<message>]
'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
@@ -51,19 +51,21 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
-push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<paths>...]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
Save your local modifications to a new 'stash' and roll them
- back both in the working tree and in the index.
+ back to HEAD (in the working tree and in the index).
The <message> part is optional and gives
the description along with the stashed state. For quickly making
a snapshot, you can omit _both_ "save" and <message>, but giving
only <message> does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
+
-If the paths argument is given in 'git stash push', only these files
-are put in the new 'stash'. In addition only the indicated files are
-changed in the working tree to match the index.
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec. The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
+
If the `--keep-index` option is used, all changes already added to the
index are left intact.
diff --git a/git-stash.sh b/git-stash.sh
index 0072a38b4c..46367d40aa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt -- $1
+ git ls-files -o -z $excl_opt -- "$@"
}
clear_stash () {
@@ -59,13 +59,12 @@ create_stash () {
stash_msg=
untracked=
new_style=
- files=
while test $# != 0
do
case "$1" in
-m|--message)
shift
- stash_msg="$1"
+ stash_msg=${1?"-m needs an argument"}
new_style=t
;;
-u|--include-untracked)
@@ -75,7 +74,6 @@ create_stash () {
;;
--)
shift
- files="$@"
new_style=t
break
;;
@@ -106,6 +104,10 @@ create_stash () {
if test -z "$new_style"
then
stash_msg="$*"
+ while test $# != 0
+ do
+ shift
+ done
fi
git update-index -q --refresh
@@ -141,7 +143,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files $files | (
+ untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -164,7 +166,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- $files >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -178,7 +180,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- $files &&
+ git add--interactive --patch=stash -- "$@" &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -268,7 +270,7 @@ push_stash () {
;;
-m|--message)
shift
- stash_msg=$1
+ stash_msg=${1?"-m needs an argument"}
;;
--help)
show_help
@@ -300,8 +302,6 @@ push_stash () {
shift
done
- files="$*"
-
if test -n "$patch_mode" && test -n "$untracked"
then
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
@@ -316,14 +316,14 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash -m "$stash_msg" -u "$untracked" -- $files
+ 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")"
if test -z "$patch_mode"
then
- if test -n "$files"
+ if test $# != 0
then
git ls-files -z -- "$@" | xargs -0 git reset --
git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
@@ -334,7 +334,7 @@ push_stash () {
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION -- $files
+ git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
fi
if test "$keep_index" = "t" && test -n "$i_tree"
@@ -357,24 +357,6 @@ save_stash () {
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
;;
@@ -383,20 +365,8 @@ save_stash () {
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
+ # pass all options through to push_stash
+ push_options="$push_options $1"
;;
*)
break
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ca4c44aa9c..461fe46045 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,7 +784,7 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
'
-test_expect_success 'deprecated version of stash create stores correct message' '
+test_expect_success 'create stores correct message' '
>foo &&
git add foo &&
STASH_ID=$(git stash create "create test message") &&
@@ -793,6 +793,15 @@ test_expect_success 'deprecated version of stash create stores correct message'
test_cmp expect actual
'
+test_expect_success 'create with multiple arguments for the message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create test untracked) &&
+ echo "On master: test untracked" >expect &&
+ git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'new style stash create stores correct message' '
>foo &&
git add foo &&
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..9a98e31a3e 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
test -s .gitignore
'
+test_expect_success 'stash push --include-untracked with pathspec' '
+ >foo &&
+ >bar &&
+ git stash push --include-untracked -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file bar &&
+ test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push --include-untracked -- foo* &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_missing foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
Thomas Gummerer (5):
Documentation/stash: remove mention of git reset --hard
stash: introduce push verb
stash: add test for the create command line arguments
stash: introduce new format create
stash: teach 'push' (and 'create') to honor pathspec
Documentation/git-stash.txt | 17 ++++-
git-stash.sh | 124 +++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 78 +++++++++++++++++++++++
t/t3905-stash-include-untracked.sh | 26 ++++++++
4 files changed, 230 insertions(+), 15 deletions(-)
--
2.12.0.rc0.208.g81c5d00b20.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
2017-02-06 15:22 ` Jeff King
2017-02-05 20:26 ` [PATCH v3 2/5] stash: introduce push verb Thomas Gummerer
` (4 subsequent siblings)
5 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
Don't mention git reset --hard in the documentation for git stash save.
It's an implementation detail that doesn't matter to the end user and
thus shouldn't be exposed to them. In addition it's not quite true for
git stash -p, and will not be true when a filename argument to limit the
stash to a few files is introduced.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..2e9e344cd7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -47,8 +47,9 @@ OPTIONS
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
- Save your local modifications to a new 'stash', and run `git reset
- --hard` to revert them. The <message> part is optional and gives
+ Save your local modifications to a new 'stash' and roll them
+ back to HEAD (in the working tree and in the index).
+ The <message> part is optional and gives
the description along with the stashed state. For quickly making
a snapshot, you can omit _both_ "save" and <message>, but giving
only <message> does not trigger this action to prevent a misspelled
--
2.12.0.rc0.208.g81c5d00b20.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard
2017-02-05 20:26 ` [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-02-06 15:22 ` Jeff King
0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2017-02-06 15:22 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sun, Feb 05, 2017 at 08:26:38PM +0000, Thomas Gummerer wrote:
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..2e9e344cd7 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>
> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them. The <message> part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back to HEAD (in the working tree and in the index).
> + The <message> part is optional and gives
Nice. I think what you ended up with here is concise and accurate.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 2/5] stash: introduce push verb
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
2017-02-05 20:26 ` [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
2017-02-06 15:46 ` Jeff King
[not found] ` <vpqlgtaz09q.fsf@anie.imag.fr>
2017-02-05 20:26 ` [PATCH v3 3/5] stash: add test for the create command line arguments Thomas Gummerer
` (3 subsequent siblings)
5 siblings, 2 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
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 given as an argument
to the -m option.
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).
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
git-stash.sh | 46 +++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++++
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..bf7863a846 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?"-m needs an argument"}
+ ;;
--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,39 @@ save_stash () {
fi
}
+save_stash () {
+ push_options=
+ while test $# != 0
+ do
+ case "$1" in
+ --help)
+ show_help
+ ;;
+ --)
+ shift
+ break
+ ;;
+ -*)
+ # pass all options through to push_stash
+ push_options="$push_options $1"
+ ;;
+ *)
+ 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 +653,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.12.0.rc0.208.g81c5d00b20.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 2/5] stash: introduce push verb
2017-02-05 20:26 ` [PATCH v3 2/5] stash: introduce push verb Thomas Gummerer
@ 2017-02-06 15:46 ` Jeff King
2017-02-11 13:33 ` Thomas Gummerer
[not found] ` <vpqlgtaz09q.fsf@anie.imag.fr>
1 sibling, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-06 15:46 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
> + -m|--message)
> + shift
> + stash_msg=${1?"-m needs an argument"}
> + ;;
I think this is our first use of the "?" parameter expansion magic. It
_is_ in POSIX, so it may be fine. We may get complaints from people on
weird shell variants, though. If that's the only reason to avoid it, I'd
be inclined to try it and see, as it is much shorter.
OTOH, most of the other usage errors call usage(), and this one doesn't.
Nor is the error message translatable. Perhaps we should stick to the
longer form (and add a helper function if necessary to reduce the
boilerplate).
> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + --help)
> + show_help
> + ;;
> + --)
> + shift
> + break
> + ;;
> + -*)
> + # pass all options through to push_stash
> + push_options="$push_options $1"
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
I suspect you could just let "--help" get handled in the pass-through
case (it generally takes precedence over errors found in other options,
but I do not see any other parsing errors that could be found by this
loop). It is not too bad to keep it, though (the important thing is that
we're not duplicating all of the push_stash options here).
> + if test -z "$stash_msg"
> + then
> + push_stash $push_options
> + else
> + push_stash $push_options -m "$stash_msg"
> + fi
Hmm. So $push_options is subject to word-splitting here. That's
necessary to split the options back apart. It does the wrong thing if
any of the options had spaces in them. But I don't think there are any
valid options which do so, and "save" would presumably not grow any new
options (they would go straight to "push").
So there is a detectable behavior change:
[before]
$ git stash "--bogus option"
error: unknown option for 'stash save': --bogus option
To provide a message, use git stash save -- '--bogus option'
[etc...]
[after]
$ git stash "--bogus option"
error: unknown option for 'stash save': --bogus
To provide a message, use git stash save -- '--bogus'
but it's probably an acceptable casualty (the "right" way would be to
shell-quote everything you stuff into $push_options and then eval the
result when you invoke push_stash).
Likewise, it's usually a mistake to just stick a new option (like "-m")
after a list of unknown options. But it's OK here because we know we
removed any "--" or non-option arguments.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v3 2/5] stash: introduce push verb
2017-02-06 15:46 ` Jeff King
@ 2017-02-11 13:33 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-11 13:33 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
[sorry for the late responses, life is keeping me busy]
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
>
> > + -m|--message)
> > + shift
> > + stash_msg=${1?"-m needs an argument"}
> > + ;;
>
> I think this is our first use of the "?" parameter expansion magic. It
> _is_ in POSIX, so it may be fine. We may get complaints from people on
> weird shell variants, though. If that's the only reason to avoid it, I'd
> be inclined to try it and see, as it is much shorter.
>
> OTOH, most of the other usage errors call usage(), and this one doesn't.
> Nor is the error message translatable. Perhaps we should stick to the
> longer form (and add a helper function if necessary to reduce the
> boilerplate).
Yeah I do agree that calling usage is the better option here.
> > +save_stash () {
> > + push_options=
> > + while test $# != 0
> > + do
> > + case "$1" in
> > + --help)
> > + show_help
> > + ;;
> > + --)
> > + shift
> > + break
> > + ;;
> > + -*)
> > + # pass all options through to push_stash
> > + push_options="$push_options $1"
> > + ;;
> > + *)
> > + break
> > + ;;
> > + esac
> > + shift
> > + done
>
> I suspect you could just let "--help" get handled in the pass-through
> case (it generally takes precedence over errors found in other options,
> but I do not see any other parsing errors that could be found by this
> loop). It is not too bad to keep it, though (the important thing is that
> we're not duplicating all of the push_stash options here).
Good point, would be good to get rid of that duplication as well.
> > + if test -z "$stash_msg"
> > + then
> > + push_stash $push_options
> > + else
> > + push_stash $push_options -m "$stash_msg"
> > + fi
>
> Hmm. So $push_options is subject to word-splitting here. That's
> necessary to split the options back apart. It does the wrong thing if
> any of the options had spaces in them. But I don't think there are any
> valid options which do so, and "save" would presumably not grow any new
> options (they would go straight to "push").
>
> So there is a detectable behavior change:
>
> [before]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus option
> To provide a message, use git stash save -- '--bogus option'
> [etc...]
>
> [after]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus
> To provide a message, use git stash save -- '--bogus'
>
> but it's probably an acceptable casualty (the "right" way would be to
> shell-quote everything you stuff into $push_options and then eval the
> result when you invoke push_stash).
>
> Likewise, it's usually a mistake to just stick a new option (like "-m")
> after a list of unknown options. But it's OK here because we know we
> removed any "--" or non-option arguments.
>
> -Peff
--
Thomas
^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <vpqlgtaz09q.fsf@anie.imag.fr>]
* Re: [PATCH v3 2/5] stash: introduce push verb
[not found] ` <vpqlgtaz09q.fsf@anie.imag.fr>
@ 2017-02-13 22:16 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-13 22:16 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On 02/13, Matthieu Moy wrote:
> Thomas Gummerer <t.gummerer@gmail.com> 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 given as an argument
> > to the -m option.
>
> Sorry if this has been discussed before, but I find 'push' rather
> confusing here. It took me a while to understand that it meant "opposite
> of pop", because in the context of Git, "push" usually means "send to
> remote repository".
There wasn't much of a discussion about it, but it was pretty much the
only thing that came to my mind, and nobody complained or suggested
anything different, so I just went with it. No other verb came to my
mind yet, but if someone has a better suggestion, I'd be happy to
change.
> Unfortunately, I didn't come up with a better name. "create" is already
> taken ...
>
> Another think to have in mind: changing the option name to break
> backward compatibility is something we can't do often, so if there's
> anything else we should change about the UI, we should do it now. I
> don't have anything particular in mind, just thinking aloud.
Now that you mention this, there actually is one inconsistency that I
introduced, which I shouldn't have. git stash push works with
--include-untracked and --all to decide whether or not to include
untracked files, and if also ignored files should be included. I also
added a --include-untracked {untracked,all} argument to git stash
create, which is clearly inconsistent.
There really should only be one way. I'd be fine with either way, but
I think using --include-untracked and --all is the better choice,
because it's easy to understand, and also makes it easier to switch
git stash without a verb over to use push_stash internally.
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 3/5] stash: add test for the create command line arguments
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
2017-02-05 20:26 ` [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
2017-02-05 20:26 ` [PATCH v3 2/5] stash: introduce push verb Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
2017-02-06 15:50 ` Jeff King
2017-02-05 20:26 ` [PATCH v3 4/5] stash: introduce new format create Thomas Gummerer
` (2 subsequent siblings)
5 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
Currently there is no test showing the expected behaviour of git stash
create's command line arguments. Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
t/t3903-stash.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0171b824c9..21cb70dc74 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
'
+test_expect_success 'create stores correct message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create "create test message") &&
+ echo "On master: create test message" >expect &&
+ git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+ >foo &&
+ git add foo &&
+ STASH_ID=$(git stash create test untracked) &&
+ echo "On master: test untracked" >expect &&
+ git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.12.0.rc0.208.g81c5d00b20.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 3/5] stash: add test for the create command line arguments
2017-02-05 20:26 ` [PATCH v3 3/5] stash: add test for the create command line arguments Thomas Gummerer
@ 2017-02-06 15:50 ` Jeff King
2017-02-11 13:55 ` Thomas Gummerer
0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-06 15:50 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:
> +test_expect_success 'create stores correct message' '
> + >foo &&
> + git add foo &&
> + STASH_ID=$(git stash create "create test message") &&
> + echo "On master: create test message" >expect &&
> + git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> + test_cmp expect actual
> +'
This misses failure exit codes from "git show" because it's on the left
side of a pipe. Perhaps "git show -s" or "git log -1" would get you the
same output without needing "head" (and saving a process and the time
spent generating the diff, as a bonus).
Ditto in the other tests from this patch and later ones.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v3 3/5] stash: add test for the create command line arguments
2017-02-06 15:50 ` Jeff King
@ 2017-02-11 13:55 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-11 13:55 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:
>
> > +test_expect_success 'create stores correct message' '
> > + >foo &&
> > + git add foo &&
> > + STASH_ID=$(git stash create "create test message") &&
> > + echo "On master: create test message" >expect &&
> > + git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> > + test_cmp expect actual
> > +'
>
> This misses failure exit codes from "git show" because it's on the left
> side of a pipe. Perhaps "git show -s" or "git log -1" would get you the
> same output without needing "head" (and saving a process and the time
> spent generating the diff, as a bonus).
>
> Ditto in the other tests from this patch and later ones.
Good catch, will fix.
> -Peff
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 4/5] stash: introduce new format create
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
` (2 preceding siblings ...)
2017-02-05 20:26 ` [PATCH v3 3/5] stash: add test for the create command line arguments Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
2017-02-06 15:56 ` Jeff King
2017-02-05 20:26 ` [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec Thomas Gummerer
2017-02-06 16:14 ` [PATCH v3 0/5] stash: support pathspec argument Jeff King
5 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
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 <t.gummerer@gmail.com>
---
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] [<message>]]
'git stash' clear
'git stash' create [<message>]
+'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 4/5] stash: introduce new format create
2017-02-05 20:26 ` [PATCH v3 4/5] stash: introduce new format create Thomas Gummerer
@ 2017-02-06 15:56 ` Jeff King
2017-02-11 14:51 ` Thomas Gummerer
0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-06 15:56 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sun, Feb 05, 2017 at 08:26:41PM +0000, 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 4/5] stash: introduce new format create
2017-02-06 15:56 ` Jeff King
@ 2017-02-11 14:51 ` Thomas Gummerer
2017-02-13 21:57 ` Jeff King
0 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-11 14:51 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:41PM +0000, 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 4/5] stash: introduce new format create
2017-02-11 14:51 ` Thomas Gummerer
@ 2017-02-13 21:57 ` Jeff King
2017-02-13 23:05 ` Jeff King
0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-13 21:57 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sat, Feb 11, 2017 at 02:51:27PM +0000, 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH v3 4/5] stash: introduce new format create
2017-02-13 21:57 ` Jeff King
@ 2017-02-13 23:05 ` Jeff King
2017-02-14 21:30 ` Thomas Gummerer
0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-13 23:05 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [PATCH v3 4/5] stash: introduce new format create
2017-02-13 23:05 ` Jeff King
@ 2017-02-14 21:30 ` Thomas Gummerer
0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-14 21:30 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
` (3 preceding siblings ...)
2017-02-05 20:26 ` [PATCH v3 4/5] stash: introduce new format create Thomas Gummerer
@ 2017-02-05 20:26 ` Thomas Gummerer
[not found] ` <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
2017-02-06 16:14 ` [PATCH v3 0/5] stash: support pathspec argument Jeff King
5 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-05 20:26 UTC (permalink / raw)
To: git
Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Thomas Gummerer
While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone. Unfortunately
git currently offers no such option. git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.
Allow 'git stash push' to take pathspec to specify which paths to stash.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 11 ++++++++++
git-stash.sh | 30 ++++++++++++++++++++-------
t/t3903-stash.sh | 42 ++++++++++++++++++++++++++++++++++++++
t/t3905-stash-include-untracked.sh | 26 +++++++++++++++++++++++
4 files changed, 102 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index a138ed6a24..be55e456fa 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
'git stash' branch <branchname> [<stash>]
'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
+ [--] [<pathspec>...]
'git stash' clear
'git stash' create [<message>]
'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
+ [-- <paths>...]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
-------
save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
@@ -56,6 +61,12 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
only <message> does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
+
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec. The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
If the `--keep-index` option is used, all changes already added to the
index are left intact.
+
diff --git a/git-stash.sh b/git-stash.sh
index 33b2d0384c..46367d40aa 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -41,7 +41,7 @@ no_changes () {
untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
- git ls-files -o -z $excl_opt
+ git ls-files -o -z $excl_opt -- "$@"
}
clear_stash () {
@@ -72,6 +72,11 @@ create_stash () {
untracked="$1"
new_style=t
;;
+ --)
+ shift
+ new_style=t
+ break
+ ;;
*)
if test -n "$new_style"
then
@@ -99,6 +104,10 @@ create_stash () {
if test -z "$new_style"
then
stash_msg="$*"
+ while test $# != 0
+ do
+ shift
+ done
fi
git update-index -q --refresh
@@ -134,7 +143,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless commit, for
# ease of unpacking later.
u_commit=$(
- untracked_files | (
+ untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -157,7 +166,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
@@ -171,7 +180,7 @@ create_stash () {
# find out what the user wants
GIT_INDEX_FILE="$TMP-index" \
- git add--interactive --patch=stash -- &&
+ git add--interactive --patch=stash -- "$@" &&
# state of the working tree
w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -307,18 +316,25 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
- create_stash -m "$stash_msg" -u "$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")"
if test -z "$patch_mode"
then
- git reset --hard ${GIT_QUIET:+-q}
+ if test $# != 0
+ then
+ git ls-files -z -- "$@" | xargs -0 git reset --
+ git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
+ git ls-files -z --others -- "$@" | xargs -0 git clean --force --
+ else
+ git reset --hard ${GIT_QUIET:+-q}
+ fi
test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
if test -n "$untracked"
then
- git clean --force --quiet -d $CLEAN_X_OPTION
+ git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
fi
if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b859e51086..461fe46045 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -811,4 +811,46 @@ test_expect_success 'new style stash create stores correct message' '
test_cmp expect actual
'
+test_expect_success 'stash -- <filename> stashes and restores the file' '
+ >foo &&
+ >bar &&
+ git add foo bar &&
+ git stash push -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+ >foo &&
+ >bar &&
+ >extra &&
+ git add foo bar extra &&
+ git stash push -- foo bar &&
+ test_path_is_missing bar &&
+ test_path_is_missing foo &&
+ test_path_is_file extra &&
+ git stash pop &&
+ test_path_is_file foo &&
+ test_path_is_file bar &&
+ test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >untracked &&
+ git add foo* &&
+ git stash push -- foo* &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_missing foo &&
+ test_path_is_file untracked &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file untracked
+'
+
test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..9a98e31a3e 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
test -s .gitignore
'
+test_expect_success 'stash push --include-untracked with pathspec' '
+ >foo &&
+ >bar &&
+ git stash push --include-untracked -- foo &&
+ test_path_is_file bar &&
+ test_path_is_missing foo &&
+ git stash pop &&
+ test_path_is_file bar &&
+ test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+ >"foo bar" &&
+ >foo &&
+ >bar &&
+ git add foo* &&
+ git stash push --include-untracked -- foo* &&
+ test_path_is_missing "foo bar" &&
+ test_path_is_missing foo &&
+ test_path_is_file bar &&
+ git stash pop &&
+ test_path_is_file "foo bar" &&
+ test_path_is_file foo &&
+ test_path_is_file bar
+'
+
test_done
--
2.12.0.rc0.208.g81c5d00b20.dirty
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH v3 0/5] stash: support pathspec argument
2017-02-05 20:26 ` [PATCH v3 0/5] stash: support pathspec argument Thomas Gummerer
` (4 preceding siblings ...)
2017-02-05 20:26 ` [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec Thomas Gummerer
@ 2017-02-06 16:14 ` Jeff King
2017-02-12 19:30 ` Thomas Gummerer
5 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-06 16:14 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:
> Thanks Junio for the review in the previous round.
>
> Changes since v2:
>
> - $IFS should now be supported by using "$@" everywhere instead of using
> a $files variable.
> - Added a new patch showing the old behaviour of git stash create is
> preserved.
> - Rephrased the documentation
> - Simplified the option parsing in save_stash, by leaving the
> actual parsing to push_stash instead.
Overall, I like the direction this is heading. I raised a few issues,
the most major of which is whether we want to allow the minor regression
in "git stash create -m foo".
This also makes "git stash push -p <pathspec...>" work, which is good. I
don't think "git stash -p <pathspec...>" does, though. I _think_ it
would be trivial to do on top, since we already consider that case an
error. That's somewhat outside the scope of your series, so I won't
complain (too much :) ) if you don't dig into it, but it might just be
trivial on top.
A few other random bits I noticed while playing with the new code:
$ git init
$ echo content >file && git add file && git commit -m file
$ echo change >file
$ git stash push -p not-file
No changes.
No changes selected
Probably only one of those is necessary. :)
Let's keep trying a few things:
$ git stash push not-file
Saved working directory and index state WIP on master: 5d5f951 file
Unstaged changes after reset:
M file
M file
The unstaged change is mentioned twice, which is weird. But weirder
still is that we created a stash at all, as it's empty. Why didn't we
hit the "no changes selected" path?
And one more:
$ echo foo >untracked
$ git stash push untracked
Saved working directory and index state WIP on master: 5d5f951 file
Unstaged changes after reset:
M file
M file
Removing untracked
We removed the untracked file, even though it wasn't actually stashed! I
thought at first this was because it was mentioned as a pathspec, but it
seems to happen even with a different name:
$ echo foo >untracked
$ git stash push does-not-exist
...
Removing untracked
That seems dangerous.
-Peff
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH v3 0/5] stash: support pathspec argument
2017-02-06 16:14 ` [PATCH v3 0/5] stash: support pathspec argument Jeff King
@ 2017-02-12 19:30 ` Thomas Gummerer
[not found] ` <vpq7f4uxjmo.fsf@anie.imag.fr>
0 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-12 19:30 UTC (permalink / raw)
To: Jeff King
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
Matthieu Moy
[+cc Matthieu Moy as author of a patch mentioned below]
On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:
>
> > Thanks Junio for the review in the previous round.
> >
> > Changes since v2:
> >
> > - $IFS should now be supported by using "$@" everywhere instead of using
> > a $files variable.
> > - Added a new patch showing the old behaviour of git stash create is
> > preserved.
> > - Rephrased the documentation
> > - Simplified the option parsing in save_stash, by leaving the
> > actual parsing to push_stash instead.
>
> Overall, I like the direction this is heading. I raised a few issues,
> the most major of which is whether we want to allow the minor regression
> in "git stash create -m foo".
>
> This also makes "git stash push -p <pathspec...>" work, which is good. I
> don't think "git stash -p <pathspec...>" does, though. I _think_ it
> would be trivial to do on top, since we already consider that case an
> error. That's somewhat outside the scope of your series, so I won't
> complain (too much :) ) if you don't dig into it, but it might just be
> trivial on top.
Hmm good idea, I think it would indeed be nice to add that. Theres a
few things to consider though. First, we need to switch git stash
without arguments over to use git stash push internally. This does
introduce a slight regression as we currently allow git stash save --
-fmessage, (only messages starting with - are allowed). I think that
regression would be acceptable however.
The other question is when we should allow the pathspec argument.
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options") made sure that all option arguments are treated the same. I
think we could use the usual disambiguation of -- to allow pathspecs
after that, so stash -p wouldn't be treated specially, otherwise the
rules could get a bit confusing again.
The other question is how we would deal with the -m flag for
specifying a stash message. I think we could special case it, but
that would allow users to do things such as git stash -m apply, which
would make the interface a bit more error prone. So I'm leaning
towards disallowing that for now.
> A few other random bits I noticed while playing with the new code:
>
> $ git init
> $ echo content >file && git add file && git commit -m file
> $ echo change >file
>
> $ git stash push -p not-file
> No changes.
> No changes selected
>
> Probably only one of those is necessary. :)
>
> Let's keep trying a few things:
>
> $ git stash push not-file
> Saved working directory and index state WIP on master: 5d5f951 file
> Unstaged changes after reset:
> M file
> M file
>
> The unstaged change is mentioned twice, which is weird. But weirder
> still is that we created a stash at all, as it's empty. Why didn't we
> hit the "no changes selected" path?
>
> And one more:
>
> $ echo foo >untracked
> $ git stash push untracked
> Saved working directory and index state WIP on master: 5d5f951 file
> Unstaged changes after reset:
> M file
> M file
> Removing untracked
>
> We removed the untracked file, even though it wasn't actually stashed! I
> thought at first this was because it was mentioned as a pathspec, but it
> seems to happen even with a different name:
>
> $ echo foo >untracked
> $ git stash push does-not-exist
> ...
> Removing untracked
>
> That seems dangerous.
Ouch, yeah this is clearly not good. I'll fix this, and add tests for
these cases.
> -Peff
^ permalink raw reply [flat|nested] 57+ messages in thread