%sh{} trims newlines from output

[cole-h]

Steps

1. :reg a "asdf<c-v><ret>"<ret>
2. :echo %reg{a} -> asdf␊ (ends with LF)
3. :echo %sh{echo "$kak_main_reg_a"} -> asdf (doesn't end with LF)
4. :nop %sh{echo "$kak_main_reg_a" > /tmp/kak-out} -> /tmp/kak-out contains asdf and an empty line below it
5. :echo %sh{cat /tmp/kak-out} -> asdf (doesn't end with LF)

Outcome

See above.

Expected

%sh{} should not trim newlines from the output.

My use case is for integration with my system clipboard. By hooking into the " register's contents, it is possible to copy the contents of that register every time it is modified (and thus copying it to my system's clipboard). However, %sh{} eats the final newline, it becomes a pain to paste from the system clipboard (e.g. moving a line elsewhere) into kakoune because the formatting gets all messed up.

• yank some full line, e.g. with <a-x>y
• paste that line and notice how it appears on the line below
• :reg dquote %sh{echo "$kak_main_reg_dquote"}
• paste again and notice how it appears after the cursor instead of on the next line

[cole-h]

The issue arises from expand_token in src/command_manager.cc -- changing the trailing_eol_count from 0 to -1 (or adding 1 to the final trailing_eol_count in the resize() call) fixes this issue (at least in the :echo %sh{echo "$kak_main_reg_dquote"} case).

This trimming was introduced in 471c75d as a fix for #698. I'd argue that there should be a way to opt for either behavior, somehow, or a plain revert of that commit.

If one wishes to trim the trailing newlines like POSIX subshells, shouldn't they execute their code in a POSIX subshell (e.g. printf %s $( ( echo asdf ) ) | hexdump -C -> 61 73 64 66)? I think of %sh{} as the same as sh -c '...', and sh -c 'echo asdf' | hexdump -C -> 61 73 64 66 0a.

[lenormf]

This behaviour is intentional, it makes %sh{…} expansions similar to subshells, c.f. #698.

Related #3470, #3669.

[cole-h]

I get that it's intentional. Would it be acceptable to introduce an option that would change this behavior? I guess it hasn't been a problem 'til now, but it breaks a few use cases (one of which being "custom" clipboard integration as I demonstrate above).

If a new option would be unacceptable, then I guess this should be closed, and I'll have to wait for #3935 to be resolved (or try my hand at resolving it).

---

FWIW, the following patch (basically reverting 471c75d) works as I'd expect and only required a minor change to plug.kak (cross-linked above):

Patch

From 8976fec582a178e564c501dac2820660ddf199a6 Mon Sep 17 00:00:00 2001
From: Cole Helbling <[email protected]>
Date: Thu, 24 Dec 2020 16:28:15 -0800
Subject: [PATCH] command_manager: don't trim shell expansion newlines

---
 src/command_manager.cc | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/command_manager.cc b/src/command_manager.cc
index ca619f7f..883012ea 100644
--- a/src/command_manager.cc
+++ b/src/command_manager.cc
@@ -319,19 +319,9 @@ expand_token(const Token& token, const Context& context, const ShellContext& she
     {
     case Token::Type::ShellExpand:
     {
-        auto str = ShellManager::instance().eval(
+        return {ShellManager::instance().eval(
             content, context, {}, ShellManager::Flags::WaitForStdout,
-            shell_context).first;
-
-        int trailing_eol_count = 0;
-        for (auto c : str | reverse())
-        {
-            if (c != '\n')
-                break;
-            ++trailing_eol_count;
-        }
-        str.resize(str.length() - trailing_eol_count, 0);
-        return {str};
+            shell_context).first};
     }
     case Token::Type::RegisterExpand:
         if constexpr (single)
--
2.29.2

---

EDIT: And a diff for enabling this based on an option:

Diff

diff --git a/src/command_manager.cc b/src/command_manager.cc
index ca619f7f..2be3e5e4 100644
--- a/src/command_manager.cc
+++ b/src/command_manager.cc
@@ -323,14 +323,19 @@ expand_token(const Token& token, const Context& context, const ShellContext& she
             content, context, {}, ShellManager::Flags::WaitForStdout,
             shell_context).first;
 
-        int trailing_eol_count = 0;
-        for (auto c : str | reverse())
-        {
-            if (c != '\n')
-                break;
-            ++trailing_eol_count;
+        bool trim_newlines = context.options()["shell_expansion_trim_newlines"].get<bool>();
+
+        if (trim_newlines) {
+            int trailing_eol_count = 0;
+            for (auto c : str | reverse())
+            {
+                if (c != '\n')
+                    break;
+                ++trailing_eol_count;
+            }
+            str.resize(str.length() - trailing_eol_count, 0);
         }
-        str.resize(str.length() - trailing_eol_count, 0);
+
         return {str};
     }
     case Token::Type::RegisterExpand:
diff --git a/src/main.cc b/src/main.cc
index fbb074a0..c9496d93 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -563,6 +563,8 @@ void register_options()
         "set of pair of characters to be considered as matching pairs",
         { '(', ')', '{', '}', '[', ']', '<', '>' });
     reg.declare_option<int>("startup_info_version", "version up to which startup info changes should be hidden", 0);
+    reg.declare_option("shell_expansion_trim_newlines",
+                       "whether or not to trim newlines from %sh{...} scopes", true);
 }
 
 static Client* local_client = nullptr;

[cole-h]

I wonder if an acceptable compromise could be trimming only the last newline (vs. "trimming-on" and "trimming-off"). This way, it still somewhat acts like a subshell in that it trims a singular newline (and all scripts that use echo rather than printf won't break), but also behaves similar to (neo)vim's :.!echo -e 'asdf\n\n\n' (except that it keeps all newlines).

EDIT: I only bring up (neo)vim because of the design document that discusses vim compatibility. I don't know if this subshell-like behavior is self-consistent, though.

[nonumeros]

but also behaves similar to (neo)vim's :.!echo -e 'asdf\n\n\n' (except that it keeps all newlines).

But what you're asking for is a contradiction. Because if you were to
echo %sh{echo "$kak_main_reg_a<c-v><ret><c-v><ret><c-v><ret>"} would return asdf which is exactly what you were trying to avoid in the first place.

And how is it possible that echo %reg{a} returns a ␊

I'm not so sure it trims the newlines either. I mean. If you were to have echo %sh{echo "$kak_main_reg_a<U+0020>" (And I guess you know what that is. It's space. And Surprise! It gives you asdf␤

But I'm not saying you may be wrong. But the sparse few use cases you're having may not warrant reverting something that's been working for years.

[mawww]

Hello,

Having such a core behaviour depend on an option is not desirable, as it means plugins can randomly break based off user configuration.

I think the best approach might be as you suggested to just remove the last end-of-line, so that printf %s\n <whatever> is guaranteed to preserve <whatever>.

[cole-h]

Thanks for chiming in, @mawww. I've opened #4005, which removes only the final eol character in %sh{} expansions.