From 9a9592ff7c8a8f0e449515c158e4a5a4895c5c23 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 22 Mar 2015 06:00:01 -0400 Subject: wt-status: don't flush before running "submodule status" This is a holdover from the original implementation in ac8d5af (builtin-status: submodule summary support, 2008-04-12), which just had the sub-process output to our descriptor; we had to make sure we had flushed any data that we produced before it started writing. Since 3ba7407 (submodule summary: ignore --for-status option, 2013-09-06), however, we pipe the sub-process output back to ourselves. So there's no longer any need to flush (it does not hurt, but it may leave readers wondering why we do it). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wt-status.c | 1 - 1 file changed, 1 deletion(-) (limited to 'wt-status.c') diff --git a/wt-status.c b/wt-status.c index 29666d0db..08d40d202 100644 --- a/wt-status.c +++ b/wt-status.c @@ -745,7 +745,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt sm_summary.git_cmd = 1; sm_summary.no_stdin = 1; - fflush(s->fp); sm_summary.out = -1; run_command(&sm_summary); -- cgit v1.2.1 From d56d966b3b03d2849ef9e20cacd7965106e8fdf0 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 22 Mar 2015 06:00:32 -0400 Subject: wt_status: fix signedness mismatch in strbuf_read call We call strbuf_read(), and want to know whether we got any output. To do so, we assign the result to a size_t, and check whether it is non-zero. But strbuf_read returns a signed ssize_t. If it encounters an error, it will return -1, and we'll end up treating this the same as if we had gotten output. Instead, we can just check whether our buffer has anything in it (which is what we care about anyway, and is the same thing since we know the buffer was empty to begin with). Note that the "len" variable actually has two roles in this function. Now that we've eliminated the first, we can push the declaration closer to the point of use for the second one. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wt-status.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'wt-status.c') diff --git a/wt-status.c b/wt-status.c index 08d40d202..05b69dc4d 100644 --- a/wt-status.c +++ b/wt-status.c @@ -729,7 +729,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt struct strbuf cmd_stdout = STRBUF_INIT; struct strbuf summary = STRBUF_INIT; char *summary_content; - size_t len; argv_array_pushf(&sm_summary.env_array, "GIT_INDEX_FILE=%s", s->index_file); @@ -749,10 +748,10 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt run_command(&sm_summary); - len = strbuf_read(&cmd_stdout, sm_summary.out, 1024); + strbuf_read(&cmd_stdout, sm_summary.out, 1024); /* prepend header, only if there's an actual output */ - if (len) { + if (cmd_stdout.len) { if (uncommitted) strbuf_addstr(&summary, _("Submodules changed but not updated:")); else @@ -763,6 +762,7 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt strbuf_release(&cmd_stdout); if (s->display_comment_prefix) { + size_t len; summary_content = strbuf_detach(&summary, &len); strbuf_add_commented_lines(&summary, summary_content, len); free(summary_content); -- cgit v1.2.1 From 5c950e9bf098b17bb37e06f7c9f50d24e9d2904f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 22 Mar 2015 23:53:52 -0400 Subject: wt-status: use capture_command When we spawn "git submodule status" to read its output, we use run_command() followed by strbuf_read() read from the pipe. This can deadlock if the subprocess output is larger than the system pipe buffer. Furthermore, if start_command() fails, we'll try to read from a bogus descriptor (probably "-1" or a descriptor we just closed, but it is a bad idea for us to make assumptions about how start_command implements its error handling). And if start_command succeeds, we leak the file descriptor for the pipe to the child. All of these can be solved by using the capture_command helper. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- wt-status.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'wt-status.c') diff --git a/wt-status.c b/wt-status.c index 05b69dc4d..ef232a74b 100644 --- a/wt-status.c +++ b/wt-status.c @@ -744,11 +744,8 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt sm_summary.git_cmd = 1; sm_summary.no_stdin = 1; - sm_summary.out = -1; - run_command(&sm_summary); - - strbuf_read(&cmd_stdout, sm_summary.out, 1024); + capture_command(&sm_summary, &cmd_stdout, 1024); /* prepend header, only if there's an actual output */ if (cmd_stdout.len) { -- cgit v1.2.1