Re: [RFC PATCHv4] repack: rewrite the shell script in C.

From: René Scharfe <l.s.r@web.de>
Date: Aug 20, 2013, 18:57 UTC
Message-ID: <5213BC0C.6090901@web.de>
In-Reply-To: <52138686.1070304@googlemail.com>

Am 20.08.2013 17:08, schrieb Stefan Beller:

Show 20 quoted lines

> On 08/20/2013 03:31 PM, Johannes Sixt wrote:
>>
>> Are the long forms of options your invention?
>
> I tried to keep strong similarity with the shell script for
> ease of review. In the shellscript the options where
> put in variables having these names, so for example there was:
>
> 	-f)	no_reuse=--no-reuse-delta ;;
> 	-F)	no_reuse=--no-reuse-object ;;
>
> So I used these variable names as well in here. And as I assumed
> the variables are meaningful in itself.
>
> In the shell script they may be meaningful, but with the option
> parser in the C version, I overlooked the possibility for
> --no-<option> being possible as you noted below.
>
> Maybe we should inverse the logic and have the variables and options
> called reuse-delta and being enabled by default.

That's what git repack-objects does, which gets it passed to eventually.

But I think Johannes also wanted to point out that the git-repack.sh doesn't recognize --no-reuse-delta, --all etc.. I think it's better to introduce new long options in a separate patch. Switching the programming language is big enough of a change already. :)

Show 8 quoted lines

>>> +        OPT_BOOL('f', "no-reuse-delta", &no_reuse_delta,
>>> +                N_("pass --no-reuse-delta to git-pack-objects")),
>>> +        OPT_BOOL('F', "no-reuse-object", &no_reuse_object,
>>> +                N_("pass --no-reuse-object to git-pack-objects")),
>>
>> Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
>
> see above, I'd try not to.

The declaration above allows --reuse-delta, --no-reuse-delta and --no-no-reuse-delta to be used. The latter looks funny, but I don't think we need to forbid it. That said, dropping the no- and thus declaring them the same way as repack-objects is a good idea.

Show 10 quoted lines

>>
>>> +        OPT_BOOL('n', NULL, &no_update_server_info,
>>> +                N_("do not run git-update-server-info")),
>>
>> No long option name?
>
> This is also a negated option, so as above, maybe
> we could have --update_server_info and --no-update_server_info
> respectively. Talking about the shortform then: Is it possible to
> negate the shortform?

Words in long options are separated by dashes, so --update-server-info. 
  The no- prefix is provided for free by parseopt, unless the flag 
PARSE_OPT_NONEG is given.

There is no automatic way to provide a short option that negates another short option. You can build such a pair explicitly using OPTION_BIT and OPTION_NEGBIT or with OPTION_SET_INT and different values.

Show 10 quoted lines

>>> +    if (pack_everything + pack_everything_but_loose == 0) {
>>> +        argv_array_push(&cmd_args, "--unpacked");
>>> +        argv_array_push(&cmd_args, "--incremental");
>>> +    } else {
>>> +        struct string_list fname_list = STRING_LIST_INIT_DUP;
>>> +        get_pack_filenames(packdir, &fname_list);
>>> +        for_each_string_list_item(item, &fname_list) {
>>> +            char *fname;
>>> +            fname = mkpathdup("%s/%s.keep", packdir, item->string);
>>> +            if (stat(fname, &statbuffer) && S_ISREG(statbuffer.st_mode)) {

"t7700-repack.sh --valgrind" fails and flags that line...

>>
>>             if (!stat(fname, &statbuffer) && ...

... but with this fix it runs fine. I suspect that explains you sporadic test failures.

Show 5 quoted lines

>>
>> But you are using file_exists() later. That should be good enough here
>> as well, no?
>
> will do.