Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rietveld patches #2

Merged
merged 3 commits into from
Aug 18, 2013
Merged

Rietveld patches #2

merged 3 commits into from
Aug 18, 2013

Conversation

jrioux
Copy link
Collaborator

@jrioux jrioux commented Aug 16, 2013

Include binary data in the patch upload to Rietveld. This works with both "git cl patch X" and "test-patches" aka Patchy. Tested this myself with https://codereview.appspot.com/13054043/

Also update to use https.

@jrioux
Copy link
Collaborator Author

jrioux commented Aug 18, 2013

So this has been tested "in the wild" and it looks like it is doing the right thing (https://codereview.appspot.com/12980044/ patch set 2 now includes the images) but it doesn't help Patchy much because Rietveld won't let you download patches that are too large. Obviously, including the binary data is bound to producing large patches. So this pull request, although still an improvement, isn't helping to the extend that I was hoping for.

@PhilHolmes
Copy link
Collaborator

I think it's still an improvement and should be used - I assume it would work with a patch with fewer image changes.

Phil Holmes

----- Original Message -----
From: Julien Rioux
To: gperciva/git-cl
Sent: Sunday, August 18, 2013 8:08 AM
Subject: Re: [git-cl] Rietveld patches (#2)

So this has been tested "in the wild" and it looks like it is doing the right thing (https://codereview.appspot.com/12980044/ patch set 2 now includes the images) but it doesn't help Patchy much because Rietveld won't let you download patches that are too large. Obviously, including the binary data is bound to producing large patches. So this pull request, although still an improvement, isn't helping to the extend that I was hoping for.


Reply to this email directly or view it on GitHub.

@gperciva
Copy link
Owner

LGTM, please push

@jrioux
Copy link
Collaborator Author

jrioux commented Aug 18, 2013

Thanks, merging...

jrioux added a commit that referenced this pull request Aug 18, 2013
Rietveld patches

Include binary data in the patch upload to Rietveld.

This works with both "git cl patch X" and "test-patches" aka Patchy.
Tested this myself with https://codereview.appspot.com/13054043/

Also update to use https.
@jrioux jrioux merged commit eb84dc1 into gperciva:master Aug 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants