{"thread":{"id":"54578","subject":"range-diff should suppress context-only changes?","startedAt":"2020-11-05T13:34:40Z","lastAt":"2020-11-17T22:57:06Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Johannes Altmanninger","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"409200","messageId":"20201105133437.GC91972@coredump.intra.peff.net","threadId":"54578","inReplyTo":null,"subject":"range-diff should suppress context-only changes?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-05T13:34:37Z","receivedAt":"2020-11-05T13:34:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 05, 2020 at 12:22:32AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Range-diff vs v3:\n> [...]\n>   7:  42633b8d03 !  7:  5e8004c728 strmap: add more utility functions\n>      @@ strmap.h: void *strmap_get(struct strmap *map, const char *str);\n>       + * iterate through @map using @iter, @var is a pointer to a type strmap_entry\n>       + */\n>       +#define strmap_for_each_entry(mystrmap, iter, var)\t\\\n>      -+\tfor (var = hashmap_iter_first_entry_offset(&(mystrmap)->map, iter, 0); \\\n>      -+\t\tvar; \\\n>      -+\t\tvar = hashmap_iter_next_entry_offset(iter, 0))\n>      ++\thashmap_for_each_entry(&(mystrmap)->map, iter, var, ent)\n>       +\n>        #endif /* STRMAP_H */\n>   8:  ea942eb803 =  8:  fd96e9fc8d strmap: enable faster clearing and reusing of strmaps\n>   9:  c1d2172171 !  9:  f499934f54 strmap: add functions facilitating use as a string->int map\n> [...]\n>       @@ strmap.h: static inline int strmap_empty(struct strmap *map)\n>      - \t\tvar; \\\n>      - \t\tvar = hashmap_iter_next_entry_offset(iter, 0))\n>      + #define strmap_for_each_entry(mystrmap, iter, var)\t\\\n>      + \thashmap_for_each_entry(&(mystrmap)->map, iter, var, ent)\n>        \n>       +\n>       +/*\n>      @@ strmap.h: static inline int strmap_empty(struct strmap *map)\n\nDefinitely not a problem with your patches, but I noticed this curiosity\nin the range-diff. Patch 7 changes the definition of the macro, but it\ngets mentioned again in patch 9, even though the code wasn't touched.\nThe issue is that it the change from 7 ends up in the context of 9; the\nactual modification in patch 9 is in those final couple lines touching a\ncomment (and they didn't change at all between the two versions).\n\nI wonder if it would be reasonable to suppress range-diff hunks in which\nall of the changed lines are context lines.\n\n-Peff\n"},{"id":"409218","messageId":"xmqqmtzvikwi.fsf@gitster.c.googlers.com","threadId":"54578","inReplyTo":"20201105133437.GC91972@coredump.intra.peff.net","subject":"Re: range-diff should suppress context-only changes?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T20:55:09Z","receivedAt":"2020-11-05T20:55:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Nov 05, 2020 at 12:22:32AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n>> Range-diff vs v3:\n>> [...]\n>>   7:  42633b8d03 !  7:  5e8004c728 strmap: add more utility functions\n>>      @@ strmap.h: void *strmap_get(struct strmap *map, const char *str);\n>>       + * iterate through @map using @iter, @var is a pointer to a type strmap_entry\n>>       + */\n>>       +#define strmap_for_each_entry(mystrmap, iter, var)\t\\\n>>      -+\tfor (var = hashmap_iter_first_entry_offset(&(mystrmap)->map, iter, 0); \\\n>>      -+\t\tvar; \\\n>>      -+\t\tvar = hashmap_iter_next_entry_offset(iter, 0))\n>>      ++\thashmap_for_each_entry(&(mystrmap)->map, iter, var, ent)\n>>       +\n>>        #endif /* STRMAP_H */\n>>   8:  ea942eb803 =  8:  fd96e9fc8d strmap: enable faster clearing and reusing of strmaps\n>>   9:  c1d2172171 !  9:  f499934f54 strmap: add functions facilitating use as a string->int map\n>> [...]\n>>       @@ strmap.h: static inline int strmap_empty(struct strmap *map)\n>>      - \t\tvar; \\\n>>      - \t\tvar = hashmap_iter_next_entry_offset(iter, 0))\n>>      + #define strmap_for_each_entry(mystrmap, iter, var)\t\\\n>>      + \thashmap_for_each_entry(&(mystrmap)->map, iter, var, ent)\n>>        \n>>       +\n>>       +/*\n>>      @@ strmap.h: static inline int strmap_empty(struct strmap *map)\n>\n> Definitely not a problem with your patches, but I noticed this curiosity\n> in the range-diff. Patch 7 changes the definition of the macro, but it\n> gets mentioned again in patch 9, even though the code wasn't touched.\n> The issue is that it the change from 7 ends up in the context of 9; the\n> actual modification in patch 9 is in those final couple lines touching a\n> comment (and they didn't change at all between the two versions).\n>\n> I wonder if it would be reasonable to suppress range-diff hunks in which\n> all of the changed lines are context lines.\n\nSounds like a reasonable thing to do.  As we know the shape of what\nis compared in the outer diff we should be able to accurately notice\nwhere hunk boundaries are and a hunk whose change is only on context\nlines.\n\n"},{"id":"410137","messageId":"20201117213551.2539438-1-aclopte@gmail.com","threadId":"54578","inReplyTo":"xmqqmtzvikwi.fsf@gitster.c.googlers.com","subject":"Re: range-diff should suppress context-only changes?","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2020-11-17T21:35:48Z","receivedAt":"2020-11-17T21:36:18Z","isPatch":false,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"> > I wonder if it would be reasonable to suppress range-diff hunks in which\n> > all of the changed lines are context lines.\n> \n> Sounds like a reasonable thing to do.  As we know the shape of what\n> is compared in the outer diff we should be able to accurately notice\n> where hunk boundaries are and a hunk whose change is only on context\n> lines.\n\nHere are patches to ignore context-only changes in range-diff's output.\nI'm not completely happy with the changes, they feel a bit too hacky.\nMaybe someone has better ideas.\n\nThis still gives output like this one, that could be improved in future\n\n\t1:  7a3dac8 ! 1:  119bc78 Change\n\t    @@ some-other-file\n\t      7\n\t      8\n\t      9\n\t    - Old context line\n\t    + New context line\n\n\t    -## file ##\n\t    +## file => renamed-file ##\n\t     @@\n\t      1\n\t     -2\n\n\nI think it should be\n\n\t1:  7a3dac8 ! 1:  119bc78 Change\n\t    -## file ##\n\t    +## file => renamed-file ##\n\t     @@\n\t      1\n\t     -2\n\nI'm not sure if this is a feasible improvement.\n\n\"## <filename> ##\" normally is a diff\nsection header (hence the \"@@ some-other-file\" hunk above) but here the\nsection header itself is changed..\n\n\n"},{"id":"410138","messageId":"20201117213551.2539438-2-aclopte@gmail.com","threadId":"54578","inReplyTo":"20201117213551.2539438-1-aclopte@gmail.com","subject":"[PATCH 1/3] range-diff: move \" ## filename ##\" headers to the first column","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2020-11-17T21:35:49Z","receivedAt":"2020-11-17T21:36:18Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Output of range-diff may include comparisons of metadata like commit messages\nand filenames. Metadata lines look like \" ## <content> ##\".\n\nWhen range-diff compares two matching commits, it computes a diff of two\nspecial commit diffs. In these commit diffs, each changed file is introduced\nwith a \" ## filename ##\" line which is followed by the diff hunks with changes\nto the file's contents.\n\nThe leading space makes it hard to distinguish between file metadata lines\nand context lines from a diff hunk, especially when looking only at the\noutput of range-diff.  Drop the space prefix to facilitate that.\n---\n range-diff.c          |  4 ++--\n t/t3206-range-diff.sh | 42 +++++++++++++++++++++---------------------\n 2 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 24dc435e48..72660453bd 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -136,7 +136,7 @@ static int read_patches(const char *range, struct string_list *list,\n \t\t\tif (len < 0)\n \t\t\t\tdie(_(\"could not parse git header '%.*s'\"),\n \t\t\t\t    orig_len, line);\n-\t\t\tstrbuf_addstr(&buf, \" ## \");\n+\t\t\tstrbuf_addstr(&buf, \"## \");\n \t\t\tif (patch.is_new > 0)\n \t\t\t\tstrbuf_addf(&buf, \"%s (new)\", patch.new_name);\n \t\t\telse if (patch.is_delete > 0)\n@@ -432,7 +432,7 @@ static void output_pair_header(struct diff_options *diffopt,\n }\n \n static struct userdiff_driver section_headers = {\n-\t.funcname = { \"^ ## (.*) ##$\\n\"\n+\t.funcname = { \"^ ?## (.*) ##$\\n\"\n \t\t      \"^.?@@ (.*)$\", REG_EXTENDED }\n };\n \ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 6eb344be03..f875843b5e 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -304,8 +304,8 @@ test_expect_success 'renamed file' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + rename file\n \t    Z\n-\t    - ## file ##\n-\t    + ## file => renamed-file ##\n+\t    -## file ##\n+\t    +## file => renamed-file ##\n \t    Z@@\n \t    Z 1\n \t    Z 2\n@@ -314,9 +314,9 @@ test_expect_success 'renamed file' '\n \t    Z ## Commit message ##\n \t    Z    s/11/B/\n \t    Z\n-\t    - ## file ##\n+\t    -## file ##\n \t    -@@ file: A\n-\t    + ## renamed-file ##\n+\t    +## renamed-file ##\n \t    +@@ renamed-file: A\n \t    Z 8\n \t    Z 9\n@@ -326,9 +326,9 @@ test_expect_success 'renamed file' '\n \t    Z ## Commit message ##\n \t    Z    s/12/B/\n \t    Z\n-\t    - ## file ##\n+\t    -## file ##\n \t    -@@ file: A\n-\t    + ## renamed-file ##\n+\t    +## renamed-file ##\n \t    +@@ renamed-file: A\n \t    Z 9\n \t    Z 10\n@@ -348,14 +348,14 @@ test_expect_success 'file with mode only change' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + add other-file\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@\n \t    @@ file\n \t    Z A\n \t    Z 6\n \t    Z 7\n \t    +\n-\t    + ## other-file (new) ##\n+\t    +## other-file (new) ##\n \t2:  $(test_oid t3) ! 2:  $(test_oid o2) s/11/B/\n \t    @@ Metadata\n \t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n@@ -364,14 +364,14 @@ test_expect_success 'file with mode only change' '\n \t    -    s/11/B/\n \t    +    s/11/B/ + mode change other-file\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \t    @@ file: A\n \t    Z 12\n \t    Z 13\n \t    Z 14\n \t    +\n-\t    + ## other-file (mode change 100644 => 100755) ##\n+\t    +## other-file (mode change 100644 => 100755) ##\n \t3:  $(test_oid t4) = 3:  $(test_oid o3) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n@@ -389,14 +389,14 @@ test_expect_success 'file added and later removed' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + new-file\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@\n \t    @@ file\n \t    Z A\n \t    Z 6\n \t    Z 7\n \t    +\n-\t    + ## new-file (new) ##\n+\t    +## new-file (new) ##\n \t3:  $(test_oid t3) ! 3:  $(test_oid s3) s/11/B/\n \t    @@ Metadata\n \t    ZAuthor: Thomas Rast <trast@inf.ethz.ch>\n@@ -405,14 +405,14 @@ test_expect_success 'file added and later removed' '\n \t    -    s/11/B/\n \t    +    s/11/B/ + remove file\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \t    @@ file: A\n \t    Z 12\n \t    Z 13\n \t    Z 14\n \t    +\n-\t    + ## new-file (deleted) ##\n+\t    +## new-file (deleted) ##\n \t4:  $(test_oid t4) = 4:  $(test_oid s4) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n@@ -434,7 +434,7 @@ test_expect_success 'changed message' '\n \t    Z\n \t    +    Also a silly comment here!\n \t    +\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@\n \t    Z 1\n \t3:  $(test_oid t3) = 3:  $(test_oid m3) s/11/B/\n@@ -453,7 +453,7 @@ test_expect_success 'dual-coloring' '\n \t:     <RESET>\n \t:    <REVERSE><GREEN>+<RESET><BOLD>    Also a silly comment here!<RESET>\n \t:    <REVERSE><GREEN>+<RESET>\n-\t:      ## file ##<RESET>\n+\t:     ## file ##<RESET>\n \t:    <CYAN> @@<RESET>\n \t:      1<RESET>\n \t:<RED>3:  $(test_oid c3) <RESET><YELLOW>!<RESET><GREEN> 3:  $(test_oid m3)<RESET><YELLOW> s/11/B/<RESET>\n@@ -537,7 +537,7 @@ test_expect_success 'range-diff compares notes by default' '\n \t    -    topic note\n \t    +    unmodified note\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \tEOF\n \ttest_cmp expect actual\n@@ -584,7 +584,7 @@ test_expect_success 'range-diff with multiple --notes' '\n \t    -    topic note2\n \t    +    unmodified note2\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \tEOF\n \ttest_cmp expect actual\n@@ -645,7 +645,7 @@ test_expect_success 'format-patch --range-diff with --notes' '\n \t    -    topic note\n \t    +    unmodified note\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \tEOF\n \tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n@@ -674,7 +674,7 @@ test_expect_success 'format-patch --range-diff with format.notes config' '\n \t    -    topic note\n \t    +    unmodified note\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \tEOF\n \tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n@@ -710,7 +710,7 @@ test_expect_success 'format-patch --range-diff with multiple notes' '\n \t    -    topic note2\n \t    +    unmodified note2\n \t    Z\n-\t    Z ## file ##\n+\t    Z## file ##\n \t    Z@@ file: A\n \tEOF\n \tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n-- \n2.29.2\n\n"},{"id":"410139","messageId":"20201117213551.2539438-3-aclopte@gmail.com","threadId":"54578","inReplyTo":"20201117213551.2539438-1-aclopte@gmail.com","subject":"[PATCH 2/3] range-diff: ignore context-only changes","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2020-11-17T21:35:50Z","receivedAt":"2020-11-17T21:36:19Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"range-diff compares matching commits by comparing their patches against\neach other. When two patches only differ in their context lines, that\ndifference would still show up in range-diff's output.\n\nThis commit uses diff's new -I/--ignore-matching-lines regex logic to ignore\ndiff hunks that only consist of changes to context lines in the input diffs.\n\nThanks to the previous commit, lines like \"## file => renamed-file ##\"\nare not considered context lines because they no longer have a leading space.\n\nThis gives some extra @@ lines because we now always calculate\ntwo diffs: one for the patch metadata, like the commit message,\nand another one for the actual file changes.\nThis is because the former contains lines with leading spaces that are not\ncontext lines, so we never want to ignore them.\n---\n range-diff.c          | 58 +++++++++++++++++++++++++-----\n t/t3206-range-diff.sh | 83 ++++++-------------------------------------\n 2 files changed, 60 insertions(+), 81 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 72660453bd..df2147ef79 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -363,6 +363,31 @@ static void get_correspondences(struct string_list *a, struct string_list *b,\n \tfree(b2a);\n }\n \n+static int are_diffs_equivalent(const char *a_diff, const char *b_diff) {\n+\tfor (\n+\t\tconst char\n+\t\t\t*a_eol = strchr(a_diff, '\\n'),\n+\t\t\t*b_eol = strchr(b_diff,\t'\\n');\n+\t\t(a_eol = strchr(a_diff,\t'\\n')) &&\n+\t\t(b_eol = strchr(b_diff,\t'\\n'));\n+\t\ta_diff = a_eol + 1, b_diff = b_eol + 1\n+\t) {\n+\t\tif (!!a_eol != !!b_eol)\n+\t\t\treturn 0;\n+\n+\t\t// Ignore context lines.\n+\t\tif (a_diff[0] == ' ' &&\tb_diff[0] == ' ')\n+\t\t\tcontinue;\n+\n+\t\tsize_t a_len = a_eol - a_diff;\n+\t\tsize_t b_len = b_eol - b_diff;\n+\t\tif (a_len != b_len || strncmp(a_diff, b_diff, a_len))\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n static void output_pair_header(struct diff_options *diffopt,\n \t\t\t       int patch_no_width,\n \t\t\t       struct strbuf *buf,\n@@ -390,8 +415,10 @@ static void output_pair_header(struct diff_options *diffopt,\n \t} else if (!a_util) {\n \t\tcolor = color_new;\n \t\tstatus = '>';\n-\t} else if (strcmp(a_util->patch, b_util->patch)) {\n-\t\tcolor = color_commit;\n+\t} else if (a_util->diff_offset != b_util->diff_offset\n+\t\t   || strncmp(a_util->patch, b_util->patch, a_util->diff_offset)\n+\t\t   || !are_diffs_equivalent(a_util->diff, b_util->diff)) {\n+\t\tcolor =\tcolor_commit;\n \t\tstatus = '!';\n \t} else {\n \t\tcolor = color_commit;\n@@ -436,13 +463,13 @@ static struct userdiff_driver section_headers = {\n \t\t      \"^.?@@ (.*)$\", REG_EXTENDED }\n };\n \n-static struct diff_filespec *get_filespec(const char *name, const char *p)\n+static struct diff_filespec *get_filespec(const char *name, const char *p, size_t size)\n {\n \tstruct diff_filespec *spec = alloc_filespec(name);\n \n \tfill_filespec(spec, &null_oid, 0, 0100644);\n \tspec->data = (char *)p;\n-\tspec->size = strlen(p);\n+\tspec->size = size;\n \tspec->should_munmap = 0;\n \tspec->is_stdin = 1;\n \tspec->driver = &section_headers;\n@@ -450,11 +477,11 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n \treturn spec;\n }\n \n-static void patch_diff(const char *a, const char *b,\n+static void patch_diff(const char *a, size_t size_a, const char *b, size_t size_b,\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+\t\t   get_filespec(\"a\", a, size_a), get_filespec(\"b\", b, size_b));\n \n \tdiffcore_std(diffopt);\n \tdiff_flush(diffopt);\n@@ -467,6 +494,14 @@ static void output(struct string_list *a, struct string_list *b,\n \tint patch_no_width = decimal_width(1 + (a->nr > b->nr ? a->nr : b->nr));\n \tint i = 0, j = 0;\n \n+\tregex_t regex;\n+\tif (regcomp(&regex, \"^ \", REG_EXTENDED | REG_NEWLINE))\n+\t\tBUG(\"invalid regex\");\n+\tALLOC_GROW(diffopt->ignore_regex, diffopt->ignore_regex_nr + 1,\n+\t\t   diffopt->ignore_regex_alloc);\n+\tdiffopt->ignore_regex[diffopt->ignore_regex_nr] = &regex;\n+\tsize_t ignoring_context_only_changes = diffopt->ignore_regex_nr + 1;\n+\n \t/*\n \t * We assume the user is really more interested in the second argument\n \t * (\"newer\" version). To that end, we print the output in the order of\n@@ -504,9 +539,14 @@ static void output(struct string_list *a, struct string_list *b,\n \t\t\ta_util = a->items[b_util->matching].util;\n \t\t\toutput_pair_header(diffopt, patch_no_width,\n \t\t\t\t\t   &buf, &dashes, a_util, b_util);\n-\t\t\tif (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))\n-\t\t\t\tpatch_diff(a->items[b_util->matching].string,\n-\t\t\t\t\t   b->items[j].string, diffopt);\n+\t\t\tif (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT)) {\n+\t\t\t\tpatch_diff(a_util->patch, a_util->diff_offset, \n+\t\t\t\t\t\tb_util->patch, b_util->diff_offset, diffopt);\n+\t\t\t        diffopt->ignore_regex_nr = ignoring_context_only_changes;\n+\t\t\t\tpatch_diff(a_util->diff, strlen(a_util->diff), \n+\t\t\t\t\t\tb_util->diff, strlen(b_util->diff), diffopt);\n+\t\t\t        diffopt->ignore_regex_nr = ignoring_context_only_changes - 1;\n+\t\t\t}\n \t\t\ta_util->shown = 1;\n \t\t\tj++;\n \t\t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex f875843b5e..9a63388bee 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -223,16 +223,7 @@ test_expect_success 'changed commit' '\n \t      12\n \t      13\n \t      14\n-\t4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/\n-\t    @@ file\n-\t     @@ file: A\n-\t      9\n-\t      10\n-\t    - B\n-\t    + BB\n-\t     -12\n-\t     +B\n-\t      13\n+\t4:  $(test_oid t4) = 4:  $(test_oid c4) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -243,7 +234,7 @@ test_expect_success 'changed commit with --no-patch diff option' '\n \t1:  $(test_oid t1) = 1:  $(test_oid c1) s/5/A/\n \t2:  $(test_oid t2) = 2:  $(test_oid c2) s/4/A/\n \t3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/\n-\t4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/\n+\t4:  $(test_oid t4) = 4:  $(test_oid c4) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -256,9 +247,7 @@ test_expect_success 'changed commit with --stat diff option' '\n \t3:  $(test_oid t3) ! 3:  $(test_oid c3) s/11/B/\n \t     a => b | 2 +-\n \t     1 file changed, 1 insertion(+), 1 deletion(-)\n-\t4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/\n-\t     a => b | 2 +-\n-\t     1 file changed, 1 insertion(+), 1 deletion(-)\n+\t4:  $(test_oid t4) = 4:  $(test_oid c4) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -278,16 +267,7 @@ test_expect_success 'changed commit with sm config' '\n \t      12\n \t      13\n \t      14\n-\t4:  $(test_oid t4) ! 4:  $(test_oid c4) s/12/B/\n-\t    @@ file\n-\t     @@ file: A\n-\t      9\n-\t      10\n-\t    - B\n-\t    + BB\n-\t     -12\n-\t     +B\n-\t      13\n+\t4:  $(test_oid t4) = 4:  $(test_oid c4) s/12/B/\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -304,16 +284,14 @@ test_expect_success 'renamed file' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + rename file\n \t    Z\n+\t    @@\n \t    -## file ##\n \t    +## file => renamed-file ##\n \t    Z@@\n \t    Z 1\n \t    Z 2\n \t3:  $(test_oid t3) ! 3:  $(test_oid n3) s/11/B/\n-\t    @@ Metadata\n-\t    Z ## Commit message ##\n-\t    Z    s/11/B/\n-\t    Z\n+\t    @@\n \t    -## file ##\n \t    -@@ file: A\n \t    +## renamed-file ##\n@@ -322,10 +300,7 @@ test_expect_success 'renamed file' '\n \t    Z 9\n \t    Z 10\n \t4:  $(test_oid t4) ! 4:  $(test_oid n4) s/12/B/\n-\t    @@ Metadata\n-\t    Z ## Commit message ##\n-\t    Z    s/12/B/\n-\t    Z\n+\t    @@\n \t    -## file ##\n \t    -@@ file: A\n \t    +## renamed-file ##\n@@ -348,8 +323,6 @@ test_expect_success 'file with mode only change' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + add other-file\n \t    Z\n-\t    Z## file ##\n-\t    Z@@\n \t    @@ file\n \t    Z A\n \t    Z 6\n@@ -364,8 +337,6 @@ test_expect_success 'file with mode only change' '\n \t    -    s/11/B/\n \t    +    s/11/B/ + mode change other-file\n \t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \t    @@ file: A\n \t    Z 12\n \t    Z 13\n@@ -389,8 +360,6 @@ test_expect_success 'file added and later removed' '\n \t    -    s/4/A/\n \t    +    s/4/A/ + new-file\n \t    Z\n-\t    Z## file ##\n-\t    Z@@\n \t    @@ file\n \t    Z A\n \t    Z 6\n@@ -405,8 +374,6 @@ test_expect_success 'file added and later removed' '\n \t    -    s/11/B/\n \t    +    s/11/B/ + remove file\n \t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \t    @@ file: A\n \t    Z 12\n \t    Z 13\n@@ -434,9 +401,6 @@ test_expect_success 'changed message' '\n \t    Z\n \t    +    Also a silly comment here!\n \t    +\n-\t    Z## file ##\n-\t    Z@@\n-\t    Z 1\n \t3:  $(test_oid t3) = 3:  $(test_oid m3) s/11/B/\n \t4:  $(test_oid t4) = 4:  $(test_oid m4) s/12/B/\n \tEOF\n@@ -453,9 +417,6 @@ test_expect_success 'dual-coloring' '\n \t:     <RESET>\n \t:    <REVERSE><GREEN>+<RESET><BOLD>    Also a silly comment here!<RESET>\n \t:    <REVERSE><GREEN>+<RESET>\n-\t:     ## file ##<RESET>\n-\t:    <CYAN> @@<RESET>\n-\t:      1<RESET>\n \t:<RED>3:  $(test_oid c3) <RESET><YELLOW>!<RESET><GREEN> 3:  $(test_oid m3)<RESET><YELLOW> s/11/B/<RESET>\n \t:    <REVERSE><CYAN>@@<RESET> <RESET>file: A<RESET>\n \t:      9<RESET>\n@@ -466,16 +427,7 @@ test_expect_success 'dual-coloring' '\n \t:      12<RESET>\n \t:      13<RESET>\n \t:      14<RESET>\n-\t:<RED>4:  $(test_oid c4) <RESET><YELLOW>!<RESET><GREEN> 4:  $(test_oid m4)<RESET><YELLOW> s/12/B/<RESET>\n-\t:    <REVERSE><CYAN>@@<RESET> <RESET>file<RESET>\n-\t:    <CYAN> @@ file: A<RESET>\n-\t:      9<RESET>\n-\t:      10<RESET>\n-\t:    <REVERSE><RED>-<RESET><FAINT> BB<RESET>\n-\t:    <REVERSE><GREEN>+<RESET><BOLD> B<RESET>\n-\t:    <RED> -12<RESET>\n-\t:    <GREEN> +B<RESET>\n-\t:      13<RESET>\n+\t:<YELLOW>4:  d966c5c = 4:  8add5f1 s/12/B/<RESET>\n \tEOF\n \tgit range-diff changed...changed-message --color --dual-color >actual.raw &&\n \ttest_decode_color >actual <actual.raw &&\n@@ -537,8 +489,6 @@ test_expect_success 'range-diff compares notes by default' '\n \t    -    topic note\n \t    +    unmodified note\n \t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -584,8 +534,6 @@ test_expect_success 'range-diff with multiple --notes' '\n \t    -    topic note2\n \t    +    unmodified note2\n \t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \tEOF\n \ttest_cmp expect actual\n '\n@@ -644,11 +592,8 @@ test_expect_success 'format-patch --range-diff with --notes' '\n \t    Z ## Notes ##\n \t    -    topic note\n \t    +    unmodified note\n-\t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \tEOF\n-\tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n+\tsed \"/@@ Commit message/,/unmodified note\\$/!d\" 0000-* >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -673,11 +618,8 @@ test_expect_success 'format-patch --range-diff with format.notes config' '\n \t    Z ## Notes ##\n \t    -    topic note\n \t    +    unmodified note\n-\t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \tEOF\n-\tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n+\tsed \"/@@ Commit message/,/unmodified note\\$/!d\" 0000-* >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -709,11 +651,8 @@ test_expect_success 'format-patch --range-diff with multiple notes' '\n \t    Z ## Notes (note2) ##\n \t    -    topic note2\n \t    +    unmodified note2\n-\t    Z\n-\t    Z## file ##\n-\t    Z@@ file: A\n \tEOF\n-\tsed \"/@@ Commit message/,/@@ file: A/!d\" 0000-* >actual &&\n+\tsed \"/@@ Commit message/,/unmodified note2\\$/!d\" 0000-* >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.29.2\n\n"},{"id":"410140","messageId":"20201117213551.2539438-4-aclopte@gmail.com","threadId":"54578","inReplyTo":"20201117213551.2539438-1-aclopte@gmail.com","subject":"[PATCH 3/3] range-diff: only compute patch diff when patches are different","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2020-11-17T21:35:51Z","receivedAt":"2020-11-17T21:36:19Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"This is a pure optimization, probably with negligible impact. I'm not sure\nif it is a good idea because it could obscure future bugs.\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 df2147ef79..343a71d3eb 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -388,7 +388,7 @@ static int are_diffs_equivalent(const char *a_diff, const char *b_diff) {\n \treturn 1;\n }\n \n-static void output_pair_header(struct diff_options *diffopt,\n+static char output_pair_header(struct diff_options *diffopt,\n \t\t\t       int patch_no_width,\n \t\t\t       struct strbuf *buf,\n \t\t\t       struct strbuf *dashes,\n@@ -456,6 +456,8 @@ static void output_pair_header(struct diff_options *diffopt,\n \tstrbuf_addf(buf, \"%s\\n\", color_reset);\n \n \tfwrite(buf->buf, buf->len, 1, diffopt->file);\n+\n+\treturn status;\n }\n \n static struct userdiff_driver section_headers = {\n@@ -537,9 +539,9 @@ static void output(struct string_list *a, struct string_list *b,\n \t\t/* Show matching LHS/RHS pair. */\n \t\tif (j < b->nr) {\n \t\t\ta_util = a->items[b_util->matching].util;\n-\t\t\toutput_pair_header(diffopt, patch_no_width,\n+\t\t\tchar status = output_pair_header(diffopt, patch_no_width,\n \t\t\t\t\t   &buf, &dashes, a_util, b_util);\n-\t\t\tif (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT)) {\n+\t\t\tif (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT) && status != '=') {\n \t\t\t\tpatch_diff(a_util->patch, a_util->diff_offset, \n \t\t\t\t\t\tb_util->patch, b_util->diff_offset, diffopt);\n \t\t\t        diffopt->ignore_regex_nr = ignoring_context_only_changes;\n-- \n2.29.2\n\n"},{"id":"410175","messageId":"CAPig+cR3XRWYmRTETWfEMSdg+Ri-L0LZzhNMavg4FCkDC19qdA@mail.gmail.com","threadId":"54578","inReplyTo":"20201117213551.2539438-3-aclopte@gmail.com","subject":"Re: [PATCH 2/3] range-diff: ignore context-only changes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-17T22:56:31Z","receivedAt":"2020-11-17T22:57:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 17, 2020 at 4:38 PM Johannes Altmanninger <aclopte@gmail.com> wrote:\n> range-diff compares matching commits by comparing their patches against\n> each other. When two patches only differ in their context lines, that\n> difference would still show up in range-diff's output.\n>\n> This commit uses diff's new -I/--ignore-matching-lines regex logic to ignore\n> diff hunks that only consist of changes to context lines in the input diffs.\n>\n> Thanks to the previous commit, lines like \"## file => renamed-file ##\"\n> are not considered context lines because they no longer have a leading space.\n>\n> This gives some extra @@ lines because we now always calculate\n> two diffs: one for the patch metadata, like the commit message,\n> and another one for the actual file changes.\n> This is because the former contains lines with leading spaces that are not\n> context lines, so we never want to ignore them.\n> ---\n\nSigned-off-by: is missing from all of your patches.\n\nJust a few lightweight style-related review comments below (I didn't\nread the patch any deeper than that)...\n\n> diff --git a/range-diff.c b/range-diff.c\n> @@ -363,6 +363,31 @@ static void get_correspondences(struct string_list *a, struct string_list *b,\n> +static int are_diffs_equivalent(const char *a_diff, const char *b_diff) {\n> +       for (\n> +               const char\n> +                       *a_eol = strchr(a_diff, '\\n'),\n> +                       *b_eol = strchr(b_diff, '\\n');\n> +               (a_eol = strchr(a_diff, '\\n')) &&\n> +               (b_eol = strchr(b_diff, '\\n'));\n> +               a_diff = a_eol + 1, b_diff = b_eol + 1\n> +       ) {\n\nThis project doesn't yet declare variable as part of 'for', so:\n\n    const char *a_eol = ...;\n    const char *b_eol = ...;\n    for ( ; (a_eol = ...) & (b_eol = ...); a_diff = ..., b_diff = ...) {\n\n> +               if (!!a_eol != !!b_eol)\n> +                       return 0;\n> +\n> +               // Ignore context lines.\n> +               if (a_diff[0] == ' ' && b_diff[0] == ' ')\n> +                       continue;\n\nAvoid //-style comments. Use /* comments */ instead.\n\n> +               size_t a_len = a_eol - a_diff;\n> +               size_t b_len = b_eol - b_diff;\n\nThis project doesn't yet allow mixing declarations and code. Instead\nplace the declarations at the top of the scope:\n\n    for (...) {\n        size_t a_len;\n        size_t b_len;\n        ...\n        a_len = ...;\n        b_len = ...;\n\n> @@ -390,8 +415,10 @@ static void output_pair_header(struct diff_options *diffopt,\n> -       } else if (strcmp(a_util->patch, b_util->patch)) {\n> -               color = color_commit;\n> +       } else if (a_util->diff_offset != b_util->diff_offset\n> +                  || strncmp(a_util->patch, b_util->patch, a_util->diff_offset)\n> +                  || !are_diffs_equivalent(a_util->diff, b_util->diff)) {\n> +               color = color_commit;\n\nStyle on this project is to break line after || operator rather than before:\n\n    if (... ||\n        ... ||\n        ...) {\n\n> @@ -467,6 +494,14 @@ static void output(struct string_list *a, struct string_list *b,\n> +       regex_t regex;\n> +       if (regcomp(&regex, \"^ \", REG_EXTENDED | REG_NEWLINE))\n> +               BUG(\"invalid regex\");\n> +       ALLOC_GROW(diffopt->ignore_regex, diffopt->ignore_regex_nr + 1,\n> +                  diffopt->ignore_regex_alloc);\n> +       diffopt->ignore_regex[diffopt->ignore_regex_nr] = &regex;\n> +       size_t ignoring_context_only_changes = diffopt->ignore_regex_nr + 1;\n\nShould you be calling regfree(&regex) at the end of the function?\n"}]}