Skip to content

Commit 4bac57b

Browse files
gitstergitster
committed
Merge branch 'jk/setup-revisions-freefix'
There are double frees and leaks around setup_revisions() API used in "git stash show", which has been fixed, and setup_revisions() API gained a wrapper to make it more ergonomic when using it with strvec-manged argc/argv pairs. * jk/setup-revisions-freefix: revision: retain argv NULL invariant in setup_revisions() treewide: pass strvecs around for setup_revisions_from_strvec() treewide: use setup_revisions_from_strvec() when we have a strvec revision: add wrapper to setup_revisions() from a strvec revision: manage memory ownership of argv in setup_revisions() stash: tell setup_revisions() to free our allocated strings
2 parents 84edf99 + a04bc71 commit 4bac57b

17 files changed

+85
-42
lines changed

‎bisect.c‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -674,9 +674,6 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
674674
const char *bad_format, const char *good_format,
675675
int read_paths)
676676
{
677-
struct setup_revision_opt opt = {
678-
.free_removed_argv_elements = 1,
679-
};
680677
int i;
681678

682679
repo_init_revisions(r, revs, prefix);
@@ -693,7 +690,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
693690
if (read_paths)
694691
read_bisect_paths(rev_argv);
695692

696-
setup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);
693+
setup_revisions_from_strvec(rev_argv, revs, NULL);
697694
}
698695

699696
static void bisect_common(struct rev_info *revs)

‎builtin/describe.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,8 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)
580580
NULL);
581581

582582
repo_init_revisions(the_repository, &revs, NULL);
583-
if (setup_revisions(args.nr, args.v, &revs, NULL) > 1)
583+
setup_revisions_from_strvec(&args, &revs, NULL);
584+
if (args.nr > 1)
584585
BUG("setup_revisions could not handle all args?");
585586

586587
if (prepare_revision_walk(&revs))

‎builtin/pack-objects.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4650,7 +4650,7 @@ static void get_object_list_path_walk(struct rev_info *revs)
46504650
die(_("failed to pack objects via path-walk"));
46514651
}
46524652

4653-
static void get_object_list(struct rev_info *revs, int ac, const char **av)
4653+
static void get_object_list(struct rev_info *revs, struct strvec *argv)
46544654
{
46554655
struct setup_revision_opt s_r_opt = {
46564656
.allow_exclude_promisor_objects = 1,
@@ -4660,7 +4660,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av)
46604660
int save_warning;
46614661

46624662
save_commit_buffer = 0;
4663-
setup_revisions(ac, av, revs, &s_r_opt);
4663+
setup_revisions_from_strvec(argv, revs, &s_r_opt);
46644664

46654665
/* make sure shallows are read */
46664666
is_repository_shallow(the_repository);
@@ -5229,7 +5229,7 @@ int cmd_pack_objects(int argc,
52295229
revs.include_check = is_not_in_promisor_pack;
52305230
revs.include_check_obj = is_not_in_promisor_pack_obj;
52315231
}
5232-
get_object_list(&revs, rp.nr, rp.v);
5232+
get_object_list(&revs, &rp);
52335233
release_revisions(&revs);
52345234
}
52355235
cleanup_preferred_base();

‎builtin/rebase.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
299299
oid_to_hex(&opts->restrict_revision->object.oid));
300300

301301
ret = sequencer_make_script(the_repository, &todo_list.buf,
302-
make_script_args.nr, make_script_args.v,
303-
flags);
302+
&make_script_args, flags);
304303
if (ret) {
305304
error(_("could not generate todo list"));
306305
goto cleanup;

‎builtin/stash.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,8 +1015,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,
10151015
}
10161016
}
10171017

1018-
argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
1019-
if (argc > 1)
1018+
setup_revisions_from_strvec(&revision_args, &rev, NULL);
1019+
if (revision_args.nr > 1)
10201020
goto usage;
10211021
if (!rev.diffopt.output_format) {
10221022
rev.diffopt.output_format = DIFF_FORMAT_PATCH;

‎builtin/submodule--helper.c‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -616,9 +616,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
616616
struct rev_info rev = REV_INFO_INIT;
617617
struct strbuf buf = STRBUF_INIT;
618618
const char *git_dir;
619-
struct setup_revision_opt opt = {
620-
.free_removed_argv_elements = 1,
621-
};
622619

623620
if (validate_submodule_path(path) < 0)
624621
die(NULL);
@@ -655,7 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
655652

656653
repo_init_revisions(the_repository, &rev, NULL);
657654
rev.abbrev = 0;
658-
setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
655+
setup_revisions_from_strvec(&diff_files_args, &rev, NULL);
659656
run_diff_files(&rev, 0);
660657

661658
if (!diff_result_code(&rev)) {
@@ -1094,9 +1091,6 @@ static int compute_summary_module_list(struct object_id *head_oid,
10941091
{
10951092
struct strvec diff_args = STRVEC_INIT;
10961093
struct rev_info rev;
1097-
struct setup_revision_opt opt = {
1098-
.free_removed_argv_elements = 1,
1099-
};
11001094
struct module_cb_list list = MODULE_CB_LIST_INIT;
11011095
int ret = 0;
11021096

@@ -1114,7 +1108,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
11141108
repo_init_revisions(the_repository, &rev, info->prefix);
11151109
rev.abbrev = 0;
11161110
precompose_argv_prefix(diff_args.nr, diff_args.v, NULL);
1117-
setup_revisions(diff_args.nr, diff_args.v, &rev, &opt);
1111+
setup_revisions_from_strvec(&diff_args, &rev, NULL);
11181112
rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;
11191113
rev.diffopt.format_callback = submodule_summary_callback;
11201114
rev.diffopt.format_callback_data = &list;

‎http-push.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1941,7 +1941,7 @@ int cmd_main(int argc, const char **argv)
19411941
strvec_pushf(&commit_argv, "^%s",
19421942
oid_to_hex(&ref->old_oid));
19431943
repo_init_revisions(the_repository, &revs, setup_git_directory());
1944-
setup_revisions(commit_argv.nr, commit_argv.v, &revs, NULL);
1944+
setup_revisions_from_strvec(&commit_argv, &revs, NULL);
19451945
revs.edge_hint = 0; /* just in case */
19461946

19471947
/* Generate a list of objects that need to be pushed */

‎remote.c‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2143,9 +2143,6 @@ static int stat_branch_pair(const char *branch_name, const char *base,
21432143
struct object_id oid;
21442144
struct commit *ours, *theirs;
21452145
struct rev_info revs;
2146-
struct setup_revision_opt opt = {
2147-
.free_removed_argv_elements = 1,
2148-
};
21492146
struct strvec argv = STRVEC_INIT;
21502147

21512148
/* Cannot stat if what we used to build on no longer exists */
@@ -2180,7 +2177,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,
21802177
strvec_push(&argv, "--");
21812178

21822179
repo_init_revisions(the_repository, &revs, NULL);
2183-
setup_revisions(argv.nr, argv.v, &revs, &opt);
2180+
setup_revisions_from_strvec(&argv, &revs, NULL);
21842181
if (prepare_revision_walk(&revs))
21852182
die(_("revision walk setup failed"));
21862183

‎revision.c‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2321,6 +2321,24 @@ static timestamp_t parse_age(const char *arg)
23212321
return num;
23222322
}
23232323

2324+
static void overwrite_argv(int *argc, const char **argv,
2325+
const char **value,
2326+
const struct setup_revision_opt *opt)
2327+
{
2328+
/*
2329+
* Detect the case when we are overwriting ourselves. The assignment
2330+
* itself would be a noop either way, but this lets us avoid corner
2331+
* cases around the free() and NULL operations.
2332+
*/
2333+
if (*value != argv[*argc]) {
2334+
if (opt && opt->free_removed_argv_elements)
2335+
free((char *)argv[*argc]);
2336+
argv[*argc] = *value;
2337+
*value = NULL;
2338+
}
2339+
(*argc)++;
2340+
}
2341+
23242342
static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
23252343
int *unkc, const char **unkv,
23262344
const struct setup_revision_opt* opt)
@@ -2342,7 +2360,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
23422360
starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
23432361
starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
23442362
{
2345-
unkv[(*unkc)++] = arg;
2363+
overwrite_argv(unkc, unkv, &argv[0], opt);
23462364
return 1;
23472365
}
23482366

@@ -2706,7 +2724,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
27062724
} else {
27072725
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
27082726
if (!opts)
2709-
unkv[(*unkc)++] = arg;
2727+
overwrite_argv(unkc, unkv, &argv[0], opt);
27102728
return opts;
27112729
}
27122730

@@ -3018,7 +3036,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
30183036

30193037
if (!strcmp(arg, "--stdin")) {
30203038
if (revs->disable_stdin) {
3021-
argv[left++] = arg;
3039+
overwrite_argv(&left, argv, &argv[i], opt);
30223040
continue;
30233041
}
30243042
if (revs->read_from_stdin++)
@@ -3174,9 +3192,34 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
31743192
revs->show_notes_given = 1;
31753193
}
31763194

3195+
if (argv) {
3196+
if (opt && opt->free_removed_argv_elements)
3197+
free((char *)argv[left]);
3198+
argv[left] = NULL;
3199+
}
3200+
31773201
return left;
31783202
}
31793203

3204+
void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
3205+
struct setup_revision_opt *opt)
3206+
{
3207+
struct setup_revision_opt fallback_opt;
3208+
int ret;
3209+
3210+
if (!opt) {
3211+
memset(&fallback_opt, 0, sizeof(fallback_opt));
3212+
opt = &fallback_opt;
3213+
}
3214+
opt->free_removed_argv_elements = 1;
3215+
3216+
ret = setup_revisions(argv->nr, argv->v, revs, opt);
3217+
3218+
for (size_t i = ret; i < argv->nr; i++)
3219+
free((char *)argv->v[i]);
3220+
argv->nr = ret;
3221+
}
3222+
31803223
static void release_revisions_cmdline(struct rev_cmdline_info *cmdline)
31813224
{
31823225
unsigned int i;

‎revision.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,8 @@ struct setup_revision_opt {
441441
};
442442
int setup_revisions(int argc, const char **argv, struct rev_info *revs,
443443
struct setup_revision_opt *);
444+
void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
445+
struct setup_revision_opt *);
444446

445447
/**
446448
* Free data allocated in a "struct rev_info" after it's been

0 commit comments

Comments
 (0)