Junio,
here is a series of patches that removes a number fork/exec pairs.
The series consists of 2 parts:
- The first half replaces a number of fork/exec pairs by start_command/

  finish_command or run_command.
- The second half introduces a new framework that runs a function

  asynchronously. New functions start_async and finish_async are implemented

  similarly to start_command and run_command. They are used to replace

  occurrences of fork() that does not exec() in the child. Such code

  could in principle be run in a thread, and on MinGW port we will go this

  route, but on Posix we stay with fork().
The series can be applied on top of 2b5a06e (Restore default verbosity for

http fetches), as you reqested, but that commit does not compile, so I

developed it on 90446a00 (bundle transport: fix an alloc_ref() call),

which is a few commits earlier.
There will be some strbuf related merge conflicts once you merge this into

master.
 builtin-archive.c     |    8 +-

 builtin-fetch-pack.c  |  101 +++++++++----------------

 cache.h               |    4 +-

 connect.c             |  131 ++++++++++++++++-----------------

 convert.c             |   87 ++++++++--------------

 diff.c                |   38 +---------

 peek-remote.c         |    8 +-

 run-command.c         |   79 +++++++++++++++++---

 run-command.h         |   24 ++++++

 send-pack.c           |    8 +-

 t/t0021-conversion.sh |    7 ++-

 transport.c           |    9 +--

 upload-pack.c         |  199 ++++++++++++++++++++++---------------------------

 13 files changed, 334 insertions(+), 369 deletions(-)
-- Hannes

-

This series looks pretty good to me.  I like seeing huge blocks

go away only to be replaced with the simple API offered by

run-command.h.  Makes the result much easier to follow.
The async interface is also quite simple.  Unfortunately there

is some risk with the canonical fork() implementation in that the

async routine might attempt to alter global data that the parent

is also using, and folks on a good UNIX that is using the fork()

implementation will not even notice as they are in totally separated

address spaces.  But you'll see it in MSYS Git.
Since builtin-pack-objects now accepts (limited) pthread support,

perhaps this should be implemented in terms of pthread support

when pthreads are available?  Most Linux/BSD/Mac OS X systems do

have pthreads these days and that's the majority of git users and

developers.  This would make it more likely that bugs in this sort
Hard to argue with that final state.  You killed 35 lines and

also made Git easier to port to "that OS unfortunately named after

transparent glass thingies".
--

Shawn.

-

Hi,
Falling back to fork() when no pthreads are available?  Yes, that makes

sense.
It might also (marginally) speed up operations, since the switches between

threads are cheaper than those between processes, right?
Ciao,

Dscho
-

Usually.  If we have a large virtual address space (say due to

opening a bunch of packfiles and reading commits out of them into

struct commit* thingies) and the OS does a giant copy of the page

tables during fork() then the pthread creation should be a heck of

a lot cheaper.
But we most definately *must* continue to support fork() for the

async functions.  Its the most common interface available on one

of our biggest platforms (UNIX).
--

Shawn.

-

Yeah that, and the fact that many of the git modules aren't

thread-safe (some modules have static buffers strbuf's or caching

variables) and should be used with care.
  The trivial way is to add a __thread keyword to make them TLS

variables, though, it's not really a step in the direction of

portability, and last time I looked at it, mingw didn't had TLS support,

not sure if msys has. Though, if Msys has, it's worth using, and we

could require that targets using the fancy pthread thingy should also

have some fancy TLS, or use fork().
  Portability for such issues, would be to use pthread_key_* and

pthread_{get,set}specific, and those are a hell of a sucky API.
--=20

=C2=B7O=C2=B7  Pierre Habouzit

=C2=B7=C2=B7O                                                madcoder@debia=

n.org

OOO                                                http://www.madism.org

Hi,
I think that was exactly the point of Shawn: expose bugs on Unix that

would otherwise only show in msysGit.
Ciao,

Dscho
-

Okay forget it, mingw and msys are one and the same *g*.

  So well, maybe threading isn't such a so great idea :/
--=20

=C2=B7O=C2=B7  Pierre Habouzit

=C2=B7=C2=B7O                                                madcoder@debia=

n.org

OOO                                                http://www.madism.org

And again last time I checked it was still a mingw 3.x in debian, now

that it's 4.2.1 it seems to support __thread (but not

__declspec(thread)) and their changelog seems to confirm that fact [0].
  So the question holds again, do we require pthread-using targets to

support TLS ? It feels sane and right to me, but =E2=80=A6
  [0] http://sourceforge.net/project/shownotes.php?release_id=3D532062

      [...]

      * The  __thread keyword is honoured.

      [...]
--=20

=C2=B7O=C2=B7  Pierre Habouzit

=C2=B7=C2=B7O                                                madcoder@debia=

n.org

OOO                                                http://www.madism.org

To me it's a sane place to start. As time goes by and people on non-TLS

capable systems come along that need the functionality (or the speedup;

fork() is expensive on some systems), they can probably implement it

themselves or at least give voice to the fact that they need it.
--

Andreas Ericsson                   andreas.ericsson@op5.se

OP5 AB                             www.op5.se

Tel: +46 8-230225                  Fax: +46 8-230231

-

This prepares the API of git_connect() and finish_connect() to operate on

a struct child_process. Currently, we just use that object as a placeholder

for the pid that we used to return. A follow-up patch will change the

implementation of git_connect() and finish_connect() to make full use

of the object.
Old code had early-return-on-error checks at the calling sites of

git_connect(), but since git_connect() dies on errors anyway, these checks

were removed.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

---

 builtin-archive.c    |    8 +++-----

 builtin-fetch-pack.c |    8 +++-----

 cache.h              |    4 ++--

 connect.c            |   31 +++++++++++++++++--------------

 peek-remote.c        |    8 +++-----

 send-pack.c          |    8 +++-----

 transport.c          |    9 ++-------

 7 files changed, 33 insertions(+), 43 deletions(-)
diff --git a/builtin-archive.c b/builtin-archive.c

index a90c65c..346444b 100644

--- a/builtin-archive.c

+++ b/builtin-archive.c

@@ -30,7 +30,7 @@ static int run_remote_archiver(const char *remote, int argc,

 {

 	char *url, buf[LARGE_PACKET_MAX];

 	int fd[2], i, len, rv;

-	pid_t pid;

+	struct child_process *conn;

 	const char *exec = "git-upload-archive";

 	int exec_at = 0;
@@ -46,9 +46,7 @@ static int run_remote_archiver(const char *remote, int argc,

 	}
 	url = xstrdup(remote);

-	pid = git_connect(fd, url, exec, 0);

-	if (pid < 0)

-		return pid;

+	conn = git_connect(fd, url, exec, 0);
 	for (i = 1; i < argc; i++) {

 		if (i == exec_at)

@@ -76,7 +74,7 @@ static int run_remote_archiver(const char *remote, int argc,

 	rv = recv_sideband("archive", fd[0], 1, 2);

 	close(fd[0]);

 	close(fd[1]);

-	rv |= finish_connect(pid);

+	rv |= finish_connect(conn);
 	return !!rv;

 }

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c

index 8f25d50..f56592f 100644

--- a/builtin-fetch-pack.c

+++ b/builtin-fetch-pack.c

@@ -762,7 +762,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,

 {

 	int i...
[ message continues ]

Hi,
Just for completeness' sake: could you do a free(conn); before return -1;?
Thanks,

Dscho
-

I know. But the loop is going away with the next patch, so I didn't bother.

Can you live with that?
-- Hannes

-

Hi,
It'll be hard, but I'll try ;-)
Ciao,

Dscho
-

The child process handling is delegated to start_command() and

finish_command().
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

---

 connect.c |  108 ++++++++++++++++++++++++++++--------------------------------

 1 files changed, 50 insertions(+), 58 deletions(-)
diff --git a/connect.c b/connect.c

index b156f92..21597bf 100644

--- a/connect.c

+++ b/connect.c

@@ -484,11 +484,15 @@ struct child_process *git_connect(int fd[2], char *url,

 	char *host, *path = url;

 	char *end;

 	int c;

-	int pipefd[2][2];

 	struct child_process *conn;

 	enum protocol protocol = PROTO_LOCAL;

 	int free_path = 0;

 	char *port = NULL;

+	const char **arg;

+	char command[MAX_CMD_LEN];

+	char *posn = command;

+	int size = MAX_CMD_LEN;

+	int of = 0;
 	/* Without this we cannot rely on waitpid() to tell

 	 * what happened to our children.

@@ -574,62 +578,51 @@ struct child_process *git_connect(int fd[2], char *url,

 		return NULL;

 	}
-	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)

-		die("unable to create pipe pair for communication");

 	conn = xcalloc(1, sizeof(*conn));

-	conn->pid = fork();

-	if (conn->pid < 0)

-		die("unable to fork");

-	if (!conn->pid) {

-		char command[MAX_CMD_LEN];

-		char *posn = command;

-		int size = MAX_CMD_LEN;

-		int of = 0;

-

-		of |= add_to_string(&posn, &size, prog, 0);

-		of |= add_to_string(&posn, &size, " ", 0);

-		of |= add_to_string(&posn, &size, path, 1);

-

-		if (of)

-			die("command line too long");

-

-		dup2(pipefd[1][0], 0);

-		dup2(pipefd[0][1], 1);

-		close(pipefd[0][0]);

-		close(pipefd[0][1]);

-		close(pipefd[1][0]);

-		close(pipefd[1][1]);

-		if (protocol == PROTO_SSH) {

-			const char *ssh, *ssh_basename;

-			ssh = getenv("GIT_SSH");

-			if (!ssh) ssh = "ssh";

-			ssh_basename = strrchr(ssh, '/');

-			if (!ssh_basename)

-				ssh_basename = ssh;

-			else

-				ssh_basename++;
-			if (!port)

-				execlp(ssh, ssh_basename, host, command, NULL);

-			else

-				exe...
[ message continues ]

The previous code already used finish_command() to wait for the process

to terminate, but did not use start_command() to run it.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

---

 convert.c |   30 +++++++-----------------------

 1 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/convert.c b/convert.c

index d77c8eb..6d64994 100644

--- a/convert.c

+++ b/convert.c

@@ -208,34 +208,18 @@ static int filter_buffer(const char *path, const char *src,

 	 * Spawn cmd and feed the buffer contents through its stdin.

 	 */

 	struct child_process child_process;

-	int pipe_feed[2];

 	int write_err, status;

+	const char *argv[] = { "sh", "-c", cmd, NULL };
 	memset(&child_process, 0, sizeof(child_process));

+	child_process.argv = argv;

+	child_process.in = -1;
-	if (pipe(pipe_feed) < 0) {

-		error("cannot create pipe to run external filter %s", cmd);

-		return 1;

-	}

-

-	child_process.pid = fork();

-	if (child_process.pid < 0) {

-		error("cannot fork to run external filter %s", cmd);

-		close(pipe_feed[0]);

-		close(pipe_feed[1]);

-		return 1;

-	}

-	if (!child_process.pid) {

-		dup2(pipe_feed[0], 0);

-		close(pipe_feed[0]);

-		close(pipe_feed[1]);

-		execlp("sh", "sh", "-c", cmd, NULL);

-		return 1;

-	}

-	close(pipe_feed[0]);

+	if (start_command(&child_process))

+		return error("cannot fork to run external filter %s", cmd);
-	write_err = (write_in_full(pipe_feed[1], src, size) < 0);

-	if (close(pipe_feed[1]))

+	write_err = (write_in_full(child_process.in, src, size) < 0);

+	if (close(child_process.in))

 		write_err = 1;

 	if (write_err)

 		error("cannot feed the input to external filter %s", cmd);

--

1.5.3.3.1134.gee562
-

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

---

 diff.c |   38 +++-----------------------------------

 1 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/diff.c b/diff.c

index 0ee9ea1..d03dc6a 100644

--- a/diff.c

+++ b/diff.c

@@ -9,6 +9,7 @@

 #include "xdiff-interface.h"

 #include "color.h"

 #include "attr.h"

+#include "run-command.h"
 #ifdef NO_FAST_WORKING_DIRECTORY

 #define FAST_WORKING_DIRECTORY 0

@@ -1791,40 +1792,6 @@ static void remove_tempfile_on_signal(int signo)

 	raise(signo);

 }
-static int spawn_prog(const char *pgm, const char **arg)

-{

-	pid_t pid;

-	int status;

-

-	fflush(NULL);

-	pid = fork();

-	if (pid < 0)

-		die("unable to fork");

-	if (!pid) {

-		execvp(pgm, (char *const*) arg);

-		exit(255);

-	}

-

-	while (waitpid(pid, &status, 0) < 0) {

-		if (errno == EINTR)

-			continue;

-		return -1;

-	}

-

-	/* Earlier we did not check the exit status because

-	 * diff exits non-zero if files are different, and

-	 * we are not interested in knowing that.  It was a

-	 * mistake which made it harder to quit a diff-*

-	 * session that uses the git-apply-patch-script as

-	 * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF

-	 * should also exit non-zero only when it wants to

-	 * abort the entire diff-* session.

-	 */

-	if (WIFEXITED(status) && !WEXITSTATUS(status))

-		return 0;

-	return -1;

-}

-

 /* An external diff command takes:

  *

  * diff-cmd name infile1 infile1-sha1 infile1-mode \

@@ -1877,7 +1844,8 @@ static void run_external_diff(const char *pgm,

 		*arg++ = name;

 	}

 	*arg = NULL;

-	retval = spawn_prog(pgm, spawn_arg);

+	fflush(NULL);

+	retval = run_command_v_opt(spawn_arg, 0);

 	remove_tempfile();

 	if (retval) {

 		fprintf(stderr, "external diff died, stopping at %s.\n", name);

--

1.5.3.3.1134.gee562
-

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

---

 builtin-fetch-pack.c |   56 ++++++++++++++-----------------------------------

 1 files changed, 16 insertions(+), 40 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c

index f56592f..871b704 100644

--- a/builtin-fetch-pack.c

+++ b/builtin-fetch-pack.c

@@ -7,6 +7,7 @@

 #include "pack.h"

 #include "sideband.h"

 #include "fetch-pack.h"

+#include "run-command.h"
 static int transfer_unpack_limit = -1;

 static int fetch_unpack_limit = -1;

@@ -491,18 +492,19 @@ static pid_t setup_sideband(int fd[2], int xd[2])
 static int get_pack(int xd[2], char **pack_lockfile)

 {

-	int status;

-	pid_t pid, side_pid;

+	pid_t side_pid;

 	int fd[2];

 	const char *argv[20];

 	char keep_arg[256];

 	char hdr_arg[256];

 	const char **av;

 	int do_keep = args.keep_pack;

-	int keep_pipe[2];

+	struct child_process cmd;
 	side_pid = setup_sideband(fd, xd);
+	memset(&cmd, 0, sizeof(cmd));

+	cmd.argv = argv;

 	av = argv;

 	*hdr_arg = 0;

 	if (!args.keep_pack && unpack_limit) {

@@ -519,8 +521,8 @@ static int get_pack(int xd[2], char **pack_lockfile)

 	}
 	if (do_keep) {

-		if (pack_lockfile && pipe(keep_pipe))

-			die("fetch-pack: pipe setup failure: %s", strerror(errno));

+		if (pack_lockfile)

+			cmd.out = -1;

 		*av++ = "index-pack";

 		*av++ = "--stdin";

 		if (!args.quiet && !args.no_progress)

@@ -544,43 +546,17 @@ static int get_pack(int xd[2], char **pack_lockfile)

 		*av++ = hdr_arg;

 	*av++ = NULL;
-	pid = fork();

-	if (pid < 0)

+	cmd.in = fd[0];

+	cmd.git_cmd = 1;

+	if (start_command(&cmd))

 		die("fetch-pack: unable to fork off %s", argv[0]);

-	if (!pid) {

-		dup2(fd[0], 0);

-		if (do_keep && pack_lockfile) {

-			dup2(keep_pipe[1], 1);

-			close(keep_pipe[0]);

-			close(keep_pipe[1]);

-		}

-		close(fd[0]);

-		close(fd[1]);

-		execv_git_cmd(argv);

-		die("%s exec failed", argv[0]);

-	}

-	close(fd[0]);

 	close(fd[1]);

-	if (...
[ message continues ]