git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <stefanbeller@googlemail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, mfick@codeaurora.org, apelisse@gmail.com,
	Matthieu.Moy@grenoble-inp.fr, pclouds@gmail.com, iveqy@iveqy.com,
	gitster@pobox.com, mackyle@gmail.com, j6t@kdbg.org
Subject: Re: [PATCH] repack: rewrite the shell script in C.
Date: Wed, 21 Aug 2013 19:25:42 +0200	[thread overview]
Message-ID: <5214F816.3010303@googlemail.com> (raw)
In-Reply-To: <20130821082527.GC2802@elie.Belkin>

On 08/21/2013 10:25 AM, Jonathan Nieder wrote:
> 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

  parent reply	other threads:[~2013-08-21 17:25 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-13 19:23 [PATCH] Rewriting git-repack in C Stefan Beller
2013-08-13 19:23 ` [PATCH] repack: rewrite the shell script " Stefan Beller
2013-08-14  7:26   ` Matthieu Moy
2013-08-14 16:26     ` Stefan Beller
2013-08-14 16:27       ` [RFC PATCH] " Stefan Beller
2013-08-14 16:49         ` Antoine Pelisse
2013-08-14 17:04           ` Stefan Beller
2013-08-14 17:19             ` Jeff King
2013-08-14 17:25           ` Martin Fick
2013-08-14 22:16             ` Stefan Beller
2013-08-14 22:28               ` Martin Fick
2013-08-14 22:53                 ` Junio C Hamano
2013-08-14 23:28                   ` Martin Fick
2013-08-15 17:15                     ` Junio C Hamano
2013-08-16  0:12                       ` [RFC PATCHv2] " Stefan Beller
2013-08-17 13:34                         ` René Scharfe
2013-08-17 19:18                           ` Kyle J. McKay
2013-08-18 14:34                           ` Stefan Beller
2013-08-18 14:36                             ` [RFC PATCHv3] " Stefan Beller
2013-08-18 15:41                               ` Kyle J. McKay
2013-08-18 16:44                               ` René Scharfe
2013-08-18 22:26                                 ` [RFC PATCHv4] " Stefan Beller
2013-08-19 23:23                                   ` Stefan Beller
2013-08-20 13:31                                     ` Johannes Sixt
2013-08-20 15:08                                       ` Stefan Beller
2013-08-20 18:38                                         ` Johannes Sixt
2013-08-20 18:57                                         ` René Scharfe
2013-08-20 22:36                                           ` Stefan Beller
2013-08-20 22:38                                             ` [PATCH] " Stefan Beller
2013-08-21  8:25                                               ` Jonathan Nieder
2013-08-21 10:37                                                 ` Stefan Beller
2013-08-21 17:25                                                 ` Stefan Beller [this message]
2013-08-21 17:28                                                   ` [RFC PATCHv6 1/2] " Stefan Beller
2013-08-21 17:28                                                     ` [RFC PATCHv6 2/2] repack: retain the return value of pack-objects Stefan Beller
2013-08-21 20:56                                                     ` [RFC PATCHv6 1/2] repack: rewrite the shell script in C Junio C Hamano
2013-08-21 21:52                                                       ` Matthieu Moy
2013-08-21 22:15                                                       ` Stefan Beller
2013-08-21 22:50                                                         ` Junio C Hamano
2013-08-21 22:57                                                           ` Stefan Beller
2013-08-22 10:46                                                         ` Johannes Sixt
2013-08-22 21:03                                                       ` Jonathan Nieder
2013-08-21  8:49                                               ` [PATCH] " Matthieu Moy
2013-08-21 12:47                                                 ` Stefan Beller
2013-08-21 13:05                                                   ` Matthieu Moy
2013-08-21 12:53                                                 ` Stefan Beller
2013-08-21 13:07                                                   ` Matthieu Moy
2013-08-22 10:46                                                     ` Johannes Sixt
2013-08-22 10:46                                                 ` Johannes Sixt
2013-08-22 20:06                                                   ` [PATCH] repack: rewrite the shell script in C (squashing proposal) Stefan Beller
2013-08-22 20:31                                                     ` Junio C Hamano
2013-08-20 22:46                                             ` [RFC PATCHv4] repack: rewrite the shell script in C Jonathan Nieder
2013-08-21  9:20                                             ` Johannes Sixt
2013-08-20 21:24                                       ` Stefan Beller
2013-08-20 21:34                                         ` Jonathan Nieder
2013-08-20 21:40                                           ` Dokumenting api-paths.txt Stefan Beller
2013-08-20 21:59                                             ` Jonathan Nieder
2013-08-21 22:43                                               ` Stefan Beller
2013-08-22 17:29                                                 ` Junio C Hamano
2013-08-14 22:51               ` [RFC PATCH] repack: rewrite the shell script in C Junio C Hamano
2013-08-14 22:59                 ` Matthieu Moy
2013-08-15  7:47                   ` Stefan Beller
2013-08-15  4:15             ` Duy Nguyen
2013-08-14 17:26           ` Junio C Hamano
2013-08-14 22:51           ` Matthieu Moy
2013-08-14 23:25             ` Martin Fick
2013-08-15  0:26               ` Martin Fick
2013-08-15  7:46               ` Stefan Beller
2013-08-15 15:04                 ` Martin Fick
2013-08-15  4:20             ` Duy Nguyen
2013-08-14 17:04         ` Junio C Hamano
2013-08-15  7:53           ` Stefan Beller
2013-08-14  7:12 ` [PATCH] Rewriting git-repack " Matthieu Moy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5214F816.3010303@googlemail.com \
    --to=stefanbeller@googlemail.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=apelisse@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=iveqy@iveqy.com \
    --cc=j6t@kdbg.org \
    --cc=jrnieder@gmail.com \
    --cc=mackyle@gmail.com \
    --cc=mfick@codeaurora.org \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).