From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:10
I noticed that Duy's project around USE_THE_INDEX_COMPATIBILITY_MACROS has
been on pause for a while. Here is my attempt to continue that project a
little.
I started going through the builtins that still use cache_name_pos() and the
first few were easy: merge-inex, mv, rm.
Then I hit update-index and it was a bit bigger. It's included here as well.
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
I had started trying to keep everything local to the method signatures, but
I hit a snag when reaching the command-line parsing callbacks, which I could
not modify their call signature. At that point, I had something that was
already much more complicated than what I present now. Outside of the first
update-index commit, everything was a mechanical find/replace.
In total, this allows us to remove four of the compatibility macros because
they are no longer used.
Thanks, -Stolee
Derrick Stolee (12):
merge-index: drop index compatibility macros
mv: remove index compatibility macros
rm: remove compatilibity macros
update-index: drop the_index, the_repository
update-index: use istate->cache over active_cache
update-index: use index->cache_nr over active_nr
update-index: use istate->cache_changed
update-index: use index_name_pos() over cache_name_pos()
update-index: use remove_file_from_index()
update-index: use add_index_entry()
update-index: replace several compatibility macros
update-index: remove ce_match_stat(), all macros
Documentation/technical/racy-git.txt | 6 +-
builtin/merge-index.c | 33 +++---
builtin/mv.c | 42 ++++----
builtin/rm.c | 56 ++++++-----
builtin/update-index.c | 145 ++++++++++++++-------------
cache.h | 4 -
6 files changed, 149 insertions(+), 137 deletions(-)
base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-830%2Fderrickstolee%2Findex-compatibility-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-830/derrickstolee/index-compatibility-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/830
--
gitgitgadget
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:10
From: Derrick Stolee <redacted>
Replace uses of the old macros for the_index and instead pass around a
'struct index_state' pointer. This allows dropping the compatibility
flag.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/merge-index.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
@@ -6,18 +5,19 @@ static const char *pgm;staticintone_shot,quiet;staticinterr;-staticintmerge_entry(intpos,constchar*path)+staticintmerge_entry(structindex_state*istate,+intpos,constchar*path){intfound;constchar*arguments[]={pgm,"","","",path,"","","",NULL};charhexbuf[4][GIT_MAX_HEXSZ+1];charownbuf[4][60];-if(pos>=active_nr)+if(pos>=istate->cache_nr)die("git merge-index: %s not in the cache",path);found=0;do{-conststructcache_entry*ce=active_cache[pos];+conststructcache_entry*ce=istate->cache[pos];intstage=ce_stage(ce);if(strcmp(ce->name,path))
@@ -27,7 +27,7 @@ static int merge_entry(int pos, const char *path)xsnprintf(ownbuf[stage],sizeof(ownbuf[stage]),"%o",ce->ce_mode);arguments[stage]=hexbuf[stage];arguments[stage+4]=ownbuf[stage];-}while(++pos<active_nr);+}while(++pos<istate->cache_nr);if(!found)die("git merge-index: %s not in the cache",path);
@@ -43,32 +43,34 @@ static int merge_entry(int pos, const char *path)returnfound;}-staticvoidmerge_one_path(constchar*path)+staticvoidmerge_one_path(structindex_state*istate,+constchar*path){-intpos=cache_name_pos(path,strlen(path));+intpos=index_name_pos(istate,path,strlen(path));/**Ifitalreadyexistsinthecacheasstage0,it's*alreadymergedandthereisnothingtodo.*/if(pos<0)-merge_entry(-pos-1,path);+merge_entry(istate,-pos-1,path);}-staticvoidmerge_all(void)+staticvoidmerge_all(structindex_state*istate){inti;-for(i=0;i<active_nr;i++){-conststructcache_entry*ce=active_cache[i];+for(i=0;i<istate->cache_nr;i++){+conststructcache_entry*ce=istate->cache[i];if(!ce_stage(ce))continue;-i+=merge_entry(i,ce->name)-1;+i+=merge_entry(istate,i,ce->name)-1;}}intcmd_merge_index(intargc,constchar**argv,constchar*prefix){inti,force_file=0;+structindex_state*istate;/* Without this we cannot rely on waitpid() to tell*whathappenedtoourchildren.
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:10
From: Derrick Stolee <redacted>
The mv builtin uses the compatibility macros to interact with the index.
Update these to use modern methods referring to a 'struct index_state'
pointer. Several helper methods need to be updated to consider such a
pointer, but the modifications are rudimentary.
Two macros can be deleted from cache.h because these are the last uses.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/mv.c | 42 +++++++++++++++++++++++-------------------
cache.h | 2 --
2 files changed, 23 insertions(+), 21 deletions(-)
@@ -75,13 +74,14 @@ static const char *add_slash(const char *path)#define SUBMODULE_WITH_GITDIR ((const char *)1)-staticvoidprepare_move_submodule(constchar*src,intfirst,+staticvoidprepare_move_submodule(structindex_state*istate,+constchar*src,intfirst,constchar**submodule_gitfile){structstrbufsubmodule_dotgit=STRBUF_INIT;-if(!S_ISGITLINK(active_cache[first]->ce_mode))+if(!S_ISGITLINK(istate->cache[first]->ce_mode))die(_("Directory %s is in index and no submodule?"),src);-if(!is_staging_gitmodules_ok(&the_index))+if(!is_staging_gitmodules_ok(istate))die(_("Please stage your changes to .gitmodules or stash them to proceed"));strbuf_addf(&submodule_dotgit,"%s/.git",src);*submodule_gitfile=read_gitfile(submodule_dotgit.buf);
@@ -92,19 +92,20 @@ static void prepare_move_submodule(const char *src, int first,strbuf_release(&submodule_dotgit);}-staticintindex_range_of_same_dir(constchar*src,intlength,+staticintindex_range_of_same_dir(structindex_state*istate,+constchar*src,intlength,int*first_p,int*last_p){constchar*src_w_slash=add_slash(src);intfirst,last,len_w_slash=length+1;-first=cache_name_pos(src_w_slash,len_w_slash);+first=index_name_pos(istate,src_w_slash,len_w_slash);if(first>=0)die(_("%.*s is in index"),len_w_slash,src_w_slash);first=-1-first;-for(last=first;last<active_nr;last++){-constchar*path=active_cache[last]->name;+for(last=first;last<istate->cache_nr;last++){+constchar*path=istate->cache[last]->name;if(strncmp(path,src_w_slash,len_w_slash))break;}
@@ -221,7 +225,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)}argc+=last-first;}-}elseif(!(ce=cache_file_exists(src,length,ignore_case))){+}elseif(!(ce=index_file_exists(istate,src,length,ignore_case))){bad=_("not under version control");}elseif(ce_stage(ce)){bad=_("conflicted");
@@ -291,15 +295,15 @@ int cmd_mv(int argc, const char **argv, const char *prefix)if(mode==WORKING_DIRECTORY)continue;-pos=cache_name_pos(src,strlen(src));+pos=index_name_pos(istate,src,strlen(src));assert(pos>=0);-rename_cache_entry_at(pos,dst);+rename_index_entry_at(istate,pos,dst);}if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:11
From: Derrick Stolee <redacted>
The rm builtin still uses the antiquated compatibility macros for
interacting with the index. Update these to the more modern uses by
passing around a 'struct index_state' pointer.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/rm.c | 56 ++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 26 deletions(-)
@@ -284,24 +287,25 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!index_only)setup_work_tree();-hold_locked_index(&lock_file,LOCK_DIE_ON_ERROR);+repo_hold_locked_index(the_repository,&lock_file,LOCK_DIE_ON_ERROR);-if(read_cache()<0)+if(repo_read_index(the_repository)<0)die(_("index file corrupt"));-refresh_index(&the_index,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);+istate=the_repository->index;+refresh_index(istate,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);seen=xcalloc(pathspec.nr,1);-for(i=0;i<active_nr;i++){-conststructcache_entry*ce=active_cache[i];-if(!ce_path_match(&the_index,ce,&pathspec,seen))+for(i=0;i<istate->cache_nr;i++){+conststructcache_entry*ce=istate->cache[i];+if(!ce_path_match(istate,ce,&pathspec,seen))continue;ALLOC_GROW(list.entry,list.nr+1,list.alloc);list.entry[list.nr].name=xstrdup(ce->name);list.entry[list.nr].is_submodule=S_ISGITLINK(ce->ce_mode);if(list.entry[list.nr++].is_submodule&&-!is_staging_gitmodules_ok(&the_index))+!is_staging_gitmodules_ok(istate))die(_("please stage your changes to .gitmodules or stash them to proceed"));}
@@ -358,7 +362,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!quiet)printf("rm '%s'\n",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))die(_("git rm: unable to remove %s"),path);}
@@ -398,10 +402,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)}strbuf_release(&buf);if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);}-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:11
From: Derrick Stolee <redacted>
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
can then be mechanically replaced by instances that use the istate
pointer instead of the version that autocompletes to use the_index.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 59 +++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 27 deletions(-)
@@ -232,13 +235,13 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(&the_index,active_cache[pos]);+mark_fsmonitor_invalid(istate,active_cache[pos]);if(mark)active_cache[pos]->ce_flags|=flag;elseactive_cache[pos]->ce_flags&=~flag;active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;-cache_tree_invalidate_path(&the_index,path);+cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;}
@@ -277,14 +280,14 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenif(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))return0;-ce=make_empty_cache_entry(&the_index,len);+ce=make_empty_cache_entry(istate,len);memcpy(ce->name,path,len);ce->ce_flags=create_ce_flags(0);ce->ce_namelen=len;-fill_stat_cache_info(&the_index,ce,st);+fill_stat_cache_info(istate,ce,st);ce->ce_mode=ce_mode_from_stat(old,st->st_mode);-if(index_path(&the_index,&ce->oid,path,st,+if(index_path(istate,&ce->oid,path,st,info_only?0:HASH_WRITE_OBJECT)){discard_cache_entry(ce);return-1;
@@ -411,7 +414,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,returnerror("Invalid path '%s'",path);len=strlen(path);-ce=make_empty_cache_entry(&the_index,len);+ce=make_empty_cache_entry(istate,len);oidcpy(&ce->oid,oid);memcpy(ce->name,path,len);
@@ -603,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which,structobject_idoid;structcache_entry*ce;-if(get_tree_entry(the_repository,ent,path,&oid,&mode)){+if(get_tree_entry(repo,ent,path,&oid,&mode)){if(which)error("%s: not in %s branch.",path,which);returnNULL;
@@ -613,7 +616,7 @@ static struct cache_entry *read_one_ent(const char *which,error("%s: not a blob in %s branch.",path,which);returnNULL;}-ce=make_empty_cache_entry(&the_index,namelen);+ce=make_empty_cache_entry(istate,namelen);oidcpy(&ce->oid,&oid);memcpy(ce->name,path,namelen);
@@ -968,7 +971,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;-structrepository*r=the_repository;structoptionoptions[]={OPT_BIT('q',NULL,&refresh_args.flags,N_("continue refresh even when index needs update"),
@@ -1077,16 +1079,19 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);+repo=the_repository;+/* we will diagnose later if it turns out that we need to update it */-newfd=hold_locked_index(&lock_file,0);+newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)lock_error=errno;-entries=read_cache();+entries=repo_read_index(repo);if(entries<0)die("cache corrupted");-the_index.updated_skipworktree=1;+istate=repo->index;+istate->updated_skipworktree=1;/**Customcopyofparse_options()becausewewanttohandle
@@ -1173,28 +1178,28 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.splitIndex is set to false; ""remove or change it, if you really want to ""enable split index"));-if(the_index.split_index)-the_index.cache_changed|=SPLIT_INDEX_ORDERED;+if(istate->split_index)+istate->cache_changed|=SPLIT_INDEX_ORDERED;else-add_split_index(&the_index);+add_split_index(istate);}elseif(!split_index){if(git_config_get_split_index()==1)warning(_("core.splitIndex is set to true; ""remove or change it, if you really want to ""disable split index"));-remove_split_index(&the_index);+remove_split_index(istate);}-prepare_repo_settings(r);+prepare_repo_settings(repo);switch(untracked_cache){caseUC_UNSPECIFIED:break;caseUC_DISABLE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)warning(_("core.untrackedCache is set to true; ""remove or change it, if you really want to ""disable the untracked cache"));-remove_untracked_cache(&the_index);+remove_untracked_cache(istate);report(_("Untracked cache disabled"));break;caseUC_TEST:
@@ -1202,11 +1207,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)return!test_if_untracked_cache_is_supported();caseUC_ENABLE:caseUC_FORCE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)warning(_("core.untrackedCache is set to false; ""remove or change it, if you really want to ""enable the untracked cache"));-add_untracked_cache(&the_index);+add_untracked_cache(istate);report(_("Untracked cache enabled for '%s'"),get_git_work_tree());break;default:
@@ -1218,14 +1223,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.fsmonitor is unset; ""set it if you really want to ""enable fsmonitor"));-add_fsmonitor(&the_index);+add_fsmonitor(istate);report(_("fsmonitor enabled"));}elseif(!fsmonitor){if(git_config_get_fsmonitor()==1)warning(_("core.fsmonitor is set; ""remove it if you really want to ""disable fsmonitor"));-remove_fsmonitor(&the_index);+remove_fsmonitor(istate);report(_("fsmonitor disabled"));}
@@ -1235,7 +1240,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)exit(128);unable_to_lock_die(get_index_file(),lock_error);}-if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))+if(write_locked_index(istate,&lock_file,COMMIT_LOCK))die("Unable to write new index file");}
@@ -242,7 +242,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)istate->cache[pos]->ce_flags&=~flag;istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);-active_cache_changed|=CE_ENTRY_CHANGED;+istate->cache_changed|=CE_ENTRY_CHANGED;return0;}return-1;
@@ -346,7 +346,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;-while(pos<active_nr){+while(pos<istate->cache_nr){conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))
@@ -766,14 +766,14 @@ static int do_reupdate(int ac, const char **av,}/* Be careful. The working tree may not have the*pathanymore,inwhichcase,under'allow_remove',-*orworseyet'allow_replace',active_nrmaydecrease.+*orworseyet'allow_replace',istate->cache_nrmaydecrease.*/-save_nr=active_nr;+save_nr=istate->cache_nr;path=xstrdup(ce->name);update_one(path);free(path);discard_cache_entry(old);-if(save_nr!=active_nr)+if(save_nr!=istate->cache_nr)gotoredo;}clear_pathspec(&pathspec);
@@ -294,7 +294,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len}option=allow_add?ADD_CACHE_OK_TO_ADD:0;option|=allow_replace?ADD_CACHE_OK_TO_REPLACE:0;-if(add_cache_entry(ce,option)){+if(add_index_entry(istate,ce,option)){discard_cache_entry(ce);returnerror("%s: cannot add to the index - missing --add option?",path);}
@@ -425,7 +425,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,ce->ce_flags|=CE_VALID;option=allow_add?ADD_CACHE_OK_TO_ADD:0;option|=allow_replace?ADD_CACHE_OK_TO_REPLACE:0;-if(add_cache_entry(ce,option))+if(add_index_entry(istate,ce,option))returnerror("%s: cannot add to the index - missing --add option?",path);report("add '%s'",path);
@@ -683,12 +683,12 @@ static int unresolve_one(const char *path)}remove_file_from_index(istate,path);-if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){+if(add_index_entry(istate,ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;gotofree_return;}-if(!add_cache_entry(ce_3,ADD_CACHE_OK_TO_ADD))+if(!add_index_entry(istate,ce_3,ADD_CACHE_OK_TO_ADD))return0;error("%s: cannot add their version to the index.",path);ret=-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:51
From: Derrick Stolee <redacted>
This is also the last usage of unmerge_cache_entry_at(), so it can be
removed from cache.h.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 11 ++++++-----
cache.h | 1 -
2 files changed, 6 insertions(+), 6 deletions(-)
@@ -252,7 +252,7 @@ static int remove_one_path(const char *path){if(!allow_remove)returnerror("%s: does not exist and --remove not passed",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -386,7 +386,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)*Ontheotherhand,removingitfromindexshouldwork*/if(!ignore_skip_worktree_entries&&allow_remove&&-remove_file_from_cache(path))+remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -567,7 +567,7 @@ static void read_index_info(int nul_term_line)if(!mode){/* mode == 0 means there is no such path -- remove */-if(remove_file_from_cache(path_name))+if(remove_file_from_index(istate,path_name))die("git update-index: unable to remove %s",ptr);}
@@ -682,7 +682,7 @@ static int unresolve_one(const char *path)gotofree_return;}-remove_file_from_cache(path);+remove_file_from_index(istate,path);if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;
@@ -233,7 +233,7 @@ static int test_if_untracked_cache_is_supported(void)staticintmark_ce_flags(constchar*path,intflag,intmark){intnamelen=strlen(path);-intpos=cache_name_pos(path,namelen);+intpos=index_name_pos(istate,path,namelen);if(0<=pos){mark_fsmonitor_invalid(istate,istate->cache[pos]);if(mark)
@@ -327,7 +327,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstaticintprocess_directory(constchar*path,intlen,structstat*st){structobject_idoid;-intpos=cache_name_pos(path,len);+intpos=index_name_pos(istate,path,len);/* Exact match: file or existing gitlink */if(pos>=0){
@@ -377,7 +377,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)if(has_symlink_leading_path(path,len))returnerror("'%s' is beyond a symbolic link",path);-pos=cache_name_pos(path,len);+pos=index_name_pos(istate,path,len);ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/*
@@ -634,7 +634,7 @@ static int unresolve_one(const char *path)structcache_entry*ce_2=NULL,*ce_3=NULL;/* See if there is such entry in the index. */-pos=cache_name_pos(path,namelen);+pos=index_name_pos(istate,path,namelen);if(0<=pos){/* already merged */pos=unmerge_cache_entry_at(pos);
@@ -235,12 +235,12 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(istate,active_cache[pos]);+mark_fsmonitor_invalid(istate,istate->cache[pos]);if(mark)-active_cache[pos]->ce_flags|=flag;+istate->cache[pos]->ce_flags|=flag;else-active_cache[pos]->ce_flags&=~flag;-active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;+istate->cache[pos]->ce_flags&=~flag;+istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;
@@ -331,7 +331,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Exact match: file or existing gitlink */if(pos>=0){-conststructcache_entry*ce=active_cache[pos];+conststructcache_entry*ce=istate->cache[pos];if(S_ISGITLINK(ce->ce_mode)){/* Do nothing to the index if there is no HEAD! */
@@ -347,7 +347,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;while(pos<active_nr){-conststructcache_entry*ce=active_cache[pos++];+conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))break;
@@ -378,7 +378,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)returnerror("'%s' is beyond a symbolic link",path);pos=cache_name_pos(path,len);-ce=pos<0?NULL:active_cache[pos];+ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/**workingdirectoryversionisassumed"good"
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-01 13:08:51
From: Derrick Stolee <redacted>
The final index compatibility macro to remove from the update-index
builtin is ce_match_state(). Further, this is the last use of that macro
anywhere, so it should be removed.
There are some remaining references in the racy-git.txt technical
document that are updated to ie_match_stat().
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/racy-git.txt | 6 +++---
builtin/update-index.c | 3 +--
cache.h | 1 -
3 files changed, 4 insertions(+), 6 deletions(-)
@@ -26,7 +26,7 @@ information obtained from the filesystem via `lstat(2)` system call when they were last updated. When checking if they differ, Git first runs `lstat(2)` on the files and compares the result with this information (this is what was originally done by the-`ce_match_stat()` function, but the current code does it in+`ie_match_stat()` function, but the current code does it in `ce_match_stat_basic()` function). If some of these "cached stat information" fields do not match, Git can tell that the files are modified without even looking at their contents.
@@ -102,7 +102,7 @@ timestamp as the index file itself. The callers that want to check if an index entry matches the corresponding file in the working tree continue to call-`ce_match_stat()`, but with this change, `ce_match_stat()` uses+`ie_match_stat()`, but with this change, `ie_match_stat()` uses `ce_modified_check_fs()` to see if racily clean ones are actually clean after comparing the cached stat information using `ce_match_stat_basic()`.
@@ -128,7 +128,7 @@ Runtime penalty --------------- The runtime penalty of falling back to `ce_modified_check_fs()`-from `ce_match_stat()` can be very expensive when there are many+from `ie_match_stat()` can be very expensive when there are many racily clean entries. An obvious way to artificially create this situation is to give the same timestamp to all the files in the working tree in a large project, run `git update-index` on
@@ -277,7 +276,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstructcache_entry*ce;/* Was the old index entry already up-to-date? */-if(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))+if(old&&!ce_stage(old)&&!ie_match_stat(istate,old,st,0))return0;ce=make_empty_cache_entry(istate,len);
On Fri, Jan 1, 2021 at 5:10 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
From: Derrick Stolee <redacted>
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
can then be mechanically replaced by instances that use the istate
pointer instead of the version that autocompletes to use the_index.
autocompletes seems a bit weird to me here. Perhaps s/autocompletes
to use/implicitly uses/ ?
Also, it seems like in the last few patches you just used
the_repository whereas here you're trying to avoid it. Is that
because there are more uses here and only one in the other patches?
Otherwise, all the changes in this patch (and the other ones I've read
so far; going through them in order) seem like the obvious mechanical
changes necessary to update to avoid the index compatibility macros.
So, looking good so far.
@@ -232,13 +235,13 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(&the_index,active_cache[pos]);+mark_fsmonitor_invalid(istate,active_cache[pos]);if(mark)active_cache[pos]->ce_flags|=flag;elseactive_cache[pos]->ce_flags&=~flag;active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;-cache_tree_invalidate_path(&the_index,path);+cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;}
@@ -277,14 +280,14 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenif(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))return0;-ce=make_empty_cache_entry(&the_index,len);+ce=make_empty_cache_entry(istate,len);memcpy(ce->name,path,len);ce->ce_flags=create_ce_flags(0);ce->ce_namelen=len;-fill_stat_cache_info(&the_index,ce,st);+fill_stat_cache_info(istate,ce,st);ce->ce_mode=ce_mode_from_stat(old,st->st_mode);-if(index_path(&the_index,&ce->oid,path,st,+if(index_path(istate,&ce->oid,path,st,info_only?0:HASH_WRITE_OBJECT)){discard_cache_entry(ce);return-1;
@@ -411,7 +414,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,returnerror("Invalid path '%s'",path);len=strlen(path);-ce=make_empty_cache_entry(&the_index,len);+ce=make_empty_cache_entry(istate,len);oidcpy(&ce->oid,oid);memcpy(ce->name,path,len);
@@ -603,7 +606,7 @@ static struct cache_entry *read_one_ent(const char *which,structobject_idoid;structcache_entry*ce;-if(get_tree_entry(the_repository,ent,path,&oid,&mode)){+if(get_tree_entry(repo,ent,path,&oid,&mode)){if(which)error("%s: not in %s branch.",path,which);returnNULL;
@@ -613,7 +616,7 @@ static struct cache_entry *read_one_ent(const char *which,error("%s: not a blob in %s branch.",path,which);returnNULL;}-ce=make_empty_cache_entry(&the_index,namelen);+ce=make_empty_cache_entry(istate,namelen);oidcpy(&ce->oid,&oid);memcpy(ce->name,path,namelen);
@@ -968,7 +971,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;-structrepository*r=the_repository;structoptionoptions[]={OPT_BIT('q',NULL,&refresh_args.flags,N_("continue refresh even when index needs update"),
@@ -1077,16 +1079,19 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);+repo=the_repository;+/* we will diagnose later if it turns out that we need to update it */-newfd=hold_locked_index(&lock_file,0);+newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)lock_error=errno;-entries=read_cache();+entries=repo_read_index(repo);if(entries<0)die("cache corrupted");-the_index.updated_skipworktree=1;+istate=repo->index;+istate->updated_skipworktree=1;/**Customcopyofparse_options()becausewewanttohandle
@@ -1173,28 +1178,28 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.splitIndex is set to false; ""remove or change it, if you really want to ""enable split index"));-if(the_index.split_index)-the_index.cache_changed|=SPLIT_INDEX_ORDERED;+if(istate->split_index)+istate->cache_changed|=SPLIT_INDEX_ORDERED;else-add_split_index(&the_index);+add_split_index(istate);}elseif(!split_index){if(git_config_get_split_index()==1)warning(_("core.splitIndex is set to true; ""remove or change it, if you really want to ""disable split index"));-remove_split_index(&the_index);+remove_split_index(istate);}-prepare_repo_settings(r);+prepare_repo_settings(repo);switch(untracked_cache){caseUC_UNSPECIFIED:break;caseUC_DISABLE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)warning(_("core.untrackedCache is set to true; ""remove or change it, if you really want to ""disable the untracked cache"));-remove_untracked_cache(&the_index);+remove_untracked_cache(istate);report(_("Untracked cache disabled"));break;caseUC_TEST:
@@ -1202,11 +1207,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)return!test_if_untracked_cache_is_supported();caseUC_ENABLE:caseUC_FORCE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)warning(_("core.untrackedCache is set to false; ""remove or change it, if you really want to ""enable the untracked cache"));-add_untracked_cache(&the_index);+add_untracked_cache(istate);report(_("Untracked cache enabled for '%s'"),get_git_work_tree());break;default:
@@ -1218,14 +1223,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.fsmonitor is unset; ""set it if you really want to ""enable fsmonitor"));-add_fsmonitor(&the_index);+add_fsmonitor(istate);report(_("fsmonitor enabled"));}elseif(!fsmonitor){if(git_config_get_fsmonitor()==1)warning(_("core.fsmonitor is set; ""remove it if you really want to ""disable fsmonitor"));-remove_fsmonitor(&the_index);+remove_fsmonitor(istate);report(_("fsmonitor disabled"));}
@@ -1235,7 +1240,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)exit(128);unable_to_lock_die(get_index_file(),lock_error);}-if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))+if(write_locked_index(istate,&lock_file,COMMIT_LOCK))die("Unable to write new index file");}--
On Fri, Jan 1, 2021 at 5:12 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
From: Derrick Stolee <redacted>
The final index compatibility macro to remove from the update-index
builtin is ce_match_state(). Further, this is the last use of that macro
I think you mean ce_match_stat(); no trailing 'e'.
quoted hunk
anywhere, so it should be removed.
There are some remaining references in the racy-git.txt technical
document that are updated to ie_match_stat().
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/racy-git.txt | 6 +++---
builtin/update-index.c | 3 +--
cache.h | 1 -
3 files changed, 4 insertions(+), 6 deletions(-)
@@ -26,7 +26,7 @@ information obtained from the filesystem via `lstat(2)` system call when they were last updated. When checking if they differ, Git first runs `lstat(2)` on the files and compares the result with this information (this is what was originally done by the-`ce_match_stat()` function, but the current code does it in+`ie_match_stat()` function, but the current code does it in
Even updating the documentation... :-)
Arguably, this change should have been done years ago when
ce_match_stat was turned into a macro, but certainly becomes more
important with your patch that removes the macro.
quoted hunk
`ce_match_stat_basic()` function). If some of these "cached
stat information" fields do not match, Git can tell that the
files are modified without even looking at their contents.
@@ -102,7 +102,7 @@ timestamp as the index file itself. The callers that want to check if an index entry matches the corresponding file in the working tree continue to call-`ce_match_stat()`, but with this change, `ce_match_stat()` uses+`ie_match_stat()`, but with this change, `ie_match_stat()` uses `ce_modified_check_fs()` to see if racily clean ones are actually clean after comparing the cached stat information using `ce_match_stat_basic()`.
@@ -128,7 +128,7 @@ Runtime penalty --------------- The runtime penalty of falling back to `ce_modified_check_fs()`-from `ce_match_stat()` can be very expensive when there are many+from `ie_match_stat()` can be very expensive when there are many racily clean entries. An obvious way to artificially create this situation is to give the same timestamp to all the files in the working tree in a large project, run `git update-index` on
@@ -277,7 +276,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstructcache_entry*ce;/* Was the old index entry already up-to-date? */-if(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))+if(old&&!ce_stage(old)&&!ie_match_stat(istate,old,st,0))return0;ce=make_empty_cache_entry(istate,len);
On Fri, Jan 1, 2021 at 5:10 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
I noticed that Duy's project around USE_THE_INDEX_COMPATIBILITY_MACROS has
been on pause for a while. Here is my attempt to continue that project a
little.
I started going through the builtins that still use cache_name_pos() and the
first few were easy: merge-inex, mv, rm.
Then I hit update-index and it was a bit bigger. It's included here as well.
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
I had started trying to keep everything local to the method signatures, but
I hit a snag when reaching the command-line parsing callbacks, which I could
not modify their call signature. At that point, I had something that was
already much more complicated than what I present now. Outside of the first
update-index commit, everything was a mechanical find/replace.
In total, this allows us to remove four of the compatibility macros because
they are no longer used.
This series is divided nicely in a way that makes review easy. I've
made some of these same types of changes elsewhere, and the whole
series is really just a long sequence of mechanical changes (plus a
case or two of fixing formatting on a line you were changing anyway,
such as adding spaces around an operator). I think the work to
continue dropping the implicit dependency on the_index is helpful.
I only found two minor suggestions for improving the commit messages;
the patches all look good to me.
From: Eric Sunshine <hidden> Date: 2021-01-02 06:13:19
On Fri, Jan 1, 2021 at 8:09 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
I had started trying to keep everything local to the method signatures, but
I hit a snag when reaching the command-line parsing callbacks, which I could
not modify their call signature. [...]
You should be able to do this, not by modifying the callback
signature, but by taking advantage of the `extra` member of `struct
option` which is available to callback functions or arbitrary use. If
you need to access the index in a callback, then assign a `struct
index_state *` to `extra`; likewise assign a `struct repository *` to
`extra` to access the repository. If you need access to both the index
and the repository, then just store the repository in `extra` since
the repository has an `index` field.
You won't be able to use any of the canned OPT_FOO() macros to
initialize an entry in the update-index.c `options[]` array which
needs `extra`-initialization since the macros don't let you specify
`extra`, but you can easily bypass the macro and initialize the
`struct option` manually. (After all, the macros exist for
convenience; they are not a hard requirement.)
Within the callback, extract the `repository` or `index_state` as you
would any other field. For instance:
const struct repository *repo = opt->extra;
This should allow you to get rid of the globals introduced by patch
[4/12] (assuming passing the index and repo arguments around
everywhere doesn't get overly hairy).
From: Alban Gruin <hidden> Date: 2021-01-03 23:32:38
Hi Derrick,
Le 01/01/2021 à 14:06, Derrick Stolee via GitGitGadget a écrit :
From: Derrick Stolee <redacted>
Replace uses of the old macros for the_index and instead pass around a
'struct index_state' pointer. This allows dropping the compatibility
flag.
Signed-off-by: Derrick Stolee <redacted>
I already libified builtin/merge-index.c in ag/merge-strategies-in-c,
and such dropped the_index. I modified merge_entry(), merge_one_path()
and merge_all() to take a callback, itself taking a repository. As
such, in my series, these functions take a `struct repository *' instead
of an index state.
I'm not sure how we should proceed with our respective patches.
Cheers,
Alban
On Fri, Jan 1, 2021 at 5:10 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
quoted
From: Derrick Stolee <redacted>
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
can then be mechanically replaced by instances that use the istate
pointer instead of the version that autocompletes to use the_index.
autocompletes seems a bit weird to me here. Perhaps s/autocompletes
to use/implicitly uses/ ?
My intention was "instead of the macro expansion that uses the_index".
The preprocessor is really just an early version of autocomplete, right?
Thanks.
Also, it seems like in the last few patches you just used
the_repository whereas here you're trying to avoid it. Is that
because there are more uses here and only one in the other patches?
My goal isn't to remove the_repository, but the earlier patches also
avoided static globals in favor of method parameters. I needed to
change the strategy for update-index because of the vast number of
methods needing an update. Since I was making a static global for
the current index, it was not a huge step to also add one for the
current repository.
Further, the cmd_update_index() already had a local pointer that
replaced using the_repository, giving me some reason to include
the_repository in these updates.
Otherwise, all the changes in this patch (and the other ones I've read
so far; going through them in order) seem like the obvious mechanical
changes necessary to update to avoid the index compatibility macros.
So, looking good so far.
On Fri, Jan 1, 2021 at 8:09 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
quoted
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
I had started trying to keep everything local to the method signatures, but
I hit a snag when reaching the command-line parsing callbacks, which I could
not modify their call signature. [...]
You should be able to do this, not by modifying the callback
signature, but by taking advantage of the `extra` member of `struct
option` which is available to callback functions or arbitrary use. If
you need to access the index in a callback, then assign a `struct
index_state *` to `extra`; likewise assign a `struct repository *` to
`extra` to access the repository. If you need access to both the index
and the repository, then just store the repository in `extra` since
the repository has an `index` field.
You won't be able to use any of the canned OPT_FOO() macros to
initialize an entry in the update-index.c `options[]` array which
needs `extra`-initialization since the macros don't let you specify
`extra`, but you can easily bypass the macro and initialize the
`struct option` manually. (After all, the macros exist for
convenience; they are not a hard requirement.)
Within the callback, extract the `repository` or `index_state` as you
would any other field. For instance:
const struct repository *repo = opt->extra;
Yes, this is definitely the way to make it possible.
This should allow you to get rid of the globals introduced by patch
[4/12] (assuming passing the index and repo arguments around
everywhere doesn't get overly hairy).
My attempts just getting to the point of hitting these callbacks was
already making me frustrated with how complicated the code became with
that approach.
Perhaps now that I've removed the compatibility macros, it would be
easier to insert the method parameters since most of the lines that
need to change would be method prototypes and the calls to those methods
(plus the callback function details).
Is that a valuable effort? I could give it a try, but I want to be sure
that adjusting all of those helper methods in the builtin would indeed
have valuable improvements over the static globals used here.
Thanks,
-Stolee
From: Eric Sunshine <hidden> Date: 2021-01-04 06:24:01
On Sun, Jan 3, 2021 at 8:01 PM Derrick Stolee [off-list ref] wrote:
On 1/2/2021 1:12 AM, Eric Sunshine wrote:
quoted
This should allow you to get rid of the globals introduced by patch
[4/12] (assuming passing the index and repo arguments around
everywhere doesn't get overly hairy).
Perhaps now that I've removed the compatibility macros, it would be
easier to insert the method parameters since most of the lines that
need to change would be method prototypes and the calls to those methods
(plus the callback function details).
Is that a valuable effort? I could give it a try, but I want to be sure
that adjusting all of those helper methods in the builtin would indeed
have valuable improvements over the static globals used here.
My impression was that the goal of the earlier work was to pass the
index and repository to each function specifically to avoid tying the
function to a particular index or repository. This helps in cases in
which client code needs to operate on a different index or repository
(for instance, a submodule). Generally speaking, making the index and
repository file-static rather than global does not help reach that
goal since functions are still tied to state which is not local to the
function itself.
Would the extra effort be valuable in this particular case? I'm not
familiar with this code, but given that `update-index` is a builtin,
such effort may not be too meaningful. If, however, any of the code
from `buildin/update-index.c` ever gets "libified" and moved into the
core library, then that would be a good time to update the functions
to take those values as arguments rather than relying on file-static
or globals. But that's not something that this series necessarily
needs to do; the task can wait until the code needs to be shared by
other modules, I would think.
Hi Derrick,
Le 01/01/2021 à 14:06, Derrick Stolee via GitGitGadget a écrit :
quoted
From: Derrick Stolee <redacted>
Replace uses of the old macros for the_index and instead pass around a
'struct index_state' pointer. This allows dropping the compatibility
flag.
Signed-off-by: Derrick Stolee <redacted>
I already libified builtin/merge-index.c in ag/merge-strategies-in-c,
and such dropped the_index. I modified merge_entry(), merge_one_path()
and merge_all() to take a callback, itself taking a repository. As
such, in my series, these functions take a `struct repository *' instead
of an index state.
I'm not sure how we should proceed with our respective patches.
Hi Alban,
Sorry I didn't realize that. I'll drop this patch. Thanks for letting
me know!
Thanks,
-Stolee
On Sun, Jan 3, 2021 at 8:01 PM Derrick Stolee [off-list ref] wrote:
quoted
On 1/2/2021 1:12 AM, Eric Sunshine wrote:
quoted
This should allow you to get rid of the globals introduced by patch
[4/12] (assuming passing the index and repo arguments around
everywhere doesn't get overly hairy).
Perhaps now that I've removed the compatibility macros, it would be
easier to insert the method parameters since most of the lines that
need to change would be method prototypes and the calls to those methods
(plus the callback function details).
Is that a valuable effort? I could give it a try, but I want to be sure
that adjusting all of those helper methods in the builtin would indeed
have valuable improvements over the static globals used here.
My impression was that the goal of the earlier work was to pass the
index and repository to each function specifically to avoid tying the
function to a particular index or repository. This helps in cases in
which client code needs to operate on a different index or repository
(for instance, a submodule). Generally speaking, making the index and
repository file-static rather than global does not help reach that
goal since functions are still tied to state which is not local to the
function itself.
Would the extra effort be valuable in this particular case? I'm not
familiar with this code, but given that `update-index` is a builtin,
such effort may not be too meaningful. If, however, any of the code
from `buildin/update-index.c` ever gets "libified" and moved into the
core library, then that would be a good time to update the functions
to take those values as arguments rather than relying on file-static
or globals. But that's not something that this series necessarily
needs to do; the task can wait until the code needs to be shared by
other modules, I would think.
I tried again tonight, and it started getting messy, but then I
realized that I could group the callbacks that need the repo and
index to use a common struct that holds the other parameters they
need. It's still a bigger patch than I'd like, but it is more
reasonable.
v2 is incoming with my attempt at this.
Thanks,
-Stolee
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:06
From: Derrick Stolee <redacted>
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
that expand to use the_index can then be mechanically replaced by
references to the istate pointer.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 59 +++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 27 deletions(-)
@@ -232,13 +235,13 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(&the_index,active_cache[pos]);+mark_fsmonitor_invalid(istate,active_cache[pos]);if(mark)active_cache[pos]->ce_flags|=flag;elseactive_cache[pos]->ce_flags&=~flag;active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;-cache_tree_invalidate_path(&the_index,path);+cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;}
@@ -277,14 +280,14 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenif(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))return0;-ce=make_empty_cache_entry(&the_index,len);+ce=make_empty_cache_entry(istate,len);memcpy(ce->name,path,len);ce->ce_flags=create_ce_flags(0);ce->ce_namelen=len;-fill_stat_cache_info(&the_index,ce,st);+fill_stat_cache_info(istate,ce,st);ce->ce_mode=ce_mode_from_stat(old,st->st_mode);-if(index_path(&the_index,&ce->oid,path,st,+if(index_path(istate,&ce->oid,path,st,info_only?0:HASH_WRITE_OBJECT)){discard_cache_entry(ce);return-1;
@@ -406,7 +409,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,{intres;-res=add_to_index_cacheinfo(&the_index,mode,oid,path,stage,+res=add_to_index_cacheinfo(istate,mode,oid,path,stage,allow_add,allow_replace,NULL);if(res==-1)returnres;
@@ -592,7 +595,7 @@ static struct cache_entry *read_one_ent(const char *which,structobject_idoid;structcache_entry*ce;-if(get_tree_entry(the_repository,ent,path,&oid,&mode)){+if(get_tree_entry(repo,ent,path,&oid,&mode)){if(which)error("%s: not in %s branch.",path,which);returnNULL;
@@ -602,7 +605,7 @@ static struct cache_entry *read_one_ent(const char *which,error("%s: not a blob in %s branch.",path,which);returnNULL;}-ce=make_empty_cache_entry(&the_index,namelen);+ce=make_empty_cache_entry(istate,namelen);oidcpy(&ce->oid,&oid);memcpy(ce->name,path,namelen);
@@ -957,7 +960,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;-structrepository*r=the_repository;structoptionoptions[]={OPT_BIT('q',NULL,&refresh_args.flags,N_("continue refresh even when index needs update"),
@@ -1066,16 +1068,19 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);+repo=the_repository;+/* we will diagnose later if it turns out that we need to update it */-newfd=hold_locked_index(&lock_file,0);+newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)lock_error=errno;-entries=read_cache();+entries=repo_read_index(repo);if(entries<0)die("cache corrupted");-the_index.updated_skipworktree=1;+istate=repo->index;+istate->updated_skipworktree=1;/**Customcopyofparse_options()becausewewanttohandle
@@ -1162,28 +1167,28 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.splitIndex is set to false; ""remove or change it, if you really want to ""enable split index"));-if(the_index.split_index)-the_index.cache_changed|=SPLIT_INDEX_ORDERED;+if(istate->split_index)+istate->cache_changed|=SPLIT_INDEX_ORDERED;else-add_split_index(&the_index);+add_split_index(istate);}elseif(!split_index){if(git_config_get_split_index()==1)warning(_("core.splitIndex is set to true; ""remove or change it, if you really want to ""disable split index"));-remove_split_index(&the_index);+remove_split_index(istate);}-prepare_repo_settings(r);+prepare_repo_settings(repo);switch(untracked_cache){caseUC_UNSPECIFIED:break;caseUC_DISABLE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)warning(_("core.untrackedCache is set to true; ""remove or change it, if you really want to ""disable the untracked cache"));-remove_untracked_cache(&the_index);+remove_untracked_cache(istate);report(_("Untracked cache disabled"));break;caseUC_TEST:
@@ -1191,11 +1196,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)return!test_if_untracked_cache_is_supported();caseUC_ENABLE:caseUC_FORCE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)warning(_("core.untrackedCache is set to false; ""remove or change it, if you really want to ""enable the untracked cache"));-add_untracked_cache(&the_index);+add_untracked_cache(istate);report(_("Untracked cache enabled for '%s'"),get_git_work_tree());break;default:
@@ -1207,14 +1212,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.fsmonitor is unset; ""set it if you really want to ""enable fsmonitor"));-add_fsmonitor(&the_index);+add_fsmonitor(istate);report(_("fsmonitor enabled"));}elseif(!fsmonitor){if(git_config_get_fsmonitor()==1)warning(_("core.fsmonitor is set; ""remove it if you really want to ""disable fsmonitor"));-remove_fsmonitor(&the_index);+remove_fsmonitor(istate);report(_("fsmonitor disabled"));}
@@ -1224,7 +1229,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)exit(128);unable_to_lock_die(get_index_file(),lock_error);}-if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))+if(write_locked_index(istate,&lock_file,COMMIT_LOCK))die("Unable to write new index file");}
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:06
UPDATE: this is now based on ag/merge-strategies-in-c to avoid conflicts in
'seen'. The changes in builtin/rm.c still conflict with
mt/rm-sparse-checkout, but that branch seems to be waiting for a clearer
plan on some corner cases. I thought about ejecting it, but 'rm' still uses
ce_match_stat(), so just dropping the patch gives less of a final stake at
the end of the series. (I'm still open to it, if necessary.)
I noticed that Duy's project around USE_THE_INDEX_COMPATIBILITY_MACROS has
been on pause for a while. Here is my attempt to continue that project a
little.
I started going through the builtins that still use cache_name_pos() and the
first was easy: mv and rm.
Then I hit update-index and it was a bit bigger.
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file. Then, these static globals were also removed by
systematically updating the local method prototypes, plus some fancy
structure stuff for the option parsing callbacks.
I had started trying to keep everything local to the method signatures, but
I hit a snag when reaching the command-line parsing callbacks, which I could
not modify their call signature. At that point, I had something that was
already much more complicated than what I present now. Outside of the first
update-index commit, everything was a mechanical find/replace.
In total, this allows us to remove four of the compatibility macros because
they are no longer used.
Updates in V2
=============
* newly based on ag/merge-strategies-in-c, as there were some interesting
conflicts in buitin/update-index.c.
* Patch to update builtin/merge-index.c was dropped as that is already
handled in ag/merge-strategies-in-c
* I added patches that remove the static globals that I injected to make
the compatibility macros easy to delete. I do this in three parts.
* Commit messages improved.
Thanks, -Stolee
Derrick Stolee (14):
mv: remove index compatibility macros
rm: remove compatilibity macros
update-index: drop the_index, the_repository
update-index: use istate->cache over active_cache
update-index: use index->cache_nr over active_nr
update-index: use istate->cache_changed
update-index: use index_name_pos() over cache_name_pos()
update-index: use remove_file_from_index()
update-index: use add_index_entry()
update-index: replace several compatibility macros
update-index: remove ce_match_stat(), all macros
update-index: reduce static globals, part 1
update-index: reduce static globals, part 2
update-index: remove static globals from callbacks
Documentation/technical/racy-git.txt | 6 +-
builtin/mv.c | 42 ++--
builtin/rm.c | 56 ++---
builtin/update-index.c | 320 +++++++++++++++------------
cache.h | 4 -
5 files changed, 233 insertions(+), 195 deletions(-)
base-commit: 3da8920d38a007157ccf8e53382e5206b909dafe
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-830%2Fderrickstolee%2Findex-compatibility-1-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-830/derrickstolee/index-compatibility-1-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/830
Range-diff vs v1:
2: 84330533d4f = 1: 5ccc464cf26 mv: remove index compatibility macros
3: 54e167d5872 = 2: e715c703cb8 rm: remove compatilibity macros
4: 77f6510bb68 ! 3: 4bf3c582f9d update-index: drop the_index, the_repository
@@ Commit message
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
- can then be mechanically replaced by instances that use the istate
- pointer instead of the version that autocompletes to use the_index.
+ that expand to use the_index can then be mechanically replaced by
+ references to the istate pointer.
Signed-off-by: Derrick Stolee [off-list ref]
@@ builtin/update-index.c: static int add_one_path(const struct cache_entry *old, c
discard_cache_entry(ce);
return -1;
@@ builtin/update-index.c: static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
- return error("Invalid path '%s'", path);
-
- len = strlen(path);
-- ce = make_empty_cache_entry(&the_index, len);
-+ ce = make_empty_cache_entry(istate, len);
-
- oidcpy(&ce->oid, oid);
- memcpy(ce->name, path, len);
+ {
+ int res;
+
+- res = add_to_index_cacheinfo(&the_index, mode, oid, path, stage,
++ res = add_to_index_cacheinfo(istate, mode, oid, path, stage,
+ allow_add, allow_replace, NULL);
+ if (res == -1)
+ return res;
@@ builtin/update-index.c: static struct cache_entry *read_one_ent(const char *which,
struct object_id oid;
struct cache_entry *ce;
5: cc5df3566df = 4: 4b509ba5fa2 update-index: use istate->cache over active_cache
6: e99b8bddb3a = 5: 6c0e019f91c update-index: use index->cache_nr over active_nr
7: f841500c663 = 6: 5091e2661d1 update-index: use istate->cache_changed
8: d2af7e21ca1 = 7: 5b14fa10a4b update-index: use index_name_pos() over cache_name_pos()
9: c576e2d9676 = 8: a1a9fb01b07 update-index: use remove_file_from_index()
10: cf091799cae ! 9: 620e300ad6b update-index: use add_index_entry()
@@ builtin/update-index.c: static int add_one_path(const struct cache_entry *old, c
discard_cache_entry(ce);
return error("%s: cannot add to the index - missing --add option?", path);
}
-@@ builtin/update-index.c: static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
- ce->ce_flags |= CE_VALID;
- option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;
- option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;
-- if (add_cache_entry(ce, option))
-+ if (add_index_entry(istate, ce, option))
- return error("%s: cannot add to the index - missing --add option?",
- path);
- report("add '%s'", path);
@@ builtin/update-index.c: static int unresolve_one(const char *path)
}
11: d7856e2e772 = 10: 68b26a11d77 update-index: replace several compatibility macros
12: 8fb307c3bee ! 11: f1cffe2b455 update-index: remove ce_match_stat(), all macros
@@ Commit message
update-index: remove ce_match_stat(), all macros
The final index compatibility macro to remove from the update-index
- builtin is ce_match_state(). Further, this is the last use of that macro
+ builtin is ce_match_stat(). Further, this is the last use of that macro
anywhere, so it should be removed.
There are some remaining references in the racy-git.txt technical
-: ----------- > 12: 79e267f39ec update-index: reduce static globals, part 1
1: 68d88b651c7 ! 13: 457402b4fdc merge-index: drop index compatibility macros
@@ Metadata
Author: Derrick Stolee [off-list ref]
## Commit message ##
- merge-index: drop index compatibility macros
+ update-index: reduce static globals, part 2
- Replace uses of the old macros for the_index and instead pass around a
- 'struct index_state' pointer. This allows dropping the compatibility
- flag.
+ In order to remove index compatibility macros cleanly, we relied upon
+ static globals 'repo' and 'istate' to be pointers to the_repository and
+ the_index, respectively. We can continue reducing the need for these
+ static globals by modifying method prototypes to use them when
+ necessary.
+
+ Move these static globals further down the file so we can identify which
+ methods need both 'struct repository *repo' and 'struct index_state
+ *istate' parameters. The only changes included here adjust method
+ prototypes and their call locations.
+
+ The only remaining change is to remove the static globals entirely, but
+ that requires updating the parse-opt callbacks, which need a different
+ solution.
Signed-off-by: Derrick Stolee [off-list ref]
- ## builtin/merge-index.c ##
-@@
--#define USE_THE_INDEX_COMPATIBILITY_MACROS
- #include "builtin.h"
- #include "run-command.h"
+ ## builtin/update-index.c ##
+@@ builtin/update-index.c: static const char * const update_index_usage[] = {
+ static struct object_id head_oid;
+ static struct object_id merge_head_oid;
-@@ builtin/merge-index.c: static const char *pgm;
- static int one_shot, quiet;
- static int err;
-
--static int merge_entry(int pos, const char *path)
-+static int merge_entry(struct index_state *istate,
-+ int pos, const char *path)
+-static struct repository *repo;
+-static struct index_state *istate;
+-
+-static struct cache_entry *read_one_ent(const char *which,
++static struct cache_entry *read_one_ent(struct repository *repo,
++ struct index_state *istate,
++ const char *which,
+ struct object_id *ent, const char *path,
+ int namelen, int stage)
{
- int found;
- const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
- char hexbuf[4][GIT_MAX_HEXSZ + 1];
- char ownbuf[4][60];
-
-- if (pos >= active_nr)
-+ if (pos >= istate->cache_nr)
- die("git merge-index: %s not in the cache", path);
- found = 0;
- do {
-- const struct cache_entry *ce = active_cache[pos];
-+ const struct cache_entry *ce = istate->cache[pos];
- int stage = ce_stage(ce);
-
- if (strcmp(ce->name, path))
-@@ builtin/merge-index.c: static int merge_entry(int pos, const char *path)
- xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
- arguments[stage] = hexbuf[stage];
- arguments[stage + 4] = ownbuf[stage];
-- } while (++pos < active_nr);
-+ } while (++pos < istate->cache_nr);
- if (!found)
- die("git merge-index: %s not in the cache", path);
-
-@@ builtin/merge-index.c: static int merge_entry(int pos, const char *path)
- return found;
+@@ builtin/update-index.c: static struct cache_entry *read_one_ent(const char *which,
+ return ce;
}
--static void merge_one_path(const char *path)
-+static void merge_one_path(struct index_state *istate,
-+ const char *path)
+-static int unresolve_one(const char *path)
++static int unresolve_one(struct repository *repo,
++ struct index_state *istate,
++ const char *path)
{
-- int pos = cache_name_pos(path, strlen(path));
-+ int pos = index_name_pos(istate, path, strlen(path));
-
- /*
- * If it already exists in the cache as stage0, it's
- * already merged and there is nothing to do.
+ int namelen = strlen(path);
+ int pos;
+@@ builtin/update-index.c: static int unresolve_one(const char *path)
+ * stuff HEAD version in stage #2,
+ * stuff MERGE_HEAD version in stage #3.
*/
- if (pos < 0)
-- merge_entry(-pos-1, path);
-+ merge_entry(istate, -pos - 1, path);
+- ce_2 = read_one_ent("our", &head_oid, path, namelen, 2);
+- ce_3 = read_one_ent("their", &merge_head_oid, path, namelen, 3);
++ ce_2 = read_one_ent(repo, istate, "our", &head_oid, path, namelen, 2);
++ ce_3 = read_one_ent(repo, istate, "their", &merge_head_oid, path, namelen, 3);
+
+ if (!ce_2 || !ce_3) {
+ ret = -1;
+@@ builtin/update-index.c: static void read_head_pointers(void)
+ }
}
--static void merge_all(void)
-+static void merge_all(struct index_state *istate)
+-static int do_unresolve(int ac, const char **av,
++static int do_unresolve(struct repository *repo,
++ struct index_state *istate,
++ int ac, const char **av,
+ const char *prefix, int prefix_length)
{
int i;
-- for (i = 0; i < active_nr; i++) {
-- const struct cache_entry *ce = active_cache[i];
-+ for (i = 0; i < istate->cache_nr; i++) {
-+ const struct cache_entry *ce = istate->cache[i];
- if (!ce_stage(ce))
- continue;
-- i += merge_entry(i, ce->name)-1;
-+ i += merge_entry(istate, i, ce->name)-1;
+@@ builtin/update-index.c: static int do_unresolve(int ac, const char **av,
+ for (i = 1; i < ac; i++) {
+ const char *arg = av[i];
+ char *p = prefix_path(prefix, prefix_length, arg);
+- err |= unresolve_one(p);
++ err |= unresolve_one(repo, istate, p);
+ free(p);
}
+ return err;
}
- int cmd_merge_index(int argc, const char **argv, const char *prefix)
+-static int do_reupdate(int ac, const char **av,
++static int do_reupdate(struct repository *repo,
++ struct index_state *istate,
++ int ac, const char **av,
+ const char *prefix)
{
- int i, force_file = 0;
-+ struct index_state *istate;
+ /* Read HEAD and run update-index on paths that are
+@@ builtin/update-index.c: static int do_reupdate(int ac, const char **av,
+ if (ce_stage(ce) || !ce_path_match(istate, ce, &pathspec, NULL))
+ continue;
+ if (has_head)
+- old = read_one_ent(NULL, &head_oid,
++ old = read_one_ent(repo, istate, NULL, &head_oid,
+ ce->name, ce_namelen(ce), 0);
+ if (old && ce->ce_mode == old->ce_mode &&
+ oideq(&ce->oid, &old->oid)) {
+@@ builtin/update-index.c: struct refresh_params {
+ int *has_errors;
+ };
- /* Without this we cannot rely on waitpid() to tell
- * what happened to our children.
-@@ builtin/merge-index.c: int cmd_merge_index(int argc, const char **argv, const char *prefix)
- if (argc < 3)
- usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
++static struct repository *repo;
++static struct index_state *istate;
++
+ static int refresh(struct refresh_params *o, unsigned int flag)
+ {
+ setup_work_tree();
+@@ builtin/update-index.c: static enum parse_opt_result unresolve_callback(
+ BUG_ON_OPT_ARG(arg);
-- read_cache();
-+ repo_read_index(the_repository);
-+ istate = the_repository->index;
+ /* consume remaining arguments. */
+- *has_errors = do_unresolve(ctx->argc, ctx->argv,
+- prefix, prefix ? strlen(prefix) : 0);
++ *has_errors = do_unresolve(repo, istate, ctx->argc, ctx->argv,
++ prefix, prefix ? strlen(prefix) : 0);
+ if (*has_errors)
+ istate->cache_changed = 0;
+
+@@ builtin/update-index.c: static enum parse_opt_result reupdate_callback(
+
+ /* consume remaining arguments. */
+ setup_work_tree();
+- *has_errors = do_reupdate(ctx->argc, ctx->argv, prefix);
++ *has_errors = do_reupdate(repo, istate, ctx->argc, ctx->argv, prefix);
+ if (*has_errors)
+ istate->cache_changed = 0;
- i = 1;
- if (!strcmp(argv[i], "-o")) {
-@@ builtin/merge-index.c: int cmd_merge_index(int argc, const char **argv, const char *prefix)
- continue;
- }
- if (!strcmp(arg, "-a")) {
-- merge_all();
-+ merge_all(istate);
- continue;
- }
- die("git merge-index: unknown option %s", arg);
- }
-- merge_one_path(arg);
-+ merge_one_path(istate, arg);
- }
- if (err && !quiet)
- die("merge program failed");
-: ----------- > 14: 2b171a142b3 update-index: remove static globals from callbacks
--
gitgitgadget
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:06
From: Derrick Stolee <redacted>
The rm builtin still uses the antiquated compatibility macros for
interacting with the index. Update these to the more modern uses by
passing around a 'struct index_state' pointer.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/rm.c | 56 ++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 26 deletions(-)
@@ -284,24 +287,25 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!index_only)setup_work_tree();-hold_locked_index(&lock_file,LOCK_DIE_ON_ERROR);+repo_hold_locked_index(the_repository,&lock_file,LOCK_DIE_ON_ERROR);-if(read_cache()<0)+if(repo_read_index(the_repository)<0)die(_("index file corrupt"));-refresh_index(&the_index,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);+istate=the_repository->index;+refresh_index(istate,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);seen=xcalloc(pathspec.nr,1);-for(i=0;i<active_nr;i++){-conststructcache_entry*ce=active_cache[i];-if(!ce_path_match(&the_index,ce,&pathspec,seen))+for(i=0;i<istate->cache_nr;i++){+conststructcache_entry*ce=istate->cache[i];+if(!ce_path_match(istate,ce,&pathspec,seen))continue;ALLOC_GROW(list.entry,list.nr+1,list.alloc);list.entry[list.nr].name=xstrdup(ce->name);list.entry[list.nr].is_submodule=S_ISGITLINK(ce->ce_mode);if(list.entry[list.nr++].is_submodule&&-!is_staging_gitmodules_ok(&the_index))+!is_staging_gitmodules_ok(istate))die(_("please stage your changes to .gitmodules or stash them to proceed"));}
@@ -358,7 +362,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!quiet)printf("rm '%s'\n",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))die(_("git rm: unable to remove %s"),path);}
@@ -398,10 +402,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)}strbuf_release(&buf);if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);}-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:06
From: Derrick Stolee <redacted>
The mv builtin uses the compatibility macros to interact with the index.
Update these to use modern methods referring to a 'struct index_state'
pointer. Several helper methods need to be updated to consider such a
pointer, but the modifications are rudimentary.
Two macros can be deleted from cache.h because these are the last uses.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/mv.c | 42 +++++++++++++++++++++++-------------------
cache.h | 2 --
2 files changed, 23 insertions(+), 21 deletions(-)
@@ -75,13 +74,14 @@ static const char *add_slash(const char *path)#define SUBMODULE_WITH_GITDIR ((const char *)1)-staticvoidprepare_move_submodule(constchar*src,intfirst,+staticvoidprepare_move_submodule(structindex_state*istate,+constchar*src,intfirst,constchar**submodule_gitfile){structstrbufsubmodule_dotgit=STRBUF_INIT;-if(!S_ISGITLINK(active_cache[first]->ce_mode))+if(!S_ISGITLINK(istate->cache[first]->ce_mode))die(_("Directory %s is in index and no submodule?"),src);-if(!is_staging_gitmodules_ok(&the_index))+if(!is_staging_gitmodules_ok(istate))die(_("Please stage your changes to .gitmodules or stash them to proceed"));strbuf_addf(&submodule_dotgit,"%s/.git",src);*submodule_gitfile=read_gitfile(submodule_dotgit.buf);
@@ -92,19 +92,20 @@ static void prepare_move_submodule(const char *src, int first,strbuf_release(&submodule_dotgit);}-staticintindex_range_of_same_dir(constchar*src,intlength,+staticintindex_range_of_same_dir(structindex_state*istate,+constchar*src,intlength,int*first_p,int*last_p){constchar*src_w_slash=add_slash(src);intfirst,last,len_w_slash=length+1;-first=cache_name_pos(src_w_slash,len_w_slash);+first=index_name_pos(istate,src_w_slash,len_w_slash);if(first>=0)die(_("%.*s is in index"),len_w_slash,src_w_slash);first=-1-first;-for(last=first;last<active_nr;last++){-constchar*path=active_cache[last]->name;+for(last=first;last<istate->cache_nr;last++){+constchar*path=istate->cache[last]->name;if(strncmp(path,src_w_slash,len_w_slash))break;}
@@ -221,7 +225,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)}argc+=last-first;}-}elseif(!(ce=cache_file_exists(src,length,ignore_case))){+}elseif(!(ce=index_file_exists(istate,src,length,ignore_case))){bad=_("not under version control");}elseif(ce_stage(ce)){bad=_("conflicted");
@@ -291,15 +295,15 @@ int cmd_mv(int argc, const char **argv, const char *prefix)if(mode==WORKING_DIRECTORY)continue;-pos=cache_name_pos(src,strlen(src));+pos=index_name_pos(istate,src,strlen(src));assert(pos>=0);-rename_cache_entry_at(pos,dst);+rename_index_entry_at(istate,pos,dst);}if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
@@ -235,12 +235,12 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(istate,active_cache[pos]);+mark_fsmonitor_invalid(istate,istate->cache[pos]);if(mark)-active_cache[pos]->ce_flags|=flag;+istate->cache[pos]->ce_flags|=flag;else-active_cache[pos]->ce_flags&=~flag;-active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;+istate->cache[pos]->ce_flags&=~flag;+istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;
@@ -331,7 +331,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Exact match: file or existing gitlink */if(pos>=0){-conststructcache_entry*ce=active_cache[pos];+conststructcache_entry*ce=istate->cache[pos];if(S_ISGITLINK(ce->ce_mode)){/* Do nothing to the index if there is no HEAD! */
@@ -347,7 +347,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;while(pos<active_nr){-conststructcache_entry*ce=active_cache[pos++];+conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))break;
@@ -378,7 +378,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)returnerror("'%s' is beyond a symbolic link",path);pos=cache_name_pos(path,len);-ce=pos<0?NULL:active_cache[pos];+ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/**workingdirectoryversionisassumed"good"
@@ -346,7 +346,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;-while(pos<active_nr){+while(pos<istate->cache_nr){conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))
@@ -755,14 +755,14 @@ static int do_reupdate(int ac, const char **av,}/* Be careful. The working tree may not have the*pathanymore,inwhichcase,under'allow_remove',-*orworseyet'allow_replace',active_nrmaydecrease.+*orworseyet'allow_replace',istate->cache_nrmaydecrease.*/-save_nr=active_nr;+save_nr=istate->cache_nr;path=xstrdup(ce->name);update_one(path);free(path);discard_cache_entry(old);-if(save_nr!=active_nr)+if(save_nr!=istate->cache_nr)gotoredo;}clear_pathspec(&pathspec);
@@ -233,7 +233,7 @@ static int test_if_untracked_cache_is_supported(void)staticintmark_ce_flags(constchar*path,intflag,intmark){intnamelen=strlen(path);-intpos=cache_name_pos(path,namelen);+intpos=index_name_pos(istate,path,namelen);if(0<=pos){mark_fsmonitor_invalid(istate,istate->cache[pos]);if(mark)
@@ -327,7 +327,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstaticintprocess_directory(constchar*path,intlen,structstat*st){structobject_idoid;-intpos=cache_name_pos(path,len);+intpos=index_name_pos(istate,path,len);/* Exact match: file or existing gitlink */if(pos>=0){
@@ -377,7 +377,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)if(has_symlink_leading_path(path,len))returnerror("'%s' is beyond a symbolic link",path);-pos=cache_name_pos(path,len);+pos=index_name_pos(istate,path,len);ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/*
@@ -623,7 +623,7 @@ static int unresolve_one(const char *path)structcache_entry*ce_2=NULL,*ce_3=NULL;/* See if there is such entry in the index. */-pos=cache_name_pos(path,namelen);+pos=index_name_pos(istate,path,namelen);if(0<=pos){/* already merged */pos=unmerge_cache_entry_at(pos);
@@ -294,7 +294,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len}option=allow_add?ADD_CACHE_OK_TO_ADD:0;option|=allow_replace?ADD_CACHE_OK_TO_REPLACE:0;-if(add_cache_entry(ce,option)){+if(add_index_entry(istate,ce,option)){discard_cache_entry(ce);returnerror("%s: cannot add to the index - missing --add option?",path);}
@@ -672,12 +672,12 @@ static int unresolve_one(const char *path)}remove_file_from_index(istate,path);-if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){+if(add_index_entry(istate,ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;gotofree_return;}-if(!add_cache_entry(ce_3,ADD_CACHE_OK_TO_ADD))+if(!add_index_entry(istate,ce_3,ADD_CACHE_OK_TO_ADD))return0;error("%s: cannot add their version to the index.",path);ret=-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:32
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We can now start reducing the need for these
static globals by modifying method prototypes to use them when
necessary.
Move these static globals further down in the file so we can identify
which method only need to add a 'struct index_state *istate' parameter.
The only changes included here adjust method prototypes and their call
locations.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 77 ++++++++++++++++++++++++------------------
1 file changed, 44 insertions(+), 33 deletions(-)
@@ -229,7 +226,8 @@ static int test_if_untracked_cache_is_supported(void)returnret;}-staticintmark_ce_flags(constchar*path,intflag,intmark)+staticintmark_ce_flags(structindex_state*istate,+constchar*path,intflag,intmark){intnamelen=strlen(path);intpos=index_name_pos(istate,path,namelen);
@@ -247,7 +245,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)return-1;}-staticintremove_one_path(constchar*path)+staticintremove_one_path(structindex_state*istate,constchar*path){if(!allow_remove)returnerror("%s: does not exist and --remove not passed",path);
@@ -263,14 +261,17 @@ static int remove_one_path(const char *path)*succeeds.*-permissionerror.That'sneverok.*/-staticintprocess_lstat_error(constchar*path,interr)+staticintprocess_lstat_error(structindex_state*istate,+constchar*path,interr){if(is_missing_file_error(err))-returnremove_one_path(path);+returnremove_one_path(istate,path);returnerror("lstat(\"%s\"): %s",path,strerror(err));}-staticintadd_one_path(conststructcache_entry*old,constchar*path,intlen,structstat*st)+staticintadd_one_path(structindex_state*istate,+conststructcache_entry*old,+constchar*path,intlen,structstat*st){intoption;structcache_entry*ce;
@@ -323,7 +324,8 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len*-itdoesn'texistatallintheindex,butitisavalid*gitdirectory,anditshouldbe*added*asagitlink.*/-staticintprocess_directory(constchar*path,intlen,structstat*st)+staticintprocess_directory(structindex_state*istate,+constchar*path,intlen,structstat*st){structobject_idoid;intpos=index_name_pos(istate,path,len);
@@ -337,10 +339,10 @@ static int process_directory(const char *path, int len, struct stat *st)if(resolve_gitlink_ref(path,"HEAD",&oid)<0)return0;-returnadd_one_path(ce,path,len,st);+returnadd_one_path(istate,ce,path,len,st);}/* Should this be an unconditional error? */-returnremove_one_path(path);+returnremove_one_path(istate,path);}/* Inexact match: is there perhaps a subdirectory match? */
@@ -361,13 +363,14 @@ static int process_directory(const char *path, int len, struct stat *st)/* No match - should we add it as a gitlink? */if(!resolve_gitlink_ref(path,"HEAD",&oid))-returnadd_one_path(NULL,path,len,st);+returnadd_one_path(istate,NULL,path,len,st);/* Error out. */returnerror("%s: is a directory - add files inside instead",path);}-staticintprocess_path(constchar*path,structstat*st,intstat_errno)+staticintprocess_path(structindex_state*istate,+constchar*path,structstat*st,intstat_errno){intpos,len;conststructcache_entry*ce;
@@ -395,15 +398,16 @@ static int process_path(const char *path, struct stat *st, int stat_errno)*whattodoaboutthepathname!*/if(stat_errno)-returnprocess_lstat_error(path,stat_errno);+returnprocess_lstat_error(istate,path,stat_errno);if(S_ISDIR(st->st_mode))-returnprocess_directory(path,len,st);+returnprocess_directory(istate,path,len,st);-returnadd_one_path(ce,path,len,st);+returnadd_one_path(istate,ce,path,len,st);}-staticintadd_cacheinfo(unsignedintmode,conststructobject_id*oid,+staticintadd_cacheinfo(structindex_state*istate,+unsignedintmode,conststructobject_id*oid,constchar*path,intstage){intres;
@@ -420,7 +424,8 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,return0;}-staticvoidchmod_path(charflip,constchar*path)+staticvoidchmod_path(structindex_state*istate,+charflip,constchar*path){intpos;structcache_entry*ce;
@@ -456,17 +461,20 @@ static void update_one(const char *path)return;}if(mark_valid_only){-if(mark_ce_flags(path,CE_VALID,mark_valid_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_VALID,+mark_valid_only==MARK_FLAG))die("Unable to mark file %s",path);return;}if(mark_skip_worktree_only){-if(mark_ce_flags(path,CE_SKIP_WORKTREE,mark_skip_worktree_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_SKIP_WORKTREE,+mark_skip_worktree_only==MARK_FLAG))die("Unable to mark file %s",path);return;}if(mark_fsmonitor_only){-if(mark_ce_flags(path,CE_FSMONITOR_VALID,mark_fsmonitor_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_FSMONITOR_VALID,+mark_fsmonitor_only==MARK_FLAG))die("Unable to mark file %s",path);return;}
@@ -477,12 +485,12 @@ static void update_one(const char *path)report("remove '%s'",path);return;}-if(process_path(path,&st,stat_errno))+if(process_path(istate,path,&st,stat_errno))die("Unable to process path %s",path);report("add '%s'",path);}-staticvoidread_index_info(intnul_term_line)+staticvoidread_index_info(structindex_state*istate,intnul_term_line){constinthexsz=the_hash_algo->hexsz;structstrbufbuf=STRBUF_INIT;
@@ -882,7 +893,7 @@ static enum parse_opt_result stdin_cacheinfo_callback(if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);allow_add=allow_replace=allow_remove=1;-read_index_info(*nul_term_line);+read_index_info(istate,*nul_term_line);return0;}
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:34
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We remove these static globals inside the
option parsing callbacks, which are the final uses in update-index.
The callbacks cannot change their method signature, so we must use the
value member of 'struct option', assigned in the array of option macros.
There are several callback methods that require at least one of 'repo'
and 'istate', but they use a variety of different data types for the
callback value.
Unify these callback methods to use a consistent 'struct callback_data'
that contains 'repo' and 'istate', ready to use. This takes the place of
the previous 'struct refresh_params' which served only to group the
'flags' and 'has_errors' ints. We also collect other one-off settings,
but only those that require access to the index or repository in their
operation.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 110 ++++++++++++++++++++++-------------------
1 file changed, 60 insertions(+), 50 deletions(-)
@@ -901,7 +905,7 @@ static enum parse_opt_result stdin_cacheinfo_callback(if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);allow_add=allow_replace=allow_remove=1;-read_index_info(istate,*nul_term_line);+read_index_info(cd->istate,cd->nul_term_line);return0;}
@@ -909,14 +913,14 @@ static enum parse_opt_result stdin_callback(structparse_opt_ctx_t*ctx,conststructoption*opt,constchar*arg,intunset){-int*read_from_stdin=opt->value;+structcallback_data*cd=opt->value;BUG_ON_OPT_NEG(unset);BUG_ON_OPT_ARG(arg);if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);-*read_from_stdin=1;+cd->read_from_stdin=1;return0;}
@@ -979,11 +984,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;+structindex_state*istate;+structoptionoptions[]={-OPT_BIT('q',NULL,&refresh_args.flags,+OPT_BIT('q',NULL,&cd.flags,N_("continue refresh even when index needs update"),REFRESH_QUIET),-OPT_BIT(0,"ignore-submodules",&refresh_args.flags,+OPT_BIT(0,"ignore-submodules",&cd.flags,N_("refresh: ignore submodules"),REFRESH_IGNORE_SUBMODULES),OPT_SET_INT(0,"add",&allow_add,
@@ -992,18 +999,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)N_("let files replace directories and vice-versa"),1),OPT_SET_INT(0,"remove",&allow_remove,N_("notice files missing from worktree"),1),-OPT_BIT(0,"unmerged",&refresh_args.flags,+OPT_BIT(0,"unmerged",&cd.flags,N_("refresh even if index contains unmerged entries"),REFRESH_UNMERGED),-OPT_CALLBACK_F(0,"refresh",&refresh_args,NULL,+OPT_CALLBACK_F(0,"refresh",&cd,NULL,N_("refresh stat information"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,refresh_callback),-OPT_CALLBACK_F(0,"really-refresh",&refresh_args,NULL,+OPT_CALLBACK_F(0,"really-refresh",&cd,NULL,N_("like --refresh, but ignore assume-unchanged setting"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,really_refresh_callback),-{OPTION_LOWLEVEL_CALLBACK,0,"cacheinfo",NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"cacheinfo",&cd,N_("<mode>,<object>,<path>"),N_("add the specified entry to the index"),PARSE_OPT_NOARG|/* disallow --cacheinfo=<mode> form */
@@ -1032,30 +1039,30 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)N_("add to index only; do not add content to object database"),1),OPT_SET_INT(0,"force-remove",&force_remove,N_("remove named paths even if present in worktree"),1),-OPT_BOOL('z',NULL,&nul_term_line,+OPT_BOOL('z',NULL,&cd.nul_term_line,N_("with --stdin: input lines are terminated by null bytes")),-{OPTION_LOWLEVEL_CALLBACK,0,"stdin",&read_from_stdin,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"stdin",&cd,NULL,N_("read list of paths to be updated from standard input"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,stdin_callback},-{OPTION_LOWLEVEL_CALLBACK,0,"index-info",&nul_term_line,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"index-info",&cd,NULL,N_("add entries from standard input to the index"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,stdin_cacheinfo_callback},-{OPTION_LOWLEVEL_CALLBACK,0,"unresolve",&has_errors,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"unresolve",&cd,NULL,N_("repopulate stages #2 and #3 for the listed paths"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,unresolve_callback},-{OPTION_LOWLEVEL_CALLBACK,'g',"again",&has_errors,NULL,+{OPTION_LOWLEVEL_CALLBACK,'g',"again",&cd,NULL,N_("only update entries that differ from HEAD"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,reupdate_callback},-OPT_BIT(0,"ignore-missing",&refresh_args.flags,+OPT_BIT(0,"ignore-missing",&cd.flags,N_("ignore files missing from worktree"),REFRESH_IGNORE_MISSING),OPT_SET_INT(0,"verbose",&verbose,N_("report actions to standard output"),1),-OPT_CALLBACK_F(0,"clear-resolve-undo",NULL,NULL,+OPT_CALLBACK_F(0,"clear-resolve-undo",&cd,NULL,N_("(for porcelains) forget saved unresolved conflicts"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,resolve_undo_clear_callback),
@@ -1087,8 +1094,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);-repo=the_repository;-/* we will diagnose later if it turns out that we need to update it */newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:34
From: Derrick Stolee <redacted>
This is also the last usage of unmerge_cache_entry_at(), so it can be
removed from cache.h.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 11 ++++++-----
cache.h | 1 -
2 files changed, 6 insertions(+), 6 deletions(-)
@@ -252,7 +252,7 @@ static int remove_one_path(const char *path){if(!allow_remove)returnerror("%s: does not exist and --remove not passed",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -386,7 +386,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)*Ontheotherhand,removingitfromindexshouldwork*/if(!ignore_skip_worktree_entries&&allow_remove&&-remove_file_from_cache(path))+remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -556,7 +556,7 @@ static void read_index_info(int nul_term_line)if(!mode){/* mode == 0 means there is no such path -- remove */-if(remove_file_from_cache(path_name))+if(remove_file_from_index(istate,path_name))die("git update-index: unable to remove %s",ptr);}
@@ -671,7 +671,7 @@ static int unresolve_one(const char *path)gotofree_return;}-remove_file_from_cache(path);+remove_file_from_index(istate,path);if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;
@@ -242,7 +242,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)istate->cache[pos]->ce_flags&=~flag;istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);-active_cache_changed|=CE_ENTRY_CHANGED;+istate->cache_changed|=CE_ENTRY_CHANGED;return0;}return-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:39
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We can continue reducing the need for these
static globals by modifying method prototypes to use them when
necessary.
Move these static globals further down the file so we can identify which
methods need both 'struct repository *repo' and 'struct index_state
*istate' parameters. The only changes included here adjust method
prototypes and their call locations.
The only remaining change is to remove the static globals entirely, but
that requires updating the parse-opt callbacks, which need a different
solution.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
@@ -721,13 +724,15 @@ static int do_unresolve(int ac, const char **av,for(i=1;i<ac;i++){constchar*arg=av[i];char*p=prefix_path(prefix,prefix_length,arg);-err|=unresolve_one(p);+err|=unresolve_one(repo,istate,p);free(p);}returnerr;}-staticintdo_reupdate(intac,constchar**av,+staticintdo_reupdate(structrepository*repo,+structindex_state*istate,+intac,constchar**av,constchar*prefix){/* Read HEAD and run update-index on paths that are
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-05 04:44:39
From: Derrick Stolee <redacted>
The final index compatibility macro to remove from the update-index
builtin is ce_match_stat(). Further, this is the last use of that macro
anywhere, so it should be removed.
There are some remaining references in the racy-git.txt technical
document that are updated to ie_match_stat().
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/racy-git.txt | 6 +++---
builtin/update-index.c | 3 +--
cache.h | 1 -
3 files changed, 4 insertions(+), 6 deletions(-)
@@ -26,7 +26,7 @@ information obtained from the filesystem via `lstat(2)` system call when they were last updated. When checking if they differ, Git first runs `lstat(2)` on the files and compares the result with this information (this is what was originally done by the-`ce_match_stat()` function, but the current code does it in+`ie_match_stat()` function, but the current code does it in `ce_match_stat_basic()` function). If some of these "cached stat information" fields do not match, Git can tell that the files are modified without even looking at their contents.
@@ -102,7 +102,7 @@ timestamp as the index file itself. The callers that want to check if an index entry matches the corresponding file in the working tree continue to call-`ce_match_stat()`, but with this change, `ce_match_stat()` uses+`ie_match_stat()`, but with this change, `ie_match_stat()` uses `ce_modified_check_fs()` to see if racily clean ones are actually clean after comparing the cached stat information using `ce_match_stat_basic()`.
@@ -128,7 +128,7 @@ Runtime penalty --------------- The runtime penalty of falling back to `ce_modified_check_fs()`-from `ce_match_stat()` can be very expensive when there are many+from `ie_match_stat()` can be very expensive when there are many racily clean entries. An obvious way to artificially create this situation is to give the same timestamp to all the files in the working tree in a large project, run `git update-index` on
@@ -277,7 +276,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstructcache_entry*ce;/* Was the old index entry already up-to-date? */-if(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))+if(old&&!ce_stage(old)&&!ie_match_stat(istate,old,st,0))return0;ce=make_empty_cache_entry(istate,len);
From: Junio C Hamano <hidden> Date: 2021-01-06 03:56:11
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
Knee-jerk reaction: swapping one pair of global with another? Would
that give us enough upside? It may allow some codepaths involved to
work on an in-core index instance that is different from the_index,
but does not make them reentrant.
Do we now have callers that actually pass an in-core index instance
that is different from the_index, and more importantly, that fail
loudly if the codepaths involved in this conversion forgets to
update some accesses to the_index not to the specified one?
If not, ...
In total, this allows us to remove four of the compatibility macros because
they are no longer used.
... a conversion like this, removing the use of the compatibility
macros for the sake of removing them, invites future headaches by
leaving untested code churn behind with potential bugs that will
only get discovered after somebody actually starts making calls
with the non-default in-core index instances.
I've come to know the competence of you well enough to trust your
patches like patches from other proficient, prolific and prominent
contributors (I won't name names, but you know who you are), but we
are all human and are prone to introduce bugs.
That's all my knee-jerk impression before actually reading the
series through, though. I'll certainloy know more after reading
them.
Thanks.
Derrick Stolee (12):
merge-index: drop index compatibility macros
mv: remove index compatibility macros
rm: remove compatilibity macros
update-index: drop the_index, the_repository
update-index: use istate->cache over active_cache
update-index: use index->cache_nr over active_nr
update-index: use istate->cache_changed
update-index: use index_name_pos() over cache_name_pos()
update-index: use remove_file_from_index()
update-index: use add_index_entry()
update-index: replace several compatibility macros
update-index: remove ce_match_stat(), all macros
Documentation/technical/racy-git.txt | 6 +-
builtin/merge-index.c | 33 +++---
builtin/mv.c | 42 ++++----
builtin/rm.c | 56 ++++++-----
builtin/update-index.c | 145 ++++++++++++++-------------
cache.h | 4 -
6 files changed, 149 insertions(+), 137 deletions(-)
base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-830%2Fderrickstolee%2Findex-compatibility-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-830/derrickstolee/index-compatibility-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/830
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted
My strategy for update-index was to create static globals "repo" and
"istate" that point to the_repository and the_index, respectively. Then, I
was able to remove macros one-by-one without changing method prototypes
within the file.
Knee-jerk reaction: swapping one pair of global with another? Would
that give us enough upside? It may allow some codepaths involved to
work on an in-core index instance that is different from the_index,
but does not make them reentrant.
My intention was to reduce the use of globals in libgit.a while keeping
with existing patterns of static globals in the builtin code. While
this can be thought of "module variables" instead of true globals, they
aren't exactly desirable. In v2, these static globals are temporary to
the series and are completely removed by the end.
The new patch sequence can hopefully be seen as "this preprocessor
macro was expanded" and then "static globals are replaced with
method parameters" which are pretty straightforward.
Do we now have callers that actually pass an in-core index instance
that is different from the_index, and more importantly, that fail
loudly if the codepaths involved in this conversion forgets to
update some accesses to the_index not to the specified one?
If not, ...
quoted
In total, this allows us to remove four of the compatibility macros because
they are no longer used.
... a conversion like this, removing the use of the compatibility
macros for the sake of removing them, invites future headaches by
leaving untested code churn behind with potential bugs that will
only get discovered after somebody actually starts making calls
with the non-default in-core index instances.
Perhaps I had misunderstood the state of the conversion project. I
thought that the full conversion was just paused because Duy moved
on to other things. I thought it might be valuable to pick up the
baton while also thinking about the space.
If this is _not_ a valuable project to continue, then I can hold
off for now.
Unfortunately, we'll never know if everything is safe from assuming
the_index until the macro itself is gone. It helps that libgit.a
doesn't use it at
I've come to know the competence of you well enough to trust your
patches like patches from other proficient, prolific and prominent
contributors (I won't name names, but you know who you are), but we
are all human and are prone to introduce bugs.
That means a lot, thanks. And yes, I'm well aware that bugs can be
introduced. I've added my share.
That's all my knee-jerk impression before actually reading the
series through, though. I'll certainloy know more after reading
them.
From: Eric Sunshine <hidden> Date: 2021-01-07 05:10:13
On Mon, Jan 4, 2021 at 11:43 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We remove these static globals inside the
option parsing callbacks, which are the final uses in update-index.
The callbacks cannot change their method signature, so we must use the
value member of 'struct option', assigned in the array of option macros.
There are several callback methods that require at least one of 'repo'
and 'istate', but they use a variety of different data types for the
callback value.
Unify these callback methods to use a consistent 'struct callback_data'
that contains 'repo' and 'istate', ready to use. This takes the place of
the previous 'struct refresh_params' which served only to group the
'flags' and 'has_errors' ints. We also collect other one-off settings,
but only those that require access to the index or repository in their
operation.
Makes sense. The patch itself is necessarily a bit noisy, but there's
nothing particularly complicated in that noise.
@@ -784,19 +784,21 @@ static int do_reupdate(struct repository *repo,-struct refresh_params {+struct callback_data {+ struct repository *repo;+ struct index_state *istate;+ unsigned int flags;- int *has_errors;+ unsigned int has_errors;+ unsigned nul_term_line;+ unsigned read_from_stdin; };
The only mildly unexpected thing here is that `has_errors` is now a
simple value rather than a pointer to a value, but you handle that
easily enough by always accessing `has_error` directly from the
structure, even within the function in which `has_error` used to be a
local variable. Fine.
quoted hunk
@@ -818,7 +820,7 @@ static int really_refresh_callback(const struct option *opt, static int chmod_callback(const struct option *opt,- const char *arg, int unset)+ const char *arg, int unset)
@@ -829,11 +831,12 @@ static int chmod_callback(const struct option *opt, static int resolve_undo_clear_callback(const struct option *opt,- const char *arg, int unset)+ const char *arg, int unset)
Will there ever be a case in which `cd.istate` will be different from
`cd.repo->index`? If not, then we could get by with having only
`cd.repo`; callers requiring access to `istate` can fetch it from
`cd.repo`. If, on the other hand, `cd.istate` can be different from
`cd.repo->istate` -- or if that might become a possibility in the
future -- then having `cd.istate` makes sense. Not a big deal, though.
Just generally curious about it.
Will there ever be a case in which `cd.istate` will be different from
`cd.repo->index`? If not, then we could get by with having only
`cd.repo`; callers requiring access to `istate` can fetch it from
`cd.repo`. If, on the other hand, `cd.istate` can be different from
`cd.repo->istate` -- or if that might become a possibility in the
future -- then having `cd.istate` makes sense. Not a big deal, though.
Just generally curious about it.
I don't believe that 'istate' and 'repo->index' will ever be
different in this file. This includes the members of the
callback_data struct, but also the method parameters throughout.
Mostly, this could be seen as an artifact of how we got here:
1. References to the_index or other compatibility macros were
converted to use the static global 'istate'.
2. References to the static global 'istate' were replaced with
method parameters for everything except these callbacks.
3. These callbacks were updated to use 'cd.istate' instead of
the (now defunct) static global 'istate'.
It could be possible to replace all references to 'istate' with
'repo->index' but the patches get slightly more messy. I also
think the code looks messier, but you do make a good point that
there is no concrete reason to separate the two.
Thanks,
-Stolee
From: Eric Sunshine <hidden> Date: 2021-01-07 18:54:55
On Thu, Jan 7, 2021 at 6:19 AM Derrick Stolee [off-list ref] wrote:
On 1/7/2021 12:09 AM, Eric Sunshine wrote:
quoted
Will there ever be a case in which `cd.istate` will be different from
`cd.repo->index`? If not, then we could get by with having only
`cd.repo`; callers requiring access to `istate` can fetch it from
`cd.repo`. If, on the other hand, `cd.istate` can be different from
`cd.repo->istate` -- or if that might become a possibility in the
future -- then having `cd.istate` makes sense. Not a big deal, though.
Just generally curious about it.
I don't believe that 'istate' and 'repo->index' will ever be
different in this file. This includes the members of the
callback_data struct, but also the method parameters throughout.
It could be possible to replace all references to 'istate' with
'repo->index' but the patches get slightly more messy. I also
think the code looks messier, but you do make a good point that
there is no concrete reason to separate the two.
I agree that it would make the code a bit noisier (to read) if
`istate` is eliminated from the callback structure, however, even
though I didn't originally feel strongly one way or the other about
having both `repo` and `istate` in the structure, I'm now leaning more
toward seeing `istate` eliminated. My one (big) concern with `istate`
is that it confuses readers into wondering whether `istate` and
`repo->istate` will ever be different. One way to avoid such confusion
would be to leave a comment in the code stating that the two values
will always be the same. The other way, of course, is to eliminate
`istate` from the structure altogether. I don't want to make more work
for you, but the more I think about it, the more I feel that removing
`istate` is the sensible thing to do. (And it doesn't require an extra
patch -- it can just be how this patch is crafted -- without ever
introducing `istate` to the structure in the first place.)
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:03:57
From: Derrick Stolee <redacted>
The mv builtin uses the compatibility macros to interact with the index.
Update these to use modern methods referring to a 'struct index_state'
pointer. Several helper methods need to be updated to consider such a
pointer, but the modifications are rudimentary.
Two macros can be deleted from cache.h because these are the last uses.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/mv.c | 42 +++++++++++++++++++++++-------------------
cache.h | 2 --
2 files changed, 23 insertions(+), 21 deletions(-)
@@ -75,13 +74,14 @@ static const char *add_slash(const char *path)#define SUBMODULE_WITH_GITDIR ((const char *)1)-staticvoidprepare_move_submodule(constchar*src,intfirst,+staticvoidprepare_move_submodule(structindex_state*istate,+constchar*src,intfirst,constchar**submodule_gitfile){structstrbufsubmodule_dotgit=STRBUF_INIT;-if(!S_ISGITLINK(active_cache[first]->ce_mode))+if(!S_ISGITLINK(istate->cache[first]->ce_mode))die(_("Directory %s is in index and no submodule?"),src);-if(!is_staging_gitmodules_ok(&the_index))+if(!is_staging_gitmodules_ok(istate))die(_("Please stage your changes to .gitmodules or stash them to proceed"));strbuf_addf(&submodule_dotgit,"%s/.git",src);*submodule_gitfile=read_gitfile(submodule_dotgit.buf);
@@ -92,19 +92,20 @@ static void prepare_move_submodule(const char *src, int first,strbuf_release(&submodule_dotgit);}-staticintindex_range_of_same_dir(constchar*src,intlength,+staticintindex_range_of_same_dir(structindex_state*istate,+constchar*src,intlength,int*first_p,int*last_p){constchar*src_w_slash=add_slash(src);intfirst,last,len_w_slash=length+1;-first=cache_name_pos(src_w_slash,len_w_slash);+first=index_name_pos(istate,src_w_slash,len_w_slash);if(first>=0)die(_("%.*s is in index"),len_w_slash,src_w_slash);first=-1-first;-for(last=first;last<active_nr;last++){-constchar*path=active_cache[last]->name;+for(last=first;last<istate->cache_nr;last++){+constchar*path=istate->cache[last]->name;if(strncmp(path,src_w_slash,len_w_slash))break;}
@@ -221,7 +225,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)}argc+=last-first;}-}elseif(!(ce=cache_file_exists(src,length,ignore_case))){+}elseif(!(ce=index_file_exists(istate,src,length,ignore_case))){bad=_("not under version control");}elseif(ce_stage(ce)){bad=_("conflicted");
@@ -291,15 +295,15 @@ int cmd_mv(int argc, const char **argv, const char *prefix)if(mode==WORKING_DIRECTORY)continue;-pos=cache_name_pos(src,strlen(src));+pos=index_name_pos(istate,src,strlen(src));assert(pos>=0);-rename_cache_entry_at(pos,dst);+rename_index_entry_at(istate,pos,dst);}if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:03:57
From: Derrick Stolee <redacted>
The rm builtin still uses the antiquated compatibility macros for
interacting with the index. Update these to the more modern uses by
passing around a 'struct index_state' pointer.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/rm.c | 56 ++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 26 deletions(-)
@@ -284,24 +287,25 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!index_only)setup_work_tree();-hold_locked_index(&lock_file,LOCK_DIE_ON_ERROR);+repo_hold_locked_index(the_repository,&lock_file,LOCK_DIE_ON_ERROR);-if(read_cache()<0)+if(repo_read_index(the_repository)<0)die(_("index file corrupt"));-refresh_index(&the_index,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);+istate=the_repository->index;+refresh_index(istate,REFRESH_QUIET|REFRESH_UNMERGED,&pathspec,NULL,NULL);seen=xcalloc(pathspec.nr,1);-for(i=0;i<active_nr;i++){-conststructcache_entry*ce=active_cache[i];-if(!ce_path_match(&the_index,ce,&pathspec,seen))+for(i=0;i<istate->cache_nr;i++){+conststructcache_entry*ce=istate->cache[i];+if(!ce_path_match(istate,ce,&pathspec,seen))continue;ALLOC_GROW(list.entry,list.nr+1,list.alloc);list.entry[list.nr].name=xstrdup(ce->name);list.entry[list.nr].is_submodule=S_ISGITLINK(ce->ce_mode);if(list.entry[list.nr++].is_submodule&&-!is_staging_gitmodules_ok(&the_index))+!is_staging_gitmodules_ok(istate))die(_("please stage your changes to .gitmodules or stash them to proceed"));}
@@ -358,7 +362,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)if(!quiet)printf("rm '%s'\n",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))die(_("git rm: unable to remove %s"),path);}
@@ -398,10 +402,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)}strbuf_release(&buf);if(gitmodules_modified)-stage_updated_gitmodules(&the_index);+stage_updated_gitmodules(istate);}-if(write_locked_index(&the_index,&lock_file,+if(write_locked_index(istate,&lock_file,COMMIT_LOCK|SKIP_IF_UNCHANGED))die(_("Unable to write new index file"));
@@ -234,12 +234,12 @@ static int mark_ce_flags(const char *path, int flag, int mark)intnamelen=strlen(path);intpos=cache_name_pos(path,namelen);if(0<=pos){-mark_fsmonitor_invalid(istate,active_cache[pos]);+mark_fsmonitor_invalid(istate,istate->cache[pos]);if(mark)-active_cache[pos]->ce_flags|=flag;+istate->cache[pos]->ce_flags|=flag;else-active_cache[pos]->ce_flags&=~flag;-active_cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;+istate->cache[pos]->ce_flags&=~flag;+istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);active_cache_changed|=CE_ENTRY_CHANGED;return0;
@@ -330,7 +330,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Exact match: file or existing gitlink */if(pos>=0){-conststructcache_entry*ce=active_cache[pos];+conststructcache_entry*ce=istate->cache[pos];if(S_ISGITLINK(ce->ce_mode)){/* Do nothing to the index if there is no HEAD! */
@@ -346,7 +346,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;while(pos<active_nr){-conststructcache_entry*ce=active_cache[pos++];+conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))break;
@@ -377,7 +377,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)returnerror("'%s' is beyond a symbolic link",path);pos=cache_name_pos(path,len);-ce=pos<0?NULL:active_cache[pos];+ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/**workingdirectoryversionisassumed"good"
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:04:00
From: Derrick Stolee <redacted>
To reduce the need for the index compatibility macros, we will replace
their uses in update-index mechanically. This is the most interesting
change, which creates global "repo" and "istate" pointers. The macros
that expand to use the_index can then be mechanically replaced by
references to the istate pointer.
We will be careful to use "repo->index" over "istate" whenever repo is
needed by a method.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 59 +++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 27 deletions(-)
@@ -957,7 +960,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;-structrepository*r=the_repository;structoptionoptions[]={OPT_BIT('q',NULL,&refresh_args.flags,N_("continue refresh even when index needs update"),
@@ -1066,16 +1068,19 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);+repo=the_repository;+/* we will diagnose later if it turns out that we need to update it */-newfd=hold_locked_index(&lock_file,0);+newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)lock_error=errno;-entries=read_cache();+entries=repo_read_index(repo);if(entries<0)die("cache corrupted");-the_index.updated_skipworktree=1;+istate=repo->index;+repo->index->updated_skipworktree=1;/**Customcopyofparse_options()becausewewanttohandle
@@ -1162,28 +1167,28 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.splitIndex is set to false; ""remove or change it, if you really want to ""enable split index"));-if(the_index.split_index)-the_index.cache_changed|=SPLIT_INDEX_ORDERED;+if(repo->index->split_index)+repo->index->cache_changed|=SPLIT_INDEX_ORDERED;else-add_split_index(&the_index);+add_split_index(repo->index);}elseif(!split_index){if(git_config_get_split_index()==1)warning(_("core.splitIndex is set to true; ""remove or change it, if you really want to ""disable split index"));-remove_split_index(&the_index);+remove_split_index(repo->index);}-prepare_repo_settings(r);+prepare_repo_settings(repo);switch(untracked_cache){caseUC_UNSPECIFIED:break;caseUC_DISABLE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_WRITE)warning(_("core.untrackedCache is set to true; ""remove or change it, if you really want to ""disable the untracked cache"));-remove_untracked_cache(&the_index);+remove_untracked_cache(repo->index);report(_("Untracked cache disabled"));break;caseUC_TEST:
@@ -1191,11 +1196,11 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)return!test_if_untracked_cache_is_supported();caseUC_ENABLE:caseUC_FORCE:-if(r->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)+if(repo->settings.core_untracked_cache==UNTRACKED_CACHE_REMOVE)warning(_("core.untrackedCache is set to false; ""remove or change it, if you really want to ""enable the untracked cache"));-add_untracked_cache(&the_index);+add_untracked_cache(repo->index);report(_("Untracked cache enabled for '%s'"),get_git_work_tree());break;default:
@@ -1207,14 +1212,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)warning(_("core.fsmonitor is unset; ""set it if you really want to ""enable fsmonitor"));-add_fsmonitor(&the_index);+add_fsmonitor(repo->index);report(_("fsmonitor enabled"));}elseif(!fsmonitor){if(git_config_get_fsmonitor()==1)warning(_("core.fsmonitor is set; ""remove it if you really want to ""disable fsmonitor"));-remove_fsmonitor(&the_index);+remove_fsmonitor(repo->index);report(_("fsmonitor disabled"));}
@@ -1224,7 +1229,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)exit(128);unable_to_lock_die(get_index_file(),lock_error);}-if(write_locked_index(&the_index,&lock_file,COMMIT_LOCK))+if(write_locked_index(repo->index,&lock_file,COMMIT_LOCK))die("Unable to write new index file");}
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:04:15
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We can now start reducing the need for these
static globals by modifying method prototypes to use them when
necessary.
Remove the 'istate' static global in favor of method parameters. This
adjusts some callers, which either use their own 'istate' parameter or
'repo->index'.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 74 +++++++++++++++++++++++-------------------
1 file changed, 41 insertions(+), 33 deletions(-)
@@ -226,9 +226,8 @@ static int test_if_untracked_cache_is_supported(void)returnret;}-staticstructindex_state*istate;--staticintmark_ce_flags(constchar*path,intflag,intmark)+staticintmark_ce_flags(structindex_state*istate,+constchar*path,intflag,intmark){intnamelen=strlen(path);intpos=index_name_pos(istate,path,namelen);
@@ -246,7 +245,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)return-1;}-staticintremove_one_path(constchar*path)+staticintremove_one_path(structindex_state*istate,constchar*path){if(!allow_remove)returnerror("%s: does not exist and --remove not passed",path);
@@ -262,14 +261,17 @@ static int remove_one_path(const char *path)*succeeds.*-permissionerror.That'sneverok.*/-staticintprocess_lstat_error(constchar*path,interr)+staticintprocess_lstat_error(structindex_state*istate,+constchar*path,interr){if(is_missing_file_error(err))-returnremove_one_path(path);+returnremove_one_path(istate,path);returnerror("lstat(\"%s\"): %s",path,strerror(err));}-staticintadd_one_path(conststructcache_entry*old,constchar*path,intlen,structstat*st)+staticintadd_one_path(structindex_state*istate,+conststructcache_entry*old,+constchar*path,intlen,structstat*st){intoption;structcache_entry*ce;
@@ -322,7 +324,8 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len*-itdoesn'texistatallintheindex,butitisavalid*gitdirectory,anditshouldbe*added*asagitlink.*/-staticintprocess_directory(constchar*path,intlen,structstat*st)+staticintprocess_directory(structindex_state*istate,+constchar*path,intlen,structstat*st){structobject_idoid;intpos=index_name_pos(istate,path,len);
@@ -336,10 +339,10 @@ static int process_directory(const char *path, int len, struct stat *st)if(resolve_gitlink_ref(path,"HEAD",&oid)<0)return0;-returnadd_one_path(ce,path,len,st);+returnadd_one_path(istate,ce,path,len,st);}/* Should this be an unconditional error? */-returnremove_one_path(path);+returnremove_one_path(istate,path);}/* Inexact match: is there perhaps a subdirectory match? */
@@ -360,13 +363,14 @@ static int process_directory(const char *path, int len, struct stat *st)/* No match - should we add it as a gitlink? */if(!resolve_gitlink_ref(path,"HEAD",&oid))-returnadd_one_path(NULL,path,len,st);+returnadd_one_path(istate,NULL,path,len,st);/* Error out. */returnerror("%s: is a directory - add files inside instead",path);}-staticintprocess_path(constchar*path,structstat*st,intstat_errno)+staticintprocess_path(structindex_state*istate,+constchar*path,structstat*st,intstat_errno){intpos,len;conststructcache_entry*ce;
@@ -394,15 +398,16 @@ static int process_path(const char *path, struct stat *st, int stat_errno)*whattodoaboutthepathname!*/if(stat_errno)-returnprocess_lstat_error(path,stat_errno);+returnprocess_lstat_error(istate,path,stat_errno);if(S_ISDIR(st->st_mode))-returnprocess_directory(path,len,st);+returnprocess_directory(istate,path,len,st);-returnadd_one_path(ce,path,len,st);+returnadd_one_path(istate,ce,path,len,st);}-staticintadd_cacheinfo(unsignedintmode,conststructobject_id*oid,+staticintadd_cacheinfo(structindex_state*istate,+unsignedintmode,conststructobject_id*oid,constchar*path,intstage){intres;
@@ -419,7 +424,8 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,return0;}-staticvoidchmod_path(charflip,constchar*path)+staticvoidchmod_path(structindex_state*istate,+charflip,constchar*path){intpos;structcache_entry*ce;
@@ -455,17 +461,20 @@ static void update_one(const char *path)return;}if(mark_valid_only){-if(mark_ce_flags(path,CE_VALID,mark_valid_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_VALID,+mark_valid_only==MARK_FLAG))die("Unable to mark file %s",path);return;}if(mark_skip_worktree_only){-if(mark_ce_flags(path,CE_SKIP_WORKTREE,mark_skip_worktree_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_SKIP_WORKTREE,+mark_skip_worktree_only==MARK_FLAG))die("Unable to mark file %s",path);return;}if(mark_fsmonitor_only){-if(mark_ce_flags(path,CE_FSMONITOR_VALID,mark_fsmonitor_only==MARK_FLAG))+if(mark_ce_flags(istate,path,CE_FSMONITOR_VALID,+mark_fsmonitor_only==MARK_FLAG))die("Unable to mark file %s",path);return;}
@@ -476,12 +485,12 @@ static void update_one(const char *path)report("remove '%s'",path);return;}-if(process_path(path,&st,stat_errno))+if(process_path(istate,path,&st,stat_errno))die("Unable to process path %s",path);report("add '%s'",path);}-staticvoidread_index_info(intnul_term_line)+staticvoidread_index_info(structindex_state*istate,intnul_term_line){constinthexsz=the_hash_algo->hexsz;structstrbufbuf=STRBUF_INIT;
@@ -882,7 +891,7 @@ static enum parse_opt_result stdin_cacheinfo_callback(if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);allow_add=allow_replace=allow_remove=1;-read_index_info(*nul_term_line);+read_index_info(repo->index,*nul_term_line);return0;}
@@ -345,7 +345,7 @@ static int process_directory(const char *path, int len, struct stat *st)/* Inexact match: is there perhaps a subdirectory match? */pos=-pos-1;-while(pos<active_nr){+while(pos<istate->cache_nr){conststructcache_entry*ce=istate->cache[pos++];if(strncmp(ce->name,path,len))
@@ -755,14 +755,14 @@ static int do_reupdate(int ac, const char **av,}/* Be careful. The working tree may not have the*pathanymore,inwhichcase,under'allow_remove',-*orworseyet'allow_replace',active_nrmaydecrease.+*orworseyet'allow_replace',repo->index->cache_nrmaydecrease.*/-save_nr=active_nr;+save_nr=repo->index->cache_nr;path=xstrdup(ce->name);update_one(path);free(path);discard_cache_entry(old);-if(save_nr!=active_nr)+if(save_nr!=repo->index->cache_nr)gotoredo;}clear_pathspec(&pathspec);
@@ -326,7 +326,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstaticintprocess_directory(constchar*path,intlen,structstat*st){structobject_idoid;-intpos=cache_name_pos(path,len);+intpos=index_name_pos(istate,path,len);/* Exact match: file or existing gitlink */if(pos>=0){
@@ -376,7 +376,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)if(has_symlink_leading_path(path,len))returnerror("'%s' is beyond a symbolic link",path);-pos=cache_name_pos(path,len);+pos=index_name_pos(istate,path,len);ce=pos<0?NULL:istate->cache[pos];if(ce&&ce_skip_worktree(ce)){/*
@@ -623,7 +623,7 @@ static int unresolve_one(const char *path)structcache_entry*ce_2=NULL,*ce_3=NULL;/* See if there is such entry in the index. */-pos=cache_name_pos(path,namelen);+pos=index_name_pos(repo->index,path,namelen);if(0<=pos){/* already merged */pos=unmerge_cache_entry_at(pos);
@@ -293,7 +293,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len}option=allow_add?ADD_CACHE_OK_TO_ADD:0;option|=allow_replace?ADD_CACHE_OK_TO_REPLACE:0;-if(add_cache_entry(ce,option)){+if(add_index_entry(istate,ce,option)){discard_cache_entry(ce);returnerror("%s: cannot add to the index - missing --add option?",path);}
@@ -672,12 +672,12 @@ static int unresolve_one(const char *path)}remove_file_from_index(repo->index,path);-if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){+if(add_index_entry(repo->index,ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;gotofree_return;}-if(!add_cache_entry(ce_3,ADD_CACHE_OK_TO_ADD))+if(!add_index_entry(repo->index,ce_3,ADD_CACHE_OK_TO_ADD))return0;error("%s: cannot add their version to the index.",path);ret=-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:04:25
From: Derrick Stolee <redacted>
The final index compatibility macro to remove from the update-index
builtin is ce_match_stat(). Further, this is the last use of that macro
anywhere, so it should be removed.
There are some remaining references in the racy-git.txt technical
document that are updated to ie_match_stat().
Signed-off-by: Derrick Stolee <redacted>
---
Documentation/technical/racy-git.txt | 6 +++---
builtin/update-index.c | 3 +--
cache.h | 1 -
3 files changed, 4 insertions(+), 6 deletions(-)
@@ -26,7 +26,7 @@ information obtained from the filesystem via `lstat(2)` system call when they were last updated. When checking if they differ, Git first runs `lstat(2)` on the files and compares the result with this information (this is what was originally done by the-`ce_match_stat()` function, but the current code does it in+`ie_match_stat()` function, but the current code does it in `ce_match_stat_basic()` function). If some of these "cached stat information" fields do not match, Git can tell that the files are modified without even looking at their contents.
@@ -102,7 +102,7 @@ timestamp as the index file itself. The callers that want to check if an index entry matches the corresponding file in the working tree continue to call-`ce_match_stat()`, but with this change, `ce_match_stat()` uses+`ie_match_stat()`, but with this change, `ie_match_stat()` uses `ce_modified_check_fs()` to see if racily clean ones are actually clean after comparing the cached stat information using `ce_match_stat_basic()`.
@@ -128,7 +128,7 @@ Runtime penalty --------------- The runtime penalty of falling back to `ce_modified_check_fs()`-from `ce_match_stat()` can be very expensive when there are many+from `ie_match_stat()` can be very expensive when there are many racily clean entries. An obvious way to artificially create this situation is to give the same timestamp to all the files in the working tree in a large project, run `git update-index` on
@@ -276,7 +275,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int lenstructcache_entry*ce;/* Was the old index entry already up-to-date? */-if(old&&!ce_stage(old)&&!ce_match_stat(old,st,0))+if(old&&!ce_stage(old)&&!ie_match_stat(istate,old,st,0))return0;ce=make_empty_cache_entry(istate,len);
@@ -241,7 +241,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)istate->cache[pos]->ce_flags&=~flag;istate->cache[pos]->ce_flags|=CE_UPDATE_IN_BASE;cache_tree_invalidate_path(istate,path);-active_cache_changed|=CE_ENTRY_CHANGED;+istate->cache_changed|=CE_ENTRY_CHANGED;return0;}return-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:04:28
From: Derrick Stolee <redacted>
This is also the last usage of unmerge_cache_entry_at(), so it can be
removed from cache.h.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 11 ++++++-----
cache.h | 1 -
2 files changed, 6 insertions(+), 6 deletions(-)
@@ -251,7 +251,7 @@ static int remove_one_path(const char *path){if(!allow_remove)returnerror("%s: does not exist and --remove not passed",path);-if(remove_file_from_cache(path))+if(remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -385,7 +385,7 @@ static int process_path(const char *path, struct stat *st, int stat_errno)*Ontheotherhand,removingitfromindexshouldwork*/if(!ignore_skip_worktree_entries&&allow_remove&&-remove_file_from_cache(path))+remove_file_from_index(istate,path))returnerror("%s: cannot remove from the index",path);return0;}
@@ -555,7 +555,7 @@ static void read_index_info(int nul_term_line)if(!mode){/* mode == 0 means there is no such path -- remove */-if(remove_file_from_cache(path_name))+if(remove_file_from_index(istate,path_name))die("git update-index: unable to remove %s",ptr);}
@@ -671,7 +671,7 @@ static int unresolve_one(const char *path)gotofree_return;}-remove_file_from_cache(path);+remove_file_from_index(repo->index,path);if(add_cache_entry(ce_2,ADD_CACHE_OK_TO_ADD)){error("%s: cannot add our version to the index.",path);ret=-1;
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:04:51
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We can continue reducing the need for these
static globals by modifying method prototypes to use them when
necessary.
Move the remaining 'struct repository *repo' further down the file and
use method parameters to pass it around instead.
The only remaining change is to remove the static global entirely, but
that requires updating the parse-opt callbacks, which need a different
solution.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
@@ -719,13 +721,14 @@ static int do_unresolve(int ac, const char **av,for(i=1;i<ac;i++){constchar*arg=av[i];char*p=prefix_path(prefix,prefix_length,arg);-err|=unresolve_one(p);+err|=unresolve_one(repo,p);free(p);}returnerr;}-staticintdo_reupdate(intac,constchar**av,+staticintdo_reupdate(structrepository*repo,+intac,constchar**av,constchar*prefix){/* Read HEAD and run update-index on paths that are
From: Derrick Stolee via GitGitGadget <hidden> Date: 2021-01-08 20:05:20
From: Derrick Stolee <redacted>
In order to remove index compatibility macros cleanly, we relied upon
static globals 'repo' and 'istate' to be pointers to the_repository and
the_index, respectively. We remove these static globals inside the
option parsing callbacks, which are the final uses in update-index.
The callbacks cannot change their method signature, so we must use the
value member of 'struct option', assigned in the array of option macros.
There are several callback methods that require at least one of 'repo'
and 'istate', but they use a variety of different data types for the
callback value.
Unify these callback methods to use a consistent 'struct callback_data'
that contains a 'repo' member, ready to use. This takes the place of
the previous 'struct refresh_params' which served only to group the
'flags' and 'has_errors' ints. We also collect other one-off settings,
but only those that require access to the index or repository in their
operation.
Signed-off-by: Derrick Stolee <redacted>
---
builtin/update-index.c | 104 ++++++++++++++++++++++-------------------
1 file changed, 56 insertions(+), 48 deletions(-)
@@ -896,7 +900,7 @@ static enum parse_opt_result stdin_cacheinfo_callback(if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);allow_add=allow_replace=allow_remove=1;-read_index_info(repo->index,*nul_term_line);+read_index_info(cd->repo->index,cd->nul_term_line);return0;}
@@ -904,14 +908,14 @@ static enum parse_opt_result stdin_callback(structparse_opt_ctx_t*ctx,conststructoption*opt,constchar*arg,intunset){-int*read_from_stdin=opt->value;+structcallback_data*cd=opt->value;BUG_ON_OPT_NEG(unset);BUG_ON_OPT_ARG(arg);if(ctx->argc!=1)returnerror("option '%s' must be the last argument",opt->long_name);-*read_from_stdin=1;+cd->read_from_stdin=1;return0;}
@@ -974,11 +978,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)structparse_opt_ctx_tctx;strbuf_getline_fngetline_fn;intparseopt_state=PARSE_OPT_UNKNOWN;+structoptionoptions[]={-OPT_BIT('q',NULL,&refresh_args.flags,+OPT_BIT('q',NULL,&cd.flags,N_("continue refresh even when index needs update"),REFRESH_QUIET),-OPT_BIT(0,"ignore-submodules",&refresh_args.flags,+OPT_BIT(0,"ignore-submodules",&cd.flags,N_("refresh: ignore submodules"),REFRESH_IGNORE_SUBMODULES),OPT_SET_INT(0,"add",&allow_add,
@@ -987,18 +992,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)N_("let files replace directories and vice-versa"),1),OPT_SET_INT(0,"remove",&allow_remove,N_("notice files missing from worktree"),1),-OPT_BIT(0,"unmerged",&refresh_args.flags,+OPT_BIT(0,"unmerged",&cd.flags,N_("refresh even if index contains unmerged entries"),REFRESH_UNMERGED),-OPT_CALLBACK_F(0,"refresh",&refresh_args,NULL,+OPT_CALLBACK_F(0,"refresh",&cd,NULL,N_("refresh stat information"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,refresh_callback),-OPT_CALLBACK_F(0,"really-refresh",&refresh_args,NULL,+OPT_CALLBACK_F(0,"really-refresh",&cd,NULL,N_("like --refresh, but ignore assume-unchanged setting"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,really_refresh_callback),-{OPTION_LOWLEVEL_CALLBACK,0,"cacheinfo",NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"cacheinfo",&cd,N_("<mode>,<object>,<path>"),N_("add the specified entry to the index"),PARSE_OPT_NOARG|/* disallow --cacheinfo=<mode> form */
@@ -1027,30 +1032,30 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)N_("add to index only; do not add content to object database"),1),OPT_SET_INT(0,"force-remove",&force_remove,N_("remove named paths even if present in worktree"),1),-OPT_BOOL('z',NULL,&nul_term_line,+OPT_BOOL('z',NULL,&cd.nul_term_line,N_("with --stdin: input lines are terminated by null bytes")),-{OPTION_LOWLEVEL_CALLBACK,0,"stdin",&read_from_stdin,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"stdin",&cd,NULL,N_("read list of paths to be updated from standard input"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,stdin_callback},-{OPTION_LOWLEVEL_CALLBACK,0,"index-info",&nul_term_line,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"index-info",&cd,NULL,N_("add entries from standard input to the index"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,stdin_cacheinfo_callback},-{OPTION_LOWLEVEL_CALLBACK,0,"unresolve",&has_errors,NULL,+{OPTION_LOWLEVEL_CALLBACK,0,"unresolve",&cd,NULL,N_("repopulate stages #2 and #3 for the listed paths"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,unresolve_callback},-{OPTION_LOWLEVEL_CALLBACK,'g',"again",&has_errors,NULL,+{OPTION_LOWLEVEL_CALLBACK,'g',"again",&cd,NULL,N_("only update entries that differ from HEAD"),PARSE_OPT_NONEG|PARSE_OPT_NOARG,NULL,0,reupdate_callback},-OPT_BIT(0,"ignore-missing",&refresh_args.flags,+OPT_BIT(0,"ignore-missing",&cd.flags,N_("ignore files missing from worktree"),REFRESH_IGNORE_MISSING),OPT_SET_INT(0,"verbose",&verbose,N_("report actions to standard output"),1),-OPT_CALLBACK_F(0,"clear-resolve-undo",NULL,NULL,+OPT_CALLBACK_F(0,"clear-resolve-undo",&cd,NULL,N_("(for porcelains) forget saved unresolved conflicts"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,resolve_undo_clear_callback),
@@ -1082,8 +1087,6 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)git_config(git_default_config,NULL);-repo=the_repository;-/* we will diagnose later if it turns out that we need to update it */newfd=repo_hold_locked_index(repo,&lock_file,0);if(newfd<0)