vijay <vi...@collab.net> wrote:
> Recently, I had a chance to look at a dump file with lot of empty revisions.
> The
> dump file was taken by syncing a sub-directory of a remote repository using
> svnsync. I decided to remove all the empty revisions from the dump file. But
> 'svndumpfilter include/exclude --drop-empty-revs' was of no use, since
> it removes only the revisions emptied by filtering process.
>
> When I checked with the issue tracker, there is already one open issue[1] for
> this problem. There was a mailing list discussion[2] which proposes to change
> the behavior of '--drop-empty-revs' to drop *all* the empty revisions
> instead of removing the ones emptied by filtering process.
>
> As cmpilato said in the thread, the change in behavior would break our
> compatibility promises and new command-line option
> '--drop-all-empty-revs' would help to resolve this issue.
>
> This patch introduces a new option '--drop-all-empty-revs' to
> svndumpfilter include/exclude.
>
> Attached the patch and log message. Please share your thoughts.
>
> [1] http://subversion.tigris.org/issues/show_bug.cgi?id=3681
> [2] http://svn.haxx.se/dev/archive-2010-07/0083.shtml
[...]
> @@ -439,14 +442,23 @@
>
> /* write out the revision */
> /* Revision is written out in the following cases:
> - 1. No --drop-empty-revs has been supplied.
> - 2. --drop-empty-revs has been supplied,
> - but revision has not all nodes dropped
> - 3. Revision had no nodes to begin with.
> + 1. If the revision has nodes or it is revision 0.
> + 2. --drop-empty-revs has been supplied,
> + but revision has not all nodes dropped.
> + 3. No --drop-all-empty-revs has been supplied.
> + 4. If no --drop-empty-revs or --drop-all-empty-revs have been supplied,
> + write out the revision which has no nodes to begin with.
This comment does not sound completely right: the rev is not always written out
in case 3.
> */
> - if (rb->has_nodes
> - || (! rb->pb->drop_empty_revs)
> - || (! rb->had_dropped_nodes))
> + if (rb->has_nodes || (rb->rev_orig == 0))
> + write_out_rev = TRUE;
> + else if (rb->pb->drop_empty_revs)
> + write_out_rev = rb->had_dropped_nodes ? FALSE : TRUE;
As a matter of style, please don't write "x ? FALSE : TRUE", but rather "! x".
> + else if (rb->pb->drop_all_empty_revs)
> + write_out_rev = FALSE;
> + else
> + write_out_rev = TRUE;
> +
> + if (write_out_rev)
> {
In your new test for this option, you test with --renumber-revs; could you also
test without that? That seems worth testing too, in my opinion, if it's as
easy to do as it looks.
Everything else looks fine, from just a read-through review.
- Julian
