Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> When postprocessing the inner diff in range-diff, we currently replace
> the whole hunk header line with just "@@". This matches how 'git
> tbdiff' used to handle hunk headers as well.
>
> Most likely this is being done because line numbers in the hunk header
> are not relevant without other changes. They can for example easily
> change if a range is rebased, and lines are added/removed before a
> change that we actually care about in our ranges.
>
> However it can still be useful to have the function name that 'git
> diff' extracts as additional context for the change.
>
> Note that it is not guaranteed that the hunk header actually shows up
> in the range-diff, and this change only aims to improve the case where
> a hunk header would already be included in the final output.
Makes sense.
> diff --git a/range-diff.c b/range-diff.c
> index 9242b8975f..f365141ade 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> strbuf_addch(&buf, '\n');
> }
> continue;
> - } else if (starts_with(line.buf, "@@ "))
> - strbuf_addstr(&buf, "@@");
> - else if (!line.buf[0] || starts_with(line.buf, "index "))
> + } else if (starts_with(line.buf, "@@ ")) {
> + char *skip_lineno = strstr(line.buf + 3, "@@");
Rather than using the magic constant "3", it would probably make sense to
declare `skip_lineno` outside of the `if` construct, and use
`skip_prefix(line.buf, "@@ ", &skip_lineno)` instead of
`starts_with(...)`.
We *will*, however, want to have a safeguard against `strstr()` not
finding anything. Maybe re-use the `p` variable that we already have, and
do this instead:
} else if (skip_prefix(line.buf, "@@ ", &p) &&
(p = strstr(p, "@@"))) {
> + strbuf_remove(&line, 0, skip_lineno - line.buf);
> + strbuf_addch(&buf, ' ');
Shorter:
strbuf_splice(&line, 0, p - line.buf, " ", 1);
(assuming that you accept my suggestion to use `p` instead of
`skip_lineno`...)
Thanks,
Dscho
> + strbuf_addbuf(&buf, &line);
> + } else if (!line.buf[0] || starts_with(line.buf, "index "))
> /*
> * A completely blank (not ' \n', which is context)
> * line is not valid in a diff. We skip it
> --
> 2.21.0.593.g511ec345e1
>
>
