[gentoo-dev] Quoting patch for repoman

[gentoo-dev] Quoting patch for repoman

[Donnie Berkholz]

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

I put together a quick repoman patch to check for the quoting issues 
that've kept coming up in reviews lately. Give it a shot and fix it if 
you have problems.

Thanks,
Donnie

[-- Attachment #2: repoman_quoting.patch --]
[-- Type: text/plain, Size: 2153 bytes --]

--- repoman.orig	2007-09-29 00:05:30.000000000 -0700
+++ repoman	2007-09-29 01:06:09.000000000 -0700
@@ -188,6 +188,7 @@
 	"ebuild.majorsyn":"This ebuild has a major syntax error that may cause the ebuild to fail partially or fully",
 	"ebuild.minorsyn":"This ebuild has a minor syntax error that contravenes gentoo coding style",
 	"ebuild.badheader":"This ebuild has a malformed header",
+	"ebuild.quoting":"This ebuild may fail to quote D, S, T, FILESDIR, or WORKDIR",
 	"metadata.missing":"Missing metadata.xml files",
 	"metadata.bad":"Bad metadata.xml files",
 	"virtual.versioned":"PROVIDE contains virtuals with versions",
@@ -218,6 +219,7 @@
 "RESTRICT.invalid",
 "ebuild.minorsyn",
 "ebuild.badheader",
+"ebuild.quoting",
 "file.size",
 "metadata.missing",
 "metadata.bad",
@@ -1329,6 +1331,8 @@
 		ignore_line = re.compile(r'(^$)|(^(\t)*#)')
 		leading_spaces = re.compile(r'^[\S\t]')
 		trailing_whitespace = re.compile(r'.*([\S]$)')
+		missing_quotes = re.compile(r'[^"]\${?(D|S|T|FILESDIR|WORKDIR)}?\W')
+		missing_quotes_exclude = re.compile(r'\[\[.*[^"]\${?(D|S|T|FILESDIR|WORKDIR)}?\W.*]/]/')
 		readonly_assignment = re.compile(r'^\s*(export\s+)?(A|CATEGORY|P|PV|PN|PR|PVR|PF|D|WORKDIR|FILESDIR|FEATURES|USE)=')
 		line_continuation = re.compile(r'([^#]*\S)(\s+|\t)\\$')
 		linenum=0
@@ -1374,6 +1378,17 @@
 						myerrormsg = "Trailing whitespace Syntax Error. Line %d" % linenum
 						stats["ebuild.minorsyn"] +=1
 						fails["ebuild.minorsyn"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+					# Missing quotes check
+					missing_quotes_line = missing_quotes.search(line)
+					if missing_quotes_line:
+						for group in missing_quotes_line.group():
+							match = missing_quotes_exclude.search(group)
+							if not match:
+								myerrormsg = "Missing Quotes Error. Line %d" % linenum
+								stats["ebuild.quoting"] += 1
+								fails["ebuild.quoting"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+								# Only need 1 match in a line to count the line
+								break
 					# Readonly variable assignment check
 					match = readonly_assignment.match(line)
 					# The regex can give a false positive for continued lines,

Re: [gentoo-dev] Quoting patch for repoman

[Donnie Berkholz]

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

On 01:09 Sat 29 Sep     , Donnie Berkholz wrote:
> I put together a quick repoman patch to check for the quoting issues 
> that've kept coming up in reviews lately. Give it a shot and fix it if 
> you have problems.
> 
> Thanks,
> Donnie

Woops, the last one didn't exclude [[ ]] cases properly because I 
screwed up the second regexp. Try this.

Thanks,
Donnie

[-- Attachment #2: repoman_quoting.patch --]
[-- Type: text/plain, Size: 2153 bytes --]

--- repoman.orig	2007-09-29 00:05:30.000000000 -0700
+++ repoman	2007-09-29 01:33:27.000000000 -0700
@@ -188,6 +188,7 @@
 	"ebuild.majorsyn":"This ebuild has a major syntax error that may cause the ebuild to fail partially or fully",
 	"ebuild.minorsyn":"This ebuild has a minor syntax error that contravenes gentoo coding style",
 	"ebuild.badheader":"This ebuild has a malformed header",
+	"ebuild.quoting":"This ebuild may fail to quote D, S, T, FILESDIR, or WORKDIR",
 	"metadata.missing":"Missing metadata.xml files",
 	"metadata.bad":"Bad metadata.xml files",
 	"virtual.versioned":"PROVIDE contains virtuals with versions",
@@ -218,6 +219,7 @@
 "RESTRICT.invalid",
 "ebuild.minorsyn",
 "ebuild.badheader",
+"ebuild.quoting",
 "file.size",
 "metadata.missing",
 "metadata.bad",
@@ -1329,6 +1331,8 @@
 		ignore_line = re.compile(r'(^$)|(^(\t)*#)')
 		leading_spaces = re.compile(r'^[\S\t]')
 		trailing_whitespace = re.compile(r'.*([\S]$)')
+		missing_quotes = re.compile(r'[^"]\${?(D|S|T|FILESDIR|WORKDIR)}?\W')
+		missing_quotes_exclude = re.compile(r'\[\[.*[^"]\${?(D|S|T|FILESDIR|WORKDIR)}?\W.*\]\]')
 		readonly_assignment = re.compile(r'^\s*(export\s+)?(A|CATEGORY|P|PV|PN|PR|PVR|PF|D|WORKDIR|FILESDIR|FEATURES|USE)=')
 		line_continuation = re.compile(r'([^#]*\S)(\s+|\t)\\$')
 		linenum=0
@@ -1374,6 +1378,17 @@
 						myerrormsg = "Trailing whitespace Syntax Error. Line %d" % linenum
 						stats["ebuild.minorsyn"] +=1
 						fails["ebuild.minorsyn"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+					# Missing quotes check
+					missing_quotes_line = missing_quotes.search(line)
+					if missing_quotes_line:
+						for group in missing_quotes_line.group():
+							match = missing_quotes_exclude.search(group)
+							if not match:
+								myerrormsg = "Missing Quotes Error. Line %d" % linenum
+								stats["ebuild.quoting"] += 1
+								fails["ebuild.quoting"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+								# Only need 1 match in a line to count the line
+								break
 					# Readonly variable assignment check
 					match = readonly_assignment.match(line)
 					# The regex can give a false positive for continued lines,

Re: [gentoo-dev] Quoting patch for repoman

[Donnie Berkholz]

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

On 01:33 Sat 29 Sep     , Donnie Berkholz wrote:

Take 3. Adds ROOT to variables checked.

Thanks,
Donnie

[-- Attachment #2: repoman_quoting.patch --]
[-- Type: text/plain, Size: 2169 bytes --]

--- repoman.orig	2007-09-29 00:05:30.000000000 -0700
+++ repoman	2007-09-29 01:33:27.000000000 -0700
@@ -188,6 +188,7 @@
 	"ebuild.majorsyn":"This ebuild has a major syntax error that may cause the ebuild to fail partially or fully",
 	"ebuild.minorsyn":"This ebuild has a minor syntax error that contravenes gentoo coding style",
 	"ebuild.badheader":"This ebuild has a malformed header",
+	"ebuild.quoting":"This ebuild may fail to quote D, S, T, ROOT, FILESDIR, or WORKDIR",
 	"metadata.missing":"Missing metadata.xml files",
 	"metadata.bad":"Bad metadata.xml files",
 	"virtual.versioned":"PROVIDE contains virtuals with versions",
@@ -218,6 +219,7 @@
 "RESTRICT.invalid",
 "ebuild.minorsyn",
 "ebuild.badheader",
+"ebuild.quoting",
 "file.size",
 "metadata.missing",
 "metadata.bad",
@@ -1329,6 +1331,8 @@
 		ignore_line = re.compile(r'(^$)|(^(\t)*#)')
 		leading_spaces = re.compile(r'^[\S\t]')
 		trailing_whitespace = re.compile(r'.*([\S]$)')
+		missing_quotes = re.compile(r'[^"]\${?(D|S|T|ROOT|FILESDIR|WORKDIR)}?\W')
+		missing_quotes_exclude = re.compile(r'\[\[.*[^"]\${?(D|S|T|ROOT|FILESDIR|WORKDIR)}?\W.*\]\]')
 		readonly_assignment = re.compile(r'^\s*(export\s+)?(A|CATEGORY|P|PV|PN|PR|PVR|PF|D|WORKDIR|FILESDIR|FEATURES|USE)=')
 		line_continuation = re.compile(r'([^#]*\S)(\s+|\t)\\$')
 		linenum=0
@@ -1374,6 +1378,17 @@
 						myerrormsg = "Trailing whitespace Syntax Error. Line %d" % linenum
 						stats["ebuild.minorsyn"] +=1
 						fails["ebuild.minorsyn"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+					# Missing quotes check
+					missing_quotes_line = missing_quotes.search(line)
+					if missing_quotes_line:
+						for group in missing_quotes_line.group():
+							match = missing_quotes_exclude.search(group)
+							if not match:
+								myerrormsg = "Missing Quotes Error. Line %d" % linenum
+								stats["ebuild.quoting"] += 1
+								fails["ebuild.quoting"].append(x+"/"+y+".ebuild: %s" % myerrormsg)
+								# Only need 1 match in a line to count the line
+								break
 					# Readonly variable assignment check
 					match = readonly_assignment.match(line)
 					# The regex can give a false positive for continued lines,

Re: [gentoo-dev] Quoting patch for repoman

[Mike Frysinger]

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

On Saturday 29 September 2007, Donnie Berkholz wrote:
> On 01:09 Sat 29 Sep     , Donnie Berkholz wrote:
> > I put together a quick repoman patch to check for the quoting issues
> > that've kept coming up in reviews lately. Give it a shot and fix it if
> > you have problems.
>
> Woops, the last one didn't exclude [[ ]] cases properly because I
> screwed up the second regexp. Try this.

does this work with multilines ?
if [[ -e ${S}/asdfasdfasdf && \
   -f ${WORKDIR}/moo ]]
then

this is a crappy example, but entirely correct when the if statement gets real 
long ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [gentoo-dev] Quoting patch for repoman

[Donnie Berkholz]

On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
> does this work with multilines ?
> if [[ -e ${S}/asdfasdfasdf && \
>    -f ${WORKDIR}/moo ]]
> then
> 
> this is a crappy example, but entirely correct when the if statement 
> gets real long ...

No, it doesn't. That's why I just made it a warning instead of a 
failure, because it's not always going to work.

Thanks,
Donnie
-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] Quoting patch for repoman

[Zac Medico]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Donnie Berkholz wrote:
> On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
>> does this work with multilines ?
>> if [[ -e ${S}/asdfasdfasdf && \
>>    -f ${WORKDIR}/moo ]]
>> then
>>
>> this is a crappy example, but entirely correct when the if statement 
>> gets real long ...
> 
> No, it doesn't. That's why I just made it a warning instead of a 
> failure, because it's not always going to work.

If there aren't many false positives then we don't have to reduce it
to a warning since they can use the new repoman --force option to
force the commit. If there are many false positives then I think we
should try to filter those out if possible so that we don't have to
reduce it to a warning and spam people with bogus warning messages.

Zac
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)

iD8DBQFG/+pF/ejvha5XGaMRAkQPAKDlh6fVaaj/z2nKROrcYlk8OHvEMACfYRCG
UF8OV9mgTbSxpN7jUF7oef0=
=EI6W
-----END PGP SIGNATURE-----
-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] Quoting patch for repoman

[Mike Frysinger]

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

On Sunday 30 September 2007, Zac Medico wrote:
> Donnie Berkholz wrote:
> > On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
> >> does this work with multilines ?
> >> if [[ -e ${S}/asdfasdfasdf && \
> >>    -f ${WORKDIR}/moo ]]
> >> then
> >>
> >> this is a crappy example, but entirely correct when the if statement
> >> gets real long ...
> >
> > No, it doesn't. That's why I just made it a warning instead of a
> > failure, because it's not always going to work.
>
> If there aren't many false positives then we don't have to reduce it
> to a warning since they can use the new repoman --force option to
> force the commit. If there are many false positives then I think we
> should try to filter those out if possible so that we don't have to
> reduce it to a warning and spam people with bogus warning messages.

i see --force as something you should use in order to get around semi-serious 
(but there's a good reason for it) ... not something that people should have 
to use to get around perfectly legit code ...

maybe a new function in repoman that would eat a line as the shell defines it 
(in other words, sucks in all line continuations)
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [gentoo-dev] Quoting patch for repoman

[Alec Warner]

On 9/30/07, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sunday 30 September 2007, Zac Medico wrote:
> > Donnie Berkholz wrote:
> > > On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
> > >> does this work with multilines ?
> > >> if [[ -e ${S}/asdfasdfasdf && \
> > >>    -f ${WORKDIR}/moo ]]
> > >> then
> > >>
> > >> this is a crappy example, but entirely correct when the if statement
> > >> gets real long ...
> > >
> > > No, it doesn't. That's why I just made it a warning instead of a
> > > failure, because it's not always going to work.
> >
> > If there aren't many false positives then we don't have to reduce it
> > to a warning since they can use the new repoman --force option to
> > force the commit. If there are many false positives then I think we
> > should try to filter those out if possible so that we don't have to
> > reduce it to a warning and spam people with bogus warning messages.
>
> i see --force as something you should use in order to get around semi-serious
> (but there's a good reason for it) ... not something that people should have
> to use to get around perfectly legit code ...
>
> maybe a new function in repoman that would eat a line as the shell defines it
> (in other words, sucks in all line continuations)
> -mike

import shlex.... ;)

I've ripped out all the ebuild content checks into seperate classes
for repoman, I just need to rewrite the warn[] fail[] stuff to work
right.

-Alec
-- 
gentoo-dev@gentoo.org mailing list

[gentoo-dev] Re: Quoting patch for repoman

[Steve Long]

Mike Frysinger wrote:

> On Sunday 30 September 2007, Zac Medico wrote:
>> Donnie Berkholz wrote:
>> > On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
>> >> does this work with multilines ?
>> >> if [[ -e ${S}/asdfasdfasdf && \
>> >>    -f ${WORKDIR}/moo ]]
>> >> then
>> >>
>> >> this is a crappy example, but entirely correct when the if statement
>> >> gets real long ...
>> >
>> > No, it doesn't. That's why I just made it a warning instead of a
>> > failure, because it's not always going to work.
>>
>> If there aren't many false positives then we don't have to reduce it
>> to a warning since they can use the new repoman --force option to
>> force the commit. If there are many false positives then I think we
>> should try to filter those out if possible so that we don't have to
>> reduce it to a warning and spam people with bogus warning messages.
>
A place to start might be a list of all known variables from say the
devmanual, along with whether they're allowed to be used as
multi-parameters in `for' or function/cmd calls. So it's legitimate to see
eg: for f in $A (even if it isn't space-proofed; that'd need an array.)
Most variables like $S and $WORKDIR are 'scalar' so their expansions should
always be quoted.

By only focussing on known variables, we wouldn't be prone to false
positives with user vars (for which we can't know what expansions are
appropriate.)

> i see --force as something you should use in order to get around
> semi-serious (but there's a good reason for it) ... not something that
> people should have to use to get around perfectly legit code ...
> 
> maybe a new function in repoman that would eat a line as the shell defines
> it (in other words, sucks in all line continuations)

Is there no way to tie into the bash parser code with PyC? I don't know it
very well, but I have a feeling you do.. ;) Other than that the KATE syntax
highlighter for BASH was recently updated and is very effective. It might
be a good starting point.


-- 
gentoo-dev@gentoo.org mailing list

Re: [gentoo-dev] Re: Quoting patch for repoman

[Mike Frysinger]

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

On Monday 01 October 2007, Steve Long wrote:
> A place to start might be a list of all known variables from say the
> devmanual, along with whether they're allowed to be used as
> multi-parameters in `for' or function/cmd calls. So it's legitimate to see
> eg: for f in $A (even if it isn't space-proofed; that'd need an array.)

i dont see how this is relevant, plus i dont really understand what you're 
trying to say.  this "multi-parameters" business just doesnt make sense.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [gentoo-dev] Quoting patch for repoman

[Alec Warner]

On 9/30/07, Alec Warner <antarus@gentoo.org> wrote:
> On 9/30/07, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Sunday 30 September 2007, Zac Medico wrote:
> > > Donnie Berkholz wrote:
> > > > On 04:52 Sat 29 Sep     , Mike Frysinger wrote:
> > > >> does this work with multilines ?
> > > >> if [[ -e ${S}/asdfasdfasdf && \
> > > >>    -f ${WORKDIR}/moo ]]
> > > >> then
> > > >>
> > > >> this is a crappy example, but entirely correct when the if statement
> > > >> gets real long ...
> > > >
> > > > No, it doesn't. That's why I just made it a warning instead of a
> > > > failure, because it's not always going to work.
> > >
> > > If there aren't many false positives then we don't have to reduce it
> > > to a warning since they can use the new repoman --force option to
> > > force the commit. If there are many false positives then I think we
> > > should try to filter those out if possible so that we don't have to
> > > reduce it to a warning and spam people with bogus warning messages.
> >
> > i see --force as something you should use in order to get around semi-serious
> > (but there's a good reason for it) ... not something that people should have
> > to use to get around perfectly legit code ...
> >
> > maybe a new function in repoman that would eat a line as the shell defines it
> > (in other words, sucks in all line continuations)
> > -mike
>
> import shlex.... ;)
>
> I've ripped out all the ebuild content checks into seperate classes
> for repoman, I just need to rewrite the warn[] fail[] stuff to work
> right.
>
> -Alec
>

The new checks went into svn this morning.  I need to clean them up a
bit though (Strings vs StringIO, and maybe some shlex foo)
-- 
gentoo-dev@gentoo.org mailing list

[gentoo-dev] Re: Re: Quoting patch for repoman

[Steve Long]

Mike Frysinger wrote:

> On Monday 01 October 2007, Steve Long wrote:
>> A place to start might be a list of all known variables from say the
>> devmanual, along with whether they're allowed to be used as
>> multi-parameters in `for' or function/cmd calls. So it's legitimate to
>> see eg: for f in $A (even if it isn't space-proofed; that'd need an
>> array.)
> 
> i dont see how this is relevant, plus i dont really understand what you're
> trying to say.  this "multi-parameters" business just doesnt make sense.

Relevance is wrt automated checks for quoting, ie having a list of
variables[1] like WORKDIR and T to check for quoting issues.

Multi-parameter: according to devmanual, A is:
"All the source files for the package (excluding those which are not
available because of USE flags)."
So it's a (presumably whitespace-separated) list of values, not just a
singleton. Sorry if "multi-parameters" was a bad choice of word.

[1] http://devmanual.gentoo.org/ebuild-writing/variables/index.html


-- 
gentoo-dev@gentoo.org mailing list