On Tue, Apr 29, 2008 at 7:18 PM, Blair Zajac <blair_at_orcaware.com> wrote:
> 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
>
I have just submitted the split up version of this patch. Hope it is
what you had in mind.
--
"Beware of spyware. If you can, use the Firefox browser." - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-01 05:32:00 CEST