threads / discuss / 50917

incorrect range-diff output?

Subject: incorrect range-diff output?

## tl;dr

29 messages between Apr 11, 2019 and Apr 24, 2019.

replies: 28people: 6as markdown or json

Duy Nguyen· Apr 11, 2019, 11:17 UTC · lore
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

Thomas Gummerer· Apr 11, 2019, 22:05 UTC · re: Duy Nguyen · lore

Re: incorrect range-diff output?

On 04/11, Duy Nguyen wrote:
Show 23 quoted lines
> 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.

Show 7 quoted lines
> 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
Johannes Schindelin· Apr 12, 2019, 08:41 UTC · re: Thomas Gummerer · lore

Re: incorrect range-diff output?

Hi Thomas,
On Thu, 11 Apr 2019, Thomas Gummerer wrote:
Show 28 quoted lines
> 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

Thomas Gummerer· Apr 14, 2019, 21:12 UTC · re: Johannes Schindelin · lore

Re: incorrect range-diff output?

On 04/12, Johannes Schindelin wrote:
Show 38 quoted lines
> 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.

Duy Nguyen· Apr 12, 2019, 09:21 UTC · re: Thomas Gummerer · lore

Re: incorrect range-diff output?

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
Junio C Hamano· Apr 12, 2019, 15:02 UTC · re: Thomas Gummerer · lore

Re: incorrect range-diff output?

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?

Thomas Gummerer· Apr 14, 2019, 21:20 UTC · re: Junio C Hamano · lore

Re: incorrect range-diff output?

On 04/13, Junio C Hamano wrote:
Show 10 quoted lines
> 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).

Show 7 quoted lines
> 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.

Junio C Hamano· Apr 15, 2019, 02:01 UTC · re: Thomas Gummerer · lore

Re: incorrect range-diff output?

Thomas Gummerer <t.gummerer@gmail.com> writes:
Show 5 quoted lines
> 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.
Johannes Schindelin· Apr 15, 2019, 12:40 UTC · re: Junio C Hamano · lore

Re: incorrect range-diff output?

Hi Junio,
On Sat, 13 Apr 2019, Junio C Hamano wrote:
Show 18 quoted lines
> 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

Thomas Gummerer· Apr 14, 2019, 21:09 UTC · re: Thomas Gummerer · lore

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

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
Thomas Gummerer· Apr 14, 2019, 21:09 UTC · re: Thomas Gummerer · lore

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

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
Thomas Gummerer· Apr 14, 2019, 21:09 UTC · re: Thomas Gummerer · lore

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

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
Eric Sunshine· Apr 14, 2019, 23:21 UTC · re: Thomas Gummerer · lore

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

On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 11 quoted lines
> [...]
> 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);
Johannes Schindelin· Apr 15, 2019, 12:54 UTC · re: Eric Sunshine · lore

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

Hi Eric,
On Sun, 14 Apr 2019, Eric Sunshine wrote:
Show 19 quoted lines
> 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

Show 6 quoted lines
>
> might be appropriate.
>
> > +                       strbuf_addch(&buf, ' ');
> > +                       strbuf_addbuf(&buf, &line);
>
Thomas Gummerer· Apr 15, 2019, 18:56 UTC · re: Johannes Schindelin · lore

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

On 04/15, Johannes Schindelin wrote:
Show 27 quoted lines
> 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.

Show 6 quoted lines
> >
> > might be appropriate.
> >
> > > +                       strbuf_addch(&buf, ' ');
> > > +                       strbuf_addbuf(&buf, &line);
> >
Johannes Schindelin· Apr 17, 2019, 11:52 UTC · re: Thomas Gummerer · lore

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

Hi Thomas,
On Mon, 15 Apr 2019, Thomas Gummerer wrote:
Show 33 quoted lines
> 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`...

Show 6 quoted lines
> 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

Thomas Gummerer· Apr 24, 2019, 18:15 UTC · re: Johannes Schindelin · lore

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

On 04/17, Johannes Schindelin wrote:
Show 15 quoted lines
> 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.

Show 9 quoted lines
> > 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.

Johannes Schindelin· Apr 15, 2019, 12:53 UTC · re: Thomas Gummerer · lore

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

Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
Show 15 quoted lines
> 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.
Show 13 quoted lines
> 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

Show 9 quoted lines
> +			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
>
>
Thomas Gummerer· Apr 15, 2019, 18:57 UTC · re: Johannes Schindelin · lore

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

On 04/15, Johannes Schindelin wrote:
Show 22 quoted lines
> 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.
Show 16 quoted lines
> 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!
Show 12 quoted lines
> 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
> >
> >
Thomas Gummerer· Apr 14, 2019, 21:09 UTC · re: Thomas Gummerer · lore

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

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
Eric Sunshine· Apr 14, 2019, 23:29 UTC · re: Thomas Gummerer · lore

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

On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 19 quoted lines
> [...]
> 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".
Show 16 quoted lines
> +                               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?

Johannes Sixt· Apr 15, 2019, 06:28 UTC · re: Eric Sunshine · lore

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

Am 15.04.19 um 01:29 schrieb Eric Sunshine:
Show 26 quoted lines
> 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
Johannes Schindelin· Apr 15, 2019, 13:01 UTC · re: Eric Sunshine · lore

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

Hi Eric,
On Sun, 14 Apr 2019, Eric Sunshine wrote:
Show 50 quoted lines
> 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

Thomas Gummerer· Apr 15, 2019, 19:09 UTC · re: Johannes Schindelin · lore

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

On 04/15, Johannes Schindelin wrote:
Show 31 quoted lines
> 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!
Show 24 quoted lines
> > > +                               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.

Show 22 quoted lines
> > 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!
Show 5 quoted lines
> 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
Thomas Gummerer· Apr 14, 2019, 21:09 UTC · re: Thomas Gummerer · lore

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

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
Johannes Schindelin· Apr 15, 2019, 12:44 UTC · re: Thomas Gummerer · lore

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

Hi Thomas,
On Sun, 14 Apr 2019, Thomas Gummerer wrote:
Show 17 quoted lines
> 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.

Show 12 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?
The rest of the patch makes a whole lot of sense to me.

Thanks, Dscho

Show 27 quoted lines
>  			} 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· Apr 15, 2019, 18:48 UTC · re: Johannes Schindelin · lore

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

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
> >
> >
Johannes Schindelin· Apr 15, 2019, 12:47 UTC · re: Thomas Gummerer · lore

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

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

Thomas Gummerer· Apr 15, 2019, 19:25 UTC · re: Johannes Schindelin · lore

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

On 04/15, Johannes Schindelin wrote:
Show 8 quoted lines
> 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.

Show 7 quoted lines
> 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.

Show 8 quoted lines
> > [...]
> > 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

← back to recent threads