On Wed, Nov 5, 2008 at 12:08 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> On Wed, 2008-11-05 at 10:27 -0500, Mark Phippard wrote:
>> 2008/11/5 Paul Burba <ptburba_at_gmail.com>:
>> > The http://svn.collab.net/repos/svn/branches/reintegrate-improvements
>> > branch is ready to be merged back to trunk.
>> >
>> > This branch makes two significant changes:
>> >
>> > 1) Allow reintegrate to work when the reintegrate merge source has
>> > explicit subtree mergeinfo, as long as that mergeinfo represents that
>> > all of the target was previously merged to the root of the source.
>>
>> Yay!
>
> That's fantasic, Paul.
>
>> > 2) Improve the error message if a reintegrate merge is legitimately
>> > thwarted by subtree mergeinfo on the source. By "legitimately" I mean
>> > if you have done subtree merges from 'trunk' to 'some_branch' in such
>> > a way that 'some_branch' has not had all of 'trunk' uniformly merged
>> > to it. Previously if the reintegrate source had *any* explicit
>> > mergeinfo we returned an error like this:
>> >
>> > C:\SVN\src-trunk-mirror-wc>svn merge --reintegrate --accept postpone
>> > http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees
>> > .
>> > svn: Cannot reintegrate from
>> > 'http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees'
>> > yet:
>> > Some revisions have been merged under it that have not been merged
>> > into the reintegration target; merge them first, then retry.
>> >
>> > I changed text of this error on the reintegrate-improvements branch,
>> > but also added a dump of the offending subtree paths and their
>> > mergeinfo so now a user has some hope(?) of seeing what is wrong:
>>
>> Yay again!
>
> +1.
>
>> > C:\SVN\src-trunk-mirror-wc>svn merge --reintegrate
>> > http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees
>> > .
>> > svn: Reintegrate can only be used if the revisions previously merged
>> > from the reintegrate target to
>> > 'http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees
>> > are the same', but there are differences:
>> > branches/issue-3067-deleted-subtrees
>> > /trunk:33301-33960
>> > branches/issue-3067-deleted-subtrees/subversion/libsvn_subr/win32_crypto.c
>> > /trunk/subversion/libsvn_subr/win32_crypto.c:33301-33923,33925-3396
>> >
>> > My hope is that a user could see the above and realize, "oh, I merged
>> > r33301-33960 from trunk to my branch, but only
>> > r33301-33923,33925-3396to win32_crypto.c, the latter needs r33924
>> > too." Of course in doing so the user violated the reintegrate
>> > workflow, maybe they reverse merged r33924 from trunk to
>> > win32_crypto.c, but at least we give them an idea of where to look for
>> > the problem.
>>
>> Why does the part about trunk show up? I find that confusing because
>> it sounds like it is flagging it as part of the problem and I do not
>> think you mean to say that.
>
> You mean why print "/trunk:..." (the root path of the merge) as well as
> "/trunk/subversion/lib...:..." (a subtree)? It's so the user can see the
> differences.
That's exactly it, I want to show the target's relative mergeinfo so
the user can see the problem with the subtree in comparison to it.
> Of course that leads to a suggestion for improvement: show the
> difference between each subtree's mergeinfo and the root's mergeinfo.
I like that idea, how to show it might be a bit tricky, how about this:
C:\SVN\src-trunk-mirror-wc>svn merge --reintegrate
http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees
.
svn: Reintegrate can only be used if the revisions previously merged
from the reintegrate target to
'http://localhost/local-trunk-mirror/trunk-mirror/branches/issue-3067-deleted-subtrees
are the same', but there are differences:
branches/issue-3067-deleted-subtrees
MERGED: /trunk:33301-33960
branches/issue-3067-deleted-subtrees/subversion/libsvn_subr/win32_crypto.c
MERGED: /trunk/subversion/libsvn_subr/win32_crypto.c:33301-33923,33925-3396
MISSING: /trunk/subversion/libsvn_subr/win32_crypto.c:33924
>> > Notice that only the subtree paths that differ from the root of the
>> > source are displayed. In the above example there are many other paths
>> > in branches/issue-3067-deleted-subtrees that have explicit mergeinfo,
>> > but they all show that r33301-33960 have been merged from trunk. Only
>> > the branches/issue-3067-deleted-subtrees/subversion/libsvn_subr/win32_crypto.c
>> > subtree differs. Notice also that all mergeinfo from other branches
>> > has been filtered out, only mergeinfo from trunk is shown as that is
>> > all that matters here.
>> >
>> > The one thing I dislike about this error is the size to which it can
>> > grow (and become completely useless) if the reintegrate source is
>> > vastly out of synch with the target -- see the attached output.
>> >
>> > Does this error make sense? Should we keep it? Any suggestions for improvement?
>>
>> I do not see how we can do anything about the possibly large
>> differences. As long as the information is correct, I do not see a
>> problem with it. Is it possible for the error message to describe
>> what they need to do next? Are there a specific set of revisions that
>> need to be merged into the "root" to resolve this subtree mergeinfo
>> problem?
>
> I think it's worth doing something to prevent ridiculously long error
> messages, as they might fill the user's console or dialogue box chock
> full of revision numbers so that the actual text of the message can't be
> seen. There are simple solutions: e.g. print up to 5 subtrees, followed
> by "and N more subtrees", and for each subtree print up to 10 rev-ranges
> followed by "...".
>
> But not before merging to trunk.
Agreed, we can work out the details later. Merged back to trunk in r34091.
>> I think the error message is fine. Here is an attempt at an alternative:
>>
>> svn: Reintegrate cannot be used with specified merge source due to
>> differences in subtree mergeinfo. Here are the differences that were
>> found:
>> branches/issue-3067-deleted-subtrees/subversion/libsvn_subr/win32_crypto.c
>> /trunk/subversion/libsvn_subr/win32_crypto.c:33301-33923,33925-3396
>>
>> I do not like this suggestion as is. I am trying to make it more to
>> the point but do not think I achieved that. I put it out there to
>> advance the conversation. I repeat that I think this should just go
>> to trunk.
I'm totally open to improving the static text of the error. You might
be on to something with not printing the whole URL. Maybe this?
svn: Reintegrate cannot be used because the target was not uniformly
merged to the source. The following inconsistencies were found:
branches/issue-3067-deleted-subtrees
MERGED: /trunk:33301-33960
branches/issue-3067-deleted-subtrees/subversion/libsvn_subr/win32_crypto.c
MERGED: /trunk/subversion/libsvn_subr/win32_crypto.c:33301-33923,33925-3396
MISSING: /trunk/subversion/libsvn_subr/win32_crypto.c:33924
>> I think you should probably merge this to trunk as is. I mainly offer
>> that last comment as a suggestion for where this could possibly go
>> next.
>
> +1. It's a _major_ improvement. Ship it!
>
> - Julian
On Wed, Nov 5, 2008 at 12:27 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> Alternative: don't print subtrees and rev-ranges at all, unless --verbose
> (or a new --bikeshed flag...) is given.
Daniel - I prefer we have the aforementioned detailed error message
(whatever its final form may be) by default. My thinking is that if
--reintegrate fails, and the user knows that --verbose will give them
additional information to solve the problem, then they are *always*
going to repeat the merge with --verbose no? So why not just give it
to them the first time?
Also, if you are dealing with an extremely large WC with a lot of
subtree mergeinfo it can still take some significant time to calculate
all this (due to the tree walk required). I think it might be
frustrating to do the merge, wait, get the error, then be forced to do
it again so you can get the useful error.
Lastly, I don't like tying the quality of the error message to an
option. Do we do that anywhere else? We'd have to add the -v option
to merge just for this.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-07 02:25:07 CET