threads / patch / 37457

patch, 2 partsbisect: remove unnecessary redirection

Subject: [PATCH 1/2] bisect: remove unnecessary redirection

## tl;dr

5 messages between Aug 30, 2014 and Aug 31, 2014. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

David Aguilar· Aug 30, 2014, 19:30 UTC · lore

`git rev-parse` is being called with --quiet so there's no need to redirect to /dev/null.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-bisect.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to git-bisect.sh +1 −1

diff --git a/git-bisect.sh b/git-bisect.sh
index 1e0d602..c1c2321 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -365,7 +365,7 @@ bisect_reset() {
 	}
 	case "$#" in
 	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
+	1) git rev-parse --quiet --verify "$1^{commit}" || {
 			invalid="$1"
 			die "$(eval_gettext "'\$invalid' is not a valid commit")"
 		}

-- 
2.1.0.29.g9b751c4

David Aguilar· Aug 30, 2014, 19:30 UTC · re: David Aguilar · lore

[PATCH 2/2] stash: prefer --quiet over shell redirection

Use `git rev-parse --quiet` instead of redirecting to /dev/null.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-stash.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

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

diff --git a/git-stash.sh b/git-stash.sh
index bcc757b..5a5185b 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -50,7 +50,7 @@ clear_stash () {
 	then
 		die "$(gettext "git stash clear with parameters is unimplemented")"
 	fi
-	if current=$(git rev-parse --verify $ref_stash 2>/dev/null)
+	if current=$(git rev-parse --quiet --verify $ref_stash)
 	then
 		git update-ref -d $ref_stash $current
 	fi
@@ -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
@@ -292,7 +292,7 @@ save_stash () {
 }
 
 have_stash () {
-	git rev-parse --verify $ref_stash >/dev/null 2>&1
+	git rev-parse --quiet --verify $ref_stash
 }
 
 list_stash () {
@@ -392,12 +392,12 @@ parse_flags_and_rev()
 		;;
 	esac
 
-	REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
+	REV=$(git rev-parse --quiet --symbolic --verify "$1") || {
 		reference="$1"
 		die "$(eval_gettext "\$reference is not valid reference")"
 	}
 
-	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+	i_commit=$(git rev-parse --quiet --verify "$REV^2") &&
 	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&
 	s=$1 &&
 	w_commit=$1 &&
@@ -409,7 +409,7 @@ parse_flags_and_rev()
 	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
 	IS_STASH_REF=t
 
-	u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+	u_commit=$(git rev-parse --quiet --verify "$REV^3") &&
 	u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
 }
 
@@ -531,7 +531,7 @@ drop_stash () {
 		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
 
 	# clear_stash if we just dropped the last stash entry
-	git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
+	git rev-parse --quiet --verify "$ref_stash@{0}" || clear_stash
 }
 
 apply_to_branch () {

-- 
2.1.0.29.g9b751c4

Johannes Sixt· Aug 30, 2014, 21:07 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] stash: prefer --quiet over shell redirection

Am 30.08.2014 21:30, schrieb David Aguilar:

Show 29 quoted lines

> Use `git rev-parse --quiet` instead of redirecting to /dev/null.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-stash.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index bcc757b..5a5185b 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -50,7 +50,7 @@ clear_stash () {
>  	then
>  		die "$(gettext "git stash clear with parameters is unimplemented")"
>  	fi
> -	if current=$(git rev-parse --verify $ref_stash 2>/dev/null)
> +	if current=$(git rev-parse --quiet --verify $ref_stash)
>  	then
>  		git update-ref -d $ref_stash $current
>  	fi
> @@ -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

The else branch calls die; wouldn't it be useful to keep the error message of rev-parse?

Show 6 quoted lines

> @@ -292,7 +292,7 @@ save_stash () {
>  }
>  
>  have_stash () {
> -	git rev-parse --verify $ref_stash >/dev/null 2>&1
> +	git rev-parse --quiet --verify $ref_stash

This change is not correct: --quiet removes only output on stderr, but not that on stdout.

Show 16 quoted lines

>  }
>  
>  list_stash () {
> @@ -392,12 +392,12 @@ parse_flags_and_rev()
>  		;;
>  	esac
>  
> -	REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
> +	REV=$(git rev-parse --quiet --symbolic --verify "$1") || {
>  		reference="$1"
>  		die "$(eval_gettext "\$reference is not valid reference")"
>  	}
>  
> -	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
> +	i_commit=$(git rev-parse --quiet --verify "$REV^2") &&
>  	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&

I see another rev-parse that you did not modify. An omission?

Show 9 quoted lines

>  	s=$1 &&
>  	w_commit=$1 &&
> @@ -409,7 +409,7 @@ parse_flags_and_rev()
>  	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
>  	IS_STASH_REF=t
>  
> -	u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
> +	u_commit=$(git rev-parse --quiet --verify "$REV^3") &&
>  	u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)

I see yet another rev-parse that you did not modify.

Show 8 quoted lines

>  }
>  
> @@ -531,7 +531,7 @@ drop_stash () {
>  		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
>  
>  	# clear_stash if we just dropped the last stash entry
> -	git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
> +	git rev-parse --quiet --verify "$ref_stash@{0}" || clear_stash

Again not correct.

>  }
>  
>  apply_to_branch () {
>

-- Hannes

David Aguilar· Aug 31, 2014, 07:35 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/2] stash: prefer --quiet over shell redirection

On Sat, Aug 30, 2014 at 11:07:13PM +0200, Johannes Sixt wrote:

Show 16 quoted lines

> Am 30.08.2014 21:30, schrieb David Aguilar:
> > @@ -392,12 +392,12 @@ parse_flags_and_rev()
> >  		;;
> >  	esac
> >  
> > -	REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
> > +	REV=$(git rev-parse --quiet --symbolic --verify "$1") || {
> >  		reference="$1"
> >  		die "$(eval_gettext "\$reference is not valid reference")"
> >  	}
> >  
> > -	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
> > +	i_commit=$(git rev-parse --quiet --verify "$REV^2") &&
> >  	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&
> 
> I see another rev-parse that you did not modify. An omission?

The docs for --quiet say, "Only meaningful in --verify mode", so I didn't touch the non-verify call-sites.

Thanks for the review. I'll address your notes and send a v2 shortly.

-- 
David

Johannes Sixt· Aug 30, 2014, 20:57 UTC · re: David Aguilar · lore

Re: [PATCH 1/2] bisect: remove unnecessary redirection

Am 30.08.2014 21:30, schrieb David Aguilar:

Show 18 quoted lines

> `git rev-parse` is being called with --quiet so there's no need to
> redirect to /dev/null.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-bisect.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 1e0d602..c1c2321 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -365,7 +365,7 @@ bisect_reset() {
>  	}
>  	case "$#" in
>  	0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
> -	1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
> +	1) git rev-parse --quiet --verify "$1^{commit}" || {

This is wrong. The redirection quells stdout, but --quiet is about stderr.

>  			invalid="$1"
>  			die "$(eval_gettext "'\$invalid' is not a valid commit")"
>  		}
>

-- Hannes