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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Apr 15, 2019, 18:48 UTC
Message-ID: <20190415184801.GA1704@hank.intra.tgummerer.com>
In-Reply-To: <nycvar.QRO.7.76.6.1904151442150.44@tvgsbejvaqbjf.bet>

On 04/15, Johannes Schindelin wrote:

Show 24 quoted lines

> 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@gmail.com>
> 
> That makes a total lot of sense. So much so that we should protect it by a
> new regression test case.

Yup, definitely. As mentioned in the cover letter, I punted on to discuss and settle on an output format first, so I don't have to duplicate a lot of work writing and re-writing the tests ;)

Show 14 quoted lines

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

I don't think so. The commit message counts as part of in_header, and starts right after the rest of the commit metadata. If we would add this after when we set in_header to false, we'd write it after the commit message, which would make the hunk header "@@ ... @@ Metadata", even when we show the commit message in the output.

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

Thanks!

Show 30 quoted lines

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