Re: [PATCH] Add '--drop-all-empty-revs' to svndumpfilter include/exclude

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