You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Neels Hofmeyr <ne...@elego.de> on 2008/08/24 03:51:21 UTC
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
RE: remove automation in svn_wc_conflicted_p()?
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-08-26 at 16:43 +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > Sent: dinsdag 26 augustus 2008 15:31
> > To: Bert Huijben
> > Cc: Neels Hofmeyr; dev@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 <ne...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: remove automation in svn_wc_conflicted_p()?
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: dinsdag 26 augustus 2008 15:31
> To: Bert Huijben
> Cc: Neels Hofmeyr; dev@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 <ne...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: remove automation in svn_wc_conflicted_p()?
Posted by Julian Foad <ju...@btopenworld.com>.
On Sun, 2008-08-24 at 18:11 +0200, Bert Huijben wrote:
> 2008/8/24 Neels Hofmeyr <ne...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: remove automation in svn_wc_conflicted_p()?
Posted by Bert Huijben <be...@vmoo.com>.
2008/8/24 Neels Hofmeyr <ne...@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.
>
> 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!
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.
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. (But I think no application should ever assume it can really
understand and/or resolve 100% of all conflicts it sees via
subversion).
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)
Bert
>
> ~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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org