Re: [SVNMERGE][PATCH] svnmerge rollback

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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org