[RFC,4/4] range-diff: add section headers to the outer hunk header
diff mbox series

Message ID 20190414210933.20875-5-t.gummerer@gmail.com
State New
Headers show
Series
  • output improvements for git range-diff
Related show

Commit Message

Thomas Gummerer April 14, 2019, 9:09 p.m. UTC
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>
---
 range-diff.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Johannes Schindelin April 15, 2019, 12:44 p.m. UTC | #1
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.

> 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
>
>
Thomas Gummerer April 15, 2019, 6:48 p.m. UTC | #2
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.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 ;)

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

Patch
diff mbox series

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");
 			} 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;
 }