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

[Stefan Beller <stefanbeller@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 9806 bytes --]


So here is an update of git-repack
Thanks for all the reviews and annotations!
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
I'll wait for the dokumentation patch of Jonathan, 
before changing all these occurences forth and back
again.

What would be perfect here would be a function
which just does string processing and returning,
so 
	fname = create_string(fmt, ...);
or with duplication:
	fname = create_string_dup(fmt, ...);

Ah wait! There are struct str_buf, but these
would require more lines (init, add to buffer, 
get as char*) 

Below there is just the diff against RFC PATCHv4,
however I'll send the whole patch as well.

Thanks,
Stefan

--8<--
From e544eb9b7bdea6c2000c5f0d3043845fb901e90b Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Wed, 21 Aug 2013 00:35:18 +0200
Subject: [PATCH] Suggestions of reviewers

---
 builtin/repack.c | 104 +++++++++++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 53 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index a87900e..9fbe636 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -67,7 +67,7 @@ void get_pack_filenames(char *packdir, struct string_list *fname_list)
 	struct dirent *e;
 	char *path, *suffix, *fname;
 
-	path = mkpathdup("%s/pack", get_object_directory());
+	path = mkpath("%s/pack", get_object_directory());
 	suffix = ".pack";
 
 	dir = opendir(path);
@@ -78,7 +78,6 @@ void get_pack_filenames(char *packdir, struct string_list *fname_list)
 			string_list_append_nodup(fname_list, fname);
 		}
 	}
-	free(path);
 	closedir(dir);
 }
 
@@ -88,14 +87,25 @@ void remove_pack(char *path, char* sha1)
 	int ext = 0;
 	for (ext = 0; ext < 3; ext++) {
 		char *fname;
-		fname = mkpathdup("%s/%s%s", path, sha1, exts[ext]);
+		fname = mkpath("%s/%s%s", path, sha1, exts[ext]);
 		unlink(fname);
-		free(fname);
 	}
 }
 
 int cmd_repack(int argc, const char **argv, const char *prefix) {
 
+	char *exts[2] = {".idx", ".pack"};
+	char *packdir, *packtmp, line[1024];
+	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;
+	FILE *out;
+
+	/* variables to be filled by option parsing */
 	int pack_everything = 0;
 	int pack_everything_but_loose = 0;
 	int delete_redundant = 0;
@@ -107,24 +117,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 	int no_update_server_info = 0;
 	int quiet = 0;
 	int local = 0;
-	char *packdir, *packtmp;
-	struct child_process cmd;
-	struct string_list_item *item;
-	struct string_list existing_packs = STRING_LIST_INIT_DUP;
-	struct stat statbuffer;
-	int ext;
-	char *exts[2] = {".idx", ".pack"};
 
 	struct option builtin_repack_options[] = {
-		OPT_BOOL('a', "all", &pack_everything,
+		OPT_BOOL('a', NULL, &pack_everything,
 				N_("pack everything in a single pack")),
-		OPT_BOOL('A', "all-but-loose", &pack_everything_but_loose,
+		OPT_BOOL('A', NULL, &pack_everything_but_loose,
 				N_("same as -a, and turn unreachable objects loose")),
-		OPT_BOOL('d', "delete-redundant", &delete_redundant,
+		OPT_BOOL('d', NULL, &delete_redundant,
 				N_("remove redundant packs, and run git-prune-packed")),
-		OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
+		OPT_BOOL('f', NULL, &no_reuse_delta,
 				N_("pass --no-reuse-delta to git-pack-objects")),
-		OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
+		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")),
@@ -154,9 +157,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 	packdir = mkpathdup("%s/pack", get_object_directory());
 	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, getpid());
 
-	remove_temporary_files();
-
-	struct argv_array cmd_args = ARGV_ARRAY_INIT;
 	argv_array_push(&cmd_args, "pack-objects");
 	argv_array_push(&cmd_args, "--keep-true-parents");
 	argv_array_push(&cmd_args, "--honor-pack-keep");
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 		for_each_string_list_item(item, &fname_list) {
 			char *fname;
 			fname = mkpathdup("%s/%s.keep", packdir, item->string);
-			if (stat(fname, &statbuffer) && S_ISREG(statbuffer.st_mode)) {
+			if (file_exists(fname)) {
 				/* when the keep file is there, we're ignoring that pack */
 			} else {
 				string_list_append(&existing_packs, item->string);
@@ -217,34 +217,34 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 	argv_array_push(&cmd_args, packtmp);
 
 	memset(&cmd, 0, sizeof(cmd));
-	cmd.argv = argv_array_detach(&cmd_args, NULL);
+	cmd.argv = cmd_args.argv;
 	cmd.git_cmd = 1;
 	cmd.out = -1;
 	cmd.no_stdin = 1;
 
-	if (run_command(&cmd))
+	if (start_command(&cmd))
 		return 1;
 
-	struct string_list names = STRING_LIST_INIT_DUP;
-	struct string_list rollback = STRING_LIST_INIT_DUP;
-
-	char line[1024];
-	int counter = 0;
-	FILE *out = xfdopen(cmd.out, "r");
+	count_packs = 0;
+	out = xfdopen(cmd.out, "r");
 	while (fgets(line, sizeof(line), out)) {
 		/* a line consists of 40 hex chars + '\n' */
-		assert(strlen(line) == 41);
+		if (strlen(line) != 41)
+			die("repack: Expecting 40 character sha1 lines only from pack-objects.");
 		line[40] = '\0';
 		string_list_append(&names, line);
-		counter++;
+		count_packs++;
 	}
-	if (!counter)
-		printf("Nothing new to pack.\n");
+	if (finish_command(&cmd))
+		return 1;
 	fclose(out);
 
+	if (!count_packs && !quiet)
+		printf("Nothing new to pack.\n");
+
 	int failed = 0;
 	for_each_string_list_item(item, &names) {
-		for (ext = 0; ext < 1; ext++) {
+		for (ext = 0; ext < 2; ext++) {
 			char *fname, *fname_old;
 			fname = mkpathdup("%s/%s%s", packdir, item->string, exts[ext]);
 			if (!file_exists(fname)) {
@@ -252,7 +252,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 				continue;
 			}
 
-			fname_old = mkpathdup("%s/old-%s%s", packdir, item->string, exts[ext]);
+			fname_old = mkpath("%s/old-%s%s", packdir, item->string, exts[ext]);
 			if (file_exists(fname_old))
 				unlink(fname_old);
 
@@ -260,23 +260,21 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 				failed = 1;
 				break;
 			}
-			free(fname_old);
 			string_list_append_nodup(&rollback, fname);
+			free(fname);
 		}
 		if (failed)
-			/* set to last element to break for_each loop */
-			item = names.items + names.nr;
+			break;
 	}
 	if (failed) {
 		struct string_list rollback_failure;
 		for_each_string_list_item(item, &rollback) {
 			char *fname, *fname_old;
 			fname = mkpathdup("%s/%s", packdir, item->string);
-			fname_old = mkpathdup("%s/old-%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);
-			free(fname_old);
 		}
 
 		if (rollback.nr) {
@@ -301,33 +299,33 @@ 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;
+			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]);
-			stat(fname_old, &statbuffer);
-			statbuffer.st_mode &= ~S_IWUSR | ~S_IWGRP | ~S_IWOTH;
-			chmod(fname_old, statbuffer.st_mode);
+			fname_old = mkpath("%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("Could not rename packfile: %s -> %s", 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 = mkpathdup("%s/old-pack-%s.idx", packdir, item->string);
+		fname = mkpath("%s/old-pack-%s.idx", packdir, item->string);
 		if (remove_path(fname))
-			die("Could not remove file: %s", fname);
-		free(fname);
+			die_errno(_("removing '%s' failed"), fname);
 
-		fname = mkpathdup("%s/old-pack-%s.pack", packdir, item->string);
+		fname = mkpath("%s/old-pack-%s.pack", packdir, item->string);
 		if (remove_path(fname))
-			die("Could not remove file: %s", fname);
-		free(fname);
+			die_errno(_("removing '%s' failed"), fname);
 	}
 
 	/* End of pack replacement. */
+
 	if (delete_redundant) {
 		sort_string_list(&names);
 		for_each_string_list_item(item, &existing_packs) {
@@ -345,7 +343,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 			argv_array_push(&cmd_args, "--quiet");
 
 		memset(&cmd, 0, sizeof(cmd));
-		cmd.argv = argv_array_detach(&cmd_args, NULL);
+		cmd.argv = cmd_args.argv;
 		cmd.git_cmd = 1;
 		run_command(&cmd);
 	}
@@ -355,7 +353,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) {
 		argv_array_push(&cmd_args, "update-server-info");
 
 		memset(&cmd, 0, sizeof(cmd));
-		cmd.argv = argv_array_detach(&cmd_args, NULL);
+		cmd.argv = cmd_args.argv;
 		cmd.git_cmd = 1;
 		run_command(&cmd);
 	}
-- 
1.8.4.rc3.1.gc1ebd90


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]