incorrect range-diff output?

incorrect range-diff output?

[Duy Nguyen]

Try

    git range-diff from...to

with those two branches from https://gitlab.com/pclouds/git.git. The
interesting part is this

      diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
      --- a/Documentation/gitcli.txt
    @@ -120,10 +111,11 @@
        * linkgit:git-commit[1] to advance the current branch.
      
     -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
    -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
    -     pathname parameters) to undo changes.
    +-    pathname parameters) to undo changes.
    ++  * linkgit:git-restore[1] to undo changes.
      
        * linkgit:git-merge[1] to merge between local branches.
    + 

This particular hunk comes from giteveryday.txt, not gitcli.txt. And
the b/Documentation/gitcli.txt line is also missing.

I'm in the middle of some other things (and don't know range-diff that
well) so I'm just dropping a bug report here.

I've tried all 'master', 'next' and 'pu'. Same result. Tried
--no-pager too in case the pager ate something up.
--
Duy

Re: incorrect range-diff output?

[Thomas Gummerer]

On 04/11, Duy Nguyen wrote:
> Try
> 
>     git range-diff from...to
> 
> with those two branches from https://gitlab.com/pclouds/git.git. The
> interesting part is this
> 
>       diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
>       --- a/Documentation/gitcli.txt
>     @@ -120,10 +111,11 @@
>         * linkgit:git-commit[1] to advance the current branch.
>       
>      -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
>     -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
>     -     pathname parameters) to undo changes.
>     +-    pathname parameters) to undo changes.
>     ++  * linkgit:git-restore[1] to undo changes.
>       
>         * linkgit:git-merge[1] to merge between local branches.
>     + 
> 
> This particular hunk comes from giteveryday.txt, not gitcli.txt. And
> the b/Documentation/gitcli.txt line is also missing.

I think the output here is technically correct, even though it is very
misleading.  range-diff doesn't currently show the filenames of the
diff that changed, which makes this a bit hard to read.  To understand
what's happening here, let's have a look at the parts of the commits
where this happens:

In 66370c3f63 we have:

    diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
    index 837707a8fd..c38bc54439 100644
    --- a/Documentation/git-revert.txt
    +++ b/Documentation/git-revert.txt
    @@ -26,8 +26,8 @@ effect of some earlier commits (often only a faulty one).  If you want to
     throw away all uncommitted changes in your working directory, you
     should see linkgit:git-reset[1], particularly the `--hard` option.  If
     you want to extract specific files as they were in another commit, you
    -should see linkgit:git-checkout[1], specifically the `git checkout
    -<commit> -- <filename>` syntax.  Take care with these alternatives as
    +should see linkgit:git-restore[1], specifically the `--source`
    +option  Take care with these alternatives as
     both will discard uncommitted changes in your working directory.
 
     OPTIONS
    diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
    index 592e06d839..e2a9674297 100644
    --- a/Documentation/gitcli.txt
    +++ b/Documentation/gitcli.txt

While in 183c6c9390 we have:

    diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
    index 018ecf49d3..9aadc36881 100644
    --- a/Documentation/git-revert.txt
    +++ b/Documentation/git-revert.txt
    @@ -26,8 +26,8 @@ effect of some earlier commits (often only a faulty one).  If you want to
     throw away all uncommitted changes in your working directory, you
     should see linkgit:git-reset[1], particularly the `--hard` option.  If
     you want to extract specific files as they were in another commit, you
    -should see linkgit:git-checkout[1], specifically the `git checkout
    -<commit> -- <filename>` syntax.  Take care with these alternatives as
    +should see linkgit:git-restore[1], specifically the `--source`
    +option. Take care with these alternatives as
     both will discard uncommitted changes in your working directory.
     
     See "Reset, restore and revert" in linkgit:git[1] for the differences
    diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
    index 592e06d839..e2a9674297 100644
    --- a/Documentation/gitcli.txt
    +++ b/Documentation/gitcli.txt


So adding a bit more context to the bits you posted above we see:

    @@ -92,10 +83,10 @@
     -should see linkgit:git-checkout[1], specifically the `git checkout
     -<commit> -- <filename>` syntax.  Take care with these alternatives as
     +should see linkgit:git-restore[1], specifically the `--source`
    -+option  Take care with these alternatives as
    ++option. Take care with these alternatives as
      both will discard uncommitted changes in your working directory.
      
    - OPTIONS
    + See "Reset, restore and revert" in linkgit:git[1] for the differences
     
      diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
      --- a/Documentation/gitcli.txt
    @@ -120,10 +111,11 @@
        * linkgit:git-commit[1] to advance the current branch.
      
     -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
    -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
    -     pathname parameters) to undo changes.
    +-    pathname parameters) to undo changes.
    ++  * linkgit:git-restore[1] to undo changes.
      
        * linkgit:git-merge[1] to merge between local branches.
    + 
     @@
      ------------
      $ git switch -c alsa-audio <1>

So the Documentation/gitcli.txt bit is actually context on the first
diff, and everything after '@@ -120,10 +111,11 @@' is completely
unrelated to that.

I'm not sure what the right solution for this is.  I think one thing
I'd like range-diff to do is to add the filename, or some context
(e.g. is this part of the commit message etc.) to the @@ line (not
sure what that is called?).

Another thing that would help in this case would be to just remove the
diff header if it is only in the context of a diff, but not actually
changed.  We can't just remove it outright, as it could be useful in
some cases, e.g. when a file has just been re-named.

So all that said, while this is technically correct, I think it's
misleading enough that we should try to fix it.  Maybe I can find some
time over the weekend to tackle this, if nobody else gets to it first.

> I'm in the middle of some other things (and don't know range-diff that
> well) so I'm just dropping a bug report here.
> 
> I've tried all 'master', 'next' and 'pu'. Same result. Tried
> --no-pager too in case the pager ate something up.
> --
> Duy

Re: incorrect range-diff output?

[Johannes Schindelin]

Hi Thomas,

On Thu, 11 Apr 2019, Thomas Gummerer wrote:

> On 04/11, Duy Nguyen wrote:
> > Try
> >
> >     git range-diff from...to
> >
> > with those two branches from https://gitlab.com/pclouds/git.git. The
> > interesting part is this
> >
> >       diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> >       --- a/Documentation/gitcli.txt
> >     @@ -120,10 +111,11 @@
> >         * linkgit:git-commit[1] to advance the current branch.
> >
> >      -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
> >     -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
> >     -     pathname parameters) to undo changes.
> >     +-    pathname parameters) to undo changes.
> >     ++  * linkgit:git-restore[1] to undo changes.
> >
> >         * linkgit:git-merge[1] to merge between local branches.
> >     +
> >
> > This particular hunk comes from giteveryday.txt, not gitcli.txt. And
> > the b/Documentation/gitcli.txt line is also missing.
>
> I think the output here is technically correct, even though it is very
> misleading.  range-diff doesn't currently show the filenames of the
> diff that changed, which makes this a bit hard to read.

True. In the spirit of the "funcname" feature of our `git diff` command,
we could add some (abbreviated) form of the corresponding `diff` lines
(maybe just the `a/` filename? Or maybe the `a/` file name, prefixed by
`-` or `+`, and if the `b/` filename is different, `old->new`? With
`/dev/null` substituted by `(new)` or `(deleted)`?).

> [...]
>
> Maybe I can find some time over the weekend to tackle this, if nobody
> else gets to it first.

Good luck ;-)

Ciao,
Dscho

Re: incorrect range-diff output?

[Duy Nguyen]

On Fri, Apr 12, 2019 at 5:05 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> I'm not sure what the right solution for this is.  I think one thing
> I'd like range-diff to do is to add the filename, or some context
> (e.g. is this part of the commit message etc.) to the @@ line (not
> sure what that is called?).

I don't know what that "@@" line is called either but if possible
please keep those lines too. I quite often depend on them to know what
function I'm reading.
-- 
Duy

Re: incorrect range-diff output?

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> I'm not sure what the right solution for this is.  I think one thing
> I'd like range-diff to do is to add the filename, or some context
> (e.g. is this part of the commit message etc.) to the @@ line (not
> sure what that is called?).

Perhaps the inner diff can be done with the usual funcname/xfuncname
logic so that the real function name of the payload is on the @@
lines (they are internally called "hunk header" lines, I think).

And then the outer diff (i.e. the one that compares two inner diff
output) can use a special funcname pattern that says "treat the
lines that begin with '@@ ' as the function name line", instead of
(or in addition to??) the logic that says "lines that begin with
'diff --git' have interesting information" which led to a misleading
piece of information in the range-diff output under discussion,
perhaps?

[RFC PATCH 0/4] output improvements for git range-diff

[Thomas Gummerer]

This series aims at improving the output of 'git range-diff',
especially in cases like the one mentioned in [*1*], where a diff
header, that was before the outer diff's hunk header was
misinterpreted as being the filename of the diff that followed it.

It does so by substituting the diff header of the inner diff with a
range-diff specific "section header", which is then parsed by a
userdiff pattern, so it's included in the outer diff's hunk header.

Additionally, instead of stripping out the whole hunk header of the
inner diff, and replacing it by just an "@@", only strip the line
numbers from that hunk header, but keep the funcname/xfuncname.

This information is not added to the outer diff's hunk header, because
it feels like that would end up in too much information, and I feel
like the filename is the more useful bit of information if we have to
pick one of them.

As an example, here's what the relevant part of the range-diff from
the original thread would look like now:

    @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt
     -should see linkgit:git-checkout[1], specifically the `git checkout
     -<commit> -- <filename>` syntax.  Take care with these alternatives as
     +should see linkgit:git-restore[1], specifically the `--source`
    -+option  Take care with these alternatives as
    ++option. Take care with these alternatives as
      both will discard uncommitted changes in your working directory.
      
    - OPTIONS
    + See "Reset, restore and revert" in linkgit:git[1] for the differences
     
     
     
    @@ -129,10 +120,11 @@ modified file Documentation/giteveryday.txt
        * linkgit:git-commit[1] to advance the current branch.
      
     -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
    -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
    -     pathname parameters) to undo changes.
    +-    pathname parameters) to undo changes.
    ++  * linkgit:git-restore[1] to undo changes.
      
        * linkgit:git-merge[1] to merge between local branches.
    + 
      @@ Create a topic branch and develop.::
      ------------
      $ git switch -c alsa-audio <1>

Note that this patch series doesn't modify or add any tests, and was
just manually tested locally, thus it is still marked as RFC.  I'd
love to get some feedback on the output format first, before touching
any tests, as I expect there might be some discussion on the final
format.

*1*: <20190411111729.GB5620@ash>

This series can also be fetched from
https://github.com/tgummerer/git tg/range-diff-output

Thomas Gummerer (4):
  range-diff: fix function parameter indentation
  range-diff: don't remove funcname from inner diff
  range-diff: add section header instead of diff header
  range-diff: add section headers to the outer hunk header

 range-diff.c | 55 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 10 deletions(-)

-- 
2.21.0.593.g511ec345e1

[RFC PATCH 1/4] range-diff: fix function parameter indentation

[Thomas Gummerer]

Fix the indentation of the function parameters for a couple of
functions, to match the style in the rest of the file.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 range-diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 48b0e1b4ce..9242b8975f 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -148,7 +148,7 @@ static int read_patches(const char *range, struct string_list *list)
 }
 
 static int patch_util_cmp(const void *dummy, const struct patch_util *a,
-		     const struct patch_util *b, const char *keydata)
+			  const struct patch_util *b, const char *keydata)
 {
 	return strcmp(a->diff, keydata ? keydata : b->diff);
 }
@@ -373,7 +373,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
 }
 
 static void patch_diff(const char *a, const char *b,
-			      struct diff_options *diffopt)
+		       struct diff_options *diffopt)
 {
 	diff_queue(&diff_queued_diff,
 		   get_filespec("a", a), get_filespec("b", b));
-- 
2.21.0.593.g511ec345e1

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

[Thomas Gummerer]

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.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 range-diff.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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, "@@");
+			strbuf_remove(&line, 0, skip_lineno - line.buf);
+			strbuf_addch(&buf, ' ');
+			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

[RFC PATCH 3/4] range-diff: add section header instead of diff header

[Thomas Gummerer]

Currently range-diff keeps the diff header of the inner diff
intact (apart from stripping lines starting with index).  This diff
header is somewhat useful, especially when files get different
names in different ranges.

However there is no real need to keep the whole diff header for that.
The main reason we currently do that is probably because it is easy to
do.

Introduce a new range diff hunk header, that's enclosed by "##",
similar to how line numbers in diff hunks are enclosed by "@@", and
give human readable information of what exactly happened to the file,
including the file name.

At this point, this is only a marginal improvement in readability of
the range-diff output.  More interestingly however, this allows us to
add these range diff hunk headers to the outer diffs hunk headers
using a custom userdiff pattern, which should help making the
range-diff more readable.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 range-diff.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index f365141ade..aa466060ef 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -33,6 +33,7 @@ static int read_patches(const char *range, struct string_list *list)
 	struct child_process cp = CHILD_PROCESS_INIT;
 	FILE *in;
 	struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
+	struct strbuf filename_a = STRBUF_INIT;
 	struct patch_util *util = NULL;
 	int in_header = 1;
 
@@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, '\n');
 			if (!util->diff_offset)
 				util->diff_offset = buf.len;
-			strbuf_addch(&buf, ' ');
-			strbuf_addbuf(&buf, &line);
+		} else if (starts_with(line.buf, "--- ")) {
+			if (!strcmp(line.buf, "--- /dev/null"))
+				strbuf_remove(&line, 0, 4);
+			else
+				strbuf_remove(&line, 0, 6);
+			strbuf_rtrim(&line);
+			strbuf_reset(&filename_a);
+			strbuf_addbuf(&filename_a, &line);
+		} else if (starts_with(line.buf, "+++ ")) {
+			strbuf_addstr(&buf, " ## ");
+			if (!strcmp(line.buf, "--- /dev/null"))
+				strbuf_remove(&line, 0, 4);
+			else
+				strbuf_remove(&line, 0, 6);
+			strbuf_rtrim(&line);
+			if (!strcmp(filename_a.buf, "/dev/null")) {
+				strbuf_addstr(&buf, "new file ");
+				strbuf_addbuf(&buf, &line);
+			} else if (!strcmp(line.buf, "/dev/null")) {
+				strbuf_addstr(&buf, "removed file ");
+				strbuf_addbuf(&buf, &line);
+			} else if (strbuf_cmp(&filename_a, &line)) {
+				strbuf_addstr(&buf, "renamed file ");
+				strbuf_addbuf(&buf, &filename_a);
+				strbuf_addstr(&buf, " -> ");
+				strbuf_addbuf(&buf, &line);
+			} else {
+				strbuf_addstr(&buf, "modified file ");
+				strbuf_addbuf(&buf, &line);
+			}
+			strbuf_addstr(&buf, " ##\n");
 		} else if (in_header) {
 			if (starts_with(line.buf, "Author: ")) {
 				strbuf_addbuf(&buf, &line);
-- 
2.21.0.593.g511ec345e1

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

[Thomas Gummerer]

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

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;
 }
-- 
2.21.0.593.g511ec345e1

Re: incorrect range-diff output?

[Thomas Gummerer]

On 04/12, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Thu, 11 Apr 2019, Thomas Gummerer wrote:
> 
> > On 04/11, Duy Nguyen wrote:
> > > Try
> > >
> > >     git range-diff from...to
> > >
> > > with those two branches from https://gitlab.com/pclouds/git.git. The
> > > interesting part is this
> > >
> > >       diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> > >       --- a/Documentation/gitcli.txt
> > >     @@ -120,10 +111,11 @@
> > >         * linkgit:git-commit[1] to advance the current branch.
> > >
> > >      -  * linkgit:git-reset[1] and linkgit:git-checkout[1] (with
> > >     -+  * linkgit:git-reset[1] and linkgit:git-restore[1] (with
> > >     -     pathname parameters) to undo changes.
> > >     +-    pathname parameters) to undo changes.
> > >     ++  * linkgit:git-restore[1] to undo changes.
> > >
> > >         * linkgit:git-merge[1] to merge between local branches.
> > >     +
> > >
> > > This particular hunk comes from giteveryday.txt, not gitcli.txt. And
> > > the b/Documentation/gitcli.txt line is also missing.
> >
> > I think the output here is technically correct, even though it is very
> > misleading.  range-diff doesn't currently show the filenames of the
> > diff that changed, which makes this a bit hard to read.
> 
> True. In the spirit of the "funcname" feature of our `git diff` command,
> we could add some (abbreviated) form of the corresponding `diff` lines
> (maybe just the `a/` filename? Or maybe the `a/` file name, prefixed by
> `-` or `+`, and if the `b/` filename is different, `old->new`? With
> `/dev/null` substituted by `(new)` or `(deleted)`?).

Yeah something like this is what I ended up doing.  Except I was a bit
more verbose, so the filename would be prefixed with "modified file",
"new file", "deleted file" or "renamed file".  Since this doesn't need
to be machine readable in any way, I felt like that might be easiest
to consume for humans. 

Re: incorrect range-diff output?

[Thomas Gummerer]

On 04/13, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > I'm not sure what the right solution for this is.  I think one thing
> > I'd like range-diff to do is to add the filename, or some context
> > (e.g. is this part of the commit message etc.) to the @@ line (not
> > sure what that is called?).
> 
> Perhaps the inner diff can be done with the usual funcname/xfuncname
> logic so that the real function name of the payload is on the @@
> lines (they are internally called "hunk header" lines, I think).

The inner diff is actually currently done this way, however we then
strip out the funcname.  My understanding is that that is mostly done
to get rid of the line numbers which can change between ranges, so I'm
re-introducing that, while only stripping the actual line numbers
out.

I do feel like the filenames should probably have priority in the
outer diffs hunk header lines though, as we can only add a limited
amount of information there, and without the filename, the function
name might be less useful (or it may be not useful at all, for things
where we don't have proper funcnames, e.g. for documentation).

> And then the outer diff (i.e. the one that compares two inner diff
> output) can use a special funcname pattern that says "treat the
> lines that begin with '@@ ' as the function name line", instead of
> (or in addition to??) the logic that says "lines that begin with
> 'diff --git' have interesting information" which led to a misleading
> piece of information in the range-diff output under discussion,
> perhaps?

I don't think there ever was a logic that says "lines that begin with
'diff --git' have interesting information", but rather that's how the
range-diff was interpreted, because there was no indication that those
lines don't have interesting information.

My series implements something very similar to  what you're describing
though, except I'm introducing a new type of header for the filename,
as I think that's more important, as mentioned above.

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

[Eric Sunshine]

On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> However it can still be useful to have the function name that 'git
> diff' extracts as additional context for the change.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> +               } else if (starts_with(line.buf, "@@ ")) {
> +                       char *skip_lineno = strstr(line.buf + 3, "@@");
> +                       strbuf_remove(&line, 0, skip_lineno - line.buf);

It makes me a bit uncomfortable that this is not checking for NULL
return from strstr() before doing pointer arithmetic (even though the
input is presumably machine-generated).

    if (!skip_lineno)
        BUG(...);

might be appropriate.

> +                       strbuf_addch(&buf, ' ');
> +                       strbuf_addbuf(&buf, &line);

Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header

[Eric Sunshine]

On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> [...]
> Introduce a new range diff hunk header, that's enclosed by "##",
> similar to how line numbers in diff hunks are enclosed by "@@", and
> give human readable information of what exactly happened to the file,
> including the file name.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> +               } else if (starts_with(line.buf, "--- ")) {
> +                       if (!strcmp(line.buf, "--- /dev/null"))
> +                               strbuf_remove(&line, 0, 4);
> +                       else
> +                               strbuf_remove(&line, 0, 6);
> +                       strbuf_rtrim(&line);
> +                       strbuf_reset(&filename_a);
> +                       strbuf_addbuf(&filename_a, &line);
> +               } else if (starts_with(line.buf, "+++ ")) {

At this point, we know that line.buf starts with "+++"...

> +                       strbuf_addstr(&buf, " ## ");
> +                       if (!strcmp(line.buf, "--- /dev/null"))

so, it seems unlikely that it's ever going to match "--- /dev/null".

> +                               strbuf_remove(&line, 0, 4);
> +                       if (!strcmp(filename_a.buf, "/dev/null")) {
> +                               strbuf_addstr(&buf, "new file ");
> +                               strbuf_addbuf(&buf, &line);
> +                       } else if (!strcmp(line.buf, "/dev/null")) {
> +                               strbuf_addstr(&buf, "removed file ");
> +                               strbuf_addbuf(&buf, &line);
> +                       } else if (strbuf_cmp(&filename_a, &line)) {
> +                               strbuf_addstr(&buf, "renamed file ");
> +                               strbuf_addbuf(&buf, &filename_a);
> +                               strbuf_addstr(&buf, " -> ");
> +                               strbuf_addbuf(&buf, &line);
> +                       } else {
> +                               strbuf_addstr(&buf, "modified file ");
> +                               strbuf_addbuf(&buf, &line);
> +                       }

All of these disposition strings end with "file", which seems
redundant. Short and sweet "new", "removed", "renamed", "modified"
provide just as much useful information.

Also, should these strings be localizable? Alternately, rather than
using prose to describe the disposition, perhaps do so symbolically
(thus universally), say with "+", "-", "->", "*" (or ""),
respectively?

Re: incorrect range-diff output?

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> I do feel like the filenames should probably have priority in the
> outer diffs hunk header lines though, as we can only add a limited
> amount of information there, and without the filename, the function
> name might be less useful (or it may be not useful at all, for things
> where we don't have proper funcnames, e.g. for documentation).

That's quite sensible.

Thanks.

Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header

[Johannes Sixt]

Am 15.04.19 um 01:29 schrieb Eric Sunshine:
> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> +                               strbuf_remove(&line, 0, 4);
>> +                       if (!strcmp(filename_a.buf, "/dev/null")) {
>> +                               strbuf_addstr(&buf, "new file ");
>> +                               strbuf_addbuf(&buf, &line);
>> +                       } else if (!strcmp(line.buf, "/dev/null")) {
>> +                               strbuf_addstr(&buf, "removed file ");
>> +                               strbuf_addbuf(&buf, &line);
>> +                       } else if (strbuf_cmp(&filename_a, &line)) {
>> +                               strbuf_addstr(&buf, "renamed file ");
>> +                               strbuf_addbuf(&buf, &filename_a);
>> +                               strbuf_addstr(&buf, " -> ");
>> +                               strbuf_addbuf(&buf, &line);
>> +                       } else {
>> +                               strbuf_addstr(&buf, "modified file ");
>> +                               strbuf_addbuf(&buf, &line);
>> +                       }
> 
> All of these disposition strings end with "file", which seems
> redundant. Short and sweet "new", "removed", "renamed", "modified"
> provide just as much useful information.
> 
> Also, should these strings be localizable? Alternately, rather than
> using prose to describe the disposition, perhaps do so symbolically
> (thus universally), say with "+", "-", "->", "*" (or ""),
> respectively?
When the strings are translated, it would be preferable to not do this
sentence lego at all and have format strings that are filled with the
values.

Then the noise word "file" is probably not that bad, in particular, when
translators tend to not add words that they don't see in the original.
For example, German translations of "new %s" I anticipate "neues %s",
"%s (neu)", "neu %s", all of which are awkward, but translations of "new
file %s" lends itself naturally to "neue Datei %s", which is fine.

-- Hannes

Re: incorrect range-diff output?

[Johannes Schindelin]

Hi Junio,

On Sat, 13 Apr 2019, Junio C Hamano wrote:

> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > I'm not sure what the right solution for this is.  I think one thing
> > I'd like range-diff to do is to add the filename, or some context
> > (e.g. is this part of the commit message etc.) to the @@ line (not
> > sure what that is called?).
>
> Perhaps the inner diff can be done with the usual funcname/xfuncname
> logic so that the real function name of the payload is on the @@
> lines (they are internally called "hunk header" lines, I think).
>
> And then the outer diff (i.e. the one that compares two inner diff
> output) can use a special funcname pattern that says "treat the
> lines that begin with '@@ ' as the function name line", instead of
> (or in addition to??) the logic that says "lines that begin with
> 'diff --git' have interesting information" which led to a misleading
> piece of information in the range-diff output under discussion,
> perhaps?

Note that the inner hunk headers are often not even in the outer hunks.

So I still think that it would make most sense to populate the outer hunk
headers with useful information.

Ciao,
Dscho

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

[Johannes Schindelin]

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

Re: [RFC PATCH 0/4] output improvements for git range-diff

[Johannes Schindelin]

Hi Thomas,

On Sun, 14 Apr 2019, Thomas Gummerer wrote:

>     @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt

A better example might be a .c file, as the function name is often a
pretty useful piece of information.

Read: I think it should be part of the outer hunk header.

Also, the text "modified file" takes up an awful lot of space. Maybe we do
not really need that information?

While at it, we could strip the line numbers, as this is not intended for
machine consumption, but for human consumption instead.

> [...]
> Note that this patch series doesn't modify or add any tests, and was
> just manually tested locally, thus it is still marked as RFC.

Oh, okay then ;-)

Thanks for working on this,
Dscho

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

[Johannes Schindelin]

Hi Thomas,


On Sun, 14 Apr 2019, Thomas Gummerer wrote:

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

> 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

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

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

[Johannes Schindelin]

Hi Eric,

On Sun, 14 Apr 2019, Eric Sunshine wrote:

> On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > However it can still be useful to have the function name that 'git
> > diff' extracts as additional context for the change.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/range-diff.c b/range-diff.c
> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > +               } else if (starts_with(line.buf, "@@ ")) {
> > +                       char *skip_lineno = strstr(line.buf + 3, "@@");
> > +                       strbuf_remove(&line, 0, skip_lineno - line.buf);
>
> It makes me a bit uncomfortable that this is not checking for NULL
> return from strstr() before doing pointer arithmetic (even though the
> input is presumably machine-generated).
>
>     if (!skip_lineno)
>         BUG(...);

Good point, but maybe we should not go so far as to declare this a bug,
and fall back to removing everything bug the initial two `at` characters
instead?

Thanks,
Dscho

>
> might be appropriate.
>
> > +                       strbuf_addch(&buf, ' ');
> > +                       strbuf_addbuf(&buf, &line);
>

Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header

[Johannes Schindelin]

Hi Eric,

On Sun, 14 Apr 2019, Eric Sunshine wrote:

> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > Introduce a new range diff hunk header, that's enclosed by "##",
> > similar to how line numbers in diff hunks are enclosed by "@@", and
> > give human readable information of what exactly happened to the file,
> > including the file name.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/range-diff.c b/range-diff.c
> > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> > +               } else if (starts_with(line.buf, "--- ")) {
> > +                       if (!strcmp(line.buf, "--- /dev/null"))
> > +                               strbuf_remove(&line, 0, 4);
> > +                       else
> > +                               strbuf_remove(&line, 0, 6);
> > +                       strbuf_rtrim(&line);
> > +                       strbuf_reset(&filename_a);
> > +                       strbuf_addbuf(&filename_a, &line);
> > +               } else if (starts_with(line.buf, "+++ ")) {
>
> At this point, we know that line.buf starts with "+++"...
>
> > +                       strbuf_addstr(&buf, " ## ");
> > +                       if (!strcmp(line.buf, "--- /dev/null"))
>
> so, it seems unlikely that it's ever going to match "--- /dev/null".
>
> > +                               strbuf_remove(&line, 0, 4);
> > +                       if (!strcmp(filename_a.buf, "/dev/null")) {
> > +                               strbuf_addstr(&buf, "new file ");
> > +                               strbuf_addbuf(&buf, &line);
> > +                       } else if (!strcmp(line.buf, "/dev/null")) {
> > +                               strbuf_addstr(&buf, "removed file ");
> > +                               strbuf_addbuf(&buf, &line);
> > +                       } else if (strbuf_cmp(&filename_a, &line)) {
> > +                               strbuf_addstr(&buf, "renamed file ");
> > +                               strbuf_addbuf(&buf, &filename_a);
> > +                               strbuf_addstr(&buf, " -> ");
> > +                               strbuf_addbuf(&buf, &line);
> > +                       } else {
> > +                               strbuf_addstr(&buf, "modified file ");
> > +                               strbuf_addbuf(&buf, &line);
> > +                       }
>
> All of these disposition strings end with "file", which seems
> redundant. Short and sweet "new", "removed", "renamed", "modified"
> provide just as much useful information.
>
> Also, should these strings be localizable?

I'd rather not.

> Alternately, rather than using prose to describe the disposition,
> perhaps do so symbolically (thus universally), say with "+", "-", "->",
> "*" (or ""), respectively?

Or maybe streamline the common case (modified) by *not* saying anything,
then? I.e.

	@@ Documentation/Makefile

for a modified file,

	@@ builtin/psuh.c (new)

for a new file,

	@@ git-add--interactive.perl (deleted)

for a removed one, and

	@@ builtin/serve.c -> t/helper/test-serve-v2.c

for a renamed one.

That should also give us a bit of wiggle room to append the function name
part of the inner hunk header, if any.

Ciao,
Dscho

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

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

[Thomas Gummerer]

On 04/15, Johannes Schindelin wrote:
> Hi Eric,
> 
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
> 
> > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > [...]
> > > However it can still be useful to have the function name that 'git
> > > diff' extracts as additional context for the change.
> > > [...]
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > > +               } else if (starts_with(line.buf, "@@ ")) {
> > > +                       char *skip_lineno = strstr(line.buf + 3, "@@");
> > > +                       strbuf_remove(&line, 0, skip_lineno - line.buf);
> >
> > It makes me a bit uncomfortable that this is not checking for NULL
> > return from strstr() before doing pointer arithmetic (even though the
> > input is presumably machine-generated).
> >
> >     if (!skip_lineno)
> >         BUG(...);
> 
> Good point, but maybe we should not go so far as to declare this a bug,
> and fall back to removing everything bug the initial two `at` characters
> instead?

I like declaring this a bug.  We are after all parsing
machine-generated output, that does come from git (which is why I
neglected the NULL checking in the first place).  If that second "@@"
is not there it's definitely a bug somewhere in the diff machinery,
I'd say.

Note that the "@@" also couldn't come from anywhere else, the diff
header has a well defined format and so does the metadata.  The diff
itself is prefixed with '<', '>' and '#' in this case, and the commit
message is also prefixed with four spaces.  So if this breaks
somewhere I'd rather hear about it loudly, than let users potentially
get wrong output because we missed something somewhere.

> >
> > might be appropriate.
> >
> > > +                       strbuf_addch(&buf, ' ');
> > > +                       strbuf_addbuf(&buf, &line);
> >

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

[Thomas Gummerer]

On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
> 
> 
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> > 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(...)`.

I like this suggestion.

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

And this is also much nicer, thanks!

> Thanks,
> Dscho
> 
> > +			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
> >
> >

Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header

[Thomas Gummerer]

On 04/15, Johannes Schindelin wrote:
> Hi Eric,
> 
> On Sun, 14 Apr 2019, Eric Sunshine wrote:
> 
> > On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > [...]
> > > Introduce a new range diff hunk header, that's enclosed by "##",
> > > similar to how line numbers in diff hunks are enclosed by "@@", and
> > > give human readable information of what exactly happened to the file,
> > > including the file name.
> > > [...]
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)
> > > +               } else if (starts_with(line.buf, "--- ")) {
> > > +                       if (!strcmp(line.buf, "--- /dev/null"))
> > > +                               strbuf_remove(&line, 0, 4);
> > > +                       else
> > > +                               strbuf_remove(&line, 0, 6);
> > > +                       strbuf_rtrim(&line);
> > > +                       strbuf_reset(&filename_a);
> > > +                       strbuf_addbuf(&filename_a, &line);
> > > +               } else if (starts_with(line.buf, "+++ ")) {
> >
> > At this point, we know that line.buf starts with "+++"...
> >
> > > +                       strbuf_addstr(&buf, " ## ");
> > > +                       if (!strcmp(line.buf, "--- /dev/null"))
> >
> > so, it seems unlikely that it's ever going to match "--- /dev/null".

Ouch yup, this is some bad copy pasta, thanks for catching!

> > > +                               strbuf_remove(&line, 0, 4);
> > > +                       if (!strcmp(filename_a.buf, "/dev/null")) {
> > > +                               strbuf_addstr(&buf, "new file ");
> > > +                               strbuf_addbuf(&buf, &line);
> > > +                       } else if (!strcmp(line.buf, "/dev/null")) {
> > > +                               strbuf_addstr(&buf, "removed file ");
> > > +                               strbuf_addbuf(&buf, &line);
> > > +                       } else if (strbuf_cmp(&filename_a, &line)) {
> > > +                               strbuf_addstr(&buf, "renamed file ");
> > > +                               strbuf_addbuf(&buf, &filename_a);
> > > +                               strbuf_addstr(&buf, " -> ");
> > > +                               strbuf_addbuf(&buf, &line);
> > > +                       } else {
> > > +                               strbuf_addstr(&buf, "modified file ");
> > > +                               strbuf_addbuf(&buf, &line);
> > > +                       }
> >
> > All of these disposition strings end with "file", which seems
> > redundant. Short and sweet "new", "removed", "renamed", "modified"
> > provide just as much useful information.
> >
> > Also, should these strings be localizable?
> 
> I'd rather not.

Dunno, why do you think they should not be localizable?  I'm tend to
agree with Eric that they could be made localizable, after all this
output is not supposed to be machine readable either way.  I don't
have a strong opinion here though.

> > Alternately, rather than using prose to describe the disposition,
> > perhaps do so symbolically (thus universally), say with "+", "-", "->",
> > "*" (or ""), respectively?
> 
> Or maybe streamline the common case (modified) by *not* saying anything,
> then? I.e.
> 
> 	@@ Documentation/Makefile
> 
> for a modified file,
> 
> 	@@ builtin/psuh.c (new)
> 
> for a new file,
> 
> 	@@ git-add--interactive.perl (deleted)
> 
> for a removed one, and
> 
> 	@@ builtin/serve.c -> t/helper/test-serve-v2.c
> 
> for a renamed one.

This looks like a good suggestion to me, thanks!

> That should also give us a bit of wiggle room to append the function name
> part of the inner hunk header, if any.
> 
> Ciao,
> Dscho

Re: [RFC PATCH 0/4] output improvements for git range-diff

[Thomas Gummerer]

On 04/15, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Sun, 14 Apr 2019, Thomas Gummerer wrote:
> 
> >     @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt
> 
> A better example might be a .c file, as the function name is often a
> pretty useful piece of information.

Yeah, maybe with your suggestions, we could fit the function name or
some of the function name into the outer hunk header.  I'll give it a
try and see how it looks.

> Read: I think it should be part of the outer hunk header.
> 
> Also, the text "modified file" takes up an awful lot of space. Maybe we do
> not really need that information?
> 
> While at it, we could strip the line numbers, as this is not intended for
> machine consumption, but for human consumption instead.

Yeah, that makes sense, the line numbers are really kind of pointless
in a range-diff.

> > [...]
> > Note that this patch series doesn't modify or add any tests, and was
> > just manually tested locally, thus it is still marked as RFC.
> 
> Oh, okay then ;-)
> 
> Thanks for working on this,
> Dscho

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

[Johannes Schindelin]

Hi Thomas,

On Mon, 15 Apr 2019, Thomas Gummerer wrote:

> On 04/15, Johannes Schindelin wrote:
>
> > On Sun, 14 Apr 2019, Eric Sunshine wrote:
> >
> > > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > > [...]
> > > > However it can still be useful to have the function name that 'git
> > > > diff' extracts as additional context for the change.
> > > > [...]
> > > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > > ---
> > > > diff --git a/range-diff.c b/range-diff.c
> > > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)
> > > > +               } else if (starts_with(line.buf, "@@ ")) {
> > > > +                       char *skip_lineno = strstr(line.buf + 3, "@@");
> > > > +                       strbuf_remove(&line, 0, skip_lineno - line.buf);
> > >
> > > It makes me a bit uncomfortable that this is not checking for NULL
> > > return from strstr() before doing pointer arithmetic (even though the
> > > input is presumably machine-generated).
> > >
> > >     if (!skip_lineno)
> > >         BUG(...);
> >
> > Good point, but maybe we should not go so far as to declare this a bug,
> > and fall back to removing everything bug the initial two `at` characters
> > instead?
>
> I like declaring this a bug.  We are after all parsing
> machine-generated output, that does come from git (which is why I
> neglected the NULL checking in the first place).  If that second "@@"
> is not there it's definitely a bug somewhere in the diff machinery,
> I'd say.

Ah, but you do know about the micro-project I proposed to optionally feed
an mbox to `range-diff`, right?

The idea behind my proposal is that this would make it possible to
generate a range-diff between the patches on public-inbox and the commits
that actually made it into Junio's `pu`...

> Note that the "@@" also couldn't come from anywhere else, the diff
> header has a well defined format and so does the metadata.  The diff
> itself is prefixed with '<', '>' and '#' in this case, and the commit
> message is also prefixed with four spaces.  So if this breaks
> somewhere I'd rather hear about it loudly, than let users potentially
> get wrong output because we missed something somewhere.

Agreed. But I could imagine that `die()`ing here would be the more
appropriate way to holler loudly ;-)

Ciao,
Dscho

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

[Thomas Gummerer]

On 04/17, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Mon, 15 Apr 2019, Thomas Gummerer wrote:
> > I like declaring this a bug.  We are after all parsing
> > machine-generated output, that does come from git (which is why I
> > neglected the NULL checking in the first place).  If that second "@@"
> > is not there it's definitely a bug somewhere in the diff machinery,
> > I'd say.
> 
> Ah, but you do know about the micro-project I proposed to optionally feed
> an mbox to `range-diff`, right?
> 
> The idea behind my proposal is that this would make it possible to
> generate a range-diff between the patches on public-inbox and the commits
> that actually made it into Junio's `pu`...

I had forgotten about that, and was only looking at what the code
currently does.

> > Note that the "@@" also couldn't come from anywhere else, the diff
> > header has a well defined format and so does the metadata.  The diff
> > itself is prefixed with '<', '>' and '#' in this case, and the commit
> > message is also prefixed with four spaces.  So if this breaks
> > somewhere I'd rather hear about it loudly, than let users potentially
> > get wrong output because we missed something somewhere.
> 
> Agreed. But I could imagine that `die()`ing here would be the more
> appropriate way to holler loudly ;-)

Yup, in the light of the potential microproject, I'm fine with just
'die()'ing here.  If we'd get into this situation right now, there
would probably be a bunch of other tests failing as well, so we might
as well make it a little more future proof.  Thanks.