> -----Original Message-----
> From: Julian Foad [mailto:julianfoad_at_btopenworld.com]
> Sent: dinsdag 26 augustus 2008 15:31
> To: Bert Huijben
> Cc: Neels Hofmeyr; dev_at_subversion.tigris.org
> Subject: Re: remove automation in svn_wc_conflicted_p()?
>
> 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.
Who would be responsible for calling such a new function?
The behavior seems nice from a svn CLI view, but it isn't half as nice when
looking as a library developer (as I am with my SharpSvn or AnkhSVN hat on).
svn_client_status3(), one of many indirect callers of svn_wc_conflicted_p(),
currently opens the working copy without taking a writer lock, and would
have to upgrade to a writer lock to update the entry. (Thereby changing the
concurrency behavior of this function that is called very frequently in GUIs
like AnkhSVN and TortoiseSVN).
Maybe this impact can be lessened by using a transaction in the new WC
database in sqlite, but until then I wouldn't like to see
svn_client_status3() take a writer lock.
And then again.. I like to see these file status changes via some event to
keep the AnkhSVN in-memory status cache up to date.
(So if possible, we should build new CLI-friendly features in the CLI
instead of in a backend library)
> > > 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
Bert
---------------------------------------------------------------------
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 18:13:47 CEST