[SVNMERGE][PATCH] svnmerge rollback

[Giovanni Bajo]

David James wrote:

> Hi Madan,
>
> This patch looks good except for one small problem in the test suite:
>   self.launch("touch newfile")
>
> 'touch' isn't available on Windows. So we should do this instead:
>    open("newfile","w").close()
>
> Other than this issue, the patch looks excellent. I am very impressed
> with the extensive suite of new tests.
>
> Since this is a major new feature, I'd like to wait a few days to see
> if anyone else has feedback before we commit. Giovanni?

The patch is absolutely wonderful and clean indeed. Many thanks Madan!

I have just a couple of very minor comments, as I wasn't able to spot any
real problem:

>>+    # Check branch directory is ready for being modified
>>+    check_dir_clean(branch_dir)
>>+
>>+    # Make sure the revision arguments are present
>>+    if not opts["revision"]:
>>+        error("The '-r' option is mandatory for rollback")

Can you check for the revision argument *before* checking for the clean wc?
I guess we should always attempt to detect command line errors earlier later
than later.

>>+    # At which revision was the dest created?
>>+    oldest_src_rev = get_created_rev(opts["head-url"])
>>+    src_pre_exist_range = RevisionSet("1-%d" % oldest_src_rev)

Is this test really necessary? Doesn't the merge command just fail if you
try reverting a revision at which the URL did not exist?

>> +    if len(revs) == 0:

if revs:

>>+    # make sure theres some revision to rollback
>>+    if len(revs) == 0:
>>+        warn("Nothing to rollback in revision range r%s" %
opts["revision"])
>>+        return

I'd rather this be a report for now, and then have a separate commit pass
which introduces warn() and uses it consistenly across the program (once the
semantic is clear).


>> +    if len(revs & src_pre_exist_range) != 0:

if revs & src_pre_exist_range:


After these things are sorted out, you get my +1 :)
-- 
Giovanni Bajo