Re: join not producing conflict

[Mark D. Baushke]

Derek Robert Price <derek@ximbiot.com> writes:

> Paul Edwards wrote:
> 
> | ie its behaviour is totally correct. I didn't see anything wrong,
> |
> |and don't see why you would seek to make any changes other
> |than the one you have already made.
> 
> 
> I'm mostly ambivalent to this change, but I think that the original
> behavior might have been an intentional decision to assume that if
> merging into the base revisionof a file  caused an empty diff, then no
> attempt should be made to merge into the user's modified copy since they
> had already seen this change and intentionally altered it.
> 
> In other words, though I agree with Paul that the comment held an
> inappropriate use of the word "optimization", his argument was not a
> valid one for the change since the comment might simply have held an
> inappropriate use of the word, rather than inappropriate code.

Actually, it was my first patch (I have rewritten the replacement
comment to hopefully be clearer) that held the word 'optimization' the
original code that we are talking about removing is as follows:

    /* If the target of the merge is the same as the working file
       revision, then there is nothing to do.  */
    if (vers->vn_user != NULL && strcmp (rev2, vers->vn_user) == 0)
    {
        if (rev1 != NULL)
            free (rev1);
        free (rev2);
        return;
    }

The effect of this code is to stop any attempt to merge the file if rev2
is already what is in the user's tree regardless of any modifications
that might currently be in the user's tree version.
 
> Regardless, since `make check' passes for me on Linux with Mark's patch
> and his changes to sanity.sh seem to be beneficial and not drastic in
> nature, I don't have any immediate objections to the patch being
> committed to stable, provided that a test is added to the new join6 that
> produces and verifies merge markers can be generated correctly by the
> new behavior.

With the patch to remove the above block of code, there is different
behavior...

If the text is different in the file at the place where a merge would
occur, the conflict markers are generated. I have added a test to join6
for this case. (In real usage, this case may actually be useful in that
it might help a user figure out if the current change they have in their
version of the file conflicts with something that was just added by the
last commit...)

However, if the code was just the elimination of the code that was added
during the patch, the

   diff3 -E -am -L file -L rev1 -L rev2 file file.rev1 file.rev2

will NOT generate conflict markers. I am unable to find any path
thru the code that is able to generate such conflict markers.

The workaround to the unpatched behavior is something like this:

   cvs -n up -p -rrev1 file > file.rev1
   cvs -n up -p  file > file.rev2
   diff3 -E -am file file.rev1 file.rev2 > file.new
   mv file.new file

(except the 'Result of merge' will not be in the CVS/Entries timestamp
for file in the workaround case).

which is what a typical user might expect the 

   cvs up -jrev1 -jrev2 file

command to do for them.

> Whether the previous behavior was by design or not, this
> could be the least suprising result of an up -j -j command since every
> time the command is run the requested merge will be attempted on the
> local workspace, modified files or no.  The new assumption being that
> the user knew about the local changes and was deliberately requesting
> the re-merge, perhaps to reverse local changes (as is currently
> happening in the join6 test).

Agreed.

> As an example, a user might have merged two or more change sets, perhaps
> made some local changes, then decided they didn't want one of the change
> sets.  This is possible with the new behavior and may not have been
> before if the merge being reversed was itself a backout of a previously
> committed change.
> 
> It might still be worth asking on info-cvs whether anyone is depending
> on the previous behavior.  There's a slight chance someone might read
> the post and respond before we get the same information in the form of a
> bug report following the next release.  :)

Good point.

I'll see what I can do about writing such a request for feeback to the
info-cvs list.

Paul? If you have a suggestion for a write-up proposal to send to
info-cvs, I would love to read it. :-)

I have included a revised patch which rewords the comment that I am
leaving behind about the code that was removed. I have also added a test
to add conflict markers and one to show code being removed.

Are there any additional comments on this topic before we raise the
issue with the info-cvs@gnu.org audience?

        Thanks,
        -- Mark

Log Message:
2003-10-21  Mark D. Baushke  <mdb@cvshome.org>

        * update.c (join_file): Do the -jrev1 -jrev2 merge even when
        the file is already at rev2.
        * sanity.sh (join6): New testcase for above.
        (Suggested by "Paul Edwards" <kerravon@nosppaam.w3.to>.)
        (import): Fix collateral damage.

Index: update.c
===================================================================
RCS file: /cvs/ccvs/src/update.c,v
retrieving revision 1.202.4.7
diff -u -p -r1.202.4.7 update.c
--- update.c    8 Oct 2003 21:15:48 -0000       1.202.4.7
+++ update.c    21 Oct 2003 21:37:52 -0000
@@ -2316,15 +2316,14 @@ join_file (finfo, vers)
        return;
     }
 
-    /* If the target of the merge is the same as the working file
-       revision, then there is nothing to do.  */
-    if (vers->vn_user != NULL && strcmp (rev2, vers->vn_user) == 0)
-    {
-       if (rev1 != NULL)
-           free (rev1);
-       free (rev2);
-       return;
-    }
+    /* At one time, there was a test here to see if the file to be
+       merged was already at rev2. If so, this function returned after
+       freeing rev1 and rev2. The assumption that there was nothing
+       useful to do appears to have been a bad one. However, not
+       having that block of code means that text that was
+       intentionally added, removed or modified in the area of the
+       merge requested will either be silently re-introduced,
+       re-removed, or result in conflict markers. */
 
     /* If rev1 is dead or does not exist, then the file was added
        between rev1 and rev2.  */
Index: sanity.sh
===================================================================
RCS file: /cvs/ccvs/src/sanity.sh,v
retrieving revision 1.752.2.46
diff -u -p -r1.752.2.46 sanity.sh
--- sanity.sh   19 Oct 2003 15:13:14 -0000      1.752.2.46
+++ sanity.sh   21 Oct 2003 21:48:37 -0000
@@ -714,8 +714,8 @@ if test x"$*" = x; then
        tests="${tests} rmadd2 dirs dirs2 branches branches2 tagc tagf"
        tests="${tests} rcslib multibranch import importb importc"
        tests="${tests} update-p import-after-initial branch-after-import"
-       tests="${tests} join join2 join3 join4 join5 join-readonly-conflict"
-       tests="${tests} join-admin join-admin-2"
+       tests="${tests} join join2 join3 join4 join5 join6"
+       tests="${tests} join-readonly-conflict join-admin join-admin-2"
        tests="${tests} new newb conflicts conflicts2 conflicts3"
        tests="${tests} clean"
        # Checking out various places (modules, checkout -d, &c)
@@ -7184,7 +7184,15 @@ RCS file: ${CVSROOT_DIRNAME}/first-dir/i
 retrieving revision 1\.1\.1\.1
 retrieving revision 1\.1\.1\.2
 Merging differences between 1\.1\.1\.1 and 1\.1\.1\.2 into imported-f2
-rcsmerge: warning: conflicts during merge"
+rcsmerge: warning: conflicts during merge
+RCS file: ${CVSROOT_DIRNAME}/first-dir/imported-f3,v
+retrieving revision 1\.1\.1\.1
+retrieving revision 1\.1\.1\.2
+Merging differences between 1\.1\.1\.1 and 1\.1\.1\.2 into imported-f3
+RCS file: ${CVSROOT_DIRNAME}/first-dir/imported-f4,v
+retrieving revision 1\.1\.1\.1
+retrieving revision 1\.1\.1\.3
+Merging differences between 1\.1\.1\.1 and 1\.1\.1\.3 into imported-f4"
 
                cd first-dir
 
@@ -8646,6 +8654,89 @@ C -file"
          cd ..
          rm -r join5
          rm -rf ${CVSROOT_DIRNAME}/join5
+         ;;
+
+        join6)
+         mkdir join6; cd join6
+          mkdir 1; cd 1
+         dotest join6-init-1 "${testcvs} -Q co -l ."
+         mkdir join6
+         dotest join6-init-2 "${testcvs} -Q add join6"
+         cd join6
+          echo aaa >temp.txt
+         echo bbb >>temp.txt
+         echo ccc >>temp.txt
+         dotest join6-1 "${testcvs} -Q add temp.txt"
+         dotest join6-2 "${testcvs} -q commit -minitial temp.txt" \
+"RCS file: ${CVSROOT_DIRNAME}/join6/temp\.txt,v
+done
+Checking in temp\.txt;
+${CVSROOT_DIRNAME}/join6/temp.txt,v  <--  temp\.txt
+initial revision: 1\.1
+done"
+         cp temp.txt temp2.txt
+         echo ddd >>temp.txt
+         dotest join6-3 "${testcvs} -q commit -madd temp.txt" \
+"Checking in temp\.txt;
+${CVSROOT_DIRNAME}/join6/temp.txt,v  <--  temp\.txt
+new revision: 1\.2; previous revision: 1\.1
+done"
+         cp temp2.txt temp.txt
+         dotest_fail join6-4 "${testcvs} diff temp.txt" \
+"Index: temp.txt
+===================================================================
+RCS file: ${CVSROOT_DIRNAME}/join6/temp\.txt,v
+retrieving revision 1\.2
+diff -r1\.2 temp\.txt
+4d3
+< ddd"
+
+         dotest join6-5 "${testcvs} update -j1.1 -j1.2 temp.txt" \
+"M temp\.txt
+RCS file: ${CVSROOT_DIRNAME}/join6/temp\.txt,v
+retrieving revision 1\.1
+retrieving revision 1\.2
+Merging differences between 1\.1 and 1\.2 into temp\.txt"
+         dotest join6-6 "${testcvs} diff temp.txt" ""
+         mv temp.txt temp3.txt
+         dotest join6-7 "sed 's/ddd/dddd/' < temp3.txt > temp.txt" ""
+         dotest join6-8 "${testcvs} update -j1.1 -j1.2 temp.txt" \
+"M temp\.txt
+RCS file: ${CVSROOT_DIRNAME}/join6/temp\.txt,v
+retrieving revision 1\.1
+retrieving revision 1\.2
+Merging differences between 1\.1 and 1\.2 into temp\.txt
+rcsmerge: warning: conflicts during merge"
+         dotest_fail join6-9 "${testcvs} diff temp.txt" \
+"Index: temp\.txt
+===================================================================
+RCS file: ${CVSROOT_DIRNAME}/join6/temp.txt,v
+retrieving revision 1\.2
+diff -r1\.2 temp\.txt
+3a4,6
+> <<<<<<< temp\.txt
+> dddd
+> =======
+4a8
+> >>>>>>> 1\.2"
+         cp temp2.txt temp.txt
+         dotest join6-10 "${testcvs} -q ci -m del temp.txt" \
+"Checking in temp\.txt;
+${CVSROOT_DIRNAME}/join6/temp.txt,v  <--  temp\.txt
+new revision: 1\.3; previous revision: 1\.2
+done"
+         dotest join6-11 "${testcvs} diff temp.txt" ""
+         dotest join6-12 "cmp temp2.txt temp.txt" ""
+
+         cd ../../..
+
+         if $keep; then
+           echo Keeping ${TESTDIR} and exiting due to --keep
+            exit 0
+         fi
+
+         rm -r join6
+         rm -rf ${CVSROOT_DIRNAME}/join6
          ;;
 
        join-readonly-conflict)