Re: [PATCH] no mergeinfo on 'mergeinfo NON-affecting' wc-to-wc copy

On Tue, Sep 16, 2008 at 5:47 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> I'm not following this thread, but here are some observations.
>
> On Tue, 2008-09-16 at 15:12 +0400, Danil Shopyrin wrote:
>> Here is the improved version of the original patch. Generally spoken,
>> the song remains the same. There are only following changes:
>> 1) things become more optimized:
>> a) we don't event call svn_client__get_wc_mergeinfo() for dst if src has
>> explicit mergeinfo set on it
>> b) we don't call svn_client__get_wc_mergeinfo() second time for src if it's
>> a 'mergeinfo affecting' copy
>> 2) there is one more test
>> 3) propagate_mergeinfo_within_wc()'s logic modified more carefully. We
>> don't check for 'mergeinfo NON-affecting' copy for a locally
>> added/replaced PAIR->src without history.
>>
>> The new log message is as follows:
>> [[[
>> Don't create any explicit (including empty) mergeinfo record during WC-to-WC
>> copy if it's a 'mergeinfo NON-affecting' copy. WC-to-WC copy can be
>> considered as 'megeinfo NON-affecting' if and only if the following
>> conditions are satisfied:
>> a) src and dst are located below the same URL
>> b) mergeinfo (if any) for src and dst is inherited from this URL
>> c) inherited mergeinfos (if any) for src and dst are equal.
>
> I noticed that those conditions sound a bit redundant and not
> crystal-clear. As I understand it, mergeinfo can only be inherited
> as-is, not with modifications. Therefore doesn't (c) follow necessarily
> from (b)? (And (a) is true for all nodes in a repository, and only
> relevant as the referent of condition (b).)

The c) condition can be untrue if WC-to-WC copy is performed between
two different working copies. For example, the destination working
copy has some merged (but uncommited) revisions.

> Would the conditions be better said as:
>
> - The destination node must inherit its mergeinfo from the same node
> as the source node did.
> - To know this, it is sufficient to determine that both chains of
> inheritance pass through a common node.

See comments above.

>> Index: subversion/libsvn_client/mergeinfo.h
>> ===================================================================
>> --- subversion/libsvn_client/mergeinfo.h (revision 33072)
>> +++ subversion/libsvn_client/mergeinfo.h (working copy)
>> @@ -83,7 +83,19 @@
>> (ignored if NULL) or beyond any switched path.
>>
>> Set *WALKED_PATH to the path climbed from WCPATH to find inherited
>> - mergeinfo, or "" if none was found. (ignored if NULL). */
>> + mergeinfo, or "" if none was found. (ignored if NULL).
>
> (I know you didn't change that sentence, but I'll note in passing that
> it's not clear to me how a "path climbed" would be represented in a
> string.)

Actually, WALKED_PATH isn't used anywhere. I plan to remove this
parameter after this patch will be committed (if it will be).

>> + Set *EFFECTIVE_MERGEINFO to the original (not adjusted) mergeinfo found on
>> + WCPATH or one of its ancestors. Set *EFFECTIVE_MERGEINFO to NULL if there is
>> + no mergeinfo found. (Ignore if EFFECTIVE_MERGEINFO is NULL).
>
> The word "effective" does not convey the meaning of "original (not
> adjusted)". "original" might be better.

Agreed. I'll update the patch.

>> + Set *EFFECTIVE_URL to the last url scanned for mergeinfo. It can be the
>> + url of WCPATH (in the case of explicit mergeinfo) or one of its ancestors
>> + (in the case of inherited mergeinfo).
>> +
>> + Don't climb through mixed revision working copy if ALLOW_MIXED_REVISIONS
>> + is FALSE.
>
> If it does climb through a mixed-revision working copy, that contradicts
> a paragraph higher up in the doc string (not visible in this diff). If
> it doesn't climb through a mixed-revision working copy, what does it do?

I agreed that the updated docstring is inconsistent. I'll fix that.

>> + */
>> svn_error_t *
>> svn_client__get_wc_mergeinfo(svn_mergeinfo_t *mergeinfo,
>> svn_boolean_t *inherited,
>> @@ -93,7 +105,10 @@
>> const char *wcpath,
>> const char *limit_path,
>> const char **walked_path,
>> + svn_mergeinfo_t *effective_mergeinfo,
>> + const char **effective_url,
>> svn_wc_adm_access_t *adm_access,
>> + svn_boolean_t allow_mixed_revisions,
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool);
>
> I wonder if this API is being made more complex than is necessary. Might
> it be better to split it, because the other callers don't want these new
> facilities? (Do you really need to know the original mergeinfo? If not,
> that would simplify the patch a bit too.)

See my paragraph above for why we need to compare original (effective)
mergeinfos. The API wasn't split to keep WC climbing logic in one
function (and not duplicate it).

--
Danil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org