You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Troy Curtis Jr <tr...@gmail.com> on 2008/04/29 22:11:44 UTC
[PATCH] Fixes for CLI repository root relative url support
Here are my fixes for the issues brought up last night and earlier today. The
tests are not in C because they require a working copy and repository
structure like that provided in cmdline.
[[[
Correctly handle non-canonical targets and targets containing peg revisions
while parsing repository root relative urls.
* subversion/libsvn_subr/opt.c
(split_arg_at_peg_revision): New function.
(svn_opt__arg_canonicalize_url,
svn_opt__arg_canonicalize_path): Allow the input argument to contain a peg
revision specifier, and preserve it in the output utilizing
split_arg_at_peg_revision().
(svn_opt_args_to_target_array3): Remove the peg revision preserving logic,
relying instead on the new peg revision support in
svn_opt__arg_canonicalize_path() and svn_opt__arg_canonicalize_url().
* subversion/include/private/svn_opt_private.h
(svn_opt__arg_canonicalize_url,
svn_opt__arg_canonicalize_path): Update the doc string to reflect the fact
that these functions can handle a peg revision in the input argument.
* subversion/libsvn_client/cmdline.c
(svn_client_args_to_target_array): Remove the peg revision preserving logic,
relying instead on the new peg support in svn_opt__arg_canonicalize_path()
and svn_opt__arg_canonicalize_url().
(resolve_repos_relative_url): Fix an assertion failure in svn_path_join() by
replacing it with apr_pstrcat() so that there is not a requirement that the
input arguments be canonical. Also updated the doc string to reflect this.
* subversion/tests/cmdline/basic_tests.py
(basic_relative_url_non_canonical): New test function.
(basic_relative_url_with_peg_revisions): New test functions.
(test_list): Call the new test functions.
]]]
--
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/)
Re: [PATCH] Fixes for CLI repository root relative url support
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, Apr 29, 2008 at 7:18 PM, Blair Zajac <bl...@orcaware.com> wrote:
> Troy Curtis Jr wrote:
>
> > On Tue, Apr 29, 2008 at 5:22 PM, Blair Zajac <bl...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixes for CLI repository root relative url support
Posted by Blair Zajac <bl...@orcaware.com>.
Troy Curtis Jr wrote:
> On Tue, Apr 29, 2008 at 5:22 PM, Blair Zajac <bl...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixes for CLI repository root relative url support
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, Apr 29, 2008 at 5:22 PM, Blair Zajac <bl...@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 :)
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.
Troy
>
> Troy Curtis Jr wrote:
>
> > Here are my fixes for the issues brought up last night and earlier today.
> The
> > tests are not in C because they require a working copy and repository
> > structure like that provided in cmdline.
> >
> > [[[
> > Correctly handle non-canonical targets and targets containing peg
> revisions
> > while parsing repository root relative urls.
> >
>
--
"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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fixes for CLI repository root relative url support
Posted by Blair Zajac <bl...@orcaware.com>.
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
Troy Curtis Jr wrote:
> Here are my fixes for the issues brought up last night and earlier today. The
> tests are not in C because they require a working copy and repository
> structure like that provided in cmdline.
>
> [[[
> Correctly handle non-canonical targets and targets containing peg revisions
> while parsing repository root relative urls.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org