On Thu, 2011-09-08 at 12:37 -0400, Paul Burba wrote: > On Thu, Sep 8, 2011 at 12:08 PM, Julian Foad <julian.f...@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 > >> > >> > > > > > >