Daniel Becroft wrote on Sat, Feb 12, 2011 at 08:37:12 +1000:
> On Sat, Feb 12, 2011 at 7:31 AM, Daniel Shahaf
> <d...@daniel.shahaf.name>wrote:
>
> > Daniel Becroft wrote on Sat, Feb 12, 2011 at 06:27:31 +1000:
> > > On Fri, Feb 11, 2011 at 11:26 PM, Daniel Shahaf <d...@daniel.shahaf.name
> > >wrote:
> > > > Daniel Becroft wrote on Thu, Feb 10, 2011 at 07:21:30 +1000:
> > > > > @@ -1118,6 +1120,33 @@ merge_binary_file(svn_skel_t **work_items,
> > > > > + /* Attempt to merge the binary file. At the moment, we can only
> > > > > + handle the special case: if the LEFT side of the merge is equal
> > > > > + to WORKING, then we can copy RIGHT directly. */
> > > >
> > > > The comment in libsvn_client mentioned two special case, what happened
> > > > to the other one? Does the existing wc code already handle it? (I'd be
> > > > surprised)
> > > >
> > > > - Alternately, if the 'left' side of the merge doesn't exist
> > in
> > > > - the repository, and the 'right' side of the merge is
> > > > - identical to the WC, pretend we did the merge (a no-op).
> > > >
> > >
> > > I've been trying to think of a valid scenario for this to occur, but I
> > can't
> > > seem to think of one. There's a comment further up:
> > >
> >
> > Concocted:
> > % svn add foo.bin
> > % svn ci -m r5
> > % svn rm ^/foo.bin -m r6
> > % svnmucc -m r7 put foo.bin ^/foo.bin
> > % svn merge -c7 ^/ ./
> >
>
> Maybe it's early, and the coffee hasn't kicked in yet, but wouldn't (and
> shouldn't) this give a tree-conflict? foo.bin@7 and foo.bin in the WC are
> two different nodes (with the same name).
>
I hadn't considered that this might be a tree conflict (an add of an
already-existing file with the same contents). My point was not the
tree conflict but that the scenario described in the "Alternately" can
happen.
> I just tried the above (without svnmucc, but just a svn rm and svn add) with
> both 1.6.x, and trunk, and both raised a tree conflict:
>
> Using 1.6.x:
> --- Merging r3 into '.':
> C foo.txt
> Summary of conflicts:
> Tree conflicts: 1
>
> Using trunk:
> --- Merging r3 into '.':
> C foo.bin
> --- Recording mergeinfo for merge of r3 into '.':
> U .
> Summary of conflicts:
> Tree conflicts: 1
>
> Replacing the -c with a -r in the 'svn merge' command gives a status of "R
> foo.bin", which is correct.
>
Thanks for testing this :)
>
> > > /* Other easy outs: if the merge target isn't under version
> > > control, or is just missing from disk, fogettaboutit. There's no
> > > way svn_wc_merge4() can do the merge. */
> > >
> > > This should apply to all situations (binary and text files), so I think
> > this
> > > second "special case" is redundant.
> > >
> >
> > Not sure. If the merge-left does not exist, and the local file doesn't
> > exist either, then the situation is 'compatible' and can be merged...
> >
>
> Isn't this just an incoming add (which is handled by a different function)?
>
I don't know the code very well. If you're saying that at the point of
the comment we know that the merge-left exists, then I agree that it
makes sense to flag a conflict if the merge target is non-existent.
> > (that depends on where exactly the local file is missing --- in BASE, in
> > WORKING, or in ACTUAL)
> >
>
> I'll look into this a bit more. As I said, the behavior for binary files is
> now the same as text files, especially in terms of missing files in
> BASE/WORKING/ACTUAL. What that behavior is, I'm not 100% sure on.
>
Heh. Fair enough for me :-)
> <snipped />
>
>
> > > > You only need HAS_LOCAL_MODS now when CONTENT_STATE is set. Shouldn't
> > > > you skip this call when CONTENT_STATE is NULL then? (It may perform
> > > > stat() or read().)
> > > >
> > >
> > > Agreed, however this was always the case. The only change I made to this
> > > section was indentation
> > > due to removing the surrounding if (merge_required) { } clause.
> >
> > Oh, you're right. I was confused because the 'diff -w' patch showed
> > these two lines as removed and later re-added; your original patch
> > shows it correctly.
> >
> > Sorry for my confusion.
> >
> > > I didn't want to make other changes that would be clouded.
> > >
> >
> > +1
> >
> > > I can submit a follow-up patch that fixes this as well, if necessary.
> >
> > That would be great, assuming that the FALSE *really* should be changed
> > to TRUE. (I haven't investigated that myself.)
> >
>
> Not sure what is is in reference to. I was thinking of just putting a if
> (content_state) before the local modifications check (again, coffee may not
> have kicked in yet).
>
I was trying to say: "It would be great if you could look further into
that matter, but I haven't done so myself yet so I don't know if a patch
is required or the current code is correct."
In other words, thanks for looking into this, but I don't know myself
whether a follow-up patch would be necessary.
> Cheers,
> Daniel^2.
HTH,
Daniel
