2009/2/14 Gergely Imreh :

...

This patch should fix the way commented lines are removed. Should generally work with all lines (variables) AUR cares about. Removed previous (incomplete) method.

Hmm this fixes it for comments outside of braces, but it breaks the parsing for parameter substitution. Oy. _kernelname=${pkgname#kernel26}

On Mon, Feb 16, 2009 at 07:25:59AM +0800, Gergely Imreh wrote:

2009/2/16 Loui Chang :

...

...

2009/2/14 Gergely Imreh :

...

This patch should fix the way commented lines are removed. Should generally work with all lines (variables) AUR cares about. Removed previous (incomplete) method.

Hmm this fixes it for comments outside of braces, but it breaks the parsing for parameter substitution. Oy. _kernelname=${pkgname#kernel26}

Ok, let me check it, I haven't seen such replacement that you mention, yet, so didn't expect it... Let bash be blessed. :)

Oh there was no replacement, but the parser was able to handle it gracefully (?) before. Now it's showing an error: Invalid name: only lowercase letters are allowed. I was using the PKGBUILD submitted in this http://bugs.archlinux.org/task/13173 for testing.

2009/2/16 Gergely Imreh :

2009/2/16 Loui Chang :

...

On Mon, Feb 16, 2009 at 07:25:59AM +0800, Gergely Imreh wrote:

...

2009/2/16 Loui Chang :

...

...

2009/2/14 Gergely Imreh :

...

This patch should fix the way commented lines are removed. Should generally work with all lines (variables) AUR cares about. Removed previous (incomplete) method.

Hmm this fixes it for comments outside of braces, but it breaks the parsing for parameter substitution. Oy. _kernelname=${pkgname#kernel26}

Ok, let me check it, I haven't seen such replacement that you mention, yet, so didn't expect it... Let bash be blessed. :)

Oh there was no replacement, but the parser was able to handle it gracefully (?) before.

Now it's showing an error: Invalid name: only lowercase letters are allowed.

I was using the PKGBUILD submitted in this http://bugs.archlinux.org/task/13173 for testing.

Okay, I checked the PKGBUILD attached to the bug report you mention. Yeah, that # wasn't replaced, because the current code only handles #s within brackets... I know how to get this patch right, just let me check it in the evening. ;) And now that I know what I should look for, probably will give a go to implement more of bash's variable replacement, I have some ideas... Cheers, Greg

How about this new version? Handles FS#13122 and FS#13173 well - by which I mean that it does not remove anything that is not comment. The # characters intended for bash parameter replacement are kept (so not breaking the upload), though not handled yet within the replacement section. That is for another day. Greg

On Mon, Feb 16, 2009 at 10:45:33PM +0800, Gergely Imreh wrote:

How about this new version? Handles FS#13122 and FS#13173 well - by which I mean that it does not remove anything that is not comment. The # characters intended for bash parameter replacement are kept (so not breaking the upload), though not handled yet within the replacement section. That is for another day.

I pushed a modded version of your patch, so it's attributed to you. It only neutralises the parameter substitution, though.

On Tue, Feb 17, 2009 at 10:08:50AM +0800, Gergely Imreh wrote:

2009/2/17 Loui Chang :

...

On Mon, Feb 16, 2009 at 10:45:33PM +0800, Gergely Imreh wrote:

...

How about this new version? Handles FS#13122 and FS#13173 well - by which I mean that it does not remove anything that is not comment. The # characters intended for bash parameter replacement are kept (so not breaking the upload), though not handled yet within the replacement section. That is for another day.

I pushed a modded version of your patch, so it's attributed to you. It only neutralises the parameter substitution, though.

You shouldn't attribute that commit to me, because it has absolutely nothing to do with the patch I sent in, it's totally yours... I'm not even sure what "neutralises" mean in this situation, yet.... Got to figure out the changes now. :)

Hah, well you wrote the patch that it was based on, and you even wrote the code that it was patching. ;) By 'neutralises' I mean that it makes the substitution no longer a substitution, so the rest of the PKGBUILD parsing doesn't trip over it. I could have been clearer about that.

As for your previous comment about foo=bar;#none, that could be handled with my version with some change, using this regular expression replacement: $line = preg_replace('/(;*)(^|\s+)#.*/', "$1", $line); This version handles things as: #none -> (empty line) foo=bar;#none -> foo=bar; foo=bar #none -> foo=bar foo=bar #none -> foo=bar foo=${bar#none} -> foo=${bar#none} foo=${bar##none} -> foo=${bar##none} foo=${bar #none} -> foo=${bar The last one would result we handle in the parameter substitution (although wrong), but that's fine since it breaks bash, so it's a wrong PKGBUILD.

OK. You're welcome to change what I pushed if you're continuing patching the parser. I just wanted to push some quick fixes. That infinite loop bug might have been part of some real problems on the server. The changes are live, but I forgot to update the version string. Doh. No big deal though.

Is there any other case that is not handled?

I'm not really sure to be honest. I'm flying by the seat of my pants on this really. I don't think anyone's bothered to improve the parsing until now because there are so many possibilities to cover. Your work has really helped cover a nice chunk of them. If we really want to protect against surprises we'd have to read the manual on bash at least a few times. I'm alright with letting users discover the inaccuracies of the parser. Side note: It might be a good idea to split the parsing into its own script and/or set of functions.