Re: svn commit: r32487 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc

["Rui, Guo" <ti...@mail.ustc.edu.cn>]

On Sun, Aug 17, 2008 at 09:44:08PM -0400, Karl Fogel wrote:
> firemeteor@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r32487 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc

[Karl Fogel <kf...@red-bean.com>]

"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> 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)))

So we don't use target_entry->absent at all?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org