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 ^/ ./
> /* 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...
(that depends on where exactly the local file is missing --- in BASE, in
WORKING, or in ACTUAL)
> > > + SVN_ERR(svn_wc__wq_build_file_install(&work_item,
> > > + db, target_abspath,
> > > + right_abspath,
> > > + FALSE /*
> > use_commit_times */,
> > > + FALSE /* record_fileinfo
> > */,
> >
> > About RECORD_FILEINFO:
> >
> > * Why are you passing FALSE? (I see that other calls do this too, so
> > this is for my education mainly)) A quick scan makes me expect that
> > parameter to control the mtime/size caches for modification detection.
> >
>
> I copied this directly from the call further down, where a file is copied
> depending on which version to use in a conflict. Even if "theirs" is used,
> we still pass in FALSE for record_fileinfo.
>
Because that call is also used for the svn_wc_conflict_choose_postpone,
which installs a non-pristine file.
> The RECORD_FILEINFO flag controls whether the file's information should be
> recorded into the wc.db. It seems that we only pass TRUE to this if we are
> installing a fresh file. It makes sense to pass in FALSE during a merge,
> otherwise the file won't be seen as modified.
>
Doh. +1.
> PS: The two calls in update_editor.c don't pass in NULL, but pass in a value
> based on comparing against NULL.
>
> record_fileinfo = new_contents == NULL;
> record_fileinfo = install_from == NULL;
>
Oops. Added parentheses now. Thanks.
> * In update_editor.c there are two places that pass NULL (not FALSE) for
> > that, and have above them comment that says that "If %s, we want to
> > pass FALSE". What is going on there?
> > (this is to the list, not specifically at Daniel)
> >
> > > + result_pool,
> > scratch_pool));
> > > + *work_items = svn_wc__wq_merge(*work_items, work_item,
> > result_pool);
> >
> > Don't you need to convert the next svn_wc__wq_build_file_install() to
> > use this 'build-then-merge' approach too? Otherwise, won't it overwrite
> > your WORK_ITEMS?
> >
>
> Which svn_wc__wq_build_file_install() call do you mean?
>
The only other call in the same function (merge_binary_file()).
> We return SVN_NO_ERROR immediately after building adding this workqueue
> item, so the other one for the binary file conflict resolution will not get
> called.
>
Ah, right. I missed that. I'll not pursue this point further then.
> > > @@ -1502,6 +1463,9 @@ merge_file_changed(const char *local_dir_abspath,
> > > const svn_wc_conflict_version_t *left;
> > > const svn_wc_conflict_version_t *right;
> > >
> > > + SVN_ERR(svn_wc_text_modified_p2(&has_local_mods,
> > merge_b->ctx->wc_ctx,
> > > + mine_abspath, FALSE, subpool));
> > > +
> >
> > 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.)
