fix: Fix plugin-test arg parsing #1084
Conversation
Should fix asdf-vm#1083.
raxod502
force-pushed
the
rr-fix-plugin-test
branch
from
5e2c0cb
to
95098be
Compare
|
can we capture this scenario as a test for regression purposes? |
| ;; | ||
| --asdf-tool-version) | ||
| tool_version="$2" | ||
| shift 2 |
There was a problem hiding this comment.
I don't understand why these lines didn't work. Can you explain what was wrong with the previous code? New code looks good, but it seems like it should be equivalent to the old code.
There was a problem hiding this comment.
Well, I feel less guilty for failing to realize the bugs last time :)
Two issues with the old code:
- We
shiftat the beginning of the loop, so although$1and${arg}are equivalent at the beginning of the loop, they are no longer equivalent aftershift. Therefore we should be using$1instead of$2. - Even if you fix that, modifying the args array in place does not affect the iteration, so even if you
shifta second time to skip over the option argument, it still shows up as the next value for${arg}. Hence the requirement to use a bool to track whether to skip the next argument.
There was a problem hiding this comment.
You'll notice that all of this is horribly obtuse, this is why writing pure POSIX compatible scripts is not so popular these days!
There was a problem hiding this comment.
You'll notice that all of this is horribly obtuse, this is why writing pure POSIX compatible scripts is not so popular these days!
Indeed 😅
There was a problem hiding this comment.
@raxod502 just curious, where does arg get set? I don't see it.
There was a problem hiding this comment.
Would a pattern like this be better? https://github.com/Stratus3D/dotfiles/blob/master/scripts/tools/to_server#L101-L143 (this is a pattern I've repeatedly used because it doesn't require setting any temporary variables. Only $1, $2, and shift are used.
There was a problem hiding this comment.
arggets set by thefor argat the top of the loop. It's equivalent tofor arg in "$@".- No, unfortunately, we can't do it the reasonable way, because we need to save an array of the command to run, for execution later, and unfortunately POSIX sh only supports having one array variable in your whole program, namely
$@. So we have to do all of the manipulation in place.
|
@jthegedus Added a regression test and verified that it fails before the changes from this PR are applied! The license file is needed because |
|
Thanks @raxod502 |
Should fix #1083.