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

Re: [PATCH] repack: rewrite the shell script in C.

From
Stefan Beller <stefanbeller@googlemail.com>
Date
Aug 21, 2013, 17:25 UTC
Message-ID
<5214F816.3010303@googlemail.com>
In-Reply-To
<20130821082527.GC2802@elie.Belkin>
On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
Show 9 quoted lines
> Hi,
> 
> Stefan Beller wrote:
> 
>> [PATCH] repack: rewrite the shell script in C.
> 
> Thanks for your work so far.  This review will have mostly cosmetic
> notes.  Hopefully others can try it out to see if the actual behavior
> is good.

Thanks for all the reviews. I hope to have included every suggestion so far or have send out mail discussing why not.

There have been quite a few changes since last round because of so many reviews.

Here is a diff to the last sent patch, I'll also send the whole patch on its one again. Last time I forgot to label correctly with [RFC PATCHv5], so the next patch should be v6.

Stefan
Changes since "[PATCH] repack: rewrite the shell script in C.":
--8<--
From 3cda569cdcd1312679c0035d151515cba7dacc59 Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Wed, 21 Aug 2013 12:33:13 +0200
Subject: [PATCH 2/3] Changes to last round.
 * get_pack_filenames: directly check for .keep files
 * packdir is a global variable now
 * fix help string for parsing options.
 * reenable the delta-base-offset being turned on by default
 * rewrite remove_temporary_files(void), remove_redundant_pack(fname)
   to use more strbuf instead of using mkpath(dup)
 * beautifying the code (line length, empty lines)
Still on the todo list for this patch:
 * Inspect the code for unlink, rename and see if we
   need to deal with their return codes.
 * Check for datatypes (--window-memory could use ulong?)
Later:
 * Move parts of cmd_repack to extra functions
 * check if subprocesses are needed (update-server-info,
   prune-packed)
---
 builtin/repack.c | 191
++++++++++++++++++++++++++++++-------------------------
 1 file changed, 103 insertions(+), 88 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 9fbe636..fb050c0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -13,7 +13,9 @@
 #include "string-list.h"
 #include "argv-array.h"

-static int delta_base_offset = 0;
+/* enabled by default since 22c79eab (2008-06-25) */
+static int delta_base_offset = 1;
+char *packdir;

 static const char *const git_repack_usage[] = {
 	N_("git repack [options]"),
@@ -29,25 +31,39 @@ static int repack_config(const char *var, const char
*value, void *cb)
 	return git_default_config(var, value, cb);
 }

-static void remove_temporary_files() {
+/*
+ * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
+ */
+static void remove_temporary_files(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+	size_t dirlen, prefixlen;
 	DIR *dir;
 	struct dirent *e;
-	char *prefix, *path;

-	prefix = mkpathdup(".tmp-%d-pack", getpid());
-	path = mkpathdup("%s/pack", get_object_directory());
+	/* .git/objects/pack */
+	strbuf_addstr(&buf, get_object_directory());
+	strbuf_addstr(&buf, "/pack");
+	dir = opendir(buf.buf);
+	if (!dir) {
+		strbuf_release(&buf);
+		return;
+	}

-	dir = opendir(path);
-	while ((e = readdir(dir)) != NULL) {
-		if (!prefixcmp(e->d_name, prefix)) {
-			struct strbuf fname = STRBUF_INIT;
-			strbuf_addf(&fname, "%s/%s", path, e->d_name);
-			unlink(strbuf_detach(&fname, NULL));
-		}
+	/* .git/objects/pack/.tmp-$$-pack-* */
+	dirlen = buf.len + 1;
+	strbuf_addf(&buf, "/.tmp-%d-pack-", (int)getpid());
+	prefixlen = buf.len - dirlen;
+
+	while ((e = readdir(dir))) {
+		if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
+			continue;
+		strbuf_setlen(&buf, dirlen);
+		strbuf_addstr(&buf, e->d_name);
+		unlink(buf.buf);
 	}
-	free(prefix);
-	free(path);
 	closedir(dir);
+	strbuf_release(&buf);
 }

 static void remove_pack_on_signal(int signo)
@@ -57,52 +73,57 @@ static void remove_pack_on_signal(int signo)
 	raise(signo);
 }

-/*
- * Fills the filename list with all the files found in the pack directory
- * ending with .pack, without that extension.
- */
-void get_pack_filenames(char *packdir, struct string_list *fname_list)
+static void get_pack_filenames(struct string_list *fname_list)
 {
 	DIR *dir;
 	struct dirent *e;
-	char *path, *suffix, *fname;
+	char *fname;

-	path = mkpath("%s/pack", get_object_directory());
-	suffix = ".pack";
+	if (!(dir = opendir(packdir)))
+		return;

-	dir = opendir(path);
 	while ((e = readdir(dir)) != NULL) {
-		if (!suffixcmp(e->d_name, suffix)) {
-			size_t len = strlen(e->d_name) - strlen(suffix);
-			fname = xmemdupz(e->d_name, len);
+		if (suffixcmp(e->d_name, ".pack"))
+			continue;
+
+		size_t len = strlen(e->d_name) - strlen(".pack");
+		fname = xmemdupz(e->d_name, len);
+
+		if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
 			string_list_append_nodup(fname_list, fname);
-		}
 	}
 	closedir(dir);
 }

-void remove_pack(char *path, char* sha1)
+static void remove_redundant_pack(const char *path, const char *sha1)
 {
-	char *exts[] = {".pack", ".idx", ".keep"};
-	int ext = 0;
-	for (ext = 0; ext < 3; ext++) {
-		char *fname;
-		fname = mkpath("%s/%s%s", path, sha1, exts[ext]);
-		unlink(fname);
+	const char *exts[] = {".pack", ".idx", ".keep"};
+	int i;
+	struct strbuf buf = STRBUF_INIT;
+	size_t plen;
+
+	strbuf_addf(&buf, "%s/%s", path, sha1);
+	plen = buf.len;
+
+	for (i = 0; i < ARRAY_SIZE(exts); i++) {
+		strbuf_setlen(&buf, plen);
+		strbuf_addstr(&buf, exts[i]);
+		unlink(buf.buf);
 	}
 }

-int cmd_repack(int argc, const char **argv, const char *prefix) {
-
-	char *exts[2] = {".idx", ".pack"};
-	char *packdir, *packtmp, line[1024];
+int cmd_repack(int argc, const char **argv, const char *prefix)
+{
+	const char *exts[2] = {".idx", ".pack"};
+	char *packtmp;
 	struct child_process cmd;
 	struct string_list_item *item;
 	struct argv_array cmd_args = ARGV_ARRAY_INIT;
 	struct string_list names = STRING_LIST_INIT_DUP;
 	struct string_list rollback = STRING_LIST_INIT_DUP;
 	struct string_list existing_packs = STRING_LIST_INIT_DUP;
-	int count_packs, ext;
+	struct strbuf line = STRBUF_INIT;
+	int count_packs, ext, ret;
 	FILE *out;

 	/* variables to be filled by option parsing */
@@ -135,7 +156,7 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 		OPT_BOOL('l', "local", &local,
 				N_("pass --local to git-pack-objects")),
 		OPT_STRING(0, "unpack-unreachable", &unpack_unreachable,
N_("approxidate"),
-				N_("with -A, do not loosen objects older than this Packing
constraints")),
+				N_("with -A, do not loosen objects older than this")),
 		OPT_INTEGER(0, "window", &window,
 				N_("size of the window used for delta compression")),
 		OPT_INTEGER(0, "window-memory", &window_memory,
@@ -155,7 +176,7 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 	sigchain_push_common(remove_pack_on_signal);

 	packdir = mkpathdup("%s/pack", get_object_directory());
-	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, getpid());
+	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());

 	argv_array_push(&cmd_args, "pack-objects");
 	argv_array_push(&cmd_args, "--keep-true-parents");
@@ -163,47 +184,33 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 	argv_array_push(&cmd_args, "--non-empty");
 	argv_array_push(&cmd_args, "--all");
 	argv_array_push(&cmd_args, "--reflog");
-
 	if (window)
 		argv_array_pushf(&cmd_args, "--window=%u", window);
-
 	if (window_memory)
 		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
-
 	if (depth)
 		argv_array_pushf(&cmd_args, "--depth=%u", depth);
-
 	if (max_pack_size)
 		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
-
 	if (no_reuse_delta)
 		argv_array_pushf(&cmd_args, "--no-reuse-delta");
-
 	if (no_reuse_object)
 		argv_array_pushf(&cmd_args, "--no-reuse-object");

-	if (pack_everything + pack_everything_but_loose == 0) {
+	if (!pack_everything && !pack_everything_but_loose) {
 		argv_array_push(&cmd_args, "--unpacked");
 		argv_array_push(&cmd_args, "--incremental");
 	} else {
-		struct string_list fname_list = STRING_LIST_INIT_DUP;
-		get_pack_filenames(packdir, &fname_list);
-		for_each_string_list_item(item, &fname_list) {
-			char *fname;
-			fname = mkpathdup("%s/%s.keep", packdir, item->string);
-			if (file_exists(fname)) {
-				/* when the keep file is there, we're ignoring that pack */
-			} else {
-				string_list_append(&existing_packs, item->string);
-			}
-			free(fname);
-		}
+		get_pack_filenames(&existing_packs);

 		if (existing_packs.nr && delete_redundant) {
 			if (unpack_unreachable)
-				argv_array_pushf(&cmd_args, "--unpack-unreachable=%s",
unpack_unreachable);
+				argv_array_pushf(&cmd_args,
+						"--unpack-unreachable=%s",
+						unpack_unreachable);
 			else if (pack_everything_but_loose)
-				argv_array_push(&cmd_args, "--unpack-unreachable");
+				argv_array_push(&cmd_args,
+						"--unpack-unreachable");
 		}
 	}

@@ -222,22 +229,24 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 	cmd.out = -1;
 	cmd.no_stdin = 1;

-	if (start_command(&cmd))
+	ret = start_command(&cmd);
+	if (ret)
 		return 1;

 	count_packs = 0;
 	out = xfdopen(cmd.out, "r");
-	while (fgets(line, sizeof(line), out)) {
-		/* a line consists of 40 hex chars + '\n' */
-		if (strlen(line) != 41)
+	while (strbuf_getline(&line, out, '\n') != EOF) {
+		if (line.len != 40)
 			die("repack: Expecting 40 character sha1 lines only from
pack-objects.");
-		line[40] = '\0';
-		string_list_append(&names, line);
+		strbuf_addstr(&line, "");
+		string_list_append(&names, line.buf);
 		count_packs++;
 	}
-	if (finish_command(&cmd))
-		return 1;
 	fclose(out);
+	ret = finish_command(&cmd);
+	if (ret)
+		return 1;
+	argv_array_clear(&cmd_args);

 	if (!count_packs && !quiet)
 		printf("Nothing new to pack.\n");
@@ -246,13 +255,15 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < 2; ext++) {
 			char *fname, *fname_old;
-			fname = mkpathdup("%s/%s%s", packdir, item->string, exts[ext]);
+			fname = mkpathdup("%s/%s%s", packdir,
+						item->string, exts[ext]);
 			if (!file_exists(fname)) {
 				free(fname);
 				continue;
 			}

-			fname_old = mkpath("%s/old-%s%s", packdir, item->string, exts[ext]);
+			fname_old = mkpathdup("%s/old-%s%s", packdir,
+						item->string, exts[ext]);
 			if (file_exists(fname_old))
 				unlink(fname_old);

@@ -262,6 +273,7 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 			}
 			string_list_append_nodup(&rollback, fname);
 			free(fname);
+			free(fname_old);
 		}
 		if (failed)
 			break;
@@ -286,7 +298,7 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 				"WARNING: file.  But the operation failed, and the\n"
 				"WARNING: attempt to rename them back to their\n"
 				"WARNING: original names also failed.\n"
-				"WARNING: Please rename them in $PACKDIR manually:\n");
+				"WARNING: Please rename them in %s manually:\n", packdir);
 			for (i = 0; i < rollback.nr; i++)
 				fprintf(stderr, "WARNING:   old-%s -> %s\n",
 					rollback.items[i].string,
@@ -300,28 +312,32 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 		for (ext = 0; ext < 2; ext++) {
 			char *fname, *fname_old;
 			struct stat statbuffer;
-			fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]);
-			fname_old = mkpath("%s-%s%s", packtmp, item->string, exts[ext]);
+			fname = mkpathdup("%s/pack-%s%s",
+					packdir, item->string, exts[ext]);
+			fname_old = mkpathdup("%s-%s%s",
+					packtmp, item->string, exts[ext]);
 			if (!stat(fname_old, &statbuffer)) {
-				statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
+				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
 				chmod(fname_old, statbuffer.st_mode);
 			}
 			if (rename(fname_old, fname))
 				die_errno(_("renaming '%s' failed"), fname_old);
 			free(fname);
+			free(fname_old);
 		}
 	}

 	/* Remove the "old-" files */
 	for_each_string_list_item(item, &names) {
-		char *fname;
-		fname = mkpath("%s/old-pack-%s.idx", packdir, item->string);
-		if (remove_path(fname))
-			die_errno(_("removing '%s' failed"), fname);
-
-		fname = mkpath("%s/old-pack-%s.pack", packdir, item->string);
-		if (remove_path(fname))
-			die_errno(_("removing '%s' failed"), fname);
+		for (ext = 0; ext < 2; ext++) {
+			char *fname;
+			fname = mkpath("%s/old-pack-%s%s",
+					packdir,
+					item->string,
+					exts[ext]);
+			if (remove_path(fname))
+				warning(_("removing '%s' failed"), fname);
+		}
 	}

 	/* End of pack replacement. */
@@ -335,9 +351,8 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 				continue;
 			sha1 = item->string + len - 40;
 			if (!string_list_has_string(&names, sha1))
-				remove_pack(packdir, item->string);
+				remove_redundant_pack(packdir, item->string);
 		}
-		argv_array_clear(&cmd_args);
 		argv_array_push(&cmd_args, "prune-packed");
 		if (quiet)
 			argv_array_push(&cmd_args, "--quiet");
@@ -346,16 +361,16 @@ int cmd_repack(int argc, const char **argv, const
char *prefix) {
 		cmd.argv = cmd_args.argv;
 		cmd.git_cmd = 1;
 		run_command(&cmd);
+		argv_array_clear(&cmd_args);
 	}

 	if (!no_update_server_info) {
-		argv_array_clear(&cmd_args);
 		argv_array_push(&cmd_args, "update-server-info");
-
 		memset(&cmd, 0, sizeof(cmd));
 		cmd.argv = cmd_args.argv;
 		cmd.git_cmd = 1;
 		run_command(&cmd);
+		argv_array_clear(&cmd_args);
 	}
 	return 0;
 }

-- 
1.8.4.rc3.1.gc1ebd90
Previous: Stefan BellerNext: Stefan Beller
Message 32 of 72 in “Rewriting git-repack in C”
  1. Rewriting git-repack in CStefan Beller, Aug 13, 2013
  2. repack: rewrite the shell script in C.Stefan Beller, Aug 13, 2013
  3. Matthieu MoyAug 14, 2013
  4. Stefan BellerAug 14, 2013
  5. repack: rewrite the shell script in C.Stefan Beller, Aug 14, 2013
  6. Antoine PelisseAug 14, 2013
  7. Stefan BellerAug 14, 2013
  8. Jeff KingAug 14, 2013
  9. Martin FickAug 14, 2013
  10. Stefan BellerAug 14, 2013
  11. Martin FickAug 14, 2013
  12. Junio C HamanoAug 14, 2013
  13. Martin FickAug 14, 2013
  14. Junio C HamanoAug 15, 2013
  15. [RFC PATCHv2] repack: rewrite the shell script in C.Stefan Beller, Aug 16, 2013
  16. René ScharfeAug 17, 2013
  17. Kyle J. McKayAug 17, 2013
  18. Stefan BellerAug 18, 2013
  19. [RFC PATCHv3] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  20. Kyle J. McKayAug 18, 2013
  21. René ScharfeAug 18, 2013
  22. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 18, 2013
  23. [RFC PATCHv4] repack: rewrite the shell script in C.Stefan Beller, Aug 19, 2013
  24. Johannes SixtAug 20, 2013
  25. Stefan BellerAug 20, 2013
  26. Johannes SixtAug 20, 2013
  27. René ScharfeAug 20, 2013
  28. Stefan BellerAug 20, 2013
  29. repack: rewrite the shell script in C.Stefan Beller, Aug 20, 2013
  30. Jonathan NiederAug 21, 2013
  31. Stefan BellerAug 21, 2013
  32. Stefan BellerAug 21, 2013
  33. 1/2 repack: rewrite the shell script in CStefan Beller, Aug 21, 2013
  34. 2/2 repack: retain the return value of pack-objectsStefan Beller, Aug 21, 2013
  35. Junio C HamanoAug 21, 2013
  36. Matthieu MoyAug 21, 2013
  37. Stefan BellerAug 21, 2013
  38. Junio C HamanoAug 21, 2013
  39. Stefan BellerAug 21, 2013
  40. Johannes SixtAug 22, 2013
  41. Jonathan NiederAug 22, 2013
  42. Matthieu MoyAug 21, 2013
  43. Stefan BellerAug 21, 2013
  44. Matthieu MoyAug 21, 2013
  45. Stefan BellerAug 21, 2013
  46. Matthieu MoyAug 21, 2013
  47. Johannes SixtAug 22, 2013
  48. Johannes SixtAug 22, 2013
  49. repack: rewrite the shell script in C (squashing proposal)Stefan Beller, Aug 22, 2013
  50. Junio C HamanoAug 22, 2013
  51. Jonathan NiederAug 20, 2013
  52. Johannes SixtAug 21, 2013
  53. Stefan BellerAug 20, 2013
  54. Jonathan NiederAug 20, 2013
  55. Dokumenting api-paths.txtStefan Beller, Aug 20, 2013
  56. Jonathan NiederAug 20, 2013
  57. Stefan BellerAug 21, 2013
  58. Junio C HamanoAug 22, 2013
  59. Junio C HamanoAug 14, 2013
  60. Matthieu MoyAug 14, 2013
  61. Stefan BellerAug 15, 2013
  62. Duy NguyenAug 15, 2013
  63. Junio C HamanoAug 14, 2013
  64. Matthieu MoyAug 14, 2013
  65. Martin FickAug 14, 2013
  66. Martin FickAug 15, 2013
  67. Stefan BellerAug 15, 2013
  68. Martin FickAug 15, 2013
  69. Duy NguyenAug 15, 2013
  70. Junio C HamanoAug 14, 2013
  71. Stefan BellerAug 15, 2013
  72. Matthieu MoyAug 14, 2013

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.