From: Stefan Sperling <stsp@stsp.name>
Subject: Re: diff: handle missing new lines in trailing context
To: Tom Jones <thj@freebsd.org>
Cc: gameoftrees@openbsd.org
Date: Thu, 29 Sep 2022 12:24:22 +0200

On Thu, Sep 29, 2022 at 10:46:35AM +0100, Tom Jones wrote:
> On Thu, Sep 29, 2022 at 11:35:45AM +0200, Stefan Sperling wrote:
> > On Thu, Sep 29, 2022 at 10:00:37AM +0100, Tom Jones wrote:
> > > Hi folks,
> > > 
> > > If there is a missing new line in the context we currently miss it and
> > > fail to print the warning. This trips up patch and leads to failures.
> > > 
> > > This patch uses the chunk at result->chunks.head[cc->chunk.end] for
> > > checking the final atom. I am not sure if can assume there is a chunk
> > > there, but I haven't seen another way to access this correctly.
> > 
> > Using chunk.end as an index will end up indexing one item beyond the
> > end of the array.
> > 
> > Does your test still work when checking at index chunk.end - 1 instead?
> 
> This seems obviously wrong to me too, but there is another chunk there,
> but not accounted for. 
> 
> Sadly checking chunk.end -1 doesn't handle this. If it did the check in
> the loop would handle this case. This block after the loop might be
> suspect, my tests show that result.len is 1 more than chunk.end.
> 
> I didn't figure out why we end up trailing context as atoms rather than
> a chunk, looking in a debugger shows a correct chunk there. Is there a
> reason why?

Well, our loops certainly take great care not to index result->chunks.head
in the way you are proposing. For example:

        for (c_idx = cc->chunk.start; c_idx < cc->chunk.end; c_idx++) {
                const struct diff_chunk *c = &result->chunks.head[c_idx];

This keeps c_idx < chunk.end, not <= chunk.end.

Can you show what you are seeing in the debugger? What are the contents
of the final chunk? In particular, what is that chunk's type?

It seems I wrote the code for diff_output_trailing_newline_msg() but I
don't remember at all why it works the way it does... :-/