git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 07/14] upload-pack: Use start_command() to run pack-objects in create_pack_file().

From
Johannes Sixt <johannes.sixt@telecom.at>
Date
Oct 13, 2007, 20:06 UTC
Message-ID
<1192305984-22594-8-git-send-email-johannes.sixt@telecom.at>
In-Reply-To
<1192305984-22594-7-git-send-email-johannes.sixt@telecom.at>
This gets rid of an explicit fork/exec.

Since upload-pack has to coordinate two processes (rev-list and pack-objects), we cannot use the normal finish_command(), but have to monitor the processes explicitly. Hence, the waitpid() call remains.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 upload-pack.c |  105 ++++++++++++++++++++++++---------------------------------
 1 files changed, 44 insertions(+), 61 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index fe96ef1..ef2894a 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -9,6 +9,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "list-objects.h"
+#include "run-command.h"
 
 static const char upload_pack_usage[] = "git-upload-pack [--strict] [--timeout=nn] <dir>";
 
@@ -98,16 +99,18 @@ static void show_edge(struct commit *commit)
 
 static void create_pack_file(void)
 {
-	/* Pipes between rev-list to pack-objects, pack-objects to us
-	 * and pack-objects error stream for progress bar.
+	/* Pipe from rev-list to pack-objects
 	 */
-	int lp_pipe[2], pu_pipe[2], pe_pipe[2];
-	pid_t pid_rev_list, pid_pack_objects;
+	int lp_pipe[2];
+	pid_t pid_rev_list;
+	struct child_process pack_objects;
 	int create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);
 	char data[8193], progress[128];
 	char abort_msg[] = "aborting due to possible repository "
 		"corruption on the remote side.";
 	int buffered = -1;
+	const char *argv[10];
+	int arg = 0;
 
 	if (pipe(lp_pipe) < 0)
 		die("git-upload-pack: unable to create pipe");
@@ -154,52 +157,32 @@ static void create_pack_file(void)
 		exit(0);
 	}
 
-	if (pipe(pu_pipe) < 0)
-		die("git-upload-pack: unable to create pipe");
-	if (pipe(pe_pipe) < 0)
-		die("git-upload-pack: unable to create pipe");
-	pid_pack_objects = fork();
-	if (pid_pack_objects < 0) {
+	/* close this so that it is not inherited by pack_objects */
+	close(lp_pipe[1]);
+
+	argv[arg++] = "pack-objects";
+	argv[arg++] = "--stdout";
+	if (!no_progress)
+		argv[arg++] = "--progress";
+	if (use_ofs_delta)
+		argv[arg++] = "--delta-base-offset";
+	argv[arg++] = NULL;
+
+	memset(&pack_objects, 0, sizeof(pack_objects));
+	pack_objects.in = lp_pipe[0];	/* start_command closes it */
+	pack_objects.out = -1;
+	pack_objects.err = -1;
+	pack_objects.git_cmd = 1;
+	pack_objects.argv = argv;
+	if (start_command(&pack_objects)) {
 		/* daemon sets things up to ignore TERM */
 		kill(pid_rev_list, SIGKILL);
 		die("git-upload-pack: unable to fork git-pack-objects");
 	}
-	if (!pid_pack_objects) {
-		const char *argv[10];
-		int i = 0;
-
-		dup2(lp_pipe[0], 0);
-		dup2(pu_pipe[1], 1);
-		dup2(pe_pipe[1], 2);
-
-		close(lp_pipe[0]);
-		close(lp_pipe[1]);
-		close(pu_pipe[0]);
-		close(pu_pipe[1]);
-		close(pe_pipe[0]);
-		close(pe_pipe[1]);
-
-		argv[i++] = "pack-objects";
-		argv[i++] = "--stdout";
-		if (!no_progress)
-			argv[i++] = "--progress";
-		if (use_ofs_delta)
-			argv[i++] = "--delta-base-offset";
-		argv[i++] = NULL;
-
-		execv_git_cmd(argv);
-		kill(pid_rev_list, SIGKILL);
-		die("git-upload-pack: unable to exec git-pack-objects");
-	}
-
-	close(lp_pipe[0]);
-	close(lp_pipe[1]);
 
-	/* We read from pe_pipe[0] to capture stderr output for
-	 * progress bar, and pu_pipe[0] to capture the pack data.
+	/* We read from pack_objects.err to capture stderr output for
+	 * progress bar, and pack_objects.out to capture the pack data.
 	 */
-	close(pe_pipe[1]);
-	close(pu_pipe[1]);
 
 	while (1) {
 		const char *who;
@@ -214,14 +197,14 @@ static void create_pack_file(void)
 		pollsize = 0;
 		pe = pu = -1;
 
-		if (0 <= pu_pipe[0]) {
-			pfd[pollsize].fd = pu_pipe[0];
+		if (0 <= pack_objects.out) {
+			pfd[pollsize].fd = pack_objects.out;
 			pfd[pollsize].events = POLLIN;
 			pu = pollsize;
 			pollsize++;
 		}
-		if (0 <= pe_pipe[0]) {
-			pfd[pollsize].fd = pe_pipe[0];
+		if (0 <= pack_objects.err) {
+			pfd[pollsize].fd = pack_objects.err;
 			pfd[pollsize].events = POLLIN;
 			pe = pollsize;
 			pollsize++;
@@ -254,13 +237,13 @@ static void create_pack_file(void)
 					*cp++ = buffered;
 					outsz++;
 				}
-				sz = xread(pu_pipe[0], cp,
+				sz = xread(pack_objects.out, cp,
 					  sizeof(data) - outsz);
 				if (0 < sz)
 						;
 				else if (sz == 0) {
-					close(pu_pipe[0]);
-					pu_pipe[0] = -1;
+					close(pack_objects.out);
+					pack_objects.out = -1;
 				}
 				else
 					goto fail;
@@ -279,13 +262,13 @@ static void create_pack_file(void)
 				/* Status ready; we ship that in the side-band
 				 * or dump to the standard error.
 				 */
-				sz = xread(pe_pipe[0], progress,
+				sz = xread(pack_objects.err, progress,
 					  sizeof(progress));
 				if (0 < sz)
 					send_client_data(2, progress, sz);
 				else if (sz == 0) {
-					close(pe_pipe[0]);
-					pe_pipe[0] = -1;
+					close(pack_objects.err);
+					pack_objects.err = -1;
 				}
 				else
 					goto fail;
@@ -293,12 +276,12 @@ static void create_pack_file(void)
 		}
 
 		/* See if the children are still there */
-		if (pid_rev_list || pid_pack_objects) {
+		if (pid_rev_list || pack_objects.pid) {
 			pid = waitpid(-1, &status, WNOHANG);
 			if (!pid)
 				continue;
 			who = ((pid == pid_rev_list) ? "git-rev-list" :
-			       (pid == pid_pack_objects) ? "git-pack-objects" :
+			       (pid == pack_objects.pid) ? "git-pack-objects" :
 			       NULL);
 			if (!who) {
 				if (pid < 0) {
@@ -317,9 +300,9 @@ static void create_pack_file(void)
 			}
 			if (pid == pid_rev_list)
 				pid_rev_list = 0;
-			if (pid == pid_pack_objects)
-				pid_pack_objects = 0;
-			if (pid_rev_list || pid_pack_objects)
+			if (pid == pack_objects.pid)
+				pack_objects.pid = 0;
+			if (pid_rev_list || pack_objects.pid)
 				continue;
 		}
 
@@ -340,8 +323,8 @@ static void create_pack_file(void)
 		return;
 	}
  fail:
-	if (pid_pack_objects)
-		kill(pid_pack_objects, SIGKILL);
+	if (pack_objects.pid)
+		kill(pack_objects.pid, SIGKILL);
 	if (pid_rev_list)
 		kill(pid_rev_list, SIGKILL);
 	send_client_data(3, abort_msg, sizeof(abort_msg));
-- 
1.5.3.3.1134.gee562
Previous: Johannes SixtNext: Johannes Sixt
Message 8 of 30 in “fork/exec removal series”
  1. 0/14 fork/exec removal seriesJohannes Sixt, Oct 13, 2007
  2. 01/14 Change git_connect() to return a struct child_process instead of a pid_t.Johannes Sixt, Oct 13, 2007
  3. 02/14 Use start_command() in git_connect() instead of explicit fork/exec.Johannes Sixt, Oct 13, 2007
  4. 03/14 Use start_command() to run content filters instead of explicit fork/exec.Johannes Sixt, Oct 13, 2007
  5. 04/14 Use run_command() to spawn external diff programs instead of fork/exec.Johannes Sixt, Oct 13, 2007
  6. 05/14 Use start_comand() in builtin-fetch-pack.c instead of explicit fork/exec.Johannes Sixt, Oct 13, 2007
  7. 06/14 Have start_command() create a pipe to read the stderr of the child.Johannes Sixt, Oct 13, 2007
  8. 07/14 upload-pack: Use start_command() to run pack-objects in create_pack_file().Johannes Sixt, Oct 13, 2007
  9. 08/14 Add infrastructure to run a function asynchronously.Johannes Sixt, Oct 13, 2007
  10. 09/14 Use the asyncronous function infrastructure in builtin-fetch-pack.c.Johannes Sixt, Oct 13, 2007
  11. 10/14 upload-pack: Move the revision walker into a separate function.Johannes Sixt, Oct 13, 2007
  12. 11/14 upload-pack: Run rev-list in an asynchronous function.Johannes Sixt, Oct 13, 2007
  13. 12/14 t0021-conversion.sh: Test that the clean filter really cleans content.Johannes Sixt, Oct 13, 2007
  14. 13/14 Avoid a dup2(2) in apply_filter() - start_command() can do it for us.Johannes Sixt, Oct 13, 2007
  15. 14/14 Use the asyncronous function infrastructure to run the content filter.Johannes Sixt, Oct 13, 2007
  16. Johannes SchindelinOct 14, 2007
  17. Johannes SixtOct 14, 2007
  18. 14/14 Use the asyncronous function infrastructure to run the content filter.Johannes Sixt, Oct 14, 2007
  19. 08/14 Add infrastructure to run a function asynchronously.Johannes Sixt, Oct 14, 2007
  20. Johannes SchindelinOct 14, 2007
  21. Johannes SixtOct 14, 2007
  22. Johannes SchindelinOct 14, 2007
  23. Shawn O. PearceOct 14, 2007
  24. Johannes SchindelinOct 14, 2007
  25. Shawn O. PearceOct 14, 2007
  26. Pierre HabouzitOct 14, 2007
  27. Pierre HabouzitOct 14, 2007
  28. Pierre HabouzitOct 14, 2007
  29. Andreas EricssonOct 14, 2007
  30. Johannes SchindelinOct 14, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.