Patch 8.2.3743

[Bram Moolenaar]

Patch 8.2.3743
Problem: ":sign" can add a highlight group without a name.
Solution: Give an error if the group name is missing. (closes #9280)
Files: src/sign.c, src/errors.h, src/testdir/test_signs.vim


*** ../vim-8.2.3742/src/sign.c 2021-11-24 16:19:41.389010087 +0000
--- src/sign.c 2021-12-05 12:56:21.113470167 +0000
***************
*** 1295,1300 ****
--- 1295,1311 ----
return lnum;
}

+ static int
+ check_empty_group(size_t len, char *name)
+ {
+ if (len == 0)
+ {
+ semsg(_(e_group_name_missing_for_str), name);
+ return FAIL;
+ }
+ return OK;
+ }
+
/*
* ":sign define {name} ..." command
*/
***************
*** 1330,1345 ****
--- 1341,1371 ----
else if (STRNCMP(arg, "linehl=", 7) == 0)
{
arg += 7;
+ if (check_empty_group(p - arg, "linehl") == FAIL)
+ {
+ failed = TRUE;
+ break;
+ }
linehl = vim_strnsave(arg, p - arg);
}
else if (STRNCMP(arg, "texthl=", 7) == 0)
{
arg += 7;
+ if (check_empty_group(p - arg, "texthl") == FAIL)
+ {
+ failed = TRUE;
+ break;
+ }
texthl = vim_strnsave(arg, p - arg);
}
else if (STRNCMP(arg, "culhl=", 6) == 0)
{
arg += 6;
+ if (check_empty_group(p - arg, "culhl") == FAIL)
+ {
+ failed = TRUE;
+ break;
+ }
culhl = vim_strnsave(arg, p - arg);
}
else
*** ../vim-8.2.3742/src/errors.h 2021-12-02 20:44:38.703106821 +0000
--- src/errors.h 2021-12-05 12:52:17.025802456 +0000
***************
*** 694,696 ****
--- 694,698 ----
INIT(= N_("E1247: Line number out of range"));
EXTERN char e_closure_called_from_invalid_context[]
INIT(= N_("E1248: Closure called from invalid context"));
+ EXTERN char e_group_name_missing_for_str[]
+ INIT(= N_("E1249: Group name missing for %s"));
*** ../vim-8.2.3742/src/testdir/test_signs.vim 2021-11-24 16:19:41.393010087 +0000
--- src/testdir/test_signs.vim 2021-12-05 13:02:14.616984049 +0000
***************
*** 126,131 ****
--- 126,135 ----
call assert_fails("sign define Sign4 text= linehl=Comment", 'E239:')
call assert_fails("sign define Sign4 text=\\ ab linehl=Comment", 'E239:')

+ call assert_fails("sign define Sign4 linehl=", 'E1249: Group name missing for linehl')
+ call assert_fails("sign define Sign4 culhl=", 'E1249: Group name missing for culhl')
+ call assert_fails("sign define Sign4 texthl=", 'E1249: Group name missing for texthl')
+
" define sign with whitespace
sign define Sign4 text=\ X linehl=Comment
sign undefine Sign4
*** ../vim-8.2.3742/src/version.c 2021-12-05 12:39:15.098817937 +0000
--- src/version.c 2021-12-05 12:54:51.993591909 +0000
***************
*** 755,756 ****
--- 755,758 ----
{ /* Add new patch number below this line */
+ /**/
+ 3743,
/**/

--
While it's true that many normal people whould prefer not to _date_ an
engineer, most normal people harbor an intense desire to _mate_ with them,
thus producing engineerlike children who will have high-paying jobs long
before losing their virginity.
(Scott Adams - The Dilbert principle)

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[James McCoy]

On Sun, Dec 05, 2021 at 01:03:15PM +0000, Bram Moolenaar wrote:
>
> Patch 8.2.3743
> Problem: ":sign" can add a highlight group without a name.
> Solution: Give an error if the group name is missing. (closes #9280)
> Files: src/sign.c, src/errors.h, src/testdir/test_signs.vim

This removes a useful capability. If I've defined a sign as such:

sign define Foo linehl=FooLine texthl=FooText

and then want to remove the linehl, I could run

sign define foo linehl= texthl=FooText

Now, I have to instead undefine the sign and then define it again.

This patch causes plugins, like Signify, to error now.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB

[Bram Moolenaar]

James McCoy wrote:

> On Sun, Dec 05, 2021 at 01:03:15PM +0000, Bram Moolenaar wrote:
> >
> > Patch 8.2.3743
> > Problem: ":sign" can add a highlight group without a name.
> > Solution: Give an error if the group name is missing. (closes #9280)
> > Files: src/sign.c, src/errors.h, src/testdir/test_signs.vim
>
> This removes a useful capability. If I've defined a sign as such:
>
> sign define Foo linehl=FooLine texthl=FooText
>
> and then want to remove the linehl, I could run
>
> sign define foo linehl= texthl=FooText
>
> Now, I have to instead undefine the sign and then define it again.
>
> This patch causes plugins, like Signify, to error now.

I see. Removing the highlight for a defined sign should be OK.
But defining a sign the first time with an empty highlight should still
be an error, right?

--
Every engineer dreams about saving the universe and having sex with aliens.
This is much more glamorous than the real life of an engineer, which consists
of hiding from the universe and having sex without the participation of other
life forms. (Scott Adams - The Dilbert principle)

[James McCoy]

On Sun, Dec 05, 2021 at 05:43:17PM +0000, Bram Moolenaar wrote:
>
> James McCoy wrote:
>
> > On Sun, Dec 05, 2021 at 01:03:15PM +0000, Bram Moolenaar wrote:
> > >
> > > Patch 8.2.3743
> > > Problem: ":sign" can add a highlight group without a name.
> > > Solution: Give an error if the group name is missing. (closes #9280)
> > > Files: src/sign.c, src/errors.h, src/testdir/test_signs.vim
> >
> > This removes a useful capability. If I've defined a sign as such:
> >
> > sign define Foo linehl=FooLine texthl=FooText
> >
> > and then want to remove the linehl, I could run
> >
> > sign define foo linehl= texthl=FooText
> >
> > Now, I have to instead undefine the sign and then define it again.
> >
> > This patch causes plugins, like Signify, to error now.
>
> I see. Removing the highlight for a defined sign should be OK.
> But defining a sign the first time with an empty highlight should still
> be an error, right?

Why? That complicates the plugin code the same way as dealing with this
patch already would. Whether a certain attribute is being defined or
not by the plugin can change during runtime.

With this patch, any plugin doing that now needs to either always
undefine signs before placing them or track whether attributes are set
and adjust their calls accordingly.

[Bram Moolenaar]

James McCoy wrote:

> > > On Sun, Dec 05, 2021 at 01:03:15PM +0000, Bram Moolenaar wrote:
> > > >
> > > > Patch 8.2.3743
> > > > Problem: ":sign" can add a highlight group without a name.
> > > > Solution: Give an error if the group name is missing. (closes #9280)
> > > > Files: src/sign.c, src/errors.h, src/testdir/test_signs.vim
> > >
> > > This removes a useful capability. If I've defined a sign as such:
> > >
> > > sign define Foo linehl=FooLine texthl=FooText
> > >
> > > and then want to remove the linehl, I could run
> > >
> > > sign define foo linehl= texthl=FooText
> > >
> > > Now, I have to instead undefine the sign and then define it again.
> > >
> > > This patch causes plugins, like Signify, to error now.
> >
> > I see. Removing the highlight for a defined sign should be OK.
> > But defining a sign the first time with an empty highlight should still
> > be an error, right?
>
> Why? That complicates the plugin code the same way as dealing with this
> patch already would. Whether a certain attribute is being defined or
> not by the plugin can change during runtime.
>
> With this patch, any plugin doing that now needs to either always
> undefine signs before placing them or track whether attributes are set
> and adjust their calls accordingly.

Can you give an example of how an empty highlight argument can be
useful?

--
EXPERIENCE - experience is a wonderful thing. It enables you to
recognise a mistake when you make it again.

[James McCoy]

On Sun, Dec 05, 2021 at 06:38:39PM +0000, Bram Moolenaar wrote:
>
> James McCoy wrote:
>

> > Why? That complicates the plugin code the same way as dealing with this
> > patch already would. Whether a certain attribute is being defined or
> > not by the plugin can change during runtime.
> >
> > With this patch, any plugin doing that now needs to either always
> > undefine signs before placing them or track whether attributes are set
> > and adjust their calls accordingly.
>
> Can you give an example of how an empty highlight argument can be
> useful?

Right now Signify just has this:

let s:delete_highlight = ['', 'SignifyLineDelete']
...
function! s:add_sign(sy, line, type, ...) abort
...
execute printf('sign define %s text=%s texthl=SignifySignDelete linehl=%s',
\ a:type,
\ a:1,
\ s:delete_highlight[g:signify_line_highlight])

Depending on whether the user wants line highlights, a value is set for
linehl. This is simple code.

Without the latest patches, we need to handle this differently. We
either need to "silent! sign undefine" before defining signs to ensure
the attributes are reset or we need to track whether
g:signify_line_highlight changes and explicitly undefine all existing
sign definitions.

Alternatively, we raise our minimum required Vim version so we can use
sign_define() since that doesn't require specifying an empty linehl to
remove the attribute.

[Bram Moolenaar]

OK, so you use the "linehl" argument always, but depending on the
situation the argument might be empty.

I suppose accidentally using an empty argument is unlikely to happen,
thus we can just remove the error message. But keep the fix for
creating a highlight group with an empty name


--
Engineers understand that their appearance only bothers other people and
therefore it is not worth optimizing.

(Scott Adams - The Dilbert principle)