Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(autochdir): extract CWD from term:// URI #16989

Closed
wants to merge 1 commit into from

Conversation

zeertzjq
Copy link
Member

@zeertzjq zeertzjq commented Jan 8, 2022

Inspired by #2252 and #16740.

@zeertzjq zeertzjq added the terminal built-in :terminal or :shell label Jan 8, 2022
@zeertzjq zeertzjq marked this pull request as ready for review January 8, 2022 04:38
Copy link
Member

@glacambre glacambre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for making this change, and sorry it took me a long time to review. I rebased #16740 on top of your PR, and then applied the following change:

diff --git a/src/nvim/eval/funcs.c b/src/nvim/eval/funcs.c
index 7701688b4..6961c3c29 100644
--- a/src/nvim/eval/funcs.c
+++ b/src/nvim/eval/funcs.c
@@ -11476,7 +11476,7 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   // the 'swapfile' option to ensure no swap file will be created
   curbuf->b_p_swf = false;
   (void)setfname(curbuf, NameBuff, NULL, true);
-  // Save the job id and pid in b:terminal_job_{id,pid}
+  // Save the job id, pid and command in b:terminal_job_{id,pid,cmd}
   Error err = ERROR_INIT;
   // deprecated: use 'channel' buffer option
   dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_id"),
@@ -11485,6 +11485,9 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_pid"),
                INTEGER_OBJ(pid), false, false, &err);
   api_clear_error(&err);
+  dict_set_var(curbuf->b_vars, cstr_as_string("terminal_job_cmd"),
+               STRING_OBJ(cstr_to_string(cmd)), false, false, &err);
+  api_clear_error(&err);
 
   channel_terminal_open(curbuf, chan);
   channel_create_event(chan, NULL);
diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c
index d1113e031..5ff74e393 100644
--- a/src/nvim/terminal.c
+++ b/src/nvim/terminal.c
@@ -186,16 +186,24 @@ int handle_osc_7(const char *command, size_t cmdlen, void *user)
     first_separator += 1;
   }
 
-  // Copy new directory to buffer local dir
   buf_T *buf = handle_get_buffer(term->buf_handle);
-  xfree(buf->b_ffname);
-  command_end[0] = '/';
-  command_end[1] = '\0';
-  buf->b_ffname = (char_u *)xstrdup(first_separator);
+
+  varnumber_T pid = tv_dict_get_number(buf->b_vars, "terminal_job_pid");
+  char* cmd = tv_dict_get_string(buf->b_vars, "terminal_job_cmd", true);
+
+  command_end[0] = '\0';
+  snprintf((char *)NameBuff, sizeof(NameBuff), "term://%s//%li:%s",
+      (char *)first_separator, pid, cmd);
   command_end[0] = '\033';
-  command_end[1] = '\\';
 
-  do_autochdir();
+  xfree(cmd);
+  xfree(buf->b_ffname);
+  buf->b_ffname = vim_strsave(NameBuff);
+
+  do_autochdir();
 
   return 1;
 }

And it works!

That, combined with the fact that I'm not seeing anything wrong in your code, leads me to think that this is ready to be merged :)

@zeertzjq zeertzjq requested review from gpanders and removed request for jamessan and bfredl January 10, 2022 22:02
@zeertzjq zeertzjq removed the request for review from gpanders January 19, 2022 09:59
@zeertzjq
Copy link
Member Author

zeertzjq commented Feb 3, 2022

I recently noticed that Vim has an 'autoshelldir' option, so I'm now a bit hesitant about this. The term:// URI may also be subject to future changes.

@zeertzjq zeertzjq marked this pull request as draft February 3, 2022 01:27
@zeertzjq zeertzjq added the needs:discussion For PRs that propose significant changes to some part of the architecture or API label Feb 3, 2022
@glacambre
Copy link
Member

@zeertzjq I think I agree. I'll try to port vim patch v8.2.2675 and close my PR if I succeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:discussion For PRs that propose significant changes to some part of the architecture or API terminal built-in :terminal or :shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants