REGRESSION: svn-apply does not handle overlapping diffs? It appears since my initial fixes for svn-apply in http://trac.webkit.org/changeset/50547 It's not surprising that these regressed as they're not run by default. However: test-webkitpy --all will run all tests including the (slow) scm tests. ====================================================================== FAIL: test_svn_apply (webkitpy.scm_unittest.SVNTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py", line 406, in test_svn_apply self.assertEquals(read_from_path('ChangeLog'), expected_changelog_contents) AssertionError: '2009-10-27 Eric Seidel <eric@webkit.org>\n\n Reviewed by Baz Bar.\n\n A more awesomer change yet!\n\n * scm_unittest.py:\n\n2009-10-26 Eric Seidel <eric@webkit.org>\n\n Reviewed by NOBODY (OOPS!).\n\n Second most awesome change ever.\n\n * scm_unittest.py:\n\n2010-03-17 Eric Seidel <eric@webkit.org>\n\n Reviewed by Foo Bar.\n\n Most awesome change ever.\n\n * scm_unittest.py:\n' != '2010-03-17 Eric Seidel <eric@webkit.org>\n\n Reviewed by Joe Cool.\n\n Second most awesome change ever.\n\n * scm_unittest.py:\n\n2009-10-27 Eric Seidel <eric@webkit.org>\n\n Reviewed by Baz Bar.\n\n A more awesomer change yet!\n\n * scm_unittest.py:\n\n2009-10-26 Eric Seidel <eric@webkit.org>\n\n Reviewed by Foo Bar.\n\n Most awesome change ever.\n\n * scm_unittest.py:\n' ----------------------------------------------------------------------
The test output is hard to read, but what svn-apply is doing is taking this overlapping diff and applying it in the *middle* of the ChangeLog instead of at the top as the test is designed to test for. Something broke in some subsequent re-write of my re-write of svn-apply.
This regression means that it's possible for the commit-queue to end up landing changes with the wrong commit message, because it depends on svn-apply to guarantee that the ChangeLog entry it created was the top-most one.
It looks like the test is failing for at least two reasons. The first issue is that the test case does not include a leading space in the unchanged lines empty lines (so that all lines are offset by the same amount): one_line_overlap_patch = """Index: ChangeLog =================================================================== --- ChangeLog (revision 5) +++ ChangeLog (working copy) @@ -1,5 +1,13 @@ 2009-10-26 Eric Seidel <eric@webkit.org> <-- no leading space + Reviewed by NOBODY (OOPS!). + + Second most awesome change ever. + + * scm_unittest.py: + +2009-10-26 Eric Seidel <eric@webkit.org> + Reviewed by Foo Bar. <-- no leading space Most awesome change ever. """ fixChangeLogPatch expects each line to have at least one character. I don't expect this to be an issue in practice since git diff, etc do this. So this can be fixed by adding the leading space to the test cases, or better yet, making fixChangeLogPatch more lenient. If we fixChangeLogPatch, we should add additional test cases to fixChangeLogPatch.pl. The second issue is unrelated. I added the spaces and got this as the result: WebKitTools/Scripts/webkitpy/scm_unittest.py", line 406, in test_svn_apply self.assertEquals(read_from_path('ChangeLog'), expected_changelog_contents) AssertionError: '2010-03-17 Eric Seidel <eric@webkit.org>\n\n Reviewed by NOBODY (OOPS!).\n\n Second most awesome change ever.\n\n * scm_unittest.py:\n\n2009-10-27 Eric Seidel <eric@webkit.org>\n\n Reviewed by Baz Bar.\n\n A more awesomer change yet!\n\n * scm_unittest.py:\n\n2009-10-26 Eric Seidel <eric@webkit.org>\n\n Reviewed by Foo Bar.\n\n Most awesome change ever.\n\n * scm_unittest.py:\n' != '2010-03-17 Eric Seidel <eric@webkit.org>\n\n Reviewed by Joe Cool.\n\n Second most awesome change ever.\n\n * scm_unittest.py:\n\n2009-10-27 Eric Seidel <eric@webkit.org>\n\n Reviewed by Baz Bar.\n\n A more awesomer change yet!\n\n * scm_unittest.py:\n\n2009-10-26 Eric Seidel <eric@webkit.org>\n\n Reviewed by Foo Bar.\n\n Most awesome change ever.\n\n * scm_unittest.py:\n' It looks like the unit test is not setting the reviewer correctly -- in the way that bugzilla.py expects. scm_unittest.py does this-- def _create_patch(self, patch_contents): patch_path = os.path.join(self.svn_checkout_path, 'patch.diff') write_into_file_at_path(patch_path, patch_contents) patch = {} patch['reviewer'] = 'Joe Cool' patch['bug_id'] = '12345' patch['url'] = 'file://%s' % urllib.pathname2url(patch_path) return Attachment(patch, None) # FIXME: This is a hack, scm.py shouldn't be fetching attachment data. But bugzilla.py seems to return None for the reviewer when patch.reviewer() is called: def reviewer(self): if not self._reviewer: self._reviewer = self._validate_flag_value("reviewer") return self._reviewer This seems to be because the validate_flag_value method is looking at other keys: def _validate_flag_value(self, flag): email = self._attachment_dictionary.get("%s_email" % flag) if not email: return None
(In reply to comment #3) > fixChangeLogPatch expects each line to have at least one character. I don't > expect this to be an issue in practice since git diff, etc do this. So this > can be fixed by adding the leading space to the test cases, or better yet, > making fixChangeLogPatch more lenient. On second thought, I'm not sure that fixChangeLogPatch should be made more lenient in this way. It might be better if it simply errors out with a message since the diff is ill-formatted. Right now it returns the patch unchanged. This is actually a FIXME in VCSUtils.pm: # FIXME: Handle errors differently from ChangeLog files that # are okay but should not be altered. That way we can find out # if improvements to the script ever become necessary.
I'm unsure from Chris's comments if this is actually a regression or not, or if something else would have caused: http://trac.webkit.org/changeset/56231 I thought that this sort of bug was solved.
(In reply to comment #5) > I'm unsure from Chris's comments if this is actually a regression or not, or if > something else would have caused: > http://trac.webkit.org/changeset/56231 For future reference, the associated report is here: https://bugs.webkit.org/show_bug.cgi?id=36352 > I thought that this sort of bug was solved. I agree. At first glance, I don't see an explanation. I'll download the patch and take a closer look.
(In reply to comment #6) > I agree. At first glance, I don't see an explanation. I'll download the patch > and take a closer look. FYI, this patch applied correctly for me when I used svn-apply on my local machine (i.e. different from what actually happened): > svn-apply ../regression.patch patching file WebKitTools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitTools/Scripts/webkitpy/buildbot.py patching file WebKitTools/Scripts/webkitpy/commands/queries.py How was the patch committed?
(In reply to comment #7) > (In reply to comment #6) > > I agree. At first glance, I don't see an explanation. I'll download the patch > > and take a closer look. > > FYI, this patch applied correctly for me when I used svn-apply on my local > machine (i.e. different from what actually happened): > > > svn-apply ../regression.patch > patching file WebKitTools/ChangeLog > Hunk #1 succeeded at 1 with fuzz 3. > patching file WebKitTools/Scripts/webkitpy/buildbot.py > patching file WebKitTools/Scripts/webkitpy/commands/queries.py > > How was the patch committed? Judging by the history and the cq-, it doesn't look like it was commit queued: https://bugs.webkit.org/show_activity.cgi?id=36352 Was this git-svn-dcommitted?
I think I can reproduce the issue that happened here now-- https://bugs.webkit.org/show_bug.cgi?id=36352 The problem doesn't seem to be with svn-apply. The problem seems to be that the git merge-driver isn't getting triggered in a case in which we'd like it to be triggered. Steps to reproduce below: git checkout master git checkout -b test-repo # The next line sets us back to the problem commit: r56231. git reset --hard 9c004bfcc6b9c69e03215705ca335d0cf05ce06b git checkout -b test-36352 git rebase --onto HEAD~6 HEAD~1 # test-36352 is now at the state it was in when submitting the patch. git checkout test-repo git reset --hard 728f779e22b395b93abe2aabadf22db36aeaabb3 # test-repo is now at the state it was in (r56230) prior to committing. git checkout test-36352 git diff HEAD^ # shows what was getting committed-- diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog index 6af3810..1371f5d 100644 --- a/WebKitTools/ChangeLog +++ b/WebKitTools/ChangeLog @@ -2,6 +2,21 @@ Reviewed by Adam Barth. + Add basic "who-broke-it" command and revision -> broken builder association code + https://bugs.webkit.org/show_bug.cgi?id=36352 + + The "what-broke" command prints builders and what revisions we suspect + broke them. who-broke-it prints revisions and what builders we suspect + they broke. The sheriff-bot needs this revision to broken builder mapping + so this change adds it! + + * Scripts/webkitpy/buildbot.py: + * Scripts/webkitpy/commands/queries.py: + +2010-03-19 Eric Seidel <eric@webkit.org> + + Reviewed by Adam Barth. + Simplify BuildBot core builder code for easier re-use https://bugs.webkit.org/show_bug.cgi?id=36350 # Rebasing with the test-repo reproduces the issue... # # The git merge-driver doesn't get triggered, so resolve-Changelogs never # gets called, and the ChangeLog entry gets inserted into the middle. # Perhaps because the overlap is large enough (4 lines in this case as you # can see above), the ChangeLog diff gets applied cleanly enough so as # not to trigger a merge scenario. git rebase test-repo
I think I'm going to close this, as I don't think we're triggering this anymore.
Well then.
Sorry, failed to actually close I guess?