Re: [PATCH] repack: Add --version parameter

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 26, 2013, 11:42 UTC
Message-ID: <CACsJy8BjTQhA6rDWk6FboxU-b6d93Jn+ZNvQ0DJYXehgrpSmwg@mail.gmail.com>
In-Reply-To: <1380184349-26380-1-git-send-email-stefanbeller@googlemail.com>

On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller <stefanbeller@googlemail.com> wrote:

Show 8 quoted lines

> This is just a direct translation of
> http://article.gmane.org/gmane.comp.version-control.git/235396
> So I don't consider this is ready for inclusion.
>
> Some notes:
> We need to have more error checking, repack shall be 0, 2 or 4 but nothing
> else. If 0 is given, no argument is passed to pack-objects, in case of
> 2 or 4 --version=<n> is passed.

It's not that bad. If you don't catch it, pack-objects will.

Show 6 quoted lines

>
> Do we really want to call it "--version"? This parameter sounds so much
> like questioning for the program version, similar to
>         git --version
>         1.8.4
> So I'd rather use "--repack-version".

Hmm.. I think it's "git repack --pack-version"? Or if you meant "git pack-objects --version", I drop the "pack-" out because there's already "pack" in "pack-objects". But I'm OK renaming --version to --pack-version too. Maybe later.

Show 8 quoted lines

> @@ -22,6 +23,9 @@ static int repack_config(const char *var, const char *value, void *cb)
>                 delta_base_offset = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp(var, "core.preferredPackVersion")) {
> +               pack_version = git_config_int(var, value);
> +       }
>         return git_default_config(var, value, cb);

In np/pack-v4 series (not the one on 'pu' yet) git_default_config will do this and save the value in core_default_pack_format. So you don't need to set it here.

Show 6 quoted lines

> @@ -220,6 +226,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>                 argv_array_push(&cmd_args,  "--quiet");
>         if (delta_base_offset)
>                 argv_array_push(&cmd_args,  "--delta-base-offset");
> +       if (pack_version)
> +               argv_array_pushf(&cmd_args, "--version=%u", pack_version);

but then you may need "if (!pack_version) pack_version = core_defaul_pack_format;" before this "if".

>
>         argv_array_push(&cmd_args, packtmp);

-- 
Duy