[PATCH] stash: Fix multiple error messages on create if no HEAD

[PATCH] stash: Fix multiple error messages on create if no HEAD

[Sebastian Morr]

create_stash() checks whether HEAD is valid via rev-parse. If this is
not the case, both itself as well as rev-parse print an error message.
Make rev-parse quiet.

In no_changes(), diff-index is called, which dies unquietly if there is
no commit. Hide it's stderr.

Signed-off-by: Sebastian Morr <sebastian@morr.cc>
---

This bugged me: In a new, empty repository:

    $ git stash
    fatal: bad revision 'HEAD'
    fatal: bad revision 'HEAD'
    fatal: Needed a single revision
    You do not have the initial commit yet

With this patch:

    $ git stash
    You do not have the initial commit yet

With the --quiet option, I wouldn't expect diff-index to print error
messages. But it does so (via revision.c, setup_revisions()). Is this
wanted?

 git-stash.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index c766692..07b6511 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -34,7 +34,7 @@ else
 fi
 
 no_changes () {
-	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
+	git diff-index --quiet --cached HEAD --ignore-submodules -- 2>/dev/null &&
 	git diff-files --quiet --ignore-submodules &&
 	(test -z "$untracked" || test -z "$(untracked_files)")
 }
@@ -67,7 +67,7 @@ create_stash () {
 	fi
 
 	# state of the base commit
-	if b_commit=$(git rev-parse --verify HEAD)
+	if b_commit=$(git rev-parse --quiet --verify HEAD)
 	then
 		head=$(git rev-list --oneline -n 1 HEAD --)
 	else
-- 
1.7.8.168.gd6118.dirty

Re: [PATCH] stash: Fix multiple error messages on create if no HEAD

[Junio C Hamano]

Sebastian Morr <sebastian@morr.cc> writes:

> This bugged me: In a new, empty repository:
>
>     $ git stash
>     fatal: bad revision 'HEAD'
>     fatal: bad revision 'HEAD'
>     fatal: Needed a single revision
>     You do not have the initial commit yet
>
> With this patch:
>
>     $ git stash
>     You do not have the initial commit yet

Yeah, that looks tidier.

> With the --quiet option, I wouldn't expect diff-index to print error
> messages.

Even with --quiet, an error is an error.

Asking for a diff with the current HEAD when you do not have one yet _is_
an error and "diff-index" is correct to report the error. The bug
(i.e. "git stash" shows that error message) is in the program that calls
diff-index when the operation does not make sense.

"Hiding error" is not desirable, especially if it is only for giving a
cleaner error message for a narrow corner case that nobody cares
(i.e. running "stash" when you do not have any commit to go back to), with
the side effect that it hides _real_ errors that may help users notice
unusual situation (e.g. corrupted index file).

Probably the right way to fix it is to check if HEAD is a valid commit
before running any command that requires to have a valid HEAD (i.e.
create and save would need one; list, show, etc. would not) and give that
"You do not have the initial commit yet" message, before passing the
control to the implementations of these individual subcommands, without
touching the places you touched in this patch.

>  no_changes () {
> -	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
> +	git diff-index --quiet --cached HEAD --ignore-submodules -- 2>/dev/null &&
>  	git diff-files --quiet --ignore-submodules &&
>  	(test -z "$untracked" || test -z "$(untracked_files)")
>  }
> @@ -67,7 +67,7 @@ create_stash () {
>  	fi
>  
>  	# state of the base commit
> -	if b_commit=$(git rev-parse --verify HEAD)
> +	if b_commit=$(git rev-parse --quiet --verify HEAD)
>  	then
>  		head=$(git rev-list --oneline -n 1 HEAD --)
>  	else