You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/06/26 13:55:06 UTC
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Hi Lieven,
On Sat, Jun 23, 2007 at 11:45:34AM -0700, lgo@tigris.org wrote:
> Fix backwards compatibility issue with svn_client_status2.
>
>
> --- trunk/subversion/include/svn_types.h (original)
> +++ trunk/subversion/include/svn_types.h Sat Jun 23 11:45:34 2007
> @@ -300,6 +300,20 @@
> #define SVN_DEPTH_FROM_RECURSE(recurse) \
> ((recurse) ? svn_depth_infinity : svn_depth_files)
>
> +
> +/* Return an @c svn_depth_t depth based on boolean @a recurse.
> + * Use this only for the status command, as it has a unique interpretation
> + * of recursion.
> + *
> + * @note New code should never need to use this, it is called only
> + * from pre-depth APIs, for compatibility.
> + *
> + * @since New in 1.5.
> + */
> +#define SVN_DEPTH_FROM_RECURSE_STATUS(recurse) \
> + ((recurse) ? svn_depth_infinity : svn_depth_immediates)
> +
> +
It strikes me that a public macro that says "don't use this" isn't
particularly useful to other users of the API. Is there any reason not
to just inline the definition into the single place it's used?
Regards,
Malcolm
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 27 Jun 2007, Lieven Govaerts wrote:
> Malcolm Rowe wrote:
> > Hi Lieven,
> >
> > On Sat, Jun 23, 2007 at 11:45:34AM -0700, lgo@tigris.org wrote:
> >
> >> Fix backwards compatibility issue with svn_client_status2.
> >>
> >>
> >> --- trunk/subversion/include/svn_types.h (original)
> >> +++ trunk/subversion/include/svn_types.h Sat Jun 23 11:45:34 2007
> >> @@ -300,6 +300,20 @@
> >> #define SVN_DEPTH_FROM_RECURSE(recurse) \
> >> ((recurse) ? svn_depth_infinity : svn_depth_files)
> >>
> >> +
> >> +/* Return an @c svn_depth_t depth based on boolean @a recurse.
> >> + * Use this only for the status command, as it has a unique interpretation
> >> + * of recursion.
> >> + *
> >> + * @note New code should never need to use this, it is called only
> >> + * from pre-depth APIs, for compatibility.
> >> + *
> >> + * @since New in 1.5.
> >> + */
> >> +#define SVN_DEPTH_FROM_RECURSE_STATUS(recurse) \
> >> + ((recurse) ? svn_depth_infinity : svn_depth_immediates)
> >> +
> >> +
> >>
> >
> > It strikes me that a public macro that says "don't use this" isn't
> > particularly useful to other users of the API. Is there any reason not
> > to just inline the definition into the single place it's used?
> >
> Malcolm, thanks for the review.
>
> There is a second place in which this macro should be used, that's in
> svn/main.c 1609. But I forgot to change it, so thx for the reminder! ;)
>
> The reason why I added a second macro is to make it clear to users of
> the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
> results in all cases, more specifically when working with status.
>
> AFAIC, neither of the two need to be in a public interface, are there
> better options?
Sure. Add an include/private/svn_types_private.h header file, and
change the macros to use our double-underscore notation.
http://subversion.tigris.org/hacking.html#other-conventions
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by Lieven Govaerts <sv...@mobsol.be>.
Quoting David Glasser <gl...@mit.edu>:
> On 6/26/07, Lieven Govaerts <sv...@mobsol.be> wrote:
> > Malcolm Rowe wrote:
> > > It strikes me that a public macro that says "don't use this" isn't
> > > particularly useful to other users of the API. Is there any reason not
> > > to just inline the definition into the single place it's used?
> > >
> > Malcolm, thanks for the review.
> >
> > There is a second place in which this macro should be used, that's in
> > svn/main.c 1609. But I forgot to change it, so thx for the reminder! ;)
> >
> > The reason why I added a second macro is to make it clear to users of
> > the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
> > results in all cases, more specifically when working with status.
>
> In fact it ought to be used in more places, like
> svnserve/serve.c(status)... (and I think somewhere in mod_dav_svn as
> well)
Thanks for the info David, I'll go over the code this weekend to find and fix
those instances.
Lieven
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by David Glasser <gl...@mit.edu>.
On 7/6/07, Karl Fogel <kf...@red-bean.com> wrote:
> I wrote:
> > "David Glasser" <gl...@mit.edu> writes:
> > >> The reason why I added a second macro is to make it clear to users of
> > >> the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
> > >> results in all cases, more specifically when working with status.
> > >
> > > In fact it ought to be used in more places, like
> > > svnserve/serve.c(status)... (and I think somewhere in mod_dav_svn as
> > > well)
> >
> > I believe the status command is special and doesn't do the same
> > recurse->depth mappings as other commands, and that's why it doesn't
> > use the macro.
> >
> > Checking... yup: it maps nonrecursive to svn_depth_empty instead of
> > svn_depth_files.
>
> Oh, sorry, I see you were actaully talking about the
> SVN_DEPTH_FROM_RECURSE_STATUS macro, it just didn't happen to be
> quoted in the context, so mistakenly read "it" as referring to
> SVN_DEPTH_FROM_RECURSE.
Yup, over-zealous context pruning on my part.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by Karl Fogel <kf...@red-bean.com>.
I wrote:
> "David Glasser" <gl...@mit.edu> writes:
> >> The reason why I added a second macro is to make it clear to users of
> >> the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
> >> results in all cases, more specifically when working with status.
> >
> > In fact it ought to be used in more places, like
> > svnserve/serve.c(status)... (and I think somewhere in mod_dav_svn as
> > well)
>
> I believe the status command is special and doesn't do the same
> recurse->depth mappings as other commands, and that's why it doesn't
> use the macro.
>
> Checking... yup: it maps nonrecursive to svn_depth_empty instead of
> svn_depth_files.
Oh, sorry, I see you were actaully talking about the
SVN_DEPTH_FROM_RECURSE_STATUS macro, it just didn't happen to be
quoted in the context, so mistakenly read "it" as referring to
SVN_DEPTH_FROM_RECURSE.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@mit.edu> writes:
>> The reason why I added a second macro is to make it clear to users of
>> the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
>> results in all cases, more specifically when working with status.
>
> In fact it ought to be used in more places, like
> svnserve/serve.c(status)... (and I think somewhere in mod_dav_svn as
> well)
I believe the status command is special and doesn't do the same
recurse->depth mappings as other commands, and that's why it doesn't
use the macro.
Checking... yup: it maps nonrecursive to svn_depth_empty instead of
svn_depth_files.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by David Glasser <gl...@mit.edu>.
On 6/26/07, Lieven Govaerts <sv...@mobsol.be> wrote:
> Malcolm Rowe wrote:
> > It strikes me that a public macro that says "don't use this" isn't
> > particularly useful to other users of the API. Is there any reason not
> > to just inline the definition into the single place it's used?
> >
> Malcolm, thanks for the review.
>
> There is a second place in which this macro should be used, that's in
> svn/main.c 1609. But I forgot to change it, so thx for the reminder! ;)
>
> The reason why I added a second macro is to make it clear to users of
> the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
> results in all cases, more specifically when working with status.
In fact it ought to be used in more places, like
svnserve/serve.c(status)... (and I think somewhere in mod_dav_svn as
well)
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25517 - in trunk/subversion: include libsvn_client
Posted by Lieven Govaerts <sv...@mobsol.be>.
Malcolm Rowe wrote:
> Hi Lieven,
>
> On Sat, Jun 23, 2007 at 11:45:34AM -0700, lgo@tigris.org wrote:
>
>> Fix backwards compatibility issue with svn_client_status2.
>>
>>
>> --- trunk/subversion/include/svn_types.h (original)
>> +++ trunk/subversion/include/svn_types.h Sat Jun 23 11:45:34 2007
>> @@ -300,6 +300,20 @@
>> #define SVN_DEPTH_FROM_RECURSE(recurse) \
>> ((recurse) ? svn_depth_infinity : svn_depth_files)
>>
>> +
>> +/* Return an @c svn_depth_t depth based on boolean @a recurse.
>> + * Use this only for the status command, as it has a unique interpretation
>> + * of recursion.
>> + *
>> + * @note New code should never need to use this, it is called only
>> + * from pre-depth APIs, for compatibility.
>> + *
>> + * @since New in 1.5.
>> + */
>> +#define SVN_DEPTH_FROM_RECURSE_STATUS(recurse) \
>> + ((recurse) ? svn_depth_infinity : svn_depth_immediates)
>> +
>> +
>>
>
> It strikes me that a public macro that says "don't use this" isn't
> particularly useful to other users of the API. Is there any reason not
> to just inline the definition into the single place it's used?
>
Malcolm, thanks for the review.
There is a second place in which this macro should be used, that's in
svn/main.c 1609. But I forgot to change it, so thx for the reminder! ;)
The reason why I added a second macro is to make it clear to users of
the SVN_DEPTH_FROM_RECURSE macro that it doesn't give the correct
results in all cases, more specifically when working with status.
AFAIC, neither of the two need to be in a public interface, are there
better options?
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org