You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by vijay <vi...@collab.net> on 2013/02/26 10:46:52 UTC
[PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Hi,
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
Thanks & Regards,
Vijayaguru
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by Julian Foad <ju...@btopenworld.com>.
vijay <vi...@collab.net> wrote:
> On Wednesday 27 February 2013 10:44 PM, Julian Foad wrote:
>>> + write_out_rev = (! rb->had_dropped_nodes) ? TRUE : FALSE;
>>
>> No, I don't mean that. [...] simply use the boolean "not" operator
>> [...] like this:
>>
>> write_out_rev = ! rb->had_dropped_nodes;
>
> Sorry, I misunderstood it. Thanks for your detailed explanation.
>
> Attached the updated patch and log message.
Thanks for tweaking that.
Seeing no other concerns from anyone, I committed this in r1451462.
Thanks for the enhancement!
- Julian
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by vijay <vi...@collab.net>.
On Wednesday 27 February 2013 10:44 PM, Julian Foad wrote:
>
>> + write_out_rev = (! rb->had_dropped_nodes) ? TRUE : FALSE;
>
> No, I don't mean that. "? TRUE : FALSE" is completely redundant, and I want us to avoid that sort of redundancy.
>
> "had_nodes_dropped" is a boolean value -- that is, a yes/no, true/false flag.
>
> "write_out_rev" is a boolean value -- true/false -- as well.
>
> You want "write_out_rev" to be true if "had_dropped_nodes" is false and false if it is true. I'm saying simply use the boolean "not" operator to say "use the opposite truth value", like this:
>
> write_out_rev = ! rb->had_dropped_nodes;
>
> so the statement reads as "write-out-rev = not had dropped nodes", instead of "x ? y : z" which says "if X is true then Y else Z".
Sorry, I misunderstood it. Thanks for your detailed explanation.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by Julian Foad <ju...@btopenworld.com>.
vijay <vi...@collab.net> wrote:
> On Wednesday 27 February 2013 11:10 AM, Julian Foad wrote:
>>> @@ -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.
>
> I have updated the code and comment slightly.
>
>>> */
>>> - 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".
>
> Done. I will keep it in mind.
> + write_out_rev = (! rb->had_dropped_nodes) ? TRUE : FALSE;
No, I don't mean that. "? TRUE : FALSE" is completely redundant, and I want us to avoid that sort of redundancy.
"had_nodes_dropped" is a boolean value -- that is, a yes/no, true/false flag.
"write_out_rev" is a boolean value -- true/false -- as well.
You want "write_out_rev" to be true if "had_dropped_nodes" is false and false if it is true. I'm saying simply use the boolean "not" operator to say "use the opposite truth value", like this:
write_out_rev = ! rb->had_dropped_nodes;
so the statement reads as "write-out-rev = not had dropped nodes", instead of "x ? y : z" which says "if X is true then Y else Z".
>>> + 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.
>
> Done. I have updated the test.
Great, thanks.
>> Everything else looks fine, from just a read-through review.
>
> Thanks Julian. As cmpilato said, I have updated the usage message and
> code docs to have a note about special-casing revision 0.
>
> Attached the updated patch and log message.
That all looks good.
- Julian
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by vijay <vi...@collab.net>.
On Wednesday 27 February 2013 11:10 AM, Julian Foad wrote:
>> @@ -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.
I have updated the code and comment slightly.
>
>> */
>> - 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".
Done. I will keep it in mind.
>
>> + 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.
>
Done. I have updated the test.
> Everything else looks fine, from just a read-through review.
>
Thanks Julian. As cmpilato said, I have updated the usage message and
code docs to have a note about special-casing revision 0.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 02/26/2013 10:08 AM, vijay wrote:
> If the dump was taken from a mirror repo, r0 might have some information
> like 'svn:sync-from-url', 'svn:sync-from-uuid', etc.
> I thought that '--drop-all-empty-revs' should not just drop r0 considering
> as an empty revision.
That's seems fair. The load logic already special-cases revision 0, never
treating it as a "real" new revision to be created in the target repository.
It's as consistent to special-case revision 0 here, too, as it would be to
not do so. But the usage message and docs should reveal this nuance, in my
opinion.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by vijay <vi...@collab.net>.
On Tuesday 26 February 2013 07:47 PM, Philip Martin wrote:
> vijay <vi...@collab.net> writes:
>
>> /* 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.
>> */
>> - 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;
>> + else if (rb->pb->drop_all_empty_revs)
>> + write_out_rev = FALSE;
>> + else
>> + write_out_rev = TRUE;
>> +
>> + if (write_out_rev)
>> {
>
> You have added the rev_orig == 0 check so that means that
> --drop-all-empty-revs doesn't drop r0. Is there a reason for that
> behaviour?
>
If the dump was taken from a mirror repo, r0 might have some information
like 'svn:sync-from-url', 'svn:sync-from-uuid', etc.
I thought that '--drop-all-empty-revs' should not just drop r0
considering as an empty revision.
Please correct me if I am wrong.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> /* 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.
> */
> - 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;
> + else if (rb->pb->drop_all_empty_revs)
> + write_out_rev = FALSE;
> + else
> + write_out_rev = TRUE;
> +
> + if (write_out_rev)
> {
You have added the rev_orig == 0 check so that means that
--drop-all-empty-revs doesn't drop r0. Is there a reason for that
behaviour?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download