You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/03/09 14:22:06 UTC
Re: svn commit: r13312 -
branches/wc-replacements/subversion/libsvn_wc
breser@tigris.org writes:
> Author: breser
> Date: Tue Mar 8 15:03:29 2005
> New Revision: 13312
This is good, I like the idea.
> --- branches/wc-replacements/subversion/libsvn_wc/copy.c (original)
> +++ branches/wc-replacements/subversion/libsvn_wc/copy.c Tue Mar 8 15:03:29 2005
> @@ -137,6 +137,7 @@
> svn_node_kind_t dst_kind;
> const svn_wc_entry_t *src_entry, *dst_entry;
> svn_boolean_t special;
> + svn_boolean_t replace = FALSE;
>
> /* The 'dst_path' is simply dst_parent/dst_basename */
> const char *dst_path
> @@ -157,10 +158,7 @@
> if (dst_entry && dst_entry->kind == svn_node_file)
> {
> if (dst_entry->schedule == svn_wc_schedule_delete)
> - return svn_error_createf (SVN_ERR_ENTRY_EXISTS, NULL,
> - _("'%s' is scheduled for deletion; it must"
> - " be committed before being overwritten"),
> - svn_path_local_style (dst_path, pool));
> + replace = TRUE;
> else
> return svn_error_createf (SVN_ERR_ENTRY_EXISTS, NULL,
> _("There is already a versioned item '%s'"),
> @@ -198,6 +196,17 @@
> TRUE, /* expand */
> TRUE, /* special */
> pool));
> +
> + /* If we're replacing the file then we need to save the destination files
> + * text base before replacing it (see comments below for why it gets
> + * replaced). This allows us to revert the entire change. */
> + if (replace)
> + {
> + const char *src_txtb = svn_wc__text_base_path (dst_path, FALSE, pool);
> + const char *dst_rvrt = svn_wc__text_revert_path (dst_path, FALSE, pool);
> +
> + SVN_ERR (svn_io_copy_file (src_txtb, dst_rvrt, TRUE, pool));
> + }
>
> /* Copy the pristine text-base over. Why? Because it's the *only*
> way we can detect any upcoming local mods on the copy.
I think there is an atomic problem here.
The code is going to copy the source text-base onto the destination
text-base, and then call svn_wc_add which will modify the checksum.
If the client gets killed in the window between copying the text-base
and modifying the checksum then the working copy is broken, the
schedule delete file will have the wrong text-base. (It can be fixed
by copying the .svn-revert back to .svn-base.) Also, if the user
attempts to repeat the aborted copy in the broken working copy the
bogus text-base will overwrite the correct one stored in .svn-revert.
Obviously such bugs are bad, on the other hand, I don't think the
existing wc code always gets atomic operations right either.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13312 -
branches/wc-replacements/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> What we need is a private function that does what svn_wc_add does but
> understands how to handle installing the admin files for us. Then the
> copy operation can use it. That private function would copy the admin
> files to their tmp locations and then write a log that modifies entries
> and renames the files into their proper location.
>
> To avoid code duplication the private function would need to handle the
> normal svn_wc_add cases so svn_wc_add can just be the public stub.
>
> This allows an interrupted operation to be completed by cleanup. If a
> user tried to repeat the operation after an interrupted operation then
> the wc would be locked and they'd be told to run cleanup...
>
> Do you agree that this would resolve the problem?
I think it would. It would also make the normal add path "loggy",
which isn't the case at present. That could be a problem if it makes
add slower as users do want to be able to run add on large sets of
files and such operations tend to be slow enough already.
On the other hand it's possible that add is one of the existing places
where we don't get atomic operations right at present, it's one of our
oldest functions. Look at the bit that calls __do_update_cleanup,
what happens if the client is interrupted before that gets done?
I've been trying to remember where our current atomic problem areas
lie, as far as I can recall some of the deleted="true" handling is
suspect, and possibly the incomplete="true" as well.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13312 - branches/wc-replacements/subversion/libsvn_wc
Posted by Ben Reser <be...@reser.org>.
On Wed, Mar 09, 2005 at 02:22:06PM +0000, Philip Martin wrote:
> I think there is an atomic problem here.
>
> The code is going to copy the source text-base onto the destination
> text-base, and then call svn_wc_add which will modify the checksum.
> If the client gets killed in the window between copying the text-base
> and modifying the checksum then the working copy is broken, the
> schedule delete file will have the wrong text-base. (It can be fixed
> by copying the .svn-revert back to .svn-base.) Also, if the user
> attempts to repeat the aborted copy in the broken working copy the
> bogus text-base will overwrite the correct one stored in .svn-revert.
>
> Obviously such bugs are bad, on the other hand, I don't think the
> existing wc code always gets atomic operations right either.
Yup you're right.
First of all when we do an add (with or without history) we don't set a
checksum on the file. I'm not sure this is a such a great idea but
that's the way it works now. It makes sense for adds without history
because there is no base so there's nothing to checksum. But for adds
with history then the checksum should be set for the base, but it isn't.
So the problem is actually worse than you thought becaues the wc
wouldn't know it was broken.
Ultimately, the problem here is that svn_wc_add is being called from the
copy code execution path. Add knows nothing about the copy so copy is
having to slip admin files in before the add gets run. Since we don't
currently support doing adds with histories over removed files in the wc
we don't run into this right now.
What we need is a private function that does what svn_wc_add does but
understands how to handle installing the admin files for us. Then the
copy operation can use it. That private function would copy the admin
files to their tmp locations and then write a log that modifies entries
and renames the files into their proper location.
To avoid code duplication the private function would need to handle the
normal svn_wc_add cases so svn_wc_add can just be the public stub.
This allows an interrupted operation to be completed by cleanup. If a
user tried to repeat the operation after an interrupted operation then
the wc would be locked and they'd be told to run cleanup...
Do you agree that this would resolve the problem?
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org