{"thread":{"id":"54333","subject":"[PATCH 2/2] t: add -I tests","startedAt":"2020-10-01T12:45:04Z","lastAt":"2021-02-11T10:50:35Z","messageCount":57,"participants":["Michał Kępień","Junio C Hamano","Johannes Schindelin","Phillip Wood","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"406758","messageId":"20201001120606.25773-3-michal@isc.org","threadId":"54333","inReplyTo":"20201001120606.25773-1-michal@isc.org","subject":"[PATCH 2/2] t: add -I tests","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-01T12:06:06Z","receivedAt":"2020-10-01T12:45:04Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Exercise the new 'git diff -I' option in various scenarios to\nensure it behaves as expected.\n\nSigned-off-by: Michał Kępień \n---\n t/t4069-diff-ignore-regex.sh | 330 +++++++++++++++++++++++++++++++++++\n 1 file changed, 330 insertions(+)\n create mode 100755 t/t4069-diff-ignore-regex.sh\n\ndiff --git a/t/t4069-diff-ignore-regex.sh b/t/t4069-diff-ignore-regex.sh\nnew file mode 100755\nindex 0000000000..323687f1dc\n--- /dev/null\n+++ b/t/t4069-diff-ignore-regex.sh\n@@ -0,0 +1,330 @@\n+#!/bin/sh\n+\n+test_description='Test diff -I'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+test_expect_success setup '\n+\ttest_seq 20 >x &&\n+\tgit update-index --add x\n+'\n+\n+test_expect_success 'one line changed' '\n+\ttest_seq 20 | sed \"s/10/100/\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -7,7 +7,7 @@\n+\t 7\n+\t 8\n+\t 9\n+\t-10\n+\t+100\n+\t 11\n+\t 12\n+\t 13\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both old and new line match regex - ignore change\n+\tgit diff -I \"^10\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only old line matches regex - do not ignore change\n+\tgit diff -I \"^10\\$\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Only new line matches regex - do not ignore change\n+\tgit diff -I \"^100\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'one line removed' '\n+\ttest_seq 20 | sed \"10d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -7,7 +7,6 @@\n+\t 7\n+\t 8\n+\t 9\n+\t-10\n+\t 11\n+\t 12\n+\t 13\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Removed line matches regex - ignore change\n+\tgit diff -I \"^10\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Removed line does not match regex - do not ignore change\n+\tgit diff -I \"^101\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'one line added' '\n+\ttest_seq 21 >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -18,3 +18,4 @@\n+\t 18\n+\t 19\n+\t 20\n+\t+21\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Added line matches regex - ignore change\n+\tgit diff -I \"^21\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Added line does not match regex - do not ignore change\n+\tgit diff -I \"^212\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'last two lines changed' '\n+\ttest_seq 20 | sed \"s/19/21/; s/20/22/\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -16,5 +16,5 @@\n+\t 16\n+\t 17\n+\t 18\n+\t-19\n+\t-20\n+\t+21\n+\t+22\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# All changed lines match regex - ignore change\n+\tgit diff -I \"^[12]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Not all changed lines match regex - do not ignore change\n+\tgit diff -I \"^2\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'two non-adjacent lines removed in the same hunk' '\n+\ttest_seq 20 | sed \"1d; 3d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,6 +1,4 @@\n+\t-1\n+\t 2\n+\t-3\n+\t 4\n+\t 5\n+\t 6\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both removed lines match regex - ignore hunk\n+\tgit diff -I \"^[1-3]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# First removed line does not match regex - do not ignore hunk\n+\tgit diff -I \"^[2-3]\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Second removed line does not match regex - do not ignore hunk\n+\tgit diff -I \"^[1-2]\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'two non-adjacent lines removed in the same hunk, with -U1' '\n+\ttest_seq 20 | sed \"1d; 3d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff -U1 >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,2 @@\n+\t-1\n+\t 2\n+\t-3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both removed lines match regex - ignore hunk\n+\tgit diff -U1 -I \"^[1-3]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# First removed line does not match regex, but is out of context - ignore second change\n+\tgit diff -U1 -I \"^[2-3]\" >actual &&\n+\tcat >second-change-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,2 +1 @@\n+\t-1\n+\t 2\n+\tEOF\n+\tcompare_diff_patch second-change-ignored actual &&\n+\n+\t# Second removed line does not match regex, but is out of context - ignore first change\n+\tgit diff -U1 -I \"^[1-2]\" >actual &&\n+\tcat >first-change-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -2,3 +1,2 @@\n+\t 2\n+\t-3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch first-change-ignored actual\n+'\n+\n+test_expect_success 'multiple hunks' '\n+\ttest_seq 20 | sed \"1d; 20d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,3 @@\n+\t-1\n+\t 2\n+\t 3\n+\t 4\n+\t@@ -17,4 +16,3 @@\n+\t 17\n+\t 18\n+\t 19\n+\t-20\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Ignore both hunks\n+\tgit diff -I \"^[12]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only ignore first hunk\n+\tgit diff -I \"^1\" >actual &&\n+\tcat >first-hunk-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -17,4 +16,3 @@\n+\t 17\n+\t 18\n+\t 19\n+\t-20\n+\tEOF\n+\tcompare_diff_patch first-hunk-ignored actual &&\n+\n+\t# Only ignore second hunk\n+\tgit diff -I \"^2\" >actual &&\n+\tcat >second-hunk-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,3 @@\n+\t-1\n+\t 2\n+\t 3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch second-hunk-ignored actual\n+'\n+\n+test_expect_success 'multiple hunks, with --ignore-blank-lines' '\n+\techo >x &&\n+\ttest_seq 21 >>x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,3 +1,4 @@\n+\t+\n+\t 1\n+\t 2\n+\t 3\n+\t@@ -18,3 +19,4 @@\n+\t 18\n+\t 19\n+\t 20\n+\t+21\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# -I does not override --ignore-blank-lines - ignore both hunks\n+\tgit diff --ignore-blank-lines -I ^21 >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'diffstat' '\n+\ttest_seq 20 | sed \"s/^5/0/p; s/^15/10/; 16d\" >x &&\n+\n+\t# Get plain diffstat\n+\tgit diff --stat >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 6 +++---\n+\t 1 file changed, 3 insertions(+), 3 deletions(-)\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# Ignore both hunks\n+\tgit diff --stat -I \"^[0-5]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only ignore first hunk\n+\tgit diff --stat -I \"^[05]\" >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 3 +--\n+\t 1 file changed, 1 insertion(+), 2 deletions(-)\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# Only ignore second hunk\n+\tgit diff --stat -I \"^1\" >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 3 ++-\n+\t 1 file changed, 2 insertions(+), 1 deletion(-)\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'invalid regex' '\n+\t>x &&\n+\tgit diff -I \"^[1\" 2>&1 | grep \"invalid regex: \"\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"406759","messageId":"20201001120606.25773-2-michal@isc.org","threadId":"54333","inReplyTo":"20201001120606.25773-1-michal@isc.org","subject":"[PATCH 1/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-01T12:06:05Z","receivedAt":"2020-10-01T12:45:05Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Add a new diff option that enables ignoring changes whose all lines\n(changed, removed, and added) match a given regular expression. This is\nsimilar to the -I option in standalone diff utilities and can be used\ne.g. to look for unrelated changes in commits containing a large number\nof automatically applied modifications (e.g. a tree-wide string\nreplacement). The difference between -G/-S and the new -I option is\nthat the latter filters output on a per-change basis.\n\nUse the 'ignore' field of xdchange_t for marking a change as ignored or\nnot. Since the same field is used by --ignore-blank-lines, identical\nhunk emitting rules apply for --ignore-blank-lines and -I. These two\noptions can also be used together at the same time.\n\nApart from adding a new field to struct diff_options, also define a new\nflag, XDF_IGNORE_REGEX, for the 'xdl_opts' field of that structure.\nThis is done because the xpparam_t structure (which is used for passing\naround the regular expression supplied to -I) is not zeroed out in all\ncall stacks involving xdl_diff() and thus only performing a NULL check\non xpp->ignore_regex could result in xdl_mark_ignorable_regex() treating\ngarbage on the stack as a regular expression. As the 'flags' field of\nxpparam_t is initialized in all call stacks involving xdl_diff(), adding\na flag check avoids that problem.\n\nSigned-off-by: Michał Kępień \n---\n Documentation/diff-options.txt | 3 +++\n diff.c | 16 +++++++++++++++\n diff.h | 2 ++\n xdiff/xdiff.h | 2 ++\n xdiff/xdiffi.c | 36 ++++++++++++++++++++++++++++++++++\n 5 files changed, 59 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 573fb9bb71..a7ef3f5645 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -687,6 +687,9 @@ endif::git-format-patch[]\n --ignore-blank-lines::\n \tIgnore changes whose lines are all blank.\n \n+-I::\n+\tIgnore changes whose all lines match .\n+\n --inter-hunk-context=::\n \tShow the context between diff hunks, up to the specified number\n \tof lines, thereby fusing hunks that are close to each other.\ndiff --git a/diff.c b/diff.c\nindex 2bb2f8f57e..c9603c941e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3587,6 +3587,7 @@ static void builtin_diff(const char *name_a,\n \t\tif (header.len && !o->flags.suppress_diff_headers)\n \t\t\tecbdata.header = &header;\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -3716,6 +3717,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -5203,6 +5205,17 @@ static int diff_opt_patience(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_ignore_regex(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct diff_options *options = opt->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\toptions->xdl_opts |= XDF_IGNORE_REGEX;\n+\toptions->ignore_regex = arg;\n+\treturn 0;\n+}\n+\n static int diff_opt_pickaxe_regex(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -5491,6 +5504,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n \t\t\t N_(\"ignore changes whose lines are all blank\"),\n \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F('I', NULL, options, N_(\"\"),\n+\t\t\t N_(\"ignore changes whose all lines match \"),\n+\t\t\t 0, diff_opt_ignore_regex),\n \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n \t\t\tXDF_INDENT_HEURISTIC),\ndiff --git a/diff.h b/diff.h\nindex 3de343270f..0b8871c0c1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -234,6 +234,8 @@ struct diff_options {\n \t */\n \tconst char *pickaxe;\n \n+\tconst char *ignore_regex;\n+\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tconst char *line_prefix;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 032e3a9f41..db28055a5e 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -40,6 +40,7 @@ extern \"C\" {\n \t\t\t XDF_IGNORE_CR_AT_EOL)\n \n #define XDF_IGNORE_BLANK_LINES (1 << 7)\n+#define XDF_IGNORE_REGEX (1 << 8)\n \n #define XDF_PATIENCE_DIFF (1 << 14)\n #define XDF_HISTOGRAM_DIFF (1 << 15)\n@@ -78,6 +79,7 @@ typedef struct s_mmbuffer {\n \n typedef struct s_xpparam {\n \tunsigned long flags;\n+\tconst char *ignore_regex;\n \n \t/* See Documentation/diff-options.txt. */\n \tchar **anchors;\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex bd035139f9..13f7ab95ac 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -1019,6 +1019,39 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n \t}\n }\n \n+static void\n+xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n+\t\t\t const char *ignore_regex)\n+{\n+\txdchange_t *xch;\n+\tregex_t regex;\n+\n+\tif (regcomp(®ex, ignore_regex, REG_EXTENDED | REG_NEWLINE))\n+\t\tdie(\"invalid regex: %s\", ignore_regex);\n+\n+\tfor (xch = xscr; xch; xch = xch->next) {\n+\t\tregmatch_t regmatch;\n+\t\txrecord_t **rec;\n+\t\tint ignore = 1;\n+\t\tlong i;\n+\n+\t\trec = &xe->xdf1.recs[xch->i1];\n+\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n+\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n+\t\t\t\t\t 1, ®match, 0);\n+\n+\t\trec = &xe->xdf2.recs[xch->i2];\n+\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n+\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n+\t\t\t\t\t 1, ®match, 0);\n+\n+\t\t/*\n+\t\t * Do not override --ignore-blank-lines.\n+\t\t */\n+\t\txch->ignore |= ignore;\n+\t}\n+}\n+\n int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n \txdchange_t *xscr;\n@@ -1040,6 +1073,9 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n \t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n \n+\t\tif ((xpp->flags & XDF_IGNORE_REGEX) && xpp->ignore_regex)\n+\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp->ignore_regex);\n+\n \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n \n \t\t\txdl_free_script(xscr);\n-- \n2.28.0\n\n"},{"id":"406760","messageId":"20201001120606.25773-1-michal@isc.org","threadId":"54333","inReplyTo":null,"subject":"[PATCH 0/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-01T12:06:04Z","receivedAt":"2020-10-01T12:45:06Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"This patch series adds a new diff option that enables ignoring changes\nwhose all lines (changed, removed, and added) match a given regular\nexpression. This is similar to the -I option in standalone diff\nutilities and can be used e.g. to look for unrelated changes in commits\ncontaining a large number of automatically applied modifications (e.g. a\ntree-wide string replacement). The difference between -G/-S and the new\n-I option is that the latter filters output on a per-change basis.\n\nI personally found this feature handy quite a few times while reviewing\npatches and a quick web search yields some questions from other people\nlooking for something like this, so I thought I would take a shot at\nsubmitting it upstream.\n\nPlease note that while this patch series in its current shape builds,\nseems to pass tests, and appears to work as I would expect it to, this\nis my first attempt at contributing to Git and I did come across a few\nstumbling blocks:\n\n - Instead of adding an extra flag for 'xdl_opts', would it be better\n to make sure xpparam_t structures are always zero-initialized before\n xdl_diff() is called (see commit message for patch 1 for context)?\n\n - If this patch series is accepted, should xdl_mark_ignorable() be\n renamed to something like xdl_mark_ignorable_blank() for slightly\n improved code clarity?\n\n - Would a more thorough explanation of the option help or hurt?\n\n - Given that hunk emitting rules are the same for the -I option\n proposed in this patch series as for --ignore-blank-lines, is it\n necessary to duplicate the (impressively extensive!)\n --inter-hunk-context tests in t/t4015-diff-whitespace.sh for -I? I\n decided against doing that for the time being (though I did add some\n basic tests for such scenarios in patch 2, see the \"with -U1\" test).\n\n - Should tests be added in a separate commit? This is what I did as I\n thought it would help with readability, but...\n\nThanks in advance for taking a look!\n\nMichał Kępień (2):\n diff: add -I that ignores matching changes\n t: add -I tests\n\n Documentation/diff-options.txt | 3 +\n diff.c | 16 ++\n diff.h | 2 +\n t/t4069-diff-ignore-regex.sh | 330 +++++++++++++++++++++++++++++++++\n xdiff/xdiff.h | 2 +\n xdiff/xdiffi.c | 36 ++++\n 6 files changed, 389 insertions(+)\n create mode 100755 t/t4069-diff-ignore-regex.sh\n\n-- \n2.28.0\n\n"},{"id":"406783","messageId":"xmqq8scpdgl8.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201001120606.25773-1-michal@isc.org","subject":"Re: [PATCH 0/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T17:02:59Z","receivedAt":"2020-10-01T17:03:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> This patch series adds a new diff option that enables ignoring changes\n> whose all lines (changed, removed, and added) match a given regular\n> expression. This is similar to the -I option in standalone diff\n> utilities and can be used e.g. to look for unrelated changes in commits\n> containing a large number of automatically applied modifications (e.g. a\n> tree-wide string replacement). The difference between -G/-S and the new\n> -I option is that the latter filters output on a per-change basis.\n\nI am uncomfortably excited to see a change to quite a low-level code\nof the diff machinery. It does not happen every day ;-)\n\n"},{"id":"406786","messageId":"xmqqy2kpbye9.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201001120606.25773-2-michal@isc.org","subject":"Re: [PATCH 1/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-01T18:21:18Z","receivedAt":"2020-10-01T18:21:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> Apart from adding a new field to struct diff_options, also define a new\n> flag, XDF_IGNORE_REGEX, for the 'xdl_opts' field of that structure.\n> This is done because the xpparam_t structure (which is used for passing\n> around the regular expression supplied to -I) is not zeroed out in all\n> call stacks involving xdl_diff() and thus only performing a NULL check\n> on xpp->ignore_regex could result in xdl_mark_ignorable_regex() treating\n> garbage on the stack as a regular expression. As the 'flags' field of\n> xpparam_t is initialized in all call stacks involving xdl_diff(), adding\n> a flag check avoids that problem.\n\nYou mentioned in your cover letter about this, and I tend to agree\nthat this feels like a hack, at least from the first glance. The\nnext feature that wants to have an optional pointer in xpparam_t and\nhave the code behave differently with the data pointed by it would\nneed to waste another bit the same way. Do you already know (read:\nI am not asking you to dig to find out immediately---just asking if\nyou already know the answer) if there is an inherent reason why they\ncannot be memset(0) before use? It seems like a much better\napproach to ensure that we clear the structure. Doesn't existing\nanchors array share the same issue (at least anchors_nr must be\ncleared if there is no interesting anchors) already? IOW, should\n\"git grep anchors_nr\" be a valid way to identify _all_ places where\nyou need to clear the ignore_regex field?\n\n> +-I::\n> +\tIgnore changes whose all lines match .\n> +\n\nThe implementation seems to allow only one regex, but I suspect we'd\nwant to mimic\n\n $ printf \"%s\\n\" a a a >test_a\n $ printf \"%s\\n\" b b b >test_b\n $ diff -Ia test_a test_b\n $ diff -Ib test_a test_b\n $ diff -Ia -Ib test_a test_b\n\nand until that happens, the explanation needs to say something about\nearlier being discarded when this option is given more than\nonce.\n\n> @@ -5203,6 +5205,17 @@ static int diff_opt_patience(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static int diff_opt_ignore_regex(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct diff_options *options = opt->value;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\toptions->xdl_opts |= XDF_IGNORE_REGEX;\n> +\toptions->ignore_regex = arg;\n\nWhen given twice or more, the earlier value gets lost (it does not\nleak, luckily, though).\n\n> +\treturn 0;\n> +}\n\nIf we somehow can lose the use of XDF_IGNORE_REGEX bit, we do not\nhave to have this as a callback. Instead, it can be OPT_STRING with\nthe current semantics of \"only the last one is used\", or we can use\nOPT_STRING_LIST to keep all the expressions.\n\nOn the other hand, I wonder if it would be a valid approach to make\nthe new field(s) in diff_options a \"regex_t *ignore_regex\" (and add\nan \"int ignore_regex_nr\" next to it, if we shoot for multiple -I\noptions), instead of \"const char *\". For that, we would need a\ncallback even without XDF_IGNORE_REGEX bit having to futz with\nxdl_opts field.\n\nDoing so would give us a chance to compile and notice a malformed\nregular expression in diff.c, before it hits xdiff/ layer, which may\nor may not be a good thing.\n\n> @@ -1019,6 +1019,39 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> \t}\n> }\n\nI agree with what you said in the cover letter that it would be a\ngood idea to name the existing xdl_mark_ignorable() and the new one\nin such a way that they look similar and parallel, by renaming the\nformer to xdl_mark_ignorable_lines or something.\n\n> +static void\n> +xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n> +\t\t\t const char *ignore_regex)\n\nI know some coding standard do chomp line immediately before the\nfunction name (I grew up with one), but that is not what this\nproject uses, and judging from the surrounding code, it is not what\nthe upstream xdiff source we borrowed uses, either.\n\n> +{\n> +\txdchange_t *xch;\n> +\tregex_t regex;\n> +\n> +\tif (regcomp(®ex, ignore_regex, REG_EXTENDED | REG_NEWLINE))\n> +\t\tdie(\"invalid regex: %s\", ignore_regex);\n\nIf we compile in diff.c layer and pass regex_t down here, we won't\nhave to fail here this deep in the callchain.\n\n> +\tfor (xch = xscr; xch; xch = xch->next) {\n> +\t\tregmatch_t regmatch;\n> +\t\txrecord_t **rec;\n> +\t\tint ignore = 1;\n> +\t\tlong i;\n> +\n> +\t\trec = &xe->xdf1.recs[xch->i1];\n> +\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n> +\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n> +\t\t\t\t\t 1, ®match, 0);\n> +\t\trec = &xe->xdf2.recs[xch->i2];\n> +\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n> +\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n> +\t\t\t\t\t 1, ®match, 0);\n> +\n> +\t\t/*\n> +\t\t * Do not override --ignore-blank-lines.\n> +\t\t */\n> +\t\txch->ignore |= ignore;\n\nWell, you could optimize out the whole regexp matching by adding\n\n\t\tif (xch->ignore)\n\t\t\tcontinue;\n\nbefore the two loops try to find a non-matching line, no?\n\n> +\t}\n> +}\n> +\n> int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n> \txdchange_t *xscr;\n> @@ -1040,6 +1073,9 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n> \t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n> \n> +\t\tif ((xpp->flags & XDF_IGNORE_REGEX) && xpp->ignore_regex)\n> +\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp->ignore_regex);\n> +\n> \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n> \n> \t\t\txdl_free_script(xscr);\n\nThanks for an exciting read ;-)\n\n"},{"id":"407094","messageId":"20201007194821.GA20549@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqy2kpbye9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-07T19:48:21Z","receivedAt":"2020-10-07T19:48:27Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"> > Apart from adding a new field to struct diff_options, also define a new\n> > flag, XDF_IGNORE_REGEX, for the 'xdl_opts' field of that structure.\n> > This is done because the xpparam_t structure (which is used for passing\n> > around the regular expression supplied to -I) is not zeroed out in all\n> > call stacks involving xdl_diff() and thus only performing a NULL check\n> > on xpp->ignore_regex could result in xdl_mark_ignorable_regex() treating\n> > garbage on the stack as a regular expression. As the 'flags' field of\n> > xpparam_t is initialized in all call stacks involving xdl_diff(), adding\n> > a flag check avoids that problem.\n> \n> You mentioned in your cover letter about this, and I tend to agree\n> that this feels like a hack, at least from the first glance. The\n> next feature that wants to have an optional pointer in xpparam_t and\n> have the code behave differently with the data pointed by it would\n> need to waste another bit the same way.\n\nAgreed.\n\n> Do you already know (read:\n> I am not asking you to dig to find out immediately---just asking if\n> you already know the answer) if there is an inherent reason why they\n> cannot be memset(0) before use? It seems like a much better\n> approach to ensure that we clear the structure.\n\nI do not know of any reason for xpparam_t structures to not be\nzero-initialized. In fact, they _are_ zero-initialized most of the\ntime; AFAICT, there are just three places in the tree in which they are\nnot.\n\nWould you like me to address that in a separate patch in this patch\nseries?\n\n> Doesn't existing\n> anchors array share the same issue (at least anchors_nr must be\n> cleared if there is no interesting anchors) already? IOW, should\n> \"git grep anchors_nr\" be a valid way to identify _all_ places where\n> you need to clear the ignore_regex field?\n\nYes, the 'anchors' and 'anchors_nr' fields of xpparam_t are also\naffected by the same problem, but the symptoms are more benign in their\ncase because these fields are only used in xdiff/xpatience.c, i.e. when\nXDF_PATIENCE_DIFF is set in 'flags' - and as I already mentioned in\ncommit messages, that field is always initialized properly, so there is\ncurrently no practical possibility of stack garbage being interpreted as\ne.g. a pointer to the anchor array.\n\n> > +-I::\n> > +\tIgnore changes whose all lines match .\n> > +\n> \n> The implementation seems to allow only one regex, but I suspect we'd\n> want to mimic\n> \n> $ printf \"%s\\n\" a a a >test_a\n> $ printf \"%s\\n\" b b b >test_b\n> $ diff -Ia test_a test_b\n> $ diff -Ib test_a test_b\n> $ diff -Ia -Ib test_a test_b\n\nAh, sure. After skimming through various man pages available online, I\nwas not sure whether all standalone versions of diff which support -I\nallow that switch to be used multiple times and I thought it would be\nbetter to start with the simplest thing possible. If you would rather\nhave the above implemented immediately, I can sure try that in v2.\n\n> and until that happens, the explanation needs to say something about\n> earlier being discarded when this option is given more than\n> once.\n\nSorry, where do I look for that explanation? I tried to make -I behave\nsimilarly to -G and -S, each of which can be specified more than once,\nbut the last value provided overrides the earlier ones - and I could not\nfind any mention of that behavior in the docs. Please help?\n\n> > @@ -5203,6 +5205,17 @@ static int diff_opt_patience(const struct option *opt,\n> > \treturn 0;\n> > }\n> > \n> > +static int diff_opt_ignore_regex(const struct option *opt,\n> > +\t\t\t\t const char *arg, int unset)\n> > +{\n> > +\tstruct diff_options *options = opt->value;\n> > +\n> > +\tBUG_ON_OPT_NEG(unset);\n> > +\toptions->xdl_opts |= XDF_IGNORE_REGEX;\n> > +\toptions->ignore_regex = arg;\n> \n> When given twice or more, the earlier value gets lost (it does not\n> leak, luckily, though).\n\nRight, as I mentioned above, I just wanted to start with something\nsimple that resembles -G/-S. I will revise this part in v2 if you would\nlike to see support for multiple regular expressions in this patch\nseries.\n\n> > +\treturn 0;\n> > +}\n> \n> If we somehow can lose the use of XDF_IGNORE_REGEX bit, we do not\n> have to have this as a callback. Instead, it can be OPT_STRING with\n> the current semantics of \"only the last one is used\", or we can use\n> OPT_STRING_LIST to keep all the expressions.\n\nI see, thanks for the hints!\n\n> On the other hand, I wonder if it would be a valid approach to make\n> the new field(s) in diff_options a \"regex_t *ignore_regex\" (and add\n> an \"int ignore_regex_nr\" next to it, if we shoot for multiple -I\n> options), instead of \"const char *\". For that, we would need a\n> callback even without XDF_IGNORE_REGEX bit having to futz with\n> xdl_opts field.\n> \n> Doing so would give us a chance to compile and notice a malformed\n> regular expression in diff.c, before it hits xdiff/ layer, which may\n> or may not be a good thing.\n\nI have not thought about this approach before, but it sounds elegant to\nme, at least at first glance - option parsing code sounds like the right\nplace for sanitizing user-provided parameters. Doubly so if we take the\nmultiple -I options approach. I will try this in v2.\n\n> > @@ -1019,6 +1019,39 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> > \t}\n> > }\n> \n> I agree with what you said in the cover letter that it would be a\n> good idea to name the existing xdl_mark_ignorable() and the new one\n> in such a way that they look similar and parallel, by renaming the\n> former to xdl_mark_ignorable_lines or something.\n\nThen I will do that in v2. Separate patch?\n\n> > +static void\n> > +xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n> > +\t\t\t const char *ignore_regex)\n> \n> I know some coding standard do chomp line immediately before the\n> function name (I grew up with one), but that is not what this\n> project uses, and judging from the surrounding code, it is not what\n> the upstream xdiff source we borrowed uses, either.\n\nOh, right, sorry - force of habit. I will fix this in v2.\n\n> > +{\n> > +\txdchange_t *xch;\n> > +\tregex_t regex;\n> > +\n> > +\tif (regcomp(®ex, ignore_regex, REG_EXTENDED | REG_NEWLINE))\n> > +\t\tdie(\"invalid regex: %s\", ignore_regex);\n> \n> If we compile in diff.c layer and pass regex_t down here, we won't\n> have to fail here this deep in the callchain.\n\nAgreed - I already responded to this remark above.\n\n> > +\tfor (xch = xscr; xch; xch = xch->next) {\n> > +\t\tregmatch_t regmatch;\n> > +\t\txrecord_t **rec;\n> > +\t\tint ignore = 1;\n> > +\t\tlong i;\n> > +\n> > +\t\trec = &xe->xdf1.recs[xch->i1];\n> > +\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n> > +\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n> > +\t\t\t\t\t 1, ®match, 0);\n> > +\t\trec = &xe->xdf2.recs[xch->i2];\n> > +\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n> > +\t\t\tignore = !regexec_buf(®ex, rec[i]->ptr, rec[i]->size,\n> > +\t\t\t\t\t 1, ®match, 0);\n> > +\n> > +\t\t/*\n> > +\t\t * Do not override --ignore-blank-lines.\n> > +\t\t */\n> > +\t\txch->ignore |= ignore;\n> \n> Well, you could optimize out the whole regexp matching by adding\n> \n> \t\tif (xch->ignore)\n> \t\t\tcontinue;\n> \n> before the two loops try to find a non-matching line, no?\n\nAh, of course, it looks so obvious now that you pointed it out :-) I\nguess I was copy-past^W^W^Wtrying to make xdl_mark_ignorable_regex()\nlook similar to xdl_mark_ignorable(). I will use your suggestion in v2.\n\n> > +\t}\n> > +}\n> > +\n> > int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> > \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n> > \txdchange_t *xscr;\n> > @@ -1040,6 +1073,9 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> > \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n> > \t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n> > \n> > +\t\tif ((xpp->flags & XDF_IGNORE_REGEX) && xpp->ignore_regex)\n> > +\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp->ignore_regex);\n> > +\n> > \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n> > \n> > \t\t\txdl_free_script(xscr);\n> \n> Thanks for an exciting read ;-)\n\nMy pleasure ;-) And thanks for taking a look. Sorry about the long\nturnaround time, but unfortunately this is the best I can do at the\nmoment.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407095","messageId":"xmqqft6p240j.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201007194821.GA20549@larwa.hq.kempniu.pl","subject":"Re: [PATCH 1/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T20:08:12Z","receivedAt":"2020-10-07T20:08:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n>> cannot be memset(0) before use? It seems like a much better\n>> approach to ensure that we clear the structure.\n>\n> I do not know of any reason for xpparam_t structures to not be\n> zero-initialized. In fact, they _are_ zero-initialized most of the\n> time; AFAICT, there are just three places in the tree in which they are\n> not.\n>\n> Would you like me to address that in a separate patch in this patch\n> series?\n\nYes, as a preliminary clean-up patch before the real/fun stuff ;-)\n\n>> > +-I::\n>> > +\tIgnore changes whose all lines match .\n>> > +\n>> \n>> The implementation seems to allow only one regex, but I suspect we'd\n>> want to mimic\n>> \n>> $ printf \"%s\\n\" a a a >test_a\n>> $ printf \"%s\\n\" b b b >test_b\n>> $ diff -Ia test_a test_b\n>> $ diff -Ib test_a test_b\n>> $ diff -Ia -Ib test_a test_b\n>\n> Ah, sure. After skimming through various man pages available online, I\n> was not sure whether all standalone versions of diff which support -I\n> allow that switch to be used multiple times and I thought it would be\n> better to start with the simplest thing possible. If you would rather\n> have the above implemented immediately, I can sure try that in v2.\n>\n>> and until that happens, the explanation needs to say something about\n>> earlier being discarded when this option is given more than\n>> once.\n>\n> Sorry, where do I look for that explanation? \n\nYou wrote \"Ignore changes whose all lines match \"; I was\nsuggesting that we also need \"when given more than once, all but the\nlast are ignored\" after that sentence, because that is not\nwhat people who know how -I works in versions of \"diff\" that support\nit expect.\n\nBut I think it should be trivial to make it a list of patterns and\ntry to match against them in a loop. So let's support multiple\npatterns from the get-go.\n\n> I have not thought about this approach before, but it sounds elegant to\n> me, at least at first glance - option parsing code sounds like the right\n> place for sanitizing user-provided parameters. Doubly so if we take the\n> multiple -I options approach. I will try this in v2.\n\nSounds good.\n\n>> I agree with what you said in the cover letter that it would be a\n>> good idea to name the existing xdl_mark_ignorable() and the new one\n>> in such a way that they look similar and parallel, by renaming the\n>> former to xdl_mark_ignorable_lines or something.\n>\n> Then I will do that in v2. Separate patch?\n\nGiven that the function has only one caller, I think it is better\ndone within the same patch. xdl_mark_ignorable() as the name of the\nfunction is not wrong per-se, so it does not deserve to be renamed\nstandalone in a preliminary clean-up patch---there is nothing to be\ncleaned up. The fact that we introduce a similarly tasked function\nmakes its current name less desirable, so adjusting to the new world\norder is better done as part of the same patch.\n\n> My pleasure ;-) And thanks for taking a look. Sorry about the long\n> turnaround time, but unfortunately this is the best I can do at the\n> moment.\n\nPleasure is mutual ;-)\n\nWe've survived without -I for 15 years. Even a few more months\nwould not hurt anybody. Take time, aim for a quality job, and most\nimportantly, have fun.\n\nThanks.\n"},{"id":"407314","messageId":"20201012091751.19594-2-michal@isc.org","threadId":"54333","inReplyTo":"20201012091751.19594-1-michal@isc.org","subject":"[PATCH v2 1/3] merge-base, xdiff: zero out xpparam_t structures","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-12T09:17:49Z","receivedAt":"2020-10-12T09:18:03Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"xpparam_t structures are usually zero-initialized before their specific\nfields are assigned to, but there are three locations in the tree where\nthat does not happen. Add the missing memset() calls in order to make\ninitialization of xpparam_t structures consistent tree-wide and to\nprevent stack garbage from being used as field values.\n\nSigned-off-by: Michał Kępień \n---\n builtin/merge-tree.c | 1 +\n xdiff/xhistogram.c | 2 ++\n xdiff/xpatience.c | 2 ++\n 3 files changed, 5 insertions(+)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex e72714a5a8..de8520778d 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -109,6 +109,7 @@ static void show_diff(struct merge_list *entry)\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex c7b35a9667..e694bfd9e3 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -235,6 +235,8 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpparam;\n+\n+\tmemset(&xpparam, 0, sizeof(xpparam));\n \txpparam.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(env, &xpparam,\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex 3c5601b602..20699a6f60 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -318,6 +318,8 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpp;\n+\n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = map->xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(map->env, &xpp,\n-- \n2.28.0\n\n"},{"id":"407315","messageId":"20201012091751.19594-3-michal@isc.org","threadId":"54333","inReplyTo":"20201012091751.19594-1-michal@isc.org","subject":"[PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-12T09:17:50Z","receivedAt":"2020-10-12T09:18:05Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Add a new diff option that enables ignoring changes whose all lines\n(changed, removed, and added) match a given regular expression. This is\nsimilar to the -I option in standalone diff utilities and can be used\ne.g. to ignore changes which only affect code comments or to look for\nunrelated changes in commits containing a large number of automatically\napplied modifications (e.g. a tree-wide string replacement). The\ndifference between -G/-S and the new -I option is that the latter\nfilters output on a per-change basis.\n\nUse the 'ignore' field of xdchange_t for marking a change as ignored or\nnot. Since the same field is used by --ignore-blank-lines, identical\nhunk emitting rules apply for --ignore-blank-lines and -I. These two\noptions can also be used together in the same git invocation (they are\ncomplementary to each other).\n\nRename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to indicate\nthat it is logically a \"sibling\" of xdl_mark_ignorable_regex() rather\nthan its \"parent\".\n\nSigned-off-by: Michał Kępień \n---\n Documentation/diff-options.txt | 4 +++\n diff.c | 23 +++++++++++++++++\n diff.h | 4 +++\n xdiff/xdiff.h | 4 +++\n xdiff/xdiffi.c | 47 ++++++++++++++++++++++++++++++++--\n 5 files changed, 80 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 573fb9bb71..f8ccf85173 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -687,6 +687,10 @@ endif::git-format-patch[]\n --ignore-blank-lines::\n \tIgnore changes whose lines are all blank.\n \n+-I::\n+\tIgnore changes whose all lines match . This option may\n+\tbe specified more than once.\n+\n --inter-hunk-context=::\n \tShow the context between diff hunks, up to the specified number\n \tof lines, thereby fusing hunks that are close to each other.\ndiff --git a/diff.c b/diff.c\nindex 2bb2f8f57e..c5d4920fee 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3587,6 +3587,8 @@ static void builtin_diff(const char *name_a,\n \t\tif (header.len && !o->flags.suppress_diff_headers)\n \t\t\tecbdata.header = &header;\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -3716,6 +3718,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_ignore_regex(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct diff_options *options = opt->value;\n+\tregex_t *regex;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tregex = xcalloc(1, sizeof(*regex));\n+\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n+\t\tdie(\"invalid regex: %s\", arg);\n+\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n+\t\t options->ignore_regex_alloc);\n+\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n+\treturn 0;\n+}\n+\n static int diff_opt_pickaxe_regex(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n \t\t\t N_(\"ignore changes whose lines are all blank\"),\n \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F('I', NULL, options, N_(\"\"),\n+\t\t\t N_(\"ignore changes whose all lines match \"),\n+\t\t\t 0, diff_opt_ignore_regex),\n \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n \t\t\tXDF_INDENT_HEURISTIC),\ndiff --git a/diff.h b/diff.h\nindex 11de52e9e9..80dbd3dfdc 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -234,6 +234,10 @@ struct diff_options {\n \t */\n \tconst char *pickaxe;\n \n+\t/* see Documentation/diff-options.txt */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr, ignore_regex_alloc;\n+\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tconst char *line_prefix;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 032e3a9f41..883a0d770e 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n typedef struct s_xpparam {\n \tunsigned long flags;\n \n+\t/* See Documentation/diff-options.txt. */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr;\n+\n \t/* See Documentation/diff-options.txt. */\n \tchar **anchors;\n \tsize_t anchors_nr;\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex bd035139f9..380eb728ed 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -998,7 +998,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \treturn 0;\n }\n \n-static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n+static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n {\n \txdchange_t *xch;\n \n@@ -1019,6 +1019,46 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n \t}\n }\n \n+static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n+\tregmatch_t regmatch;\n+\tint i;\n+\n+\tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n+\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n+\t\t\t\t ®match, 0))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n+\t\t\t\t xpparam_t const *xpp)\n+{\n+\txdchange_t *xch;\n+\n+\tfor (xch = xscr; xch; xch = xch->next) {\n+\t\txrecord_t **rec;\n+\t\tint ignore = 1;\n+\t\tlong i;\n+\n+\t\t/*\n+\t\t * Do not override --ignore-blank-lines.\n+\t\t */\n+\t\tif (xch->ignore)\n+\t\t\tcontinue;\n+\n+\t\trec = &xe->xdf1.recs[xch->i1];\n+\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\trec = &xe->xdf2.recs[xch->i2];\n+\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\txch->ignore = ignore;\n+\t}\n+}\n+\n int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n \txdchange_t *xscr;\n@@ -1038,7 +1078,10 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t}\n \tif (xscr) {\n \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n-\t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n+\t\t\txdl_mark_ignorable_lines(xscr, &xe, xpp->flags);\n+\n+\t\tif (xpp->ignore_regex)\n+\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp);\n \n \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n \n-- \n2.28.0\n\n"},{"id":"407316","messageId":"20201012091751.19594-4-michal@isc.org","threadId":"54333","inReplyTo":"20201012091751.19594-1-michal@isc.org","subject":"[PATCH v2 3/3] t: add -I tests","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-12T09:17:51Z","receivedAt":"2020-10-12T09:18:05Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Exercise the new -I diff option in various scenarios to ensure it\nbehaves as expected.\n\nSigned-off-by: Michał Kępień \n---\n t/t4069-diff-ignore-regex.sh | 426 +++++++++++++++++++++++++++++++++++\n 1 file changed, 426 insertions(+)\n create mode 100755 t/t4069-diff-ignore-regex.sh\n\ndiff --git a/t/t4069-diff-ignore-regex.sh b/t/t4069-diff-ignore-regex.sh\nnew file mode 100755\nindex 0000000000..4ddf5c67ae\n--- /dev/null\n+++ b/t/t4069-diff-ignore-regex.sh\n@@ -0,0 +1,426 @@\n+#!/bin/sh\n+\n+test_description='Test diff -I'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+test_expect_success setup '\n+\ttest_seq 20 >x &&\n+\tgit update-index --add x\n+'\n+\n+test_expect_success 'one line changed' '\n+\ttest_seq 20 | sed \"s/10/100/\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -7,7 +7,7 @@\n+\t 7\n+\t 8\n+\t 9\n+\t-10\n+\t+100\n+\t 11\n+\t 12\n+\t 13\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both old and new line match regex - ignore change\n+\tgit diff -I \"^10\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Both old and new line match some regex - ignore change\n+\tgit diff -I \"^10\\$\" -I \"^100\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only old line matches regex - do not ignore change\n+\tgit diff -I \"^10\\$\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Only new line matches regex - do not ignore change\n+\tgit diff -I \"^100\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Only old line matches some regex - do not ignore change\n+\tgit diff -I \"^10\\$\" -I \"^101\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Only new line matches some regex - do not ignore change\n+\tgit diff -I \"^11\\$\" -I \"^100\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'one line removed' '\n+\ttest_seq 20 | sed \"10d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -7,7 +7,6 @@\n+\t 7\n+\t 8\n+\t 9\n+\t-10\n+\t 11\n+\t 12\n+\t 13\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Removed line matches regex - ignore change\n+\tgit diff -I \"^10\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Removed line matches some regex - ignore change\n+\tgit diff -I \"^10\" -I \"^100\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Removed line does not match regex - do not ignore change\n+\tgit diff -I \"^101\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Removed line does not match any regex - do not ignore change\n+\tgit diff -I \"^100\" -I \"^101\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'one line added' '\n+\ttest_seq 21 >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -18,3 +18,4 @@\n+\t 18\n+\t 19\n+\t 20\n+\t+21\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Added line matches regex - ignore change\n+\tgit diff -I \"^21\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Added line matches some regex - ignore change\n+\tgit diff -I \"^21\" -I \"^22\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Added line does not match regex - do not ignore change\n+\tgit diff -I \"^212\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Added line does not match any regex - do not ignore change\n+\tgit diff -I \"^211\" -I \"^212\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'last two lines changed' '\n+\ttest_seq 20 | sed \"s/19/21/; s/20/22/\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -16,5 +16,5 @@\n+\t 16\n+\t 17\n+\t 18\n+\t-19\n+\t-20\n+\t+21\n+\t+22\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# All changed lines match regex - ignore change\n+\tgit diff -I \"^[12]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# All changed lines match some regex - ignore change\n+\tgit diff -I \"^1\" -I \"^2\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Not all changed lines match regex - do not ignore change\n+\tgit diff -I \"^2\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Not all changed lines match some regex - do not ignore change\n+\tgit diff -I \"^2\" -I \"^3\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'two non-adjacent lines removed in the same hunk' '\n+\ttest_seq 20 | sed \"1d; 3d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,6 +1,4 @@\n+\t-1\n+\t 2\n+\t-3\n+\t 4\n+\t 5\n+\t 6\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both removed lines match regex - ignore hunk\n+\tgit diff -I \"^[1-3]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Both removed lines match some regex - ignore hunk\n+\tgit diff -I \"^1\" -I \"^3\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# First removed line does not match regex - do not ignore hunk\n+\tgit diff -I \"^[2-3]\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# First removed line does not match any regex - do not ignore hunk\n+\tgit diff -I \"^2\" -I \"^3\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Second removed line does not match regex - do not ignore hunk\n+\tgit diff -I \"^[1-2]\" >actual &&\n+\tcompare_diff_patch plain actual &&\n+\n+\t# Second removed line does not match any regex - do not ignore hunk\n+\tgit diff -I \"^1\" -I \"^2\" >actual &&\n+\tcompare_diff_patch plain actual\n+'\n+\n+test_expect_success 'two non-adjacent lines removed in the same hunk, with -U1' '\n+\ttest_seq 20 | sed \"1d; 3d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff -U1 >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,2 @@\n+\t-1\n+\t 2\n+\t-3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Both removed lines match regex - ignore hunk\n+\tgit diff -U1 -I \"^[1-3]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Both removed lines match some regex - ignore hunk\n+\tgit diff -U1 -I \"^1\" -I \"^3\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# First removed line does not match regex, but is out of context - ignore second change\n+\tgit diff -U1 -I \"^[2-3]\" >actual &&\n+\tcat >second-change-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,2 +1 @@\n+\t-1\n+\t 2\n+\tEOF\n+\tcompare_diff_patch second-change-ignored actual &&\n+\n+\t# First removed line does not match any regex, but is out of context - ignore second change\n+\tgit diff -U1 -I \"^2\" -I \"^3\" >actual &&\n+\tcompare_diff_patch second-change-ignored actual &&\n+\n+\t# Second removed line does not match regex, but is out of context - ignore first change\n+\tgit diff -U1 -I \"^[1-2]\" >actual &&\n+\tcat >first-change-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -2,3 +1,2 @@\n+\t 2\n+\t-3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch first-change-ignored actual &&\n+\n+\t# Second removed line does not match any regex, but is out of context - ignore first change\n+\tgit diff -U1 -I \"^1\" -I \"^2\" >actual &&\n+\tcompare_diff_patch first-change-ignored actual\n+'\n+\n+test_expect_success 'multiple hunks' '\n+\ttest_seq 20 | sed \"1d; 20d\" >x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,3 @@\n+\t-1\n+\t 2\n+\t 3\n+\t 4\n+\t@@ -17,4 +16,3 @@\n+\t 17\n+\t 18\n+\t 19\n+\t-20\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# Ignore both hunks (single regex)\n+\tgit diff -I \"^[12]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Ignore both hunks (multiple regexes)\n+\tgit diff -I \"^1\" -I \"^2\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only ignore first hunk (single regex)\n+\tgit diff -I \"^1\" >actual &&\n+\tcat >first-hunk-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -17,4 +16,3 @@\n+\t 17\n+\t 18\n+\t 19\n+\t-20\n+\tEOF\n+\tcompare_diff_patch first-hunk-ignored actual &&\n+\n+\t# Only ignore first hunk (multiple regexes)\n+\tgit diff -I \"^0\" -I \"^1\" >actual &&\n+\tcompare_diff_patch first-hunk-ignored actual &&\n+\n+\t# Only ignore second hunk (single regex)\n+\tgit diff -I \"^2\" >actual &&\n+\tcat >second-hunk-ignored <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,4 +1,3 @@\n+\t-1\n+\t 2\n+\t 3\n+\t 4\n+\tEOF\n+\tcompare_diff_patch second-hunk-ignored actual &&\n+\n+\t# Only ignore second hunk (multiple regexes)\n+\tgit diff -I \"^2\" -I \"^3\" >actual &&\n+\tcompare_diff_patch second-hunk-ignored actual\n+'\n+\n+test_expect_success 'multiple hunks, with --ignore-blank-lines' '\n+\techo >x &&\n+\ttest_seq 21 >>x &&\n+\n+\t# Get plain diff\n+\tgit diff >plain &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/x b/x\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1,3 +1,4 @@\n+\t+\n+\t 1\n+\t 2\n+\t 3\n+\t@@ -18,3 +19,4 @@\n+\t 18\n+\t 19\n+\t 20\n+\t+21\n+\tEOF\n+\tcompare_diff_patch expected plain &&\n+\n+\t# -I does not override --ignore-blank-lines - ignore both hunks (single regex)\n+\tgit diff --ignore-blank-lines -I \"^21\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# -I does not override --ignore-blank-lines - ignore both hunks (multiple regexes)\n+\tgit diff --ignore-blank-lines -I \"^21\" -I \"^12\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'diffstat' '\n+\ttest_seq 20 | sed \"s/^5/0/p; s/^15/10/; 16d\" >x &&\n+\n+\t# Get plain diffstat\n+\tgit diff --stat >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 6 +++---\n+\t 1 file changed, 3 insertions(+), 3 deletions(-)\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# Ignore both hunks (single regex)\n+\tgit diff --stat -I \"^[0-5]\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Ignore both hunks (multiple regexes)\n+\tgit diff --stat -I \"^0\" -I \"^1\" -I \"^5\" >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# Only ignore first hunk (single regex)\n+\tgit diff --stat -I \"^[05]\" >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 3 +--\n+\t 1 file changed, 1 insertion(+), 2 deletions(-)\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# Only ignore first hunk (multiple regexes)\n+\tgit diff --stat -I \"^0\" -I \"^5\" >actual &&\n+\ttest_cmp expected actual &&\n+\n+\t# Only ignore second hunk (single regex)\n+\tgit diff --stat -I \"^1\" >actual &&\n+\tcat >expected <<-EOF &&\n+\t x | 3 ++-\n+\t 1 file changed, 2 insertions(+), 1 deletion(-)\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# Only ignore second hunk (multiple regexes)\n+\tgit diff --stat -I \"^1\" -I \"^2\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'invalid regexes' '\n+\t>x &&\n+\n+\t# Single invalid regex\n+\tgit diff -I \"^[1\" 2>&1 | grep \"invalid regex: \" &&\n+\n+\t# Two regexes: first invalid, second valid\n+\tgit diff -I \"^[1\" -I \"^1\" 2>&1 | grep \"invalid regex: \" &&\n+\n+\t# Two invalid regexes\n+\tgit diff -I \"^[1\" -I \"^[2\" 2>&1 | grep \"invalid regex: \"\n+'\n+\n+test_done\n-- \n2.28.0\n\n"},{"id":"407317","messageId":"20201012091751.19594-1-michal@isc.org","threadId":"54333","inReplyTo":"20201001120606.25773-1-michal@isc.org","subject":"[PATCH v2 0/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-12T09:17:48Z","receivedAt":"2020-10-12T09:18:06Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"This patch series adds a new diff option that enables ignoring changes\nwhose all lines (changed, removed, and added) match a given regular\nexpression. This is similar to the -I option in standalone diff\nutilities and can be used e.g. to ignore changes which only affect code\ncomments or to look for unrelated changes in commits containing a large\nnumber of automatically applied modifications (e.g. a tree-wide string\nreplacement). The difference between -G/-S and the new -I option is\nthat the latter filters output on a per-change basis.\n\nChanges from v1:\n\n - Add a new preliminary cleanup patch which ensures xpparam_t\n structures are always zero-initialized. (This was a prerequisite\n for the next change below.)\n\n - Do not add a new 'xdl_opts' flag to check whether -I was used;\n instead, just check whether the array of regular expressions to\n match against is non-NULL.\n\n - Enable the -I option to be used multiple times. As a consequence of\n this, regular expressions are now \"pre-compiled\" in the option's\n callback (and passed around as an array of regex_t structures)\n rather than deep down in xdiff code. Add test cases exercising use\n of multiple -I options in the same git invocation. Update\n documentation accordingly.\n\n - Rename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to\n indicate that it is logically a \"sibling\" of\n xdl_mark_ignorable_regex() rather than its \"parent\".\n\n - Optimize xdl_mark_ignorable_regex() by making it immediately skip\n changes already marked as ignored by xdl_mark_ignorable_lines().\n\n - Fix coding style issue in the prototype part of the definition of\n xdl_mark_ignorable_regex().\n\n - Add \"/* see Documentation/diff-options.txt */\" comments for the\n fields added to struct diff_options and xpparam_t, mimicking the\n comments used for 'anchors', 'anchors_nr', and 'anchors_alloc'.\n\n - Revise commit log messages to reflect all of the above.\n\nMichał Kępień (3):\n merge-base, xdiff: zero out xpparam_t structures\n diff: add -I that ignores matching changes\n t: add -I tests\n\n Documentation/diff-options.txt | 4 +\n builtin/merge-tree.c | 1 +\n diff.c | 23 ++\n diff.h | 4 +\n t/t4069-diff-ignore-regex.sh | 426 +++++++++++++++++++++++++++++++++\n xdiff/xdiff.h | 4 +\n xdiff/xdiffi.c | 47 +++-\n xdiff/xhistogram.c | 2 +\n xdiff/xpatience.c | 2 +\n 9 files changed, 511 insertions(+), 2 deletions(-)\n create mode 100755 t/t4069-diff-ignore-regex.sh\n\n-- \n2.28.0\n\n"},{"id":"407327","messageId":"nycvar.QRO.7.76.6.2010121313020.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201012091751.19594-2-michal@isc.org","subject":"Re: [PATCH v2 1/3] merge-base, xdiff: zero out xpparam_t structures","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T11:14:50Z","receivedAt":"2020-10-12T11:17:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Mon, 12 Oct 2020, Michał Kępień wrote:\n\n\n> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\n> index c7b35a9667..e694bfd9e3 100644\n> --- a/xdiff/xhistogram.c\n> +++ b/xdiff/xhistogram.c\n> @@ -235,6 +235,8 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,\n> \t\tint line1, int count1, int line2, int count2)\n> {\n> \txpparam_t xpparam;\n> +\n> +\tmemset(&xpparam, 0, sizeof(xpparam));\n\nA slightly more elegant way to do this would be\n\n\txpparam_t xpparam = { 0l };\n\nOr even\n\n\txpparam_t xpparam = XPPARAM_T_INIT;\n\nwith\n\n\t#define XPPARAM_T_INIT { 0l, NULL, 0 }\n\nin `xdiff/xdiff.h`.\n\nThanks,\nDscho\n\n> \txpparam.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n>\n> \treturn xdl_fall_back_diff(env, &xpparam,\n> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\n> index 3c5601b602..20699a6f60 100644\n> --- a/xdiff/xpatience.c\n> +++ b/xdiff/xpatience.c\n> @@ -318,6 +318,8 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n> \t\tint line1, int count1, int line2, int count2)\n> {\n> \txpparam_t xpp;\n> +\n> +\tmemset(&xpp, 0, sizeof(xpp));\n> \txpp.flags = map->xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n>\n> \treturn xdl_fall_back_diff(map->env, &xpp,\n> --\n> 2.28.0\n>\n>\n>\n"},{"id":"407329","messageId":"nycvar.QRO.7.76.6.2010121315170.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201012091751.19594-3-michal@isc.org","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T11:20:13Z","receivedAt":"2020-10-12T11:25:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Mon, 12 Oct 2020, Michał Kępień wrote:\n\n> @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n> \treturn 0;\n> }\n>\n> +static int diff_opt_ignore_regex(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct diff_options *options = opt->value;\n> +\tregex_t *regex;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\tregex = xcalloc(1, sizeof(*regex));\n> +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n> +\t\tdie(\"invalid regex: %s\", arg);\n> +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n> +\t\t options->ignore_regex_alloc);\n> +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n\nA slightly more elegant way would be to have `ignore_regex` have the type\n`regex_t *` and use `ALLOC_GROW_BY()` (which zeroes the newly-added\nelements automagically).\n\n> +\treturn 0;\n> +}\n> +\n> static int diff_opt_pickaxe_regex(const struct option *opt,\n> \t\t\t\t const char *arg, int unset)\n> {\n> [...]\n> @@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n> \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n> \t\t\t N_(\"ignore changes whose lines are all blank\"),\n> \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n> +\t\tOPT_CALLBACK_F('I', NULL, options, N_(\"\"),\n> +\t\t\t N_(\"ignore changes whose all lines match \"),\n> +\t\t\t 0, diff_opt_ignore_regex),\n> \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n> \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n> \t\t\tXDF_INDENT_HEURISTIC),\n\nAre we releasing the `ignore_regex` anywhere?\n\nThanks,\nDscho\n\n> diff --git a/diff.h b/diff.h\n> index 11de52e9e9..80dbd3dfdc 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -234,6 +234,10 @@ struct diff_options {\n> \t */\n> \tconst char *pickaxe;\n>\n> +\t/* see Documentation/diff-options.txt */\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr, ignore_regex_alloc;\n> +\n> \tconst char *single_follow;\n> \tconst char *a_prefix, *b_prefix;\n> \tconst char *line_prefix;\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 032e3a9f41..883a0d770e 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n> typedef struct s_xpparam {\n> \tunsigned long flags;\n>\n> +\t/* See Documentation/diff-options.txt. */\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr;\n> +\n> \t/* See Documentation/diff-options.txt. */\n> \tchar **anchors;\n> \tsize_t anchors_nr;\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index bd035139f9..380eb728ed 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -998,7 +998,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n> \treturn 0;\n> }\n>\n> -static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> +static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> {\n> \txdchange_t *xch;\n>\n> @@ -1019,6 +1019,46 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> \t}\n> }\n>\n> +static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n> +\tregmatch_t regmatch;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n> +\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n> +\t\t\t\t ®match, 0))\n> +\t\t\treturn 1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n> +\t\t\t\t xpparam_t const *xpp)\n> +{\n> +\txdchange_t *xch;\n> +\n> +\tfor (xch = xscr; xch; xch = xch->next) {\n> +\t\txrecord_t **rec;\n> +\t\tint ignore = 1;\n> +\t\tlong i;\n> +\n> +\t\t/*\n> +\t\t * Do not override --ignore-blank-lines.\n> +\t\t */\n> +\t\tif (xch->ignore)\n> +\t\t\tcontinue;\n> +\n> +\t\trec = &xe->xdf1.recs[xch->i1];\n> +\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n> +\t\t\tignore = record_matches_regex(rec[i], xpp);\n> +\n> +\t\trec = &xe->xdf2.recs[xch->i2];\n> +\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n> +\t\t\tignore = record_matches_regex(rec[i], xpp);\n> +\n> +\t\txch->ignore = ignore;\n> +\t}\n> +}\n> +\n> int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n> \txdchange_t *xscr;\n> @@ -1038,7 +1078,10 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t}\n> \tif (xscr) {\n> \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n> -\t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n> +\t\t\txdl_mark_ignorable_lines(xscr, &xe, xpp->flags);\n> +\n> +\t\tif (xpp->ignore_regex)\n> +\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp);\n>\n> \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n>\n> --\n> 2.28.0\n>\n>\n"},{"id":"407330","messageId":"nycvar.QRO.7.76.6.2010121320190.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201012091751.19594-4-michal@isc.org","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T11:49:32Z","receivedAt":"2020-10-12T11:54:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Mon, 12 Oct 2020, Michał Kępień wrote:\n\n> Exercise the new -I diff option in various scenarios to ensure it\n> behaves as expected.\n\nExcellent. I was actually looking for a test in patch 2/3 and almost\ncommented about that.\n\n>\n> Signed-off-by: Michał Kępień \n> ---\n> t/t4069-diff-ignore-regex.sh | 426 +++++++++++++++++++++++++++++++++++\n\nHmm. I wonder whether we could do with a much more concise test script.\nThe test suite already takes a quite long time to run, which is not a\nlaughing matter: we had issues in the past where contributors would skip\nrunning it because it took too long, and this test is sure to exacerbate\nthat problem.\n\nI could imagine, for example, that it would be plenty enough to do\nsomething like this instead:\n\n-- snip --\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5c7b0122b4f..bf158be137f 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -6,6 +6,7 @@\n test_description='Various diff formatting options'\n\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n\n test_expect_success setup '\n\n@@ -473,4 +474,24 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success '-I' '\n+\tseq 50 >I.txt &&\n+\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n+\ttest_must_fail git diff --no-index -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/I.txt b/J.txt\n+\t--- a/I.txt\n+\t+++ b/J.txt\n+\t@@ -34,7 +31,6 @@\n+\t 34\n+\t 35\n+\t 36\n+\t-37\n+\t 38\n+\t 39\n+\t 40\n+\tEOF\n+\tcompare_diff_patch expect actual\n+'\n+\n test_done\n-- snap --\n\nNote how it tests various things in one go?\n\nThanks,\nDscho\n\nP.S.: My main interest in the `-I` option is its use case for `git\nrange-diff` in Git's own context: if you want to compare your patches to\nwhat entered the `seen` branch, there will _always_ be a difference\nbecause Junio adds their DCO. Something like this can help that:\n\ngit range-diff \\\n\t-I'^ Signed-off-by: Junio C Hamano $' \\\n\t \n\n> 1 file changed, 426 insertions(+)\n> create mode 100755 t/t4069-diff-ignore-regex.sh\n>\n> diff --git a/t/t4069-diff-ignore-regex.sh b/t/t4069-diff-ignore-regex.sh\n> new file mode 100755\n> index 0000000000..4ddf5c67ae\n> --- /dev/null\n> +++ b/t/t4069-diff-ignore-regex.sh\n> @@ -0,0 +1,426 @@\n> +#!/bin/sh\n> +\n> +test_description='Test diff -I'\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/diff-lib.sh\n> +\n> +test_expect_success setup '\n> +\ttest_seq 20 >x &&\n> +\tgit update-index --add x\n> +'\n> +\n> +test_expect_success 'one line changed' '\n> +\ttest_seq 20 | sed \"s/10/100/\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -7,7 +7,7 @@\n> +\t 7\n> +\t 8\n> +\t 9\n> +\t-10\n> +\t+100\n> +\t 11\n> +\t 12\n> +\t 13\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Both old and new line match regex - ignore change\n> +\tgit diff -I \"^10\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Both old and new line match some regex - ignore change\n> +\tgit diff -I \"^10\\$\" -I \"^100\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Only old line matches regex - do not ignore change\n> +\tgit diff -I \"^10\\$\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Only new line matches regex - do not ignore change\n> +\tgit diff -I \"^100\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Only old line matches some regex - do not ignore change\n> +\tgit diff -I \"^10\\$\" -I \"^101\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Only new line matches some regex - do not ignore change\n> +\tgit diff -I \"^11\\$\" -I \"^100\" >actual &&\n> +\tcompare_diff_patch plain actual\n> +'\n> +\n> +test_expect_success 'one line removed' '\n> +\ttest_seq 20 | sed \"10d\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -7,7 +7,6 @@\n> +\t 7\n> +\t 8\n> +\t 9\n> +\t-10\n> +\t 11\n> +\t 12\n> +\t 13\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Removed line matches regex - ignore change\n> +\tgit diff -I \"^10\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Removed line matches some regex - ignore change\n> +\tgit diff -I \"^10\" -I \"^100\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Removed line does not match regex - do not ignore change\n> +\tgit diff -I \"^101\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Removed line does not match any regex - do not ignore change\n> +\tgit diff -I \"^100\" -I \"^101\" >actual &&\n> +\tcompare_diff_patch plain actual\n> +'\n> +\n> +test_expect_success 'one line added' '\n> +\ttest_seq 21 >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -18,3 +18,4 @@\n> +\t 18\n> +\t 19\n> +\t 20\n> +\t+21\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Added line matches regex - ignore change\n> +\tgit diff -I \"^21\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Added line matches some regex - ignore change\n> +\tgit diff -I \"^21\" -I \"^22\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Added line does not match regex - do not ignore change\n> +\tgit diff -I \"^212\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Added line does not match any regex - do not ignore change\n> +\tgit diff -I \"^211\" -I \"^212\" >actual &&\n> +\tcompare_diff_patch plain actual\n> +'\n> +\n> +test_expect_success 'last two lines changed' '\n> +\ttest_seq 20 | sed \"s/19/21/; s/20/22/\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -16,5 +16,5 @@\n> +\t 16\n> +\t 17\n> +\t 18\n> +\t-19\n> +\t-20\n> +\t+21\n> +\t+22\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# All changed lines match regex - ignore change\n> +\tgit diff -I \"^[12]\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# All changed lines match some regex - ignore change\n> +\tgit diff -I \"^1\" -I \"^2\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Not all changed lines match regex - do not ignore change\n> +\tgit diff -I \"^2\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Not all changed lines match some regex - do not ignore change\n> +\tgit diff -I \"^2\" -I \"^3\" >actual &&\n> +\tcompare_diff_patch plain actual\n> +'\n> +\n> +test_expect_success 'two non-adjacent lines removed in the same hunk' '\n> +\ttest_seq 20 | sed \"1d; 3d\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,6 +1,4 @@\n> +\t-1\n> +\t 2\n> +\t-3\n> +\t 4\n> +\t 5\n> +\t 6\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Both removed lines match regex - ignore hunk\n> +\tgit diff -I \"^[1-3]\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Both removed lines match some regex - ignore hunk\n> +\tgit diff -I \"^1\" -I \"^3\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# First removed line does not match regex - do not ignore hunk\n> +\tgit diff -I \"^[2-3]\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# First removed line does not match any regex - do not ignore hunk\n> +\tgit diff -I \"^2\" -I \"^3\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Second removed line does not match regex - do not ignore hunk\n> +\tgit diff -I \"^[1-2]\" >actual &&\n> +\tcompare_diff_patch plain actual &&\n> +\n> +\t# Second removed line does not match any regex - do not ignore hunk\n> +\tgit diff -I \"^1\" -I \"^2\" >actual &&\n> +\tcompare_diff_patch plain actual\n> +'\n> +\n> +test_expect_success 'two non-adjacent lines removed in the same hunk, with -U1' '\n> +\ttest_seq 20 | sed \"1d; 3d\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff -U1 >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,4 +1,2 @@\n> +\t-1\n> +\t 2\n> +\t-3\n> +\t 4\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Both removed lines match regex - ignore hunk\n> +\tgit diff -U1 -I \"^[1-3]\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Both removed lines match some regex - ignore hunk\n> +\tgit diff -U1 -I \"^1\" -I \"^3\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# First removed line does not match regex, but is out of context - ignore second change\n> +\tgit diff -U1 -I \"^[2-3]\" >actual &&\n> +\tcat >second-change-ignored <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,2 +1 @@\n> +\t-1\n> +\t 2\n> +\tEOF\n> +\tcompare_diff_patch second-change-ignored actual &&\n> +\n> +\t# First removed line does not match any regex, but is out of context - ignore second change\n> +\tgit diff -U1 -I \"^2\" -I \"^3\" >actual &&\n> +\tcompare_diff_patch second-change-ignored actual &&\n> +\n> +\t# Second removed line does not match regex, but is out of context - ignore first change\n> +\tgit diff -U1 -I \"^[1-2]\" >actual &&\n> +\tcat >first-change-ignored <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -2,3 +1,2 @@\n> +\t 2\n> +\t-3\n> +\t 4\n> +\tEOF\n> +\tcompare_diff_patch first-change-ignored actual &&\n> +\n> +\t# Second removed line does not match any regex, but is out of context - ignore first change\n> +\tgit diff -U1 -I \"^1\" -I \"^2\" >actual &&\n> +\tcompare_diff_patch first-change-ignored actual\n> +'\n> +\n> +test_expect_success 'multiple hunks' '\n> +\ttest_seq 20 | sed \"1d; 20d\" >x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,4 +1,3 @@\n> +\t-1\n> +\t 2\n> +\t 3\n> +\t 4\n> +\t@@ -17,4 +16,3 @@\n> +\t 17\n> +\t 18\n> +\t 19\n> +\t-20\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# Ignore both hunks (single regex)\n> +\tgit diff -I \"^[12]\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Ignore both hunks (multiple regexes)\n> +\tgit diff -I \"^1\" -I \"^2\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Only ignore first hunk (single regex)\n> +\tgit diff -I \"^1\" >actual &&\n> +\tcat >first-hunk-ignored <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -17,4 +16,3 @@\n> +\t 17\n> +\t 18\n> +\t 19\n> +\t-20\n> +\tEOF\n> +\tcompare_diff_patch first-hunk-ignored actual &&\n> +\n> +\t# Only ignore first hunk (multiple regexes)\n> +\tgit diff -I \"^0\" -I \"^1\" >actual &&\n> +\tcompare_diff_patch first-hunk-ignored actual &&\n> +\n> +\t# Only ignore second hunk (single regex)\n> +\tgit diff -I \"^2\" >actual &&\n> +\tcat >second-hunk-ignored <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,4 +1,3 @@\n> +\t-1\n> +\t 2\n> +\t 3\n> +\t 4\n> +\tEOF\n> +\tcompare_diff_patch second-hunk-ignored actual &&\n> +\n> +\t# Only ignore second hunk (multiple regexes)\n> +\tgit diff -I \"^2\" -I \"^3\" >actual &&\n> +\tcompare_diff_patch second-hunk-ignored actual\n> +'\n> +\n> +test_expect_success 'multiple hunks, with --ignore-blank-lines' '\n> +\techo >x &&\n> +\ttest_seq 21 >>x &&\n> +\n> +\t# Get plain diff\n> +\tgit diff >plain &&\n> +\tcat >expected <<-EOF &&\n> +\tdiff --git a/x b/x\n> +\t--- a/x\n> +\t+++ b/x\n> +\t@@ -1,3 +1,4 @@\n> +\t+\n> +\t 1\n> +\t 2\n> +\t 3\n> +\t@@ -18,3 +19,4 @@\n> +\t 18\n> +\t 19\n> +\t 20\n> +\t+21\n> +\tEOF\n> +\tcompare_diff_patch expected plain &&\n> +\n> +\t# -I does not override --ignore-blank-lines - ignore both hunks (single regex)\n> +\tgit diff --ignore-blank-lines -I \"^21\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# -I does not override --ignore-blank-lines - ignore both hunks (multiple regexes)\n> +\tgit diff --ignore-blank-lines -I \"^21\" -I \"^12\" >actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> +test_expect_success 'diffstat' '\n> +\ttest_seq 20 | sed \"s/^5/0/p; s/^15/10/; 16d\" >x &&\n> +\n> +\t# Get plain diffstat\n> +\tgit diff --stat >actual &&\n> +\tcat >expected <<-EOF &&\n> +\t x | 6 +++---\n> +\t 1 file changed, 3 insertions(+), 3 deletions(-)\n> +\tEOF\n> +\ttest_cmp expected actual &&\n> +\n> +\t# Ignore both hunks (single regex)\n> +\tgit diff --stat -I \"^[0-5]\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Ignore both hunks (multiple regexes)\n> +\tgit diff --stat -I \"^0\" -I \"^1\" -I \"^5\" >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\t# Only ignore first hunk (single regex)\n> +\tgit diff --stat -I \"^[05]\" >actual &&\n> +\tcat >expected <<-EOF &&\n> +\t x | 3 +--\n> +\t 1 file changed, 1 insertion(+), 2 deletions(-)\n> +\tEOF\n> +\ttest_cmp expected actual &&\n> +\n> +\t# Only ignore first hunk (multiple regexes)\n> +\tgit diff --stat -I \"^0\" -I \"^5\" >actual &&\n> +\ttest_cmp expected actual &&\n> +\n> +\t# Only ignore second hunk (single regex)\n> +\tgit diff --stat -I \"^1\" >actual &&\n> +\tcat >expected <<-EOF &&\n> +\t x | 3 ++-\n> +\t 1 file changed, 2 insertions(+), 1 deletion(-)\n> +\tEOF\n> +\ttest_cmp expected actual &&\n> +\n> +\t# Only ignore second hunk (multiple regexes)\n> +\tgit diff --stat -I \"^1\" -I \"^2\" >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'invalid regexes' '\n> +\t>x &&\n> +\n> +\t# Single invalid regex\n> +\tgit diff -I \"^[1\" 2>&1 | grep \"invalid regex: \" &&\n> +\n> +\t# Two regexes: first invalid, second valid\n> +\tgit diff -I \"^[1\" -I \"^1\" 2>&1 | grep \"invalid regex: \" &&\n> +\n> +\t# Two invalid regexes\n> +\tgit diff -I \"^[1\" -I \"^[2\" 2>&1 | grep \"invalid regex: \"\n> +'\n> +\n> +test_done\n> --\n> 2.28.0\n>\n>\n"},{"id":"407351","messageId":"xmqq1ri3qsll.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010121313020.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] merge-base, xdiff: zero out xpparam_t structures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T17:09:10Z","receivedAt":"2020-10-12T17:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> \txpparam_t xpparam;\n>> +\n>> +\tmemset(&xpparam, 0, sizeof(xpparam));\n>\n> A slightly more elegant way to do this would be\n>\n> \txpparam_t xpparam = { 0l };\n>\n> Or even\n>\n> \txpparam_t xpparam = XPPARAM_T_INIT;\n>\n> with\n>\n> \t#define XPPARAM_T_INIT { 0l, NULL, 0 }\n>\n> in `xdiff/xdiff.h`.\n\nLet's not suggest any of the above.\n\nI think we had a recent thread [*1*] on which we agreed that \"{ 0 }\"\nis the way to spell \"a naturally zero\" initialization like this one,\nif we were to spell it out.\n\nTBH, I'd say that memset() is also OK as-is in this codebase as an\nestablished way (git grep 'memset([^,]*, 0, sizeof' gives too many\nhits).\n\nThanks.\n\n\n[Reference]\n\n*1* https://lore.kernel.org/git/xmqq4knca7c6.fsf@gitster.c.googlers.com/\n"},{"id":"407356","messageId":"xmqqk0vvpbmp.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201012091751.19594-3-michal@isc.org","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T18:01:02Z","receivedAt":"2020-10-12T18:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> Add a new diff option that enables ignoring changes whose all lines\n> (changed, removed, and added) match a given regular expression. This is\n> similar to the -I option in standalone diff utilities and can be used\n> e.g. to ignore changes which only affect code comments or to look for\n> unrelated changes in commits containing a large number of automatically\n> applied modifications (e.g. a tree-wide string replacement). The\n> difference between -G/-S and the new -I option is that the latter\n> filters output on a per-change basis.\n>\n> Use the 'ignore' field of xdchange_t for marking a change as ignored or\n> not. Since the same field is used by --ignore-blank-lines, identical\n> hunk emitting rules apply for --ignore-blank-lines and -I. These two\n> options can also be used together in the same git invocation (they are\n> complementary to each other).\n>\n> Rename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to indicate\n> that it is logically a \"sibling\" of xdl_mark_ignorable_regex() rather\n> than its \"parent\".\n>\n> Signed-off-by: Michał Kępień \n\nWell explained.\n\n> +-I::\n\nSince we are mimicking other folks' feature, giving also the\n\n--ignore-matching-lines=\n\nsynonym to that their users are familiar with would be a good idea,\nno?\n\n> +\tIgnore changes whose all lines match . This option may\n> +\tbe specified more than once.\n> +\n> --inter-hunk-context=::\n> \tShow the context between diff hunks, up to the specified number\n> \tof lines, thereby fusing hunks that are close to each other.\n> diff --git a/diff.c b/diff.c\n> index 2bb2f8f57e..c5d4920fee 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3587,6 +3587,8 @@ static void builtin_diff(const char *name_a,\n> \t\tif (header.len && !o->flags.suppress_diff_headers)\n> \t\t\tecbdata.header = &header;\n> \t\txpp.flags = o->xdl_opts;\n> +\t\txpp.ignore_regex = o->ignore_regex;\n> +\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n> \t\txpp.anchors = o->anchors;\n> \t\txpp.anchors_nr = o->anchors_nr;\n> \t\txecfg.ctxlen = o->context;\n> @@ -3716,6 +3718,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\tmemset(&xpp, 0, sizeof(xpp));\n> \t\tmemset(&xecfg, 0, sizeof(xecfg));\n> \t\txpp.flags = o->xdl_opts;\n> +\t\txpp.ignore_regex = o->ignore_regex;\n> +\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n> \t\txpp.anchors = o->anchors;\n> \t\txpp.anchors_nr = o->anchors_nr;\n> \t\txecfg.ctxlen = o->context;\n> @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static int diff_opt_ignore_regex(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct diff_options *options = opt->value;\n> +\tregex_t *regex;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\tregex = xcalloc(1, sizeof(*regex));\n> +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n> +\t\tdie(\"invalid regex: %s\", arg);\n> +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n> +\t\t options->ignore_regex_alloc);\n> +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n> +\treturn 0;\n> +}\n\nDon't these parse-options callback functions have a way to tell the\ncaller die instead of them themselves dying like this? Would it\nwork better to \"return error(... message ...)\", or would that give\ntwo error messages? In anycase, this is end-user facing, so we'd\nwant it to be localizable, e.g.\n\n\tdie(_(\"invalid regexp given to -I: '%s'\", arg));\n\nprobably.\n\n> static int diff_opt_pickaxe_regex(const struct option *opt,\n> \t\t\t\t const char *arg, int unset)\n> {\n\nOther than that, nicely done.\n\nThanks.\n"},{"id":"407377","messageId":"xmqqsgajnrvo.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201012091751.19594-2-michal@isc.org","subject":"Re: [PATCH v2 1/3] merge-base, xdiff: zero out xpparam_t structures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T19:52:59Z","receivedAt":"2020-10-12T19:53:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index e72714a5a8..de8520778d 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -109,6 +109,7 @@ static void show_diff(struct merge_list *entry)\n> \txdemitconf_t xecfg;\n> \txdemitcb_t ecb;\n> \n> +\tmemset(&xpp, 0, sizeof(xpp));\n> \txpp.flags = 0;\n> \tmemset(&xecfg, 0, sizeof(xecfg));\n> \txecfg.ctxlen = 3;\n\nThe existing \"xpp.flags=0\" can then go away, and as Dscho hinted,\n\n\txpparam_t xpp = {\n\t\t.flags = 0,\n\t};\n\nmay also be a valid way to write this. What it says is that we want\nthe entirety of xpp to be reasonably initialized, but we do care\nthat its .flags field to be exactly zero.\n\n> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\n> index c7b35a9667..e694bfd9e3 100644\n> --- a/xdiff/xhistogram.c\n> +++ b/xdiff/xhistogram.c\n> @@ -235,6 +235,8 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,\n> \t\tint line1, int count1, int line2, int count2)\n> {\n> \txpparam_t xpparam;\n> +\n> +\tmemset(&xpparam, 0, sizeof(xpparam));\n> \txpparam.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n\nLikewise. Giving an initializer to the local variable def, e.g.\n\n\txpparam_t xpparam = {\n\t\t.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK,\n\t};\n\nwould allow us to lose one line.\n\n> \treturn xdl_fall_back_diff(env, &xpparam,\n> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\n> index 3c5601b602..20699a6f60 100644\n> --- a/xdiff/xpatience.c\n> +++ b/xdiff/xpatience.c\n> @@ -318,6 +318,8 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n> \t\tint line1, int count1, int line2, int count2)\n> {\n> \txpparam_t xpp;\n> +\n> +\tmemset(&xpp, 0, sizeof(xpp));\n> \txpp.flags = map->xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n\nLikewise.\n\n> \treturn xdl_fall_back_diff(map->env, &xpp,\n\nBut the patch is good as-is, given especially the way how xecfg is\ncleared the same way in builtin/merge-tree.c::show_diff().\n\nWill queue. Thanks.\n"},{"id":"407380","messageId":"xmqqo8l7nrjc.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010121315170.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T20:00:23Z","receivedAt":"2020-10-12T20:01:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Michał,\n>\n> On Mon, 12 Oct 2020, Michał Kępień wrote:\n>\n>> @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n>> \treturn 0;\n>> }\n>>\n>> +static int diff_opt_ignore_regex(const struct option *opt,\n>> +\t\t\t\t const char *arg, int unset)\n>> +{\n>> +\tstruct diff_options *options = opt->value;\n>> +\tregex_t *regex;\n>> +\n>> +\tBUG_ON_OPT_NEG(unset);\n>> +\tregex = xcalloc(1, sizeof(*regex));\n>> +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n>> +\t\tdie(\"invalid regex: %s\", arg);\n>> +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n>> +\t\t options->ignore_regex_alloc);\n>> +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n>\n> A slightly more elegant way would be to have `ignore_regex` have the type\n> `regex_t *` and use `ALLOC_GROW_BY()` (which zeroes the newly-added\n> elements automagically).\n\nIt may be \"elegant\", but we we know if it is \"correct\" on\neverybody's implementation of regex_t?\n\nA struct like\n\n\tstruct foo {\n\t\tchar *str;\n\t\tchar in_place_buffer[10];\n\t};\n\nwhere str points at in_place_buffer[] only when it needs to point at\na very short string, and points at an allocated memory on heap, can\nnot be safely copied and used, and an array of such a struct breaks\nwhen ALLOC_GROW_BY() needs to call realloc(). Keeping an array of\npointers to regex_t and growing it would not break even for such a\nstructure type.\n\nSince we cannot rely on the implementation detail of regex_t on a\nsingle platform, I'd rather not to see anybody suggest such an\n\"elegant\" approach.\n\nThanks.\n"},{"id":"407381","messageId":"xmqqk0vvnrcb.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201012091751.19594-3-michal@isc.org","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T20:04:36Z","receivedAt":"2020-10-12T20:04:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> +\t/* see Documentation/diff-options.txt */\n\nThis comment adds negative value.\n\nIf it were\n\n\t/* \"-I\" */\n\nthe readers won't have to switch to the file only to find out that\nthe comment didn't tell them where in the file to look at. \n\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr, ignore_regex_alloc;\n> +\n> \tconst char *single_follow;\n> \tconst char *a_prefix, *b_prefix;\n> \tconst char *line_prefix;\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 032e3a9f41..883a0d770e 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n> typedef struct s_xpparam {\n> \tunsigned long flags;\n> \n> +\t/* See Documentation/diff-options.txt. */\n\nLikewise.\n\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr;\n> +\n> \t/* See Documentation/diff-options.txt. */\n> \tchar **anchors;\n> \tsize_t anchors_nr;\n"},{"id":"407389","messageId":"nycvar.QRO.7.76.6.2010122236130.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"xmqqo8l7nrjc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-12T20:39:20Z","receivedAt":"2020-10-12T20:39:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 12 Oct 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Hi Michał,\n> >\n> > On Mon, 12 Oct 2020, Michał Kępień wrote:\n> >\n> >> @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n> >> \treturn 0;\n> >> }\n> >>\n> >> +static int diff_opt_ignore_regex(const struct option *opt,\n> >> +\t\t\t\t const char *arg, int unset)\n> >> +{\n> >> +\tstruct diff_options *options = opt->value;\n> >> +\tregex_t *regex;\n> >> +\n> >> +\tBUG_ON_OPT_NEG(unset);\n> >> +\tregex = xcalloc(1, sizeof(*regex));\n> >> +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n> >> +\t\tdie(\"invalid regex: %s\", arg);\n> >> +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n> >> +\t\t options->ignore_regex_alloc);\n> >> +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n> >\n> > A slightly more elegant way would be to have `ignore_regex` have the type\n> > `regex_t *` and use `ALLOC_GROW_BY()` (which zeroes the newly-added\n> > elements automagically).\n>\n> It may be \"elegant\", but we we know if it is \"correct\" on\n> everybody's implementation of regex_t?\n>\n> A struct like\n>\n> \tstruct foo {\n> \t\tchar *str;\n> \t\tchar in_place_buffer[10];\n> \t};\n>\n> where str points at in_place_buffer[] only when it needs to point at\n> a very short string, and points at an allocated memory on heap, can\n> not be safely copied and used, and an array of such a struct breaks\n> when ALLOC_GROW_BY() needs to call realloc(). Keeping an array of\n> pointers to regex_t and growing it would not break even for such a\n> structure type.\n>\n> Since we cannot rely on the implementation detail of regex_t on a\n> single platform, I'd rather not to see anybody suggest such an\n> \"elegant\" approach.\n\nTrue, such an implementation detail is totally conceivable.\n\nThanks for the sanity check.\n\nHaving said that, my suggestion to use `ALLOC_GROW_BY()` was triggered by\nthe use of `xcalloc()`, and now that I think further about it, an\n`xmalloc()` should be plenty enough: `regcomp()` does not need that struct\nto be initialized to all zero.\n\nCiao,\nDscho\n"},{"id":"407396","messageId":"xmqqpn5nm87u.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010122236130.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T21:43:01Z","receivedAt":"2020-10-12T21:43:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> the use of `xcalloc()`, and now that I think further about it, an\n> `xmalloc()` should be plenty enough: `regcomp()` does not need that struct\n> to be initialized to all zero.\n\nYup, I think it makes sense.\n"},{"id":"407427","messageId":"20201013063549.GA3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqsgajnrvo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] merge-base, xdiff: zero out xpparam_t structures","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:35:49Z","receivedAt":"2020-10-13T06:35:56Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Junio, Johannes,\n\nThank you for your feedback.\n\n> But the patch is good as-is, given especially the way how xecfg is\n> cleared the same way in builtin/merge-tree.c::show_diff().\n\nI just wanted to explain that I initially planned to take the \"= { 0 }\"\napproach and I started checking whether all xpparam_t structures in the\nsource tree are stack-allocated, but I quickly noticed that, as Junio\npointed out, existing code showed an inclination towards memset(), so I\nsettled for that instead.\n\n> Will queue. Thanks.\n\nI have a process question: since the other two patches in this patch\nseries will need a v3, would you like me to keep bundling patch 1 in\nthis series or should I only send revised version of patches 2 & 3 in\nv3?\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407428","messageId":"20201013063646.GB3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010121315170.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:36:46Z","receivedAt":"2020-10-13T06:36:53Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Hi Johannes,\n\n> > @@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n> > \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n> > \t\t\t N_(\"ignore changes whose lines are all blank\"),\n> > \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n> > +\t\tOPT_CALLBACK_F('I', NULL, options, N_(\"\"),\n> > +\t\t\t N_(\"ignore changes whose all lines match \"),\n> > +\t\t\t 0, diff_opt_ignore_regex),\n> > \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n> > \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n> > \t\t\tXDF_INDENT_HEURISTIC),\n> \n> Are we releasing the `ignore_regex` anywhere?\n\nOops, I tried to mimic what is done for 'anchors', and I failed to\nnotice that apparently the elements of the options->anchors array are\nonly free()'d when --patience is also used and the array pointer itself\nis never free()'d at all. Given this, I believe I need to fix\ndiff_opt_ignore_regex() in patch 2 and also make sure that the memory\nallocated in diff_opt_anchored() gets properly released - in another\npreliminary clean-up patch?\n\nAt first glance, diff_flush() - specifically the part below the\n'free_queue' label - looks like a sane place to free() things. Am I\nmistaken?\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407429","messageId":"20201013063722.GC3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqpn5nm87u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:37:22Z","receivedAt":"2020-10-13T06:37:28Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"> > the use of `xcalloc()`, and now that I think further about it, an\n> > `xmalloc()` should be plenty enough: `regcomp()` does not need that struct\n> > to be initialized to all zero.\n> \n> Yup, I think it makes sense.\n\nSure thing, I will switch to xmalloc() in v3. xcalloc()'s semantics\n(\"allocate one structure of this size\") appealed to me, but there is\nindeed no need to zero-initialize the allocated memory before passing it\nto regcomp().\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407430","messageId":"20201013063803.GD3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqk0vvpbmp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:38:03Z","receivedAt":"2020-10-13T06:38:09Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"> > +-I::\n> \n> Since we are mimicking other folks' feature, giving also the\n> \n> --ignore-matching-lines=\n> \n> synonym to that their users are familiar with would be a good idea,\n> no?\n\nSome diff implementations (e.g. the OpenBSD one) lack the long option\nand I was aiming for the \"greatest common divisor\" for all diff\nimplementations, but sure, I do not see how adding the synonym could\nhurt. I will do that in v3.\n\n> > @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n> > \treturn 0;\n> > }\n> > \n> > +static int diff_opt_ignore_regex(const struct option *opt,\n> > +\t\t\t\t const char *arg, int unset)\n> > +{\n> > +\tstruct diff_options *options = opt->value;\n> > +\tregex_t *regex;\n> > +\n> > +\tBUG_ON_OPT_NEG(unset);\n> > +\tregex = xcalloc(1, sizeof(*regex));\n> > +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n> > +\t\tdie(\"invalid regex: %s\", arg);\n> > +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n> > +\t\t options->ignore_regex_alloc);\n> > +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n> > +\treturn 0;\n> > +}\n> \n> Don't these parse-options callback functions have a way to tell the\n> caller die instead of them themselves dying like this? Would it\n> work better to \"return error(... message ...)\", or would that give\n> two error messages?\n\nYes, that would indeed look better, thanks - just one error message is\ngenerated when \"return error(...)\" is used. I just failed to notice\nthat it is a thing. I will use it in v3.\n\n> In anycase, this is end-user facing, so we'd\n> want it to be localizable, e.g.\n> \n> \tdie(_(\"invalid regexp given to -I: '%s'\", arg));\n> \n> probably.\n\nAbsolutely, I will fix that in v3. I tried to mimic what other similar\ncode in the tree does and a quick `git grep 'die.*regex'` yielded mostly\nnon-localized strings, so I settled for one as well.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407431","messageId":"20201013063819.GE3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqk0vvnrcb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:38:19Z","receivedAt":"2020-10-13T06:38:25Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"> > +\t/* see Documentation/diff-options.txt */\n> \n> This comment adds negative value.\n> \n> If it were\n> \n> \t/* \"-I\" */\n> \n> the readers won't have to switch to the file only to find out that\n> the comment didn't tell them where in the file to look at. \n\nAgreed, thanks, I will fix this in v3.\n\n> > +\tregex_t **ignore_regex;\n> > +\tsize_t ignore_regex_nr, ignore_regex_alloc;\n> > +\n> > \tconst char *single_follow;\n> > \tconst char *a_prefix, *b_prefix;\n> > \tconst char *line_prefix;\n> > diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> > index 032e3a9f41..883a0d770e 100644\n> > --- a/xdiff/xdiff.h\n> > +++ b/xdiff/xdiff.h\n> > @@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n> > typedef struct s_xpparam {\n> > \tunsigned long flags;\n> > \n> > +\t/* See Documentation/diff-options.txt. */\n> \n> Likewise.\n\nI will fix this in v3.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407432","messageId":"20201013063846.GF3278@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010121320190.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T06:38:46Z","receivedAt":"2020-10-13T06:38:52Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Hi Johannes,\n\n> > Exercise the new -I diff option in various scenarios to ensure it\n> > behaves as expected.\n> \n> Excellent. I was actually looking for a test in patch 2/3 and almost\n> commented about that.\n\nRight, I expressed my doubts in this area at the end of the cover letter\nfor v1:\n\n>> - Should tests be added in a separate commit? This is what I did as I\n>> thought it would help with readability, but...\n\nI will be glad to follow any guidance provided, I just picked one of the\ntwo possible routes for v1.\n\n> Hmm. I wonder whether we could do with a much more concise test script.\n> The test suite already takes a quite long time to run, which is not a\n> laughing matter: we had issues in the past where contributors would skip\n> running it because it took too long, and this test is sure to exacerbate\n> that problem.\n\nFirst, let me say that the goal of minimizing the run time of a test\nsuite is close to my heart (it is an issue at my day job). Yet, I\nassumed that this new test would not be detrimental to test suite run\ntimes as it takes about half a second to run t4069-diff-ignore-regex.sh\non my machine - and (I hope) its contents are in line with the \"tests\nare the best documentation\" proverb. That being said, I realize that\nthe hosts used in various test environments may have different\nprocessing capabilities. I tried preparing something exhaustive and\nwell-commented, so that it is clear what to expect from the new feature.\nYet, if you would rather have me cut some things out, I am certainly not\nparticularly attached to the tests from patch 3 and I will be glad to\nrip them out if that is the recommendation :-)\n\n> I could imagine, for example, that it would be plenty enough to do\n> something like this instead:\n> \n> -- snip --\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 5c7b0122b4f..bf158be137f 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -6,6 +6,7 @@\n> test_description='Various diff formatting options'\n> \n> . ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/diff-lib.sh\n> \n> test_expect_success setup '\n> \n> @@ -473,4 +474,24 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '-I' '\n> +\tseq 50 >I.txt &&\n> +\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n> +\ttest_must_fail git diff --no-index -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tdiff --git a/I.txt b/J.txt\n> +\t--- a/I.txt\n> +\t+++ b/J.txt\n> +\t@@ -34,7 +31,6 @@\n> +\t 34\n> +\t 35\n> +\t 36\n> +\t-37\n> +\t 38\n> +\t 39\n> +\t 40\n> +\tEOF\n> +\tcompare_diff_patch expect actual\n> +'\n> +\n> test_done\n> -- snap --\n> \n> Note how it tests various things in one go?\n\nRight, neat, though this does not (yet) test:\n\n - the interaction between -I and --ignore-blank-lines (this is visible\n in code coverage),\n\n - whether the list of hunks emitted varies for different -U values,\n\n - diffstat with -I,\n\n - invalid regular expressions.\n\nWould you like me to add these tests to your proposal or to skip them,\ngiven that -I uses the same field for marking changes as ignored as\n--ignore-blank-lines does?\n\n> P.S.: My main interest in the `-I` option is its use case for `git\n> range-diff` in Git's own context: if you want to compare your patches to\n> what entered the `seen` branch, there will _always_ be a difference\n> because Junio adds their DCO. Something like this can help that:\n> \n> git range-diff \\\n> \t-I'^ Signed-off-by: Junio C Hamano $' \\\n> \t \n\nRight, makes sense, I have not thought of that use case.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407443","messageId":"nycvar.QRO.7.76.6.2010131337320.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201013063846.GF3278@larwa.hq.kempniu.pl","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-13T12:00:31Z","receivedAt":"2020-10-13T14:08:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Tue, 13 Oct 2020, Michał Kępień wrote:\n\n> > Hmm. I wonder whether we could do with a much more concise test script.\n> > The test suite already takes a quite long time to run, which is not a\n> > laughing matter: we had issues in the past where contributors would skip\n> > running it because it took too long, and this test is sure to exacerbate\n> > that problem.\n>\n> First, let me say that the goal of minimizing the run time of a test\n> suite is close to my heart (it is an issue at my day job). Yet, I\n> assumed that this new test would not be detrimental to test suite run\n> times as it takes about half a second to run t4069-diff-ignore-regex.sh\n> on my machine - and (I hope) its contents are in line with the \"tests\n> are the best documentation\" proverb.\n\nSadly, the test is not quite as fast on Windows. I just ran this (on a not\nquite idle machine, admittedly) and it ended in this:\n\n\t# passed all 11 test(s)\n\t1..11\n\n\treal 0m51.470s\n\tuser 0m0.046s\n\tsys 0m0.015s\n\nYes, that's almost a minute.\n\n> > I could imagine, for example, that it would be plenty enough to do\n> > something like this instead:\n> >\n> > -- snip --\n> > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> > index 5c7b0122b4f..bf158be137f 100755\n> > --- a/t/t4013-diff-various.sh\n> > +++ b/t/t4013-diff-various.sh\n> > @@ -6,6 +6,7 @@\n> > test_description='Various diff formatting options'\n> >\n> > . ./test-lib.sh\n> > +. \"$TEST_DIRECTORY\"/diff-lib.sh\n> >\n> > test_expect_success setup '\n> >\n> > @@ -473,4 +474,24 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success '-I' '\n> > +\tseq 50 >I.txt &&\n> > +\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n> > +\ttest_must_fail git diff --no-index -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> > +\tcat >expect <<-\\EOF &&\n> > +\tdiff --git a/I.txt b/J.txt\n> > +\t--- a/I.txt\n> > +\t+++ b/J.txt\n> > +\t@@ -34,7 +31,6 @@\n> > +\t 34\n> > +\t 35\n> > +\t 36\n> > +\t-37\n> > +\t 38\n> > +\t 39\n> > +\t 40\n> > +\tEOF\n> > +\tcompare_diff_patch expect actual\n> > +'\n> > +\n> > test_done\n> > -- snap --\n> >\n> > Note how it tests various things in one go?\n>\n> Right, neat, though this does not (yet) test:\n>\n> - the interaction between -I and --ignore-blank-lines (this is visible\n> in code coverage),\n\nRight. Any chance you can finagle that in, e.g. by yet another `-e`\nargument to the `sed` call?\n\n> - whether the list of hunks emitted varies for different -U values,\n\nI am not worried about that.\n\n> - diffstat with -I,\n\nI am not worried about that, either, as `diffstat` consumes `xdiff`'s\noutput, therefore if one consumer works, another consumer will work, too.\n\n> - invalid regular expressions.\n\nRight, that should be super easy (and quick) to test.\n\n> Would you like me to add these tests to your proposal or to skip them,\n> given that -I uses the same field for marking changes as ignored as\n> --ignore-blank-lines does?\n\nI'd say it depends how easily (read: in how small a test case or\nmodifications to an existing test case) you can add a test for that\ninteraction.\n\nThanks,\nDscho\n"},{"id":"407444","messageId":"nycvar.QRO.7.76.6.2010131401110.50@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201013063646.GB3278@larwa.hq.kempniu.pl","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-13T12:02:05Z","receivedAt":"2020-10-13T14:10:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Tue, 13 Oct 2020, Michał Kępień wrote:\n\n> Hi Johannes,\n>\n> > > @@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n> > > \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n> > > \t\t\t N_(\"ignore changes whose lines are all blank\"),\n> > > \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n> > > +\t\tOPT_CALLBACK_F('I', NULL, options, N_(\"\"),\n> > > +\t\t\t N_(\"ignore changes whose all lines match \"),\n> > > +\t\t\t 0, diff_opt_ignore_regex),\n> > > \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n> > > \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n> > > \t\t\tXDF_INDENT_HEURISTIC),\n> >\n> > Are we releasing the `ignore_regex` anywhere?\n>\n> Oops, I tried to mimic what is done for 'anchors', and I failed to\n> notice that apparently the elements of the options->anchors array are\n> only free()'d when --patience is also used and the array pointer itself\n> is never free()'d at all. Given this, I believe I need to fix\n> diff_opt_ignore_regex() in patch 2 and also make sure that the memory\n> allocated in diff_opt_anchored() gets properly released - in another\n> preliminary clean-up patch?\n>\n> At first glance, diff_flush() - specifically the part below the\n> 'free_queue' label - looks like a sane place to free() things. Am I\n> mistaken?\n\nOh wow, from a cursory look it seems as if the diff machinery was not\nexactly careful with releasing memory. I might be mistaken, but if I am\nnot, then this would deserve a separate patch series, methinks.\n\nCiao,\nDscho\n"},{"id":"407457","messageId":"xmqqk0vuktxm.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201013063722.GC3278@larwa.hq.kempniu.pl","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T15:49:09Z","receivedAt":"2020-10-13T15:49:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> ... xcalloc()'s semantics\n> (\"allocate one structure of this size\") appealed to me, ...\n\nFWIW, it appeals to me, too ;-) But if we are going to pass it to\nregcomp(), there is no point clearing it beforehand.\n"},{"id":"407458","messageId":"xmqqft6iktq0.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010131401110.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T15:53:43Z","receivedAt":"2020-10-13T15:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Oh wow, from a cursory look it seems as if the diff machinery was not\n> exactly careful with releasing memory. I might be mistaken, but if I am\n> not, then this would deserve a separate patch series, methinks.\n\nI wouldn't be surprised if newer parts of it is much less careful\nthan the older parts of the machinery. Most callers of the diff\nmachinery, including \"log -p\" that repeatedly generates patches,\nwould however run just a single setup for the entire series of diff\ninvocations before tearing it down AFIAR, so leaking the result of\nparsing the command line option may not be such a big deal to these\ncallers. But some callers added recently may not follow the access\npattern, in which case the machinery may have to be made more\nleakproof.\n\nThanks.\n"},{"id":"407459","messageId":"xmqqblh6ktfa.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010131337320.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-13T16:00:09Z","receivedAt":"2020-10-13T16:00:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> - diffstat with -I,\n>\n> I am not worried about that, either, as `diffstat` consumes `xdiff`'s\n> output, therefore if one consumer works, another consumer will work, too.\n\nCareful. Such a \"we know what happens in the code\" transparent-box\ntesting attitude is laying a minefield for later our selves.\n\nAs we learned in a recent bug in sequencer, some corners of\nimplementation do the same thing in different codepaths as\noptimization. The really bad part of the story is that such an\nimplementation detail can and will change over time, since that is\nthe kind of thing we do when optimizing code.\n\nIn other words, we only know what happens in the current code. And\nautomated tests protect us from the future, when done right. If\nwritten with too intimate knowledge of how the current code works,\nwell, what are we really testing? It's a balancing act and there is\nno single \"right\" answer, but I'd draw the line on a bit more\ncareful side than you are drawing here.\n\nThanks.\n\n"},{"id":"407467","messageId":"20201013184553.GA9341@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010131401110.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T18:45:53Z","receivedAt":"2020-10-13T18:45:59Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Hi Johannes,\n\n> Oh wow, from a cursory look it seems as if the diff machinery was not\n> exactly careful with releasing memory. I might be mistaken, but if I am\n> not, then this would deserve a separate patch series, methinks.\n\nOh, okay. Since that sounds like a non-trivial task, I should probably\nfinish this patch series first, making sure it releases memory properly,\nand only then move on to looking for memory leaks in diff.c.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407469","messageId":"20201013190113.GB9341@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2010131337320.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-13T19:01:13Z","receivedAt":"2020-10-13T19:01:18Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Hi Johannes,\n\n> > First, let me say that the goal of minimizing the run time of a test\n> > suite is close to my heart (it is an issue at my day job). Yet, I\n> > assumed that this new test would not be detrimental to test suite run\n> > times as it takes about half a second to run t4069-diff-ignore-regex.sh\n> > on my machine - and (I hope) its contents are in line with the \"tests\n> > are the best documentation\" proverb.\n> \n> Sadly, the test is not quite as fast on Windows. I just ran this (on a not\n> quite idle machine, admittedly) and it ended in this:\n> \n> \t# passed all 11 test(s)\n> \t1..11\n> \n> \treal 0m51.470s\n> \tuser 0m0.046s\n> \tsys 0m0.015s\n> \n> Yes, that's almost a minute.\n\nOut of curiosity: is that under Cygwin? I have seen shell-based tests\nfinishing in 15 *seconds* on Unix-like systems and in 15 *minutes* under\nCygwin, which would be in line with your measurements provided above.\n\n> > Right, neat, though this does not (yet) test:\n> >\n> > - the interaction between -I and --ignore-blank-lines (this is visible\n> > in code coverage),\n> \n> Right. Any chance you can finagle that in, e.g. by yet another `-e`\n> argument to the `sed` call?\n\nI will try in v3 (while also looking at what I can do for other missing\n-I tests I pointed out).\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407580","messageId":"20201015072406.4506-2-michal@isc.org","threadId":"54333","inReplyTo":"20201015072406.4506-1-michal@isc.org","subject":"[PATCH v3 1/2] merge-base, xdiff: zero out xpparam_t structures","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-15T07:24:05Z","receivedAt":"2020-10-15T07:24:17Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"xpparam_t structures are usually zero-initialized before their specific\nfields are assigned to, but there are three locations in the tree where\nthat does not happen. Add the missing memset() calls in order to make\ninitialization of xpparam_t structures consistent tree-wide and to\nprevent stack garbage from being used as field values.\n\nSigned-off-by: Michał Kępień \n---\n builtin/merge-tree.c | 1 +\n xdiff/xhistogram.c | 2 ++\n xdiff/xpatience.c | 2 ++\n 3 files changed, 5 insertions(+)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex e72714a5a8..de8520778d 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -109,6 +109,7 @@ static void show_diff(struct merge_list *entry)\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex c7b35a9667..e694bfd9e3 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -235,6 +235,8 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpparam;\n+\n+\tmemset(&xpparam, 0, sizeof(xpparam));\n \txpparam.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(env, &xpparam,\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex 3c5601b602..20699a6f60 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -318,6 +318,8 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpp;\n+\n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = map->xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(map->env, &xpp,\n-- \n2.28.0\n\n"},{"id":"407581","messageId":"20201015072406.4506-1-michal@isc.org","threadId":"54333","inReplyTo":"20201012091751.19594-1-michal@isc.org","subject":"[PATCH v3 0/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-15T07:24:04Z","receivedAt":"2020-10-15T07:24:18Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"This patch series adds a new diff option that enables ignoring changes\nwhose all lines (changed, removed, and added) match a given regular\nexpression. This is similar to the -I/--ignore-matching-lines option in\nstandalone diff utilities and can be used e.g. to ignore changes which\nonly affect code comments or to look for unrelated changes in commits\ncontaining a large number of automatically applied modifications (e.g. a\ntree-wide string replacement). The difference between -G/-S and the new\n-I option is that the latter filters output on a per-change basis.\n\nChanges from v2:\n\n - Add a long option for -I (--ignore-matching-lines) as it is\n commonplace in standalone diff utilities. Update documentation and\n commit log messages accordingly.\n\n - Use xmalloc() instead of xcalloc() for allocating regex_t\n structures in diff_opt_ignore_regex().\n\n - Ensure the memory allocated in diff_opt_ignore_regex() gets\n released.\n\n - Use \"return error(...)\" instead of die() in the -I option callback.\n Make the relevant error message localizable.\n\n - Drastically reduce the number of -I tests due to excessive\n run time of t/t4069-diff-ignore-regex.sh from v1/v2 on some\n platforms (notably Windows). Use a tweaked version of a test\n suggested by Johannes Schindelin (thanks!). Given its reduction in\n size, squash patch 3 (which contained the tests) into patch 2.\n\n - Replace \"see Documentation/diff-options.txt\" with \"-I\" in the\n comments for the added structure fields, in order to make these\n comments more useful.\n\nChanges from v1:\n\n - Add a new preliminary cleanup patch which ensures xpparam_t\n structures are always zero-initialized. (This was a prerequisite\n for the next change below.)\n\n - Do not add a new 'xdl_opts' flag to check whether -I was used;\n instead, just check whether the array of regular expressions to\n match against is non-NULL.\n\n - Enable the -I option to be used multiple times. As a consequence of\n this, regular expressions are now \"pre-compiled\" in the option's\n callback (and passed around as an array of regex_t structures)\n rather than deep down in xdiff code. Add test cases exercising use\n of multiple -I options in the same git invocation. Update\n documentation accordingly.\n\n - Rename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to\n indicate that it is logically a \"sibling\" of\n xdl_mark_ignorable_regex() rather than its \"parent\".\n\n - Optimize xdl_mark_ignorable_regex() by making it immediately skip\n changes already marked as ignored by xdl_mark_ignorable_lines().\n\n - Fix coding style issue in the prototype part of the definition of\n xdl_mark_ignorable_regex().\n\n - Add \"/* see Documentation/diff-options.txt */\" comments for the\n fields added to struct diff_options and xpparam_t, mimicking the\n comments used for 'anchors', 'anchors_nr', and 'anchors_alloc'.\n\n - Revise commit log messages to reflect all of the above.\n\nMichał Kępień (2):\n merge-base, xdiff: zero out xpparam_t structures\n diff: add -I that ignores matching changes\n\n Documentation/diff-options.txt | 5 ++++\n builtin/merge-tree.c | 1 +\n diff.c | 28 ++++++++++++++++++++\n diff.h | 4 +++\n t/t4013-diff-various.sh | 33 ++++++++++++++++++++++++\n xdiff/xdiff.h | 4 +++\n xdiff/xdiffi.c | 47 ++++++++++++++++++++++++++++++++--\n xdiff/xhistogram.c | 2 ++\n xdiff/xpatience.c | 2 ++\n 9 files changed, 124 insertions(+), 2 deletions(-)\n\n-- \n2.28.0\n\n"},{"id":"407582","messageId":"20201015072406.4506-3-michal@isc.org","threadId":"54333","inReplyTo":"20201015072406.4506-1-michal@isc.org","subject":"[PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-15T07:24:06Z","receivedAt":"2020-10-15T07:24:22Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Add a new diff option that enables ignoring changes whose all lines\n(changed, removed, and added) match a given regular expression. This is\nsimilar to the -I/--ignore-matching-lines option in standalone diff\nutilities and can be used e.g. to ignore changes which only affect code\ncomments or to look for unrelated changes in commits containing a large\nnumber of automatically applied modifications (e.g. a tree-wide string\nreplacement). The difference between -G/-S and the new -I option is\nthat the latter filters output on a per-change basis.\n\nUse the 'ignore' field of xdchange_t for marking a change as ignored or\nnot. Since the same field is used by --ignore-blank-lines, identical\nhunk emitting rules apply for --ignore-blank-lines and -I. These two\noptions can also be used together in the same git invocation (they are\ncomplementary to each other).\n\nRename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to indicate\nthat it is logically a \"sibling\" of xdl_mark_ignorable_regex() rather\nthan its \"parent\".\n\nSigned-off-by: Michał Kępień \n---\n Documentation/diff-options.txt | 5 ++++\n diff.c | 28 ++++++++++++++++++++\n diff.h | 4 +++\n t/t4013-diff-various.sh | 33 ++++++++++++++++++++++++\n xdiff/xdiff.h | 4 +++\n xdiff/xdiffi.c | 47 ++++++++++++++++++++++++++++++++--\n 6 files changed, 119 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 573fb9bb71..ee52b65e46 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -687,6 +687,11 @@ endif::git-format-patch[]\n --ignore-blank-lines::\n \tIgnore changes whose lines are all blank.\n \n+-I::\n+--ignore-matching-lines=::\n+\tIgnore changes whose all lines match . This option may\n+\tbe specified more than once.\n+\n --inter-hunk-context=::\n \tShow the context between diff hunks, up to the specified number\n \tof lines, thereby fusing hunks that are close to each other.\ndiff --git a/diff.c b/diff.c\nindex 2bb2f8f57e..677de23352 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3587,6 +3587,8 @@ static void builtin_diff(const char *name_a,\n \t\tif (header.len && !o->flags.suppress_diff_headers)\n \t\t\tecbdata.header = &header;\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -3716,6 +3718,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_ignore_regex(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct diff_options *options = opt->value;\n+\tregex_t *regex;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tregex = xmalloc(sizeof(*regex));\n+\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n+\t\treturn error(_(\"invalid regex given to -I: '%s'\"), arg);\n+\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n+\t\t options->ignore_regex_alloc);\n+\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n+\treturn 0;\n+}\n+\n static int diff_opt_pickaxe_regex(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n \t\t\t N_(\"ignore changes whose lines are all blank\"),\n \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F('I', \"ignore-matching-lines\", options, N_(\"\"),\n+\t\t\t N_(\"ignore changes whose all lines match \"),\n+\t\t\t 0, diff_opt_ignore_regex),\n \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n \t\t\tXDF_INDENT_HEURISTIC),\n@@ -6405,6 +6428,11 @@ void diff_flush(struct diff_options *options)\n \tDIFF_QUEUE_CLEAR(q);\n \tif (options->close_file)\n \t\tfclose(options->file);\n+\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n+\t\tregfree(options->ignore_regex[i]);\n+\t\tfree(options->ignore_regex[i]);\n+\t}\n+\tfree(options->ignore_regex);\n \n \t/*\n \t * Report the content-level differences with HAS_CHANGES;\ndiff --git a/diff.h b/diff.h\nindex 11de52e9e9..a402227b80 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -234,6 +234,10 @@ struct diff_options {\n \t */\n \tconst char *pickaxe;\n \n+\t/* -I */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr, ignore_regex_alloc;\n+\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tconst char *line_prefix;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5c7b0122b4..efaaee2ef0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -6,6 +6,7 @@\n test_description='Various diff formatting options'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n \n test_expect_success setup '\n \n@@ -473,4 +474,36 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff -I' '\n+\ttest_seq 50 >I.txt &&\n+\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n+\techo >>J.txt &&\n+\n+\ttest_expect_code 1 git diff --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/I.txt b/J.txt\n+\t--- a/I.txt\n+\t+++ b/J.txt\n+\t@@ -34,7 +31,6 @@\n+\t 34\n+\t 35\n+\t 36\n+\t-37\n+\t 38\n+\t 39\n+\t 40\n+\tEOF\n+\tcompare_diff_patch expect actual &&\n+\n+\ttest_expect_code 1 git diff --stat --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t I.txt => J.txt | 1 -\n+\t 1 file changed, 1 deletion(-)\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\ttest_expect_code 129 git diff --no-index --ignore-matching-lines=\"^[124-9]\" --ignore-matching-lines=\"^[124-9\" I.txt J.txt >output 2>&1 &&\n+\ttest_i18ngrep \"invalid regex given to -I: \" output\n+'\n+\n test_done\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 032e3a9f41..7a04605146 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n typedef struct s_xpparam {\n \tunsigned long flags;\n \n+\t/* -I */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr;\n+\n \t/* See Documentation/diff-options.txt. */\n \tchar **anchors;\n \tsize_t anchors_nr;\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex bd035139f9..380eb728ed 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -998,7 +998,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \treturn 0;\n }\n \n-static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n+static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n {\n \txdchange_t *xch;\n \n@@ -1019,6 +1019,46 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n \t}\n }\n \n+static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n+\tregmatch_t regmatch;\n+\tint i;\n+\n+\tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n+\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n+\t\t\t\t ®match, 0))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n+\t\t\t\t xpparam_t const *xpp)\n+{\n+\txdchange_t *xch;\n+\n+\tfor (xch = xscr; xch; xch = xch->next) {\n+\t\txrecord_t **rec;\n+\t\tint ignore = 1;\n+\t\tlong i;\n+\n+\t\t/*\n+\t\t * Do not override --ignore-blank-lines.\n+\t\t */\n+\t\tif (xch->ignore)\n+\t\t\tcontinue;\n+\n+\t\trec = &xe->xdf1.recs[xch->i1];\n+\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\trec = &xe->xdf2.recs[xch->i2];\n+\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\txch->ignore = ignore;\n+\t}\n+}\n+\n int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n \txdchange_t *xscr;\n@@ -1038,7 +1078,10 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t}\n \tif (xscr) {\n \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n-\t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n+\t\t\txdl_mark_ignorable_lines(xscr, &xe, xpp->flags);\n+\n+\t\tif (xpp->ignore_regex)\n+\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp);\n \n \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n \n-- \n2.28.0\n\n"},{"id":"407594","messageId":"nycvar.QRO.7.76.6.2010151343570.56@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201013190113.GB9341@larwa.hq.kempniu.pl","subject":"Re: [PATCH v2 3/3] t: add -I tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-15T11:45:09Z","receivedAt":"2020-10-15T11:46:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Tue, 13 Oct 2020, Michał Kępień wrote:\n\n> > > First, let me say that the goal of minimizing the run time of a test\n> > > suite is close to my heart (it is an issue at my day job). Yet, I\n> > > assumed that this new test would not be detrimental to test suite run\n> > > times as it takes about half a second to run t4069-diff-ignore-regex.sh\n> > > on my machine - and (I hope) its contents are in line with the \"tests\n> > > are the best documentation\" proverb.\n> >\n> > Sadly, the test is not quite as fast on Windows. I just ran this (on a not\n> > quite idle machine, admittedly) and it ended in this:\n> >\n> > \t# passed all 11 test(s)\n> > \t1..11\n> >\n> > \treal 0m51.470s\n> > \tuser 0m0.046s\n> > \tsys 0m0.015s\n> >\n> > Yes, that's almost a minute.\n>\n> Out of curiosity: is that under Cygwin? I have seen shell-based tests\n> finishing in 15 *seconds* on Unix-like systems and in 15 *minutes* under\n> Cygwin, which would be in line with your measurements provided above.\n\nClose enough: it's an MSYS2 Bash (and MSYS2 is a close fork of Cygwin). It\nis what Git for Windows uses.\n\nCiao,\nDscho\n"},{"id":"407720","messageId":"nycvar.QRO.7.76.6.2010161200090.56@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20201015072406.4506-1-michal@isc.org","subject":"Re: [PATCH v3 0/2] diff: add -I that ignores matching changes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-16T10:00:54Z","receivedAt":"2020-10-16T15:11:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michał,\n\nOn Thu, 15 Oct 2020, Michał Kępień wrote:\n\n> This patch series adds a new diff option that enables ignoring changes\n> whose all lines (changed, removed, and added) match a given regular\n> expression. This is similar to the -I/--ignore-matching-lines option in\n> standalone diff utilities and can be used e.g. to ignore changes which\n> only affect code comments or to look for unrelated changes in commits\n> containing a large number of automatically applied modifications (e.g. a\n> tree-wide string replacement). The difference between -G/-S and the new\n> -I option is that the latter filters output on a per-change basis.\n>\n> Changes from v2:\n>\n> - Add a long option for -I (--ignore-matching-lines) as it is\n> commonplace in standalone diff utilities. Update documentation and\n> commit log messages accordingly.\n>\n> - Use xmalloc() instead of xcalloc() for allocating regex_t\n> structures in diff_opt_ignore_regex().\n>\n> - Ensure the memory allocated in diff_opt_ignore_regex() gets\n> released.\n>\n> - Use \"return error(...)\" instead of die() in the -I option callback.\n> Make the relevant error message localizable.\n>\n> - Drastically reduce the number of -I tests due to excessive\n> run time of t/t4069-diff-ignore-regex.sh from v1/v2 on some\n> platforms (notably Windows). Use a tweaked version of a test\n> suggested by Johannes Schindelin (thanks!). Given its reduction in\n> size, squash patch 3 (which contained the tests) into patch 2.\n>\n> - Replace \"see Documentation/diff-options.txt\" with \"-I\" in the\n> comments for the added structure fields, in order to make these\n> comments more useful.\n\nThank you for this diligent work! I looked over the patches and like them\na lot!\n\nThanks,\nDscho\n"},{"id":"407721","messageId":"afd3b1cf-b883-6df5-bea5-28f8e06d8702@gmail.com","threadId":"54333","inReplyTo":"20201015072406.4506-3-michal@isc.org","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-10-16T15:32:47Z","receivedAt":"2020-10-16T15:33:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michał\n\nThanks for working on this, it will be a useful addition. Unfortunately \nthere's a use-after-free error see below\n\nOn 15/10/2020 08:24, Michał Kępień wrote:\n> Add a new diff option that enables ignoring changes whose all lines\n> (changed, removed, and added) match a given regular expression. This is\n> similar to the -I/--ignore-matching-lines option in standalone diff\n> utilities and can be used e.g. to ignore changes which only affect code\n> comments or to look for unrelated changes in commits containing a large\n> number of automatically applied modifications (e.g. a tree-wide string\n> replacement). The difference between -G/-S and the new -I option is\n> that the latter filters output on a per-change basis.\n> \n> Use the 'ignore' field of xdchange_t for marking a change as ignored or\n> not. Since the same field is used by --ignore-blank-lines, identical\n> hunk emitting rules apply for --ignore-blank-lines and -I. These two\n> options can also be used together in the same git invocation (they are\n> complementary to each other).\n> \n> Rename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to indicate\n> that it is logically a \"sibling\" of xdl_mark_ignorable_regex() rather\n> than its \"parent\".\n> \n> Signed-off-by: Michał Kępień \n> ---\n> Documentation/diff-options.txt | 5 ++++\n> diff.c | 28 ++++++++++++++++++++\n> diff.h | 4 +++\n> t/t4013-diff-various.sh | 33 ++++++++++++++++++++++++\n> xdiff/xdiff.h | 4 +++\n> xdiff/xdiffi.c | 47 ++++++++++++++++++++++++++++++++--\n> 6 files changed, 119 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 573fb9bb71..ee52b65e46 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -687,6 +687,11 @@ endif::git-format-patch[]\n> --ignore-blank-lines::\n> \tIgnore changes whose lines are all blank.\n> \n> +-I::\n> +--ignore-matching-lines=::\n> +\tIgnore changes whose all lines match . This option may\n> +\tbe specified more than once.\n> +\n> --inter-hunk-context=::\n> \tShow the context between diff hunks, up to the specified number\n> \tof lines, thereby fusing hunks that are close to each other.\n> diff --git a/diff.c b/diff.c\n> index 2bb2f8f57e..677de23352 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3587,6 +3587,8 @@ static void builtin_diff(const char *name_a,\n> \t\tif (header.len && !o->flags.suppress_diff_headers)\n> \t\t\tecbdata.header = &header;\n> \t\txpp.flags = o->xdl_opts;\n> +\t\txpp.ignore_regex = o->ignore_regex;\n> +\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n> \t\txpp.anchors = o->anchors;\n> \t\txpp.anchors_nr = o->anchors_nr;\n> \t\txecfg.ctxlen = o->context;\n> @@ -3716,6 +3718,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\tmemset(&xpp, 0, sizeof(xpp));\n> \t\tmemset(&xecfg, 0, sizeof(xecfg));\n> \t\txpp.flags = o->xdl_opts;\n> +\t\txpp.ignore_regex = o->ignore_regex;\n> +\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n> \t\txpp.anchors = o->anchors;\n> \t\txpp.anchors_nr = o->anchors_nr;\n> \t\txecfg.ctxlen = o->context;\n> @@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n> \treturn 0;\n> }\n> \n> +static int diff_opt_ignore_regex(const struct option *opt,\n> +\t\t\t\t const char *arg, int unset)\n> +{\n> +\tstruct diff_options *options = opt->value;\n> +\tregex_t *regex;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\tregex = xmalloc(sizeof(*regex));\n> +\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n> +\t\treturn error(_(\"invalid regex given to -I: '%s'\"), arg);\n> +\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n> +\t\t options->ignore_regex_alloc);\n> +\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n> +\treturn 0;\n> +}\n> +\n> static int diff_opt_pickaxe_regex(const struct option *opt,\n> \t\t\t\t const char *arg, int unset)\n> {\n> @@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n> \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n> \t\t\t N_(\"ignore changes whose lines are all blank\"),\n> \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n> +\t\tOPT_CALLBACK_F('I', \"ignore-matching-lines\", options, N_(\"\"),\n> +\t\t\t N_(\"ignore changes whose all lines match \"),\n> +\t\t\t 0, diff_opt_ignore_regex),\n> \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n> \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n> \t\t\tXDF_INDENT_HEURISTIC),\n> @@ -6405,6 +6428,11 @@ void diff_flush(struct diff_options *options)\n> \tDIFF_QUEUE_CLEAR(q);\n> \tif (options->close_file)\n> \t\tfclose(options->file);\n> +\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n> +\t\tregfree(options->ignore_regex[i]);\n> +\t\tfree(options->ignore_regex[i]);\n> +\t}\n> +\tfree(options->ignore_regex);\n\nIf I run `git log -p -I foo` then the address sanitizer reports\n\nAddressSanitizer: heap-use-after-free xdiff/xdiffi.c:1027 in \nrecord_matches_regex\n\nafter it has printed the diff for the first commit. I think freeing the \nregex here is the cause of the problem.\n\nBest Wishes\n\nPhillip\n\n> \t/*\n> \t * Report the content-level differences with HAS_CHANGES;\n> diff --git a/diff.h b/diff.h\n> index 11de52e9e9..a402227b80 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -234,6 +234,10 @@ struct diff_options {\n> \t */\n> \tconst char *pickaxe;\n> \n> +\t/* -I */\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr, ignore_regex_alloc;\n> +\n> \tconst char *single_follow;\n> \tconst char *a_prefix, *b_prefix;\n> \tconst char *line_prefix;\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 5c7b0122b4..efaaee2ef0 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -6,6 +6,7 @@\n> test_description='Various diff formatting options'\n> \n> . ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/diff-lib.sh\n> \n> test_expect_success setup '\n> \n> @@ -473,4 +474,36 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'diff -I' '\n> +\ttest_seq 50 >I.txt &&\n> +\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n> +\techo >>J.txt &&\n> +\n> +\ttest_expect_code 1 git diff --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tdiff --git a/I.txt b/J.txt\n> +\t--- a/I.txt\n> +\t+++ b/J.txt\n> +\t@@ -34,7 +31,6 @@\n> +\t 34\n> +\t 35\n> +\t 36\n> +\t-37\n> +\t 38\n> +\t 39\n> +\t 40\n> +\tEOF\n> +\tcompare_diff_patch expect actual &&\n> +\n> +\ttest_expect_code 1 git diff --stat --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\t I.txt => J.txt | 1 -\n> +\t 1 file changed, 1 deletion(-)\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\n> +\ttest_expect_code 129 git diff --no-index --ignore-matching-lines=\"^[124-9]\" --ignore-matching-lines=\"^[124-9\" I.txt J.txt >output 2>&1 &&\n> +\ttest_i18ngrep \"invalid regex given to -I: \" output\n> +'\n> +\n> test_done\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 032e3a9f41..7a04605146 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n> typedef struct s_xpparam {\n> \tunsigned long flags;\n> \n> +\t/* -I */\n> +\tregex_t **ignore_regex;\n> +\tsize_t ignore_regex_nr;\n> +\n> \t/* See Documentation/diff-options.txt. */\n> \tchar **anchors;\n> \tsize_t anchors_nr;\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index bd035139f9..380eb728ed 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -998,7 +998,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n> \treturn 0;\n> }\n> \n> -static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> +static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> {\n> \txdchange_t *xch;\n> \n> @@ -1019,6 +1019,46 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n> \t}\n> }\n> \n> +static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n> +\tregmatch_t regmatch;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n> +\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n> +\t\t\t\t ®match, 0))\n> +\t\t\treturn 1;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n> +\t\t\t\t xpparam_t const *xpp)\n> +{\n> +\txdchange_t *xch;\n> +\n> +\tfor (xch = xscr; xch; xch = xch->next) {\n> +\t\txrecord_t **rec;\n> +\t\tint ignore = 1;\n> +\t\tlong i;\n> +\n> +\t\t/*\n> +\t\t * Do not override --ignore-blank-lines.\n> +\t\t */\n> +\t\tif (xch->ignore)\n> +\t\t\tcontinue;\n> +\n> +\t\trec = &xe->xdf1.recs[xch->i1];\n> +\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n> +\t\t\tignore = record_matches_regex(rec[i], xpp);\n> +\n> +\t\trec = &xe->xdf2.recs[xch->i2];\n> +\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n> +\t\t\tignore = record_matches_regex(rec[i], xpp);\n> +\n> +\t\txch->ignore = ignore;\n> +\t}\n> +}\n> +\n> int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n> \txdchange_t *xscr;\n> @@ -1038,7 +1078,10 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> \t}\n> \tif (xscr) {\n> \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n> -\t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n> +\t\t\txdl_mark_ignorable_lines(xscr, &xe, xpp->flags);\n> +\n> +\t\tif (xpp->ignore_regex)\n> +\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp);\n> \n> \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n> \n> \n"},{"id":"407731","messageId":"xmqqeelycajx.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"afd3b1cf-b883-6df5-bea5-28f8e06d8702@gmail.com","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T18:04:02Z","receivedAt":"2020-10-16T18:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> @@ -6405,6 +6428,11 @@ void diff_flush(struct diff_options *options)\n>> \tDIFF_QUEUE_CLEAR(q);\n>> \tif (options->close_file)\n>> \t\tfclose(options->file);\n>> +\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n>> +\t\tregfree(options->ignore_regex[i]);\n>> +\t\tfree(options->ignore_regex[i]);\n>> +\t}\n>> +\tfree(options->ignore_regex);\n>\n> If I run `git log -p -I foo` then the address sanitizer reports\n>\n> AddressSanitizer: heap-use-after-free xdiff/xdiffi.c:1027 in\n> record_matches_regex\n>\n> after it has printed the diff for the first commit. I think freeing\n> the regex here is the cause of the problem.\n\nYes, this is a good location to clear the diff_queue, which is\nper-invocation. But diff_options->ignore_regex[] can and should be\nshared across multiple invocations of the diff machinery, as we are\nparsing upfront in the command line option parser just once to be\nused throughout the life of the program. It is wrong to free them\nearly like this.\n\nI really hate to suggest this, one common API call made into the\ndiff machinery from various consumers, when they are absolutely done\nwith diff_options, is probably diff_result_code(). Its purpose is\nto collect bits computed to participate in the overall result into\nthe final result code and anybody who ran one or more diff and wants\nto learn the outcome must call it, so it is unlikely that callers\nthat matter are missing a call to finalize their uses of the diff\nmachinery. So if freeing .ignore_regex[] is really necessary, it\ncould be used to tell us where to do so [*1*].\n\nUnlike per-invocation resources that MUST be freed for repeated\ninvocations of the diff machinery made in \"git log\" family,\nresources held for and repeatedly used during the entire session\nlike this may not have to be freed, to be honest, though.\n\nThanks.\n\n\n[Footnote]\n\n*1* Adding calls to free() in diff_result_code() directly is\n probably not a good idea. Some callers may not even need result\n code (e.g. \"blame\") and do not call it at all, but we may want\n to free the resource in diff_options that is not per-invocation.\n So if we were to do this, we probably need a new API function,\n perhaps diff_options_clear() or something, and call that from\n diff_result_code(), and then find callers that do not care about\n the result code and call diff_options_clear() directly from\n them.\n\n If a caller does a single diff_setup_done(), makes repeated\n calls to the diff machinery, and calls diff_result_code() once\n per each iteration (e.g. imagine \"git log -p\" that detects the\n status for each commit), then having the diff_options_clear()\n call in diff_result_code() will break the machinery, so if we\n were to follow the outline given in the previous paragraph, we\n need to make sure there is no such caller. But I see that\n diff_result_code() does not clear the fields it looks at in the\n diff_options fields after it uses them, so the second and\n subsequent calls into the diff machinery by such a caller may\n already be broken (not in the \"resource leakage\" sense, but in\n the correctness sense).\n \n"},{"id":"407733","messageId":"xmqqa6wmc9yo.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201015072406.4506-3-michal@isc.org","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T18:16:47Z","receivedAt":"2020-10-16T18:16:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n> +test_expect_success 'diff -I' '\n> +\ttest_seq 50 >I.txt &&\n> +\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n> +\techo >>J.txt &&\n> +\n> +\ttest_expect_code 1 git diff --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n\nPlease wrap an overly long line like this oned, perhaps like:\n\n\ttest_expect_code 1 git diff --no-index --ignore-blank-lines \\\n\t\t-I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n\n> +\ttest_expect_code 1 git diff --stat --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n\n> +\ttest_expect_code 129 git diff --no-index --ignore-matching-lines=\"^[124-9]\" --ignore-matching-lines=\"^[124-9\" I.txt J.txt >output 2>&1 &&\n\nCramming unrelated tests into a single one made me puzzled, staring\nat this line for longer than necessary before realizing that this is\nan attempt to catch a malformed regexp. If this were in a separate\ntest with its own title, e.g.\n\n\ttest_expect_success 'diff -I: setup' '\n\t\t... set up the sample input, perhaps sample history ...\n\t\t... perhaps prepare the expected output in \"expect\" ...\n\t'\n\n\ttest_expect_success 'diff -I -p' '\n\t\tgit diff --I\"ten.*e\" ... >actual &&\n\t\ttest_cmp expect actual\n\t'\n\n\ttest_expect_success 'diff -I --stat' '\n\t\tgit diff --stat --I\"ten.*e\" ... >actual &&\n\t\ttest_cmp expect actual\n\t'\n\n\ttest_expect_success 'diff -I: detect malformed regex'\n\t\ttest_must_fail git diff -I=\"[^124-9\" ... 2>error &&\n\t\ttest_i18ngrep \"invalid regex\" error\n\t'\n\t\t\nIt would have been much easier to follow.\n\nIt also is curious that it only tests --no-index, which is a bolted\non feature that may not exercise the codepath the users depend on\nworking correctly. If this were tested with \"git log -p\" for a few\ncommits, the early destruction of regexp may have been caught,\nespecially with ASAN build.\n\nOther than that, and also the premature destruction of regexp\npointed out by Phillip already, looks reasonably done.\n\nThanks.\n"},{"id":"407872","messageId":"20201019094850.GB3717@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqeelycajx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-19T09:48:50Z","receivedAt":"2020-10-19T09:48:57Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"First, thanks to Phillip for testing this.\n\n> Yes, this is a good location to clear the diff_queue, which is\n> per-invocation. But diff_options->ignore_regex[] can and should be\n> shared across multiple invocations of the diff machinery, as we are\n> parsing upfront in the command line option parser just once to be\n> used throughout the life of the program. It is wrong to free them\n> early like this.\n\nUgh, sorry, I am only now starting to see the big picture.\n\n> I really hate to suggest this, one common API call made into the\n> diff machinery from various consumers, when they are absolutely done\n> with diff_options, is probably diff_result_code(). Its purpose is\n> to collect bits computed to participate in the overall result into\n> the final result code and anybody who ran one or more diff and wants\n> to learn the outcome must call it, so it is unlikely that callers\n> that matter are missing a call to finalize their uses of the diff\n> machinery. So if freeing .ignore_regex[] is really necessary, it\n> could be used to tell us where to do so [*1*].\n> \n> Unlike per-invocation resources that MUST be freed for repeated\n> invocations of the diff machinery made in \"git log\" family,\n> resources held for and repeatedly used during the entire session\n> like this may not have to be freed, to be honest, though.\n\nAfter reading your message, taking a closer look, and doing a few\nValgrind runs, I tend to agree that perhaps there is no need to free the\n'ignore_regex' array after all - it does *not* get allocated repeatedly\nduring each invocation of the diff machinery and when Git exits, the OS\nwill reclaim any memory allocated by the process anyway. On my laptop,\na Valgrind run done for a -DSUPPRESS_ANNOTATED_LEAKS build claims that\n'ignore_regex' is one of 1,300+ memory blocks still reachable at exit\ntime, which amount to about 2.5 MB of memory in total. Given this, it\nfeels slightly weird to single 'ignore_regex' out.\n\nI will revert the diff_flush() changes in v4 unless somebody disagrees\nwith the above reasoning.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407873","messageId":"20201019095554.GC3717@larwa.hq.kempniu.pl","threadId":"54333","inReplyTo":"xmqqa6wmc9yo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-19T09:55:54Z","receivedAt":"2020-10-19T09:56:01Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"> Michał Kępień writes:\n> \n> > +test_expect_success 'diff -I' '\n> > +\ttest_seq 50 >I.txt &&\n> > +\tsed -e \"s/13/ten and three/\" -e \"/7\\$/d\" J.txt &&\n> > +\techo >>J.txt &&\n> > +\n> > +\ttest_expect_code 1 git diff --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> \n> Please wrap an overly long line like this oned, perhaps like:\n> \n> \ttest_expect_code 1 git diff --no-index --ignore-blank-lines \\\n> \t\t-I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n\nYeah, thanks, I was unsure what the line length limit for test code is.\nI will fix this in v4.\n\n> > +\ttest_expect_code 1 git diff --stat --no-index --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" I.txt J.txt >actual &&\n> \n> > +\ttest_expect_code 129 git diff --no-index --ignore-matching-lines=\"^[124-9]\" --ignore-matching-lines=\"^[124-9\" I.txt J.txt >output 2>&1 &&\n> \n> Cramming unrelated tests into a single one made me puzzled, staring\n> at this line for longer than necessary before realizing that this is\n> an attempt to catch a malformed regexp. If this were in a separate\n> test with its own title, e.g.\n> \n> (...)\n> \t\t\n> It would have been much easier to follow.\n> \n> It also is curious that it only tests --no-index, which is a bolted\n> on feature that may not exercise the codepath the users depend on\n> working correctly. If this were tested with \"git log -p\" for a few\n> commits, the early destruction of regexp may have been caught,\n> especially with ASAN build.\n\nSorry, I think I got a bit carried away trying to use the test suggested\nby Johannes while also extending it with the missing bits. I will try\nto come up with something more balanced in v4.\n\n-- \nBest regards,\nMichał Kępień\n"},{"id":"407883","messageId":"xmqqimb6unsb.fsf@gitster.c.googlers.com","threadId":"54333","inReplyTo":"20201019095554.GC3717@larwa.hq.kempniu.pl","subject":"Re: [PATCH v3 2/2] diff: add -I that ignores matching changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-19T17:29:56Z","receivedAt":"2020-10-19T17:30:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kępień writes:\n\n>> Cramming unrelated tests into a single one made me puzzled, staring\n>> at this line for longer than necessary before realizing that this is\n>> an attempt to catch a malformed regexp. If this were in a separate\n>> test with its own title, e.g.\n>> \t\t\n>> It would have been much easier to follow.\n> ... I will try\n> to come up with something more balanced in v4.\n\nWe want to avoid going overboard and doing a full test matrix. \n\nBut having it tested fully with one variant while testing just the\nbasic for other variants may give us a good balance, e.g. \"git log\n-p -I\" is tested but we do not bother testing interactions\nwith other options\", while we try different combinations when\ntesting \"git diff --no-index -I\", like multiple -I options\netc.\n\nThanks.\n\n\n"},{"id":"407959","messageId":"20201020064809.14297-1-michal@isc.org","threadId":"54333","inReplyTo":"20201015072406.4506-1-michal@isc.org","subject":"[PATCH v4 0/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-20T06:48:07Z","receivedAt":"2020-10-20T06:48:19Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"This patch series adds a new diff option that enables ignoring changes\nwhose all lines (changed, removed, and added) match a given regular\nexpression. This is similar to the -I/--ignore-matching-lines option in\nstandalone diff utilities and can be used e.g. to ignore changes which\nonly affect code comments or to look for unrelated changes in commits\ncontaining a large number of automatically applied modifications (e.g. a\ntree-wide string replacement). The difference between -G/-S and the new\n-I option is that the latter filters output on a per-change basis.\n\nChanges from v3:\n\n - Do not release the memory allocated in diff_opt_ignore_regex() after\n all as the free() calls added in v2 were triggering use-after-free\n errors when the diff machinery was invoked multiple times throughout\n the lifetime of a single Git command (e.g. by \"git log -p\").\n\n - Further test improvements: split various -I checks into\n multiple tests; add a check for \"git log -p -I\" to trigger\n the issue mentioned above; avoid using --no-index.\n\nChanges from v2:\n\n - Add a long option for -I (--ignore-matching-lines) as it is\n commonplace in standalone diff utilities. Update documentation and\n commit log messages accordingly.\n\n - Use xmalloc() instead of xcalloc() for allocating regex_t\n structures in diff_opt_ignore_regex().\n\n - Ensure the memory allocated in diff_opt_ignore_regex() gets\n released.\n\n - Use \"return error(...)\" instead of die() in the -I option callback.\n Make sure the error message is localizable.\n\n - Drastically reduce the number of -I tests due to excessive\n run time of t/t4069-diff-ignore-regex.sh from v1/v2 on some\n platforms (notably Windows). Use a tweaked version of a test\n suggested by Johannes Schindelin (thanks!). Squash patch 3 into\n patch 2.\n\n - Replace \"see Documentation/diff-options.txt\" with \"-I\" in the\n comments for the added structure fields, in order to make these\n comments more useful.\n\nChanges from v1:\n\n - Add a new preliminary cleanup patch which ensures xpparam_t\n structures are always zero-initialized. (This was a prerequisite\n for the next change below.)\n\n - Do not add a new 'xdl_opts' flag to check whether -I was used;\n instead, just check whether the array of regular expressions to\n match against is non-NULL.\n\n - Enable the -I option to be used multiple times. As a consequence of\n this, regular expressions are now \"pre-compiled\" in the option's\n callback (and passed around as an array of regex_t structures)\n rather than deep down in xdiff code. Add test cases exercising use\n of multiple -I options in the same git invocation. Update\n documentation accordingly.\n\n - Rename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to\n indicate that it is logically a \"sibling\" of\n xdl_mark_ignorable_regex() rather than its \"parent\".\n\n - Optimize xdl_mark_ignorable_regex() by making it immediately skip\n changes already marked as ignored by xdl_mark_ignorable_lines().\n\n - Fix coding style issue in the prototype part of the definition of\n xdl_mark_ignorable_regex().\n\n - Add \"/* see Documentation/diff-options.txt */\" comments for the\n fields added to struct diff_options and xpparam_t, mimicking the\n comments used for 'anchors', 'anchors_nr', and 'anchors_alloc'.\n\n - Revise commit log messages to reflect all of the above.\n\nMichał Kępień (2):\n merge-base, xdiff: zero out xpparam_t structures\n diff: add -I that ignores matching changes\n\n Documentation/diff-options.txt | 5 ++\n builtin/merge-tree.c | 1 +\n diff.c | 23 +++++\n diff.h | 4 +\n t/t4013-diff-various.sh | 41 +++++++++\n t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master | 99 ++++++++++++++++++++++\n xdiff/xdiff.h | 4 +\n xdiff/xdiffi.c | 47 +++++++++-\n xdiff/xhistogram.c | 2 +\n xdiff/xpatience.c | 2 +\n 10 files changed, 226 insertions(+), 2 deletions(-)\n create mode 100644 t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master\n\n-- \n2.28.0\n\n"},{"id":"407960","messageId":"20201020064809.14297-2-michal@isc.org","threadId":"54333","inReplyTo":"20201020064809.14297-1-michal@isc.org","subject":"[PATCH v4 1/2] merge-base, xdiff: zero out xpparam_t structures","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-20T06:48:08Z","receivedAt":"2020-10-20T06:48:21Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"xpparam_t structures are usually zero-initialized before their specific\nfields are assigned to, but there are three locations in the tree where\nthat does not happen. Add the missing memset() calls in order to make\ninitialization of xpparam_t structures consistent tree-wide and to\nprevent stack garbage from being used as field values.\n\nSigned-off-by: Michał Kępień \n---\n builtin/merge-tree.c | 1 +\n xdiff/xhistogram.c | 2 ++\n xdiff/xpatience.c | 2 ++\n 3 files changed, 5 insertions(+)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex e72714a5a8..de8520778d 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -109,6 +109,7 @@ static void show_diff(struct merge_list *entry)\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex c7b35a9667..e694bfd9e3 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -235,6 +235,8 @@ static int fall_back_to_classic_diff(xpparam_t const *xpp, xdfenv_t *env,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpparam;\n+\n+\tmemset(&xpparam, 0, sizeof(xpparam));\n \txpparam.flags = xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(env, &xpparam,\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex 3c5601b602..20699a6f60 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -318,6 +318,8 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n \t\tint line1, int count1, int line2, int count2)\n {\n \txpparam_t xpp;\n+\n+\tmemset(&xpp, 0, sizeof(xpp));\n \txpp.flags = map->xpp->flags & ~XDF_DIFF_ALGORITHM_MASK;\n \n \treturn xdl_fall_back_diff(map->env, &xpp,\n-- \n2.28.0\n\n"},{"id":"407961","messageId":"20201020064809.14297-3-michal@isc.org","threadId":"54333","inReplyTo":"20201020064809.14297-1-michal@isc.org","subject":"[PATCH v4 2/2] diff: add -I that ignores matching changes","fromName":"Michał Kępień","fromEmail":"michal@isc.org","sentAt":"2020-10-20T06:48:09Z","receivedAt":"2020-10-20T06:48:22Z","isPatch":true,"sender":{"key":"michal@isc.org","avatar":"https://avatars.githubusercontent.com/u/1301146?v=4"},"body":"Add a new diff option that enables ignoring changes whose all lines\n(changed, removed, and added) match a given regular expression. This is\nsimilar to the -I/--ignore-matching-lines option in standalone diff\nutilities and can be used e.g. to ignore changes which only affect code\ncomments or to look for unrelated changes in commits containing a large\nnumber of automatically applied modifications (e.g. a tree-wide string\nreplacement). The difference between -G/-S and the new -I option is\nthat the latter filters output on a per-change basis.\n\nUse the 'ignore' field of xdchange_t for marking a change as ignored or\nnot. Since the same field is used by --ignore-blank-lines, identical\nhunk emitting rules apply for --ignore-blank-lines and -I. These two\noptions can also be used together in the same git invocation (they are\ncomplementary to each other).\n\nRename xdl_mark_ignorable() to xdl_mark_ignorable_lines(), to indicate\nthat it is logically a \"sibling\" of xdl_mark_ignorable_regex() rather\nthan its \"parent\".\n\nSigned-off-by: Michał Kępień \n---\n Documentation/diff-options.txt | 5 ++\n diff.c | 23 +++++\n diff.h | 4 +\n t/t4013-diff-various.sh | 41 +++++++++\n t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master | 99 ++++++++++++++++++++++\n xdiff/xdiff.h | 4 +\n xdiff/xdiffi.c | 47 +++++++++-\n 7 files changed, 221 insertions(+), 2 deletions(-)\n create mode 100644 t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 573fb9bb71..ee52b65e46 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -687,6 +687,11 @@ endif::git-format-patch[]\n --ignore-blank-lines::\n \tIgnore changes whose lines are all blank.\n \n+-I::\n+--ignore-matching-lines=::\n+\tIgnore changes whose all lines match . This option may\n+\tbe specified more than once.\n+\n --inter-hunk-context=::\n \tShow the context between diff hunks, up to the specified number\n \tof lines, thereby fusing hunks that are close to each other.\ndiff --git a/diff.c b/diff.c\nindex 2bb2f8f57e..d24f47df99 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3587,6 +3587,8 @@ static void builtin_diff(const char *name_a,\n \t\tif (header.len && !o->flags.suppress_diff_headers)\n \t\t\tecbdata.header = &header;\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -3716,6 +3718,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n+\t\txpp.ignore_regex = o->ignore_regex;\n+\t\txpp.ignore_regex_nr = o->ignore_regex_nr;\n \t\txpp.anchors = o->anchors;\n \t\txpp.anchors_nr = o->anchors_nr;\n \t\txecfg.ctxlen = o->context;\n@@ -5203,6 +5207,22 @@ static int diff_opt_patience(const struct option *opt,\n \treturn 0;\n }\n \n+static int diff_opt_ignore_regex(const struct option *opt,\n+\t\t\t\t const char *arg, int unset)\n+{\n+\tstruct diff_options *options = opt->value;\n+\tregex_t *regex;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tregex = xmalloc(sizeof(*regex));\n+\tif (regcomp(regex, arg, REG_EXTENDED | REG_NEWLINE))\n+\t\treturn error(_(\"invalid regex given to -I: '%s'\"), arg);\n+\tALLOC_GROW(options->ignore_regex, options->ignore_regex_nr + 1,\n+\t\t options->ignore_regex_alloc);\n+\toptions->ignore_regex[options->ignore_regex_nr++] = regex;\n+\treturn 0;\n+}\n+\n static int diff_opt_pickaxe_regex(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -5491,6 +5511,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_BIT_F(0, \"ignore-blank-lines\", &options->xdl_opts,\n \t\t\t N_(\"ignore changes whose lines are all blank\"),\n \t\t\t XDF_IGNORE_BLANK_LINES, PARSE_OPT_NONEG),\n+\t\tOPT_CALLBACK_F('I', \"ignore-matching-lines\", options, N_(\"\"),\n+\t\t\t N_(\"ignore changes whose all lines match \"),\n+\t\t\t 0, diff_opt_ignore_regex),\n \t\tOPT_BIT(0, \"indent-heuristic\", &options->xdl_opts,\n \t\t\tN_(\"heuristic to shift diff hunk boundaries for easy reading\"),\n \t\t\tXDF_INDENT_HEURISTIC),\ndiff --git a/diff.h b/diff.h\nindex 11de52e9e9..a402227b80 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -234,6 +234,10 @@ struct diff_options {\n \t */\n \tconst char *pickaxe;\n \n+\t/* -I */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr, ignore_regex_alloc;\n+\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tconst char *line_prefix;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5c7b0122b4..f72d456d3b 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -6,6 +6,7 @@\n test_description='Various diff formatting options'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n \n test_expect_success setup '\n \n@@ -333,6 +334,7 @@ log -SF master --max-count=2\n log -GF master\n log -GF -p master\n log -GF -p --pickaxe-all master\n+log -IA -IB -I1 -I2 -p master\n log --decorate --all\n log --decorate=full --all\n \n@@ -473,4 +475,43 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'diff -I: setup' '\n+\tgit checkout master &&\n+\ttest_seq 50 >file0 &&\n+\tgit commit -m \"Set up -I test file\" file0 &&\n+\ttest_seq 50 | sed -e \"s/13/ten and three/\" -e \"/7\\$/d\" >file0 &&\n+\techo >>file0\n+'\n+test_expect_success 'diff -I' '\n+\tgit diff --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/file0 b/file0\n+\t--- a/file0\n+\t+++ b/file0\n+\t@@ -34,7 +31,6 @@\n+\t 34\n+\t 35\n+\t 36\n+\t-37\n+\t 38\n+\t 39\n+\t 40\n+\tEOF\n+\tcompare_diff_patch expect actual\n+'\n+\n+test_expect_success 'diff -I --stat' '\n+\tgit diff --stat --ignore-blank-lines -I\"ten.*e\" -I\"^[124-9]\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t file0 | 1 -\n+\t 1 file changed, 1 deletion(-)\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff -I: detect malformed regex' '\n+\ttest_expect_code 129 git diff --ignore-matching-lines=\"^[124-9\" 2>error &&\n+\ttest_i18ngrep \"invalid regex given to -I: \" error\n+'\n+\n test_done\ndiff --git a/t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master b/t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master\nnew file mode 100644\nindex 0000000000..929f35a05b\n--- /dev/null\n+++ b/t/t4013/diff.log_-IA_-IB_-I1_-I2_-p_master\n@@ -0,0 +1,99 @@\n+$ git log -IA -IB -I1 -I2 -p master\n+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64\n+Merge: 9a6d494 c7a2ab9\n+Author: A U Thor \n+Date: Mon Jun 26 00:04:00 2006 +0000\n+\n+ Merge branch 'side'\n+\n+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a\n+Author: A U Thor \n+Date: Mon Jun 26 00:03:00 2006 +0000\n+\n+ Side\n+\n+diff --git a/file0 b/file0\n+index 01e79c3..f4615da 100644\n+--- a/file0\n++++ b/file0\n+@@ -1,3 +1,6 @@\n+ 1\n+ 2\n+ 3\n++A\n++B\n++C\n+diff --git a/file3 b/file3\n+new file mode 100644\n+index 0000000..7289e35\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \n+Date: Mon Jun 26 00:02:00 2006 +0000\n+\n+ Third\n+\n+diff --git a/dir/sub b/dir/sub\n+index 8422d40..cead32e 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -2,3 +2,5 @@ A\n+ B\n+ C\n+ D\n++E\n++F\n+diff --git a/file1 b/file1\n+new file mode 100644\n+index 0000000..b1e6722\n+--- /dev/null\n++++ b/file1\n+@@ -0,0 +1,3 @@\n++A\n++B\n++C\n+\n+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44\n+Author: A U Thor \n+Date: Mon Jun 26 00:01:00 2006 +0000\n+\n+ Second\n+ \n+ This is the second commit.\n+\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..8422d40 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++C\n++D\n+diff --git a/file0 b/file0\n+index 01e79c3..b414108 100644\n+--- a/file0\n++++ b/file0\n+@@ -1,3 +1,6 @@\n+ 1\n+ 2\n+ 3\n++4\n++5\n++6\n+diff --git a/file2 b/file2\n+deleted file mode 100644\n+index 01e79c3..0000000\n+--- a/file2\n++++ /dev/null\n+@@ -1,3 +0,0 @@\n+-1\n+-2\n+-3\n+\n+commit 444ac553ac7612cc88969031b02b3767fb8a353a\n+Author: A U Thor \n+Date: Mon Jun 26 00:00:00 2006 +0000\n+\n+ Initial\n+$\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 032e3a9f41..7a04605146 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -79,6 +79,10 @@ typedef struct s_mmbuffer {\n typedef struct s_xpparam {\n \tunsigned long flags;\n \n+\t/* -I */\n+\tregex_t **ignore_regex;\n+\tsize_t ignore_regex_nr;\n+\n \t/* See Documentation/diff-options.txt. */\n \tchar **anchors;\n \tsize_t anchors_nr;\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex bd035139f9..380eb728ed 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -998,7 +998,7 @@ static int xdl_call_hunk_func(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \treturn 0;\n }\n \n-static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n+static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)\n {\n \txdchange_t *xch;\n \n@@ -1019,6 +1019,46 @@ static void xdl_mark_ignorable(xdchange_t *xscr, xdfenv_t *xe, long flags)\n \t}\n }\n \n+static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {\n+\tregmatch_t regmatch;\n+\tint i;\n+\n+\tfor (i = 0; i < xpp->ignore_regex_nr; i++)\n+\t\tif (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,\n+\t\t\t\t ®match, 0))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n+static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,\n+\t\t\t\t xpparam_t const *xpp)\n+{\n+\txdchange_t *xch;\n+\n+\tfor (xch = xscr; xch; xch = xch->next) {\n+\t\txrecord_t **rec;\n+\t\tint ignore = 1;\n+\t\tlong i;\n+\n+\t\t/*\n+\t\t * Do not override --ignore-blank-lines.\n+\t\t */\n+\t\tif (xch->ignore)\n+\t\t\tcontinue;\n+\n+\t\trec = &xe->xdf1.recs[xch->i1];\n+\t\tfor (i = 0; i < xch->chg1 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\trec = &xe->xdf2.recs[xch->i2];\n+\t\tfor (i = 0; i < xch->chg2 && ignore; i++)\n+\t\t\tignore = record_matches_regex(rec[i], xpp);\n+\n+\t\txch->ignore = ignore;\n+\t}\n+}\n+\n int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t xdemitconf_t const *xecfg, xdemitcb_t *ecb) {\n \txdchange_t *xscr;\n@@ -1038,7 +1078,10 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t}\n \tif (xscr) {\n \t\tif (xpp->flags & XDF_IGNORE_BLANK_LINES)\n-\t\t\txdl_mark_ignorable(xscr, &xe, xpp->flags);\n+\t\t\txdl_mark_ignorable_lines(xscr, &xe, xpp->flags);\n+\n+\t\tif (xpp->ignore_regex)\n+\t\t\txdl_mark_ignorable_regex(xscr, &xe, xpp);\n \n \t\tif (ef(&xe, xscr, ecb, xecfg) < 0) {\n \n-- \n2.28.0\n\n"},{"id":"416319","messageId":"20210205141320.18076-1-avarab@gmail.com","threadId":"54333","inReplyTo":"20201020064809.14297-1-michal@isc.org","subject":"[PATCH 1/2] diff: add an API for deferred freeing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-05T14:13:19Z","receivedAt":"2021-02-05T22:24:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a diff_free() function to free anything we may have allocated in\nthe \"diff_options\" struct, and the ability to make calling it a noop\nby setting \"no_free\" in \"diff_options\".\n\nThis is required because when e.g. \"git diff\" is run we'll allocate\nthings in that struct, use the diff machinery once, and then exit, but\nif we run e.g. \"git log -p\" we're going to re-use what we allocated\nacross multiple diff_flush() calls, and only want to free things at\nthe end.\n\nWe've thus ended up with features like the recently added \"diff -I\"[1]\nwhere we'll leak memory. As it turns out it could have simply used the\npattern established in 6ea57703f6 (log: prepare log/log-tree to reuse\nthe diffopt.close_file attribute, 2016-06-22).\n\nManually adding more such flags to things log_tree_commit() every time\nwe need to allocate something would be tedious.\n\nLet's instead move that fclose() code it to a new diff_free(), in\nanticipation of freeing more things in that function in follow-up\ncommits. I'm renaming the \"close_file\" struct member to \"fclose_file\"\nfor the ease of validating this, we can be certain that these are all\nthe relevant callsites.\n\nSome functions such as log_tree_commit() need an idiom of optionally\nretaining a previous \"no_free\", as they may either free the memory\nthemselves, or their caller may do so. I'm keeping that idiom in\nlog_show_early() even though I don't think it's currently called in\nthis manner, since it also gets passed an existing \"struct rev_info\"..\n\n1. 296d4a94e7 (diff: add -I that ignores matching changes,\n 2020-10-20)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/add.c | 2 +-\n builtin/am.c | 6 ++++--\n builtin/log.c | 27 ++++++++++++++-------------\n diff.c | 18 +++++++++++++-----\n diff.h | 14 +++++++++++++-\n log-tree.c | 10 ++++++----\n wt-status.c | 2 +-\n 7 files changed, 52 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex a825887c50..6319710186 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -282,7 +282,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tif (out < 0)\n \t\tdie(_(\"Could not open '%s' for writing.\"), file);\n \trev.diffopt.file = xfdopen(out, \"w\");\n-\trev.diffopt.close_file = 1;\n+\trev.diffopt.fclose_file = 1;\n \tif (run_diff_files(&rev, 0))\n \t\tdie(_(\"Could not write patch\"));\n \ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8355e3566f..157d264583 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1315,7 +1315,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trev_info.diffopt.flags.full_index = 1;\n \trev_info.diffopt.use_color = 0;\n \trev_info.diffopt.file = fp;\n-\trev_info.diffopt.close_file = 1;\n+\trev_info.diffopt.fclose_file = 1; /* log_tree_commit() sets .no_free=1 */\n \tadd_pending_object(&rev_info, &commit->object, \"\");\n \tdiff_setup_done(&rev_info.diffopt);\n \tlog_tree_commit(&rev_info, commit);\n@@ -1347,10 +1347,12 @@ static void write_index_patch(const struct am_state *state)\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\n \trev_info.diffopt.file = fp;\n-\trev_info.diffopt.close_file = 1;\n+\trev_info.diffopt.fclose_file = 1;\n+\trev_info.diffopt.no_free = 1;\n \tadd_pending_object(&rev_info, &tree->object, \"\");\n \tdiff_setup_done(&rev_info.diffopt);\n \trun_diff_index(&rev_info, 1);\n+\tdiff_free(&rev_info.diffopt);\n }\n \n /**\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fd282def43..604ee29ec0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -306,10 +306,11 @@ static struct itimerval early_output_timer;\n \n static void log_show_early(struct rev_info *revs, struct commit_list *list)\n {\n-\tint i = revs->early_output, close_file = revs->diffopt.close_file;\n+\tint i = revs->early_output;\n \tint show_header = 1;\n+\tint no_free = revs->diffopt.no_free;\n \n-\trevs->diffopt.close_file = 0;\n+\trevs->diffopt.no_free = 0;\n \tsort_in_topological_order(&list, revs->sort_order);\n \twhile (list && i) {\n \t\tstruct commit *commit = list->item;\n@@ -326,8 +327,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \t\tcase commit_ignore:\n \t\t\tbreak;\n \t\tcase commit_error:\n-\t\t\tif (close_file)\n-\t\t\t\tfclose(revs->diffopt.file);\n+\t\t\trevs->diffopt.no_free = no_free;\n+\t\t\tdiff_free(&revs->diffopt);\n \t\t\treturn;\n \t\t}\n \t\tlist = list->next;\n@@ -335,8 +336,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \n \t/* Did we already get enough commits for the early output? */\n \tif (!i) {\n-\t\tif (close_file)\n-\t\t\tfclose(revs->diffopt.file);\n+\t\trevs->diffopt.no_free = 0;\n+\t\tdiff_free(&revs->diffopt);\n \t\treturn;\n \t}\n \n@@ -400,7 +401,7 @@ static int cmd_log_walk(struct rev_info *rev)\n {\n \tstruct commit *commit;\n \tint saved_nrl = 0;\n-\tint saved_dcctc = 0, close_file = rev->diffopt.close_file;\n+\tint saved_dcctc = 0;\n \n \tif (rev->early_output)\n \t\tsetup_early_output();\n@@ -416,7 +417,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n-\trev->diffopt.close_file = 0;\n+\trev->diffopt.no_free = 1;\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tif (!log_tree_commit(rev, commit) && rev->max_count >= 0)\n \t\t\t/*\n@@ -441,8 +442,8 @@ static int cmd_log_walk(struct rev_info *rev)\n \t}\n \trev->diffopt.degraded_cc_to_c = saved_dcctc;\n \trev->diffopt.needed_rename_limit = saved_nrl;\n-\tif (close_file)\n-\t\tfclose(rev->diffopt.file);\n+\trev->diffopt.no_free = 0;\n+\tdiff_free(&rev->diffopt);\n \n \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n \t rev->diffopt.flags.check_failed) {\n@@ -1952,18 +1953,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.show_notes)\n \t\tload_display_notes(&rev.notes_opt);\n \n-\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n+\tif (use_stdout + rev.diffopt.fclose_file + !!output_directory > 1)\n \t\tdie(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n \n \tif (use_stdout) {\n \t\tsetup_pager();\n-\t} else if (rev.diffopt.close_file) {\n+\t} else if (rev.diffopt.fclose_file) {\n \t\t/*\n \t\t * The diff code parsed --output; it has already opened the\n \t\t * file, but but we must instruct it not to close after each\n \t\t * diff.\n \t\t */\n-\t\trev.diffopt.close_file = 0;\n+\t\trev.diffopt.no_free = 1;\n \t} else {\n \t\tint saved;\n \ndiff --git a/diff.c b/diff.c\nindex 69e3bc00ed..3e6f8f0a71 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5187,7 +5187,7 @@ static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,\n \tBUG_ON_OPT_NEG(unset);\n \tpath = prefix_filename(ctx->prefix, arg);\n \toptions->file = xfopen(path, \"w\");\n-\toptions->close_file = 1;\n+\toptions->fclose_file = 1;\n \tif (options->use_color != GIT_COLOR_ALWAYS)\n \t\toptions->use_color = GIT_COLOR_NEVER;\n \tfree(path);\n@@ -6399,10 +6399,10 @@ void diff_flush(struct diff_options *options)\n \t\t * options->file to /dev/null should be safe, because we\n \t\t * aren't supposed to produce any output anyway.\n \t\t */\n-\t\tif (options->close_file)\n+\t\tif (options->fclose_file)\n \t\t\tfclose(options->file);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n-\t\toptions->close_file = 1;\n+\t\toptions->fclose_file = 1;\n \t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n@@ -6433,8 +6433,7 @@ void diff_flush(struct diff_options *options)\n free_queue:\n \tfree(q->queue);\n \tDIFF_QUEUE_CLEAR(q);\n-\tif (options->close_file)\n-\t\tfclose(options->file);\n+\tdiff_free(options);\n \n \t/*\n \t * Report the content-level differences with HAS_CHANGES;\n@@ -6449,6 +6448,15 @@ void diff_flush(struct diff_options *options)\n \t}\n }\n \n+void diff_free(struct diff_options *options)\n+{\n+\tif (options->no_free)\n+\t\treturn;\n+\tif (options->fclose_file)\n+\t\tfclose(options->file);\n+}\n+\t\n+\n static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n {\n \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\ndiff --git a/diff.h b/diff.h\nindex 2ff2b1c7f2..d1d74c3a9e 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -49,7 +49,16 @@\n * - Once you finish feeding the pairs of files, call `diffcore_std()`.\n * This will tell the diffcore library to go ahead and do its work.\n *\n+ * - The `diff_opt_parse()` etc. functions might allocate memory in\n+ * `struct diff_options`. When running the API `N > 1` set `.no_free\n+ * = 1` to make the `diff_free()` invoked by `diff_flush()` below a\n+ * noop.\n+ *\n * - Calling `diff_flush()` will produce the output.\n+ *\n+ * - If you set `.no_free = 1` before set it to `0` and call\n+ * `diff_free()` again. If `.no_free = 1` was not set there's no\n+ * need to call `diff_free()`, `diff_flush()` will call it.\n */\n \n struct combine_diff_path;\n@@ -328,7 +337,7 @@ struct diff_options {\n \tvoid (*set_default)(struct diff_options *);\n \n \tFILE *file;\n-\tint close_file;\n+\tint fclose_file;\n \n #define OUTPUT_INDICATOR_NEW 0\n #define OUTPUT_INDICATOR_OLD 1\n@@ -365,6 +374,8 @@ struct diff_options {\n \n \tstruct repository *repo;\n \tstruct option *parseopts;\n+\n+\tint no_free;\n };\n \n unsigned diff_filter_bit(char status);\n@@ -559,6 +570,7 @@ void diffcore_fix_diff_index(void);\n \n int diff_queue_is_empty(void);\n void diff_flush(struct diff_options*);\n+void diff_free(struct diff_options*);\n void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n \n /* diff-raw status letters */\ndiff --git a/log-tree.c b/log-tree.c\nindex fd0dde97ec..bb946f15f1 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -952,12 +952,14 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n int log_tree_commit(struct rev_info *opt, struct commit *commit)\n {\n \tstruct log_info log;\n-\tint shown, close_file = opt->diffopt.close_file;\n+\tint shown;\n+\t/* maybe called by e.g. cmd_log_walk(), maybe stand-alone */\n+\tint no_free = opt->diffopt.no_free;\n \n \tlog.commit = commit;\n \tlog.parent = NULL;\n \topt->loginfo = &log;\n-\topt->diffopt.close_file = 0;\n+\topt->diffopt.no_free = 1;\n \n \tif (opt->line_level_traverse)\n \t\treturn line_log_print(opt, commit);\n@@ -974,7 +976,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tfprintf(opt->diffopt.file, \"\\n%s\\n\", opt->break_bar);\n \topt->loginfo = NULL;\n \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n-\tif (close_file)\n-\t\tfclose(opt->diffopt.file);\n+\topt->diffopt.no_free = no_free;\n+\tdiff_free(&opt->diffopt);\n \treturn shown;\n }\ndiff --git a/wt-status.c b/wt-status.c\nindex 0c8287a023..41b08474e5 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1052,7 +1052,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \trev.diffopt.file = s->fp;\n-\trev.diffopt.close_file = 0;\n+\trev.diffopt.fclose_file = 0; /* wt_status owns the s->fp */\n \t/*\n \t * If we're not going to stdout, then we definitely don't\n \t * want color, since we are going to the commit message\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416320","messageId":"20210205141320.18076-2-avarab@gmail.com","threadId":"54333","inReplyTo":"20201020064809.14297-1-michal@isc.org","subject":"[PATCH 2/2] diff: plug memory leak from regcomp() on {log,diff} -I","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-05T14:13:20Z","receivedAt":"2021-02-05T22:28:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in 296d4a94e7 (diff: add -I that ignores\nmatching changes, 2020-10-20) by freeing the memory it allocates in\nthe newly introduced diff_free(). See the previous commit for details\non that.\n\nThis memory leak was intentionally introduced in 296d4a94e7, see the\ndiscussion on a previous iteration of it in\nhttps://lore.kernel.org/git/xmqqeelycajx.fsf@gitster.c.googlers.com/\n\nAt that time freeing the memory was somewhat tedious, but since it\nisn't anymore with the newly introduced diff_free() let's use it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 3e6f8f0a71..24257759bb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6450,10 +6450,17 @@ void diff_flush(struct diff_options *options)\n \n void diff_free(struct diff_options *options)\n {\n+\tint i;\n \tif (options->no_free)\n \t\treturn;\n \tif (options->fclose_file)\n \t\tfclose(options->file);\n+\n+\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n+\t\tregfree(options->ignore_regex[i]);\n+\t\tfree(options->ignore_regex[i]);\n+\t}\n+\tfree(options->ignore_regex);\n }\n \t\n \n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416637","messageId":"nycvar.QRO.7.76.6.2102101557160.29765@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"20210205141320.18076-1-avarab@gmail.com","subject":"Re: [PATCH 1/2] diff: add an API for deferred freeing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-10T16:00:45Z","receivedAt":"2021-02-10T16:02:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Fri, 5 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> Add a diff_free() function to free anything we may have allocated in\n> the \"diff_options\" struct, and the ability to make calling it a noop\n> by setting \"no_free\" in \"diff_options\".\n\nWhy do we need a `no_free` flag? Why not simply set the `free()`d (or\n`fclose()`d) attributes to `NULL`?\n\n> This is required because when e.g. \"git diff\" is run we'll allocate\n> things in that struct, use the diff machinery once, and then exit, but\n> if we run e.g. \"git log -p\" we're going to re-use what we allocated\n> across multiple diff_flush() calls, and only want to free things at\n> the end.\n>\n> We've thus ended up with features like the recently added \"diff -I\"[1]\n> where we'll leak memory. As it turns out it could have simply used the\n> pattern established in 6ea57703f6 (log: prepare log/log-tree to reuse\n> the diffopt.close_file attribute, 2016-06-22).\n>\n> Manually adding more such flags to things log_tree_commit() every time\n> we need to allocate something would be tedious.\n>\n> Let's instead move that fclose() code it to a new diff_free(), in\n> anticipation of freeing more things in that function in follow-up\n> commits. I'm renaming the \"close_file\" struct member to \"fclose_file\"\n> for the ease of validating this, we can be certain that these are all\n> the relevant callsites.\n\nI like that idea a lot.\n\n> Some functions such as log_tree_commit() need an idiom of optionally\n> retaining a previous \"no_free\", as they may either free the memory\n> themselves, or their caller may do so. I'm keeping that idiom in\n> log_show_early() even though I don't think it's currently called in\n> this manner, since it also gets passed an existing \"struct rev_info\"..\n>\n> 1. 296d4a94e7 (diff: add -I that ignores matching changes,\n> 2020-10-20)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/add.c | 2 +-\n> builtin/am.c | 6 ++++--\n> builtin/log.c | 27 ++++++++++++++-------------\n> diff.c | 18 +++++++++++++-----\n> diff.h | 14 +++++++++++++-\n> log-tree.c | 10 ++++++----\n> wt-status.c | 2 +-\n> 7 files changed, 52 insertions(+), 27 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index a825887c50..6319710186 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -282,7 +282,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n> \tif (out < 0)\n> \t\tdie(_(\"Could not open '%s' for writing.\"), file);\n> \trev.diffopt.file = xfdopen(out, \"w\");\n> -\trev.diffopt.close_file = 1;\n> +\trev.diffopt.fclose_file = 1;\n\nThis rename makes the patch unnecessarily tedious to review, and I do not\neven agree with leaking the implementation detail that we need to\n`fclose()` the file.\n\nLet's just not?\n\n> \tif (run_diff_files(&rev, 0))\n> \t\tdie(_(\"Could not write patch\"));\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 8355e3566f..157d264583 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1315,7 +1315,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n> \trev_info.diffopt.flags.full_index = 1;\n> \trev_info.diffopt.use_color = 0;\n> \trev_info.diffopt.file = fp;\n> -\trev_info.diffopt.close_file = 1;\n> +\trev_info.diffopt.fclose_file = 1; /* log_tree_commit() sets .no_free=1 */\n> \tadd_pending_object(&rev_info, &commit->object, \"\");\n> \tdiff_setup_done(&rev_info.diffopt);\n> \tlog_tree_commit(&rev_info, commit);\n> @@ -1347,10 +1347,12 @@ static void write_index_patch(const struct am_state *state)\n> \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n> \trev_info.diffopt.use_color = 0;\n> \trev_info.diffopt.file = fp;\n> -\trev_info.diffopt.close_file = 1;\n> +\trev_info.diffopt.fclose_file = 1;\n> +\trev_info.diffopt.no_free = 1;\n> \tadd_pending_object(&rev_info, &tree->object, \"\");\n> \tdiff_setup_done(&rev_info.diffopt);\n> \trun_diff_index(&rev_info, 1);\n> +\tdiff_free(&rev_info.diffopt);\n> }\n>\n> /**\n> diff --git a/builtin/log.c b/builtin/log.c\n> index fd282def43..604ee29ec0 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -306,10 +306,11 @@ static struct itimerval early_output_timer;\n>\n> static void log_show_early(struct rev_info *revs, struct commit_list *list)\n> {\n> -\tint i = revs->early_output, close_file = revs->diffopt.close_file;\n> +\tint i = revs->early_output;\n> \tint show_header = 1;\n> +\tint no_free = revs->diffopt.no_free;\n>\n> -\trevs->diffopt.close_file = 0;\n> +\trevs->diffopt.no_free = 0;\n> \tsort_in_topological_order(&list, revs->sort_order);\n> \twhile (list && i) {\n> \t\tstruct commit *commit = list->item;\n> @@ -326,8 +327,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n> \t\tcase commit_ignore:\n> \t\t\tbreak;\n> \t\tcase commit_error:\n> -\t\t\tif (close_file)\n> -\t\t\t\tfclose(revs->diffopt.file);\n> +\t\t\trevs->diffopt.no_free = no_free;\n> +\t\t\tdiff_free(&revs->diffopt);\n> \t\t\treturn;\n> \t\t}\n> \t\tlist = list->next;\n> @@ -335,8 +336,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n>\n> \t/* Did we already get enough commits for the early output? */\n> \tif (!i) {\n> -\t\tif (close_file)\n> -\t\t\tfclose(revs->diffopt.file);\n> +\t\trevs->diffopt.no_free = 0;\n> +\t\tdiff_free(&revs->diffopt);\n> \t\treturn;\n> \t}\n>\n> @@ -400,7 +401,7 @@ static int cmd_log_walk(struct rev_info *rev)\n> {\n> \tstruct commit *commit;\n> \tint saved_nrl = 0;\n> -\tint saved_dcctc = 0, close_file = rev->diffopt.close_file;\n> +\tint saved_dcctc = 0;\n>\n> \tif (rev->early_output)\n> \t\tsetup_early_output();\n> @@ -416,7 +417,7 @@ static int cmd_log_walk(struct rev_info *rev)\n> \t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n> \t * retain that state information if replacing rev->diffopt in this loop\n> \t */\n> -\trev->diffopt.close_file = 0;\n> +\trev->diffopt.no_free = 1;\n> \twhile ((commit = get_revision(rev)) != NULL) {\n> \t\tif (!log_tree_commit(rev, commit) && rev->max_count >= 0)\n> \t\t\t/*\n> @@ -441,8 +442,8 @@ static int cmd_log_walk(struct rev_info *rev)\n> \t}\n> \trev->diffopt.degraded_cc_to_c = saved_dcctc;\n> \trev->diffopt.needed_rename_limit = saved_nrl;\n> -\tif (close_file)\n> -\t\tfclose(rev->diffopt.file);\n> +\trev->diffopt.no_free = 0;\n> +\tdiff_free(&rev->diffopt);\n>\n> \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n> \t rev->diffopt.flags.check_failed) {\n> @@ -1952,18 +1953,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tif (rev.show_notes)\n> \t\tload_display_notes(&rev.notes_opt);\n>\n> -\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n> +\tif (use_stdout + rev.diffopt.fclose_file + !!output_directory > 1)\n> \t\tdie(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n>\n> \tif (use_stdout) {\n> \t\tsetup_pager();\n> -\t} else if (rev.diffopt.close_file) {\n> +\t} else if (rev.diffopt.fclose_file) {\n> \t\t/*\n> \t\t * The diff code parsed --output; it has already opened the\n> \t\t * file, but but we must instruct it not to close after each\n> \t\t * diff.\n> \t\t */\n> -\t\trev.diffopt.close_file = 0;\n> +\t\trev.diffopt.no_free = 1;\n> \t} else {\n> \t\tint saved;\n>\n> diff --git a/diff.c b/diff.c\n> index 69e3bc00ed..3e6f8f0a71 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5187,7 +5187,7 @@ static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,\n> \tBUG_ON_OPT_NEG(unset);\n> \tpath = prefix_filename(ctx->prefix, arg);\n> \toptions->file = xfopen(path, \"w\");\n> -\toptions->close_file = 1;\n> +\toptions->fclose_file = 1;\n> \tif (options->use_color != GIT_COLOR_ALWAYS)\n> \t\toptions->use_color = GIT_COLOR_NEVER;\n> \tfree(path);\n> @@ -6399,10 +6399,10 @@ void diff_flush(struct diff_options *options)\n> \t\t * options->file to /dev/null should be safe, because we\n> \t\t * aren't supposed to produce any output anyway.\n> \t\t */\n> -\t\tif (options->close_file)\n> +\t\tif (options->fclose_file)\n> \t\t\tfclose(options->file);\n\nAnd at this stage, we should set `options->file = NULL`.\n\n> \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n> -\t\toptions->close_file = 1;\n> +\t\toptions->fclose_file = 1;\n> \t\toptions->color_moved = 0;\n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> @@ -6433,8 +6433,7 @@ void diff_flush(struct diff_options *options)\n> free_queue:\n> \tfree(q->queue);\n> \tDIFF_QUEUE_CLEAR(q);\n> -\tif (options->close_file)\n> -\t\tfclose(options->file);\n> +\tdiff_free(options);\n>\n> \t/*\n> \t * Report the content-level differences with HAS_CHANGES;\n> @@ -6449,6 +6448,15 @@ void diff_flush(struct diff_options *options)\n> \t}\n> }\n>\n> +void diff_free(struct diff_options *options)\n> +{\n> +\tif (options->no_free)\n> +\t\treturn;\n> +\tif (options->fclose_file)\n> +\t\tfclose(options->file);\n\nAnd at this stage, we should set `options->file = NULL`.\n\n> +}\n> +\n> +\n> static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n> {\n> \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n> diff --git a/diff.h b/diff.h\n> index 2ff2b1c7f2..d1d74c3a9e 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -49,7 +49,16 @@\n> * - Once you finish feeding the pairs of files, call `diffcore_std()`.\n> * This will tell the diffcore library to go ahead and do its work.\n> *\n> + * - The `diff_opt_parse()` etc. functions might allocate memory in\n> + * `struct diff_options`. When running the API `N > 1` set `.no_free\n> + * = 1` to make the `diff_free()` invoked by `diff_flush()` below a\n> + * noop.\n\nI have serious trouble parsing that last sentence. Would you mind\nrephrasing it?\n\n> + *\n> * - Calling `diff_flush()` will produce the output.\n> + *\n> + * - If you set `.no_free = 1` before set it to `0` and call\n> + * `diff_free()` again. If `.no_free = 1` was not set there's no\n> + * need to call `diff_free()`, `diff_flush()` will call it.\n\nAgain, as I mentioned before, the indicator whether things need to be\nreleased should be whether the attribute is `NULL` or not. And once\nreleased, ot should be set to `NULL`.\n\nOther than that, it looks fine to me. And I definitely like the idea of\nintroducing a function to release all of the stuff in `struct diffopt`.\n\nThanks,\nDscho\n\n> */\n>\n> struct combine_diff_path;\n> @@ -328,7 +337,7 @@ struct diff_options {\n> \tvoid (*set_default)(struct diff_options *);\n>\n> \tFILE *file;\n> -\tint close_file;\n> +\tint fclose_file;\n>\n> #define OUTPUT_INDICATOR_NEW 0\n> #define OUTPUT_INDICATOR_OLD 1\n> @@ -365,6 +374,8 @@ struct diff_options {\n>\n> \tstruct repository *repo;\n> \tstruct option *parseopts;\n> +\n> +\tint no_free;\n> };\n>\n> unsigned diff_filter_bit(char status);\n> @@ -559,6 +570,7 @@ void diffcore_fix_diff_index(void);\n>\n> int diff_queue_is_empty(void);\n> void diff_flush(struct diff_options*);\n> +void diff_free(struct diff_options*);\n> void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n>\n> /* diff-raw status letters */\n> diff --git a/log-tree.c b/log-tree.c\n> index fd0dde97ec..bb946f15f1 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -952,12 +952,14 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n> int log_tree_commit(struct rev_info *opt, struct commit *commit)\n> {\n> \tstruct log_info log;\n> -\tint shown, close_file = opt->diffopt.close_file;\n> +\tint shown;\n> +\t/* maybe called by e.g. cmd_log_walk(), maybe stand-alone */\n> +\tint no_free = opt->diffopt.no_free;\n>\n> \tlog.commit = commit;\n> \tlog.parent = NULL;\n> \topt->loginfo = &log;\n> -\topt->diffopt.close_file = 0;\n> +\topt->diffopt.no_free = 1;\n>\n> \tif (opt->line_level_traverse)\n> \t\treturn line_log_print(opt, commit);\n> @@ -974,7 +976,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n> \t\tfprintf(opt->diffopt.file, \"\\n%s\\n\", opt->break_bar);\n> \topt->loginfo = NULL;\n> \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n> -\tif (close_file)\n> -\t\tfclose(opt->diffopt.file);\n> +\topt->diffopt.no_free = no_free;\n> +\tdiff_free(&opt->diffopt);\n> \treturn shown;\n> }\n> diff --git a/wt-status.c b/wt-status.c\n> index 0c8287a023..41b08474e5 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1052,7 +1052,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n> \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n> \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n> \trev.diffopt.file = s->fp;\n> -\trev.diffopt.close_file = 0;\n> +\trev.diffopt.fclose_file = 0; /* wt_status owns the s->fp */\n> \t/*\n> \t * If we're not going to stdout, then we definitely don't\n> \t * want color, since we are going to the commit message\n> --\n> 2.30.0.284.gd98b1dd5eaa7\n>\n>\n"},{"id":"416708","messageId":"878s7vcnqo.fsf@evledraar.gmail.com","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2102101557160.29765@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] diff: add an API for deferred freeing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-11T03:00:31Z","receivedAt":"2021-02-11T03:01:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 10 2021, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Fri, 5 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Add a diff_free() function to free anything we may have allocated in\n>> the \"diff_options\" struct, and the ability to make calling it a noop\n>> by setting \"no_free\" in \"diff_options\".\n>\n> Why do we need a `no_free` flag? Why not simply set the `free()`d (or\n> `fclose()`d) attributes to `NULL`?\n\nBecause we're calling the diff API N times, so we need to not have those\nbe NULL while we're using them in a loop, and we need to not free()\nthem, and then flip \"no_free = 0\" at the end and free() them.\n\n>> diff --git a/builtin/add.c b/builtin/add.c\n>> index a825887c50..6319710186 100644\n>> --- a/builtin/add.c\n>> +++ b/builtin/add.c\n>> @@ -282,7 +282,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n>> \tif (out < 0)\n>> \t\tdie(_(\"Could not open '%s' for writing.\"), file);\n>> \trev.diffopt.file = xfdopen(out, \"w\");\n>> -\trev.diffopt.close_file = 1;\n>> +\trev.diffopt.fclose_file = 1;\n>\n> This rename makes the patch unnecessarily tedious to review, and I do not\n> even agree with leaking the implementation detail that we need to\n> `fclose()` the file.\n>\n> Let's just not?\n\nFair enough, I figured it would be easier for reviewers to reason about\nto be guaranteed to see all the uses of the flag, but I'll drop it.\n\n>> @@ -6399,10 +6399,10 @@ void diff_flush(struct diff_options *options)\n>> \t\t * options->file to /dev/null should be safe, because we\n>> \t\t * aren't supposed to produce any output anyway.\n>> \t\t */\n>> -\t\tif (options->close_file)\n>> +\t\tif (options->fclose_file)\n>> \t\t\tfclose(options->file);\n>\n> And at this stage, we should set `options->file = NULL`.\n\nSure, why not, but unless we get rid of the need for \"close_file\"\nthere's not much point other than ease of inspection in a\ndebugger...[cont].\n\n>> [...]\n>> +void diff_free(struct diff_options *options)\n>> +{\n>> +\tif (options->no_free)\n>> +\t\treturn;\n>> +\tif (options->fclose_file)\n>> +\t\tfclose(options->file);\n>\n> And at this stage, we should set `options->file = NULL`.\n\n...ditto...[cont]\n\n>> +}\n>> +\n>> +\n>> static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n>> {\n>> \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n>> diff --git a/diff.h b/diff.h\n>> index 2ff2b1c7f2..d1d74c3a9e 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -49,7 +49,16 @@\n>> * - Once you finish feeding the pairs of files, call `diffcore_std()`.\n>> * This will tell the diffcore library to go ahead and do its work.\n>> *\n>> + * - The `diff_opt_parse()` etc. functions might allocate memory in\n>> + * `struct diff_options`. When running the API `N > 1` set `.no_free\n>> + * = 1` to make the `diff_free()` invoked by `diff_flush()` below a\n>> + * noop.\n>\n> I have serious trouble parsing that last sentence. Would you mind\n> rephrasing it?\n\nWilldo.\n\n>> + *\n>> * - Calling `diff_flush()` will produce the output.\n>> + *\n>> + * - If you set `.no_free = 1` before set it to `0` and call\n>> + * `diff_free()` again. If `.no_free = 1` was not set there's no\n>> + * need to call `diff_free()`, `diff_flush()` will call it.\n>\n> Again, as I mentioned before, the indicator whether things need to be\n> released should be whether the attribute is `NULL` or not. And once\n> released, ot should be set to `NULL`.\n>\n> Other than that, it looks fine to me. And I definitely like the idea of\n> introducing a function to release all of the stuff in `struct diffopt`.\n\n[cont]...I don't think it's possible in anything like the current API to\ndo that.\n\nWe don't want to \"if (file) fclose(file)\", instead we need an opt-in \"if\n(close_file) fclose(file)\". That's because usually the \"file\" is\nstdout. So close_file=1 is only set when we're opening a \"real\" file,\n/dev/null or whatever.\n\nThat along with the \"no_free\" flag as noted above means we need both a\n\"no_free\" and \"close_file\" flags.\n\nUnless there's some way of re-arranging this that I'm missing.\n"},{"id":"416735","messageId":"nycvar.QRO.7.76.6.2102111039080.29765@tvgsbejvaqbjf.bet","threadId":"54333","inReplyTo":"878s7vcnqo.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] diff: add an API for deferred freeing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-02-11T09:40:45Z","receivedAt":"2021-02-11T09:47:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 11 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Feb 10 2021, Johannes Schindelin wrote:\n>\n> > On Fri, 5 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> Add a diff_free() function to free anything we may have allocated in\n> >> the \"diff_options\" struct, and the ability to make calling it a noop\n> >> by setting \"no_free\" in \"diff_options\".\n> >\n> > Why do we need a `no_free` flag? Why not simply set the `free()`d (or\n> > `fclose()`d) attributes to `NULL`?\n\nHmm. That was not even clear to me until I read this reply.\n\nDoesn't this indicate that the closing is done at the wrong layer? If we\nwant to call a function N times and only at the last iteration should it\nclean up our resources, doesn't that indicate that the clean-up should be\npulled out from that function?\n\nI thought that's exactly what you did, but I must have glanced over\nsomething obvious...\n\nCiao,\nDscho\n"},{"id":"416736","messageId":"YCUFNVj7qlt9wzlX@coredump.intra.peff.net","threadId":"54333","inReplyTo":"nycvar.QRO.7.76.6.2102111039080.29765@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] diff: add an API for deferred freeing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-11T10:21:41Z","receivedAt":"2021-02-11T10:25:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 11, 2021 at 10:40:45AM +0100, Johannes Schindelin wrote:\n\n> On Thu, 11 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n> \n> > On Wed, Feb 10 2021, Johannes Schindelin wrote:\n> >\n> > > On Fri, 5 Feb 2021, Ævar Arnfjörð Bjarmason wrote:\n> > >\n> > >> Add a diff_free() function to free anything we may have allocated in\n> > >> the \"diff_options\" struct, and the ability to make calling it a noop\n> > >> by setting \"no_free\" in \"diff_options\".\n> > >\n> > > Why do we need a `no_free` flag? Why not simply set the `free()`d (or\n> > > `fclose()`d) attributes to `NULL`?\n> \n> Hmm. That was not even clear to me until I read this reply.\n> \n> Doesn't this indicate that the closing is done at the wrong layer? If we\n> want to call a function N times and only at the last iteration should it\n> clean up our resources, doesn't that indicate that the clean-up should be\n> pulled out from that function?\n> \n> I thought that's exactly what you did, but I must have glanced over\n> something obvious...\n\nI think the issue then is that every caller must now be modified to do\nthe clean up (which otherwise happens automatically when flushing the\ndiff).\n\nSo I definitely agree that the current API is weird and backwards. But\nflipping it now may introduce new leaks in existing callers.\n\n-Peff\n"},{"id":"416737","messageId":"20210211104535.16076-3-avarab@gmail.com","threadId":"54333","inReplyTo":"YCUFNVj7qlt9wzlX@coredump.intra.peff.net","subject":"[PATCH v2 2/2] diff: plug memory leak from regcomp() on {log,diff} -I","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-11T10:45:35Z","receivedAt":"2021-02-11T10:49:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in 296d4a94e7 (diff: add -I that ignores\nmatching changes, 2020-10-20) by freeing the memory it allocates in\nthe newly introduced diff_free(). See the previous commit for details\non that.\n\nThis memory leak was intentionally introduced in 296d4a94e7, see the\ndiscussion on a previous iteration of it in\nhttps://lore.kernel.org/git/xmqqeelycajx.fsf@gitster.c.googlers.com/\n\nAt that time freeing the memory was somewhat tedious, but since it\nisn't anymore with the newly introduced diff_free() let's use it.\n\nLet's retain the pattern for diff_free_file() and add a\ndiff_free_ignore_regex(), even though (unlike \"diff_free_file\") we\ndon't need to call it elsewhere. I think this'll make for more\nreadable code than gradually accumulating a giant diff_free()\nfunction, sharing \"int i\" across unrelated code etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex a63c9ecae79..bf2cbf15e77 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6342,12 +6342,24 @@ static void diff_free_file(struct diff_options *options)\n \t\tfclose(options->file);\n }\n \n+static void diff_free_ignore_regex(struct diff_options *options)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n+\t\tregfree(options->ignore_regex[i]);\n+\t\tfree(options->ignore_regex[i]);\n+\t}\n+\tfree(options->ignore_regex);\n+}\n+\n void diff_free(struct diff_options *options)\n {\n \tif (options->no_free)\n \t\treturn;\n \n \tdiff_free_file(options);\n+\tdiff_free_ignore_regex(options);\n }\n \n void diff_flush(struct diff_options *options)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416738","messageId":"20210211104535.16076-1-avarab@gmail.com","threadId":"54333","inReplyTo":"YCUFNVj7qlt9wzlX@coredump.intra.peff.net","subject":"[PATCH v2 0/2] diff: add an API for deferred freeing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-11T10:45:33Z","receivedAt":"2021-02-11T10:49:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I skipped the rename of the \"close_file\" flag, and updated the code &\ncommit messages in response to Johannes's feedback on v1. Hopefully\nthis is all better now.\n\nÆvar Arnfjörð Bjarmason (2):\n diff: add an API for deferred freeing\n diff: plug memory leak from regcomp() on {log,diff} -I\n\n builtin/log.c | 23 ++++++++++++-----------\n diff.c | 32 ++++++++++++++++++++++++++++----\n diff.h | 15 ++++++++++++++-\n log-tree.c | 10 ++++++----\n 4 files changed, 60 insertions(+), 20 deletions(-)\n\nRange-diff:\n1: 531fed77f4c ! 1: 045d3f72d15 diff: add an API for deferred freeing\n @@ Commit message\n by setting \"no_free\" in \"diff_options\".\n \n This is required because when e.g. \"git diff\" is run we'll allocate\n - things in that struct, use the diff machinery once, and then exit, but\n - if we run e.g. \"git log -p\" we're going to re-use what we allocated\n - across multiple diff_flush() calls, and only want to free things at\n - the end.\n + things in that struct, use the diff machinery once, and then exit.\n +\n + But if we run e.g. \"git log -p\" we're going to re-use what we\n + allocated across multiple diff_flush() calls, and only want to free\n + things at the end.\n \n We've thus ended up with features like the recently added \"diff -I\"[1]\n where we'll leak memory. As it turns out it could have simply used the\n @@ Commit message\n the diffopt.close_file attribute, 2016-06-22).\n \n Manually adding more such flags to things log_tree_commit() every time\n - we need to allocate something would be tedious.\n -\n - Let's instead move that fclose() code it to a new diff_free(), in\n - anticipation of freeing more things in that function in follow-up\n - commits. I'm renaming the \"close_file\" struct member to \"fclose_file\"\n - for the ease of validating this, we can be certain that these are all\n - the relevant callsites.\n + we need to allocate something would be tedious. Let's instead move\n + that fclose() code it to a new diff_free(), in anticipation of freeing\n + more things in that function in follow-up commits.\n \n Some functions such as log_tree_commit() need an idiom of optionally\n retaining a previous \"no_free\", as they may either free the memory\n themselves, or their caller may do so. I'm keeping that idiom in\n - log_show_early() even though I don't think it's currently called in\n - this manner, since it also gets passed an existing \"struct rev_info\"..\n + log_show_early() for good measure, even though I don't think it's\n + currently called in this manner. It also gets passed an existing\n + \"struct rev_info\", so future callers may want to set the \"no_free\"\n + flag.\n +\n + This change is a bit hard to read because while the freeing pattern\n + we're introducing isn't unusual, the \"file\" member is a special\n + snowflake. We usually don't want to fclose() it. This is because\n + \"file\" is usually stdout, in which case we don't want to fclose()\n + it. We only want to opt-in to closing it when we e.g. open a file on\n + the filesystem. Thus the opt-in \"close_file\" flag.\n +\n + So the API in general just needs a \"no_free\" flag to defer freeing,\n + but the \"file\" member still needs its \"close_file\" flag. This is made\n + more confusing because while refactoring this code we could replace\n + some \"close_file=0\" with \"no_free=1\", whereas others need to set both\n + flags.\n +\n + This is because there were some cases where an existing \"close_file=0\"\n + meant \"let's defer deallocation\", and others where it meant \"we don't\n + want to close this file handle at all\".\n \n 1. 296d4a94e7 (diff: add -I that ignores matching changes,\n 2020-10-20)\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## builtin/add.c ##\n -@@ builtin/add.c: static int edit_patch(int argc, const char **argv, const char *prefix)\n - \tif (out < 0)\n - \t\tdie(_(\"Could not open '%s' for writing.\"), file);\n - \trev.diffopt.file = xfdopen(out, \"w\");\n --\trev.diffopt.close_file = 1;\n -+\trev.diffopt.fclose_file = 1;\n - \tif (run_diff_files(&rev, 0))\n - \t\tdie(_(\"Could not write patch\"));\n - \n -\n - ## builtin/am.c ##\n -@@ builtin/am.c: static void write_commit_patch(const struct am_state *state, struct commit *comm\n - \trev_info.diffopt.flags.full_index = 1;\n - \trev_info.diffopt.use_color = 0;\n - \trev_info.diffopt.file = fp;\n --\trev_info.diffopt.close_file = 1;\n -+\trev_info.diffopt.fclose_file = 1; /* log_tree_commit() sets .no_free=1 */\n - \tadd_pending_object(&rev_info, &commit->object, \"\");\n - \tdiff_setup_done(&rev_info.diffopt);\n - \tlog_tree_commit(&rev_info, commit);\n -@@ builtin/am.c: static void write_index_patch(const struct am_state *state)\n - \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n - \trev_info.diffopt.use_color = 0;\n - \trev_info.diffopt.file = fp;\n --\trev_info.diffopt.close_file = 1;\n -+\trev_info.diffopt.fclose_file = 1;\n -+\trev_info.diffopt.no_free = 1;\n - \tadd_pending_object(&rev_info, &tree->object, \"\");\n - \tdiff_setup_done(&rev_info.diffopt);\n - \trun_diff_index(&rev_info, 1);\n -+\tdiff_free(&rev_info.diffopt);\n - }\n - \n - /**\n -\n ## builtin/log.c ##\n @@ builtin/log.c: static struct itimerval early_output_timer;\n \n @@ builtin/log.c: static int cmd_log_walk(struct rev_info *rev)\n \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n \t rev->diffopt.flags.check_failed) {\n @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)\n - \tif (rev.show_notes)\n - \t\tload_display_notes(&rev.notes_opt);\n - \n --\tif (use_stdout + rev.diffopt.close_file + !!output_directory > 1)\n -+\tif (use_stdout + rev.diffopt.fclose_file + !!output_directory > 1)\n - \t\tdie(_(\"--stdout, --output, and --output-directory are mutually exclusive\"));\n - \n - \tif (use_stdout) {\n - \t\tsetup_pager();\n --\t} else if (rev.diffopt.close_file) {\n -+\t} else if (rev.diffopt.fclose_file) {\n - \t\t/*\n - \t\t * The diff code parsed --output; it has already opened the\n \t\t * file, but but we must instruct it not to close after each\n \t\t * diff.\n \t\t */\n @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n \n \n ## diff.c ##\n -@@ diff.c: static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,\n - \tBUG_ON_OPT_NEG(unset);\n - \tpath = prefix_filename(ctx->prefix, arg);\n - \toptions->file = xfopen(path, \"w\");\n --\toptions->close_file = 1;\n -+\toptions->fclose_file = 1;\n - \tif (options->use_color != GIT_COLOR_ALWAYS)\n - \t\toptions->use_color = GIT_COLOR_NEVER;\n - \tfree(path);\n +@@ diff.c: static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n + \t}\n + }\n + \n ++static void diff_free_file(struct diff_options *options)\n ++{\n ++\tif (options->close_file)\n ++\t\tfclose(options->file);\n ++}\n ++\n ++void diff_free(struct diff_options *options)\n ++{\n ++\tif (options->no_free)\n ++\t\treturn;\n ++\n ++\tdiff_free_file(options);\n ++}\n ++\n + void diff_flush(struct diff_options *options)\n + {\n + \tstruct diff_queue_struct *q = &diff_queued_diff;\n @@ diff.c: void diff_flush(struct diff_options *options)\n \t\t * options->file to /dev/null should be safe, because we\n \t\t * aren't supposed to produce any output anyway.\n \t\t */\n -\t\tif (options->close_file)\n -+\t\tif (options->fclose_file)\n - \t\t\tfclose(options->file);\n +-\t\t\tfclose(options->file);\n ++\t\tdiff_free_file(options);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n --\t\toptions->close_file = 1;\n -+\t\toptions->fclose_file = 1;\n + \t\toptions->close_file = 1;\n \t\toptions->color_moved = 0;\n - \t\tfor (i = 0; i < q->nr; i++) {\n - \t\t\tstruct diff_filepair *p = q->queue[i];\n @@ diff.c: void diff_flush(struct diff_options *options)\n free_queue:\n \tfree(q->queue);\n @@ diff.c: void diff_flush(struct diff_options *options)\n \n \t/*\n \t * Report the content-level differences with HAS_CHANGES;\n -@@ diff.c: void diff_flush(struct diff_options *options)\n - \t}\n - }\n - \n -+void diff_free(struct diff_options *options)\n -+{\n -+\tif (options->no_free)\n -+\t\treturn;\n -+\tif (options->fclose_file)\n -+\t\tfclose(options->file);\n -+}\n -+\t\n -+\n - static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n - {\n - \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n \n ## diff.h ##\n @@\n * - Once you finish feeding the pairs of files, call `diffcore_std()`.\n * This will tell the diffcore library to go ahead and do its work.\n *\n -+ * - The `diff_opt_parse()` etc. functions might allocate memory in\n -+ * `struct diff_options`. When running the API `N > 1` set `.no_free\n -+ * = 1` to make the `diff_free()` invoked by `diff_flush()` below a\n -+ * noop.\n -+ *\n - * - Calling `diff_flush()` will produce the output.\n +- * - Calling `diff_flush()` will produce the output.\n ++ * - Calling `diff_flush()` will produce the output, it will call\n ++ * `diff_free()` to free any resources, e.g. those allocated in\n ++ * `diff_opt_parse()`.\n + *\n -+ * - If you set `.no_free = 1` before set it to `0` and call\n -+ * `diff_free()` again. If `.no_free = 1` was not set there's no\n -+ * need to call `diff_free()`, `diff_flush()` will call it.\n ++ * - Set `.no_free = 1` before calling `diff_flush()` to defer the\n ++ * freeing of allocated memory in diff_options. This is useful when\n ++ * `diff_flush()` is being called in a loop, rather than as a\n ++ * one-off. When setting `.no_free = 1` you must ensure that\n ++ * `diff_free()` is called at the end, either by flipping the flag\n ++ * before the last `diff_flush()` call, or by flipping it before\n ++ * calling `diff_free()` yourself.\n */\n \n struct combine_diff_path;\n @@ diff.h: struct diff_options {\n - \tvoid (*set_default)(struct diff_options *);\n - \n - \tFILE *file;\n --\tint close_file;\n -+\tint fclose_file;\n - \n - #define OUTPUT_INDICATOR_NEW 0\n - #define OUTPUT_INDICATOR_OLD 1\n -@@ diff.h: struct diff_options {\n \n \tstruct repository *repo;\n \tstruct option *parseopts;\n @@ log-tree.c: int log_tree_commit(struct rev_info *opt, struct commit *commit)\n +\tdiff_free(&opt->diffopt);\n \treturn shown;\n }\n -\n - ## wt-status.c ##\n -@@ wt-status.c: static void wt_longstatus_print_verbose(struct wt_status *s)\n - \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n - \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n - \trev.diffopt.file = s->fp;\n --\trev.diffopt.close_file = 0;\n -+\trev.diffopt.fclose_file = 0; /* wt_status owns the s->fp */\n - \t/*\n - \t * If we're not going to stdout, then we definitely don't\n - \t * want color, since we are going to the commit message\n2: 7192cf01e71 ! 2: f571524e6d8 diff: plug memory leak from regcomp() on {log,diff} -I\n @@ Commit message\n At that time freeing the memory was somewhat tedious, but since it\n isn't anymore with the newly introduced diff_free() let's use it.\n \n + Let's retain the pattern for diff_free_file() and add a\n + diff_free_ignore_regex(), even though (unlike \"diff_free_file\") we\n + don't need to call it elsewhere. I think this'll make for more\n + readable code than gradually accumulating a giant diff_free()\n + function, sharing \"int i\" across unrelated code etc.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## diff.c ##\n -@@ diff.c: void diff_flush(struct diff_options *options)\n +@@ diff.c: static void diff_free_file(struct diff_options *options)\n + \t\tfclose(options->file);\n + }\n \n - void diff_free(struct diff_options *options)\n - {\n ++static void diff_free_ignore_regex(struct diff_options *options)\n ++{\n +\tint i;\n - \tif (options->no_free)\n - \t\treturn;\n - \tif (options->fclose_file)\n - \t\tfclose(options->file);\n +\n +\tfor (i = 0; i < options->ignore_regex_nr; i++) {\n +\t\tregfree(options->ignore_regex[i]);\n +\t\tfree(options->ignore_regex[i]);\n +\t}\n +\tfree(options->ignore_regex);\n ++}\n ++\n + void diff_free(struct diff_options *options)\n + {\n + \tif (options->no_free)\n + \t\treturn;\n + \n + \tdiff_free_file(options);\n ++\tdiff_free_ignore_regex(options);\n }\n - \t\n \n + void diff_flush(struct diff_options *options)\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"},{"id":"416739","messageId":"20210211104535.16076-2-avarab@gmail.com","threadId":"54333","inReplyTo":"YCUFNVj7qlt9wzlX@coredump.intra.peff.net","subject":"[PATCH v2 1/2] diff: add an API for deferred freeing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-11T10:45:34Z","receivedAt":"2021-02-11T10:50:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a diff_free() function to free anything we may have allocated in\nthe \"diff_options\" struct, and the ability to make calling it a noop\nby setting \"no_free\" in \"diff_options\".\n\nThis is required because when e.g. \"git diff\" is run we'll allocate\nthings in that struct, use the diff machinery once, and then exit.\n\nBut if we run e.g. \"git log -p\" we're going to re-use what we\nallocated across multiple diff_flush() calls, and only want to free\nthings at the end.\n\nWe've thus ended up with features like the recently added \"diff -I\"[1]\nwhere we'll leak memory. As it turns out it could have simply used the\npattern established in 6ea57703f6 (log: prepare log/log-tree to reuse\nthe diffopt.close_file attribute, 2016-06-22).\n\nManually adding more such flags to things log_tree_commit() every time\nwe need to allocate something would be tedious. Let's instead move\nthat fclose() code it to a new diff_free(), in anticipation of freeing\nmore things in that function in follow-up commits.\n\nSome functions such as log_tree_commit() need an idiom of optionally\nretaining a previous \"no_free\", as they may either free the memory\nthemselves, or their caller may do so. I'm keeping that idiom in\nlog_show_early() for good measure, even though I don't think it's\ncurrently called in this manner. It also gets passed an existing\n\"struct rev_info\", so future callers may want to set the \"no_free\"\nflag.\n\nThis change is a bit hard to read because while the freeing pattern\nwe're introducing isn't unusual, the \"file\" member is a special\nsnowflake. We usually don't want to fclose() it. This is because\n\"file\" is usually stdout, in which case we don't want to fclose()\nit. We only want to opt-in to closing it when we e.g. open a file on\nthe filesystem. Thus the opt-in \"close_file\" flag.\n\nSo the API in general just needs a \"no_free\" flag to defer freeing,\nbut the \"file\" member still needs its \"close_file\" flag. This is made\nmore confusing because while refactoring this code we could replace\nsome \"close_file=0\" with \"no_free=1\", whereas others need to set both\nflags.\n\nThis is because there were some cases where an existing \"close_file=0\"\nmeant \"let's defer deallocation\", and others where it meant \"we don't\nwant to close this file handle at all\".\n\n1. 296d4a94e7 (diff: add -I that ignores matching changes,\n 2020-10-20)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/log.c | 23 ++++++++++++-----------\n diff.c | 20 ++++++++++++++++----\n diff.h | 15 ++++++++++++++-\n log-tree.c | 10 ++++++----\n 4 files changed, 48 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d0cbaaf68a0..fffaf51970d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -307,10 +307,11 @@ static struct itimerval early_output_timer;\n \n static void log_show_early(struct rev_info *revs, struct commit_list *list)\n {\n-\tint i = revs->early_output, close_file = revs->diffopt.close_file;\n+\tint i = revs->early_output;\n \tint show_header = 1;\n+\tint no_free = revs->diffopt.no_free;\n \n-\trevs->diffopt.close_file = 0;\n+\trevs->diffopt.no_free = 0;\n \tsort_in_topological_order(&list, revs->sort_order);\n \twhile (list && i) {\n \t\tstruct commit *commit = list->item;\n@@ -327,8 +328,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \t\tcase commit_ignore:\n \t\t\tbreak;\n \t\tcase commit_error:\n-\t\t\tif (close_file)\n-\t\t\t\tfclose(revs->diffopt.file);\n+\t\t\trevs->diffopt.no_free = no_free;\n+\t\t\tdiff_free(&revs->diffopt);\n \t\t\treturn;\n \t\t}\n \t\tlist = list->next;\n@@ -336,8 +337,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)\n \n \t/* Did we already get enough commits for the early output? */\n \tif (!i) {\n-\t\tif (close_file)\n-\t\t\tfclose(revs->diffopt.file);\n+\t\trevs->diffopt.no_free = 0;\n+\t\tdiff_free(&revs->diffopt);\n \t\treturn;\n \t}\n \n@@ -401,7 +402,7 @@ static int cmd_log_walk(struct rev_info *rev)\n {\n \tstruct commit *commit;\n \tint saved_nrl = 0;\n-\tint saved_dcctc = 0, close_file = rev->diffopt.close_file;\n+\tint saved_dcctc = 0;\n \n \tif (rev->early_output)\n \t\tsetup_early_output();\n@@ -417,7 +418,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n-\trev->diffopt.close_file = 0;\n+\trev->diffopt.no_free = 1;\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tif (!log_tree_commit(rev, commit) && rev->max_count >= 0)\n \t\t\t/*\n@@ -442,8 +443,8 @@ static int cmd_log_walk(struct rev_info *rev)\n \t}\n \trev->diffopt.degraded_cc_to_c = saved_dcctc;\n \trev->diffopt.needed_rename_limit = saved_nrl;\n-\tif (close_file)\n-\t\tfclose(rev->diffopt.file);\n+\trev->diffopt.no_free = 0;\n+\tdiff_free(&rev->diffopt);\n \n \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n \t rev->diffopt.flags.check_failed) {\n@@ -1955,7 +1956,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t * file, but but we must instruct it not to close after each\n \t\t * diff.\n \t\t */\n-\t\trev.diffopt.close_file = 0;\n+\t\trev.diffopt.no_free = 1;\n \t} else {\n \t\tint saved;\n \ndiff --git a/diff.c b/diff.c\nindex 69e3bc00ed8..a63c9ecae79 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6336,6 +6336,20 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t}\n }\n \n+static void diff_free_file(struct diff_options *options)\n+{\n+\tif (options->close_file)\n+\t\tfclose(options->file);\n+}\n+\n+void diff_free(struct diff_options *options)\n+{\n+\tif (options->no_free)\n+\t\treturn;\n+\n+\tdiff_free_file(options);\n+}\n+\n void diff_flush(struct diff_options *options)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -6399,8 +6413,7 @@ void diff_flush(struct diff_options *options)\n \t\t * options->file to /dev/null should be safe, because we\n \t\t * aren't supposed to produce any output anyway.\n \t\t */\n-\t\tif (options->close_file)\n-\t\t\tfclose(options->file);\n+\t\tdiff_free_file(options);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n \t\toptions->close_file = 1;\n \t\toptions->color_moved = 0;\n@@ -6433,8 +6446,7 @@ void diff_flush(struct diff_options *options)\n free_queue:\n \tfree(q->queue);\n \tDIFF_QUEUE_CLEAR(q);\n-\tif (options->close_file)\n-\t\tfclose(options->file);\n+\tdiff_free(options);\n \n \t/*\n \t * Report the content-level differences with HAS_CHANGES;\ndiff --git a/diff.h b/diff.h\nindex 2ff2b1c7f2c..527fb56d851 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -49,7 +49,17 @@\n * - Once you finish feeding the pairs of files, call `diffcore_std()`.\n * This will tell the diffcore library to go ahead and do its work.\n *\n- * - Calling `diff_flush()` will produce the output.\n+ * - Calling `diff_flush()` will produce the output, it will call\n+ * `diff_free()` to free any resources, e.g. those allocated in\n+ * `diff_opt_parse()`.\n+ *\n+ * - Set `.no_free = 1` before calling `diff_flush()` to defer the\n+ * freeing of allocated memory in diff_options. This is useful when\n+ * `diff_flush()` is being called in a loop, rather than as a\n+ * one-off. When setting `.no_free = 1` you must ensure that\n+ * `diff_free()` is called at the end, either by flipping the flag\n+ * before the last `diff_flush()` call, or by flipping it before\n+ * calling `diff_free()` yourself.\n */\n \n struct combine_diff_path;\n@@ -365,6 +375,8 @@ struct diff_options {\n \n \tstruct repository *repo;\n \tstruct option *parseopts;\n+\n+\tint no_free;\n };\n \n unsigned diff_filter_bit(char status);\n@@ -559,6 +571,7 @@ void diffcore_fix_diff_index(void);\n \n int diff_queue_is_empty(void);\n void diff_flush(struct diff_options*);\n+void diff_free(struct diff_options*);\n void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n \n /* diff-raw status letters */\ndiff --git a/log-tree.c b/log-tree.c\nindex e0484676507..e7fcd70ba17 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -958,12 +958,14 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n int log_tree_commit(struct rev_info *opt, struct commit *commit)\n {\n \tstruct log_info log;\n-\tint shown, close_file = opt->diffopt.close_file;\n+\tint shown;\n+\t/* maybe called by e.g. cmd_log_walk(), maybe stand-alone */\n+\tint no_free = opt->diffopt.no_free;\n \n \tlog.commit = commit;\n \tlog.parent = NULL;\n \topt->loginfo = &log;\n-\topt->diffopt.close_file = 0;\n+\topt->diffopt.no_free = 1;\n \n \tif (opt->line_level_traverse)\n \t\treturn line_log_print(opt, commit);\n@@ -980,7 +982,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tfprintf(opt->diffopt.file, \"\\n%s\\n\", opt->break_bar);\n \topt->loginfo = NULL;\n \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n-\tif (close_file)\n-\t\tfclose(opt->diffopt.file);\n+\topt->diffopt.no_free = no_free;\n+\tdiff_free(&opt->diffopt);\n \treturn shown;\n }\n-- \n2.30.0.284.gd98b1dd5eaa7\n\n"}]}