From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <stefanbeller@googlemail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] repack: rewrite the shell script in C
Date: Tue, 17 Sep 2013 13:25:14 -0700 [thread overview]
Message-ID: <xmqqa9jbxkk5.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <5238B7E4.6060506@googlemail.com> (Stefan Beller's message of "Tue, 17 Sep 2013 22:13:24 +0200")
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)
prev parent reply other threads:[~2013-09-17 20:25 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
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=xmqqa9jbxkk5.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=stefanbeller@googlemail.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).