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

Sebastian Morr· Dec 14, 2011, 00:14 UTC · lore

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(-)

Show changes to git-stash.sh +2 −2

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

Junio C Hamano· Dec 14, 2011, 01:16 UTC · re: Sebastian Morr · lore

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

Sebastian Morr <sebastian@morr.cc> writes:

Show 12 quoted lines

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

Show 15 quoted lines

>  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

patchstash: Fix multiple error messages on create if no HEAD