meiji163 / gh-notify

GitHub CLI extension to display GitHub notifications
The Unlicense
209 stars 21 forks source link

fix: checking version was too strict #92

Closed lnc3l0t closed 3 months ago

lnc3l0t commented 3 months ago

In my system I have, for some reasons, a developer version of fzf:

> fzf --version
0.29 (devel)

Although this version meets the minimum required version of 0.29.0 specified by MIN_FZF_VERSION="0.29.0", the check_version function incorrectly fails the check.

This PR addresses the issue by modifying the check_version function. Specifically, the function now allows the version check to pass when the installed version has one fewer segment than the threshold version, provided that the missing segment in the threshold version is 0. For instance, if the threshold version is 0.29.0, then an installed version of 0.29 is considered sufficient, as the trailing 0 does not imply a higher version requirement.

Therefore my additional check

if (( i == ${#threshold_parts[@]} - 1 )) && ((${#ver_parts[@]} == ${#threshold_parts[@]} - 1)) && ((${threshold_parts[i]} == 0));

result in the case tool=0.29 (2 parts) and threshold=0.29.0 (3 parts) and i=2 in

if (( 2 == 3-1 )) && ((2 == 3-1)) && ((0 == 0));
LangLangBart commented 3 months ago

Thanks, I can reproduce the issue as well. I will look into it soon.

LangLangBart commented 3 months ago

This will fail if the user has version 1 and the required version is 1.0.0.


can u try ?

--- a/gh-notify
+++ b/gh-notify
@@ -619,5 +619,5 @@ select_notif() {
 check_version() {
     local tool=$1 threshold=$2 on_error=${3:-die}
-    local user_version
+    local user_version user_version_part index
     declare -a ver_parts threshold_parts
     user_version=$(command $tool --version 2>&1 |
@@ -628,8 +628,9 @@ check_version() {
     IFS='.' read -ra threshold_parts <<<"$threshold"

-    for i in "${!threshold_parts[@]}"; do
-        if ((i >= ${#ver_parts[@]})) || ((ver_parts[i] < threshold_parts[i])); then
+    for index in "${!threshold_parts[@]}"; do
+        user_version_part=${ver_parts[index]:-0}
+        if ((user_version_part < threshold_parts[index])); then
             $on_error "Your '$tool' version '$user_version' is insufficient. The minimum required version is '$threshold'."
-        elif ((ver_parts[i] > threshold_parts[i])); then
+        elif ((user_version_part > threshold_parts[index])); then
             break
         fi
lnc3l0t commented 3 months ago

I agree with you @LangLangBart. Your version works for me.

Thank you, I didn't know the bash-variable-default-value syntax ${var:-def}.

LangLangBart commented 3 months ago

Thank you.