Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: local policy)
Date: Thu, 6 Oct 2011 22:29:45 +0200
From: Stefan Sperling <stsp@elego.de>
To: dev@subversion.apache.org
Subject: Re: svn commit: r1179135 - in
 /subversion/branches/showing-merge-info/subversion:
 include/private/svn_client_private.h include/svn_client.h
 libsvn_client/prop_commands.c libsvn_client/url.c svn/main.c
 svn/mergeinfo-cmd.c
Message-ID: <20111006202945.GH11507@jack.stsp.name>
Mail-Followup-To: dev@subversion.apache.org
References: <20111005100931.CC43D23888EA@eris.apache.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20111005100931.CC43D23888EA@eris.apache.org>
User-Agent: Mutt/1.5.21 (2010-09-15)

On Wed, Oct 05, 2011 at 10:09:31AM -0000, julianfoad@apache.org wrote:
> Author: julianfoad
> Date: Wed Oct  5 10:09:30 2011
> New Revision: 1179135
> 
> URL: http://svn.apache.org/viewvc?rev=1179135&view=rev
> Log:
> On the showing-merge-info branch: Check in some work in progress.
> 
> Make 'svn mergeinfo' print a more friendly description of the merging
> situation.

> * subversion/include/private/svn_client_private.h
>   (SVN_PROP_BRANCHING_ROOT): New property name.
>   (svn_client__check_branch_root_marker): New function.

[...]

> +/* This property marks a branch root. Branches with the same value of this
> + * property are mergeable. */
> +#define SVN_PROP_BRANCHING_ROOT "svn:ignore" /* ### should be "svn:branching-root" */
> +

I think your addition of a 'branch root' property is quite a significant
step. Is this really necessary in order to improve the output of
'svn mergeinfo' or do you have additional steps planned that go beyond
tuning output?

There has been some discussion about adding a property for this
and similar purposes in the past, see
http://svn.haxx.se/dev/archive-2009-09/0156.shtml
(there are probably more threads about this topic)

One idea I've had in mind which relates to the notion of a branch root
is to have a property which specifies the merge policy for the branch,
called 'svn:mergepolicy' (or maybe even use the 'branch root' property
itself for this purpose). It could use keywords such as "cherry-pick, 
sync, reintegrate" to define the set of allowed types of merges.

Simple sketch:
 - feature-branch (can only receive catch-up merges from its direct parent
                   branch (copyfrom-source), or be reintegrated)
    => svn:mergepolicy on branch root: 'sync ^/trunk
                                        reintegrate ^/trunk'

 - release-branch (cannot be reintegrated into its parent)
    => svn:mergepolicy on branch root: 'cherry-pick ^/trunk'


I haven't though this through yet.
I think your approach of "Branches with the same value of this
property are mergeable" is a design decision which has a similar
kind of impact on how a branch-root property would be used.
How do we really want it to be used?

I think this is an exciting feature with lots of potential but
it has a lot more inherent complexity than improving 'svn mergeinfo'
output. Could we split improving the output of 'svn mergeinfo' and
identifying branch roots into two distinct feature branches?

> +/* Set *MARKER to the project-root marker that is common to SOURCE and
> + * TARGET, or to NULL if neither has such a marker.

Why do you need to introduce a new term "project"?

> +  /* Check the source's and target's branch-marker properties. */
> +  SVN_ERR(get_branch_root_marker(&target_marker, target, ctx, pool));
> +  SVN_ERR(get_branch_root_marker(&source_marker, source, ctx, pool));
> +  if (! source_marker && ! target_marker)
> +    {
> +      /* Old-style branches, not marked as such. Marker will be NULL. */
> +    }
> +  else if (! source_marker || ! target_marker)
> +    {
> +      if (source_marker)
> +        return svn_error_createf(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
> +                                 _("Source is marked as a branch of "
> +                                   "project '%s', but target is not marked"),

Same here. This terminology can confuse users because Subversion
doesn't have a 'project' concept.

Why don't you just say "marked as a branch of '%s'"?

Or are you suggesting Subversion should have a "project" concept?
If so, what is your definition of this term?

More instances of this term are below:

> +                                 source_marker);
> +      else
> +        return svn_error_createf(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
> +                                 _("Target is marked as a branch of "
> +                                   "project '%s', but source is not marked"),
> +                                 target_marker);
> +    }
> +  else if (strcmp(source_marker, target_marker) != 0)
> +    {
> +      /* ### The '.99' is just for display tidiness when I'm messing about
> +       * with using 'svn:ignore' as the branch marker property. */
> +      return svn_error_createf(SVN_ERR_CLIENT_NOT_READY_TO_MERGE, NULL,
> +                               _("error: Source is marked as branch of project '%.99s' "
> +                                 "but target is marked as branch of project '%.99s'"),
> +                               source_marker, target_marker);
> +    }
> +  *marker = source_marker;
> +  return SVN_NO_ERROR;
> +}