You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Rui, Guo" <ti...@mail.ustc.edu.cn> on 2008/07/08 03:32:41 UTC
Re: svn commit: r31986 - in branches/issue-2843-dev/subversion:
libsvn_client libsvn_wc
I was not very confident to make these modifications, since I'm not familar
with them. It is good to hear your positive reply.
On Mon, Jul 07, 2008 at 03:32:56PM -0400, Karl Fogel wrote:
> firemeteor@tigris.org writes:
> > --- branches/issue-2843-dev/subversion/libsvn_client/merge.c (r31985)
> > +++ branches/issue-2843-dev/subversion/libsvn_client/merge.c (r31986)
> > @@ -3260,6 +3260,8 @@ get_mergeinfo_walk_cb(const char *path,
> > !svn_path_compare_paths(path, wb->merge_target_path);
> > const char *parent_path = svn_path_dirname(path, pool);
> >
> > + /* TODO(#2843) How to deal with a excluded item on merge? */
> > +
> > /* We're going to receive dirents twice; we want to ignore the
> > first one (where it's a child of a parent dir), and only use
> > the second one (where we're looking at THIS_DIR). The exception
>
> I think we should handle that the same way we would if the item were
> deleted... which we can see a bit later in the code:
>
> /* Ignore the entry if it does not exist at the time of interest. */
> if (entry->deleted)
> return SVN_NO_ERROR;
Thanks for pointing it out. I do not understand the code very well.
>
> > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31985)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31986)
> > @@ -538,6 +538,14 @@ process_committed_internal(int *log_numb
> > if (! strcmp(name, SVN_WC_ENTRY_THIS_DIR))
> > continue;
> >
> > + /* TODO(#2843)
> > + We come to this branch since we are commiting an added tree.
> > + Check this again after the behavior of croping an newly added
> > + tree is definined. Anyway, it will be safer to check for excluded
> > + items here. */
> > + if (current_entry->depth == svn_depth_exclude)
> > + continue;
> > +
> > /* Create child path by telescoping the main path. */
> > this_path = svn_path_join(path, name, subpool);
>
> "cropping" and "defined", and be careful of line length in the comment.
>
> What exactly do you mean by "safer" above? (I don't think the code is
> wrong, I'm just trying to understand the comment.)
The check does not make things worse. But maybe we just don't need the check
here? If we do not set up exclude flag for new added items. Whether the
code branch handles copy is important.
>
> > @@ -1212,6 +1220,8 @@ svn_wc_delete3(const char *path,
> > /* The deleted state is only available in the entry in parent's
> > entries file */
> > SVN_ERR(svn_wc_adm_retrieve(&parent_access, adm_access, parent, pool));
> > + /* We don't need to check for excluded item, since we won't fall into
> > + this path in that case. */
> > SVN_ERR(svn_wc_entries_read(&entries, parent_access, TRUE, pool));
> > entry_in_parent = apr_hash_get(entries, base_name, APR_HASH_KEY_STRING);
> > was_deleted = entry_in_parent ? entry_in_parent->deleted : FALSE;
>
> When you say "path", you're talking about a code path, not a filesystem
> path, right? Might want to clarify that.
Okay.
> > --- branches/issue-2843-dev/subversion/libsvn_wc/copy.c (r31985)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/copy.c (r31986)
> > @@ -223,6 +223,7 @@ copy_added_dir_administratively(const ch
> > SVN_ERR(svn_wc_entry(&entry, src_fullpath, src_child_dir_access,
> > TRUE, subpool));
> >
> > + /* TODO(#2843) Should we skip the excluded src? */
> > /* Recurse on directories; add files; ignore the rest. */
> > if (this_entry.filetype == APR_DIR)
> > {
>
> Hmmm. I'm not sure. If someone copies a tree T that contains excluded
> subdir S, then when whey commit, will the new tree T_NEW (in the
> repository) also contain S_NEW? Once we answer this question, I think
> it will be clear how other things should behave.
Some day I did an experiment on this. The version I used just copied the
exclude flag to the new tree. And after commit, the server side will have a
full source tree in the target position. I think this behavior is just fine.
Maybe it is a good idea to maintain this behavior and make the code more
robust?
> > --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31985)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c (r31986)
> > @@ -209,6 +209,10 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> > if (!entry || entry->kind != svn_node_dir)
> > return SVN_NO_ERROR;
> >
> > + /* TODO(#2843): Re-consider the behavior of cropping items with scheduled
> > + add/delete. Maybe we don't need to setup the exclude flag when the taget
> > + is just added without history. */
> > +
> > /* Crop the target itself if we are requested to. */
> > if (depth == svn_depth_exclude)
> > {
>
> "target", not "taget"
>
> I think I understand. We don't need to set up the exclude flag, we just
> need to unversion the target tree, right?
Yep.
>
> > --- branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31985)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31986)
> > @@ -3029,7 +3029,7 @@ svn_wc_walk_entries3(const char *path,
> > svn_path_local_style(path, pool)),
> > walk_baton, pool);
> >
> > - if (entry->kind == svn_node_file)
> > + if (entry->kind == svn_node_file || entry->depth == svn_depth_exclude)
> > return walk_callbacks->handle_error
> > (path, walk_callbacks->found_entry(path, entry, walk_baton, pool),
> > walk_baton, pool);
>
> Should we ce balling walk_callbacks->found_entry() on the path at all,
> in the svn_depth_exclude case? What would happen if we just did
> nothing?
svn_wc_walk_entries3() is just a general purpose wc walking framework. I don't
think it is a good idea to preclude anything from the framework, unless that
is definitely unnecessary. The client can choose whether they want hidden
entries. If they choose to see hidden entries, they should prepared to handle
excluded target. As far as I know, there is only one call to this function
with show_hidden set to TRUE -- in the merge logic.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31986 - in branches/issue-2843-dev/subversion: libsvn_client libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31985)
>> > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_ops.c (r31986)
>> > @@ -538,6 +538,14 @@ process_committed_internal(int *log_numb
>> > if (! strcmp(name, SVN_WC_ENTRY_THIS_DIR))
>> > continue;
>> >
>> > + /* TODO(#2843)
>> > + We come to this branch since we are commiting an added tree.
>> > + Check this again after the behavior of croping an newly added
>> > + tree is definined. Anyway, it will be safer to check for excluded
>> > + items here. */
>> > + if (current_entry->depth == svn_depth_exclude)
>> > + continue;
>> > +
>> > /* Create child path by telescoping the main path. */
>> > this_path = svn_path_join(path, name, subpool);
>>
>> "cropping" and "defined", and be careful of line length in the comment.
>>
>> What exactly do you mean by "safer" above? (I don't think the code is
>> wrong, I'm just trying to understand the comment.)
>
> The check does not make things worse. But maybe we just don't need the check
> here? If we do not set up exclude flag for new added items. Whether the
> code branch handles copy is important.
Ah. A good way to write that is:
We're here because we are commiting an added tree.
Check this code again after we've defined the behavior of
cropping a newly-added tree.
Checking for excluded items here does not make things worse, but
possibly we don't need to check at all, since we don't set up an
exclude flag for newly-added items anyway.
>> > --- branches/issue-2843-dev/subversion/libsvn_wc/copy.c (r31985)
>> > +++ branches/issue-2843-dev/subversion/libsvn_wc/copy.c (r31986)
>> > @@ -223,6 +223,7 @@ copy_added_dir_administratively(const ch
>> > SVN_ERR(svn_wc_entry(&entry, src_fullpath, src_child_dir_access,
>> > TRUE, subpool));
>> >
>> > + /* TODO(#2843) Should we skip the excluded src? */
>> > /* Recurse on directories; add files; ignore the rest. */
>> > if (this_entry.filetype == APR_DIR)
>> > {
>>
>> Hmmm. I'm not sure. If someone copies a tree T that contains excluded
>> subdir S, then when whey commit, will the new tree T_NEW (in the
>> repository) also contain S_NEW? Once we answer this question, I think
>> it will be clear how other things should behave.
>
> Some day I did an experiment on this. The version I used just copied the
> exclude flag to the new tree. And after commit, the server side will have a
> full source tree in the target position. I think this behavior is just fine.
> Maybe it is a good idea to maintain this behavior and make the code more
> robust?
Agreed. This question is similar to the longer discussion we had about
doing 'svn cp non-depth-infinity-tree new-tree', and I think the
decision here is the same as our decision there.
>> > --- branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31985)
>> > +++ branches/issue-2843-dev/subversion/libsvn_wc/entries.c (r31986)
>> > @@ -3029,7 +3029,7 @@ svn_wc_walk_entries3(const char *path,
>> > svn_path_local_style(path, pool)),
>> > walk_baton, pool);
>> >
>> > - if (entry->kind == svn_node_file)
>> > + if (entry->kind == svn_node_file || entry->depth == svn_depth_exclude)
>> > return walk_callbacks->handle_error
>> > (path, walk_callbacks->found_entry(path, entry, walk_baton, pool),
>> > walk_baton, pool);
>>
>> Should we ce balling walk_callbacks->found_entry() on the path at all,
>> in the svn_depth_exclude case? What would happen if we just did
>> nothing?
>
> svn_wc_walk_entries3() is just a general purpose wc walking framework. I don't
> think it is a good idea to preclude anything from the framework, unless that
> is definitely unnecessary. The client can choose whether they want hidden
> entries. If they choose to see hidden entries, they should prepared to handle
> excluded target. As far as I know, there is only one call to this function
> with show_hidden set to TRUE -- in the merge logic.
Okay, that makes sense to me.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org