These patches spread the use of parse_options() to some more commands.
I tried to come up with a short description for every option, but
I'm not sure I fully succeeded. In particular, the option "sparse"
in builtin-rev-list.c has no description.
Michele Ballabio (9):
builtin-verify-tag.c: use parse_options()
builtin-write-tree.c: use parse_options()
builtin-prune-packed.c: use parse_options()
builtin-ls-tree.c: use parse_options()
builtin-rev-list.c: use parse_options()
builtin-init-db.c: use parse_options()
builtin-checkout-index.c: use parse_options()
builtin-fetch-pack.c: use parse_options()
builtin-mailinfo.c: use parse_options()
builtin-checkout-index.c | 146 +++++++++++++++++++++++++---------------------
builtin-fetch-pack.c | 144 ++++++++++++++++++++++++++++-----------------
builtin-init-db.c | 56 +++++++++++-------
builtin-ls-tree.c | 92 +++++++++++------------------
builtin-mailinfo.c | 39 +++++++------
builtin-prune-packed.c | 38 ++++++------
builtin-rev-list.c | 132 ++++++++++++++++++++---------------------
builtin-verify-tag.c | 25 +++++---
builtin-write-tree.c | 31 +++++-----
9 files changed, 376 insertions(+), 327 deletions(-)
@@ -153,18 +154,76 @@ static void checkout_all(const char *prefix, int prefix_length)exit(128);}-staticconstcharcheckout_cache_usage[]=-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";+staticconstchar*constcheckout_cache_usage[]={+"git checkout-index [options] [--] <file>...",+NULL+};++staticintparse_state_force_cb(conststructoption*opt,constchar*arg,intunset)+{+structcheckout*t_state=opt->value;+t_state->force=unset?0:1;+return0;+}++staticintparse_state_quiet_cb(conststructoption*opt,constchar*arg,intunset)+{+structcheckout*t_state=opt->value;+t_state->quiet=unset?0:1;+return0;+}++staticintparse_state_no_create_cb(conststructoption*opt,constchar*arg,intunset)+{+structcheckout*t_state=opt->value;+t_state->not_new=1;+return0;+}++staticintparse_state_index_cb(conststructoption*opt,constchar*arg,intunset)+{+structcheckout*t_state=opt->value;+t_state->refresh_cache=unset?0:1;+return0;+}staticstructlock_filelock_file;intcmd_checkout_index(intargc,constchar**argv,constchar*prefix){-inti;intnewfd=-1;intall=0;intread_from_stdin=0;intprefix_length;+char*stage=NULL;++conststructoptionoptions[]={+OPT_BOOLEAN('a',"all",&all,+"checks out all files in the index"),+{OPTION_CALLBACK,'f',"force",&state,NULL,+"force overwrite of existing files",+PARSE_OPT_NOARG,parse_state_force_cb,0},+{OPTION_CALLBACK,'q',"quiet",&state,NULL,"be quiet",+PARSE_OPT_NOARG,parse_state_quiet_cb,0},+{OPTION_CALLBACK,'n',"no-create",&state,NULL,+"do not checkout new files, refresh existing ones",+PARSE_OPT_NOARG|PARSE_OPT_NONEG,+parse_state_no_create_cb,0},+{OPTION_CALLBACK,'u',"index",&state,NULL,+"update stat information in the index",+PARSE_OPT_NOARG,parse_state_index_cb,0},+OPT_SET_INT('z',NULL,&line_termination,+"separate paths with NUL",0),+OPT_BOOLEAN(0,"stdin",&read_from_stdin,+"read paths from stdin"),+OPT_BOOLEAN(0,"temp",&to_tempfile,+"write content to temporary files"),+OPT_STRING(0,"prefix",&state.base_dir,"string",+"prepend <string> when creating files"),+OPT_STRING(0,"stage",&stage,"1|2|3|all",+"copy out files from the named stage"),+OPT_END()+};git_config(git_default_config,NULL);state.base_dir="";
@@ -174,71 +233,24 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)die("invalid cache");}-for(i=1;i<argc;i++){-constchar*arg=argv[i];+argc=parse_options(argc,argv,options,checkout_cache_usage,0);-if(!strcmp(arg,"--")){-i++;-break;-}-if(!strcmp(arg,"-a")||!strcmp(arg,"--all")){-all=1;-continue;-}-if(!strcmp(arg,"-f")||!strcmp(arg,"--force")){-state.force=1;-continue;-}-if(!strcmp(arg,"-q")||!strcmp(arg,"--quiet")){-state.quiet=1;-continue;-}-if(!strcmp(arg,"-n")||!strcmp(arg,"--no-create")){-state.not_new=1;-continue;-}-if(!strcmp(arg,"-u")||!strcmp(arg,"--index")){-state.refresh_cache=1;-if(newfd<0)-newfd=hold_locked_index(&lock_file,1);-continue;-}-if(!strcmp(arg,"-z")){-line_termination=0;-continue;-}-if(!strcmp(arg,"--stdin")){-if(i!=argc-1)-die("--stdin must be at the end");-read_from_stdin=1;-i++;/* do not consider arg as a file name */-break;-}-if(!strcmp(arg,"--temp")){+if((state.refresh_cache)&&(newfd<0))+newfd=hold_locked_index(&lock_file,1);+if(state.base_dir)+state.base_dir_len=strlen(state.base_dir);++if(stage){+if(!strcmp(stage,"all")){to_tempfile=1;-continue;-}-if(!prefixcmp(arg,"--prefix=")){-state.base_dir=arg+9;-state.base_dir_len=strlen(state.base_dir);-continue;-}-if(!prefixcmp(arg,"--stage=")){-if(!strcmp(arg+8,"all")){-to_tempfile=1;-checkout_stage=CHECKOUT_ALL;-}else{-intch=arg[8];-if('1'<=ch&&ch<='3')-checkout_stage=arg[8]-'0';-else-die("stage should be between 1 and 3 or all");-}-continue;+checkout_stage=CHECKOUT_ALL;+}else{+intch=stage[0];+if('1'<=ch&&ch<='3')+checkout_stage=stage[0]-'0';+else+die("stage should be between 1 and 3 or all");}-if(arg[0]=='-')-usage(checkout_cache_usage);-break;}if(state.base_dir_len||to_tempfile){
@@ -253,8 +265,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)}/* Check out named files first */-for(;i<argc;i++){-constchar*arg=argv[i];+while(argc-->0){+constchar*arg=*argv++;constchar*p;if(all)
@@ -367,25 +377,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)constchar*git_dir;constchar*template_dir=NULL;unsignedintflags=0;-inti;--for(i=1;i<argc;i++,argv++){-constchar*arg=argv[1];-if(!prefixcmp(arg,"--template="))-template_dir=arg+11;-elseif(!strcmp(arg,"--bare")){-staticchargit_dir[PATH_MAX+1];-is_bare_repository_cfg=1;-setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,-sizeof(git_dir)),0);-}elseif(!strcmp(arg,"--shared"))-shared_repository=PERM_GROUP;-elseif(!prefixcmp(arg,"--shared="))-shared_repository=git_config_perm("arg",arg+9);-elseif(!strcmp(arg,"-q")||!strcmp(arg,"--quiet"))-flags|=INIT_DB_QUIET;-else-usage(init_db_usage);+intbare=0;++conststructoptionoptions[]={+OPT_STRING(0,"template",&template_dir,"dir",+"directory from which templates will be used"),+OPT_BOOLEAN(0,"bare",&bare,"set up a bare repo"),+{OPTION_CALLBACK,0,"shared",&shared_repository,+"type","type of shared repository",+PARSE_OPT_OPTARG,parse_opt_shared_cb,PERM_GROUP},+OPT_BIT('q',"quiet",&flags,"be quiet",INIT_DB_QUIET),+OPT_END()+};++argc=parse_options(argc,argv,options,init_db_usage,0);++if(argc>0)+usage_with_options(init_db_usage,options);++if(bare){+staticchargit_dir[PATH_MAX+1];+is_bare_repository_cfg=1;+setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,+sizeof(git_dir)),0);}/*
@@ -677,60 +730,43 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)nr_heads=0;heads=NULL;-for(i=1;i<argc;i++){-constchar*arg=argv[i];-if(*arg=='-'){-if(!prefixcmp(arg,"--upload-pack=")){-args.uploadpack=arg+14;-continue;-}-if(!prefixcmp(arg,"--exec=")){-args.uploadpack=arg+7;-continue;-}-if(!strcmp("--quiet",arg)||!strcmp("-q",arg)){-args.quiet=1;-continue;-}-if(!strcmp("--keep",arg)||!strcmp("-k",arg)){-args.lock_pack=args.keep_pack;-args.keep_pack=1;-continue;-}-if(!strcmp("--thin",arg)){-args.use_thin_pack=1;-continue;-}-if(!strcmp("--include-tag",arg)){-args.include_tag=1;-continue;-}-if(!strcmp("--all",arg)){-args.fetch_all=1;-continue;-}-if(!strcmp("-v",arg)){-args.verbose=1;-continue;-}-if(!prefixcmp(arg,"--depth=")){-args.depth=strtol(arg+8,NULL,0);-continue;-}-if(!strcmp("--no-progress",arg)){-args.no_progress=1;-continue;-}-usage(fetch_pack_usage);-}-dest=(char*)arg;-heads=(char**)(argv+i+1);-nr_heads=argc-i-1;-break;-}+conststructoptionoptions[]={+{OPTION_CALLBACK,0,"all",&args,NULL,+"fetch all remote refs",PARSE_OPT_NOARG,+parse_opt_fetch_all_cb},+OPT_STRING(0,"upload-pack",&args.uploadpack,"git-upload-pack",+"specify path to git-upload-pack on remote"),+OPT_STRING(0,"exec",&args.uploadpack,"git-upload-pack",+"same as --upload-pack <git-upload-pack>."),+{OPTION_CALLBACK,0,"no-progress",&args,NULL,+"do not show the progress",PARSE_OPT_NOARG|PARSE_OPT_NONEG,+parse_opt_no_progress_cb},+{OPTION_CALLBACK,'q',"quiet",&args,NULL,+"be quiet",PARSE_OPT_NOARG,parse_opt_quiet_cb},+{OPTION_CALLBACK,'v',"verbose",&args,NULL,+"be verbose",PARSE_OPT_NOARG,parse_opt_verbose_cb},+OPT_INTEGER(0,"depth",&args.depth,"fetch chains not longer than <n>"),+{OPTION_CALLBACK,'k',"keep",&args,NULL,+"create a single packfile of received data",+PARSE_OPT_NOARG|PARSE_OPT_NONEG,parse_opt_keep_pack_cb},+{OPTION_CALLBACK,0,"include-tag",&args,NULL,+"download annotated tags too",PARSE_OPT_NOARG,+parse_opt_include_tag_cb},+{OPTION_CALLBACK,0,"thin",&args,NULL,+"minimize number of objects to be sent",+PARSE_OPT_NOARG,parse_opt_thin_cb},+OPT_END()+};++argc=parse_options(argc,argv,options,fetch_pack_usage,0);++dest=(char*)argv[0];+heads=(char**)(argv+1);+nr_heads=argc-1;+if(!dest)-usage(fetch_pack_usage);+usage_with_options(fetch_pack_usage,options);conn=git_connect(fd,(char*)dest,args.uploadpack,args.verbose?CONNECT_VERBOSE:0);
@@ -88,23 +91,25 @@ static int verify_tag(const char *name, int verbose)intcmd_verify_tag(intargc,constchar**argv,constchar*prefix){-inti=1,verbose=0,had_error=0;+intverbose=0,had_error=0;git_config(git_default_config,NULL);+conststructoptionoptions[]={+OPT__VERBOSE(&verbose),+OPT_END()+};+if(argc==1)-usage(builtin_verify_tag_usage);+usage_with_options(builtin_verify_tag_usage,options);-if(!strcmp(argv[i],"-v")||!strcmp(argv[i],"--verbose")){-verbose=1;-i++;-}+argc=parse_options(argc,argv,options,builtin_verify_tag_usage,0);/* sometimes the program was terminated because this signal*wasreceivedintheprocessofwritingthegpginput:*/signal(SIGPIPE,SIG_IGN);-while(i<argc)-if(verify_tag(argv[i++],verbose))+while(argc-->0)+if(verify_tag(*argv++,verbose))had_error=1;returnhad_error;}
@@ -19,19 +22,19 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)constchar*me="git-write-tree";git_config(git_default_config,NULL);-while(1<argc){-constchar*arg=argv[1];-if(!strcmp(arg,"--missing-ok"))-missing_ok=1;-elseif(!prefixcmp(arg,"--prefix="))-prefix=arg+9;-else-usage(write_tree_usage);-argc--;argv++;-}-if(argc>2)-die("too many options");+conststructoptionoptions[]={+OPT_BOOLEAN(0,"missing-ok",&missing_ok,+"disable existence check"),+OPT_STRING(0,"prefix",&prefix,"directory",+"write a tree object for <directory>"),+OPT_END()+};++argc=parse_options(argc,argv,options,write_tree_usage,0);++if(argc>0)+usage_with_options(write_tree_usage,options);ret=write_cache_as_tree(sha1,missing_ok,prefix);switch(ret){
@@ -122,70 +125,47 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)unsignedcharsha1[20];structtree*tree;+conststructoptionoptions[]={+OPT_SET_INT('z',NULL,&line_termination,+"\\0 line termination on output",0),+OPT_BIT('r',NULL,&ls_options,+"recurse into sub-trees",LS_RECURSIVE),+OPT_BIT('d',NULL,&ls_options,+"show only the tree, not its children",+LS_TREE_ONLY),+OPT_BIT('t',NULL,&ls_options,+"show tree entries",LS_SHOW_TREES),+OPT_BIT('l',"long",&ls_options,+"show object size of blob (file) entries",+LS_SHOW_SIZE),+OPT_BIT(0,"name-only",&ls_options,+"list only filenames",LS_NAME_ONLY),+OPT_BIT(0,"name-status",&ls_options,+"same as --name-only",LS_NAME_ONLY),+OPT_SET_INT(0,"full-name",&chomp_prefix,+"show the full path name",0),+OPT__ABBREV(&abbrev),+OPT_END()+};+git_config(git_default_config,NULL);ls_tree_prefix=prefix;if(prefix&&*prefix)chomp_prefix=strlen(prefix);-while(1<argc&&argv[1][0]=='-'){-switch(argv[1][1]){-case'z':-line_termination=0;-break;-case'r':-ls_options|=LS_RECURSIVE;-break;-case'd':-ls_options|=LS_TREE_ONLY;-break;-case't':-ls_options|=LS_SHOW_TREES;-break;-case'l':-ls_options|=LS_SHOW_SIZE;-break;-case'-':-if(!strcmp(argv[1]+2,"name-only")||-!strcmp(argv[1]+2,"name-status")){-ls_options|=LS_NAME_ONLY;-break;-}-if(!strcmp(argv[1]+2,"long")){-ls_options|=LS_SHOW_SIZE;-break;-}-if(!strcmp(argv[1]+2,"full-name")){-chomp_prefix=0;-break;-}-if(!prefixcmp(argv[1]+2,"abbrev=")){-abbrev=strtoul(argv[1]+9,NULL,10);-if(abbrev&&abbrev<MINIMUM_ABBREV)-abbrev=MINIMUM_ABBREV;-elseif(abbrev>40)-abbrev=40;-break;-}-if(!strcmp(argv[1]+2,"abbrev")){-abbrev=DEFAULT_ABBREV;-break;-}-/* otherwise fallthru */-default:-usage(ls_tree_usage);-}-argc--;argv++;-}++argc=parse_options(argc,argv,options,ls_tree_usage,0);+/* -d -r should imply -t, but -d by itself should not have to. */if((LS_TREE_ONLY|LS_RECURSIVE)==((LS_TREE_ONLY|LS_RECURSIVE)&ls_options))ls_options|=LS_SHOW_TREES;-if(argc<2)-usage(ls_tree_usage);-if(get_sha1(argv[1],sha1))-die("Not a valid object name %s",argv[1]);+if(argc<1)+usage_with_options(ls_tree_usage,options);+if(get_sha1(argv[0],sha1))+die("Not a valid object name %s",argv[0]);-pathspec=get_pathspec(prefix,argv+2);+pathspec=get_pathspec(prefix,argv+1);tree=parse_tree_indirect(sha1);if(!tree)die("not a tree object");
@@ -575,15 +547,65 @@ static struct commit_list *find_bisection(struct commit_list *list,returnbest;}+staticintparse_header_cb(conststructoption*opt,constchar*arg,intunset)+{+structrev_info*t_revs=opt->value;+t_revs->verbose_header=unset?0:1;+return0;+}+intcmd_rev_list(intargc,constchar**argv,constchar*prefix){structcommit_list*list;-inti;intread_from_stdin=0;intbisect_show_vars=0;intbisect_find_all=0;intquiet=0;+conststructoptionoptions[]={+OPT_GROUP("limiting output:"),+OPT_ARGUMENT("max-count=nr","limit number of commits output"),+OPT_ARGUMENT("max-age=epoch","limit commits output by time"),+OPT_ARGUMENT("min-age=epoch","limit commits output by time"),+OPT_ARGUMENT("sparse",""),+OPT_ARGUMENT("no-merges","do not print merges"),+OPT_ARGUMENT("remove-empty","stop when a given path disappears from the tree"),+OPT_ARGUMENT("all","all refs"),+OPT_ARGUMENT("branches","show local branches"),+OPT_ARGUMENT("tags","show tags"),+OPT_ARGUMENT("remotes","show remote-tracking branches"),+OPT_BOOLEAN(0,"stdin",&read_from_stdin,+"read commits also from command line"),+OPT__QUIET(&quiet),+OPT_GROUP("ordering output:"),+OPT_ARGUMENT("topo-order","show commits in topological order"),+OPT_ARGUMENT("date-order","use date order, preserving topology"),+OPT_ARGUMENT("reverse","output commits in reverse order"),+OPT_GROUP("formatting output:"),+OPT_ARGUMENT("parents","print the parents of the commit"),+OPT_ARGUMENT("children","print the children of the commit"),+OPT_ARGUMENT("objects","print all objects"),+OPT_ARGUMENT("objects-edge","similar to --objects, used by git-pack-objects"),+OPT_ARGUMENT("unpacked","print objects not in packs"),+{OPTION_CALLBACK,0,"header",&revs,NULL,+"use raw-format",PARSE_OPT_NOARG,parse_header_cb,0},+OPT_ARGUMENT("pretty","print contents in a given format"),+OPT_BOOLEAN(0,"timestamp",&show_timestamp,+"print the raw commit timestamp"),+OPT_ARGUMENT("abbrev-commit","show short sha1"),+OPT_ARGUMENT("abbrev=nr","number of digits used for short sha1"),+OPT_ARGUMENT("no-abbrev","do not use short sha1"),+OPT_ARGUMENT("left-right","mark side of symmetric diff"),+OPT_ARGUMENT("graph","show an ASCII graph"),+OPT_GROUP("special purpose:"),+OPT_BOOLEAN(0,"bisect",&bisect_list,"useful for binary searches"),+OPT_BOOLEAN(0,"bisect-all",&bisect_find_all,+"order commits by their distance from given commits"),+OPT_BOOLEAN(0,"bisect-vars",&bisect_show_vars,+"like --bisect, but ready to be eval'ed"),+OPT_END()+};+git_config(git_default_config,NULL);init_revisions(&revs,prefix);revs.abbrev=0;
@@ -591,40 +613,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)argc=setup_revisions(argc,argv,&revs,NULL);quiet=DIFF_OPT_TST(&revs.diffopt,QUIET);-for(i=1;i<argc;i++){-constchar*arg=argv[i];+argc=parse_options(argc,argv,options,rev_list_usage,0);-if(!strcmp(arg,"--header")){-revs.verbose_header=1;-continue;-}-if(!strcmp(arg,"--timestamp")){-show_timestamp=1;-continue;-}-if(!strcmp(arg,"--bisect")){-bisect_list=1;-continue;-}-if(!strcmp(arg,"--bisect-all")){-bisect_list=1;-bisect_find_all=1;-continue;-}-if(!strcmp(arg,"--bisect-vars")){-bisect_list=1;-bisect_show_vars=1;-continue;-}-if(!strcmp(arg,"--stdin")){-if(read_from_stdin++)-die("--stdin given twice?");-read_revisions_from_stdin(&revs);-continue;-}-usage(rev_list_usage);+if(argc>0)+usage_with_options(rev_list_usage,options);++if(bisect_find_all||bisect_show_vars)+bisect_list=1;+if(read_from_stdin)+read_revisions_from_stdin(&revs);-}if(revs.commit_format!=CMIT_FMT_UNSPECIFIED){/* The command line has a --pretty */hdr_termination='\n';
Are you sure the default value is really used here?
Also, perhaps we can play it safer by avoiding changing "share_repository"
directly.
$ git init -> shared_repository == PERM_UMASK
$ git init --shared --no-shared -> shared_repository == 0
It works because PERM_UMASK == 0, but it is a side effect. Don't you think?
if (argc == 1)
- usage(builtin_verify_tag_usage);
+ usage_with_options(builtin_verify_tag_usage, options);
It seems this is broken since the C rewrite: "git verify-tag -v" just do
nothing instead of printing usage message.
Moving the if() after parse_options() call with s/argc == 1/argc == 0/
should do the trick.
Are you sure the default value is really used here?
Also, perhaps we can play it safer by avoiding changing "share_repository"
directly.
I do not see how that would be any safer.
$ git init -> shared_repository == PERM_UMASK
$ git init --shared --no-shared -> shared_repository == 0
It works because PERM_UMASK == 0, but it is a side effect. Don't you think?
Then the callback is wrong, too. I think, however, that it is by design,
and correct.
We rely on shared_repository == 0 for non-shared repositories _almost
everywhere_.
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:00
Hi,
On Thu, 24 Jul 2008, Olivier Marin wrote:
Michele Ballabio a écrit :
quoted
if (argc == 1)
- usage(builtin_verify_tag_usage);
+ usage_with_options(builtin_verify_tag_usage, options);
It seems this is broken since the C rewrite: "git verify-tag -v" just do
nothing instead of printing usage message.
Moving the if() after parse_options() call with s/argc == 1/argc == 0/
should do the trick.
That would be a bugfix. As such, it belongs into a different commit.
Care to provide a patch?
Are you sure the default value is really used here?
Yes. Perhaps I don't understand your question. Can you explain what you mean?
Also, perhaps we can play it safer by avoiding changing "share_repository"
directly.
$ git init -> shared_repository == PERM_UMASK
$ git init --shared --no-shared -> shared_repository == 0
It works because PERM_UMASK == 0, but it is a side effect. Don't you think?
Would you like this better, with PARSE_OPT_NONEG?
+ { OPTION_CALLBACK, 0, "shared", &shared_repository,
+ "permissions", "set up a shared repository",
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, parse_opt_shared_cb, PERM_GROUP },
Or do you prefer changing the callback like this:
+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+ int unset)
+{
+ *(int *)(opt->value) = unset ? PERM_UMASK : git_config_perm("arg", arg);
+ return 0;
+}
Did it this way (and changed help strings).
builtin-init-db.c | 57 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 36 insertions(+), 21 deletions(-)
@@ -367,25 +378,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)constchar*git_dir;constchar*template_dir=NULL;unsignedintflags=0;-inti;--for(i=1;i<argc;i++,argv++){-constchar*arg=argv[1];-if(!prefixcmp(arg,"--template="))-template_dir=arg+11;-elseif(!strcmp(arg,"--bare")){-staticchargit_dir[PATH_MAX+1];-is_bare_repository_cfg=1;-setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,-sizeof(git_dir)),0);-}elseif(!strcmp(arg,"--shared"))-shared_repository=PERM_GROUP;-elseif(!prefixcmp(arg,"--shared="))-shared_repository=git_config_perm("arg",arg+9);-elseif(!strcmp(arg,"-q")||!strcmp(arg,"--quiet"))-flags|=INIT_DB_QUIET;-else-usage(init_db_usage);+intbare=0;++conststructoptionoptions[]={+OPT_STRING(0,"template",&template_dir,"path",+"path to the template directory"),+OPT_BOOLEAN(0,"bare",&bare,"set up a bare repository"),+{OPTION_CALLBACK,0,"shared",&shared_repository,+"permissions","set up a shared repository",+PARSE_OPT_OPTARG,parse_opt_shared_cb,PERM_GROUP},+OPT_BIT('q',"quiet",&flags,"be quiet",INIT_DB_QUIET),+OPT_END()+};++argc=parse_options(argc,argv,options,init_db_usage,0);++if(argc>0)+usage_with_options(init_db_usage,options);++if(bare){+staticchargit_dir[PATH_MAX+1];+is_bare_repository_cfg=1;+setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,+sizeof(git_dir)),0);}/*
I wonder if this could not be written as
OPT_BOOLEAN('f', "force", &state.force,
"force overwrite of existing files"),
I did it that way because 'force' is a bitfield.
I thought there is an OPT_BIT?
OPT_BIT is for flags and bitmasks, not for bitfields.
Since you can't get the address of a bitfield member, a function that
wants to change its value needs to know its name. Switching to bitmasks
would make the option parsing code look cleaner, but you'd have to
change all those bitfield accesses to explicit bitmask operations, e.g.:
if (state.force)
state.force = 0;
vs.
if (state.flags & CHECKOUT_FORCE)
state.flags &= ~CHECKOUT_FORCE;
In the case of struct checkout, though, we could simply make the
bitfield members full ints, because there are only a few instances of
this structure in memory at any given time. Wasting a few bytes of RAM
in order to gain much simpler code is OK in this case, I think.
OPT_BOOLEAN looks a lot nicer than a callback.
René
+ OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
s/set up/setup/
No. "setup" is a noun.
Right, sorry.
We rely on shared_repository == 0 for non-shared repositories _almost
everywhere_.
I think we rely on the fact that PERM_UMASK == 0 and not on the value
of shared_repository. Not the same thing.
That said, perhaps you are right: it is harmless.
Are you sure the default value is really used here?
Yes. Perhaps I don't understand your question. Can you explain what you mean?
If I read the code correctly in parse-options.c, with OPTION_CALLBACK, the
default value is not "automatically" used. You can use it in your callback
if you want, but because you don't, I think it's never used.
Would you like this better, with PARSE_OPT_NONEG?
No, I'm fine with the negated option.
Or do you prefer changing the callback like this:
+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+ int unset)
+{
+ *(int *)(opt->value) = unset ? PERM_UMASK : git_config_perm("arg", arg);
+ return 0;
+}
I think it's better but what I suggested is more something like:
static int parse_opt_shared_cb(const struct option *opt, const char *arg,
int unset)
{
*(int *)(opt->value) = unset ? -1 : git_config_perm("arg", arg);
return 0;
}
int shared = -1;
{ OPTION_CALLBACK, 0, "shared", &shared,
"permissions", "setup as shared repository",
PARSE_OPT_OPTARG, parse_perm_callback },
if (shared >= 0)
shared_repository = shared;
This way we do not change shared_repository during parsing, so we do not
loose the initial value.
But it seems nobody care about this kind of details, so perhaps, you can
just ignore this suggestion.
Olivier.
In the case of struct checkout, though, we could simply make the
bitfield members full ints, because there are only a few instances of
this structure in memory at any given time. Wasting a few bytes of RAM
in order to gain much simpler code is OK in this case, I think.
OPT_BOOLEAN looks a lot nicer than a callback.
Yes. I only wanted the changes to be minimal, and only affect the option
parsing. In this sense, I still think the old patch is better. Here it is
the one you suggested (maybe Johannes suggested the same, but I didn't
understand :).
builtin-checkout-index.c | 113 +++++++++++++++++++---------------------------
cache.h | 8 ++--
2 files changed, 50 insertions(+), 71 deletions(-)
@@ -153,18 +154,43 @@ static void checkout_all(const char *prefix, int prefix_length)exit(128);}-staticconstcharcheckout_cache_usage[]=-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";+staticconstchar*constcheckout_cache_usage[]={+"git checkout-index [options] [--] <file>...",+NULL+};staticstructlock_filelock_file;intcmd_checkout_index(intargc,constchar**argv,constchar*prefix){-inti;intnewfd=-1;intall=0;intread_from_stdin=0;intprefix_length;+char*stage=NULL;++conststructoptionoptions[]={+OPT_BOOLEAN('a',"all",&all,+"checks out all files in the index"),+OPT_BOOLEAN('f',"force",&state.force,+"force overwrite of existing files"),+OPT__QUIET(&state.quiet),+OPT_SET_INT('n',"no-create",&state.not_new,+"do not checkout new files, refresh existing ones",1),+OPT_BOOLEAN('u',"index",&state.refresh_cache,+"update stat information in the index"),+OPT_SET_INT('z',NULL,&line_termination,+"separate paths with NUL",0),+OPT_BOOLEAN(0,"stdin",&read_from_stdin,+"read paths from stdin"),+OPT_BOOLEAN(0,"temp",&to_tempfile,+"write content to temporary files"),+OPT_STRING(0,"prefix",&state.base_dir,"string",+"prepend <string> when creating files"),+OPT_STRING(0,"stage",&stage,"1|2|3|all",+"copy out files from the named stage"),+OPT_END()+};git_config(git_default_config,NULL);state.base_dir="";
@@ -174,71 +200,24 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)die("invalid cache");}-for(i=1;i<argc;i++){-constchar*arg=argv[i];+argc=parse_options(argc,argv,options,checkout_cache_usage,0);-if(!strcmp(arg,"--")){-i++;-break;-}-if(!strcmp(arg,"-a")||!strcmp(arg,"--all")){-all=1;-continue;-}-if(!strcmp(arg,"-f")||!strcmp(arg,"--force")){-state.force=1;-continue;-}-if(!strcmp(arg,"-q")||!strcmp(arg,"--quiet")){-state.quiet=1;-continue;-}-if(!strcmp(arg,"-n")||!strcmp(arg,"--no-create")){-state.not_new=1;-continue;-}-if(!strcmp(arg,"-u")||!strcmp(arg,"--index")){-state.refresh_cache=1;-if(newfd<0)-newfd=hold_locked_index(&lock_file,1);-continue;-}-if(!strcmp(arg,"-z")){-line_termination=0;-continue;-}-if(!strcmp(arg,"--stdin")){-if(i!=argc-1)-die("--stdin must be at the end");-read_from_stdin=1;-i++;/* do not consider arg as a file name */-break;-}-if(!strcmp(arg,"--temp")){+if((state.refresh_cache)&&(newfd<0))+newfd=hold_locked_index(&lock_file,1);+if(state.base_dir)+state.base_dir_len=strlen(state.base_dir);++if(stage){+if(!strcmp(stage,"all")){to_tempfile=1;-continue;-}-if(!prefixcmp(arg,"--prefix=")){-state.base_dir=arg+9;-state.base_dir_len=strlen(state.base_dir);-continue;-}-if(!prefixcmp(arg,"--stage=")){-if(!strcmp(arg+8,"all")){-to_tempfile=1;-checkout_stage=CHECKOUT_ALL;-}else{-intch=arg[8];-if('1'<=ch&&ch<='3')-checkout_stage=arg[8]-'0';-else-die("stage should be between 1 and 3 or all");-}-continue;+checkout_stage=CHECKOUT_ALL;+}else{+intch=stage[0];+if('1'<=ch&&ch<='3')+checkout_stage=stage[0]-'0';+else+die("stage should be between 1 and 3 or all");}-if(arg[0]=='-')-usage(checkout_cache_usage);-break;}if(state.base_dir_len||to_tempfile){
@@ -253,8 +232,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)}/* Check out named files first */-for(;i<argc;i++){-constchar*arg=argv[i];+while(argc-->0){+constchar*arg=*argv++;constchar*p;if(all)
Are you sure the default value is really used here?
Yes. Perhaps I don't understand your question. Can you explain what you mean?
If I read the code correctly in parse-options.c, with OPTION_CALLBACK, the
default value is not "automatically" used. You can use it in your callback
if you want, but because you don't, I think it's never used.
Oh, you're right, but git_config_perm() handles NULL just fine, so I can
remove it. Done in this patch, thanks.
what I suggested is more something like:
static int parse_opt_shared_cb(const struct option *opt, const char *arg,
int unset)
{
*(int *)(opt->value) = unset ? -1 : git_config_perm("arg", arg);
return 0;
}
int shared = -1;
{ OPTION_CALLBACK, 0, "shared", &shared,
"permissions", "setup as shared repository",
PARSE_OPT_OPTARG, parse_perm_callback },
if (shared >= 0)
shared_repository = shared;
This way we do not change shared_repository during parsing, so we do not
loose the initial value.
But it seems nobody care about this kind of details, so perhaps, you can
just ignore this suggestion.
I might be wrong, but shared_repository is initialized to PERM_UMASK and
does not change before parse_options() is called, so this is not much
useful.
builtin-init-db.c | 57 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 36 insertions(+), 21 deletions(-)
@@ -367,25 +378,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)constchar*git_dir;constchar*template_dir=NULL;unsignedintflags=0;-inti;--for(i=1;i<argc;i++,argv++){-constchar*arg=argv[1];-if(!prefixcmp(arg,"--template="))-template_dir=arg+11;-elseif(!strcmp(arg,"--bare")){-staticchargit_dir[PATH_MAX+1];-is_bare_repository_cfg=1;-setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,-sizeof(git_dir)),0);-}elseif(!strcmp(arg,"--shared"))-shared_repository=PERM_GROUP;-elseif(!prefixcmp(arg,"--shared="))-shared_repository=git_config_perm("arg",arg+9);-elseif(!strcmp(arg,"-q")||!strcmp(arg,"--quiet"))-flags|=INIT_DB_QUIET;-else-usage(init_db_usage);+intbare=0;++conststructoptionoptions[]={+OPT_STRING(0,"template",&template_dir,"path",+"path to the template directory"),+OPT_BOOLEAN(0,"bare",&bare,"set up a bare repository"),+{OPTION_CALLBACK,0,"shared",&shared_repository,+"permissions","set up a shared repository",+PARSE_OPT_OPTARG,parse_opt_shared_cb},+OPT_BIT('q',"quiet",&flags,"be quiet",INIT_DB_QUIET),+OPT_END()+};++argc=parse_options(argc,argv,options,init_db_usage,0);++if(argc>0)+usage_with_options(init_db_usage,options);++if(bare){+staticchargit_dir[PATH_MAX+1];+is_bare_repository_cfg=1;+setenv(GIT_DIR_ENVIRONMENT,getcwd(git_dir,+sizeof(git_dir)),0);}/*
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:02
Hi,
On Fri, 25 Jul 2008, Olivier Marin wrote:
Johannes Schindelin a écrit :
quoted
We rely on shared_repository == 0 for non-shared repositories _almost
everywhere_.
I think we rely on the fact that PERM_UMASK == 0 and not on the value of
shared_repository. Not the same thing.
Just look at all the cases where we ask for "if (shared_repository)".
And then look where PERM_UMASK is assigned to. It _is_ the same thing.
Hth,
Dscho
From: Olivier Marin <redacted>
Since the C rewrite, "git verify-tag -v" just does nothing instead of
printing the usage message with an error. This patch fix the regression.
Signed-off-by: Olivier Marin <redacted>
---
builtin-verify-tag.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
@@ -92,14 +92,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);-if(argc==1)-usage(builtin_verify_tag_usage);--if(!strcmp(argv[i],"-v")||!strcmp(argv[i],"--verbose")){+if(argc>1&&+(!strcmp(argv[i],"-v")||!strcmp(argv[i],"--verbose"))){verbose=1;i++;}+if(argc<=i)+usage(builtin_verify_tag_usage);+/* sometimes the program was terminated because this signal*wasreceivedintheprocessofwritingthegpginput:*/signal(SIGPIPE,SIG_IGN);
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:45:03
Hi,
On Mon, 28 Jul 2008, Olivier Marin wrote:
From: Olivier Marin <redacted>
Since the C rewrite, "git verify-tag -v" just does nothing instead of
printing the usage message with an error. This patch fix the regression.
Maybe a better solution would be to convert (trivially) to
parse-options...
Ciao,
Dscho
Since the C rewrite, "git verify-tag -v" just does nothing instead of
printing the usage message with an error. This patch fix the regression.
Maybe a better solution would be to convert (trivially) to
parse-options...
I am very puzzled.
You first asked me to do a separate commit with just the fix and now
you seem to want the fix with the conversion...
What do you mean by "trivially"?
Olivier.