You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Hyrum K Wright <hy...@hyrumwright.org> on 2012/10/11 18:20:32 UTC
Re: svn commit: r1387696 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
On Wed, Sep 19, 2012 at 2:06 PM, <st...@apache.org> wrote:
> Author: stsp
> Date: Wed Sep 19 18:06:13 2012
> New Revision: 1387696
>
> URL: http://svn.apache.org/viewvc?rev=1387696&view=rev
> Log:
> * subversion/libsvn_wc/conflicts.c
> (read_prop_conflicts): New helper to prepare for the times when new
> conflict storage is enabled. Currently, this describes each property
> conflict in a separate svn_wc_conflict_description2_t, within the
> limitations of svn_wc_conflict_description2_t. These limitations
> and suggested improvements have been recorded in comments.
> (svn_wc__read_conflicts): Use the new read_prop_conflicts() helper
> instead of creating just one conflict description.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/conflicts.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/conflicts.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/conflicts.c?rev=1387696&r1=1387695&r2=1387696&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/conflicts.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/conflicts.c Wed Sep 19 18:06:13 2012
> @@ -1907,6 +1907,153 @@ svn_wc__conflict_invoke_resolver(svn_wc_
> return SVN_NO_ERROR;
> }
>
> +/* Read all property conflicts contained in CONFLICT_SKEL into
> + * individual conflict descriptions, and append those descriptions
> + * to the CONFLICTS array. Allocate results in RESULT_POOL.
> + * SCRATCH_POOL is used for temporary allocations. */
> +static svn_error_t *
> +read_prop_conflicts(apr_array_header_t *conflicts,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + svn_skel_t *conflict_skel,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + const char *prop_reject_file;
> + apr_hash_t *my_props;
> + apr_hash_t *their_old_props;
> + apr_hash_t *their_props;
> + apr_hash_t *conflicted_props;
> + apr_hash_index_t *hi;
> + apr_pool_t *iterpool;
> +
> + SVN_ERR(svn_wc__conflict_read_prop_conflict(&prop_reject_file,
> + &my_props,
> + &their_old_props,
> + &their_props,
> + &conflicted_props,
> + db, local_abspath,
> + conflict_skel,
> + scratch_pool, scratch_pool));
> +
> + if (apr_hash_count(conflicted_props) == 0)
> + {
> + /* Legacy prop conflict with only a .reject file. */
> + svn_wc_conflict_description2_t *desc;
> +
> + desc = svn_wc_conflict_description_create_prop2(local_abspath,
> + svn_node_unknown,
> + "", result_pool);
> +
> + /* ### This should be changed. The prej file should be stored
> + * ### separately from the other files. We need to rev the
> + * ### conflict description struct for this. */
> + desc->their_abspath = apr_pstrdup(result_pool, prop_reject_file);
> +
> + APR_ARRAY_PUSH(conflicts, svn_wc_conflict_description2_t*) = desc;
> +
> + return SVN_NO_ERROR;
> + }
> +
> + iterpool = svn_pool_create(scratch_pool);
> + for (hi = apr_hash_first(scratch_pool, conflicted_props);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *propname = svn__apr_hash_index_key(hi);
> + svn_string_t *old_value;
> + svn_string_t *my_value;
> + svn_string_t *their_value;
> + svn_wc_conflict_description2_t *desc;
> +
> + svn_pool_clear(iterpool);
> +
> + desc = svn_wc_conflict_description_create_prop2(local_abspath,
> + svn_node_unknown,
> + propname,
> + result_pool);
> +
> + desc->property_name = apr_pstrdup(result_pool, propname);
> +
> + my_value = apr_hash_get(my_props, propname, APR_HASH_KEY_STRING);
> + their_value = apr_hash_get(their_props, propname,
> + APR_HASH_KEY_STRING);
> +
> + /* Compute the incoming side of the conflict ('action'). */
> + if (their_value == NULL)
> + desc->action = svn_wc_conflict_action_delete;
> + else if (my_value == NULL)
> + desc->action = svn_wc_conflict_action_add;
Wrong enum type assignment here, the source is an
svn_wc_conflict_reason_t, but the destination is an
svn_wc_conflict_action_t. While this might work now, it feels like a
real opportunity for obscure bugs later. (And my compiler complains
about it. :) )
> + else
> + desc->action = svn_wc_conflict_action_edit;
Same.
> +
> + /* Compute the local side of the conflict ('reason'). */
> + if (my_value == NULL)
> + desc->reason = svn_wc_conflict_reason_deleted;
> + else if (their_value == NULL)
> + desc->action = svn_wc_conflict_reason_added;
> + else
> + desc->action = svn_wc_conflict_reason_edited;
> +
> + /* ### This should be changed. The prej file should be stored
> + * ### separately from the other files. We need to rev the
> + * ### conflict description struct for this. */
> + desc->their_abspath = apr_pstrdup(result_pool, prop_reject_file);
> +
> + /* ### This should be changed. The conflict description for
> + * ### props should contain these values as svn_string_t,
> + * ### rather than in temporary files. We need to rev the
> + * ### conflict description struct for this. */
> + if (my_value)
> + {
> + svn_stream_t *s;
> + apr_size_t len;
> +
> + SVN_ERR(svn_stream_open_unique(&s, &desc->my_abspath, NULL,
> + svn_io_file_del_on_pool_cleanup,
> + result_pool, iterpool));
> + len = my_value->len;
> + SVN_ERR(svn_stream_write(s, my_value->data, &len));
> + SVN_ERR(svn_stream_close(s));
> + }
> +
> + if (their_value)
> + {
> + svn_stream_t *s;
> + apr_size_t len;
> +
> + /* ### Currently, their_abspath is used for the prop reject file.
> + * ### Put their value into merged instead...
> + * ### We need to rev the conflict description struct to fix this. */
> + SVN_ERR(svn_stream_open_unique(&s, &desc->merged_file, NULL,
> + svn_io_file_del_on_pool_cleanup,
> + result_pool, iterpool));
> + len = their_value->len;
> + SVN_ERR(svn_stream_write(s, their_value->data, &len));
> + SVN_ERR(svn_stream_close(s));
> + }
> +
> + old_value = apr_hash_get(their_old_props, propname, APR_HASH_KEY_STRING);
> + if (old_value)
> + {
> + svn_stream_t *s;
> + apr_size_t len;
> +
> + SVN_ERR(svn_stream_open_unique(&s, &desc->base_abspath, NULL,
> + svn_io_file_del_on_pool_cleanup,
> + result_pool, iterpool));
> + len = old_value->len;
> + SVN_ERR(svn_stream_write(s, old_value->data, &len));
> + SVN_ERR(svn_stream_close(s));
> + }
> +
> + APR_ARRAY_PUSH(conflicts, svn_wc_conflict_description2_t*) = desc;
> + }
> + svn_pool_destroy(iterpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_wc__read_conflicts(const apr_array_header_t **conflicts,
> svn_wc__db_t *db,
> @@ -1942,21 +2089,8 @@ svn_wc__read_conflicts(const apr_array_h
> sizeof(svn_wc_conflict_description2_t*));
>
> if (prop_conflicted)
> - {
> - svn_wc_conflict_description2_t *desc;
> - desc = svn_wc_conflict_description_create_prop2(local_abspath,
> - svn_node_unknown,
> - "",
> - result_pool);
> -
> - SVN_ERR(svn_wc__conflict_read_prop_conflict(&desc->their_abspath,
> - NULL, NULL, NULL, NULL,
> - db, local_abspath,
> - conflict_skel,
> - result_pool, scratch_pool));
> -
> - APR_ARRAY_PUSH(cflcts, svn_wc_conflict_description2_t*) = desc;
> - }
> + SVN_ERR(read_prop_conflicts(cflcts, db, local_abspath, conflict_skel,
> + result_pool, scratch_pool));
>
> if (text_conflicted)
> {
>
>
Re: svn commit: r1387696 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Oct 11, 2012 at 7:15 PM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Oct 11, 2012 at 05:25:03PM -0400, Hyrum K Wright wrote:
>> Does this (untested) patch make sense?
>
> Oh yes, very much! Thanks!
r1397425
-Hyrum
>
>> [[[
>> Index: subversion/libsvn_wc/conflicts.c
>> ===================================================================
>> --- subversion/libsvn_wc/conflicts.c (revision 1397318)
>> +++ subversion/libsvn_wc/conflicts.c (working copy)
>> @@ -2000,9 +2000,9 @@
>> if (my_value == NULL)
>> desc->reason = svn_wc_conflict_reason_deleted;
>> else if (their_value == NULL)
>> - desc->action = svn_wc_conflict_reason_added;
>> + desc->reason = svn_wc_conflict_reason_added;
>> else
>> - desc->action = svn_wc_conflict_reason_edited;
>> + desc->reason = svn_wc_conflict_reason_edited;
>>
>> /* ### This should be changed. The prej file should be stored
>> * ### separately from the other files. We need to rev the
>> ]]]
>>
>> -Hyrum
Re: svn commit: r1387696 -
/subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 11, 2012 at 05:25:03PM -0400, Hyrum K Wright wrote:
> Does this (untested) patch make sense?
Oh yes, very much! Thanks!
> [[[
> Index: subversion/libsvn_wc/conflicts.c
> ===================================================================
> --- subversion/libsvn_wc/conflicts.c (revision 1397318)
> +++ subversion/libsvn_wc/conflicts.c (working copy)
> @@ -2000,9 +2000,9 @@
> if (my_value == NULL)
> desc->reason = svn_wc_conflict_reason_deleted;
> else if (their_value == NULL)
> - desc->action = svn_wc_conflict_reason_added;
> + desc->reason = svn_wc_conflict_reason_added;
> else
> - desc->action = svn_wc_conflict_reason_edited;
> + desc->reason = svn_wc_conflict_reason_edited;
>
> /* ### This should be changed. The prej file should be stored
> * ### separately from the other files. We need to rev the
> ]]]
>
> -Hyrum
Re: svn commit: r1387696 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Oct 11, 2012 at 3:01 PM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Oct 11, 2012 at 12:20:32PM -0400, Hyrum K Wright wrote:
>> > + /* Compute the incoming side of the conflict ('action'). */
>> > + if (their_value == NULL)
>> > + desc->action = svn_wc_conflict_action_delete;
>> > + else if (my_value == NULL)
>> > + desc->action = svn_wc_conflict_action_add;
>>
>> Wrong enum type assignment here, the source is an
>> svn_wc_conflict_reason_t, but the destination is an
>> svn_wc_conflict_action_t.
>
> Really? I see neither svn_wc_conflict_action_delete nor
> svn_wc_conflict_action_add in the definition of svn_wc_conflict_reason_t.
Whoops, sorry, I should have referenced a different line below,
currently line 2003.
>
>> While this might work now, it feels like a
>> real opportunity for obscure bugs later. (And my compiler complains
>> about it. :) )
>>
>> > + else
>> > + desc->action = svn_wc_conflict_action_edit;
>>
>> Same.
>
> Seems fine to me. Both are saying 'action'... did you mean to
> quote a different part of the code?
Yes, I meant to quote a different part.
This is the compiler warning I see:
subversion/libsvn_wc/conflicts.c:2003:24: warning: implicit conversion from
enumeration type 'enum svn_wc_conflict_reason_t' to different
enumeration type
'svn_wc_conflict_action_t' (aka 'enum svn_wc_conflict_action_t')
[-Wconversion]
desc->action = svn_wc_conflict_reason_added;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/libsvn_wc/conflicts.c:2005:24: warning: implicit conversion from
enumeration type 'enum svn_wc_conflict_reason_t' to different
enumeration type
'svn_wc_conflict_action_t' (aka 'enum svn_wc_conflict_action_t')
[-Wconversion]
desc->action = svn_wc_conflict_reason_edited;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Does this (untested) patch make sense?
[[[
Index: subversion/libsvn_wc/conflicts.c
===================================================================
--- subversion/libsvn_wc/conflicts.c (revision 1397318)
+++ subversion/libsvn_wc/conflicts.c (working copy)
@@ -2000,9 +2000,9 @@
if (my_value == NULL)
desc->reason = svn_wc_conflict_reason_deleted;
else if (their_value == NULL)
- desc->action = svn_wc_conflict_reason_added;
+ desc->reason = svn_wc_conflict_reason_added;
else
- desc->action = svn_wc_conflict_reason_edited;
+ desc->reason = svn_wc_conflict_reason_edited;
/* ### This should be changed. The prej file should be stored
* ### separately from the other files. We need to rev the
]]]
-Hyrum
Re: svn commit: r1387696 -
/subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 11, 2012 at 12:20:32PM -0400, Hyrum K Wright wrote:
> > + /* Compute the incoming side of the conflict ('action'). */
> > + if (their_value == NULL)
> > + desc->action = svn_wc_conflict_action_delete;
> > + else if (my_value == NULL)
> > + desc->action = svn_wc_conflict_action_add;
>
> Wrong enum type assignment here, the source is an
> svn_wc_conflict_reason_t, but the destination is an
> svn_wc_conflict_action_t.
Really? I see neither svn_wc_conflict_action_delete nor
svn_wc_conflict_action_add in the definition of svn_wc_conflict_reason_t.
> While this might work now, it feels like a
> real opportunity for obscure bugs later. (And my compiler complains
> about it. :) )
>
> > + else
> > + desc->action = svn_wc_conflict_action_edit;
>
> Same.
Seems fine to me. Both are saying 'action'... did you mean to
quote a different part of the code?