You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/05/12 18:02:51 UTC
svn commit: r1337579 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Author: stsp
Date: Sat May 12 16:02:50 2012
New Revision: 1337579
URL: http://svn.apache.org/viewvc?rev=1337579&view=rev
Log:
Make the conflict resolver use a work queue for working copy manipulations.
* subversion/libsvn_wc/conflicts.c
(resolve_conflict_on_node): Use a work queue instead of copying/removing
files in a non-atomic fashion. Resolves a long-standing FIXME note.
(attempt_deletion): Remove. No longer needed since this functionality is
provided by the work queue.
Modified:
subversion/trunk/subversion/libsvn_wc/conflicts.c
Modified: subversion/trunk/subversion/libsvn_wc/conflicts.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/conflicts.c?rev=1337579&r1=1337578&r2=1337579&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/conflicts.c (original)
+++ subversion/trunk/subversion/libsvn_wc/conflicts.c Sat May 12 16:02:50 2012
@@ -44,6 +44,7 @@
#include "wc.h"
#include "wc_db.h"
#include "conflicts.h"
+#include "workqueue.h"
#include "private/svn_wc_private.h"
#include "private/svn_skel.h"
@@ -115,33 +116,6 @@ svn_wc__conflict_skel_add_prop_conflict(
/*** Resolving a conflict automatically ***/
-
-/* Helper for resolve_conflict_on_entry. Delete the file FILE_ABSPATH
- in if it exists. Set WAS_PRESENT to TRUE if the file existed, and
- leave it UNTOUCHED otherwise. */
-static svn_error_t *
-attempt_deletion(const char *file_abspath,
- svn_boolean_t *was_present,
- apr_pool_t *scratch_pool)
-{
- svn_error_t *err;
-
- if (file_abspath == NULL)
- return SVN_NO_ERROR;
-
- err = svn_io_remove_file2(file_abspath, FALSE, scratch_pool);
-
- if (err == NULL || !APR_STATUS_IS_ENOENT(err->apr_err))
- {
- *was_present = TRUE;
- return svn_error_trace(err);
- }
-
- svn_error_clear(err);
- return SVN_NO_ERROR;
-}
-
-
/* Conflict resolution involves removing the conflict files, if they exist,
and clearing the conflict filenames from the entry. The latter needs to
be done whether or not the conflict files exist.
@@ -155,13 +129,6 @@ attempt_deletion(const char *file_abspat
else do not change *DID_RESOLVE.
See svn_wc_resolved_conflict5() for how CONFLICT_CHOICE behaves.
-
- ### FIXME: This function should be loggy, otherwise an interruption can
- ### leave, for example, one of the conflict artifact files deleted but
- ### the entry still referring to it and trying to use it for the next
- ### attempt at resolving.
-
- ### Does this still apply in the world of WC-NG? -hkw
*/
static svn_error_t *
resolve_conflict_on_node(svn_wc__db_t *db,
@@ -172,7 +139,6 @@ resolve_conflict_on_node(svn_wc__db_t *d
svn_boolean_t *did_resolve,
apr_pool_t *pool)
{
- svn_boolean_t found_file;
const char *conflict_old = NULL;
const char *conflict_new = NULL;
const char *conflict_working = NULL;
@@ -181,6 +147,8 @@ resolve_conflict_on_node(svn_wc__db_t *d
int i;
const apr_array_header_t *conflicts;
const char *conflict_dir_abspath;
+ svn_skel_t *work_items = NULL;
+ svn_skel_t *work_item;
*did_resolve = FALSE;
@@ -278,39 +246,51 @@ resolve_conflict_on_node(svn_wc__db_t *d
}
if (auto_resolve_src)
- SVN_ERR(svn_io_copy_file(
- svn_dirent_join(conflict_dir_abspath, auto_resolve_src, pool),
- local_abspath, TRUE, pool));
+ {
+ SVN_ERR(svn_wc__wq_build_file_copy_translated(
+ &work_item, db, local_abspath,
+ svn_dirent_join(conflict_dir_abspath,
+ auto_resolve_src, pool),
+ local_abspath, pool, pool));
+ work_items = svn_wc__wq_merge(work_items, work_item, pool);
+ }
}
- /* Records whether we found any of the conflict files. */
- found_file = FALSE;
-
if (resolve_text)
{
- SVN_ERR(attempt_deletion(conflict_old, &found_file, pool));
- SVN_ERR(attempt_deletion(conflict_new, &found_file, pool));
- SVN_ERR(attempt_deletion(conflict_working, &found_file, pool));
+ SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, conflict_old,
+ pool, pool));
+ work_items = svn_wc__wq_merge(work_items, work_item, pool);
+
+ SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, conflict_new,
+ pool, pool));
+ work_items = svn_wc__wq_merge(work_items, work_item, pool);
+
+ SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, conflict_working,
+ pool, pool));
+ work_items = svn_wc__wq_merge(work_items, work_item, pool);
+
resolve_text = conflict_old || conflict_new || conflict_working;
}
if (resolve_props)
{
if (prop_reject_file != NULL)
- SVN_ERR(attempt_deletion(prop_reject_file, &found_file, pool));
+ SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, prop_reject_file,
+ pool, pool));
else
resolve_props = FALSE;
}
if (resolve_text || resolve_props)
{
+ SVN_ERR(svn_wc__db_wq_add(db, local_abspath, work_items, pool));
SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
resolve_text, resolve_props,
FALSE, pool));
-
- /* No feedback if no files were deleted and all we did was change the
- entry, such a file did not appear as a conflict */
- if (found_file)
- *did_resolve = TRUE;
+ SVN_ERR(svn_wc__wq_run(db, local_abspath,
+ NULL, NULL, /* cancellation */
+ pool));
+ *did_resolve = TRUE;
}
return SVN_NO_ERROR;
Re: svn commit: r1337579 -
/subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 12, 2012 at 03:27:50PM -0400, Greg Stein wrote:
> It basically is because "everywhere" we check for a recorded text conflict,
> we double-check to see if the conflict files were removed. Removal
> suppresses the conflict.
I still think this is entirely stupid behaviour that just forces us to
write special-case code for text conflicts, but I concede.
See 1337844. Does that look sound?
Re: svn commit: r1337579 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Greg Stein <gs...@gmail.com>.
On May 12, 2012 6:54 AM, "Stefan Sperling" <st...@elego.de> wrote:
>...
> Why have a 'resolved' command at all if removing these files is the
> functional equivalent?
Two ways to resolve.
> It isn't really the technical equivalent since
> even 1.6 has to clear conflict data from entries files in any case.
It basically is because "everywhere" we check for a recorded text conflict,
we double-check to see if the conflict files were removed. Removal
suppresses the conflict.
> All
> that happens is that a 'resolved' notification is suppressed if the helper
> files are missing from disk.
As Bert noted, status is cleared. Commits can occur. Etc.
Much more than notifications.
> What's the value in that? This is inconsistent
> behaviour. And I doubt any of our users are relying on not receiving
> this notification.
You can't assume that.
Cheers,
-g
Re: svn commit: r1337579 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Greg Stein <gs...@gmail.com>.
On May 12, 2012 6:54 AM, "Stefan Sperling" <st...@elego.de> wrote:
>
> On Sat, May 12, 2012 at 06:38:53PM +0200, Bert Huijben wrote:
> > > Might be, but it only affects notification. And I don't think it is
> > > a good idea to give users the illusion that a conflict can be marked
> > > resolved by removing the temp helper files (which is apparently what
> > > this was about).
> >
> > This is documented behavior since 1.0 :(
>
> Where and how is it documented?
Who knows. Doesn't matter. It has always been one way to resolve text
conflicts.
It has to stay, or you're talking about a regression.
Cheers,
-g
Re: svn commit: r1337579 -
/subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 12, 2012 at 06:38:53PM +0200, Bert Huijben wrote:
> > Might be, but it only affects notification. And I don't think it is
> > a good idea to give users the illusion that a conflict can be marked
> > resolved by removing the temp helper files (which is apparently what
> > this was about).
>
> This is documented behavior since 1.0 :(
Where and how is it documented?
> (If it wasn't we could remove some expensive code from the status walker)
>
> I can see the reasoning at the time (and the cvs idea), but it is one thing
> I would like to break when we can restore conflicts. But I'm not sure if we
> can.
Why have a 'resolved' command at all if removing these files is the
functional equivalent? It isn't really the technical equivalent since
even 1.6 has to clear conflict data from entries files in any case. All
that happens is that a 'resolved' notification is suppressed if the helper
files are missing from disk. What's the value in that? This is inconsistent
behaviour. And I doubt any of our users are relying on not receiving
this notification.
RE: svn commit: r1337579 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: zaterdag 12 mei 2012 18:21
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1337579 -
> /subversion/trunk/subversion/libsvn_wc/conflicts.c
>
> On Sat, May 12, 2012 at 06:09:05PM +0200, Bert Huijben wrote:
> > > if (resolve_props)
> > > {
> > > if (prop_reject_file != NULL)
> > > - SVN_ERR(attempt_deletion(prop_reject_file, &found_file,
pool));
> > > + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db,
> > > prop_reject_file,
> > > + pool, pool));
> >
> > When resolving property and tekst conflicts this will remove the older
work
> items for the text resolving.
>
> Not sure what you mean here.
>
> All I see is a missing workq merge call that I'll add now. Is that
> what you mean?
>
> >
> > > else
> > > resolve_props = FALSE;
> > > }
> > >
> > > if (resolve_text || resolve_props)
> > > {
> > > + SVN_ERR(svn_wc__db_wq_add(db, local_abspath, work_items,
pool));
> > > SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
> > > resolve_text,
resolve_props,
> > > FALSE, pool));
> >
> > svn_wc__db_op_mark_resolved() should be extended to add the wq items
> within the same transaction.
> > (Note that there is a very simple helper function to accomplish this)
>
> Yes, I'll look into that.
>
> > Without this change it is still not atomic, but at least better than
before.
> >
> > > -
> > > - /* No feedback if no files were deleted and all we did was
change the
> > > - entry, such a file did not appear as a conflict */
> > > - if (found_file)
> > > - *did_resolve = TRUE;
> > > + SVN_ERR(svn_wc__wq_run(db, local_abspath,
> > > + NULL, NULL, /* cancellation */
> > > + pool));
> > > + *did_resolve = TRUE;
> >
> > This might be a behavior change?
>
> Might be, but it only affects notification. And I don't think it is
> a good idea to give users the illusion that a conflict can be marked
> resolved by removing the temp helper files (which is apparently what
> this was about).
This is documented behavior since 1.0 :(
(If it wasn't we could remove some expensive code from the status walker)
I can see the reasoning at the time (and the cvs idea), but it is one thing
I would like to break when we can restore conflicts. But I'm not sure if we
can.
Bert
Re: svn commit: r1337579 -
/subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 12, 2012 at 06:09:05PM +0200, Bert Huijben wrote:
> > if (resolve_props)
> > {
> > if (prop_reject_file != NULL)
> > - SVN_ERR(attempt_deletion(prop_reject_file, &found_file, pool));
> > + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db,
> > prop_reject_file,
> > + pool, pool));
>
> When resolving property and tekst conflicts this will remove the older work items for the text resolving.
Not sure what you mean here.
All I see is a missing workq merge call that I'll add now. Is that
what you mean?
>
> > else
> > resolve_props = FALSE;
> > }
> >
> > if (resolve_text || resolve_props)
> > {
> > + SVN_ERR(svn_wc__db_wq_add(db, local_abspath, work_items, pool));
> > SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
> > resolve_text, resolve_props,
> > FALSE, pool));
>
> svn_wc__db_op_mark_resolved() should be extended to add the wq items within the same transaction.
> (Note that there is a very simple helper function to accomplish this)
Yes, I'll look into that.
> Without this change it is still not atomic, but at least better than before.
>
> > -
> > - /* No feedback if no files were deleted and all we did was change the
> > - entry, such a file did not appear as a conflict */
> > - if (found_file)
> > - *did_resolve = TRUE;
> > + SVN_ERR(svn_wc__wq_run(db, local_abspath,
> > + NULL, NULL, /* cancellation */
> > + pool));
> > + *did_resolve = TRUE;
>
> This might be a behavior change?
Might be, but it only affects notification. And I don't think it is
a good idea to give users the illusion that a conflict can be marked
resolved by removing the temp helper files (which is apparently what
this was about).
RE: svn commit: r1337579 - /subversion/trunk/subversion/libsvn_wc/conflicts.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stsp@apache.org [mailto:stsp@apache.org]
> Sent: zaterdag 12 mei 2012 18:03
> To: commits@subversion.apache.org
> Subject: svn commit: r1337579 -
> /subversion/trunk/subversion/libsvn_wc/conflicts.c
>
> Author: stsp
> Date: Sat May 12 16:02:50 2012
> New Revision: 1337579
>
> URL: http://svn.apache.org/viewvc?rev=1337579&view=rev
> Log:
> Make the conflict resolver use a work queue for working copy manipulations.
>
> * subversion/libsvn_wc/conflicts.c
> (resolve_conflict_on_node): Use a work queue instead of copying/removing
> files in a non-atomic fashion. Resolves a long-standing FIXME note.
> (attempt_deletion): Remove. No longer needed since this functionality is
> provided by the work queue.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/conflicts.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/conflicts.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/conflicts.
> c?rev=1337579&r1=1337578&r2=1337579&view=diff
> =================================================================
> =============
> --- subversion/trunk/subversion/libsvn_wc/conflicts.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/conflicts.c Sat May 12 16:02:50
> 2012
> @@ -44,6 +44,7 @@
> #include "wc.h"
> #include "wc_db.h"
> #include "conflicts.h"
> +#include "workqueue.h"
>
> #include "private/svn_wc_private.h"
> #include "private/svn_skel.h"
> @@ -115,33 +116,6 @@ svn_wc__conflict_skel_add_prop_conflict(
>
> /*** Resolving a conflict automatically ***/
>
> -
> -/* Helper for resolve_conflict_on_entry. Delete the file FILE_ABSPATH
> - in if it exists. Set WAS_PRESENT to TRUE if the file existed, and
> - leave it UNTOUCHED otherwise. */
> -static svn_error_t *
> -attempt_deletion(const char *file_abspath,
> - svn_boolean_t *was_present,
> - apr_pool_t *scratch_pool)
> -{
> - svn_error_t *err;
> -
> - if (file_abspath == NULL)
> - return SVN_NO_ERROR;
> -
> - err = svn_io_remove_file2(file_abspath, FALSE, scratch_pool);
> -
> - if (err == NULL || !APR_STATUS_IS_ENOENT(err->apr_err))
> - {
> - *was_present = TRUE;
> - return svn_error_trace(err);
> - }
> -
> - svn_error_clear(err);
> - return SVN_NO_ERROR;
> -}
> -
> -
> /* Conflict resolution involves removing the conflict files, if they exist,
> and clearing the conflict filenames from the entry. The latter needs to
> be done whether or not the conflict files exist.
> @@ -155,13 +129,6 @@ attempt_deletion(const char *file_abspat
> else do not change *DID_RESOLVE.
>
> See svn_wc_resolved_conflict5() for how CONFLICT_CHOICE behaves.
> -
> - ### FIXME: This function should be loggy, otherwise an interruption can
> - ### leave, for example, one of the conflict artifact files deleted but
> - ### the entry still referring to it and trying to use it for the next
> - ### attempt at resolving.
> -
> - ### Does this still apply in the world of WC-NG? -hkw
> */
> static svn_error_t *
> resolve_conflict_on_node(svn_wc__db_t *db,
> @@ -172,7 +139,6 @@ resolve_conflict_on_node(svn_wc__db_t *d
> svn_boolean_t *did_resolve,
> apr_pool_t *pool)
> {
> - svn_boolean_t found_file;
> const char *conflict_old = NULL;
> const char *conflict_new = NULL;
> const char *conflict_working = NULL;
> @@ -181,6 +147,8 @@ resolve_conflict_on_node(svn_wc__db_t *d
> int i;
> const apr_array_header_t *conflicts;
> const char *conflict_dir_abspath;
> + svn_skel_t *work_items = NULL;
> + svn_skel_t *work_item;
>
> *did_resolve = FALSE;
>
> @@ -278,39 +246,51 @@ resolve_conflict_on_node(svn_wc__db_t *d
> }
>
> if (auto_resolve_src)
> - SVN_ERR(svn_io_copy_file(
> - svn_dirent_join(conflict_dir_abspath, auto_resolve_src, pool),
> - local_abspath, TRUE, pool));
> + {
> + SVN_ERR(svn_wc__wq_build_file_copy_translated(
> + &work_item, db, local_abspath,
> + svn_dirent_join(conflict_dir_abspath,
> + auto_resolve_src, pool),
> + local_abspath, pool, pool));
> + work_items = svn_wc__wq_merge(work_items, work_item, pool);
> + }
> }
>
> - /* Records whether we found any of the conflict files. */
> - found_file = FALSE;
> -
> if (resolve_text)
> {
> - SVN_ERR(attempt_deletion(conflict_old, &found_file, pool));
> - SVN_ERR(attempt_deletion(conflict_new, &found_file, pool));
> - SVN_ERR(attempt_deletion(conflict_working, &found_file, pool));
> + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, conflict_old,
> + pool, pool));
> + work_items = svn_wc__wq_merge(work_items, work_item, pool);
> +
> + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db, conflict_new,
> + pool, pool));
> + work_items = svn_wc__wq_merge(work_items, work_item, pool);
> +
> + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db,
> conflict_working,
> + pool, pool));
> + work_items = svn_wc__wq_merge(work_items, work_item, pool);
> +
> resolve_text = conflict_old || conflict_new || conflict_working;
> }
> if (resolve_props)
> {
> if (prop_reject_file != NULL)
> - SVN_ERR(attempt_deletion(prop_reject_file, &found_file, pool));
> + SVN_ERR(svn_wc__wq_build_file_remove(&work_item, db,
> prop_reject_file,
> + pool, pool));
When resolving property and tekst conflicts this will remove the older work items for the text resolving.
> else
> resolve_props = FALSE;
> }
>
> if (resolve_text || resolve_props)
> {
> + SVN_ERR(svn_wc__db_wq_add(db, local_abspath, work_items, pool));
> SVN_ERR(svn_wc__db_op_mark_resolved(db, local_abspath,
> resolve_text, resolve_props,
> FALSE, pool));
svn_wc__db_op_mark_resolved() should be extended to add the wq items within the same transaction.
(Note that there is a very simple helper function to accomplish this)
Without this change it is still not atomic, but at least better than before.
> -
> - /* No feedback if no files were deleted and all we did was change the
> - entry, such a file did not appear as a conflict */
> - if (found_file)
> - *did_resolve = TRUE;
> + SVN_ERR(svn_wc__wq_run(db, local_abspath,
> + NULL, NULL, /* cancellation */
> + pool));
> + *did_resolve = TRUE;
This might be a behavior change?
Bert
> }
>
> return SVN_NO_ERROR;
>