{"thread":{"id":"57422","subject":"[PATCH 0/2] diff_free(): free a bit more, fix leaks","startedAt":"2022-02-16T10:56:39Z","lastAt":"2022-02-17T10:24:44Z","messageCount":8,"participants":["Ævar Arnfjörð Bjarmason","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"448582","messageId":"cover-0.2-00000000000-20220216T105250Z-avarab@gmail.com","threadId":"57422","inReplyTo":null,"subject":"[PATCH 0/2] diff_free(): free a bit more, fix leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T10:56:27Z","receivedAt":"2022-02-16T10:56:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A short and simple two-part series to fix some leaks in after\ndiff_flush() and friends are called by making in diff_free() more\nuseful.\n\nThis series along with other small leak fixes I have outstanding\non-list now is the last step I have before the big one of submitting a\nseries to fix the \"big one\" across the test suite: the common leaks in\nthe revisions API.\n\nÆvar Arnfjörð Bjarmason (2):\n  diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)\n  diff.[ch]: have diff_free() free options->parseopts\n\n add-interactive.c | 6 +++---\n blame.c           | 3 ---\n builtin/reset.c   | 1 -\n diff.c            | 2 ++\n notes-merge.c     | 2 --\n 5 files changed, 5 insertions(+), 9 deletions(-)\n\n-- \n2.35.1.1028.g2d2d4be19de\n\n"},{"id":"448583","messageId":"patch-1.2-1c6c7f0f52f-20220216T105250Z-avarab@gmail.com","threadId":"57422","inReplyTo":"cover-0.2-00000000000-20220216T105250Z-avarab@gmail.com","subject":"[PATCH 1/2] diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T10:56:28Z","receivedAt":"2022-02-16T10:56:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Have the diff_free() function call clear_pathspec(). Since the\ndiff_flush() function calls this all its callers can be simplified to\nrely on it instead.\n\nWhen I added the diff_free() function in e900d494dcf (diff: add an API\nfor deferred freeing, 2021-02-11) I simply missed this, or wasn't\ninterested in it. Let's consolidate this now. This means that any\nfuture callers (and I've got revision.c in mind) that embed a \"struct\ndiff_options\" can simply call diff_free() instead of needing know that\nit has an embedded pathspec.\n\nThis does fix a bunch of leaks, but I can't mark any test here as\npassing under the SANITIZE=leak testing mode because in\n886e1084d78 (builtin/: add UNLEAKs, 2017-10-01) an UNLEAK(rev) was\nadded, which plasters over the memory\nleak. E.g. \"t4011-diff-symlink.sh\" would report fewer leaks with this\nfix, but because of the UNLEAK() reports none.\n\nI'll eventually loop around to removing that UNLEAK(rev) annotation as\nI'll fix deeper issues with the revisions API leaking. This is one\nsmall step on the way there, a new freeing function in revisions.c\nwill want to call this diff_free().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n add-interactive.c | 6 +++---\n blame.c           | 3 ---\n builtin/reset.c   | 1 -\n diff.c            | 1 +\n notes-merge.c     | 2 --\n 5 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6498ae196f1..e1ab39cce30 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n \tdiffopt.flags.override_submodule_config = 1;\n \tdiffopt.repo = s->r;\n \n-\tif (do_diff_cache(&oid, &diffopt))\n+\tif (do_diff_cache(&oid, &diffopt)) {\n+\t\tdiff_free(&diffopt);\n \t\tres = -1;\n-\telse {\n+\t} else {\n \t\tdiffcore_std(&diffopt);\n \t\tdiff_flush(&diffopt);\n \t}\n \tfree(paths);\n-\tclear_pathspec(&diffopt.pathspec);\n \n \tif (!res && write_locked_index(s->r->index, &index_lock,\n \t\t\t\t       COMMIT_LOCK) < 0)\ndiff --git a/blame.c b/blame.c\nindex 206c295660f..401990726e7 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1403,7 +1403,6 @@ static struct blame_origin *find_origin(struct repository *r,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n-\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -1447,7 +1446,6 @@ static struct blame_origin *find_rename(struct repository *r,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n-\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -2328,7 +2326,6 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,\n \t} while (unblamed);\n \ttarget->suspects = reverse_blame(leftover, NULL);\n \tdiff_flush(&diff_opts);\n-\tclear_pathspec(&diff_opts.pathspec);\n }\n \n /*\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex b97745ee94e..24968dd6282 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n \t\treturn 1;\n \tdiffcore_std(&opt);\n \tdiff_flush(&opt);\n-\tclear_pathspec(&opt.pathspec);\n \n \treturn 0;\n }\ndiff --git a/diff.c b/diff.c\nindex c862771a589..0aef3db6e10 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)\n \n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n+\tclear_pathspec(&options->pathspec);\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex b4a3a903e86..7ba40cfb080 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -175,7 +175,6 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n \t\t       oid_to_hex(&mp->remote));\n \t}\n \tdiff_flush(&opt);\n-\tclear_pathspec(&opt.pathspec);\n \n \t*num_changes = len;\n \treturn changes;\n@@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,\n \t\t       oid_to_hex(&mp->local));\n \t}\n \tdiff_flush(&opt);\n-\tclear_pathspec(&opt.pathspec);\n }\n \n static void check_notes_merge_worktree(struct notes_merge_options *o)\n-- \n2.35.1.1028.g2d2d4be19de\n\n"},{"id":"448584","messageId":"patch-2.2-7f320062419-20220216T105250Z-avarab@gmail.com","threadId":"57422","inReplyTo":"cover-0.2-00000000000-20220216T105250Z-avarab@gmail.com","subject":"[PATCH 2/2] diff.[ch]: have diff_free() free options->parseopts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-16T10:56:29Z","receivedAt":"2022-02-16T10:56:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"struct option\" added in 4a288478394 (diff.c: prepare to use\nparse_options() for parsing, 2019-01-27) would be free'd in the case\nof diff_setup_done() being called.\n\nBut not all codepaths that allocate it reach that,\ne.g. \"t6427-diff3-conflict-markers.sh\" will now free memory that it\ndidn't free before. By using FREE_AND_NULL() here (which\ndiff_setup_done() also does) we ensure that we free the memory, and\nthat we won't have double-free's.\n\nBefore this running:\n\n    ./t6427-diff3-conflict-markers.sh -vixd --run=7\n\nWould report:\n\n    SUMMARY: LeakSanitizer: 7823 byte(s) leaked in 6 allocation(s).\n\nBut now we'll report:\n\n    SUMMARY: LeakSanitizer: 703 byte(s) leaked in 5 allocation(s).\n\nI.e. the largest leak in that particular test has now been addressed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n diff.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/diff.c b/diff.c\nindex 0aef3db6e10..fb8bc8aadbf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6346,6 +6346,7 @@ void diff_free(struct diff_options *options)\n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n+\tFREE_AND_NULL(options->parseopts);\n }\n \n void diff_flush(struct diff_options *options)\n-- \n2.35.1.1028.g2d2d4be19de\n\n"},{"id":"448612","messageId":"CABPp-BHYR8a9YyPGGrUhjMbzPws286FeHjyNcOmgk=8XnwOiVg@mail.gmail.com","threadId":"57422","inReplyTo":"patch-1.2-1c6c7f0f52f-20220216T105250Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-16T16:50:47Z","receivedAt":"2022-02-16T16:51:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 16, 2022 at 7:34 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> Have the diff_free() function call clear_pathspec(). Since the\n> diff_flush() function calls this all its callers can be simplified to\n> rely on it instead.\n>\n> When I added the diff_free() function in e900d494dcf (diff: add an API\n> for deferred freeing, 2021-02-11) I simply missed this, or wasn't\n> interested in it. Let's consolidate this now. This means that any\n> future callers (and I've got revision.c in mind) that embed a \"struct\n> diff_options\" can simply call diff_free() instead of needing know that\n> it has an embedded pathspec.\n>\n> This does fix a bunch of leaks, but I can't mark any test here as\n> passing under the SANITIZE=leak testing mode because in\n> 886e1084d78 (builtin/: add UNLEAKs, 2017-10-01) an UNLEAK(rev) was\n> added, which plasters over the memory\n> leak. E.g. \"t4011-diff-symlink.sh\" would report fewer leaks with this\n> fix, but because of the UNLEAK() reports none.\n>\n> I'll eventually loop around to removing that UNLEAK(rev) annotation as\n> I'll fix deeper issues with the revisions API leaking. This is one\n> small step on the way there, a new freeing function in revisions.c\n> will want to call this diff_free().\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  add-interactive.c | 6 +++---\n>  blame.c           | 3 ---\n>  builtin/reset.c   | 1 -\n>  diff.c            | 1 +\n>  notes-merge.c     | 2 --\n>  5 files changed, 4 insertions(+), 9 deletions(-)\n>\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 6498ae196f1..e1ab39cce30 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n>         diffopt.flags.override_submodule_config = 1;\n>         diffopt.repo = s->r;\n>\n> -       if (do_diff_cache(&oid, &diffopt))\n> +       if (do_diff_cache(&oid, &diffopt)) {\n> +               diff_free(&diffopt);\n>                 res = -1;\n> -       else {\n> +       } else {\n>                 diffcore_std(&diffopt);\n>                 diff_flush(&diffopt);\n>         }\n>         free(paths);\n> -       clear_pathspec(&diffopt.pathspec);\n>\n>         if (!res && write_locked_index(s->r->index, &index_lock,\n>                                        COMMIT_LOCK) < 0)\n> diff --git a/blame.c b/blame.c\n> index 206c295660f..401990726e7 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -1403,7 +1403,6 @@ static struct blame_origin *find_origin(struct repository *r,\n>                 }\n>         }\n>         diff_flush(&diff_opts);\n> -       clear_pathspec(&diff_opts.pathspec);\n>         return porigin;\n>  }\n>\n> @@ -1447,7 +1446,6 @@ static struct blame_origin *find_rename(struct repository *r,\n>                 }\n>         }\n>         diff_flush(&diff_opts);\n> -       clear_pathspec(&diff_opts.pathspec);\n>         return porigin;\n>  }\n>\n> @@ -2328,7 +2326,6 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,\n>         } while (unblamed);\n>         target->suspects = reverse_blame(leftover, NULL);\n>         diff_flush(&diff_opts);\n> -       clear_pathspec(&diff_opts.pathspec);\n>  }\n>\n>  /*\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index b97745ee94e..24968dd6282 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n>                 return 1;\n>         diffcore_std(&opt);\n>         diff_flush(&opt);\n> -       clear_pathspec(&opt.pathspec);\n>\n>         return 0;\n>  }\n> diff --git a/diff.c b/diff.c\n> index c862771a589..0aef3db6e10 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)\n>\n>         diff_free_file(options);\n>         diff_free_ignore_regex(options);\n> +       clear_pathspec(&options->pathspec);\n>  }\n>\n>  void diff_flush(struct diff_options *options)\n> diff --git a/notes-merge.c b/notes-merge.c\n> index b4a3a903e86..7ba40cfb080 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -175,7 +175,6 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n>                        oid_to_hex(&mp->remote));\n>         }\n>         diff_flush(&opt);\n> -       clear_pathspec(&opt.pathspec);\n>\n>         *num_changes = len;\n>         return changes;\n> @@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,\n>                        oid_to_hex(&mp->local));\n>         }\n>         diff_flush(&opt);\n> -       clear_pathspec(&opt.pathspec);\n>  }\n>\n>  static void check_notes_merge_worktree(struct notes_merge_options *o)\n> --\n> 2.35.1.1028.g2d2d4be19de\n\nI have occasionally found it surprising that we have places where\ncallers are responsible for free'ing a single piece of some struct\ndespite the fact that we have a function for free'ing memory\nassociated with that struct.  Thanks for getting rid of another one of\nthese.\n"},{"id":"448613","messageId":"CABPp-BEyTnfEANPii-33y0g3fQgkRY8Bz3zaXScN__Ad7z4n1g@mail.gmail.com","threadId":"57422","inReplyTo":"patch-2.2-7f320062419-20220216T105250Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] diff.[ch]: have diff_free() free options->parseopts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-16T16:51:27Z","receivedAt":"2022-02-16T16:51:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 16, 2022 at 7:59 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> The \"struct option\" added in 4a288478394 (diff.c: prepare to use\n> parse_options() for parsing, 2019-01-27) would be free'd in the case\n> of diff_setup_done() being called.\n>\n> But not all codepaths that allocate it reach that,\n> e.g. \"t6427-diff3-conflict-markers.sh\" will now free memory that it\n> didn't free before. By using FREE_AND_NULL() here (which\n> diff_setup_done() also does) we ensure that we free the memory, and\n> that we won't have double-free's.\n>\n> Before this running:\n>\n>     ./t6427-diff3-conflict-markers.sh -vixd --run=7\n>\n> Would report:\n>\n>     SUMMARY: LeakSanitizer: 7823 byte(s) leaked in 6 allocation(s).\n>\n> But now we'll report:\n>\n>     SUMMARY: LeakSanitizer: 703 byte(s) leaked in 5 allocation(s).\n>\n> I.e. the largest leak in that particular test has now been addressed.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  diff.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 0aef3db6e10..fb8bc8aadbf 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6346,6 +6346,7 @@ void diff_free(struct diff_options *options)\n>         diff_free_file(options);\n>         diff_free_ignore_regex(options);\n>         clear_pathspec(&options->pathspec);\n> +       FREE_AND_NULL(options->parseopts);\n>  }\n>\n>  void diff_flush(struct diff_options *options)\n> --\n> 2.35.1.1028.g2d2d4be19de\n\nMakes sense.\n"},{"id":"448614","messageId":"CABPp-BE6Zuo=L3zCHHee2T-WkbF7m+dRp9MzoB_JQuP0vns6eA@mail.gmail.com","threadId":"57422","inReplyTo":"cover-0.2-00000000000-20220216T105250Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] diff_free(): free a bit more, fix leaks","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-16T16:52:14Z","receivedAt":"2022-02-16T16:52:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 16, 2022 at 7:56 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> A short and simple two-part series to fix some leaks in after\n> diff_flush() and friends are called by making in diff_free() more\n> useful.\n\nReviewed-by: Elijah Newren <newren@gmail.com>\n\n> This series along with other small leak fixes I have outstanding\n> on-list now is the last step I have before the big one of submitting a\n> series to fix the \"big one\" across the test suite: the common leaks in\n> the revisions API.\n\nWahoo!\n"},{"id":"448641","messageId":"xmqqh78ycwjc.fsf@gitster.g","threadId":"57422","inReplyTo":"patch-1.2-1c6c7f0f52f-20220216T105250Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-16T21:48:55Z","receivedAt":"2022-02-16T21:48:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> diff --git a/diff.c b/diff.c\n> index c862771a589..0aef3db6e10 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)\n>  \n>  \tdiff_free_file(options);\n>  \tdiff_free_ignore_regex(options);\n> +\tclear_pathspec(&options->pathspec);\n>  }\n>  \n>  void diff_flush(struct diff_options *options)\n\nInteresting.  As diff_flush() is the way to conclude the diff\nsession whose state was kept in the diff_options structure, it\nprobably makes sense to allow pathspec to be also cleared from\nthere.  It is somewhat surprising that we didn't do this when we\nintroduced diff_free(), but better late than never ;-)\n\n> diff --git a/notes-merge.c b/notes-merge.c\n> index b4a3a903e86..7ba40cfb080 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -175,7 +175,6 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n>  \t\t       oid_to_hex(&mp->remote));\n>  \t}\n>  \tdiff_flush(&opt);\n> -\tclear_pathspec(&opt.pathspec);\n>  \n>  \t*num_changes = len;\n>  \treturn changes;\n> @@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,\n>  \t\t       oid_to_hex(&mp->local));\n>  \t}\n>  \tdiff_flush(&opt);\n> -\tclear_pathspec(&opt.pathspec);\n>  }\n>  \n>  static void check_notes_merge_worktree(struct notes_merge_options *o)\n\n\nLooks quite sensible.  Will queue.  Thanks.\n\n"},{"id":"448678","messageId":"220217.865ypdiyei.gmgdl@evledraar.gmail.com","threadId":"57422","inReplyTo":"CABPp-BHYR8a9YyPGGrUhjMbzPws286FeHjyNcOmgk=8XnwOiVg@mail.gmail.com","subject":"Re: [PATCH 1/2] diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-17T10:22:15Z","receivedAt":"2022-02-17T10:24:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Elijah Newren wrote:\n\n> On Wed, Feb 16, 2022 at 7:34 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> Have the diff_free() function call clear_pathspec(). Since the\n>> diff_flush() function calls this all its callers can be simplified to\n>> rely on it instead.\n>>\n>> When I added the diff_free() function in e900d494dcf (diff: add an API\n>> for deferred freeing, 2021-02-11) I simply missed this, or wasn't\n>> interested in it. Let's consolidate this now. This means that any\n>> future callers (and I've got revision.c in mind) that embed a \"struct\n>> diff_options\" can simply call diff_free() instead of needing know that\n>> it has an embedded pathspec.\n>>\n>> This does fix a bunch of leaks, but I can't mark any test here as\n>> passing under the SANITIZE=leak testing mode because in\n>> 886e1084d78 (builtin/: add UNLEAKs, 2017-10-01) an UNLEAK(rev) was\n>> added, which plasters over the memory\n>> leak. E.g. \"t4011-diff-symlink.sh\" would report fewer leaks with this\n>> fix, but because of the UNLEAK() reports none.\n>>\n>> I'll eventually loop around to removing that UNLEAK(rev) annotation as\n>> I'll fix deeper issues with the revisions API leaking. This is one\n>> small step on the way there, a new freeing function in revisions.c\n>> will want to call this diff_free().\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  add-interactive.c | 6 +++---\n>>  blame.c           | 3 ---\n>>  builtin/reset.c   | 1 -\n>>  diff.c            | 1 +\n>>  notes-merge.c     | 2 --\n>>  5 files changed, 4 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/add-interactive.c b/add-interactive.c\n>> index 6498ae196f1..e1ab39cce30 100644\n>> --- a/add-interactive.c\n>> +++ b/add-interactive.c\n>> @@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,\n>>         diffopt.flags.override_submodule_config = 1;\n>>         diffopt.repo = s->r;\n>>\n>> -       if (do_diff_cache(&oid, &diffopt))\n>> +       if (do_diff_cache(&oid, &diffopt)) {\n>> +               diff_free(&diffopt);\n>>                 res = -1;\n>> -       else {\n>> +       } else {\n>>                 diffcore_std(&diffopt);\n>>                 diff_flush(&diffopt);\n>>         }\n>>         free(paths);\n>> -       clear_pathspec(&diffopt.pathspec);\n>>\n>>         if (!res && write_locked_index(s->r->index, &index_lock,\n>>                                        COMMIT_LOCK) < 0)\n>> diff --git a/blame.c b/blame.c\n>> index 206c295660f..401990726e7 100644\n>> --- a/blame.c\n>> +++ b/blame.c\n>> @@ -1403,7 +1403,6 @@ static struct blame_origin *find_origin(struct repository *r,\n>>                 }\n>>         }\n>>         diff_flush(&diff_opts);\n>> -       clear_pathspec(&diff_opts.pathspec);\n>>         return porigin;\n>>  }\n>>\n>> @@ -1447,7 +1446,6 @@ static struct blame_origin *find_rename(struct repository *r,\n>>                 }\n>>         }\n>>         diff_flush(&diff_opts);\n>> -       clear_pathspec(&diff_opts.pathspec);\n>>         return porigin;\n>>  }\n>>\n>> @@ -2328,7 +2326,6 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,\n>>         } while (unblamed);\n>>         target->suspects = reverse_blame(leftover, NULL);\n>>         diff_flush(&diff_opts);\n>> -       clear_pathspec(&diff_opts.pathspec);\n>>  }\n>>\n>>  /*\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index b97745ee94e..24968dd6282 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n>>                 return 1;\n>>         diffcore_std(&opt);\n>>         diff_flush(&opt);\n>> -       clear_pathspec(&opt.pathspec);\n>>\n>>         return 0;\n>>  }\n>> diff --git a/diff.c b/diff.c\n>> index c862771a589..0aef3db6e10 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)\n>>\n>>         diff_free_file(options);\n>>         diff_free_ignore_regex(options);\n>> +       clear_pathspec(&options->pathspec);\n>>  }\n>>\n>>  void diff_flush(struct diff_options *options)\n>> diff --git a/notes-merge.c b/notes-merge.c\n>> index b4a3a903e86..7ba40cfb080 100644\n>> --- a/notes-merge.c\n>> +++ b/notes-merge.c\n>> @@ -175,7 +175,6 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n>>                        oid_to_hex(&mp->remote));\n>>         }\n>>         diff_flush(&opt);\n>> -       clear_pathspec(&opt.pathspec);\n>>\n>>         *num_changes = len;\n>>         return changes;\n>> @@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,\n>>                        oid_to_hex(&mp->local));\n>>         }\n>>         diff_flush(&opt);\n>> -       clear_pathspec(&opt.pathspec);\n>>  }\n>>\n>>  static void check_notes_merge_worktree(struct notes_merge_options *o)\n>> --\n>> 2.35.1.1028.g2d2d4be19de\n>\n> I have occasionally found it surprising that we have places where\n> callers are responsible for free'ing a single piece of some struct\n> despite the fact that we have a function for free'ing memory\n> associated with that struct.  Thanks for getting rid of another one of\n> these.\n\nYes, much of the later memory leak effort is focused on consolidating\nthose, there's a lot of it around \"struct rev_info\", e.g. callers\nreaching into it to free a pathspec, the \"struct object_array *pending\"\netc.\n\nThere's some gray area there, particularly when the caller originally\nallocates the resource and \"hands it off\" to the struct, but even in\nthose cases they should probably be freeing their own original variable,\nnot referring to it by reaching into a struct they handed it to.\n\nThat would amount to the exact same thing for the machine (same\npointer), but the former is surely more readable to the human reader.\n"}]}