vijay <vijay_at_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
Received on 2013-02-27 18:15:32 CET