Skip to content

Commit

Permalink
diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)
Browse files Browse the repository at this point in the history
Have the diff_free() function call clear_pathspec(). Since the
diff_flush() function calls this all its callers can be simplified to
rely on it instead.

When I added the diff_free() function in e900d49 (diff: add an API
for deferred freeing, 2021-02-11) I simply missed this, or wasn't
interested in it. Let's consolidate this now. This means that any
future callers (and I've got revision.c in mind) that embed a "struct
diff_options" can simply call diff_free() instead of needing know that
it has an embedded pathspec.

This does fix a bunch of leaks, but I can't mark any test here as
passing under the SANITIZE=leak testing mode because in
886e108 (builtin/: add UNLEAKs, 2017-10-01) an UNLEAK(rev) was
added, which plasters over the memory
leak. E.g. "t4011-diff-symlink.sh" would report fewer leaks with this
fix, but because of the UNLEAK() reports none.

I'll eventually loop around to removing that UNLEAK(rev) annotation as
I'll fix deeper issues with the revisions API leaking. This is one
small step on the way there, a new freeing function in revisions.c
will want to call this diff_free().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
avar authored and gitster committed Feb 16, 2022
1 parent 4c53a8c commit 244c272
Show file tree
Hide file tree
Showing 5 changed files with 4 additions and 9 deletions.
6 changes: 3 additions & 3 deletions add-interactive.c
Expand Up @@ -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))
if (do_diff_cache(&oid, &diffopt)) {
diff_free(&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)
Expand Down
3 changes: 0 additions & 3 deletions blame.c
Expand Up @@ -1403,7 +1403,6 @@ static struct blame_origin *find_origin(struct repository *r,
}
}
diff_flush(&diff_opts);
clear_pathspec(&diff_opts.pathspec);
return porigin;
}

Expand Down Expand Up @@ -1447,7 +1446,6 @@ static struct blame_origin *find_rename(struct repository *r,
}
}
diff_flush(&diff_opts);
clear_pathspec(&diff_opts.pathspec);
return porigin;
}

Expand Down Expand Up @@ -2328,7 +2326,6 @@ 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);
}

/*
Expand Down
1 change: 0 additions & 1 deletion builtin/reset.c
Expand Up @@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,
return 1;
diffcore_std(&opt);
diff_flush(&opt);
clear_pathspec(&opt.pathspec);

return 0;
}
Expand Down
1 change: 1 addition & 0 deletions diff.c
Expand Up @@ -6345,6 +6345,7 @@ 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)
Expand Down
2 changes: 0 additions & 2 deletions notes-merge.c
Expand Up @@ -175,7 +175,6 @@ 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;
Expand Down Expand Up @@ -261,7 +260,6 @@ 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)
Expand Down

0 comments on commit 244c272

Please sign in to comment.