Bug: Can't use quotes in `ui.editor`/`$EDITOR`

[Cretezy]

Description

Using one of the following does not apply arguments correctly:

ui.editor = "nvim --cmd 'let g:flatten_wait=1'"
# or
ui.editor = "code '-w'"

EDITOR="nvim --cmd 'let g:flatten_wait=1'"
# or
JJ_EDITOR="code '-w'" jj describe

Although this works:

ui.editor = ["nvim", "--cmd", "let g:flatten_wait=1"]
# or
ui.editor = ["code", "-w"]

Steps to Reproduce the Problem

1. Set ui.editor or $EDITOR to have quotes
2. Run jj describe

E.g. JJ_EDITOR="code '-w'" jj describe

Expected Behavior

Quoted arguments should be used correctly.

Actual Behavior

Quoted arguments are ignored (at least for the code '-w' example above, the nvim throws an error but that's likely because let isn't quoted properly).

Specifications

• Platform: Arch Linux, same behavior on macOS IIRC
• Version: 0.17.1/main

[Cretezy]

From a quick investigation, it seems to stem from run_ui_editor in cli/src/cli_util.rs.

It seems like CommandNameAndArgs::split_name_and_args naively splits on ' ' and doesn't handle any shell escaping (there's a TODO) there.

---

I believe using something like https://github.com/allenap/shell-quote or https://github.com/tmiasko/shell-words for parsing would be more robust.

I am willing to work on this bug fix. A few questions first:

1. Is it okay to bring in a new dependency to handle this?
2. If yes, which dependency should we bring in?
   1. https://github.com/allenap/shell-quote seems to be the most robust, but should we use the Bash or sh modules?
   2. https://github.com/tmiasko/shell-words seems to be simpler, but would likely handle all cases required.
   3. https://github.com/comex/rust-shlex is another alternative

[martinvonz]

What do other $EDITOR-supporting tools do?

[arxanas]

Is there a reason to continue to support string-based editor configuration, other than history/inertia? I guess one reason could be the interpolation of multiple arguments in a certain position, but I don't know if we have anything like that (like none of our $MAGIC_VARIABLES expand to multiple files, right?).

I might simply prefer to warn if an editor string has spaces that the user should use the list syntax instead.

Oh, okay, I see that ad-hoc JJ_EDITOR overrides can't really support structured lists, so I guess that's a legitimate reason... but maybe we'd prefer that users override the setting with --config-toml in those cases? Or that our environment variables support TOML somehow?

[martinvonz]

FWIW, I agree with @arxanas. That's why I asked what other tools do. If they don't bother to support it, then I don't think we need to bother either.

[Cretezy]

Git seems handles strings in GIT_EDITOR:

GIT_EDITOR="nvim --cmd 'let g:flatten_wait=1'" git commit
# or
GIT_EDITOR="code '-w'" git commit

Reference: https://github.com/git/git/blob/b9cfe4845cb2562584837bc0101c0ab76490a239/editor.c#L58C12-L58C35

[arxanas]

@martinvonz I think they generally do support it, by lexing the strings and splitting them like the shell might do it — but perhaps because they don't have a better alternative?

[martinvonz]

@martinvonz I think they generally do support it, by lexing the strings and splitting them like the shell might do it — but perhaps because they don't have a better alternative?

True. I think both Git and Mercurial have config formats with more limited value types (strings, integers, boolean values?).

[yuja]

AFAIK, Mercurial just passes string args to API like system() (which basically invokes sh -c "$args" on Unix-like systems.)

I don't think we'll need to support string version in config files forever, but we'll still need to parse $EDITOR, $PAGER, etc.

[arxanas]

The alternative that I would imagine is having JJ_EDITOR etc. only support single-string binary names, and then JJ_EDITOR_TOML would let you pass structured data, including arguments. But it sounds like a nightmare to use interactively.

Spawning the shell seems like a better solution than lexing the strings ourselves. We could hope that it would handle different lexing on Windows (hypothetically, I don't know if it would be a problem) or other platforms.

[yuja]

Spawning shell is easy only when full args string is available. We'll have to escape filename argument to construct a safe shell command string.

[Cretezy]

Although spawning a shell would work, I think splitting the string as a Bash-like shell would is simpler, and I can't think of any example which it wouldn't be enough (open to suggestions!).

[arxanas]

I think it's simpler only if we commit to adding a new dependency. For a project like jj, it has to carefully consider reliability of third-party code, supply chain attacks, licensing, dependency bloat (jj also has to be a library), etc. Google has some standard rules/checks as well, but not sure how much they encompass. (At least one check is to make sure that certain vulnerable libraries don't appear in the dependency graph, and it can be annoying to resolve those through layers of transitivity.)

[dbarnett]

Dupe of #1966, right?

Although this one has better description/explanations.

[martinvonz]

Dupe of #1966, right?

Yes, seems like it.

Although this one has better description/explanations.

Yes, probably better to mark that one a dupe of this one.

[heygarrett]

Now that there's a schema available for the config file, the workaround of using a list instead of a string raises a TOML error:

Even Better TOML: ["nvim","--cmd","let g:launched_by_shell=1"] is not of type "string"

[martinvonz]

That sounds like a bug in the config schema. Feel free to file a separate bug for that.

[indirect]

I am happy to confirm that not just git, but every other program I have ever used that respects $EDITOR works as I would expect it to work (passing the quoted value as a single argument to the editor binary), rather than splitting a quoted string into space-separated pieces. I guess I can work around this by manually adding an actual array to my jj config, but it seems unsporting to break in ways that other command line tools do not.