You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2012/03/27 11:14:31 UTC
Re: Symmetric Merge -- no local mods or switched subtrees by default
Branko Čibej wrote:
> Perhaps we had a bit of a misunderstanding here ... My point was, the
> checks you mention have to always happen, not just optionally happen if
> one uses the (by now quite misnamed) --reintegrate option. I'm not
> arguing against sanity checks, quite the opposite, I'm arguing against
> skipping them by default.
Oh, OK, at least we're clear now! So, my views...
A 'mixed-rev' check is already done by default. There are two additional checks to consider at the moment, that are currently done for 'reintegrate' merges only. We might also consider adding further checks, such as whether the merge source or target has subtree mergeinfo that's inconsistent with its root mergeinfo, but let's leave such proposals for another thread and discuss here the two existing additional checks.
The check for local mods in the WC: I think we should enable this by default.
In the past I've felt that the user should be able to merge sub-ranges of the desired source in successive stages into a target WC, resolving any conflicts at each stage, and then commit the result. That requires merging into a target WC with local mods, for all but the first stage. That is also how Subversion's merge works internally: it breaks the merge source range down into successive sub-ranges if it needs to.
However, in typical daily usage I suspect that accidentally merging with local mods is far more common than wanting to do a merge in multiple stages, and so I think enabling this check by default would be a good trade-off for needing an extra command-line option to do multi-stage merges.
For exactly which merge commands should we enable this check? For all merges where a starting revision is not specified and merge tracking is used. I think these kinds of 'automatic' merges are the ones most commonly performed by non-expert users, and where this kind of simple safeguard is most effective and most needed.
I would suggest we should not enable it for other merges, to maximize backward compatibility. In particular I note that cherry-pick merges are often performed in batches (merge -c140 ^/trunk; merge -c155 ^/trunk; commit). In principle that is exactly the same as performing a 'sync' merge in stages, but in practice more common, and because cherry-picking is also seen as more 'advanced' merging I think the safety advantage of a no-local-mods check would be outweighed by the inconvenience.
The check for switched subtrees: I think we should enable this by default.
I suppose the issue here must be that we don't have a compelling use case for supporting such a merge, and so we haven't defined and don't want to spend the effort of trying to define what such a merge should mean. We can explain what the merge code *does* in such a case, we just can't give a requirements-based reason for *why* it does what it does. Paul or anyone, please enlighten me if that's not the case. If that is the case, then surely it must make sense to enable that check by default.
For exactly which merges? For similar reasons, and to keep the rules simple, I would sugegst this should apply to the same merges as for the no-local-mods check.
The above discussion focused on the user's needs. There's a second aspect, which is whether the checks are necessary to protect our implementation from cases it can't handle. That's irrelevant to the local-mods check, as the implementation *must* be able to merge into a WC with local mods because it internally breaks a given merge source range into successive sub-range merges. I don't know it if handles switched subtrees 'properly'.
This discussion doesn't seem to be specific to the 'symmetric' mode as far as I can see. The default checks of a '--symmetric' merge should be the same as those of a 'sync' merge, which should be a subset of or the same as those of a '--reintegrate' merge.
Thoughts?
- Julian
Re: Symmetric Merge -- no local mods or switched subtrees by default
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 27, 2012 at 10:14:31AM +0100, Julian Foad wrote:
> Oh, OK, at least we're clear now! So, my views...
[...]
> Thoughts?
I agree.
Re: Symmetric Merge -- switched subtrees [was: no local mods or
switched subtrees by default]
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Apr 20, 2012 at 1:12 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Hi Paul, just one clarification request on a very brief skimmed reading:
>
>> * If the incoming change deletes the path SS, then it is interpreted as
>> deleting the entry SS from SSP's list of children.
>>
>> Correct, SS@BASE is deleted, *but* the repository location represented
>> by the switched subtree is also deleted -- Both are deleted.
>
> Are you saying that if we commit this merge, it will delete the repository path SS from SS's parent-in-the-repository, and also delete the repository path SS@BASE from SS@BASE's parent-in-the-repository? Well, I never would have guessed that!
Yes, that's what I'm saying. Again, merge has always worked this way,
nothing changed with the advent of merge-tracking.
Arguments can be made for either deletion I think:
1) Deleting the base: Unlike an edit to SS, a deletion doesn't require
the base, so even though the it's not "present" in the WC the merge
wants us to delete it, so we do.
2) Deleting the switched path from its parent-in-the-repostitory: If
this was an edit we'd make the change, why not with a delete?
Not saying this is ideal, just that when we are presented with this
particular use case this is what happens. Honestly I'm not sure what
the ideal behavior should be!
Paul
>> Not sure
>> what the ideal behavior is, but this is nothing new (it pre-dates
>> merge-tracking). Honestly this particular use-case fell through the
>> cracks, I don't recall it ever being discussed.
>
> OK, and thanks for the details; I'll study the rest on Monday.
>
> - Julian
Re: Symmetric Merge -- switched subtrees [was: no local mods or switched subtrees by default]
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Paul, just one clarification request on a very brief skimmed reading:
> * If the incoming change deletes the path SS, then it is interpreted as
> deleting the entry SS from SSP's list of children.
>
> Correct, SS@BASE is deleted, *but* the repository location represented
> by the switched subtree is also deleted -- Both are deleted.
Are you saying that if we commit this merge, it will delete the repository path SS from SS's parent-in-the-repository, and also delete the repository path SS@BASE from SS@BASE's parent-in-the-repository? Well, I never would have guessed that!
> Not sure
> what the ideal behavior is, but this is nothing new (it pre-dates
> merge-tracking). Honestly this particular use-case fell through the
> cracks, I don't recall it ever being discussed.
OK, and thanks for the details; I'll study the rest on Monday.
- Julian
Re: Symmetric Merge -- switched subtrees [was: no local mods or
switched subtrees by default]
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Apr 20, 2012 at 5:36 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Paul Burba wrote on 27 March 2012:
>> Julian Foad wrote:
>>> The check for switched subtrees: I think we should enable this by default.
>>>
>>> I suppose the issue here must be that we don't have a compelling use
>>> case for supporting such a merge, and so we haven't defined and don't
>>> want to spend the effort of trying to define what such a merge should
>>> mean. We can explain what the merge code *does* in such a case, we
>>> just can't give a requirements-based reason for *why* it does what it
>>> does. Paul or anyone, please enlighten me if that's not the case.
>>
>> There are two "whys" to consider here:
>>
>> First, "why" does a user merge into a WC with switched subtrees? On
>> that I agree with you, I've never seen a compelling case is. The few
>> users I've come across who did it usually did so by mistake or were
>> merging changes that they knew would not touch the switched subtree.
>
> OK; nor me. (I can see it could sometimes be convenient to merge into a WC with switched subtrees when the user knows that the merge will not touch the switched subtrees, but even so, that doesn't necessarily mean it's a good idea.)
>
>> Regardless of why users do it, they do it and it was something we
>> allowed merge to do pre-1.5 (i.e. pre-mergetracking), so I tried to
>> support it.
>>
>> Second: Why does the merge code do what it does when faced with a
>> switched subtree. I'm guessing you see this "what" the code does,
>> but
>> that makes it sound as if the current behavior is somehow arbitrary.
>
> Sorry, I did indeed imply it may be a bit unfounded, as I felt I had no point of reference for the "why". But I agree it makes sense to behave as you described below...
>
>> The "why" of the current behavior is tied to the fact that by default
>> svn:mergeinfo is inheritable. [...]:
>>
>> WC-Root('/SRC:N')
>> |
>> SSP('/SRC:N*')
>> | \
>> | \
>> | SSS('/SRC:N')
>> |
>> SS('/SRC:N')
>>
>> a) Non-inheritable mergeinfo is recorded on the parent [...]
>>
>> b) Normal inheritable mergeinfo is recorded on the root [...]
>>
>> c) Normal inheritable mergeinfo is recorded on siblings [...]
>>
>> d) If the WC-Root isn't the parent [it gets normal mergeinfo ...]
> [...]
>> [1] There are other reasons a subtree might be missing [...]
>> mergetracking behavior and reasoning for this behavior is the same in
>> all these cases, though a bit simpler in the non-switched cases
>> because the subtree is really missing and not "replaced" by something
>> else.
>
> This description would make a superior replacement for the
> first two bullet points of
> <http://svn.apache.org/repos/asf/subversion/trunk/notes/merge-tracking/func-spec.html#switched-paths>,
> would it not?
>
> There is an implied logic for the question "Why behave like this?" that seems pretty intuitive; let's see if I can actually capture it in words:
>
> [[[
> If a subtree (SS) is switched, that means the user has chosen for
> the time being to work with a substitute for the original subtree
> (SS@BASE), knowing that any modifications made in SS can be
> committed only to the repository location of SS and the original
> subtree SS@BASE remains hidden and unaffected.
>
> The general semantics of a merge is to apply local modifications
> to the working copy and record the merge as having been applied
> to the tree that is represented by the working copy.
>
> Merge tracking should ensure that the subtree of the merge that
> goes into SS is recorded as being applied to SS, while the
> subtree SS@BASE should be recorded as not having received that
> merge.
This makes a bit more sense to me:
"Merge tracking should ensure that any changes made by the merge to SS
are recorded as merged to the repository location which SS actually
represents while the subtree SS@BASE should be recorded as not having
received that merge."
> Since the working copy represents parts of two different branches,
> two parts of the merge are thus applied to the two different
> branches, and recorded as such when the user commits the result.
>
> If the user is doing a merge that may affect SS, it is reasonable
> to assume that SS is an alternative variant of SS@BASE rather than
> some totally unrelated item. So, in terms of Subversion's loose
> branching semantics, SS is a 'branch' of SS@BASE. If the user
> chooses to merge when the assumption is false and SS doesn't have
> a sensible branching relationship with SS@BASE, the result will be
> nonsensical or, in concrete terms, there will be merge conflicts.
>
> Note: Many typical branching policies would forbid committing to
> two branches at once, let alone committing merges to two branches
> at once. However, the user may have reasons for doing this merge
> without intending to commit the result as-is.
> ]]]
>
>
> Your description cover cases where the merge may or may not affect the inside of the subtree. One case not made explicit is where the merge deletes the switched subtree. Would the rule there be:
>
> * If the incoming change deletes the path SS, then it is interpreted as deleting the entry SS from SSP's list of children.
Correct, SS@BASE is deleted, *but* the repository location represented
by the switched subtree is also deleted -- Both are deleted. Not sure
what the ideal behavior is, but this is nothing new (it pre-dates
merge-tracking). Honestly this particular use-case fell through the
cracks, I don't recall it ever being discussed.
> Therefore SS is removed from the WC, and so is no longer a switched path, and the mergeinfo change can be recorded normally on SSP without any need for non-inheritable or subtree mergeinfo.
Agreed, non-inheritable mergeinfo should not be necessary, but
non-inheritable mergeinfo is recorded. I filed an issue for this:
http://subversion.tigris.org/issues/show_bug.cgi?id=4163
> And as for the repository path that SS represented while it was still switched, we do not want to record any mergeinfo change on that path at all
We couldn't record anything on that path anyway, because we deleted it.
> (nor on that path's repository-parent path,
In a perfect world we'd have a way to record this, since a merge *did*
remove one of it's children, and that information is lost, but this
seems very much an edge case.
Paul
> nor anything to do with it).
>
> ?
>
> - Julian
Re: Symmetric Merge -- switched subtrees [was: no local mods or switched subtrees by default]
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote on 27 March 2012:
> Julian Foad wrote:
>> The check for switched subtrees: I think we should enable this by default.
>>
>> I suppose the issue here must be that we don't have a compelling use
>> case for supporting such a merge, and so we haven't defined and don't
>> want to spend the effort of trying to define what such a merge should
>> mean. We can explain what the merge code *does* in such a case, we
>> just can't give a requirements-based reason for *why* it does what it
>> does. Paul or anyone, please enlighten me if that's not the case.
>
> There are two "whys" to consider here:
>
> First, "why" does a user merge into a WC with switched subtrees? On
> that I agree with you, I've never seen a compelling case is. The few
> users I've come across who did it usually did so by mistake or were
> merging changes that they knew would not touch the switched subtree.
OK; nor me. (I can see it could sometimes be convenient to merge into a WC with switched subtrees when the user knows that the merge will not touch the switched subtrees, but even so, that doesn't necessarily mean it's a good idea.)
> Regardless of why users do it, they do it and it was something we
> allowed merge to do pre-1.5 (i.e. pre-mergetracking), so I tried to
> support it.
>
> Second: Why does the merge code do what it does when faced with a
> switched subtree. I'm guessing you see this "what" the code does,
> but
> that makes it sound as if the current behavior is somehow arbitrary.
Sorry, I did indeed imply it may be a bit unfounded, as I felt I had no point of reference for the "why". But I agree it makes sense to behave as you described below...
> The "why" of the current behavior is tied to the fact that by default
> svn:mergeinfo is inheritable. [...]:
>
> WC-Root('/SRC:N')
> |
> SSP('/SRC:N*')
> | \
> | \
> | SSS('/SRC:N')
> |
> SS('/SRC:N')
>
> a) Non-inheritable mergeinfo is recorded on the parent [...]
>
> b) Normal inheritable mergeinfo is recorded on the root [...]
>
> c) Normal inheritable mergeinfo is recorded on siblings [...]
>
> d) If the WC-Root isn't the parent [it gets normal mergeinfo ...]
[...]
> [1] There are other reasons a subtree might be missing [...]
> mergetracking behavior and reasoning for this behavior is the same in
> all these cases, though a bit simpler in the non-switched cases
> because the subtree is really missing and not "replaced" by something
> else.
This description would make a superior replacement for the
first two bullet points of
<http://svn.apache.org/repos/asf/subversion/trunk/notes/merge-tracking/func-spec.html#switched-paths>,
would it not?
There is an implied logic for the question "Why behave like this?" that seems pretty intuitive; let's see if I can actually capture it in words:
[[[
If a subtree (SS) is switched, that means the user has chosen for
the time being to work with a substitute for the original subtree
(SS@BASE), knowing that any modifications made in SS can be
committed only to the repository location of SS and the original
subtree SS@BASE remains hidden and unaffected.
The general semantics of a merge is to apply local modifications
to the working copy and record the merge as having been applied
to the tree that is represented by the working copy.
Merge tracking should ensure that the subtree of the merge that
goes into SS is recorded as being applied to SS, while the
subtree SS@BASE should be recorded as not having received that
merge.
Since the working copy represents parts of two different branches,
two parts of the merge are thus applied to the two different
branches, and recorded as such when the user commits the result.
If the user is doing a merge that may affect SS, it is reasonable
to assume that SS is an alternative variant of SS@BASE rather than
some totally unrelated item. So, in terms of Subversion's loose
branching semantics, SS is a 'branch' of SS@BASE. If the user
chooses to merge when the assumption is false and SS doesn't have
a sensible branching relationship with SS@BASE, the result will be
nonsensical or, in concrete terms, there will be merge conflicts.
Note: Many typical branching policies would forbid committing to
two branches at once, let alone committing merges to two branches
at once. However, the user may have reasons for doing this merge
without intending to commit the result as-is.
]]]
Your description cover cases where the merge may or may not affect the inside of the subtree. One case not made explicit is where the merge deletes the switched subtree. Would the rule there be:
* If the incoming change deletes the path SS, then it is interpreted as deleting the entry SS from SSP's list of children. Therefore SS is removed from the WC, and so is no longer a switched path, and the mergeinfo change can be recorded normally on SSP without any need for non-inheritable or subtree mergeinfo. And as for the repository path that SS represented while it was still switched, we do not want to record any mergeinfo change on that path at all (nor on that path's repository-parent path, nor anything to do with it).
?
- Julian
Re: Symmetric Merge -- no local mods or switched subtrees by default
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Mar 27, 2012 at 5:14 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Branko Čibej wrote:
>> Perhaps we had a bit of a misunderstanding here ... My point was, the
>> checks you mention have to always happen, not just optionally happen if
>> one uses the (by now quite misnamed) --reintegrate option. I'm not
>> arguing against sanity checks, quite the opposite, I'm arguing against
>> skipping them by default.
>
>
> Oh, OK, at least we're clear now! So, my views...
>
> A 'mixed-rev' check is already done by default. There are two additional checks to consider at the moment, that are currently done for 'reintegrate' merges only. We might also consider adding further checks, such as whether the merge source or target has subtree mergeinfo that's inconsistent with its root mergeinfo, but let's leave such proposals for another thread and discuss here the two existing additional checks.
>
> The check for local mods in the WC: I think we should enable this by default.
>
> In the past I've felt that the user should be able to merge sub-ranges of the desired source in successive stages into a target WC, resolving any conflicts at each stage, and then commit the result. That requires merging into a target WC with local mods, for all but the first stage. That is also how Subversion's merge works internally: it breaks the merge source range down into successive sub-ranges if it needs to.
>
> However, in typical daily usage I suspect that accidentally merging with local mods is far more common than wanting to do a merge in multiple stages, and so I think enabling this check by default would be a good trade-off for needing an extra command-line option to do multi-stage merges.
>
> For exactly which merge commands should we enable this check? For all merges where a starting revision is not specified and merge tracking is used. I think these kinds of 'automatic' merges are the ones most commonly performed by non-expert users, and where this kind of simple safeguard is most effective and most needed.
That seems reasonable and fits with what I have observed users doing.
> I would suggest we should not enable it for other merges, to maximize backward compatibility. In particular I note that cherry-pick merges are often performed in batches (merge -c140 ^/trunk; merge -c155 ^/trunk; commit). In principle that is exactly the same as performing a 'sync' merge in stages, but in practice more common, and because cherry-picking is also seen as more 'advanced' merging I think the safety advantage of a no-local-mods check would be outweighed by the inconvenience.
Agreed.
> The check for switched subtrees: I think we should enable this by default.
>
> I suppose the issue here must be that we don't have a compelling use case for supporting such a merge, and so we haven't defined and don't want to spend the effort of trying to define what such a merge should mean. We can explain what the merge code *does* in such a case, we just can't give a requirements-based reason for *why* it does what it does. Paul or anyone, please enlighten me if that's not the case.
There are two "whys" to consider here:
First, "why" does a user merge into a WC with switched subtrees? On
that I agree with you, I've never seen a compelling case is. The few
users I've come across who did it usually did so by mistake or were
merging changes that they knew would not touch the switched subtree.
Regardless of why users do it, they do it and it was something we
allowed merge to do pre-1.5 (i.e. pre-mergetracking), so I tried to
support it.
Second: Why does the merge code do what it does when faced with a
switched subtree. I'm guessing you see this "what" the code does, but
that makes it sound as if the current behavior is somehow arbitrary.
The "why" of the current behavior is tied to the fact that by default
svn:mergeinfo is inheritable. So if any subtree of a merge target is
"missing"[1] the merge needs to record svn:mergeinfo to accurately
describe what revisions were and weren't merged to what parts of the
WC. We do this with non-inheritable mergeinfo and/or subtree
mergeinfo:
(Julian I suspect you already know this, but bear with me)
For an arbitrary switched subtree SS:
SSP - Switched Subtree Parent (may be same as WC-Root)
SSPS - Switched Subtree Sibling (zero or more)
SS - Switched Subtree
WC-Root
|
SSP
| \
| \
SS SSS
If we merge revision N from ^/SRC, mergeinfo is recorded like so:
WC-Root('/SRC:N')
|
SSP('/SRC:N*')
| \
| \
| SSS('/SRC:N')
|
SS('/SRC:N')
a) Non-inheritable mergeinfo is recorded on the parent of the switched
subtree. Why? Because the unswitched subtree beneath the parent
obviously didn't have rN merged to it. If this merge were committed,
the switched subtree unswitched, and the merge repeated, this allows
the merge logic to realize that the previously switched subtree never
had rN merged to it, and so will permit rN to be merged to that
subtree only.
b) Normal inheritable mergeinfo is recorded on the root of the
switched subtree. Why? Because that repository location now has
changes merged to it from ^/SRC, but since switches are a strictly
client side concept the only way to record that this merge was done is
to add explicit mergeinfo to switched location.
c) Normal inheritable mergeinfo is recorded on switched subtree's
siblings. Why? Because the common parent of the switched root and
it's siblings only has non-inheritable mergeinfo recorded. So to
accurately record the merge we need to add explicit mergeinfo to the
sibling. If we didn't, then a repeat merge of ^/SRC -cN would attempt
to remerge rN to that subtree, possibly resulting in spurious
conflicts.
d) If the WC-Root isn't the parent of the switched subtree it gets
normal inheritable mergeinfo (i.e. as it would if the WC had no
switched subtrees).
Note: 1.5 did this in a brute-force manner, recording the
non-inheritable and subtree mergeinfo unconditionally. Since then the
code has gotten a smarter and attempts to minimize the amount of
subtree and non-inheritable mergeinfo recorded. For example, it won't
bother recording non-inheritable or subtree mergeinfo if the "missing"
switched subtree and/or its siblings wouldn't have been effected by
the merge if it was present.
[1] There are other reasons a subtree might be missing of course: The
WC might be shallow, the user might not have access to them, or the WC
might not be shallow but the operational depth of the merge might be <
infinity (the subtrees may not be literally missing from the WC in
this last case, but they are effectively off-limits as far as
application of the diff, so they might as well not be there). The
mergetracking behavior and reasoning for this behavior is the same in
all these cases, though a bit simpler in the non-switched cases
because the subtree is really missing and not "replaced" by something
else.
So while it's wonderful that we may now disallow merges to WCs with
switched subtrees by default, unless we plan to forbid it entirely we
still need to account for the switched subtrees when recording
mergeinfo describing a merge.
> If that is the case, then surely it must make sense to enable that check by default.
>
> For exactly which merges? For similar reasons, and to keep the rules simple, I would sugegst this should apply to the same merges as for the no-local-mods check.
+1
Paul
> The above discussion focused on the user's needs. There's a second aspect, which is whether the checks are necessary to protect our implementation from cases it can't handle. That's irrelevant to the local-mods check, as the implementation *must* be able to merge into a WC with local mods because it internally breaks a given merge source range into successive sub-range merges. I don't know it if handles switched subtrees 'properly'.
>
> This discussion doesn't seem to be specific to the 'symmetric' mode as far as I can see. The default checks of a '--symmetric' merge should be the same as those of a 'sync' merge, which should be a subset of or the same as those of a '--reintegrate' merge.
>
>
> Thoughts?
>
> - Julian
>