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

[Stefan Beller <stefanbeller@googlemail.com>]

[-- 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 --]