Am 20.08.2013 17:08, schrieb Stefan Beller:
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 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. :)

+        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.

+        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.

+    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)) {

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

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

will do.

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to