| Message ID | 20180314015205.20781-5-lukeshu@lukeshu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Backports from Parabola | expand |
On 03/13/2018 09:52 PM, Luke Shumaker wrote: > From: Luke Shumaker <lukeshu@parabola.nu> > > `grep -q` may exit as soon as it finds a match; this is a good optimization > for when the input is a file. However, if the input is the output of > another program, then that other program will receive SIGPIPE, and further > writes will fail. When this happens, it might (bsdtar does) print a > message about a "write error" to stderr. Which is going to confuse and > alarm the user. > > In one of the cases, this had already been mitigated by wrapping > bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain > if it gets SIGPIPE. However, that means we're storing the entire > output of bsdtar in memory, which is silly. > --- > db-functions | 2 +- > test/lib/common.bash | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/db-functions b/db-functions > index 58b753a..ee390ff 100644 > --- a/db-functions > +++ b/db-functions > @@ -303,7 +303,7 @@ check_pkgfile() { > > in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1 > > - if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then > + if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then But echo should be fine anyway? Regardless this could be so much more elegant. if [[ $pkgfile = $pkgname-$pkgver-$pkgrel-$arch* ]]; then > return 0 > else > return 1 > diff --git a/test/lib/common.bash b/test/lib/common.bash > index 45e4800..ab805dd 100644 > --- a/test/lib/common.bash > +++ b/test/lib/common.bash > @@ -215,7 +215,7 @@ checkPackageDB() { > > for db in ${DBEXT} ${FILESEXT}; do > [ -r "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" ] > - bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT}}" > + bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep "${pkgfile%${PKGEXT}}" &>/dev/null > done > done > done > @@ -269,7 +269,7 @@ checkRemovedPackageDB() { > for tarch in ${tarches[@]}; do > if [ -r "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" ]; then > for pkgname in ${pkgnames[@]}; do > - echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname} > + bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null > done > fi > done >
On Tue, 13 Mar 2018 21:52:01 -0400, Luke Shumaker wrote: > > From: Luke Shumaker <lukeshu@parabola.nu> > > `grep -q` may exit as soon as it finds a match; this is a good optimization > for when the input is a file. However, if the input is the output of > another program, then that other program will receive SIGPIPE, and further > writes will fail. When this happens, it might (bsdtar does) print a > message about a "write error" to stderr. Which is going to confuse and > alarm the user. > > In one of the cases, this had already been mitigated by wrapping > bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain > if it gets SIGPIPE. However, that means we're storing the entire > output of bsdtar in memory, which is silly. > --- > - echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname} > + bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null This is broken. As the commit message said, the subshell soaks up the full output to avoid SIGPIPE. But it also has a nother subtle purpose: to ensure that at least one "\n" is written to grep's stdin (as echo appends "\n"). Otherwise, if the db is now empty, grep will fail because it didn't recieve anything. The correct command is ! bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep ${pkgname} &>/dev/null
On Wed, 14 Mar 2018 00:11:12 -0400, Eli Schwartz wrote: > > On 03/13/2018 09:52 PM, Luke Shumaker wrote: > > From: Luke Shumaker <lukeshu@parabola.nu> > > > > `grep -q` may exit as soon as it finds a match; this is a good optimization > > for when the input is a file. However, if the input is the output of > > another program, then that other program will receive SIGPIPE, and further > > writes will fail. When this happens, it might (bsdtar does) print a > > message about a "write error" to stderr. Which is going to confuse and > > alarm the user. > > > > In one of the cases, this had already been mitigated by wrapping > > bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain > > if it gets SIGPIPE. However, that means we're storing the entire > > output of bsdtar in memory, which is silly. > > --- > > db-functions | 2 +- > > test/lib/common.bash | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/db-functions b/db-functions > > index 58b753a..ee390ff 100644 > > --- a/db-functions > > +++ b/db-functions > > @@ -303,7 +303,7 @@ check_pkgfile() { > > > > in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1 > > > > - if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then > > + if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then > > But echo should be fine anyway? Yeah, in this case it's for consistency with the others. It's easier to remember "don't use `grep -q` on other commands' stdout" that it is to work out when it's ok and when it isn't. > Regardless this could be so much more elegant. > > if [[ $pkgfile = $pkgname-$pkgver-$pkgrel-$arch* ]]; then You're right, that is better.
diff --git a/db-functions b/db-functions index 58b753a..ee390ff 100644 --- a/db-functions +++ b/db-functions @@ -303,7 +303,7 @@ check_pkgfile() { in_array "${pkgarch}" "${ARCHES[@]}" 'any' || return 1 - if echo "${pkgfile##*/}" | grep -q "^${pkgname}-${pkgver}-${pkgarch}"; then + if echo "${pkgfile##*/}" | grep "^${pkgname}-${pkgver}-${pkgarch}" &>/dev/null; then return 0 else return 1 diff --git a/test/lib/common.bash b/test/lib/common.bash index 45e4800..ab805dd 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -215,7 +215,7 @@ checkPackageDB() { for db in ${DBEXT} ${FILESEXT}; do [ -r "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" ] - bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep -q "${pkgfile%${PKGEXT}}" + bsdtar -xf "${FTP_BASE}/${repo}/os/${repoarch}/${repo}${db%.tar.*}" -O | grep "${pkgfile%${PKGEXT}}" &>/dev/null done done done @@ -269,7 +269,7 @@ checkRemovedPackageDB() { for tarch in ${tarches[@]}; do if [ -r "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" ]; then for pkgname in ${pkgnames[@]}; do - echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname} + bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep -v ${pkgname} &>/dev/null done fi done
From: Luke Shumaker <lukeshu@parabola.nu> `grep -q` may exit as soon as it finds a match; this is a good optimization for when the input is a file. However, if the input is the output of another program, then that other program will receive SIGPIPE, and further writes will fail. When this happens, it might (bsdtar does) print a message about a "write error" to stderr. Which is going to confuse and alarm the user. In one of the cases, this had already been mitigated by wrapping bsdtar in "echo "$(bsdtar ...)", as Bash builtin echo doesn't complain if it gets SIGPIPE. However, that means we're storing the entire output of bsdtar in memory, which is silly. --- db-functions | 2 +- test/lib/common.bash | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)