git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] vim: configuration and sharness syntax
@ 2020-12-09  6:55 Felipe Contreras
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-09  6:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder, Felipe Contreras

After investigating alternatives for exrc I found too many, doing a wide
range of irrelevant stuff, many unmaintained, others requiring multiple
dependencies, and some loaded the configuration too late.

The only one that seemed to fit the bill is vim-addon-local-vimrc, which
does work straightofrwardly, but hasn't been updated since 2015.

Instead I chose to simply vim-addon-local-vimrc, and take advantage of
git ('git rev-parse --show-toplevel' saves us a lot of the complexity of
these loaders).

The result is a very simple loader which is also secure, since it cannot
do anything unless you manually whitelist the project(s) you want load
.vimrc files from.

And since I already created some files in 'contrib/vim' I decided to put
the sharness syntax file there too.


Felipe Contreras (2):
  Add project-wide .vimrc configuration
  contrib: vim: add sharness syntax file

 .vimrc                          | 23 ++++++++++++++++++++++
 contrib/vim/plugin/gitvimrc.vim | 21 ++++++++++++++++++++
 contrib/vim/syntax/sharness.vim | 34 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100644 .vimrc
 create mode 100644 contrib/vim/plugin/gitvimrc.vim
 create mode 100644 contrib/vim/syntax/sharness.vim

-- 
2.29.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09  6:55 [PATCH v2 0/2] vim: configuration and sharness syntax Felipe Contreras
@ 2020-12-09  6:55 ` Felipe Contreras
  2020-12-09  8:53   ` Christian Brabandt
                     ` (2 more replies)
  2020-12-09  6:55 ` [PATCH v2 2/2] contrib: vim: add sharness syntax file Felipe Contreras
  2020-12-09 17:11 ` [PATCH v2 0/2] vim: configuration and sharness syntax Jeff King
  2 siblings, 3 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-09  6:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder, Felipe Contreras

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

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()
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 2/2] contrib: vim: add sharness syntax file
  2020-12-09  6:55 [PATCH v2 0/2] vim: configuration and sharness syntax Felipe Contreras
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
@ 2020-12-09  6:55 ` Felipe Contreras
  2020-12-09  7:05   ` Eric Sunshine
  2020-12-09 17:11 ` [PATCH v2 0/2] vim: configuration and sharness syntax Jeff King
  2 siblings, 1 reply; 22+ messages in thread
From: Felipe Contreras @ 2020-12-09  6:55 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder, Felipe Contreras

It gets a bit tedious to see all the tests in the same color, so I
wrote a vim syntax file to relax my eyes.

I've tried to make it work in as many situations as possible, yet there
are still some issues with HEREDOC strings.

Much better than nothing though.

This can be enabled with the following pattern:

  au BufRead,BufNewFile */t/*.sh set filetype=sh.sharness

Whoever, that's already added to the project .vimrc.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 .vimrc                          |  1 +
 contrib/vim/syntax/sharness.vim | 34 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 contrib/vim/syntax/sharness.vim

diff --git a/.vimrc b/.vimrc
index 602c746477..31600aaeca 100644
--- a/.vimrc
+++ b/.vimrc
@@ -11,6 +11,7 @@
 "
 
 augroup git
+	au BufRead,BufNewFile */t/*.sh set filetype=sh.sharness
 	au BufRead,BufNewFile */Documentation/*.txt set filetype=asciidoc
 
 	au FileType c setl noexpandtab tabstop=8 shiftwidth=0 cino=(s,:0,l1,t0
diff --git a/contrib/vim/syntax/sharness.vim b/contrib/vim/syntax/sharness.vim
new file mode 100644
index 0000000000..6ffc64ff06
--- /dev/null
+++ b/contrib/vim/syntax/sharness.vim
@@ -0,0 +1,34 @@
+let b:is_bash=1
+runtime! syntax/sh.vim
+
+syn keyword shsStatement test_done
+syn keyword shsStatement test_set_editor test_set_index_version test_decode_color lf_to_nul nul_to_q q_to_nul q_to_cr q_to_tab qz_to_tab_space append_cr remove_cr generate_zero_bytes sane_unset test_tick test_pause debug test_commit test_merge test_commit_bulk test_chmod test_modebits test_unconfig test_config test_config_global write_script test_unset_prereq test_set_prereq test_have_prereq test_declared_prereq test_verify_prereq test_external test_external_without_stderr test_path_is_file test_path_is_dir test_path_exists test_dir_is_empty test_file_not_empty test_path_is_missing test_line_count test_file_size list_contains test_must_fail_acceptable test_must_fail test_might_fail test_expect_code test_i18ncmp test_i18ngrep verbose test_must_be_empty test_cmp_rev test_cmp_fspath test_seq test_when_finished test_atexit test_create_repo test_ln_s_add test_write_lines perl test_bool_env test_skip_or_die mingw_test_cmp test_env test_match_signal test_copy_bytes nongit depacketize hex2oct test_set_hash test_detect_hash test_oid_init test_oid_cache test_oid test_oid_to_path test_set_port test_bitmap_traversal test_path_is_hidden test_subcommand
+syn keyword shsStatement test_cmp test_cmp_config test_cmp_bin packetize
+
+syn region shsTest fold start="\<test_expect_\w\+\>" end="$" contains=shsTestTitle
+syn region shsTest fold start="\<test_expect_\w\+\>\s\+\<[A-Z_,]\+\>" end="$" contains=shsPrereq
+syn region shsTest fold start="\<test_lazy_prereq\>\s\+\<[A-Z_,]\+\>" end="$" contains=shsPrereqLazy
+
+syn keyword shsTestStatement contained containedin=shsTest test_expect_success test_expect_failure test_expect_unstable test_lazy_prereq
+
+syn region shsTestTitle contained start=' 'hs=s+1 end=' 'me=e-1 nextgroup=shsTestBody contains=shSingleQuote,shDoubleQuote
+
+" multiple line body
+syn region shsTestBody contained transparent excludenl matchgroup=shQuote start=+ '$+hs=s+1,rs=e end=+'$+ contains=@shSubShList
+syn region shsTestBody contained transparent excludenl matchgroup=shQuote start=+ "$+hs=s+1,rs=e end=+"$+ contains=@shSubShList
+
+" single line body
+syn region shsTestBody contained oneline transparent excludenl keepend matchgroup=shQuote start=+ '+hs=s+1 end=+'$+ contains=@shSubShList
+syn region shsTestBody contained oneline transparent excludenl keepend matchgroup=shQuote start=+ "+hs=s+1 end=+"$+ contains=@shSubShList
+
+syn match shsPrereq contained "\<[A-Z_,]\+\>" nextgroup=shsTestTitle
+syn match shsPrereqLazy contained "\<[A-Z_,]\+\>" nextgroup=shsTestBody
+
+syn cluster shCommandSubList add=shsTest,shsStatement
+
+hi def link shsStatement Statement
+hi def link shsTestStatement Function
+hi def link shsPrereq Identifier
+hi def link shsPrereqLazy shsPrereq
+
+let b:current_syntax='sharness'
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 2/2] contrib: vim: add sharness syntax file
  2020-12-09  6:55 ` [PATCH v2 2/2] contrib: vim: add sharness syntax file Felipe Contreras
@ 2020-12-09  7:05   ` Eric Sunshine
  2020-12-09 10:39     ` Felipe Contreras
  0 siblings, 1 reply; 22+ messages in thread
From: Eric Sunshine @ 2020-12-09  7:05 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Git List, Junio C Hamano, Jeff King, Emily Shaffer,
	Brian M. Carlson, Aaron Schrab, Denton Liu, Christian Couder

On Wed, Dec 9, 2020 at 1:56 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> It gets a bit tedious to see all the tests in the same color, so I
> wrote a vim syntax file to relax my eyes.
>
> I've tried to make it work in as many situations as possible, yet there
> are still some issues with HEREDOC strings.
>
> Much better than nothing though.
>
> This can be enabled with the following pattern:
>
>   au BufRead,BufNewFile */t/*.sh set filetype=sh.sharness
>
> Whoever, that's already added to the project .vimrc.

s/Whoever/However/

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
@ 2020-12-09  8:53   ` Christian Brabandt
  2020-12-09 10:29     ` Felipe Contreras
  2020-12-09 17:27   ` Jeff King
  2020-12-10  3:50   ` brian m. carlson
  2 siblings, 1 reply; 22+ messages in thread
From: Christian Brabandt @ 2020-12-09  8:53 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Jeff King, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09  8:53   ` Christian Brabandt
@ 2020-12-09 10:29     ` Felipe Contreras
  2020-12-09 10:45       ` Christian Brabandt
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Contreras @ 2020-12-09 10:29 UTC (permalink / raw)
  To: Felipe Contreras, Git, Junio C Hamano, Jeff King, Emily Shaffer,
	Brian M. Carlson, Aaron Schrab, Denton Liu, Christian Couder

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.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 2/2] contrib: vim: add sharness syntax file
  2020-12-09  7:05   ` Eric Sunshine
@ 2020-12-09 10:39     ` Felipe Contreras
  0 siblings, 0 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-09 10:39 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Git List, Junio C Hamano, Jeff King, Emily Shaffer,
	Brian M. Carlson, Aaron Schrab, Denton Liu, Christian Couder

On Wed, Dec 9, 2020 at 1:05 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Dec 9, 2020 at 1:56 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > It gets a bit tedious to see all the tests in the same color, so I
> > wrote a vim syntax file to relax my eyes.
> >
> > I've tried to make it work in as many situations as possible, yet there
> > are still some issues with HEREDOC strings.
> >
> > Much better than nothing though.
> >
> > This can be enabled with the following pattern:
> >
> >   au BufRead,BufNewFile */t/*.sh set filetype=sh.sharness
> >
> > Whoever, that's already added to the project .vimrc.
>
> s/Whoever/However/

Oops. Thanks.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09 10:29     ` Felipe Contreras
@ 2020-12-09 10:45       ` Christian Brabandt
  0 siblings, 0 replies; 22+ messages in thread
From: Christian Brabandt @ 2020-12-09 10:45 UTC (permalink / raw)
  To: Felipe Contreras


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
-- 
Achte auf Deine Gedanken! Sie sind der Anfang Deiner Taten.
		-- Chinesisches Sprichwort

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/2] vim: configuration and sharness syntax
  2020-12-09  6:55 [PATCH v2 0/2] vim: configuration and sharness syntax Felipe Contreras
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
  2020-12-09  6:55 ` [PATCH v2 2/2] contrib: vim: add sharness syntax file Felipe Contreras
@ 2020-12-09 17:11 ` Jeff King
  2020-12-10  3:25   ` Felipe Contreras
  2 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2020-12-09 17:11 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

On Wed, Dec 09, 2020 at 12:55:35AM -0600, Felipe Contreras wrote:

> After investigating alternatives for exrc I found too many, doing a wide
> range of irrelevant stuff, many unmaintained, others requiring multiple
> dependencies, and some loaded the configuration too late.

I'm not opposed to this solution, but I probably wouldn't use it myself.
I wonder if it would be sufficient to just say "here are some sensible
vim options", coupled with human-readable instructions for how to
integrate them into your .vimrc, along with some path-selection.

It's perhaps not quite as turnkey. On the other hand, it's easy for
people who are even moderate vim users to understand what each line
does. In the plugin solution, there are more lines dedicated to loading
the config than there are actual config lines.

I dunno.

> And since I already created some files in 'contrib/vim' I decided to put
> the sharness syntax file there too.

This part I like very much. The actual policy logic is sufficiently
complex that I hope people will be able to contribute back small fixes.

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
  2020-12-09  8:53   ` Christian Brabandt
@ 2020-12-09 17:27   ` Jeff King
  2020-12-10  1:55     ` Felipe Contreras
  2020-12-10  3:50   ` brian m. carlson
  2 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2020-12-09 17:27 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09 17:27   ` Jeff King
@ 2020-12-10  1:55     ` Felipe Contreras
  2020-12-10 15:27       ` Jeff King
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Contreras @ 2020-12-10  1:55 UTC (permalink / raw)
  To: Jeff King
  Cc: Git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

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

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/2] vim: configuration and sharness syntax
  2020-12-09 17:11 ` [PATCH v2 0/2] vim: configuration and sharness syntax Jeff King
@ 2020-12-10  3:25   ` Felipe Contreras
  0 siblings, 0 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-10  3:25 UTC (permalink / raw)
  To: Jeff King
  Cc: Git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

On Wed, Dec 9, 2020 at 11:11 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Dec 09, 2020 at 12:55:35AM -0600, Felipe Contreras wrote:
>
> > After investigating alternatives for exrc I found too many, doing a wide
> > range of irrelevant stuff, many unmaintained, others requiring multiple
> > dependencies, and some loaded the configuration too late.
>
> I'm not opposed to this solution, but I probably wouldn't use it myself.
> I wonder if it would be sufficient to just say "here are some sensible
> vim options", coupled with human-readable instructions for how to
> integrate them into your .vimrc, along with some path-selection.
>
> It's perhaps not quite as turnkey. On the other hand, it's easy for
> people who are even moderate vim users to understand what each line
> does. In the plugin solution, there are more lines dedicated to loading
> the config than there are actual config lines.

If they only code for Git, it's straightforward to tell them how to
configure vim.

But if the user contributes to two projects with two different
code-styles it gets to get tricky to tell them what to do. And when
you get to three, my bet is that the vast majority of people wouldn't
know what's the best solution for the user.

This is the most non-intrusive solution.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
  2020-12-09  8:53   ` Christian Brabandt
  2020-12-09 17:27   ` Jeff King
@ 2020-12-10  3:50   ` brian m. carlson
  2020-12-11  1:08     ` Felipe Contreras
  2 siblings, 1 reply; 22+ messages in thread
From: brian m. carlson @ 2020-12-10  3:50 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Jeff King, Emily Shaffer, Aaron Schrab,
	Denton Liu, Christian Couder

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-10  1:55     ` Felipe Contreras
@ 2020-12-10 15:27       ` Jeff King
  2020-12-11  0:43         ` Felipe Contreras
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2020-12-10 15:27 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-10 15:27       ` Jeff King
@ 2020-12-11  0:43         ` Felipe Contreras
  0 siblings, 0 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-11  0:43 UTC (permalink / raw)
  To: Jeff King
  Cc: Git, Junio C Hamano, Emily Shaffer, Brian M. Carlson,
	Aaron Schrab, Denton Liu, Christian Couder

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.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-10  3:50   ` brian m. carlson
@ 2020-12-11  1:08     ` Felipe Contreras
  2020-12-11  2:56       ` brian m. carlson
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Contreras @ 2020-12-11  1:08 UTC (permalink / raw)
  To: brian m. carlson, Felipe Contreras, Git, Junio C Hamano,
	Jeff King, Emily Shaffer, Aaron Schrab, Denton Liu,
	Christian Couder

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

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-11  1:08     ` Felipe Contreras
@ 2020-12-11  2:56       ` brian m. carlson
  2020-12-11  4:37         ` Felipe Contreras
  2020-12-15  1:39         ` Jeff King
  0 siblings, 2 replies; 22+ messages in thread
From: brian m. carlson @ 2020-12-11  2:56 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: Git, Junio C Hamano, Jeff King, Emily Shaffer, Aaron Schrab,
	Denton Liu, Christian Couder

[-- Attachment #1: Type: text/plain, Size: 7237 bytes --]

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
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-11  2:56       ` brian m. carlson
@ 2020-12-11  4:37         ` Felipe Contreras
  2020-12-15  1:39         ` Jeff King
  1 sibling, 0 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-11  4:37 UTC (permalink / raw)
  To: brian m. carlson, Felipe Contreras, Git, Junio C Hamano,
	Jeff King, Emily Shaffer, Aaron Schrab, Denton Liu,
	Christian Couder

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.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-11  2:56       ` brian m. carlson
  2020-12-11  4:37         ` Felipe Contreras
@ 2020-12-15  1:39         ` Jeff King
  2020-12-15  3:03           ` Felipe Contreras
  1 sibling, 1 reply; 22+ messages in thread
From: Jeff King @ 2020-12-15  1:39 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Felipe Contreras, Git, Junio C Hamano, Emily Shaffer,
	Aaron Schrab, Denton Liu, Christian Couder

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-15  1:39         ` Jeff King
@ 2020-12-15  3:03           ` Felipe Contreras
  2020-12-15  5:28             ` Jeff King
  0 siblings, 1 reply; 22+ messages in thread
From: Felipe Contreras @ 2020-12-15  3:03 UTC (permalink / raw)
  To: Jeff King, brian m. carlson
  Cc: Felipe Contreras, Git, Junio C Hamano, Emily Shaffer,
	Aaron Schrab, Denton Liu, Christian Couder

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.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-15  3:03           ` Felipe Contreras
@ 2020-12-15  5:28             ` Jeff King
  2020-12-15  6:56               ` Felipe Contreras
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2020-12-15  5:28 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: brian m. carlson, Git, Junio C Hamano, Emily Shaffer,
	Aaron Schrab, Denton Liu, Christian Couder

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/2] Add project-wide .vimrc configuration
  2020-12-15  5:28             ` Jeff King
@ 2020-12-15  6:56               ` Felipe Contreras
  0 siblings, 0 replies; 22+ messages in thread
From: Felipe Contreras @ 2020-12-15  6:56 UTC (permalink / raw)
  To: Jeff King, Felipe Contreras
  Cc: brian m. carlson, Git, Junio C Hamano, Emily Shaffer,
	Aaron Schrab, Denton Liu, Christian Couder

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.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-12-15  6:58 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09  6:55 [PATCH v2 0/2] vim: configuration and sharness syntax Felipe Contreras
2020-12-09  6:55 ` [PATCH v2 1/2] Add project-wide .vimrc configuration Felipe Contreras
2020-12-09  8:53   ` Christian Brabandt
2020-12-09 10:29     ` Felipe Contreras
2020-12-09 10:45       ` Christian Brabandt
2020-12-09 17:27   ` Jeff King
2020-12-10  1:55     ` Felipe Contreras
2020-12-10 15:27       ` Jeff King
2020-12-11  0:43         ` Felipe Contreras
2020-12-10  3:50   ` brian m. carlson
2020-12-11  1:08     ` Felipe Contreras
2020-12-11  2:56       ` brian m. carlson
2020-12-11  4:37         ` Felipe Contreras
2020-12-15  1:39         ` Jeff King
2020-12-15  3:03           ` Felipe Contreras
2020-12-15  5:28             ` Jeff King
2020-12-15  6:56               ` Felipe Contreras
2020-12-09  6:55 ` [PATCH v2 2/2] contrib: vim: add sharness syntax file Felipe Contreras
2020-12-09  7:05   ` Eric Sunshine
2020-12-09 10:39     ` Felipe Contreras
2020-12-09 17:11 ` [PATCH v2 0/2] vim: configuration and sharness syntax Jeff King
2020-12-10  3:25   ` Felipe Contreras

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).