When reverting, just reverse merge the entire revision.

When reverting, just reverse merge the entire revision.

Since we already checked out all files that were touched in the revision
to revert, it should just work[tm].

BUG=64643
R=agable@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org
TEST=tested manually that svn merge -r (without -N) reverts directory deletes

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219094

[agable, 2013-08-22 09:15:14]

This pretty much lgtm. Using SVN's built-in capabilities is better than
hardcoding something complex, so I have even a further simplification to propose
in the comment.

https://codereview.chromium.org/22814005/diff/1/drover.py
File drover.py (right):

https://codereview.chromium.org/22814005/diff/1/drover.py#newcode278
drover.py:278: command = ('svn merge -r ' + str(revision) + ":" +
str(revision-1) +
This could also simply be
'svn merge -c -' + str(revision)
or
'svn merge -c -%d %s .' % (revision, url)

[Paweł Hajdan Jr., 2013-08-22 18:13:40]

https://codereview.chromium.org/22814005/diff/6001/drover.py
File drover.py (right):

https://codereview.chromium.org/22814005/diff/6001/drover.py#newcode278
drover.py:278: command = ('svn merge -c -%d %s .' % (revision, url))
Drive-by: have you tested revert of a revert? And revert of revert of revert and
so on?

svn merge adds metadata to the repo, and so e.g. second merge of the same thing
would be a no-op. I recommend forcing no ancestry checking here.

[M-A Ruel, 2013-08-22 18:18:49]

IIRC, Anthony came up with this logic. There were issues like performance that I
don't know if they are still a blocker.

[jochen (gone - plz use gerrit), 2013-08-22 18:21:01]

On 2013/08/22 18:13:40, Paweł Hajdan Jr. wrote:
> https://codereview.chromium.org/22814005/diff/6001/drover.py
> File drover.py (right):
> 
> https://codereview.chromium.org/22814005/diff/6001/drover.py#newcode278
> drover.py:278: command = ('svn merge -c -%d %s .' % (revision, url))
> Drive-by: have you tested revert of a revert? And revert of revert of revert
and
> so on?

I'll try that. However, we've used svn merge before to revert, just one
directory at a time instead of all of it, so I don't think there's much of a
difference

> svn merge adds metadata to the repo, and so e.g. second merge of the same
thing
> would be a no-op. I recommend forcing no ancestry checking here.

that amounts to adding --ignore-ancestry, right?

[M-A Ruel, 2013-08-22 18:23:34]

On 2013/08/22 18:21:01, jochen wrote:
> On 2013/08/22 18:13:40, Paweł Hajdan Jr. wrote:
> > svn merge adds metadata to the repo, and so e.g. second merge of the same
> thing
> > would be a no-op. I recommend forcing no ancestry checking here.
> 
> that amounts to adding --ignore-ancestry, right?

Likely.

lgtm, at worst, it's easy to revert (pun intended).

[laforge, 2013-08-22 18:29:51]

On 2013/08/22 18:21:01, jochen wrote:
> On 2013/08/22 18:13:40, Paweł Hajdan Jr. wrote:
> > https://codereview.chromium.org/22814005/diff/6001/drover.py
> > File drover.py (right):
> > 
> > https://codereview.chromium.org/22814005/diff/6001/drover.py#newcode278
> > drover.py:278: command = ('svn merge -c -%d %s .' % (revision, url))
> > Drive-by: have you tested revert of a revert? And revert of revert of revert
> and
> > so on?
> 
> I'll try that. However, we've used svn merge before to revert, just one
> directory at a time instead of all of it, so I don't think there's much of a
> difference
> 
> > svn merge adds metadata to the repo, and so e.g. second merge of the same
> thing
> > would be a no-op. I recommend forcing no ancestry checking here.
> 
> that amounts to adding --ignore-ancestry, right?

It's been a few years, but I recall there being definite performance
implications (i.e. merge times taking an order of magnitude longer).  This may
be acceptable logic for the revert path, but unless something has changed in the
svn client, it's not particularly ideal for merges.

--ignore-ancestry needs to be in place for branch merges (otherwise most merges
won't take)... learned that the painfully hard way in Chrome 3-5.

[jochen (gone - plz use gerrit), 2013-08-22 20:15:07]

added --ignore-ancestry

[Paweł Hajdan Jr., 2013-08-22 20:36:28]

LGTM, thanks!

[jochen (gone - plz use gerrit), 2013-08-22 20:40:21]

I tried reverting r218441 with and without my patch.

Without my patch, it took 2 minutes 56 secs, with my patch it took 22 secs...

[jochen (gone - plz use gerrit), 2013-08-22 20:43:42]

Committed patchset #3 manually as r219094 (presubmit successful).