Repository: junegunn/fzf (language: go)

Issue:
.fzf/install fails to install if .bashrc changes from previous installation are commented out
- [X] I have read through the manual page (`man fzf`)
- [X] I have the latest version of fzf (0.46.1 (3c0a630))
- [X] I have searched through the existing issues

## Info

- OS
    - [X] Linux
- Shell
    - [X] bash

## Problem / Steps to reproduce

I was reinstalling `fzf` after deciding my package manager's version was too far out of date.

I just commented out fzf's .bashrc changes like so:

```bash
#  Comment out fzf changes; let new install redo in case something has changed
#  Original changes:  - [ -f ~/.fzf.bash ] && source ~/.fzf.bash
```
When running `~/.fzf/install` again, I got the message:

```bash
 - [ -f ~/.fzf.bash ] && source ~/.fzf.bash
    - Already exists: line #189 
```

Unfortunately, the message was correct, the text was there, but `install` did *not notice that the text was commented out*.

Since the required changes were "already there," `install` did not make the required changes to insert `fzf` into the PATH, load completion, and set key bindings.  IOW, even though `install` went through all the steps without error, not noticing that the required text was commented out made the install "fail."

After looking at  `append_line()` in the install script, I determined that attempting to submit a pull request for a fix would probably  waste more of everyone's time than just reporting the issue.



Discussion / hints:
Thanks for the report. Do you think it would make sense to look for the pattern that is not preceded by `#`?

```diff
diff --git a/install b/install
index 5d031fd..3ca5a80 100755
--- a/install
+++ b/install
@@ -311,7 +311,7 @@ append_line() {
     if [ $# -lt 4 ]; then
       lno=$(\grep -nF "$line" "$file" | sed 's/:.*//' | tr '\n' ' ')
     else
-      lno=$(\grep -nF "$pat" "$file" | sed 's/:.*//' | tr '\n' ' ')
+      lno=$(\grep -n "$pat" "$file" | sed 's/:.*//' | tr '\n' ' ')
     fi
   fi
   if [ -n "$lno" ]; then
@@ -349,7 +349,7 @@ echo
 for shell in $shells; do
   [[ "$shell" = fish ]] && continue
   [ $shell = zsh ] && dest=${ZDOTDIR:-~}/.zshrc || dest=~/.bashrc
-  append_line $update_config "[ -f ${prefix}.${shell} ] && source ${prefix}.${shell}" "$dest" "${prefix}.${shell}"
+  append_line $update_config "[ -f ${prefix}.${shell} ] && source ${prefix}.${shell}" "$dest" "^[^#]*${prefix}\.${shell}"
 done
 
 if [ $key_bindings -eq 1 ] && [[ "$shells" =~ fish ]]; then
```
"preceded by a '#'" is not going to be 100% reliable -- if the "#" is inside any kind of quotes, it probably doesn't comment out anything in the current file but that's probably pretty nasty to regex around.

As the old saying goes:

> Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems.

That's why I declined to attempt a patch.

That said, even just looking for a '#' as the first non-whitespace character of the line would have covered my case and would almost certainly handle 99% of these issues because I'm sure most of them are just like mine and for the same reason.  I just commented it out with a "#" as the first character on the line without touching anything else to make sure any updated code would be added to my .bashrc (or wherever the heck it was going) when I reinstalled.

As to the regex below, I would *not* assume that it is anchored at the beginning of the line, only that it's the first non-whitespace character.

ssteinerX
The regex requires that the full prefix does not contain any `#`, so we need the anchor.

Anyway, I'm not targeting 100% accuracy with this approach. See, the line could be inside a multi-line string, or inside a function that is never called. We would never know. But it will work 99.99% of the time.

The real question is, is this the right thing to do? It's hard to say. If a user manually commented out the line, maybe they want to keep it that way, and we should probably not add another line.

We could instead just print the found line to the console, and let the user have a better understanding of what's happening.
Or you could say:

I was about to add "xxxxxxx" to .bashrc and found "xxxxxxx" commented out.
<echo commented out line(s)>

Proceed to modify .bashrc? (y/N)?




Fix patch:
diff --git a/install b/install
index 5b67e30a147..556d9dfa62a 100755
--- a/install
+++ b/install
@@ -295,35 +295,44 @@ EOF
 fi
 
 append_line() {
-  set -e
-
-  local update line file pat lno
+  local update line file pat lines
   update="$1"
   line="$2"
   file="$3"
   pat="${4:-}"
-  lno=""
+  lines=""
 
   echo "Update $file:"
   echo "  - $line"
   if [ -f "$file" ]; then
     if [ $# -lt 4 ]; then
-      lno=$(\grep -nF "$line" "$file" | sed 's/:.*//' | tr '\n' ' ')
+      lines=$(\grep -nF "$line" "$file")
     else
-      lno=$(\grep -nF "$pat" "$file" | sed 's/:.*//' | tr '\n' ' ')
+      lines=$(\grep -nF "$pat" "$file")
     fi
   fi
-  if [ -n "$lno" ]; then
-    echo "    - Already exists: line #$lno"
-  else
-    if [ $update -eq 1 ]; then
-      [ -f "$file" ] && echo >> "$file"
-      echo "$line" >> "$file"
-      echo "    + Added"
-    else
-      echo "    ~ Skipped"
+
+  if [ -n "$lines" ]; then
+    echo "    - Already exists:"
+    sed 's/^/        Line /' <<< "$lines"
+
+    update=0
+    if ! grep -qv "^[0-9]*:[[:space:]]*#" <<< "$lines" ; then
+      echo "    - But they all seem to be commented"
+      ask  "    - Continue modifying $file?"
+      update=$?
     fi
   fi
+
+  set -e
+  if [ "$update" -eq 1 ]; then
+    [ -f "$file" ] && echo >> "$file"
+    echo "$line" >> "$file"
+    echo "    + Added"
+  else
+    echo "    ~ Skipped"
+  fi
+
   echo
   set +e
 }


Test patch: