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

[Stefan Beller <stefanbeller@googlemail.com>]

On 09/26/2013 01:42 PM, Duy Nguyen wrote:
> On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
>> 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.

Ok, noted.

>
>>
>> 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.
>
>> @@ -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.
>
>> @@ -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".

The reason I put the pack_version not here is for structural clarity.
("All config is done in either the parse_options section or in the
repack_config function"). This may help having a the actual core logic
easier and more understandable?
If you feel otherwise, I'd change it to your proposal.

Thanks,
Stefan