On Sun, 2008-08-24 at 18:11 +0200, Bert Huijben wrote:
> 2008/8/24 Neels Hofmeyr <neels_at_elego.de>:
> > 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.
This was designed as a feature that people wanted at the user interface
level. However, this WC-layer API is too closely tied to the UI feature
and tries to do too much.
> > 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?
Yes, that's the sort of improvement I'd like to make.
> > 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!
>
> In concept I'm very much in favor of removing this checking of file
> state in the non-administrative region of the working copy.
> (Especially if the entry is not updated to the current state).
>
> I can't think of a reason to keep the old behavior in any of my
> personal use cases and removing this test would make it easier for the
> tree-conficts support and the new working copy database to integrate.
We wouldn't want to change the existing API, as there are bound to be
people relying on that behaviour and liking it. The question is whether
we should add a new API that does just one task and does it well. My
answer: Of course we should!
> But: changing this side-effect, like introducing the concept of tree
> conficts next to the original conflicts are both behavior changes.
>
> In both cases an application previously linked to 1.5 can get into
> conflicts it does not understand and can't resolve in the way it used
> to be.
Yes, but we don't have to change this old API in order to progress, so
we shouldn't.
> (But I think no application should ever assume it can really
> understand and/or resolve 100% of all conflicts it sees via
> subversion).
That's true.
> So I am waiting on comments of others to see if we should see this as
> a breaking change for their use cases or if we can declare it an old
> implementation detail that we can change.
>
> (I can imagine the original reason for this behavior was that we
> couldn't show any information on the conflict when the .rej and .prej
> files were lost.. That would most certainly be an implementation
> detail of the current (or perhaps previous?) working copy version)
No, it was designed as a feature.
- 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-26 15:31:02 CEST