| Message ID | 20201209065537.48802-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vim: configuration and sharness syntax | expand |
Hi, Felipe Contreras schrieb am Mittwoch, den 09. Dezember 2020: > +augroup git > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > + > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 > + au FileType sh setl noexpandtab tabstop=8 shiftwidth=0 > + au FileType perl setl noexpandtab tabstop=8 shiftwidth=0 > + au FileType asciidoc setl noexpandtab tabstop=8 shiftwidth=0 autoindent > +augroup END This will set filetype specific options. So after this file has been loaded, it will set e.g. set tabstop and shiftwidth options for filetypes outside of the git project. Shouldn't this only apply to files inside the git code repository? > + > +" vim: noexpandtab tabstop=8 shiftwidth=0 > diff --git a/contrib/vim/plugin/gitvimrc.vim b/contrib/vim/plugin/gitvimrc.vim > new file mode 100644 > index 0000000000..c3946e5410 > --- /dev/null > +++ b/contrib/vim/plugin/gitvimrc.vim > @@ -0,0 +1,21 @@ > +let s:gitvimrc_whitelist = get(g:, 'gitvimrc_whitelist', []) > + > +function LoadGitVimrc() > + let l:top = trim(system('git rev-parse --show-toplevel')) trim needs at least vim 8.0.1630. Is this recent enough? Could also use systemlist()[0] which is available starting at vim 7.4.248 or just a simple split(system(), "\n")[0] which should be compatible with vim 7. > + if l:top == '' | return | endif > + let l:file = l:top . '/.vimrc' > + if !filereadable(l:file) | return | endif > + > + let l:found = 0 > + for l:pattern in s:gitvimrc_whitelist You could directly use `get(g:, 'gitvimrc_whitelist', [])` directly, so the script local var s:gitvimrc_whitelist is not really needed. > + if (match(l:top, l:pattern) != -1) This uses a regex match. Perhaps do a string comparsion? If this is needed, consider adding "\C" to force matching case and perhaps also \V to force a literal match. Otherwise the options magic, ignorecase, smartcase etc are applied to the matching. > + let l:found = 1 > + break > + endif > + endfor > + if !l:found | return | endif > + > + exec 'source ' . fnameescape(l:file) > +endf > + > +call LoadGitVimrc() On the style: I personally dislike the `l:` prefix for function local variables, as this does not add anything. But perhaps this is just my personal preference. Best, Christian
Hello, On Wed, Dec 9, 2020 at 2:54 AM Christian Brabandt <cb@256bit.org> wrote: > Felipe Contreras schrieb am Mittwoch, den 09. Dezember 2020: > > > +augroup git > > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > > + > > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 > > + au FileType sh setl noexpandtab tabstop=8 shiftwidth=0 > > + au FileType perl setl noexpandtab tabstop=8 shiftwidth=0 > > + au FileType asciidoc setl noexpandtab tabstop=8 shiftwidth=0 autoindent > > +augroup END > > This will set filetype specific options. So after this file has been > loaded, it will set e.g. set tabstop and shiftwidth options for > filetypes outside of the git project. > > Shouldn't this only apply to files inside the git code repository? Yes. But this file can only be loaded if your cwd is inside this repository. That is; if "git rev-parse --show-toplevel" shows the same directory as this file. > > + > > +" vim: noexpandtab tabstop=8 shiftwidth=0 > > diff --git a/contrib/vim/plugin/gitvimrc.vim b/contrib/vim/plugin/gitvimrc.vim > > new file mode 100644 > > index 0000000000..c3946e5410 > > --- /dev/null > > +++ b/contrib/vim/plugin/gitvimrc.vim > > @@ -0,0 +1,21 @@ > > +let s:gitvimrc_whitelist = get(g:, 'gitvimrc_whitelist', []) > > + > > +function LoadGitVimrc() > > + let l:top = trim(system('git rev-parse --show-toplevel')) > > trim needs at least vim 8.0.1630. Is this recent enough? 2018? I think that's good enough. If not I'd be happy to include any other suggestion. > Could also use > systemlist()[0] which is available starting at vim 7.4.248 or just a > simple split(system(), "\n")[0] which should be compatible with vim 7. Yeah, in Linux. Will that work in Windows where carriage returns are "\r\n"? > > + if l:top == '' | return | endif > > + let l:file = l:top . '/.vimrc' > > + if !filereadable(l:file) | return | endif > > + > > + let l:found = 0 > > + for l:pattern in s:gitvimrc_whitelist > > You could directly use `get(g:, 'gitvimrc_whitelist', [])` directly, so > the script local var s:gitvimrc_whitelist is not really needed. True. It's just a force of habit to copy the global scope to the script scope. That being said; the "for" would call the get() function multiple times (probably). So I'm not entirely sure what is being gained. > > + if (match(l:top, l:pattern) != -1) > > This uses a regex match. Perhaps do a string comparsion? If this is > needed, consider adding "\C" to force matching case and perhaps also \V > to force a literal match. Otherwise the options magic, ignorecase, > smartcase etc are applied to the matching. This was straight-up copied from another solution. I just checked :h match() and didn't find any low-hanging fruit. If you have a better proposal just type it out. I'm not overly familiar with vimscript, I just know the above works. > > + let l:found = 1 > > + break > > + endif > > + endfor > > + if !l:found | return | endif > > + > > + exec 'source ' . fnameescape(l:file) > > +endf > > + > > +call LoadGitVimrc() > > On the style: I personally dislike the `l:` prefix for function local > variables, as this does not add anything. But perhaps this is just my > personal preference. I don't mind either way. I just add it for consistency since the syntax sometimes doesn't identify such variables (e.g "if !found"), but most of the time the syntax doesn't do it either way (which is odd). So just s/l:// ? Cheers.
On Mi, 09 Dez 2020, Felipe Contreras wrote: > On Wed, Dec 9, 2020 at 2:54 AM Christian Brabandt <cb@256bit.org> wrote: > > > Felipe Contreras schrieb am Mittwoch, den 09. Dezember 2020: > > > > > +augroup git > > > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > > > + > > > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 > > > + au FileType sh setl noexpandtab tabstop=8 shiftwidth=0 > > > + au FileType perl setl noexpandtab tabstop=8 shiftwidth=0 > > > + au FileType asciidoc setl noexpandtab tabstop=8 shiftwidth=0 autoindent > > > +augroup END > > > > This will set filetype specific options. So after this file has been > > loaded, it will set e.g. set tabstop and shiftwidth options for > > filetypes outside of the git project. > > > > Shouldn't this only apply to files inside the git code repository? > > Yes. But this file can only be loaded if your cwd is inside this > repository. That is; if "git rev-parse --show-toplevel" shows the same > directory as this file. Yes, however what I was trying to say was: Once I edited a file from within the git source repository, this means it will apply to all further files I will edit in this session. So I do `:e ~/bin/my_precious_shell_script.sh` it will apply those settings there as well. So I would rather call a function in the FileType autocommand, that checks the path of the currently edited file before it applies those settings. > > > + > > > +" vim: noexpandtab tabstop=8 shiftwidth=0 > > > diff --git a/contrib/vim/plugin/gitvimrc.vim b/contrib/vim/plugin/gitvimrc.vim > > > new file mode 100644 > > > index 0000000000..c3946e5410 > > > --- /dev/null > > > +++ b/contrib/vim/plugin/gitvimrc.vim > > > @@ -0,0 +1,21 @@ > > > +let s:gitvimrc_whitelist = get(g:, 'gitvimrc_whitelist', []) > > > + > > > +function LoadGitVimrc() > > > + let l:top = trim(system('git rev-parse --show-toplevel')) > > > > trim needs at least vim 8.0.1630. Is this recent enough? > > 2018? I think that's good enough. If not I'd be happy to include any > other suggestion. Not sure. CentOS 7 seems to have 7.4.629 and CentOS 8 8.0.1763, Ubuntu LTS 16.04 7.4.1689, all according to https://repology.org/project/vim/versions And then there is neovim. I suppose it has trim() > > Could also use > > systemlist()[0] which is available starting at vim 7.4.248 or just a > > simple split(system(), "\n")[0] which should be compatible with vim 7. > > Yeah, in Linux. Will that work in Windows where carriage returns are "\r\n"? Yes. > > > + if l:top == '' | return | endif > > > + let l:file = l:top . '/.vimrc' > > > + if !filereadable(l:file) | return | endif > > > + > > > + let l:found = 0 > > > + for l:pattern in s:gitvimrc_whitelist > > > > You could directly use `get(g:, 'gitvimrc_whitelist', [])` directly, so > > the script local var s:gitvimrc_whitelist is not really needed. > > True. It's just a force of habit to copy the global scope to the > script scope. That being said; the "for" would call the get() function > multiple times (probably). So I'm not entirely sure what is being > gained. This function is called only once and get() should be quite fast. > > > > + if (match(l:top, l:pattern) != -1) > > > > This uses a regex match. Perhaps do a string comparsion? If this is > > needed, consider adding "\C" to force matching case and perhaps also \V > > to force a literal match. Otherwise the options magic, ignorecase, > > smartcase etc are applied to the matching. > > This was straight-up copied from another solution. I just checked :h > match() and didn't find any low-hanging fruit. > > If you have a better proposal just type it out. I'm not overly > familiar with vimscript, I just know the above works. Is comparing literally good enough? e.g. if top ==# pattern (this would match case, or use ==? to ignore case). In any case, make case matching explicit, so that the options `ignorecase` and `smartcase` are not used. > > > > + let l:found = 1 > > > + break > > > + endif > > > + endfor > > > + if !l:found | return | endif > > > + > > > + exec 'source ' . fnameescape(l:file) > > > +endf > > > + > > > +call LoadGitVimrc() > > > > On the style: I personally dislike the `l:` prefix for function local > > variables, as this does not add anything. But perhaps this is just my > > personal preference. > > I don't mind either way. I just add it for consistency since the > syntax sometimes doesn't identify such variables (e.g "if !found"), > but most of the time the syntax doesn't do it either way (which is > odd). You mean the vimscript syntax? I don't remember seeing such. > So just s/l:// ? Yes, unless you use a variable called count, which would be shadowed by v:count Best, Christian
On Wed, Dec 09, 2020 at 12:55:36AM -0600, Felipe Contreras wrote: > +augroup git > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > + > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 I had to read up on a few of these settings, and I'm still slightly puzzled: - I generally leave shiftwidth=8, but reading the documentation says that 0 is equivalent to "1 tabstop". So that should be equivalent. - I've been using "(0" for years for my git work (which indents to align new lines with the unclosed parenthesis). I'm not quite sure what "(s" means. The documentation says "1s" would be "one shiftwidth". Is just "s" the same? - I also have ":0", which doesn't indent case labels. Matches our style. - I didn't have "l" set myself. I never noticed because it only matters if you open a case with an extra brace, which is relatively rare. For non-vim folks, it is preferring: switch (foo) { case 0: { break; } to: switch (foo) { case 0: { break; } which seems consistent with our style. So I think that is worth doing. - t0 is specifying not to indent function return types when they appear on a separate line. But our style is not to put those return types on a separate line, anyway. Do we need this? -Peff
On Wed, Dec 9, 2020 at 11:27 AM Jeff King <peff@peff.net> wrote: > > On Wed, Dec 09, 2020 at 12:55:36AM -0600, Felipe Contreras wrote: > > > +augroup git > > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > > + > > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 > > I had to read up on a few of these settings, and I'm still slightly > puzzled: > > - I generally leave shiftwidth=8, but reading the documentation says > that 0 is equivalent to "1 tabstop". So that should be equivalent. Yes. It is. If you read the help of tabstop [1] it says there are four main ways of using tab, and we are using the fourth one: "always set 'tabstop' and 'shiftwidth' to the same value, and 'noexpandtab'." Other projects use a different tabstop, and expandtab (mode 2), however, I have *never* found a use case where it made sense to have a different shiftwidth than tabstop. And it gets tedious to *always* do ts=X sw=X, when you can just do sw=0 in your ~/.vimrc, and ts=X per project. > - I've been using "(0" for years for my git work (which indents to > align new lines with the unclosed parenthesis). I'm not quite sure > what "(s" means. The documentation says "1s" would be "one > shiftwidth". Is just "s" the same? Yes. If you read CodingGuidelines it says there are two schools of thought when it comes to splitting long logical lines. The first example is "(s", the second one is "(0". The reason why I prefer "(s" is that this is more commonly used in the Linux kernel. However, it's not quite the same in vim (when there's more than one parenthesis). I've planned to contact vim developers about that, but I haven't yet. Just for that reason it might make sense to use "(0" for the project. > - I also have ":0", which doesn't indent case labels. Matches our > style. > > - I didn't have "l" set myself. I never noticed because it only > matters if you open a case with an extra brace, which is relatively > rare. For non-vim folks, it is preferring: > > switch (foo) { > case 0: { > break; > } > > to: > > switch (foo) { > case 0: { > break; > } > > which seems consistent with our style. So I think that is worth > doing. > > - t0 is specifying not to indent function return types when they > appear on a separate line. But our style is not to put those return > types on a separate line, anyway. Do we need this? Right. I recall at some point it was annoying me that types were auto indented magically at wrong times. Testing "ts" that doesn't seem to happen anymore, but it also doesn't seem to be working at all. Do you see some difference from "t0" and "ts" with: void main(void) { } Cheers. [1] https://vimhelp.org/options.txt.html#%27tabstop%27
On 2020-12-09 at 06:55:36, Felipe Contreras wrote: > diff --git a/.vimrc b/.vimrc > new file mode 100644 > index 0000000000..602c746477 > --- /dev/null > +++ b/.vimrc > @@ -0,0 +1,22 @@ > +" To make use of these configurations install the git plugin provided in > +" the contrib section: > +" > +" cp -aT contrib/vim ~/.vim/pack/plugins/start/git > +" > +" Then whitelist the location of this directory to your ~/.vimrc: > +" > +" let g:gitvimrc_whitelist = [ expand('$HOME') . '/dev/git' ] > +" > +" You can add multiple locations, or specify a regexp pattern. > +" > + > +augroup git > + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc > + > + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 > + au FileType sh setl noexpandtab tabstop=8 shiftwidth=0 > + au FileType perl setl noexpandtab tabstop=8 shiftwidth=0 > + au FileType asciidoc setl noexpandtab tabstop=8 shiftwidth=0 autoindent > +augroup END I don't think this should go in this location. It should go in contrib. Here's why: * We should not ship editor-specific files in the main directory of the repository. Even though Vim is very popular, it is one of many editors, and it is not even the most popular editor (which is now VS Code). We have editor-independent files, and users can copy this into the root of the repository and ignore it if they want it there. * Whether a user wants to use automatic indentation is a personal preference. I do happen to like it, but there are others who don't and prefer to leave it off. Similarly, whether to use cindent, smartindent, or autoindent is a preference, as is which cindent options to use (I use different ones). * These settings affect every file that's loaded in the same editor process. While many people open different editor windows for different projects, other people prefer to use the client-server functionality to load all of their projects in the same editor. These are not, for example, the editor settings I normally use for non-Git AsciiDoc files. So while I agree that these are common settings, they are not universally applicable, even for Vim and Neovim users, and we shouldn't try to claim that all or even most Vim and Neovim users should use them. In contrast, the .editorconfig file specifies things which are (a) guaranteed to affect only this repository and (b) are essential parts of our coding style. It notably omits things like line endings which are a matter of user or platform preference. So I think contrib makes more sense here.
On Wed, Dec 09, 2020 at 07:55:55PM -0600, Felipe Contreras wrote: > > - t0 is specifying not to indent function return types when they > > appear on a separate line. But our style is not to put those return > > types on a separate line, anyway. Do we need this? > > Right. I recall at some point it was annoying me that types were auto > indented magically at wrong times. Testing "ts" that doesn't seem to > happen anymore, but it also doesn't seem to be working at all. > > Do you see some difference from "t0" and "ts" with: > > void > main(void) { } No, but picking it does seem to impact a larger example. If I open up wt-status.c and modify the first function to be: static const char * color(int slot, struct wt_status *s) { then reindenting it with t0 versus ts makes a difference (and I do prefer the t0 behavior). But we would not use that split-line style in our project in the first place, I don't think. -Peff
On Thu, Dec 10, 2020 at 9:27 AM Jeff King <peff@peff.net> wrote: > > On Wed, Dec 09, 2020 at 07:55:55PM -0600, Felipe Contreras wrote: > > > > - t0 is specifying not to indent function return types when they > > > appear on a separate line. But our style is not to put those return > > > types on a separate line, anyway. Do we need this? > > > > Right. I recall at some point it was annoying me that types were auto > > indented magically at wrong times. Testing "ts" that doesn't seem to > > happen anymore, but it also doesn't seem to be working at all. > > > > Do you see some difference from "t0" and "ts" with: > > > > void > > main(void) { } > > No, but picking it does seem to impact a larger example. If I open up > wt-status.c and modify the first function to be: > > static const char * > color(int slot, struct wt_status *s) > { > > then reindenting it with t0 versus ts makes a difference (and I do > prefer the t0 behavior). I see. For some reason this is indented: void main(void) { But not this: void main(void) { > But we would not use that split-line style in > our project in the first place, I don't think. No, we don't use it, but I recall some problems when not setting it (perhaps pasting code with that style). Anyway, I can't reproduce any of the problems, so I'm fine with dropping it. Cheers.
On Wed, Dec 9, 2020 at 9:51 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > On 2020-12-09 at 06:55:36, Felipe Contreras wrote: > I don't think this should go in this location. It should go in contrib. > Here's why: > > * We should not ship editor-specific files in the main directory of the > repository. Why not? > Even though Vim is very popular, it is one of many > editors, and it is not even the most popular editor (which is now VS > Code). Even if vim is not the most popular, it certainly is among the top 3 (and I doubt VS Code is the most popular, I would like to see some numbers on that, but even then; VS Code is not an editor). Nobody is arguing to have editor-specific files for "every editor under the sun", just perhaps 2 (or maybe even 3). No slippery slope fallacy here. > We have editor-independent files, and users can copy this into > the root of the repository and ignore it if they want it there. Which are insufficient. They are certainly better than nothing. Plus, it's unclear how many people are actually using those. And I'm still waiting for the argument against adding such a top-level file. What is the harm? > * Whether a user wants to use automatic indentation is a personal > preference. I do happen to like it, but there are others who don't > and prefer to leave it off. Similarly, whether to use cindent, > smartindent, or autoindent is a preference, as is which cindent > options to use (I use different ones). So? These options will not be forced on users, they have to specifically enable them by doing at least two steps, *and* they can still selectively override them in their ~/.vim files. > * These settings affect every file that's loaded in the same editor > process. That is not true. :setlocal [1] applies the setting to the current buffer only, not globally, and *only* when the buffer is of the filetype specified in the autocommand. > So while I agree that these are common settings, they are not > universally applicable, even for Vim and Neovim users, and we shouldn't > try to claim that all or even most Vim and Neovim users should use them. We don't. These are defaults, which a) the user must consciously choose to apply them, and b) can be easily overridden (as is explained in the commit message). > So I think contrib makes more sense here. Clearly. But you haven't put forward an argument about how precisely will this negatively affect *any* user (or the project). Cheers. [1] https://vimhelp.org/options.txt.html#%3Asetlocal
On 2020-12-11 at 01:08:00, Felipe Contreras wrote: > On Wed, Dec 9, 2020 at 9:51 PM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > On 2020-12-09 at 06:55:36, Felipe Contreras wrote: > > > I don't think this should go in this location. It should go in contrib. > > Here's why: > > > > * We should not ship editor-specific files in the main directory of the > > repository. > > Why not? Best practices indicate that we don't check in files which are specific to a developer. Anything that controls the specific editor people use is by definition specific to the developer. Checking in these files leads to conflicts over which settings to apply and whose settings are better when they could just be avoided. If we have style policies, those should be expressed in a general, universal way so that all users can take advantage of them in the same way. Furthermore, some editors want entire large directories of configuration files in order to work correctly, which we don't want to include. If we treat all editors in the same way, then every developer gets the same experience when they work on our code. If that experience is inadequate, our time would be better spent improving it in a universal way so that all developers can benefit. > > Even though Vim is very popular, it is one of many > > editors, and it is not even the most popular editor (which is now VS > > Code). > > Even if vim is not the most popular, it certainly is among the top 3 > (and I doubt VS Code is the most popular, I would like to see some > numbers on that, but even then; VS Code is not an editor). > > Nobody is arguing to have editor-specific files for "every editor > under the sun", just perhaps 2 (or maybe even 3). > > No slippery slope fallacy here. Because we don't need them. Your solution requires the user to configure Vim with a plugin _and then_ allow the specific directory in order to be secure, which means it doesn't work with worktrees. It also requires that the user never pull an untrusted branch into their repository. It also has other undesirable effects which I mentioned in my original email. The .editorconfig file also requires a user to configure a plugin, once, and then things automatically work in a secure way across projects. In other words, the existing solution requires a user to affirmatively act, but with less effort, less potential for security problems, and better cross-project support. So the .vimrc solution requires more effort, has more potential security problems, is less flexible, is less like how other projects solve this problem, and is less general. > > We have editor-independent files, and users can copy this into > > the root of the repository and ignore it if they want it there. > > Which are insufficient. They are certainly better than nothing. Plus, > it's unclear how many people are actually using those. Why are they insufficient? Multiple developers are using them on Git already. They're used on projects from Microsoft[0], W3C[1], and folks working on JSONPath[2]. They are the de facto standard for this purpose. In contrast, searching GitHub commits for ".vimrc" shows overwhelmingly that the repositories in which these commits are named are called "dotfiles". I was unable to find any projects from major organizations using this configuration style. My general rule is that when I'm unsure what decision to make on a project, I should make the decision that everybody else has made, because users and developers will expect my project to work just like everyone else's. > And I'm still waiting for the argument against adding such a top-level file. > > What is the harm? As mentioned, enabling the use of this file is still risky from a security perspective because it precludes even pulling in an untrusted branch and then spawning an editor. We already have a more general solution that is more widely adopted and has fewer downsides, so there's no point in adding files which really provide little benefit over what we already have. If there's little benefit, we shouldn't carry files which are going to be subject mostly to pointless arguments over personal preference. The fact that two heavy Vim users disagree so strongly over relatively simple settings is an argument for not adopting this approach as a set of project settings. > > * Whether a user wants to use automatic indentation is a personal > > preference. I do happen to like it, but there are others who don't > > and prefer to leave it off. Similarly, whether to use cindent, > > smartindent, or autoindent is a preference, as is which cindent > > options to use (I use different ones). > > So? > > These options will not be forced on users, they have to specifically > enable them by doing at least two steps, *and* they can still > selectively override them in their ~/.vim files. Right, but why are your preferred settings checked into Git as a project setting? They are objectively no better than my settings, which differ. Absent a compelling reason that these settings are objectively better, we should not endorse them as preferred project settings. > > * These settings affect every file that's loaded in the same editor > > process. > > That is not true. > > :setlocal [1] applies the setting to the current buffer only, not > globally, and *only* when the buffer is of the filetype specified in > the autocommand. So if I spawn an editor process using this .vimrc in my Git directory and then I load an AsciiDoc file from a different repository into that same Vim process, are you arguing that the Git settings will not be applied to the AsciiDoc file from other directory? I'm pretty sure that Vim will in fact use the Git settings. It's possible, however, that I've misunderstood how Vim works. .editorconfig doesn't have these downsides. > > So while I agree that these are common settings, they are not > > universally applicable, even for Vim and Neovim users, and we shouldn't > > try to claim that all or even most Vim and Neovim users should use them. > > We don't. These are defaults, which a) the user must consciously > choose to apply them, and b) can be easily overridden (as is explained > in the commit message). I'm arguing that they are not universal enough to be defaults. Moreover, a set of defaults for how a user _could_ configure their editor would belong in contrib, much like defaults for how a user _could_ configure their MUA to send properly to the mailing list. We already have files for Emacs and VS Code, and those live properly in contrib, along with code for Thunderbird and alternative build systems. If we're treating this proposal like existing code, it belongs in contrib. The .editorconfig file, on the other hand, doesn't express defaults. It expresses only project standards and doesn't specify any other settings. [0] https://github.com/microsoft/fabrikate [1] https://github.com/w3c/specberus [2] https://github.com/jsonpath-standard/internet-draft
On Thu, Dec 10, 2020 at 8:57 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On 2020-12-11 at 01:08:00, Felipe Contreras wrote: > > On Wed, Dec 9, 2020 at 9:51 PM brian m. carlson > > <sandals@crustytoothpaste.net> wrote: > > > On 2020-12-09 at 06:55:36, Felipe Contreras wrote: > > > > > I don't think this should go in this location. It should go in contrib. > > > Here's why: > > > > > > * We should not ship editor-specific files in the main directory of the > > > repository. > > > > Why not? > > Best practices indicate that we don't check in files which are specific > to a developer. But it's not specific to a developer. > Anything that controls the specific editor people use > is by definition specific to the developer. Not really. Everyone that replied to the patch agreed on those settings. Did anyone say they use tabstop=4? > Checking in these files > leads to conflicts over which settings to apply and whose settings are > better when they could just be avoided. > > If we have style policies, those should be expressed in a general, > universal way so that all users can take advantage of them in the same > way. Do you have any specifics? Because nobody complained about the proposed settings. > Furthermore, some editors want entire large directories of configuration > files in order to work correctly, which we don't want to include. That's a problem for "some editors". Not vim. > If we treat all editors in the same way, then every developer gets the > same experience when they work on our code. But we don't want every developer to get the same experience. We want developers to get the best experience they can get from their editor of choice. We don't want the least common denominator. > If that experience is > inadequate, our time would be better spent improving it in a universal > way so that all developers can benefit. This is the nirvana fallacy. We don't have to wait for a perfect solution when we have a perfectly good enough solution. *Right now* we can help the vast majority of vim users. There's no reason not to do so. Feel free to contribute patches to editorconfig until the experience matches the proposed .vimrc. In the meantime the proposal is still the best solution. > > > Even though Vim is very popular, it is one of many > > > editors, and it is not even the most popular editor (which is now VS > > > Code). > > > > Even if vim is not the most popular, it certainly is among the top 3 > > (and I doubt VS Code is the most popular, I would like to see some > > numbers on that, but even then; VS Code is not an editor). > > > > Nobody is arguing to have editor-specific files for "every editor > > under the sun", just perhaps 2 (or maybe even 3). > > > > No slippery slope fallacy here. > > Because we don't need them. We don't need to need them. All we need is to want them (because it's better than the current situation). > Your solution requires the user to > configure Vim with a plugin _and then_ allow the specific directory in > order to be secure, which means it doesn't work with worktrees. The editorconfig solution also requires a plugin. And the .vimrc solution does work with worktrees. All the user has to do is specify them. Or just: let g:gitvimrc_whitelist = [ '.*' ] Plus, even if it didn't work with worktrees, it's still better than the current situation, where it works nowhere. > It also > requires that the user never pull an untrusted branch into their > repository. This is always the case. An untrusted branch can modify the git binary to do whatever it wants. > The .editorconfig file also requires a user to configure a plugin, once, > and then things automatically work in a secure way across projects. And have a *much poorer* configuration as a result. It's not even close. > So the .vimrc solution requires more effort, has more potential security > problems, is less flexible, is less like how other projects solve this > problem, and is less general. All that is hypothetical. What is *factually* the case is that the resulting configuration is much superior. > > > We have editor-independent files, and users can copy this into > > > the root of the repository and ignore it if they want it there. > > > > Which are insufficient. They are certainly better than nothing. Plus, > > it's unclear how many people are actually using those. > > Why are they insufficient? Multiple developers are using them on Git > already. They're used on projects from Microsoft[0], W3C[1], and folks > working on JSONPath[2]. They are the de facto standard for this > purpose. I already explained: 1. The sharness syntax is not set for tests 2. The asciidoc syntax is not set for the documentation 3. The specific cinoptios for C code are not set: "(s,:0,l1" All of these are improvements the people that replied to the proposal seem to want. > In contrast, searching GitHub commits for ".vimrc" shows overwhelmingly > that the repositories in which these commits are named are called > "dotfiles". I was unable to find any projects from major organizations > using this configuration style. This is the naturalistic fallacy. Just because in the current state most projects do not have a .vimrc does not mean we should follow the steps of most projects. Most projects have vim modelines at the end of each file. Shall we follow what most projects do? In addition it's the bandwagon fallacy: if all your friends jumped off a cliff, would you? The reason why most projects don't have a .vimrc file is that nobody has taken the time out of their normal tasks to improve the current situation. But I just did. > > And I'm still waiting for the argument against adding such a top-level file. > > > > What is the harm? > > As mentioned, enabling the use of this file is still risky from a > security perspective because it precludes even pulling in an untrusted > branch and then spawning an editor. That is always a risk. Have you ever pulled a branch from an untrusted source and not looked at the commits? > We already have a more general > solution that is more widely adopted and has fewer downsides, so there's > no point in adding files which really provide little benefit over what > we already have. Is it widely adopted? I've never heard of editorconfig. > If there's little benefit, we shouldn't carry files which are going to > be subject mostly to pointless arguments over personal preference. Who says there's little benefit? Nobody that replied objects to this change (except you). If you see little benefit, then you don't use this .vimrc solution. Why are you against the rest of us making our own decision out of our own volition? > The > fact that two heavy Vim users disagree so strongly over relatively > simple settings is an argument for not adopting this approach as a set > of project settings. Who are these two heavy vim users that disagree so strongly? > > > * Whether a user wants to use automatic indentation is a personal > > > preference. I do happen to like it, but there are others who don't > > > and prefer to leave it off. Similarly, whether to use cindent, > > > smartindent, or autoindent is a preference, as is which cindent > > > options to use (I use different ones). > > > > So? > > > > These options will not be forced on users, they have to specifically > > enable them by doing at least two steps, *and* they can still > > selectively override them in their ~/.vim files. > > Right, but why are your preferred settings checked into Git as a project > setting? They are not my preferred settings. Everyone (so far) has agreed these are good project-wide settings. > They are objectively no better than my settings, which differ. How do they differ? What are your settings? > Absent a compelling reason that these settings are objectively better, > we should not endorse them as preferred project settings. They don't have to be better than *your* settings. They have to be better than vim's default settings, which they are. *Your* settings will not be overridden. > > > * These settings affect every file that's loaded in the same editor > > > process. > > > > That is not true. > > > > :setlocal [1] applies the setting to the current buffer only, not > > globally, and *only* when the buffer is of the filetype specified in > > the autocommand. > > So if I spawn an editor process using this .vimrc in my Git directory > and then I load an AsciiDoc file from a different repository into that > same Vim process, are you arguing that the Git settings will not be > applied to the AsciiDoc file from other directory? I'm pretty sure that > Vim will in fact use the Git settings. It's possible, however, that > I've misunderstood how Vim works. In that particular case; yes, those settings would be applied. Configurations are never perfect. If this particular configuration bothers you, and I fix that. Would you then approve of this change? > > > So while I agree that these are common settings, they are not > > > universally applicable, even for Vim and Neovim users, and we shouldn't > > > try to claim that all or even most Vim and Neovim users should use them. > > > > We don't. These are defaults, which a) the user must consciously > > choose to apply them, and b) can be easily overridden (as is explained > > in the commit message). > > I'm arguing that they are not universal enough to be defaults. And yet everyone else that replied is fine with them. > We already have files for Emacs and VS Code, and those live properly in > contrib, along with code for Thunderbird and alternative build systems. > If we're treating this proposal like existing code, it belongs in > contrib. And yet we have .editorconfig, .clang-format, and .tsan-suppressions, which don't seem to be hurting anybody. > The .editorconfig file, on the other hand, doesn't express defaults. It > expresses only project standards and doesn't specify any other settings. Fine. The .vimrc file doesn't express defaults. It expresses project standards. There. Now conceptually they are the same. Cheers.
On Fri, Dec 11, 2020 at 02:56:22AM +0000, brian m. carlson wrote: > > > * We should not ship editor-specific files in the main directory of the > > > repository. > > > > Why not? > > Best practices indicate that we don't check in files which are specific > to a developer. Anything that controls the specific editor people use > is by definition specific to the developer. Checking in these files > leads to conflicts over which settings to apply and whose settings are > better when they could just be avoided. I think that's a good general policy, but it's not unreasonable to help people make configure some widely used tools. The key things to me are: - we should do so at the most general level possible. I agree that .editorconfig is the right level for features it supports. But there are bits being suggested here that I think it does not (like how to indent case labels). We also have .clang-format, for which there's a vim plugin (but I've not used it, nor editorconfig, myself). It seems like it may support more options. - people who use the editor config take responsibility for maintaining it, and nobody else needs to care. E.g., I'd expect editorconfig to more of a source of truth than any vim config, and if there's a conflict for people who care about vim to sort it out (and not somebody who touched .editorconfig). - it doesn't suggest any actions that might be bad practices. I agree that the instructions for auto-loading this .vimrc are more complicated than necessary and might have security implications. Carrying a file in contrib/vim that says "copy this to ~/.vim/foo" or even "copy these lines to your ~/.vimrc" seems a lot safer. And it makes it easier for people who prefer to adapt the config to their own setup. So I'm not opposed to carrying some vim config, but I think it's best to focus on simplicity and providing human-readable instructions, rather than ad-hoc plugin infrastructure. -Peff
Jeff King wrote: > On Fri, Dec 11, 2020 at 02:56:22AM +0000, brian m. carlson wrote: > - it doesn't suggest any actions that might be bad practices. I agree > that the instructions for auto-loading this .vimrc are more > complicated than necessary and might have security implications. > Carrying a file in contrib/vim that says "copy this to ~/.vim/foo" > or even "copy these lines to your ~/.vimrc" seems a lot safer. And > it makes it easier for people who prefer to adapt the config to > their own setup. > > So I'm not opposed to carrying some vim config, but I think it's best to > focus on simplicity and providing human-readable instructions, rather > than ad-hoc plugin infrastructure. Generally I would agree, but do you know what such instructions would look like? In particular what instructions would look like for a person that contributes to more than 3 projects with different C code-style. I can assure they are anything but human-readable. Cheers.
On Mon, Dec 14, 2020 at 09:03:56PM -0600, Felipe Contreras wrote: > Jeff King wrote: > > On Fri, Dec 11, 2020 at 02:56:22AM +0000, brian m. carlson wrote: > > > - it doesn't suggest any actions that might be bad practices. I agree > > that the instructions for auto-loading this .vimrc are more > > complicated than necessary and might have security implications. > > Carrying a file in contrib/vim that says "copy this to ~/.vim/foo" > > or even "copy these lines to your ~/.vimrc" seems a lot safer. And > > it makes it easier for people who prefer to adapt the config to > > their own setup. > > > > So I'm not opposed to carrying some vim config, but I think it's best to > > focus on simplicity and providing human-readable instructions, rather > > than ad-hoc plugin infrastructure. > > Generally I would agree, but do you know what such instructions would look like? > > In particular what instructions would look like for a person that > contributes to more than 3 projects with different C code-style. > > I can assure they are anything but human-readable. Mostly what I'm suggesting is asking the user to copy the settings they want, rather than sourcing a file in the repository that may contain arbitrary options. So something like: " Settings to match Git's style/indentation preferences. " " You can put these straight in your .vimrc if you want to use " them all the time. Or if you want to use them only inside " certain directories, wrap them like this: " if match(getcwd(), "/path/to/your/git/repo") " au Filetype c setl ...etc... " endif " au FileType c setl ...etc... That means they won't automatically pick up new options if they change, but that's the point. They should be inspecting and deciding which options they want to take. The conditional above definitely has some flaws. It relies on the working directory rather than the location of the file (which is the same as your plugin; yours is just picking it up implicitly from calling git). And once the autoloaders are set up, I think they'd trigger for any C file, even outside the repository directory. Ideally we'd combine the autoloader for BufRead and FileType, but it seems non-trivial to do so. I think: au BufNewFile,BufRead /path/to/git/* if &filetype == "c" | setl ... | endif works, though it's a little clunky, as each line would need to repeat it. There might be a better way. I'm not that familiar with doing tricky things with vim's autoloading. But my point is mostly that the value in the information is saying "here are some useful vim settings you might want to use". I don't think we need to solve "here's how to trigger some settings for some directories" for everyone. We should let them integrate the settings as they see fit. -Peff
Jeff King wrote: > Ideally we'd combine the autoloader for BufRead and FileType, but it > seems non-trivial to do so. I think: > > au BufNewFile,BufRead /path/to/git/* if &filetype == "c" | setl ... | endif > > works, though it's a little clunky, as each line would need to repeat > it. Yeah, that works, *temporarily*. If the user has configured ~/.vim/after/ftplugin/c.vim, that would override those autocommand settings when the file is reloaded. Which is precisely why the above is not recommended. > I don't think we need to solve "here's how to trigger some settings > for some directories" for everyone. We should let them integrate the > settings as they see fit. Yeah. But how? I already explored this at dept, and I arrived at only one sensible option.
diff --git a/.vimrc b/.vimrc new file mode 100644 index 0000000000..602c746477 --- /dev/null +++ b/.vimrc @@ -0,0 +1,22 @@ +" To make use of these configurations install the git plugin provided in +" the contrib section: +" +" cp -aT contrib/vim ~/.vim/pack/plugins/start/git +" +" Then whitelist the location of this directory to your ~/.vimrc: +" +" let g:gitvimrc_whitelist = [ expand('$HOME') . '/dev/git' ] +" +" You can add multiple locations, or specify a regexp pattern. +" + +augroup git + au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc + + au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0 + au FileType sh setl noexpandtab tabstop=8 shiftwidth=0 + au FileType perl setl noexpandtab tabstop=8 shiftwidth=0 + au FileType asciidoc setl noexpandtab tabstop=8 shiftwidth=0 autoindent +augroup END + +" vim: noexpandtab tabstop=8 shiftwidth=0 diff --git a/contrib/vim/plugin/gitvimrc.vim b/contrib/vim/plugin/gitvimrc.vim new file mode 100644 index 0000000000..c3946e5410 --- /dev/null +++ b/contrib/vim/plugin/gitvimrc.vim @@ -0,0 +1,21 @@ +let s:gitvimrc_whitelist = get(g:, 'gitvimrc_whitelist', []) + +function LoadGitVimrc() + let l:top = trim(system('git rev-parse --show-toplevel')) + if l:top == '' | return | endif + let l:file = l:top . '/.vimrc' + if !filereadable(l:file) | return | endif + + let l:found = 0 + for l:pattern in s:gitvimrc_whitelist + if (match(l:top, l:pattern) != -1) + let l:found = 1 + break + endif + endfor + if !l:found | return | endif + + exec 'source ' . fnameescape(l:file) +endf + +call LoadGitVimrc()
It's not efficient that everyone must set specific configurations in all their ~/.vimrc files; we can have a project-wide .vimrc that everyone can use. There's different ways to load this configuration, for example with vim-addon-local-vimrc [1], but we don't need much of the complexity of these solutions. Instead I created a simple loader that is in the contrib area, which can be installed with: cp -aT contrib/vim ~/.vim/pack/plugins/start/git Then, add the location of the Git repository to your ~/.vimrc: let g:gitvimrc_whitelist = [ expand('$HOME') . '/dev/git' ] Then the project-wide configuration will be loaded, which sets the correct filetype for the documentation, and also the default indentation of c, sh, perl, and asciidoc files. These default configurations can be overridden in the typical way (by adding the corresponding file in ~/.vim/after/ftplugin). We could add the vim modelines at the bottom of every file, like other projects do, but this seems more sensible. [1] https://github.com/MarcWeber/vim-addon-local-vimrc Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- .vimrc | 22 ++++++++++++++++++++++ contrib/vim/plugin/gitvimrc.vim | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 .vimrc create mode 100644 contrib/vim/plugin/gitvimrc.vim