[webkit-reviews] review denied: [Bug 221982] Add support for syncing repo with commit revision label : [Attachment 427699] 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 427699: Patch

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




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

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

r-. Please add all my comments above and below.

> Websites/perf.webkit.org/tools/sync-commits.py:-38
> -	       try:
>		   repository.fetch_commits_and_submit(server_config,
args.max_fetch_count, args.max_ancestor_fetch_count)
> -	       except Exception as error:
> -		   print "Failed to fetch and sync:", error

Again, I don't think we want to make this change.

> Websites/perf.webkit.org/tools/sync-commits.py:48
> +	   return GitRepository(name=repository['name'],
git_url=repository['url'], git_checkout=repository['gitCheckout'],
> +				git_branch=repository.get('branch'),
report_revision_identifier_in_commit_msg=repository.get('reportRevisionIdentifi
er'),
> +			       
report_svn_revison=repository.get('reportSVNRevision'))

Nit: wrong indentation. Despite of what PEP8 says, we don't align arguments.
We always indent by 4 spaces instead.

> Websites/perf.webkit.org/tools/sync-commits.py:214
> +		   # translate the last svn revision to git hash
> +		   last_fetched_git_hash =
self._git_hash_from_svn_revision(last_fetched)

Again, this should be extracted as a helper method instead of adding a comment.

> Websites/perf.webkit.org/tools/sync-commits.py:216
> +		       self._fetch_remote()

Again, we don't want to fetch all git commits just because we can't find some
svn revisions.