[webkit-reviews] review granted: [Bug 219982] [webkitscmpy] Add command to canonicalize unpushed commits : [Attachment 417028] Patch

[bugzilla-daemon at webkit.org]

dewei_zhu at apple.com has granted Jonathan Bedard <jbedard at apple.com>'s request
for review:
Bug 219982: [webkitscmpy] Add command to canonicalize unpushed commits
https://bugs.webkit.org/show_bug.cgi?id=219982

Attachment 417028: Patch

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




--- Comment #16 from dewei_zhu at apple.com ---
Comment on attachment 417028
  --> https://bugs.webkit.org/attachment.cgi?id=417028
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.