On Tue, Mar 27, 2012 at 5:14 AM, Julian Foad <julianfoad_at_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
>
Received on 2012-03-27 17:54:02 CEST