Troy Curtis Jr wrote:
> On Tue, Apr 29, 2008 at 5:22 PM, Blair Zajac <blair_at_orcaware.com> wrote:
>> Hi Tony,
>>
>> It would be nice to split these patches up into separate logical changes;
>> it would make the review easier. For example, the change to split
>> split_arg_at_peg_revision() out should be a separate patch would leave the
>> logical changes in the successive patch.
>>
>> The reviews would probably be faster this way also, less to keep track of.
>>
>> Regards,
>> Blair
>>
>>
>
> s/Tony/Troy/...I get that all the time :)
My apology for that. I hate it when people get my name wrong, or, are you
related to Pat Sajak :)
> I'm not surprised that it was suggested that I split the patch, but
> where you are proposing does seem odd to me. Perhaps it's because
> it's too large a patch to quickly review :)
>
> I had initially thought that I should include the fix for a failed
> assert when a user entered a non-canonical repo root relative target,
> but then decided it fit with what I was already doing. Guess not.
>
> But I'm curious about your suggestion. split_arg_at_peg_revision()
> was created to factor out (move) common code from inside
> svn_client_args_to_target_array() and svn_opt_args_to_target_array3().
> Are you proposing that I create the common code (copy) submit that as
> a patch. Then in a follow up actually change the code in the
> *to_target_array() functions to use the new function? I must admit I
> don't understand that.
No, one patch for creating the new method and using it in *to_target_array()
since that's a lot of lines of diffs and then another patch for the logic
changes to correct the code.
If the refactoring also changed the logic of the code being moved, then that's
not a good idea, since it's hard to review in the diff emails, a person would
have to go line by line to see what was changed. I'm not saying that's happened
here, since I didn't fully review the patch.
Regards,
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-30 02:18:29 CEST