On Tue, 2008-08-26 at 16:43 +0200, Bert Huijben wrote:
> > -----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).
I wasn't proposing to take away the ability to do what you can do now,
or to force "svn_client_status3" to take a write lock and change the
entry, or anything like that. I was just proposing to provide smaller
chunks of functionality at the lower WC-layer API.
> 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)
Certainly.
- 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-27 20:09:54 CEST