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)

Bert Huijben wrote:
> The mailer broke again on this commit.
>
> The real patch is at the bottom. (Very local changes instead of replacing a complete file)
>
> Bert
>
>> -----Original Message-----
>> From: rhuijben_at_tigris.org [mailto:rhuijben_at_tigris.org]
>> Sent: Saturday, November 22, 2008 2:59 PM
>> To: svn_at_subversion.tigris.org
>> Subject: svn commit: r34332 - in branches/tc_url_rev/subversion:
>> include libsvn_wc
>>
>> Author: rhuijben
>> Date: Sat Nov 22 05:58:46 2008
>> New Revision: 34332
>>
>> Log:
>> On the tc_url_rev branch:
>> Allow passing conflict versions to the conflict resolver for text
>> conflicts.
>>
>> * subversion/include/svn_wc.h
>> (svn_wc_conflict_version_t): Update documentation to state that the
>> structure can be extended in future versions and is new in 1.6.
>> * subversion/libsvn_wc/wc.h
>> (svn_wc__merge_internal): Add left_version and right_version
>> arguments and
>> update documentation to refer to svn_wc_merge3 instead of
>> svn_wc_merge2.
>> * subversion/libsvn_wc/merge.c
>> (svn_wc__merge_internal): Set older and their version in the conflict
>> description to left_version and right_version.
>> (svn_wc_merge3): Pass NULL to svn_wc__merge_internal for left_version
>> and
>> right_version.
>> * subversion/libsvn_wc/log.c
>> (log_do_merge): Pass NULL to svn_wc__merge_internal for left_version
>> and
>> right_version.
>> * subversion/libsvn_wc/update_editor.c
>> (merge_file): Pass NULL to svn_wc__merge_internal for left_version
>> and
>> right_version.
>>
>> Modified:
>> branches/tc_url_rev/subversion/include/svn_wc.h
>> branches/tc_url_rev/subversion/libsvn_wc/log.c
>> branches/tc_url_rev/subversion/libsvn_wc/merge.c
>> branches/tc_url_rev/subversion/libsvn_wc/update_editor.c
>> branches/tc_url_rev/subversion/libsvn_wc/wc.h
>>
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 34331)
> +++ subversion/include/svn_wc.h (revision 34332)
> @@ -1176,7 +1176,14 @@ svn_wc_operation_str(svn_wc_operation_t operation,
>
> /** Info about one of the conflicting versions of a node. Each field may
> * have its respective null/invalid/unknown value if the corresponding
> - * information is not relevant or not available. */
> + * information is not relevant or not available.
> + *
> + * @note Fields may be added to the end of this structure in future
> + * versions. Therefore, to preserve binary compatibility, users
> + * should not directly allocate structures of this type.
> + *
> + * @since New in 1.6.
> +*/
> typedef struct svn_wc_conflict_version_t
> {
> /* Where to find this node version in a repository */
> Index: subversion/libsvn_wc/wc.h
> ===================================================================
> --- subversion/libsvn_wc/wc.h (revision 34331)
> +++ subversion/libsvn_wc/wc.h (revision 34332)
> @@ -255,16 +255,24 @@ svn_wc__text_modified_internal_p(svn_boolean_t *mo
> conflict is encountered, giving the callback a chance to resolve
> the conflict (before marking the file 'conflicted').
>
> + When LEFT_VERSION and RIGHT_VERSION are non-NULL, pass them to the
> + conflict resolver as older_version and their_version.
> +
> + ## TODO: We should store the information in LEFT_VERSION and RIGHT_VERSION
> + in the workingcopy for future retrieval via svn info.
> +
> Property changes sent by the update are provided in PROP_DIFF.
>
> - For a complete description, see svn_wc_merge2() for which this is
> + For a complete description, see svn_wc_merge3() for which this is
> the (loggy) implementation.
> */
> 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,
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 34331)
> +++ subversion/libsvn_wc/update_editor.c (revision 34332)
> @@ -3813,8 +3813,8 @@ merge_file(svn_wc_notify_state_t *content_state,
> Remember that this function wants full paths! */
> SVN_ERR(svn_wc__merge_internal
> (&log_accum, &merge_outcome,
> - merge_left,
> - new_text_base_path,
> + merge_left, NULL,
> + new_text_base_path, NULL,
> fb->path,
> fb->copied_working_text,
> adm_access,
> 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.
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).

I haven't yet understood why these can be omitted in some cases. Couldn't we
instead send an "empty" dummy struct or something?

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.

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.

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
>