You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Prabhu Gnana Sundar <pr...@collab.net> on 2010/12/08 13:58:35 UTC
[PATCH] fix a compilation warning
Hi,
I have attached a patch with a minor change which fixes a compiler
warning.
Thanks and regards
Prabhu
Re: [PATCH] fix a compilation warning
Posted by Philip Martin <ph...@wandisco.com>.
Prabhu Gnana Sundar <pr...@collab.net> writes:
> So I can say my patch holds good.
I agree, r1044016.
--
Philip
Re: [PATCH] fix a compilation warning
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Hi all,
I am sorry for the delayed reply and the carelessness. Initially, I did
*not* have much context to comment on the patch.
Just because the svn_client_relocate() was *deprecated* and it uses
svn_client_relocate2() to implement itself in deprecated.c by hardcoding
ignore_externals as TRUE, I thought that the patch would solve the
compilation warning.
But now, with help of Kamesh I got to know some context of how the code
goes and had some experimentation with one of our internal server which
allows "checkout" only through "https".
Here is the experiment:
1. checked out a directory from a repo using "https".
2. added a property "svn:externals"
"ext_dir1 ....url_to_other_dir_in_same_repo'.
3. now did "svn up" and got 'ext_dir1'
4. now again added a property "svn:externals"
"ext_dir2 ....url_to_some_other_dir_in_same_repo' on ext_dir1
Thereby acheiving an "external" from an "external",
direct--> external --> external
5. did "svn up" and got 'ext_dir1/ext_dir2'
6. Now, manually updated the wc.db(update repository set root='http...
where id=1') of all these three to point to "http"
(so that I can trigger the code, proving the redirection of URL to
"https" to get the "corrected_url")
Now just tried to catch the "svn_client__update_internal" in the GDB
debgger,
it was clear that
"subversion/libsvn_client/externals.c:switch_dir_external()" does the
relocation for all ext_dir1 and ext_dir2 'ignore-externals=FALSE'.
My patch gets exercised only for top-level target which is supposed to
relocate only itself not any of its deep down external items.
So I can say my patch holds good.
And as a side-note, I was able to observe that upon these
experimentation, the "repository" table in wc.db did not update the
existing record, rather inserted a new record with "https".
I doubt if this is the actual behaviour....?
On Wed, 2010-12-08 at 17:29 +0200, Daniel Shahaf wrote:
> FWIW, my concerns here are:
>
> * svn_client_relocate{,2} have the same signature. This might be
> confusing sometimes. (but probably should be left alone)
>
> * svn_client_relocate2() takes an IGNORE_EXTERNALS parameter. Should
> we pass TRUE always to that parameter
yes it should always be TRUE to this parameter as per my above
experiment/explanation.
> , or should we pass the
> identically-named parameter of the calling function? (the calling
> function *happens* to have an appropriately-named parameter, but
> I haven't checked its semantics)
>
> Daniel
Thanks and regards
Prabhu
Re: [PATCH] fix a compilation warning
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> * svn_client_relocate2() takes an IGNORE_EXTERNALS parameter. Should
> we pass TRUE always to that parameter, or should we pass the
> identically-named parameter of the calling function? (the calling
> function *happens* to have an appropriately-named parameter, but
> I haven't checked its semantics)
I suppose passing the ignore_externals parameter would be a good idea.
Where svn_client__update_internal calls update_internal to implement
make_parents we probably don't want externals introduced.
--
Philip
Re: [PATCH] fix a compilation warning
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Wed, Dec 8, 2010 at 3:29 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> FWIW, my concerns here are:
>
> * svn_client_relocate{,2} have the same signature. This might be
> confusing sometimes. (but probably should be left alone)
>
> * svn_client_relocate2() takes an IGNORE_EXTERNALS parameter. Should
> we pass TRUE always to that parameter, or should we pass the
> identically-named parameter of the calling function? (the calling
> function *happens* to have an appropriately-named parameter, but
> I haven't checked its semantics)
I'm of the latter persuasion, which is why I generally frown upon
updating deprecated function calls just for the sake of doing so.
Typically there are additional steps which need to happen in adjusting
the call, and adding the default parameters is not necessarily
sufficient. Keeping the deprecated call (and the attendant warning)
helps flag that "something else" needs to be done.
-Hyrum
Re: [PATCH] fix a compilation warning
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
FWIW, my concerns here are:
* svn_client_relocate{,2} have the same signature. This might be
confusing sometimes. (but probably should be left alone)
* svn_client_relocate2() takes an IGNORE_EXTERNALS parameter. Should
we pass TRUE always to that parameter, or should we pass the
identically-named parameter of the calling function? (the calling
function *happens* to have an appropriately-named parameter, but
I haven't checked its semantics)
Daniel
Julian Foad wrote on Wed, Dec 08, 2010 at 15:09:32 +0000:
> On Wed, 2010-12-08, Philip Martin wrote:
> > Julian Foad <ju...@wandisco.com> writes:
> >
> > > On Wed, 2010-12-08 at 14:26 +0000, Julian Foad wrote:
> > >> Prabhu Gnana Sundar wrote:
> > >> > I have attached a patch with a minor change which fixes a compiler
> > >> > warning.
> > >>
> > >> Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
> > >> replacement for svn_client_relocate() in this case? What is difference
> > >> between svn_client_relocate() and svn_client_relocate2()? Does it
> > >> matter? Can you think of any way of testing or verifying it?
> > >
> > > Did you run the test suite? Does the test suite exercise this code
> > > path?
> >
> > That's the http redirect code, tested by redirect_tests.py. The patch
> > is correct.
>
> Great. Thanks, Philip. I over-reacted and made a mistake. I
> immediately looked at the doc string of svn_client_relocate() and saw
> the words "dir is required to be a working copy root", and mis-read it
> as specifying a restriction of relocate2() compared to relocate(). As I
> did not know whether Prabhu had verified or tested anything, that
> misunderstanding made me suspect that the patch was broken.
>
> Prabhu: I'm sorry I responded with a barrage of questions. It appears
> your patch is fine.
>
> In the future, if you could say just a few words about what
> investigation and/or testing you have done, each time you submit a
> patch, that would help me to know what level of confidence I should have
> when I start looking at it. Thanks in advance.
>
> And in the future I'll try not to be so hasty in my responses.
>
> - Julian
>
>
Re: [PATCH] fix a compilation warning
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-08, Philip Martin wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > On Wed, 2010-12-08 at 14:26 +0000, Julian Foad wrote:
> >> Prabhu Gnana Sundar wrote:
> >> > I have attached a patch with a minor change which fixes a compiler
> >> > warning.
> >>
> >> Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
> >> replacement for svn_client_relocate() in this case? What is difference
> >> between svn_client_relocate() and svn_client_relocate2()? Does it
> >> matter? Can you think of any way of testing or verifying it?
> >
> > Did you run the test suite? Does the test suite exercise this code
> > path?
>
> That's the http redirect code, tested by redirect_tests.py. The patch
> is correct.
Great. Thanks, Philip. I over-reacted and made a mistake. I
immediately looked at the doc string of svn_client_relocate() and saw
the words "dir is required to be a working copy root", and mis-read it
as specifying a restriction of relocate2() compared to relocate(). As I
did not know whether Prabhu had verified or tested anything, that
misunderstanding made me suspect that the patch was broken.
Prabhu: I'm sorry I responded with a barrage of questions. It appears
your patch is fine.
In the future, if you could say just a few words about what
investigation and/or testing you have done, each time you submit a
patch, that would help me to know what level of confidence I should have
when I start looking at it. Thanks in advance.
And in the future I'll try not to be so hasty in my responses.
- Julian
Re: [PATCH] fix a compilation warning
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> On Wed, 2010-12-08 at 14:26 +0000, Julian Foad wrote:
>> Prabhu Gnana Sundar wrote:
>> > I have attached a patch with a minor change which fixes a compiler
>> > warning.
>>
>> Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
>> replacement for svn_client_relocate() in this case? What is difference
>> between svn_client_relocate() and svn_client_relocate2()? Does it
>> matter? Can you think of any way of testing or verifying it?
>
> Did you run the test suite? Does the test suite exercise this code
> path?
That's the http redirect code, tested by redirect_tests.py. The patch
is correct.
--
Philip
Re: [PATCH] fix a compilation warning
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-08 at 14:26 +0000, Julian Foad wrote:
> Prabhu Gnana Sundar wrote:
> > I have attached a patch with a minor change which fixes a compiler
> > warning.
>
> Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
> replacement for svn_client_relocate() in this case? What is difference
> between svn_client_relocate() and svn_client_relocate2()? Does it
> matter? Can you think of any way of testing or verifying it?
Did you run the test suite? Does the test suite exercise this code
path?
- Julian
> > [[[
> > fix a compiler warning.
> >
> > * subversion/libsvn_client/update.c
> > (update_internal): changed the 'relocate' to 'relocate2' since
> > 'relocate' is deprecated.
> >
> > Patch by: Prabhu Gnana Sundar <pr...@collab.net>
> > Suggested by: Kamesh Jayachandran <ka...@collab.net>
> > ]]]
>
>
> > Index: subversion/libsvn_client/update.c
> > ===================================================================
> > --- subversion/libsvn_client/update.c (revision 1041694)
> > +++ subversion/libsvn_client/update.c (working copy)
> > @@ -180,7 +180,7 @@
> > relocate our working copy first. */
> > if (corrected_url)
> > {
> > - SVN_ERR(svn_client_relocate(anchor_abspath, anchor_url, corrected_url,
> > + SVN_ERR(svn_client_relocate2(anchor_abspath, anchor_url, corrected_url,
> > TRUE, ctx, pool));
> > anchor_url = corrected_url;
> > }
>
Re: [PATCH] fix a compilation warning
Posted by Julian Foad <ju...@wandisco.com>.
Prabhu Gnana Sundar wrote:
> I have attached a patch with a minor change which fixes a compiler
> warning.
Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
replacement for svn_client_relocate() in this case? What is difference
between svn_client_relocate() and svn_client_relocate2()? Does it
matter? Can you think of any way of testing or verifying it?
- Julian
> [[[
> fix a compiler warning.
>
> * subversion/libsvn_client/update.c
> (update_internal): changed the 'relocate' to 'relocate2' since
> 'relocate' is deprecated.
>
> Patch by: Prabhu Gnana Sundar <pr...@collab.net>
> Suggested by: Kamesh Jayachandran <ka...@collab.net>
> ]]]
> Index: subversion/libsvn_client/update.c
> ===================================================================
> --- subversion/libsvn_client/update.c (revision 1041694)
> +++ subversion/libsvn_client/update.c (working copy)
> @@ -180,7 +180,7 @@
> relocate our working copy first. */
> if (corrected_url)
> {
> - SVN_ERR(svn_client_relocate(anchor_abspath, anchor_url, corrected_url,
> + SVN_ERR(svn_client_relocate2(anchor_abspath, anchor_url, corrected_url,
> TRUE, ctx, pool));
> anchor_url = corrected_url;
> }