You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/04/30 20:25:14 UTC
RE: svn commit: r1591301
-/subversion/trunk/subversion/libsvn_client/mergeinfo.c
This might make us add svn:mergeinfo on nodes that didn't have this property before eliding, while the old code tried to avoid that by checking to see if the value was inherited from an ancestor.
Bert
-----Original Message-----
From: "julianfoad@apache.org" <ju...@apache.org>
Sent: 30-4-2014 16:12
To: "commits@subversion.apache.org" <co...@subversion.apache.org>
Subject: svn commit: r1591301 -/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Author: julianfoad
Date: Wed Apr 30 14:12:08 2014
New Revision: 1591301
URL: http://svn.apache.org/r1591301
Log:
* subversion/libsvn_client/mergeinfo.c
(svn_client__elide_mergeinfo): A tiny simplification: when we want only
explicit mergeinfo, ask for only explicit mergeinfo.
Modified:
subversion/trunk/subversion/libsvn_client/mergeinfo.c
Modified: subversion/trunk/subversion/libsvn_client/mergeinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/mergeinfo.c?rev=1591301&r1=1591300&r2=1591301&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/mergeinfo.c (original)
+++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Wed Apr 30 14:12:08 2014
@@ -922,13 +922,12 @@ svn_client__elide_mergeinfo(const char *
{
svn_mergeinfo_t target_mergeinfo;
svn_mergeinfo_t mergeinfo = NULL;
- svn_boolean_t inherited;
const char *walk_path;
svn_error_t *err;
/* Get the TARGET_WCPATH's explicit mergeinfo. */
- err = svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
- svn_mergeinfo_inherited,
+ err = svn_client__get_wc_mergeinfo(&target_mergeinfo, NULL,
+ svn_mergeinfo_explicit,
target_abspath,
limit_abspath,
&walk_path, FALSE,
@@ -951,7 +950,7 @@ svn_client__elide_mergeinfo(const char *
/* If TARGET_WCPATH has no explicit mergeinfo, there's nothing to
elide, we're done. */
- if (inherited || target_mergeinfo == NULL)
+ if (target_mergeinfo == NULL)
return SVN_NO_ERROR;
/* Get TARGET_WCPATH's inherited mergeinfo from the WC. */
RE: svn commit:
r1591301-/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Posted by Bert Huijben <be...@qqmail.nl>.
Thanks for the detailed response,
Bert
-----Original Message-----
From: "Julian Foad" <ju...@btopenworld.com>
Sent: 1-5-2014 15:12
To: "Bert Huijben" <be...@qqmail.nl>
Cc: "dev@subversion.apache.org" <de...@subversion.apache.org>
Subject: Re: svn commit: r1591301-/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Bert Huijben wrote:
> Are you sure that it doesn't return the mergeinfo as it applies to the
> target, even if it has found that information by looking at the parent?
> (That is what I tried to say)
Hi, Bert. Note the comment on the code I changed:
/* Get the TARGET_WCPATH's explicit mergeinfo. */
That comment describes exactly what the code was doing (in a long-winded way), and what it still is doing now (more succinctly). I think the reason why it was originally written in that long-winded way is
because the 'get explicit only' option to that API was not available
when that code was first written.
Before my change:
svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
svn_mergeinfo_inherited,
gets the mergeinfo as it applies to the target path, even if it has found that information by looking at the parent, as you say. But if it finds that information from a parent, it sets the 'inherited' output flag to true. And then the code said, if we got that info by inheriting it, then just return.
Now, instead of allowing this function call to get inherited mergeinfo which we don't want, we just ask it not to get inherited mergeinfo. The end result is identical.
- Julian
Re: svn commit: r1591301 -/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> Are you sure that it doesn't return the mergeinfo as it applies to the
> target, even if it has found that information by looking at the parent?
> (That is what I tried to say)
Hi, Bert. Note the comment on the code I changed:
/* Get the TARGET_WCPATH's explicit mergeinfo. */
That comment describes exactly what the code was doing (in a long-winded way), and what it still is doing now (more succinctly). I think the reason why it was originally written in that long-winded way is
because the 'get explicit only' option to that API was not available
when that code was first written.
Before my change:
svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
svn_mergeinfo_inherited,
gets the mergeinfo as it applies to the target path, even if it has found that information by looking at the parent, as you say. But if it finds that information from a parent, it sets the 'inherited' output flag to true. And then the code said, if we got that info by inheriting it, then just return.
Now, instead of allowing this function call to get inherited mergeinfo which we don't want, we just ask it not to get inherited mergeinfo. The end result is identical.
- Julian
Re: svn commit: r1591301 -/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Posted by Bert Huijben <be...@qqmail.nl>.
Are you sure that it doesn't return the mergeinfo as it applies to the target, even if it has found that information by looking at the parent? (That is what I tried to say)
I'm not familiar enough with this code to really review it from just the code, but this eliding code is quite sensitive and isn't that well tested (as in general it doesn't affect merge results for common merge patterns.. and our test mostly just check for changed nodes, not the actual property changes).
In general we should try not to add svn:mergeinfo in more places than necessary to record the actual merges and I was(/am?) afraid that this might happen as a side effect of this merge. The result would be that all new merges to the same tree would be slower, while the eliding process was added to make it faster if a catch-up merge made mergeinfo match the inherited mergeinfo.
(Perhaps this last part makes your patch safe… I just find it hard to verify. And hard to tell where the performance costs are)
Bert
Sent from Windows Mail
From: Julian Foad
Sent: Thursday, May 1, 2014 1:12 PM
To: Bert Huijben
Cc: dev@subversion.apache.org
Bert Huijben wrote:
> This might make us add svn:mergeinfo on nodes that didn't have this property
> before eliding, while the old code tried to avoid that by checking to see if
> the value was inherited from an ancestor.
Hi Bert. I can't quite parse your sentence unambiguously, but I believe this change has no functional effect.
In the case where this node (at target_abspath) has no explicit mergeinfo, this code said before my change:
set 'target_mergeinfo' to <inherited mergeinfo, if any found, else null>
set 'inherited' to <true, if inherited mergeinfo found, else false>
if (inherited || target_mergeinfo == NULL) return
Therefore it would always return at this point.
Now, in the same case, after my change, the code says:
set 'target_mergeinfo' to <null>
if (target_mergeinfo == NULL) return
So, again, it will always return at this point.
The only way this 'target_mergeinfo' value can propagate past that 'if' statement is if it is explicit mergeinfo. That's the case both before and after my change.
- Julian
>URL: http://svn.apache.org/r1591301
>Log:
>* subversion/libsvn_client/mergeinfo.c
> (svn_client__elide_mergeinfo): A tiny simplification: when we want only
> explicit mergeinfo, ask for only explicit mergeinfo.
>
>Modified:
> subversion/trunk/subversion/libsvn_client/mergeinfo.c
>
>Modified: subversion/trunk/subversion/libsvn_client/mergeinfo.c
>==============================================================================
>--- subversion/trunk/subversion/libsvn_client/mergeinfo.c (original)
>+++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Wed Apr 30 14:12:08 2014
>@@ -922,13 +922,12 @@ svn_client__elide_mergeinfo(const char *
> {
> svn_mergeinfo_t target_mergeinfo;
> svn_mergeinfo_t mergeinfo = NULL;
>- svn_boolean_t inherited;
> const char *walk_path;
> svn_error_t *err;
>
> /* Get the TARGET_WCPATH's explicit mergeinfo. */
>- err = svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
>- svn_mergeinfo_inherited,
>+ err = svn_client__get_wc_mergeinfo(&target_mergeinfo, NULL,
>+ svn_mergeinfo_explicit,
> target_abspath,
> limit_abspath,
> &walk_path, FALSE,
>@@ -951,7 +950,7 @@ svn_client__elide_mergeinfo(const char *
>
> /* If TARGET_WCPATH has no explicit mergeinfo, there's nothing to
> elide, we're done. */
>- if (inherited || target_mergeinfo == NULL)
>+ if (target_mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> /* Get TARGET_WCPATH's inherited mergeinfo from the WC. */
Re: svn commit: r1591301 -/subversion/trunk/subversion/libsvn_client/mergeinfo.c
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> This might make us add svn:mergeinfo on nodes that didn't have this property
> before eliding, while the old code tried to avoid that by checking to see if
> the value was inherited from an ancestor.
Hi Bert. I can't quite parse your sentence unambiguously, but I believe this change has no functional effect.
In the case where this node (at target_abspath) has no explicit mergeinfo, this code said before my change:
set 'target_mergeinfo' to <inherited mergeinfo, if any found, else null>
set 'inherited' to <true, if inherited mergeinfo found, else false>
if (inherited || target_mergeinfo == NULL) return
Therefore it would always return at this point.
Now, in the same case, after my change, the code says:
set 'target_mergeinfo' to <null>
if (target_mergeinfo == NULL) return
So, again, it will always return at this point.
The only way this 'target_mergeinfo' value can propagate past that 'if' statement is if it is explicit mergeinfo. That's the case both before and after my change.
- Julian
>URL: http://svn.apache.org/r1591301
>Log:
>* subversion/libsvn_client/mergeinfo.c
> (svn_client__elide_mergeinfo): A tiny simplification: when we want only
> explicit mergeinfo, ask for only explicit mergeinfo.
>
>Modified:
> subversion/trunk/subversion/libsvn_client/mergeinfo.c
>
>Modified: subversion/trunk/subversion/libsvn_client/mergeinfo.c
>==============================================================================
>--- subversion/trunk/subversion/libsvn_client/mergeinfo.c (original)
>+++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Wed Apr 30 14:12:08 2014
>@@ -922,13 +922,12 @@ svn_client__elide_mergeinfo(const char *
> {
> svn_mergeinfo_t target_mergeinfo;
> svn_mergeinfo_t mergeinfo = NULL;
>- svn_boolean_t inherited;
> const char *walk_path;
> svn_error_t *err;
>
> /* Get the TARGET_WCPATH's explicit mergeinfo. */
>- err = svn_client__get_wc_mergeinfo(&target_mergeinfo, &inherited,
>- svn_mergeinfo_inherited,
>+ err = svn_client__get_wc_mergeinfo(&target_mergeinfo, NULL,
>+ svn_mergeinfo_explicit,
> target_abspath,
> limit_abspath,
> &walk_path, FALSE,
>@@ -951,7 +950,7 @@ svn_client__elide_mergeinfo(const char *
>
> /* If TARGET_WCPATH has no explicit mergeinfo, there's nothing to
> elide, we're done. */
>- if (inherited || target_mergeinfo == NULL)
>+ if (target_mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> /* Get TARGET_WCPATH's inherited mergeinfo from the WC. */