On 23-02-21 10:25PM, Stefan Sperling wrote:
> The cat_file() function in tog is unprepared for diffs between
> commits that do not have a direct parent link in history.

Oops, that's a really bad mistake! Sorry :/

> To reproduce in the got.git repo, run:
> 
> $ tog diff 249b637c main
> tog: fseek: Invalid argument
> $
> 
> tog will either show an empty diff or fail as above with an error
> from fseek when it tries to use a bogus line offset.
> 
> Line offsets for commit info are only created when diffing a commit
> against its direct parent. Otherwise the value of *d_nlines will be zero.
> But cat_file() will unconditionally use *d_nlines - 1 as an index into
> the line offsets array to copy from. This ends up messing up the line
> offsets array that is used when the diff gets rendered in draw_file().
> 
> Is this fix correct? Basically, this function somehow needs to deal
> with zero-length commit info and/or zero-length diff text.

Thanks, Stefan. The fix is correct, but we also need to account for
line/offset 0 as explained below.

The function cats the tmp_diff_file (src) to s->f (dst), the latter of
which was stupidly assumed to always have commit info already. (I'm
still kicking myself for forgetting about such diffs!)

In the case we have commit info (d_nlines > 0), we need to increase each
line offset of the diff content in src (s_lines[1..s_nlines-1].offset)
by the bytes of commit info already in dst (d_lines[d_nlines - 1].offset).
Otherwise scrolling will be out of wack.

We skip index 0 as both s_lines[0] and d_lines[0] may already be zero,
(see lib/diff.c:151:diff_blobs() and tog/tog.c:4637:write_commit_info(),
respectively). As such, if we unconditionally copied this line, in the
case where we do have commit info (they are both 0), it would result in:

  s->lines[d_nlines - 1].offset == s->_lines[d_nlines].offset

This would be incorrect and also cause scrolling issues..

If there's no commit info (d_nlines == 0), though, we don't want to make
this adjustment, so we copy all of s_lines (diff content line type and
offset) to d_lines (s->lines).

With that in mind, the below diff tweaks yours a bit by skipping index
0 if we have commit info, and removes the extra `*d_nlines > 0` check
from the for loop's termination condition because this loop is guarded
by the same check.  It also tweaks the existing comment in an attempt to
make this simpler to grok.

Thanks again for finding and fixing this :)

diff /home/mark/src/got
commit - fd8d60a2d11af314daec9c6c7ad0ea5c7ac0abd0
path + /home/mark/src/got
blob - 0d706f1c0a5161fd7a132b213b89c64e4cd17da0
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -4578,14 +4578,23 @@ cat_diff(FILE *dst, FILE *src, struct got_diff_line **
 			return got_ferror(dst, GOT_ERR_IO);
 	}
 
+	if (s_nlines == 0 && *d_nlines == 0)
+		return NULL;
+
 	/*
-	 * The diff driver initialises the first line at offset zero when the
-	 * array isn't prepopulated, skip it; we already have it in *d_lines.
+	 * If commit info was in dst, increment line offsets
+	 * of the appended diff content, but skip s_lines[0]
+	 * because offset zero is already in *d_lines.
 	 */
-	for (i = 1; i < s_nlines; ++i)
-		s_lines[i].offset += (*d_lines)[*d_nlines - 1].offset;
+	if (*d_nlines > 0) {
+		for (i = 1; i < s_nlines; ++i)
+			s_lines[i].offset += (*d_lines)[*d_nlines - 1].offset;
 
-	--s_nlines;
+		if (s_nlines > 0) {
+			--s_nlines;
+			++s_lines;
+		}
+	}
 
 	p = reallocarray(*d_lines, *d_nlines + s_nlines, sizeof(*p));
 	if (p == NULL) {
@@ -4595,7 +4604,7 @@ cat_diff(FILE *dst, FILE *src, struct got_diff_line **
 
 	*d_lines = p;
 
-	memcpy(*d_lines + *d_nlines, s_lines + 1, s_nlines * sizeof(*s_lines));
+	memcpy(*d_lines + *d_nlines, s_lines, s_nlines * sizeof(*s_lines));
 	*d_nlines += s_nlines;
 
 	return NULL;

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68