Check every shim in SHIM_PATH instead of just the first one; break once the first diff is found

[richiethomas]

TL;DR:
If we only check the first shim in the SHIM_PATH directory for diffing (and therefore removal), we might miss some edge cases (for example, if there's no diff w/ the first file but there is a diff with subsequent files). This PR changes the remove_outdated_shims function so that it checks each shim against the prototype, and if it finds a diff, it removes all the shim files and breaks out of the loop.

As a side note, this PR also removes two functions which are no longer used in rbenv-rehash.

UPDATE:
I now understand the meaning of the comments above remove_outdated_shims to mean that we are intentionally only checking the first shim in the SHIM_PATH directory. That said, is there a chance that the install_registered_shims function could fail mid-way through, updating the first shim(s) in the SHIM_PATH but leaving subsequent shims un-replaced?

If so, I would think a subsequent call to rbenv rehash would not perform as expected as long as we're only checking the first shim file, in which case does it make sense to check the diffs of all the shim files, not just the first one? There might be a small chance of that happening, but the question is how bad is the result if it does happen. It seems to me like being unable to update those remaining shims would be a pretty bad result indeed, but y'all are the experts so I defer to you.

I'm guessing that diffing only the first shim in the directory is a performance optimization, which makes sense. For what it's worth, I ran this experiment on my local machine and (subjectively) it didn't feel especially slow to diff on all the gems, but then I only have 97 gems currently installed in ~/.rbenv/shims/ so I'm probably not the p99 power-user that you'd want to perf test this against.

Details:
It's quite possible I'm in the wrong here, but the way this line of code reads to me, it seems like the break statement should be inside the if check, shouldn't it? As it's currently written, it seems to always break after the first iteration of the for-loop. I think I replicated this phenomenon locally, via the following:

Here I do the following:

• I create a prototype file containing the character “a”,
• I make a directory named shims with 4 text files, 3 of which match the prototype and one of which does not.
• I then paste an identical remove_outdated_shims function into the shell, with only some extra loglines and a counter variable added.
• I increment the counter before every run and log the count # to the screen.
• Lastly, I call the function in the terminal with the appropriate env var values set for $SHIM_PATH and PROTOTYPE_SHIM_PATH.

In my shell function, the counter only outputs once, and we never see the about to delete all shims logline, meaning we didn’t reach the shim whose contents differ from the prototype. Am I missing something, or is this a bug in the code?

Just in case, here's a PR to address the issue. If the code already works as intended, I'd love to know what piece of info I appear to be missing. Thanks for maintaining a great tool.

[richiethomas]

As an update, I tried the following investigation and wanted to get your thoughts on it.

TL;DR
My takeaway is that the old shims are actually not getting deleted by this line of code since it appears we always break from this for loop after its first iteration (due to where the break is placed), and therefore any outdated shim files aren't being replaced by this line of code because the [ -e "$file" ] || check returns true if the file exists and the shell never executes the cp code after the or condition, which would otherwise over-write the old shim.

Details
I added loglines to print out the state of the old shim files, including timestamps so we could judge whether the old shim files had successfully been replaced by new ones. Apologies for not knowing how to make this code dump shorter:

remove_outdated_shims() {
  local shim
  for file in $(ls "$SHIM_PATH"/*); do.           # Set the timestamps on each file in $SHIM_PATH to a known date in the past
    touch -d "2020-09-01T00:00:00" $file
  done
  >&2 echo "gemfiles before remove op:"
  >&2 ls -la "$SHIM_PATH"/*
  >&2 echo "-------------"
  for shim in "$SHIM_PATH"/*; do
    if ! diff "$PROTOTYPE_SHIM_PATH" "$shim" >/dev/null 2>&1; then
      rm -f "$SHIM_PATH"/*
    fi
    break
  done
}

install_registered_shims() {
  local shim file
  >&2 echo "gemfiles before install op:"
  >&2 ls -la "$SHIM_PATH"/*
  >&2 echo "-------------"

  for shim in "${registered_shims[@]}"; do
    file="${SHIM_PATH}/${shim}"
    [ -e "$file" ] || cp "$PROTOTYPE_SHIM_PATH" "$file"
  done
  >&2 echo "gemfiles after install op:"
  >&2 ls -la "$SHIM_PATH"/*
  >&2 echo "-------------"
}

....

install_registered_shims
remove_stale_shims
>&2 echo "end of rehash results:"
>&2 ls -la "$SHIM_PATH"/*

Basically, other than the 3-line for-loop which I added a comment to, the only lines I added are log lines to STDERR which begin with >&2. Otherwise, it should be the same as commit [410e05b](https://github.com/rbenv/rbenv/commit/410e05bf8ca09d736985806e9849e675635a4230) on the main branch.

One final change was that I added a trivial difference to one of my current shim files to trigger the diffing condition in the for loop, making sure that the shim I modified was not the first file listed in the directory (to test my hypothesis that the for loop exits after the first iteration):

# /Users/richiethomas/.rbenv/shims/unicorn

  1 #!/usr/bin/env bash
  2 echo "Hello world"                           # this is the only line I added
  3 set -e
  4 [ -n "$RBENV_DEBUG" ] && set -x
  5 
  6 program="${0##*/}"
  7 if [ "$program" = "ruby" ]; then
  8   for arg; do
  9     case "$arg" in
 10     -e* | -- ) break ;;
 11     */* )
 12       if [ -f "$arg" ]; then
 13         export RBENV_DIR="${arg%/*}"
 14         break
 15       fi
 16       ;;
 17     esac
 18   done
 19 fi
 20 
 21 export RBENV_ROOT="/Users/richiethomas/.rbenv"
 22 exec "/usr/local/bin/rbenv" exec "$program" "$@"

When I run rbenv rehash with the break in its old position, I get the following:

~/Workspace/OpenSource/rbenv (master)  $ rbenv rehash
about to run sh-rehash
gemfiles before remove op:
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  412 Sep  1  2020 /Users/richiethomas/.rbenv/shims/unicorn
...
-------------
gemfiles before install op:
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  412 Sep  1  2020 /Users/richiethomas/.rbenv/shims/unicorn
...
-------------
gemfiles after install op:
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  412 Sep  1  2020 /Users/richiethomas/.rbenv/shims/unicorn
...
-------------
end of rehash results:
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  412 Sep  1  2020 /Users/richiethomas/.rbenv/shims/unicorn
....

The important part to zero in on is the timestamp. I intentionally set the timestamp of the shim files to a known past date, to see if the final timestamp after the install operation would be today's date. Looks like they aren't, at least on my machine.

When I add the same echo statement to the unicorn shim and move the break statement to inside the if check inside rbenv-rehash, like so:

88 remove_outdated_shims() {
 89   local shim
 90   for file in $(ls "$SHIM_PATH"/*); do
 91     touch -d "2020-09-01T00:00:00" $file
 92   done
 93   >&2 echo "gemfiles before remove op:"
 94   >&2 ls -la "$SHIM_PATH"/*
 95   >&2 echo "-------------"
 96   for shim in "$SHIM_PATH"/*; do
 97     if ! diff "$PROTOTYPE_SHIM_PATH" "$shim" >/dev/null 2>&1; then
 98       rm -f "$SHIM_PATH"/*
 99       break
100     fi
101   done
102 }

I get the output I would expect from rbenv rehash:

~/Workspace/OpenSource/rbenv (master)  $ rbenv rehash
about to run sh-rehash
gemfiles before remove op:
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Sep  1  2020 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  412 Sep  1  2020 /Users/richiethomas/.rbenv/shims/unicorn
...
-------------
gemfiles before install op:
total 80
drwxr-xr-x  18 richiethomas  staff    576 Oct  8 19:28 .
drwxr-xr-x   7 richiethomas  staff    224 Oct  3 09:41 ..
drwxr-xr-x  15 richiethomas  staff    480 Oct  8 19:09 .git
-rw-r--r--   1 richiethomas  staff     47 Oct  8 12:23 .gitattributes
drwxr-xr-x   3 richiethomas  staff     96 Sep 29 09:01 .github
-rw-r--r--   1 richiethomas  staff     97 Sep 29 09:01 .gitignore
-rw-r--r--   1 richiethomas  staff     35 Sep 29 09:01 .vimrc
-rw-r--r--   1 richiethomas  staff   3390 Sep 29 09:01 CODE_OF_CONDUCT.md
-rw-r--r--   1 richiethomas  staff   1058 Sep 29 09:01 LICENSE
-rw-r--r--   1 richiethomas  staff    163 Oct  8 12:23 Makefile
-rw-r--r--   1 richiethomas  staff  12624 Oct  8 12:23 README.md
drwxr-xr-x   3 richiethomas  staff     96 Sep 29 09:01 bin
drwxr-xr-x   4 richiethomas  staff    128 Sep 30 10:33 completions
drwxr-xr-x  27 richiethomas  staff    864 Oct  8 12:48 libexec
drwxr-xr-x   3 richiethomas  staff     96 Sep 29 09:01 rbenv.d
drwxr-xr-x   3 richiethomas  staff     96 Oct  8 12:23 share
drwxr-xr-x   4 richiethomas  staff    128 Oct  8 12:23 src
drwxr-xr-x  28 richiethomas  staff    896 Oct  8 12:23 test
-------------
gemfiles after install op:
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/unicorn
...
-------------
end of rehash results:
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bluepill
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bootsnap
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/bpsv
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/brakeman
...
-rwxr-xr-x  1 richiethomas  staff  393 Oct  8 19:29 /Users/richiethomas/.rbenv/shims/unicorn
...

Here we can see that the timestamp changes from the pre-determined one (1 Sept 2020) to today's date at the time I'm writing this (8 Oct 2022).

Hoping this helps clarify things. I'm happy to take a crack at writing a test to cover this as well, but I might do that tomorrow as my brain is fried for the day lol. Let me know if you have any questions, and thanks for your patience with this word salad of a PR thread. I hope you don't get an email every time there's an update to a comment here, otherwise I may have wrecked your inbox today. 😛

[mislav]

Thank you for the in-depth look into this, but the current functionality is intentional. Per the function's documentation:

If the contents of the prototype shim file differ from the contents of the first shim in the shims directory, assume rbenv has been upgraded and the existing shims need to be removed.

All shims in the shims directory will always have the same contents (it's impossible for rbenv rehash to produce any other result), so it doesn't make sense to check each of them; moreover, running diff on all the shims will slow down rehash considerably. Thus, the loop only checks the first one and breaks immediately.

[mislav]

These functions may not be used in this codebase anymore, but they are kept for backwards compatibility with plugins that may still use them.