Bug#940588: git-debpush: pipefail in get_file_from_ref

[Sean Whitton]

Hello,

On Tue 17 Sep 2019 at 12:24PM +02, Andrej Shadura wrote:

> In line 61, grep -Eq may cause a pipefail if grep exits before git
> ls-tree concludes. With a debug print for $? I can see this:

Here's a patch implementing the fix.  Thank you for the report!

-- 
Sean Whitton
From 446b5b8c9c8179cd6f516fc5460ba70147c94e61 Mon Sep 17 00:00:00 2001
From: Sean Whitton 
Date: Sat, 19 Oct 2019 10:33:10 -0700
Subject: [PATCH] git-debpush: avoid a pipefail problem in get_file_from_ref

`grep -q` exits as soon as it finds a matching line, potentially
sending a SIGPIPE to git-ls-tree.  We have pipefail turned on, so that
can make the whole pipeline exit nonzero, which is wrong when grep did
in fact find a match.

This solution is more readable than disabling pipefail just for this
line (as is done elsewhere in git-debpush).

Reported-by: Andrej Shadura 
Signed-off-by: Sean Whitton 
---
 git-debpush | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-debpush b/git-debpush
index c3b067dc..2790560c 100755
--- a/git-debpush
+++ b/git-debpush
@@ -59,8 +59,10 @@ badusage () {
 get_file_from_ref () {
 local path=$1
 
+# redirect to /dev/null instead of using `grep -Eq` to avoid grep
+# SIGPIPEing git-ls-tree
 if git ls-tree --name-only -r "$branch" \
-| grep -Eq "^$path$"; then
+| grep -E "^$path$" >/dev/null; then
 git cat-file blob $branch:$path
 fi
 }
-- 
2.20.1



signature.asc
Description: PGP signature

Bug#940588: git-debpush: pipefail in get_file_from_ref

[Ian Jackson]

Control: user d...@packages.debian.org
Control: usertags -1 rsn

Andrej Shadura writes ("Bug#940588: git-debpush: pipefail in 
get_file_from_ref"):
> Package: git-debpush
> Version: 9.9
> Severity: important
...
> In line 61, grep -Eq may cause a pipefail if grep exits before git
> ls-tree concludes. With a debug print for $? I can see this:
> 
> ++ get_file_from_ref debian/source/format
> ++ local path=debian/source/format
> ++ git ls-tree --name-only -r refs/heads/debian/buster-security
> ++ grep -Eq '^debian/source/format$'
> ++ echo  141
> 
> It helped when I replaced it with a redirect:
> 
> if git ls-tree --name-only -r "$branch" \
> | grep -E "^$path$" >/dev/null; then
> git cat-file blob $branch:$path
> fi
> 
> ++ local path=debian/source/format
> ++ git ls-tree --name-only -r refs/heads/debian/buster-security
> ++ grep -E '^debian/source/format$'
> ++ echo  0

I agree with the suggestion to redirect to >/dev/null

Background from irc:

11:37  The thing with pipefail and SIGPIPE is very annoying.
11:37  IMO this is a bug in set -o pipefail
11:38  if ( git ls-tree --name-only -r "$branch" || test $? = 141 ) | 
   ...
11:38  What even weirder this is all in an if
11:39  I’d just >/dev/null tbh
11:39  http://paste.debian.net/1101208/#

ie:

mariner:~> bash -xec 'set -o pipefail; if yes | head -1 ; then id; fi'
+ set -o pipefail
+ yes
+ head -1
y
mariner:~> 

11:39  that’s more readable to me at least
11:40  I guess the performance is not really a consideration since it's 
   O(n) either way
11:40  (both because the worst case is O(n) and because the rest of it 
   is O(n))


-- 
Ian JacksonThese opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Bug#940588: git-debpush: pipefail in get_file_from_ref

[Andrej Shadura]

Package: git-debpush
Version: 9.9
Severity: important

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Dear Maintainer,

In line 61, grep -Eq may cause a pipefail if grep exits before git
ls-tree concludes. With a debug print for $? I can see this:

++ get_file_from_ref debian/source/format
++ local path=debian/source/format
++ git ls-tree --name-only -r refs/heads/debian/buster-security
++ grep -Eq '^debian/source/format$'
++ echo  141

It helped when I replaced it with a redirect:

if git ls-tree --name-only -r "$branch" \
| grep -E "^$path$" >/dev/null; then
git cat-file blob $branch:$path
fi

++ local path=debian/source/format
++ git ls-tree --name-only -r refs/heads/debian/buster-security
++ grep -E '^debian/source/format$'
++ echo  0

- -- 
Cheers,
  Andrej

-BEGIN PGP SIGNATURE-

iQFIBAEBCAAyFiEEeuS9ZL8A0js0NGiOXkCM2RzYOdIFAl2AtGwUHGFuZHJld3No
QGRlYmlhbi5vcmcACgkQXkCM2RzYOdJwAAgAlMAsuhQhpGGCWA5OXGfbhBIG3yT/
rwHjpo+qtiC2HJVOTkrS7NtP6CSDC+alxvBTfamLG2qtnbNR4ddk5z3AfHTPxoNL
ZlE/3n3qqTHe4G0YGSY925aoR09okxRa/mjagIlKqDwg45xdE2W9ZdtkStipFno2
mPbvCzmzKo+XToXKpzOi4wRgbZcnFeum1U4qezHdKhSRApRmuq95nQ+uIJfbVrt4
Rn7ewarbvJFHsBXotVaX/rHWvxpzNlWBVQZYR5Bq7Y8wunEF6iY2Hxxq7MxAXkg4
DuUCtLbjpkHI1DGZKKm+6zcCKGaMAG3BGvuHqEy/IWwGbwkjDEoKtLo4kQ==
=/dwb
-END PGP SIGNATURE-