You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2014/04/07 10:34:49 UTC
Three-way merge markers by default
What do people feel about applying this patch (from the FreeBSD port) to trunk?
[[[
http://svn.freebsd.org/ports/head/devel/subversion/files/extra-patch-3way-conflict-markers (mod_dav_svn)
https://svnweb.freebsd.org/ports/head/devel/subversion/files/extra-patch-3way-conflict-markers?view=log (viewvc)
(and mentioned previously in context of custom keywords: <ht...@ted.stsp.name>)
diff -ruN subversion/libsvn_wc/merge.c subversion/libsvn_wc/merge.c
--- subversion/libsvn_wc/merge.c 2011-08-06 19:15:44.000000000 +0400
+++ subversion/libsvn_wc/merge.c 2011-09-07 21:47:19.000000000 +0400
@@ -413,7 +413,7 @@
target_marker,
right_marker,
"=======", /* separator */
- svn_diff_conflict_display_modified_latest,
+ svn_diff_conflict_display_modified_original_latest,
pool));
SVN_ERR(svn_stream_close(ostream));
]]]
All it does is change the contents of conflict files in --accept=postpone mode:
it causes conflict markers to be rendered not as two sides
<<<<<<
mine
======
theirs
>>>>>>
but as three sides
<<<<<<
mine
||||||
original
======
theirs
>>>>>>
.
I find the latter style much easier to work with --- it is easier for me to
"Apply diff(ORIGINAL, THEIRS) to MINE" than to "Compare (MINE, THEIRS) and
sort out which parts of the delta to keep and which to reverse".
Does this break compatibility somehow? Scripts have the foo.c.{mine,theirs,
merged} files to work with. As to users, they may already be familiar with the
three-way syntax since the interactive conflicts resolver generates it[1]; if
not, the book could explain the |||||| block like it explains <<<<<< and >>>>>>.
Daniel
[1] The interactive conflicts resolver generates three-way diffs in the 'dc'
(display-conflicts) command:
[[[
wc/trunk% svn merge ../branch
Conflict discovered in 'foo'.
Select: (p) postpone, (df) diff-full, (e) edit,
(mc) mine-conflict, (tc) theirs-conflict,
(s) show all options: dc
<<<<<<< MINE (select with 'mc') (1)
foo on trunk
||||||| ORIGINAL (1)
foo
=======
foo on branch
>>>>>>> THEIRS (select with 'tc') (1)
]]]
The inclusion of the "foo\n" version makes the conflict easier to
resolve.
Re: Three-way merge markers by default
Posted by 'Daniel Shahaf' <d....@daniel.shahaf.name>.
Julian Foad wrote on Mon, Apr 07, 2014 at 13:53:36 +0100:
> Bert Huijben wrote:
> > Julian Foad wrote:
> >> +1 to the suggestion. The current two-way info is pretty difficult to use in any
> >> non-trivial case, and the 3-way suggestion is significantly more usable and not
> >> much more difficult to understand. Furthermore it is what I expect to see for
> >> a three-way merge, and, if I remember correctly, is what I already do see in
> >> the interactive conflict resolver.
> >
> > +1 on the idea of improving the merge information..
> >
> > But how this eventually should end up in 'svn' depends really on the
> > question:
> > * Is the file containing the markers the place to learn about the different
> > sides or is it just a file created to allow easy resolving.
>
>
> Good question. I suppose we need to decide whether this file is more an API or more a UI. If we decide it's more important to maintain backward compatibility and treat it as an API, then we should not change it, and should provide the requested change another way.
>
We seem to be in agreement that foo.c is not an API, but
foo.c{.merge-left,.merge-right,.mine} _are_ an API. The former file can
be derived from the latter three files.
> Is your merge tool, or any software you know of, relying on this file's current format?
>
>
> > The file only contains the information about the conflicting hunks, while
> > you would really need to know about all changes to decide what to do. And in
> > that respect the 'svn resolve' interactive resolver provides much more
> > information.
>
What is that additional information?
To be clear, my use-case had plenty of conflicts that couldn't be
resolved by any of the pre-cooked uptions (mf/mc/tf/tc/12/12/etc); they
had to be manually merged. Conflicts that can be solved by the
pre-cooked option I just resolve during the update or merge, so they
never get as far as being rendered in the target file on disk.
(Full disclosure: my use-case was with git-svn, not with svn itself.
When I say later "I used 3-way conflicts rendering", it was with
git-svn, not with a patched svn build. But I believe the experience
carries over.)
>
> Yes, I agree. Or, to talk about the same level of API, the set of
> conflict 'artifact' files (filename.mine/filename.yours/etc.) hold the
> full information needed.
>
>
> > Before 1.7/1.8 we couldn't really restart the resolving after an operation,
> > but this is now a much better tool to resolve conflicts than we can ever
> > create in a plain text file.
>
>
> [...]
> >> It's true that the target file has a pristine version, but the pristine version isn't
> >> *particularly* associated with the merge itself. We merge changes into the
> >> working/actual version, and whether I commit local mods before starting the
> >> merge makes no difference to the merging procedure or to the desired
> >> result. The pristine version is just "the version before the version I'm merging
> >> into". The fact that it was uncommitted makes it interesting from a WC
> >> management perspective, but not particularly from a merge perspective.
> >
> > Essentially a merge is incorporating the changes between LEFT and RIGHT into
> > the PRISTINE file.
>
>
> We probably mean the same thing but are describing it in different
> ways. To me, the essence of a merge is it merges into the working
> file. That's the purpose, the logical idea, the intended
> functionality. When there are no local changes the pristine text is
> the same as the working text but that's just just incidental.
>
Agreed, merge takes the delta from LEFT to RIGHT and applies it to WORKING.
The PRISTINE contents is ignored.
>
> > But as we support merging into a locally changed file we just ignore the
> > PRISTINE file and use the original WORKING as if it were the pristine file.
> >
> > That doesn't make the difference between PRISTINE and WORKING not
> > interesting... It still contains the important notion of what is really an
> > uncommitted local change (MINE) vs what was already there on checkout
> > (BASE/PRISTINE).
>
Certainly. But this isn't a property unique to PRISTINE; just like a
PRISTINE-aware merge tool could say that a conflict is caused by the
pre-merge local changes, a PREV-aware merge tool could flag a conflict
as caused by PREV's successor commit (the one that resulted in
PRISTINE). More generally, a merge tool might run 'blame -r WORKING' on
the .mine file and attribute each conflict that way.
>
> Again, we understand the same thing in different ways.
>
> > Sure there are even more interesting revisions if you know the full graph of
> > changes, but the merge code in libsvn_wc just knows these 4
> > files(PRISTINE,LEFT,RIGHT,WORKING), and produces a MERGED file from them:
> > and a Boolean conflicted vs not conflicted.
>
>
> Right, but the only reason the WC library needs to know about the pristine version is because that's such a big part of how the WC is architected. (There is no fully abstracted API for accessing the working/actual layer without talking about the pristines.) If you look at the merge logic in libsvn_client, it doesn't care.
>
>
>
> > I'm pretty sure that you need more than a plain text file GUI to add the
> > notion of more than 3 trees in a way that our users are able to understand
> > it.
>
>
> Very likely.
>
True, but not really relevant: users don't need to understand the
concept of three trees to be able to resolve a conflict rendered in
the three-way form.
All users need to know is "Apply the delta from |||||| to ======
to the <<<<<< part".
And, mind you, that's a hell of a clearer instruction then "Take the
<<<< and ==== part, solve the problem and let me know when you have done
so. Good luck.", which is basically what the 2-way display gives.
> > But I'm thinking that calling either LEFT or PRISTINE 'original' in the
> > MERGED file is not really going to help our typical user.
Then the labels can say something else. We could use single words or
even sentences (up to 60 bytes long, assuming an 80-column file).
> > And I wish my merge tool was smart enough to really understand 4 inputs...
> > Showing LEFT as the older version is showing a lot of non-changes as if they
> > were interesting for the merge, while they could have been filtered as
> > unrelated with access to PRISTINE.
>
What does this have to do with the proposed behaviour change?
It sounds like you're describing general problems you see with merging
(lack of good three-way tree-aware diff3, and a merge tool that flags
false-positive changes). I don't understand what any of those problems
have to do with the proposed one-line change.
The --accept=postpone text rendering isn't supposed to be a bells and
whistles tool. It's the built-in, default tool. The question that
matters isn't what's the best merge tool, or the best way to approach a
given merge; the question that matters is: _provided_ that the user is
using the built-in tool, _and_ has chosen --accept=postpone, whether
they would be better served by a 2-way or 3-way rendering of conflicts.
And I think the 3-way is better. I've used it and it makes resolution a
breeze. All the information I need is right there in front of me, in
the editor instance I already have open. That's a superset of the
information given by the 2-way renderer, and the additional information
is useful: it means I have a *clear algorithm* to follow to resolve the
problem and move on with whatever I'm working on.
It's not a general three-way tree-aware merge tool. It simply is a
change that solves a real problem.
Daniel
Re: Three-way merge markers by default
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> Julian Foad wrote:
>> +1 to the suggestion. The current two-way info is pretty difficult to use in any
>> non-trivial case, and the 3-way suggestion is significantly more usable and not
>> much more difficult to understand. Furthermore it is what I expect to see for
>> a three-way merge, and, if I remember correctly, is what I already do see in
>> the interactive conflict resolver.
>
> +1 on the idea of improving the merge information..
>
> But how this eventually should end up in 'svn' depends really on the
> question:
> * Is the file containing the markers the place to learn about the different
> sides or is it just a file created to allow easy resolving.
Good question. I suppose we need to decide whether this file is more an API or more a UI. If we decide it's more important to maintain backward compatibility and treat it as an API, then we should not change it, and should provide the requested change another way.
Is your merge tool, or any software you know of, relying on this file's current format?
> The file only contains the information about the conflicting hunks, while
> you would really need to know about all changes to decide what to do. And in
> that respect the 'svn resolve' interactive resolver provides much more
> information.
Yes, I agree. Or, to talk about the same level of API, the set of conflict 'artifact' files (filename.mine/filename.yours/etc.) hold the full information needed.
> Before 1.7/1.8 we couldn't really restart the resolving after an operation,
> but this is now a much better tool to resolve conflicts than we can ever
> create in a plain text file.
[...]
>> It's true that the target file has a pristine version, but the pristine version isn't
>> *particularly* associated with the merge itself. We merge changes into the
>> working/actual version, and whether I commit local mods before starting the
>> merge makes no difference to the merging procedure or to the desired
>> result. The pristine version is just "the version before the version I'm merging
>> into". The fact that it was uncommitted makes it interesting from a WC
>> management perspective, but not particularly from a merge perspective.
>
> Essentially a merge is incorporating the changes between LEFT and RIGHT into
> the PRISTINE file.
We probably mean the same thing but are describing it in different ways. To me, the essence of a merge is it merges into the working file. That's the purpose, the logical idea, the intended functionality. When there are no local changes the pristine text is the same as the working text but that's just just incidental.
> But as we support merging into a locally changed file we just ignore the
> PRISTINE file and use the original WORKING as if it were the pristine file.
>
> That doesn't make the difference between PRISTINE and WORKING not
> interesting... It still contains the important notion of what is really an
> uncommitted local change (MINE) vs what was already there on checkout
> (BASE/PRISTINE).
Again, we understand the same thing in different ways.
> Sure there are even more interesting revisions if you know the full graph of
> changes, but the merge code in libsvn_wc just knows these 4
> files(PRISTINE,LEFT,RIGHT,WORKING), and produces a MERGED file from them:
> and a Boolean conflicted vs not conflicted.
Right, but the only reason the WC library needs to know about the pristine version is because that's such a big part of how the WC is architected. (There is no fully abstracted API for accessing the working/actual layer without talking about the pristines.) If you look at the merge logic in libsvn_client, it doesn't care.
> I'm pretty sure that you need more than a plain text file GUI to add the
> notion of more than 3 trees in a way that our users are able to understand
> it.
Very likely.
> But I'm thinking that calling either LEFT or PRISTINE 'original' in the
> MERGED file is not really going to help our typical user.
>
> And I wish my merge tool was smart enough to really understand 4 inputs...
> Showing LEFT as the older version is showing a lot of non-changes as if they
> were interesting for the merge, while they could have been filtered as
> unrelated with access to PRISTINE.
Really? I'm interested in that case. Can you point me to a concrete example?
The 'variance-adjusted patching' paper explains why this sort of problem can be solved by adding the youngest common ancestor into the algorithm, but I cannot yet see why taking account of the WC pristine version is any more useful than taking account of any other version (that is, a committed revision) between the YCA and target versions.
- Julian
RE: Three-way merge markers by default
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: maandag 7 april 2014 12:32
> To: Bert Huijben
> Cc: 'Daniel Shahaf'; 'Lev Serebryakov'; dev@subversion.apache.org
> Subject: Re: Three-way merge markers by default
>
> Bert Huijben wrote:
> > Daniel Shahaf wrote:
> >> What do people feel about applying this patch (from the FreeBSD port)
to
> >> trunk?
> [...]
> >> All it does is change the contents of conflict files in
--accept=postpone
> >> mode: it causes conflict markers to be rendered not as two sides
> >> <<<<<<
> >> mine
> >> ======
> >> theirs
> >> >>>>>>
> >> but as three sides
> >> <<<<<<
> >> mine
> >> ||||||
> >> original
> >> ======
> >> theirs
> >> >>>>>>
> >> .
> >> I find the latter style much easier to work with --- it is easier for
me
> >> to "Apply diff(ORIGINAL, THEIRS) to MINE" than to "Compare (MINE,
> THEIRS)
> >> and sort out which parts of the delta to keep and which to reverse".
> >>
> >> Does this break compatibility somehow? Scripts have the
> foo.c.{mine,theirs,
> >> merged} files to work with. As to users, they may already be familiar
with
> >> the three-way syntax since the interactive conflicts resolver generates
> it[1];
> >> if not, the book could explain the |||||| block like it explains <<<<<<
and
> >> >>>>>>.
>
> +1 to the suggestion. The current two-way info is pretty difficult to use
in any
> non-trivial case, and the 3-way suggestion is significantly more usable
and not
> much more difficult to understand. Furthermore it is what I expect to see
for
> a three-way merge, and, if I remember correctly, is what I already do see
in
> the interactive conflict resolver.
+1 on the idea of improving the merge information..
But how this eventually should end up in 'svn' depends really on the
question:
* Is the file containing the markers the place to learn about the different
sides or is it just a file created to allow easy resolving.
The file only contains the information about the conflicting hunks, while
you would really need to know about all changes to decide what to do. And in
that respect the 'svn resolve' interactive resolver provides much more
information.
Before 1.7/1.8 we couldn't really restart the resolving after an operation,
but this is now a much better tool to resolve conflicts than we can ever
create in a plain text file.
>
> > Note that while this gives complete information for update and switch,
for
> > merging there are actually 4 sides instead of 3...
> >
> > svn merge merges the difference between two files (left and right) into
a
> > third file (mine), which also happens to have an original version (its
> > pristine version).
> >
> > So theoretically in case of an update and switch the older versions are
the
> > same file(left=pristine).
>
> It's true that the target file has a pristine version, but the pristine
version isn't
> *particularly* associated with the merge itself. We merge changes into
the
> working/actual version, and whether I commit local mods before starting
the
> merge makes no difference to the merging procedure or to the desired
> result. The pristine version is just "the version before the version I'm
merging
> into". The fact that it was uncommitted makes it interesting from a WC
> management perspective, but not particularly from a merge perspective.
Essentially a merge is incorporating the changes between LEFT and RIGHT into
the PRISTINE file.
But as we support merging into a locally changed file we just ignore the
PRISTINE file and use the original WORKING as if it were the pristine file.
That doesn't make the difference between PRISTINE and WORKING not
interesting... It still contains the important notion of what is really an
uncommitted local change (MINE) vs what was already there on checkout
(BASE/PRISTINE).
Sure there are even more interesting revisions if you know the full graph of
changes, but the merge code in libsvn_wc just knows these 4
files(PRISTINE,LEFT,RIGHT,WORKING), and produces a MERGED file from them:
and a Boolean conflicted vs not conflicted.
I'm pretty sure that you need more than a plain text file GUI to add the
notion of more than 3 trees in a way that our users are able to understand
it.
But I'm thinking that calling either LEFT or PRISTINE 'original' in the
MERGED file is not really going to help our typical user.
And I wish my merge tool was smart enough to really understand 4 inputs...
Showing LEFT as the older version is showing a lot of non-changes as if they
were interesting for the merge, while they could have been filtered as
unrelated with access to PRISTINE.
Bert
>
> There's another version that's potentially interesting during a merge. In
a
> cherry-pick merge, the source-left version is (typically) not the youngest
> common ancestor (YCA) of the two lines of history. In that case, the YCA
is
> also an interesting version from a merging perspective. If I were asking
to see
> a fourth version, I might well want to see the YCA before wanting to see
the
> pristine WC version.
>
> . . . ----L----R
> /
> /
> ---Y---------------B
> \
> W
>
> (Y=YCA, L=source-left, R=source-right, B=WC-base, W=WC-working=target)
>
> A powerful UI could of course offer options to display all the potentially
> interesting versions, and differences between them. But here we're talking
> only about how to make the default text conflict markers in 'svn' show up,
> for those users not using a powerful (G)UI.
>
> I don't think it's sensible for a GUI to use the conflict markers we're
talking
> about here -- it should generate its own. (So I don't think this code
really
> belongs in libsvn_wc -- it's here for historical reasons but really it
should be in
> the 'svn' command-line client only.)
>
>
> > The problem is now: Are you now showing left or pristine as original?
> > Or what is "original"?
>
> That's not a problem -- it's exactly the same as in all typical graphical
three-
> way merge tools that users are already using, as well as the interactive
> conflict resolver. It's easy enough to document it as such, for
completeness.
>
>
> > The answer for update/switch is easy, but I don't think the per file
merge
> > code knows the difference between the operations. And I don't think our
> > users will really understand.
> > (I haven't found a merge tool that really uses all 4 versions except of
some
> > experimental code in our libraries)
>
> The "experimental code" you mention is for a kind of 4-way merge known as
> "variance-adjusted patching" as described by Karl Fogel in
<notes/variance-
> adjusted-patching.html>. The fourth version involved here is the YCA, not
> the pristine version.
>
> - Julian
Re: Three-way merge markers by default
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> Daniel Shahaf wrote:
>> What do people feel about applying this patch (from the FreeBSD port) to
>> trunk?
[...]
>> All it does is change the contents of conflict files in --accept=postpone
>> mode: it causes conflict markers to be rendered not as two sides
>> <<<<<<
>> mine
>> ======
>> theirs
>> >>>>>>
>> but as three sides
>> <<<<<<
>> mine
>> ||||||
>> original
>> ======
>> theirs
>> >>>>>>
>> .
>> I find the latter style much easier to work with --- it is easier for me
>> to "Apply diff(ORIGINAL, THEIRS) to MINE" than to "Compare (MINE, THEIRS)
>> and sort out which parts of the delta to keep and which to reverse".
>>
>> Does this break compatibility somehow? Scripts have the foo.c.{mine,theirs,
>> merged} files to work with. As to users, they may already be familiar with
>> the three-way syntax since the interactive conflicts resolver generates it[1];
>> if not, the book could explain the |||||| block like it explains <<<<<< and
>> >>>>>>.
+1 to the suggestion. The current two-way info is pretty difficult to use in any non-trivial case, and the 3-way suggestion is significantly more usable and not much more difficult to understand. Furthermore it is what I expect to see for a three-way merge, and, if I remember correctly, is what I already do see in the interactive conflict resolver.
> Note that while this gives complete information for update and switch, for
> merging there are actually 4 sides instead of 3...
>
> svn merge merges the difference between two files (left and right) into a
> third file (mine), which also happens to have an original version (its
> pristine version).
>
> So theoretically in case of an update and switch the older versions are the
> same file(left=pristine).
It's true that the target file has a pristine version, but the pristine version isn't *particularly* associated with the merge itself. We merge changes into the working/actual version, and whether I commit local mods before starting the merge makes no difference to the merging procedure or to the desired result. The pristine version is just "the version before the version I'm merging into". The fact that it was uncommitted makes it interesting from a WC management perspective, but not particularly from a merge perspective.
There's another version that's potentially interesting during a merge. In a cherry-pick merge, the source-left version is (typically) not the youngest common ancestor (YCA) of the two lines of history. In that case, the YCA is also an interesting version from a merging perspective. If I were asking to see a fourth version, I might well want to see the YCA before wanting to see the pristine WC version.
. . . ----L----R
/
/
---Y---------------B
\
W
(Y=YCA, L=source-left, R=source-right, B=WC-base, W=WC-working=target)
A powerful UI could of course offer options to display all the potentially interesting versions, and differences between them. But here we're talking only about how to make the default text conflict markers in 'svn' show up, for those users not using a powerful (G)UI.
I don't think it's sensible for a GUI to use the conflict markers we're talking about here -- it should generate its own. (So I don't think this code really belongs in libsvn_wc -- it's here for historical reasons but really it should be in the 'svn' command-line client only.)
> The problem is now: Are you now showing left or pristine as original?
> Or what is "original"?
That's not a problem -- it's exactly the same as in all typical graphical three-way merge tools that users are already using, as well as the interactive conflict resolver. It's easy enough to document it as such, for completeness.
> The answer for update/switch is easy, but I don't think the per file merge
> code knows the difference between the operations. And I don't think our
> users will really understand.
> (I haven't found a merge tool that really uses all 4 versions except of some
> experimental code in our libraries)
The "experimental code" you mention is for a kind of 4-way merge known as "variance-adjusted patching" as described by Karl Fogel in <notes/variance-adjusted-patching.html>. The fourth version involved here is the YCA, not the pristine version.
- Julian
RE: Three-way merge markers by default
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: maandag 7 april 2014 10:35
> To: dev@subversion.apache.org
> Cc: Lev Serebryakov
> Subject: Three-way merge markers by default
>
> What do people feel about applying this patch (from the FreeBSD port) to
> trunk?
>
> [[[
> http://svn.freebsd.org/ports/head/devel/subversion/files/extra-patch-
> 3way-conflict-markers (mod_dav_svn)
> https://svnweb.freebsd.org/ports/head/devel/subversion/files/extra-
> patch-3way-conflict-markers?view=log (viewvc)
> (and mentioned previously in context of custom keywords:
> <ht...@ted.stsp.name>)
> diff -ruN subversion/libsvn_wc/merge.c subversion/libsvn_wc/merge.c
> --- subversion/libsvn_wc/merge.c 2011-08-06 19:15:44.000000000 +0400
> +++ subversion/libsvn_wc/merge.c 2011-09-07 21:47:19.000000000 +0400
> @@ -413,7 +413,7 @@
> target_marker,
> right_marker,
> "=======", /* separator */
> -
svn_diff_conflict_display_modified_latest,
> +
svn_diff_conflict_display_modified_original_latest,
> pool));
> SVN_ERR(svn_stream_close(ostream));
>
> ]]]
>
> All it does is change the contents of conflict files in --accept=postpone
mode:
> it causes conflict markers to be rendered not as two sides
> <<<<<<
> mine
> ======
> theirs
> >>>>>>
> but as three sides
> <<<<<<
> mine
> ||||||
> original
> ======
> theirs
> >>>>>>
> .
> I find the latter style much easier to work with --- it is easier for me
to
> "Apply diff(ORIGINAL, THEIRS) to MINE" than to "Compare (MINE, THEIRS)
> and
> sort out which parts of the delta to keep and which to reverse".
>
> Does this break compatibility somehow? Scripts have the
foo.c.{mine,theirs,
> merged} files to work with. As to users, they may already be familiar
with
> the
> three-way syntax since the interactive conflicts resolver generates it[1];
if
> not, the book could explain the |||||| block like it explains <<<<<< and
> >>>>>>.
Note that while this gives complete information for update and switch, for
merging there are actually 4 sides instead of 3...
svn merge merges the difference between two files (left and right) into a
third file (mine), which also happens to have an original version (its
pristine version).
So theoretically in case of an update and switch the older versions are the
same file(left=pristine).
The problem is now: Are you now showing left or pristine as original?
Or what is "original"?
The answer for update/switch is easy, but I don't think the per file merge
code knows the difference between the operations. And I don't think our
users will really understand.
(I haven't found a merge tool that really uses all 4 versions except of some
experimental code in our libraries)
Bert
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> An error in my testsuite change. This passes all the tests with the
>> 3-way output:
>>
>> Index: subversion/libsvn_wc/merge.c
>> ===================================================================
>> --- subversion/libsvn_wc/merge.c (revision 1591407)
>> +++ subversion/libsvn_wc/merge.c (working copy)
>> @@ -422,7 +422,7 @@ do_text_merge(svn_boolean_t *contains_conflicts,
>> target_marker,
>> right_marker,
>> "=======", /* separator */
>> - svn_diff_conflict_display_modified_latest,
>> + svn_diff_conflict_display_modified_original_latest,
>> pool));
>> SVN_ERR(svn_stream_close(ostream));
>>
>
> Need to fix the Ruby tests as well:
>
> Index: swig/ruby/test/test_client.rb
> ===================================================================
> --- swig/ruby/test/test_client.rb (revision 1591407)
> +++ swig/ruby/test/test_client.rb (working copy)
> @@ -2498,7 +2498,7 @@ class SvnClientTest < Test::Unit::TestCase
> assert_not_nil(info)
> assert_equal(3, info.revision)
>
> - assert_equal("<<<<<<< .mine\nafter\n=======\nbefore\n>>>>>>> .r2\n",
> + assert_equal("<<<<<<< .mine\nafter\n||||||| .r1\n=======\nbefore\n>>>>>>> .r2\n",
> File.read(path))
> end
> end
So this works and I've enhanced tools/diff/diff3 so that it can be used
as an external diff. I like this change so I have committed it. If
anyone objects I am open to reverting it.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> An error in my testsuite change. This passes all the tests with the
> 3-way output:
>
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 1591407)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -422,7 +422,7 @@ do_text_merge(svn_boolean_t *contains_conflicts,
> target_marker,
> right_marker,
> "=======", /* separator */
> - svn_diff_conflict_display_modified_latest,
> + svn_diff_conflict_display_modified_original_latest,
> pool));
> SVN_ERR(svn_stream_close(ostream));
>
Need to fix the Ruby tests as well:
Index: swig/ruby/test/test_client.rb
===================================================================
--- swig/ruby/test/test_client.rb (revision 1591407)
+++ swig/ruby/test/test_client.rb (working copy)
@@ -2498,7 +2498,7 @@ class SvnClientTest < Test::Unit::TestCase
assert_not_nil(info)
assert_equal(3, info.revision)
- assert_equal("<<<<<<< .mine\nafter\n=======\nbefore\n>>>>>>> .r2\n",
+ assert_equal("<<<<<<< .mine\nafter\n||||||| .r1\n=======\nbefore\n>>>>>>> .r2\n",
File.read(path))
end
end
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>>
>>> --- subversion/libsvn_wc/merge.c 2011-08-06 19:15:44.000000000 +0400
>>> +++ subversion/libsvn_wc/merge.c 2011-09-07 21:47:19.000000000 +0400
>>> @@ -413,7 +413,7 @@
>>> target_marker,
>>> right_marker,
>>> "=======", /* separator */
>>> - svn_diff_conflict_display_modified_latest,
>>> + svn_diff_conflict_display_modified_original_latest,
>>> pool));
>>> SVN_ERR(svn_stream_close(ostream));
>>>
>>
>> We will also need to fix our testsuite to expect the new output.
>
> Here's a patch that makes most of the testsuite PASS with the the new
> output. I'm not sure it's correct as I cannot get merge_tests.py 34 to
> PASS, at one point it seems to require both .merge-left.r2 and
> .merge-left.r4.
An error in my testsuite change. This passes all the tests with the
3-way output:
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 1591407)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -422,7 +422,7 @@ do_text_merge(svn_boolean_t *contains_conflicts,
target_marker,
right_marker,
"=======", /* separator */
- svn_diff_conflict_display_modified_latest,
+ svn_diff_conflict_display_modified_original_latest,
pool));
SVN_ERR(svn_stream_close(ostream));
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 1591407)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -682,6 +682,7 @@ def basic_conflict(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -690,6 +691,7 @@ def basic_conflict(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -2200,6 +2202,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'lambda'.",
"<<<<<<< .mine",
"Conflicting appended text for lambda",
+ "||||||| .r1",
"=======",
"Original appended text for lambda",
">>>>>>> .r2",
@@ -2208,6 +2211,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -2216,6 +2220,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -2224,6 +2229,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'tau'.",
"<<<<<<< .mine",
"Conflicting appended text for tau",
+ "||||||| .r1",
"=======",
"Original appended text for tau",
">>>>>>> .r2",
@@ -2232,6 +2238,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'omega'.",
"<<<<<<< .mine",
"Conflicting appended text for omega",
+ "||||||| .r1",
"=======",
"Original appended text for omega",
">>>>>>> .r2",
@@ -2334,6 +2341,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'omega'.",
"<<<<<<< .mine",
"Conflicting appended text for omega",
+ "||||||| .r1",
"=======",
"Original appended text for omega",
">>>>>>> .r2",
Index: subversion/tests/cmdline/checkout_tests.py
===================================================================
--- subversion/tests/cmdline/checkout_tests.py (revision 1591407)
+++ subversion/tests/cmdline/checkout_tests.py (working copy)
@@ -858,6 +858,7 @@ def co_with_obstructing_local_adds(sbox):
'A/D/H/I/K/xi' : Item("This is file 'xi'\n"),
'A/D/H/I/K/eta' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'eta'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'eta'",
">>>>>>> .r2",
@@ -865,6 +866,7 @@ def co_with_obstructing_local_adds(sbox):
'A/D/H/I/L' : Item(),
'A/D/kappa' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'kappa'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'kappa'",
">>>>>>> .r2",
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 1591407)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -247,7 +247,7 @@ def textual_merges_galore(sbox):
inject_conflict_into_expected_state('A/D/G/tau', expected_disk,
expected_status, other_tau_text, tau_text,
- 3)
+ 1, 3)
expected_skip = wc.State('', { })
@@ -336,7 +336,7 @@ def textual_merges_galore(sbox):
})
inject_conflict_into_expected_state('tau', expected_disk, expected_status,
- other_tau_text, tau_text, 3)
+ other_tau_text, tau_text, 1, 3)
# Do the merge, but check svn:mergeinfo props separately since
# run_and_verify_merge would attempt to proplist tau's conflict
@@ -3000,7 +3000,7 @@ def cherry_pick_text_conflict(sbox):
})
expected_disk = wc.State('', {
'mu' : Item("This is the file 'mu'.\n"
- + make_conflict_marker_text("r3\n" * 3, "r4\n" * 3, 4)),
+ + make_conflict_marker_text("r3\n" * 3, "r4\n" * 3, 3, 4)),
'B' : Item(),
'B/lambda' : Item("This is the file 'lambda'.\n"),
'B/E' : Item(),
@@ -3515,6 +3515,7 @@ def merge_conflict_markers_matching_eol(sbox):
'A/mu' : Item(contents= "This is the file 'mu'." + eolchar +
"<<<<<<< .working" + eolchar +
"Conflicting appended text for mu" + eolchar +
+ "||||||| .merge-left.r" + str(cur_rev - 1) + eolchar +
"=======" + eolchar +
"Original appended text for mu" + eolchar +
">>>>>>> .merge-right.r" + str(cur_rev) + eolchar),
@@ -14836,6 +14837,8 @@ def merge_automatic_conflict_resolution(sbox):
})
expected_disk.tweak('D/H/psi', contents="<<<<<<< .working\n"
"BASE.\n"
+ "||||||| .merge-left.r2\n"
+ "This is the file 'psi'.\n"
"=======\n"
"New content>>>>>>> .merge-right.r3\n")
expected_status.tweak('D/H/psi', status='C ')
@@ -18729,6 +18732,8 @@ def conflict_naming(sbox):
'file.txt.r2' : Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .mine\n" \
"This is conflicting content\n" \
+ "||||||| .r3\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .r2\n"),
@@ -18760,6 +18765,8 @@ def conflict_naming(sbox):
'file.txt.r2.txt' : Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .mine.txt\n" \
"This is conflicting content\n" \
+ "||||||| .r3.txt\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .r2.txt\n"),
@@ -18789,6 +18796,8 @@ def conflict_naming(sbox):
'file.txt.merge-right.r2': Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .working\n" \
"This is conflicting content\n" \
+ "||||||| .merge-left.r3\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .merge-right.r2\n"),
@@ -18814,6 +18823,8 @@ def conflict_naming(sbox):
'file.txt.merge-right.r2.txt': Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .working.txt\n" \
"This is conflicting content\n" \
+ "||||||| .merge-left.r3.txt\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .merge-right.r2.txt\n"),
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 1591407)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -2145,7 +2145,7 @@ def inject_conflict_into_wc(sbox, state_path, file
inject_conflict_into_expected_state(state_path,
expected_disk, expected_status,
conflicting_contents, contents,
- merged_rev)
+ prev_rev, merged_rev)
exit_code, output, errput = main.run_svn(None, "up", "-r", str(merged_rev),
file_path)
if expected_status:
@@ -2153,7 +2153,8 @@ def inject_conflict_into_wc(sbox, state_path, file
def inject_conflict_into_expected_state(state_path,
expected_disk, expected_status,
- wc_text, merged_text, merged_rev):
+ wc_text, merged_text, prev_rev,
+ merged_rev):
"""Update the EXPECTED_DISK and EXPECTED_STATUS trees for the
conflict at STATE_PATH (ignored if None). WC_TEXT, MERGED_TEXT, and
MERGED_REV are used to determine the contents of the conflict (the
@@ -2160,7 +2161,7 @@ def inject_conflict_into_expected_state(state_path
text parameters should be newline-terminated)."""
if expected_disk:
conflict_marker = make_conflict_marker_text(wc_text, merged_text,
- merged_rev)
+ prev_rev, merged_rev)
existing_text = expected_disk.desc[state_path].contents or ""
expected_disk.tweak(state_path, contents=existing_text + conflict_marker)
@@ -2167,12 +2168,13 @@ def inject_conflict_into_expected_state(state_path
if expected_status:
expected_status.tweak(state_path, status='C ')
-def make_conflict_marker_text(wc_text, merged_text, merged_rev):
+def make_conflict_marker_text(wc_text, merged_text, prev_rev, merged_rev):
"""Return the conflict marker text described by WC_TEXT (the current
text in the working copy, MERGED_TEXT (the conflicting text merged
in), and MERGED_REV (the revision from whence the conflicting text
came)."""
- return "<<<<<<< .working\n" + wc_text + "=======\n" + \
+ return "<<<<<<< .working\n" + wc_text + \
+ "||||||| .merge-left.r" + str(prev_rev) + "\n=======\n" + \
merged_text + ">>>>>>> .merge-right.r" + str(merged_rev) + "\n"
Index: subversion/tests/cmdline/trans_tests.py
===================================================================
--- subversion/tests/cmdline/trans_tests.py (revision 1591407)
+++ subversion/tests/cmdline/trans_tests.py (working copy)
@@ -515,6 +515,17 @@ def update_modified_with_translation(sbox):
"8",
"9",
"10",
+ "||||||| .r3",
+ "1",
+ "2",
+ "3",
+ "4",
+ "4.5",
+ "5",
+ "6",
+ "7",
+ "8",
+ "9",
"=======",
"This is the file 'rho'.",
">>>>>>> .r1",
Index: subversion/tests/cmdline/update_tests.py
===================================================================
--- subversion/tests/cmdline/update_tests.py (revision 1591407)
+++ subversion/tests/cmdline/update_tests.py (working copy)
@@ -658,6 +658,7 @@ def update_to_resolve_text_conflicts(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -666,6 +667,7 @@ def update_to_resolve_text_conflicts(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -1798,6 +1800,7 @@ def conflict_markers_matching_eol(sbox):
'A/mu' : Item(contents= "This is the file 'mu'." + eolchar +
"<<<<<<< .mine" + eolchar +
"Conflicting appended text for mu" + eolchar +
+ "||||||| .r" + str(cur_rev - 1) + eolchar +
"=======" + eolchar +
"Original appended text for mu" + eolchar +
">>>>>>> .r" + str(cur_rev) + eolchar),
@@ -2737,6 +2740,7 @@ def update_with_obstructing_additions(sbox):
'A/D/H/I/J' : Item(props={'propname1' : 'propval-WC'}),
'A/D/H/I/J/eta' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'eta'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'eta'",
">>>>>>> .r2",
@@ -2746,6 +2750,7 @@ def update_with_obstructing_additions(sbox):
'A/D/H/I/L' : Item(),
'A/D/kappa' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'kappa'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'kappa'",
">>>>>>> .r2",
@@ -2753,6 +2758,7 @@ def update_with_obstructing_additions(sbox):
props={'propname1' : 'propval-WC'}),
'A/D/epsilon' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'epsilon'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'epsilon'",
">>>>>>> .r2",
@@ -3033,6 +3039,7 @@ def update_conflicted(sbox):
contents="\n".join(["This is the file 'iota'.",
"<<<<<<< .mine",
"Conflicting appended text for iota",
+ "||||||| .r1",
"=======",
"Original appended text for iota",
">>>>>>> .r2",
@@ -3041,6 +3048,7 @@ def update_conflicted(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -3876,6 +3884,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('iota', contents=("This is the file 'iota'.\n"
'<<<<<<< .mine\n'
'My appended text for iota\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for iota\n'
'>>>>>>> .r2\n'))
@@ -3882,6 +3891,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/B/lambda', contents=("This is the file 'lambda'.\n"
'<<<<<<< .mine\n'
'My appended text for lambda\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for lambda\n'
'>>>>>>> .r2\n'))
@@ -3893,6 +3903,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/D/G/pi', contents=("This is the file 'pi'.\n"
'<<<<<<< .mine\n'
'My appended text for pi\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for pi\n'
'>>>>>>> .r2\n'
@@ -3900,6 +3911,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/D/G/rho', contents=("This is the file 'rho'.\n"
'<<<<<<< .mine\n'
'My appended text for rho\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for rho\n'
'>>>>>>> .r2\n'
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
>> --- subversion/libsvn_wc/merge.c 2011-08-06 19:15:44.000000000 +0400
>> +++ subversion/libsvn_wc/merge.c 2011-09-07 21:47:19.000000000 +0400
>> @@ -413,7 +413,7 @@
>> target_marker,
>> right_marker,
>> "=======", /* separator */
>> - svn_diff_conflict_display_modified_latest,
>> + svn_diff_conflict_display_modified_original_latest,
>> pool));
>> SVN_ERR(svn_stream_close(ostream));
>>
>
> We will also need to fix our testsuite to expect the new output.
Here's a patch that makes most of the testsuite PASS with the the new
output. I'm not sure it's correct as I cannot get merge_tests.py 34 to
PASS, at one point it seems to require both .merge-left.r2 and
.merge-left.r4.
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 1591407)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -422,7 +422,7 @@ do_text_merge(svn_boolean_t *contains_conflicts,
target_marker,
right_marker,
"=======", /* separator */
- svn_diff_conflict_display_modified_latest,
+ svn_diff_conflict_display_modified_original_latest,
pool));
SVN_ERR(svn_stream_close(ostream));
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 1591407)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -682,6 +682,7 @@ def basic_conflict(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -690,6 +691,7 @@ def basic_conflict(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -2200,6 +2202,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'lambda'.",
"<<<<<<< .mine",
"Conflicting appended text for lambda",
+ "||||||| .r1",
"=======",
"Original appended text for lambda",
">>>>>>> .r2",
@@ -2208,6 +2211,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -2216,6 +2220,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -2224,6 +2229,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'tau'.",
"<<<<<<< .mine",
"Conflicting appended text for tau",
+ "||||||| .r1",
"=======",
"Original appended text for tau",
">>>>>>> .r2",
@@ -2232,6 +2238,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'omega'.",
"<<<<<<< .mine",
"Conflicting appended text for omega",
+ "||||||| .r1",
"=======",
"Original appended text for omega",
">>>>>>> .r2",
@@ -2334,6 +2341,7 @@ def automatic_conflict_resolution(sbox):
contents="\n".join(["This is the file 'omega'.",
"<<<<<<< .mine",
"Conflicting appended text for omega",
+ "||||||| .r1",
"=======",
"Original appended text for omega",
">>>>>>> .r2",
Index: subversion/tests/cmdline/checkout_tests.py
===================================================================
--- subversion/tests/cmdline/checkout_tests.py (revision 1591407)
+++ subversion/tests/cmdline/checkout_tests.py (working copy)
@@ -858,6 +858,7 @@ def co_with_obstructing_local_adds(sbox):
'A/D/H/I/K/xi' : Item("This is file 'xi'\n"),
'A/D/H/I/K/eta' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'eta'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'eta'",
">>>>>>> .r2",
@@ -865,6 +866,7 @@ def co_with_obstructing_local_adds(sbox):
'A/D/H/I/L' : Item(),
'A/D/kappa' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'kappa'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'kappa'",
">>>>>>> .r2",
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 1591407)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -247,7 +247,7 @@ def textual_merges_galore(sbox):
inject_conflict_into_expected_state('A/D/G/tau', expected_disk,
expected_status, other_tau_text, tau_text,
- 3)
+ 1, 3)
expected_skip = wc.State('', { })
@@ -336,7 +336,7 @@ def textual_merges_galore(sbox):
})
inject_conflict_into_expected_state('tau', expected_disk, expected_status,
- other_tau_text, tau_text, 3)
+ other_tau_text, tau_text, 1, 3)
# Do the merge, but check svn:mergeinfo props separately since
# run_and_verify_merge would attempt to proplist tau's conflict
@@ -3000,7 +3000,7 @@ def cherry_pick_text_conflict(sbox):
})
expected_disk = wc.State('', {
'mu' : Item("This is the file 'mu'.\n"
- + make_conflict_marker_text("r3\n" * 3, "r4\n" * 3, 4)),
+ + make_conflict_marker_text("r3\n" * 3, "r4\n" * 3, 3, 4)),
'B' : Item(),
'B/lambda' : Item("This is the file 'lambda'.\n"),
'B/E' : Item(),
@@ -3515,6 +3515,7 @@ def merge_conflict_markers_matching_eol(sbox):
'A/mu' : Item(contents= "This is the file 'mu'." + eolchar +
"<<<<<<< .working" + eolchar +
"Conflicting appended text for mu" + eolchar +
+ "||||||| .merge-left.r2" + eolchar +
"=======" + eolchar +
"Original appended text for mu" + eolchar +
">>>>>>> .merge-right.r" + str(cur_rev) + eolchar),
@@ -14836,6 +14837,8 @@ def merge_automatic_conflict_resolution(sbox):
})
expected_disk.tweak('D/H/psi', contents="<<<<<<< .working\n"
"BASE.\n"
+ "||||||| .merge-left.r2\n"
+ "This is the file 'psi'.\n"
"=======\n"
"New content>>>>>>> .merge-right.r3\n")
expected_status.tweak('D/H/psi', status='C ')
@@ -18729,6 +18732,8 @@ def conflict_naming(sbox):
'file.txt.r2' : Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .mine\n" \
"This is conflicting content\n" \
+ "||||||| .r3\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .r2\n"),
@@ -18760,6 +18765,8 @@ def conflict_naming(sbox):
'file.txt.r2.txt' : Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .mine.txt\n" \
"This is conflicting content\n" \
+ "||||||| .r3.txt\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .r2.txt\n"),
@@ -18789,6 +18796,8 @@ def conflict_naming(sbox):
'file.txt.merge-right.r2': Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .working\n" \
"This is conflicting content\n" \
+ "||||||| .merge-left.r3\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .merge-right.r2\n"),
@@ -18814,6 +18823,8 @@ def conflict_naming(sbox):
'file.txt.merge-right.r2.txt': Item(contents="This is the initial content\n"),
'file.txt' : Item(contents="<<<<<<< .working.txt\n" \
"This is conflicting content\n" \
+ "||||||| .merge-left.r3.txt\n" \
+ "This is the new content\n" \
"=======\n" \
"This is the initial content\n" \
">>>>>>> .merge-right.r2.txt\n"),
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 1591407)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -2145,7 +2145,7 @@ def inject_conflict_into_wc(sbox, state_path, file
inject_conflict_into_expected_state(state_path,
expected_disk, expected_status,
conflicting_contents, contents,
- merged_rev)
+ prev_rev, merged_rev)
exit_code, output, errput = main.run_svn(None, "up", "-r", str(merged_rev),
file_path)
if expected_status:
@@ -2153,7 +2153,8 @@ def inject_conflict_into_wc(sbox, state_path, file
def inject_conflict_into_expected_state(state_path,
expected_disk, expected_status,
- wc_text, merged_text, merged_rev):
+ wc_text, merged_text, prev_rev,
+ merged_rev):
"""Update the EXPECTED_DISK and EXPECTED_STATUS trees for the
conflict at STATE_PATH (ignored if None). WC_TEXT, MERGED_TEXT, and
MERGED_REV are used to determine the contents of the conflict (the
@@ -2160,7 +2161,7 @@ def inject_conflict_into_expected_state(state_path
text parameters should be newline-terminated)."""
if expected_disk:
conflict_marker = make_conflict_marker_text(wc_text, merged_text,
- merged_rev)
+ prev_rev, merged_rev)
existing_text = expected_disk.desc[state_path].contents or ""
expected_disk.tweak(state_path, contents=existing_text + conflict_marker)
@@ -2167,12 +2168,13 @@ def inject_conflict_into_expected_state(state_path
if expected_status:
expected_status.tweak(state_path, status='C ')
-def make_conflict_marker_text(wc_text, merged_text, merged_rev):
+def make_conflict_marker_text(wc_text, merged_text, prev_rev, merged_rev):
"""Return the conflict marker text described by WC_TEXT (the current
text in the working copy, MERGED_TEXT (the conflicting text merged
in), and MERGED_REV (the revision from whence the conflicting text
came)."""
- return "<<<<<<< .working\n" + wc_text + "=======\n" + \
+ return "<<<<<<< .working\n" + wc_text + \
+ "||||||| .merge-left.r" + str(prev_rev) + "\n=======\n" + \
merged_text + ">>>>>>> .merge-right.r" + str(merged_rev) + "\n"
Index: subversion/tests/cmdline/trans_tests.py
===================================================================
--- subversion/tests/cmdline/trans_tests.py (revision 1591407)
+++ subversion/tests/cmdline/trans_tests.py (working copy)
@@ -515,6 +515,17 @@ def update_modified_with_translation(sbox):
"8",
"9",
"10",
+ "||||||| .r3",
+ "1",
+ "2",
+ "3",
+ "4",
+ "4.5",
+ "5",
+ "6",
+ "7",
+ "8",
+ "9",
"=======",
"This is the file 'rho'.",
">>>>>>> .r1",
Index: subversion/tests/cmdline/update_tests.py
===================================================================
--- subversion/tests/cmdline/update_tests.py (revision 1591407)
+++ subversion/tests/cmdline/update_tests.py (working copy)
@@ -658,6 +658,7 @@ def update_to_resolve_text_conflicts(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -666,6 +667,7 @@ def update_to_resolve_text_conflicts(sbox):
contents="\n".join(["This is the file 'rho'.",
"<<<<<<< .mine",
"Conflicting appended text for rho",
+ "||||||| .r1",
"=======",
"Original appended text for rho",
">>>>>>> .r2",
@@ -1798,6 +1800,7 @@ def conflict_markers_matching_eol(sbox):
'A/mu' : Item(contents= "This is the file 'mu'." + eolchar +
"<<<<<<< .mine" + eolchar +
"Conflicting appended text for mu" + eolchar +
+ "||||||| .r2" + eolchar +
"=======" + eolchar +
"Original appended text for mu" + eolchar +
">>>>>>> .r" + str(cur_rev) + eolchar),
@@ -2737,6 +2740,7 @@ def update_with_obstructing_additions(sbox):
'A/D/H/I/J' : Item(props={'propname1' : 'propval-WC'}),
'A/D/H/I/J/eta' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'eta'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'eta'",
">>>>>>> .r2",
@@ -2746,6 +2750,7 @@ def update_with_obstructing_additions(sbox):
'A/D/H/I/L' : Item(),
'A/D/kappa' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'kappa'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'kappa'",
">>>>>>> .r2",
@@ -2753,6 +2758,7 @@ def update_with_obstructing_additions(sbox):
props={'propname1' : 'propval-WC'}),
'A/D/epsilon' : Item("\n".join(["<<<<<<< .mine",
"This is WC file 'epsilon'",
+ "||||||| .r0",
"=======",
"This is REPOS file 'epsilon'",
">>>>>>> .r2",
@@ -3033,6 +3039,7 @@ def update_conflicted(sbox):
contents="\n".join(["This is the file 'iota'.",
"<<<<<<< .mine",
"Conflicting appended text for iota",
+ "||||||| .r1",
"=======",
"Original appended text for iota",
">>>>>>> .r2",
@@ -3041,6 +3048,7 @@ def update_conflicted(sbox):
contents="\n".join(["This is the file 'mu'.",
"<<<<<<< .mine",
"Conflicting appended text for mu",
+ "||||||| .r1",
"=======",
"Original appended text for mu",
">>>>>>> .r2",
@@ -3876,6 +3884,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('iota', contents=("This is the file 'iota'.\n"
'<<<<<<< .mine\n'
'My appended text for iota\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for iota\n'
'>>>>>>> .r2\n'))
@@ -3882,6 +3891,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/B/lambda', contents=("This is the file 'lambda'.\n"
'<<<<<<< .mine\n'
'My appended text for lambda\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for lambda\n'
'>>>>>>> .r2\n'))
@@ -3893,6 +3903,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/D/G/pi', contents=("This is the file 'pi'.\n"
'<<<<<<< .mine\n'
'My appended text for pi\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for pi\n'
'>>>>>>> .r2\n'
@@ -3900,6 +3911,7 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('A/D/G/rho', contents=("This is the file 'rho'.\n"
'<<<<<<< .mine\n'
'My appended text for rho\n'
+ '||||||| .r1\n'
'=======\n'
'Their appended text for rho\n'
'>>>>>>> .r2\n'
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Three-way merge markers by default
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, May 01, 2014 at 22:52:06 +0000:
> Philip Martin wrote on Thu, May 01, 2014 at 19:04:04 +0100:
> > Philip Martin <ph...@wandisco.com> writes:
> >
> > > svn_diff_conflict_display_modified_latest is similar to the output of
> > > GNU diff3 which is probably why it was chosen. Changing it might cause
> > > problems for tools that parse the output, but one option for anyone
> > > affected would be to use GNU diff3 with --diff3-cmd.
> >
> > We have our own standalone diff3 in tools/diff/diff3. It currently
> > hard-codes the same 2-way output that is hard-coded in libsvn_wc but we
> > could make it a command line option. Users who want precise control
> > over the ouput could configure it as an external diff3 command, although
> > we might want to rename it svndiff3.
> >
>
> "svndiff3" specifically might not be a good name because of the
> potential for confusion with svndiff0 and svndiff1 (the libsvn_delta
> binary delta formats). "svn-diff3" looks like a subcommand "diff3" of
> "svn" (following a pattern used by git, hg, pkgng, etc). Perhaps
> 'svn_diff3'?
'make install-tools' puts tools/diff/diff3 in $(prefix)/bin/svn-tools/diff3,
along with all other tools. Why would we need to rename it, then?
Actually, at least on unix, we could implicitly provide "prefix" as a
replaceable in the config file, so that people could write:
% cat ~/.subversion/config
[helpers]
diff3-cmd = %(prefix)s/svn-tools/diff3
and it would work regardless of where the 'svn' binary happened to be
installed. (For example, this could be useful if the homedir is shared
between multiple boxes that don't all have svn installed into the same
prefix.)
Daniel
Re: Three-way merge markers by default
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Thu, May 01, 2014 at 19:04:04 +0100:
> Philip Martin <ph...@wandisco.com> writes:
>
> > svn_diff_conflict_display_modified_latest is similar to the output of
> > GNU diff3 which is probably why it was chosen. Changing it might cause
> > problems for tools that parse the output, but one option for anyone
> > affected would be to use GNU diff3 with --diff3-cmd.
>
> We have our own standalone diff3 in tools/diff/diff3. It currently
> hard-codes the same 2-way output that is hard-coded in libsvn_wc but we
> could make it a command line option. Users who want precise control
> over the ouput could configure it as an external diff3 command, although
> we might want to rename it svndiff3.
>
"svndiff3" specifically might not be a good name because of the
potential for confusion with svndiff0 and svndiff1 (the libsvn_delta
binary delta formats). "svn-diff3" looks like a subcommand "diff3" of
"svn" (following a pattern used by git, hg, pkgng, etc). Perhaps
'svn_diff3'?
> What is the best way to produce a command line option for the enum
> below?
>
> typedef enum svn_diff_conflict_display_style_t
> {
> /** Display modified and latest, with conflict markers. */
> svn_diff_conflict_display_modified_latest,
..
> } svn_diff_conflict_display_style_t;
>
An implementation of this was committed in r1591750.
> --
> Philip
Thanks for picking this thread up.
Daniel
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> svn_diff_conflict_display_modified_latest is similar to the output of
> GNU diff3 which is probably why it was chosen. Changing it might cause
> problems for tools that parse the output, but one option for anyone
> affected would be to use GNU diff3 with --diff3-cmd.
We have our own standalone diff3 in tools/diff/diff3. It currently
hard-codes the same 2-way output that is hard-coded in libsvn_wc but we
could make it a command line option. Users who want precise control
over the ouput could configure it as an external diff3 command, although
we might want to rename it svndiff3.
What is the best way to produce a command line option for the enum
below?
typedef enum svn_diff_conflict_display_style_t
{
/** Display modified and latest, with conflict markers. */
svn_diff_conflict_display_modified_latest,
/** Like svn_diff_conflict_display_modified_latest, but with an
extra effort to identify common sequences between modified and
latest. */
svn_diff_conflict_display_resolved_modified_latest,
/** Display modified, original, and latest, with conflict
markers. */
svn_diff_conflict_display_modified_original_latest,
/** Just display modified, with no markers. */
svn_diff_conflict_display_modified,
/** Just display latest, with no markers. */
svn_diff_conflict_display_latest,
/** Like svn_diff_conflict_display_modified_original_latest, but
*only* showing conflicts. */
svn_diff_conflict_display_only_conflicts
} svn_diff_conflict_display_style_t;
--
Philip
Re: Three-way merge markers by default
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> --- subversion/libsvn_wc/merge.c 2011-08-06 19:15:44.000000000 +0400
> +++ subversion/libsvn_wc/merge.c 2011-09-07 21:47:19.000000000 +0400
> @@ -413,7 +413,7 @@
> target_marker,
> right_marker,
> "=======", /* separator */
> - svn_diff_conflict_display_modified_latest,
> + svn_diff_conflict_display_modified_original_latest,
> pool));
> SVN_ERR(svn_stream_close(ostream));
>
Somebody asked me about this exact change at one of our Subversion Live
events, as I recall they were rebuilding Subversion to get this
feature. I did look at making it configureable but I wasn't sure
whether configuration at such a low level was desirable. It's also
a tedious API change to get the configuration passed all the way down,
although we might be able to use (abuse?) the merge_options hash.
svn_diff_conflict_display_modified_latest is similar to the output of
GNU diff3 which is probably why it was chosen. Changing it might cause
problems for tools that parse the output, but one option for anyone
affected would be to use GNU diff3 with --diff3-cmd. We will also need
to fix our testsuite to expect the new output.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*