From 5eb7f7ead8537420fde3eb344640096fc330bc16 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 15 May 2014 04:33:40 -0400 Subject: [PATCH] run_column_filter: use argv_array We currently set up the argv array by hand in a fixed-size stack-local array. Using an argv array is more readable, as it handles buffer allocation us (not to mention makes it obvious we do not overflow the array). However, there's a more subtle benefit, too. We leave the function having run start_command (with the child_process in a static global), and then later run finish_command from another function. That means when we run finish_command, neither column_process.argv nor the memory it points to is valid any longer. Most of the time finish_command does not bother looking at argv, but it may if it encounters an error (e.g., waitpid failure or signal death). This is unusual, which is why nobody has noticed. But by using run-command's built-in argv_array, the memory ownership is handled for us automatically. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- column.c | 43 +++++++++++++------------------------------ 1 file changed, 13 insertions(+), 30 deletions(-) diff --git a/column.c b/column.c index 8d1ce88d14..1a468debb4 100644 --- a/column.c +++ b/column.c @@ -370,46 +370,29 @@ static struct child_process column_process; int run_column_filter(int colopts, const struct column_options *opts) { - const char *av[10]; - int ret, ac = 0; - struct strbuf sb_colopt = STRBUF_INIT; - struct strbuf sb_width = STRBUF_INIT; - struct strbuf sb_padding = STRBUF_INIT; + struct argv_array *argv; if (fd_out != -1) return -1; - av[ac++] = "column"; - strbuf_addf(&sb_colopt, "--raw-mode=%d", colopts); - av[ac++] = sb_colopt.buf; - if (opts && opts->width) { - strbuf_addf(&sb_width, "--width=%d", opts->width); - av[ac++] = sb_width.buf; - } - if (opts && opts->indent) { - av[ac++] = "--indent"; - av[ac++] = opts->indent; - } - if (opts && opts->padding) { - strbuf_addf(&sb_padding, "--padding=%d", opts->padding); - av[ac++] = sb_padding.buf; - } - av[ac] = NULL; + memset(&column_process, 0, sizeof(column_process)); + argv = &column_process.args; + + argv_array_push(argv, "column"); + argv_array_pushf(argv, "--raw-mode=%d", colopts); + if (opts && opts->width) + argv_array_pushf(argv, "--width=%d", opts->width); + if (opts && opts->indent) + argv_array_pushf(argv, "--indent=%s", opts->indent); + if (opts && opts->padding) + argv_array_pushf(argv, "--padding=%d", opts->padding); fflush(stdout); - memset(&column_process, 0, sizeof(column_process)); column_process.in = -1; column_process.out = dup(1); column_process.git_cmd = 1; - column_process.argv = av; - ret = start_command(&column_process); - - strbuf_release(&sb_colopt); - strbuf_release(&sb_width); - strbuf_release(&sb_padding); - - if (ret) + if (start_command(&column_process)) return -2; fd_out = dup(1);