[webkit-reviews] review denied: [Bug 221982] Add support for syncing repo with commit revision label : [Attachment 423083] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 221982: Add support for syncing repo with commit revision label
https://bugs.webkit.org/show_bug.cgi?id=221982

Attachment 423083: Patch

https://bugs.webkit.org/attachment.cgi?id=423083&action=review




--- Comment #17 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 423083
  --> https://bugs.webkit.org/attachment.cgi?id=423083
Patch

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

r- because I don't think we want to land this as is.

> Websites/perf.webkit.org/tools/sync-commits.py:46
> +	   return GitRepository(name=repository['name'],
git_url=repository['url'], git_checkout=repository['gitCheckout'],
git_branch=repository.get('branch'),
fetch_revision_label=repository.get("fetchRevisionLabel", False))

This isn't right. We aren't really fetching anything.
If anything this is really about whether repository has revision identifier or
not.

> Websites/perf.webkit.org/tools/sync-commits.py:194
> +    REVISION_LABEL_RE = re.compile(r'Canonical link:
https://commits.webkit.org/(?P<revision_label>\d+@\S+)\n')

I don't think it's right that we're hard-coding into the script.
This script is supposed to work with any repository.
This should probably be specified in fetch_revision_label in the config file.

> Websites/perf.webkit.org/tools/sync-commits.py:245
> +	   if self._fetch_revision_label:
> +	       revision_label =
self.REVISION_LABEL_RE.search(message).group('revision_label')
> +	       result['revisionLabel'] = revision_label

This is unnecessarily complex. Given our API is perfectly capable of accepting
null, we should just do this:
'revisionIdentifier': ~