From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 25 Mar 2015 19:54:27 +0000 (-0700)
Subject: Merge branch 'jk/run-command-capture'
X-Git-Tag: v2.4.0-rc0~6
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/ea1fd481b4e689f143142662a82fb62c9b2efb65?ds=inline;hp=-c

Merge branch 'jk/run-command-capture'

The run-command interface was easy to abuse and make a pipe for us
to read from the process, wait for the process to finish and then
attempt to read its output, which is a pattern that lead to a
deadlock.  Fix such uses by introducing a helper to do this
correctly (i.e. we need to read first and then wait the process to
finish) and also add code to prevent such abuse in the run-command
helper.

* jk/run-command-capture:
run-command: forbid using run_command with piped output
trailer: use capture_command
submodule: use capture_command
wt-status: use capture_command
run-command: introduce capture_command helper
wt_status: fix signedness mismatch in strbuf_read call
wt-status: don't flush before running "submodule status"
---

ea1fd481b4e689f143142662a82fb62c9b2efb65
diff --combined run-command.c
index 3afb124c79,4184e8d9f3..aad03ab705
--- a/run-command.c
+++ b/run-command.c
@@@ -4,6 -4,10 +4,6 @@@
  #include "sigchain.h"
  #include "argv-array.h"
  
 -#ifndef SHELL_PATH
 -# define SHELL_PATH "/bin/sh"
 -#endif
 -
  void child_process_init(struct child_process *child)
  {
  	memset(child, 0, sizeof(*child));
@@@ -557,7 -561,12 +557,12 @@@ int finish_command(struct child_proces
  
  int run_command(struct child_process *cmd)
  {
- 	int code = start_command(cmd);
+ 	int code;
+ 
+ 	if (cmd->out < 0 || cmd->err < 0)
+ 		die("BUG: run_command with a pipe can cause deadlock");
+ 
+ 	code = start_command(cmd);
  	if (code)
  		return code;
  	return finish_command(cmd);
@@@ -829,3 -838,19 +834,19 @@@ int run_hook_le(const char *const *env
  
  	return ret;
  }
+ 
+ int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
+ {
+ 	cmd->out = -1;
+ 	if (start_command(cmd) < 0)
+ 		return -1;
+ 
+ 	if (strbuf_read(buf, cmd->out, hint) < 0) {
+ 		close(cmd->out);
+ 		finish_command(cmd); /* throw away exit code */
+ 		return -1;
+ 	}
+ 
+ 	close(cmd->out);
+ 	return finish_command(cmd);
+ }
diff --combined wt-status.c
index 7036fa28dc,ef232a74be..853419f05f
--- a/wt-status.c
+++ b/wt-status.c
@@@ -729,7 -729,6 +729,6 @@@ static void wt_status_print_submodule_s
  	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);
@@@ -745,15 -744,11 +744,11 @@@
  
  	sm_summary.git_cmd = 1;
  	sm_summary.no_stdin = 1;
- 	fflush(s->fp);
- 	sm_summary.out = -1;
  
- 	run_command(&sm_summary);
- 
- 	len = 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 (len) {
+ 	if (cmd_stdout.len) {
  		if (uncommitted)
  			strbuf_addstr(&summary, _("Submodules changed but not updated:"));
  		else
@@@ -764,6 -759,7 +759,7 @@@
  	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);
@@@ -849,8 -845,6 +845,8 @@@ static void wt_status_print_verbose(str
  {
  	struct rev_info rev;
  	struct setup_revision_opt opt;
 +	int dirty_submodules;
 +	const char *c = color(WT_STATUS_HEADER, s);
  
  	init_revisions(&rev, NULL);
  	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
@@@ -875,25 -869,7 +871,25 @@@
  		rev.diffopt.use_color = 0;
  		wt_status_add_cut_line(s->fp);
  	}
 +	if (s->verbose > 1 && s->commitable) {
 +		/* print_updated() printed a header, so do we */
 +		if (s->fp != stdout)
 +			wt_status_print_trailer(s);
 +		status_printf_ln(s, c, _("Changes to be committed:"));
 +		rev.diffopt.a_prefix = "c/";
 +		rev.diffopt.b_prefix = "i/";
 +	} /* else use prefix as per user config */
  	run_diff_index(&rev, 1);
 +	if (s->verbose > 1 &&
 +	    wt_status_check_worktree_changes(s, &dirty_submodules)) {
 +		status_printf_ln(s, c,
 +			"--------------------------------------------------");
 +		status_printf_ln(s, c, _("Changes not staged for commit:"));
 +		setup_work_tree();
 +		rev.diffopt.a_prefix = "i/";
 +		rev.diffopt.b_prefix = "w/";
 +		run_diff_files(&rev, 0);
 +	}
  }
  
  static void wt_status_print_tracking(struct wt_status *s)
@@@ -1242,8 -1218,6 +1238,8 @@@ static void wt_status_get_detached_from
  		state->detached_from =
  			xstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV));
  	hashcpy(state->detached_sha1, cb.nsha1);
 +	state->detached_at = !get_sha1("HEAD", sha1) &&
 +			     !hashcmp(sha1, state->detached_sha1);
  
  	free(ref);
  	strbuf_release(&cb.buf);
@@@ -1332,8 -1306,10 +1328,8 @@@ void wt_status_print(struct wt_status *
  				on_what = _("rebase in progress; onto ");
  				branch_name = state.onto;
  			} else if (state.detached_from) {
 -				unsigned char sha1[20];
  				branch_name = state.detached_from;
 -				if (!get_sha1("HEAD", sha1) &&
 -				    !hashcmp(sha1, state.detached_sha1))
 +				if (state.detached_at)
  					on_what = _("HEAD detached at ");
  				else
  					on_what = _("HEAD detached from ");