[PATCH] repack: rewrite the shell script in C (squashing proposal)

2013-08-22 Thread Stefan Beller
This patch is meant to be squashed into bb4335a21441a0
(repack: rewrite the shell script in C), I'll do so when rerolling
the series. For reviewing I'll just send this patch.

* Remove comments, which likely get out of date (authorship is kept in
  git anyway)
* rename get_pack_filenames to get_non_kept_pack_filenames
* catch return value of unlink and fail as the shell version did
* beauty fixes to remove_temporary_files as Junio proposed
* install signal handling after static variables packdir, packtmp are set
* remove adding the empty string to the buffer.
* fix the rollback mechanism (wrong variable name)

Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
 builtin/repack.c | 78 ++--
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 1f13e0d..e0d1f17 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -1,8 +1,3 @@
-/*
- * The shell version was written by Linus Torvalds (2005) and many others.
- * This is a translation into C by Stefan Beller (2013)
- */
-
 #include builtin.h
 #include cache.h
 #include dir.h
@@ -13,9 +8,8 @@
 #include string-list.h
 #include argv-array.h
 
-/* enabled by default since 22c79eab (2008-06-25) */
 static int delta_base_offset = 1;
-char *packdir;
+static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
N_(git repack [options]),
@@ -41,18 +35,16 @@ static void remove_temporary_files(void)
DIR *dir;
struct dirent *e;
 
-   /* .git/objects/pack */
-   strbuf_addstr(buf, get_object_directory());
-   strbuf_addstr(buf, /pack);
-   dir = opendir(buf.buf);
-   if (!dir) {
-   strbuf_release(buf);
+   dir = opendir(packdir);
+   if (!dir)
return;
-   }
 
-   /* .git/objects/pack/.tmp-$$-pack-* */
+   strbuf_addstr(buf, packdir);
+
+   /* dirlen holds the length of the path before the file name */
dirlen = buf.len + 1;
-   strbuf_addf(buf, /.tmp-%d-pack-, (int)getpid());
+   strbuf_addf(buf, %s, packtmp);
+   /* prefixlen holds the length of the prefix */
prefixlen = buf.len - dirlen;
 
while ((e = readdir(dir))) {
@@ -73,11 +65,16 @@ static void remove_pack_on_signal(int signo)
raise(signo);
 }
 
-static void get_pack_filenames(struct string_list *fname_list)
+/*
+ * 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;
@@ -86,7 +83,7 @@ static void get_pack_filenames(struct string_list *fname_list)
if (suffixcmp(e-d_name, .pack))
continue;
 
-   size_t len = strlen(e-d_name) - strlen(.pack);
+   len = strlen(e-d_name) - strlen(.pack);
fname = xmemdupz(e-d_name, len);
 
if (!file_exists(mkpath(%s/%s.keep, packdir, fname)))
@@ -95,14 +92,14 @@ static void get_pack_filenames(struct string_list 
*fname_list)
closedir(dir);
 }
 
-static void remove_redundant_pack(const char *path, const char *sha1)
+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, sha1);
+   strbuf_addf(buf, %s/%s, path_prefix, hex);
plen = buf.len;
 
for (i = 0; i  ARRAY_SIZE(exts); i++) {
@@ -115,15 +112,14 @@ static void remove_redundant_pack(const char *path, const 
char *sha1)
 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 rollback = STRING_LIST_INIT_NODUP;
struct string_list existing_packs = STRING_LIST_INIT_DUP;
struct strbuf line = STRBUF_INIT;
-   int count_packs, ext, ret;
+   int nr_packs, ext, ret, failed;
FILE *out;
 
/* variables to be filled by option parsing */
@@ -173,11 +169,11 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
 
-   sigchain_push_common(remove_pack_on_signal);
-
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
 
+   sigchain_push_common(remove_pack_on_signal);
+

Re: [PATCH] repack: rewrite the shell script in C (squashing proposal)

2013-08-22 Thread Junio C Hamano
Stefan Beller stefanbel...@googlemail.com writes:

 @@ -41,18 +35,16 @@ static void remove_temporary_files(void)
   DIR *dir;
   struct dirent *e;
  
 + dir = opendir(packdir);
 + if (!dir)
   return;
  
 + strbuf_addstr(buf, packdir);
 +
 + /* dirlen holds the length of the path before the file name */
   dirlen = buf.len + 1;
 + strbuf_addf(buf, %s, packtmp);
 + /* prefixlen holds the length of the prefix */

Thanks to the name of the variable that is self-describing, this
comment does not add much value.

But it misses the whole point of my suggestion in the earlier
message to phrase these like so:

/* Point at the slash at the end of .../objects/pack/ */
dirlen = strlen(packdir) + 1;
/* Point at the dash at the end of .../.tmp-%d-pack- */
prefixlen = buf.len - dirlen;

to clarify what the writer considers as the prefix is, which may
be quite different from what the readers think the prefix is.  In
.tmp-2342-pack-0d8beaa5b76e824c9869f0d1f1b19ec7acf4982f.pack, is
the prefix .tmp-2342-, .tmp-2342-pack, or .tmp-2342-pack-?

  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, (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);
 ...
 + rollback_failure.items[i].string,
 + rollback_failure.items[i].string);
   }
   exit(1);
   }

The scripted version uses

trap 'rm -f $PACKTMP-*' 0 1 2 3 15

so remove_temporary_files() needs to be called before exiting from
the program without getting killed by a signal.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html