From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"René Scharfe" <l.s.r@web.de>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH 03/10] run-command API: add and use a run_command_l_opt()
Date: Fri, 14 Oct 2022 17:40:15 +0200 [thread overview]
Message-ID: <patch-03.10-fd81d44f221-20221014T153426Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com>
When run_command() is used with strvec_pushl() as in the pre-image of
b004c902827 (gc: simplify maintenance_task_pack_refs(), 2022-10-04) we
get the benefit of the LAST_ARG_MUST_BE_NULL. See 9fe3edc47f1 (Add the
LAST_ARG_MUST_BE_NULL macro, 2013-07-18).
Let's provide and use a run_command_l_opt() function, which gives us
the best of both worlds. We'll now need less code to accomplish the
same thing, and this version's safer as we can assert that the
terminating NULL is present.
The "builtin/bisect--helper.c" would be a large beneficiary of this
API, with "15 insertions(+), 26 deletions(-)", but let's leave that
aside for now, as there's an outstanding topic that's extensively
modifying it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
bisect.c | 19 +++++++++----------
builtin/clone.c | 16 ++++++----------
builtin/difftool.c | 14 ++++++--------
builtin/gc.c | 22 ++++++++--------------
builtin/merge.c | 35 ++++++-----------------------------
builtin/remote.c | 10 ++++------
compat/mingw.c | 8 +++-----
ll-merge.c | 4 +---
run-command.c | 15 +++++++++++++++
run-command.h | 13 +++++++++++--
sequencer.c | 4 +---
t/helper/test-fake-ssh.c | 4 +---
12 files changed, 71 insertions(+), 93 deletions(-)
diff --git a/bisect.c b/bisect.c
index fd581b85a72..415b0e7b0b2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -738,18 +738,17 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
argv_checkout[2] = bisect_rev_hex;
- if (no_checkout) {
+ if (no_checkout)
update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
- } else {
- if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
- /*
- * Errors in `run_command()` itself, signaled by res < 0,
- * and errors in the child process, signaled by res > 0
- * can both be treated as regular BISECT_FAILED (-1).
- */
- return BISECT_FAILED;
- }
+ else if (run_command_l_opt(RUN_GIT_CMD, "checkout", "-q",
+ bisect_rev_hex, "--", NULL))
+ /*
+ * Errors in `run_command()` itself, signaled by res < 0,
+ * and errors in the child process, signaled by res > 0
+ * can both be treated as regular BISECT_FAILED (-1).
+ */
+ return BISECT_FAILED;
commit = lookup_commit_reference(the_repository, bisect_rev);
format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6ab..13fc7a4bc5d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,23 +651,19 @@ static void update_head(const struct ref *our, const struct ref *remote,
static int git_sparse_checkout_init(const char *repo)
{
- struct strvec argv = STRVEC_INIT;
- int result = 0;
- strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
-
/*
* We must apply the setting in the current process
* for the later checkout to use the sparse-checkout file.
*/
core_apply_sparse_checkout = 1;
- if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+ if (run_command_l_opt(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
+ "set", NULL)) {
error(_("failed to initialize sparse-checkout"));
- result = 1;
+ return 1;
}
- strvec_clear(&argv);
- return result;
+ return 0;
}
static int checkout(int submodule_progress, int filter_submodules)
@@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
static void dissociate_from_references(void)
{
- static const char* argv[] = { "repack", "-a", "-d", NULL };
char *alternates = git_pathdup("objects/info/alternates");
if (!access(alternates, F_OK)) {
- if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
+ if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
+ "repack", "-a", "-d", NULL))
die(_("cannot repack to clean up"));
if (unlink(alternates) && errno != ENOENT)
die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..ed211a87322 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,8 @@ static int difftool_config(const char *var, const char *value, void *cb)
static int print_tool_help(void)
{
- const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
- return run_command_v_opt(argv, RUN_GIT_CMD);
+ return run_command_l_opt(RUN_GIT_CMD, "mergetool", "--tool-help=diff",
+ NULL);
}
static int parse_index_info(char *p, int *mode1, int *mode2,
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
struct index_state wtindex;
struct checkout lstate, rstate;
int flags = RUN_GIT_CMD, err = 0;
- const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+ const char *helper_command = "difftool--helper";
struct hashmap wt_modified, tmp_modified;
int indices_loaded = 0;
@@ -563,16 +563,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
strbuf_setlen(&ldir, ldir_len);
- helper_argv[1] = ldir.buf;
strbuf_setlen(&rdir, rdir_len);
- helper_argv[2] = rdir.buf;
-
if (extcmd) {
- helper_argv[0] = extcmd;
+ helper_command = extcmd;
flags = 0;
} else
setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
- ret = run_command_v_opt(helper_argv, flags);
+ ret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,
+ NULL);
/* TODO: audit for interaction with sparse-index. */
ensure_full_index(&wtindex);
diff --git a/builtin/gc.c b/builtin/gc.c
index 243ee85d283..075b4637660 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,6 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
static struct strvec reflog = STRVEC_INIT;
static struct strvec repack = STRVEC_INIT;
static struct strvec prune = STRVEC_INIT;
-static struct strvec prune_worktrees = STRVEC_INIT;
-static struct strvec rerere = STRVEC_INIT;
static struct tempfile *pidfile;
static struct lock_file log_lock;
@@ -167,9 +165,8 @@ static void gc_config(void)
struct maintenance_run_opts;
static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
{
- const char *argv[] = { "pack-refs", "--all", "--prune", NULL };
-
- return run_command_v_opt(argv, RUN_GIT_CMD);
+ return run_command_l_opt(RUN_GIT_CMD, "pack-refs", "--all", "--prune",
+ NULL);
}
static int too_many_loose_objects(void)
@@ -574,8 +571,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
strvec_pushl(&repack, "repack", "-d", "-l", NULL);
strvec_pushl(&prune, "prune", "--expire", NULL);
- strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
- strvec_pushl(&rerere, "rerere", "gc", NULL);
/* default expiry time, overwritten in gc_config */
gc_config();
@@ -688,14 +683,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
}
}
- if (prune_worktrees_expire) {
- strvec_push(&prune_worktrees, prune_worktrees_expire);
- if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
- die(FAILED_RUN, prune_worktrees.v[0]);
- }
+ if (prune_worktrees_expire &&
+ run_command_l_opt(RUN_GIT_CMD, "worktree", "prune", "--expire",
+ prune_worktrees_expire, NULL))
+ die(FAILED_RUN, "worktree");
- if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
- die(FAILED_RUN, rerere.v[0]);
+ if (run_command_l_opt(RUN_GIT_CMD, "rerere", "gc", NULL))
+ die(FAILED_RUN, "rerere");
report_garbage = report_pack_garbage;
reprepare_packed_git(the_repository);
diff --git a/builtin/merge.c b/builtin/merge.c
index 3bb49d805b4..9c04b588f68 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,55 +347,32 @@ static int save_state(struct object_id *stash)
static void read_empty(const struct object_id *oid)
{
- int i = 0;
- const char *args[7];
-
- args[i++] = "read-tree";
- args[i++] = "-m";
- args[i++] = "-u";
- args[i++] = empty_tree_oid_hex();
- args[i++] = oid_to_hex(oid);
- args[i] = NULL;
-
- if (run_command_v_opt(args, RUN_GIT_CMD))
+ if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-m", "-u",
+ empty_tree_oid_hex(), oid_to_hex(oid), NULL))
die(_("read-tree failed"));
}
static void reset_hard(const struct object_id *oid)
{
- int i = 0;
- const char *args[6];
-
- args[i++] = "read-tree";
- args[i++] = "-v";
- args[i++] = "--reset";
- args[i++] = "-u";
- args[i++] = oid_to_hex(oid);
- args[i] = NULL;
-
- if (run_command_v_opt(args, RUN_GIT_CMD))
+ if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-v", "--reset", "-u",
+ oid_to_hex(oid), NULL))
die(_("read-tree failed"));
}
static void restore_state(const struct object_id *head,
const struct object_id *stash)
{
- struct strvec args = STRVEC_INIT;
-
reset_hard(head);
if (is_null_oid(stash))
goto refresh_cache;
- strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
- strvec_push(&args, oid_to_hex(stash));
-
/*
* It is OK to ignore error here, for example when there was
* nothing to restore.
*/
- run_command_v_opt(args.v, RUN_GIT_CMD);
- strvec_clear(&args);
+ run_command_l_opt(RUN_GIT_CMD, "stash", "apply", "--index", "--quiet",
+ oid_to_hex(stash), NULL);
refresh_cache:
if (discard_cache() < 0 || read_cache() < 0)
diff --git a/builtin/remote.c b/builtin/remote.c
index 910f7b9316a..1d86c14297b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,11 @@ static int verbose;
static int fetch_remote(const char *name)
{
- const char *argv[] = { "fetch", name, NULL, NULL };
- if (verbose) {
- argv[1] = "-v";
- argv[2] = name;
- }
printf_ln(_("Updating %s"), name);
- if (run_command_v_opt(argv, RUN_GIT_CMD))
+ if (verbose && run_command_l_opt(RUN_GIT_CMD, "-v", "fetch", name,
+ NULL))
+ return error(_("Could not fetch %s"), name);
+ else if (run_command_l_opt(RUN_GIT_CMD, "fetch", name, NULL))
return error(_("Could not fetch %s"), name);
return 0;
}
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d5841..4f5392c5796 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,17 +196,15 @@ static int read_yes_no_answer(void)
static int ask_yes_no_if_possible(const char *format, ...)
{
char question[4096];
- const char *retry_hook[] = { NULL, NULL, NULL };
+ char *retry_hook;
va_list args;
va_start(args, format);
vsnprintf(question, sizeof(question), format, args);
va_end(args);
- if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
- retry_hook[1] = question;
- return !run_command_v_opt(retry_hook, 0);
- }
+ if ((retry_hook = mingw_getenv("GIT_ASK_YESNO")))
+ return !run_command_l_opt(0, retry_hook, question, NULL);
if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
return 0;
diff --git a/ll-merge.c b/ll-merge.c
index 8955d7e1f6e..740689b7bd6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
struct strbuf cmd = STRBUF_INIT;
struct strbuf_expand_dict_entry dict[6];
struct strbuf path_sq = STRBUF_INIT;
- const char *args[] = { NULL, NULL };
int status, fd, i;
struct stat st;
enum ll_merge_result ret;
@@ -219,8 +218,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
- args[0] = cmd.buf;
- status = run_command_v_opt(args, RUN_USING_SHELL);
+ status = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);
fd = open(temp[1], O_RDONLY);
if (fd < 0)
goto bad;
diff --git a/run-command.c b/run-command.c
index cf4a431c839..0be5626f7f2 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1016,6 +1016,21 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
}
+int run_command_l_opt(int opt, ...)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ va_list ap;
+ const char *arg;
+
+ va_start(ap, opt);
+ while ((arg = va_arg(ap, const char *)))
+ strvec_push(&cmd.args, arg);
+ va_end(ap);
+
+ run_command_set_opts(&cmd, opt);
+ return run_command(&cmd);
+}
+
int run_command_v_opt(const char **argv, int opt)
{
return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
diff --git a/run-command.h b/run-command.h
index 0e85e5846a5..6320d70f062 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,8 +151,8 @@ struct child_process {
/**
* The functions: child_process_init, start_command, finish_command,
- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear
- * do the following:
+ * run_command, run_command_l_opt, run_command_v_opt,
+ * run_command_v_opt_cd_env, child_process_clear do the following:
*
* - If a system call failed, errno is set and -1 is returned. A diagnostic
* is printed.
@@ -254,6 +254,15 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
const char *const *env, const char *tr2_class);
+/**
+ * The run_command_l_opt() function run_command_v_opt() takes a list
+ * of strings terminated by a NULL. Use it instead of
+ * run_command_v_opt() when possible, as it allows the compiler to
+ * check that the "argv" is NULL-delimited.
+ */
+LAST_ARG_MUST_BE_NULL
+int run_command_l_opt(int opt, ...);
+
/**
* Execute the given command, sending "in" to its stdin, and capturing its
* stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/sequencer.c b/sequencer.c
index debb2ecbafe..20495db9de2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3558,12 +3558,10 @@ static int error_failed_squash(struct repository *r,
static int do_exec(struct repository *r, const char *command_line)
{
- const char *child_argv[] = { NULL, NULL };
int dirty, status;
fprintf(stderr, _("Executing: %s\n"), command_line);
- child_argv[0] = command_line;
- status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+ status = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);
/* force re-reading of the cache */
if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad2..7967478a40a 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,6 @@ int cmd_main(int argc, const char **argv)
struct strbuf buf = STRBUF_INIT;
FILE *f;
int i;
- const char *child_argv[] = { NULL, NULL };
/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
if (!trash_directory)
@@ -25,6 +24,5 @@ int cmd_main(int argc, const char **argv)
/* Now, evaluate the *last* parameter */
if (argc < 2)
return 0;
- child_argv[0] = argv[argc - 1];
- return run_command_v_opt(child_argv, RUN_USING_SHELL);
+ return run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);
}
--
2.38.0.1092.g8c0298861b0
next prev parent reply other threads:[~2022-10-14 15:40 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-14 15:40 [PATCH 00/10] run-command API: add run_command_{l,sv}_opt() Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers Ævar Arnfjörð Bjarmason
2022-10-14 18:22 ` Junio C Hamano
2022-10-14 15:40 ` [PATCH 02/10] merge: remove always-the-same "verbose" arguments Ævar Arnfjörð Bjarmason
2022-10-14 18:31 ` Junio C Hamano
2022-10-14 15:40 ` Ævar Arnfjörð Bjarmason [this message]
2022-10-14 18:40 ` [PATCH 03/10] run-command API: add and use a run_command_l_opt() Junio C Hamano
2022-10-14 20:03 ` Jeff King
2022-10-14 20:27 ` Junio C Hamano
2022-10-14 15:40 ` [PATCH 04/10] am: use run_command_l_opt() for show_patch() Ævar Arnfjörð Bjarmason
2022-10-14 18:43 ` Junio C Hamano
2022-10-14 15:40 ` [PATCH 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env() Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 08/10] run-command API & users: remove run_command_v_opt_tr2() Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach() Ævar Arnfjörð Bjarmason
2022-10-14 15:40 ` [PATCH 10/10] run-command API: add and use a run_command_sv_opt() Ævar Arnfjörð Bjarmason
2022-10-14 19:21 ` René Scharfe
2022-10-17 17:49 ` [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 02/10] merge: remove always-the-same "verbose" arguments Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 03/10] run-command API: add and use a run_command_l_opt() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 04/10] am: use run_command_l_opt() for show_patch() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 08/10] run-command API & users: remove run_command_v_opt_tr2() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach() Ævar Arnfjörð Bjarmason
2022-10-17 17:49 ` [PATCH v2 10/10] run-command API: add and use a run_command_sv_opt() Ævar Arnfjörð Bjarmason
2022-10-18 9:11 ` [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt() Junio C Hamano
2022-10-18 13:28 ` Ævar Arnfjörð Bjarmason
2022-10-18 20:42 ` Jeff King
2022-10-19 15:43 ` Junio C Hamano
2022-10-19 17:06 ` Jeff King
2022-10-19 18:00 ` Junio C Hamano
2022-10-19 18:57 ` Jeff King
2022-10-19 19:41 ` Junio C Hamano
2022-10-20 18:34 ` René Scharfe
2022-10-20 23:50 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=patch-03.10-fd81d44f221-20221014T153426Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).