Bug 47863 – scm.py should be able tell us what revisions made changes to a given file

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

scm.py should be able tell us what revisions made changes to a given file

[Adam Barth]

Created attachment 71101 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 71101 [details]
Patch

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

Looks OK.  Testing could be better.

r- for no error-case testing.  Otherwise looks fine.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:249
> +        self._subclass_must_implement()

using a helper function to raise confuses pylint.  I had it in an earlier version of scm.py, but then moved to copy/paste exception throwing instead.  Someone must have added it back.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:434
> +        revisions = []

Any time you find yourself doing this, it may be cleaner as a list-comprehension. :)

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:437
> +            match = re.search('^r(?P<revision>\d+) ', line)

If you worry this is hot we could store this in a class variable as a compiled regexp.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:440
> +            if not match:
> +                continue
> +            revisions.append(int(match.group('revision')))

So you coudl break this out into a self._revision_from_line(line) and then this function is a simple list-comprehension. :)

I guess it would have to be
revisions = [self._revision_from_line(line) for line in lines]
return [revision for revision in revisions if revision]  # filter out None from non-revision lines of output.

I'm not sure that's actually cleaner, but maybe I'm missing a slick way to do the filtering.

> WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:356
> +        self.assertEqual(self.scm.revisions_changing_file("test_file"), [5, 4, 3, 2])

Are there other interesting cases to test?

What happens if you pass a non-existant file?

What about a file which hasnt' been changed in the latest revision?

[Eric Seidel (no email)]

Comment on attachment 71101 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:357
> +        self.assertRaises(ScriptError, self.scm.revisions_changing_file, "non_existent_file")

Um.  Clearly I'm blind.  OK.

[WebKit Commit Bot]

Comment on attachment 71101 [details]
Patch

Clearing flags on attachment: 71101

Committed r70014: <http://trac.webkit.org/changeset/70014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.