-
Notifications
You must be signed in to change notification settings - Fork 553
Description
Line 375 in c049fb6
system(merge_tool, temp.path, destination) |
I was trying to update a rails app with THOR_MERGE="nvim -d" bin/rails app:update
and the m
merge option didn't work.
Since I needed to be able to merge the changes but also didn't want to do it manually with the d
option I had a look around and probably found a regression from the last change starting with ruby >= 3.3.
Starting in ruby >= 3.3 the first required argument to system()
has some constraints to it:
command_line
is a command line to be passed to a shell; it must begin with a shell reserved word, begin with a special built-in, or contain meta charactersexe_path
string path to an executable or 2-element array containing the path to an executable and the string to be used as the name of the executing process
See here
nvim -d
is neither a builtin, nor a reserved word and apparently -
is not a meta character. It's also not a exe_path
so the line only returns nil
Creating a bash script in my PATH
which wraps nvim -d
fixes it for me, probably because it is now considered an exe_path
. An alias
did not fix it.
# This does work
tool_string = "nvim -d #{temp.path} #{destination}"
system(tool_string)
# This does work
# Script in PATH "~/.local/bin/nvimdiff"
#
# /usr/bin/env bash
# nvim -d $1 $2
#
system("nvimdiff", temp.path, destination)
# This does NOT work
# THOR_MERGE="nvim -d"
#
# The reason probably is that merge_tool is now neither a `command_line` nor an `exe_path`
# system(merge_tool, temp.path, destination)
# This does work, same as `system(tool_string)`
system %(#{merge_tool} #{temp.path} #{destination})
I was hoping to provide a pull-request, but I'm not sure what the correct fix or behavior would look like when MERGE_TOOL
includes short or long option arguments for the tool used.
Maybe creating wrapper scripts in PATH
and documenting the behavior is the fix going forward.
Nonetheless I wanted to leave something here in case someone else is struggling to use a merge tool.