{"thread":{"id":"50917","subject":"incorrect range-diff output?","startedAt":"2019-04-11T11:17:37Z","lastAt":"2019-04-24T18:15:10Z","messageCount":29,"participants":["Duy Nguyen","Thomas Gummerer","Johannes Schindelin","Junio C Hamano","Eric Sunshine","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"373631","messageId":"20190411111729.GB5620@ash","threadId":"50917","inReplyTo":null,"subject":"incorrect range-diff output?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-11T11:17:29Z","receivedAt":"2019-04-11T11:17:37Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Try\n\n git range-diff from...to\n\nwith those two branches from https://gitlab.com/pclouds/git.git. The\ninteresting part is this\n\n diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n --- a/Documentation/gitcli.txt\n @@ -120,10 +111,11 @@\n * linkgit:git-commit[1] to advance the current branch.\n \n - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n - pathname parameters) to undo changes.\n +- pathname parameters) to undo changes.\n ++ * linkgit:git-restore[1] to undo changes.\n \n * linkgit:git-merge[1] to merge between local branches.\n + \n\nThis particular hunk comes from giteveryday.txt, not gitcli.txt. And\nthe b/Documentation/gitcli.txt line is also missing.\n\nI'm in the middle of some other things (and don't know range-diff that\nwell) so I'm just dropping a bug report here.\n\nI've tried all 'master', 'next' and 'pu'. Same result. Tried\n--no-pager too in case the pager ate something up.\n--\nDuy\n"},{"id":"373692","messageId":"20190411220532.GG32487@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"20190411111729.GB5620@ash","subject":"Re: incorrect range-diff output?","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-11T22:05:32Z","receivedAt":"2019-04-11T22:05:39Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/11, Duy Nguyen wrote:\n> Try\n> \n> git range-diff from...to\n> \n> with those two branches from https://gitlab.com/pclouds/git.git. The\n> interesting part is this\n> \n> diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n> --- a/Documentation/gitcli.txt\n> @@ -120,10 +111,11 @@\n> * linkgit:git-commit[1] to advance the current branch.\n> \n> - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n> -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n> - pathname parameters) to undo changes.\n> +- pathname parameters) to undo changes.\n> ++ * linkgit:git-restore[1] to undo changes.\n> \n> * linkgit:git-merge[1] to merge between local branches.\n> + \n> \n> This particular hunk comes from giteveryday.txt, not gitcli.txt. And\n> the b/Documentation/gitcli.txt line is also missing.\n\nI think the output here is technically correct, even though it is very\nmisleading. range-diff doesn't currently show the filenames of the\ndiff that changed, which makes this a bit hard to read. To understand\nwhat's happening here, let's have a look at the parts of the commits\nwhere this happens:\n\nIn 66370c3f63 we have:\n\n diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\n index 837707a8fd..c38bc54439 100644\n --- a/Documentation/git-revert.txt\n +++ b/Documentation/git-revert.txt\n @@ -26,8 +26,8 @@ effect of some earlier commits (often only a faulty one). If you want to\n throw away all uncommitted changes in your working directory, you\n should see linkgit:git-reset[1], particularly the `--hard` option. If\n you want to extract specific files as they were in another commit, you\n -should see linkgit:git-checkout[1], specifically the `git checkout\n - -- ` syntax. Take care with these alternatives as\n +should see linkgit:git-restore[1], specifically the `--source`\n +option Take care with these alternatives as\n both will discard uncommitted changes in your working directory.\n \n OPTIONS\n diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n index 592e06d839..e2a9674297 100644\n --- a/Documentation/gitcli.txt\n +++ b/Documentation/gitcli.txt\n\nWhile in 183c6c9390 we have:\n\n diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt\n index 018ecf49d3..9aadc36881 100644\n --- a/Documentation/git-revert.txt\n +++ b/Documentation/git-revert.txt\n @@ -26,8 +26,8 @@ effect of some earlier commits (often only a faulty one). If you want to\n throw away all uncommitted changes in your working directory, you\n should see linkgit:git-reset[1], particularly the `--hard` option. If\n you want to extract specific files as they were in another commit, you\n -should see linkgit:git-checkout[1], specifically the `git checkout\n - -- ` syntax. Take care with these alternatives as\n +should see linkgit:git-restore[1], specifically the `--source`\n +option. Take care with these alternatives as\n both will discard uncommitted changes in your working directory.\n \n See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n index 592e06d839..e2a9674297 100644\n --- a/Documentation/gitcli.txt\n +++ b/Documentation/gitcli.txt\n\n\nSo adding a bit more context to the bits you posted above we see:\n\n @@ -92,10 +83,10 @@\n -should see linkgit:git-checkout[1], specifically the `git checkout\n - -- ` syntax. Take care with these alternatives as\n +should see linkgit:git-restore[1], specifically the `--source`\n -+option Take care with these alternatives as\n ++option. Take care with these alternatives as\n both will discard uncommitted changes in your working directory.\n \n - OPTIONS\n + See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n \n diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n --- a/Documentation/gitcli.txt\n @@ -120,10 +111,11 @@\n * linkgit:git-commit[1] to advance the current branch.\n \n - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n - pathname parameters) to undo changes.\n +- pathname parameters) to undo changes.\n ++ * linkgit:git-restore[1] to undo changes.\n \n * linkgit:git-merge[1] to merge between local branches.\n + \n @@\n ------------\n $ git switch -c alsa-audio <1>\n\nSo the Documentation/gitcli.txt bit is actually context on the first\ndiff, and everything after '@@ -120,10 +111,11 @@' is completely\nunrelated to that.\n\nI'm not sure what the right solution for this is. I think one thing\nI'd like range-diff to do is to add the filename, or some context\n(e.g. is this part of the commit message etc.) to the @@ line (not\nsure what that is called?).\n\nAnother thing that would help in this case would be to just remove the\ndiff header if it is only in the context of a diff, but not actually\nchanged. We can't just remove it outright, as it could be useful in\nsome cases, e.g. when a file has just been re-named.\n\nSo all that said, while this is technically correct, I think it's\nmisleading enough that we should try to fix it. Maybe I can find some\ntime over the weekend to tackle this, if nobody else gets to it first.\n\n> I'm in the middle of some other things (and don't know range-diff that\n> well) so I'm just dropping a bug report here.\n> \n> I've tried all 'master', 'next' and 'pu'. Same result. Tried\n> --no-pager too in case the pager ate something up.\n> --\n> Duy\n"},{"id":"373714","messageId":"nycvar.QRO.7.76.6.1904121038540.41@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"20190411220532.GG32487@hank.intra.tgummerer.com","subject":"Re: incorrect range-diff output?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-12T08:41:49Z","receivedAt":"2019-04-12T08:42:08Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Thu, 11 Apr 2019, Thomas Gummerer wrote:\n\n> On 04/11, Duy Nguyen wrote:\n> > Try\n> >\n> > git range-diff from...to\n> >\n> > with those two branches from https://gitlab.com/pclouds/git.git. The\n> > interesting part is this\n> >\n> > diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n> > --- a/Documentation/gitcli.txt\n> > @@ -120,10 +111,11 @@\n> > * linkgit:git-commit[1] to advance the current branch.\n> >\n> > - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n> > -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n> > - pathname parameters) to undo changes.\n> > +- pathname parameters) to undo changes.\n> > ++ * linkgit:git-restore[1] to undo changes.\n> >\n> > * linkgit:git-merge[1] to merge between local branches.\n> > +\n> >\n> > This particular hunk comes from giteveryday.txt, not gitcli.txt. And\n> > the b/Documentation/gitcli.txt line is also missing.\n>\n> I think the output here is technically correct, even though it is very\n> misleading. range-diff doesn't currently show the filenames of the\n> diff that changed, which makes this a bit hard to read.\n\nTrue. In the spirit of the \"funcname\" feature of our `git diff` command,\nwe could add some (abbreviated) form of the corresponding `diff` lines\n(maybe just the `a/` filename? Or maybe the `a/` file name, prefixed by\n`-` or `+`, and if the `b/` filename is different, `old->new`? With\n`/dev/null` substituted by `(new)` or `(deleted)`?).\n\n> [...]\n>\n> Maybe I can find some time over the weekend to tackle this, if nobody\n> else gets to it first.\n\nGood luck ;-)\n\nCiao,\nDscho\n"},{"id":"373720","messageId":"CACsJy8DTQV=U3gP_b5eOX7fXy4_rs+iyJFVGWLkC3VwagdVnPA@mail.gmail.com","threadId":"50917","inReplyTo":"20190411220532.GG32487@hank.intra.tgummerer.com","subject":"Re: incorrect range-diff output?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-12T09:21:49Z","receivedAt":"2019-04-12T09:22:18Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 12, 2019 at 5:05 AM Thomas Gummerer wrote:\n> I'm not sure what the right solution for this is. I think one thing\n> I'd like range-diff to do is to add the filename, or some context\n> (e.g. is this part of the commit message etc.) to the @@ line (not\n> sure what that is called?).\n\nI don't know what that \"@@\" line is called either but if possible\nplease keep those lines too. I quite often depend on them to know what\nfunction I'm reading.\n-- \nDuy\n"},{"id":"373749","messageId":"xmqqo95bxo4p.fsf@gitster-ct.c.googlers.com","threadId":"50917","inReplyTo":"20190411220532.GG32487@hank.intra.tgummerer.com","subject":"Re: incorrect range-diff output?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-12T15:02:46Z","receivedAt":"2019-04-12T15:02:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> I'm not sure what the right solution for this is. I think one thing\n> I'd like range-diff to do is to add the filename, or some context\n> (e.g. is this part of the commit message etc.) to the @@ line (not\n> sure what that is called?).\n\nPerhaps the inner diff can be done with the usual funcname/xfuncname\nlogic so that the real function name of the payload is on the @@\nlines (they are internally called \"hunk header\" lines, I think).\n\nAnd then the outer diff (i.e. the one that compares two inner diff\noutput) can use a special funcname pattern that says \"treat the\nlines that begin with '@@ ' as the function name line\", instead of\n(or in addition to??) the logic that says \"lines that begin with\n'diff --git' have interesting information\" which led to a misleading\npiece of information in the range-diff output under discussion,\nperhaps?\n\n"},{"id":"373824","messageId":"20190414210933.20875-1-t.gummerer@gmail.com","threadId":"50917","inReplyTo":"20190411220532.GG32487@hank.intra.tgummerer.com","subject":"[RFC PATCH 0/4] output improvements for git range-diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:09:29Z","receivedAt":"2019-04-14T21:09:45Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"This series aims at improving the output of 'git range-diff',\nespecially in cases like the one mentioned in [*1*], where a diff\nheader, that was before the outer diff's hunk header was\nmisinterpreted as being the filename of the diff that followed it.\n\nIt does so by substituting the diff header of the inner diff with a\nrange-diff specific \"section header\", which is then parsed by a\nuserdiff pattern, so it's included in the outer diff's hunk header.\n\nAdditionally, instead of stripping out the whole hunk header of the\ninner diff, and replacing it by just an \"@@\", only strip the line\nnumbers from that hunk header, but keep the funcname/xfuncname.\n\nThis information is not added to the outer diff's hunk header, because\nit feels like that would end up in too much information, and I feel\nlike the filename is the more useful bit of information if we have to\npick one of them.\n\nAs an example, here's what the relevant part of the range-diff from\nthe original thread would look like now:\n\n @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt\n -should see linkgit:git-checkout[1], specifically the `git checkout\n - -- ` syntax. Take care with these alternatives as\n +should see linkgit:git-restore[1], specifically the `--source`\n -+option Take care with these alternatives as\n ++option. Take care with these alternatives as\n both will discard uncommitted changes in your working directory.\n \n - OPTIONS\n + See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n \n \n \n @@ -129,10 +120,11 @@ modified file Documentation/giteveryday.txt\n * linkgit:git-commit[1] to advance the current branch.\n \n - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n - pathname parameters) to undo changes.\n +- pathname parameters) to undo changes.\n ++ * linkgit:git-restore[1] to undo changes.\n \n * linkgit:git-merge[1] to merge between local branches.\n + \n @@ Create a topic branch and develop.::\n ------------\n $ git switch -c alsa-audio <1>\n\nNote that this patch series doesn't modify or add any tests, and was\njust manually tested locally, thus it is still marked as RFC. I'd\nlove to get some feedback on the output format first, before touching\nany tests, as I expect there might be some discussion on the final\nformat.\n\n*1*: <20190411111729.GB5620@ash>\n\nThis series can also be fetched from\nhttps://github.com/tgummerer/git tg/range-diff-output\n\nThomas Gummerer (4):\n range-diff: fix function parameter indentation\n range-diff: don't remove funcname from inner diff\n range-diff: add section header instead of diff header\n range-diff: add section headers to the outer hunk header\n\n range-diff.c | 55 ++++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 45 insertions(+), 10 deletions(-)\n\n-- \n2.21.0.593.g511ec345e1\n\n"},{"id":"373825","messageId":"20190414210933.20875-2-t.gummerer@gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-1-t.gummerer@gmail.com","subject":"[RFC PATCH 1/4] range-diff: fix function parameter indentation","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:09:30Z","receivedAt":"2019-04-14T21:09:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Fix the indentation of the function parameters for a couple of\nfunctions, to match the style in the rest of the file.\n\nSigned-off-by: Thomas Gummerer \n---\n range-diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 48b0e1b4ce..9242b8975f 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -148,7 +148,7 @@ static int read_patches(const char *range, struct string_list *list)\n }\n \n static int patch_util_cmp(const void *dummy, const struct patch_util *a,\n-\t\t const struct patch_util *b, const char *keydata)\n+\t\t\t const struct patch_util *b, const char *keydata)\n {\n \treturn strcmp(a->diff, keydata ? keydata : b->diff);\n }\n@@ -373,7 +373,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n }\n \n static void patch_diff(const char *a, const char *b,\n-\t\t\t struct diff_options *diffopt)\n+\t\t struct diff_options *diffopt)\n {\n \tdiff_queue(&diff_queued_diff,\n \t\t get_filespec(\"a\", a), get_filespec(\"b\", b));\n-- \n2.21.0.593.g511ec345e1\n\n"},{"id":"373826","messageId":"20190414210933.20875-3-t.gummerer@gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-1-t.gummerer@gmail.com","subject":"[RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:09:31Z","receivedAt":"2019-04-14T21:09:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When postprocessing the inner diff in range-diff, we currently replace\nthe whole hunk header line with just \"@@\". This matches how 'git\ntbdiff' used to handle hunk headers as well.\n\nMost likely this is being done because line numbers in the hunk header\nare not relevant without other changes. They can for example easily\nchange if a range is rebased, and lines are added/removed before a\nchange that we actually care about in our ranges.\n\nHowever it can still be useful to have the function name that 'git\ndiff' extracts as additional context for the change.\n\nNote that it is not guaranteed that the hunk header actually shows up\nin the range-diff, and this change only aims to improve the case where\na hunk header would already be included in the final output.\n\nSigned-off-by: Thomas Gummerer \n---\n range-diff.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 9242b8975f..f365141ade 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\t}\n \t\t\tcontinue;\n-\t\t} else if (starts_with(line.buf, \"@@ \"))\n-\t\t\tstrbuf_addstr(&buf, \"@@\");\n-\t\telse if (!line.buf[0] || starts_with(line.buf, \"index \"))\n+\t\t} else if (starts_with(line.buf, \"@@ \")) {\n+\t\t\tchar *skip_lineno = strstr(line.buf + 3, \"@@\");\n+\t\t\tstrbuf_remove(&line, 0, skip_lineno - line.buf);\n+\t\t\tstrbuf_addch(&buf, ' ');\n+\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t} else if (!line.buf[0] || starts_with(line.buf, \"index \"))\n \t\t\t/*\n \t\t\t * A completely blank (not ' \\n', which is context)\n \t\t\t * line is not valid in a diff. We skip it\n-- \n2.21.0.593.g511ec345e1\n\n"},{"id":"373827","messageId":"20190414210933.20875-4-t.gummerer@gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-1-t.gummerer@gmail.com","subject":"[RFC PATCH 3/4] range-diff: add section header instead of diff header","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:09:32Z","receivedAt":"2019-04-14T21:09:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently range-diff keeps the diff header of the inner diff\nintact (apart from stripping lines starting with index). This diff\nheader is somewhat useful, especially when files get different\nnames in different ranges.\n\nHowever there is no real need to keep the whole diff header for that.\nThe main reason we currently do that is probably because it is easy to\ndo.\n\nIntroduce a new range diff hunk header, that's enclosed by \"##\",\nsimilar to how line numbers in diff hunks are enclosed by \"@@\", and\ngive human readable information of what exactly happened to the file,\nincluding the file name.\n\nAt this point, this is only a marginal improvement in readability of\nthe range-diff output. More interestingly however, this allows us to\nadd these range diff hunk headers to the outer diffs hunk headers\nusing a custom userdiff pattern, which should help making the\nrange-diff more readable.\n\nSigned-off-by: Thomas Gummerer \n---\n range-diff.c | 34 ++++++++++++++++++++++++++++++++--\n 1 file changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex f365141ade..aa466060ef 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -33,6 +33,7 @@ static int read_patches(const char *range, struct string_list *list)\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tFILE *in;\n \tstruct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;\n+\tstruct strbuf filename_a = STRBUF_INIT;\n \tstruct patch_util *util = NULL;\n \tint in_header = 1;\n \n@@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (!util->diff_offset)\n \t\t\t\tutil->diff_offset = buf.len;\n-\t\t\tstrbuf_addch(&buf, ' ');\n-\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t} else if (starts_with(line.buf, \"--- \")) {\n+\t\t\tif (!strcmp(line.buf, \"--- /dev/null\"))\n+\t\t\t\tstrbuf_remove(&line, 0, 4);\n+\t\t\telse\n+\t\t\t\tstrbuf_remove(&line, 0, 6);\n+\t\t\tstrbuf_rtrim(&line);\n+\t\t\tstrbuf_reset(&filename_a);\n+\t\t\tstrbuf_addbuf(&filename_a, &line);\n+\t\t} else if (starts_with(line.buf, \"+++ \")) {\n+\t\t\tstrbuf_addstr(&buf, \" ## \");\n+\t\t\tif (!strcmp(line.buf, \"--- /dev/null\"))\n+\t\t\t\tstrbuf_remove(&line, 0, 4);\n+\t\t\telse\n+\t\t\t\tstrbuf_remove(&line, 0, 6);\n+\t\t\tstrbuf_rtrim(&line);\n+\t\t\tif (!strcmp(filename_a.buf, \"/dev/null\")) {\n+\t\t\t\tstrbuf_addstr(&buf, \"new file \");\n+\t\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t\t} else if (!strcmp(line.buf, \"/dev/null\")) {\n+\t\t\t\tstrbuf_addstr(&buf, \"removed file \");\n+\t\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t\t} else if (strbuf_cmp(&filename_a, &line)) {\n+\t\t\t\tstrbuf_addstr(&buf, \"renamed file \");\n+\t\t\t\tstrbuf_addbuf(&buf, &filename_a);\n+\t\t\t\tstrbuf_addstr(&buf, \" -> \");\n+\t\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addstr(&buf, \"modified file \");\n+\t\t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t\t}\n+\t\t\tstrbuf_addstr(&buf, \" ##\\n\");\n \t\t} else if (in_header) {\n \t\t\tif (starts_with(line.buf, \"Author: \")) {\n \t\t\t\tstrbuf_addbuf(&buf, &line);\n-- \n2.21.0.593.g511ec345e1\n\n"},{"id":"373828","messageId":"20190414210933.20875-5-t.gummerer@gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-1-t.gummerer@gmail.com","subject":"[RFC PATCH 4/4] range-diff: add section headers to the outer hunk header","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:09:33Z","receivedAt":"2019-04-14T21:09:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Add the section headers we introduced in the previous commit to the\nouter diff's hunk headers. This makes it easier to understand which\nchange we are actually looking at. For example an outer hunk header\nmight now look like:\n\n @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt\n\nwhile previously it would have only been\n\n @@ -77,15 +78,43 @@\n\nwhich doesn't give a lot of context for the change that follows.\n\nFor completeness also add section headers for the commit metadata and\nthe commit message, although they are arguably less important.\n\nSigned-off-by: Thomas Gummerer \n---\n range-diff.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex aa466060ef..15ac54f369 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\tstrbuf_addstr(&buf, \" ##\\n\");\n \t\t} else if (in_header) {\n \t\t\tif (starts_with(line.buf, \"Author: \")) {\n+\t\t\t\tstrbuf_addstr(&buf, \" ## Metadata ##\\n\");\n \t\t\t\tstrbuf_addbuf(&buf, &line);\n \t\t\t\tstrbuf_addstr(&buf, \"\\n\\n\");\n+\t\t\t\tstrbuf_addstr(&buf, \" ## Commit message ##\\n\");\n \t\t\t} else if (starts_with(line.buf, \" \")) {\n \t\t\t\tstrbuf_rtrim(&line);\n \t\t\t\tstrbuf_addbuf(&buf, &line);\n@@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,\n \tfwrite(buf->buf, buf->len, 1, diffopt->file);\n }\n \n-static struct userdiff_driver no_func_name = {\n-\t.funcname = { \"$^\", 0 }\n+static struct userdiff_driver section_headers = {\n+\t.funcname = { \"^ ## (.*) ##$\", REG_EXTENDED }\n };\n \n static struct diff_filespec *get_filespec(const char *name, const char *p)\n@@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n \tspec->size = strlen(p);\n \tspec->should_munmap = 0;\n \tspec->is_stdin = 1;\n-\tspec->driver = &no_func_name;\n+\tspec->driver = §ion_headers;\n \n \treturn spec;\n }\n-- \n2.21.0.593.g511ec345e1\n\n"},{"id":"373830","messageId":"20190414211214.GH32487@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904121038540.41@tvgsbejvaqbjf.bet","subject":"Re: incorrect range-diff output?","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:12:14Z","receivedAt":"2019-04-14T21:12:20Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/12, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Thu, 11 Apr 2019, Thomas Gummerer wrote:\n> \n> > On 04/11, Duy Nguyen wrote:\n> > > Try\n> > >\n> > > git range-diff from...to\n> > >\n> > > with those two branches from https://gitlab.com/pclouds/git.git. The\n> > > interesting part is this\n> > >\n> > > diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt\n> > > --- a/Documentation/gitcli.txt\n> > > @@ -120,10 +111,11 @@\n> > > * linkgit:git-commit[1] to advance the current branch.\n> > >\n> > > - * linkgit:git-reset[1] and linkgit:git-checkout[1] (with\n> > > -+ * linkgit:git-reset[1] and linkgit:git-restore[1] (with\n> > > - pathname parameters) to undo changes.\n> > > +- pathname parameters) to undo changes.\n> > > ++ * linkgit:git-restore[1] to undo changes.\n> > >\n> > > * linkgit:git-merge[1] to merge between local branches.\n> > > +\n> > >\n> > > This particular hunk comes from giteveryday.txt, not gitcli.txt. And\n> > > the b/Documentation/gitcli.txt line is also missing.\n> >\n> > I think the output here is technically correct, even though it is very\n> > misleading. range-diff doesn't currently show the filenames of the\n> > diff that changed, which makes this a bit hard to read.\n> \n> True. In the spirit of the \"funcname\" feature of our `git diff` command,\n> we could add some (abbreviated) form of the corresponding `diff` lines\n> (maybe just the `a/` filename? Or maybe the `a/` file name, prefixed by\n> `-` or `+`, and if the `b/` filename is different, `old->new`? With\n> `/dev/null` substituted by `(new)` or `(deleted)`?).\n\nYeah something like this is what I ended up doing. Except I was a bit\nmore verbose, so the filename would be prefixed with \"modified file\",\n\"new file\", \"deleted file\" or \"renamed file\". Since this doesn't need\nto be machine readable in any way, I felt like that might be easiest\nto consume for humans. \n"},{"id":"373834","messageId":"20190414212056.GI32487@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"xmqqo95bxo4p.fsf@gitster-ct.c.googlers.com","subject":"Re: incorrect range-diff output?","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-14T21:20:56Z","receivedAt":"2019-04-14T21:21:02Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/13, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n> \n> > I'm not sure what the right solution for this is. I think one thing\n> > I'd like range-diff to do is to add the filename, or some context\n> > (e.g. is this part of the commit message etc.) to the @@ line (not\n> > sure what that is called?).\n> \n> Perhaps the inner diff can be done with the usual funcname/xfuncname\n> logic so that the real function name of the payload is on the @@\n> lines (they are internally called \"hunk header\" lines, I think).\n\nThe inner diff is actually currently done this way, however we then\nstrip out the funcname. My understanding is that that is mostly done\nto get rid of the line numbers which can change between ranges, so I'm\nre-introducing that, while only stripping the actual line numbers\nout.\n\nI do feel like the filenames should probably have priority in the\nouter diffs hunk header lines though, as we can only add a limited\namount of information there, and without the filename, the function\nname might be less useful (or it may be not useful at all, for things\nwhere we don't have proper funcnames, e.g. for documentation).\n\n> And then the outer diff (i.e. the one that compares two inner diff\n> output) can use a special funcname pattern that says \"treat the\n> lines that begin with '@@ ' as the function name line\", instead of\n> (or in addition to??) the logic that says \"lines that begin with\n> 'diff --git' have interesting information\" which led to a misleading\n> piece of information in the range-diff output under discussion,\n> perhaps?\n\nI don't think there ever was a logic that says \"lines that begin with\n'diff --git' have interesting information\", but rather that's how the\nrange-diff was interpreted, because there was no indication that those\nlines don't have interesting information.\n\nMy series implements something very similar to what you're describing\nthough, except I'm introducing a new type of header for the filename,\nas I think that's more important, as mentioned above.\n"},{"id":"373836","messageId":"CAPig+cSmh1rCTPj0Y=G4OA3TmoGqVBnj95S+KogOgiDtW_0DiQ@mail.gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-3-t.gummerer@gmail.com","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-14T23:21:35Z","receivedAt":"2019-04-14T23:21:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer wrote:\n> [...]\n> However it can still be useful to have the function name that 'git\n> diff' extracts as additional context for the change.\n> [...]\n> Signed-off-by: Thomas Gummerer \n> ---\n> diff --git a/range-diff.c b/range-diff.c\n> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> + } else if (starts_with(line.buf, \"@@ \")) {\n> + char *skip_lineno = strstr(line.buf + 3, \"@@\");\n> + strbuf_remove(&line, 0, skip_lineno - line.buf);\n\nIt makes me a bit uncomfortable that this is not checking for NULL\nreturn from strstr() before doing pointer arithmetic (even though the\ninput is presumably machine-generated).\n\n if (!skip_lineno)\n BUG(...);\n\nmight be appropriate.\n\n> + strbuf_addch(&buf, ' ');\n> + strbuf_addbuf(&buf, &line);\n"},{"id":"373837","messageId":"CAPig+cTJyYLGAfHM_PrZd9DDaZBcVUetcWFcANGCYPvaMh2qxA@mail.gmail.com","threadId":"50917","inReplyTo":"20190414210933.20875-4-t.gummerer@gmail.com","subject":"Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-14T23:29:54Z","receivedAt":"2019-04-14T23:30:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer wrote:\n> [...]\n> Introduce a new range diff hunk header, that's enclosed by \"##\",\n> similar to how line numbers in diff hunks are enclosed by \"@@\", and\n> give human readable information of what exactly happened to the file,\n> including the file name.\n> [...]\n> Signed-off-by: Thomas Gummerer \n> ---\n> diff --git a/range-diff.c b/range-diff.c\n> @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)\n> + } else if (starts_with(line.buf, \"--- \")) {\n> + if (!strcmp(line.buf, \"--- /dev/null\"))\n> + strbuf_remove(&line, 0, 4);\n> + else\n> + strbuf_remove(&line, 0, 6);\n> + strbuf_rtrim(&line);\n> + strbuf_reset(&filename_a);\n> + strbuf_addbuf(&filename_a, &line);\n> + } else if (starts_with(line.buf, \"+++ \")) {\n\nAt this point, we know that line.buf starts with \"+++\"...\n\n> + strbuf_addstr(&buf, \" ## \");\n> + if (!strcmp(line.buf, \"--- /dev/null\"))\n\nso, it seems unlikely that it's ever going to match \"--- /dev/null\".\n\n> + strbuf_remove(&line, 0, 4);\n> + if (!strcmp(filename_a.buf, \"/dev/null\")) {\n> + strbuf_addstr(&buf, \"new file \");\n> + strbuf_addbuf(&buf, &line);\n> + } else if (!strcmp(line.buf, \"/dev/null\")) {\n> + strbuf_addstr(&buf, \"removed file \");\n> + strbuf_addbuf(&buf, &line);\n> + } else if (strbuf_cmp(&filename_a, &line)) {\n> + strbuf_addstr(&buf, \"renamed file \");\n> + strbuf_addbuf(&buf, &filename_a);\n> + strbuf_addstr(&buf, \" -> \");\n> + strbuf_addbuf(&buf, &line);\n> + } else {\n> + strbuf_addstr(&buf, \"modified file \");\n> + strbuf_addbuf(&buf, &line);\n> + }\n\nAll of these disposition strings end with \"file\", which seems\nredundant. Short and sweet \"new\", \"removed\", \"renamed\", \"modified\"\nprovide just as much useful information.\n\nAlso, should these strings be localizable? Alternately, rather than\nusing prose to describe the disposition, perhaps do so symbolically\n(thus universally), say with \"+\", \"-\", \"->\", \"*\" (or \"\"),\nrespectively?\n"},{"id":"373838","messageId":"xmqq36mkvxfj.fsf@gitster-ct.c.googlers.com","threadId":"50917","inReplyTo":"20190414212056.GI32487@hank.intra.tgummerer.com","subject":"Re: incorrect range-diff output?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-15T02:01:36Z","receivedAt":"2019-04-15T02:01:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> I do feel like the filenames should probably have priority in the\n> outer diffs hunk header lines though, as we can only add a limited\n> amount of information there, and without the filename, the function\n> name might be less useful (or it may be not useful at all, for things\n> where we don't have proper funcnames, e.g. for documentation).\n\nThat's quite sensible.\n\nThanks.\n"},{"id":"373846","messageId":"46e31ffc-60f6-1aef-5159-6796b7f13294@kdbg.org","threadId":"50917","inReplyTo":"CAPig+cTJyYLGAfHM_PrZd9DDaZBcVUetcWFcANGCYPvaMh2qxA@mail.gmail.com","subject":"Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-04-15T06:28:22Z","receivedAt":"2019-04-15T06:28:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.04.19 um 01:29 schrieb Eric Sunshine:\n> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer wrote:\n>> + strbuf_remove(&line, 0, 4);\n>> + if (!strcmp(filename_a.buf, \"/dev/null\")) {\n>> + strbuf_addstr(&buf, \"new file \");\n>> + strbuf_addbuf(&buf, &line);\n>> + } else if (!strcmp(line.buf, \"/dev/null\")) {\n>> + strbuf_addstr(&buf, \"removed file \");\n>> + strbuf_addbuf(&buf, &line);\n>> + } else if (strbuf_cmp(&filename_a, &line)) {\n>> + strbuf_addstr(&buf, \"renamed file \");\n>> + strbuf_addbuf(&buf, &filename_a);\n>> + strbuf_addstr(&buf, \" -> \");\n>> + strbuf_addbuf(&buf, &line);\n>> + } else {\n>> + strbuf_addstr(&buf, \"modified file \");\n>> + strbuf_addbuf(&buf, &line);\n>> + }\n> \n> All of these disposition strings end with \"file\", which seems\n> redundant. Short and sweet \"new\", \"removed\", \"renamed\", \"modified\"\n> provide just as much useful information.\n> \n> Also, should these strings be localizable? Alternately, rather than\n> using prose to describe the disposition, perhaps do so symbolically\n> (thus universally), say with \"+\", \"-\", \"->\", \"*\" (or \"\"),\n> respectively?\nWhen the strings are translated, it would be preferable to not do this\nsentence lego at all and have format strings that are filled with the\nvalues.\n\nThen the noise word \"file\" is probably not that bad, in particular, when\ntranslators tend to not add words that they don't see in the original.\nFor example, German translations of \"new %s\" I anticipate \"neues %s\",\n\"%s (neu)\", \"neu %s\", all of which are awkward, but translations of \"new\nfile %s\" lends itself naturally to \"neue Datei %s\", which is fine.\n\n-- Hannes\n"},{"id":"373857","messageId":"nycvar.QRO.7.76.6.1904151439370.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"xmqqo95bxo4p.fsf@gitster-ct.c.googlers.com","subject":"Re: incorrect range-diff output?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T12:40:21Z","receivedAt":"2019-04-15T12:40:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 13 Apr 2019, Junio C Hamano wrote:\n\n> Thomas Gummerer writes:\n>\n> > I'm not sure what the right solution for this is. I think one thing\n> > I'd like range-diff to do is to add the filename, or some context\n> > (e.g. is this part of the commit message etc.) to the @@ line (not\n> > sure what that is called?).\n>\n> Perhaps the inner diff can be done with the usual funcname/xfuncname\n> logic so that the real function name of the payload is on the @@\n> lines (they are internally called \"hunk header\" lines, I think).\n>\n> And then the outer diff (i.e. the one that compares two inner diff\n> output) can use a special funcname pattern that says \"treat the\n> lines that begin with '@@ ' as the function name line\", instead of\n> (or in addition to??) the logic that says \"lines that begin with\n> 'diff --git' have interesting information\" which led to a misleading\n> piece of information in the range-diff output under discussion,\n> perhaps?\n\nNote that the inner hunk headers are often not even in the outer hunks.\n\nSo I still think that it would make most sense to populate the outer hunk\nheaders with useful information.\n\nCiao,\nDscho\n"},{"id":"373858","messageId":"nycvar.QRO.7.76.6.1904151442150.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"20190414210933.20875-5-t.gummerer@gmail.com","subject":"Re: [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T12:44:08Z","receivedAt":"2019-04-15T12:44:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Sun, 14 Apr 2019, Thomas Gummerer wrote:\n\n> Add the section headers we introduced in the previous commit to the\n> outer diff's hunk headers. This makes it easier to understand which\n> change we are actually looking at. For example an outer hunk header\n> might now look like:\n>\n> @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt\n>\n> while previously it would have only been\n>\n> @@ -77,15 +78,43 @@\n>\n> which doesn't give a lot of context for the change that follows.\n>\n> For completeness also add section headers for the commit metadata and\n> the commit message, although they are arguably less important.\n>\n> Signed-off-by: Thomas Gummerer \n\nThat makes a total lot of sense. So much so that we should protect it by a\nnew regression test case.\n\n> diff --git a/range-diff.c b/range-diff.c\n> index aa466060ef..15ac54f369 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)\n> \t\t\tstrbuf_addstr(&buf, \" ##\\n\");\n> \t\t} else if (in_header) {\n> \t\t\tif (starts_with(line.buf, \"Author: \")) {\n> +\t\t\t\tstrbuf_addstr(&buf, \" ## Metadata ##\\n\");\n> \t\t\t\tstrbuf_addbuf(&buf, &line);\n> \t\t\t\tstrbuf_addstr(&buf, \"\\n\\n\");\n> +\t\t\t\tstrbuf_addstr(&buf, \" ## Commit message ##\\n\");\n\nShould this not rather be added when `in_header` is set to false?\n\nThe rest of the patch makes a whole lot of sense to me.\n\nThanks,\nDscho\n\n> \t\t\t} else if (starts_with(line.buf, \" \")) {\n> \t\t\t\tstrbuf_rtrim(&line);\n> \t\t\t\tstrbuf_addbuf(&buf, &line);\n> @@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,\n> \tfwrite(buf->buf, buf->len, 1, diffopt->file);\n> }\n>\n> -static struct userdiff_driver no_func_name = {\n> -\t.funcname = { \"$^\", 0 }\n> +static struct userdiff_driver section_headers = {\n> +\t.funcname = { \"^ ## (.*) ##$\", REG_EXTENDED }\n> };\n>\n> static struct diff_filespec *get_filespec(const char *name, const char *p)\n> @@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> \tspec->size = strlen(p);\n> \tspec->should_munmap = 0;\n> \tspec->is_stdin = 1;\n> -\tspec->driver = &no_func_name;\n> +\tspec->driver = §ion_headers;\n>\n> \treturn spec;\n> }\n> --\n> 2.21.0.593.g511ec345e1\n>\n>\n"},{"id":"373859","messageId":"nycvar.QRO.7.76.6.1904151445250.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"20190414210933.20875-1-t.gummerer@gmail.com","subject":"Re: [RFC PATCH 0/4] output improvements for git range-diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T12:47:48Z","receivedAt":"2019-04-15T12:47:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Sun, 14 Apr 2019, Thomas Gummerer wrote:\n\n> @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt\n\nA better example might be a .c file, as the function name is often a\npretty useful piece of information.\n\nRead: I think it should be part of the outer hunk header.\n\nAlso, the text \"modified file\" takes up an awful lot of space. Maybe we do\nnot really need that information?\n\nWhile at it, we could strip the line numbers, as this is not intended for\nmachine consumption, but for human consumption instead.\n\n> [...]\n> Note that this patch series doesn't modify or add any tests, and was\n> just manually tested locally, thus it is still marked as RFC.\n\nOh, okay then ;-)\n\nThanks for working on this,\nDscho\n"},{"id":"373860","messageId":"nycvar.QRO.7.76.6.1904151448530.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"20190414210933.20875-3-t.gummerer@gmail.com","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T12:53:36Z","receivedAt":"2019-04-15T12:53:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\n\nOn Sun, 14 Apr 2019, Thomas Gummerer wrote:\n\n> When postprocessing the inner diff in range-diff, we currently replace\n> the whole hunk header line with just \"@@\". This matches how 'git\n> tbdiff' used to handle hunk headers as well.\n>\n> Most likely this is being done because line numbers in the hunk header\n> are not relevant without other changes. They can for example easily\n> change if a range is rebased, and lines are added/removed before a\n> change that we actually care about in our ranges.\n>\n> However it can still be useful to have the function name that 'git\n> diff' extracts as additional context for the change.\n>\n> Note that it is not guaranteed that the hunk header actually shows up\n> in the range-diff, and this change only aims to improve the case where\n> a hunk header would already be included in the final output.\n\nMakes sense.\n\n> diff --git a/range-diff.c b/range-diff.c\n> index 9242b8975f..f365141ade 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> \t\t\t\tstrbuf_addch(&buf, '\\n');\n> \t\t\t}\n> \t\t\tcontinue;\n> -\t\t} else if (starts_with(line.buf, \"@@ \"))\n> -\t\t\tstrbuf_addstr(&buf, \"@@\");\n> -\t\telse if (!line.buf[0] || starts_with(line.buf, \"index \"))\n> +\t\t} else if (starts_with(line.buf, \"@@ \")) {\n> +\t\t\tchar *skip_lineno = strstr(line.buf + 3, \"@@\");\n\nRather than using the magic constant \"3\", it would probably make sense to\ndeclare `skip_lineno` outside of the `if` construct, and use\n`skip_prefix(line.buf, \"@@ \", &skip_lineno)` instead of\n`starts_with(...)`.\n\nWe *will*, however, want to have a safeguard against `strstr()` not\nfinding anything. Maybe re-use the `p` variable that we already have, and\ndo this instead:\n\n\t\t} else if (skip_prefix(line.buf, \"@@ \", &p) &&\n\t\t\t (p = strstr(p, \"@@\"))) {\n\n> +\t\t\tstrbuf_remove(&line, 0, skip_lineno - line.buf);\n> +\t\t\tstrbuf_addch(&buf, ' ');\n\nShorter:\n\n\t\t\tstrbuf_splice(&line, 0, p - line.buf, \" \", 1);\n\n(assuming that you accept my suggestion to use `p` instead of\n`skip_lineno`...)\n\nThanks,\nDscho\n\n> +\t\t\tstrbuf_addbuf(&buf, &line);\n> +\t\t} else if (!line.buf[0] || starts_with(line.buf, \"index \"))\n> \t\t\t/*\n> \t\t\t * A completely blank (not ' \\n', which is context)\n> \t\t\t * line is not valid in a diff. We skip it\n> --\n> 2.21.0.593.g511ec345e1\n>\n>\n"},{"id":"373862","messageId":"nycvar.QRO.7.76.6.1904151454010.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"CAPig+cSmh1rCTPj0Y=G4OA3TmoGqVBnj95S+KogOgiDtW_0DiQ@mail.gmail.com","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T12:54:46Z","receivedAt":"2019-04-15T12:54:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sun, 14 Apr 2019, Eric Sunshine wrote:\n\n> On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer wrote:\n> > [...]\n> > However it can still be useful to have the function name that 'git\n> > diff' extracts as additional context for the change.\n> > [...]\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> > diff --git a/range-diff.c b/range-diff.c\n> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> > + } else if (starts_with(line.buf, \"@@ \")) {\n> > + char *skip_lineno = strstr(line.buf + 3, \"@@\");\n> > + strbuf_remove(&line, 0, skip_lineno - line.buf);\n>\n> It makes me a bit uncomfortable that this is not checking for NULL\n> return from strstr() before doing pointer arithmetic (even though the\n> input is presumably machine-generated).\n>\n> if (!skip_lineno)\n> BUG(...);\n\nGood point, but maybe we should not go so far as to declare this a bug,\nand fall back to removing everything bug the initial two `at` characters\ninstead?\n\nThanks,\nDscho\n\n>\n> might be appropriate.\n>\n> > + strbuf_addch(&buf, ' ');\n> > + strbuf_addbuf(&buf, &line);\n>\n"},{"id":"373863","messageId":"nycvar.QRO.7.76.6.1904151457310.44@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"CAPig+cTJyYLGAfHM_PrZd9DDaZBcVUetcWFcANGCYPvaMh2qxA@mail.gmail.com","subject":"Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T13:01:42Z","receivedAt":"2019-04-15T13:01:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sun, 14 Apr 2019, Eric Sunshine wrote:\n\n> On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer wrote:\n> > [...]\n> > Introduce a new range diff hunk header, that's enclosed by \"##\",\n> > similar to how line numbers in diff hunks are enclosed by \"@@\", and\n> > give human readable information of what exactly happened to the file,\n> > including the file name.\n> > [...]\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> > diff --git a/range-diff.c b/range-diff.c\n> > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)\n> > + } else if (starts_with(line.buf, \"--- \")) {\n> > + if (!strcmp(line.buf, \"--- /dev/null\"))\n> > + strbuf_remove(&line, 0, 4);\n> > + else\n> > + strbuf_remove(&line, 0, 6);\n> > + strbuf_rtrim(&line);\n> > + strbuf_reset(&filename_a);\n> > + strbuf_addbuf(&filename_a, &line);\n> > + } else if (starts_with(line.buf, \"+++ \")) {\n>\n> At this point, we know that line.buf starts with \"+++\"...\n>\n> > + strbuf_addstr(&buf, \" ## \");\n> > + if (!strcmp(line.buf, \"--- /dev/null\"))\n>\n> so, it seems unlikely that it's ever going to match \"--- /dev/null\".\n>\n> > + strbuf_remove(&line, 0, 4);\n> > + if (!strcmp(filename_a.buf, \"/dev/null\")) {\n> > + strbuf_addstr(&buf, \"new file \");\n> > + strbuf_addbuf(&buf, &line);\n> > + } else if (!strcmp(line.buf, \"/dev/null\")) {\n> > + strbuf_addstr(&buf, \"removed file \");\n> > + strbuf_addbuf(&buf, &line);\n> > + } else if (strbuf_cmp(&filename_a, &line)) {\n> > + strbuf_addstr(&buf, \"renamed file \");\n> > + strbuf_addbuf(&buf, &filename_a);\n> > + strbuf_addstr(&buf, \" -> \");\n> > + strbuf_addbuf(&buf, &line);\n> > + } else {\n> > + strbuf_addstr(&buf, \"modified file \");\n> > + strbuf_addbuf(&buf, &line);\n> > + }\n>\n> All of these disposition strings end with \"file\", which seems\n> redundant. Short and sweet \"new\", \"removed\", \"renamed\", \"modified\"\n> provide just as much useful information.\n>\n> Also, should these strings be localizable?\n\nI'd rather not.\n\n> Alternately, rather than using prose to describe the disposition,\n> perhaps do so symbolically (thus universally), say with \"+\", \"-\", \"->\",\n> \"*\" (or \"\"), respectively?\n\nOr maybe streamline the common case (modified) by *not* saying anything,\nthen? I.e.\n\n\t@@ Documentation/Makefile\n\nfor a modified file,\n\n\t@@ builtin/psuh.c (new)\n\nfor a new file,\n\n\t@@ git-add--interactive.perl (deleted)\n\nfor a removed one, and\n\n\t@@ builtin/serve.c -> t/helper/test-serve-v2.c\n\nfor a renamed one.\n\nThat should also give us a bit of wiggle room to append the function name\npart of the inner hunk header, if any.\n\nCiao,\nDscho\n"},{"id":"373887","messageId":"20190415184801.GA1704@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904151442150.44@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 4/4] range-diff: add section headers to the outer hunk header","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-15T18:48:01Z","receivedAt":"2019-04-15T18:48:40Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/15, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Sun, 14 Apr 2019, Thomas Gummerer wrote:\n> \n> > Add the section headers we introduced in the previous commit to the\n> > outer diff's hunk headers. This makes it easier to understand which\n> > change we are actually looking at. For example an outer hunk header\n> > might now look like:\n> >\n> > @@ -77,15 +78,43 @@ modified file Documentation/config/interactive.txt\n> >\n> > while previously it would have only been\n> >\n> > @@ -77,15 +78,43 @@\n> >\n> > which doesn't give a lot of context for the change that follows.\n> >\n> > For completeness also add section headers for the commit metadata and\n> > the commit message, although they are arguably less important.\n> >\n> > Signed-off-by: Thomas Gummerer \n> \n> That makes a total lot of sense. So much so that we should protect it by a\n> new regression test case.\n\nYup, definitely. As mentioned in the cover letter, I punted on to\ndiscuss and settle on an output format first, so I don't have to\nduplicate a lot of work writing and re-writing the tests ;)\n\n> > diff --git a/range-diff.c b/range-diff.c\n> > index aa466060ef..15ac54f369 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -124,8 +124,10 @@ static int read_patches(const char *range, struct string_list *list)\n> > \t\t\tstrbuf_addstr(&buf, \" ##\\n\");\n> > \t\t} else if (in_header) {\n> > \t\t\tif (starts_with(line.buf, \"Author: \")) {\n> > +\t\t\t\tstrbuf_addstr(&buf, \" ## Metadata ##\\n\");\n> > \t\t\t\tstrbuf_addbuf(&buf, &line);\n> > \t\t\t\tstrbuf_addstr(&buf, \"\\n\\n\");\n> > +\t\t\t\tstrbuf_addstr(&buf, \" ## Commit message ##\\n\");\n> \n> Should this not rather be added when `in_header` is set to false?\n\nI don't think so. The commit message counts as part of in_header, and\nstarts right after the rest of the commit metadata. If we would add\nthis after when we set in_header to false, we'd write it after the\ncommit message, which would make the hunk header \"@@ ... @@ Metadata\",\neven when we show the commit message in the output.\n\n> The rest of the patch makes a whole lot of sense to me.\n\nThanks!\n\n> Thanks,\n> Dscho\n> \n> > \t\t\t} else if (starts_with(line.buf, \" \")) {\n> > \t\t\t\tstrbuf_rtrim(&line);\n> > \t\t\t\tstrbuf_addbuf(&buf, &line);\n> > @@ -387,8 +389,8 @@ static void output_pair_header(struct diff_options *diffopt,\n> > \tfwrite(buf->buf, buf->len, 1, diffopt->file);\n> > }\n> >\n> > -static struct userdiff_driver no_func_name = {\n> > -\t.funcname = { \"$^\", 0 }\n> > +static struct userdiff_driver section_headers = {\n> > +\t.funcname = { \"^ ## (.*) ##$\", REG_EXTENDED }\n> > };\n> >\n> > static struct diff_filespec *get_filespec(const char *name, const char *p)\n> > @@ -400,7 +402,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> > \tspec->size = strlen(p);\n> > \tspec->should_munmap = 0;\n> > \tspec->is_stdin = 1;\n> > -\tspec->driver = &no_func_name;\n> > +\tspec->driver = §ion_headers;\n> >\n> > \treturn spec;\n> > }\n> > --\n> > 2.21.0.593.g511ec345e1\n> >\n> >\n"},{"id":"373888","messageId":"20190415185627.GB1704@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904151454010.44@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-15T18:56:27Z","receivedAt":"2019-04-15T18:56:32Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/15, Johannes Schindelin wrote:\n> Hi Eric,\n> \n> On Sun, 14 Apr 2019, Eric Sunshine wrote:\n> \n> > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer wrote:\n> > > [...]\n> > > However it can still be useful to have the function name that 'git\n> > > diff' extracts as additional context for the change.\n> > > [...]\n> > > Signed-off-by: Thomas Gummerer \n> > > ---\n> > > diff --git a/range-diff.c b/range-diff.c\n> > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> > > + } else if (starts_with(line.buf, \"@@ \")) {\n> > > + char *skip_lineno = strstr(line.buf + 3, \"@@\");\n> > > + strbuf_remove(&line, 0, skip_lineno - line.buf);\n> >\n> > It makes me a bit uncomfortable that this is not checking for NULL\n> > return from strstr() before doing pointer arithmetic (even though the\n> > input is presumably machine-generated).\n> >\n> > if (!skip_lineno)\n> > BUG(...);\n> \n> Good point, but maybe we should not go so far as to declare this a bug,\n> and fall back to removing everything bug the initial two `at` characters\n> instead?\n\nI like declaring this a bug. We are after all parsing\nmachine-generated output, that does come from git (which is why I\nneglected the NULL checking in the first place). If that second \"@@\"\nis not there it's definitely a bug somewhere in the diff machinery,\nI'd say.\n\nNote that the \"@@\" also couldn't come from anywhere else, the diff\nheader has a well defined format and so does the metadata. The diff\nitself is prefixed with '<', '>' and '#' in this case, and the commit\nmessage is also prefixed with four spaces. So if this breaks\nsomewhere I'd rather hear about it loudly, than let users potentially\nget wrong output because we missed something somewhere.\n\n> >\n> > might be appropriate.\n> >\n> > > + strbuf_addch(&buf, ' ');\n> > > + strbuf_addbuf(&buf, &line);\n> >\n"},{"id":"373889","messageId":"20190415185740.GC1704@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904151448530.44@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-15T18:57:40Z","receivedAt":"2019-04-15T18:57:44Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/15, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> \n> On Sun, 14 Apr 2019, Thomas Gummerer wrote:\n> > diff --git a/range-diff.c b/range-diff.c\n> > index 9242b8975f..f365141ade 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> > \t\t\t\tstrbuf_addch(&buf, '\\n');\n> > \t\t\t}\n> > \t\t\tcontinue;\n> > -\t\t} else if (starts_with(line.buf, \"@@ \"))\n> > -\t\t\tstrbuf_addstr(&buf, \"@@\");\n> > -\t\telse if (!line.buf[0] || starts_with(line.buf, \"index \"))\n> > +\t\t} else if (starts_with(line.buf, \"@@ \")) {\n> > +\t\t\tchar *skip_lineno = strstr(line.buf + 3, \"@@\");\n> \n> Rather than using the magic constant \"3\", it would probably make sense to\n> declare `skip_lineno` outside of the `if` construct, and use\n> `skip_prefix(line.buf, \"@@ \", &skip_lineno)` instead of\n> `starts_with(...)`.\n\nI like this suggestion.\n\n> We *will*, however, want to have a safeguard against `strstr()` not\n> finding anything. Maybe re-use the `p` variable that we already have, and\n> do this instead:\n> \n> \t\t} else if (skip_prefix(line.buf, \"@@ \", &p) &&\n> \t\t\t (p = strstr(p, \"@@\"))) {\n> \n> > +\t\t\tstrbuf_remove(&line, 0, skip_lineno - line.buf);\n> > +\t\t\tstrbuf_addch(&buf, ' ');\n> \n> Shorter:\n> \n> \t\t\tstrbuf_splice(&line, 0, p - line.buf, \" \", 1);\n> \n> (assuming that you accept my suggestion to use `p` instead of\n> `skip_lineno`...)\n\nAnd this is also much nicer, thanks!\n\n> Thanks,\n> Dscho\n> \n> > +\t\t\tstrbuf_addbuf(&buf, &line);\n> > +\t\t} else if (!line.buf[0] || starts_with(line.buf, \"index \"))\n> > \t\t\t/*\n> > \t\t\t * A completely blank (not ' \\n', which is context)\n> > \t\t\t * line is not valid in a diff. We skip it\n> > --\n> > 2.21.0.593.g511ec345e1\n> >\n> >\n"},{"id":"373892","messageId":"20190415190939.GD1704@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904151457310.44@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 3/4] range-diff: add section header instead of diff header","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-15T19:09:39Z","receivedAt":"2019-04-15T19:20:55Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/15, Johannes Schindelin wrote:\n> Hi Eric,\n> \n> On Sun, 14 Apr 2019, Eric Sunshine wrote:\n> \n> > On Sun, Apr 14, 2019 at 5:10 PM Thomas Gummerer wrote:\n> > > [...]\n> > > Introduce a new range diff hunk header, that's enclosed by \"##\",\n> > > similar to how line numbers in diff hunks are enclosed by \"@@\", and\n> > > give human readable information of what exactly happened to the file,\n> > > including the file name.\n> > > [...]\n> > > Signed-off-by: Thomas Gummerer \n> > > ---\n> > > diff --git a/range-diff.c b/range-diff.c\n> > > @@ -90,8 +91,37 @@ static int read_patches(const char *range, struct string_list *list)\n> > > + } else if (starts_with(line.buf, \"--- \")) {\n> > > + if (!strcmp(line.buf, \"--- /dev/null\"))\n> > > + strbuf_remove(&line, 0, 4);\n> > > + else\n> > > + strbuf_remove(&line, 0, 6);\n> > > + strbuf_rtrim(&line);\n> > > + strbuf_reset(&filename_a);\n> > > + strbuf_addbuf(&filename_a, &line);\n> > > + } else if (starts_with(line.buf, \"+++ \")) {\n> >\n> > At this point, we know that line.buf starts with \"+++\"...\n> >\n> > > + strbuf_addstr(&buf, \" ## \");\n> > > + if (!strcmp(line.buf, \"--- /dev/null\"))\n> >\n> > so, it seems unlikely that it's ever going to match \"--- /dev/null\".\n\nOuch yup, this is some bad copy pasta, thanks for catching!\n\n> > > + strbuf_remove(&line, 0, 4);\n> > > + if (!strcmp(filename_a.buf, \"/dev/null\")) {\n> > > + strbuf_addstr(&buf, \"new file \");\n> > > + strbuf_addbuf(&buf, &line);\n> > > + } else if (!strcmp(line.buf, \"/dev/null\")) {\n> > > + strbuf_addstr(&buf, \"removed file \");\n> > > + strbuf_addbuf(&buf, &line);\n> > > + } else if (strbuf_cmp(&filename_a, &line)) {\n> > > + strbuf_addstr(&buf, \"renamed file \");\n> > > + strbuf_addbuf(&buf, &filename_a);\n> > > + strbuf_addstr(&buf, \" -> \");\n> > > + strbuf_addbuf(&buf, &line);\n> > > + } else {\n> > > + strbuf_addstr(&buf, \"modified file \");\n> > > + strbuf_addbuf(&buf, &line);\n> > > + }\n> >\n> > All of these disposition strings end with \"file\", which seems\n> > redundant. Short and sweet \"new\", \"removed\", \"renamed\", \"modified\"\n> > provide just as much useful information.\n> >\n> > Also, should these strings be localizable?\n> \n> I'd rather not.\n\nDunno, why do you think they should not be localizable? I'm tend to\nagree with Eric that they could be made localizable, after all this\noutput is not supposed to be machine readable either way. I don't\nhave a strong opinion here though.\n\n> > Alternately, rather than using prose to describe the disposition,\n> > perhaps do so symbolically (thus universally), say with \"+\", \"-\", \"->\",\n> > \"*\" (or \"\"), respectively?\n> \n> Or maybe streamline the common case (modified) by *not* saying anything,\n> then? I.e.\n> \n> \t@@ Documentation/Makefile\n> \n> for a modified file,\n> \n> \t@@ builtin/psuh.c (new)\n> \n> for a new file,\n> \n> \t@@ git-add--interactive.perl (deleted)\n> \n> for a removed one, and\n> \n> \t@@ builtin/serve.c -> t/helper/test-serve-v2.c\n> \n> for a renamed one.\n\nThis looks like a good suggestion to me, thanks!\n\n> That should also give us a bit of wiggle room to append the function name\n> part of the inner hunk header, if any.\n> \n> Ciao,\n> Dscho\n"},{"id":"373894","messageId":"20190415192510.GF1704@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904151445250.44@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 0/4] output improvements for git range-diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-15T19:25:10Z","receivedAt":"2019-04-15T19:25:19Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/15, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Sun, 14 Apr 2019, Thomas Gummerer wrote:\n> \n> > @@ -99,10 +90,10 @@ modified file Documentation/git-revert.txt\n> \n> A better example might be a .c file, as the function name is often a\n> pretty useful piece of information.\n\nYeah, maybe with your suggestions, we could fit the function name or\nsome of the function name into the outer hunk header. I'll give it a\ntry and see how it looks.\n\n> Read: I think it should be part of the outer hunk header.\n> \n> Also, the text \"modified file\" takes up an awful lot of space. Maybe we do\n> not really need that information?\n> \n> While at it, we could strip the line numbers, as this is not intended for\n> machine consumption, but for human consumption instead.\n\nYeah, that makes sense, the line numbers are really kind of pointless\nin a range-diff.\n\n> > [...]\n> > Note that this patch series doesn't modify or add any tests, and was\n> > just manually tested locally, thus it is still marked as RFC.\n> \n> Oh, okay then ;-)\n> \n> Thanks for working on this,\n> Dscho\n"},{"id":"374030","messageId":"nycvar.QRO.7.76.6.1904171350310.46@tvgsbejvaqbjf.bet","threadId":"50917","inReplyTo":"20190415185627.GB1704@hank.intra.tgummerer.com","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-17T11:52:37Z","receivedAt":"2019-04-17T11:52:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Mon, 15 Apr 2019, Thomas Gummerer wrote:\n\n> On 04/15, Johannes Schindelin wrote:\n>\n> > On Sun, 14 Apr 2019, Eric Sunshine wrote:\n> >\n> > > On Sun, Apr 14, 2019 at 5:09 PM Thomas Gummerer wrote:\n> > > > [...]\n> > > > However it can still be useful to have the function name that 'git\n> > > > diff' extracts as additional context for the change.\n> > > > [...]\n> > > > Signed-off-by: Thomas Gummerer \n> > > > ---\n> > > > diff --git a/range-diff.c b/range-diff.c\n> > > > @@ -102,9 +102,12 @@ static int read_patches(const char *range, struct string_list *list)\n> > > > + } else if (starts_with(line.buf, \"@@ \")) {\n> > > > + char *skip_lineno = strstr(line.buf + 3, \"@@\");\n> > > > + strbuf_remove(&line, 0, skip_lineno - line.buf);\n> > >\n> > > It makes me a bit uncomfortable that this is not checking for NULL\n> > > return from strstr() before doing pointer arithmetic (even though the\n> > > input is presumably machine-generated).\n> > >\n> > > if (!skip_lineno)\n> > > BUG(...);\n> >\n> > Good point, but maybe we should not go so far as to declare this a bug,\n> > and fall back to removing everything bug the initial two `at` characters\n> > instead?\n>\n> I like declaring this a bug. We are after all parsing\n> machine-generated output, that does come from git (which is why I\n> neglected the NULL checking in the first place). If that second \"@@\"\n> is not there it's definitely a bug somewhere in the diff machinery,\n> I'd say.\n\nAh, but you do know about the micro-project I proposed to optionally feed\nan mbox to `range-diff`, right?\n\nThe idea behind my proposal is that this would make it possible to\ngenerate a range-diff between the patches on public-inbox and the commits\nthat actually made it into Junio's `pu`...\n\n> Note that the \"@@\" also couldn't come from anywhere else, the diff\n> header has a well defined format and so does the metadata. The diff\n> itself is prefixed with '<', '>' and '#' in this case, and the commit\n> message is also prefixed with four spaces. So if this breaks\n> somewhere I'd rather hear about it loudly, than let users potentially\n> get wrong output because we missed something somewhere.\n\nAgreed. But I could imagine that `die()`ing here would be the more\nappropriate way to holler loudly ;-)\n\nCiao,\nDscho\n"},{"id":"374412","messageId":"20190424181504.GA2068@hank.intra.tgummerer.com","threadId":"50917","inReplyTo":"nycvar.QRO.7.76.6.1904171350310.46@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH 2/4] range-diff: don't remove funcname from inner diff","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-04-24T18:15:04Z","receivedAt":"2019-04-24T18:15:10Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 04/17, Johannes Schindelin wrote:\n> Hi Thomas,\n> \n> On Mon, 15 Apr 2019, Thomas Gummerer wrote:\n> > I like declaring this a bug. We are after all parsing\n> > machine-generated output, that does come from git (which is why I\n> > neglected the NULL checking in the first place). If that second \"@@\"\n> > is not there it's definitely a bug somewhere in the diff machinery,\n> > I'd say.\n> \n> Ah, but you do know about the micro-project I proposed to optionally feed\n> an mbox to `range-diff`, right?\n> \n> The idea behind my proposal is that this would make it possible to\n> generate a range-diff between the patches on public-inbox and the commits\n> that actually made it into Junio's `pu`...\n\nI had forgotten about that, and was only looking at what the code\ncurrently does.\n\n> > Note that the \"@@\" also couldn't come from anywhere else, the diff\n> > header has a well defined format and so does the metadata. The diff\n> > itself is prefixed with '<', '>' and '#' in this case, and the commit\n> > message is also prefixed with four spaces. So if this breaks\n> > somewhere I'd rather hear about it loudly, than let users potentially\n> > get wrong output because we missed something somewhere.\n> \n> Agreed. But I could imagine that `die()`ing here would be the more\n> appropriate way to holler loudly ;-)\n\nYup, in the light of the potential microproject, I'm fine with just\n'die()'ing here. If we'd get into this situation right now, there\nwould probably be a bunch of other tests failing as well, so we might\nas well make it a little more future proof. Thanks.\n"}]}