Re: Merging bug (wrong conflicts)

[Derek R. Price]

Jacob Burckhardt wrote:

> Derek R. Price writes:
> > Could you send me your 8 merge errors as sanity.sh test cases?
>
> A patch to sanity.sh is below, but first here is some description of
> what I did.

Thanks!  This looks good and seems to prove Karl Tomlinson's patch.  I've checked it 
in along with Karl's fix.  If
this does everything I think it does, it may almost be time for a new release.  Those 
merging bugs have caused me,
and I gather others, a lot of headaches in the past.

Derek

--
Derek Price  CVS Solutions Architect ( http://CVSHome.org )
mailto:[EMAIL PROTECTED] CollabNet ( http://collab.net )
--
I will return the seeing-eye dog.
I will return the seeing-eye dog.
I will return the seeing-eye dog...

  - Bart Simpson on chalkboard, _The Simpsons_




___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Derek R. Price writes:
> Could you send me your 8 merge errors as sanity.sh test cases?

A patch to sanity.sh is below, but first here is some description of
what I did.

I spent about 10 hours trying to find other test cases to test and
trying to reduce my large test cases.  I gave up on trying to reduce
the large ones, but I did find some interesting new small ones.  They
may not be all that unique but at least they are small, so I decided
to include them in sanity.sh.  If anyone else wants to try to get
better test cases, I will be glad to help add them to sanity.sh.  But
for now, I am satisfied with my changes below.  They catch bugs in the
unpatched cvs, so for regression testing purposes they are good.

The existing diffmerge1 test case was pretty easy to follow.  It used
a CVS module with one file.  I wanted to add additional files which
would be additional test cases.  Then the existing single update
command would merge all of these files that are in a single module
instead of just merging the one file which used to be there.  It takes
so much room to write the code to create these extra files, that I
decided to move this code out of the way into separate shell functions,
so that the large amount of code would not distract from the
high level flow of this test.  I think this helps make it more top
down, and easier to follow.  I was not sure where I should put these
large functions which create the input files.  I picked a place close
to the beginning of "The big loop".  If you prefer a different place,
go ahead and move it or you can ask me to move it.

Note that 3 of the test cases worked on the unpatched cvs.  One of
these cases was already fixed a few years ago, and I think the others
have always worked with cvs.  Even though cvs always handled these, I
included them anyway, because Peter Miller's fmerge utility fails on
one of these, and so therefore it may be a difficult case, which makes
it good for cvs regression testing.  The other case which has always
worked, was recommend by Karl Tomlinson.  It has MINE and YOURS be
the same file.  The three cases which the unpatched cvs passes are:

testcase01
testcase07
testcase09

Therefore when you run the test on the unpatched file, you will see
diff output for 7 of the test cases but not for the 3 mentioned above.
Note that I use the directory_cmp function to do the compare, but if
it finds a difference, it does not say what file was different, so if
you want to know what files are different you may temporarily add a
diff command by uncommenting my example diff command.  I did not
want to leave it uncommented because it uses GNU diff and I think some
people might use sanity.sh who do not have GNU diff.

As long as my employer keeps me in charge of cvs issues for our
project, I intend to continue to monitor the info-cvs and bug-cvs
mailing lists and if I see anyone ask about something which might be a
merge bug, I will try to reproduce their problem and determine if
Karl's patch fixes it.

If there is anything else I can help with related to merge bugs,
please let me know.

*** /reliant/bjacob/original/cvs-1.11/src/sanity.sh Wed Aug 23 13:02:22 2000
--- ./sanity.sh Mon Mar 19 18:00:21 2001
***
*** 715,718 
--- 715,1453 
  }
  
+ # The following 4 functions are used by the diffmerge1 test case:
+ diffmerge_create_older_files() {
+ # Note, that to examine only one test case you must look in
+ # 4 functions (this function, and the 3 that follow it).
+ # For example to examine test case 1, look at what each
+ # function writes to the filename "testcase01".  To examine the
+ # next case, look at what each function writes to the file
+ # named testcase02.
+ 
+ # The text of the file is inlined here because `echo' loses
+ # newlines, and I don't know how portable the -e flag is.
+ # This test case was supplied by Noah Friedman:
+ echo "// Button.java
+ 
+ package random.application;
+ 
+ import random.util.star;
+ 
+ public class Button
+ {
+   /star Instantiates a Button with origin (0, 0) and zero width and height.
+star You must call an initializer method to properly initialize the Button.
+star/
+   public Button ()
+   {
+ super ();
+ 
+ _titleColor = Color.black;
+ _disabledTitleColor = Color.gray;
+ _titleFont = Font.defaultFont ();
+   }
+ 
+   /star Convenience constructor for instantiating a Button with
+star bounds x, y, width, and height.  Equivalent to
+star foo = new Button ();
+star foo.init (x, y, width, height);
+star/
+   public Button (int x, int y, int width, int height)
+   {
+ this ();
+ init (x, y, width, height);
+   }
+ }" > testcase01
+ 
+ # This test case was supplied by Jacob Burckhardt:
+ echo "a
+ a
+ a
+ a
+ a" > testcase02
+ 
+ # This test case was supplied by Karl Tomlinson who also wrote the
+ # patch which lets CVS correctly handle this and several other cases:
+ 

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

Jacob Burckhardt wrote:
> 
> I saw different results than you.  You said no conflicts with older as
> common file, but I did get conflicts.  Maybe we used different
> commands?  My command was:
> 
> cat YOURS | diff3 -E -am MINE OLDER -
> 

> Below, I show one
> reasonable interpretation of the changes (although there are other
> reasonable interpretations).  Your original files did not have any
> blank lines in it, but I have added blanks lines so I can show what I
> think diff considers to be similar lines.  I made it so that similar
> liens are on the same horizontal level so it makes it it easy to make
> your eyes horizontally scan it.  Note that I run diff3 on your
> original files with no blank lines.  I only add blank lines here as a
> way to show how I think diff is matching things up:
> 
> R means result of merge.
> 
> M  O  Y  R
> vv
> s  s  s  s
> x  x
> m1 m1 m1 m1
> m2 m2 m2 m2
> m1 m1
> x  x
> m2 m2
> s  s  s  s
> v  v  v  v
> s  s  s  s
>x
>  <<< MINE
> m1 m1m1
> x  x x
> m2 m2m2
>  ===
>  >>> -
> m1 m1 m1 m1
> m2 m2 m2 m2
> x  x
> s  s  s  s
> vv

That layout makes things clear thanks.

> I think it did something reasonable.  Change M deleted 1 line with an
> "x" on it, and change Y deleted that same line plus the 3 lines just
> below.  Those 3 lines are shown contained in the conflict brackets.  I
> think it is not a burden to make a human manually look at that and
> decide what to do.  And there is a good chance that deleting all 4
> lines is not the right thing to do.  So therefore cvs should not just
> assume that is right.  As long as you pick the right section of the
> conflict, it will work.  Where as with some of my other test cases, the
> unpatched cvs has a conflict where there is obviously no conflict or
> it loses one person's changes with no hint to the user that it did so.
> 
> So to summarize, I think you patch makes cvs handle this case
> reasonably.  So far, I am satisfied with your patch the way it is.

Yes I agree it should be a conflict, and would be troubled if it wasn't.

> However, I am still worried that you and I get different results with
> the unpatched diff3.  Perhaps that indicates that I am not testing
> some case I *should* be testing.  So please let me know if I ran diff3
> wrong.

You did run diff3 correctly.

Sorry, I think I must've accidentally added that "x" that only exists
in O after I tested the files, because now I get the same results as you.

> 
> BTW, I have not yet tried out your other cases in your other e-mails.
> 

To check what I sent in 

http://www.mail-archive.com/bug-cvs%40gnu.org/msg00814.html

I copied from the web site and pasted into new files

for configuration 3)

> /usr/local/bin/diff3 -E -am MINE MINE YOURS
v
s
<<< MINE
x
m
===
>>> YOURS
m
<<< MINE
x
x
x
x
x
x
x
x
x
x
===
>>> YOURS
s
s
s
s
s
s
s
s
s
s
v

> cat YOURS | /usr/local/bin/diff3 -E -am MINE MINE - | diff YOURS -

gives no output as expected.

For configuration 2)

> /usr/local/bin/diff3 -E -am MINE OLDER YOURS
v
s
<<< MINE
x
m
===
>>> YOURS
m
<<< MINE
x
===
>>> YOURS
s
v
s
<<< MINE
x
===
>>> YOURS
m
<<< MINE
m
x
===
>>> YOURS
s
v

> cat YOURS | /usr/local/bin/diff3 -E -am MINE OLDER -
v
s
m
s
v
s
m
s
v

For configuration 1) of
http://www.mail-archive.com/bug-cvs%40gnu.org/msg00803.html

I'm not happy with

> /usr/local/bin/diff3 -E -am MINE OLDER YOURS | diff MINE -

having no output

M  O  Y  R=M

x  x  x  x
s  s  s  s
c  a  a  c
   b  b  
s  s  s  s
b b  b
s s  s
y  y  y  y

as it seems to easy to loose changes in Y.

I'm happier with

> cat YOURS | /usr/local/bin/diff3 -E -am MINE OLDER -
x
s
c
s
b
s
b
s
y

M  O  Y  R

x  x  x  x
s  s  s  s
cc
s  a  a  s
b  b  b  b
s  s  s  s
  b  b
  s  s
y  y  y  y

and would even be happy with

M  O  Y  R

x  x  x  x
s  s  s  s
 <<< MINE
cc
s  a  a  s
  b
  s
 ===
c
s  a  a  a
  b  b
  s  s
 >>> YOURS
b  b  b  b
s  s  s  s
y  y  y  y

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Karl Tomlinson writes:
> So I recommend as a test example for configuration 2) of
> http://www.mail-archive.com/bug-cvs%40gnu.org/msg00803.html
> 
> M:
> v
> s
> x
> m1
> m2
> m1
> x
> m2
> s
> v
> s
> m1
> x
> m2
> m1
> m2
> x
> s
> v
> 
> O:
> s
> x
> m1
> m2
> m1
> x
> m2
> s
> v
> s
> x
> m1
> x
> m2
> m1
> m2
> x
> s
> 
> Y:
> s
> m1
> m2
> s
> v
> s
> m1
> m2
> s
> 
> I tested this with diffutils diff3 version 2.7 and it produced conflicts.
> No conflicts with older as the common file.

I saw different results than you.  You said no conflicts with older as
common file, but I did get conflicts.  Maybe we used different
commands?  My command was:

cat YOURS | diff3 -E -am MINE OLDER -

That gave:

v
s
m1
m2
s
v
s
<<< MINE
m1
x
m2
===
>>> -
m1
m2
s
v

But I think it is OK that it is in conflict.  Like you said, there is
often more than one equivalent diff and if all you have is the end
result of a person's edits then cvs doesn't necessarily know exactly
what part of the file he edited.  So cvs can't be expected to always
pick the diff which matched what the person did.  Below, I show one
reasonable interpretation of the changes (although there are other
reasonable interpretations).  Your original files did not have any
blank lines in it, but I have added blanks lines so I can show what I
think diff considers to be similar lines.  I made it so that similar
liens are on the same horizontal level so it makes it it easy to make
your eyes horizontally scan it.  Note that I run diff3 on your
original files with no blank lines.  I only add blank lines here as a
way to show how I think diff is matching things up:

R means result of merge.

M  O  Y  R
vv
s  s  s  s
x  x  
m1 m1 m1 m1   
m2 m2 m2 m2   
m1 m1 
x  x  
m2 m2 
s  s  s  s
v  v  v  v
s  s  s  s
   x  
 <<< MINE 
m1 m1m1   
x  x x
m2 m2m2   
 ===  
 >>> -
m1 m1 m1 m1   
m2 m2 m2 m2   
x  x  
s  s  s  s
vv   

I think it did something reasonable.  Change M deleted 1 line with an
"x" on it, and change Y deleted that same line plus the 3 lines just
below.  Those 3 lines are shown contained in the conflict brackets.  I
think it is not a burden to make a human manually look at that and
decide what to do.  And there is a good chance that deleting all 4
lines is not the right thing to do.  So therefore cvs should not just
assume that is right.  As long as you pick the right section of the
conflict, it will work.  Where as with some of my other test cases, the
unpatched cvs has a conflict where there is obviously no conflict or
it loses one person's changes with no hint to the user that it did so.

So to summarize, I think you patch makes cvs handle this case
reasonably.  So far, I am satisfied with your patch the way it is.

However, I am still worried that you and I get different results with
the unpatched diff3.  Perhaps that indicates that I am not testing
some case I *should* be testing.  So please let me know if I ran diff3
wrong.

BTW, I have not yet tried out your other cases in your other e-mails.

> 
> The prepatch cvs analyze.c is slightly different to diffutils (in shift_boundaries).
> I haven't tried it but suspect that the conflicts will most likely still occur.


___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

Here is a short (25 line) test case for configuration 3) of
http://www.mail-archive.com/bug-cvs%40gnu.org/msg00803.html

M=O (or M=Y):

s
x
m
m
x
x
x
x
x
x
x
x
x
x
s
s
s
s
s
s
s
s
s
s
v

Y (or O):

v
s
m
s
s
s
s
s
s
s
s
s
s
v

And here is a slightly simpler test case for configuration 2):

M:

v
s
x
m
m
x
s
v
s
x
m
m
x
s
v

O:

s
x
m
m
x
s
v
s
x
m
m
x
s

Y:

s
m
s
v
s
m
s

If any of the test cases you mention don't fit into the configurations
I described, I would be interested to see them.

I imagine that it is still possible to break the merging code in cvs
without making these test cases fail, but I'm not sure that we could
ever provide enough test cases to prevent this.  At least these test
cases provide an indication of what sort of thing my patch fixed.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

Karl Tomlinson wrote:
> 
> Karl Tomlinson wrote:
> >
> > 2&3)
> >
> > Both depend on a difference between files that can be represented
> > by more than one minimal hunk.
> >
> > An example is
> >
> > Difference A:
> >
> >  1  2
> >
> >  x  y
> >  m  m
> >  x  y
> > m
> > y
> >
> > Does the m in file 1 match the first or second m in file 2?
> >
> > The logic in diag and compareseq is not consistent in analyze.c.
> > The match selected depends on where in the file this difference occurs
> > and on the surrounding differences.
> >
> 
> It seems more complicated than I thought.  I haven't been able to
> come up with any examples based on differences such as this or
> variations.

I finally had some success with

Difference B:

1  2

s  s
x  m1
m1 m2
m2 s
m1
x
m2
s

diff can produce hunks with either the first or second 
m1 and m2 in file 1 matching in file 2.

File 2 must be the common file to produce conflicts.

So I recommend as a test example for configuration 2) of
http://www.mail-archive.com/bug-cvs%40gnu.org/msg00803.html

M:
v
s
x
m1
m2
m1
x
m2
s
v
s
m1
x
m2
m1
m2
x
s
v

O:
s
x
m1
m2
m1
x
m2
s
v
s
x
m1
x
m2
m1
m2
x
s

Y:
s
m1
m2
s
v
s
m1
m2
s

I tested this with diffutils diff3 version 2.7 and it produced conflicts.
No conflicts with older as the common file.

The prepatch cvs analyze.c is slightly different to diffutils (in shift_boundaries).
I haven't tried it but suspect that the conflicts will most likely still occur.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

Karl Tomlinson wrote:
> 
> 2&3)
> 
> Both depend on a difference between files that can be represented
> by more than one minimal hunk.
> 
> An example is
> 
> Difference A:
> 
>  1  2
> 
>  x  y
>  m  m
>  x  y
> m
> y
> 
> Does the m in file 1 match the first or second m in file 2?
> 
> The logic in diag and compareseq is not consistent in analyze.c.
> The match selected depends on where in the file this difference occurs
> and on the surrounding differences.
> 

It seems more complicated than I thought.  I haven't been able to
come up with any examples based on differences such as this or
variations.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

"Derek R. Price" wrote:
> 
> The short test cases might hold clues as to common causes for these errors?
> 
> Karl, you managed to fix the algorithm without knowing what file configurations cause
> spurious conflicts?
> 

The short answer is yes, but I may be able to provide some help.
I didn't have much success analysing the problem by trying to determine
how the problem files were different so I looked at the algorithm and
noticed some things that I thought should be improved.
Having seen the algorithm, I can provide some theories on the problem
cases.  This was a while ago but my memory's coming back slowly.

I can now think of 3 possible configurations that could cause problems.

M(ine) O(lder) Y(ours)

1)

http://www.mail-archive.com/bug-cvs%40gnu.org/msg00429.html

M  O  Y

x  x  x
s  s  s
c  a  a
s  b  b
b  s  s
s  y  b
y s
  y

The erroreous merge with no conflicts may depend on the change
from O to Y repeating a portion of text.

2&3)

Both depend on a difference between files that can be represented
by more than one minimal hunk.

An example is

Difference A:

 1  2

 x  y
 m  m
 x  y
m
y

Does the m in file 1 match the first or second m in file 2?

The logic in diag and compareseq is not consistent in analyze.c.
The match selected depends on where in the file this difference occurs
and on the surrounding differences.

If a difference such as this exists between O and Y but O and M are
the same in this region of the file,
then this should be recognized as a change from O to Y with no conflict.
However, when diff3 compared M and Y, although it should find the same
difference as between O and Y but sometimes got a different hunk, and
reported a conflict.

Careful when trying to create an test example from Difference A.
If y never occurs in file 1 and/or x never occurs in file 2 then then
y and/or x are discarded before compareseq and diag.  The
difference

 1  2

 m  m
m

may produce much more consistent hunks.

2)

A different hunk may occur if there are changes from O to M in a
different region of text.

I think it should be fairly easy to find a small test example
for this case.

3)

This is the case where O and M are the same but different hunks are still
generated for diffs between O and Y, and between M and Y.  Things are more
complicated here but it may be possible to generate a smallish (<50 line)
test example.

This happens when the --horizon-lines option changes between two diffs run
by diff3.  The value of the argument is 10 for the first diff, but for the
second diff I think it is based on the sizes of the hunks for the first
diff.  So the first diff needs a hunk of more than 10 lines.

The horizon is the number of matching lines at the beginning or end of the
file that are not trimmed before performing the diff algorithm.
There need to be more than 10 matching lines at the beginning or end of the
file for a different --horizon-lines option to produce different hunks.

If there is a line in the extra horizon lines that is unique in its own
file, but exists in the other file (as a line that is not trimmed)
then its inclusion will prevent the line in the other file from being
discarded before diag and compareseq.  My theory is that this can cause a
change in the hunk reported.

An example may be:
O = M.  There is a difference between M and Y like Difference A.
There is also a difference (potentially the same difference) producing
a hunk of more than 10 lines.
There are more than 10 matching lines at the end of the file.
The (10+i)th (i>1) matching line (past the last non-matching line) is y.

It is going to be quite random whether a different hunk is produced.
You may need to play with extra non-matching lines elsewhere in the file
to get a different hunk produced.

Perhaps you could look at your current large failed examples, see if they
satisfy these prerequisites that I expect here, then remove lines that don't
affect these prerequisites.  You could possibly find that reducing the size
of the files a little removes the conflicts, and reducing them further brings
the conflict back.

Sorry, I can't think of anything better than trial and error.

Thanks Jacob for your efforts.

If some of this needs more explaining then please let me know specifically
what doesn't make sense, and I'll see what I can do.

P.S.

Don't forget that O is the common file after the patch.
So configuration 3) should be tested with M=Y.

Configuration 2) with Y and O interchanged could be a problem after the patch,
but this represents the same change made on branch and trunk.  i.e. a difference
where branch and trunk are the same but older is different.  But this should
be rarer than a difference between older and branch with trunk the same as
older.  I think this can actually be fixed also, but requires significant
changes to diag and compareseq, and turning off heuristics.
I have made these changes in diffutils but haven't transferred them to cvs.

Karl.

Re: Merging bug (wrong conflicts)

[Derek R. Price]

Jacob Burckhardt wrote:

> Derek R. Price writes:
>
> > Here-docs and escaped here-docs are easier for long texts:
>
> Thanks.  I think that will be helpful.

You're welcome.  :)


> 4 test cases have files of less that 26 lines.
> 4 test cases have sizes ranging from 600 to 2,000 lines
> 1 test case has files of 10,000 lines
>
> Obviously the first 4 cases are small enough to be directly in
> sanity.sh.

I agree.


> Then next 4 are probably too big and the last one is
> defiantly too big.  I will try to see if some cases are testing
> similar things, and if so, I will eliminate the redundant cases.  If I
> still think I need some of the big ones, I will try to make them
> smaller.  I'll work on reducing it for a few hours, but if it still
> seems difficult, I will let you know if I think I need to put the cases
> in separate files instead of directly in sanity.sh.

The short test cases might hold clues as to common causes for these errors?

Karl, you managed to fix the algorithm without knowing what file configurations cause
spurious conflicts?

If files do end up needing to be copied, I think the compromise might be that when the
files can't be found, the tests pass anyhow, perhaps with a warning in the log.

Derek

--
Derek Price  CVS Solutions Architect ( http://CVSHome.org )
mailto:[EMAIL PROTECTED] OpenAvenue ( http://OpenAvenue.com )
--
Laws that forbid the carrying of arms...  disarm only those who are neither
inclined nor determined to commit crimes...  Such laws make things worse for
the assaulted and better for the assailants; they serve rather to encourage
than to prevent homicides, for an unarmed man may be attacked with greater
confidence than an armed man.

- Thomas Jefferson, quoting Cesare Beccaria




___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Derek R. Price writes:
> Jacob Burckhardt wrote:
> > ... large files would be in sanity.sh if we wanted to follow the
> > current system that sanity.sh uses to test merges.  The current system
> > is to echo the contents of the file and redirect the echo command to a
> > file.  For example:
> >
> > echo "contents of
> > file to
> > be merged" > file_to_be_merged
> 
> Here-docs and escaped here-docs are easier for long texts:
> 
> 
>  cat >file <  text
>  text
>  text
>  EOF
> 
> and
> 
> 
>  cat >file <<\EOF
>  text
>  text
>  text
>  EOF
> 
> The EOF can be an arbitrary string and the \ tells the shell not to perform shell
> substitutions.

Thanks.  I think that will be helpful.

> 
> 
> > cp cvs/src/file_to_be_merged ${TESTDIR}/working_dir
> >
> > Another idea is to have sanity.sh use the expression $0 to get the
> > pathname of sanity.sh, and then it removes the sanity.sh from that
> 
> Not _completely_ a bad thing, though I might like to hear some other opinions on the
> matter.  I think I would try and avoid it though, if you can.  On the other hand, if
> the files are _really_ big, then maybe it is best.  How many lines are they - min,
> max, average?

I first reported that I had 8 test cases, but a co-worker recently
gave me a 9nth case.

4 test cases have files of less that 26 lines.
4 test cases have sizes ranging from 600 to 2,000 lines
1 test case has files of 10,000 lines

Obviously the first 4 cases are small enough to be directly in
sanity.sh.  Then next 4 are probably too big and the last one is
defiantly too big.  I will try to see if some cases are testing
similar things, and if so, I will eliminate the redundant cases.  If I
still think I need some of the big ones, I will try to make them
smaller.  I'll work on reducing it for a few hours, but if it still
seems difficult, I will let you know if I think I need to put the cases
in separate files instead of directly in sanity.sh.


___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Derek R. Price]

Sorry it took me son long to respond.  I was leaving this for someone else/thinking
about it and forgot until I spotted your more recent email.

Jacob Burckhardt wrote:

> Karl Tomlinson writes:
> > "Derek R. Price" wrote:
> > >
> > > > Karl, do you know what, specifically, was causing the merging problems?
> > > I can't come up with a minimal test case based on the message and comments in
> > > your patch.
> > >
> > > I'm thinking that the eight cases Jacob has might all be examples of only a
> > > few or even one error case...  Is it possible to pare those down into one or a
> > > few simple tests?
>
> ...
>
> > The problem would only occur if the differences between versions
> > were complicated enough so that there was more than one possible
> > diff output.  All the examples of this problem I have seen have been
> > quite large files, and the problem goes away on trying to reduce the
> > size of the files.
>
> Those large files would be in sanity.sh if we wanted to follow the
> current system that sanity.sh uses to test merges.  The current system
> is to echo the contents of the file and redirect the echo command to a
> file.  For example:
>
> echo "contents of
> file to
> be merged" > file_to_be_merged

Here-docs and escaped here-docs are easier for long texts:


 cat >file  cp cvs/src/file_to_be_merged ${TESTDIR}/working_dir
>
> Another idea is to have sanity.sh use the expression $0 to get the
> pathname of sanity.sh, and then it removes the sanity.sh from that

Not _completely_ a bad thing, though I might like to hear some other opinions on the
matter.  I think I would try and avoid it though, if you can.  On the other hand, if
the files are _really_ big, then maybe it is best.  How many lines are they - min,
max, average?

Derek

--
Derek Price  CVS Solutions Architect ( http://CVSHome.org )
mailto:[EMAIL PROTECTED] OpenAvenue ( http://OpenAvenue.com )
--
Old musicians don't die... they just decompose.




___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Karl Tomlinson writes:
> "Derek R. Price" wrote:
> > 
> > > Karl, do you know what, specifically, was causing the merging problems?
> > I can't come up with a minimal test case based on the message and comments in
> > your patch.
> > 
> > I'm thinking that the eight cases Jacob has might all be examples of only a
> > few or even one error case...  Is it possible to pare those down into one or a
> > few simple tests?

...

> The problem would only occur if the differences between versions
> were complicated enough so that there was more than one possible
> diff output.  All the examples of this problem I have seen have been
> quite large files, and the problem goes away on trying to reduce the
> size of the files.

Those large files would be in sanity.sh if we wanted to follow the
current system that sanity.sh uses to test merges.  The current system
is to echo the contents of the file and redirect the echo command to a
file.  For example:

echo "contents of
file to
be merged" > file_to_be_merged

So if the file to be merged is a big file, this will make
sanity.sh be much bigger, and sanity.sh is already a very big file, so
it would better to have small test cases.

But, if as you say, the bug goes away when you reduce the file, then
maybe we should try to find a practical way to keep the files big.
Here is one idea: Instead of creating the files to be merged by
echoing the contents of those files and redirecting, instead I propose
copying the file to be merged.  For example:

cp cvs/src/file_to_be_merged ${TESTDIR}/working_dir

I.e. the sanity.sh script would have a cp command instead of an echo
command.  It would copy the file from the cvs/src/ directory (the same
directory where sanity.sh is).

One problem with this idea is how sanity.sh figures out where this
directory is.  One solution might be to add a command line option to
sanity.sh to specify this directory.  But that would make the
interface to sanity.sh change and probably many people are used to the
existing interface and some people might have scripts to call it.  So
maybe changing the interface is a bad idea.

Another idea is to have sanity.sh use the expression $0 to get the
pathname of sanity.sh, and then it removes the sanity.sh from that
string which leaves it with the directory name.  Then it appends the
name of the merge file to that directory name to get the source
location for the cp command.  Of course even this assumes that
sanity.sh and the files to be merged are in the same directory.  It
will not work if someone moves sanity.sh to another directory.  But
maybe someone moving it is unlikely?

Please comment on these ideas.  If you prefer that I do not use the cp
idea, then I suppose I could try to reduce the size of the test cases
to where the echo command would be practical to use.  But remember that
Karl pointed out that reducing it is difficult.


___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

"Derek R. Price" wrote:
> 
> > Karl, do you know what, specifically, was causing the merging problems?
> I can't come up with a minimal test case based on the message and comments in
> your patch.
> 
> I'm thinking that the eight cases Jacob has might all be examples of only a
> few or even one error case...  Is it possible to pare those down into one or a
> few simple tests?
> 

There are two things that I think could be tested.  The first should definitely be 
tested.  The second is less important.

1) That cvs is making a reasonable effort to guess the intended changes between each 
version which I believe is dependent on using the ancestor file as the common file for 
the 2 2-way diffs.

The files in diff-bug.tar.gz at

http://www.mail-archive.com/bug-cvs%40gnu.org/msg00429.html

are very simple and test this well but don't look much like files you'd expect to see 
under cvs.  I suspect Ingolf's example at

http://www.mail-archive.com/bug-cvs%40gnu.org/msg00548.html

may also test the same thing but I haven't actually looked at this and the files are 
somewhat larger.

2) That the 2 2-ways in diff3 produce the same output when they should.  This has 
caused problems in the past when the two non-common files are the same.  These files 
used to be MINE and OLDER so the problem would occur sometimes when merging a change 
into an unmodified copy of a working file.  See Kevin's bug report

http://www.mail-archive.com/bug-cvs%40gnu.org/msg00288.html

With the patch, OLDER is the common file, so the potential problem could occur if the 
same change was on the branch as on the trunk.  i.e. the changes have already been 
merged in.  So a test might have testcase0-1.1 of Kevin's problem on the trunk, 
testcase0-1.1.2.2 on the branch, then merge the branch changes into the working copy 
twice.  I don't think the current standard version of cvs would have any problem with 
this.

My patch avoids this problem by using the same --horizon-lines arg for each call to 
diff, which Kevin also recommends.

The problem would only occur if the differences between versions were complicated 
enough so that there was more than one possible diff output.  All the examples of this 
problem I have seen have been quite large files, and the problem goes away on trying 
to reduce the size of the files.

Sorry, I don't have time now to assemble tests into a shell script.

Karl.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Derek R. Price]

Karl Tomlinson wrote:

> Thanks very much for your feedback.
> Pleased to hear that things are working.
> It seems from Derek's post that the most urgent thing required to
> get this into the official cvs is some test cases.
> I would be grateful it if you could set up some of these.
> I had thought that I'd complied fairly closely with the other
> requirements in HACKING.
> Please feel free to make modifications to coding style if it is not
> consistent with cvs.  Extra documentation or clarification of existing
> documentation would be very welcome.  I would appreciate being consulted
> if you were considering any algorithm changes though.

Karl, do you know what, specifically, was causing the merging problems?
I can't come up with a minimal test case based on the message and comments in
your patch.

I'm thinking that the eight cases Jacob has might all be examples of only a
few or even one error case...  Is it possible to pare those down into one or a
few simple tests?

Derek

--
Derek Price  CVS Solutions Architect ( http://CVSHome.org )
mailto:[EMAIL PROTECTED] OpenAvenue ( http://OpenAvenue.com )
--
Predeterminism was doomed from the start.




___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

Jacob Burckhardt wrote:
> 
> I tested your cvs patch and it works great.  Please try to get it into
> the official cvs.  I will be glad to help with that.  For example, I
> would volunteer to help make your patch follow all of the rules in
> the cvs distribution file HACKING.
> 
> I have 8 test cases in which either cvs 1.11 or diff3 merges incorrectly.
> Seven of these cases were mailed by various people to the cvs mailing
> lists, and the 8th case was discovered by my co-worker but I have not
> reported it yet.
> 
> I manually verified that your patch merges correctly in 7 of these
> test cases.  But one of the cases was so hard to verify that I was not
> able to thoroughly verify it (but I am still pretty confident that
> your patch works even in this case).
> 
> Just in case I made a mistake in my manual verification, I also ran each
> test case through fmerge and fmerge agrees with your patched cvs on
> every test case.  For more info on the fmerge utility, see:
> 
> http://www.canb.auug.org.au/~millerp/fhist.html
> 
> So please let me know if you would like my help in making your patch
> meet the requirements in the file HACKING.
> 

Thanks very much for your feedback.
Pleased to hear that things are working.
It seems from Derek's post that the most urgent thing required to
get this into the official cvs is some test cases.
I would be grateful it if you could set up some of these.
I had thought that I'd complied fairly closely with the other
requirements in HACKING.
Please feel free to make modifications to coding style if it is not
consistent with cvs.  Extra documentation or clarification of existing
documentation would be very welcome.  I would appreciate being consulted
if you were considering any algorithm changes though.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Derek R. Price]

Jacob Burckhardt wrote:

> I have 8 test cases in which either cvs 1.11 or diff3 merges incorrectly.
> Seven of these cases were mailed by various people to the cvs mailing
> lists, and the 8th case was discovered by my co-worker but I have not
> reported it yet.
>
> I manually verified that your patch merges correctly in 7 of these
> test cases.  But one of the cases was so hard to verify that I was not
> able to thoroughly verify it (but I am still pretty confident that
> your patch works even in this case).
>
> Just in case I made a mistake in my manual verification, I also ran each
> test case through fmerge and fmerge agrees with your patched cvs on
> every test case.  For more info on the fmerge utility, see:

Jacob,

Could you send me your 8 merge errors as sanity.sh test cases?

Derek

--
Derek Price  CVS Solutions Architect ( http://CVSHome.org )
mailto:[EMAIL PROTECTED] OpenAvenue ( http://OpenAvenue.com )
--
I will not say "Springfield" just to get applause.
I will not say "Springfield" just to get applause.
I will not say "Springfield" just to get applause...

  - Bart Simpson on chalkboard, _The Simpsons_




___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Tony Hoyle writes:
> Jacob Burckhardt wrote:
> > I tested your cvs patch and it works great.  Please try to get it into
> > the official cvs.  I will be glad to help with that.  For example, I
> > would volunteer to help make your patch follow all of the rules in
> > the cvs distribution file HACKING.
> 
> I only just caught this thread.  I'm currently seeing some really nasty
> merging errors, and hope that this can go some way to fixing it.
> 
> Is there a URL do download the patch from?

The patch is in an email sent to this mailing list.  You can get that
email at:

http://www.mail-archive.com/bug-cvs@gnu.org/msg00554.html


___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Tony Hoyle]

Jacob Burckhardt wrote:
> I tested your cvs patch and it works great.  Please try to get it into
> the official cvs.  I will be glad to help with that.  For example, I
> would volunteer to help make your patch follow all of the rules in
> the cvs distribution file HACKING.

I only just caught this thread.  I'm currently seeing some really nasty
merging errors, and hope that this can go some way to fixing it.

Is there a URL do download the patch from?

Tony

-- 

The only secure computer is one that's unplugged, locked in a safe,
and buried 20 feet under the ground in a secret location... and i'm
not even too sure about that one"--Dennis Huges, FBI.

[EMAIL PROTECTED]

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Merging bug (wrong conflicts)

[Jacob Burckhardt]

Karl Tomlinson writes:
> The problem you mention has been around for a while.  Some attempts have
> been made to improve things.  These have solved some problems and
> created others.
> 
> see http://www.mail-archive.com/bug-cvs%40gnu.org/msg00429.html
> 
> The problem is in diff3, which runs two 2-way diffs and then merges them
> together to make a 3-way diff.  The 2-way diffs do not always represent
> same changes in the same way.  When they are differences, diff3 starts
> reporting conflicts that aren't really there.
> 
> The first step in solving the problem is to change diff3 so that the two
> 2-way diffs both involve the ancestor file as the common file.  (cvs
> version 1.11 uses YOURFILE as the common file.)  This minimizes the
> number of instances where similar changes exist in the two diffs and
> means that the diffs are more likely to represent the intended changes. 
> The attached patch will do this.  See comments in the patch for more
> info.
> 
> There are still problems if the same changes are made on the branch and
> trunk, but this shouldn't happen too often.  The situation can be
> improved by modifying diff to represent the similar changes more
> consistently.  I can supply patches for diffutils to implement this if
> anyone is interested but haven't transfered the changes to cvs yet.  I
> just use pcl-cvs and ediff when there are conflicts to resolve.

I tested your cvs patch and it works great.  Please try to get it into
the official cvs.  I will be glad to help with that.  For example, I
would volunteer to help make your patch follow all of the rules in
the cvs distribution file HACKING.

I have 8 test cases in which either cvs 1.11 or diff3 merges incorrectly.
Seven of these cases were mailed by various people to the cvs mailing
lists, and the 8th case was discovered by my co-worker but I have not
reported it yet.

I manually verified that your patch merges correctly in 7 of these
test cases.  But one of the cases was so hard to verify that I was not
able to thoroughly verify it (but I am still pretty confident that
your patch works even in this case).

Just in case I made a mistake in my manual verification, I also ran each
test case through fmerge and fmerge agrees with your patched cvs on
every test case.  For more info on the fmerge utility, see:

http://www.canb.auug.org.au/~millerp/fhist.html

So please let me know if you would like my help in making your patch
meet the requirements in the file HACKING.

> 
> Karl.
> 
> Index: ccvs/diff/ChangeLog
> diff -c ccvs/diff/ChangeLog:1.1 ccvs/diff/ChangeLog:1.2
> *** ccvs/diff/ChangeLog:1.1   Fri Jun 30 14:33:00 2000
> --- ccvs/diff/ChangeLog   Wed Jul 26 15:14:56 2000
> ***
> *** 1,3 
> --- 1,18 
> + 2000-07-26  Karl Tomlinson  <[EMAIL PROTECTED]>
> + 
> + * diff3.c (main): changed the common file of the two diffs to
> + OLDFILE for merges and edscripts so that the diffs are more likely
> + to contain the intended changes.  Not changing the horizon-lines
> + arg for the second diff.  If the two diffs have the same parameters
> + equal changes in each diff are more likely to appear the same.
> + 
> + * analyze.c (shift_boundaries): undid Paul Eggert's patch to fix
> + the diff3 merge bug described in ccvs/doc/DIFFUTILS-2.7-BUG.  The
> + patch is no longer necessary now that diff3 does its differences
> + differently.  I think the hunk merges provide a better indication
> + of the area modified by the user now that the diffs are actually
> + done between the appropriate revisions.
> + 
>   1999-12-29  Jim Kingdon  
>   
>   * diff.c (compare_files): Use explicit braces with if-if-else, per
> Index: ccvs/diff/analyze.c
> diff -c ccvs/diff/analyze.c:1.1 ccvs/diff/analyze.c:1.2
> *** ccvs/diff/analyze.c:1.1   Fri Jun 30 14:33:01 2000
> --- ccvs/diff/analyze.c   Wed Jul 26 15:10:46 2000
> ***
> *** 621,627 
>struct file_data filevec[];
>   {
> int f;
> !   int inhibit_hunk_merge = horizon_lines != context;
>   
> for (f = 0; f < 2; f++)
>   {
> --- 621,629 
>struct file_data filevec[];
>   {
> int f;
> ! 
> !   if (inhibit)
> ! return;
>   
> for (f = 0; f < 2; f++)
>   {
> ***
> *** 664,684 
>we can later determine whether the run has grown.  */
> runlength = i - start;
>   
> !   if (! inhibit_hunk_merge)
>   {
> !   /* Move the changed region back, so long as the
> !  previous unchanged line matches the last changed one.
> !  This merges with previous changed regions.  */
> ! 
> !   while (start && equivs[start - 1] == equivs[i - 1])
> ! {
> !   changed[--start] = 1;
> !   changed[--i] = 0;
> !   while (changed[start - 1])
> ! start--;
> !   while (other

Re: Merging bug (wrong conflicts)

[Karl Tomlinson]

The problem you mention has been around for a while.  Some attempts have
been made to improve things.  These have solved some problems and
created others.

see http://www.mail-archive.com/bug-cvs%40gnu.org/msg00429.html

The problem is in diff3, which runs two 2-way diffs and then merges them
together to make a 3-way diff.  The 2-way diffs do not always represent
same changes in the same way.  When they are differences, diff3 starts
reporting conflicts that aren't really there.

The first step in solving the problem is to change diff3 so that the two
2-way diffs both involve the ancestor file as the common file.  (cvs
version 1.11 uses YOURFILE as the common file.)  This minimizes the
number of instances where similar changes exist in the two diffs and
means that the diffs are more likely to represent the intended changes. 
The attached patch will do this.  See comments in the patch for more
info.

There are still problems if the same changes are made on the branch and
trunk, but this shouldn't happen too often.  The situation can be
improved by modifying diff to represent the similar changes more
consistently.  I can supply patches for diffutils to implement this if
anyone is interested but haven't transfered the changes to cvs yet.  I
just use pcl-cvs and ediff when there are conflicts to resolve.

Karl.

Index: ccvs/diff/ChangeLog
diff -c ccvs/diff/ChangeLog:1.1 ccvs/diff/ChangeLog:1.2
*** ccvs/diff/ChangeLog:1.1 Fri Jun 30 14:33:00 2000
--- ccvs/diff/ChangeLog Wed Jul 26 15:14:56 2000
***
*** 1,3 
--- 1,18 
+ 2000-07-26  Karl Tomlinson  <[EMAIL PROTECTED]>
+ 
+   * diff3.c (main): changed the common file of the two diffs to
+   OLDFILE for merges and edscripts so that the diffs are more likely
+   to contain the intended changes.  Not changing the horizon-lines
+   arg for the second diff.  If the two diffs have the same parameters
+   equal changes in each diff are more likely to appear the same.
+ 
+   * analyze.c (shift_boundaries): undid Paul Eggert's patch to fix
+   the diff3 merge bug described in ccvs/doc/DIFFUTILS-2.7-BUG.  The
+   patch is no longer necessary now that diff3 does its differences
+   differently.  I think the hunk merges provide a better indication
+   of the area modified by the user now that the diffs are actually
+   done between the appropriate revisions.
+ 
  1999-12-29  Jim Kingdon  
  
* diff.c (compare_files): Use explicit braces with if-if-else, per
Index: ccvs/diff/analyze.c
diff -c ccvs/diff/analyze.c:1.1 ccvs/diff/analyze.c:1.2
*** ccvs/diff/analyze.c:1.1 Fri Jun 30 14:33:01 2000
--- ccvs/diff/analyze.c Wed Jul 26 15:10:46 2000
***
*** 621,627 
   struct file_data filevec[];
  {
int f;
!   int inhibit_hunk_merge = horizon_lines != context;
  
for (f = 0; f < 2; f++)
  {
--- 621,629 
   struct file_data filevec[];
  {
int f;
! 
!   if (inhibit)
! return;
  
for (f = 0; f < 2; f++)
  {
***
*** 664,684 
 we can later determine whether the run has grown.  */
  runlength = i - start;
  
! if (! inhibit_hunk_merge)
{
! /* Move the changed region back, so long as the
!previous unchanged line matches the last changed one.
!This merges with previous changed regions.  */
! 
! while (start && equivs[start - 1] == equivs[i - 1])
!   {
! changed[--start] = 1;
! changed[--i] = 0;
! while (changed[start - 1])
!   start--;
! while (other_changed[--j])
!   continue;
!   }
}
  
  /* Set CORRESPONDING to the end of the changed run, at the last
--- 666,683 
 we can later determine whether the run has grown.  */
  runlength = i - start;
  
! /* Move the changed region back, so long as the
!previous unchanged line matches the last changed one.
!This merges with previous changed regions.  */
! 
! while (start && equivs[start - 1] == equivs[i - 1])
{
! changed[--start] = 1;
! changed[--i] = 0;
! while (changed[start - 1])
!   start--;
! while (other_changed[--j])
!   continue;
}
  
  /* Set CORRESPONDING to the end of the changed run, at the last
***
*** 686,700 
 CORRESPONDING == I_END means no such point has been found.  */
  corresponding = other_changed[j - 1] ? i : i_end;
  
! /* Shift the changed region forward, so long as the
!first changed line matches the following unchanged one,
!but if I

Re: Merging bug (wrong conflicts)

[Ingolf Steinbach]

To illustrate the problem, please use the attached bug,v.gz
file in the repository and

cvs update -A bug

after that. Look at the changes made in the "external"
branch:

cvs diff -r 1.1 -r 1.1.2.1 bug

(Note e.g. the last two changes: two blocks are deleted.)

Look at the changes between 1.1 and 1.2:

cvs diff -r 1.1 -r 1.2 bug

(The part changed in the "external" branch is not affected
 by these differences.)

Try to merge the changes from the external branch into the
main line:

cvs update -j external bug

CVS reports a conflict for a line which was only changed in
the external branch. The deleted blocks (see above) are
still there.

(Thanks, Cameron, for the "sed"-tip.)

Regards
Ingolf
-- 

Ingolf Steinbach   Jena-Optronik GmbH
[EMAIL PROTECTED] ++49 3641 200-147
PGP: 0x7B3B5661  213C 828E 0C92 16B5  05D0 4D5B A324 EC04

 gzip'ed repository file