threads / patch / 34802

patch, 2 partsrepack: retain the return value of pack-objects

Subject: [PATCH 2/2] repack: retain the return value of pack-objects

## tl;dr

9 messages between Aug 29, 2013 and Sep 17, 2013. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Stefan Beller· Aug 29, 2013, 20:39 UTC · lore

[PATCHv7 0/2] Rewriting repack in C

Hi, I am sorry to have been delaying this patch series for a few days, as I was busy. I'll continue to be busy for the next 2 weeks, so my replies (if needed), will take some time.

The first patch rewrites the repack shell script in C and tries to resemble the functionality as close to the original as possible. The second patch introduces some functional changes intentionally.

Stefan
Here is a diff since the last time sending this patch series:
---
 builtin/repack.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)
Show changes to builtin/repack.c +19 −10
diff --git a/builtin/repack.c b/builtin/repack.c
index 0ace2a3..0cc823d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -41,10 +41,10 @@ static void remove_temporary_files(void)
 
 	strbuf_addstr(&buf, packdir);
 
-	/* dirlen holds the length of the path before the file name */
+	/* Point at the slash at the end of ".../objects/pack/" */
 	dirlen = buf.len + 1;
 	strbuf_addf(&buf, "%s", packtmp);
-	/* prefixlen holds the length of the prefix */
+	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
 	prefixlen = buf.len - dirlen;
 
 	while ((e = readdir(dir))) {
@@ -109,9 +109,12 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)
 	}
 }
 
+#define ALL_INTO_ONE 1
+#define LOOSE_UNREACHABLE 2
+
 int cmd_repack(int argc, const char **argv, const char *prefix)
 {
-	const char *exts[2] = {".idx", ".pack"};
+	const char *exts[2] = {".pack", ".idx"};
 	struct child_process cmd;
 	struct string_list_item *item;
 	struct argv_array cmd_args = ARGV_ARRAY_INIT;
@@ -124,7 +127,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	/* variables to be filled by option parsing */
 	int pack_everything = 0;
-	int pack_everything_but_loose = 0;
 	int delete_redundant = 0;
 	char *unpack_unreachable = NULL;
 	int window = 0, window_memory = 0;
@@ -136,10 +138,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	int local = 0;
 
 	struct option builtin_repack_options[] = {
-		OPT_BOOL('a', NULL, &pack_everything,
-				N_("pack everything in a single pack")),
-		OPT_BOOL('A', NULL, &pack_everything_but_loose,
-				N_("same as -a, and turn unreachable objects loose")),
+		OPT_BIT('a', NULL, &pack_everything,
+				N_("pack everything in a single pack"), ALL_INTO_ONE),
+		OPT_BIT('A', NULL, &pack_everything,
+				N_("same as -a, and turn unreachable objects loose"), LOOSE_UNREACHABLE),
 		OPT_BOOL('d', NULL, &delete_redundant,
 				N_("remove redundant packs, and run git-prune-packed")),
 		OPT_BOOL('f', NULL, &no_reuse_delta,
@@ -193,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (no_reuse_object)
 		argv_array_pushf(&cmd_args, "--no-reuse-object");
 
-	if (!pack_everything && !pack_everything_but_loose) {
+	if (!pack_everything) {
 		argv_array_push(&cmd_args, "--unpacked");
 		argv_array_push(&cmd_args, "--incremental");
 	} else {
@@ -204,7 +206,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				argv_array_pushf(&cmd_args,
 						"--unpack-unreachable=%s",
 						unpack_unreachable);
-			else if (pack_everything_but_loose)
+			else if (pack_everything & LOOSE_UNREACHABLE)
 				argv_array_push(&cmd_args,
 						"--unpack-unreachable");
 		}
@@ -246,6 +248,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (!nr_packs && !quiet)
 		printf("Nothing new to pack.\n");
 
+	/*
+	 * Ok we have prepared all new packfiles.
+	 * First see if there are packs of the same name and if so
+	 * if we can move them out of the way (this can happen if we
+	 * repacked immediately after packing fully.
+	 */
 	failed = 0;
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < 2; ext++) {
@@ -366,5 +374,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		run_command(&cmd);
 		argv_array_clear(&cmd_args);
 	}
+	remove_temporary_files();
 	return 0;
 }
-- 
1.8.4

Stefan Beller (2):
  repack: rewrite the shell script in C
  repack: retain the return value of pack-objects

 Makefile                                        |   2 +-
 builtin.h                                       |   1 +
 builtin/repack.c                                | 379 ++++++++++++++++++++++++
 git-repack.sh => contrib/examples/git-repack.sh |   0
 git.c                                           |   1 +
 5 files changed, 382 insertions(+), 1 deletion(-)
 create mode 100644 builtin/repack.c
 rename git-repack.sh => contrib/examples/git-repack.sh (100%)

-- 
1.8.4
Stefan Beller· Aug 29, 2013, 20:39 UTC · re: Stefan Beller · lore

[PATCH 1/2] repack: rewrite the shell script in C

The motivation of this patch is to get closer to a goal of being able to have a core subset of git functionality built in to git. That would mean

 * people on Windows could get a copy of at least the core parts
   of Git without having to install a Unix-style shell
 * people using git in on servers with chrooted environments
   do not need to worry about standard tools lacking for shell
   scripts.

This patch is meant to be mostly a literal translation of the git-repack script; the intent is that later patches would start using more library facilities, but this patch is meant to be as close to a no-op as possible so it doesn't do that kind of thing.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 Makefile                                        |   2 +-
 builtin.h                                       |   1 +
 builtin/repack.c                                | 379 ++++++++++++++++++++++++
 git-repack.sh => contrib/examples/git-repack.sh |   0
 git.c                                           |   1 +
 5 files changed, 382 insertions(+), 1 deletion(-)
 create mode 100644 builtin/repack.c
 rename git-repack.sh => contrib/examples/git-repack.sh (100%)
Show changes to 5 files +382 −1

Makefile, builtin.h, builtin/repack.c, git-repack.sh, git.c

diff --git a/Makefile b/Makefile
index 3588ca1..69e5267 100644
--- a/Makefile
+++ b/Makefile
@@ -464,7 +464,6 @@ SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
-SCRIPT_SH += git-repack.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
@@ -971,6 +970,7 @@ BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
 BUILTIN_OBJS += builtin/remote-ext.o
 BUILTIN_OBJS += builtin/remote-fd.o
+BUILTIN_OBJS += builtin/repack.o
 BUILTIN_OBJS += builtin/replace.o
 BUILTIN_OBJS += builtin/rerere.o
 BUILTIN_OBJS += builtin/reset.o
diff --git a/builtin.h b/builtin.h
index 8afa2de..b56cf07 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
+extern int cmd_repack(int argc, const char **argv, const char *prefix);
 extern int cmd_repo_config(int argc, const char **argv, const char *prefix);
 extern int cmd_rerere(int argc, const char **argv, const char *prefix);
 extern int cmd_reset(int argc, const char **argv, const char *prefix);
diff --git a/builtin/repack.c b/builtin/repack.c
new file mode 100644
index 0000000..0cc823d
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,379 @@
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+#include "parse-options.h"
+#include "run-command.h"
+#include "sigchain.h"
+#include "strbuf.h"
+#include "string-list.h"
+#include "argv-array.h"
+
+static int delta_base_offset = 1;
+static char *packdir, *packtmp;
+
+static const char *const git_repack_usage[] = {
+	N_("git repack [options]"),
+	NULL
+};
+
+static int repack_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "repack.usedeltabaseoffset")) {
+		delta_base_offset = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
+
+/*
+ * 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;
+
+	dir = opendir(packdir);
+	if (!dir)
+		return;
+
+	strbuf_addstr(&buf, packdir);
+
+	/* Point at the slash at the end of ".../objects/pack/" */
+	dirlen = buf.len + 1;
+	strbuf_addf(&buf, "%s", packtmp);
+	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
+	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);
+	}
+	closedir(dir);
+	strbuf_release(&buf);
+}
+
+static void remove_pack_on_signal(int signo)
+{
+	remove_temporary_files();
+	sigchain_pop(signo);
+	raise(signo);
+}
+
+/*
+ * Adds all packs hex strings to the fname list, which do not
+ * have a corresponding .keep file.
+ */
+static void get_non_kept_pack_filenames(struct string_list *fname_list)
+{
+	DIR *dir;
+	struct dirent *e;
+	char *fname;
+	size_t len;
+
+	if (!(dir = opendir(packdir)))
+		return;
+
+	while ((e = readdir(dir)) != NULL) {
+		if (suffixcmp(e->d_name, ".pack"))
+			continue;
+
+		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);
+}
+
+static void remove_redundant_pack(const char *path_prefix, const char *hex)
+{
+	const char *exts[] = {".pack", ".idx", ".keep"};
+	int i;
+	struct strbuf buf = STRBUF_INIT;
+	size_t plen;
+
+	strbuf_addf(&buf, "%s/%s", path_prefix, hex);
+	plen = buf.len;
+
+	for (i = 0; i < ARRAY_SIZE(exts); i++) {
+		strbuf_setlen(&buf, plen);
+		strbuf_addstr(&buf, exts[i]);
+		unlink(buf.buf);
+	}
+}
+
+#define ALL_INTO_ONE 1
+#define LOOSE_UNREACHABLE 2
+
+int cmd_repack(int argc, const char **argv, const char *prefix)
+{
+	const char *exts[2] = {".pack", ".idx"};
+	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_NODUP;
+	struct string_list existing_packs = STRING_LIST_INIT_DUP;
+	struct strbuf line = STRBUF_INIT;
+	int nr_packs, ext, ret, failed;
+	FILE *out;
+
+	/* variables to be filled by option parsing */
+	int pack_everything = 0;
+	int delete_redundant = 0;
+	char *unpack_unreachable = NULL;
+	int window = 0, window_memory = 0;
+	int depth = 0;
+	int max_pack_size = 0;
+	int no_reuse_delta = 0, no_reuse_object = 0;
+	int no_update_server_info = 0;
+	int quiet = 0;
+	int local = 0;
+
+	struct option builtin_repack_options[] = {
+		OPT_BIT('a', NULL, &pack_everything,
+				N_("pack everything in a single pack"), ALL_INTO_ONE),
+		OPT_BIT('A', NULL, &pack_everything,
+				N_("same as -a, and turn unreachable objects loose"), LOOSE_UNREACHABLE),
+		OPT_BOOL('d', NULL, &delete_redundant,
+				N_("remove redundant packs, and run git-prune-packed")),
+		OPT_BOOL('f', NULL, &no_reuse_delta,
+				N_("pass --no-reuse-delta to git-pack-objects")),
+		OPT_BOOL('F', NULL, &no_reuse_object,
+				N_("pass --no-reuse-object to git-pack-objects")),
+		OPT_BOOL('n', NULL, &no_update_server_info,
+				N_("do not run git-update-server-info")),
+		OPT__QUIET(&quiet, N_("be quiet")),
+		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")),
+		OPT_INTEGER(0, "window", &window,
+				N_("size of the window used for delta compression")),
+		OPT_INTEGER(0, "window-memory", &window_memory,
+				N_("same as the above, but limit memory size instead of entries count")),
+		OPT_INTEGER(0, "depth", &depth,
+				N_("limits the maximum delta depth")),
+		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+				N_("maximum size of each packfile")),
+		OPT_END()
+	};
+
+	git_config(repack_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, builtin_repack_options,
+				git_repack_usage, 0);
+
+	packdir = mkpathdup("%s/pack", get_object_directory());
+	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
+
+	sigchain_push_common(remove_pack_on_signal);
+
+	argv_array_push(&cmd_args, "pack-objects");
+	argv_array_push(&cmd_args, "--keep-true-parents");
+	argv_array_push(&cmd_args, "--honor-pack-keep");
+	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) {
+		argv_array_push(&cmd_args, "--unpacked");
+		argv_array_push(&cmd_args, "--incremental");
+	} else {
+		get_non_kept_pack_filenames(&existing_packs);
+
+		if (existing_packs.nr && delete_redundant) {
+			if (unpack_unreachable)
+				argv_array_pushf(&cmd_args,
+						"--unpack-unreachable=%s",
+						unpack_unreachable);
+			else if (pack_everything & LOOSE_UNREACHABLE)
+				argv_array_push(&cmd_args,
+						"--unpack-unreachable");
+		}
+	}
+
+	if (local)
+		argv_array_push(&cmd_args,  "--local");
+	if (quiet)
+		argv_array_push(&cmd_args,  "--quiet");
+	if (delta_base_offset)
+		argv_array_push(&cmd_args,  "--delta-base-offset");
+
+	argv_array_push(&cmd_args, packtmp);
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.argv = cmd_args.argv;
+	cmd.git_cmd = 1;
+	cmd.out = -1;
+	cmd.no_stdin = 1;
+
+	ret = start_command(&cmd);
+	if (ret)
+		return 1;
+
+	nr_packs = 0;
+	out = xfdopen(cmd.out, "r");
+	while (strbuf_getline(&line, out, '\n') != EOF) {
+		if (line.len != 40)
+			die("repack: Expecting 40 character sha1 lines only from pack-objects.");
+		string_list_append(&names, line.buf);
+		nr_packs++;
+	}
+	fclose(out);
+	ret = finish_command(&cmd);
+	if (ret)
+		return 1;
+	argv_array_clear(&cmd_args);
+
+	if (!nr_packs && !quiet)
+		printf("Nothing new to pack.\n");
+
+	/*
+	 * Ok we have prepared all new packfiles.
+	 * First see if there are packs of the same name and if so
+	 * if we can move them out of the way (this can happen if we
+	 * repacked immediately after packing fully.
+	 */
+	failed = 0;
+	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]);
+			if (!file_exists(fname)) {
+				free(fname);
+				continue;
+			}
+
+			fname_old = mkpath("%s/old-%s%s", packdir,
+						item->string, exts[ext]);
+			if (file_exists(fname_old))
+				if (unlink(fname_old))
+					failed = 1;
+
+			if (!failed && rename(fname, fname_old)) {
+				failed = 1;
+				break;
+			}
+			string_list_append(&rollback, fname);
+		}
+		if (failed)
+			break;
+	}
+	if (failed) {
+		struct string_list rollback_failure = STRING_LIST_INIT_DUP;
+		for_each_string_list_item(item, &rollback) {
+			char *fname, *fname_old;
+			fname = mkpathdup("%s/%s", packdir, item->string);
+			fname_old = mkpath("%s/old-%s", packdir, item->string);
+			if (rename(fname_old, fname))
+				string_list_append(&rollback_failure, fname);
+			free(fname);
+		}
+
+		if (rollback_failure.nr) {
+			int i;
+			fprintf(stderr,
+				"WARNING: Some packs in use have been renamed by\n"
+				"WARNING: prefixing old- to their name, in order to\n"
+				"WARNING: replace them with the new version of the\n"
+				"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 %s manually:\n", packdir);
+			for (i = 0; i < rollback_failure.nr; i++)
+				fprintf(stderr, "WARNING:   old-%s -> %s\n",
+					rollback_failure.items[i].string,
+					rollback_failure.items[i].string);
+		}
+		exit(1);
+	}
+
+	/* Now the ones with the same name are out of the way... */
+	for_each_string_list_item(item, &names) {
+		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 = mkpathdup("%s-%s%s",
+					packtmp, item->string, exts[ext]);
+			if (!stat(fname_old, &statbuffer)) {
+				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) {
+		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. */
+
+	if (delete_redundant) {
+		sort_string_list(&names);
+		for_each_string_list_item(item, &existing_packs) {
+			char *sha1;
+			size_t len = strlen(item->string);
+			if (len < 40)
+				continue;
+			sha1 = item->string + len - 40;
+			if (!string_list_has_string(&names, sha1))
+				remove_redundant_pack(packdir, item->string);
+		}
+		argv_array_push(&cmd_args, "prune-packed");
+		if (quiet)
+			argv_array_push(&cmd_args, "--quiet");
+
+		memset(&cmd, 0, sizeof(cmd));
+		cmd.argv = cmd_args.argv;
+		cmd.git_cmd = 1;
+		run_command(&cmd);
+		argv_array_clear(&cmd_args);
+	}
+
+	if (!no_update_server_info) {
+		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);
+	}
+	remove_temporary_files();
+	return 0;
+}
diff --git a/git-repack.sh b/contrib/examples/git-repack.sh
similarity index 100%
rename from git-repack.sh
rename to contrib/examples/git-repack.sh
diff --git a/git.c b/git.c
index 2025f77..03510be 100644
--- a/git.c
+++ b/git.c
@@ -396,6 +396,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "remote", cmd_remote, RUN_SETUP },
 		{ "remote-ext", cmd_remote_ext },
 		{ "remote-fd", cmd_remote_fd },
+		{ "repack", cmd_repack, RUN_SETUP },
 		{ "replace", cmd_replace, RUN_SETUP },
 		{ "repo-config", cmd_repo_config, RUN_SETUP_GENTLY },
 		{ "rerere", cmd_rerere, RUN_SETUP },
-- 
1.8.4
Felipe Contreras· Aug 29, 2013, 21:04 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] repack: rewrite the shell script in C

On Thu, Aug 29, 2013 at 3:39 PM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 6 quoted lines
> The motivation of this patch is to get closer to a goal of being
> able to have a core subset of git functionality built in to git.
> That would mean
>
>  * people on Windows could get a copy of at least the core parts
>    of Git without having to install a Unix-style shell

I think this is great, I'm looking forward to improve the situation in this regard.

Do you have in mind any other command that should also be replaced this way?
> This patch is meant to be mostly a literal translation of the
> git-repack script; the intent is that later patches would start using
> more library facilities, but this patch is meant to be as close to a
> no-op as possible so it doesn't do that kind of thing.

I'm not sure if this has been tackled already, or you could take a look into that, but:

http://article.gmane.org/gmane.comp.version-control.git/147190
Cheers.
-- 
Felipe Contreras
René Scharfe· Sep 15, 2013, 11:42 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] repack: rewrite the shell script in C

Am 29.08.2013 22:39, schrieb Stefan Beller:
Show 107 quoted lines
> The motivation of this patch is to get closer to a goal of being
> able to have a core subset of git functionality built in to git.
> That would mean
>
>  * people on Windows could get a copy of at least the core parts
>    of Git without having to install a Unix-style shell
>
>  * people using git in on servers with chrooted environments
>    do not need to worry about standard tools lacking for shell
>    scripts.
>
> This patch is meant to be mostly a literal translation of the
> git-repack script; the intent is that later patches would start using
> more library facilities, but this patch is meant to be as close to a
> no-op as possible so it doesn't do that kind of thing.
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
>  Makefile                                        |   2 +-
>  builtin.h                                       |   1 +
>  builtin/repack.c                                | 379 ++++++++++++++++++++++++
>  git-repack.sh => contrib/examples/git-repack.sh |   0
>  git.c                                           |   1 +
>  5 files changed, 382 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/repack.c
>  rename git-repack.sh => contrib/examples/git-repack.sh (100%)
>
> diff --git a/Makefile b/Makefile
> index 3588ca1..69e5267 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -464,7 +464,6 @@ SCRIPT_SH += git-pull.sh
>  SCRIPT_SH += git-quiltimport.sh
>  SCRIPT_SH += git-rebase.sh
>  SCRIPT_SH += git-remote-testgit.sh
> -SCRIPT_SH += git-repack.sh
>  SCRIPT_SH += git-request-pull.sh
>  SCRIPT_SH += git-stash.sh
>  SCRIPT_SH += git-submodule.sh
> @@ -971,6 +970,7 @@ BUILTIN_OBJS += builtin/reflog.o
>  BUILTIN_OBJS += builtin/remote.o
>  BUILTIN_OBJS += builtin/remote-ext.o
>  BUILTIN_OBJS += builtin/remote-fd.o
> +BUILTIN_OBJS += builtin/repack.o
>  BUILTIN_OBJS += builtin/replace.o
>  BUILTIN_OBJS += builtin/rerere.o
>  BUILTIN_OBJS += builtin/reset.o
> diff --git a/builtin.h b/builtin.h
> index 8afa2de..b56cf07 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -102,6 +102,7 @@ extern int cmd_reflog(int argc, const char **argv, const char *prefix);
>  extern int cmd_remote(int argc, const char **argv, const char *prefix);
>  extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
>  extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
> +extern int cmd_repack(int argc, const char **argv, const char *prefix);
>  extern int cmd_repo_config(int argc, const char **argv, const char *prefix);
>  extern int cmd_rerere(int argc, const char **argv, const char *prefix);
>  extern int cmd_reset(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/repack.c b/builtin/repack.c
> new file mode 100644
> index 0000000..0cc823d
> --- /dev/null
> +++ b/builtin/repack.c
> @@ -0,0 +1,379 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +#include "parse-options.h"
> +#include "run-command.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +#include "string-list.h"
> +#include "argv-array.h"
> +
> +static int delta_base_offset = 1;
> +static char *packdir, *packtmp;
> +
> +static const char *const git_repack_usage[] = {
> +	N_("git repack [options]"),
> +	NULL
> +};
> +
> +static int repack_config(const char *var, const char *value, void *cb)
> +{
> +	if (!strcmp(var, "repack.usedeltabaseoffset")) {
> +		delta_base_offset = git_config_bool(var, value);
> +		return 0;
> +	}
> +	return git_default_config(var, value, cb);
> +}
> +
> +/*
> + * 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;
> +
> +	dir = opendir(packdir);
> +	if (!dir)
> +		return;
> +
> +	strbuf_addstr(&buf, packdir);

Let's say packdir is ".git/objects/pack" (no trailing slash). Then buf is ".git/objects/pack" now as well.

> +
> +	/* Point at the slash at the end of ".../objects/pack/" */
> +	dirlen = buf.len + 1;
> +	strbuf_addf(&buf, "%s", packtmp);

packtmp starts with packdir, so by adding it here we have it twice. buf is now ".git/objects/pack.git/objects/pack/.tmp-123-pack" (if our pid is 123), no?

Perhaps don't add the packdir to the strbuf in the first place and calculate dirlen as strlen(packdir) + 1?

Besided, strbuf_addstr(x, y) is better for adding a string than strbuf_addf(x, "%s", y).

> +	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
> +	prefixlen = buf.len - dirlen;
This is the length of "git/objects/pack/.tmp-123-pack" now.
Show 42 quoted lines
> +
> +	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);
> +	}
> +	closedir(dir);
> +	strbuf_release(&buf);
> +}
> +
> +static void remove_pack_on_signal(int signo)
> +{
> +	remove_temporary_files();
> +	sigchain_pop(signo);
> +	raise(signo);
> +}
> +
> +/*
> + * Adds all packs hex strings to the fname list, which do not
> + * have a corresponding .keep file.
> + */
> +static void get_non_kept_pack_filenames(struct string_list *fname_list)
> +{
> +	DIR *dir;
> +	struct dirent *e;
> +	char *fname;
> +	size_t len;
> +
> +	if (!(dir = opendir(packdir)))
> +		return;
> +
> +	while ((e = readdir(dir)) != NULL) {
> +		if (suffixcmp(e->d_name, ".pack"))
> +			continue;
> +
> +		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);
This leaks fname for packs with .keep files.
Show 20 quoted lines
> +	}
> +	closedir(dir);
> +}
> +
> +static void remove_redundant_pack(const char *path_prefix, const char *hex)
> +{
> +	const char *exts[] = {".pack", ".idx", ".keep"};
> +	int i;
> +	struct strbuf buf = STRBUF_INIT;
> +	size_t plen;
> +
> +	strbuf_addf(&buf, "%s/%s", path_prefix, hex);
> +	plen = buf.len;
> +
> +	for (i = 0; i < ARRAY_SIZE(exts); i++) {
> +		strbuf_setlen(&buf, plen);
> +		strbuf_addstr(&buf, exts[i]);
> +		unlink(buf.buf);
> +	}
> +}

hex must also include the "pack-" prefix and path_prefix must be a directory name. Perhaps call them base_name and dir_name or similar to make that clearer?

buf is leaked.
Show 166 quoted lines
> +
> +#define ALL_INTO_ONE 1
> +#define LOOSE_UNREACHABLE 2
> +
> +int cmd_repack(int argc, const char **argv, const char *prefix)
> +{
> +	const char *exts[2] = {".pack", ".idx"};
> +	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_NODUP;
> +	struct string_list existing_packs = STRING_LIST_INIT_DUP;
> +	struct strbuf line = STRBUF_INIT;
> +	int nr_packs, ext, ret, failed;
> +	FILE *out;
> +
> +	/* variables to be filled by option parsing */
> +	int pack_everything = 0;
> +	int delete_redundant = 0;
> +	char *unpack_unreachable = NULL;
> +	int window = 0, window_memory = 0;
> +	int depth = 0;
> +	int max_pack_size = 0;
> +	int no_reuse_delta = 0, no_reuse_object = 0;
> +	int no_update_server_info = 0;
> +	int quiet = 0;
> +	int local = 0;
> +
> +	struct option builtin_repack_options[] = {
> +		OPT_BIT('a', NULL, &pack_everything,
> +				N_("pack everything in a single pack"), ALL_INTO_ONE),
> +		OPT_BIT('A', NULL, &pack_everything,
> +				N_("same as -a, and turn unreachable objects loose"), LOOSE_UNREACHABLE),
> +		OPT_BOOL('d', NULL, &delete_redundant,
> +				N_("remove redundant packs, and run git-prune-packed")),
> +		OPT_BOOL('f', NULL, &no_reuse_delta,
> +				N_("pass --no-reuse-delta to git-pack-objects")),
> +		OPT_BOOL('F', NULL, &no_reuse_object,
> +				N_("pass --no-reuse-object to git-pack-objects")),
> +		OPT_BOOL('n', NULL, &no_update_server_info,
> +				N_("do not run git-update-server-info")),
> +		OPT__QUIET(&quiet, N_("be quiet")),
> +		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")),
> +		OPT_INTEGER(0, "window", &window,
> +				N_("size of the window used for delta compression")),
> +		OPT_INTEGER(0, "window-memory", &window_memory,
> +				N_("same as the above, but limit memory size instead of entries count")),
> +		OPT_INTEGER(0, "depth", &depth,
> +				N_("limits the maximum delta depth")),
> +		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
> +				N_("maximum size of each packfile")),
> +		OPT_END()
> +	};
> +
> +	git_config(repack_config, NULL);
> +
> +	argc = parse_options(argc, argv, prefix, builtin_repack_options,
> +				git_repack_usage, 0);
> +
> +	packdir = mkpathdup("%s/pack", get_object_directory());
> +	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
> +
> +	sigchain_push_common(remove_pack_on_signal);
> +
> +	argv_array_push(&cmd_args, "pack-objects");
> +	argv_array_push(&cmd_args, "--keep-true-parents");
> +	argv_array_push(&cmd_args, "--honor-pack-keep");
> +	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) {
> +		argv_array_push(&cmd_args, "--unpacked");
> +		argv_array_push(&cmd_args, "--incremental");
> +	} else {
> +		get_non_kept_pack_filenames(&existing_packs);
> +
> +		if (existing_packs.nr && delete_redundant) {
> +			if (unpack_unreachable)
> +				argv_array_pushf(&cmd_args,
> +						"--unpack-unreachable=%s",
> +						unpack_unreachable);
> +			else if (pack_everything & LOOSE_UNREACHABLE)
> +				argv_array_push(&cmd_args,
> +						"--unpack-unreachable");
> +		}
> +	}
> +
> +	if (local)
> +		argv_array_push(&cmd_args,  "--local");
> +	if (quiet)
> +		argv_array_push(&cmd_args,  "--quiet");
> +	if (delta_base_offset)
> +		argv_array_push(&cmd_args,  "--delta-base-offset");
> +
> +	argv_array_push(&cmd_args, packtmp);
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.argv = cmd_args.argv;
> +	cmd.git_cmd = 1;
> +	cmd.out = -1;
> +	cmd.no_stdin = 1;
> +
> +	ret = start_command(&cmd);
> +	if (ret)
> +		return 1;
> +
> +	nr_packs = 0;
> +	out = xfdopen(cmd.out, "r");
> +	while (strbuf_getline(&line, out, '\n') != EOF) {
> +		if (line.len != 40)
> +			die("repack: Expecting 40 character sha1 lines only from pack-objects.");
> +		string_list_append(&names, line.buf);
> +		nr_packs++;
> +	}
> +	fclose(out);
> +	ret = finish_command(&cmd);
> +	if (ret)
> +		return 1;
> +	argv_array_clear(&cmd_args);
> +
> +	if (!nr_packs && !quiet)
> +		printf("Nothing new to pack.\n");
> +
> +	/*
> +	 * Ok we have prepared all new packfiles.
> +	 * First see if there are packs of the same name and if so
> +	 * if we can move them out of the way (this can happen if we
> +	 * repacked immediately after packing fully.
> +	 */
> +	failed = 0;
> +	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]);
> +			if (!file_exists(fname)) {
> +				free(fname);
> +				continue;
> +			}
> +
> +			fname_old = mkpath("%s/old-%s%s", packdir,
> +						item->string, exts[ext]);
> +			if (file_exists(fname_old))
> +				if (unlink(fname_old))
> +					failed = 1;
> +
> +			if (!failed && rename(fname, fname_old)) {
> +				failed = 1;
> +				break;
This leaks fname.
> +			}
> +			string_list_append(&rollback, fname);

Mental note: This is effectively string_list_append_nodup because rollback is initialized with STRING_LIST_INIT_NODUP, so there's no leak here.

Show 48 quoted lines
> +		}
> +		if (failed)
> +			break;
> +	}
> +	if (failed) {
> +		struct string_list rollback_failure = STRING_LIST_INIT_DUP;
> +		for_each_string_list_item(item, &rollback) {
> +			char *fname, *fname_old;
> +			fname = mkpathdup("%s/%s", packdir, item->string);
> +			fname_old = mkpath("%s/old-%s", packdir, item->string);
> +			if (rename(fname_old, fname))
> +				string_list_append(&rollback_failure, fname);
> +			free(fname);
> +		}
> +
> +		if (rollback_failure.nr) {
> +			int i;
> +			fprintf(stderr,
> +				"WARNING: Some packs in use have been renamed by\n"
> +				"WARNING: prefixing old- to their name, in order to\n"
> +				"WARNING: replace them with the new version of the\n"
> +				"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 %s manually:\n", packdir);
> +			for (i = 0; i < rollback_failure.nr; i++)
> +				fprintf(stderr, "WARNING:   old-%s -> %s\n",
> +					rollback_failure.items[i].string,
> +					rollback_failure.items[i].string);
> +		}
> +		exit(1);
> +	}
> +
> +	/* Now the ones with the same name are out of the way... */
> +	for_each_string_list_item(item, &names) {
> +		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 = mkpathdup("%s-%s%s",
> +					packtmp, item->string, exts[ext]);
> +			if (!stat(fname_old, &statbuffer)) {
> +				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);

The Shell script exits silently in that case. How about improving error reporting in a separate patch?

Show 15 quoted lines
> +			free(fname);
> +			free(fname_old);
> +		}
> +	}
> +
> +	/* Remove the "old-" files */
> +	for_each_string_list_item(item, &names) {
> +		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);
Similar case here: The Shell script continues silently.
Show 36 quoted lines
> +		}
> +	}
> +
> +	/* End of pack replacement. */
> +
> +	if (delete_redundant) {
> +		sort_string_list(&names);
> +		for_each_string_list_item(item, &existing_packs) {
> +			char *sha1;
> +			size_t len = strlen(item->string);
> +			if (len < 40)
> +				continue;
> +			sha1 = item->string + len - 40;
> +			if (!string_list_has_string(&names, sha1))
> +				remove_redundant_pack(packdir, item->string);
> +		}
> +		argv_array_push(&cmd_args, "prune-packed");
> +		if (quiet)
> +			argv_array_push(&cmd_args, "--quiet");
> +
> +		memset(&cmd, 0, sizeof(cmd));
> +		cmd.argv = cmd_args.argv;
> +		cmd.git_cmd = 1;
> +		run_command(&cmd);
> +		argv_array_clear(&cmd_args);
> +	}
> +
> +	if (!no_update_server_info) {
> +		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);
> +	}
> +	remove_temporary_files();

If you take care to clear the argv_arrays then perhaps do the same for the string_lists? The OS cleans up after us anyway, but calming Valgrind or similar leak checkers is a good practice nevertheless.

Show 20 quoted lines
> +	return 0;
> +}
> diff --git a/git-repack.sh b/contrib/examples/git-repack.sh
> similarity index 100%
> rename from git-repack.sh
> rename to contrib/examples/git-repack.sh
> diff --git a/git.c b/git.c
> index 2025f77..03510be 100644
> --- a/git.c
> +++ b/git.c
> @@ -396,6 +396,7 @@ static void handle_internal_command(int argc, const char **argv)
>  		{ "remote", cmd_remote, RUN_SETUP },
>  		{ "remote-ext", cmd_remote_ext },
>  		{ "remote-fd", cmd_remote_fd },
> +		{ "repack", cmd_repack, RUN_SETUP },
>  		{ "replace", cmd_replace, RUN_SETUP },
>  		{ "repo-config", cmd_repo_config, RUN_SETUP_GENTLY },
>  		{ "rerere", cmd_rerere, RUN_SETUP },
> -- 1.8.4
>
Stefan Beller· Sep 15, 2013, 15:31 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] repack: rewrite the shell script in C

Rene, thank you very much for the review!
On 09/15/2013 01:42 PM, René Scharfe wrote:
Show 30 quoted lines
>> +static void remove_temporary_files(void)
>> +{
>> +    struct strbuf buf = STRBUF_INIT;
>> +    size_t dirlen, prefixlen;
>> +    DIR *dir;
>> +    struct dirent *e;
>> +
>> +    dir = opendir(packdir);
>> +    if (!dir)
>> +        return;
>> +
>> +    strbuf_addstr(&buf, packdir);
> 
> Let's say packdir is ".git/objects/pack" (no trailing slash).  Then buf
> is ".git/objects/pack" now as well.
> 
>> +
>> +    /* Point at the slash at the end of ".../objects/pack/" */
>> +    dirlen = buf.len + 1;
>> +    strbuf_addf(&buf, "%s", packtmp);
> 
> packtmp starts with packdir, so by adding it here we have it twice.  buf
> is now ".git/objects/pack.git/objects/pack/.tmp-123-pack" (if our pid is
> 123), no?
> 
> Perhaps don't add the packdir to the strbuf in the first place and
> calculate dirlen as strlen(packdir) + 1?
> 
> Besided, strbuf_addstr(x, y) is better for adding a string than
> strbuf_addf(x, "%s", y).
Oops, thanks for catching that. I'll be sending a fixup commit.
Show 5 quoted lines
> 
>> +    /* Point at the dash at the end of ".../.tmp-%d-pack-" */
>> +    prefixlen = buf.len - dirlen;
> 
> This is the length of "git/objects/pack/.tmp-123-pack" now.
Also fixed.
Show 22 quoted lines
>> +static void get_non_kept_pack_filenames(struct string_list *fname_list)
>> +{
>> +    DIR *dir;
>> +    struct dirent *e;
>> +    char *fname;
>> +    size_t len;
>> +
>> +    if (!(dir = opendir(packdir)))
>> +        return;
>> +
>> +    while ((e = readdir(dir)) != NULL) {
>> +        if (suffixcmp(e->d_name, ".pack"))
>> +            continue;
>> +
>> +        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);
> 
> This leaks fname for packs with .keep files.
> 
fixed.
Show 23 quoted lines
>> +static void remove_redundant_pack(const char *path_prefix, const char
>> *hex)
>> +{
>> +    const char *exts[] = {".pack", ".idx", ".keep"};
>> +    int i;
>> +    struct strbuf buf = STRBUF_INIT;
>> +    size_t plen;
>> +
>> +    strbuf_addf(&buf, "%s/%s", path_prefix, hex);
>> +    plen = buf.len;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(exts); i++) {
>> +        strbuf_setlen(&buf, plen);
>> +        strbuf_addstr(&buf, exts[i]);
>> +        unlink(buf.buf);
>> +    }
>> +}
> 
> hex must also include the "pack-" prefix and path_prefix must be a
> directory name.  Perhaps call them base_name and dir_name or similar to
> make that clearer?
> 
> buf is leaked.
buf will be freed.

the parameter hex contains the "pack-" already. The remove_redundant_pack function is called in a loop iterating over elements of existing_packs, which is filled in get_non_kept_pack_filenames, which doesn't fill in the hex, but filenames without extension.

I'll rename the variables to base_name and dir_name as you suggested.
Show 22 quoted lines
>> +    failed = 0;
>> +    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]);
>> +            if (!file_exists(fname)) {
>> +                free(fname);
>> +                continue;
>> +            }
>> +
>> +            fname_old = mkpath("%s/old-%s%s", packdir,
>> +                        item->string, exts[ext]);
>> +            if (file_exists(fname_old))
>> +                if (unlink(fname_old))
>> +                    failed = 1;
>> +
>> +            if (!failed && rename(fname, fname_old)) {
>> +                failed = 1;
>> +                break;
> 
> This leaks fname.
will fix.
Show 5 quoted lines
>> +            if (rename(fname_old, fname))
>> +                die_errno(_("renaming '%s' failed"), fname_old);
> 
> The Shell script exits silently in that case.  How about 	improving error
> reporting in a separate patch?
Moved out of the main patch.
Show 5 quoted lines
>> +            if (remove_path(fname))
>> +                warning(_("removing '%s' failed"), fname);
> 
> Similar case here: The Shell script continues silently.
> 
here as well.
Show 5 quoted lines
> 
> If you take care to clear the argv_arrays then perhaps do the same for
> the string_lists?  The OS cleans up after us anyway, but calming
> Valgrind or similar leak checkers is a good practice nevertheless.
> 
I'll do it.
So here are the changes, I'll resend the series as well.
--8<--
From 63c94df8c74c6643d6291c324661a939b9945619 Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Sun, 15 Sep 2013 17:05:49 +0200
Subject: [PATCH 1/2] Suggestions by Rene
---
 builtin/repack.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)
Show changes to builtin/repack.c +16 −9
diff --git a/builtin/repack.c b/builtin/repack.c
index a0314be..d345d16 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -39,12 +39,10 @@ static void remove_temporary_files(void)
 	if (!dir)
 		return;
 
-	strbuf_addstr(&buf, packdir);
-
 	/* Point at the slash at the end of ".../objects/pack/" */
-	dirlen = buf.len + 1;
-	strbuf_addf(&buf, "%s", packtmp);
-	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
+	dirlen = strlen(packdir) + 1;
+	strbuf_addstr(&buf, packtmp);
+	/* Hold the length of  ".tmp-%d-pack-" */
 	prefixlen = buf.len - dirlen;
 
 	while ((e = readdir(dir))) {
@@ -88,6 +86,8 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
 
 		if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
 			string_list_append_nodup(fname_list, fname);
+		else
+			free(fname);
 	}
 	closedir(dir);
 }
@@ -107,6 +107,7 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)
 		strbuf_addstr(&buf, exts[i]);
 		unlink(buf.buf);
 	}
+	strbuf_release(&buf);
 }
 
 #define ALL_INTO_ONE 1
@@ -273,10 +274,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 					failed = 1;
 
 			if (!failed && rename(fname, fname_old)) {
+				free(fname);
 				failed = 1;
 				break;
+			} else {
+				string_list_append(&rollback, fname);
 			}
-			string_list_append(&rollback, fname);
 		}
 		if (failed)
 			break;
@@ -324,7 +327,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				chmod(fname_old, statbuffer.st_mode);
 			}
 			if (rename(fname_old, fname))
-				die_errno(_("renaming '%s' failed"), fname_old);
+				exit(errno);
 			free(fname);
 			free(fname_old);
 		}
@@ -338,8 +341,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 					packdir,
 					item->string,
 					exts[ext]);
-			if (remove_path(fname))
-				warning(_("removing '%s' failed"), fname);
+			remove_path(fname);
 		}
 	}
 
@@ -376,5 +378,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		argv_array_clear(&cmd_args);
 	}
 	remove_temporary_files();
+	string_list_clear(&names, 0);
+	string_list_clear(&rollback, 0);
+	string_list_clear(&existing_packs, 0);
+	strbuf_release(&line);
+
 	return 0;
 }
-- 
1.8.4.273.ga194ead
Stefan Beller· Sep 17, 2013, 20:15 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] repack: rewrite the shell script in C

On 09/15/2013 05:31 PM, Stefan Beller wrote:
> Rene, thank you very much for the review!
Show 9 quoted lines
> 
> the parameter hex contains the "pack-" already.
> The remove_redundant_pack function is called in a loop iterating over
> elements of existing_packs, which is filled in get_non_kept_pack_filenames,
> which doesn't fill in the hex, but filenames without extension.
> 
> I'll rename the variables to base_name and dir_name as you suggested.
> 
> 

I did write a lengthy paragraph, but forgot to actually to it. ---8<---

From 92af79a5a89929d8834ff6d585c0455c867a774f Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Tue, 17 Sep 2013 22:04:14 +0200
Subject: [PATCH 1/2] Fixup missing by Rene.
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/repack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/repack.c +2 −2
diff --git a/builtin/repack.c b/builtin/repack.c
index f7e91bf..e5f90c6 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -92,14 +92,14 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
 	closedir(dir);
 }
 
-static void remove_redundant_pack(const char *path_prefix, const char *hex)
+static void remove_redundant_pack(const char *dir_name, const char *base_name)
 {
 	const char *exts[] = {".pack", ".idx", ".keep"};
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 	size_t plen;
 
-	strbuf_addf(&buf, "%s/%s", path_prefix, hex);
+	strbuf_addf(&buf, "%s/%s", dir_name, base_name);
 	plen = buf.len;
 
 	for (i = 0; i < ARRAY_SIZE(exts); i++) {
-- 
1.8.4.273.ga194ead
Stefan Beller· Aug 29, 2013, 20:39 UTC · re: Stefan Beller · lore

During the review process of the previous commit (repack: rewrite the shell script in C), Johannes Sixt proposed to retain any exit codes from the sub-process, which makes it probably more obvious in case of failure.

As the commit before should behave as close to the original shell script, the proposed change is put in this extra commit. The infrastructure however was already setup in the previous commit. (Having a local 'ret' variable)

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/repack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/repack.c +2 −2
diff --git a/builtin/repack.c b/builtin/repack.c
index 0cc823d..2fb3b91 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -229,7 +229,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	ret = start_command(&cmd);
 	if (ret)
-		return 1;
+		return ret;
 
 	nr_packs = 0;
 	out = xfdopen(cmd.out, "r");
@@ -242,7 +242,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	fclose(out);
 	ret = finish_command(&cmd);
 	if (ret)
-		return 1;
+		return ret;
 	argv_array_clear(&cmd_args);
 
 	if (!nr_packs && !quiet)
-- 
1.8.4
Junio C Hamano· Aug 29, 2013, 20:53 UTC · re: Stefan Beller · lore

Re: [PATCHv7 0/2] Rewriting repack in C

Stefan Beller <stefanbeller@googlemail.com> writes:
> Here is a diff since the last time sending this patch series:
This is very readable.

There may be people who misread "LOOSE" as "LOSE"; the option -A is about making the unreachable ones loose so that they can be expired, so let's rename it LOOSEN_UNREACHABLE to avoid confusion.

Thanks.
Show 108 quoted lines
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0ace2a3..0cc823d 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -41,10 +41,10 @@ static void remove_temporary_files(void)
>  
>  	strbuf_addstr(&buf, packdir);
>  
> -	/* dirlen holds the length of the path before the file name */
> +	/* Point at the slash at the end of ".../objects/pack/" */
>  	dirlen = buf.len + 1;
>  	strbuf_addf(&buf, "%s", packtmp);
> -	/* prefixlen holds the length of the prefix */
> +	/* Point at the dash at the end of ".../.tmp-%d-pack-" */
>  	prefixlen = buf.len - dirlen;
>  
>  	while ((e = readdir(dir))) {
> @@ -109,9 +109,12 @@ static void remove_redundant_pack(const char *path_prefix, const char *hex)
>  	}
>  }
>  
> +#define ALL_INTO_ONE 1
> +#define LOOSE_UNREACHABLE 2
> +
>  int cmd_repack(int argc, const char **argv, const char *prefix)
>  {
> -	const char *exts[2] = {".idx", ".pack"};
> +	const char *exts[2] = {".pack", ".idx"};
>  	struct child_process cmd;
>  	struct string_list_item *item;
>  	struct argv_array cmd_args = ARGV_ARRAY_INIT;
> @@ -124,7 +127,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  
>  	/* variables to be filled by option parsing */
>  	int pack_everything = 0;
> -	int pack_everything_but_loose = 0;
>  	int delete_redundant = 0;
>  	char *unpack_unreachable = NULL;
>  	int window = 0, window_memory = 0;
> @@ -136,10 +138,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	int local = 0;
>  
>  	struct option builtin_repack_options[] = {
> -		OPT_BOOL('a', NULL, &pack_everything,
> -				N_("pack everything in a single pack")),
> -		OPT_BOOL('A', NULL, &pack_everything_but_loose,
> -				N_("same as -a, and turn unreachable objects loose")),
> +		OPT_BIT('a', NULL, &pack_everything,
> +				N_("pack everything in a single pack"), ALL_INTO_ONE),
> +		OPT_BIT('A', NULL, &pack_everything,
> +				N_("same as -a, and turn unreachable objects loose"), LOOSE_UNREACHABLE),
>  		OPT_BOOL('d', NULL, &delete_redundant,
>  				N_("remove redundant packs, and run git-prune-packed")),
>  		OPT_BOOL('f', NULL, &no_reuse_delta,
> @@ -193,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (no_reuse_object)
>  		argv_array_pushf(&cmd_args, "--no-reuse-object");
>  
> -	if (!pack_everything && !pack_everything_but_loose) {
> +	if (!pack_everything) {
>  		argv_array_push(&cmd_args, "--unpacked");
>  		argv_array_push(&cmd_args, "--incremental");
>  	} else {
> @@ -204,7 +206,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  				argv_array_pushf(&cmd_args,
>  						"--unpack-unreachable=%s",
>  						unpack_unreachable);
> -			else if (pack_everything_but_loose)
> +			else if (pack_everything & LOOSE_UNREACHABLE)
>  				argv_array_push(&cmd_args,
>  						"--unpack-unreachable");
>  		}
> @@ -246,6 +248,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (!nr_packs && !quiet)
>  		printf("Nothing new to pack.\n");
>  
> +	/*
> +	 * Ok we have prepared all new packfiles.
> +	 * First see if there are packs of the same name and if so
> +	 * if we can move them out of the way (this can happen if we
> +	 * repacked immediately after packing fully.
> +	 */
>  	failed = 0;
>  	for_each_string_list_item(item, &names) {
>  		for (ext = 0; ext < 2; ext++) {
> @@ -366,5 +374,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		run_command(&cmd);
>  		argv_array_clear(&cmd_args);
>  	}
> +	remove_temporary_files();
>  	return 0;
>  }
> -- 
> 1.8.4
>
> Stefan Beller (2):
>   repack: rewrite the shell script in C
>   repack: retain the return value of pack-objects
>
>  Makefile                                        |   2 +-
>  builtin.h                                       |   1 +
>  builtin/repack.c                                | 379 ++++++++++++++++++++++++
>  git-repack.sh => contrib/examples/git-repack.sh |   0
>  git.c                                           |   1 +
>  5 files changed, 382 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/repack.c
>  rename git-repack.sh => contrib/examples/git-repack.sh (100%)
Stefan Beller· Aug 30, 2013, 07:17 UTC · re: Junio C Hamano · lore

Re: [PATCHv7 0/2] Rewriting repack in C

On 08/29/2013 10:53 PM, Junio C Hamano wrote:
Show 12 quoted lines
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> Here is a diff since the last time sending this patch series:
> 
> This is very readable.
> 
> There may be people who misread "LOOSE" as "LOSE"; the option -A is 
> about making the unreachable ones loose so that they can be expired,
> so let's rename it LOOSEN_UNREACHABLE to avoid confusion.
> 
> Thanks.
> 

This would be very appreciated. I am no native speaker, so please correct any variable naming, which could potentially be missleading.

I've seen you've already put a squashing proposal on top of origin/sb/repack-in-c, it looks great, so feel free to squash it into the first commit.

Thanks, Stefan

← back to recent threads