Bug 36257 – git merge-driver not getting triggered enough?

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Tools / Tests

[Eric Seidel (no email)]

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  <[e-mail]>\n\n        Reviewed by Baz Bar.\n\n        A more awesomer change yet!\n\n        * scm_unittest.py:\n\n2009-10-26  Eric Seidel  <[e-mail]>\n\n        Reviewed by NOBODY (OOPS!).\n\n        Second most awesome change ever.\n\n        * scm_unittest.py:\n\n2010-03-17  Eric Seidel  <[e-mail]>\n\n        Reviewed by Foo Bar.\n\n        Most awesome change ever.\n\n        * scm_unittest.py:\n' != '2010-03-17  Eric Seidel  <[e-mail]>\n\n        Reviewed by Joe Cool.\n\n        Second most awesome change ever.\n\n        * scm_unittest.py:\n\n2009-10-27  Eric Seidel  <[e-mail]>\n\n        Reviewed by Baz Bar.\n\n        A more awesomer change yet!\n\n        * scm_unittest.py:\n\n2009-10-26  Eric Seidel  <[e-mail]>\n\n        Reviewed by Foo Bar.\n\n        Most awesome change ever.\n\n        * scm_unittest.py:\n'

----------------------------------------------------------------------

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Chris Jerdonek]

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  <[e-mail]>
 <-- no leading space
+        Reviewed by NOBODY (OOPS!).
+
+        Second most awesome change ever.
+
+        * scm_unittest.py:
+
[phone]  Eric Seidel  <[e-mail]>
+
         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  <[e-mail]>\n\n        Reviewed by NOBODY (OOPS!).\n\n        Second most awesome change ever.\n\n        * scm_unittest.py:\n\n2009-10-27  Eric Seidel  <[e-mail]>\n\n        Reviewed by Baz Bar.\n\n        A more awesomer change yet!\n\n        * scm_unittest.py:\n\n2009-10-26  Eric Seidel  <[e-mail]>\n\n        Reviewed by Foo Bar.\n\n        Most awesome change ever.\n\n        * scm_unittest.py:\n' != '2010-03-17  Eric Seidel  <[e-mail]>\n\n        Reviewed by Joe Cool.\n\n        Second most awesome change ever.\n\n        * scm_unittest.py:\n\n2009-10-27  Eric Seidel  <[e-mail]>\n\n        Reviewed by Baz Bar.\n\n        A more awesomer change yet!\n\n        * scm_unittest.py:\n\n2009-10-26  Eric Seidel  <[e-mail]>\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

[Chris Jerdonek]

(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.

[Eric Seidel (no email)]

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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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?

[Chris Jerdonek]

(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?

[Chris Jerdonek]

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:
+
[phone]  Eric Seidel  <[e-mail]>
+
+        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

[Eric Seidel (no email)]

I think I'm going to close this, as I don't think we're triggering this anymore.

[Nico Weber]

Well then.

[Eric Seidel (no email)]

Sorry, failed to actually close I guess?