* [PATCH 1/3] repack: rewrite the shell script in C
@ 2013-09-15 15:33 Stefan Beller
2013-09-15 15:33 ` [PATCH 2/3] repack: retain the return value of pack-objects Stefan Beller
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Stefan Beller @ 2013-09-15 15:33 UTC (permalink / raw
To: gitster, git; +Cc: Stefan Beller
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Makefile | 2 +-
builtin.h | 1 +
builtin/repack.c | 387 +++++++++++++++++++++++++++++++++++++++++
contrib/examples/git-repack.sh | 194 +++++++++++++++++++++
git-repack.sh | 194 ---------------------
git.c | 1 +
6 files changed, 584 insertions(+), 195 deletions(-)
create mode 100644 builtin/repack.c
create mode 100755 contrib/examples/git-repack.sh
delete mode 100755 git-repack.sh
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..a15bd9b
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,387 @@
+#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;
+
+ /* Point at the slash at the end of ".../objects/pack/" */
+ dirlen = strlen(packdir) + 1;
+ strbuf_addstr(&buf, packtmp);
+ /* Hold the length 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);
+ else
+ free(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);
+ }
+ strbuf_release(&buf);
+}
+
+#define ALL_INTO_ONE 1
+#define LOOSEN_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"),
+ LOOSEN_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 & LOOSEN_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)) {
+ free(fname);
+ failed = 1;
+ break;
+ } else {
+ 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))
+ exit(errno);
+ 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]);
+ remove_path(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();
+ string_list_clear(&names, 0);
+ string_list_clear(&rollback, 0);
+ string_list_clear(&existing_packs, 0);
+ strbuf_release(&line);
+
+ return 0;
+}
diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
new file mode 100755
index 0000000..7579331
--- /dev/null
+++ b/contrib/examples/git-repack.sh
@@ -0,0 +1,194 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Linus Torvalds
+#
+
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git repack [options]
+--
+a pack everything in a single pack
+A same as -a, and turn unreachable objects loose
+d remove redundant packs, and run git-prune-packed
+f pass --no-reuse-delta to git-pack-objects
+F pass --no-reuse-object to git-pack-objects
+n do not run git-update-server-info
+q,quiet be quiet
+l pass --local to git-pack-objects
+unpack-unreachable= with -A, do not loosen objects older than this
+ Packing constraints
+window= size of the window used for delta compression
+window-memory= same as the above, but limit memory size instead of entries count
+depth= limits the maximum delta depth
+max-pack-size= maximum size of each packfile
+"
+SUBDIRECTORY_OK='Yes'
+. git-sh-setup
+
+no_update_info= all_into_one= remove_redundant= unpack_unreachable=
+local= no_reuse= extra=
+while test $# != 0
+do
+ case "$1" in
+ -n) no_update_info=t ;;
+ -a) all_into_one=t ;;
+ -A) all_into_one=t
+ unpack_unreachable=--unpack-unreachable ;;
+ --unpack-unreachable)
+ unpack_unreachable="--unpack-unreachable=$2"; shift ;;
+ -d) remove_redundant=t ;;
+ -q) GIT_QUIET=t ;;
+ -f) no_reuse=--no-reuse-delta ;;
+ -F) no_reuse=--no-reuse-object ;;
+ -l) local=--local ;;
+ --max-pack-size|--window|--window-memory|--depth)
+ extra="$extra $1=$2"; shift ;;
+ --) shift; break;;
+ *) usage ;;
+ esac
+ shift
+done
+
+case "`git config --bool repack.usedeltabaseoffset || echo true`" in
+true)
+ extra="$extra --delta-base-offset" ;;
+esac
+
+PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
+PACKTMP="$PACKDIR/.tmp-$$-pack"
+rm -f "$PACKTMP"-*
+trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
+
+# There will be more repacking strategies to come...
+case ",$all_into_one," in
+,,)
+ args='--unpacked --incremental'
+ ;;
+,t,)
+ args= existing=
+ if [ -d "$PACKDIR" ]; then
+ for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
+ | sed -e 's/^\.\///' -e 's/\.pack$//'`
+ do
+ if [ -e "$PACKDIR/$e.keep" ]; then
+ : keep
+ else
+ existing="$existing $e"
+ fi
+ done
+ if test -n "$existing" -a -n "$unpack_unreachable" -a \
+ -n "$remove_redundant"
+ then
+ # This may have arbitrary user arguments, so we
+ # have to protect it against whitespace splitting
+ # when it gets run as "pack-objects $args" later.
+ # Fortunately, we know it's an approxidate, so we
+ # can just use dots instead.
+ args="$args $(echo "$unpack_unreachable" | tr ' ' .)"
+ fi
+ fi
+ ;;
+esac
+
+mkdir -p "$PACKDIR" || exit
+
+args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra"
+names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
+ exit 1
+if [ -z "$names" ]; then
+ say Nothing new to pack.
+fi
+
+# 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.
+rollback=
+failed=
+for name in $names
+do
+ for sfx in pack idx
+ do
+ file=pack-$name.$sfx
+ test -f "$PACKDIR/$file" || continue
+ rm -f "$PACKDIR/old-$file" &&
+ mv "$PACKDIR/$file" "$PACKDIR/old-$file" || {
+ failed=t
+ break
+ }
+ rollback="$rollback $file"
+ done
+ test -z "$failed" || break
+done
+
+# If renaming failed for any of them, roll the ones we have
+# already renamed back to their original names.
+if test -n "$failed"
+then
+ rollback_failure=
+ for file in $rollback
+ do
+ mv "$PACKDIR/old-$file" "$PACKDIR/$file" ||
+ rollback_failure="$rollback_failure $file"
+ done
+ if test -n "$rollback_failure"
+ then
+ echo >&2 "WARNING: Some packs in use have been renamed by"
+ echo >&2 "WARNING: prefixing old- to their name, in order to"
+ echo >&2 "WARNING: replace them with the new version of the"
+ echo >&2 "WARNING: file. But the operation failed, and"
+ echo >&2 "WARNING: attempt to rename them back to their"
+ echo >&2 "WARNING: original names also failed."
+ echo >&2 "WARNING: Please rename them in $PACKDIR manually:"
+ for file in $rollback_failure
+ do
+ echo >&2 "WARNING: old-$file -> $file"
+ done
+ fi
+ exit 1
+fi
+
+# Now the ones with the same name are out of the way...
+fullbases=
+for name in $names
+do
+ fullbases="$fullbases pack-$name"
+ chmod a-w "$PACKTMP-$name.pack"
+ chmod a-w "$PACKTMP-$name.idx"
+ mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
+ mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" ||
+ exit
+done
+
+# Remove the "old-" files
+for name in $names
+do
+ rm -f "$PACKDIR/old-pack-$name.idx"
+ rm -f "$PACKDIR/old-pack-$name.pack"
+done
+
+# End of pack replacement.
+
+if test "$remove_redundant" = t
+then
+ # We know $existing are all redundant.
+ if [ -n "$existing" ]
+ then
+ ( cd "$PACKDIR" &&
+ for e in $existing
+ do
+ case " $fullbases " in
+ *" $e "*) ;;
+ *) rm -f "$e.pack" "$e.idx" "$e.keep" ;;
+ esac
+ done
+ )
+ fi
+ git prune-packed ${GIT_QUIET:+-q}
+fi
+
+case "$no_update_info" in
+t) : ;;
+*) git update-server-info ;;
+esac
diff --git a/git-repack.sh b/git-repack.sh
deleted file mode 100755
index 7579331..0000000
--- a/git-repack.sh
+++ /dev/null
@@ -1,194 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Linus Torvalds
-#
-
-OPTIONS_KEEPDASHDASH=
-OPTIONS_SPEC="\
-git repack [options]
---
-a pack everything in a single pack
-A same as -a, and turn unreachable objects loose
-d remove redundant packs, and run git-prune-packed
-f pass --no-reuse-delta to git-pack-objects
-F pass --no-reuse-object to git-pack-objects
-n do not run git-update-server-info
-q,quiet be quiet
-l pass --local to git-pack-objects
-unpack-unreachable= with -A, do not loosen objects older than this
- Packing constraints
-window= size of the window used for delta compression
-window-memory= same as the above, but limit memory size instead of entries count
-depth= limits the maximum delta depth
-max-pack-size= maximum size of each packfile
-"
-SUBDIRECTORY_OK='Yes'
-. git-sh-setup
-
-no_update_info= all_into_one= remove_redundant= unpack_unreachable=
-local= no_reuse= extra=
-while test $# != 0
-do
- case "$1" in
- -n) no_update_info=t ;;
- -a) all_into_one=t ;;
- -A) all_into_one=t
- unpack_unreachable=--unpack-unreachable ;;
- --unpack-unreachable)
- unpack_unreachable="--unpack-unreachable=$2"; shift ;;
- -d) remove_redundant=t ;;
- -q) GIT_QUIET=t ;;
- -f) no_reuse=--no-reuse-delta ;;
- -F) no_reuse=--no-reuse-object ;;
- -l) local=--local ;;
- --max-pack-size|--window|--window-memory|--depth)
- extra="$extra $1=$2"; shift ;;
- --) shift; break;;
- *) usage ;;
- esac
- shift
-done
-
-case "`git config --bool repack.usedeltabaseoffset || echo true`" in
-true)
- extra="$extra --delta-base-offset" ;;
-esac
-
-PACKDIR="$GIT_OBJECT_DIRECTORY/pack"
-PACKTMP="$PACKDIR/.tmp-$$-pack"
-rm -f "$PACKTMP"-*
-trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
-
-# There will be more repacking strategies to come...
-case ",$all_into_one," in
-,,)
- args='--unpacked --incremental'
- ;;
-,t,)
- args= existing=
- if [ -d "$PACKDIR" ]; then
- for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
- | sed -e 's/^\.\///' -e 's/\.pack$//'`
- do
- if [ -e "$PACKDIR/$e.keep" ]; then
- : keep
- else
- existing="$existing $e"
- fi
- done
- if test -n "$existing" -a -n "$unpack_unreachable" -a \
- -n "$remove_redundant"
- then
- # This may have arbitrary user arguments, so we
- # have to protect it against whitespace splitting
- # when it gets run as "pack-objects $args" later.
- # Fortunately, we know it's an approxidate, so we
- # can just use dots instead.
- args="$args $(echo "$unpack_unreachable" | tr ' ' .)"
- fi
- fi
- ;;
-esac
-
-mkdir -p "$PACKDIR" || exit
-
-args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra"
-names=$(git pack-objects --keep-true-parents --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
- exit 1
-if [ -z "$names" ]; then
- say Nothing new to pack.
-fi
-
-# 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.
-rollback=
-failed=
-for name in $names
-do
- for sfx in pack idx
- do
- file=pack-$name.$sfx
- test -f "$PACKDIR/$file" || continue
- rm -f "$PACKDIR/old-$file" &&
- mv "$PACKDIR/$file" "$PACKDIR/old-$file" || {
- failed=t
- break
- }
- rollback="$rollback $file"
- done
- test -z "$failed" || break
-done
-
-# If renaming failed for any of them, roll the ones we have
-# already renamed back to their original names.
-if test -n "$failed"
-then
- rollback_failure=
- for file in $rollback
- do
- mv "$PACKDIR/old-$file" "$PACKDIR/$file" ||
- rollback_failure="$rollback_failure $file"
- done
- if test -n "$rollback_failure"
- then
- echo >&2 "WARNING: Some packs in use have been renamed by"
- echo >&2 "WARNING: prefixing old- to their name, in order to"
- echo >&2 "WARNING: replace them with the new version of the"
- echo >&2 "WARNING: file. But the operation failed, and"
- echo >&2 "WARNING: attempt to rename them back to their"
- echo >&2 "WARNING: original names also failed."
- echo >&2 "WARNING: Please rename them in $PACKDIR manually:"
- for file in $rollback_failure
- do
- echo >&2 "WARNING: old-$file -> $file"
- done
- fi
- exit 1
-fi
-
-# Now the ones with the same name are out of the way...
-fullbases=
-for name in $names
-do
- fullbases="$fullbases pack-$name"
- chmod a-w "$PACKTMP-$name.pack"
- chmod a-w "$PACKTMP-$name.idx"
- mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
- mv -f "$PACKTMP-$name.idx" "$PACKDIR/pack-$name.idx" ||
- exit
-done
-
-# Remove the "old-" files
-for name in $names
-do
- rm -f "$PACKDIR/old-pack-$name.idx"
- rm -f "$PACKDIR/old-pack-$name.pack"
-done
-
-# End of pack replacement.
-
-if test "$remove_redundant" = t
-then
- # We know $existing are all redundant.
- if [ -n "$existing" ]
- then
- ( cd "$PACKDIR" &&
- for e in $existing
- do
- case " $fullbases " in
- *" $e "*) ;;
- *) rm -f "$e.pack" "$e.idx" "$e.keep" ;;
- esac
- done
- )
- fi
- git prune-packed ${GIT_QUIET:+-q}
-fi
-
-case "$no_update_info" in
-t) : ;;
-*) git update-server-info ;;
-esac
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.273.ga194ead
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] repack: retain the return value of pack-objects
2013-09-15 15:33 [PATCH 1/3] repack: rewrite the shell script in C Stefan Beller
@ 2013-09-15 15:33 ` Stefan Beller
2013-09-15 15:33 ` [PATCH 3/3] repack: improve warnings about failure of renaming and removing files Stefan Beller
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2013-09-15 15:33 UTC (permalink / raw
To: gitster, git; +Cc: Stefan Beller
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(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index a15bd9b..d345d16 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -231,7 +231,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");
@@ -244,7 +244,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.273.ga194ead
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] repack: improve warnings about failure of renaming and removing files
2013-09-15 15:33 [PATCH 1/3] repack: rewrite the shell script in C Stefan Beller
2013-09-15 15:33 ` [PATCH 2/3] repack: retain the return value of pack-objects Stefan Beller
@ 2013-09-15 15:33 ` Stefan Beller
2013-09-15 17:54 ` [PATCH 1/3] repack: rewrite the shell script in C Ramkumar Ramachandra
2013-09-17 18:17 ` Junio C Hamano
3 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2013-09-15 15:33 UTC (permalink / raw
To: gitster, git; +Cc: Stefan Beller
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
builtin/repack.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index d345d16..f7e91bf 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -327,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))
- exit(errno);
+ die_errno(_("renaming '%s' failed"), fname_old);
free(fname);
free(fname_old);
}
@@ -341,7 +341,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
packdir,
item->string,
exts[ext]);
- remove_path(fname);
+ if (remove_path(fname))
+ warning(_("removing '%s' failed"), fname);
}
}
--
1.8.4.273.ga194ead
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] repack: rewrite the shell script in C
2013-09-15 15:33 [PATCH 1/3] repack: rewrite the shell script in C Stefan Beller
2013-09-15 15:33 ` [PATCH 2/3] repack: retain the return value of pack-objects Stefan Beller
2013-09-15 15:33 ` [PATCH 3/3] repack: improve warnings about failure of renaming and removing files Stefan Beller
@ 2013-09-15 17:54 ` Ramkumar Ramachandra
2013-09-17 16:42 ` Junio C Hamano
2013-09-17 18:17 ` Junio C Hamano
3 siblings, 1 reply; 8+ messages in thread
From: Ramkumar Ramachandra @ 2013-09-15 17:54 UTC (permalink / raw
To: Stefan Beller; +Cc: Junio C Hamano, Git List
Stefan Beller wrote:
> Makefile | 2 +-
> builtin.h | 1 +
> builtin/repack.c | 387 +++++++++++++++++++++++++++++++++++++++++
> contrib/examples/git-repack.sh | 194 +++++++++++++++++++++
> git-repack.sh | 194 ---------------------
> git.c | 1 +
> 6 files changed, 584 insertions(+), 195 deletions(-)
> create mode 100644 builtin/repack.c
> create mode 100755 contrib/examples/git-repack.sh
> delete mode 100755 git-repack.sh
Looks like repack.c is significantly larger than git-repack.sh. I look
forward to reading the code.
> diff --git a/builtin/repack.c b/builtin/repack.c
> new file mode 100644
> index 0000000..a15bd9b
> --- /dev/null
> +++ b/builtin/repack.c
> @@ -0,0 +1,387 @@
> +#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);
> +}
Configuration option: one bool. I wonder what other configuration
options the future will bring in.
> +/*
> + * Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
> + */
Is $GIT_OBJECT_DIRECTORY a standard variable, or should it be
$GIT_DIR/objects? A quick grep tells me that there are a few
references to it, but I'm yet to be convinced that it can be something
other than $GIT_DIR/objects.
> +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;
Wait a minute: where did we initalize packdir? Ah, it's static, so it
must have been initalized before the function was called (in some sort
of setup function), right? Why don't you return an int from the
function so it's possible to differentiate between success and
failure?
> + /* Point at the slash at the end of ".../objects/pack/" */
Is packdir a relative or absolute path? The ... isn't helping.
> + dirlen = strlen(packdir) + 1;
> + strbuf_addstr(&buf, packtmp);
Mysterious initalization of packtmp.
> + /* Hold the length of ".tmp-%d-pack-" */
> + prefixlen = buf.len - dirlen;
Okay, so that's what packtmp contains: a reading of repack.sh told me
that you didn't even change the variable names.
> + while ((e = readdir(dir))) {
> + if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
> + continue;
Skip the dentry that points to the .tmp-* thing.
> + strbuf_setlen(&buf, dirlen);
> + strbuf_addstr(&buf, e->d_name);
> + unlink(buf.buf);
> + }
> + closedir(dir);
> + strbuf_release(&buf);
> +}
Okay.
> +static void remove_pack_on_signal(int signo)
> +{
> + remove_temporary_files();
> + sigchain_pop(signo);
> + raise(signo);
> +}
Okay, although I'd have named the variable "signal", not "signo".
> +/*
> + * Adds all packs hex strings to the fname list, which do not
> + * have a corresponding .keep file.
> + */
The packs which don't have a corresponding .keep file must be removed/
repacked. Okay.
> +static void get_non_kept_pack_filenames(struct string_list *fname_list)
> +{
> + DIR *dir;
> + struct dirent *e;
> + char *fname;
Filename, I assume.
> + size_t len;
> +
> + if (!(dir = opendir(packdir)))
> + return;
An int return, please.
> + while ((e = readdir(dir)) != NULL) {
> + if (suffixcmp(e->d_name, ".pack"))
> + continue;
If we didn't find a .pack file, skip over?
> + len = strlen(e->d_name) - strlen(".pack");
> + fname = xmemdupz(e->d_name, len);
You can probably use the pathbufs to save some memory here, but I
wouldn't worry about it at this stage.
> + if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
> + string_list_append_nodup(fname_list, fname);
> + else
> + free(fname);
> + }
> + closedir(dir);
> +}
Okay.
> +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);
hex is the sha1-hex written out to the full 40 characters?
> + plen = buf.len;
> +
> + for (i = 0; i < ARRAY_SIZE(exts); i++) {
> + strbuf_setlen(&buf, plen);
> + strbuf_addstr(&buf, exts[i]);
> + unlink(buf.buf);
> + }
> + strbuf_release(&buf);
> +}
So you find the redundant sha1-hexes and unlink the corresponding
.pack, .idx and .keep files. Okay.
> +#define ALL_INTO_ONE 1
> +#define LOOSEN_UNREACHABLE 2
Wait, isn't all_into_one supposed to be configurable? What is
loosen_unreachable?
> +int cmd_repack(int argc, const char **argv, const char *prefix)
> +{
> [...]
So argument parsing and spawning pack-objects appropriately is what
blew up the size of this file. I'll review this monolith in another
sitting.
Thanks for the enjoyable read.
Ram
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] repack: rewrite the shell script in C
2013-09-15 17:54 ` [PATCH 1/3] repack: rewrite the shell script in C Ramkumar Ramachandra
@ 2013-09-17 16:42 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2013-09-17 16:42 UTC (permalink / raw
To: Ramkumar Ramachandra; +Cc: Stefan Beller, Git List
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Is $GIT_OBJECT_DIRECTORY a standard variable, or should it be
> $GIT_DIR/objects?
"man git" ;-) It has been there since early May 2005
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] repack: rewrite the shell script in C
2013-09-15 15:33 [PATCH 1/3] repack: rewrite the shell script in C Stefan Beller
` (2 preceding siblings ...)
2013-09-15 17:54 ` [PATCH 1/3] repack: rewrite the shell script in C Ramkumar Ramachandra
@ 2013-09-17 18:17 ` Junio C Hamano
2013-09-17 20:13 ` Stefan Beller
3 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-09-17 18:17 UTC (permalink / raw
To: Stefan Beller; +Cc: git
Stefan Beller <stefanbeller@googlemail.com> writes:
> + 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"),
> + LOOSEN_UNREACHABLE),
Micronit.
With the current version of the code in cmd_repack() that uses the
pack_everything variable this may not make a difference, but I think
this should logically be "LOOSEN_UNREACHABLE | ALL_INTO_ONE" instead,
and the code should check (pack_evertying & ALL_INTO_ONE) instead of
checking "!pack_everything". You may want to add to this flag variable
a new bit that does _not_ cause it to pack everything into one.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] repack: rewrite the shell script in C
2013-09-17 18:17 ` Junio C Hamano
@ 2013-09-17 20:13 ` Stefan Beller
2013-09-17 20:25 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Beller @ 2013-09-17 20:13 UTC (permalink / raw
To: Junio C Hamano; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 4735 bytes --]
On 09/17/2013 08:17 PM, Junio C Hamano wrote:
> Stefan Beller <stefanbeller@googlemail.com> writes:
>
>> + 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"),
>> + LOOSEN_UNREACHABLE),
>
> Micronit.
>
> With the current version of the code in cmd_repack() that uses the
> pack_everything variable this may not make a difference, but I think
> this should logically be "LOOSEN_UNREACHABLE | ALL_INTO_ONE" instead,
> and the code should check (pack_evertying & ALL_INTO_ONE) instead of
> checking "!pack_everything". You may want to add to this flag variable
> a new bit that does _not_ cause it to pack everything into one.
>
I do understand the "LOOSEN_UNREACHABLE | ALL_INTO_ONE" here, as that
is the logical thing we are doing. Combined with your second idea this
would result in
---8<---
From 4bbbfb312bf23efa7e702e200fbc2d4479e3477e Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Tue, 17 Sep 2013 22:04:35 +0200
Subject: [PATCH 2/2] Suggestions by Junio
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
builtin/repack.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index e5f90c6..a0ff5c7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -143,7 +143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
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"),
- LOOSEN_UNREACHABLE),
+ LOOSEN_UNREACHABLE | ALL_INTO_ONE),
OPT_BOOL('d', NULL, &delete_redundant,
N_("remove redundant packs, and run git-prune-packed")),
OPT_BOOL('f', NULL, &no_reuse_delta,
@@ -197,10 +197,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) {
- argv_array_push(&cmd_args, "--unpacked");
- argv_array_push(&cmd_args, "--incremental");
- } else {
+ if (pack_everything & ALL_INTO_ONE) {
get_non_kept_pack_filenames(&existing_packs);
if (existing_packs.nr && delete_redundant) {
@@ -212,6 +209,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
argv_array_push(&cmd_args,
"--unpack-unreachable");
}
+ } else {
+ argv_array_push(&cmd_args, "--unpacked");
+ argv_array_push(&cmd_args, "--incremental");
}
if (local)
--
1.8.4.273.ga194ead
However I assume you mean to even ease up the conditions now, because now
both -a as well as -A set ALL_INTO_ONE we could apply the following
on top of the previous.
---8<---
From 80199368ab6c7ab72f81a5c531f79073a99d2498 Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Tue, 17 Sep 2013 22:11:08 +0200
Subject: [PATCH] Further improvements by reducing nested ifs
This may pass --unpacked and --unpack-unreachable to pack-objects in one
command, which is redundant. On the other hand we may gain simplicity in
repack.
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
builtin/repack.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index a0ff5c7..3e56614 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -197,23 +197,23 @@ 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 & ALL_INTO_ONE) {
+ if (pack_everything & ALL_INTO_ONE)
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 & LOOSEN_UNREACHABLE)
- argv_array_push(&cmd_args,
- "--unpack-unreachable");
- }
- } else {
+ else {
argv_array_push(&cmd_args, "--unpacked");
argv_array_push(&cmd_args, "--incremental");
}
+ if (existing_packs.nr && delete_redundant) {
+ if (unpack_unreachable)
+ argv_array_pushf(&cmd_args,
+ "--unpack-unreachable=%s",
+ unpack_unreachable);
+ else if (pack_everything & LOOSEN_UNREACHABLE)
+ argv_array_push(&cmd_args,
+ "--unpack-unreachable");
+ }
+
if (local)
argv_array_push(&cmd_args, "--local");
if (quiet)
--
1.8.4.273.ga194ead
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] repack: rewrite the shell script in C
2013-09-17 20:13 ` Stefan Beller
@ 2013-09-17 20:25 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2013-09-17 20:25 UTC (permalink / raw
To: Stefan Beller; +Cc: git
Stefan Beller <stefanbeller@googlemail.com> writes:
> On 09/17/2013 08:17 PM, Junio C Hamano wrote:
>> Stefan Beller <stefanbeller@googlemail.com> writes:
>>
>>> + 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"),
>>> + LOOSEN_UNREACHABLE),
>>
>> Micronit.
>>
>> With the current version of the code in cmd_repack() that uses the
>> pack_everything variable this may not make a difference, but I think
>> this should logically be "LOOSEN_UNREACHABLE | ALL_INTO_ONE" instead,
>> and the code should check (pack_evertying & ALL_INTO_ONE) instead of
>> checking "!pack_everything". You may want to add to this flag variable
>> a new bit that does _not_ cause it to pack everything into one.
>>
>
> I do understand the "LOOSEN_UNREACHABLE | ALL_INTO_ONE" here, as that
> is the logical thing we are doing. Combined with your second idea this
> would result in
> ---8<---
>
> From 4bbbfb312bf23efa7e702e200fbc2d4479e3477e Mon Sep 17 00:00:00 2001
> From: Stefan Beller <stefanbeller@googlemail.com>
> Date: Tue, 17 Sep 2013 22:04:35 +0200
> Subject: [PATCH 2/2] Suggestions by Junio
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
> builtin/repack.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index e5f90c6..a0ff5c7 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -143,7 +143,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> 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"),
> - LOOSEN_UNREACHABLE),
> + LOOSEN_UNREACHABLE | ALL_INTO_ONE),
> OPT_BOOL('d', NULL, &delete_redundant,
> N_("remove redundant packs, and run git-prune-packed")),
> OPT_BOOL('f', NULL, &no_reuse_delta,
> @@ -197,10 +197,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) {
> - argv_array_push(&cmd_args, "--unpacked");
> - argv_array_push(&cmd_args, "--incremental");
> - } else {
> + if (pack_everything & ALL_INTO_ONE) {
> get_non_kept_pack_filenames(&existing_packs);
>
> if (existing_packs.nr && delete_redundant) {
> @@ -212,6 +209,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> argv_array_push(&cmd_args,
> "--unpack-unreachable");
> }
> + } else {
> + argv_array_push(&cmd_args, "--unpacked");
> + argv_array_push(&cmd_args, "--incremental");
> }
>
> if (local)
> --
> 1.8.4.273.ga194ead
Yes.
Above was what I would have expected from a straight *.sh to *.c
conversion.
But I didn't think about the change in the patch below.
> However I assume you mean to even ease up the conditions now, because now
> both -a as well as -A set ALL_INTO_ONE we could apply the following
> on top of the previous.
> ---8<---
>
> From 80199368ab6c7ab72f81a5c531f79073a99d2498 Mon Sep 17 00:00:00 2001
> From: Stefan Beller <stefanbeller@googlemail.com>
> Date: Tue, 17 Sep 2013 22:11:08 +0200
> Subject: [PATCH] Further improvements by reducing nested ifs
>
> This may pass --unpacked and --unpack-unreachable to pack-objects in one
> command, which is redundant. On the other hand we may gain simplicity in
> repack.
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
> builtin/repack.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index a0ff5c7..3e56614 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -197,23 +197,23 @@ 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 & ALL_INTO_ONE) {
> + if (pack_everything & ALL_INTO_ONE)
> 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 & LOOSEN_UNREACHABLE)
> - argv_array_push(&cmd_args,
> - "--unpack-unreachable");
> - }
> - } else {
> + else {
> argv_array_push(&cmd_args, "--unpacked");
> argv_array_push(&cmd_args, "--incremental");
> }
>
> + if (existing_packs.nr && delete_redundant) {
> + if (unpack_unreachable)
> + argv_array_pushf(&cmd_args,
> + "--unpack-unreachable=%s",
> + unpack_unreachable);
> + else if (pack_everything & LOOSEN_UNREACHABLE)
> + argv_array_push(&cmd_args,
> + "--unpack-unreachable");
> + }
> +
> if (local)
> argv_array_push(&cmd_args, "--local");
> if (quiet)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-17 20:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-15 15:33 [PATCH 1/3] repack: rewrite the shell script in C Stefan Beller
2013-09-15 15:33 ` [PATCH 2/3] repack: retain the return value of pack-objects Stefan Beller
2013-09-15 15:33 ` [PATCH 3/3] repack: improve warnings about failure of renaming and removing files Stefan Beller
2013-09-15 17:54 ` [PATCH 1/3] repack: rewrite the shell script in C Ramkumar Ramachandra
2013-09-17 16:42 ` Junio C Hamano
2013-09-17 18:17 ` Junio C Hamano
2013-09-17 20:13 ` Stefan Beller
2013-09-17 20:25 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.