C. Michael Pilato wrote:
> On 03/13/2013 11:37 AM, Julian Foad wrote:
>> svn_ra_do_update2(..., pool)
>> svn_ra_do_switch3(..., ignore_ancestry, ..., result_pool, scratch_pool)
>>
>> - switch3() is recently revised from switch2(), specifically to add the
>> 'ignore_ancestry' option to allow it to *not* ignore ancestry.
>> Previously switch2() always ignored ancestry while update2() did not, so
>> this is a step towards consistency. Further consistency would be
>> attained by providing the option to update as well.
>
> Hrm. Ignoring ancestry in the switch sense is a workaround provided for
> folks who got into the habit of switching a working copy from any ol'
> repository location to any other ol' repository location intentionally.
>
> There should be no ignore_ancestry option to update, as its primary
> functionality is and has always been intimately bound to the ancestry of the
> target being updated.
>
> In other words, you could add this option to update today, but it would have
> exactly one meaningful and supported value (FALSE). And in fact, were we
> starting from scratch here, there would be ignore_ancestry option to switch,
> either.
(Assuming you mean 'there would be no ignore_ancestry option...'.)
The 'ignore_ancestry' parameter at the RA level is changing the way diffs of replaced files are reported by the server. It is not the same as the '--ignore-ancestry' flag that 'svn switch' accepts. 'svn switch --ignore-ancestry' causes libsvn_client to bypass a check that the switch source and destination URLs have a common ancestor before starting the switch.
The '--ignore-ancestry' flag also sets this 'ignore_ancestry' parameter in the RA call to cause the server to send diffs of replaced files in a different way, but that is a secondary effect. It controls whether the diff of a file that happens to exist at the same relative path is reported as text-modify or as delete-and-add, and so affects whether and how conflicts are raised if the user had local mods.
It is this 'do diffs as text-mods' meaning that we should make consistently available to both 'update' and 'switch' at the RA level.
At the svn_client_switch() level and/or the UI level, on the other hand, we might not want to make both the meanings separately available, because that's the level at which the library is supplying some more complex behaviour and at this level it makes sense to differentiate more.
>> svn_wc__get_update_editor(..., adds_as_modification, clean_checkout, ...)
>> svn_wc__get_switch_editor(...)
>>
>> - These APIs are the 1.8 replacements for deprecated
>> svn_wc_get_{update,switch}_editor4().
>>
>> - 'clean_checkout' appears to be just an optimisation hint to bypass some
>> unnecessary checks when we're using 'update' to do a checkout, and as
>> such it doesn't need to be available in 'switch'.
>>
>> - 'adds_as_modification' controls whether a tree conflict is raised for
>> an incoming add onto a local add, and makes just as much sense for
>> 'switch' as for 'update'.
>
> Taking your word on this. I have no experience with these flags.
>
>> svn_client__update_internal(..., adds_as_modification, make_parents, ...)
>> svn_client__switch_internal(..., ignore_ancestry, ...)
>> svn_client_update4(..., adds_as_modification, make_parents, ...)
>> svn_client_switch3(..., ignore_ancestry, ...)
>>
>> - 'adds_as_modification' and 'ignore_ancestry' are as described above.
>>
>> - I'm not sure whether 'make_parents' would make sense for 'switch'.
>
> Not so much.
Agreed, so I didn't propose changing that.
>> PROPOSAL
>>
>> - svn_ra_do_update2(): revise to svn_ra_do_update3(), add the
>> 'ignore_ancestry' option there too, and use two pools while we're at it.
>> Track this change in the RA vtable 'update' method, protocols, etc.
>>
>> - svn_wc__get_switch_editor(): add 'adds_as_modification'.
>>
>> - svn_client__update_internal(), svn_client_update4(): add
>> 'ignore_ancestry'.
>>
>> - svn_client__switch_internal(), svn_client_switch4(): add
>> 'adds_as_modification'.
>>
>>
>> RATIONALE
>>
>> Do I really have to explain why Consistency is Good?
>
> Well, actually, yes. :-) You need to establish that the consistency you
> seeks brings some other value -- easier maintenance, more code sharing,
> etc. -- beyond mere similarity.
Above all, it reduces the cognitive load on the developers -- and yes I'm thinking of myself.
As for code sharing, I observed that the RA methods 'update' and 'switch' are already implemented by delegation to a common 'update or switch' routine in RA-local and RA-serf, and by two very similar blocks of code in RA-svn. The more we can make their behaviour consistent with just one well defined difference (viz: switch all paths to the given URL, or don't if a URL is not given), the more this commonality can usefully be extended. For example, the svnserve protocol could have a single 'update-or-switch' command string, the RA vtable could be reduced to have a single 'update_or_switch' method, and so on.
> As I've already pointed out, some of your suggestions don't make sense.
> The dual-pool thing for update, and the 'adds_as_modification' for switch,
> remain as the ones that *do* make sense. So +1 to make those changes.
I'll wait for resolution of the 'ignore_ancestry' point under discussion.
- Julian
Received on 2013-03-13 18:37:18 CET