Makepatch doesn't always include quite the right changes

Bugs
1

Connor Smith suggests there might be a fencepost error that could be remedied with:

diff --git a/src/cset.c b/src/cset.c
index 27ba3d9ae..4841b2e56 100644
--- a/src/cset.c
+++ b/src/cset.c
@@ -755,7 +755,7 @@ doDiff(sccs *sc)
                printf("\n");
                return;
        }
-       e = PARENT(sc, e);
+       e--;
        if (e == d) return;
        sccs_diffs(sc, REV(sc, e), REV(sc, d), &dop, stdout);
 }

… and offers this test case from bk’s own source:

cosmith@hilbert:~/tmp/work/bk-7.3.1ce$ bk changes -v -d'$if(:REPOTYPE:=product){$if(:CHANGESET:){${1=:REV:}}$else{$if(:MERGE:){ChangeSet@$1\t:GFILE:@:REV:\n}}}' | head
ChangeSet@1.2869        src/Makefile@1.552
ChangeSet@1.2869        src/utils/Makefile@1.67
ChangeSet@1.2859        src/slib.c@1.1073
ChangeSet@1.2858        src/csetprune.c@1.122
ChangeSet@1.2858        src/fast-export.c@1.17
ChangeSet@1.2858        src/ndiff.c@1.35
ChangeSet@1.2858        src/sccs.h@1.867
ChangeSet@1.2858        src/slib.c@1.1072
ChangeSet@1.2831.1.2    src/ndiff.c@1.31.1.1
ChangeSet@1.2831.1.2    src/slib.c@1.1068.1.1
cosmith@hilbert:~/tmp/work/bk-7.3.1ce$ bk changes -v -d'$if(:GFILE:=src/utils/Makefile){:GFILE:@:REV:\n}' -r1.2868..1.2869
src/utils/Makefile@1.67
src/utils/Makefile@1.66
cosmith@hilbert:~/tmp/work/bk-7.3.1ce$ bk-graph -r1.64..1.67 src/utils/Makefile
* src/utils/Makefile@1.67, 2016-07-05 13:07:32-04:00, wscott@x99.wscott.bitkeeper.com +0 -0
|\
| * src/utils/Makefile@1.65.1.1, 2016-07-05 13:07:22-04:00, wscott@x99.wscott.bitkeeper.com +1 -0
* | src/utils/Makefile@1.66, 2016-07-01 17:02:39-04:00, wscott@x99.wscott.bitkeeper.com +1 -0
|/
* src/utils/Makefile@1.65, 2016-04-21 14:29:35-07:00, ob@dirac.bitkeeper.com +6 -6
cosmith@hilbert:~/tmp/work/bk-7.3.1ce$ bkargs='bk makepatch -d -r1.2868..1.2869'; diff -u <($bkargs) <(~/tmp/work/bitkeeper/src/$bkargs)
--- /dev/fd/63  2024-01-24 16:20:18.870258417 +0000
+++ /dev/fd/62  2024-01-24 16:20:18.870258417 +0000
@@ -5,6 +5,14 @@
 # Host:        hilbert.dev.bluearc.com
 # Root:        /home/cosmith/tmp/work/bk-7.3.1ce

+#--- 1.392.1.2/BitKeeper/etc/attr      2016-07-05 15:09:19 +01:00
+#+++ 1.394/BitKeeper/etc/attr  2016-07-05 18:07:32 +01:00
+#@@ -5,4 +5,4 @@
+# @ID
+# lm@lm.bitmover.com|ChangeSet|19990319224848|02682|B:1000:6a5ce40345b2dee1
+# @VERSION
+#-20160701210239
+#+20160630190712
 #--- 1.23/BitKeeper/etc/ignore 2016-06-06 18:16:53 +01:00
 #+++ 1.24/BitKeeper/etc/ignore 2016-07-01 22:02:39 +01:00
 #@@ -128,3 +128,4 @@
@@ -590,16 +598,16 @@
 #      out("\r\n");
 #      out("Cache-Control: no-cache\r\n");     /* for http 1.1 */
 #      out("Pragma: no-cache\r\n");            /* for http 1.0 */
-#--- 1.66/src/utils/Makefile   2016-07-01 22:02:39 +01:00
+#--- 1.65.1.1/src/utils/Makefile       2016-07-05 18:07:22 +01:00
 #+++ 1.67/src/utils/Makefile   2016-07-05 18:07:32 +01:00
-#@@ -42,6 +42,7 @@
-#         SYS=unix
-#      EXE=
-#      EXT=.bin
-#+     LIBS=-lz
-#      H=../unix.h
-#      CFLAGS=-Os
-#      LD=$(CC)
+#@@ -12,6 +12,7 @@
+# # See the License for the specific language governing permissions and
+# # limitations under the License.
+#
+#+BK=../bk
+#
+#
+# # Override Solaris make.rules
 #--- 1.59/src/bkd_version.c    2016-03-09 13:22:25 +00:00
 #+++ 1.60/src/bkd_version.c    2016-07-01 22:02:39 +01:00
 #@@ -71,7 +71,7 @@
@@ -706,7 +714,7 @@
 # W=-Wall -Wno-parentheses -Wno-strict-aliasing
 #

-# Diff checksum=2170eeaa
+# Diff checksum=076c4ebd


 # Patch vers:  1.4
cosmith@hilbert:~/tmp/work/bk-7.3.1ce$

… wherein we see that the shipping makepatch missed changes and included an extra one.

Our-ref D158146.

2

Good find. bk makepatch -d doesn’t use good graph math in a number of ways. The Newfie path uses the file tip, even if asking for an old patch like you are. The diff path would use something like librange.c:range_unrange() but that uncolors D_SET, but the idea is right: to have a diff between 2 nodes color a region, then given the colored region, get the 2 nodes back. We want the newest D_SET colored for one, and the newest PARENT of a colored node, which is uncolored for the other. In the case you show, the Makefile has a merge node with one parent colored and other other uncolored. The existing code just happens to pick the wrong one. Your fix happens to pick the right one, but isn’t the right fix as it does use the graph to find the other. This is better:

===== cset.c 1.341 vs edited =====
--- 1.341/src/cset.c    2018-11-21 06:59:43 -05:00
+++ edited/src/cset.c   2026-04-25 19:29:57 -04:00
@@ -734,29 +734,33 @@
 private void
 doDiff(sccs *sc)
 {
-       ser_t   d, e = 0;
+       ser_t   d, p, e = 0;
        df_opt  dop = {0};
+       int     j;

        dop.out_unified = 1;
        if (CSET(sc)) return;           /* no changeset diffs */
        for (d = TABLE(sc); d >= TREE(sc); d--) {
                if (FLAGS(sc, d) & D_SET) {
-                       e = d;
-               } else if (e) {
-                       break;
+                       unless (e) e = d;
+                       EACH_PARENT(sc, d, p, j) {
+                               unless (FLAGS(sc, p) & D_SET) {
+                                       d = p;
+                                       goto breakout;
+                               }
+                       }
                }
        }
-       for (d = TABLE(sc); (d >= TREE(sc)) && !(FLAGS(sc, d) & D_SET); d--);
-       if (!d) return;
-       unless (PARENT(sc, e)) {
+    breakout:
+       unless (e) return;
+       unless (d) {
                printf("--- New file ---\n+++ %s\t%s\n",
                    sc->gfile, delta_sdate(sc, sccs_ino(sc)));
-               sccs_get(sc, 0, 0, 0, 0, SILENT, 0, stdout);
+               sccs_get(sc, REV(sc, e), 0, 0, 0, SILENT, 0, stdout);
                printf("\n");
                return;
        }
-       e = PARENT(sc, e);
-       if (e == d) return;
+       if (e == d) return; /* really an assert - cannot happen */
        sccs_diffs(sc, REV(sc, e), REV(sc, d), &dop, stdout);
 }

Enough has rotted that it would take a little work to get a test environment up. I also hit some problems with gcc where it compiles but core dumps, but clang seems to work. This has been sitting here more than a year, so may not be an issue. All that said, I’ll make a local repo to at least document this part and make notes about a test case.

3

Sigh, had the diff backwards. Should be:

sccs_diffs(sc, REV(sc, d), REV(sc, e), &dop, stdout);

4

Thinking about this as I slept (just like the old days!), a one sided graph difference (in set language, A & ~B) doesn’t necessarily form a lattice, but could have multiple roots: A,B,C..D. That is, in A…B, if A is not in the history of B, then the greatest common nodes to both A and B could be a list of more than one. While parts of BK do things to activate that version in a diff, this code doesn’t. bk makepatch is seldom used manually, while being fundamental to how pull works. As such, this is the first time I’ve seen -d used. If you want it to be correct, then you need to say what you think correct is. BK can activate the set of deltas that compute when naming multiple tips, but in some ways, it can be non-sense. That’s why smerge came into being: to make sense of a merge given both the internal structure of the weave, graph and set, plus the human level of what a merge means through a set of heuristics. In non-bk terms, a multi-tip activation is really a DAG of linked tokens, not a strict sequence. All that to say, while the patch I mentioned “works” in your case, it is because the lower bound is in the history of the upper bound. In the general case, it requires more of a discussion. The range_unrange() way includes a D_INVALID response if the D_SET region can’t be expressed as a one sided difference of 2 graph nodes. Maybe that’s what you would like or maybe not.