From: Phillip Wood Date: Tue, 26 Apr 2022 10:09:55 GMT Subject: Re: [PATCH] 2.36 gitk/diff-tree --stdin regression fix Message-ID: <83a76d46-5069-d6c7-b8b3-f3a063637abb@gmail.com> In-Reply-To: On 25/04/2022 18:45, Junio C Hamano wrote: > This reverts commit 244c2724 (diff.[ch]: have diff_free() call > clear_pathspec(opts.pathspec), 2022-02-16). > > The diff_free() call is to be used after a diffopt structure is used > to compare two sets of paths to release resources that were needed > only for that comparison, and keep the data such as pathspec that > are reused by the diffopt structure to make the next and subsequent > comparison (imagine "git log -p - -- " where the > options and pathspec are kept in the diffopt structure, used to > compare HEAD and HEAD~, then used again when HEAD~ and HEAD~2 are > compared). > > We by mistake started clearing the pathspec in diff_free(), so > programs like gitk that runs > > git diff-tree --stdin -- > > downstream of a pipe, processing one commit after another, started > showing irrelevant comparison outside the given from the > second commit. I notice from the patch context that we are still calling diff_free_ignore_regex(options) which was added in c45dc9cf30 ("diff: plug memory leak from regcomp() on {log,diff} -I", 2021-02-11). I think that will need reverting as well as it freeing data that is needed when options is reused by "diff-tree --stdin" or "log -p". Best Wishes Phillip > The buggy commit may have been hiding the places where diff > machinery is used only once and called diff_free() to release that > per-comparison resources, but forgetting to call clear_pathspec() to > release the resource held for the (potentially) repeated comparison, > and we eventually would want to add clear_pathspec() to clear > resources to be released after a (potentially repeated) diff session > is done (if there are similar resources other than pathspec that > need to be cleared at the end, we should then know where to clear > them), but that is "per program invocation" leak that will be > cleaned up by calling exit(3) and of lower priority than fixing this > behavior-breaking regression. > > Reported-by: Matthias Aßhauer > Helped-by: René Scharfe > Signed-off-by: Junio C Hamano > --- > add-interactive.c | 6 +++--- > blame.c | 3 +++ > > builtin/reset.c | 1 + > diff.c | 1 - > notes-merge.c | 2 ++ > 5 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/add-interactive.c b/add-interactive.c > index e1ab39cce3..6498ae196f 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps, > diffopt.flags.override_submodule_config = 1; > diffopt.repo = s->r; > > - if (do_diff_cache(&oid, &diffopt)) { > - diff_free(&diffopt); > + if (do_diff_cache(&oid, &diffopt)) > res = -1; > - } else { > + else { > diffcore_std(&diffopt); > diff_flush(&diffopt); > } > free(paths); > + clear_pathspec(&diffopt.pathspec); > > if (!res && write_locked_index(s->r->index, &index_lock, > COMMIT_LOCK) < 0) > diff --git a/blame.c b/blame.c > index 401990726e..206c295660 100644 > --- a/blame.c > +++ b/blame.c > @@ -1403,6 +1403,7 @@ static struct blame_origin *find_origin(struct repository *r, > } > } > diff_flush(&diff_opts); > + clear_pathspec(&diff_opts.pathspec); > return porigin; > } > > @@ -1446,6 +1447,7 @@ static struct blame_origin *find_rename(struct repository *r, > } > } > diff_flush(&diff_opts); > + clear_pathspec(&diff_opts.pathspec); > return porigin; > } > > @@ -2326,6 +2328,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb, > } while (unblamed); > target->suspects = reverse_blame(leftover, NULL); > diff_flush(&diff_opts); > + clear_pathspec(&diff_opts.pathspec); > } > > /* > diff --git a/builtin/reset.c b/builtin/reset.c > index 24968dd628..b97745ee94 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -274,6 +274,7 @@ static int read_from_tree(const struct pathspec *pathspec, > return 1; > diffcore_std(&opt); > diff_flush(&opt); > + clear_pathspec(&opt.pathspec); > > return 0; > } > diff --git a/diff.c b/diff.c > index 0aef3db6e1..c862771a58 100644 > --- a/diff.c > +++ b/diff.c > @@ -6345,7 +6345,6 @@ void diff_free(struct diff_options *options) > > diff_free_file(options); > diff_free_ignore_regex(options); > - clear_pathspec(&options->pathspec); > } > > void diff_flush(struct diff_options *options) > diff --git a/notes-merge.c b/notes-merge.c > index 7ba40cfb08..b4a3a903e8 100644 > --- a/notes-merge.c > +++ b/notes-merge.c > @@ -175,6 +175,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o, > oid_to_hex(&mp->remote)); > } > diff_flush(&opt); > + clear_pathspec(&opt.pathspec); > > *num_changes = len; > return changes; > @@ -260,6 +261,7 @@ static void diff_tree_local(struct notes_merge_options *o, > oid_to_hex(&mp->local)); > } > diff_flush(&opt); > + clear_pathspec(&opt.pathspec); > } > > static void check_notes_merge_worktree(struct notes_merge_options *o)