Applying patches with image/binary files

[Philip Rogers]

blink-dev,

"git cl patch [issue#]" is useful for applying patches locally and I use it all the time when reviewing patches or moving patches between computers. Sadly, this script has a limitation where it cannot apply patches that contain image/binary files, such as LayoutTest image expectations. Today I discovered a secret** script that can patch a CL with binary files:

(from [chromium root]/src):

apply_issue --root_dir . --issue [issue#] --server https://codereview.chromium.org --ignore_deps -v

Happy patching!

** Should we let folks know about this secret script? All our docs reference "git cl patch" without mentioning the image file limitation. If nobody pipes up with a better approach, I'll go ahead and update the docs to mention this.

The error message you get when applying a patch with binary files using git cl patch is:

fatal: git diff header lacks filename information when removing 0 leading pathname components

Failed to apply the patch

[Elliott Sprehn]

Can we just fix git cl patch instead?

[Dirk Pranke]

+1. I don't know why we wouldn't just fix git_cl.

-- Dirk

[Steve Kobes]

+1

The bug I filed about this was duped into http://crbug.com/416255, which suggests waiting for a switch to Gerrit. :-(

[Robert Iannucci]

Hm... if apply_issue can apply the issue, then CQ should be able to as well (since that's how CQ applies issues... I thought?)

Maybe CQ is using `git cl patch` to apply the issue? That would really surprise me... 

/me looks

*is really surprised*

It looks like CQ has its very own separate implementation of patch application? +Andrii Shyshkalov +Paweł Hajdan, Jr. am I reading this correctly?

:(

R

[tan...@chromium.org]

+sergiyb@

CQ uses a fork of apply_patch functions (for each checkout type) from checkouts.py of depot_tools (see here), which is what apply_issue is using too [2].

So, if apply_issue works for you, then maybe there is an upstream change that we need need to get into CQ?

Also, with respect to dup for switch to Gerrit: current plan of mine is to allow projects to use both Rietveld and Gerrit at the same time.

What that should mean in practice: you'd be able to upload patch to Gerrit with just 1 extra flag "git cl upload --gerrit", do review there and land it through CQ, bypassing Rietveld completely.

Cheers,

Andrii

[1] https://chrome-internal.googlesource.com/infra/infra_internal/+/7c87d1dbbe11e4d833148226ca8f2d55ac2fd2ed/commit_queue/checkouts.py#632  (sorry, the link only works for Googlers). 

[2] https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/checkout.py&sq=package:chromium&type=cs&q=apply_patch&l=633

[Steve Kobes]

I should add that the behavior described in http://crbug.com/416255 doesn't actually match my experience.  I have successfully used the CQ to land patches containing binary files.

I have un-duped http://crbug.com/557512 after reading this thread.

[Dirk Pranke]

Generally speaking, patches that include binary files are supposed to work (and do). Rietveld will choke 

on really large patches but that's a separate issue.

If you're having trouble w/ specific patches, please file bugs for them.

Hopefully we can get all of the infra scripts using the same logic to apply patches as well.

-- Dirk

[Steve Kobes]

http://crbug.com/557512 is fixed, and 'git cl patch' should work with binary files now.   Let me know if you run into any issues.