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

[Thomas Gummerer]

On 04/15, Johannes Schindelin wrote:
> 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.gumme...@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 ;)

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

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