On 09/26/2013 01:42 PM, Duy Nguyen wrote: > On Thu, Sep 26, 2013 at 3:32 PM, Stefan Beller > <stefanbeller@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html