remove automation in svn_wc_conflicted_p()?

Hi all,

We've had a recent commit clarifying the comment of
 subversion/include/svn_wc.h svn_wc_conflicted_p()

It describes how svn_wc_conflicted_p() makes assumptions when conflict files
that are expected to be there cannot be found:
[[[

/** 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 @a entry mentions that a text conflict file (.rej suffix)
 * exists, but it cannot be found, assume the text conflict has been
 * resolved by the user and return FALSE in @a *text_conflicted_p.
 *
 * Similarly, if the @a entry mentions that a property conflicts file
 * (.prej suffix) exists, but it cannot be found, assume the property
 * conflicts have been resolved by the user and return FALSE in
 * @a *prop_conflicted_p.
 *
 * The @a entry is not updated.
 */
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);

]]]
(Implementation in subversion/libsvn_wc/questions.c)

I see a very simple function made very complicated by the assumptions it makes.

I also see potential problems: what if the conflict markers were set
correctly, but writing the conflict files failed? This function would assume
that the user removed the files and would mysteriously ignore conflicts.

Plus, there doesn't seem to be a strong will behind this, because it doesn't
even update the entries file if a conflict is assumed resolved.

I invite anyone to name good reasons why this function changes its return
values if it cannot find certain files.

Are there good usability reasons behind this? Then maybe this should be
redesigned to be a separate function, e.g. svn_wc_auto_resolve(), which also
updates ENTRY once and for all?

IMHO, it would be better to just return whatever flags are set in the ENTRY.
It would make this function clearer and safer, and would enforce an explicit
`svn resolve[d]' by the user, and ensure that she knows what's going on.

BTW, I checked, and implementing the simpler version of this function does
not break any tests, except the two tests that explicitly check for this
automation (revert_tests.py 9 and 10). My diff for testing is attached. In
revert_tests.py 9 and 10, removing conflict files is called "resolving
manually".

Have I got this wrong? Please comment!

~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