Bug 31734

Summary:

REGRESSION(r51230): commit-queue made a non-sensical rejection during git svn dcommit

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

29914

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	eric: review+

Eric Seidel (no email)

Reported 2009-11-20 11:33:38 PST

commit-queue made a non-sensical rejection during git svn dcommit https://bugs.webkit.org/show_bug.cgi?id=31180#c5 This bug is to remind me to investigate why and fix it if necessary.

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2009-11-24 15:26:53 PST

We saw a similar rejection just now: https://bugs.webkit.org/show_bug.cgi?id=31689#c20

Eric Seidel (no email)

Comment 2 2009-11-24 15:49:18 PST

Now that I understand what's going wrong this should be simple to fix! :)

Eric Seidel (no email)

Comment 3 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.

Zoltan Horvath

Comment 4 2009-11-25 06:49:57 PST

Created attachment 43839 [details] Patch

Eric Seidel (no email)

Comment 5 2009-11-25 08:18:14 PST

Comment on attachment 43839 [details] Patch svn-create-patch is actually the culprit of the stderr in diffs. :) As you can see in this diff you uploaded.

Zoltan Horvath

Comment 6 2009-11-25 11:51:49 PST

Created attachment 43861 [details] Patch

Eric Seidel (no email)

Comment 7 2009-11-25 11:52:49 PST

Comment on attachment 43861 [details] Patch LGTM.

Eric Seidel (no email)

Comment 8 2009-11-25 11:53:24 PST

Comment on attachment 43861 [details] Patch 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.

Zoltan Horvath

Comment 9 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?

Eric Seidel (no email)

Comment 10 2009-11-25 11:58:38 PST

You're welcome to do either. :) Thanks!

Zoltan Horvath

Comment 11 2009-11-25 12:02:32 PST

Committed r51391: <http://trac.webkit.org/changeset/51391>

Zoltan Horvath

Comment 12 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

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