Re: [PATCH] Correct and simplify prop-change merges.

On Tue, Oct 7, 2008 at 3:56 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> On Mon, 2008-10-06 at 15:56 -0700, David Glasser wrote:
>> This logic makes sense to me when svn_wc__merge_props gets a non-NULL
>> server_baseprops argument (ie, the props actually represent what the
>> *server* thinks the old revision from the diff had) but not when that
>> argument is NULL and that argument just gets replaced by the wc BASE.
>
> David,
>
> Thanks for examining this.
>
> [[[
> /* Given ADM_ACCESS/PATH and an array of PROPCHANGES based on
> SERVER_BASEPROPS, merge the changes into the working copy.
> Append all necessary log entries to ENTRY_ACCUM.
>
> If BASE_PROPS or WORKING_PROPS is NULL, use the props from the
> working copy.
>
> If SERVER_BASEPROPS is NULL then use base props as PROPCHANGES
> base.
>
> [...]
> */
> svn_error_t *svn_wc__merge_props(svn_wc_notify_state_t *state,
> svn_wc_adm_access_t *adm_access,
> const char *path,
> apr_hash_t *server_baseprops,
> apr_hash_t *base_props,
> apr_hash_t *working_props,
> const apr_array_header_t *propchanges,
> [...]
> ]]]
>
> I read the doc-string of svn_wc__merge_props() as meaning that it would
> apply the changes defined by 'propchanges' relative to
> ('server_baseprops', which defaults to 'base_props' for convenience if
> not given).
>
>> Have you checked to make sure that all the relevant codepaths pass a
>> good server_baseprops?
>
> I assumed that callers would pay attention to this, and supply the
> arguments as required, and I did not check them.
>
> Checking now...
>
> This is called from one place in the "merge" code path, which supplies
> server_baseprops explicitly. It is called from two places in the
> "update" code path, which both pass server_baseprops as NULL. This
> appears to be correct, as during an update the base is known to be the
> server's starting point.
>
> Does this make sense or did you understand something different about
> when server_baseprops is passed as NULL?

My main recollection was that I ran into this before and it was
somewhat subtle and worth explicitly verifying. Now you have done so,
and it does all seem correct. Thanks.

--dave

>> 2008/10/6 Julian Foad <julianfoad_at_btopenworld.com>:
>> > On Thu, 2008-09-25 at 19:19 +0100, Julian Foad wrote:
>> >> This patch fixes merge_tests.py 106.
>> >
>> > This version adjusts merge_tests.py 41 to expect success in its second
>> > prop-change merge, so no failure is introduced.
>> >
>> > I will commit this soon if no further comments.
>> >
>> > It would be good if a mergeinfo expert could take a look at the
>> > stripped-down mergeinfo-merging function here,
>> > apply_single_mergeinfo_prop_change().
>> >
>> > - Julian
>> >
>> >
>> >> Problem:
>> >>
>> >> Merging a prop-change into the WC, the existing code regarded it as a
>> >> conflict if the property had any local modification in the WC, even if
>> >> this mod was exactly what was needed to make the working copy of the
>> >> prop value be the same as the old value of the incoming change. For
>> >> example, if you tried to merge two successive changes to the same
>> >> property into an initially clean and up-to-date WC, you would get a
>> >> conflict if you didn't commit in between the two halves. This breaks the
>> >> merge-tracking merges, and merge_tests.py 106 is an XFail test for it.
>> >>
>> >> Solution:
>> >>
>> >> I noticed that the logic paid great attention to the "base" version in
>> >> the WC, in ways that are rather obscure to me, whereas I would expect it
>> >> could safely ignore the "base" value altogether and just merge into the
>> >> working value. The attached patch rewrites that bit of logic using an
>> >> extremely simple condition: "if WORKING is the same as OLD then it's a
>> >> simple update, else call the resolver".
>> >>
>> >> Help needed:
>> >>
>> >> The only outstanding problem I know of is that merge_tests.py 41 appears
>> >> to have bogus expectations relating to whether to such a merge. I tried
>> >> briefly to fix it but have put it aside for now. If anyone can help with
>> >> this, that would be much appreciated.
>> >>
>> >> Theory:
>> >>
>> >> I believe we always want to perform merges into the "working" state of
>> >> the WC, without much or any regard to the "base" state. (Note that
>> >> updating the BASE during an "update" or "switch" is certainly needed, it
>> >> just doesn't impact the merge into WORKING.) Indeed, I believe the
>> >> current merge tracking implementation depends on this theory because it
>> >> splits merges into arbitrary sub-ranges. Is there any doubt? I keep
>> >> seeing old bits of code and tests that say explicitly that they expect a
>> >> conflict if there is any local mod. I intend to keep removing/fixing
>> >> such erroneous claims, unless someone points out a contrary theory.
>> >>
>> >> Thanks.
>> >> - Julian
>> >>
>> >>
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
>> > For additional commands, e-mail: dev-help_at_subversion.tigris.org
>> >
>>
>>
>>
>
>

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org