From: Thomas Gummerer <hidden> Date: 2017-01-21 20:08:02
This is the first try to implement the RFC I posted a week ago [1]. It
introduces a new push verb for git stash. I couldn't come up with
any better name that wasn't already taken. If anyone has ideas I'd be
very happy to hear them.
Thanks everyone for your input in the previous thread.
[1]: https://public-inbox.org/git/20170115142542.11999-1-t.gummerer@gmail.com/T/
Thomas Gummerer (3):
Documentation/stash: remove mention of git reset --hard
stash: introduce push verb
stash: support filename argument
Documentation/git-stash.txt | 13 ++++-
git-stash.sh | 123 ++++++++++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 36 +++++++++++++
3 files changed, 159 insertions(+), 13 deletions(-)
--
2.11.0.483.g087da7b7c
From: Thomas Gummerer <hidden> Date: 2017-01-21 20:08:06
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.
Signed-off-by: Thomas Gummerer <redacted>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -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 revert the+ the changes in the working tree to match 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
From: Øyvind A. Holm <hidden> Date: 2017-01-22 01:27:19
On 2017-01-21 20:08:02, Thomas Gummerer wrote:
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.
[...]
+ Save your local modifications to a new 'stash', and revert the
+ the changes in the working tree to match the index.
The patch contains a duplicated "the".
Regards,
Øyvind
+-| Øyvind A. Holm [off-list ref] - N 60.37604° E 5.33339° |-+
| OpenPGP: 0xFB0CBEE894A506E5 - http://www.sunbase.org/pubkey.asc |
| Fingerprint: A006 05D6 E676 B319 55E2 E77E FB0C BEE8 94A5 06E5 |
+------------| 42073b1c-e041-11e6-bae1-db5caa6d21d3 |-------------+
From: Jakub Narębski <hidden> Date: 2017-01-24 19:51:28
W dniu 21.01.2017 o 21:08, Thomas Gummerer pisze:
quoted hunk
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.
Signed-off-by: Thomas Gummerer <redacted>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -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 revert the+ the changes in the working tree to match the index.
I think the following might be better:
..., and set the working tree to match the index.
Or not, as it ignores problem of untracked files.
Anyway, removing internal implementation detail looks like a good idea.
OTOH the reader should be familiar with what `git reset --hard` does,
and if not, he knows where to find the information.
+ 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
@@ -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 revert the+ the changes in the working tree to match the index.+ The <message> part is optional and gives
Hrm. "git reset --hard" doesn't just make the working tree match the
index. It also resets the index to HEAD. So either the original or your
new description is wrong.
I think it's the latter. We really do reset the index unless
--keep-index is specified.
I also wondered if it was worth avoiding the word "revert", as "git
revert" has a much different meaning (as opposed to "svn revert", which
does what you're talking about here). But I see that "git add -i"
already uses the word revert in this way (and there are probably
others).
So it may not be worth worrying about, but "set" or "reset" probably
serves the same purpose.
-Peff
@@ -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 revert the+ the changes in the working tree to match the index.+ The <message> part is optional and gives
Hrm. "git reset --hard" doesn't just make the working tree match the
index. It also resets the index to HEAD. So either the original or your
new description is wrong.
I think it's the latter. We really do reset the index unless
--keep-index is specified.
I wonder if it is worth mentioning that "saving local modifications"
in 'git stash' means storing both the state of the worktree (of tracked
files in it) _and_ the state of the index, before both get set to
state of HEAD.
Best,
--
Jakub Narębski
From: Thomas Gummerer <hidden> Date: 2017-01-21 20:08:09
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 <redacted>
---
git-stash.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++
2 files changed, 87 insertions(+), 3 deletions(-)
@@ -251,8 +256,6 @@ save_stash () {die"$(gettext"Can't use --patch and --include-untracked or --all at the same time")"fi-stash_msg="$*"-gitupdate-index-q--refreshifno_changesthen
@@ -291,6 +294,74 @@ save_stash () {fi}+save_stash(){+push_options=+whiletest$#!=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+Toprovideamessage,usegitstashsave--'\$option'"+usage+;;+*)+break+;;+esac+shift+done++# if test -n "$patch_mode" && test -n "$untracked"+# then+# die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"+# fi++stash_msg="$*"++iftest-zstash_msg+then+push_stash$push_options+else+push_stash$push_options-m"$stash_msg"+fi+}+ have_stash(){gitrev-parse--verify--quiet$ref_stash>/dev/null}
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' 'test_path_is_missingfile'+test_expect_success'push -m shows right message''+>foo&&+gitaddfoo&&+gitstashpush-m"test message"&&+echo"stash@{0}: On master: test message">expect&&+gitstashlist|head-n1>actual&&+test_cmpexpectactual+'+ test_done
From: Thomas Gummerer <hidden> Date: 2017-01-21 20:08:12
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 <redacted>
---
Documentation/git-stash.txt | 8 ++++++++
git-stash.sh | 42 ++++++++++++++++++++++++++++++++++--------
t/t3903-stash.sh | 27 +++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 8 deletions(-)
@@ -46,6 +49,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 revert the the changes in the working tree to match the index.
@@ -55,6 +59,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. +
@@ -92,7 +109,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"&&exportGIT_INDEX_FILE&&rm-f"$TMPindex"&&
@@ -129,7 +146,7 @@ create_stash () {# find out what the user wantsGIT_INDEX_FILE="$TMP-index"\-gitadd--interactive--patch=stash--&&+gitadd--interactive--patch=stash--$files&&# state of the working treew_tree=$(GIT_INDEX_FILE="$TMP-index"gitwrite-tree)||
@@ -251,6 +268,8 @@ push_stash () {shiftdone+files="$*"+iftest-n"$patch_mode"&&test-n"$untracked"thendie"$(gettext"Can't use --patch and --include-untracked or --all at the same time")"
@@ -265,18 +284,25 @@ push_stash () {gitreflogexists$ref_stash||clear_stash||die"$(gettext"Cannot initialize stash")"-create_stash"$stash_msg"$untracked+create_stash--files$files--"$stash_msg""$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")"iftest-z"$patch_mode"then-gitreset--hard${GIT_QUIET:+-q}+iftest-n"$files"+then+gitreset--$files+gitcheckoutHEAD--$(gitls-files--modified--$files)+gitclean--force--quiet--$(gitls-files--others--$files)+else+gitreset--hard${GIT_QUIET:+-q}+fitest"$untracked"="all"&&CLEAN_X_OPTION=-x||CLEAN_X_OPTION=iftest-n"$untracked"then-gitclean--force--quiet-d$CLEAN_X_OPTION+gitclean--force--quiet-d$CLEAN_X_OPTION--$filesfiiftest"$keep_index"="t"&&test-n"$i_tree"
From: Johannes Schindelin <hidden> Date: 2017-01-24 10:57:15
Hi Thomas,
On Sat, 21 Jan 2017, Thomas Gummerer wrote:
This is the first try to implement the RFC I posted a week ago [1]. It
introduces a new push verb for git stash. I couldn't come up with
any better name that wasn't already taken. If anyone has ideas I'd be
very happy to hear them.
I would have preferred a series of patches that essentially adds a new and
improved `save` syntax:
git stash [save] [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m <message>]]
[-- <path>...]
and keeps the legacy syntax, but deprecates it:
git stash [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [<message>]]
The problem with that is, of course, that 3c2eb80fe3 (stash: simplify
defaulting to "save" and reject unknown options, 2009-08-18) in its
infinite wisdom *already* introduced the `--` separator to drop out of
option parsing.
On a positive note, it is a thorn in Git's CUI that `git stash` implies
the `save` command, and that `save` is not at all the opposite of `apply`
or `pop`. Your introduction of the `push` command will fix that flaw, and
we can *still* deprecate the `save` command.
Ciao,
Johannes
From: Thomas Gummerer <hidden> Date: 2017-01-29 20:24:58
Previous round is at:
http://public-inbox.org/git/20170121200804.19009-1-t.gummerer@gmail.com/.
Thanks Junio, Peff, Øyvind, Jakub and Johannes for your feedback on
the previous round.
Changes since the previous round:
- Re-phrased the Documentation update.
- Added missing $ in 2/3
- Added an extra patch introducing a new syntax for git stash create,
where the message can be specified with the -m flag, instead of as a
positional argument
- Filenames with $IFS in their name are now supported. Added a test
for that as well.
Interdiff below:
@@ -51,8 +53,8 @@ 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 revert the- the changes in the working tree to match the index.+ 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
@@ -56,25 +56,57 @@ clear_stash () {} create_stash(){+stash_msg=+untracked=+new_style=files=whiletest$#!=0docase"$1"in+-m|--message)+shift+stash_msg="$1"+new_style=t+;;+-u|--include-untracked)+shift+untracked="$1"+new_style=t+;;--)shift+files="$@"+new_style=tbreak;;---files)-;;*)-files="$1$files"+iftest-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;;esacshiftdone-stash_msg="$1"-untracked="$2"+iftest-z"$new_style"+then+stash_msg="$*"+figitupdate-index-q--refreshifno_changes
@@ -284,7 +316,7 @@ push_stash () {gitreflogexists$ref_stash||clear_stash||die"$(gettext"Cannot initialize stash")"-create_stash--files$files--"$stash_msg""$untracked"+create_stash-m"$stash_msg"-u"$untracked"--$filesstore_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")"
@@ -373,14 +405,9 @@ save_stash () {shiftdone-# if test -n "$patch_mode" && test -n "$untracked"-# then-# die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"-# fi-stash_msg="$*"-iftest-zstash_msg+iftest-z"$stash_msg"thenpush_stash$push_optionselse
@@ -784,6 +784,24 @@ test_expect_success 'push -m shows right message' 'test_cmpexpectactual'+test_expect_success'deprecated version of stash create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate"create test message")&&+echo"On master: create test message">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'++test_expect_success'new style stash create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate-m"create test message new style")&&+echo"On master: create test message new style">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'+ test_expect_success'stash -- <filename> stashes and restores the file''>foo&&>bar&&
@@ -811,4 +829,19 @@ test_expect_success 'stash with multiple filename arguments' 'test_path_is_fileextra'+test_expect_success'stash with file including $IFS character''+>"foo bar"&&+>foo&&+>untracked&&+gitaddfoo*&&+gitstashpush--foo*&&+test_path_is_missing"foo bar"&&+test_path_is_missingfoo&&+test_path_is_fileuntracked&&+gitstashpop&&+test_path_is_file"foo bar"&&+test_path_is_filefoo&&+test_path_is_fileuntracked+'+ test_done
Thomas Gummerer (4):
Documentation/stash: remove mention of git reset --hard
stash: introduce push verb
introduce new format for git stash create
stash: support filename argument
Documentation/git-stash.txt | 14 +++-
git-stash.sh | 154 ++++++++++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 69 ++++++++++++++++++++
3 files changed, 222 insertions(+), 15 deletions(-)
--
2.11.0.297.g9a2118ac0b.dirty
From: Thomas Gummerer <hidden> Date: 2017-01-29 20:23:07
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 <redacted>
---
Documentation/git-stash.txt | 1 +
git-stash.sh | 50 +++++++++++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 18 ++++++++++++++++
3 files changed, 65 insertions(+), 4 deletions(-)
@@ -56,8 +56,50 @@ clear_stash () {} create_stash(){-stash_msg="$1"-untracked="$2"+stash_msg=+untracked=+new_style=+whiletest$#!=0+do+case"$1"in+-m|--message)+shift+stash_msg="$1"+new_style=t+;;+-u|--include-untracked)+shift+untracked="$1"+new_style=t+;;+*)+iftest-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++iftest-z"$new_style"+then+stash_msg="$*"+figitupdate-index-q--refreshifno_changes
@@ -265,7 +307,7 @@ push_stash () {gitreflogexists$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")"
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' 'test_cmpexpectactual'+test_expect_success'deprecated version of stash create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate"create test message")&&+echo"On master: create test message">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'++test_expect_success'new style stash create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate-m"create test message new style")&&+echo"On master: create test message new style">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'+ test_done
From: Thomas Gummerer <hidden> Date: 2017-01-29 20:25:14
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 <redacted>
---
git-stash.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++
2 files changed, 82 insertions(+), 3 deletions(-)
@@ -251,8 +256,6 @@ save_stash () {die"$(gettext"Can't use --patch and --include-untracked or --all at the same time")"fi-stash_msg="$*"-gitupdate-index-q--refreshifno_changesthen
@@ -291,6 +294,69 @@ save_stash () {fi}+save_stash(){+push_options=+whiletest$#!=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+Toprovideamessage,usegitstashsave--'\$option'"+usage+;;+*)+break+;;+esac+shift+done++stash_msg="$*"++iftest-z"$stash_msg"+then+push_stash$push_options+else+push_stash$push_options-m"$stash_msg"+fi+}+ have_stash(){gitrev-parse--verify--quiet$ref_stash>/dev/null}
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' 'test_path_is_missingfile'+test_expect_success'push -m shows right message''+>foo&&+gitaddfoo&&+gitstashpush-m"test message"&&+echo"stash@{0}: On master: test message">expect&&+gitstashlist|head-n1>actual&&+test_cmpexpectactual+'+ test_done
From: Thomas Gummerer <hidden> Date: 2017-01-29 20:25:17
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 <redacted>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -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
From: Thomas Gummerer <hidden> Date: 2017-01-29 20:47:25
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 <redacted>
---
Documentation/git-stash.txt | 9 +++++++++
git-stash.sh | 30 +++++++++++++++++++++++-------
t/t3903-stash.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 74 insertions(+), 7 deletions(-)
@@ -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. +
@@ -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"&&exportGIT_INDEX_FILE&&rm-f"$TMPindex"&&
@@ -171,7 +178,7 @@ create_stash () {# find out what the user wantsGIT_INDEX_FILE="$TMP-index"\-gitadd--interactive--patch=stash--&&+gitadd--interactive--patch=stash--$files&&# state of the working treew_tree=$(GIT_INDEX_FILE="$TMP-index"gitwrite-tree)||
@@ -293,6 +300,8 @@ push_stash () {shiftdone+files="$*"+iftest-n"$patch_mode"&&test-n"$untracked"thendie"$(gettext"Can't use --patch and --include-untracked or --all at the same time")"
@@ -307,18 +316,25 @@ push_stash () {gitreflogexists$ref_stash||clear_stash||die"$(gettext"Cannot initialize stash")"-create_stash-m"$stash_msg"-u"$untracked"+create_stash-m"$stash_msg"-u"$untracked"--$filesstore_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")"iftest-z"$patch_mode"then-gitreset--hard${GIT_QUIET:+-q}+iftest-n"$files"+then+gitls-files-z--"$@"|xargs-0gitreset--+gitls-files-z--modified--"$@"|xargs-0gitcheckoutHEAD--+gitls-files-z--others--"$@"|xargs-0gitclean--force--+else+gitreset--hard${GIT_QUIET:+-q}+fitest"$untracked"="all"&&CLEAN_X_OPTION=-x||CLEAN_X_OPTION=iftest-n"$untracked"then-gitclean--force--quiet-d$CLEAN_X_OPTION+gitclean--force--quiet-d$CLEAN_X_OPTION--$filesfiiftest"$keep_index"="t"&&test-n"$i_tree"
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:33
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:
@@ -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.
@@ -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"&&exportGIT_INDEX_FILE&&rm-f"$TMPindex"&&
@@ -178,7 +180,7 @@ create_stash () {# find out what the user wantsGIT_INDEX_FILE="$TMP-index"\-gitadd--interactive--patch=stash--$files&&+gitadd--interactive--patch=stash--"$@"&&# state of the working treew_tree=$(GIT_INDEX_FILE="$TMP-index"gitwrite-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 () {shiftdone-files="$*"-iftest-n"$patch_mode"&&test-n"$untracked"thendie"$(gettext"Can't use --patch and --include-untracked or --all at the same time")"
@@ -316,14 +316,14 @@ push_stash () {gitreflogexists$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")"iftest-z"$patch_mode"then-iftest-n"$files"+iftest$#!=0thengitls-files-z--"$@"|xargs-0gitreset--gitls-files-z--modified--"$@"|xargs-0gitcheckoutHEAD--
@@ -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-Toprovideamessage,usegitstashsave--'\$option'"-usage+# pass all options through to push_stash+push_options="$push_options$1";;*)break
@@ -784,7 +784,7 @@ test_expect_success 'push -m shows right message' 'test_cmpexpectactual'-test_expect_success'deprecated version of stash create stores correct message''+test_expect_success'create stores correct message''>foo&&gitaddfoo&&STASH_ID=$(gitstashcreate"create test message")&&
@@ -793,6 +793,15 @@ test_expect_success 'deprecated version of stash create stores correct message'test_cmpexpectactual'+test_expect_success'create with multiple arguments for the message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreatetestuntracked)&&+echo"On master: test untracked">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'+ test_expect_success'new style stash create stores correct message''>foo&&gitaddfoo&&
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:26
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 [off-list ref]
Signed-off-by: Thomas Gummerer <redacted>
---
git-stash.sh | 46 +++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++++
2 files changed, 52 insertions(+), 3 deletions(-)
@@ -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="$*"-gitupdate-index-q--refreshifno_changesthen
@@ -291,6 +294,39 @@ save_stash () {fi}+save_stash(){+push_options=+whiletest$#!=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="$*"++iftest-z"$stash_msg"+then+push_stash$push_options+else+push_stash$push_options-m"$stash_msg"+fi+}+ have_stash(){gitrev-parse--verify--quiet$ref_stash>/dev/null}
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' 'test_path_is_missingfile'+test_expect_success'push -m shows right message''+>foo&&+gitaddfoo&&+gitstashpush-m"test message"&&+echo"stash@{0}: On master: test message">expect&&+gitstashlist|head-n1>actual&&+test_cmpexpectactual+'+ test_done
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
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.
quoted
+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.
quoted
+ 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
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:30
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 <redacted>
---
Documentation/git-stash.txt | 1 +
git-stash.sh | 50 +++++++++++++++++++++++++++++++++++++++++----
t/t3903-stash.sh | 9 ++++++++
3 files changed, 56 insertions(+), 4 deletions(-)
@@ -56,8 +56,50 @@ clear_stash () {} create_stash(){-stash_msg="$1"-untracked="$2"+stash_msg=+untracked=+new_style=+whiletest$#!=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+;;+*)+iftest-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++iftest-z"$new_style"+then+stash_msg="$*"+figitupdate-index-q--refreshifno_changes
@@ -265,7 +307,7 @@ push_stash () {gitreflogexists$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")"
@@ -802,4 +802,13 @@ test_expect_success 'create with multiple arguments for the message' 'test_cmpexpectactual'+test_expect_success'new style stash create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate-m"create test message new style")&&+echo"On master: create test message new style">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'+ test_done
From: Jeff King <hidden> Date: 2017-02-06 15:56:13
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
From: Thomas Gummerer <hidden> Date: 2017-02-11 14:51:01
On 02/06, Jeff King wrote:
On Sun, Feb 05, 2017 at 08:26:41PM +0000, Thomas Gummerer wrote:
quoted
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.
From: Jeff King <hidden> Date: 2017-02-13 21:57:42
On Sat, Feb 11, 2017 at 02:51:27PM +0000, Thomas Gummerer wrote:
quoted
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
From: Jeff King <hidden> Date: 2017-02-13 23:05:41
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.
quoted
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:
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
From: Thomas Gummerer <hidden> Date: 2017-02-14 21:30:12
On 02/13, Jeff King wrote:
quoted hunk
On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:
quoted
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.
quoted
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:
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!
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:36
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 <redacted>
---
t/t3903-stash.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' 'test_cmpexpectactual'+test_expect_success'create stores correct message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreate"create test message")&&+echo"On master: create test message">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'++test_expect_success'create with multiple arguments for the message''+>foo&&+gitaddfoo&&+STASH_ID=$(gitstashcreatetestuntracked)&&+echo"On master: test untracked">expect&&+gitshow--pretty=%s${STASH_ID}|head-n1>actual&&+test_cmpexpectactual+'+ test_done
From: Jeff King <hidden> Date: 2017-02-06 15:50:19
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
From: Thomas Gummerer <hidden> Date: 2017-02-11 13:55:23
On 02/06, Jeff King wrote:
On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:
quoted
+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.
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:38
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 <redacted>
---
Documentation/git-stash.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -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
@@ -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
From: Thomas Gummerer <hidden> Date: 2017-02-05 20:26:42
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 [off-list ref]
Signed-off-by: Thomas Gummerer <redacted>
---
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(-)
@@ -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. +
@@ -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"&&exportGIT_INDEX_FILE&&rm-f"$TMPindex"&&
@@ -171,7 +180,7 @@ create_stash () {# find out what the user wantsGIT_INDEX_FILE="$TMP-index"\-gitadd--interactive--patch=stash--&&+gitadd--interactive--patch=stash--"$@"&&# state of the working treew_tree=$(GIT_INDEX_FILE="$TMP-index"gitwrite-tree)||
@@ -307,18 +316,25 @@ push_stash () {gitreflogexists$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")"iftest-z"$patch_mode"then-gitreset--hard${GIT_QUIET:+-q}+iftest$#!=0+then+gitls-files-z--"$@"|xargs-0gitreset--+gitls-files-z--modified--"$@"|xargs-0gitcheckoutHEAD--+gitls-files-z--others--"$@"|xargs-0gitclean--force--+else+gitreset--hard${GIT_QUIET:+-q}+fitest"$untracked"="all"&&CLEAN_X_OPTION=-x||CLEAN_X_OPTION=iftest-n"$untracked"then-gitclean--force--quiet-d$CLEAN_X_OPTION+gitclean--force--quiet-d$CLEAN_X_OPTION--"$@"fiiftest"$keep_index"="t"&&test-n"$i_tree"
From: Jeff King <hidden> Date: 2017-02-06 16:14:40
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
From: Thomas Gummerer <hidden> Date: 2017-02-12 19:30:26
[+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:
quoted
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.