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_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-07-08 05:32:58 CEST