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

[RFC] default to 'nocompatible' #1622

Merged
merged 3 commits into from Dec 13, 2014

Conversation

Projects
None yet
7 participants
@klusark
Contributor

klusark commented Dec 7, 2014

I'm not really that happy with the changes I had to make to the first test in order to get it to work, so maybe someone else has some ideas to make it a bit better.

For the "this shouldn't be deleted" test, the actual output doesn't really make much sense (the line that says it should be deleted is not deleted, and the one that says it should not be modified is modified), so I made it expect the output that makes sense

@three-comrades

This comment has been minimized.

Show comment
Hide comment
@three-comrades

three-comrades Dec 7, 2014

Contributor

To stay as close as possible to the old test, we could just set the required options: three-comrades@84e1a06

Contributor

three-comrades commented Dec 7, 2014

To stay as close as possible to the old test, we could just set the required options: three-comrades@84e1a06

@klusark klusark changed the title from Make test29 work without compatible to [RDY] Make test29 work without compatible Dec 7, 2014

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 7, 2014

Contributor

@three-comrades, ya, I like that a lot better, so I replaced my commit with yours.

Contributor

klusark commented Dec 7, 2014

@three-comrades, ya, I like that a lot better, so I replaced my commit with yours.

@marvim marvim added the RDY label Dec 7, 2014

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 8, 2014

Member

Can you add b73ff33 to this?

Member

justinmk commented Dec 8, 2014

Can you add b73ff33 to this?

@justinmk justinmk changed the title from [RDY] Make test29 work without compatible to [RFC] Make test29 work without compatible Dec 8, 2014

@marvim marvim added RFC and removed RDY labels Dec 8, 2014

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor

@justinmk, done.

Contributor

klusark commented Dec 8, 2014

@justinmk, done.

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor

@Pyrohh, Good idea.

Contributor

klusark commented Dec 8, 2014

@Pyrohh, Good idea.

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor

Anyone have any idea what could be causing the memory leak that travis-ci is complaining about?

Contributor

klusark commented Dec 8, 2014

Anyone have any idea what could be causing the memory leak that travis-ci is complaining about?

@@ -949,7 +949,6 @@ static void parse_command_name(mparm_T *parmp)
exmode_active = EXMODE_VIM;
else
exmode_active = EXMODE_NORMAL;
change_compatible(TRUE); /* set 'compatible' */

This comment has been minimized.

@justinmk

justinmk Dec 8, 2014

Member

If we don't do this then I guess "ex" won't work as intended. We may want to document this.

@justinmk

justinmk Dec 8, 2014

Member

If we don't do this then I guess "ex" won't work as intended. We may want to document this.

* When 'compatible' is unset: Set all options that have a different default
* for Vim (without the P_VI_DEF flag) to that default.
*/
static void compatible_set(void)

This comment has been minimized.

@justinmk

justinmk Dec 8, 2014

Member

For now, I think we need to keep this, and call it somewhere in set_init_1().

@justinmk

justinmk Dec 8, 2014

Member

For now, I think we need to keep this, and call it somewhere in set_init_1().

Show outdated Hide outdated src/nvim/option.c Outdated
if (!(options[opt_idx].flags & (P_WAS_SET|P_VI_DEF)))
set_option_default(opt_idx, OPT_FREE, FALSE);
didset_options();
}

This comment has been minimized.

@justinmk

justinmk Dec 8, 2014

Member

Instead of my other suggestions, try restoring this block. That should take care of initializing everything correctly, and maybe it will make the memory leak go away.

@justinmk

justinmk Dec 8, 2014

Member

Instead of my other suggestions, try restoring this block. That should take care of initializing everything correctly, and maybe it will make the memory leak go away.

This comment has been minimized.

@klusark

klusark Dec 8, 2014

Contributor

Won't it only initialize things correctly if a vimrc file exists? It looks to me that we should just call compatible_set from set_init_1, and remove all the other calls. That way it's always setup correctly.

@klusark

klusark Dec 8, 2014

Contributor

Won't it only initialize things correctly if a vimrc file exists? It looks to me that we should just call compatible_set from set_init_1, and remove all the other calls. That way it's always setup correctly.

This comment has been minimized.

@justinmk

justinmk Dec 8, 2014

Member

Yep, I somehow forgot about that. A good time to call it would be wherever it would normally be called for the -N option.

@justinmk

justinmk Dec 8, 2014

Member

Yep, I somehow forgot about that. A good time to call it would be wherever it would normally be called for the -N option.

@justinmk justinmk changed the title from [RFC] Make test29 work without compatible to [RFC] default to 'nocompatible' Dec 8, 2014

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor

I tried just putting it in set_init_1, but that didn't resolve the memory leak... I'll see if I can reproduce it locally to see what's going on.

Contributor

klusark commented Dec 8, 2014

I tried just putting it in set_init_1, but that didn't resolve the memory leak... I'll see if I can reproduce it locally to see what's going on.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 8, 2014

Member

@klusark I should have thought of this earlier, but I bet if you remove aea8b63, ASAN will stop complaining. It could be that you're just indirectly exposing a long-existing bug.

Wait, nevermind, it was passing until you added bbfbfba, wasn't it? I think it's time for me to go to bed...

Member

justinmk commented Dec 8, 2014

@klusark I should have thought of this earlier, but I bet if you remove aea8b63, ASAN will stop complaining. It could be that you're just indirectly exposing a long-existing bug.

Wait, nevermind, it was passing until you added bbfbfba, wasn't it? I think it's time for me to go to bed...

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor
Contributor

klusark commented Dec 8, 2014

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 8, 2014

Contributor

It's actually a memory leak that already exists, but is just always triggered by this PR. To reproduce, create a .nvimrc file, run nvim build with master and execute

:wv! Xviminfo
:rv! Xviminfo

Should I report it as a separate bug? I'm not really familiar with vim's internals to fix it quickly.

Contributor

klusark commented Dec 8, 2014

It's actually a memory leak that already exists, but is just always triggered by this PR. To reproduce, create a .nvimrc file, run nvim build with master and execute

:wv! Xviminfo
:rv! Xviminfo

Should I report it as a separate bug? I'm not really familiar with vim's internals to fix it quickly.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 8, 2014

Member

Good to know. Since you found reproduction steps, reporting it to https://code.google.com/p/vim/issues/list or on vim_dev may result in Christian fixing it quickly.

Member

justinmk commented Dec 8, 2014

Good to know. Since you found reproduction steps, reporting it to https://code.google.com/p/vim/issues/list or on vim_dev may result in Christian fixing it quickly.

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark
Contributor

klusark commented Dec 8, 2014

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 9, 2014

Contributor

Chris posted a patch to the google code bug that fixes the memory leak. I added it as aa36c55, but I'm not really sure how to credit him properly. How does nvim normally handle cases like these?

Contributor

klusark commented Dec 9, 2014

Chris posted a patch to the google code bug that fixes the memory leak. I added it as aa36c55, but I'm not really sure how to credit him properly. How does nvim normally handle cases like these?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 9, 2014

Member

Well, normally we wait for the patch to hit Vim repo. In this case we could credit Christian using git commit --amend --author. We also need vim-patch:? in the commit message. (There's no patch number, so just use ?)

Member

justinmk commented Dec 9, 2014

Well, normally we wait for the patch to hit Vim repo. In this case we could credit Christian using git commit --amend --author. We also need vim-patch:? in the commit message. (There's no patch number, so just use ?)

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 10, 2014

Member

Much better! Need to credit @oni-link then too :)

Member

justinmk commented Dec 10, 2014

Much better! Need to credit @oni-link then too :)

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Dec 10, 2014

Member

Much better! Need to credit @oni-link then too :)

Yes, awesome work all around, LGTM!

Member

aktau commented Dec 10, 2014

Much better! Need to credit @oni-link then too :)

Yes, awesome work all around, LGTM!

Show outdated Hide outdated src/nvim/option.c Outdated
if (!p_cp) {
break;
}
continue;

This comment has been minimized.

@justinmk

justinmk Dec 10, 2014

Member

Is p != NULL guaranteed now?

@justinmk

justinmk Dec 10, 2014

Member

Is p != NULL guaranteed now?

This comment has been minimized.

@klusark

klusark Dec 10, 2014

Contributor

Yes. The text used to have NULL in order to separate compatible information, but that must have been removed at some time.

@klusark

klusark Dec 10, 2014

Contributor

Yes. The text used to have NULL in order to separate compatible information, but that must have been removed at some time.

@three-comrades

This comment has been minimized.

Show comment
Hide comment
@three-comrades

three-comrades Dec 12, 2014

Contributor

After giving this some more thought, I think that:

  • set nocp should just do nothing
  • set cp (and probably set cp!) should print a meaningful error message.

If someone uses set cp he expects a certain behavior. If we just do nothing this means that the command silently 'fails' and might result in unexpected behavior later on. I think that silent breakage is a lot worse for a first time user of nvim than an explicit failure with a meaningful error message that could refer to more information in the docs which explain the change.

Contributor

three-comrades commented Dec 12, 2014

After giving this some more thought, I think that:

  • set nocp should just do nothing
  • set cp (and probably set cp!) should print a meaningful error message.

If someone uses set cp he expects a certain behavior. If we just do nothing this means that the command silently 'fails' and might result in unexpected behavior later on. I think that silent breakage is a lot worse for a first time user of nvim than an explicit failure with a meaningful error message that could refer to more information in the docs which explain the change.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk
Member

justinmk commented Dec 13, 2014

@three-comrades Agreed.

@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 13, 2014

Contributor

I made it output "E???: Compatible not supported". I'm not sure what number I should assign it. Is there a scheme for doing that? I guess we don't want to conflict with vim's error codes if it adds some. Has neovim added error codes?

Contributor

klusark commented Dec 13, 2014

I made it output "E???: Compatible not supported". I'm not sure what number I should assign it. Is there a scheme for doing that? I guess we don't want to conflict with vim's error codes if it adds some. Has neovim added error codes?

@three-comrades

This comment has been minimized.

Show comment
Hide comment
@three-comrades

three-comrades Dec 13, 2014

Contributor

Has neovim added error codes?

I have no idea. But if not and to be future proof towards changes in vim, we probably need to use an identifier unique to nvim - sth. like ENVIM001?

Another thing: To be consistent, the -N command line option should probably also throw an error. I guess you could either use
ME_UNKNOWN_OPTION in main.c, when calling merror, or define sth. like ME_REMOVED_OPTION. IMO the latter would be more helpful.

Edit: #1656 just removes a command line option altogether, so we can do the same here, I guess, which will trigger the ME_UNKNOWN_OPTION error.

Contributor

three-comrades commented Dec 13, 2014

Has neovim added error codes?

I have no idea. But if not and to be future proof towards changes in vim, we probably need to use an identifier unique to nvim - sth. like ENVIM001?

Another thing: To be consistent, the -N command line option should probably also throw an error. I guess you could either use
ME_UNKNOWN_OPTION in main.c, when calling merror, or define sth. like ME_REMOVED_OPTION. IMO the latter would be more helpful.

Edit: #1656 just removes a command line option altogether, so we can do the same here, I guess, which will trigger the ME_UNKNOWN_OPTION error.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 13, 2014

Member

-N should be silently ignored

Member

justinmk commented Dec 13, 2014

-N should be silently ignored

@splinterofchaos

This comment has been minimized.

Show comment
Hide comment
@splinterofchaos

splinterofchaos Dec 13, 2014

Member

we probably need to use an identifier unique to nvim - sth. like ENVIM001?

nvim errors start at "E900" with e_invjob

Member

splinterofchaos commented Dec 13, 2014

we probably need to use an identifier unique to nvim - sth. like ENVIM001?

nvim errors start at "E900" with e_invjob

Show outdated Hide outdated src/nvim/globals.h Outdated
Show outdated Hide outdated src/nvim/option.c Outdated
@klusark

This comment has been minimized.

Show comment
Hide comment
@klusark

klusark Dec 13, 2014

Contributor

Shouldn't we keep -N and have it do nothing, but remove -C?

Contributor

klusark commented Dec 13, 2014

Shouldn't we keep -N and have it do nothing, but remove -C?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 13, 2014

Member

Shouldn't we keep -N and have it do nothing, but remove -C?

Yes, exactly.

Member

justinmk commented Dec 13, 2014

Shouldn't we keep -N and have it do nothing, but remove -C?

Yes, exactly.

@three-comrades

This comment has been minimized.

Show comment
Hide comment
@three-comrades

three-comrades Dec 13, 2014

Contributor

Shouldn't we keep -N and have it do nothing, but remove -C?

Sry, I mixed the options up.

Contributor

three-comrades commented Dec 13, 2014

Shouldn't we keep -N and have it do nothing, but remove -C?

Sry, I mixed the options up.

Show outdated Hide outdated src/nvim/main.c Outdated
@@ -1138,7 +1137,7 @@ static void command_line_scan(mparm_T *parmp)
break;
case 'N': /* "-N" Nocompatible */
change_compatible(FALSE);
/* No-op */
break;

This comment has been minimized.

@justinmk

justinmk Dec 13, 2014

Member

This block should stay as you have it, so it is silently ignored.

But the nvim --help messages for both -C and -N should be removed.

@justinmk

justinmk Dec 13, 2014

Member

This block should stay as you have it, so it is silently ignored.

But the nvim --help messages for both -C and -N should be removed.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 13, 2014

Member

When I run ./build/bin/nvim -C on this branch, cp is still set somehow.

Never mind, my mistake. Looks good so far.

Member

justinmk commented Dec 13, 2014

When I run ./build/bin/nvim -C on this branch, cp is still set somehow.

Never mind, my mistake. Looks good so far.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 13, 2014

Member

After the last 2 comments above, this LGTM.

Member

justinmk commented Dec 13, 2014

After the last 2 comments above, this LGTM.

justinmk added a commit that referenced this pull request Dec 13, 2014

@justinmk justinmk merged commit 9af6485 into neovim:master Dec 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@justinmk justinmk removed the RFC label Dec 13, 2014

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 13, 2014

Member

@klusark @three-comrades Thanks for following through with this!

Member

justinmk commented Dec 13, 2014

@klusark @three-comrades Thanks for following through with this!

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