On 09/26/2013 01:42 PM, Duy Nguyen wrote:
> On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller
> <stefanbel...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
