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

[Junio C Hamano <gitster@pobox.com>]

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)