revision: add wrapper to setup_revisions() from a strvec

The setup_revisions() function was designed to take the argc/argv pair
from the operating system. But we sometimes construct our own argv using
a strvec and pass that in. There are a few gotchas that callers need to
deal with here:

  1. You should always pass the free_removed_argv_elements option via
     setup_revision_opt. Otherwise, entries may be leaked if
     setup_revisions() re-shuffles options.

  2. After setup_revisions() returns, the strvec state is odd. We get a
     reduced argc from setup_revisions() telling us how many unknown
     options were left in place. Entries after that in argv may be
     retained, or may be NULL (depending on how the reshuffling
     happened). But the strvec's "nr" field still represents the
     original value, and some of the entries it thinks it is still
     storing may be NULL. Callers must be careful with how they access
     it.

Some callers deal with (1), but not all. In practice they are OK because
they do not pass any options that would cause setup_revisions() to
re-shuffle (namely unknown options which may be relayed from the user,
and the use of the "--" separator). But it's probably a good idea to
consistently pass this option anyway to future-proof ourselves against
the details of setup_revisions() changing.

No callers address (2), though I don't think there any visible bugs.
Most of them simply call strvec_clear() and never otherwise look at the
result. And in fact, if they naively set foo.nr to the argc returned by
setup_revisions(), that would cause leaks!  Because setup_revisions()
does not free consumed options[1], we have to leave the "nr" field of
the strvec at its original value to find and free them during
strvec_clear().

So I don't think there are any bugs to fix here, but we can make things
safer and simpler for callers. Let's introduce a helper function that
sets the free_removed_argv_elements automatically and shrinks the strvec
to represent the retained options afterwards (taking care to free the
now-obsolete entries).

We'll start by converting all of the call-sites which use the
free_removed_argv_elements option. There should be no behavior change
for them, except that their "shrunken" entries are cleaned up
immediately, rather than waiting for a strvec_clear() call.

[1] Arguably setup_revisions() should be doing this step for us if we
    told it to free removed options, but there are many existing callers
    which will be broken if it did. Introducing this helper is a
    possible first step towards that.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/bisect.c b/bisect.c
index f24474542ec3b16a4bc3379f7eb4eb1e3e83057e..a6dc76b15c910b9963b556b66aa3d2c63834bed9 100644 (file)
--- a/bisect.c
+++ b/bisect.c
@@ -674,9 +674,6 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
                             const char *bad_format, const char *good_format,
                             int read_paths)
 {
-       struct setup_revision_opt opt = {
-               .free_removed_argv_elements = 1,
-       };
        int i;
 
        repo_init_revisions(r, revs, prefix);
@@ -693,7 +690,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
        if (read_paths)
                read_bisect_paths(rev_argv);
 
-       setup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);
+       setup_revisions_from_strvec(rev_argv, revs, NULL);
 }
 
 static void bisect_common(struct rev_info *revs)

diff --git a/builtin/stash.c b/builtin/stash.c
index 01751ce28d625dfc558e25ed260244c5ef38d6af..3a89d9b7f3b36681ae10134f8ecf9aab5aa5691e 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -956,7 +956,6 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
 static int show_stash(int argc, const char **argv, const char *prefix,
                      struct repository *repo UNUSED)
 {
-       struct setup_revision_opt opt = { .free_removed_argv_elements = 1 };
        int i;
        int ret = -1;
        struct stash_info info = STASH_INFO_INIT;
@@ -1015,8 +1014,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,
                }
        }
 
-       argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);
-       if (argc > 1)
+       setup_revisions_from_strvec(&revision_args, &rev, NULL);
+       if (revision_args.nr > 1)
                goto usage;
        if (!rev.diffopt.output_format) {
                rev.diffopt.output_format = DIFF_FORMAT_PATCH;

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 07a1935cbe1a69d813402430967213d03ab8a128..fcd73abe5336a9f8476087de44c1ee7f892effdd 100644 (file)
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -616,9 +616,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
        struct rev_info rev = REV_INFO_INIT;
        struct strbuf buf = STRBUF_INIT;
        const char *git_dir;
-       struct setup_revision_opt opt = {
-               .free_removed_argv_elements = 1,
-       };
 
        if (validate_submodule_path(path) < 0)
                die(NULL);
@@ -655,7 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 
        repo_init_revisions(the_repository, &rev, NULL);
        rev.abbrev = 0;
-       setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
+       setup_revisions_from_strvec(&diff_files_args, &rev, NULL);
        run_diff_files(&rev, 0);
 
        if (!diff_result_code(&rev)) {
@@ -1094,9 +1091,6 @@ static int compute_summary_module_list(struct object_id *head_oid,
 {
        struct strvec diff_args = STRVEC_INIT;
        struct rev_info rev;
-       struct setup_revision_opt opt = {
-               .free_removed_argv_elements = 1,
-       };
        struct module_cb_list list = MODULE_CB_LIST_INIT;
        int ret = 0;
 
@@ -1114,7 +1108,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
        repo_init_revisions(the_repository, &rev, info->prefix);
        rev.abbrev = 0;
        precompose_argv_prefix(diff_args.nr, diff_args.v, NULL);
-       setup_revisions(diff_args.nr, diff_args.v, &rev, &opt);
+       setup_revisions_from_strvec(&diff_args, &rev, NULL);
        rev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;
        rev.diffopt.format_callback = submodule_summary_callback;
        rev.diffopt.format_callback_data = &list;

diff --git a/remote.c b/remote.c
index 88f991795b2683cd5267c7bd24d374fc473b0184..929c6887ce066e007b3452748c4ac393ec89584f 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -2137,9 +2137,6 @@ static int stat_branch_pair(const char *branch_name, const char *base,
        struct object_id oid;
        struct commit *ours, *theirs;
        struct rev_info revs;
-       struct setup_revision_opt opt = {
-               .free_removed_argv_elements = 1,
-       };
        struct strvec argv = STRVEC_INIT;
 
        /* Cannot stat if what we used to build on no longer exists */
@@ -2174,7 +2171,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,
        strvec_push(&argv, "--");
 
        repo_init_revisions(the_repository, &revs, NULL);
-       setup_revisions(argv.nr, argv.v, &revs, &opt);
+       setup_revisions_from_strvec(&argv, &revs, NULL);
        if (prepare_revision_walk(&revs))
                die(_("revision walk setup failed"));
 

diff --git a/revision.c b/revision.c
index 335f77fa98c24b7223e658d9167fc7a716c8c062..d4788aedab8d6eed06ca2d87363148fa69420b5a 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -3178,6 +3178,25 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
        return left;
 }
 
+void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
+                                struct setup_revision_opt *opt)
+{
+       struct setup_revision_opt fallback_opt;
+       int ret;
+
+       if (!opt) {
+               memset(&fallback_opt, 0, sizeof(fallback_opt));
+               opt = &fallback_opt;
+       }
+       opt->free_removed_argv_elements = 1;
+
+       ret = setup_revisions(argv->nr, argv->v, revs, opt);
+
+       for (size_t i = ret; i < argv->nr; i++)
+               free((char *)argv->v[i]);
+       argv->nr = ret;
+}
+
 static void release_revisions_cmdline(struct rev_cmdline_info *cmdline)
 {
        unsigned int i;

diff --git a/revision.h b/revision.h
index 21e288c5baa2b5c0f340f66fe6b31fff1b1d7d7d..a28e349044b8d1eb25813f78ef19f3d53ec903c6 100644 (file)
--- a/revision.h
+++ b/revision.h
@@ -441,6 +441,8 @@ struct setup_revision_opt {
 };
 int setup_revisions(int argc, const char **argv, struct rev_info *revs,
                    struct setup_revision_opt *);
+void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
+                                struct setup_revision_opt *);
 
 /**
  * Free data allocated in a "struct rev_info" after it's been