External editor does not work with command line argument

[mc-butler]

Important

This issue was migrated from Trac:

Origin	https://midnight-commander.org/ticket/4533
Reporter	Dhalgren (th.hammer@….net)

When using an external editor for the "Edit" command (i.e. "Use internal edit" in the Configure Options is unchecked) the environment variable EDITOR is used. However, if $EDITOR contains a command line argument after the executable name, these arguments are not processed properly, and the editor might not be started at all.
How to reproduce:
(Precondition: vi is available on the system)
1.) On the command line, execute: export EDITOR="vi +" && mc
(the + argument should let vi start at the document's end instead of the beginning)
2.) Go to the Options menu -> Configuration -> uncheck "Use internal edit"
3.) Move the cursor to a file that is larger than a single screen (e.g. ABOUT-NLS in mc's source directory)
4.) Press F4 to start the external editor
Result: Nothing visible happens
Expected result: vi is opened showing the end of the file ABOUT-NLS

Output of LC_MESSAGES=C mc -V:

GNU Midnight Commander 4.8.30
Built with GLib 2.78.3
Built with S-Lang 2.3.3 with terminfo database
Built with libssh2 1.11.0
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
With ext2fs attributes support
Virtual File Systems:
 cpiofs, tarfs, sfs, extfs, ftpfs, sftpfs
Data types:
 char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

Output of LC_MESSAGES=C mc -F:

Home directory: /home/user
Profile root directory: /home/user

[System data]
    Config directory: /etc/mc/
    Data directory:   /usr/share/mc/
    File extension handlers: /usr/libexec/mc/ext.d/
    VFS plugins and scripts: /usr/libexec/mc/
	extfs.d:        /usr/libexec/mc/extfs.d/

[User data]
    Config directory: /home/user/.config/mc/
    Data directory:   /home/user/.local/share/mc/
	skins:          /home/user/.local/share/mc/skins/
	extfs.d:        /home/user/.local/share/mc/extfs.d/
	mcedit macros:  /home/user/.local/share/mc/mc.macros
	mcedit external macros: /home/user/.local/share/mc/mcedit/macros.d/macro.*
    Cache directory:  /home/user/.cache/mc/

I have determined the cause of the problem and will provide a fix (which works for a single command line argument) via Pull Request.
Summary: In function utilunix.c:my_system_make_arg_array the GPtrArray storing the contents of argv is not filled correctly.

Note

Original attachments:

• 4533_ext_editor_cmd_line_args.patch (raw) by Dhalgren (th.hammer@….net) on Mar 28, 2024 at 6:43 UTC

[mc-butler]

Changed by Dhalgren (th.hammer@….net) on Mar 28, 2024 at 6:43 UTC

• Attachment 4533_ext_editor_cmd_line_args.patch (raw) added

[mc-butler]

Changed by Dhalgren (th.hammer@….net) on Mar 28, 2024 at 6:44 UTC (comment 1)

I see you don't use Pull Requests. I've attached a patch file.

[mc-butler]

Changed by zaytsev (@zyv) on Mar 28, 2024 at 14:21 UTC (comment 2)

Thank you for the patch. Is this function covered by tests? Can you add a test?

[mc-butler]

Changed by andrew_b (@aborodin) on Mar 30, 2024 at 6:29 UTC (comment 3)

Unfortunately, the proposed patch is designed for single argument only. If more than one arguments are in $EDITOR: EDITOR="vi -R +", external editor isn't invocated at all.

The full-featured tokenizing in my_system_make_arg_array() is required.

[mc-butler]

Changed by Dhalgren (th.hammer@….net) on Mar 30, 2024 at 8:50 UTC (comment 4)

@zaytsev:
It seems that there is a test "execute_execute_external_editor_or_viewer" which pretends to test the function do_executev which subsequently calls the function containing the bug. But actually, only a mock version of do_executev is called which just copies the argv arguments that are passed in. This does not really test the execution of an external editor.
Afterwards it is (erroneously) checked that the arguments are contained in argv starting with index 0 (instead of 1). Of course one could adapt this check, but still the actual call and the fixed code would not be tested.

@andrew_b:
I would say this fix is better than no fix at all. But of course you decide if you want to accept it.

[mc-butler]

Changed by andrew_b (@aborodin) on Mar 31, 2024 at 15:53 UTC (comment 5)

• Owner set to andrew_b
• Description edited
• Milestone changed from Future Releases to 4.8.32
• Status changed from new to accepted

[mc-butler]

Changed by andrew_b (@aborodin) on Mar 31, 2024 at 16:09 UTC (comment 6)

• Branch state changed from no branch to on review

Branch: 4533_external_editor_tokens
[44d8213]

[mc-butler]

Changed by andrew_b (@aborodin) on Apr 7, 2024 at 12:50 UTC (comment 7)

• Votes set to andrew_b
• Branch state changed from on review to approved

[mc-butler]

Changed by andrew_b (@aborodin) on Apr 7, 2024 at 12:52 UTC (comment 8)

• Branch state changed from approved to merged
• Votes changed from andrew_b to committed-master
• Resolution set to fixed
• Status changed from accepted to testing

Merged to master: [7b3c427].

[mc-butler]

Changed by andrew_b (@aborodin) on Apr 7, 2024 at 12:54 UTC (comment 9)

• Status changed from testing to closed

[mc-butler]

Changed by zaytsev (@zyv) on Apr 7, 2024 at 19:34 UTC (comment 10)

Thank you very much, Andrew - especially for the tests!