Bug 219982 – [webkitscmpy] Add command to canonicalize unpushed commits

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

We need to add a command to modify authorship and insert identifiers into commit before they are pushed to the remote.

[Radar WebKit Bug Importer]

<rdar://problem/72427536>

[Jonathan Bedard]

Created attachment 416434 [details]
Patch

[Jonathan Bedard]

Comment on attachment 416434 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/message.py:60
> +        lines[identifier_index - 1] = 'Identifier: {}'.format(commit)

I know Alexey has an opinion on this bit in that he thinks it should be a URL, which would look like:
<https://scm.webkit.org/1234@main>

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 416434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416434&action=review
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/message.py:60
> > +        lines[identifier_index - 1] = 'Identifier: {}'.format(commit)
> 
> I know Alexey has an opinion on this bit in that he thinks it should be a
> URL, which would look like:
> <https://scm.webkit.org/1234@main>

Note: This service does not yet exist, so this URL is dead.

[Jonathan Bedard]

Created attachment 416477 [details]
Patch

[Jonathan Bedard]

Created attachment 416516 [details]
Patch

[Jonathan Bedard]

Created attachment 416542 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 416542 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/message.py:62
> +        lines.insert(identifier_index, 'Identifier: {}'.format(identifier_template.format(commit)))

I continue to think that we want a descriptive name here that gives at least a hint for how it's expected to be used by people, e.g. "Canonical link".

[Alexey Proskuryakov]

Comment on attachment 416542 [details]
Patch

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

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/message.py:62
>> +        lines.insert(identifier_index, 'Identifier: {}'.format(identifier_template.format(commit)))
> 
> I continue to think that we want a descriptive name here that gives at least a hint for how it's expected to be used by people, e.g. "Canonical link".

Also, on a purely nitpicking level, the URL is not the identifier, only the last part of it is.

[Jonathan Bedard]

Created attachment 416543 [details]
Patch

[Jonathan Bedard]

Created attachment 416558 [details]
Patch

[Jonathan Bedard]

Created attachment 417028 [details]
Patch

Comment on attachment 417028 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program.py:186
> +    from webkitscmpy.canonicalize import Canonicalize

There was a version of patch that splits the commands into different files, but we decided to keep them in a single file since there were too few commands/code to split them.
I see the deferred import is to avoid circular import. Do you think it's time to consider re-split commands?

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/committer.py:50
> +    REPOSITORY_PATH = os.environ.get('OLDPWD')

I do see GIT_* are set somewhere, but I don't see we set OLDPWD anywhere. How do we guarantee this since OLDPWD might be different based on PWD history.

[Jonathan Bedard]

Comment on attachment 417028 [details]
Patch

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

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/committer.py:50
>> +    REPOSITORY_PATH = os.environ.get('OLDPWD')
> 
> I do see GIT_* are set somewhere, but I don't see we set OLDPWD anywhere. How do we guarantee this since OLDPWD might be different based on PWD history.

The GIT_* variables are both read and written, OLDPWD is only read. The bash code you see is actually reading the values of the GIT_* variables printed out by this script and exporting that to git filter-branch.

Seems like git filter-branch always sets OLDPWD, although I'm not entirely certain why (seems likely to be something about git's underlying bash-y nature)

[Jonathan Bedard]

Comment on attachment 417028 [details]
Patch

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

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program.py:186
>> +    from webkitscmpy.canonicalize import Canonicalize
> 
> There was a version of patch that splits the commands into different files, but we decided to keep them in a single file since there were too few commands/code to split them.
> I see the deferred import is to avoid circular import. Do you think it's time to consider re-split commands?

I do, but I think that should be a stand-alone patch. No reason to make this one more complicated than it already is

Comment on attachment 417028 [details]
Patch

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

>>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program.py:186
>>> +    from webkitscmpy.canonicalize import Canonicalize
>> 
>> There was a version of patch that splits the commands into different files, but we decided to keep them in a single file since there were too few commands/code to split them.
>> I see the deferred import is to avoid circular import. Do you think it's time to consider re-split commands?
> 
> I do, but I think that should be a stand-alone patch. No reason to make this one more complicated than it already is

Sure.

>>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/committer.py:50
>>> +    REPOSITORY_PATH = os.environ.get('OLDPWD')
>> 
>> I do see GIT_* are set somewhere, but I don't see we set OLDPWD anywhere. How do we guarantee this since OLDPWD might be different based on PWD history.
> 
> The GIT_* variables are both read and written, OLDPWD is only read. The bash code you see is actually reading the values of the GIT_* variables printed out by this script and exporting that to git filter-branch.
> 
> Seems like git filter-branch always sets OLDPWD, although I'm not entirely certain why (seems likely to be something about git's underlying bash-y nature)

Got it.

r=me. Discussed with Jonathan in person, `git filter-branch` will set OLDPWD to the repo. Another approach Jonathan mentioned is to use 'GIT_DIR'. I'm OK with both approach.

[EWS]

Committed r271182: <https://trac.webkit.org/changeset/271182>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417028 [details].

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 417065 [details]
Patch

[Aakash Jain]

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/__init__.py:62
>              if not repository.path:

Should repository.path be changed here as well?

[Aakash Jain]

Comment on attachment 417065 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/__init__.py:76
>              '{remote}/{branch}..{branch}'.format(remote=args.remote, branch=branch),

test

[Jonathan Bedard]

(In reply to Aakash Jain from comment #21)
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/__init__.py:62
> >              if not repository.path:
> 
> Should repository.path be changed here as well?

root_path is derived from path, what we're basically checking for here is "is the repository the user specified something actually existing on the filesystem, or is it referring to a remote repository accessed via a web API"

[Jonathan Bedard]

Created attachment 417084 [details]
Patch for landing

[EWS]

Committed r271203: <https://trac.webkit.org/changeset/271203>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417084 [details].

[Jonathan Bedard]

A follow-up note on this change: We are now using this script in the post-commit SVN hook to push to https://github.com/WebKit/WebKit.