Re: [PATCH/RFC 1/3] stash: show less information for stash push -- <pathspec>

Jeff King [off-list ref] writes:

On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:

quoted

quoted

diff --git a/git-stash.sh b/git-stash.sh
index 9c70662cc8..59f055e27b 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -299,10 +299,10 @@ push_stash () {
 	then
 		if test $# != 0
 		then
-			git reset ${GIT_QUIET:+-q} -- "$@"
+			git reset -q -- "$@"
 			git ls-files -z --modified -- "$@" |
 			git checkout-index -z --force --stdin
-			git clean --force ${GIT_QUIET:+-q} -d -- "$@"
+			git clean --force -q -d -- "$@"
 		else
 			git reset --hard ${GIT_QUIET:+-q}
 		fi

Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
we of course still do).  We didn't report changes to which paths
have been reverted in (or which new paths are removed from) the
working tree to the original state (and we of course still don't).

The messages from reset and clean that reports these probably do not
need to be shown by default to the users (they can always check with
"git stash show" when they are curious or when they want to double
check).

I'm not sure if you are arguing here that the non-pathspec case should
move to an unconditional "-q", too, to suppress the "HEAD is now at"
message.  But I think that is a good suggestion. It would make the two
cases consistent, and it is not really adding anything of value (it is
always just HEAD, and if you do not provide a custom message, the
short-sha1 and subject are already in the "Saved..." line above).

I wasn't suggesting it (I was just saying that these extra messages
are not something we found necessary for consistency with the
original codepath when we added the pathspec support).  I wasn't
even thinking about what the original codepath did, i.e. when the
command is run without pathspec.

I too suspect that most of the ${GIT_QUIET:+-q} can just become an
unconditional -q as you do.

On 03/20, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Mon, Mar 20, 2017 at 10:51:16AM -0700, Junio C Hamano wrote:

quoted

quoted

diff --git a/git-stash.sh b/git-stash.sh
index 9c70662cc8..59f055e27b 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -299,10 +299,10 @@ push_stash () {
 	then
 		if test $# != 0
 		then
-			git reset ${GIT_QUIET:+-q} -- "$@"
+			git reset -q -- "$@"
 			git ls-files -z --modified -- "$@" |
 			git checkout-index -z --force --stdin
-			git clean --force ${GIT_QUIET:+-q} -d -- "$@"
+			git clean --force -q -d -- "$@"
 		else
 			git reset --hard ${GIT_QUIET:+-q}
 		fi

Yup, we only said "HEAD is now at ..." in the non-pathspec case (and
we of course still do).  We didn't report changes to which paths
have been reverted in (or which new paths are removed from) the
working tree to the original state (and we of course still don't).

The messages from reset and clean that reports these probably do not
need to be shown by default to the users (they can always check with
"git stash show" when they are curious or when they want to double
check).

I'm not sure if you are arguing here that the non-pathspec case should
move to an unconditional "-q", too, to suppress the "HEAD is now at"
message.  But I think that is a good suggestion. It would make the two
cases consistent, and it is not really adding anything of value (it is
always just HEAD, and if you do not provide a custom message, the
short-sha1 and subject are already in the "Saved..." line above).

I wasn't suggesting it (I was just saying that these extra messages
are not something we found necessary for consistency with the
original codepath when we added the pathspec support).  I wasn't
even thinking about what the original codepath did, i.e. when the
command is run without pathspec.

I too suspect that most of the ${GIT_QUIET:+-q} can just become an
unconditional -q as you do.

Thanks both, I do agree that passing -q unconditionally is probably
the right thing to do.  Will do that, and also pass -q unconditionally
to the git reset I addressed in 2/3 here in the re-roll.