784352 - show a warning when interdiff reports errors

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[:glob ✱]

if interdiff fails to process two diffs, it outputs an error to STDERR, which ends up in the webserver error log instead of displayed to the user.

this is sub optimal.

for example:

https://bugzilla.mozilla.org/attachment.cgi?oldid=640459&action=interdiff&newid=652168&headers=1

displays no output, however interdiff reports:

1 out of 1 hunk FAILED -- saving rejects to file /tmp/interdiff-1.aqPrKk.rej
interdiff: Error applying patch1 to reconstructed file

Comment 1

[:glob ✱]

Attachment: patch v1

Comment 2

[Frédéric Buclin]

Comment on attachment 653758 [details] [diff] [review]
patch v1

Now that we require Perl 5.10.1, I think you can achieve the same goal by using autodie, which is much cleaner: http://perldoc.perl.org/autodie.html

Note that I don't really see the point to bother the user with this error message. He cannot do anything about it anyway.

Comment 3

[:glob ✱]

Comment on attachment 653758 [details] [diff] [review]
patch v1

(In reply to Frédéric Buclin from comment #2)
> Now that we require Perl 5.10.1, I think you can achieve the same goal by
> using autodie, which is much cleaner: http://perldoc.perl.org/autodie.html

autodie's won't catch these sorts or errors, nor will it capture the error message.  interdiff is executed, so the open() statement doesn't fail.

here's what it reports:
Can't close(GLOB(0x41a2dd8)) filehandle: '' at Bugzilla/Attachment/PatchReader.pm line 131
line 131: close $interdiff_fh;
 
> Note that I don't really see the point to bother the user with this error
> message. He cannot do anything about it anyway.

displaying an error message is better than displaying a blank page, which is the current behavior.

Comment 4

[Frédéric Buclin]

Comment on attachment 653758 [details] [diff] [review]
patch v1

>+        $error && ThrowUserError('interdiff_error',
>+                                 { error_message => $error });

Nit: I prefer the |ThrowUserError() if $error;| syntax, which I think is less cryptic and more intuitive than |$error && ThrowUserError();|. Also, maybe it should be a code error instead of a user error? I have no strong opinion on this.


Otherwise your patch looks good and works fine, so r=LpSolit. I'm not sure which kind of error messages we can get besides the common

    Interdiff failed to process your request:
        /usr/bin/interdiff: /tmp/QuQpVyU_YO doesn't contain a patch

The problem with this error message is that 1) it leaks paths used in your system (see the long discussion in bug 319140 about this topic), and 2) it points to the temporary file instead of the name of the original attachment (which attachment is /tmp/QuQpVyU_YO?). This makes me wonder if this error message is really useful to the user. But we can give it a try for Bugzilla 5.0, and back out the patch if we think point 1) above is not acceptable.

Comment 5

[:glob ✱]

in case you're wondering, i haven't committed this yet because LpSolit's concerns about the value of displaying the full error message have got me thinking.

i intend to see if interdiff ever shows a message that contains useful information ("doesn't contain a patch" isn't useful).  if it doesn't, then i suspect it would be better to display a generic "interdiff failed to process the patches" error.

Comment 6

[:glob ✱]

from my experiments, interdiff never throws a useful error message, so a generic message would be better.  updated patch coming soon.

Comment 7

[:glob ✱]

Attachment: patch v2

change to using a generic warning.

i also switched from throwing an error to displaying a warning, to allow us to display partial results if possible (eg. https://bugzilla.mozilla.org/attachment.cgi?oldid=702216&action=interdiff&newid=702770&headers=1 )

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 705772 [details] [diff] [review]
patch v2

Review of attachment 705772 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 9

[Frédéric Buclin]

Comment on attachment 705772 [details] [diff] [review]
patch v2

>+        if (defined(my $error = <$interdiff_stderr>)) {
>+            warn($error);
>+            $warning = 'interdiff3';
>+        }

2 things:
- If $warning is already set by warn_if_interdiff_might_fail(), do we really want to override it?
- I fear warn($error) could fill the error log if the attachment is incorrectly formatted or for any other reason. This looks like debug code to me. It should go away, isn't it?


Holding approval till these questions are answered.

Comment 10

[:glob ✱]

(In reply to Frédéric Buclin from comment #9)
> - If $warning is already set by warn_if_interdiff_might_fail(), do we really
> want to override it?

yes, if interdiff actually throws errors, it's more important than our "this /may/ fail" warnings.

> - I fear warn($error) could fill the error log if the attachment is
> incorrectly formatted or for any other reason. This looks like debug code to
> me. It should go away, isn't it?

currently interdiff outputs its warnings to STDERR, which are logged to the error log, so this patch won't add any additional logging.  i've used it many times to determine what was happening with interdiff on our production environment.

Comment 11

[:glob ✱]

i encountered a problem where interdiff would output "Warning: something's wrong" onto stderr, but apparently work correctly regardless.  investigating.

Comment 12

[:glob ✱]

Attachment: patch v3

the problem was we were getting a defined but empty string from reading stderr, which was incorrectly triggering the warning.

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 708552 [details] [diff] [review]
patch v3

r=dkl

Comment 14

[Frédéric Buclin]

Comment on attachment 708552 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla/Attachment/PatchReader.pm'

>+        if (defined($error) && $error ne '') {

On checkin, please simply write: if ($error). I really doubt $error can be 0.

Comment 15

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/attachment/diff-header.html.tmpl
Committed revision 8571.