Last Comment Bug 784352 - show a warning when interdiff reports errors
: show a warning when interdiff reports errors
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: unspecified
: All All
: -- minor (vote)
: Bugzilla 5.0
Assigned To: Byron Jones ‹:glob› [PTO until 2016-10-10]
: default-qa
:
Mentors:
Depends on:
Blocks: 839095 845725
  Show dependency treegraph
 
Reported: 2012-08-21 07:53 PDT by Byron Jones ‹:glob› [PTO until 2016-10-10]
Modified: 2013-02-27 01:45 PST (History)
2 users (show)
LpSolit: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (2.71 KB, patch)
2012-08-21 08:12 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review+
Details | Diff | Splinter Review
patch v2 (3.21 KB, patch)
2013-01-24 01:00 PST, Byron Jones ‹:glob› [PTO until 2016-10-10]
dkl: review+
Details | Diff | Splinter Review
patch v3 (3.24 KB, patch)
2013-01-31 06:15 PST, Byron Jones ‹:glob› [PTO until 2016-10-10]
dkl: review+
Details | Diff | Splinter Review

Description Byron Jones ‹:glob› [PTO until 2016-10-10] 2012-08-21 07:53:18 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2012-08-21 08:12:34 PDT
Created attachment 653758 [details] [diff] [review]
patch v1
Comment 2 Frédéric Buclin 2012-09-04 15:30:21 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2012-09-04 21:55:26 PDT
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 2012-09-24 10:33:37 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2012-10-16 01:53:29 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-01-24 00:56:31 PST
from my experiments, interdiff never throws a useful error message, so a generic message would be better.  updated patch coming soon.
Comment 7 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-01-24 01:00:10 PST
Created attachment 705772 [details] [diff] [review]
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] 2013-01-29 14:31:09 PST
Comment on attachment 705772 [details] [diff] [review]
patch v2

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

r=dkl
Comment 9 Frédéric Buclin 2013-01-30 08:47:35 PST
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-01-30 20:23:09 PST
(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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-01-30 23:41:20 PST
i encountered a problem where interdiff would output "Warning: something's wrong" onto stderr, but apparently work correctly regardless.  investigating.
Comment 12 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-01-31 06:15:24 PST
Created attachment 708552 [details] [diff] [review]
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] 2013-02-01 07:56:45 PST
Comment on attachment 708552 [details] [diff] [review]
patch v3

r=dkl
Comment 14 Frédéric Buclin 2013-02-06 06:21:09 PST
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-02-06 23:03:45 PST
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/attachment/diff-header.html.tmpl
Committed revision 8571.

Note You need to log in before you can comment on or make changes to this bug.