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