Be smarter about dive renumbering when merging dives

We really have two different cases for merging dives:

 (a) downloading a new dive from a dive computer, and merging it with an
     existing dive that we had already created using a different dive
     computer.  This is the "try_to_merge()" case, called from
     "process_dives()

 (b) merging two different dives into one longer dive.  This is the
     "merge_two_dives()" case when you explicitly merge dives using the
     divelist.

While a lot of the issues are the same, many details differ, and one of
the details is how dive numbering should be handled.

In particular, when you download from a dive computer and merge with an
existing dive, you want too take the *maximum* dive number, because the
dive computer notion of which dive it is may well not match what the
user dive number is.

On the other hand, when you explicitly merge in the dive list, you end
up renumbering not just the dive you are merging, but also all
subsequent dives, since you now have one fewer dives overall.  So that
case already has to be handled by the caller.

Now, the simpler "download from dive computer" case was broken by commit
ce3a78efcac2 ("Assign lower number to a merged dive instead of higher
one").  It fixed the numbering for the divelist case, but broke the
download case.

So this commit reverts commit ce3a78efcac2, and instead extends and
clarifies the dive renumbering that "merge_two_dives()" already did.  It
now explicitly renumbers not just the following dives, but also
renumbers the merged dive itself, so now we can go back to the old "take
the bigger dive number" for the core merging, which fixes the download
case.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Dirk Hohndel <dirk@hohndel.org>

2 files changed, 49 insertions, 6 deletions

diff --git a/subsurface-core/dive.c b/subsurface-core/dive.c
index ccb27aaf7..8fc9e993b 100644
--- a/subsurface-core/dive.c
+++ b/subsurface-core/dive.c
@@ -2848,7 +2848,7 @@ struct dive *merge_dives(struct dive *a, struct dive *b, int offset, bool prefer
 	MERGE_TXT(res, a, b, divemaster);
 	MERGE_MAX(res, a, b, rating);
 	MERGE_TXT(res, a, b, suit);
-	MERGE_MIN(res, a, b, number);
+	MERGE_MAX(res, a, b, number);
 	MERGE_NONZERO(res, a, b, cns);
 	MERGE_NONZERO(res, a, b, visibility);
 	MERGE_NONZERO(res, a, b, picture_list);
diff --git a/subsurface-core/divelist.c b/subsurface-core/divelist.c
index 99e09eab6..543d9e17b 100644
--- a/subsurface-core/divelist.c
+++ b/subsurface-core/divelist.c
@@ -835,10 +835,14 @@ bool consecutive_selected()
 	return consecutive;
 }
 
+/*
+ * Merge two dives. 'a' is always before 'b' in the dive list
+ * (and thus in time).
+ */
 struct dive *merge_two_dives(struct dive *a, struct dive *b)
 {
 	struct dive *res;
-	int i, j, factor;
+	int i, j, nr, nrdiff;
 	int id;
 
 	if (!a || !b)
@@ -854,7 +858,30 @@ struct dive *merge_two_dives(struct dive *a, struct dive *b)
 	if (!res)
 		return NULL;
 
-	factor = (a->number == 0 || b->number == 0) ? 0 : abs(b->number - a->number);
+	/*
+	 * If 'a' and 'b' were numbered, and in proper order,
+	 * then the resulting dive will get the first number,
+	 * and the subsequent dives will be renumbered by the
+	 * difference.
+	 *
+	 * So if you had a dive list  1 3 6 7 8, and you
+	 * merge 1 and 3, the resulting numbered list will
+	 * be 1 4 5 6, because we assume that there were
+	 * some missing dives (originally dives 4 and 5),
+	 * that now will still be missing (dives 2 and 3
+	 * in the renumbered world).
+	 *
+	 * Obviously the normal case is that everything is
+	 * consecutive, and the difference will be 1, so the
+	 * above example is not supposed to be normal.
+	 */
+	nrdiff = 0;
+	nr = a->number;
+	if (a->number && b->number > a->number) {
+		res->number = nr;
+		nrdiff = b->number - nr;
+	}
+
 	add_single_dive(i, res);
 	delete_single_dive(i + 1);
 	delete_single_dive(j);
@@ -865,9 +892,25 @@ struct dive *merge_two_dives(struct dive *a, struct dive *b)
 
 	// renumber dives from merged one in advance by difference between
 	// merged dives numbers. Do not renumber if actual number is zero.
-	for (; j < dive_table.nr; j++)
-		if (dive_table.dives[j]->number != 0)
-			dive_table.dives[j]->number -= factor;
+	for (; j < dive_table.nr; j++) {
+		struct dive *dive = dive_table.dives[j];
+		int newnr;
+
+		if (!dive->number)
+			continue;
+		newnr = dive->number - nrdiff;
+
+		/*
+		 * Don't renumber stuff that isn't in order!
+		 *
+		 * So if the new dive number isn't larger than the
+		 * previous dive number, just stop here.
+		 */
+		if (newnr <= nr)
+			break;
+		dive->number = newnr;
+		nr = newnr;
+	}
 
 	mark_divelist_changed(true);
 	return res;