Re: svn commit: r34247 - in trunk/subversion: include/private libsvn_client libsvn_wc tests/libsvn_wc

[Julian Foad <ju...@btopenworld.com>]

neels@tigris.org wrote:
> Author: neels
> Date: Mon Nov 17 22:13:52 2008
> New Revision: 34247
> 
> Log:
> Fix (probably most of) issue #3320: Commit fails to block on tree-conflict.

Excellent.

> * subversion/libsvn_client/commit_util.c
>   (harvest_committables):
>     Already check for tree-conflicts on immediate child nodes when harvesting
>     a directory, adhering to the requested DEPTH. Thus catch tree-conflicts
>     on nodes that don't have an entry (only when recursing; when the target
>     is a victim and has no entry, "commit" still gives a different error).
[...]
> Modified: trunk/subversion/libsvn_client/commit_util.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/commit_util.c?pathrev=34247&r1=34246&r2=34247
> ==============================================================================
> --- trunk/subversion/libsvn_client/commit_util.c	Mon Nov 17 17:30:52 2008	(r34246)
> +++ trunk/subversion/libsvn_client/commit_util.c	Mon Nov 17 22:13:52 2008	(r34247)
> @@ -343,6 +343,48 @@ harvest_committables(apr_hash_t *committ
>                                   svn_path_local_style(path, pool));
>      }
>  
> +  /* If this is a dir, walk all tree-conflicts recorded on immediate
> +   * children. Adhering to the requested depth, abort the commit on a
> +   * tree-conflicted child node.
> +   * Tree-conflicts information is stored in the victim's immediate parent.
> +   * In some cases of an absent tree-conflicted victim, the tree-conflict
> +   * information in its parent dir is the only indication that the node
> +   * is under version control. This check is necessary for this case.
> +   * In all other cases, this simply bails out a little bit earlier.
> +   * Note: Tree-conflicts can only be found in "THIS_DIR" entries. */
> +  if ((entry->kind == svn_node_dir) && (depth != svn_depth_empty)
> +      && (strcmp(entry->name, SVN_WC_ENTRY_THIS_DIR) == 0))
> +    {
> +      apr_array_header_t *conflicts;
> +      const svn_wc_conflict_description_t *conflict;
> +      int i;
> +
> +      conflicts = apr_array_make(pool, 0,
> +                                 sizeof(svn_wc_conflict_description_t *));
> +      SVN_ERR(svn_wc__read_tree_conflicts_from_entry(conflicts, entry,
> +                                                     path, pool));
> +
> +      for (i = 0; i < conflicts->nelts; i++)
> +        {
> +          conflict = APR_ARRAY_IDX(conflicts, i,
> +                                   svn_wc_conflict_description_t *);
> +
> +          if ((conflict->node_kind == svn_node_dir) &&
> +              (depth == svn_depth_files))
> +            continue;
> +
> +          /* So we've encountered a conflict that is included in DEPTH.
> +           * Bail out. */
> +
> +          if (SVN_WC__CL_MATCH(changelists, entry))

Here, "entry" is the parent's entry, but we should be checking against
the child's entry (if it has one).

> +            return svn_error_createf(
> +                     SVN_ERR_WC_FOUND_CONFLICT, NULL,
> +                     _("Aborting commit: '%s' remains in conflict"),
> +                     svn_path_local_style(conflict->path, pool));
> +        }

The attached uncompiled untested patch is the sort of thing I think it
needs to check changelist memership correctly.

We also should put this chunk of code in a subroutine, so as not to
complicate the reading and understanding of harvest_committables().

- Julian