From: Stefan Sperling Subject: Re: fix tog diff between arbitrary commits To: Mark Jamsek Cc: gameoftrees@openbsd.org Date: Wed, 22 Feb 2023 10:07:43 +0100 On Wed, Feb 22, 2023 at 04:17:17PM +1100, Mark Jamsek wrote: > 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 :/ No worries, mistakes happen. It could be worse :) I once broke the boot loader on sparc64 .iso install media and as a result one particular release of OpenBSD ended up shipping CD-ROMs which could not boot sparc64 machines. Nowadays there are no OpenBSD release CDs anymore and that helps me relax again. > > 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. Makes sense. Thanks for the revised fix. ok stsp@ The redundant *d_nlines check was just a left-over from early edits I forgot to remove again. > 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 > GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68 >