On Fri, 2008-08-22 at 02:41 +0200, Neels Hofmeyr wrote:
> In reply to
> Re: Tree-conflicts branch - log message / review
> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142040
>
> Picking a ###:
[...]
> /** Given a @a dir_path under version control, decide if one of its
> * entries (@a entry) is in state of conflict; return the answers in
> * @a text_conflicted_p, @a prop_conflicted_p and @a tree_conflicted_p.
> * @a tree_conflicted_p refers to @a entry being the victim of a conflict.
> *
> * Only files can be text conflicted.
> * Only directories can be tree conflicted.
> * Property conflicts apply to both.
> *
> * If the entry mentions that a text conflict file, property conflicts
> * file, or a tree conflict report file (### obsolete) exist, but they are all
> * removed, assume the conflict has been resolved by the user. ### and
> adjust the entry?
> * ### Shouldn't this WC function just report what the entry says, and let
> the client
> * clear the conflict if files are gone (etc.) if that's what it wants to do?
> *
> * @since New in 1.6.
> */
> svn_error_t *
> svn_wc_conflicted_p2(svn_boolean_t *text_conflicted_p,
> svn_boolean_t *prop_conflicted_p,
> svn_boolean_t *tree_conflicted_p,
> const char *dir_path,
> const svn_wc_entry_t *entry,
> apr_pool_t *pool);
>
>
>
> 1) "### obsolete"
> I looked, and the function does no checking for any tree conflict report
> file. So, reference to tree-conflicts in this paragraph should be removed,
> in effect making it what it was on trunk.
Yes.
> 2) "### and adjust the entry?"
> That's a whole other issue: should this function auto-resolve conflicts if
> certain files aren't lying around anymore and should it then adjust the
> entries file, or should it just stick to the entry data without automation?
>
> This discussion is definitely not of concern to tree conflicts, not since
> tree conflicts aren't reported in explicit files anymore. So I suggest we
> stick to what trunk does and discuss/patch on trunk instead.
Good.
> trunk does:
>
> /** Given a @a dir_path under version control, decide if one of its
> * entries (@a entry) is in state of conflict; return the answers in
> * @a text_conflicted_p and @a prop_conflicted_p.
> *
> * (If the entry mentions that a .rej or .prej exist, but they are
> * both removed, assume the conflict has been resolved by the user.)
> */
> svn_error_t *
> svn_wc_conflicted_p(svn_boolean_t *text_conflicted_p,
> svn_boolean_t *prop_conflicted_p,
> const char *dir_path,
> const svn_wc_entry_t *entry,
> apr_pool_t *pool);
>
>
>
> I'd simply add the tree-conflict related stuff keeping the rest maximally
> similar, making:
>
>
> /** Given a @a dir_path under version control, decide if one of its
> * entries (@a entry) is in state of conflict; return the answers in
> * @a text_conflicted_p, @a prop_conflicted_p and @a tree_conflicted_p.
> * @a tree_conflicted_p refers to @a entry being the victim of a conflict.
> *
> * Only files can be text conflicted.
> * Only directories can be tree conflicted.
> * Property conflicts apply to both.
> *
> * (If the entry mentions that a .rej or .prej exist, but they are
> * both removed, assume the conflict has been resolved by the user.)
> *
> * @since New in 1.6.
> */
> svn_error_t *
> svn_wc_conflicted_p2(svn_boolean_t *text_conflicted_p,
> svn_boolean_t *prop_conflicted_p,
> svn_boolean_t *tree_conflicted_p,
> const char *dir_path,
> const svn_wc_entry_t *entry,
> apr_pool_t *pool);
>
> /** Like svn_wc_conflicted_p2, but without the capability to
> * detect tree conflicts.
> */
> svn_error_t *
> svn_wc_conflicted_p(svn_boolean_t *text_conflicted_p,
> svn_boolean_t *prop_conflicted_p,
> const char *dir_path,
> const svn_wc_entry_t *entry,
> apr_pool_t *pool);
>
>
> ...adjusting the .c file in similar fashion.
>
> This solution consciously accepts that
> - The comment isn't clear about whether the entries file is adjusted or not.
> - In "[If] both [files] are removed, assume the conflict has been resolved",
> saying "both" and "*the* conflict" is misleading. The two conflict states
> are handled independently from another by this function.
> - The automated behaviour is altogether questionable.
>
> These things shouldn't be fixed on the tree-conflicts branch, confusing
> people, right? I could post a trunk-patch for the error in the comment, to
> start with.
Yes. I committed a fix for the branch in r32774, making it document the way it is.
> 3) The third ### marker is in the function definition and comes from trunk.
> It asks why the function uses a subpool. I have no idea, but if at all,
> should be fixed on trunk.
Yes. I'm not much interested in that at the moment.
Thanks for the review.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-29 17:05:15 CEST