Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 15, 2019, 12:53 UTC
Message-ID: <nycvar.QRO.7.76.6.1904151448530.44@tvgsbejvaqbjf.bet>
In-Reply-To: <20190414210933.20875-3-t.gummerer@gmail.com>

Hi Thomas,

On Sun, 14 Apr 2019, Thomas Gummerer wrote:

Show 15 quoted lines

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

Show 13 quoted lines

> 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

Show 9 quoted lines

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