Re: [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header

[Johannes Schindelin <Johannes.Schindelin@xxxxxx>]

Hi Thomas,

On Sun, 14 Apr 2019, Thomas Gummerer wrote:

> Add the section headers we introduced in the previous commit to the
> outer diff's hunk headers.  This makes it easier to understand which
> change we are actually looking at.  For example an outer hunk header
> might now look like:
>
>     @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt
>
> while previously it would have only been
>
>     @@ -77,15 +78,43 @@
>
> which doesn't give a lot of context for the change that follows.
>
> For completeness also add section headers for the commit metadata and
> the commit message, although they are arguably less important.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>

That makes a total lot of sense. So much so that we should protect it by a
new regression test case.

> diff --git a/range-diff.c b/range-diff.c
> index aa466060ef..15ac54f369 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)
>  			strbuf_addstr(&buf, " ##\n");
>  		} else if (in_header) {
>  			if (starts_with(line.buf, "Author: ")) {
> +				strbuf_addstr(&buf, " ## Metadata ##\n");
>  				strbuf_addbuf(&buf, &line);
>  				strbuf_addstr(&buf, "\n\n");
> +				strbuf_addstr(&buf, " ## Commit message ##\n");

Should this not rather be added when `in_header` is set to false?

The rest of the patch makes a whole lot of sense to me.

Thanks,
Dscho

>  			} else if (starts_with(line.buf, "    ")) {
>  				strbuf_rtrim(&line);
>  				strbuf_addbuf(&buf, &line);
> @@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,
>  	fwrite(buf->buf, buf->len, 1, diffopt->file);
>  }
>
> -static struct userdiff_driver no_func_name = {
> -	.funcname = { "$^", 0 }
> +static struct userdiff_driver section_headers = {
> +	.funcname = { "^ ## (.*) ##$", REG_EXTENDED }
>  };
>
>  static struct diff_filespec *get_filespec(const char *name, const char *p)
> @@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
>  	spec->size = strlen(p);
>  	spec->should_munmap = 0;
>  	spec->is_stdin = 1;
> -	spec->driver = &no_func_name;
> +	spec->driver = &section_headers;
>
>  	return spec;
>  }
> --
> 2.21.0.593.g511ec345e1
>
>