From: Mark Jamsek <mark@jamsek.com>
Subject: Re: fix tog diff between arbitrary commits
To: gameoftrees@openbsd.org
Date: Wed, 22 Feb 2023 21:37:10 +1100

On 23-02-22 10:07AM, Stefan Sperling wrote:
> 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 :)

Thank you :)

> 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.

Ouch! Thanks for sharing this experience. I can certainly appreciate how
it would've felt, and I'm glad you're able to relax more now. I imagine
you might be your own worst critic like I am to myself. I'm super quick
to castigate myself but awfully slow to forgive myself when I make
mistakes!

> > > 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.

I thought as much.

Thanks again for fixing this, and also for being so cool about it :)

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