Summary: | [webkitscmpy] Add command to canonicalize unpushed commits | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aakash_jain, ap, dewei_zhu, slewis, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=219988 https://bugs.webkit.org/show_bug.cgi?id=219989 https://bugs.webkit.org/show_bug.cgi?id=219992 https://bugs.webkit.org/show_bug.cgi?id=220369 |
||||||||||||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2020-12-17 08:36:27 PST
Created attachment 416434 [details]
Patch
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> (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. Created attachment 416477 [details]
Patch
Created attachment 416516 [details]
Patch
Created attachment 416542 [details]
Patch
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". 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. Created attachment 416543 [details]
Patch
Created attachment 416558 [details]
Patch
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. 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) 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. Committed r271182: <https://trac.webkit.org/changeset/271182> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417028 [details]. Reopening to attach new patch. Created attachment 417065 [details]
Patch
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/canonicalize/__init__.py:62
> if not repository.path:
Should repository.path be changed here as well?
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 (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" Created attachment 417084 [details]
Patch for landing
Committed r271203: <https://trac.webkit.org/changeset/271203> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417084 [details]. 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. |