Having just taken a look at these patches here are my thoughts:
0001 - looks good
0002 - we'd need some explanation why this is a good idea since it completely reverts the fix for bug 2745292 LXR CGI Scripts Fail on Windows and hence puts that bug back in the code
0003 - I don't understand what this is protecting lxr from. More explanation needed.
0004 - looks like a unnecessary comment.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> 0002 - we'd need some explanation why this is a good idea since it completely reverts the fix for bug 2745292 LXR CGI Scripts Fail on Windows and hence puts that bug back in the code
Well if #!perl really works on Windows, then I guess something more involved will have to be done. This here is just to make it work on unix.
> 0003 - I don't understand what this is protecting lxr from. More explanation needed.
Ehr, join builds a single string from all arguments. Which is passed to a shell. So $path could do pretty much anything it wants from the shell.
And it could simply be a space in a path and it would break. Right?
> 0004 - looks like a unnecessary comment.
It is the result of me trying to understand the code. You simply set $debug etc. in the same file, without any hint as to what that does, and only after 10 or 15 minutes I found out that it *sets imported variables*, which I've never seen elsewhere I think. Importing those variables explicitely or adding a comment will help people understand it as they grep upwards to find the declaration of those variables.
So I'd say it's definitely not unnecessary. Explicit import would be the alternative, but I didn't go that way so as to not make changes to the code that existing coders would have to get used to.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
0001: may even be improved - to be considered in genxref
0002: already in trunk
0003: ctags can now only be invoked from genxref. It is supposed in this context that the local LXR-site manager will not try to hack himself. No need to quote the $path (moreover, quotemeta is probably not the right function to use since it is intended to sanitize regexps).
0004: whole source underwent commenting review in release 1.0, though the exports have not been specifically commented.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Having just taken a look at these patches here are my thoughts:
0001 - looks good
0002 - we'd need some explanation why this is a good idea since it completely reverts the fix for bug 2745292 LXR CGI Scripts Fail on Windows and hence puts that bug back in the code
0003 - I don't understand what this is protecting lxr from. More explanation needed.
0004 - looks like a unnecessary comment.
> 0002 - we'd need some explanation why this is a good idea since it completely reverts the fix for bug 2745292 LXR CGI Scripts Fail on Windows and hence puts that bug back in the code
Well if #!perl really works on Windows, then I guess something more involved will have to be done. This here is just to make it work on unix.
> 0003 - I don't understand what this is protecting lxr from. More explanation needed.
Ehr, join builds a single string from all arguments. Which is passed to a shell. So $path could do pretty much anything it wants from the shell.
And it could simply be a space in a path and it would break. Right?
> 0004 - looks like a unnecessary comment.
It is the result of me trying to understand the code. You simply set $debug etc. in the same file, without any hint as to what that does, and only after 10 or 15 minutes I found out that it *sets imported variables*, which I've never seen elsewhere I think. Importing those variables explicitely or adding a comment will help people understand it as they grep upwards to find the declaration of those variables.
So I'd say it's definitely not unnecessary. Explicit import would be the alternative, but I didn't go that way so as to not make changes to the code that existing coders would have to get used to.
0001: may even be improved - to be considered in genxref
0002: already in trunk
0003: ctags can now only be invoked from genxref. It is supposed in this context that the local LXR-site manager will not try to hack himself. No need to quote the $path (moreover, quotemeta is probably not the right function to use since it is intended to sanitize regexps).
0004: whole source underwent commenting review in release 1.0, though the exports have not been specifically commented.