archlinux-downgrade / downgrade

Downgrade packages in Arch Linux
GNU General Public License v2.0
570 stars 24 forks source link

Bugfix for `sed` error messages #190

Closed atreyasha closed 2 years ago

atreyasha commented 2 years ago

Checklist

Description

This PR addresses #184 by directing stderr from all sed commands to /dev/null.

I considered wrapping the blocks/pipes where sed is used with an if-statement that checks for their exit codes as per https://github.com/pbrisbin/downgrade/issues/184#issuecomment-1053652095. But given there are several such blocks with sed, I thought it might be simpler to just direct the error message to /dev/null. The downside is that we don't get a special error message specifying invalid input, but we would still get the No results found error message which is not incorrect either.

WDYT?

atreyasha commented 2 years ago

I guess I'm just not clear on the behavior we're trying to fix. I think you're suggesting the current behavior is:

% downgrade whatever
sed error...
No results found
[exit 1]

Exactly, this is the current behaviour and I would like to ideally silence the sed error messages in such cases.

So I would suggest that we only change the sed that we know failed in that bug report, the one that we know fails because of an invalid search term. I think that would be the most minimal bugfix and a net improvement.

True. I looked deeper and found that the sed errors arise from the process_term function:

https://github.com/pbrisbin/downgrade/blob/4c3af2cfee79f2c4265abe69961fa1cc2dca2f4b/downgrade#L322-L326

Somehow despite the inner sed commands from process_term returning exit-code 1, the process_term function still continues to completion. This stops when I remove the if-block around process_term.

As more comprehensive alternative, we could introduce something like this:

sed_msg() {
 local msg=$1
 shift

 if ! sed "$@"; then
   echo "Failed $msg" >&2
   exit 1
 fi
}

Works like a charm. Modified the PR.

WDYT?

atreyasha commented 2 years ago

Yes, done