> -----Original Message-----
> From: Neels J Hofmeyr [mailto:neels_at_elego.de]
> Sent: Sunday, November 23, 2008 3:55 AM
> To: Bert Huijben
> Cc: dev_at_subversion.tigris.org
> Subject: Re: svn commit: r34332 - in branches/tc_url_rev/subversion:
> include libsvn_wc
>
> Hi Bert,
>
> thanks for your help on branch tc_url_rev!
>
> However, I see a problem with ... (see inline comment)
>
...
> > Index: subversion/libsvn_wc/merge.c
> > ===================================================================
> > --- subversion/libsvn_wc/merge.c (revision 34331)
> > +++ subversion/libsvn_wc/merge.c (revision 34332)
> > @@ -271,7 +271,9 @@ svn_error_t *
> > svn_wc__merge_internal(svn_stringbuf_t **log_accum,
> > enum svn_wc_merge_outcome_t *merge_outcome,
> > const char *left,
> > + svn_wc_conflict_version_t *left_version,
> > const char *right,
> > + svn_wc_conflict_version_t *right_version,
> > const char *merge_target,
> > const char *copyfrom_text,
> > svn_wc_adm_access_t *adm_access,
> > @@ -424,6 +426,9 @@ svn_wc__merge_internal(svn_stringbuf_t
> **log_accum
> > cdesc->my_file = tmp_target;
> > cdesc->merged_file = result_target;
> >
> > + cdesc->older_version = left_version;
> > + cdesc->their_version = right_version;
>
> ... this! Many callers pass NULL for left_version and right_version.
All callers pass null to keep the current behavior.
> The conflict description is currently not allowed to have NULL for
> older_version or their_version; originally, they weren't pointers but
> inline
> structs, and no code checks this for NULLness, because stsp made them
> to be
> pointers and didn't add NULL checking (AFAIK).
Where is this documented?
Leaving unused structure references null is very common in libsvn_wc and other subversion libraries.
svn_wc__conflict_description_dup assumes older_version and newer_version can be null and svn_wc_conflict_description_create_text and svn_wc_conflict_description_create_prop create the field with value null, so I didn't change any behavior yet.
It was ALWAYS NULL in this case. Where is the problem?
>
> I haven't yet understood why these can be omitted in some cases.
> Couldn't we
> instead send an "empty" dummy struct or something?
Why allocate a structure if all fields have their default value? I would have to check 4 variables instead of just 1 when I want to know if I can show a peg URL to a user from a conflict handler.
> I am concerned about allowing these to be NULL because it adds a lot of
> "if"
> statements everywhere around and increases the risk of new errors. See
> also
> my mail to stsp sent just now, which is about r34326.
As I said, I didn't change behavior, I just opened the option of filling the structure for non tree conflicts. Before this commit the field would always be null in all these cases and it still is. (But I can start passing the information from update and switch in a future commit; that is the reason for this change)
> Could you take another look at it and/or explain to me why the NULLness
> is
> needed? Keep in mind that OLDER_VERSION and THEIR_VERSION might become
> non-pointers again; in case I'm right, that is.
* The svn_wc_conflict_description is not a tree conflicts only structure
* If we would make it an inline structure it can't be copied and it can't be extended in later subversion releases without revving the entire conflict api.
Bert
>
> Thanks!
> ~Neels
>
>
> > +
> > SVN_ERR(conflict_func(&result, cdesc, conflict_baton,
> pool));
> > if (result == NULL)
> > return
> svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
> > @@ -732,6 +737,9 @@ svn_wc__merge_internal(svn_stringbuf_t
> **log_accum
> > cdesc->my_file = tmp_target;
> > cdesc->merged_file = NULL; /* notice there is NO
> merged file! */
> >
> > + cdesc->older_version = left_version;
> > + cdesc->their_version = right_version;
> > +
> > SVN_ERR(conflict_func(&result, cdesc, conflict_baton,
> pool));
> > if (result == NULL)
> > return
> svn_error_create(SVN_ERR_WC_CONFLICT_RESOLVER_FAILURE,
> > @@ -910,7 +918,9 @@ svn_wc_merge3(enum svn_wc_merge_outcome_t
> *merge_o
> > svn_stringbuf_t *log_accum = svn_stringbuf_create("", pool);
> >
> > SVN_ERR(svn_wc__merge_internal(&log_accum, merge_outcome,
> > - left, right, merge_target,
> > + left, NULL,
> > + right, NULL,
> > + merge_target,
> > NULL,
> > adm_access,
> > left_label, right_label,
> target_label,
> > Index: subversion/libsvn_wc/log.c
> > ===================================================================
> > --- subversion/libsvn_wc/log.c (revision 34331)
> > +++ subversion/libsvn_wc/log.c (revision 34332)
> > @@ -563,10 +563,10 @@ log_do_merge(struct log_runner *loggy,
> >
> > /* Now do the merge with our full paths. */
> > err = svn_wc__merge_internal(&log_accum, &merge_outcome,
> > - left, right, name, NULL, loggy-
> >adm_access,
> > - left_label, right_label,
> target_label,
> > - FALSE, loggy->diff3_cmd, NULL, NULL,
> > - NULL, NULL, loggy->pool);
> > + left, NULL, right, NULL, name, NULL,
> > + loggy->adm_access, left_label,
> right_label,
> > + target_label, FALSE, loggy-
> >diff3_cmd, NULL,
> > + NULL, NULL, NULL, loggy->pool);
> > if (err && loggy->rerun && APR_STATUS_IS_ENOENT(err->apr_err))
> > {
> > svn_error_clear(err);
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> > For additional commands, e-mail: dev-help_at_subversion.tigris.org
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-23 11:23:43 CET