On Thu, 2011-09-08 at 12:37 -0400, Paul Burba wrote:
> On Thu, Sep 8, 2011 at 12:08 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> > Can I file an issue for this?
>
> Hi Julian,
>
> What problem(s) is the current behavior causing? I understand your
> point, but I hesitate to add merge tracking awareness to diff unless
> there is some benefit.
I'm currently looking at merging from a high-level POV, looking at what
clues and information we give the users about what they're doing, that
hopefully guide them in doing the Right Thing and don't mislead and
distract them. That's where this comes in: I do a simple little merge
and run "svn diff" to check what's happened in the WC and suddenly it
says loads of stuff has been "merged", not the simple little merge that
I expected.
> > Philip said the server makes (or used to make?) the same mistake
> > internally when processing mergeinfo - presumably in the guts of the
> > ra_get_mergeinfo2 API?
> >
> > I assume the deletion (elision)
>
> Minor semantic nitpick, elision isn't synonymous with mergeinfo
> deletion. A svn:mergeinfo property might be deleted outside of merge
> tracking (e.g. svn pd).
Ack.
> Which makes me wonder: The current behavior is arguably wrong *if* the
> mergeinfo change in question was made by a merge tracking aware merge.
> But what if we simply delete a svn:mergeinfo property with 'svn pd'?
> Shouldn't the diff show that all the mergeinfo was removed in that
> case? Of course there is currently no fool-proof way to differentiate
> between a "real" mergetracking merge and manual edits of mergeinfo.
> Or do we just assume that all mergeinfo changes originate in merge
> tracking aware merges?
We have to treat mergeinfo as describing merges. Even if it was a
manual edit. There's no mileage in attempting to distinguish "real"
from "fake" merges; rather, the definition of a "merge" in terms of
tracking has to be "what a mergeinfo diff says". If the user's intended
text edits accompany that mergeinfo change, that's well and good, the
correct text change will be associated with the logical "merge" that's
recorded; if no text edits or the wrong ones accompany the logical
"merge" as described by the mergeinfo, then the user has partially lost
track of exactly when the merge happened and may have difficulties
selecting the right revisions to cherry-pick etc. That's just tough on
the user, we have no better way (short of dump+load) to do manual
mergeinfo edits.
So in "diff" we have a choice. Do we tell the user the physical
difference of a particular mergeinfo property, or do we interpret and
display what it means? It appears from the wording "Merged" that we are
attempting to display the meaning. If so, we're doing it wrong in the
cases where the property is added or removed.
If, instead, we simply want to show a diff of the mergeinfo property
itself rather than trying to interpret what it means, the current
behaviour would not be surprising. (Nor would it be particularly
useful; the raw change of mergeinfo in a particular prop is the sort of
thing the user generally doesn't want to know about, beyond the fact
that there is some change that they need to commit.) But then we should
not say "Merged" but rather "mergeinfo diff" or something.
I think the interpreted output is useful.
I share your hesitation about "add merge tracking awareness to diff" but
there definitely *is* a benefit in showing the user what's logically
changed.
- Julian
> > of a mergeinfo prop would generate an
> > all-revs-unmerged diff output, but haven't tested that.
>
> It does.
>
> Paul
>
> > - Julian
> >
> >
> > On Thu, 2011-09-08 at 16:59 +0100, Julian Foad wrote:
> >> If Subversion creates subtree mergeinfo on a path that previously didn't
> >> have any, then "svn diff" incorrectly shows all the source revs in that
> >> mergeinfo as newly "merged".
> >>
> >> In a Subversion trunk WC, using 1.7.0-rc2:
> >>
> >> $ svn merge -c 1000000 ^/subversion/branches/1.6.x/INSTALL INSTALL
> >> --- Merging r1000000 into 'INSTALL':
> >> U INSTALL
> >> --- Recording mergeinfo for merge of r1000000 into 'INSTALL':
> >> G INSTALL
> >>
> >> [Note that r1000000 is a no-op on trunk, so in fact no content change
> >> was made despite the 'U' marker.]
> >>
> >> $ svn diff INSTALL
> >> Index: INSTALL
> >> ===================================================================
> >> --- INSTALL (revision 1166027)
> >> +++ INSTALL (working copy)
> >>
> >> Property changes on: INSTALL
> >> ___________________________________________________________________
> >> Added: svn:mergeinfo
> >> Merged /subversion/branches/atomic-revprop/INSTALL:r965046-1000689
> >> Merged /subversion/branches/diff-callbacks3/INSTALL:r870059-870761
> >> Merged /subversion/branches/bdb-reverse-deltas/INSTALL:r872050-872529
> >> Merged /subversion/branches/double-delete/INSTALL:r870511-872970
> >> [...]
> >>
> >> This is wrong. The property certainly was added, but that does not mean
> >> all those revisions were merged. The expected output is something like:
> >>
> >> [...]
> >> Added: svn:mergeinfo
> >> Merged /subversion/branches/1.6.x/INSTALL:r1000000
> >>
> >>
> >> The bug is that "svn diff" shows a mergeinfo diff assuming that the
> >> previous merginfo was an explicit empty set of mergeinfo, but it wasn't,
> >> it was inherited mergeinfo.
> >>
> >> - Julian
> >>
> >>
> >
> >
> >
Received on 2011-09-08 22:40:16 CEST