Re: [PATCH] add -p: introduce 'w' command to view hunk with --word-diff

[Phillip Wood <phillip.wood123@gmail.com>]

On 07/05/2026 08:55, Pablo wrote:
> El jue, 7 may 2026 a las 1:58, Javier Bassi (<javierbassi@gmail.com>) escribió:
>>
>> +static void add_word_diff_line(struct strbuf *old, struct strbuf *new,
>> +                              const char *line, size_t len, char marker)
>> +{
>> +       if (marker == '-' || marker == '+' || *line == ' ') {
>> +               line++;
>> +               len--;
>> +       }
> 
> Maybe a tiny comment here would help, to know why '*line' is being
> checked here instead of 'marker'. They seem the same and one has to go
> to marker declaration and see the comment at 'normalize_marker()'
> 
>    /* Empty context lines may omit the leading ' ' */

That's a good point - it might be clearer to use

	if (marker == *line) {
		line++;
		len--;
	}

instead. That also trims lines starting with '\' but that shouldn't 
matter as the code should be checking "marker" rather than "line".

>> +
>> +               if (marker == '\\') {
>> +                       if (last_marker != '+')
>> +                               trim_trailing_lf(old);
>> +                       if (last_marker != '-')
>> +                               trim_trailing_lf(new);
>> +                       continue;
>> +               }
> 
> Here we check about "\No newline at end of file", after this point I
> believe that 'buf->buf[buf->len - 1] == '\n'' will always be true.
> Same should be for 'buf->len' because "\No newline at end of file"
> shouldn't come first and a '+' '-' line should have been added on a
> previous iteration, but the check it's fine, just in case I'm wrong.
> 
> What I want to point out is, is the 'trim_trailing_lf' function
> necessary? It's only called in the same place and it carries a check
> that could be on the caller instead, leaving  the function only with
> 'strbuf_setlen(buf, buf->len - 1);" making sense to inline it at this
> point.
> You could keep the buf->len check:
> 
>    if (marker == '\\') {
>            if (last_marker != '+' && old->len)
>                     strbuf_setlen(old, old->len - 1);
>            if (last_marker != '-' && new->len)
>                     strbuf_setlen(new, new->len - 1);
>            continue;
>    }

Should we be trimming '\r\n' if the file has dos style line endings?

Thanks

Phillip