On Wed, Aug 13, 2008 at 12:55:09PM +0200, Stefan Sperling wrote:
> 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:
Yep! You've got the whole story.
>
> > 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.
Yep.
>
> 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?
Well the word 'hidden' just comes from the parameter show_hidden in
svn_wc_entry() series of functions (There are three of them, if I remember
correctly). Originally, the parameter show_hidden includes two kinds of
entries: deleted and absent. And now, in my branch, the excluded entry is also
considered hidden.
>
> But if the client library is already aware of hidden directories
> on your branch, then it's fine to put this check there, too.
What I can say is that the client should have the concept of 'excluded'
directory, just as the know something about deleted and absent now. I think
the check here does not make use of any internal detail that the client should
not know. Or have I lost the position to make a fair judgment? :-)
>
> > 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.
>
Again, the word 'hidden' is not specific to excluded directories. Even though
deleted directory should not be a problem here, there is always a possibility
for absent entry (Very unlikely, though).
> 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?
I think this is not specific to my branch, even though it will benefit the
most. So, if you think it should go into my branch first, I'll be OK. But
personally I still believe it can go into trunk.
But (there will always be a but :-)), consistent error report is really a
serious requirement. The most typical error report till now will fall back to
the default 'Entry XXX is already under version control.' I know that is far
from perfect, but it will be really painful to check each of this report and
provide a better one for hidden entries. I even begin to doubt that whether I
should use the default 'under version control' message in my patch.
Whatever message I use, your scripts are of great value. I'll check if the
current state of the error report is satisfactory. But I may not be able to
further improve the error report within the following days -- The final
deadline of the gsoc is approaching. I'm supposed to cleanup codes, write
tests and prepare documents for the final evaluation. I have no idea what
material Karl will ask for. If I got no explicit requirement, the plan for
the final days would be:
1. Merge back my patch to my branch and catch up with the trunk
2. Check suspicious show_hidden=TRUE calls marked as TODO items.
3. The mergeinfo issue.
PS: I'm going to go offline next week (visting Stanford), I think I have to
hurry up. What a busy summer 'vacation'!
>
> 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 :)
Yes, I do use gdb. It's very powerful, but not very handy. Do you use any
front end such as ddd or insight?
Thanks,
Rui
---------------------------------------------------------------------
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-13 20:07:35 CEST