git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] stash: support filename argument
@ 2017-01-21 20:08 Thomas Gummerer
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
                   ` (4 more replies)
  0 siblings, 5 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-21 20:08 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, Thomas Gummerer

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


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
@ 2017-01-21 20:08 ` Thomas Gummerer
  2017-01-22  1:27   ` Øyvind A. Holm
                     ` (2 more replies)
  2017-01-21 20:08 ` [PATCH 2/3] stash: introduce push verb Thomas Gummerer
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-21 20:08 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, 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.

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..0ad5335a3e 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 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
-- 
2.11.0.483.g087da7b7c


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 2/3] stash: introduce push verb
  2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-01-21 20:08 ` Thomas Gummerer
  2017-01-23 18:27   ` Junio C Hamano
  2017-01-21 20:08 ` [PATCH 3/3] stash: support filename argument Thomas Gummerer
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-21 20:08 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, 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     | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 t/t3903-stash.sh |  9 +++++++
 2 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..d6b4ae3290 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,74 @@ 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
+
+	# 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="$*"
+
+	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 +688,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.483.g087da7b7c


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 3/3] stash: support filename argument
  2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
  2017-01-21 20:08 ` [PATCH 2/3] stash: introduce push verb Thomas Gummerer
@ 2017-01-21 20:08 ` Thomas Gummerer
  2017-01-23 18:50   ` Junio C Hamano
  2017-01-24 10:56 ` [PATCH 0/3] " Johannes Schindelin
  2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
  4 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-21 20:08 UTC (permalink / raw)
  To: git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin, 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 |  8 ++++++++
 git-stash.sh                | 42 ++++++++++++++++++++++++++++++++++--------
 t/t3903-stash.sh            | 27 +++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 0ad5335a3e..871a3b246c 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,6 +15,9 @@ 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' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -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.
 +
diff --git a/git-stash.sh b/git-stash.sh
index d6b4ae3290..7dcce629bd 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 () {
@@ -56,6 +56,23 @@ clear_stash () {
 }
 
 create_stash () {
+	files=
+	while test $# != 0
+	do
+		case "$1" in
+		--)
+			shift
+			break
+			;;
+		--files)
+			;;
+		*)
+			files="$1 $files"
+			;;
+		esac
+		shift
+	done
+
 	stash_msg="$1"
 	untracked="$2"
 
@@ -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" &&
 				export GIT_INDEX_FILE &&
 				rm -f "$TMPindex" &&
@@ -115,7 +132,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"
@@ -129,7 +146,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) ||
@@ -251,6 +268,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")"
@@ -265,18 +284,25 @@ push_stash () {
 	git reflog exists $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")"
 
 	if test -z "$patch_mode"
 	then
-		git reset --hard ${GIT_QUIET:+-q}
+		if test -n "$files"
+		then
+			git reset -- $files
+			git checkout HEAD -- $(git ls-files --modified -- $files)
+			git clean --force --quiet -- $(git ls-files --others -- $files)
+		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"
@@ -702,7 +728,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..3e763ff766 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,31 @@ test_expect_success 'push -m shows right 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_done
-- 
2.11.0.483.g087da7b7c


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
@ 2017-01-22  1:27   ` Øyvind A. Holm
  2017-01-24 19:51   ` Jakub Narębski
  2017-01-24 20:14   ` Jeff King
  2 siblings, 0 replies; 57+ messages in thread
From: Øyvind A. Holm @ 2017-01-22  1:27 UTC (permalink / raw)
  To: Thomas Gummerer; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

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 <sunny@sunbase.org> - 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 |-------------+

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/3] stash: introduce push verb
  2017-01-21 20:08 ` [PATCH 2/3] stash: introduce push verb Thomas Gummerer
@ 2017-01-23 18:27   ` Junio C Hamano
  2017-01-29 12:39     ` Thomas Gummerer
  0 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2017-01-23 18:27 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin

Thomas Gummerer <t.gummerer@gmail.com> writes:

> +	stash_msg="$*"
> +
> +	if test -z stash_msg

A dollar-sign is missing here, I think.

> +	then
> +		push_stash $push_options
> +	else
> +		push_stash $push_options -m "$stash_msg"
> +	fi
> +}

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 3/3] stash: support filename argument
  2017-01-21 20:08 ` [PATCH 3/3] stash: support filename argument Thomas Gummerer
@ 2017-01-23 18:50   ` Junio C Hamano
  2017-01-29 13:29     ` Thomas Gummerer
  0 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2017-01-23 18:50 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin

Thomas Gummerer <t.gummerer@gmail.com> writes:

> diff --git a/git-stash.sh b/git-stash.sh
> index d6b4ae3290..7dcce629bd 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

Does $1 need to be quoted to prevent it from split at $IFS?

> @@ -56,6 +56,23 @@ clear_stash () {
>  }
>  
>  create_stash () {
> +	files=
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		--)
> +			shift
> +			break
> +			;;
> +		--files)
> +			;;
> +		*)
> +			files="$1 $files"
> +			;;

Hmph.  What is this "no-op" option about?  Did you mean to say
something like this instead?

	case "$1" in
	...
	--file)
		case $# in
		1)
			die "--file needs a pathspec" ;;
		*)
			shift
			files="$files$1 " ;;
		esac
		;;


Another thing I noticed.  We won't support filenames with embedded
$IFS characters at all?

I somehow had an impression that the script was carefully done
(e.g. by using -z option where appropriate) to add such a
limitation.

Perhaps we have broken it over time and it no longer matters
(i.e. there already may be existing breakages), but this troubles
me somehow.

By the way, in addition to "push" thing that corrects the argument
convention by requiring "-m" before the message, we need to correct
create_stash that is used internally from "stash push" somehow?

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 0/3] stash: support filename argument
  2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
                   ` (2 preceding siblings ...)
  2017-01-21 20:08 ` [PATCH 3/3] stash: support filename argument Thomas Gummerer
@ 2017-01-24 10:56 ` Johannes Schindelin
  2017-01-29 20:16 ` [PATCH v2 0/4] stash: create " Thomas Gummerer
  4 siblings, 0 replies; 57+ messages in thread
From: Johannes Schindelin @ 2017-01-24 10:56 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King

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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
  2017-01-22  1:27   ` Øyvind A. Holm
@ 2017-01-24 19:51   ` Jakub Narębski
  2017-01-24 20:14   ` Jeff King
  2 siblings, 0 replies; 57+ messages in thread
From: Jakub Narębski @ 2017-01-24 19:51 UTC (permalink / raw)
  To: Thomas Gummerer, git
  Cc: Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
	Johannes Schindelin

W dniu 21.01.2017 o 21:08, Thomas Gummerer pisze:
> 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 <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..0ad5335a3e 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 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
> 

-- 
Jakub Narębski

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
  2017-01-22  1:27   ` Øyvind A. Holm
  2017-01-24 19:51   ` Jakub Narębski
@ 2017-01-24 20:14   ` Jeff King
  2017-01-25 13:02     ` Jakub Narębski
  2017-01-25 21:20     ` Junio C Hamano
  2 siblings, 2 replies; 57+ messages in thread
From: Jeff King @ 2017-01-24 20:14 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz, Johannes Schindelin

On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..0ad5335a3e 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 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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-24 20:14   ` Jeff King
@ 2017-01-25 13:02     ` Jakub Narębski
  2017-01-25 21:20       ` Junio C Hamano
  2017-01-25 21:20     ` Junio C Hamano
  1 sibling, 1 reply; 57+ messages in thread
From: Jakub Narębski @ 2017-01-25 13:02 UTC (permalink / raw)
  To: Jeff King, Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz, Johannes Schindelin

W dniu 24.01.2017 o 21:14, Jeff King pisze:
> On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:
> 
>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> index 2e9cef06e6..0ad5335a3e 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 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

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-24 20:14   ` Jeff King
  2017-01-25 13:02     ` Jakub Narębski
@ 2017-01-25 21:20     ` Junio C Hamano
  2017-01-28 19:30       ` Thomas Gummerer
  1 sibling, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2017-01-25 21:20 UTC (permalink / raw)
  To: Jeff King
  Cc: Thomas Gummerer, git, Stephan Beyer, Marc Strapetz, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:
>
>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> index 2e9cef06e6..0ad5335a3e 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 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.

Correct.  So the patch is a net loss.  Perhaps not requiring readers
to know "reset --hard" might be an improvement (I personally do not
think so), but this loses crucial information from the description.

	Save your local modifications (both in the working tree and
	to the index) to a new 'stash', and resets the index to HEAD
	and makes the working tree match the index (i.e. runs "git
	reset --hard").

That's one-and-a-half lines longer than the original, though.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-25 13:02     ` Jakub Narębski
@ 2017-01-25 21:20       ` Junio C Hamano
  0 siblings, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2017-01-25 21:20 UTC (permalink / raw)
  To: Jakub Narębski
  Cc: Jeff King, Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
	Johannes Schindelin

Jakub Narębski <jnareb@gmail.com> writes:

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

Yes, it is, I would say.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-25 21:20     ` Junio C Hamano
@ 2017-01-28 19:30       ` Thomas Gummerer
  2017-01-28 23:54         ` Jeff King
  0 siblings, 1 reply; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-28 19:30 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, git, Stephan Beyer, Marc Strapetz, Johannes Schindelin

On 01/25, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:
> >
> >> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> >> index 2e9cef06e6..0ad5335a3e 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 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.
> 
> Correct.  So the patch is a net loss.  Perhaps not requiring readers
> to know "reset --hard" might be an improvement (I personally do not
> think so), but this loses crucial information from the description.
> 
> 	Save your local modifications (both in the working tree and
> 	to the index) to a new 'stash', and resets the index to HEAD
> 	and makes the working tree match the index (i.e. runs "git
> 	reset --hard").
> 
> That's one-and-a-half lines longer than the original, though.

Thanks all who chimed in here.  My new description is definitely not
right.  The reason I wanted to change it is part because it's an
implementation detail, and part because it's going to be not quite
right when the filename argument is introduced.

How about the following:

 	Save your local modifications to a new 'stash' and roll them back
 	both in the working tree and in the index.

As an added bonus this also matches what git stash save -p does.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
  2017-01-28 19:30       ` Thomas Gummerer
@ 2017-01-28 23:54         ` Jeff King
  0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2017-01-28 23:54 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: Junio C Hamano, git, Stephan Beyer, Marc Strapetz, Johannes Schindelin

On Sat, Jan 28, 2017 at 07:30:28PM +0000, Thomas Gummerer wrote:

> Thanks all who chimed in here.  My new description is definitely not
> right.  The reason I wanted to change it is part because it's an
> implementation detail, and part because it's going to be not quite
> right when the filename argument is introduced.
> 
> How about the following:
> 
>  	Save your local modifications to a new 'stash' and roll them back
>  	both in the working tree and in the index.
> 
> As an added bonus this also matches what git stash save -p does.

IMHO that is both informative and accurate. You could add:
 
  (unless --keep-index was used)

at the end of the sentence, though I am not sure it is necessary.

-Peff

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/3] stash: introduce push verb
  2017-01-23 18:27   ` Junio C Hamano
@ 2017-01-29 12:39     ` Thomas Gummerer
  0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 12:39 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin

On 01/23, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > +	stash_msg="$*"
> > +
> > +	if test -z stash_msg
> 
> A dollar-sign is missing here, I think.

Yes, thanks.

> > +	then
> > +		push_stash $push_options
> > +	else
> > +		push_stash $push_options -m "$stash_msg"
> > +	fi
> > +}

-- 
Thomas

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 3/3] stash: support filename argument
  2017-01-23 18:50   ` Junio C Hamano
@ 2017-01-29 13:29     ` Thomas Gummerer
  0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-01-29 13:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin

On 01/23, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > diff --git a/git-stash.sh b/git-stash.sh
> > index d6b4ae3290..7dcce629bd 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
> 
> Does $1 need to be quoted to prevent it from split at $IFS?

Not sure, I'll check and add a test for the re-roll.

> > @@ -56,6 +56,23 @@ clear_stash () {
> >  }
> >  
> >  create_stash () {
> > +	files=
> > +	while test $# != 0
> > +	do
> > +		case "$1" in
> > +		--)
> > +			shift
> > +			break
> > +			;;
> > +		--files)
> > +			;;
> > +		*)
> > +			files="$1 $files"
> > +			;;
> 
> Hmph.  What is this "no-op" option about?  Did you mean to say
> something like this instead?
> 
> 	case "$1" in
> 	...
> 	--file)
> 		case $# in
> 		1)
> 			die "--file needs a pathspec" ;;
> 		*)
> 			shift
> 			files="$files$1 " ;;
> 		esac
> 		;;
>

Hmm that would require multiple --file arguments to create_stash,
which I wanted to avoid.  But probably the correct solution is to
introduce a new format for create_stash, which allows a -m before the
message, and uses -- to disambiguate before the file name arguments.

This would be similar to what Johannes suggested in [1], deprecating
the old syntax in git stash create.  While this didn't work in git
stash save, it would work in git stash create, as "--" isn't used to
disambiguate anything there yet.

> Another thing I noticed.  We won't support filenames with embedded
> $IFS characters at all?
> 
> I somehow had an impression that the script was carefully done
> (e.g. by using -z option where appropriate) to add such a
> limitation.
> 
> Perhaps we have broken it over time and it no longer matters
> (i.e. there already may be existing breakages), but this troubles
> me somehow.

Good point, I didn't think about $IFS characters.  Fill fix in the
next round.

> By the way, in addition to "push" thing that corrects the argument
> convention by requiring "-m" before the message, we need to correct
> create_stash that is used internally from "stash push" somehow?

Yeah, I think that would make sense, see above.

[1]: http://public-inbox.org/git/alpine.DEB.2.20.1701241148300.3469@virtualbox/

^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH v2 0/4] stash: create filename argument
  2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
                   ` (3 preceding siblings ...)
  2017-01-24 10:56 ` [PATCH 0/3] " Johannes Schindelin
@ 2017-01-29 20:16 ` Thomas Gummerer
  2017-01-29 20:16   ` [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
                     ` (4 more replies)
  4 siblings, 5 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

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:

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 871a3b246c..8306bac397 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -20,6 +20,8 @@ SYNOPSIS
 	     [--] [<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
@@ -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
diff --git a/git-stash.sh b/git-stash.sh
index 7dcce629bd..0072a38b4c 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -56,25 +56,57 @@ clear_stash () {
 }
 
 create_stash () {
+	stash_msg=
+	untracked=
+	new_style=
 	files=
 	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
+			;;
 		--)
 			shift
+			files="$@"
+			new_style=t
 			break
 			;;
-		--files)
-			;;
 		*)
-			files="$1 $files"
+			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
 
-	stash_msg="$1"
-	untracked="$2"
+	if test -z "$new_style"
+	then
+		stash_msg="$*"
+	fi
 
 	git update-index -q --refresh
 	if no_changes
@@ -284,7 +316,7 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash --files $files -- "$stash_msg" "$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")"
@@ -293,9 +325,9 @@ push_stash () {
 	then
 		if test -n "$files"
 		then
-			git reset -- $files
-			git checkout HEAD -- $(git ls-files --modified -- $files)
-			git clean --force --quiet -- $(git ls-files --others -- $files)
+			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
@@ -373,14 +405,9 @@ save_stash () {
 		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="$*"
 
-	if test -z stash_msg
+	if test -z "$stash_msg"
 	then
 		push_stash $push_options
 	else
@@ -728,7 +755,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 3e763ff766..ca4c44aa9c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,6 +784,24 @@ 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_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_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

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


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [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

* [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

* [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

* [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 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

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

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

* 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

* 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

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

* [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

* [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

* [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

* [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 5/5] stash: teach 'push' (and 'create') to honor pathspec
       [not found]       ` <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
@ 2017-02-06 15:20         ` Jeff King
  2017-02-11 16:50         ` Thomas Gummerer
  1 sibling, 0 replies; 57+ messages in thread
From: Jeff King @ 2017-02-06 15:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski

On Sun, Feb 05, 2017 at 11:09:14PM -0800, Junio C Hamano wrote:

> > @@ -99,6 +104,10 @@ create_stash () {
> >  	if test -z "$new_style"
> >  	then
> >  		stash_msg="$*"
> > +		while test $# != 0
> > +		do
> > +			shift
> > +		done
> >  	fi
> 
> The intent is correct, but I would probaly empty the "$@" not with
> an iteration of shifts but with a single
> 
> 	set x && shift
> 
> ;-)

Perhaps it is not portable, but I think "set --" does the same thing and
is perhaps less obscure.

-Peff

^ permalink raw reply	[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

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

* 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

* 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 5/5] stash: teach 'push' (and 'create') to honor pathspec
       [not found]       ` <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
  2017-02-06 15:20         ` Jeff King
@ 2017-02-11 16:50         ` Thomas Gummerer
  1 sibling, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-11 16:50 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Stephan Beyer, Marc Strapetz, Jeff King,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski

On 02/05, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > @@ -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
> 
> The intent is correct, but I would probaly empty the "$@" not with
> an iteration of shifts but with a single
> 
> 	set x && shift
> 
> ;-)

Thanks, I knew there had to be a better way, but I was unable to find
it.  I think I like Andreas suggestion of "shift $#" the best here.

> > @@ -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 "$@" | (
> 
> The above (and all other uses of "$@" was exactly what I had in mind
> when I gave you the "can we leave the '$@' the user gave us as-is?"
> comment during the review of the previous round.  
> 
> Looks much nicer.
> 
> > +test_expect_success 'stash push with $IFS character' '
> > +	>"foo	bar" &&
> 
> IIRC, a pathname with HT in it cannot be tested on MINGW.  For the
> purpose of this test, I think it is sufficient to use SP instead of
> HT here (and matching change below in the expectation, of course).

Will change.

> > +	>foo &&
> > +	>bar &&
> > +	git add foo* &&
> > +	git stash push --include-untracked -- foo* &&
> 
> While it is good that you are testing "foo*", you would also want
> another test to cover a pathspec with $IFS in it.  That is the case
> the code in the previous round had problems with.  Perhaps try
> something like
> 
> 	>foo && >bar && >"foo bar" && git add . &&
> 	echo modify foo >foo &&
> 	echo modify bar >bar &&
> 	echo modify "foo bar" >"foo bar" &&
> 	git stash push -- "foo b*"
> 
> which should result in the changes to "foo bar" in the resulting
> stash, while changes to "foo" and "bar" are not (and left in the
> working tree).  And make sure that is what happens?  I think with
> the code from the previous round, the above would instead stash
> changes to "foo" and "bar" without catching changes to "foo bar",
> because the single pathspec "foo b*" would have been mistakenly
> split into two, i.e. "foo" and "b*", and failed to match "foo bar"
> while matching the other two.

Thanks, I'll add a test for that.

^ 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

* Re: [PATCH v3 0/5] stash: support pathspec argument
       [not found]         ` <vpq7f4uxjmo.fsf@anie.imag.fr>
@ 2017-02-13 20:09           ` Jeff King
       [not found]             ` <vpqo9y5lqos.fsf@anie.imag.fr>
  0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-13 20:09 UTC (permalink / raw)
  To: Matthieu Moy
  Cc: Thomas Gummerer, git, Stephan Beyer, Junio C Hamano,
	Marc Strapetz, Johannes Schindelin, Øyvind A . Holm,
	Jakub Narębski

On Mon, Feb 13, 2017 at 03:14:55PM +0100, Matthieu Moy wrote:

> I don't remember doing this intentionally. The goal was really to
> prevent typos like "git stash -p drop" (which would have been
> interpreted as "git stash save -p drop" previously). So, let's consider
> this "only messages starting with - are accepted" as a bug: moving to
> the new "push" command it a nice opportunity to fix it without worrying
> about compatibility. A bit more about this in this old thread:

Yeah, I consider it a bug that we treat "-foo" as a message there.
Fortunately I think you really have to try hard to abuse it:

  # doesn't work, because "save" complains about "-foo" as an option
  git stash -foo

  # does save stash with message "-foo"
  git stash -- -foo

  # doesn't work, because _any_ non-option argument is rejected
  git stash -- use --foo option

> I think we can safely allow both
> 
>   git stash -p -- <pathspec...>
>   git stash push -p <pathspec...>
> 
> But allowing the same without -- is a bit more dangerous for the reason
> above:
> 
>   git stash -p drop
> 
> would be interpreted as
> 
>   git stash push -p drop
> 
> (i.e. limit the stash to file named "drop"), and this would almost
> certainly not be what the user wanted.

Is it really that dangerous, though? The likely outcome is Git saying
"nope, you don't have any changes to the file named drop". Of course the
user may have meant something different, but I feel like "-p" is a good
indicator that they are interested in making an actual stash.

Think about this continuum of commands:

  1. git stash droop

  2. git stash -p drop

  3. git stash -p -- drop

  4. git stash push -p drop

I think we can all agree that (4) is reasonable and safe. And I suspect
we'd all agree that (1) is suspect (you probably meant "drop", not "push
droop").

But between (2) and (3), I don't see much difference. The interesting
difference between (1) and (2) is the addition of "-p", which tells us
that the user expects to save a stash.

Another way of thinking of it is that "-p" as a command name is an alias
for "push -p". If you wanted to err on the conservative side, you could
literally implement it that way (so "git stash -k -p foo" would not
work; you'd have to use "git stash -p -k foo").

> > 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.
> 
> We already have "git commit -m <msg> <pathspec...>", so I think stash
> should just do the same thing as commit. Or, did I miss something?

The complexity is that right now, the first-level decision of "which
stash sub-command am I running?" doesn't know about any options. So "git
stash -m foo" would be rejected in the name of typo prevention, unless
that outer decision learns about "-m" as an option.

-Peff

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v3 0/5] stash: support pathspec argument
       [not found]             ` <vpqo9y5lqos.fsf@anie.imag.fr>
@ 2017-02-13 21:45               ` Jeff King
  2017-02-13 22:33                 ` Thomas Gummerer
  0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-13 21:45 UTC (permalink / raw)
  To: Matthieu Moy
  Cc: Thomas Gummerer, git, Stephan Beyer, Junio C Hamano,
	Marc Strapetz, Johannes Schindelin, Øyvind A . Holm,
	Jakub Narębski

On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:

> > Is it really that dangerous, though? The likely outcome is Git saying
> > "nope, you don't have any changes to the file named drop". Of course the
> > user may have meant something different, but I feel like "-p" is a good
> > indicator that they are interested in making an actual stash.
> 
> Indeed -p is not the best example. In the old thread, I used -q which is
> much more problematic:
> 
>   git stash -q drop => interpreted as: git stash push -q drop
>   git stash drop -q => drop with option -q

Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
rather to treat "-p" specially.

> It's not really "dangerous" at least in this case, since we misinterpret
> a destructive command for a less destructive one, but it is rather
> confusing that changing the order between command and options change the
> behavior.
> 
> I actually find it a reasonable expectation to allow swapping commands
> and options, some programs other than git allow it.

I think we may have already crossed that bridge with "git -p stash".

Not to mention that the ordering already _is_ relevant (we disallow one
order but not the other). If we really wanted to allow swapping, it
would mean making:

  git stash -p drop

the same as:

  git stash drop -p

I actually find _that_ more confusing. It would perhaps make more sense
with something like "-q", which is more of a "global" option than a
command-specific one. But I think we'd want to whitelist such global
options (and "-p" would not be on that list).

> > The complexity is that right now, the first-level decision of "which
> > stash sub-command am I running?" doesn't know about any options. So "git
> > stash -m foo" would be rejected in the name of typo prevention, unless
> > that outer decision learns about "-m" as an option.
> 
> Ah, OK. But that's not really hard to implement: when going through the
> option list looking for non-option, shift one more time when finding -m.

No, it's not hard conceptually. It just means implementing the
option-parsing policy in two places. That's not too bad now, but if we
started using rev-parse's options helper, then I think you have corner
cases like "git stash -km foo".

My "-p" suggestion suffers from a similar problem if you treat it as
"you can omit the 'push' if you say "-p", rather than "if -p is the
first option, it is a synonym for 'push -p'".

-Peff

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

* Re: [PATCH v3 0/5] stash: support pathspec argument
  2017-02-13 21:45               ` Jeff King
@ 2017-02-13 22:33                 ` Thomas Gummerer
       [not found]                   ` <xmqqwpctabvw.fsf@gitster.mtv.corp.google.com>
       [not found]                   ` <vpq60kdjl63.fsf@anie.imag.fr>
  0 siblings, 2 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-13 22:33 UTC (permalink / raw)
  To: Jeff King
  Cc: Matthieu Moy, 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 10:35:31PM +0100, Matthieu Moy wrote:
> 
> > > Is it really that dangerous, though? The likely outcome is Git saying
> > > "nope, you don't have any changes to the file named drop". Of course the
> > > user may have meant something different, but I feel like "-p" is a good
> > > indicator that they are interested in making an actual stash.
> > 
> > Indeed -p is not the best example. In the old thread, I used -q which is
> > much more problematic:
> > 
> >   git stash -q drop => interpreted as: git stash push -q drop
> >   git stash drop -q => drop with option -q
> 
> Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
> rather to treat "-p" specially.
> 
> > It's not really "dangerous" at least in this case, since we misinterpret
> > a destructive command for a less destructive one, but it is rather
> > confusing that changing the order between command and options change the
> > behavior.
> > 
> > I actually find it a reasonable expectation to allow swapping commands
> > and options, some programs other than git allow it.
> 
> I think we may have already crossed that bridge with "git -p stash".
> 
> Not to mention that the ordering already _is_ relevant (we disallow one
> order but not the other). If we really wanted to allow swapping, it
> would mean making:
> 
>   git stash -p drop
> 
> the same as:
> 
>   git stash drop -p
> 
> I actually find _that_ more confusing. It would perhaps make more sense
> with something like "-q", which is more of a "global" option than a
> command-specific one. But I think we'd want to whitelist such global
> options (and "-p" would not be on that list).
>
> > > The complexity is that right now, the first-level decision of "which
> > > stash sub-command am I running?" doesn't know about any options. So "git
> > > stash -m foo" would be rejected in the name of typo prevention, unless
> > > that outer decision learns about "-m" as an option.
> > 
> > Ah, OK. But that's not really hard to implement: when going through the
> > option list looking for non-option, shift one more time when finding -m.
> 
> No, it's not hard conceptually. It just means implementing the
> option-parsing policy in two places. That's not too bad now, but if we
> started using rev-parse's options helper, then I think you have corner
> cases like "git stash -km foo".
> 
> My "-p" suggestion suffers from a similar problem if you treat it as
> "you can omit the 'push' if you say "-p", rather than "if -p is the
> first option, it is a synonym for 'push -p'".

I'm almost convinced of special casing "-p".  (Maybe I'm easy to
convince as well, because it would be convenient ;) ) However it's a
bit weird that now "git stash -p file" would work, but "git stash -m
message" wouldn't.  Maybe we should do it the other way around, and
only special case "-q", and see if there is an non option argument
after that?  From a glance at the options that's the only one where
"git stash -<option> <verb>" could make sense to the user.

^ 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 0/5] stash: support pathspec argument
       [not found]                   ` <xmqqwpctabvw.fsf@gitster.mtv.corp.google.com>
@ 2017-02-14  0:27                     ` Jeff King
       [not found]                       ` <xmqqpoila9rt.fsf@gitster.mtv.corp.google.com>
  0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2017-02-14  0:27 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Gummerer, Matthieu Moy, git, Stephan Beyer, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski

On Mon, Feb 13, 2017 at 03:50:43PM -0800, Junio C Hamano wrote:

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> >> My "-p" suggestion suffers from a similar problem if you treat it as
> >> "you can omit the 'push' if you say "-p", rather than "if -p is the
> >> first option, it is a synonym for 'push -p'".
> >
> > I'm almost convinced of special casing "-p".  (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't.
> 
> I am not sure why this matters.  The original "git stash <msg>" was
> just "Are you being extremely busy and cannot even afford to type
> 'save'?  Ok, let me assume you meant that!".  Now we are talking
> about picking and choosing hunks carefully going through interactive
> process, I really do not think there is any justification to infer
> 'push' when 'push' was omitted in "git stash push -p" the user wants
> to do.

Maybe it is just me and my muscle memory, but "git stash -p" is quite a
common command for me[1]. And I have typed "git stash -p foo" many times
and been annoyed that it didn't work. I was hoping to end that
annoyance.

I guess I could make an alias and retrain my fingers.

-Peff

[1] I almost never run "reset --hard", preferring instead to stash away
    changes just in case I would change my mind later and want them. And
    I quite often use "stash -p" because I like to double check what I
    am throwing away.

    I also use "stash -p" heavily when picking apart changes from the
    working tree.

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH v3 0/5] stash: support pathspec argument
       [not found]                       ` <xmqqpoila9rt.fsf@gitster.mtv.corp.google.com>
@ 2017-02-14  0:37                         ` Jeff King
  0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2017-02-14  0:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Gummerer, Matthieu Moy, git, Stephan Beyer, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski

On Mon, Feb 13, 2017 at 04:36:22PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Maybe it is just me and my muscle memory, but "git stash -p" is quite a
> > common command for me[1]. And I have typed "git stash -p foo" many times
> > and been annoyed that it didn't work. I was hoping to end that
> > annoyance.
> 
> OK, I never run "stash -p" and did not realize people already find
> it useful that it becomes "stash save -p".  Please ignore me, then.
> I agree that breaking the established use is not nice.

To be fair, I don't think anybody is proposing breaking the established
use of "stash -p". I was just hoping the new pathspec feature could
trickle down to it, as well. :)

-Peff

^ permalink raw reply	[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

* Re: [PATCH v3 0/5] stash: support pathspec argument
       [not found]                   ` <vpq60kdjl63.fsf@anie.imag.fr>
@ 2017-02-14 21:36                     ` Thomas Gummerer
  0 siblings, 0 replies; 57+ messages in thread
From: Thomas Gummerer @ 2017-02-14 21:36 UTC (permalink / raw)
  To: Matthieu Moy
  Cc: Jeff King, git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski

On 02/14, Matthieu Moy wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > I'm almost convinced of special casing "-p".  (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't.  Maybe we should do it the other way around, and
> > only special case "-q", and see if there is an non option argument
> > after that?  From a glance at the options that's the only one where
> > "git stash -<option> <verb>" could make sense to the user.
> 
> Special-casing the allowed cases makes it easier to change the behavior
> in the future if needed: it's easy to add special cases and it doesn't
> break backward compatibility.
> 
> So, if we don't have a good reason to do otherwise, I'd rather stay on
> the future-proof side.

Ok, after reading the rest of the messages in the thread I'm convinced
we should just special case "-p" for now.

It's by far my most used stash command as well, after using git stash
without any arguments.

> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/


^ permalink raw reply	[flat|nested] 57+ messages in thread

end of thread, other threads:[~2017-02-14 21:36 UTC | newest]

Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-21 20:08 [PATCH 0/3] stash: support filename argument Thomas Gummerer
2017-01-21 20:08 ` [PATCH 1/3] Documentation/stash: remove mention of git reset --hard Thomas Gummerer
2017-01-22  1:27   ` Øyvind A. Holm
2017-01-24 19:51   ` Jakub Narębski
2017-01-24 20:14   ` Jeff King
2017-01-25 13:02     ` Jakub Narębski
2017-01-25 21:20       ` Junio C Hamano
2017-01-25 21:20     ` Junio C Hamano
2017-01-28 19:30       ` Thomas Gummerer
2017-01-28 23:54         ` Jeff King
2017-01-21 20:08 ` [PATCH 2/3] stash: introduce push verb Thomas Gummerer
2017-01-23 18:27   ` Junio C Hamano
2017-01-29 12:39     ` Thomas Gummerer
2017-01-21 20:08 ` [PATCH 3/3] stash: support filename argument Thomas Gummerer
2017-01-23 18:50   ` Junio C Hamano
2017-01-29 13:29     ` Thomas Gummerer
2017-01-24 10:56 ` [PATCH 0/3] " Johannes Schindelin
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-30 21:13     ` Junio C Hamano
2017-02-05 12:13       ` Thomas Gummerer
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>
2017-02-04 12:19       ` Thomas Gummerer
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>
2017-02-04 13:18       ` Thomas Gummerer
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
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-06 15:22       ` Jeff King
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>
2017-02-13 22:16         ` Thomas Gummerer
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
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
2017-02-13 21:57           ` Jeff King
2017-02-13 23:05             ` Jeff King
2017-02-14 21:30               ` Thomas Gummerer
2017-02-05 20:26     ` [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec Thomas Gummerer
     [not found]       ` <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>
2017-02-06 15:20         ` Jeff King
2017-02-11 16:50         ` Thomas Gummerer
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>
2017-02-13 20:09           ` Jeff King
     [not found]             ` <vpqo9y5lqos.fsf@anie.imag.fr>
2017-02-13 21:45               ` Jeff King
2017-02-13 22:33                 ` Thomas Gummerer
     [not found]                   ` <xmqqwpctabvw.fsf@gitster.mtv.corp.google.com>
2017-02-14  0:27                     ` Jeff King
     [not found]                       ` <xmqqpoila9rt.fsf@gitster.mtv.corp.google.com>
2017-02-14  0:37                         ` Jeff King
     [not found]                   ` <vpq60kdjl63.fsf@anie.imag.fr>
2017-02-14 21:36                     ` Thomas Gummerer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).