Bug 29914 - bugzilla-tool post-diff inserts stderr into patch
Summary: bugzilla-tool post-diff inserts stderr into patch
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 31734
  Show dependency treegraph
Reported: 2009-09-30 00:48 PDT by Shinichiro Hamaji
Modified: 2009-11-24 15:49 PST (History)
4 users (show)

See Also:

proposed patch (15.58 KB, patch)
2009-11-18 02:15 PST, Zoltan Horvath
abarth: review-
Details | Formatted Diff | Diff
other solution (2.87 KB, patch)
2009-11-19 01:22 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2009-09-30 00:48:46 PDT
After Bug 29316, bugzilla-tool post-diff inserts unnecessary stderr outputs into patches.

Maybe we should add an optional parameter into SCM.run_command to determine how to handle stderr and use the parameter in create_patch()?
Comment 1 Eric Seidel (no email) 2009-10-28 14:55:37 PDT
is the line which is "wrong".  We should not be piping stderr to stdout but rather we should rout it to a separate buffer (including to system.stderr) in some circumstances.
Comment 2 Zoltan Horvath 2009-11-18 02:15:55 PST
Created attachment 43419 [details]
proposed patch
Comment 3 Adam Barth 2009-11-18 08:21:50 PST
Do we really need to proliferate [0] all over the code?  That's pretty ugly and error-prone.
Comment 4 Zoltan Horvath 2009-11-18 15:13:36 PST
No, we don't. We can give back only the stdout by default (with run_command). Currently we use stderr output only in one unittest. 
So this is also a possible way. If it's better I'll file a new patch at the morning.
Comment 5 Eric Seidel (no email) 2009-11-18 15:16:59 PST
Yeah, I think we should just go back to teh "just return stdout" model.  The one problem with this new implementation is that places which want both stderr and stdout ouput can't get them interlaced anymore. :(  I think that if scripts like run-webkit-tests fail we want to report both outputs.  Although I'm not sure that any of our scripts use both, so we could go with this change for now and see what harm it does.

I also need to fix run_command to know how to echo to the current stdout and stderr as well.
Comment 6 Zoltan Horvath 2009-11-18 15:27:20 PST
By the way, we can use [stderr] and [stdout] instead of [1], [0]. It's more clearly.
Comment 7 Zoltan Horvath 2009-11-19 01:22:48 PST
Created attachment 43493 [details]
other solution
Comment 8 Adam Barth 2009-11-19 22:13:25 PST
Comment on attachment 43493 [details]
other solution

I think we should do this.
Comment 9 WebKit Commit Bot 2009-11-19 22:21:00 PST
Comment on attachment 43493 [details]
other solution

Clearing flags on attachment: 43493

Committed r51230: <http://trac.webkit.org/changeset/51230>
Comment 10 WebKit Commit Bot 2009-11-19 22:21:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Seidel (no email) 2009-11-19 22:58:05 PST
Comment on attachment 43493 [details]
other solution

So the behavior of this is that stderr will now go to the parent's stderr.  Which is OK.  And might actually be better.  But if you intended for None to mean /dev/null, that is not the POpen behavior. :)

I think this is better than what we had before.  I think that some of our callsites definitely want the return_stderr behavior.

run-webkit-tests calls being one such callsite.