[vim/vim] sync terminal buffer's cwd with shell's pwd (#6290)
Coiby
In current version of vim, when a new terminal instance is started, the cwd will be set to ~. On Linux, when there is the change of current working directory, the shell will notify the terminal by the OSC 7; command. Use this feature to chdir buffer's cwd to the shell's working directory.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/6290
Commit Summary
- sync terminal buffer's cwd with shell's pwd
- add test
File Changes
- M src/terminal.c (29)
- M src/testdir/test_terminal.vim (14)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
lgtm-com[bot]
This pull request introduces 1 alert when merging f012d10 into 3d9207a - view on LGTM.com
new alerts:
- 1 for Constant return type
codecov[bot]
Codecov Report
Merging #6290 (991d775) into master (52c124d) will decrease coverage by
0.09%.
The diff coverage is73.07%.
@@ Coverage Diff @@ ## master #6290 +/- ## ========================================== - Coverage 88.96% 88.87% -0.10% ========================================== Files 148 148 Lines 163329 161309 -2020 ========================================== - Hits 145302 143357 -1945 + Misses 18027 17952 -75
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 86.10% <ø> (-1.89%) |
⬇️ |
| huge-gcc-none | 88.38% <73.07%> (+<0.01%) |
⬆️ |
| huge-gcc-testgui | 86.82% <73.07%> (-0.01%) |
⬇️ |
| huge-gcc-unittests | 2.49% <0.00%> (-0.01%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/terminal.c | 90.69% <73.07%> (-0.26%) |
⬇️ |
| src/help.c | 89.76% <0.00%> (-1.08%) |
⬇️ |
| src/textobject.c | 91.69% <0.00%> (-0.71%) |
⬇️ |
| src/gui.c | 61.62% <0.00%> (-0.58%) |
⬇️ |
| src/ops.c | 91.80% <0.00%> (-0.57%) |
⬇️ |
| src/ex_cmds2.c | 90.68% <0.00%> (-0.54%) |
⬇️ |
| src/move.c | 93.58% <0.00%> (-0.54%) |
⬇️ |
| src/bufwrite.c | 76.47% <0.00%> (-0.53%) |
⬇️ |
| src/drawline.c | 84.49% <0.00%> (-0.47%) |
⬇️ |
| src/getchar.c | 85.72% <0.00%> (-0.46%) |
⬇️ |
| ... and 97 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 52c124d...991d775. Read the comment docs.
Bram Moolenaar
I do not see a change to the help files - what will the user experience?
Changing directories can have side effects. Perhaps it should be possible to disable this feature?
Or it should be disabled by default and only used when the user wants it?
Coiby
I do not see a change to the help files - what will the user experience?
Changing directories can have side effects. Perhaps it should be possible to disable this feature?
Or it should be disabled by default and only used when the user wants it?
Thank you for the suggestion! Change applied.
Coiby
@brammool glib provides g_filename_from_uri to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
- borrow the code of g_filename_from_uri (~100 lines) from glib
- only enable this feature for glib
Which one do you prefer?
Bram Moolenaar
> @brammool glib provides [g_filename_from_uri](https://github.com/GNOME/glib/blob/ce005e83c64d3d4fbf482f4bfd27a8aefc50b719/glib/gconvert.c#L1621) to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
> - borrow the code of g_filename_from_uri (~100 lines) from glib
> - only enable this feature for glib
>
> Which one do you prefer?
here and you want to replace it?
I believe glib is GPL licensed, thus we can't just copy that code. You
can look at the code and rewrite it (leave out parts we don't need,
etc.).
There are many places where glib won't be available, disabling this
feature there would be disappointing.
The uri_decode() I see in the patch mentions it's coming from github.
Did you check the license on that code? It's also possible to rewrite
that code. You need to use "char_u" instead of "unsigned char", for
example. And we don't use "const" much in Vim code.
--
Eight Megabytes And Continually Swapping.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Coiby
>
>> @brammool glib provides [g_filename_from_uri](https://github.com/GNOME/glib/blob/ce005e83c64d3d4fbf482f4bfd27a8aefc50b719/glib/gconvert.c#L1621) to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
>> - borrow the code of g_filename_from_uri (~100 lines) from glib
>> - only enable this feature for glib
>>
>> Which one do you prefer?
>
>In the patch I see syn_cwd() already doing this. Is something missing
>here and you want to replace it?
>
with g_filename_from_uri file://host/path or other corner cases as shown
in the test suite for g_filename_from_uri [1].
[1] http://web.mit.edu/barnowl/src/glib/glib-2.16.3/tests/uri-test.c
>I believe glib is GPL licensed, thus we can't just copy that code. You
>can look at the code and rewrite it (leave out parts we don't need,
>etc.).
>
>There are many places where glib won't be available, disabling this
>feature there would be disappointing.
>
>The uri_decode() I see in the patch mentions it's coming from github.
>Did you check the license on that code? It's also possible to rewrite
>that code. You need to use "char_u" instead of "unsigned char", for
>example. And we don't use "const" much in Vim code.
>
programming tips for Vim.
>--
>Eight Megabytes And Continually Swapping.
>
> /// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
>/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
>\\\ an exciting new programming language -- http://www.Zimbu.org ///
> \\\ help me help AIDS victims -- http://ICCF-Holland.org ///
>
>
>You are receiving this because you authored the thread.
>Reply to this email directly or view it on GitHub:
>https://github.com/vim/vim/pull/6290#issuecomment-772595015
--
Best regards,
Coiby
K.Takata
@k-takata commented on this pull request.
> @@ -4836,7 +4842,7 @@ A jump table for the options with a short description can be found at |Q_op|. The cursor is displayed at the start of the space a Tab character occupies, not at the end as usual in Normal mode. To get this cursor position while displaying Tabs with spaces, use: > - :set list lcs=tab:\ \ + :set list lcs=tab:\ \
The last space is intentional. Should not be removed.
> @@ -6802,7 +6808,7 @@ A jump table for the options with a short description can be found at |Q_op|. feature} String to put at the start of lines that have been wrapped. Useful values are "> " or "+++ ": > - :set showbreak=>\ + :set showbreak=>\
Same as above.
Coiby
@coiby commented on this pull request.
> @@ -6802,7 +6808,7 @@ A jump table for the options with a short description can be found at |Q_op|. feature} String to put at the start of lines that have been wrapped. Useful values are "> " or "+++ ": > - :set showbreak=>\ + :set showbreak=>\
Thank you for spotting this issue! I removed it unintentionally because I set up vim to automatically remove trailing spaces. I''l send another patch to revert this change.
Coiby
@coiby pushed 4 commits.
- 7961f27 implementing URL decoding for shell's OSC 7 escape sequence
- f6e0e22 sync terminal buffer's cwd with shell's pwd
- a337fa7 Provide global option acds to enable automically setting cwd to shell's
- abc8edf add test for sync terminal buffer cwd with shell's pwd
—
You are receiving this because you are subscribed to this thread.
Coiby
Coiby
@brammool It turns out we needn't to deal with all the corner cases listed in http://web.mit.edu/barnowl/src/glib/glib-2.16.3/tests/uri-test.c since we can assume the emitted OSC 7 escape sequence is correctly uri-encoded. If there's something wrong, whoever emits the OSC 7 escape sequence should take the responsibility. So the license issues have been resolved.
I've also fixed the test failure for FreeBSD. But there is one failed test on macos (huge, clang). Unfortunately, I don't have a Mac system to test on.
LemonBoy
@LemonBoy commented on this pull request.
In src/terminal.c:
> +{
+ int offset = 7; // len of "file://" is 7
+ char *pos = (char *)frag->str + offset;
+
+ // remove HOSTNAME to get PWD
+ while (*pos != '/' && offset < frag->len) {
+ offset += 1;
+ pos += 1;
+ }
+
+ if (offset >= frag->len) {
+ semsg("Failed to extract PWD from %s, please check your shell's config related OSC 7", frag->str);
+ return;
+ }
+
+ char_u *new_dir = malloc(frag->len - offset + 1);
This variable should be declared at the top of the file, that'd fix the C2275 error on Appveyor.
Coiby
@coiby commented on this pull request.
In src/terminal.c:
> +{
+ int offset = 7; // len of "file://" is 7
+ char *pos = (char *)frag->str + offset;
+
+ // remove HOSTNAME to get PWD
+ while (*pos != '/' && offset < frag->len) {
+ offset += 1;
+ pos += 1;
+ }
+
+ if (offset >= frag->len) {
+ semsg("Failed to extract PWD from %s, please check your shell's config related OSC 7", frag->str);
+ return;
+ }
+
+ char_u *new_dir = malloc(frag->len - offset + 1);
Thanks for providing the fix!
Bram Moolenaar
The name 'autochdirsh' is hard to read. How about making it 'autoshelldir' ?
Please use the Vim usual C code formatting. E.g. "} else {" is written in three lines.
"char buffer[3] = {src[i+1], src[i+2], 0};" I'm not sure all C90 compilers accept this.
You can use the hexhex2nr() function.
syn_cwd() should be renamed to sync_shell_dir().
The test should probably CheckNotBSD now.
Coiby
@coiby pushed 6 commits.
- 8148be7 use vim's hexhex2nr to replace strtol
- 11ba400 syn_cwd renamed to sync_shell_dir
- 9d29e0c autochdirsh replaced with autoshelldir
- f0e7096 use URL-encoded chars to test url_decode
- 7019f71 use the Vim usual C code formatting
- 7f3f12a syn_pwd renamed to sync_shell_dir in test
—
You are receiving this because you are subscribed to this thread.
Coiby
>The name 'autochdirsh' is hard to read. How about making it 'autoshelldir' ?
>Please use the Vim usual C code formatting. E.g. "} else {" is written in three lines.
>
>"char buffer[3] = {src[i+1], src[i+2], 0};" I'm not sure all C90 compilers accept this.
>You can use the hexhex2nr() function.
>
>syn_cwd() should be renamed to sync_shell_dir().
>
hexhex2nr also reminds me previously the test case didn't cover
part of code in url_decode. So I've added some percent-code
characters (Chinese "中文") to the path in the test.
>The test should probably CheckNotBSD now.
>
sequence. If we use bash for the test in FreeBSD, the test could
pass. Since vim use sh for testing, the test has to be disabled for
FreeBSD.
>
>--
>Reply to this email directly or view it on GitHub:
--
Best regards,
Coiby
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
For the change in parse_osc(), we should perhaps always recognize the code, but ignore it when p_asd is not set.
Or is the code dropped anyway when we don't recognize it?
Coiby
For the change in parse_osc(), we should perhaps always recognize the code, but ignore it when p_asd is not set.
Or is the code dropped anyway when we don't recognize it?
Thank you for spotting this issue. Yes, to be consistent with OSC 52, it seems we should return 1 as long as osc 7 is recognized regardless of p_asd being set or not.
But at least right now, what's returned doesn't matter because string_fragment doesn't check what's returned according to https://github.com/vim/vim/blob/fa79be6b10e1d34fd697a56e85f6c0ce101f3d62/src/libvterm/src/parser.c#L76
static void string_fragment(VTerm *vt, const char *str, size_t len, int final) { ... switch(vt->parser.state) { case OSC: if(vt->parser.callbacks && vt->parser.callbacks->osc) (*vt->parser.callbacks->osc)(vt->parser.v.osc.command, frag, vt->parser.cbdata); break;
Bram Moolenaar
You need to adjust the #ifdef for hexhex2nr(). See test failures.
Coiby
You need to adjust the #ifdef for hexhex2nr(). See test failures.
Thanks. I have used FEAT_AUTOSHELLDIR to enable hexhex2nr to fix this kind of test failures.
commit 4355894 ("patch 8.2.2627: no need to check for BSD after checking for not root") removed CheckNotBSD. What can I do to skip the tests for BSD (CheckNotRoot doesn't work)?
—
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> > You need to adjust the #ifdef for hexhex2nr(). See test failures.
>
> Thanks. I have used FEAT_AUTOSHELLDIR to enable hexhex2nr to fix this kind of test failures.
>
> need to check for BSD after checking for not root") removed
> CheckNotBSD. What can I do to skip the tests for BSD (CheckNotRoot
> doesn't work)?
I see a test failure on Linux, looks like there is still something to
fix.
--
"Space is big. Really big. You just won't believe how vastly hugely mind-
bogglingly big it is. I mean, you may think it's a long way down the
road to the chemist, but that's just peanuts to space."
-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy" \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
Bram Moolenaar
Looks OK now. We don't need a feature for this, just check that the 'autoshelldir' option works.
—
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
8b9abfd#r48888218
// should not recover to default value later?
Shane-XB-Qian
not sure (or not understand) the user case of this change too much,
but the doc of this change mentioned to source '/etc/profile.d/vte.sh' (ubuntu20.04 is vte-2.91.sh), BTW: be careful to do this, this looks had bug or flaw, PROMPT_COMMAND="__vte_prompt_command" may overwrite your previous 'PROMOT_COMMAND', and if it was not login shell then maynot find '__vte_prompt_command', note the order or timing of/to 'source'......
Coiby
8b9abfd#r48888218
// should not recover to default value later?
Do you mean there should be an "set noacd" after finishing the test?
Coiby
not sure (or not understand) the user case of this change too much,
but the doc of this change mentioned to source '/etc/profile.d/vte.sh' (ubuntu20.04 is vte-2.91.sh), BTW: be careful to do this, this looks had bug or flaw,PROMPT_COMMAND="__vte_prompt_command"may overwrite your previous 'PROMOT_COMMAND', and if it was not login shell then maynot find '__vte_prompt_command', note the order or timing of/to 'source'......
Thanks! It seems I should have mentioned this issue in the doc which according to https://gitlab.gnome.org/GNOME/vte/-/issues/37 is still not resolved.
Coiby
Looks OK now. We don't need a feature for this, just check that the 'autoshelldir' option works.
Thanks! I'm not sure I understand you correctly. Do you mean FEAT_AUTOSHELLDIR is not needed?