Bug 106402 – Adding tool to check local changes have been successfully landed via a third-party (such as the commit queue).

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim 'mithro' Ansell]

Adding tool to check a branch from git was successfully landed.

[Tim 'mithro' Ansell]

Created attachment 181819 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review

Do you want this?

> Tools/Scripts/webkitpy/tool/steps/landcheck.py:1
> +# Copyright (C) 2010 Google Inc. All rights reserved.

2013. :)

> Tools/Scripts/webkitpy/tool/steps/landcheck.py:52
> +            raise ScriptError(message="Unable to find committed subversion revision.")
> +
> +        if not scm.working_directory_is_clean():
> +            raise ScriptError(message="Uncommited Changes")

ScriptError is an error from Executive.  We should rename it to ExecutiveError to stop confusion. :) (You're not the first).  I would just use Exception or your own error.

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 181819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review
> 
> Do you want this?
Bah.  I misclicked, and typed that in the wrong box. :)

[Eric Seidel (no email)]

Comment on attachment 181819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review

This is fantastic, btw.

I assume you didn't run the SCM unit tests? (since they're probably broken.  You ahve to pass --all to test-webkitpy and then go get a sandwitch.  I recommend adding unittests but running only your new ones and not the whole SVN/SCM suite. :(

> Tools/Scripts/webkitpy/tool/steps/landcheck.py:57
> +        print "Diff with"
> +        print "git diff %s" % commit_id

Do you want this?

[Tim 'mithro' Ansell]

Comment on attachment 181819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181819&action=review

>> Tools/Scripts/webkitpy/tool/steps/landcheck.py:57
>> +        print "git diff %s" % commit_id
> 
> Do you want this?

So I ran into a problem, the "scm.rebase_onto_revision()" will become interactive if there is a problem rebasing. This means I can't just run the diff command for you.

Any idea on how to solve it? One thought is to do the rebase and check if it succeeded properly, otherwise reset to before the rebase and then output the manual commands you need to run?

[Tim 'mithro' Ansell]

Seems to get a lot of errors with;

[67/1734] webkitpy.common.checkout.scm.scm_unittest.GitSVNTest.test_commit_with_message_not_synced_with_conflict erred:
  Traceback (most recent call last):
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 1300, in test_commit_with_message_not_synced_with_conflict
      self.assertRaises(ScriptError, scm.commit_with_message, "another test commit", force_squash=True)
    File "/usr/lib/python2.7/unittest/case.py", line 471, in assertRaises
      callableObj(*args, **kwargs)
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 377, in commit_with_message
      return self.push_local_commits_to_server(username=username, password=password)
    File "/fast/chrome/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 462, in push_local_commits_to_server
      raise AuthenticationError(SVN.svn_server_host, prompt_for_password=True)
  AuthenticationError

[Tim 'mithro' Ansell]

Without my patch;
  Ran 1733 tests in 34.321s                                                                                               
  FAILED (failures=1, errors=22)


With my patch;
  Ran 1734 tests in 30.736s                                                                             
  FAILED (failures=1, errors=22)

[Eric Seidel (no email)]

test-webkitpy (without --all) should have no errors.

Sadly scm_unittests.py has been ignored for a *long* time and is likely full of failures. :(

[Tim 'mithro' Ansell]

Added 1 extra test which doesn't fail.

I'll go hit the tests with a stick in another patch.

[Tim 'mithro' Ansell]

Mike also suggested that I try the most comment case, then if that fails print what a human would need to do.

[Dirk Pranke]

I think this change is a little problematic as-is. I'm not entirely sure what the intent is, as it looks like it's checking two things: 1) that a given bug id has landed, and 2) that if we rebase a git branch against that bug id, then the branch is empty (fully merged), right? Given that this command has side effects (actually rebasing the branch), and isn't actually landing things, calling it "land-check" seems like a bad name. 

I'm also a bit torn on having a command that only works in git checkouts, but then again, you're not likely to need this at all in an svn checkout.

[Tim 'mithro' Ansell]

It seems like there has been provisions for git only commands already because of the following;

        AbstractDeclarativeCommand.__init__(self, options=options, requires_local_commits=True)

[Eric Seidel (no email)]

Correct, webkitpy works best with Git, but still supports SVN:
https://lists.webkit.org/pipermail/webkit-dev/2012-April/020198.html

[Tim 'mithro' Ansell]

There are already;

    def ensure_clean_working_directory(self, force_clean):
        if self.working_directory_is_clean():
            return
        if not force_clean:
            print self.run(self.status_command(), error_handler=Executive.ignore_error, cwd=self.checkout_root)
            raise ScriptError(message="Working directory has modifications, pass --force-clean or --no-clean to continue.")
        _log.info("Cleaning working directory")
        self.clean_working_directory()



    def ensure_no_local_commits(self, force):
        if not self.supports_local_commits():
            return
        commits = self.local_commits()
        if not len(commits):
            return
        if not force:
            _log.error("Working directory has local commits, pass --force-clean to continue.")
            sys.exit(1) #### WHY!? Oh god why!?
        self.discard_local_commits()

[Tim 'mithro' Ansell]

Any better idea for a name then?

 landed-rebase?
 compare-to-landed?

[Dirk Pranke]

is your intent to actually rebase as well, or is that just a (necessary?) side effect of checking to see if the branch is merged?

[Tim 'mithro' Ansell]

Its just a side effect of doing the diff. It also means that any uncommitted changes are easier to rebase onto HEAD.

[Dirk Pranke]

I'm not wild about the side effects. It seems like you can try doing a rebase plus a diff, and if you either get a diff or run into rebasing problems, you know the branch is not fully merged. Either way, abort and do a reset back to the previous branch point.

If you took that path, then something like 'check-merged' might be a good name.

[Tim 'mithro' Ansell]

The original idea was to make sure that I could delete a local git branch because everything on it has been merged successfully.

Can you think of a better way to do this?

[Dirk Pranke]

(In reply to comment #19)
> The original idea was to make sure that I could delete a local git branch because everything on it has been merged successfully.
> 

Right, doesn't my idea still let you know if it's safe to do that?

> Can you think of a better way to do this?

Unfortunately, no. I don't know of a good way to do this w/ git either, given the way we land changes.

[Tim 'mithro' Ansell]

Created attachment 183322 [details]
Patch - Have yet to finish the tests, but would like a review anyway.

[WebKit Review Bot]

Attachment 183322 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded.py:51:  [HasLanded.strip_change_log] Passing unexpected keyword argument 'flags' in function call  [pylint/E1123] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

Comment on attachment 183322 [details]
Patch - Have yet to finish the tests, but would like a review anyway.

View in context: https://bugs.webkit.org/attachment.cgi?id=183322&action=review

> Tools/ChangeLog:3
> +        Adding tool to check your local changes where successfully landed.

please go into a bit more detail in your changelogs, so that readers can understand what's going on. I wouuld add the name of the new webkit-patch command, and that it is a new webkit-patch command, at least :). I would also describe in a bit of detail what the command is doing.

> Tools/Scripts/webkitpy/common/checkout/diff_parser.py:61
> +                           ("^index [0-9a-f]*\.\.[0-9a-f]*", lambda matched: "===================================================================\n"),

why was this change needed?

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130
> +        # revision.

You don't have to limit yourself to 80 cols, so you could probably put this comment on one line.

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:132
> +        for cmt in sorted(self.comments(), reverse=True):

webkit style is to not use abbreviated variable names unless they're absolutely common and clear. Even I don't think 'cmt' makes it over that bar. In addition, it's pretty idiomatic, when iterating over a plural, to use the singular as the variable name (e.g., for comment in comments).

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:70
> +        # Now this is there it gets complicated, we need to compare out diff to the diff at landed_revision.

typo: "our" instead of "out"?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88
> +            ["interdiff", landed_file.name, local_file.name], decode_output=False)

interdiff? what's interdiff?

So, it looks like you've abandoned the 'merge the branch and see if we still have a diff' approach, and now you're attempting to see if the local diff basically matches the diff that landed? Does that actually work in practice? I assume that's what this interdiff thing helps with?

[Tim 'mithro' Ansell]

Comment on attachment 183322 [details]
Patch - Have yet to finish the tests, but would like a review anyway.

View in context: https://bugs.webkit.org/attachment.cgi?id=183322&action=review

>> Tools/ChangeLog:3
>> +        Adding tool to check your local changes where successfully landed.
> 
> please go into a bit more detail in your changelogs, so that readers can understand what's going on. I wouuld add the name of the new webkit-patch command, and that it is a new webkit-patch command, at least :). I would also describe in a bit of detail what the command is doing.

Done.

>> Tools/Scripts/webkitpy/common/checkout/diff_parser.py:61
>> +                           ("^index [0-9a-f]*\.\.[0-9a-f]*", lambda matched: "===================================================================\n"),
> 
> why was this change needed?

That is a good question, I'm trying to figure out why my diffs have a different format to the output of "git diff".

diff --git a/Tools/Scripts/webkitpy/tool/steps/haslanded.py b/Tools/Scripts/webkitpy/tool/steps/haslanded.py
new file mode 100644
index 0000000000000000000000000000000000000000..102b7890d5e029fb1c07c0a1ae1017813a12fee8

>> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130
>> +        # revision.
> 
> You don't have to limit yourself to 80 cols, so you could probably put this comment on one line.

Done

>> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:132
>> +        for cmt in sorted(self.comments(), reverse=True):
> 
> webkit style is to not use abbreviated variable names unless they're absolutely common and clear. Even I don't think 'cmt' makes it over that bar. In addition, it's pretty idiomatic, when iterating over a plural, to use the singular as the variable name (e.g., for comment in comments).

Done.

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:70
>> +        # Now this is there it gets complicated, we need to compare out diff to the diff at landed_revision.
> 
> typo: "our" instead of "out"?

Done.

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88
>> +            ["interdiff", landed_file.name, local_file.name], decode_output=False)
> 
> interdiff? what's interdiff?
> 
> So, it looks like you've abandoned the 'merge the branch and see if we still have a diff' approach, and now you're attempting to see if the local diff basically matches the diff that landed? Does that actually work in practice? I assume that's what this interdiff thing helps with?

interdiff is part of the standard GNU diff tools;

NAME
       interdiff - show differences between two diff files

SYNOPSIS
       interdiff [[-p n] | [--strip-match=n]] [[-U n] | [--unified=n]] [[-d PAT] | [--drop-context=PAT]] [[-q] | [--quiet]] [[-z] |
                 [--decompress]] [[-b] | [--ignore-space-change]] [[-B] | [--ignore-blank-lines]] [[-i] | [--ignore-case]] [[-w] |
                 [--ignore-all-space]] [[--interpolate] | [--combine] | [--flip]] [--no-revert-omitted] diff1 diff2

       interdiff {[--help] | [--version]}

DESCRIPTION
       interdiff creates a unified format diff that expresses the difference between two diffs. The diffs must both be relative to the
       same files. For best results, the diffs must have at least three lines of context.


Yes, I've abandoned the merge and diff approach and now comparing the two diff files. I hope to add more features to this in the future like "Looks like the diff partially landed, do you want to rebase onto it?"

[Tim 'mithro' Ansell]

Created attachment 183331 [details]
Patch

[WebKit Review Bot]

Attachment 183331 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded.py:51:  [HasLanded.strip_change_log] Passing unexpected keyword argument 'flags' in function call  [pylint/E1123] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

Comment on attachment 183331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183331&action=review

> Tools/ChangeLog:6
> +        This command compares the finally commit patch to the changes which

nit: commit -> committed.

This is actually a bit more detail than I would've written :). I'd probably just have put something like "was successfully landed by comparing the local diff to what was actually committed".

Note that I'm only being so picky here because you're new to the project and I'm trying to give some guidelines; I hope this is helpful.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88
> +            ["interdiff", landed_file.name, local_file.name], decode_output=False)

looks like interdiff is only available on my ubuntu precise box by default (not mac or win), so you might want to trap this in case the commands' not available and log something useful.

This approach seems reasonable and I'm fine w/ you landing something along these lines to see if it actually works and is useful. Needs at least some minimal testing of the new command, and you need to figure out what's up with the change to the diff_parser before you get an r+, though.

[Tim 'mithro' Ansell]

Comment on attachment 183331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183331&action=review

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:88
>> +            ["interdiff", landed_file.name, local_file.name], decode_output=False)
> 
> looks like interdiff is only available on my ubuntu precise box by default (not mac or win), so you might want to trap this in case the commands' not available and log something useful.

As the moment it'll raise an error when interdiff isn't found.

> 
> This approach seems reasonable and I'm fine w/ you landing something along these lines to see if it actually works and is useful. 

I've had quite a few people say they liked the idea of this patch locally, so I think we'll find it gets used quite a bit.

> Needs at least some minimal testing of the new command,

Adding now.

> and you need to figure out what's up with the change to the diff_parser before you get an r+, though.

The diff_parser change was caused by create_patch passing --full-index to diff.

I've added two test cases to diff_parser to make sure both types of diffs convert successfully.

[Tim 'mithro' Ansell]

Created attachment 183706 [details]
Patch

[WebKit Review Bot]

Attachment 183706 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:194:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292:  trailing whitespace  [pep8/W291] [5]
Total errors found: 10 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim 'mithro' Ansell]

Created attachment 183727 [details]
Patch

[WebKit Review Bot]

Attachment 183727 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:194:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292:  trailing whitespace  [pep8/W291] [5]
Total errors found: 10 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim 'mithro' Ansell]

Hey guys,

Could I get another review on this patch? I'm unsure the correct way to test the run method, should I just use the MockExecutive?

Tim

[Eric Seidel (no email)]

I think what's blocking me from reviewing this patch is understanding the use case.  I worry that we haven't achieved sufficent buy-in from the relevant parties here.  Namely Dirk Pranke, Adam and myself.

Perhaps its easiest for you and Dirk to have a heart-to-heart over irc, or you and I can?

Mostly I want to understand why we want this?  This makes it so that git users know that their git svn dcommit (aka webkit-patch land) succeeded?

I personally only webkit-patch land about once every 6 months. :)  Otherwise I use the commit-queue for all my patches, so I'm probably not your target audience. :)

Or maybe this is a use case that I haven't even conceived of?

[Tim 'mithro' Ansell]

This patch is useful for anyone who lands patches via the commit-queue rather than landing using git svn dcommit/svn commit. (Also works in any case where someone else land patches for you). 

It makes sure your current local changes match the changes that were landed and they didn't accidently land an old patch. This allows you to then discard your local changes without fear of accidentally missing something.

It should now work for both svn users and git users. Git users will probably use it more as they are more likely to have lots of local changes being sent out for review.

I've updated the title to make more sense.

[Eric Seidel (no email)]

Ah!  I see!  So this is not something we would add to "land" but rather just a separate command to verify "did this ever get committed" to a local commit?

[Eric Seidel (no email)]

I assume this only works if provided a bug id. :)  It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct?

[Tim 'mithro' Ansell]

(In reply to comment #37)
> I assume this only works if provided a bug id. :)  It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct?

It looks up the bugid in the Changelog (in the same way other commands do) or can be provided one on the commandline.

[Tim 'mithro' Ansell]

(In reply to comment #36)
> Ah!  I see!  So this is not something we would add to "land" but rather just a separate command to verify "did this ever get committed" to a local commit?

Yes, correct! It basically is verifying that you can throw away your local changes because they have already been committed via an alternative route.

[Tim 'mithro' Ansell]

(In reply to comment #37)
> I assume this only works if provided a bug id. :)  It checks the bug for the commit revision and then compares the commit revision to the local commit diff, correct?

Yes, that is correct.

[Dirk Pranke]

Comment on attachment 183727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183727&action=review

I'm basically okay with this patch and approach. You should fix (or suppress) the style warnings; otherwise, I'm just looking at minor issues.

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:136
> +        # If we don't find a revision, return None

nit: this comment doesn't add much.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:69
> +        # Now this is there it gets complicated, we need to compare our diff to the diff at landed_revision.

typo: "there" -> "where"?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:84
> +            ["interdiff", diff1_patch.name, diff2_patch.name], decode_output=False)

I'm still not fond of just raising an uncaught exception if interdiff isn't available. Tools shouldn't just return hard-to-follow stack traces to the user. Can you catch this somewhere and print a more helpful message?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:89
> +            return True

Should we log some sort of a message in this case rather than silently succeeding?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:118
> +        self._tool.scm().discard_local_changes()

Do you want this line?

[Dirk Pranke]

Comment on attachment 183727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183727&action=review

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:118
>> +        self._tool.scm().discard_local_changes()
> 
> Do you want this line?

Whoops, never mind, wasn't thinking about the fact that you told it to discard the changes :).

[Tim 'mithro' Ansell]

Created attachment 183866 [details]
Patch

[WebKit Review Bot]

Attachment 183866 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:194:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292:  trailing whitespace  [pep8/W291] [5]
Total errors found: 10 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim 'mithro' Ansell]

Friendly poke.

The style failures are caused by the tests which have patches in """ strings and thus need the extra space.

[Eric Seidel (no email)]

Comment on attachment 183866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183866&action=review

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130
> +        r = re.compile("Committed r(?P<svn_revision>\d+)")

It's slightly sad that we don't keep this string in the same place as it's generated, but OK.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:46
> +    @staticmethod

Why does everyone love @staticmethod so much?   Am I the only hater? :)  Statics are impossible to mock, short of replacing them on the actual object.  You can't even subclass to mock out a static.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:49
> +        convert = diff_parser.get_diff_converter(lines[0])

We don't generally use "get" in getters in WebKit, but obviously this one already exists.  Probably imported from some chromium code. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:54
> +        lines = StringIO.StringIO(diff).readlines()

Does readlines return you a generator?  Can one just write: for line in StringIO.StringIO(diff)?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:58
> +            indexline = re.match("^Index: ([^\\n]*/)?([^/\\n]*)$", line)

It's nice to put constants like these somewhere easily identifiable/reusable (like on the SVN object?), but if this is the only place we need this regexp then that's OK.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:72
> +    @staticmethod
> +    def diff_diff(diff1, diff2, diff1_suffix, diff2_suffix, executive=None):
> +        # Now this is where it gets complicated, we need to compare our diff to the diff at landed_revision.
> +        diff1_patch = tempfile.NamedTemporaryFile(suffix=diff1_suffix + '.patch')
> +        diff1_patch.write(diff1)
> +        diff1_patch.flush()

Here again, @staticmethod makes testing/mocking more difficult.  FileSystem has a temporary file method iirc, making it possible to test this method with a MockFileSystem instead of a real one.

FileSystem also has a write_binary_file helper function which does a full write. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:81
> +        # Diff the two diff's together...
> +        if not executive:
> +            executive = Executive()

If we're adidng new code, we should require an Executive.  Our current testing strategy tries to leverage our central mocks as much as possible.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:99
> +            raise ScriptError("Unable to find landed message in associated bug.")

ScriptError is an Executive-only thing and really should be renamed ExecutiveError.  It does have some special handling in webkit-patch, but it feels wrong to reuse-it here.  Callers will expect it to have properties which make no sense in this case.  I would just use Exception here.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:114
> +        pretty_diff_file = self._show_pretty_diff(diff_diff)

Use the with construction, and then you don't have to close. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:117
> +        diff_correct = self._tool.user.confirm("Can I discard local changes?")

I believe the grammar nazis would tell us it's "may I" :)

>> Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292
>> + 
> 
> trailing whitespace  [pep8/W291] [5]

Is there some way to tell pep8 to ignore this warning for these """?

[Tim 'mithro' Ansell]

Comment on attachment 183866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183866&action=review

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:46
>> +    @staticmethod
> 
> Why does everyone love @staticmethod so much?   Am I the only hater? :)  Statics are impossible to mock, short of replacing them on the actual object.  You can't even subclass to mock out a static.

Change to @classmethod

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:49
>> +        convert = diff_parser.get_diff_converter(lines[0])
> 
> We don't generally use "get" in getters in WebKit, but obviously this one already exists.  Probably imported from some chromium code. :)

Okay :)

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:54
>> +        lines = StringIO.StringIO(diff).readlines()
> 
> Does readlines return you a generator?  Can one just write: for line in StringIO.StringIO(diff)?

Done, it's leftover from when I was doing things with lines first.

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:58
>> +            indexline = re.match("^Index: ([^\\n]*/)?([^/\\n]*)$", line)
> 
> It's nice to put constants like these somewhere easily identifiable/reusable (like on the SVN object?), but if this is the only place we need this regexp then that's OK.

I think it's okay for now. Ideally we'd replace this function with something in the SCM object which knows how to discard parts of a patch properly...

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:114
>> +        pretty_diff_file = self._show_pretty_diff(diff_diff)
> 
> Use the with construction, and then you don't have to close. :)

Done.

>> Tools/Scripts/webkitpy/tool/steps/haslanded.py:117
>> +        diff_correct = self._tool.user.confirm("Can I discard local changes?")
> 
> I believe the grammar nazis would tell us it's "may I" :)

Done.

>>> Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292
>>> + 
>> 
>> trailing whitespace  [pep8/W291] [5]
> 
> Is there some way to tell pep8 to ignore this warning for these """?

You can use # nopep8 to stop pep8 from apply to a line, but that would break the patches...

[Tim 'mithro' Ansell]

Created attachment 184417 [details]
Patch

[WebKit Review Bot]

Attachment 184417 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/net/bugzilla/bug.py', u'Tools/Scripts/webkitpy/common/net/bugzilla/bug_unittest.py', u'Tools/Scripts/webkitpy/tool/commands/upload.py', u'Tools/Scripts/webkitpy/tool/steps/__init__.py', u'Tools/Scripts/webkitpy/tool/steps/haslanded.py', u'Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:194:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:204:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:215:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:227:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:239:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:250:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:262:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:272:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:283:  trailing whitespace  [pep8/W291] [5]
Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292:  trailing whitespace  [pep8/W291] [5]
Total errors found: 10 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 184417 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 184417 [details]
Patch

Rejecting attachment 184417 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/16076699

[Eric Seidel (no email)]

The flakiness must end!

[WebKit Review Bot]

Comment on attachment 184417 [details]
Patch

Clearing flags on attachment: 184417

Committed r140674: <http://trac.webkit.org/changeset/140674>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on bots:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/6087/steps/webkitpy-test/logs/stdio

[1623/1639] webkitpy.tool.steps.haslanded_unittest.HasLandedTest.test_run erred:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py", line 134, in test_run
      HasLanded.diff_diff(diff1, diff1_add_line, '', 'add-line'),
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/tool/steps/haslanded.py", line 84, in diff_diff
      ["interdiff", diff1_patch.name, diff2_patch.name], decode_output=False)
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 411, in run_command
      close_fds=self._should_close_fds())
    File "/Volumes/Data/slave/mountainlion-release-tests-wk1/build/Tools/Scripts/webkitpy/common/system/executive.py", line 482, in popen
      return subprocess.Popen(string_args, **kwargs)
    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 679, in __init__
      errread, errwrite)
    File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1228, in _execute_child
      raise child_exception
  OSError: [Errno 2] No such file or directory

[Eric Seidel (no email)]

Unfortunately the CQ/EWS no longer run test-webkitpy.  They used to,but because we don't parse test-webkitpy output currently, the CQ/EWS have no way to ignore known failures and would fail forever if the tree was red.

I suspect the fix here is easy.  Lets see if mithro is around #webkit.

[Ryosuke Niwa]

Skipped the test temporarily in http://trac.webkit.org/changeset/140776.

[Eric Seidel (no email)]

Made the test run on machines with interdiff, in http://trac.webkit.org/changeset/140777

[Dirk Pranke]

(In reply to comment #58)
> Made the test run on machines with interdiff, in http://trac.webkit.org/changeset/140777

'which interdiff' isn't going to work on windows. I guess maybe that'll sort of be okay, but probably only accidentally :). Also, having to call out to a subprocess in a decorator might be kinda slow.

We should probably fix this more robustly.