Bug 31734 - REGRESSION(r51230): commit-queue made a non-sensical rejection during git svn dcommit
Summary: REGRESSION(r51230): commit-queue made a non-sensical rejection during git svn...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on: 29914
  Show dependency treegraph
Reported: 2009-11-20 11:33 PST by Eric Seidel (no email)
Modified: 2009-11-26 08:54 PST (History)
2 users (show)

See Also:

Patch (3.34 KB, patch)
2009-11-25 06:49 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (2.75 KB, patch)
2009-11-25 11:51 PST, Zoltan Horvath
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-11-20 11:33:38 PST
commit-queue made a non-sensical rejection during git svn dcommit

This bug is to remind me to investigate why and fix it if necessary.
Comment 1 Eric Seidel (no email) 2009-11-24 15:26:53 PST
We saw a similar rejection just now:
Comment 2 Eric Seidel (no email) 2009-11-24 15:49:18 PST
Now that I understand what's going wrong this should be simple to fix!  :)
Comment 3 Eric Seidel (no email) 2009-11-24 15:51:58 PST
the git svn dcommit call just needs to pass the include_stderr flag.  Many of the calls probably need to have that enabled.  In fact, I would suggest that include_stderr be enabled for all callers of run_command except for the one or two that the patch was trying to fix.
Comment 4 Zoltan Horvath 2009-11-25 06:49:57 PST
Created attachment 43839 [details]
Comment 5 Eric Seidel (no email) 2009-11-25 08:18:14 PST
Comment on attachment 43839 [details]

svn-create-patch is actually the culprit of the stderr in diffs. :)  As you can see in this diff you uploaded.
Comment 6 Zoltan Horvath 2009-11-25 11:51:49 PST
Created attachment 43861 [details]
Comment 7 Eric Seidel (no email) 2009-11-25 11:52:49 PST
Comment on attachment 43861 [details]

Comment 8 Eric Seidel (no email) 2009-11-25 11:53:24 PST
Comment on attachment 43861 [details]

We could also unit test this, with an scm_unittest.py test.  My review still stands, but a unit test would be nice, so that we don't break this again.
Comment 9 Zoltan Horvath 2009-11-25 11:56:35 PST
You were very fast!
I'd wrote this: "You're right! Now it works well. (I don't see the ? xxx dir in the patch. :) )"

I will write the unit test at tomorrow morning! Should I file it here or make a new bug for it?
Comment 10 Eric Seidel (no email) 2009-11-25 11:58:38 PST
You're welcome to do either. :)  Thanks!
Comment 11 Zoltan Horvath 2009-11-25 12:02:32 PST
Committed r51391: <http://trac.webkit.org/changeset/51391>
Comment 12 Zoltan Horvath 2009-11-26 08:54:05 PST
I'll have time only next week for the unit-test. 
I opened a bug for it: #31916