You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2008/04/05 03:01:51 UTC
Re: reintegrate and renames
So we haven't bothered to do the complex solution outlined here.
I chatted with dlr a little today about this. I think the right
solution for 1.5 is to provide some sort of force option that allows
the user to ignore the "branch has inconsistent mergeinfo" issue.
Specifically, I'd like to change svn_client_merge_reintegrate from a
"calculate the right two URL/revs pairs and then merge immediately"
API into a "return two URL/rev pairs, which the client can then pass
to svn_client_merge3 if it wishes" API. (This will presumably be
helpful for other merge tools as well as the svn client.) We can make
part of the return value be a list of paths that have inconsistent
mergeinfo and let the user decide if they want to do the merge anyway.
Without this, I fear --reintegrate will be a joke; it's bad enough
that Subversion barely handles merging renamed files in the first
place, but to forbid reintegrating branches with that is barely
usable.
This should be a lot simpler than the other stuff we discussed in this thread.
--dave
On Wed, Feb 27, 2008 at 11:15 AM, Karl Fogel <kf...@red-bean.com> wrote:
> "David Glasser" <gl...@davidglasser.net> writes:
>
>
> >> >> When you encounter a file ("/branch/bar") whose mergeinfo disturbs the
> >> >> desired uniformity, run:
> >> >>
> >> >> svn_repos_node_location_segments(repos, "/branch/bar"
> >> >> <HIGHER_REV_OF_MERGE__PROBABLY_HEAD>,
> >> >> SVN_INVALID_REVNUM,
> >> >> SVN_INVALID_REVNUM,
> >> >> receiver_func, receiver_baton,
> >> >> authz_read_func, authz_read_baton,
> >> >> pool);
> >> >>
> >> >> The receiver_func should check the (svn_location_segment_t *)->path on
> >> >> each invocation. If any of invocations have a path P that
> >> >>
> >> >> a) corresponds to a path T in the merge target, and
> >> >> b) T matches the path given in the problematic mergeinfo, and
> >> >> c) P has no mergeinfo /or/ has mergeinfo that would be compatible
> >> >> with the uniform mergeinfo requirement
> >> >>
> >> >> then we can proceed with the reintegration. (Remember, P is really a
> >> >> rev/path pair, since it comes from an 'svn_location_segment_t'.)
> >> >
> >> > OK. But is this likely to actually occur in the common case? I mean,
> >> > great, so we trace back /branch/bar to /trunk/foo and then, um, I
> >> > guess do yet another svn_ra_get_mergeinfo on that. But there's no
> >> > mergeinfo *on* /trunk/foo *from* /trunk/foo, which is exactly the
> >> > mergeinfo we're looking for. So does this actually work?
> >>
> >> Sure, I think it does -- step (c) is compatible with what you say
> >> above, no? We're not looking for mergeinfo per se, we're just
> >> determining uniformity.
> >
> > OK. Hmm. Can you give me a case where (c) would be false? ie, how
> > could P possibly have incompatible mergeinfo?
>
> Fascinating.
>
> I just erased a long mail in which I started down different scenarios,
> only to realize that in each one, the reintegrate checks would only
> declare non-uniformity if the merge source was, in fact, not uniform
> w.r.t. the target. In other words, the existing algorithm should
> work.
>
> So, can we skip (c) ??
>
>
>
> -Karl
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reintegrate and renames
Posted by David Glasser <gl...@davidglasser.net>.
Here's a half-assed stab at the first part of what I'm talking about.
Didn't actually update the docs yet. Unfortunately I don't think I
will be able to work on this myself in the next few days but maybe
someone else can pick up from here. This just moves the actual
merging out of the reintegrate API (which perhaps should be renamed
too). Next step would be to add a returned data structure from
calculate_left_hand_side/svn_client_merge_reintegrate listing the
inconsistent mergeinfo (instead of throwing an error); then the CLI
client can decide not to go forward, leaving us the option in 1.5.1
(or whatever) to add an interactive prompt or a force option or
whatever.
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 30348)
+++ subversion/include/svn_client.h (working copy)
@@ -2532,7 +2532,11 @@
* @since New in 1.5.
*/
svn_error_t *
-svn_client_merge_reintegrate(const char *source,
+svn_client_merge_reintegrate(const char **url1,
+ svn_revnum_t *rev1,
+ const char **url2,
+ svn_revnum_t *rev2,
+ const char *source,
const svn_opt_revision_t *peg_revision,
const char *target_wcpath,
svn_boolean_t dry_run,
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 30348)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -5490,7 +5490,11 @@
svn_error_t *
-svn_client_merge_reintegrate(const char *source,
+svn_client_merge_reintegrate(const char **url1_p,
+ svn_revnum_t *rev1_p,
+ const char **url2_p,
+ svn_revnum_t *rev2_p,
+ const char *source,
const svn_opt_revision_t *peg_revision,
const char *target_wcpath,
svn_boolean_t dry_run,
@@ -5621,16 +5625,20 @@
### of the other (what's erroneously referred to as "ancestrally
### related" in this source file). We can merge to trunk without
### implementing this. */
- SVN_ERR(merge_cousins_and_supplement_mergeinfo(target_wcpath, entry,
- adm_access, ra_session,
- url1, rev1, url2, rev2,
- yc_ancestor_rev,
- source_repos_root,
- wc_repos_root,
- svn_depth_infinity,
- FALSE,
- FALSE, FALSE, dry_run,
- merge_options, ctx, pool));
+ *url1_p = url1;
+ *rev1_p = rev1;
+ *url2_p = url2;
+ *rev2_p = rev2;
+/* SVN_ERR(merge_cousins_and_supplement_mergeinfo(target_wcpath, entry, */
+/* adm_access, ra_session, */
+/* url1, rev1, url2, rev2, */
+/* yc_ancestor_rev, */
+/* source_repos_root, */
+/* wc_repos_root, */
+/* svn_depth_infinity, */
+/* FALSE, */
+/* FALSE, FALSE, dry_run, */
+/* merge_options,
ctx, pool)); */
/* Shutdown the administrative session. */
SVN_ERR(svn_wc_adm_close(adm_access));
Index: subversion/svn/merge-cmd.c
===================================================================
--- subversion/svn/merge-cmd.c (revision 30348)
+++ subversion/svn/merge-cmd.c (working copy)
@@ -45,7 +45,7 @@
apr_array_header_t *targets;
const char *sourcepath1 = NULL, *sourcepath2 = NULL, *targetpath = "";
svn_boolean_t two_sources_specified = TRUE;
- svn_error_t *err;
+ svn_error_t *err = SVN_NO_ERROR;
svn_opt_revision_t first_range_start, first_range_end, peg_revision1,
peg_revision2;
apr_array_header_t *options, *ranges_to_merge = opt_state->revision_ranges;
@@ -287,16 +287,32 @@
if (opt_state->reintegrate)
{
+ const char *url1, *url2;
+ svn_revnum_t rev1, rev2;
+ svn_opt_revision_t opt_rev1, opt_rev2;
if (opt_state->force)
return svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
_("--force cannot be used with "
"--reintegrate"));
- err = svn_client_merge_reintegrate(sourcepath1,
- &peg_revision1,
- targetpath,
- opt_state->dry_run,
- options, ctx, pool);
+ SVN_ERR(svn_client_merge_reintegrate(&url1,
+ &rev1,
+ &url2,
+ &rev2,
+ sourcepath1,
+ &peg_revision1,
+ targetpath,
+ opt_state->dry_run,
+ options, ctx, pool));
+
+ opt_rev1.kind = opt_rev2.kind = svn_opt_revision_number;
+ opt_rev1.value.number = rev1;
+ opt_rev2.value.number = rev2;
+
+ SVN_ERR(svn_client_merge3(url1, &opt_rev1, url2, &opt_rev2,
+ targetpath, svn_depth_infinity,
+ FALSE, FALSE, FALSE, opt_state->dry_run,
+ options, ctx, pool));
}
else
err = svn_client_merge_peg3(sourcepath1,
On Sat, Apr 5, 2008 at 11:34 AM, Mark Phippard <ma...@gmail.com> wrote:
> On Sat, Apr 5, 2008 at 2:13 PM, David Glasser <gl...@davidglasser.net> wrote:
> > On Sat, Apr 5, 2008 at 8:33 AM, Mark Phippard <ma...@gmail.com> wrote:
> > > On Fri, Apr 4, 2008 at 11:01 PM, David Glasser <gl...@davidglasser.net> wrote:
>
> > > I guess I am just proposing we wait until we know the entirety of the
> > > problem in real situations. The user has a relatively easy "out" in
> > > the current code (namely they can run the 2-URL merge and get the
> > > identical results. We can optimize this for 1.6 if we see the need,
> > > or we can even do it before RC2 if after you look at the problem
> > > deeper you become more convinced it is the right thing to do now.
> >
> > Well, take a peek at one of the XFailing reintegrate tests and see how
> > unreasonable it is...
>
> I am not doubting how easy it is to create the scenario. I still have
> two thoughts:
>
> 1) Maybe we should be trying to attack the problem at the point where
> we create it. In other words, be smarter about when we create the
> mergeinfo that causes the problem.
>
> 2) Merges with renames basically do not work anyway. So maybe people
> just avoid renames? I doubt that is the case. But this issue is just
> a minor part of the larger problem.
>
>
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reintegrate and renames
Posted by Mark Phippard <ma...@gmail.com>.
On Sat, Apr 5, 2008 at 2:13 PM, David Glasser <gl...@davidglasser.net> wrote:
> On Sat, Apr 5, 2008 at 8:33 AM, Mark Phippard <ma...@gmail.com> wrote:
> > On Fri, Apr 4, 2008 at 11:01 PM, David Glasser <gl...@davidglasser.net> wrote:
> > I guess I am just proposing we wait until we know the entirety of the
> > problem in real situations. The user has a relatively easy "out" in
> > the current code (namely they can run the 2-URL merge and get the
> > identical results. We can optimize this for 1.6 if we see the need,
> > or we can even do it before RC2 if after you look at the problem
> > deeper you become more convinced it is the right thing to do now.
>
> Well, take a peek at one of the XFailing reintegrate tests and see how
> unreasonable it is...
I am not doubting how easy it is to create the scenario. I still have
two thoughts:
1) Maybe we should be trying to attack the problem at the point where
we create it. In other words, be smarter about when we create the
mergeinfo that causes the problem.
2) Merges with renames basically do not work anyway. So maybe people
just avoid renames? I doubt that is the case. But this issue is just
a minor part of the larger problem.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reintegrate and renames
Posted by David Glasser <gl...@davidglasser.net>.
On Sat, Apr 5, 2008 at 8:33 AM, Mark Phippard <ma...@gmail.com> wrote:
> On Fri, Apr 4, 2008 at 11:01 PM, David Glasser <gl...@davidglasser.net> wrote:
> > So we haven't bothered to do the complex solution outlined here.
> >
> > I chatted with dlr a little today about this. I think the right
> > solution for 1.5 is to provide some sort of force option that allows
> > the user to ignore the "branch has inconsistent mergeinfo" issue.
> >
> > Specifically, I'd like to change svn_client_merge_reintegrate from a
> > "calculate the right two URL/revs pairs and then merge immediately"
> > API into a "return two URL/rev pairs, which the client can then pass
> > to svn_client_merge3 if it wishes" API. (This will presumably be
> > helpful for other merge tools as well as the svn client.) We can make
> > part of the return value be a list of paths that have inconsistent
> > mergeinfo and let the user decide if they want to do the merge anyway.
> >
> > Without this, I fear --reintegrate will be a joke; it's bad enough
> > that Subversion barely handles merging renamed files in the first
> > place, but to forbid reintegrating branches with that is barely
> > usable.
> >
> > This should be a lot simpler than the other stuff we discussed in this thread.
>
> Are you just being hard on this because it was your idea? I am not
> sure it is as bad as you say. It is possible that when you, Karl and
> Dan designed this you had bigger plans for this option, but I thought
> it was always about giving the user some syntactic sugar. Basically,
> make using 2-URL merge easier with some checks to make it safe. If
> those checks do not pass, the user still has a "force" option
> available, namely they can run the 2-URL merge themselves. Of course
> another option they have is to remove the extra mergeinfo if they do
> not want it.
Well, the big hole here is that svn doesn't actually tell the user
what the URLs would be to run it themselves. Separating the APIs
would help here.
> I am not against relaxing the checks or providing more "sugar" if we
> are comfortable doing so, but I am not certain it cannot wait until
> after RC1 or even after GA.
>
> First, while we know of situations that will easily create this
> problem situation, we still do not know what the real world
> ramifications are. How prevalent will the problem be, and just as
> importantly, whether in those situations the "right" answer would be
> to relax the checking. It is possible the solution can come from
> elsewhere as well. For example, maybe in the scenarios where this
> comes up, and relaxed checking is the solution, we could have solved
> the problem when it was created. In other words, maybe there was more
> checking we could have done before we created the extra mergeinfo that
> caused the problem.
>
> Second, while we could do some 2-URL merges now and test this out, I
> wonder if there will be some new eliding scenarios that will come out
> of this. In other words, you have this extra mergeinfo, you do not
> care about it, you tell reintegrate to --force itself to run. Now it
> runs the merge. Do we know what the 2-URL merge does with the
> mergeinfo? I think we know it does the "right" merge, but will we be
> creating new expectations about what it does with that extra
> mergeinfo?
>
> I guess I am just proposing we wait until we know the entirety of the
> problem in real situations. The user has a relatively easy "out" in
> the current code (namely they can run the 2-URL merge and get the
> identical results. We can optimize this for 1.6 if we see the need,
> or we can even do it before RC2 if after you look at the problem
> deeper you become more convinced it is the right thing to do now.
Well, take a peek at one of the XFailing reintegrate tests and see how
unreasonable it is...
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: reintegrate and renames
Posted by Mark Phippard <ma...@gmail.com>.
On Fri, Apr 4, 2008 at 11:01 PM, David Glasser <gl...@davidglasser.net> wrote:
> So we haven't bothered to do the complex solution outlined here.
>
> I chatted with dlr a little today about this. I think the right
> solution for 1.5 is to provide some sort of force option that allows
> the user to ignore the "branch has inconsistent mergeinfo" issue.
>
> Specifically, I'd like to change svn_client_merge_reintegrate from a
> "calculate the right two URL/revs pairs and then merge immediately"
> API into a "return two URL/rev pairs, which the client can then pass
> to svn_client_merge3 if it wishes" API. (This will presumably be
> helpful for other merge tools as well as the svn client.) We can make
> part of the return value be a list of paths that have inconsistent
> mergeinfo and let the user decide if they want to do the merge anyway.
>
> Without this, I fear --reintegrate will be a joke; it's bad enough
> that Subversion barely handles merging renamed files in the first
> place, but to forbid reintegrating branches with that is barely
> usable.
>
> This should be a lot simpler than the other stuff we discussed in this thread.
Are you just being hard on this because it was your idea? I am not
sure it is as bad as you say. It is possible that when you, Karl and
Dan designed this you had bigger plans for this option, but I thought
it was always about giving the user some syntactic sugar. Basically,
make using 2-URL merge easier with some checks to make it safe. If
those checks do not pass, the user still has a "force" option
available, namely they can run the 2-URL merge themselves. Of course
another option they have is to remove the extra mergeinfo if they do
not want it.
I am not against relaxing the checks or providing more "sugar" if we
are comfortable doing so, but I am not certain it cannot wait until
after RC1 or even after GA.
First, while we know of situations that will easily create this
problem situation, we still do not know what the real world
ramifications are. How prevalent will the problem be, and just as
importantly, whether in those situations the "right" answer would be
to relax the checking. It is possible the solution can come from
elsewhere as well. For example, maybe in the scenarios where this
comes up, and relaxed checking is the solution, we could have solved
the problem when it was created. In other words, maybe there was more
checking we could have done before we created the extra mergeinfo that
caused the problem.
Second, while we could do some 2-URL merges now and test this out, I
wonder if there will be some new eliding scenarios that will come out
of this. In other words, you have this extra mergeinfo, you do not
care about it, you tell reintegrate to --force itself to run. Now it
runs the merge. Do we know what the 2-URL merge does with the
mergeinfo? I think we know it does the "right" merge, but will we be
creating new expectations about what it does with that extra
mergeinfo?
I guess I am just proposing we wait until we know the entirety of the
problem in real situations. The user has a relatively easy "out" in
the current code (namely they can run the 2-URL merge and get the
identical results. We can optimize this for 1.6 if we see the need,
or we can even do it before RC2 if after you look at the problem
deeper you become more convinced it is the right thing to do now.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org