Skip to content

Commit

Permalink
run_column_filter: use argv_array
Browse files Browse the repository at this point in the history
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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
peff authored and gitster committed May 15, 2014
1 parent c460c0e commit 5eb7f7e
Showing 1 changed file with 13 additions and 30 deletions.
43 changes: 13 additions & 30 deletions column.c
Expand Up @@ -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);
Expand Down

0 comments on commit 5eb7f7e

Please sign in to comment.