Re: [PATCH] copy file to existing directory.

On Wed, Aug 13, 2008 at 02:36:47AM +0800, Rui, Guo wrote:
> What a shame! The same mistake once again!

:)
 
> Rui
> On Tue, Aug 12, 2008 at 06:49:15PM +0200, Stefan Sperling wrote:
> > On Wed, Aug 13, 2008 at 12:27:16AM +0800, Rui, Guo wrote:
> > > Choose either one you think better. :-)
> > > Thanks
> > >
> > > Rui
> >
> > Hey Rui,
> >
> > I think it looks like you forgot to attach the diffs.
> >
> > Stefan

The last time I reviewed your change, I didn't realise that
looking at dst_parent does not tell us anything about the
copy destination. The example change to svn_wc_copy2() I proposed
earlier in the thread is wrong, because it only checks dst_parent.
But we need to look at dst_basename, which, if it already exists,
might either be a file or directory. Sorry about that.

I've tried looking at this problem from the context of the branch
you're working on. I think I know now why the bug you're trying
to fix is a big problem for your branch.

Let me explain to make sure I have understood things correctly:

When a directory is hidden, there exists meta data about it
in the entries file of its parent directory. But the directory
does not exist on disk.

In trunk, the problem you demonstrated can only happen when someone
removes something from the working copy without telling Subversion
about it. We can always tell those people "Well, you know, you should
not be using 'rm', you should be using 'svn remove'".

But once your branch is merged to trunk, this is no longer true!
Users can then set directories as hidden, which means the same
situation can happen via normal svn commands. If they now try
to (accidentally) copy something into a hidden directory, the
working copy gets corrupted beyond repair. Unless people manually
edit the log and remove the copy command, the log cannot be replayed,
and 'svn cleanup' will keep failing.

Did I explain the problem correctly?

If so, I am convinced now that your change to copy_file_administratively
is absolutely correct. I've verified that it makes the corruption problem
go away.

So, +1 on comitting this to trunk:

> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 32410)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -403,14 +403,13 @@
> _("'%s' already exists and is in the way"),
> svn_path_local_style(dst_path, pool));
>
> - /* Even if DST_PATH doesn't exist it may still be a versioned file; it
> + /* Even if DST_PATH doesn't exist it may still be a versioned item; it
> may be scheduled for deletion, or the user may simply have removed the
> working copy. Since we are going to write to DST_PATH text-base and
> prop-base we need to detect such cases and abort. */
> SVN_ERR(svn_wc_entry(&dst_entry, dst_path, dst_parent, FALSE, pool));
> - if (dst_entry && dst_entry->kind == svn_node_file)
> + if (dst_entry && dst_entry->schedule != svn_wc_schedule_delete)
> {
> - if (dst_entry->schedule != svn_wc_schedule_delete)
> return svn_error_createf(SVN_ERR_ENTRY_EXISTS, NULL,
> _("There is already a versioned item '%s'"),
> svn_path_local_style(dst_path, pool));

Now, on to your other changes:

These changes seem to be about making Subversion report a better
error message, not about fixing the working copy corruption.

Looking at the log messages on your branch, it seems you have done
most of the hidden directory implementation in the working copy library.

However, these changes are both made in the client library, and
therefore make it aware of "hidden" directories. Do we really want
the client library to know about "hidden" directories?

But if the client library is already aware of hidden directories
on your branch, then it's fine to put this check there, too.

> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 32410)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -1801,6 +1801,45 @@
>

[...]

> + /* The container directory not exists in the disk.
> + Check if it is a missing/hidden directory or just a normal
> + non-existing directory. */
> + svn_path_split(dst_parent, &dst_pparent, &dst_pbase, pool);
> + err = svn_wc_adm_probe_open3(&adm_parent, NULL,
> + dst_pparent, FALSE, 0,
> + NULL, NULL, pool);

In the comment above, you're talking about hidden directories.
These exist on your branch, but not on trunk (yet). I don't
think we can apply this patch to trunk with this comment included.

But more importantly, since the working copy corruption problem
is fixed with the change to copy_file_administratively, I think
we should only apply that fix to trunk. Making Subversion report
more useful error messages in case directories are missing/hidden
can be done on your branch.

Do you agree?

If you do this on your branch, you should make sure that Subversion
reports similar errors in cases which are similar to this one.

I like the error message you put in your diff:
"Versioned path '%s' is either missing or hidden."

A more explicit message could be:
"The target '%s' is a versioned item and is either missing from
disk or hidden."

In case you want to do this, I have created a few shell scripts
you can use to check each case:

- copy a locally added directory (copy-added-dir-recipe.sh)
- move a locally added directory (mv-added-dir-recipe.sh)
- copy locally added file (copy-added-file-recipe.sh)
- move locally added file (mv-added-file-recipe.sh)
- copy directory (copy-dir-recipe.sh)
- move directory (mv-dir-recipe.sh)
- copy file (copy-file-recipe.sh)
- move file (mv-file-recipe.sh)

The scripts are attached to this mail.
They should later be replaced by proper unit tests which check for
the correct error message.

Note that the scripts run the final svn command in gdb.
If you're not familiar with gdb: You just need to type
        run
and press enter to run the svn command from within gdb.
You should learn about gdb if you don't know it yet, it's nice :)

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org