[RDY]Implement tab-local working directory feature. #3229

Merged
merged 1 commit into from Apr 22, 2016

Conversation

Projects
None yet
@HiPhish
Contributor

HiPhish commented Aug 24, 2015

The new :tcd command was requested by me in issue #3175. It adds tab-local working directories in addition to window-local ones.

I had already a pull request for this (#3178) but I messed up the commit history, so I deleted to old repository and I'm submitting the new one as ready to merge. Please delete the old pull request.

runtime/doc/editing.txt
+When changing tabs the same behaviour applies. If the current tab has no
+local working directory the global working directory is used. When a |:cd|
+command is used, the current window and tab will lose their local current
+directoriesies and will use the global current directory from now on. When a

This comment has been minimized.

@oni-link

oni-link Aug 24, 2015

Contributor

directoriesies?

@oni-link

oni-link Aug 24, 2015

Contributor

directoriesies?

This comment has been minimized.

@HiPhish

HiPhish Aug 24, 2015

Contributor

Fixed :P

@HiPhish

HiPhish Aug 24, 2015

Contributor

Fixed :P

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Aug 24, 2015

Contributor

What is it with the merge? You should rebase your work on neovim's master branch.

Contributor

oni-link commented Aug 24, 2015

What is it with the merge? You should rebase your work on neovim's master branch.

src/nvim/eval.c
@@ -10005,9 +10006,6 @@ static void f_has(typval_T *argvars, typval_T *rettv)
#ifdef UNIX
"unix",
#endif
-#if defined(WIN32)
- "win32",
-#endif

This comment has been minimized.

@ghost

ghost Aug 24, 2015

Make sure this is added back after you rebase.

@ghost

ghost Aug 24, 2015

Make sure this is added back after you rebase.

This comment has been minimized.

@HiPhish

HiPhish Aug 25, 2015

Contributor

Added it back, no idea how that could have gotten lost.

@HiPhish

HiPhish Aug 25, 2015

Contributor

Added it back, no idea how that could have gotten lost.

runtime/doc/eval.txt
+hastabdir() *hastabdir()*
+ The result is a Number, which is 1 when the current
+ tab has set a local path via |:tcd|, and 0 otherwise.
+

This comment has been minimized.

@splinterofchaos

splinterofchaos Aug 25, 2015

Member

What about :lcd?

@splinterofchaos

splinterofchaos Aug 25, 2015

Member

What about :lcd?

This comment has been minimized.

@HiPhish

HiPhish Aug 25, 2015

Contributor

No, only if a tab directory is set. For :lcd use haslocaldir(). A window can have a window-local directory and be in a tab without tab-local directory.

Personally I would have preferred changing haslocaldir() to return something like 0 for no local directory, 1 for window-local, 2 for tab-local and 3 for both, like a bitfield, but I think this is simpler than working with bitmasks. We can still later decide to have both.

@HiPhish

HiPhish Aug 25, 2015

Contributor

No, only if a tab directory is set. For :lcd use haslocaldir(). A window can have a window-local directory and be in a tab without tab-local directory.

Personally I would have preferred changing haslocaldir() to return something like 0 for no local directory, 1 for window-local, 2 for tab-local and 3 for both, like a bitfield, but I think this is simpler than working with bitmasks. We can still later decide to have both.

This comment has been minimized.

@justinmk

justinmk Sep 2, 2015

Member

I like the 0,1,2,3 idea.

  • It is backwards compatible
  • It avoids a new function
  • It emphasizes that tab-local is a special case of local.
@justinmk

justinmk Sep 2, 2015

Member

I like the 0,1,2,3 idea.

  • It is backwards compatible
  • It avoids a new function
  • It emphasizes that tab-local is a special case of local.
src/nvim/eval.c
+/// `hastabdir()` function
+static void f_hastabdir(typval_T *argvars, typval_T *rettv)
+{
+ rettv->vval.v_number = (curtab->localdir != NULL);

This comment has been minimized.

@splinterofchaos

splinterofchaos Aug 25, 2015

Member

Would it make sense to optionally supply a window number?

@splinterofchaos

splinterofchaos Aug 25, 2015

Member

Would it make sense to optionally supply a window number?

This comment has been minimized.

@HiPhish

HiPhish Aug 25, 2015

Contributor

Yes, but not in this feature request. I wanted to get :tcd to the same level as :cd and :lcd and everything from there on will affect all three, so it should be its own issue.

@HiPhish

HiPhish Aug 25, 2015

Contributor

Yes, but not in this feature request. I wanted to get :tcd to the same level as :cd and :lcd and everything from there on will affect all three, so it should be its own issue.

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Aug 25, 2015

Contributor

@oni-link I'm still struggling to figure out this rebasing thing and how it interacts with my remote, that's how I blew up my previous repository. Can you please tell me the commands to type and from which branch?

Contributor

HiPhish commented Aug 25, 2015

@oni-link I'm still struggling to figure out this rebasing thing and how it interacts with my remote, that's how I blew up my previous repository. Can you please tell me the commands to type and from which branch?

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Aug 25, 2015

Contributor

I tried to reuse your old commits from this PR to create a new branch that could replace ex-tcd. Here I create a temp branch based on neovim's master branch (called upstream) with your work, that is later renamed to ex-tcd:

git checkout upstream
git checkout -b ex-tcd2
git cherry-pick 58464263ed255f9572e1809305b672960cf68e40
git cherry-pick f64c811d0ddd72408020ec45cc6426c082e477fd
git branch -D ex-tcd
git branch -m ex-tcd

If you want to rebase your work on upstream in branch ex-tcd:

git checkout ex-tcd
git rebase upstream

If github won't let you push your new branch you can force this with option -f:

git push origin ex-tcd -f
Contributor

oni-link commented Aug 25, 2015

I tried to reuse your old commits from this PR to create a new branch that could replace ex-tcd. Here I create a temp branch based on neovim's master branch (called upstream) with your work, that is later renamed to ex-tcd:

git checkout upstream
git checkout -b ex-tcd2
git cherry-pick 58464263ed255f9572e1809305b672960cf68e40
git cherry-pick f64c811d0ddd72408020ec45cc6426c082e477fd
git branch -D ex-tcd
git branch -m ex-tcd

If you want to rebase your work on upstream in branch ex-tcd:

git checkout ex-tcd
git rebase upstream

If github won't let you push your new branch you can force this with option -f:

git push origin ex-tcd -f
+ // Remember this local directory for the tab page.
+ if (os_dirname(NameBuff, MAXPATHL) == OK) {
+ curtab->localdir = vim_strsave(NameBuff);
+ }

This comment has been minimized.

@splinterofchaos

splinterofchaos Aug 26, 2015

Member

Why not just

curtab->localdir = xmalloc(MAXPATHL);
if (!os_dirname(curtab->localdir, MAXPATHL)) {
  xfree(curtab->localdir);
  curtab->localdir = NULL;
}

Not simpler, but I don't see why we need to clobber NameBuff and do this copying. If we do need to, maybe we should note that in the function's documentation.

@splinterofchaos

splinterofchaos Aug 26, 2015

Member

Why not just

curtab->localdir = xmalloc(MAXPATHL);
if (!os_dirname(curtab->localdir, MAXPATHL)) {
  xfree(curtab->localdir);
  curtab->localdir = NULL;
}

Not simpler, but I don't see why we need to clobber NameBuff and do this copying. If we do need to, maybe we should note that in the function's documentation.

This comment has been minimized.

@HiPhish

HiPhish Aug 26, 2015

Contributor

I don't know, I took the existing code and modified it. I'd say we can still refactor it later if it really matters.

@HiPhish

HiPhish Aug 26, 2015

Contributor

I don't know, I took the existing code and modified it. I'd say we can still refactor it later if it really matters.

This comment has been minimized.

@splinterofchaos

splinterofchaos Aug 27, 2015

Member

ok 👍

@splinterofchaos

This comment has been minimized.

Show comment
Hide comment
@splinterofchaos

splinterofchaos Aug 26, 2015

Member

LGTM 👍 after you fix the clint errors -- I have a nit, but doubt it has any functional impact.

lint:

src/nvim/ex_docmd.c:6887:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
src/nvim/ex_docmd.c:6888:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/nvim/window.c:3543:  Use true instead of TRUE.  [readability/bool] [4]
src/nvim/window.c:3544:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

I don't see anything nvim-specific about this, so let's send it upstream when you're ready. I believe posting a link to this PR from vim_dev should be sufficient.

Member

splinterofchaos commented Aug 26, 2015

LGTM 👍 after you fix the clint errors -- I have a nit, but doubt it has any functional impact.

lint:

src/nvim/ex_docmd.c:6887:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]
src/nvim/ex_docmd.c:6888:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/nvim/window.c:3543:  Use true instead of TRUE.  [readability/bool] [4]
src/nvim/window.c:3544:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

I don't see anything nvim-specific about this, so let's send it upstream when you're ready. I believe posting a link to this PR from vim_dev should be sufficient.

+local directories = {
+ 'Xtest-functional-ex_cmds-cd_spec.1', -- Tab
+ 'Xtest-functional-ex_cmds-cd_spec.2', -- Window
+ 'Xtest-functional-ex_cmds-cd_spec.3', -- New global

This comment has been minimized.

@justinmk

justinmk Aug 26, 2015

Member

This is good, we should get in this habit (as @ZyX-I suggested). Or provide a helper function that auto-generates a name based on the test file.

@justinmk

justinmk Aug 26, 2015

Member

This is good, we should get in this habit (as @ZyX-I suggested). Or provide a helper function that auto-generates a name based on the test file.

+ end
+ end)
+
+ it('works', function()

This comment has been minimized.

@justinmk

justinmk Aug 26, 2015

Member

Would prefer if this test were broken into multiple small it() blocks instead of one giant block, unless that is painful for some reason.

@justinmk

justinmk Aug 26, 2015

Member

Would prefer if this test were broken into multiple small it() blocks instead of one giant block, unless that is painful for some reason.

This comment has been minimized.

@HiPhish

HiPhish Aug 26, 2015

Contributor

It would be nicer, but the steps build on top of each other. The first thing I test is whether creating a new tab preserves the working directory, so i have to create new tab. The next step checks that :tcd works, so I have to create a new tab as well. Then the next test requires two tabs and two windows, so I would have to repeat the above steps again.

And so on, turning it into a pyramid of instructions. I guess I could make a function like this:

function doStuff(step) 
curtab->localdir
if step < 2 then return end
curtab->localdir
if step < 3 then return end
-- and so on
end

and call it with different step. Would that be worth it?

@HiPhish

HiPhish Aug 26, 2015

Contributor

It would be nicer, but the steps build on top of each other. The first thing I test is whether creating a new tab preserves the working directory, so i have to create new tab. The next step checks that :tcd works, so I have to create a new tab as well. Then the next test requires two tabs and two windows, so I would have to repeat the above steps again.

And so on, turning it into a pyramid of instructions. I guess I could make a function like this:

function doStuff(step) 
curtab->localdir
if step < 2 then return end
curtab->localdir
if step < 3 then return end
-- and so on
end

and call it with different step. Would that be worth it?

@justinmk justinmk added this to the 0.2 milestone Aug 26, 2015

+ break;
+ }
+
+ post_chdir(scope);

This comment has been minimized.

@oni-link

oni-link Aug 26, 2015

Contributor

ex_cd() function description should be updated.

@oni-link

oni-link Aug 26, 2015

Contributor

ex_cd() function description should be updated.

src/nvim/window.c
+ if (os_chdir((char *)curtab->localdir) == 0) {
+ shorten_fnames(true);
+ }
+

This comment has been minimized.

@oni-link

oni-link Aug 26, 2015

Contributor

We could reuse the code block for the window local directory:

  // Check for window or tab local directory.
  char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir ? curtab->localdir : NULL;
  if (new_dir) {
    /* Save current directory as global
     * directory (unless that was done already) and change to the local
     * directory. */
    if (globaldir == NULL) {
      char_u cwd[MAXPATHL];

      if (os_dirname(cwd, MAXPATHL) == OK)
        globaldir = vim_strsave(cwd);
    }
    if (os_chdir((char *)new_dir) == 0)
      shorten_fnames(TRUE);
  }   else if (globaldir != NULL) {
      ...
  }
@oni-link

oni-link Aug 26, 2015

Contributor

We could reuse the code block for the window local directory:

  // Check for window or tab local directory.
  char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir ? curtab->localdir : NULL;
  if (new_dir) {
    /* Save current directory as global
     * directory (unless that was done already) and change to the local
     * directory. */
    if (globaldir == NULL) {
      char_u cwd[MAXPATHL];

      if (os_dirname(cwd, MAXPATHL) == OK)
        globaldir = vim_strsave(cwd);
    }
    if (os_chdir((char *)new_dir) == 0)
      shorten_fnames(TRUE);
  }   else if (globaldir != NULL) {
      ...
  }

This comment has been minimized.

@HiPhish

HiPhish Aug 26, 2015

Contributor

Right, that was an eyesore. Also

char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir ? curtab->localdir : NULL;

can be shortened to

char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir;

Whether curtab->localdir is NULL or not, it's what we want in both cases.

@HiPhish

HiPhish Aug 26, 2015

Contributor

Right, that was an eyesore. Also

char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir ? curtab->localdir : NULL;

can be shortened to

char_u *new_dir = curwin->w_localdir
                        ? curwin->w_localdir
                        : curtab->localdir;

Whether curtab->localdir is NULL or not, it's what we want in both cases.

+ case kCdScopeTab:
+ // Remember this local directory for the tab page.
+ if (os_dirname(NameBuff, MAXPATHL) == OK) {
+ curtab->localdir = vim_strsave(NameBuff);

This comment has been minimized.

@oni-link

oni-link Aug 26, 2015

Contributor

Memory for localdir should also be freed in window.c:free_tabpage()

@oni-link

oni-link Aug 26, 2015

Contributor

Memory for localdir should also be freed in window.c:free_tabpage()

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Aug 26, 2015

Contributor

OK, I fixed the lint errors and I hope this time the rebasing went right.

Contributor

HiPhish commented Aug 26, 2015

OK, I fixed the lint errors and I hope this time the rebasing went right.

src/nvim/window.c
globaldir = vim_strsave(cwd);
+ }
+ }
+ if (os_chdir((char *)curtab->localdir) == 0) {

This comment has been minimized.

@oni-link

oni-link Aug 26, 2015

Contributor

curtab->localdir -> new_dir

@oni-link

oni-link Aug 26, 2015

Contributor

curtab->localdir -> new_dir

This comment has been minimized.

@HiPhish

HiPhish Aug 27, 2015

Contributor

Thanks, fixed.

@HiPhish

HiPhish Aug 27, 2015

Contributor

Thanks, fixed.

src/nvim/window.c
- * directory. */
+ // The new directory is either the local directory of the window, of the tab
+ // or NULL.
+ char_u new_dir = curwin->w_localdir ? curwin->w_localdir : curtab->localdir;

This comment has been minimized.

@oni-link

oni-link Aug 27, 2015

Contributor

new_dir needs to be a char_u pointer.

@oni-link

oni-link Aug 27, 2015

Contributor

new_dir needs to be a char_u pointer.

This comment has been minimized.

@HiPhish

HiPhish Aug 27, 2015

Contributor

facepalm

@HiPhish

HiPhish Aug 27, 2015

Contributor

facepalm

src/nvim/ex_docmd.c
- * directory as global directory. */
- if (globaldir == NULL && prev_dir != NULL)
+
+ // Overwrite the local directory of current the tab page for `cd` and `tcd`

This comment has been minimized.

@oni-link

oni-link Aug 27, 2015

Contributor

current the -> the current

@oni-link

oni-link Aug 27, 2015

Contributor

current the -> the current

runtime/doc/editing.txt
+local working directory of the current window takes precedence over the local
+working directory of the tab, which in turn takes precedence over the global
+working directory. If a local working directory, tab or window, does not
+exist, the next one in the hierarchy applies.
Changing directory fails when the current buffer is modified, the '.' flag is
present in 'cpoptions' and "!" is not used in the command.

This comment has been minimized.

@oni-link

oni-link Aug 29, 2015

Contributor

Flag ! was removed from 'cpoptions', so changing a directory cannot fail anymore (as described here). Sentence can be removed.

Do we still have commands cd!, lcd! and tcd! for compatibility reasons?

@oni-link

oni-link Aug 29, 2015

Contributor

Flag ! was removed from 'cpoptions', so changing a directory cannot fail anymore (as described here). Sentence can be removed.

Do we still have commands cd!, lcd! and tcd! for compatibility reasons?

This comment has been minimized.

@HiPhish

HiPhish Aug 30, 2015

Contributor

Yes, we do. Should we just silently ignore the exclamation mark in the documentation, pretending it doesn't exist, or write a note that it doesn't do anything? I'd go for the latter.

@HiPhish

HiPhish Aug 30, 2015

Contributor

Yes, we do. Should we just silently ignore the exclamation mark in the documentation, pretending it doesn't exist, or write a note that it doesn't do anything? I'd go for the latter.

runtime/doc/editing.txt
+
+ *:tch* *:tchdir*
+:tch[dir][!] Same as |:tcd|.
+

This comment has been minimized.

@oni-link

oni-link Aug 29, 2015

Contributor

Do we need to mention :tcd - (or :lcd -)?

@oni-link

oni-link Aug 29, 2015

Contributor

Do we need to mention :tcd - (or :lcd -)?

This comment has been minimized.

@HiPhish

HiPhish Aug 30, 2015

Contributor

:lcd - existed but wasn't mentioned. I think if it exists we should mention it.

@HiPhish

HiPhish Aug 30, 2015

Contributor

:lcd - existed but wasn't mentioned. I think if it exists we should mention it.

src/nvim/ex_docmd.c
@@ -6858,10 +6882,25 @@ void ex_cd(exarg_T *eap)
new_dir = NameBuff;
}
#endif
- if (new_dir == NULL || vim_chdir(new_dir))
+ if (new_dir == NULL || vim_chdir(new_dir)) {

This comment has been minimized.

@oni-link

oni-link Aug 29, 2015

Contributor

Where could new_dir be set to NULL?

@oni-link

oni-link Aug 29, 2015

Contributor

Where could new_dir be set to NULL?

This comment has been minimized.

@HiPhish

HiPhish Aug 30, 2015

Contributor

Good question. I was just taking the original code as it was. The only way for new_dir to be NULL is if eap->arg is NULL, which if I understand should never happen. Removed the check.

@HiPhish

HiPhish Aug 30, 2015

Contributor

Good question. I was just taking the original code as it was. The only way for new_dir to be NULL is if eap->arg is NULL, which if I understand should never happen. Removed the check.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 29, 2015

Member

@HiPhish Bram requires a real name for non-trivial contributions: https://groups.google.com/d/msg/vim_dev/u9RzSeSmvgE/au1TvIJcAwAJ

So, I need to think about whether we should too.

Member

justinmk commented Aug 29, 2015

@HiPhish Bram requires a real name for non-trivial contributions: https://groups.google.com/d/msg/vim_dev/u9RzSeSmvgE/au1TvIJcAwAJ

So, I need to think about whether we should too.

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Aug 30, 2015

Contributor

@justinmk Bram seems to live in the past where only nerds used the internet and one person managing all development was an efficient way of coordinating a project. Times have changed, now everyone is online, including people who will screw you over just to their own ego.

These days privacy and anonymity is more important than ever with companies making a fortune mining your data and selling it. We live in a time when someone overhearing you talking about dongles and forking repos can cause a shitstorm on Twitter and get you fired (look up "donglegate", I'm not making this up). Sometimes I just want to be a faceless voice with a made-up name.

I am no lawyer, so I don't know if Neovim really needs my full name for legal reasons, but if it doesn't then please don't make me give it to you. I don't really care about the copyright, I just want to see this feature realised. That to me is the point of collaborative open-source projects, if you want something you get your hands dirty and do it, and then everyone profits from it.

Contributor

HiPhish commented Aug 30, 2015

@justinmk Bram seems to live in the past where only nerds used the internet and one person managing all development was an efficient way of coordinating a project. Times have changed, now everyone is online, including people who will screw you over just to their own ego.

These days privacy and anonymity is more important than ever with companies making a fortune mining your data and selling it. We live in a time when someone overhearing you talking about dongles and forking repos can cause a shitstorm on Twitter and get you fired (look up "donglegate", I'm not making this up). Sometimes I just want to be a faceless voice with a made-up name.

I am no lawyer, so I don't know if Neovim really needs my full name for legal reasons, but if it doesn't then please don't make me give it to you. I don't really care about the copyright, I just want to see this feature realised. That to me is the point of collaborative open-source projects, if you want something you get your hands dirty and do it, and then everyone profits from it.

@raichoo

This comment has been minimized.

Show comment
Hide comment
@raichoo

raichoo Aug 30, 2015

Contributor

@HiPhish +1 on anonymity.

Contributor

raichoo commented Aug 30, 2015

@HiPhish +1 on anonymity.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 31, 2015

Member

@HiPhish So that means you definitely are not going to submit this to vim_dev?

A web search suggests that copyright is valid under a pseudonym.

http://www.legalgenealogist.com/blog/2013/01/02/copyright-and-the-pen-name/

http://programmers.stackexchange.com/a/154782/45819

Plus we did not require it up to now. So don't worry about it.

Member

justinmk commented Aug 31, 2015

@HiPhish So that means you definitely are not going to submit this to vim_dev?

A web search suggests that copyright is valid under a pseudonym.

http://www.legalgenealogist.com/blog/2013/01/02/copyright-and-the-pen-name/

http://programmers.stackexchange.com/a/154782/45819

Plus we did not require it up to now. So don't worry about it.

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Aug 31, 2015

Contributor

@justinmk The contributor license agreement wants my full name:
https://docs.google.com/forms/d/1u54bpbwzneDIRltFx1TGi2evKxY3w0cOV3vlpj8DPbg/viewform

So is it OK if I sign it with a pen name? I'm being so pedantic with it because I don't want to screw up the entire project down the road because of some legal technicality.

Contributor

HiPhish commented Aug 31, 2015

@justinmk The contributor license agreement wants my full name:
https://docs.google.com/forms/d/1u54bpbwzneDIRltFx1TGi2evKxY3w0cOV3vlpj8DPbg/viewform

So is it OK if I sign it with a pen name? I'm being so pedantic with it because I don't want to screw up the entire project down the road because of some legal technicality.

@splinterofchaos

This comment has been minimized.

Show comment
Hide comment
@splinterofchaos

splinterofchaos Aug 31, 2015

Member

Bram seems to live in the past where only nerds used the internet and one person managing all development was an efficient way of coordinating a project.

Open source communities are based on the trust model. In a way, this neovim repository is no different from Bram in terms of being a bottleneck for development, but it's not because this is the only way to coordinate a repository. Hundreds of vim forks exist, so why use of them rather than Bram's repo or vim.org's distribution? Because people trust Bram, vim.org, and they also trust neovim. vim, the linux kernel, GNU-managed repositories, all require real names for contributors because they don't trust people who use pseudonyms, aside from making accountability more difficult and allowing people to contributing under multiple names.

I don't have a real problem with anyone contributing under a pseudonym, I just wanted to point out that using real names in code is not an old fashioned idea. And that companies still use people's pseudonyms to collect and sell user data--real names aren't that important, or helpful, to modern internet-based companies.

Anyway, if Samuel Clemens could publish and copywrite under the name "Mark Twain", I see no reason you couldn't sign that form with whatever name you please. Unfortunately, I don't think we have any lawyers watching these PRs to give us advice.

Member

splinterofchaos commented Aug 31, 2015

Bram seems to live in the past where only nerds used the internet and one person managing all development was an efficient way of coordinating a project.

Open source communities are based on the trust model. In a way, this neovim repository is no different from Bram in terms of being a bottleneck for development, but it's not because this is the only way to coordinate a repository. Hundreds of vim forks exist, so why use of them rather than Bram's repo or vim.org's distribution? Because people trust Bram, vim.org, and they also trust neovim. vim, the linux kernel, GNU-managed repositories, all require real names for contributors because they don't trust people who use pseudonyms, aside from making accountability more difficult and allowing people to contributing under multiple names.

I don't have a real problem with anyone contributing under a pseudonym, I just wanted to point out that using real names in code is not an old fashioned idea. And that companies still use people's pseudonyms to collect and sell user data--real names aren't that important, or helpful, to modern internet-based companies.

Anyway, if Samuel Clemens could publish and copywrite under the name "Mark Twain", I see no reason you couldn't sign that form with whatever name you please. Unfortunately, I don't think we have any lawyers watching these PRs to give us advice.

runtime/doc/editing.txt
+working directory. If a local working directory, tab or window, does not
+exist, the next one in the hierarchy applies.
+
+All comands for changing the working directory can be suffixed with an

This comment has been minimized.

@justinmk

justinmk Sep 2, 2015

Member

typo

*:lc* *:lcd*
:lc[d][!] {path} Like |:cd|, but only set the current directory for the
current window. The current directory for other
- windows is not changed.
+ windows or any tabs is not changed.

This comment has been minimized.

@justinmk

justinmk Sep 2, 2015

Member

Instead of a new command (:tcd), what if we enabled :lcd to take a range, and special-cased :0lcd to set the tab-local working directory? Anyone like this idea, or is it too cryptic?

@justinmk

justinmk Sep 2, 2015

Member

Instead of a new command (:tcd), what if we enabled :lcd to take a range, and special-cased :0lcd to set the tab-local working directory? Anyone like this idea, or is it too cryptic?

This comment has been minimized.

@fdinoff

fdinoff Sep 3, 2015

Too cryptic. What would the other elements in the range represent?

@fdinoff

fdinoff Sep 3, 2015

Too cryptic. What would the other elements in the range represent?

This comment has been minimized.

@HiPhish

HiPhish Sep 3, 2015

Contributor

I don't think using numbers makes sense. A letter prefix is more intuitive and that's exactly what I did: t for tab, l for window and nothing for global. I think wcd would be a better choice than lcd, but that's the way it is (we could make an alias, but I don't think we need yet another command).

Or did you mean like a bitmask? 0 for global, 1 for window, 2 for tab and 3 for window and tab? That would be superfluous, a :tcd already overwrites the window-local working directory and a :cd overwrites both.

@HiPhish

HiPhish Sep 3, 2015

Contributor

I don't think using numbers makes sense. A letter prefix is more intuitive and that's exactly what I did: t for tab, l for window and nothing for global. I think wcd would be a better choice than lcd, but that's the way it is (we could make an alias, but I don't think we need yet another command).

Or did you mean like a bitmask? 0 for global, 1 for window, 2 for tab and 3 for window and tab? That would be superfluous, a :tcd already overwrites the window-local working directory and a :cd overwrites both.

This comment has been minimized.

@justinmk

justinmk Sep 3, 2015

Member

The difference between a "letter prefix" and a range is that a range avoids the extra command.

Anyways, it does not seem popular so forget it. But at least avoid the hastabdir() function.

@justinmk

justinmk Sep 3, 2015

Member

The difference between a "letter prefix" and a range is that a range avoids the extra command.

Anyways, it does not seem popular so forget it. But at least avoid the hastabdir() function.

This comment has been minimized.

@kopischke

kopischke Sep 3, 2015

@justinmk if there is a new scope for working directories, we need a way to test for it in Vim script. To me, hastabdir() has a nice symmetry to :tcd, but other options might be available – what would you suggest?

@kopischke

kopischke Sep 3, 2015

@justinmk if there is a new scope for working directories, we need a way to test for it in Vim script. To me, hastabdir() has a nice symmetry to :tcd, but other options might be available – what would you suggest?

This comment has been minimized.

@HiPhish

HiPhish Sep 3, 2015

Contributor

I had suggested in another comment to return a number from haslocaldir(): Vim already returns 0 for no local directory and 1 for window-local directory. My suggestions was to tread this like a bitfield: 2 for tab-local directory and 3 for window- and tab-local directory:

0b00 "nothing
0b01 "window
0b10 "tab
0b11 "both

It's backwards-compatible with the current implementation. The only problem I see is if plugin authors are testing the result of hastabdir() like a boolean instead of the number 1. In that case the plugins would tread the number 2 as having a window-local directory even if it hasn't. And testing for 1 explicitly would fail for 3 even though there is a window-local directory. We are working with a running system so we must take this into consideration.

The correct way to test for a window-local directory would be to bitwise AND the result of hastabdir() with 1 and test whether that is 1 or 0. I don't know if VimScript even has bitwise operations.

@HiPhish

HiPhish Sep 3, 2015

Contributor

I had suggested in another comment to return a number from haslocaldir(): Vim already returns 0 for no local directory and 1 for window-local directory. My suggestions was to tread this like a bitfield: 2 for tab-local directory and 3 for window- and tab-local directory:

0b00 "nothing
0b01 "window
0b10 "tab
0b11 "both

It's backwards-compatible with the current implementation. The only problem I see is if plugin authors are testing the result of hastabdir() like a boolean instead of the number 1. In that case the plugins would tread the number 2 as having a window-local directory even if it hasn't. And testing for 1 explicitly would fail for 3 even though there is a window-local directory. We are working with a running system so we must take this into consideration.

The correct way to test for a window-local directory would be to bitwise AND the result of hastabdir() with 1 and test whether that is 1 or 0. I don't know if VimScript even has bitwise operations.

This comment has been minimized.

@kopischke

kopischke Sep 3, 2015

@HiPhish Vim script has bitwise operators, implemented as functions called … and(), or() and xor(). I kid you not. Anyway, as you pointed out, this might introduce some odd bugs in non-Neovim aware scripts testing the return value of haslocaldir(), as leveraging implicit Boolean coercion is a pervasive bad practice in Vim script.

@kopischke

kopischke Sep 3, 2015

@HiPhish Vim script has bitwise operators, implemented as functions called … and(), or() and xor(). I kid you not. Anyway, as you pointed out, this might introduce some odd bugs in non-Neovim aware scripts testing the return value of haslocaldir(), as leveraging implicit Boolean coercion is a pervasive bad practice in Vim script.

This comment has been minimized.

@justinmk

justinmk Sep 3, 2015

Member

I don't see what bitmasks have to do with it. Plugins could check haslocaldir() == 1 || haslocaldir == 3. Anyways, the back-compat card was played, so we get another special-case function.

@justinmk

justinmk Sep 3, 2015

Member

I don't see what bitmasks have to do with it. Plugins could check haslocaldir() == 1 || haslocaldir == 3. Anyways, the back-compat card was played, so we get another special-case function.

This comment has been minimized.

@HiPhish

HiPhish Sep 3, 2015

Contributor

@kopischke OK, I'm not surprised, I just didn't know. I never used VimScript outside of the usual .vimrc settings, and it doesn't help that the language has a huge standard library and more edge cases than a stairway. Is there book to recommend? I read The VimL Primer by Benjamin Klein, but it's really just an introduction, something that should be the first two chapters of a much larger book.

@justinmk And what if yet another scope gets added in the future? Off the top of my head I could imagine a UI managing several instances of NeoVim and we would need an :icd command. Then the unified haslocaldir() would need to return a three-bit number:

0b000 "global
0b001 "window
0b010 "tab
0b011 "window & tab
0b100 "instance
0b101 "instance & window
0b110 "instance & tab
0b111 "instance & tab & window

So plugin authors would have to update their plugins again. Whereas if they AND the result with 1 it will work for all eternity.

@HiPhish

HiPhish Sep 3, 2015

Contributor

@kopischke OK, I'm not surprised, I just didn't know. I never used VimScript outside of the usual .vimrc settings, and it doesn't help that the language has a huge standard library and more edge cases than a stairway. Is there book to recommend? I read The VimL Primer by Benjamin Klein, but it's really just an introduction, something that should be the first two chapters of a much larger book.

@justinmk And what if yet another scope gets added in the future? Off the top of my head I could imagine a UI managing several instances of NeoVim and we would need an :icd command. Then the unified haslocaldir() would need to return a three-bit number:

0b000 "global
0b001 "window
0b010 "tab
0b011 "window & tab
0b100 "instance
0b101 "instance & window
0b110 "instance & tab
0b111 "instance & tab & window

So plugin authors would have to update their plugins again. Whereas if they AND the result with 1 it will work for all eternity.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Sep 4, 2015

Member

And what if yet another scope gets added in the future? Off the top of my head I could imagine a UI managing several instances of NeoVim

IMO that is more reason not to add :tcd and hastabdir() in the first place. Special-case functions are really unpleasant for user discovery and developing a sense for the patterns in a system.

:cd started life as the only option, and later :lcd was added. Tab-local and UI-local are (we agree) under the "local" umbrella.

Then the unified haslocaldir() would need to return a three-bit number:

Alternative approach:

  • modify haslocaldir() to take an optional string argument: "w" or "t". This allows future expansion (the string arg could be a UI identifier, for example). It continues to return only 0 or 1.
  • modify :lcd to take an optional argument:
    • :lcd tab /path/to/dir invokes "tab-local" feature
    • :lcd win /path/to/dir behaves the same as :lcd
      • This is backwards compatible because :lcd already requires paths with whitespace to be escaped.

This is backwards compatible, allows future expansion, and is more intuitive for users.

Member

justinmk commented Sep 4, 2015

And what if yet another scope gets added in the future? Off the top of my head I could imagine a UI managing several instances of NeoVim

IMO that is more reason not to add :tcd and hastabdir() in the first place. Special-case functions are really unpleasant for user discovery and developing a sense for the patterns in a system.

:cd started life as the only option, and later :lcd was added. Tab-local and UI-local are (we agree) under the "local" umbrella.

Then the unified haslocaldir() would need to return a three-bit number:

Alternative approach:

  • modify haslocaldir() to take an optional string argument: "w" or "t". This allows future expansion (the string arg could be a UI identifier, for example). It continues to return only 0 or 1.
  • modify :lcd to take an optional argument:
    • :lcd tab /path/to/dir invokes "tab-local" feature
    • :lcd win /path/to/dir behaves the same as :lcd
      • This is backwards compatible because :lcd already requires paths with whitespace to be escaped.

This is backwards compatible, allows future expansion, and is more intuitive for users.

@kopischke

This comment has been minimized.

Show comment
Hide comment
@kopischke

kopischke Sep 4, 2015

@justinmk I agree the Vim function and command API is messy, and I share your annoyance with it as a matter of principle. However, scoping via special case commands (we have tabdo and windo, not do tab and do win) and special case functions (we have gettabvar() and getwinvar(), not a parametrised getvar() function) is the status quo of the API, and deviating from that for new features will just add more inconsistency instead of reducing it.

@justinmk I agree the Vim function and command API is messy, and I share your annoyance with it as a matter of principle. However, scoping via special case commands (we have tabdo and windo, not do tab and do win) and special case functions (we have gettabvar() and getwinvar(), not a parametrised getvar() function) is the status quo of the API, and deviating from that for new features will just add more inconsistency instead of reducing it.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Sep 4, 2015

Member

deviating from that for new features will just add more inconsistency instead of reducing it.

But adding an optional parameter is not a deviation, and is a common practice in VimL. Tab-local is local, so we should emphasize that and support user intuition where we can.

And the idea of :icd is just horrible, in my opinion.

Member

justinmk commented Sep 4, 2015

deviating from that for new features will just add more inconsistency instead of reducing it.

But adding an optional parameter is not a deviation, and is a common practice in VimL. Tab-local is local, so we should emphasize that and support user intuition where we can.

And the idea of :icd is just horrible, in my opinion.

@kopischke

This comment has been minimized.

Show comment
Hide comment
@kopischke

kopischke Sep 4, 2015

But adding an optional parameter is not a deviation, and is a common practice in VimL

True, but not for scoping (see my above comment).

But adding an optional parameter is not a deviation, and is a common practice in VimL

True, but not for scoping (see my above comment).

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Sep 4, 2015

Contributor

@justinmk @kopischke I don't have enough experience with VimScript, I can't decide this topic. Should we have a poll or something? A discussion between only two people won't get any results done.

Contributor

HiPhish commented Sep 4, 2015

@justinmk @kopischke I don't have enough experience with VimScript, I can't decide this topic. Should we have a poll or something? A discussion between only two people won't get any results done.

@kopischke

This comment has been minimized.

Show comment
Hide comment
@kopischke

kopischke Sep 5, 2015

A discussion between only two people won't get any results done.

True that.

A discussion between only two people won't get any results done.

True that.

@Shougo

This comment has been minimized.

Show comment
Hide comment
@Shougo

Shougo Sep 7, 2015

Contributor

@justinmk @kopischke I don't have enough experience with VimScript, I can't decide this topic. Should we have a poll or something? A discussion between only two people won't get any results done.

VimL API is messy and horrible.
VimL has a lot of built-in commands. It is not easy to use it in plugins.
So, I think the function is better if you rewrite all VimL API.

I like @justinmk 's approach.
It is a bit messy. But it is extendable and backward compatible.

Contributor

Shougo commented Sep 7, 2015

@justinmk @kopischke I don't have enough experience with VimScript, I can't decide this topic. Should we have a poll or something? A discussion between only two people won't get any results done.

VimL API is messy and horrible.
VimL has a lot of built-in commands. It is not easy to use it in plugins.
So, I think the function is better if you rewrite all VimL API.

I like @justinmk 's approach.
It is a bit messy. But it is extendable and backward compatible.

src/nvim/eval.c
+ break;
+ }
+ // Fall through intentionally
+ case kCdScopeGlobal:

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

the convention in all existing code is:

case kCdScopeGlobal:  // FALLTHROUGH

Actually I thought this was enforced by the linter...

@justinmk

justinmk Feb 19, 2016

Member

the convention in all existing code is:

case kCdScopeGlobal:  // FALLTHROUGH

Actually I thought this was enforced by the linter...

src/nvim/eval.c
+ [kCdScopeWindow] = 0, // Number of window to look at.
+ [kCdScopeTab ] = 0, // Number of tab to look at.
+ [kCdScopeGlobal] = 0, // Number of tab to look at.
+ };

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

The only cases where NUMBER_OF_CD_SCOPES is used is for initialization. But in both cases, the array is initialized in the declaration, so NUMBER_OF_CD_SCOPES is redundant.

NUMBER_OF_CD_SCOPES could be eliminated. Since you don't have default: cases in the switch statements, the compiler will warn us if we add a new CdScope enum item.

@justinmk

justinmk Feb 19, 2016

Member

The only cases where NUMBER_OF_CD_SCOPES is used is for initialization. But in both cases, the array is initialized in the declaration, so NUMBER_OF_CD_SCOPES is redundant.

NUMBER_OF_CD_SCOPES could be eliminated. Since you don't have default: cases in the switch statements, the compiler will warn us if we add a new CdScope enum item.

src/nvim/ex_docmd.c
{
+ assert(scope <= MAX_CD_SCOPE);

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

compiler will warn about this implicitly because the switch statements don't have a default case.

@justinmk

justinmk Feb 19, 2016

Member

compiler will warn about this implicitly because the switch statements don't have a default case.

src/nvim/ex_docmd.c
-void post_chdir(int local)
+/// Deal with the side effects of changing the current directory.
+///
+/// @param scope Scope of the function call (editor, tab or window).

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

"editor" => "global"

@justinmk

justinmk Feb 19, 2016

Member

"editor" => "global"

src/nvim/ex_docmd.h
+/// The lower a number, the deeper the scope.
+typedef enum {
+ kCdScopeWindow, ///< Affects one window.
+ kCdScopeTab, ///< Affects on tab page.

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

typo

src/nvim/eval.c
+ if (scope_number[kCdScopeWindow] > 0) {
+ win = find_win_by_nr(&argvars[0], curtab);
+ if (!win) {
+ EMSG(_("E740: Cannot find window number."));

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

why is E740 used for these various errors?

@justinmk

justinmk Feb 19, 2016

Member

why is E740 used for these various errors?

This comment has been minimized.

@HiPhish

HiPhish Feb 24, 2016

Contributor

I don't know, I was just looking for some error that that something to do with arguments. I don't know about Vim error codes and I haven't been able to find a concise list. If you have a better suggestion or a link let me know, changing the codes is trivial.

@HiPhish

HiPhish Feb 24, 2016

Contributor

I don't know, I was just looking for some error that that something to do with arguments. I don't know about Vim error codes and I haven't been able to find a concise list. If you have a better suggestion or a link let me know, changing the codes is trivial.

This comment has been minimized.

@justinmk

justinmk Feb 24, 2016

Member

@HiPhish it'll take some research which I can take care of, just need your input on my other comments here

@justinmk

justinmk Feb 24, 2016

Member

@HiPhish it'll take some research which I can take care of, just need your input on my other comments here

This comment has been minimized.

@HiPhish

HiPhish Feb 24, 2016

Contributor

@justinmk I have implemented your other comments, pushed the updated commit last night.

@HiPhish

HiPhish Feb 24, 2016

Contributor

@justinmk I have implemented your other comments, pushed the updated commit last night.

This comment has been minimized.

@justinmk

justinmk Feb 24, 2016

Member

@HiPhish Thanks :)

src/nvim/eval.c
+ if (scope_number[kCdScopeWindow] == -1) {
+ win = NULL;
+ } else if (scope_number[kCdScopeWindow] >= 0) {
+ // If `tp` is `NULL` that's an error

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

don't need this comment since the EMSG call below serves the same purpose ;)

@justinmk

justinmk Feb 19, 2016

Member

don't need this comment since the EMSG call below serves the same purpose ;)

src/nvim/eval.c
+
+ // It the highest scope number is `-1` lower the scope by one.
+ if (scope_number[scope] < 0) {
+ ++scope;

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

the inverse relationship makes for very odd phrasing. "lower the scope by one" followed by ++scope ...

@justinmk

justinmk Feb 19, 2016

Member

the inverse relationship makes for very odd phrasing. "lower the scope by one" followed by ++scope ...

+ scope_number[i] = argvars[i].vval.v_number;
+ // The scope is the current iteration step.
+ scope = i;
+ // It is an error for the scope number to be less than `-1`.

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

redundant comment

@justinmk

justinmk Feb 19, 2016

Member

redundant comment

src/nvim/eval.c
#endif
- }
+
+fail:

This comment has been minimized.

@justinmk

justinmk Feb 19, 2016

Member

end would be a better name for this label, because it falls through to this in the non-failure case. Most of the existing code is consistent about this AFAICT.

@justinmk

justinmk Feb 19, 2016

Member

end would be a better name for this label, because it falls through to this in the non-failure case. Most of the existing code is consistent about this AFAICT.

src/nvim/eval.c
+ break;
+ }
+ // Fall through intentionally
+ case kCdScopeTab:

This comment has been minimized.

@justinmk

justinmk Feb 24, 2016

Member
/// FALLTHROUGH
@justinmk

justinmk Feb 24, 2016

Member
/// FALLTHROUGH
src/nvim/eval.c
+ }
+ }
+
+ // Act according to the scope. Add new scopes to the switch as well.

This comment has been minimized.

@justinmk

justinmk Feb 24, 2016

Member

the "Add new scopes to the switch as well." comment here and elsewhere is confusing since they're both switches. Might as well just leave that comment out, since the compiler will warn us if new scopes are added.

@justinmk

justinmk Feb 24, 2016

Member

the "Add new scopes to the switch as well." comment here and elsewhere is confusing since they're both switches. Might as well just leave that comment out, since the compiler will warn us if new scopes are added.

@justinmk justinmk referenced this pull request Mar 2, 2016

Closed

[RFC] vim-patch:7.4.1126 #4379

@jwhitley

This comment has been minimized.

Show comment
Hide comment
@jwhitley

jwhitley Mar 14, 2016

Contributor

oh wow, this feature will be fantastic. Huge thanks to @HiPhish for submitting this PR!

Contributor

jwhitley commented Mar 14, 2016

oh wow, this feature will be fantastic. Huge thanks to @HiPhish for submitting this PR!

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 14, 2016

Member

Only thing blocking this is #3229 (comment) , if someone wants to add a commit that chooses sensible error numbers.

Member

justinmk commented Mar 14, 2016

Only thing blocking this is #3229 (comment) , if someone wants to add a commit that chooses sensible error numbers.

@jwhitley

This comment has been minimized.

Show comment
Hide comment
@jwhitley

jwhitley Mar 14, 2016

Contributor

@justinmk To what extent, if any, are error codes reusable? I spot checked some possible "Invalid arguments" type error codes occuring in eval.c, but each of them occurs in the source in exactly one location.

Contributor

jwhitley commented Mar 14, 2016

@justinmk To what extent, if any, are error codes reusable? I spot checked some possible "Invalid arguments" type error codes occuring in eval.c, but each of them occurs in the source in exactly one location.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 14, 2016

Member

@jwhitley I don't know of any written guidelines. Unfortunately it's a matter of reviewing the existing error messages, trying to find a "group", and using judgement to assign new error numbers (if justified; re-using existing ones is better when appropriate) to new error messages.

Mostly the important thing is to avoid conflict with Vim error numbers that may be added in the future. In fact we may want to claim the 9xxx space for that, just to avoid having to think about it again.

Member

justinmk commented Mar 14, 2016

@jwhitley I don't know of any written guidelines. Unfortunately it's a matter of reviewing the existing error messages, trying to find a "group", and using judgement to assign new error numbers (if justified; re-using existing ones is better when appropriate) to new error messages.

Mostly the important thing is to avoid conflict with Vim error numbers that may be added in the future. In fact we may want to claim the 9xxx space for that, just to avoid having to think about it again.

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

@jwhitley I usually use the following:

  1. If Vim has some error condition that gives error message I use Vim code, but may also different error message with the same code. E.g. in #4131 I have loads of error messages with one code because Bram again has written absolutely useless error reporting.
  2. If Vim has some close, but not identical, error conditions I may reuse its code if that condition got removed in my PR: see #2506. Or current repository, comments near error codes in shada.c.
  3. If Vim does not have something similar I may use error code that was not already taken: see #3034. This is some 9xx error code, but usually not the first free one just in case.

I would really prefer this to transform into E{code}:{component (logical)}[/{subcomponent}]*/{error}: {error message} like E474:json/parser/duplicate-comma: Duplicate comma: ,,1], but this looks ugly unless a hack is added to have E474: Duplicate comma: ,,1] displayed to user and Vim(let):E474:json/parser/duplicate-comma: Duplicate comma: ,,1] caught as an exception which is now not so ugly to look at, but inconvenient for use by script writers. Also this needs to be pushed through Bram to be actually useful.


@justinmk Did you mean exactly 9xxx (three digits) or 9xx? Bram already started to use the second. I have run

ag --nofilename -o 'msgid "E\d+(?=:).*' | sort -k2 -tE -n | uniq

for Vim and Neovim and it looks like we do not have problematic name conflicts yet, but look at E902:

--- our-messages    2016-03-15 01:50:35.411958325 +0300
+++ their-messages  2016-03-15 01:50:19.008378366 +0300
@@ -18,3 +18,2 @@
 msgid "E25: GUI cannot be used: Not enabled at compile time"
-msgid "E25: Nvim does not have a built-in GUI"
 msgid "E26: Hebrew cannot be used: Not enabled at compile time\n"
@@ -85,5 +84,3 @@
 msgid "E86: Buffer %ld does not exist"
-msgid "E86: Buffer %<PRId64> does not exist"
 msgid "E86: Cannot go to buffer %ld"
-msgid "E86: Cannot go to buffer %<PRId64>"
 msgid "E87: Cannot go beyond last buffer"
@@ -95,3 +92,2 @@
 msgid "E92: Buffer %ld not found"
-msgid "E92: Buffer %<PRId64> not found"
 msgid "E93: More than one match for %s"
@@ -100,3 +96,2 @@
 msgid "E96: Can not diff more than %ld buffers"
-msgid "E96: Can not diff more than %<PRId64> buffers"
 msgid "E97: Cannot create diffs"
@@ -151,3 +146,2 @@
 msgid "E141: No file name for buffer %ld"
-msgid "E141: No file name for buffer %<PRId64>"
 msgid "E142: File not written: Writing is disabled by 'write' option"
@@ -168,3 +162,2 @@
 msgid "E157: Invalid sign ID: %ld"
-msgid "E157: Invalid sign ID: %<PRId64>"
 msgid "E158: Invalid buffer name: %s"
@@ -187,3 +180,2 @@
 msgid "E173: %ld more files to edit"
-msgid "E173: %<PRId64> more files to edit"
 msgid "E174: Command already exists: add ! to replace it"
@@ -294,3 +286,2 @@
 msgid "E262: error reading cscope connection %ld"
-msgid "E262: error reading cscope connection %<PRId64>"
 msgid "E263: Sorry, this command is disabled, the Python library could not be loaded."
@@ -352,5 +343,3 @@
 msgid "E315: ml_get: invalid lnum: %ld"
-msgid "E315: ml_get: invalid lnum: %<PRId64>"
 msgid "E316: ml_get: cannot find line %ld"
-msgid "E316: ml_get: cannot find line %<PRId64>"
 msgid "E317: pointer block id wrong"
@@ -361,10 +350,6 @@
 msgid "E319: Sorry, the command is not available in this version"
-msgid "E319: The command is not available in this version"
 msgid "E320: Cannot find line %ld"
-msgid "E320: Cannot find line %<PRId64>"
 msgid "E321: Could not reload \"%s\""
 msgid "E322: line number out of range: %ld past the end"
-msgid "E322: line number out of range: %<PRId64> past the end"
 msgid "E323: line count wrong in block %ld"
-msgid "E323: line count wrong in block %<PRId64>"
 msgid "E324: Can't open PostScript output file"
@@ -388,5 +373,3 @@
 msgid "E341: Internal error: lalloc(%ld, )"
-msgid "E341: Internal error: lalloc(%<PRId64>, )"
 msgid "E342: Out of memory!  (allocating %lu bytes)"
-msgid "E342: Out of memory!  (allocating %<PRIu64> bytes)"
 msgid "E343: Invalid path: '**[number]' must be at the end of the path or be followed by '%s'."
@@ -675,3 +658,2 @@
 msgid "E658: NetBeans connection lost for buffer %ld"
-msgid "E658: NetBeans connection lost for buffer %<PRId64>"
 msgid "E659: Cannot invoke Python recursively"
@@ -701,3 +683,2 @@
 msgid "E684: list index out of range: %ld"
-msgid "E684: list index out of range: %<PRId64>"
 msgid "E685: Internal error: %s"
@@ -710,3 +691,2 @@
 msgid "E692: Invalid operation for List"
-msgid "E692: Invalid operation for Lists"
 msgid "E693: Can only compare Funcref with Funcref"
@@ -789,3 +769,2 @@
 msgid "E765: 'spellfile' does not have %ld entries"
-msgid "E765: 'spellfile' does not have %<PRId64> entries"
 msgid "E766: Insufficient arguments for printf()"
@@ -848,3 +827,2 @@
 msgid "E830: Undo number %ld not found"
-msgid "E830: Undo number %<PRId64> not found"
 msgid "E831: bf_key_init() called with empty password"
@@ -876,6 +854,3 @@
 msgid "E859: Failed to convert returned python object to vim value"
-msgid "E860: Eval did not return a valid python 3 object"
-msgid "E861: Failed to convert returned python 3 object to vim value"
 msgid "E862: Cannot use g: here"
-msgid "E863: return value must be an instance of str"
 msgid "E865: (NFA) Regexp end encountered prematurely"
@@ -893,3 +868,2 @@
 msgid "E877: (NFA regexp) Invalid character class: %ld"
-msgid "E877: (NFA regexp) Invalid character class: %<PRId64>"
 msgid "E878: (NFA) Could not allocate memory for branch traversal!"
@@ -910,6 +884,3 @@
 msgid "E899: Cannot connect to port after retry2"
-msgid "E900: Invalid job id"
-msgid "E901: Job table is full"
 msgid "E902: Cannot connect to port"
-msgid "E902: \"%s\" is not an executable"
 msgid "E906: not an open channel"
@@ -924,3 +895,2 @@
 msgid "E999: Invalid callback argument"
-msgid "E999: (NFA regexp internal error) Should not process NOT node !"
 msgid "E999: not an open channel"
Contributor

ZyX-I commented Mar 14, 2016

@jwhitley I usually use the following:

  1. If Vim has some error condition that gives error message I use Vim code, but may also different error message with the same code. E.g. in #4131 I have loads of error messages with one code because Bram again has written absolutely useless error reporting.
  2. If Vim has some close, but not identical, error conditions I may reuse its code if that condition got removed in my PR: see #2506. Or current repository, comments near error codes in shada.c.
  3. If Vim does not have something similar I may use error code that was not already taken: see #3034. This is some 9xx error code, but usually not the first free one just in case.

I would really prefer this to transform into E{code}:{component (logical)}[/{subcomponent}]*/{error}: {error message} like E474:json/parser/duplicate-comma: Duplicate comma: ,,1], but this looks ugly unless a hack is added to have E474: Duplicate comma: ,,1] displayed to user and Vim(let):E474:json/parser/duplicate-comma: Duplicate comma: ,,1] caught as an exception which is now not so ugly to look at, but inconvenient for use by script writers. Also this needs to be pushed through Bram to be actually useful.


@justinmk Did you mean exactly 9xxx (three digits) or 9xx? Bram already started to use the second. I have run

ag --nofilename -o 'msgid "E\d+(?=:).*' | sort -k2 -tE -n | uniq

for Vim and Neovim and it looks like we do not have problematic name conflicts yet, but look at E902:

--- our-messages    2016-03-15 01:50:35.411958325 +0300
+++ their-messages  2016-03-15 01:50:19.008378366 +0300
@@ -18,3 +18,2 @@
 msgid "E25: GUI cannot be used: Not enabled at compile time"
-msgid "E25: Nvim does not have a built-in GUI"
 msgid "E26: Hebrew cannot be used: Not enabled at compile time\n"
@@ -85,5 +84,3 @@
 msgid "E86: Buffer %ld does not exist"
-msgid "E86: Buffer %<PRId64> does not exist"
 msgid "E86: Cannot go to buffer %ld"
-msgid "E86: Cannot go to buffer %<PRId64>"
 msgid "E87: Cannot go beyond last buffer"
@@ -95,3 +92,2 @@
 msgid "E92: Buffer %ld not found"
-msgid "E92: Buffer %<PRId64> not found"
 msgid "E93: More than one match for %s"
@@ -100,3 +96,2 @@
 msgid "E96: Can not diff more than %ld buffers"
-msgid "E96: Can not diff more than %<PRId64> buffers"
 msgid "E97: Cannot create diffs"
@@ -151,3 +146,2 @@
 msgid "E141: No file name for buffer %ld"
-msgid "E141: No file name for buffer %<PRId64>"
 msgid "E142: File not written: Writing is disabled by 'write' option"
@@ -168,3 +162,2 @@
 msgid "E157: Invalid sign ID: %ld"
-msgid "E157: Invalid sign ID: %<PRId64>"
 msgid "E158: Invalid buffer name: %s"
@@ -187,3 +180,2 @@
 msgid "E173: %ld more files to edit"
-msgid "E173: %<PRId64> more files to edit"
 msgid "E174: Command already exists: add ! to replace it"
@@ -294,3 +286,2 @@
 msgid "E262: error reading cscope connection %ld"
-msgid "E262: error reading cscope connection %<PRId64>"
 msgid "E263: Sorry, this command is disabled, the Python library could not be loaded."
@@ -352,5 +343,3 @@
 msgid "E315: ml_get: invalid lnum: %ld"
-msgid "E315: ml_get: invalid lnum: %<PRId64>"
 msgid "E316: ml_get: cannot find line %ld"
-msgid "E316: ml_get: cannot find line %<PRId64>"
 msgid "E317: pointer block id wrong"
@@ -361,10 +350,6 @@
 msgid "E319: Sorry, the command is not available in this version"
-msgid "E319: The command is not available in this version"
 msgid "E320: Cannot find line %ld"
-msgid "E320: Cannot find line %<PRId64>"
 msgid "E321: Could not reload \"%s\""
 msgid "E322: line number out of range: %ld past the end"
-msgid "E322: line number out of range: %<PRId64> past the end"
 msgid "E323: line count wrong in block %ld"
-msgid "E323: line count wrong in block %<PRId64>"
 msgid "E324: Can't open PostScript output file"
@@ -388,5 +373,3 @@
 msgid "E341: Internal error: lalloc(%ld, )"
-msgid "E341: Internal error: lalloc(%<PRId64>, )"
 msgid "E342: Out of memory!  (allocating %lu bytes)"
-msgid "E342: Out of memory!  (allocating %<PRIu64> bytes)"
 msgid "E343: Invalid path: '**[number]' must be at the end of the path or be followed by '%s'."
@@ -675,3 +658,2 @@
 msgid "E658: NetBeans connection lost for buffer %ld"
-msgid "E658: NetBeans connection lost for buffer %<PRId64>"
 msgid "E659: Cannot invoke Python recursively"
@@ -701,3 +683,2 @@
 msgid "E684: list index out of range: %ld"
-msgid "E684: list index out of range: %<PRId64>"
 msgid "E685: Internal error: %s"
@@ -710,3 +691,2 @@
 msgid "E692: Invalid operation for List"
-msgid "E692: Invalid operation for Lists"
 msgid "E693: Can only compare Funcref with Funcref"
@@ -789,3 +769,2 @@
 msgid "E765: 'spellfile' does not have %ld entries"
-msgid "E765: 'spellfile' does not have %<PRId64> entries"
 msgid "E766: Insufficient arguments for printf()"
@@ -848,3 +827,2 @@
 msgid "E830: Undo number %ld not found"
-msgid "E830: Undo number %<PRId64> not found"
 msgid "E831: bf_key_init() called with empty password"
@@ -876,6 +854,3 @@
 msgid "E859: Failed to convert returned python object to vim value"
-msgid "E860: Eval did not return a valid python 3 object"
-msgid "E861: Failed to convert returned python 3 object to vim value"
 msgid "E862: Cannot use g: here"
-msgid "E863: return value must be an instance of str"
 msgid "E865: (NFA) Regexp end encountered prematurely"
@@ -893,3 +868,2 @@
 msgid "E877: (NFA regexp) Invalid character class: %ld"
-msgid "E877: (NFA regexp) Invalid character class: %<PRId64>"
 msgid "E878: (NFA) Could not allocate memory for branch traversal!"
@@ -910,6 +884,3 @@
 msgid "E899: Cannot connect to port after retry2"
-msgid "E900: Invalid job id"
-msgid "E901: Job table is full"
 msgid "E902: Cannot connect to port"
-msgid "E902: \"%s\" is not an executable"
 msgid "E906: not an open channel"
@@ -924,3 +895,2 @@
 msgid "E999: Invalid callback argument"
-msgid "E999: (NFA regexp internal error) Should not process NOT node !"
 msgid "E999: not an open channel"
@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 14, 2016

Member

@justinmk Did you mean exactly 9xxx (three digits) or 9xx?

I meant 9xxx, with the assumption that we won't add more than 999 new error numbers. There are some 1000-level error numbers, so 4-digit numbers already exist.

it looks like we do not have problematic name conflicts yet, but look at E902:

Looks like we need to relocate E902. Is there any objection to claiming 9xxx or some other high 4-digit level?

Member

justinmk commented Mar 14, 2016

@justinmk Did you mean exactly 9xxx (three digits) or 9xx?

I meant 9xxx, with the assumption that we won't add more than 999 new error numbers. There are some 1000-level error numbers, so 4-digit numbers already exist.

it looks like we do not have problematic name conflicts yet, but look at E902:

Looks like we need to relocate E902. Is there any objection to claiming 9xxx or some other high 4-digit level?

@ZyX-I

This comment has been minimized.

Show comment
Hide comment
@ZyX-I

ZyX-I Mar 14, 2016

Contributor

@justinmk I can easily add more then 999 new error numbers. Just reassign every single error with different error message distinct error code and you will already have a huge number of new error messages. shada.c have 67 occurrences of emsg or EMSG, json-functions branch has 37 occurrences of emsgf, some other PRs would also be better if they were not using e_invarg[2], luaviml branch has loads of new error messages which would really be nice to see in master even without a separated parser. A clear policy is needed here.

Contributor

ZyX-I commented Mar 14, 2016

@justinmk I can easily add more then 999 new error numbers. Just reassign every single error with different error message distinct error code and you will already have a huge number of new error messages. shada.c have 67 occurrences of emsg or EMSG, json-functions branch has 37 occurrences of emsgf, some other PRs would also be better if they were not using e_invarg[2], luaviml branch has loads of new error messages which would really be nice to see in master even without a separated parser. A clear policy is needed here.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 14, 2016

Member

@ZyX-I Then let's claim 5xxx and above.

Member

justinmk commented Mar 14, 2016

@ZyX-I Then let's claim 5xxx and above.

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Mar 18, 2016

Contributor

So what's the consensus on error numbers now?

@jwhitley Thanks. I find myself using the feature all the time, for example when I work on my website which is built using Pelican I might occasionally find a mistake in one of my templates, so I open a new tab, :tcd to the template project and juggle back and forth between website- and template project, transplanting code between the two until I get things right. Or recently I was writing a C# domain for Sphinx and I needed my code, the Sphinx code, and the Docutils code at the same time, each of which was in its own tab and I was able to grep each one in its own directory.

Contributor

HiPhish commented Mar 18, 2016

So what's the consensus on error numbers now?

@jwhitley Thanks. I find myself using the feature all the time, for example when I work on my website which is built using Pelican I might occasionally find a mistake in one of my templates, so I open a new tab, :tcd to the template project and juggle back and forth between website- and template project, transplanting code between the two until I get things right. Or recently I was writing a C# domain for Sphinx and I needed my code, the Sphinx code, and the Docutils code at the same time, each of which was in its own tab and I was able to grep each one in its own directory.

@jwhitley

This comment has been minimized.

Show comment
Hide comment
@jwhitley

jwhitley Mar 25, 2016

Contributor

@justinmk Any advice on allocating 5xxx-space error codes for this PR? Just start numbering from 5001?

Contributor

jwhitley commented Mar 25, 2016

@justinmk Any advice on allocating 5xxx-space error codes for this PR? Just start numbering from 5001?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 25, 2016

Member

@jwhitley Yes. I think we should just always increment by 1 (starting with 5000, as discussed above) and not attempt to "group" similar types of errors: the holes between groups are a source of confusion and I cannot think of any case where "grouping" similar errors helps anyone.

Member

justinmk commented Mar 25, 2016

@jwhitley Yes. I think we should just always increment by 1 (starting with 5000, as discussed above) and not attempt to "group" similar types of errors: the holes between groups are a source of confusion and I cannot think of any case where "grouping" similar errors helps anyone.

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Mar 26, 2016

Contributor

@justinmk So should I just go ahead and start enumerating all the errors starting with 5000 or is there something else needed in addition?

Contributor

HiPhish commented Mar 26, 2016

@justinmk So should I just go ahead and start enumerating all the errors starting with 5000 or is there something else needed in addition?

@joshaw

This comment has been minimized.

Show comment
Hide comment
@joshaw

joshaw Mar 26, 2016

Is there anything that requires the error numbers be purely numeric with
the leading "E"? Would it make sense to have an nvim "namespace" to ensure
there were no conflicts with newer vim error codes? Something like EN1,
EN234, or En342.

On 26 March 2016 at 12:37, Alejandro Sanchez notifications@github.com
wrote:

@justinmk https://github.com/justinmk So should I just go ahead and
start enumerating all the errors starting with 5000 or is there something
else needed in addition?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3229 (comment)

joshaw commented Mar 26, 2016

Is there anything that requires the error numbers be purely numeric with
the leading "E"? Would it make sense to have an nvim "namespace" to ensure
there were no conflicts with newer vim error codes? Something like EN1,
EN234, or En342.

On 26 March 2016 at 12:37, Alejandro Sanchez notifications@github.com
wrote:

@justinmk https://github.com/justinmk So should I just go ahead and
start enumerating all the errors starting with 5000 or is there something
else needed in addition?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3229 (comment)

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Mar 26, 2016

Contributor

@joshaw This feature is relatively easy to port to Vim (I won't do it because I have no interest in it, but someone else could), so I don't think using Neovim-specific error codes would be appropriate.

Contributor

HiPhish commented Mar 26, 2016

@joshaw This feature is relatively easy to port to Vim (I won't do it because I have no interest in it, but someone else could), so I don't think using Neovim-specific error codes would be appropriate.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Mar 26, 2016

Member

So should I just go ahead and start enumerating all the errors starting with 5000

@HiPhish That should be enough.

Would it make sense to have an nvim "namespace" to ensure there were no conflicts with newer vim error codes? Something like EN1, EN234, or En342.

@joshaw That's not a bad idea, though I'm not sure what good prefix would be. @ZyX-I What's your opinion on that idea?

This feature is relatively easy to port to Vim (I won't do it because I have no interest in it, but someone else could), so I don't think using Neovim-specific error codes would be appropriate.

@HiPhish If this is ported to Vim and the Vim patch re-uses the 5xxx range then we didn't succeed in avoiding conflicts with Vim error numbers (Bram may increment some Vim-only error to 5xxx+1, for example).

Member

justinmk commented Mar 26, 2016

So should I just go ahead and start enumerating all the errors starting with 5000

@HiPhish That should be enough.

Would it make sense to have an nvim "namespace" to ensure there were no conflicts with newer vim error codes? Something like EN1, EN234, or En342.

@joshaw That's not a bad idea, though I'm not sure what good prefix would be. @ZyX-I What's your opinion on that idea?

This feature is relatively easy to port to Vim (I won't do it because I have no interest in it, but someone else could), so I don't think using Neovim-specific error codes would be appropriate.

@HiPhish If this is ported to Vim and the Vim patch re-uses the 5xxx range then we didn't succeed in avoiding conflicts with Vim error numbers (Bram may increment some Vim-only error to 5xxx+1, for example).

@joshaw

This comment has been minimized.

Show comment
Hide comment
@joshaw

joshaw Mar 26, 2016

You're right, I hadn't thought of new features that might end up in both. I
was more thinking of the general case of how errors are assigned that will
be specific to neovim. Just wanted to put the idea out there, for this I
agree, probably doesn't make sense.
On 26 Mar 2016 1:06 p.m., "Alejandro Sanchez" notifications@github.com
wrote:

@joshaw https://github.com/joshaw This feature is relatively easy to
port to Vim (I won't do it because I have no interest in it, but someone
else could), so I don't think using Neovim-specific error codes would be
appropriate.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3229 (comment)

joshaw commented Mar 26, 2016

You're right, I hadn't thought of new features that might end up in both. I
was more thinking of the general case of how errors are assigned that will
be specific to neovim. Just wanted to put the idea out there, for this I
agree, probably doesn't make sense.
On 26 Mar 2016 1:06 p.m., "Alejandro Sanchez" notifications@github.com
wrote:

@joshaw https://github.com/joshaw This feature is relatively easy to
port to Vim (I won't do it because I have no interest in it, but someone
else could), so I don't think using Neovim-specific error codes would be
appropriate.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3229 (comment)

@jwhitley

This comment has been minimized.

Show comment
Hide comment
@jwhitley

jwhitley Apr 19, 2016

Contributor

Ping? It looks like we're sitting right next to the goal on this one. AFAICT, the errors just need to be re-enumerated starting from 5000.

Contributor

jwhitley commented Apr 19, 2016

Ping? It looks like we're sitting right next to the goal on this one. AFAICT, the errors just need to be re-enumerated starting from 5000.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 19, 2016

Member

This was the next thing on my list after the JSON merge.

Member

justinmk commented Apr 19, 2016

This was the next thing on my list after the JSON merge.

HiPhish HiPhish
Implement tab-local working directory feature.
New ex commands: 'tcd', 'tchdir'
Changed Vimscript functions: 'haslocaldir', 'getcwd'

The ex-commands ':tcd' and ':tchdir' are the tab-local equivalents of
':lcd' and ':lchdir'. There are no new Vimscript functions introduced,
instead the functions 'haslocaldir' and 'getcwd' take in optional
arguments. See the documentation for details

Since there is now different levels of local directory a simple boolean
at source level is no longer sufficient; a new enumeration type is used
for the scope-level from now on.

The documentation has been accommodated for these new commands and
functional tests have been written to test the feature.
@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Apr 20, 2016

Contributor

I had updated the error numbers quite a while ago, so as far as I'm concerned I'm just waiting for the merge.

Contributor

HiPhish commented Apr 20, 2016

I had updated the error numbers quite a while ago, so as far as I'm concerned I'm just waiting for the merge.

@justinmk justinmk merged commit ec71d87 into neovim:master Apr 22, 2016

1 of 3 checks passed

default Build pr-3229 finished with status FAILED
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

justinmk added a commit that referenced this pull request Apr 22, 2016

@justinmk justinmk removed the RDY label Apr 22, 2016

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Apr 22, 2016

Member

Tweaked the docs and error messages. @HiPhish Thanks for following through with this!

Member

justinmk commented Apr 22, 2016

Tweaked the docs and error messages. @HiPhish Thanks for following through with this!

@HiPhish

This comment has been minimized.

Show comment
Hide comment
@HiPhish

HiPhish Apr 22, 2016

Contributor

@justinmk

Thanks for following through with this!

Sure thing, my motto is "if it doesn't exist I'll make it". I should thank you for guiding me through this task :)

Contributor

HiPhish commented Apr 22, 2016

@justinmk

Thanks for following through with this!

Sure thing, my motto is "if it doesn't exist I'll make it". I should thank you for guiding me through this task :)

@kopischke kopischke referenced this pull request in zhimsel/vim-stay Apr 24, 2016

Closed

Prevent changing directory when restoring view. #11

@HiPhish HiPhish deleted the HiPhish:ex-tcd branch Aug 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment