One-Click Revert in Rietveld

One-Click Revert in Rietveld.

This is the implementation of the design discussions in https://codereview.chromium.org/23483019/.

Workflow:
* When an issue is closed the revert button shows up.
* Clicking on the revert button brings up a popup text field where the user has to enter a reason for requesting the revert.
* A revert CL is then automatically created with the original patches in the patchset inverted.
* The CQ flag of the revert CL is checked.
* The revert CL description contains a link back to the original CL, the reason for revert, TBR, NOTRY and NOTREECHECKS (will be added to the CQ soon).
* The original CL gets a new comment that notifies recipients about the reverted CL, who created it and the reason for its creation.


What does not work:
* This implementation only works with Git patches, it is probably easy to invert SVN patches but I will leave that for another time (if it is required, I suspect it is not).
* 'A +' patches with 100% similarity indexes are not reversed because we do not easily know the SHA-1 index of the previous patch.
* Very large patches.
An error message is displayed in /revert_issue if any of the above are detected so that the system can fail early without creating unnecessary Issues/PatchSets/Patches/Contents.


Tested with:
* Comprehensive unit tests have been added that test 'A', 'A+', 'M', 'D' and chmod patches for both binaries and ASCII files.
* Tested end-to-end using a local modified CQ and the skiabot-test repository in https://skia.googlesource.com/ (it is hidden). Examples of CLs I tested with are:
** https://skia-codereview-staging.appspot.com/341001/
** https://skia-codereview-staging.appspot.com/191013/
(Feel free to click the 'Revert' button on the above CLs)
* Also tested DeadlineExeededErrors and OOMs with the following CLs:
** https://skia-codereview-staging2.appspot.com/40003/
** https://skia-codereview-staging2.appspot.com/90001/
** https://skia-codereview-staging2.appspot.com/50003/


Additional Information:
* The latest patchset has been rebased with Rietveld rev 45476f54945d.
* Support for NOTREECHECKS in the CQ has been added in https://codereview.chromium.org/68113009/ it is required for reverting breaking CLs from Rietveld while the tree is closed (one of the main use-cases of One-Click Revert).
* A design doc that details the workflow (with pretty pictures) is coming soon.

[iannucci, 2013-11-12 03:20:01]

I like this a lot, thank you for working on this :)

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches...
codereview/invert_patches.py:1: # Copyright 2013 Google Inc.
RE: A+ patches, I'm not sure the index line of git patches are actually used by
anything, so it may be OK to just use 0?

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches...
codereview/invert_patches.py:26: MODIFIED_STATUS = 'M'
These should probably go in model.py so we can do Patch.status and get a
symbolic constant rather than the freeform string. WDYT?

https://codereview.appspot.com/22410044/diff/340001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/models.py#newc...
codereview/models.py:925: def get_inverted_git_patch(self, patchset):
maybe make_inverted, and then have it assert that patch is a git patch. Then we
can support svn patches later without changing the callsites.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:853: def revert_issue(request):
let's put all these views in a separate file. we're trying to move away from the
views.py black hole.  It would probably even make sense to put them in
invert_patches.py

Also, shouldn't this be

POST api/%(issue)d/%(patchset)d/revert

Or something like that?

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:868: 'patchset =', original_patchset.key())
Direct datastore queries are the bads. Probably want to use
original_patchset.patches.

The reason for this is I'm going to be changing the Issue schema soon, and some
things like Patches will no longer be entities in the datastore (they'll be
virtual objects returned by the new Patcheset model, etc.)

I think there's the possibility for this to run the appengine frontend out of
memory as well. Right now rietveld hacks this by nulling out the data fields on
patches as it iterates. I would test this method by generating a huge patchset
(maybe 100+ files each a couple hundred lines). Generating this patchset
programmatically would probably be a good idea. I /think/ the dev_appserver will
emit the proper OOMs, but I'm not sure.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:877: ' inconvenience.', status=404)
I would move this error checking into the model, catch the exception and return
the Response then. i.e. this error checking will be done inline with the
processing loop.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:890: models.Issue.kind(),
you don't need the .kind() I don't think.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:918: private=original_issue.private,
Should we check that the reverting user can see the original Issue? Otherwise
you could get access to private patches by fishing revert URLs, since the user
will own the new revert issue.

We should also ensure that the original issue is closed.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:922: issue.put()
keep in mind that this .put() a) can throw and b) Will block.

I would look into buffering all the data to put, and then putting them all at
once with db.put()
https://developers.google.com/appengine/docs/python/datastore/functions#put

It may also be worth using a transaction for this entire method so all the
get()s and put()s happen at once. I'm not sure what will happen if you try to
revert a patchset that's being deleted, for example.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:937: for original_patch in original_patches:
I know that rietveld doesn't have very many examples of doing this sort of thing
correctly, but it would be a lot safer (to avoid exceeding deadline/running out
of memory) to fire off a bunch of taskqueue tasks to create these new Patch
objects (including the base files). Then they could be treated entirely as
normal content uploads. WDYT?

Actually, even just having the revert fire off a taskqueue task to generate the
inverted CL as you have done here would be safer, since the taskqueue can be run
by a beefier backend instance than the wimpy frontend. Then the protocol would
be:
  * POST /revert...
  * 202 (Accepted: creating)
  * poll: POST /revert
  * 201 (Created: here's the new issue number)

This could be done by having a new field on the Patchset which is the revert
status. Then on the first request, you could transactionally set the revert
status from None to 'needed'. Then you could have a cron job which queries for
Patchsets which need a revert, does the processing, and updates the status to
the new CL number.

That would also make it easy to replace the revert button with a link to the
revert CL for subsequent page loads (just look at the revert status when you
render the patchset).

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html#n...
templates/patchset.html:172: {%endif%}
should we have a button here that's greyed out with the message of 'issue must
be closed to revert'?

[rmistry, 2013-11-12 22:00:37]

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches...
codereview/invert_patches.py:1: # Copyright 2013 Google Inc.
On 2013/11/12 03:20:02, iannucci wrote:
> RE: A+ patches, I'm not sure the index line of git patches are actually used
by
> anything, so it may be OK to just use 0?

'git apply --index ...' fails when the index line is missing or if the SHA-1 is
incorrect. I am not sure what SvnCheckout in depot_tools does with those
patches, it may work but one of my goals is to make this work with
depot_tools/GitCheckout and also ensure that the patches stored in Rietveld are
inverses of the original patches (this is unfortunately not possible right now
with A+ 100% similarity indexes patches).

https://codereview.appspot.com/22410044/diff/340001/codereview/invert_patches...
codereview/invert_patches.py:26: MODIFIED_STATUS = 'M'
On 2013/11/12 03:20:02, iannucci wrote:
> These should probably go in model.py so we can do Patch.status and get a
> symbolic constant rather than the freeform string. WDYT?

I see SVN patches have more statuses than the ones listed here. It is a good
idea but I would rather leave this here for now instead of risking this CL
breaking some SVN patches.

https://codereview.appspot.com/22410044/diff/340001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/models.py#newc...
codereview/models.py:925: def get_inverted_git_patch(self, patchset):
On 2013/11/12 03:20:02, iannucci wrote:
> maybe make_inverted, and then have it assert that patch is a git patch. Then
we
> can support svn patches later without changing the callsites.

Makes sense. Done.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:853: def revert_issue(request):
On 2013/11/12 03:20:02, iannucci wrote:
> let's put all these views in a separate file. we're trying to move away from
the
> views.py black hole.  It would probably even make sense to put them in
> invert_patches.py

Created revert_patchset.py for the view. invert_patches.py I see as having only
the logic to do patch inversion.

> 
> Also, shouldn't this be
> 
> POST api/%(issue)d/%(patchset)d/revert
> 
> Or something like that?

api/%(issue)d/%(patchset)d/revert is so much better, thanks. Done.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:868: 'patchset =', original_patchset.key())
On 2013/11/12 03:20:02, iannucci wrote:
> Direct datastore queries are the bads. Probably want to use
> original_patchset.patches.

Done.

> 
> The reason for this is I'm going to be changing the Issue schema soon, and
some
> things like Patches will no longer be entities in the datastore (they'll be
> virtual objects returned by the new Patcheset model, etc.)
> 
> I think there's the possibility for this to run the appengine frontend out of
> memory as well. Right now rietveld hacks this by nulling out the data fields
on
> patches as it iterates. I would test this method by generating a huge patchset
> (maybe 100+ files each a couple hundred lines). Generating this patchset
> programmatically would probably be a good idea. I /think/ the dev_appserver
will
> emit the proper OOMs, but I'm not sure.

Yes it will be great to verify what exactly happens. I am working on creating a
patch with 100s of files, will respond back here when I find out what it does.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:877: ' inconvenience.', status=404)
On 2013/11/12 03:20:02, iannucci wrote:
> I would move this error checking into the model, catch the exception and
return
> the Response then. i.e. this error checking will be done inline with the
> processing loop.

The problem is that the new issue is added to the datastore before
original_patch.make_inverted is called which is why I was doing this right at
the top. If we can figure out how to do batch commits only at the end then this
will be possible.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:890: models.Issue.kind(),
On 2013/11/12 03:20:02, iannucci wrote:
> you don't need the .kind() I don't think.

Without .kind() it throws-
Details: Expected a string kind as argument 1; received <class
'codereview.models.Issue'> (a PropertiedClass). [BadArgumentError]

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:918: private=original_issue.private,
On 2013/11/12 03:20:02, iannucci wrote:
> Should we check that the reverting user can see the original Issue? Otherwise
> you could get access to private patches by fishing revert URLs, since the user
> will own the new revert issue.

Not sure I understand, if a user could click on the 'Revert' button then it
means he/she could see it. Or do you mean if '/revert_patchset' is called
programatically?

> 
> We should also ensure that the original issue is closed.

I put in an assert at the start to make sure the issue is closed.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:922: issue.put()
On 2013/11/12 03:20:02, iannucci wrote:
> keep in mind that this .put() a) can throw and b) Will block.
> 
> I would look into buffering all the data to put, and then putting them all at
> once with db.put()
> https://developers.google.com/appengine/docs/python/datastore/functions#put

I tried this but creating the Content objects below does not work till its
parent exists in the Datastore. I get:

Details: Patch instance must have a complete key before it can be used as a
parent. [BadValueError]

> 
> It may also be worth using a transaction for this entire method so all the
> get()s and put()s happen at once. I'm not sure what will happen if you try to
> revert a patchset that's being deleted, for example.

I tried adding the '@db.transactional' decorator to the method. But the line
original_patches = original_patchset.patches failed with
'BadRequestError: queries inside transactions must have ancestors'

As it is right now, if a patchset is being deleted this method will throw an
exception and fail, which is probably ok but it would be nice to fail
gracefully.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:937: for original_patch in original_patches:
On 2013/11/12 03:20:02, iannucci wrote:
> I know that rietveld doesn't have very many examples of doing this sort of
thing
> correctly, but it would be a lot safer (to avoid exceeding deadline/running
out
> of memory) to fire off a bunch of taskqueue tasks to create these new Patch
> objects (including the base files). Then they could be treated entirely as
> normal content uploads. WDYT?
> 
> Actually, even just having the revert fire off a taskqueue task to generate
the
> inverted CL as you have done here would be safer, since the taskqueue can be
run
> by a beefier backend instance than the wimpy frontend. Then the protocol would
> be:
>   * POST /revert...
>   * 202 (Accepted: creating)
>   * poll: POST /revert
>   * 201 (Created: here's the new issue number)
> 
> This could be done by having a new field on the Patchset which is the revert
> status. Then on the first request, you could transactionally set the revert
> status from None to 'needed'. Then you could have a cron job which queries for
> Patchsets which need a revert, does the processing, and updates the status to
> the new CL number.
> 
> That would also make it easy to replace the revert button with a link to the
> revert CL for subsequent page loads (just look at the revert status when you
> render the patchset).

I see what you mean but I think that going to the new revert CL directly after
clicking on the button is a good user experience. Also I would guess 95% (or
maybe more) patchsets will not have any risk of exceeding deadlines or running
out of memory which means that the current approach should work fine for the
majority case.

It would be great for this view to detect looking at the patchset whether there
is a risk of running out of memory. This should also certainly be documented as
a shortcoming of the current implementation. The taskqueue approach could be a
good future optimization to handle these edge cases. WDYT?

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html#n...
templates/patchset.html:172: {%endif%}
On 2013/11/12 03:20:02, iannucci wrote:
> should we have a button here that's greyed out with the message of 'issue must
> be closed to revert'?

Done. The greyed out button looks kind of long but either way works for me.

https://codereview.appspot.com/22410044/diff/360001/app.yaml
File app.yaml (right):

https://codereview.appspot.com/22410044/diff/360001/app.yaml#newcode1
app.yaml:1: application: skia-codereview-staging2
Only for testing.

[iannucci, 2013-11-13 00:12:32]

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:877: ' inconvenience.', status=404)
Yeah, I know that's the way it's done. You can allocate ID's though to give all
the models full keys. I'm not sure if that's really better or not.

Since all the objects are rooted on the Issue, it should be pretty possible to
give them a common ancestor (and thus be able to commit them all as a
transaction).

Transactions have a max size of 10MB though :(. The new storage model I'm
working on should address that, so maybe we can just wait for that.

On 2013/11/12 22:00:38, rmistry wrote:
> On 2013/11/12 03:20:02, iannucci wrote:
> > I would move this error checking into the model, catch the exception and
> return
> > the Response then. i.e. this error checking will be done inline with the
> > processing loop.
> 
> The problem is that the new issue is added to the datastore before
> original_patch.make_inverted is called which is why I was doing this right at
> the top. If we can figure out how to do batch commits only at the end then
this
> will be possible.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:890: models.Issue.kind(),
On 2013/11/12 22:00:38, rmistry wrote:
> On 2013/11/12 03:20:02, iannucci wrote:
> > you don't need the .kind() I don't think.
> 
> Without .kind() it throws-
> Details: Expected a string kind as argument 1; received <class
> 'codereview.models.Issue'> (a PropertiedClass). [BadArgumentError]

bummer.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:918: private=original_issue.private,
On 2013/11/12 22:00:38, rmistry wrote:
> On 2013/11/12 03:20:02, iannucci wrote:
> > Should we check that the reverting user can see the original Issue?
Otherwise
> > you could get access to private patches by fishing revert URLs, since the
user
> > will own the new revert issue.
> 
> Not sure I understand, if a user could click on the 'Revert' button then it
> means he/she could see it. Or do you mean if '/revert_patchset' is called
> programatically?
> 

Right, if they explore the api programmatically (like by looking for gaps in
public issue numbers), then they could scan the range in between with revert
POSTs, and then when they hit a private issue, they'll see the full patchset,
content, description, etc.

> > 
> > We should also ensure that the original issue is closed.
> 
> I put in an assert at the start to make sure the issue is closed.

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html#n...
templates/patchset.html:172: {%endif%}
On 2013/11/12 22:00:38, rmistry wrote:
> On 2013/11/12 03:20:02, iannucci wrote:
> > should we have a button here that's greyed out with the message of 'issue
must
> > be closed to revert'?
> 
> Done. The greyed out button looks kind of long but either way works for me.

Oh, I was meaning to have it take the same text "Revert Issue", but have the
alt-text say why it's greyed out.

Also, you may want to say "Revert Patchset" since that's what it really is.

[rmistry, 2013-11-13 17:51:35]

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:868: 'patchset =', original_patchset.key())
On 2013/11/12 22:00:38, rmistry wrote:
> On 2013/11/12 03:20:02, iannucci wrote:
> > Direct datastore queries are the bads. Probably want to use
> > original_patchset.patches.
> 
> Done.
> 
> > 
> > The reason for this is I'm going to be changing the Issue schema soon, and
> some
> > things like Patches will no longer be entities in the datastore (they'll be
> > virtual objects returned by the new Patcheset model, etc.)
> > 
> > I think there's the possibility for this to run the appengine frontend out
of
> > memory as well. Right now rietveld hacks this by nulling out the data fields
> on
> > patches as it iterates. I would test this method by generating a huge
patchset
> > (maybe 100+ files each a couple hundred lines). Generating this patchset
> > programmatically would probably be a good idea. I /think/ the dev_appserver
> will
> > emit the proper OOMs, but I'm not sure.
> 
> Yes it will be great to verify what exactly happens. I am working on creating
a
> patch with 100s of files, will respond back here when I find out what it does.

Created a patchset with ~200 files
https://skia-codereview-staging2.appspot.com/90001/
It throws a DeadlineExceededError calling patchset.patches (after around 1 min).
I put in a try/except around the call to the model so that a useful error
message is generated for the user.

Then I created a patchset with 10 very large files:
https://skia-codereview-staging2.appspot.com/90002/
Reverting this gave:
Exceeded soft private memory limit with 156.922 MB after servicing 1 requests
total

We definitely need to check to detect what kind of patchsets could cause
crashes, I added a check to the view, let me know what you think.

Also, binary files do not seem to have this problem maybe because the actual
patch diffs are small. Tested in
https://skia-codereview-staging2.appspot.com/50003/

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:877: ' inconvenience.', status=404)
On 2013/11/13 00:12:32, iannucci wrote:
> Yeah, I know that's the way it's done. You can allocate ID's though to give
all
> the models full keys. I'm not sure if that's really better or not.
> 
> Since all the objects are rooted on the Issue, it should be pretty possible to
> give them a common ancestor (and thus be able to commit them all as a
> transaction).
> 
> Transactions have a max size of 10MB though :(. The new storage model I'm
> working on should address that, so maybe we can just wait for that.
> 
> On 2013/11/12 22:00:38, rmistry wrote:
> > On 2013/11/12 03:20:02, iannucci wrote:
> > > I would move this error checking into the model, catch the exception and
> > return
> > > the Response then. i.e. this error checking will be done inline with the
> > > processing loop.
> > 
> > The problem is that the new issue is added to the datastore before
> > original_patch.make_inverted is called which is why I was doing this right
at
> > the top. If we can figure out how to do batch commits only at the end then
> this
> > will be possible.
> 

I allocated IDs and now can buffer all the pending Issue, PatchSet, Patches and
Contents and then commit them at the end. I am committing them in a separate
method with the '@db.transactional' decorator. I tried to put the decorator on
the revert_patchset method but that fails at this line:
original_patches = original_patchset.patches

I am not sure how to lock the original issue but maybe what I have now is what
you had in mind, let me know.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:890: models.Issue.kind(),
On 2013/11/13 00:12:32, iannucci wrote:
> On 2013/11/12 22:00:38, rmistry wrote:
> > On 2013/11/12 03:20:02, iannucci wrote:
> > > you don't need the .kind() I don't think.
> > 
> > Without .kind() it throws-
> > Details: Expected a string kind as argument 1; received <class
> > 'codereview.models.Issue'> (a PropertiedClass). [BadArgumentError]
> 
> bummer.

Acknowledged.

https://codereview.appspot.com/22410044/diff/340001/codereview/views.py#newco...
codereview/views.py:918: private=original_issue.private,
On 2013/11/13 00:12:32, iannucci wrote:
> On 2013/11/12 22:00:38, rmistry wrote:
> > On 2013/11/12 03:20:02, iannucci wrote:
> > > Should we check that the reverting user can see the original Issue?
> Otherwise
> > > you could get access to private patches by fishing revert URLs, since the
> user
> > > will own the new revert issue.
> > 
> > Not sure I understand, if a user could click on the 'Revert' button then it
> > means he/she could see it. Or do you mean if '/revert_patchset' is called
> > programatically?
> > 
> 
> Right, if they explore the api programmatically (like by looking for gaps in
> public issue numbers), then they could scan the range in between with revert
> POSTs, and then when they hit a private issue, they'll see the full patchset,
> content, description, etc.
> 
> > > 
> > > We should also ensure that the original issue is closed.
> > 
> > I put in an assert at the start to make sure the issue is closed.
> 

Yes this makes sense, thanks for the catch. Added an assert at the top.

[rmistry, 2013-11-13 18:13:52]

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/22410044/diff/340001/templates/patchset.html#n...
templates/patchset.html:172: {%endif%}
On 2013/11/13 00:12:32, iannucci wrote:
> On 2013/11/12 22:00:38, rmistry wrote:
> > On 2013/11/12 03:20:02, iannucci wrote:
> > > should we have a button here that's greyed out with the message of 'issue
> must
> > > be closed to revert'?
> > 
> > Done. The greyed out button looks kind of long but either way works for me.
> 
> Oh, I was meaning to have it take the same text "Revert Issue", but have the
> alt-text say why it's greyed out.
> 
> Also, you may want to say "Revert Patchset" since that's what it really is.

Done.

[M-A, 2013-11-13 21:57:50]

A few style nits.

https://codereview.appspot.com/22410044/diff/450001/app.yaml
File app.yaml (right):

https://codereview.appspot.com/22410044/diff/450001/app.yaml#newcode1
app.yaml:1: application: skia-codereview-staging
Please don't commit this.

https://codereview.appspot.com/22410044/diff/450001/codereview/exceptions.py
File codereview/exceptions.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/exceptions.py#...
codereview/exceptions.py:24: 
2 lines

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches...
codereview/invert_patches.py:24: COPIED_AND_MODIFIED_STATUS = 'A +'
I think it's svn version specific, it could also be 'A+'. Lovely. But I could be
as well hallucinating.

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches...
codereview/invert_patches.py:189: 
remove line

https://codereview.appspot.com/22410044/diff/450001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/models.py#newc...
codereview/models.py:926: def make_inverted(self, patchset):
I don't really like having it be a member of the class. I'd prefer it to be a
standalone function. Models are meant to be simpler (which is not generally the
case in Rietveld).

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:17: import md5
import hashlib

md5 is the old deprecated module.

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:223: if original_patch.status != 'A +':
Why create constants if you are going to spread the string in the code?

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:276: 
remove line

https://codereview.appspot.com/22410044/diff/450001/tests/test_invert_patches.py
File tests/test_invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/450001/tests/test_invert_patches...
tests/test_invert_patches.py:269: def
test_get_inverted_patch_for_binary_copy_and_modify_with_existing_mode(self):
80 cols, so:
def test_get_inverted_patch_for_binary_copy_and_modify_with_existing_mode(
    self):

[rmistry, 2013-11-14 13:20:04]

https://codereview.appspot.com/22410044/diff/450001/app.yaml
File app.yaml (right):

https://codereview.appspot.com/22410044/diff/450001/app.yaml#newcode1
app.yaml:1: application: skia-codereview-staging
On 2013/11/13 21:57:51, M-A wrote:
> Please don't commit this.

Reverted.

https://codereview.appspot.com/22410044/diff/450001/codereview/exceptions.py
File codereview/exceptions.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/exceptions.py#...
codereview/exceptions.py:24: 
On 2013/11/13 21:57:51, M-A wrote:
> 2 lines

Done.

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches...
codereview/invert_patches.py:24: COPIED_AND_MODIFIED_STATUS = 'A +'
On 2013/11/13 21:57:51, M-A wrote:
> I think it's svn version specific, it could also be 'A+'. Lovely. But I could
be
> as well hallucinating.

Yea, I am not sure about SVN, this is only for Git. In the future if we support
SVN we could add additional constants here.

Though, running "SELECT * FROM Patch WHERE status = 'A+';" in
chromiumcodereview-hr got 0 hits but many for 'A +'.

https://codereview.appspot.com/22410044/diff/450001/codereview/invert_patches...
codereview/invert_patches.py:189: 
On 2013/11/13 21:57:51, M-A wrote:
> remove line

Done.

https://codereview.appspot.com/22410044/diff/450001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/models.py#newc...
codereview/models.py:926: def make_inverted(self, patchset):
On 2013/11/13 21:57:51, M-A wrote:
> I don't really like having it be a member of the class. I'd prefer it to be a
> standalone function. Models are meant to be simpler (which is not generally
the
> case in Rietveld).

I tried to be consistent with the complexity in this module :)

I like make_inverted here because it is handy to say:
patch.make_inverted(new_patchset)
Other we would have to do:
invert_patches.make_inverted(original_patch, new_patchset)
which is not terrible I suppose.

If you strongly prefer moving it out of this Model I can do it.

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:17: import md5
On 2013/11/13 21:57:51, M-A wrote:
> import hashlib
> 
> md5 is the old deprecated module.

Done.

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:223: if original_patch.status != 'A +':
On 2013/11/13 21:57:51, M-A wrote:
> Why create constants if you are going to spread the string in the code?

Whoops. Fixed.

https://codereview.appspot.com/22410044/diff/450001/codereview/revert_patchse...
codereview/revert_patchset.py:276: 
On 2013/11/13 21:57:51, M-A wrote:
> remove line

Done.

https://codereview.appspot.com/22410044/diff/450001/tests/test_invert_patches.py
File tests/test_invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/450001/tests/test_invert_patches...
tests/test_invert_patches.py:269: def
test_get_inverted_patch_for_binary_copy_and_modify_with_existing_mode(self):
On 2013/11/13 21:57:51, M-A wrote:
> 80 cols, so:
> def test_get_inverted_patch_for_binary_copy_and_modify_with_existing_mode(
>     self):

Done.

[M-A, 2013-11-14 14:12:31]

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:49: self._status = status
Frankly, I wouldn't trust the status at all and just figure it out from the diff
itself. Then generate a new status based on the diff header.

There are been cases when the status was completely misleading.

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:99: def _get_inverted_patch_headers(self):
Personally, I would split the code to be more readable:
- split the header from the chunks
- invert the header
- invert the chunks

Keeping the whole thing as a big blob in self._patch_text makes the code more
brittle IMHO.

See depot_tools/patch.py as a way to parse it.

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
codereview/revert_patchset.py:273: @db.transactional
It's really a huge transaction. It'll likely fail for anything non trivial (e.g.
slightly larger patches). I'd recommend getting away from that.

https://codereview.appspot.com/22410044/diff/470001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/views.py#newco...
codereview/views.py:1258: msg = make_message(request, request.issue,
request.POST.get('message', ''),
[optional] You could create a CL with just that to make this patch slightly
simpler.

[rmistry, 2013-11-14 15:05:04]

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:49: self._status = status
On 2013/11/14 14:12:31, M-A wrote:
> Frankly, I wouldn't trust the status at all and just figure it out from the
diff
> itself. Then generate a new status based on the diff header.
> 
> There are been cases when the status was completely misleading.

Yes, I saw a misleading status during pure permission changes, it shows up as
'A', but other than that I did not really notice any other misleading statuses
(that does not mean they do not exist).

Agreed that it is better to infer what the status should be but this was harder
to do. I started off in this direction in the initial implementation but ended
up with some messy logic. Things are cleaner if the code trusts that the status
is accurate (I had identified only one case where it was not accurate but the
patch still applied successfully).

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:99: def _get_inverted_patch_headers(self):
On 2013/11/14 14:12:31, M-A wrote:
> Personally, I would split the code to be more readable:
> - split the header from the chunks
> - invert the header
> - invert the chunks
> 
> Keeping the whole thing as a big blob in self._patch_text makes the code more
> brittle IMHO.
> 
> See depot_tools/patch.py as a way to parse it.

I see a few things I can borrow from patch.py, changes coming soon...

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
codereview/revert_patchset.py:273: @db.transactional
On 2013/11/14 14:12:31, M-A wrote:
> It's really a huge transaction. It'll likely fail for anything non trivial
(e.g.
> slightly larger patches). I'd recommend getting away from that.

In my experiments it seems to handle 'larger' patches fine:
https://skia-codereview-staging2.appspot.com/40003

but I do believe removing 'db.transaction' is probably going to cause less
failures. Removed it.

[M-A, 2013-11-14 17:30:55]

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:49: self._status = status
On 2013/11/14 15:05:04, rmistry wrote:
> Yes, I saw a misleading status during pure permission changes, it shows up as
> 'A', but other than that I did not really notice any other misleading statuses
> (that does not mean they do not exist).
> 
> Agreed that it is better to infer what the status should be but this was
harder
> to do. I started off in this direction in the initial implementation but ended
> up with some messy logic. Things are cleaner if the code trusts that the
status
> is accurate (I had identified only one case where it was not accurate but the
> patch still applied successfully).

I'll let Robbie decide about that.

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
codereview/revert_patchset.py:273: @db.transactional
On 2013/11/14 15:05:04, rmistry wrote:
> but I do believe removing 'db.transaction' is probably going to cause less
> failures. Removed it.

Transactions only work if they are in the same entity group. Otherwise you want
an XG transaction.
https://developers.google.com/appengine/docs/python/datastore/transactions

[rmistry, 2013-11-15 19:32:50]

Added logic to calculate the patch status from the diff header.
Also, added support for 'git mv' patches.

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:49: self._status = status
On 2013/11/14 17:30:55, M-A wrote:
> On 2013/11/14 15:05:04, rmistry wrote:
> > Yes, I saw a misleading status during pure permission changes, it shows up
as
> > 'A', but other than that I did not really notice any other misleading
statuses
> > (that does not mean they do not exist).
> > 
> > Agreed that it is better to infer what the status should be but this was
> harder
> > to do. I started off in this direction in the initial implementation but
ended
> > up with some messy logic. Things are cleaner if the code trusts that the
> status
> > is accurate (I had identified only one case where it was not accurate but
the
> > patch still applied successfully).
> 
> I'll let Robbie decide about that.

I went ahead and added logic to calculate the status from the diff header in a
standalone function, I covered all the cases in my tests.
Also restructured this module a little.

https://codereview.appspot.com/22410044/diff/470001/codereview/invert_patches...
codereview/invert_patches.py:99: def _get_inverted_patch_headers(self):
On 2013/11/14 15:05:04, rmistry wrote:
> On 2013/11/14 14:12:31, M-A wrote:
> > Personally, I would split the code to be more readable:
> > - split the header from the chunks
> > - invert the header
> > - invert the chunks
> > 
> > Keeping the whole thing as a big blob in self._patch_text makes the code
more
> > brittle IMHO.
> > 
> > See depot_tools/patch.py as a way to parse it.
> 
> I see a few things I can borrow from patch.py, changes coming soon...

Borrowed a few things (and a few ideas) from patch.py, PTAL.

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/470001/codereview/revert_patchse...
codereview/revert_patchset.py:273: @db.transactional
On 2013/11/14 17:30:55, M-A wrote:
> On 2013/11/14 15:05:04, rmistry wrote:
> > but I do believe removing 'db.transaction' is probably going to cause less
> > failures. Removed it.
> 
> Transactions only work if they are in the same entity group. Otherwise you
want
> an XG transaction.
> https://developers.google.com/appengine/docs/python/datastore/transactions

What is your recommendation? To have XG transactions or to not have transactions
at all since they may fail?

[rmistry, 2013-11-19 18:53:16]

Friendly ping.

[rmistry, 2013-11-21 22:36:06]

Friendly Ping.

[M-A, 2013-11-22 18:40:09]

lgtm

https://codereview.appspot.com/22410044/diff/550001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/550001/codereview/revert_patchse...
codereview/revert_patchset.py:272: 
2 lines

[rmistry, 2013-11-22 18:49:23]

https://codereview.appspot.com/22410044/diff/550001/codereview/revert_patchse...
File codereview/revert_patchset.py (right):

https://codereview.appspot.com/22410044/diff/550001/codereview/revert_patchse...
codereview/revert_patchset.py:272: 
On 2013/11/22 18:40:10, M-A wrote:
> 2 lines

Done.

[iannucci, 2013-11-22 19:07:47]

Dang, M-A beat me to it. I didn't really have many comments anyway. LGTM as
well.

https://codereview.appspot.com/22410044/diff/550001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/550001/codereview/invert_patches...
codereview/invert_patches.py:100: self._status =
get_patch_status(self._diff_header)
why have this be a function instead of a @staticmethod?

[rmistry, 2013-11-22 21:11:15]

https://codereview.appspot.com/22410044/diff/550001/codereview/invert_patches.py
File codereview/invert_patches.py (right):

https://codereview.appspot.com/22410044/diff/550001/codereview/invert_patches...
codereview/invert_patches.py:100: self._status =
get_patch_status(self._diff_header)
On 2013/11/22 19:07:48, iannucci wrote:
> why have this be a function instead of a @staticmethod?

Changed to @staticmethod. Done.

[rmistry, 2013-11-22 21:49:58]

Submitted, thanks Robbie and M-A for the detailed review.

I deployed it to https://1212-7e4874c31aa8.chromiumcodereview-hr.appspot.com/
Will test on Monday and give #chromium a heads-up (as recommended by Robbie)
before making it the Default.