On Sun, Aug 17, 2008 at 09:44:08PM -0400, Karl Fogel wrote:
> firemeteor_at_tigris.org writes:
> > --- branches/issue-2843-dev/subversion/libsvn_client/copy.c
> > +++ branches/issue-2843-dev/subversion/libsvn_client/copy.c
> > @@ -1638,13 +1638,28 @@ repos_to_wc_copy(const apr_array_header_
> >
> > svn_pool_clear(iterpool);
> >
> > - SVN_ERR(svn_wc_entry(&ent, pair->dst, adm_access, FALSE, iterpool));
> > - if (ent && (ent->kind != svn_node_dir) &&
> > - (ent->schedule != svn_wc_schedule_delete))
> > - return svn_error_createf
> > - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > - _("Entry for '%s' exists (though the working file is missing)"),
> > - svn_path_local_style(pair->dst, pool));
> > + SVN_ERR(svn_wc_entry(&ent, pair->dst, adm_access, TRUE, iterpool));
> > + if (ent)
> > + {
> > + /* TODO(#2843): Rework the error report. Maybe we can simplify the
> > + condition. Currently, the first is about hidden items and the
> > + second is for missing items. */
> > + if (ent->depth == svn_depth_exclude
> > + || ent->absent)
> > + {
> > + return svn_error_createf
> > + (SVN_ERR_ENTRY_EXISTS,
> > + NULL, _("'%s' is already under version control"),
> > + svn_path_local_style(pair->dst, pool));
> > + }
> > + else if ((ent->kind != svn_node_dir) &&
> > + (ent->schedule != svn_wc_schedule_delete)
> > + && ! ent->deleted)
> > + return svn_error_createf
> > + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("Entry for '%s' exists (though the working file is missing)"),
> > + svn_path_local_style(pair->dst, pool));
> > + }
> > }
>
> I think the detailed error reporting here is good, and I'm not sure
> there's any way to simplify the conditional (nor is there any need to).
The "Rework the error report" is inspired by the suggestion from Stefan in the
thread [PATCH] copy item to existing directory. I think there can be some
potential simplication if we provide some better error report for hidden
enries. (The "already under version control" is far from perfect.)
>
> > --- branches/issue-2843-dev/subversion/libsvn_wc/copy.c
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/copy.c
> > @@ -810,6 +813,21 @@ svn_wc_copy2(const char *src_path,
> > _("Cannot copy to '%s' as it is scheduled for deletion"),
> > svn_path_local_style(svn_wc_adm_access_path(dst_parent), pool));
> >
> > + /* TODO(#2843): Rework the erroer report. */
> > + /* Check if the copy target is missing or hidden and thus not exist on the
> > + disk, before actually doing the file copy. */
> > + target_path = svn_path_join(dst_path, dst_basename, pool);
> > + SVN_ERR(svn_wc_entry(&target_entry, target_path, dst_parent, TRUE, pool));
> > + if (target_entry
> > + && ((target_entry->depth == svn_depth_exclude)
> > + || target_entry->absent))
> > + {
> > + return svn_error_createf
> > + (SVN_ERR_ENTRY_EXISTS,
> > + NULL, _("'%s' is already under version control"),
> > + svn_path_local_style(target_path, pool));
> > + }
> > +
> > SVN_ERR(svn_io_check_path(src_path, &src_kind, pool));
> >
> > if (src_kind == svn_node_file)
>
> Hmmm. I expected the opposite condition:
>
> if (target_entry
> && (! ((target_entry->depth == svn_depth_exclude)
> || target_entry->absent)))
>
> That is: "if there's an entry already here, and it's *not* excluded or
> absent, then let's error".
>
> But your conditional seems to say: "if there's entry already here *and*
> it is excluded or absent, then let's error (otherwise, let's not error)"
>
> Isn't that the opposite of what we want?
The purpose of this test is to catch mising/hidden items before we do the
actual copy. Both your condition and mine do not cover all the cases. A better
one should like this:
(target_entry &&
(!target_entry->deleted ||
(target_entry->schedule == add/replace)))
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-28 18:42:15 CEST