[vim/vim] channel: term_start() out_cb/err_cb no longer deliver raw chunks (PR #20045)
h_east
Problem: channel: term_start() out_cb/err_cb no longer deliver raw
chunks (regression from patch 9.2.0224, breaks callers like
vim-fugitive that parse multi-line output).
Solution: Remove the PTY-specific per-line splitting in
may_invoke_callback() so RAW callbacks again receive the
raw chunk as returned by read(), preserving embedded NL.
If per-line handling is desired, the callback must split
"msg" on NL and strip the trailing CR itself; document
this behavior in term_start(). Replace
Test_term_start_cb_per_line() with
Test_term_start_cb_raw_chunk() to verify the raw-chunk
contract.
fixes: #20041
Note:
- Reflected on the "per-line split" added in #19776: it pushed too much responsibility (splitting and CR stripping) from the callback side into the channel core. This change reverts that behavior and restores the original RAW mode contract—passing raw chunks as-is.
- Explicitly documented the callback data format in
:help term_start().
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20045
Commit Summary
- 0cc3ab1 channel: term_start() out_cb/err_cb no longer deliver raw chunks
File Changes
(3 files)
- M runtime/doc/terminal.txt (14)
- M src/channel.c (41)
- M src/testdir/test_channel.vim (22)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20045)gosh, why are suddenly all those tests failing?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
benknoble left a comment (vim/vim#20045)I can confirm this fixes my issue. I also got (some) failures with make test; I didn't look to see if they were exactly the same as these.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20045)Not sure what local failures you have, but CI starts failing a few tests, #20050 is my attempt to fix those.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
benknoble left a comment (vim/vim#20045)Not sure what local failures you have, but CI starts failing a few tests, #20050 is my attempt to fix those.
Ah, interesting; meaning, they aren't the result of this PR?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20045)don't think so. Same failures started to appear here: #20039 which just added a text file and should not fail any test
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
benknoble left a comment (vim/vim#20045)FWIW, I tried a much smaller patch to just keep the newlines:
diff --git i/src/channel.c w/src/channel.c index dc8645b06..1d30c10d5 100644 --- i/src/channel.c +++ w/src/channel.c @@ -3525,9 +3525,6 @@ may_invoke_callback(channel_T *channel, ch_part_T part) while ((nl = vim_strchr(cp, NL)) != NULL) { long_u len = (long_u)(nl - cp); - - if (len > 0 && cp[len - 1] == CAR) - --len; argv[1].vval.v_string = vim_strnsave(cp, len); if (argv[1].vval.v_string != NULL) invoke_callback(channel, callback, argv); @@ -3537,9 +3534,6 @@ may_invoke_callback(channel_T *channel, ch_part_T part) if (*cp != NUL) { long_u len = STRLEN(cp); - - if (len > 0 && cp[len - 1] == CAR) - --len; argv[1].vval.v_string = vim_strnsave(cp, len); if (argv[1].vval.v_string != NULL) invoke_callback(channel, callback, argv);
and it breaks because the received data, when echo'd, overwrites each previous line. That is, you seen only the last line (you might see the flicker of prior lines as they are overwritten).
So I agree this is probably the way to go.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
benknoble left a comment (vim/vim#20045)Gah, I think I got the arithmetic wrong! The intermediate lines need 1 more len to keep the NL, right?
diff --git i/src/channel.c w/src/channel.c index dc8645b06..792586739 100644 --- i/src/channel.c +++ w/src/channel.c @@ -3524,10 +3524,7 @@ may_invoke_callback(channel_T *channel, ch_part_T part) while ((nl = vim_strchr(cp, NL)) != NULL) { - long_u len = (long_u)(nl - cp);
- - if (len > 0 && cp[len - 1] == CAR) - --len;
+ long_u len = (long_u)(nl - cp + 1);argv[1].vval.v_string = vim_strnsave(cp, len); if (argv[1].vval.v_string != NULL) invoke_callback(channel, callback, argv); @@ -3537,9 +3534,6 @@ may_invoke_callback(channel_T *channel, ch_part_T part) if (*cp != NUL) { long_u len = STRLEN(cp); - - if (len > 0 && cp[len - 1] == CAR) - --len; argv[1].vval.v_string = vim_strnsave(cp, len); if (argv[1].vval.v_string != NULL) invoke_callback(channel, callback, argv);
This patch also works for me. I didn't run the tests but will do so now (might have to step away).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
benknoble left a comment (vim/vim#20045)With the extra test-fix (that I can't well justify) on top, this passed the tests that pass for me prior to this change.
diff --git i/src/testdir/test_channel.vim w/src/testdir/test_channel.vim index 23f8a0fc3..20565fa27 100644 --- i/src/testdir/test_channel.vim +++ w/src/testdir/test_channel.vim @@ -2933,8 +2933,8 @@ func Test_error_callback_terminal() unlet! g:out g:error endfunc -" Verify that term_start() with out_cb/err_cb delivers one line per callback -" call (no embedded newlines, no trailing CR), matching the user's expectation. +" Verify that term_start() with out_cb/err_cb delivers one RAW line per +" callback call, matching the user's expectation. func Test_term_start_cb_per_line() CheckUnix CheckFeature terminal @@ -2948,8 +2948,7 @@ func Test_term_start_cb_per_line() \ 'out_cb': {ch, msg -> add(g:Ch_msgs, msg)}, \ 'err_cb': {ch, msg -> add(g:Ch_msgs, msg)}}) call WaitForAssert({-> assert_equal(3, len(g:Ch_msgs))}, 5000) - " Each line must arrive as a separate callback call with no embedded CR/NL. - call assert_equal(['err:1', 'err:2', 'out:3'], g:Ch_msgs) + call assert_equal(["err:1\n", "err:2\n", "out:3\r\n"], g:Ch_msgs) call job_stop(term_getjob(ptybuf)) unlet g:Ch_msgs endfunc
Possibly this PR is still the better of the 2 approaches, I'm not sure.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.