Re: [RFC] Issue #3603 Fix - Should we do more?

On Wed, Apr 14, 2010 at 4:49 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Wed, Apr 14, 2010 at 3:49 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>> C. Michael Pilato wrote:
>>> Paul Burba wrote:
>>>> On Wed, Apr 14, 2010 at 2:34 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>>>>> Paul Burba wrote:
>>>>>> In a perfect world maybe we'd give a error along the lines of 'hey,
>>>>>> you are trying to reintegrate into a shallow WC and some of the paths
>>>>>> affected by the merge aren't present, you are going to get tree
>>>>>> conflicts, is this really what you want? :-)'
>>>>>>
>>>>>> But going this route adds more merge special casing and obviously has
>>>>>> a performance penalty, two things we definitely don't need more of.
>>>>> Can we give this feedback at the time of the conflict rather than up front?
>>>>>  That is, can we avoid the performance penalty of an upfront merge forecast
>>>>> but still tell folks, when they get those tree conflicts, "Hey, you could
>>>>> avoid this kind of conflict by simply not having directory FOO missing by
>>>>> sparse configuration; go flesh that sucker out and retry this reintegration."
>>>> Mike,
>>>>
>>>> Do you mean to let the merge complete and give the warning at the
>>>> *end* rather than stopping the merge on the first tree conflict due to
>>>> a missing subtree-caused-by-a-shallow-WC?  After all, the user might
>>>> not care about some tree conflicts and want the merge to complete as
>>>> best it can.
>>>
>>> Oh, gosh, no.  I think my comment is less about your proposed change and
>>> more of a general tree conflict thing.  I would feel more warm and fuzzy if
>>> I knew that the tree conflict information that we leave around is clear
>>> about the reason for the conflict.  That a missing-due-to-sparse-checkouts
>>> directory is the reason for the conflict, not just some generic "something
>>> is missing" note.  That way folks can immediately know, if not by explicit
>>> recommendation stored in the conflict information then at least by
>>> inference, that they could probably avoid the conflict by reverting the
>>> merge, de-shallowing the directories that the merge wished were present, and
>>> then repeating the merge.
>>
>> Had I read your question with my brain fully engaged in the task, I think I
>> would have chosen different words of response.  I certainly wouldn't have
>> started off with "Oh, gosh, no".  Sorry, Paul.
>>
>> I think it would be great if there was a way at the end of the merge to say,
>> "Hey, we notice that you had some tree conflicts, all of the variety caused
>> by directories missing-due-to-shallowness.  Here's a recommended fix, if you
>> care."
>
> Ok, I got you now.  I agree that would be nice to have.  In the same
> vein, it would probably be beneficial to flag tree conflicts
> due-to-missing-subtrees-due-to-auth restrictions.  This is probably
> quite rare, but wow, talk about confusing:
>
> I have an unmodified, single-rev WC:
>
>  >svn up
>  At revision 5.
>
>  >svn st
>
> I merge all available revs:
>
>  >svn merge ^^/A A_COPY
>     C A_COPY\D\H
>  --- Recording mergeinfo for merge of r2 through r5 into 'A_COPY':
>   U   A_COPY
>  --- Recording mergeinfo for merge of r2 through r5 into 'A_COPY\D':
>   U   A_COPY\D
>  Summary of conflicts:
>    Tree conflicts: 1
>
> Local delete?
>
>  >svn st
>   M      A_COPY
>   M      A_COPY\D
>  !     C A_COPY\D\H
>        >   local delete, incoming edit upon merge
>  Summary of conflicts:
>    Tree conflicts: 1
>
> Nothing has been deleted on this branch since it was copied!
>
>  >svn log -v -r1:HEAD A_COPY
>  ------------------------------------------------------------------------
>  r1 | jrandom | 2010-04-14 16:23:08 -0400 (Wed, 14 Apr 2010) | 1 line
>  Changed paths:
>     A /A
>     A /A/B
>     A /A/B/E
>     A /A/B/E/alpha
>     A /A/B/E/beta
>     A /A/B/F
>     A /A/B/lambda
>     A /A/C
>     A /A/D
>     A /A/D/G
>     A /A/D/G/pi
>     A /A/D/G/rho
>     A /A/D/G/tau
>     A /A/D/H
>     A /A/D/H/chi
>     A /A/D/H/omega
>     A /A/D/H/psi
>     A /A/D/gamma
>     A /A/mu
>     A /iota
>
>  Log message for revision 1.
>  ------------------------------------------------------------------------
>  r2 | jrandom | 2010-04-14 16:23:14 -0400 (Wed, 14 Apr 2010) | 1 line
>  Changed paths:
>     A /A_COPY (from /A:1)
>
>  log msg
>  ------------------------------------------------------------------------
>  r3 | jrandom | 2010-04-14 16:23:16 -0400 (Wed, 14 Apr 2010) | 1 line
>  Changed paths:
>     M /A_COPY/mu
>
> Good luck guessing that the problem is that you are not authorized to
> a subtree in your target:
>
>  >type ..\..\repositories\merge_tests-83\conf\authz
>  [/]
>  * = rw
>  [/A_COPY/D/H]
>  * =
>
> Of course in that case our recommended course of action would be "svn:
> Wow, your authz configuration and/or repository configuration is
> insane! Go throttle the person responsible for it until they change
> it".
>
>> Do we have that kind of data and opportunity available?
>
> From Stefan's reply elsewhere in this thread I gather the answer is
> currently no re the data.  Re the opportunity, not sure I totally
> understand you there, but at the very end of
> libsvn_client/merge.c:do_directory_merge() would be the place to do
> it.
>
> Paul

Since the whole "post-merge recommended fix" discussion here is really
a separate issue from #3603, I went ahead and committed the patch from
the start of this thread,
http://svn.apache.org/viewvc?view=revision&revision=934454

I also created a new issue to track the "post-merge help" question,
http://subversion.tigris.org/issues/show_bug.cgi?id=3618

Paul
Received on 2010-04-15 17:35:49 CEST