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

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




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

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

Sorry for the delay. I got caught up with other things.

> Websites/perf.webkit.org/ChangeLog:2
> +2021-04-14  Zhifei Fang  <zhifei_fang at apple.com>
> +Add support for syncing repo with commit revision label

Something went awfully wrong with this change log entry.

> Websites/perf.webkit.org/tools/sync-commits.py:39
>	   for repository in repositories:
> -	       try:
>		   repository.fetch_commits_and_submit(server_config,
args.max_fetch_count, args.max_ancestor_fetch_count)

Nit: wrong indentation.

> Websites/perf.webkit.org/tools/sync-commits.py:47
> -	   return GitRepository(name=repository['name'],
git_url=repository['url'], git_checkout=repository['gitCheckout'],
git_branch=repository.get('branch'))
> +	   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'))

This line is way too long!
Please insert a hard line break somewhere.

> Websites/perf.webkit.org/tools/sync-commits.py:212
> +		   # translate the last svn revision to git hash

This kind of "what" comment is a code smell.
We should be extracting this logic as a helper method instead.

> Websites/perf.webkit.org/tools/sync-commits.py:216
> +		   last_fetched_git_hash = self._run_git_command(['svn',
'find-rev', 'r{}'.format(last_fetched)]).strip()
> +		   if not last_fetched_git_hash:
> +		       self._fetch_all_hashes()
> +		       last_fetched_git_hash = self._run_git_command(['svn',
'find-rev', 'r{}'.format(last_fetched)]).strip()

What is this retry logic about? When is this useful?

> Websites/perf.webkit.org/tools/sync-commits.py:219
> +		   last_fetched = last_fetched_git_hash

Why are we updating last_fetched? This is just wrong.
last_fetched comes from the server, it's nothing to do with the local state of
git clone or git svn state.
This script is meant to be fault tolerant, meaning that if the script had
failed / got killed mid way,
it can continue syncing the commit info from where it left off.
r- because of this.

> Websites/perf.webkit.org/tools/sync-commits.py:235
> +    def _get_svn_revision_from_message(self, message):

Nit: our coding style guidelines mandates that we do not prefix a getter
function with "get" unless it has an out argument.
Here, it should read _svn_revision_from_commit_message instead
https://webkit.org/code-style-guidelines/#names-out-argument

> Websites/perf.webkit.org/tools/sync-commits.py:239
> +	   if svn_revision_match:
> +	       return svn_revision_match.group('svn_revision')
> +	   return None

We prefer early exit over nested if. i.e. normal flow of logic should continue.
So this should read:
if not svn_revision_match:
    return None
return svn_revision_match.group('svn_revision')

or better yet:
return svn_revision_match.group('svn_revision') if svn_revision_match else None

> Websites/perf.webkit.org/tools/sync-commits.py:260
> +	       svn_revision = self._get_svn_revision_from_message(message)

Why are we parsing subversion revision from git commit?
Can't we just run git svn find-rev instead?
It can convert git hash to subversion revision as well as subversion to git
hash.

> Websites/perf.webkit.org/tools/sync-commits.py:269
> +	       'revision': current_hash if not svn_revision else svn_revision,
> +	       'revisionIdentifier': revision_identifier,
> +	       'previousCommit': previous_hash if not previous_revision else
previous_revision,

This isn't right. We can't switch between git hashes and revisions in a single
repository.
That would cause all sorts of havoc down the line.
We should instead error and exit if _report_svn_revision is specified but we
didn't find a subversion information.
r- because of this.