Jakub Narebski <jnareb@xxxxxxxxx> writes:
> Try to avoid "Use of uninitialized value ..." errors, due to bad
> revision, incorrect filename, wrong object id, bad file etc. (wrong
> value of 'h', 'hb', 'f', etc. parameters). This avoids polluting web
> server errors log.
>
> Signed-off-by: Jakub Narebski <jnareb@xxxxxxxxx>
> ---
> This is a bit of "bandaid" patch, as if possible the callers should
> be corrected, and should check if there is something to pass along.
If the bad values come from the end user (via the browser), I
suspect that should be checked and rejected far earlier than
this sub on the output path is called. Are there code that
internally needs to pass bogus values to this function?
@@ -594,6 +594,9 @@ sub esc_html ($;%) {
my $str = shift;
my %opts = @_;
+ # empty or undefined
+ return $str unless $str;
+
I think this is wrong, as the callers of esc_html typically
concatenate the return value with other strings. If $str could
be undef, the caller would end up getting the warning when doing
the concatenation, so you did not solve anything.
$str could be '0' (literal string constant whose length is 1 and
has character *zero* in it), in which case you return it intact.
It is safe only because esc_html('0') is '0' itself; use of
"unless $str" here is a very bad style.
A more straightforward way obviously is:
if (!defined $str) {
return '';
}
@@ -1059,6 +1062,7 @@ sub git_get_hash_by_path {
or die_error(undef, "Open git-ls-tree failed");
my $line = <$fd>;
close $fd or return undef;
+ $line or return undef;
#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa panic.c'
$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/;
I think this one means well but not quite to my taste; path may
be given by the end user and we need to run ls-tree to find out
if that path exists or not here. $line would be undefined if
path does not exist, so...
if (!defined $line) {
return undef;
}
Note that it is very unlikely that $line consists of single '0'
here, so "$line or ..." would probably not break in practice.
My preference to use defined is more style and discipline thing.
@@ -1377,7 +1381,7 @@ sub parse_commit_text {
pop @commit_lines; # Remove '\0'
my $header = shift @commit_lines;
- if (!($header =~ m/^[0-9a-fA-F]{40}/)) {
+ if (!defined $header || $header !~ m/^[0-9a-fA-F]{40}/) {
return;
}
($co{'id'}, my @parents) = split ' ', $header;
I would prefer checking the length of @commit_linse before
blindly shifting it out.
if (!@commit_lines) {
return;
}
my $header = shift @commit_lines;
...
I am Ok with "return if (!@commit_lines);" if you feel it is
more Perl-ish.
One final note.
If you think using postfix "Statement Modifiers" somehow makes
your program look more Perl-ish, I think you should reconsider.
IMHO, coding more carefully to distinguish undef and other forms
of falsehood where the difference matters would make your code
look much more Perl-ish.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
