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 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. :)
+ 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)) {
"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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html