You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <ch...@gmail.com> on 2005/10/21 12:34:58 UTC
[PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Hi!
I propose to disable call to svn_wc__get_specialI() unless
HAVE_SYMLINK defined. It should improve "svn status" performance on
Windows. I have already mantain such version of subversion for my
company. It increase performance about 30%. Anybody against?
[[
Disable call to svn_wc__get_specialI() unless HAVE_SYMLINK defined.
* subversion/libsvn_wc/status.c
(assemble_status): Don't call to svn_wc__get_special() unless
HAVE_SYMLINK defined.
]]
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/21/05, Ivan Zhakov <ch...@gmail.com> wrote:
> Hi!
> I propose to disable call to svn_wc__get_specialI() unless
> HAVE_SYMLINK defined. It should improve "svn status" performance on
> Windows. I have already mantain such version of subversion for my
> company. It increase performance about 30%. Anybody against?
>
> [[
> Disable call to svn_wc__get_specialI() unless HAVE_SYMLINK defined.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): Don't call to svn_wc__get_special() unless
> HAVE_SYMLINK defined.
> ]]
>
To be clear: I propose it's as temporary solution for 1.3. After
wc-propcaching branch will be implemented these #ifdef should be
removed. I am plaining done this before 1.4.
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
> Ivan Zhakov <ch...@gmail.com> writes:
>
> > Because svn_wc__get_special should return value of svn:special
> > property, so it not true always return false. May be I am wrong.
>
> Equally, it doesn't make sense for status to ignore
> svn_wc__get_special :)
> As far as I can see it makes as much sense for status to be the only
> function to ignore special as it does for all functions to ignore it.
> I don't really care what Windows does, I simply want to avoid having
> HAVE_SYMLINK several times in the status function.
I consider hacking svn_wc__get_special is dangerous for 1.3. My change
is temporary trick for 1.3 and Windows. I hope wc-propcaching will be
done before 1.4 and this #ifdef could be revised. I'll document it
with "### FIXME", Ok?
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Philip Martin <ph...@codematters.co.uk>.
Ivan Zhakov <ch...@gmail.com> writes:
> Because svn_wc__get_special should return value of svn:special
> property, so it not true always return false. May be I am wrong.
Equally, it doesn't make sense for status to ignore
svn_wc__get_special :)
As far as I can see it makes as much sense for status to be the only
function to ignore special as it does for all functions to ignore it.
I don't really care what Windows does, I simply want to avoid having
HAVE_SYMLINK several times in the status function.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
> Ivan Zhakov <ch...@gmail.com> writes:
>
> > On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
> >> Ivan Zhakov <ch...@gmail.com> writes:
> >>
> >> >> perhaps the other HAVE_SYMLINK bits could be removed? I think it's
> >> >> better to have the #ifdef code in one place if possible.
> >> > May be, but don't use wc_speciall variable unless HAVE_SYMLINK defined
> >> > clearer for me.
> >>
> >> I think HAVE_SYMLINK is a hack that should be kept as small as
> >> possible, it's currently spread out over 4 places in the function.
> >>
> >> Perhaps the HAVE_SYMLINK should be moved inside svn_wc__get_special,
> >> so that windows always returns false? That would speed up all the
> >> other operations as well as status.
> > I was thinking about it. Because I am not wc expert I was fear do
> > it, but if you consider this safe it would be better. (In my company
> > I have done it in your way)
>
> Are you referring to my svn_wc__get_special suggestion? I think it's
> the best solution, I don't understand why status should be different
> from the other operations.
Yes, I'm about your suggestion.
Because svn_wc__get_special should return value of svn:special
property, so it not true always return false. May be I am wrong.
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Philip Martin <ph...@codematters.co.uk>.
Ivan Zhakov <ch...@gmail.com> writes:
> On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
>> Ivan Zhakov <ch...@gmail.com> writes:
>>
>> >> perhaps the other HAVE_SYMLINK bits could be removed? I think it's
>> >> better to have the #ifdef code in one place if possible.
>> > May be, but don't use wc_speciall variable unless HAVE_SYMLINK defined
>> > clearer for me.
>>
>> I think HAVE_SYMLINK is a hack that should be kept as small as
>> possible, it's currently spread out over 4 places in the function.
>>
>> Perhaps the HAVE_SYMLINK should be moved inside svn_wc__get_special,
>> so that windows always returns false? That would speed up all the
>> other operations as well as status.
> I was thinking about it. Because I am not wc expert I was fear do
> it, but if you consider this safe it would be better. (In my company
> I have done it in your way)
Are you referring to my svn_wc__get_special suggestion? I think it's
the best solution, I don't understand why status should be different
from the other operations.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
> Ivan Zhakov <ch...@gmail.com> writes:
>
> >> perhaps the other HAVE_SYMLINK bits could be removed? I think it's
> >> better to have the #ifdef code in one place if possible.
> > May be, but don't use wc_speciall variable unless HAVE_SYMLINK defined
> > clearer for me.
>
> I think HAVE_SYMLINK is a hack that should be kept as small as
> possible, it's currently spread out over 4 places in the function.
>
> Perhaps the HAVE_SYMLINK should be moved inside svn_wc__get_special,
> so that windows always returns false? That would speed up all the
> other operations as well as status.
I was thinking about it. Because I am not wc expert I was fear do it,
but if you consider this safe it would be better. (In my company I
have done it in your way)
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Philip Martin <ph...@codematters.co.uk>.
Ivan Zhakov <ch...@gmail.com> writes:
>> perhaps the other HAVE_SYMLINK bits could be removed? I think it's
>> better to have the #ifdef code in one place if possible.
> May be, but don't use wc_speciall variable unless HAVE_SYMLINK defined
> clearer for me.
I think HAVE_SYMLINK is a hack that should be kept as small as
possible, it's currently spread out over 4 places in the function.
Perhaps the HAVE_SYMLINK should be moved inside svn_wc__get_special,
so that windows always returns false? That would speed up all the
other operations as well as status.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/24/05, Philip Martin <ph...@codematters.co.uk> wrote:
> Ivan Zhakov <ch...@gmail.com> writes:
>
> > Index: subversion/libsvn_wc/status.c
> > ===================================================================
> > --- subversion/libsvn_wc/status.c (revision 16889)
> > +++ subversion/libsvn_wc/status.c (working copy)
> > @@ -238,7 +238,9 @@
> > svn_boolean_t prop_modified_p = FALSE;
> > svn_boolean_t locked_p = FALSE;
> > svn_boolean_t switched_p = FALSE;
> > +#ifdef HAVE_SYMLINK
> > svn_boolean_t wc_special;
> > +#endif /* HAVE_SYMLINK */
> >
> > /* Defaults for two main variables. */
> > enum svn_wc_status_kind final_text_status = svn_wc_status_normal;
> > @@ -357,10 +359,12 @@
> > SVN_ERR (svn_wc_props_modified_p (&prop_modified_p, path, adm_access,
> > pool));
> >
> > +#ifdef HAVE_SYMLINK
> > if (has_props)
> > SVN_ERR (svn_wc__get_special (&wc_special, path, adm_access, pool));
> > else
> > wc_special = FALSE;
> > +#endif /* HAVE_SYMLINK */
>
> If we made that
>
> #else
> wc_special = path_special;
> #endif
>
> perhaps the other HAVE_SYMLINK bits could be removed? I think it's
> better to have the #ifdef code in one place if possible.
May be, but don't use wc_speciall variable unless HAVE_SYMLINK defined
clearer for me.
> >
> > /* If the entry is a file, check for textual modifications */
> > if ((entry->kind == svn_node_file)
> > @@ -447,12 +451,12 @@
> > }
> > else if (path_kind != entry->kind)
> > final_text_status = svn_wc_status_obstructed;
> > +#ifdef HAVE_SYMLINK
> > else if (((! wc_special) && (path_special))
> > -#ifdef HAVE_SYMLINK
> > || (wc_special && (! path_special))
>
> How about
>
> if (wc_special != path_special)
>
> which complements the earlier (wc_special == path_special)?
I was wondered also. This was written my somebody else, and I don't
take myself to change it. May be because any non zero value treated as
"true" ? For example if wc_special=1 and path_special=2? I understand
that it is near to be impossible.
>
> > -#endif /* HAVE_SYMLINK */
> > )
> > final_text_status = svn_wc_status_obstructed;
> > +#endif /* HAVE_SYMLINK */
> >
> > if (path_kind == svn_node_dir && entry->kind == svn_node_dir)
> > SVN_ERR (svn_wc_locked (&locked_p, path, pool));
>
> --
> Philip Martin
>
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Philip Martin <ph...@codematters.co.uk>.
Ivan Zhakov <ch...@gmail.com> writes:
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 16889)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -238,7 +238,9 @@
> svn_boolean_t prop_modified_p = FALSE;
> svn_boolean_t locked_p = FALSE;
> svn_boolean_t switched_p = FALSE;
> +#ifdef HAVE_SYMLINK
> svn_boolean_t wc_special;
> +#endif /* HAVE_SYMLINK */
>
> /* Defaults for two main variables. */
> enum svn_wc_status_kind final_text_status = svn_wc_status_normal;
> @@ -357,10 +359,12 @@
> SVN_ERR (svn_wc_props_modified_p (&prop_modified_p, path, adm_access,
> pool));
>
> +#ifdef HAVE_SYMLINK
> if (has_props)
> SVN_ERR (svn_wc__get_special (&wc_special, path, adm_access, pool));
> else
> wc_special = FALSE;
> +#endif /* HAVE_SYMLINK */
If we made that
#else
wc_special = path_special;
#endif
perhaps the other HAVE_SYMLINK bits could be removed? I think it's
better to have the #ifdef code in one place if possible.
>
> /* If the entry is a file, check for textual modifications */
> if ((entry->kind == svn_node_file)
> @@ -447,12 +451,12 @@
> }
> else if (path_kind != entry->kind)
> final_text_status = svn_wc_status_obstructed;
> +#ifdef HAVE_SYMLINK
> else if (((! wc_special) && (path_special))
> -#ifdef HAVE_SYMLINK
> || (wc_special && (! path_special))
How about
if (wc_special != path_special)
which complements the earlier (wc_special == path_special)?
> -#endif /* HAVE_SYMLINK */
> )
> final_text_status = svn_wc_status_obstructed;
> +#endif /* HAVE_SYMLINK */
>
> if (path_kind == svn_node_dir && entry->kind == svn_node_dir)
> SVN_ERR (svn_wc_locked (&locked_p, path, pool));
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by Ivan Zhakov <ch...@gmail.com>.
On 10/21/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> On Fri, 21 Oct 2005, Ivan Zhakov wrote:
>
> > I propose to disable call to svn_wc__get_specialI() unless
> > HAVE_SYMLINK defined. It should improve "svn status" performance on
> > Windows. I have already mantain such version of subversion for my
> > company. It increase performance about 30%. Anybody against?
>
> One of the planned changes on wc-propcaching is to store svn:special (and
> other well-chosen properties) in the entries file, which will increase the
> performance on all platforms.
Of course I know about caching svn:special. I propose solution for
1.3. Because status that takes more than 5 minutes annoyes.
> The problem with your approach is ath it
> introduces one more platform specific #ifdef:-) Can we let your patch wait
> until we know if wc-propcaching will be ready for 1.4?
Agreed that #ifdef is bad, but there are already #ifdef HAVE_SYMLINK
in current code, I simply extended it.
--
Ivan Zhakov
Re: [PATCH]: Check for symlinks only if HAVE_SYMLINK defined.
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 21 Oct 2005, Ivan Zhakov wrote:
> I propose to disable call to svn_wc__get_specialI() unless
> HAVE_SYMLINK defined. It should improve "svn status" performance on
> Windows. I have already mantain such version of subversion for my
> company. It increase performance about 30%. Anybody against?
One of the planned changes on wc-propcaching is to store svn:special (and
other well-chosen properties) in the entries file, which will increase the
performance on all platforms. The problem with your approach is ath it
introduces one more platform specific #ifdef:-) Can we let your patch wait
until we know if wc-propcaching will be ready for 1.4?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org