Tree-conflicts branch - svn_wc_conflicted_p2

In reply to
Re: Tree-conflicts branch - log message / review
http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142040

Picking a ###:

Julian Foad wrote:
> On Wed, 2008-08-20 at 10:58 +0100, Julian Foad wrote:
>> Here is a log message for the changes currently sitting on the
>> tree-conflicts branch.
>>
>> If you are interested in this branch, please take a look and perhaps
>> pick one part of it to review, comment on and/or help with. I added some
>> notes with '###' in the log message about things that look wrong, but
>> they are by no means a review.
>>
>> I'll do the same.
>
> I've started removing some of the distractions:
>
> First, I fixed up some proerty differences that weren't mentioned in the
> log message.
>
> [[[
>
>> # PUBLIC API
...
>> * subversion/include/svn_wc.h
...
>> (svn_wc_conflicted_p2): New function to support tree conflicts,
>> superseding svn_wc_conflicted_p() which becomes deprecated.
>> ### Has '###' comments.

Let's look at the ### comments:

declaration:

/** 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.

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.

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.

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.

Do you agree?

Neels

-- 
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696  mobile: +49 177 2345869  fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194