Ping. This submission has received no new comments.
On 22/02/2011, at 8:31 AM, Daniel Becroft wrote: > On Sun, Feb 13, 2011 at 2:15 AM, Daniel Shahaf <d...@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...@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'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. > > <3686_3.patch>
