On Sun, Feb 13, 2011 at 2:15 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name>wrote:
> 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.s_at_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.s_at_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_at_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've looked through this several times now to try and be sure that I'm not
missing anything.
Previously, the "Special case" block handled both when to merge the binary
file, as well as how. Now, the "when" logic is now the same logic as text
files, and the how has been moved into libsvn_wc (rather than
libsvn_client).
I've attached the file version of the patch.
> > > /* 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.
>
Yep, this is already done as part of the standard merge logic (ie why
doesn't left exist? It is an incoming add, or already deleted in the WC,
etc).
>
> > > > 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."
>
Gotcha (I just couldn't figure out where the TRUE/FALSE reference came
from), but anywho ...
> 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
Yep. I've attached the (final) version of the patch (generated with the C
functions this time), and the log message is below.
[[[
Fix issue #3686 - executable bit not set during merge.
The cause was the special case in libsvn_client, which bypassed the use of
the
workqueue. This logic has now been moved into libsvn_wc.
Additionally, this change allows the status of binary files (during a
dry-run
merge) to be reported correctly (previously, all binary files were reported
as
conflicted).
* subversion/libsvn_client/merge.c
(merge_file_changed): Remove binary-merge special case (now in libsvn_wc).
Remove merge_required variable (resulting in indentation changes).
* subversion/libsvn_wc/merge.c
(merge_binary_file): Add dry_run parameter. Add the special case merging
of binary files.
(svn_wc__internal_merge): Remove dry_run check for binary files, and pass
to
merge_binary_file instead.
* subversion/tests/cmdline/merge_tests.py
(merge_change_to_file_with_executable): Remove @XFail decorator.
]]]
Let me know if there's anything else I need to fix up.
Cheers,
Daniel B.
Received on 2011-02-21 22:32:14 CET