Bug 174438 – Add script to rebase patches during the WebKit2->WebKit/WebKit->WebKitLegacy transition

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We need a script to rebase patches during the WebKit2->WebKit/WebKit->WebKitLegacy transition.

[Radar WebKit Bug Importer]

<rdar://problem/33277112>

[Jonathan Bedard]

Created attachment 315296 [details]
Patch

[Matthew Hanson]

Comment on attachment 315296 [details]
Patch

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

Looks good to me pending successful tests cases for the most likely scenarios:
1. Patch that has not been rebased but touches WebKit (now WebKitLegacy)
2. Patch that has not been rebased but touches WebKit2 (now WebKit)
3. Patch that has not been rebased but touches both WebKit (now WebKitLegacy) and WebKIt2 (now WebKit)
4. Already rebased patch

> Tools/Scripts/rebase-patch-after-webkit-move:35
> +

Please include two lines between above function definitions.

> Tools/Scripts/rebase-patch-after-webkit-move:44
> +

I think that your approach is very clear.

I have two comments:
1. Is the lack of space after '+++' intentional? The first two cases have trailing spaces.
2. Functional techniques provide an elegant solution for this type of problem if the scope is larger. This function could be written as:

def is_editable_line(line):
    editable_prefixes = ('Index: ', '--- ', '+++')
    return any(map(line.startswith, editable_prefixes))

> Tools/Scripts/rebase-patch-after-webkit-move:52
> +            return ALREADY_REBASED

It would be good to call out that the `not in REBASE_DICTIONARY` check accounts for rebased paths that contain Source/WebKit (which could be either a source or a destination).

Ditto for current paths below.

> Tools/Scripts/rebase-patch-after-webkit-move:60
> +        if os.path.join('Source', current_name) in line:

The `os.path.join('Source, foo) in line` idiom is repeated a lot. I think a function would make it even easier to read.

> Tools/Scripts/rebase-patch-after-webkit-move:81
> +

I would like to see an isolated function parse the command line inputs / stdin and for this function to simply take the lines. Perhaps you could pass in a source keyword argument as well.

> Tools/Scripts/rebase-patch-after-webkit-move:89
> +            rebase_status = line_status

You could rewrite this phrase as:

rebase_status = max(rebase_status, line_status)

> Tools/Scripts/rebase-patch-after-webkit-move:97
> +

I'm tempted to suggest forcing input to stdin. That way you avoid situations like this where you are implicitly doing nothing for files.

> Tools/Scripts/rebase-patch-after-webkit-move:100
> +

Perhaps we could prompt instead of always rebasing?

Or check for the existence of the existing paths?

> Tools/Scripts/rebase-patch-after-webkit-move:110
> +

I suggest embedding your top-level script in a block like this:

if __name__ == '__main__':
    ....

This prevents your script from running if it is ever imported as a module. That shouldn't happen here, but could easily happen if unit tests were ever added, for example.

> Tools/Scripts/rebase-patch-after-webkit-move:124
> +    exit(0)

It's not necessary for this script, but you should look into the argparse module the next time you need to parse arguments from the command line.

> Tools/Scripts/rebase-patch-after-webkit-move:126
> +if len(files_to_rebase) == 0:

You can just write:

if files_to_rebase:

[Jonathan Bedard]

Comment on attachment 315296 [details]
Patch

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

>> Tools/Scripts/rebase-patch-after-webkit-move:81
>> +
> 
> I would like to see an isolated function parse the command line inputs / stdin and for this function to simply take the lines. Perhaps you could pass in a source keyword argument as well.

This is going to render your comment on line 97 moot.

>> Tools/Scripts/rebase-patch-after-webkit-move:124
>> +    exit(0)
> 
> It's not necessary for this script, but you should look into the argparse module the next time you need to parse arguments from the command line.

I'm aware of argparse.  I figured that given the simplicity of this script, it was better to just manually parse them.

[Jonathan Bedard]

(In reply to Matthew Hanson from comment #3)
> Comment on attachment 315296 [details]
> Patch

I have comments on each of your 4 cases.
 
> Looks good to me pending successful tests cases for the most likely
> scenarios:
> 1. Patch that has not been rebased but touches WebKit (now WebKitLegacy)

This is the complicated case.  In this instance, we can't tell if the patch has already been rebased (ie, those paths were originally WebKit2).  As I mention in my response to #4, I like the idea of prompting the user, but that will only work for files.

> 2. Patch that has not been rebased but touches WebKit2 (now WebKit)

Definitely works.  These patches will trigger the NEED_REBASE code path.

> 3. Patch that has not been rebased but touches both WebKit (now
> WebKitLegacy) and WebKIt2 (now WebKit)

Definitely works.  Like situation 2, this type of patch will trigger the NEED_REBASE code path.

> 4. Already rebased patch

If the patch contains WebKitLegacy, it will trigger the DO_NOT_NEED_REBASE path.  This is the same path triggered by a patch which does not contain WebKit or WebKit2 in it.

If, however, the patch only contains WebKit in it, we are in the same situation as #1.  I think your suggestion to prompt is a good one, although, we can't prompt if the file was passed through stdin.

[Jonathan Bedard]

Created attachment 315347 [details]
Patch

[Jonathan Bedard]

Created attachment 315361 [details]
Patch

[Jonathan Bedard]

Needed to add support for git-svn patches.

[Jonathan Bedard]

Created attachment 315378 [details]
Patch

[Jonathan Bedard]

Created attachment 315384 [details]
Patch

[Jonathan Bedard]

Committed r219477: <http://trac.webkit.org/changeset/219477>

[Daniel Bates]

While I like the idea of behind this script historically we have used svn-apply to do such migrations.

[Daniel Bates]

(In reply to Daniel Bates from comment #12)
> While I like the idea of behind this script historically we have used
> svn-apply to do such migrations.

To elaborate further the reason we use svn-apply to do such migrations is because svn-apply familiar to those that use a SVN checkout of WebKit, is used by the EWS bots, commit queue, and webkit-patch. This has the benefit that patches posted to Bugzilla pre-migration can be migrated by the commit queue on landing among other benefits.

[Jonathan Bedard]

(In reply to Daniel Bates from comment #13)
> (In reply to Daniel Bates from comment #12)
> > While I like the idea of behind this script historically we have used
> > svn-apply to do such migrations.
> 
> To elaborate further the reason we use svn-apply to do such migrations is
> because svn-apply familiar to those that use a SVN checkout of WebKit, is
> used by the EWS bots, commit queue, and webkit-patch. This has the benefit
> that patches posted to Bugzilla pre-migration can be migrated by the commit
> queue on landing among other benefits.

I had considered using SVN apply to do this, but the trouble is, since this is a double move, it's very easy to construct a patch where the script cannot know if it has already been rebased.  Any patch with a 'Source/WebKit/...' path in it may require a rebase, or it may not.  Since we can't prompt the user on bots, I decided a temporary stand-alone script would be best in this case.