Tool for importing webrev patches?

Jorn Vernee jbvernee at xs4all.nl
Wed Jul 3 18:13:52 UTC 2019


Comments inline...

On 2019-07-03 17:12, Erik Helin wrote:
> On 7/3/19 2:19 PM, Jorn Vernee wrote:
>> The git workflow I've figured out so far is to:
>>    1. download the attached .patch file
>>    2. create a new local branch based on the one the patch is 
>> targeting
>>    3. switch to newly created branch
>>    4. use `git apply the_patch.patch` to apply the changes
>>    5. make a commit for the applied changes
>>    6. run the tests and/or play with the code before submitting a 
>> review.
>>    7. delete the local branch.
>> 
>> I think steps 2-5 could be merged into 1 using some tool (maybe even 
>> step 1. as well). Then, after reviewing, I just have to delete the 
>> newly created local branch.
>> 
>> Does such a tool exist in Skara already? (couldn't find it on the wiki 
>> [1]) Or, is there another recommended way for doing this? Is there 
>> interest in adding such a tool to Skara?
> 
> We would be happy to accept such a contribution :)

Great! I'll keep working on it.

> I think it is possible to automate quite a bit of what you are
> describing here, this is probably how I would approach it:
> 
> 1. Name the tool, perhaps: git-webrev-apply? A little verbose maybe, 
> but
>    it gets the job done and won't conflict with other git commands.
> 2. Take the URL for a webrev as input. Since webrev does not use a
>    standardized name for the patch file, we must parse the path to the
>    .patch file from the index.html file. Fortunately this is fairly
>    easy, see [0]. The pattern works with webrevs generated by both
>    git-webrev and webrev.ksh.
> 3. For applying the patch you can see the code in GitPr.java [1].

Thanks for the pointers!

I had made a quick prototype with the name 'wimport'. I was thinking 
about adding a command line switch to switch between webrev url and 
local file path, to be able to support both.

> The three steps above would result in something like:
> 
> $ git webrev-apply \
>   http://cr.openjdk.java.net/~stefank/8227085/webrev.01/
> 
> which just applies the patch on top of HEAD.
> 
> I'm not sure about creating a branch unless you also intend to commit
> the applied patch?

That is the plan.

I think making a commit for the patch makes it easier to see which code 
belongs to the patch when making my own changes on top of it, as well as 
making it easier to revert them later. Similarly, when applying multiple 
patches, this makes it easier to see which code belongs to which patch, 
rather than having everything mixed together in the working tree.

> Otherwise the working tree will be dirty and if the
> user switch back to master it will remain dirty. It is fairly easy to
> use the above tool as part of an alias:
> 
> [alias]
> webrev-fetch = !git checkout -b review && git webrev-apply $1 && git
> commit -m 'Webrev'
> 
> But I have an hard time coming with a clean way of doing the above
> from a tool. For example, what would the commit message be? A webrev
> is much more like a patch than a commit...

I was thinking of mimicking what mq does, which creates a change set for 
the patch with a message like "imported patch the_patch.patch". The 
branch could also be named "the_patch.patch".

> One drawback of the tool will be when the webrev has become stale
> (i.e. master has moved on and the patch in the webrev no longer
> applies cleanly). Since webrev does not include the base commit hash
> there is not much we can do here, besides fail :/
> 
> What do you think?

That seems fine to me. HG will create a bunch of .rej files in that case 
for files that conflicted. But, since the problem is usually not being 
on the tip of the branch, it seems better to just fail outright. Then 
it's easier to do a pull and then re-apply the patch.

Jorn

> Thanks,
> Erik
> 
> [0]: curl -s https://openjdk.github.io/cr/skara/26/webrev.00/ |\
>      grep -P '[ ]*(<td>)?<a href\=\".*\">.*\.patch</a></td>(</tr>)?$'
> [1]:
> http://git.openjdk.java.net/skara/blob/master/cli/src/main/java/org/openjdk/skara/cli/GitPr.java#L150


More information about the skara-dev mailing list