This is based on a previous series I sent[1] which got a bunch of reviews at the
time from Christian, Danh, Junio, Rafael and Eric.
This series only includes changes pertaining to the 'add-clone' subcommand, and
the patches have remained the same as before, except for the inclusion of a
custom die routine 'submodule_die()' that makes all the die()'s behave identical
to the shell version of 'die'. I am particularly interested in opinions about
this change.
A test has been added to ensure no regressions are introduced during the
conversion.
I am holding on to a full conversion of submodule add[2], and sending only a
portion of it, so that the parts that already have been seen before on the list
and refined can be merged. I then plan to send the rest of the patches that
build on top of this series.
You can fetch this series from: https://github.com/tfidfwastaken/git.git
[1]: https://lore.kernel.org/git/20210615145745.33382-1-raykar.ath@gmail.com/
[2]: https://github.com/tfidfwastaken/git/commits/submodule-helper-add-6
Atharva Raykar (3):
t7400: test failure to add submodule in tracked path
submodule--helper: refactor module_clone()
submodule--helper: introduce add-clone subcommand
builtin/submodule--helper.c | 428 ++++++++++++++++++++++++++----------
git-submodule.sh | 38 +---
t/t7400-submodule-basic.sh | 11 +
3 files changed, 327 insertions(+), 150 deletions(-)
--
2.32.0
Add a test to ensure failure on adding a submodule to a directory with
tracked contents in the index.
As we are going to refactor and port to C some parts of `git submodule
add`, let's add a test to help ensure no regression is introduced.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
---
t/t7400-submodule-basic.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' ')'+test_expect_success'submodule add to path with tracked content fails''+(+cdaddtest&&+echo"'\''dir-tracked'\'' already exists in the index">expect&&+mkdirdir-tracked&&+test_commitfoodir-tracked/bar&&+test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&+test_cmpexpectactual+)+'+ test_expect_success'submodule add to reconfigure existing submodule with --force''(cdaddtest-ignore&&
Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Suggested-by: Junio C Hamano <redacted>
---
builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
1 file changed, 128 insertions(+), 113 deletions(-)
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,free(error_strategy);}+staticintclone_submodule(structmodule_clone_data*clone_data)+{+char*p,*sm_gitdir;+char*sm_alternate=NULL,*error_strategy=NULL;+structstrbufsb=STRBUF_INIT;+structchild_processcp=CHILD_PROCESS_INIT;++strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),clone_data->name);+sm_gitdir=absolute_pathdup(sb.buf);+strbuf_reset(&sb);++if(!is_absolute_path(clone_data->path)){+strbuf_addf(&sb,"%s/%s",get_git_work_tree(),clone_data->path);+clone_data->path=strbuf_detach(&sb,NULL);+}else{+clone_data->path=xstrdup(clone_data->path);+}++if(validate_submodule_git_dir(sm_gitdir,clone_data->name)<0)+die(_("refusing to create/use '%s' in another submodule's "+"git dir"),sm_gitdir);++if(!file_exists(sm_gitdir)){+if(safe_create_leading_directories_const(sm_gitdir)<0)+die(_("could not create directory '%s'"),sm_gitdir);++prepare_possible_alternates(clone_data->name,&clone_data->reference);++strvec_push(&cp.args,"clone");+strvec_push(&cp.args,"--no-checkout");+if(clone_data->quiet)+strvec_push(&cp.args,"--quiet");+if(clone_data->progress)+strvec_push(&cp.args,"--progress");+if(clone_data->depth&&*(clone_data->depth))+strvec_pushl(&cp.args,"--depth",clone_data->depth,NULL);+if(clone_data->reference.nr){+structstring_list_item*item;+for_each_string_list_item(item,&clone_data->reference)+strvec_pushl(&cp.args,"--reference",+item->string,NULL);+}+if(clone_data->dissociate)+strvec_push(&cp.args,"--dissociate");+if(sm_gitdir&&*sm_gitdir)+strvec_pushl(&cp.args,"--separate-git-dir",sm_gitdir,NULL);+if(clone_data->single_branch>=0)+strvec_push(&cp.args,clone_data->single_branch?+"--single-branch":+"--no-single-branch");++strvec_push(&cp.args,"--");+strvec_push(&cp.args,clone_data->url);+strvec_push(&cp.args,clone_data->path);++cp.git_cmd=1;+prepare_submodule_repo_env(&cp.env_array);+cp.no_stdin=1;++if(run_command(&cp))+die(_("clone of '%s' into submodule path '%s' failed"),+clone_data->url,clone_data->path);+}else{+if(clone_data->require_init&&!access(clone_data->path,X_OK)&&+!is_empty_dir(clone_data->path))+die(_("directory not empty: '%s'"),clone_data->path);+if(safe_create_leading_directories_const(clone_data->path)<0)+die(_("could not create directory '%s'"),clone_data->path);+strbuf_addf(&sb,"%s/index",sm_gitdir);+unlink_or_warn(sb.buf);+strbuf_reset(&sb);+}++connect_work_tree_and_git_dir(clone_data->path,sm_gitdir,0);++p=git_pathdup_submodule(clone_data->path,"config");+if(!p)+die(_("could not get submodule directory for '%s'"),clone_data->path);++/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */+git_config_get_string("submodule.alternateLocation",&sm_alternate);+if(sm_alternate)+git_config_set_in_file(p,"submodule.alternateLocation",+sm_alternate);+git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);+if(error_strategy)+git_config_set_in_file(p,"submodule.alternateErrorStrategy",+error_strategy);++free(sm_alternate);+free(error_strategy);++strbuf_release(&sb);+free(sm_gitdir);+free(p);+return0;+}+staticintmodule_clone(intargc,constchar**argv,constchar*prefix){-constchar*name=NULL,*url=NULL,*depth=NULL;-intquiet=0;-intprogress=0;-char*p,*path=NULL,*sm_gitdir;-structstrbufsb=STRBUF_INIT;-structstring_listreference=STRING_LIST_INIT_NODUP;-intdissociate=0,require_init=0;-char*sm_alternate=NULL,*error_strategy=NULL;-intsingle_branch=-1;+intdissociate=0,quiet=0,progress=0,require_init=0;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;structoptionmodule_clone_options[]={-OPT_STRING(0,"prefix",&prefix,+OPT_STRING(0,"prefix",&clone_data.prefix,N_("path"),N_("alternative anchor for relative paths")),-OPT_STRING(0,"path",&path,+OPT_STRING(0,"path",&clone_data.path,N_("path"),N_("where the new submodule will be cloned to")),-OPT_STRING(0,"name",&name,+OPT_STRING(0,"name",&clone_data.name,N_("string"),N_("name of the new submodule")),-OPT_STRING(0,"url",&url,+OPT_STRING(0,"url",&clone_data.url,N_("string"),N_("url where to clone the submodule from")),-OPT_STRING_LIST(0,"reference",&reference,+OPT_STRING_LIST(0,"reference",&clone_data.reference,N_("repo"),N_("reference repository")),OPT_BOOL(0,"dissociate",&dissociate,N_("use --reference only while cloning")),-OPT_STRING(0,"depth",&depth,+OPT_STRING(0,"depth",&clone_data.depth,N_("string"),N_("depth for shallow clones")),OPT__QUIET(&quiet,"Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)N_("force cloning progress")),OPT_BOOL(0,"require-init",&require_init,N_("disallow cloning into non-empty directory")),-OPT_BOOL(0,"single-branch",&single_branch,+OPT_BOOL(0,"single-branch",&clone_data.single_branch,N_("clone only one branch, HEAD or --branch")),OPT_END()};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,module_clone_options,git_submodule_helper_usage,0);-if(argc||!url||!path||!*path)+clone_data.dissociate=!!dissociate;+clone_data.quiet=!!quiet;+clone_data.progress=!!progress;+clone_data.require_init=!!require_init;++if(argc||!clone_data.url||!clone_data.path||!*(clone_data.path))usage_with_options(git_submodule_helper_usage,module_clone_options);-strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),name);-sm_gitdir=absolute_pathdup(sb.buf);-strbuf_reset(&sb);--if(!is_absolute_path(path)){-strbuf_addf(&sb,"%s/%s",get_git_work_tree(),path);-path=strbuf_detach(&sb,NULL);-}else-path=xstrdup(path);--if(validate_submodule_git_dir(sm_gitdir,name)<0)-die(_("refusing to create/use '%s' in another submodule's "-"git dir"),sm_gitdir);--if(!file_exists(sm_gitdir)){-if(safe_create_leading_directories_const(sm_gitdir)<0)-die(_("could not create directory '%s'"),sm_gitdir);--prepare_possible_alternates(name,&reference);--if(clone_submodule(path,sm_gitdir,url,depth,&reference,dissociate,-quiet,progress,single_branch))-die(_("clone of '%s' into submodule path '%s' failed"),-url,path);-}else{-if(require_init&&!access(path,X_OK)&&!is_empty_dir(path))-die(_("directory not empty: '%s'"),path);-if(safe_create_leading_directories_const(path)<0)-die(_("could not create directory '%s'"),path);-strbuf_addf(&sb,"%s/index",sm_gitdir);-unlink_or_warn(sb.buf);-strbuf_reset(&sb);-}--connect_work_tree_and_git_dir(path,sm_gitdir,0);--p=git_pathdup_submodule(path,"config");-if(!p)-die(_("could not get submodule directory for '%s'"),path);--/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */-git_config_get_string("submodule.alternateLocation",&sm_alternate);-if(sm_alternate)-git_config_set_in_file(p,"submodule.alternateLocation",-sm_alternate);-git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);-if(error_strategy)-git_config_set_in_file(p,"submodule.alternateErrorStrategy",-error_strategy);--free(sm_alternate);-free(error_strategy);--strbuf_release(&sb);-free(sm_gitdir);-free(path);-free(p);+clone_submodule(&clone_data);return0;}
Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.
This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged.
The 'die' that is used in git-submodule.sh is not the same as the
'die()' in C--the latter prefixes with 'fatal:' and exits with an error
code of 128, while the shell die exits with code 1.
Introduce a custom die routine, that can be used by converted
subcommands to emulate the shell 'die'.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Prathamesh Chavan [off-list ref]
Helped-by: Đoàn Trần Công Danh [off-list ref]
---
builtin/submodule--helper.c | 187 ++++++++++++++++++++++++++++++++++++
git-submodule.sh | 38 +-------
2 files changed, 188 insertions(+), 37 deletions(-)
@@ -2760,6 +2768,184 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)return!!ret;}+structadd_data{+constchar*prefix;+constchar*branch;+constchar*reference_path;+constchar*sm_path;+constchar*sm_name;+constchar*repo;+constchar*realrepo;+intdepth;+unsignedintforce:1;+unsignedintquiet:1;+unsignedintprogress:1;+unsignedintdissociate:1;+};+#define ADD_DATA_INIT { .depth = -1 }++staticvoidshow_fetch_remotes(FILE*output,constchar*sm_name,constchar*git_dir_path)+{+structchild_processcp_remote=CHILD_PROCESS_INIT;+structstrbufsb_remote_out=STRBUF_INIT;++cp_remote.git_cmd=1;+strvec_pushf(&cp_remote.env_array,+"GIT_DIR=%s",git_dir_path);+strvec_push(&cp_remote.env_array,"GIT_WORK_TREE=.");+strvec_pushl(&cp_remote.args,"remote","-v",NULL);+if(!capture_command(&cp_remote,&sb_remote_out,0)){+char*next_line;+char*line=sb_remote_out.buf;+while((next_line=strchr(line,'\n'))!=NULL){+size_tlen=next_line-line;+if(strip_suffix_mem(line,&len," (fetch)"))+fprintf(output," %.*s\n",(int)len,line);+line=next_line+1;+}+}++strbuf_release(&sb_remote_out);+}++staticintadd_submodule(conststructadd_data*add_data)+{+char*submod_gitdir_path;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;++/* perhaps the path already exists and is already a git repo, else clone it */+if(is_directory(add_data->sm_path)){+structstrbufsm_path=STRBUF_INIT;+strbuf_addstr(&sm_path,add_data->sm_path);+submod_gitdir_path=xstrfmt("%s/.git",add_data->sm_path);+if(is_nonbare_repository_dir(&sm_path))+printf(_("Adding existing repo at '%s' to the index\n"),+add_data->sm_path);+else+die(_("'%s' already exists and is not a valid git repo"),+add_data->sm_path);+strbuf_release(&sm_path);+free(submod_gitdir_path);+}else{+structchild_processcp=CHILD_PROCESS_INIT;+submod_gitdir_path=xstrfmt(".git/modules/%s",add_data->sm_name);++if(is_directory(submod_gitdir_path)){+if(!add_data->force){+fprintf(stderr,_("A git directory for '%s' is found "+"locally with remote(s):"),+add_data->sm_name);+show_fetch_remotes(stderr,add_data->sm_name,+submod_gitdir_path);+free(submod_gitdir_path);+die(_("If you want to reuse this local git "+"directory instead of cloning again from\n"+" %s\n"+"use the '--force' option. If the local git "+"directory is not the correct repo\n"+"or if you are unsure what this means, choose "+"another name with the '--name' option.\n"),+add_data->realrepo);+}else{+printf(_("Reactivating local git directory for "+"submodule '%s'\n"),add_data->sm_name);+}+}+free(submod_gitdir_path);++clone_data.prefix=add_data->prefix;+clone_data.path=add_data->sm_path;+clone_data.name=add_data->sm_name;+clone_data.url=add_data->realrepo;+clone_data.quiet=add_data->quiet;+clone_data.progress=add_data->progress;+if(add_data->reference_path)+string_list_append(&clone_data.reference,+xstrdup(add_data->reference_path));+clone_data.dissociate=add_data->dissociate;+if(add_data->depth>=0)+clone_data.depth=xstrfmt("%d",add_data->depth);++if(clone_submodule(&clone_data))+return-1;++prepare_submodule_repo_env(&cp.env_array);+cp.git_cmd=1;+cp.dir=add_data->sm_path;+strvec_pushl(&cp.args,"checkout","-f","-q",NULL);++if(add_data->branch){+strvec_pushl(&cp.args,"-B",add_data->branch,NULL);+strvec_pushf(&cp.args,"origin/%s",add_data->branch);+}++if(run_command(&cp))+die(_("unable to checkout submodule '%s'"),add_data->sm_path);+}+return0;+}++staticintadd_clone(intargc,constchar**argv,constchar*prefix)+{+intforce=0,quiet=0,dissociate=0,progress=0;+structadd_dataadd_data=ADD_DATA_INIT;++structoptionoptions[]={+OPT_STRING('b',"branch",&add_data.branch,+N_("branch"),+N_("branch of repository to checkout on cloning")),+OPT_STRING(0,"prefix",&prefix,+N_("path"),+N_("alternative anchor for relative paths")),+OPT_STRING(0,"path",&add_data.sm_path,+N_("path"),+N_("where the new submodule will be cloned to")),+OPT_STRING(0,"name",&add_data.sm_name,+N_("string"),+N_("name of the new submodule")),+OPT_STRING(0,"url",&add_data.realrepo,+N_("string"),+N_("url where to clone the submodule from")),+OPT_STRING(0,"reference",&add_data.reference_path,+N_("repo"),+N_("reference repository")),+OPT_BOOL(0,"dissociate",&dissociate,+N_("use --reference only while cloning")),+OPT_INTEGER(0,"depth",&add_data.depth,+N_("depth for shallow clones")),+OPT_BOOL(0,"progress",&progress,+N_("force cloning progress")),+OPT__FORCE(&force,N_("allow adding an otherwise ignored submodule path"),+PARSE_OPT_NOCOMPLETE),+OPT__QUIET(&quiet,"suppress output for cloning a submodule"),+OPT_END()+};++constchar*constusage[]={+N_("git submodule--helper add-clone [<options>...] "+"--url <url> --path <path> --name <name>"),+NULL+};++argc=parse_options(argc,argv,prefix,options,usage,0);++if(argc!=0)+usage_with_options(usage,options);++add_data.prefix=prefix;+add_data.progress=!!progress;+add_data.dissociate=!!dissociate;+add_data.force=!!force;+add_data.quiet=!!quiet;++set_die_routine(submodule_die);++if(add_submodule(&add_data))+return1;++return0;+}+#define SUPPORT_SUPER_PREFIX (1<<0)structcmd_struct{
@@ -241,43 +241,7 @@ cmd_add()die"$(eval_gettext"'$sm_name' is not a valid submodule name")"fi-# perhaps the path exists and is already a git repo, else clone it-iftest-e"$sm_path"-then-iftest-d"$sm_path"/.git||test-f"$sm_path"/.git-then-eval_gettextln"Adding existing repo at '\$sm_path' to the index"-else-die"$(eval_gettext"'\$sm_path' already exists and is not a valid git repo")"-fi--else-iftest-d".git/modules/$sm_name"-then-iftest-z"$force"-then-eval_gettextln>&2"A git directory for '\$sm_name' is found locally with remote(s):"-GIT_DIR=".git/modules/$sm_name"GIT_WORK_TREE=.gitremote-v|grep'(fetch)'|sed-es,^," ",-es,' (fetch)',,>&2-die"$(eval_gettextln"\-Ifyouwanttoreusethislocalgitdirectoryinsteadofcloningagainfrom-\$realrepo-usethe'--force'option.Ifthelocalgitdirectoryisnotthecorrectrepo-oryouareunsurewhatthismeanschooseanothernamewiththe'--name'option.")"-else-eval_gettextln"Reactivating local git directory for submodule '\$sm_name'."-fi-fi-gitsubmodule--helperclone${GIT_QUIET:+--quiet}${progress:+"--progress"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exit-(-sanitize_submodule_env-cd"$sm_path"&&-# ash fails to wordsplit ${branch:+-b "$branch"...}-case"$branch"in-'')gitcheckout-f-q;;-?*)gitcheckout-f-q-B"$branch""origin/$branch";;-esac-)||die"$(eval_gettext"Unable to checkout submodule '\$sm_path'")"-fi+gitsubmodule--helperadd-clone${GIT_QUIET:+--quiet}${force:+"--force"}${progress:+"--progress"}${branch:+--branch "$branch"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exitgitconfigsubmodule."$sm_name".url"$realrepo"gitadd--no-warn-embedded-repo$force"$sm_path"||
Changes since v1:
* A custom die routine is no longer used. Instead, the shell script is changed
to prefix all die messages with 'fatal' to match what the standard 'die()'
function in C will output when converted.
Fetch-It-Via:
git fetch https://github.com/tfidfwastaken/git.git submodule-helper-add-clone-2
Atharva Raykar (4):
t7400: test failure to add submodule in tracked path
submodule: prefix die messages with 'fatal'
submodule--helper: refactor module_clone()
submodule--helper: introduce add-clone subcommand
builtin/submodule--helper.c | 418 ++++++++++++++++++++++++++----------
git-submodule.sh | 76 ++-----
t/t7400-submodule-basic.sh | 13 +-
t/t7406-submodule-update.sh | 10 +-
4 files changed, 342 insertions(+), 175 deletions(-)
--
2.32.0
Add a test to ensure failure on adding a submodule to a directory with
tracked contents in the index.
As we are going to refactor and port to C some parts of `git submodule
add`, let's add a test to help ensure no regression is introduced.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
---
t/t7400-submodule-basic.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' ')'+test_expect_success'submodule add to path with tracked content fails''+(+cdaddtest&&+echo"'\''dir-tracked'\'' already exists in the index">expect&&+mkdirdir-tracked&&+test_commitfoodir-tracked/bar&&+test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&+test_cmpexpectactual+)+'+ test_expect_success'submodule add to reconfigure existing submodule with --force''(cdaddtest-ignore&&
The standard `die()` function that is used in C code prefixes all the
messages passed to it with 'fatal: '. This does not happen with the
`die` used in 'git-submodule.sh'.
Let's prefix each of the shell die messages with 'fatal: ' so that when
they are converted to C code, the error messages stay the same as before
the conversion.
Note that the shell version of `die` exits with error code 1, while the
C version exits with error code 128. In practice, this does not change
any behaviour, as no functionality in 'submodule add' and 'submodule
update' relies on the value of the exit code.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
---
git-submodule.sh | 38 ++++++++++++++++++-------------------
t/t7400-submodule-basic.sh | 4 ++--
t/t7406-submodule-update.sh | 10 +++++-----
3 files changed, 26 insertions(+), 26 deletions(-)
@@ -147,7 +147,7 @@ cmd_add()if!gitsubmodule--helperconfig--check-writeable>/dev/null2>&1then-die"$(eval_gettext"please make sure that the .gitmodules file is in the working tree")"+die"$(eval_gettext"fatal: please make sure that the .gitmodules file is in the working tree")"fiiftest-n"$reference_path"
@@ -176,7 +176,7 @@ cmd_add()case"$repo"in./*|../*)test-z"$wt_prefix"||-die"$(gettext"Relative path can only be used from the toplevel of the working tree")"+die"$(gettext"fatal: Relative path can only be used from the toplevel of the working tree")"# dereference source url relative to parent's urlrealrepo=$(gitsubmodule--helperresolve-relative-url"$repo")||exit
@@ -186,7 +186,7 @@ cmd_add()realrepo=$repo;;*)-die"$(eval_gettext"repo URL: '\$repo' must be absolute or begin with ./|../")"+die"$(eval_gettext"fatal: repo URL: '\$repo' must be absolute or begin with ./|../")";;esac
@@ -205,17 +205,17 @@ cmd_add()iftest-z"$force"thengitls-files--error-unmatch"$sm_path">/dev/null2>&1&&-die"$(eval_gettext"'\$sm_path' already exists in the index")"+die"$(eval_gettext"fatal: '\$sm_path' already exists in the index")"elsegitls-files-s"$sm_path"|sane_grep-v"^160000">/dev/null2>&1&&-die"$(eval_gettext"'\$sm_path' already exists in the index and is not a submodule")"+die"$(eval_gettext"fatal: '\$sm_path' already exists in the index and is not a submodule")"fiiftest-d"$sm_path"&&test-z$(git-C"$sm_path"rev-parse--show-cdup2>/dev/null)thengit-C"$sm_path"rev-parse--verify-qHEAD>/dev/null||-die"$(eval_gettext"'\$sm_path' does not have a commit checked out")"+die"$(eval_gettext"fatal: '\$sm_path' does not have a commit checked out")"fiiftest-z"$force"
@@ -238,7 +238,7 @@ cmd_add()if!gitsubmodule--helpercheck-name"$sm_name"then-die"$(eval_gettext"'$sm_name' is not a valid submodule name")"+die"$(eval_gettext"fatal: '$sm_name' is not a valid submodule name")"fi# perhaps the path exists and is already a git repo, else clone it
@@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option."gitconfigsubmodule."$sm_name".url"$realrepo"gitadd--no-warn-embedded-repo$force"$sm_path"||-die"$(eval_gettext"Failed to add submodule '\$sm_path'")"+die"$(eval_gettext"fatal: Failed to add submodule '\$sm_path'")"gitsubmodule--helperconfigsubmodule."$sm_name".path"$sm_path"&&gitsubmodule--helperconfigsubmodule."$sm_name".url"$repo"&&
@@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option."gitsubmodule--helperconfigsubmodule."$sm_name".branch"$branch"fi&&gitadd--force.gitmodules||-die"$(eval_gettext"Failed to register submodule '\$sm_path'")"+die"$(eval_gettext"fatal: Failed to register submodule '\$sm_path'")"# NEEDSWORK: In a multi-working-tree world, this needs to be# set in the per-worktree config.
@@ -565,7 +565,7 @@ cmd_update()elsesubsha1=$(sanitize_submodule_env;cd"$sm_path"&&gitrev-parse--verifyHEAD)||-die"$(eval_gettext"Unable to find current revision in submodule path '\$displaypath'")"+die"$(eval_gettext"fatal: Unable to find current revision in submodule path '\$displaypath'")"fiiftest-n"$remote"
@@ -575,12 +575,12 @@ cmd_update()then# Fetch remote before determining tracking $sha1fetch_in_submodule"$sm_path"$depth||-die"$(eval_gettext"Unable to fetch in submodule path '\$sm_path'")"+die"$(eval_gettext"fatal: Unable to fetch in submodule path '\$sm_path'")"firemote_name=$(sanitize_submodule_env;cd"$sm_path"&&gitsubmodule--helperprint-default-remote)sha1=$(sanitize_submodule_env;cd"$sm_path"&&gitrev-parse--verify"${remote_name}/${branch}")||-die"$(eval_gettext"Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"+die"$(eval_gettext"fatal: Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"fiiftest"$subsha1"!="$sha1"||test-n"$force"
@@ -604,36 +604,36 @@ cmd_update()# not be reachable from any of the refsis_tip_reachable"$sm_path""$sha1"||fetch_in_submodule"$sm_path""$depth""$sha1"||-die"$(eval_gettext"Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"+die"$(eval_gettext"fatal: Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"fimust_die_on_failure=case"$update_module"incheckout)command="git checkout $subforce -q"-die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+die_msg="$(eval_gettext"fatal: Unable to checkout '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")";;rebase)command="git rebase ${GIT_QUIET:+--quiet}"-die_msg="$(eval_gettext"Unable to rebase '\$sha1' in submodule path '\$displaypath'")"+die_msg="$(eval_gettext"fatal: Unable to rebase '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': rebased into '\$sha1'")"must_die_on_failure=yes;;merge)command="git merge ${GIT_QUIET:+--quiet}"-die_msg="$(eval_gettext"Unable to merge '\$sha1' in submodule path '\$displaypath'")"+die_msg="$(eval_gettext"fatal: Unable to merge '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': merged in '\$sha1'")"must_die_on_failure=yes;;!*)command="${update_module#!}"-die_msg="$(eval_gettext"Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"+die_msg="$(eval_gettext"fatal: Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': '\$command \$sha1'")"must_die_on_failure=yes;;*)-die"$(eval_gettext"Invalid update mode '$update_module' for submodule path '$path'")"+die"$(eval_gettext"fatal: Invalid update mode '$update_module' for submodule path '$path'")"esacif(sanitize_submodule_env;cd"$sm_path"&&$command"$sha1")
@@ -660,7 +660,7 @@ cmd_update()res=$?iftest$res-gt0then-die_msg="$(eval_gettext"Failed to recurse into submodule path '\$displaypath'")"+die_msg="$(eval_gettext"fatal: Failed to recurse into submodule path '\$displaypath'")"iftest$res-ne2thenerr="${err};$die_msg"
@@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' ' test_expect_success'add aborts on repository with no commits''cat>expect<<-\EOF&&-'"'repo-no-commits'"'doesnothaveacommitcheckedout+fatal:'"'repo-no-commits'"'doesnothaveacommitcheckedoutEOFgitinitrepo-no-commits&&test_must_failgitsubmoduleadd../a./repo-no-commits2>actual&&
@@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' ' test_expect_success'submodule add to path with tracked content fails''(cdaddtest&&-echo"'\''dir-tracked'\'' already exists in the index">expect&&+echo"fatal: '\''dir-tracked'\'' already exists in the index">expect&&mkdirdir-tracked&&test_commitfoodir-tracked/bar&&test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&
Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Suggested-by: Junio C Hamano <redacted>
---
builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
1 file changed, 128 insertions(+), 113 deletions(-)
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,free(error_strategy);}+staticintclone_submodule(structmodule_clone_data*clone_data)+{+char*p,*sm_gitdir;+char*sm_alternate=NULL,*error_strategy=NULL;+structstrbufsb=STRBUF_INIT;+structchild_processcp=CHILD_PROCESS_INIT;++strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),clone_data->name);+sm_gitdir=absolute_pathdup(sb.buf);+strbuf_reset(&sb);++if(!is_absolute_path(clone_data->path)){+strbuf_addf(&sb,"%s/%s",get_git_work_tree(),clone_data->path);+clone_data->path=strbuf_detach(&sb,NULL);+}else{+clone_data->path=xstrdup(clone_data->path);+}++if(validate_submodule_git_dir(sm_gitdir,clone_data->name)<0)+die(_("refusing to create/use '%s' in another submodule's "+"git dir"),sm_gitdir);++if(!file_exists(sm_gitdir)){+if(safe_create_leading_directories_const(sm_gitdir)<0)+die(_("could not create directory '%s'"),sm_gitdir);++prepare_possible_alternates(clone_data->name,&clone_data->reference);++strvec_push(&cp.args,"clone");+strvec_push(&cp.args,"--no-checkout");+if(clone_data->quiet)+strvec_push(&cp.args,"--quiet");+if(clone_data->progress)+strvec_push(&cp.args,"--progress");+if(clone_data->depth&&*(clone_data->depth))+strvec_pushl(&cp.args,"--depth",clone_data->depth,NULL);+if(clone_data->reference.nr){+structstring_list_item*item;+for_each_string_list_item(item,&clone_data->reference)+strvec_pushl(&cp.args,"--reference",+item->string,NULL);+}+if(clone_data->dissociate)+strvec_push(&cp.args,"--dissociate");+if(sm_gitdir&&*sm_gitdir)+strvec_pushl(&cp.args,"--separate-git-dir",sm_gitdir,NULL);+if(clone_data->single_branch>=0)+strvec_push(&cp.args,clone_data->single_branch?+"--single-branch":+"--no-single-branch");++strvec_push(&cp.args,"--");+strvec_push(&cp.args,clone_data->url);+strvec_push(&cp.args,clone_data->path);++cp.git_cmd=1;+prepare_submodule_repo_env(&cp.env_array);+cp.no_stdin=1;++if(run_command(&cp))+die(_("clone of '%s' into submodule path '%s' failed"),+clone_data->url,clone_data->path);+}else{+if(clone_data->require_init&&!access(clone_data->path,X_OK)&&+!is_empty_dir(clone_data->path))+die(_("directory not empty: '%s'"),clone_data->path);+if(safe_create_leading_directories_const(clone_data->path)<0)+die(_("could not create directory '%s'"),clone_data->path);+strbuf_addf(&sb,"%s/index",sm_gitdir);+unlink_or_warn(sb.buf);+strbuf_reset(&sb);+}++connect_work_tree_and_git_dir(clone_data->path,sm_gitdir,0);++p=git_pathdup_submodule(clone_data->path,"config");+if(!p)+die(_("could not get submodule directory for '%s'"),clone_data->path);++/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */+git_config_get_string("submodule.alternateLocation",&sm_alternate);+if(sm_alternate)+git_config_set_in_file(p,"submodule.alternateLocation",+sm_alternate);+git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);+if(error_strategy)+git_config_set_in_file(p,"submodule.alternateErrorStrategy",+error_strategy);++free(sm_alternate);+free(error_strategy);++strbuf_release(&sb);+free(sm_gitdir);+free(p);+return0;+}+staticintmodule_clone(intargc,constchar**argv,constchar*prefix){-constchar*name=NULL,*url=NULL,*depth=NULL;-intquiet=0;-intprogress=0;-char*p,*path=NULL,*sm_gitdir;-structstrbufsb=STRBUF_INIT;-structstring_listreference=STRING_LIST_INIT_NODUP;-intdissociate=0,require_init=0;-char*sm_alternate=NULL,*error_strategy=NULL;-intsingle_branch=-1;+intdissociate=0,quiet=0,progress=0,require_init=0;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;structoptionmodule_clone_options[]={-OPT_STRING(0,"prefix",&prefix,+OPT_STRING(0,"prefix",&clone_data.prefix,N_("path"),N_("alternative anchor for relative paths")),-OPT_STRING(0,"path",&path,+OPT_STRING(0,"path",&clone_data.path,N_("path"),N_("where the new submodule will be cloned to")),-OPT_STRING(0,"name",&name,+OPT_STRING(0,"name",&clone_data.name,N_("string"),N_("name of the new submodule")),-OPT_STRING(0,"url",&url,+OPT_STRING(0,"url",&clone_data.url,N_("string"),N_("url where to clone the submodule from")),-OPT_STRING_LIST(0,"reference",&reference,+OPT_STRING_LIST(0,"reference",&clone_data.reference,N_("repo"),N_("reference repository")),OPT_BOOL(0,"dissociate",&dissociate,N_("use --reference only while cloning")),-OPT_STRING(0,"depth",&depth,+OPT_STRING(0,"depth",&clone_data.depth,N_("string"),N_("depth for shallow clones")),OPT__QUIET(&quiet,"Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)N_("force cloning progress")),OPT_BOOL(0,"require-init",&require_init,N_("disallow cloning into non-empty directory")),-OPT_BOOL(0,"single-branch",&single_branch,+OPT_BOOL(0,"single-branch",&clone_data.single_branch,N_("clone only one branch, HEAD or --branch")),OPT_END()};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,module_clone_options,git_submodule_helper_usage,0);-if(argc||!url||!path||!*path)+clone_data.dissociate=!!dissociate;+clone_data.quiet=!!quiet;+clone_data.progress=!!progress;+clone_data.require_init=!!require_init;++if(argc||!clone_data.url||!clone_data.path||!*(clone_data.path))usage_with_options(git_submodule_helper_usage,module_clone_options);-strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),name);-sm_gitdir=absolute_pathdup(sb.buf);-strbuf_reset(&sb);--if(!is_absolute_path(path)){-strbuf_addf(&sb,"%s/%s",get_git_work_tree(),path);-path=strbuf_detach(&sb,NULL);-}else-path=xstrdup(path);--if(validate_submodule_git_dir(sm_gitdir,name)<0)-die(_("refusing to create/use '%s' in another submodule's "-"git dir"),sm_gitdir);--if(!file_exists(sm_gitdir)){-if(safe_create_leading_directories_const(sm_gitdir)<0)-die(_("could not create directory '%s'"),sm_gitdir);--prepare_possible_alternates(name,&reference);--if(clone_submodule(path,sm_gitdir,url,depth,&reference,dissociate,-quiet,progress,single_branch))-die(_("clone of '%s' into submodule path '%s' failed"),-url,path);-}else{-if(require_init&&!access(path,X_OK)&&!is_empty_dir(path))-die(_("directory not empty: '%s'"),path);-if(safe_create_leading_directories_const(path)<0)-die(_("could not create directory '%s'"),path);-strbuf_addf(&sb,"%s/index",sm_gitdir);-unlink_or_warn(sb.buf);-strbuf_reset(&sb);-}--connect_work_tree_and_git_dir(path,sm_gitdir,0);--p=git_pathdup_submodule(path,"config");-if(!p)-die(_("could not get submodule directory for '%s'"),path);--/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */-git_config_get_string("submodule.alternateLocation",&sm_alternate);-if(sm_alternate)-git_config_set_in_file(p,"submodule.alternateLocation",-sm_alternate);-git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);-if(error_strategy)-git_config_set_in_file(p,"submodule.alternateErrorStrategy",-error_strategy);--free(sm_alternate);-free(error_strategy);--strbuf_release(&sb);-free(sm_gitdir);-free(path);-free(p);+clone_submodule(&clone_data);return0;}
Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.
This is meant to be a faithful conversion that leaves the behaviour of
'cmd_add()' script unchanged.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Prathamesh Chavan [off-list ref]
Helped-by: Đoàn Trần Công Danh [off-list ref]
---
builtin/submodule--helper.c | 177 ++++++++++++++++++++++++++++++++++++
git-submodule.sh | 38 +-------
2 files changed, 178 insertions(+), 37 deletions(-)
@@ -2760,6 +2760,182 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)return!!ret;}+structadd_data{+constchar*prefix;+constchar*branch;+constchar*reference_path;+constchar*sm_path;+constchar*sm_name;+constchar*repo;+constchar*realrepo;+intdepth;+unsignedintforce:1;+unsignedintquiet:1;+unsignedintprogress:1;+unsignedintdissociate:1;+};+#define ADD_DATA_INIT { .depth = -1 }++staticvoidshow_fetch_remotes(FILE*output,constchar*sm_name,constchar*git_dir_path)+{+structchild_processcp_remote=CHILD_PROCESS_INIT;+structstrbufsb_remote_out=STRBUF_INIT;++cp_remote.git_cmd=1;+strvec_pushf(&cp_remote.env_array,+"GIT_DIR=%s",git_dir_path);+strvec_push(&cp_remote.env_array,"GIT_WORK_TREE=.");+strvec_pushl(&cp_remote.args,"remote","-v",NULL);+if(!capture_command(&cp_remote,&sb_remote_out,0)){+char*next_line;+char*line=sb_remote_out.buf;+while((next_line=strchr(line,'\n'))!=NULL){+size_tlen=next_line-line;+if(strip_suffix_mem(line,&len," (fetch)"))+fprintf(output," %.*s\n",(int)len,line);+line=next_line+1;+}+}++strbuf_release(&sb_remote_out);+}++staticintadd_submodule(conststructadd_data*add_data)+{+char*submod_gitdir_path;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;++/* perhaps the path already exists and is already a git repo, else clone it */+if(is_directory(add_data->sm_path)){+structstrbufsm_path=STRBUF_INIT;+strbuf_addstr(&sm_path,add_data->sm_path);+submod_gitdir_path=xstrfmt("%s/.git",add_data->sm_path);+if(is_nonbare_repository_dir(&sm_path))+printf(_("Adding existing repo at '%s' to the index\n"),+add_data->sm_path);+else+die(_("'%s' already exists and is not a valid git repo"),+add_data->sm_path);+strbuf_release(&sm_path);+free(submod_gitdir_path);+}else{+structchild_processcp=CHILD_PROCESS_INIT;+submod_gitdir_path=xstrfmt(".git/modules/%s",add_data->sm_name);++if(is_directory(submod_gitdir_path)){+if(!add_data->force){+fprintf(stderr,_("A git directory for '%s' is found "+"locally with remote(s):"),+add_data->sm_name);+show_fetch_remotes(stderr,add_data->sm_name,+submod_gitdir_path);+free(submod_gitdir_path);+die(_("If you want to reuse this local git "+"directory instead of cloning again from\n"+" %s\n"+"use the '--force' option. If the local git "+"directory is not the correct repo\n"+"or if you are unsure what this means, choose "+"another name with the '--name' option.\n"),+add_data->realrepo);+}else{+printf(_("Reactivating local git directory for "+"submodule '%s'\n"),add_data->sm_name);+}+}+free(submod_gitdir_path);++clone_data.prefix=add_data->prefix;+clone_data.path=add_data->sm_path;+clone_data.name=add_data->sm_name;+clone_data.url=add_data->realrepo;+clone_data.quiet=add_data->quiet;+clone_data.progress=add_data->progress;+if(add_data->reference_path)+string_list_append(&clone_data.reference,+xstrdup(add_data->reference_path));+clone_data.dissociate=add_data->dissociate;+if(add_data->depth>=0)+clone_data.depth=xstrfmt("%d",add_data->depth);++if(clone_submodule(&clone_data))+return-1;++prepare_submodule_repo_env(&cp.env_array);+cp.git_cmd=1;+cp.dir=add_data->sm_path;+strvec_pushl(&cp.args,"checkout","-f","-q",NULL);++if(add_data->branch){+strvec_pushl(&cp.args,"-B",add_data->branch,NULL);+strvec_pushf(&cp.args,"origin/%s",add_data->branch);+}++if(run_command(&cp))+die(_("unable to checkout submodule '%s'"),add_data->sm_path);+}+return0;+}++staticintadd_clone(intargc,constchar**argv,constchar*prefix)+{+intforce=0,quiet=0,dissociate=0,progress=0;+structadd_dataadd_data=ADD_DATA_INIT;++structoptionoptions[]={+OPT_STRING('b',"branch",&add_data.branch,+N_("branch"),+N_("branch of repository to checkout on cloning")),+OPT_STRING(0,"prefix",&prefix,+N_("path"),+N_("alternative anchor for relative paths")),+OPT_STRING(0,"path",&add_data.sm_path,+N_("path"),+N_("where the new submodule will be cloned to")),+OPT_STRING(0,"name",&add_data.sm_name,+N_("string"),+N_("name of the new submodule")),+OPT_STRING(0,"url",&add_data.realrepo,+N_("string"),+N_("url where to clone the submodule from")),+OPT_STRING(0,"reference",&add_data.reference_path,+N_("repo"),+N_("reference repository")),+OPT_BOOL(0,"dissociate",&dissociate,+N_("use --reference only while cloning")),+OPT_INTEGER(0,"depth",&add_data.depth,+N_("depth for shallow clones")),+OPT_BOOL(0,"progress",&progress,+N_("force cloning progress")),+OPT__FORCE(&force,N_("allow adding an otherwise ignored submodule path"),+PARSE_OPT_NOCOMPLETE),+OPT__QUIET(&quiet,"suppress output for cloning a submodule"),+OPT_END()+};++constchar*constusage[]={+N_("git submodule--helper add-clone [<options>...] "+"--url <url> --path <path> --name <name>"),+NULL+};++argc=parse_options(argc,argv,prefix,options,usage,0);++if(argc!=0)+usage_with_options(usage,options);++add_data.prefix=prefix;+add_data.progress=!!progress;+add_data.dissociate=!!dissociate;+add_data.force=!!force;+add_data.quiet=!!quiet;++if(add_submodule(&add_data))+return1;++return0;+}+#define SUPPORT_SUPER_PREFIX (1<<0)structcmd_struct{
@@ -241,43 +241,7 @@ cmd_add()die"$(eval_gettext"fatal: '$sm_name' is not a valid submodule name")"fi-# perhaps the path exists and is already a git repo, else clone it-iftest-e"$sm_path"-then-iftest-d"$sm_path"/.git||test-f"$sm_path"/.git-then-eval_gettextln"Adding existing repo at '\$sm_path' to the index"-else-die"$(eval_gettext"'\$sm_path' already exists and is not a valid git repo")"-fi--else-iftest-d".git/modules/$sm_name"-then-iftest-z"$force"-then-eval_gettextln>&2"A git directory for '\$sm_name' is found locally with remote(s):"-GIT_DIR=".git/modules/$sm_name"GIT_WORK_TREE=.gitremote-v|grep'(fetch)'|sed-es,^," ",-es,' (fetch)',,>&2-die"$(eval_gettextln"\-Ifyouwanttoreusethislocalgitdirectoryinsteadofcloningagainfrom-\$realrepo-usethe'--force'option.Ifthelocalgitdirectoryisnotthecorrectrepo-oryouareunsurewhatthismeanschooseanothernamewiththe'--name'option.")"-else-eval_gettextln"Reactivating local git directory for submodule '\$sm_name'."-fi-fi-gitsubmodule--helperclone${GIT_QUIET:+--quiet}${progress:+"--progress"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exit-(-sanitize_submodule_env-cd"$sm_path"&&-# ash fails to wordsplit ${branch:+-b "$branch"...}-case"$branch"in-'')gitcheckout-f-q;;-?*)gitcheckout-f-q-B"$branch""origin/$branch";;-esac-)||die"$(eval_gettext"Unable to checkout submodule '\$sm_path'")"-fi+gitsubmodule--helperadd-clone${GIT_QUIET:+--quiet}${force:+"--force"}${progress:+"--progress"}${branch:+--branch "$branch"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exitgitconfigsubmodule."$sm_name".url"$realrepo"gitadd--no-warn-embedded-repo$force"$sm_path"||
From: Đoàn Trần Công Danh <hidden> Date: 2021-07-09 14:52:36
On 2021-07-08 15:25:31+0530, Atharva Raykar [off-list ref] wrote:
The standard `die()` function that is used in C code prefixes all the
messages passed to it with 'fatal: '. This does not happen with the
`die` used in 'git-submodule.sh'.
Let's prefix each of the shell die messages with 'fatal: ' so that when
they are converted to C code, the error messages stay the same as before
the conversion.
That sounds good.
quoted hunk
--- a/git-submodule.sh+++ b/git-submodule.sh
@@ -147,7 +147,7 @@ cmd_add()if!gitsubmodule--helperconfig--check-writeable>/dev/null2>&1then-die"$(eval_gettext"please make sure that the .gitmodules file is in the working tree")"+die"$(eval_gettext"fatal: please make sure that the .gitmodules file is in the working tree")"
Except that, "fatal: " isn't subjected to translation. And this will
create new translatable item for translator. Perhaps:
- die "$(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
+ die "fatal: $(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
-- Danh
quoted hunk
fi
if test -n "$reference_path"
@@ -176,7 +176,7 @@ cmd_add() case "$repo" in ./*|../*) test -z "$wt_prefix" ||- die "$(gettext "Relative path can only be used from the toplevel of the working tree")"+ die "$(gettext "fatal: Relative path can only be used from the toplevel of the working tree")" # dereference source url relative to parent's url realrepo=$(git submodule--helper resolve-relative-url "$repo") || exit
@@ -186,7 +186,7 @@ cmd_add() realrepo=$repo ;; *)- die "$(eval_gettext "repo URL: '\$repo' must be absolute or begin with ./|../")"+ die "$(eval_gettext "fatal: repo URL: '\$repo' must be absolute or begin with ./|../")" ;; esac
@@ -205,17 +205,17 @@ cmd_add() if test -z "$force" then git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&- die "$(eval_gettext "'\$sm_path' already exists in the index")"+ die "$(eval_gettext "fatal: '\$sm_path' already exists in the index")" else git ls-files -s "$sm_path" | sane_grep -v "^160000" > /dev/null 2>&1 &&- die "$(eval_gettext "'\$sm_path' already exists in the index and is not a submodule")"+ die "$(eval_gettext "fatal: '\$sm_path' already exists in the index and is not a submodule")" fi if test -d "$sm_path" && test -z $(git -C "$sm_path" rev-parse --show-cdup 2>/dev/null) then git -C "$sm_path" rev-parse --verify -q HEAD >/dev/null ||- die "$(eval_gettext "'\$sm_path' does not have a commit checked out")"+ die "$(eval_gettext "fatal: '\$sm_path' does not have a commit checked out")" fi if test -z "$force"
@@ -238,7 +238,7 @@ cmd_add() if ! git submodule--helper check-name "$sm_name" then- die "$(eval_gettext "'$sm_name' is not a valid submodule name")"+ die "$(eval_gettext "fatal: '$sm_name' is not a valid submodule name")" fi # perhaps the path exists and is already a git repo, else clone it
@@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option." git config submodule."$sm_name".url "$realrepo" git add --no-warn-embedded-repo $force "$sm_path" ||- die "$(eval_gettext "Failed to add submodule '\$sm_path'")"+ die "$(eval_gettext "fatal: Failed to add submodule '\$sm_path'")" git submodule--helper config submodule."$sm_name".path "$sm_path" && git submodule--helper config submodule."$sm_name".url "$repo" &&
@@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option." git submodule--helper config submodule."$sm_name".branch "$branch" fi && git add --force .gitmodules ||- die "$(eval_gettext "Failed to register submodule '\$sm_path'")"+ die "$(eval_gettext "fatal: Failed to register submodule '\$sm_path'")" # NEEDSWORK: In a multi-working-tree world, this needs to be # set in the per-worktree config.
@@ -565,7 +565,7 @@ cmd_update() else subsha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD) ||- die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"+ die "$(eval_gettext "fatal: Unable to find current revision in submodule path '\$displaypath'")" fi if test -n "$remote"
@@ -575,12 +575,12 @@ cmd_update() then # Fetch remote before determining tracking $sha1 fetch_in_submodule "$sm_path" $depth ||- die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"+ die "$(eval_gettext "fatal: Unable to fetch in submodule path '\$sm_path'")" fi remote_name=$(sanitize_submodule_env; cd "$sm_path" && git submodule--helper print-default-remote) sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify "${remote_name}/${branch}") ||- die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"+ die "$(eval_gettext "fatal: Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" fi if test "$subsha1" != "$sha1" || test -n "$force"
@@ -604,36 +604,36 @@ cmd_update() # not be reachable from any of the refs is_tip_reachable "$sm_path" "$sha1" || fetch_in_submodule "$sm_path" "$depth" "$sha1" ||- die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"+ die "$(eval_gettext "fatal: Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")" fi must_die_on_failure= case "$update_module" in checkout) command="git checkout $subforce -q"- die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+ die_msg="$(eval_gettext "fatal: Unable to checkout '\$sha1' in submodule path '\$displaypath'")" say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")" ;; rebase) command="git rebase ${GIT_QUIET:+--quiet}"- die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")"+ die_msg="$(eval_gettext "fatal: Unable to rebase '\$sha1' in submodule path '\$displaypath'")" say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")" must_die_on_failure=yes ;; merge) command="git merge ${GIT_QUIET:+--quiet}"- die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")"+ die_msg="$(eval_gettext "fatal: Unable to merge '\$sha1' in submodule path '\$displaypath'")" say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")" must_die_on_failure=yes ;; !*) command="${update_module#!}"- die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"+ die_msg="$(eval_gettext "fatal: Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")" say_msg="$(eval_gettext "Submodule path '\$displaypath': '\$command \$sha1'")" must_die_on_failure=yes ;; *)- die "$(eval_gettext "Invalid update mode '$update_module' for submodule path '$path'")"+ die "$(eval_gettext "fatal: Invalid update mode '$update_module' for submodule path '$path'")" esac if (sanitize_submodule_env; cd "$sm_path" && $command "$sha1")
@@ -660,7 +660,7 @@ cmd_update() res=$? if test $res -gt 0 then- die_msg="$(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"+ die_msg="$(eval_gettext "fatal: Failed to recurse into submodule path '\$displaypath'")" if test $res -ne 2 then err="${err};$die_msg"
@@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' ' test_expect_success'add aborts on repository with no commits''cat>expect<<-\EOF&&-'"'repo-no-commits'"'doesnothaveacommitcheckedout+fatal:'"'repo-no-commits'"'doesnothaveacommitcheckedoutEOFgitinitrepo-no-commits&&test_must_failgitsubmoduleadd../a./repo-no-commits2>actual&&
@@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' ' test_expect_success'submodule add to path with tracked content fails''(cdaddtest&&-echo"'\''dir-tracked'\'' already exists in the index">expect&&+echo"fatal: '\''dir-tracked'\'' already exists in the index">expect&&mkdirdir-tracked&&test_commitfoodir-tracked/bar&&test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&
Changes since v2:
* In 2/4, the prefix 'fatal' is left out of the gettext functions so that it
will not create new translatable items for the translators.
Atharva Raykar (4):
t7400: test failure to add submodule in tracked path
submodule: prefix die messages with 'fatal'
submodule--helper: refactor module_clone()
submodule--helper: introduce add-clone subcommand
builtin/submodule--helper.c | 418 ++++++++++++++++++++++++++----------
git-submodule.sh | 76 ++-----
t/t7400-submodule-basic.sh | 13 +-
t/t7406-submodule-update.sh | 10 +-
4 files changed, 342 insertions(+), 175 deletions(-)
--
2.32.0
Add a test to ensure failure on adding a submodule to a directory with
tracked contents in the index.
As we are going to refactor and port to C some parts of `git submodule
add`, let's add a test to help ensure no regression is introduced.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
---
t/t7400-submodule-basic.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' ')'+test_expect_success'submodule add to path with tracked content fails''+(+cdaddtest&&+echo"'\''dir-tracked'\'' already exists in the index">expect&&+mkdirdir-tracked&&+test_commitfoodir-tracked/bar&&+test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&+test_cmpexpectactual+)+'+ test_expect_success'submodule add to reconfigure existing submodule with --force''(cdaddtest-ignore&&
The standard `die()` function that is used in C code prefixes all the
messages passed to it with 'fatal: '. This does not happen with the
`die` used in 'git-submodule.sh'.
Let's prefix each of the shell die messages with 'fatal: ' so that when
they are converted to C code, the error messages stay the same as before
the conversion.
Note that the shell version of `die` exits with error code 1, while the
C version exits with error code 128. In practice, this does not change
any behaviour, as no functionality in 'submodule add' and 'submodule
update' relies on the value of the exit code.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
---
git-submodule.sh | 38 ++++++++++++++++++-------------------
t/t7400-submodule-basic.sh | 4 ++--
t/t7406-submodule-update.sh | 10 +++++-----
3 files changed, 26 insertions(+), 26 deletions(-)
@@ -147,7 +147,7 @@ cmd_add()if!gitsubmodule--helperconfig--check-writeable>/dev/null2>&1then-die"$(eval_gettext"please make sure that the .gitmodules file is in the working tree")"+die"fatal: $(eval_gettext"please make sure that the .gitmodules file is in the working tree")"fiiftest-n"$reference_path"
@@ -176,7 +176,7 @@ cmd_add()case"$repo"in./*|../*)test-z"$wt_prefix"||-die"$(gettext"Relative path can only be used from the toplevel of the working tree")"+die"fatal: $(gettext"Relative path can only be used from the toplevel of the working tree")"# dereference source url relative to parent's urlrealrepo=$(gitsubmodule--helperresolve-relative-url"$repo")||exit
@@ -186,7 +186,7 @@ cmd_add()realrepo=$repo;;*)-die"$(eval_gettext"repo URL: '\$repo' must be absolute or begin with ./|../")"+die"fatal: $(eval_gettext"repo URL: '\$repo' must be absolute or begin with ./|../")";;esac
@@ -205,17 +205,17 @@ cmd_add()iftest-z"$force"thengitls-files--error-unmatch"$sm_path">/dev/null2>&1&&-die"$(eval_gettext"'\$sm_path' already exists in the index")"+die"fatal: $(eval_gettext"'\$sm_path' already exists in the index")"elsegitls-files-s"$sm_path"|sane_grep-v"^160000">/dev/null2>&1&&-die"$(eval_gettext"'\$sm_path' already exists in the index and is not a submodule")"+die"fatal: $(eval_gettext"'\$sm_path' already exists in the index and is not a submodule")"fiiftest-d"$sm_path"&&test-z$(git-C"$sm_path"rev-parse--show-cdup2>/dev/null)thengit-C"$sm_path"rev-parse--verify-qHEAD>/dev/null||-die"$(eval_gettext"'\$sm_path' does not have a commit checked out")"+die"fatal: $(eval_gettext"'\$sm_path' does not have a commit checked out")"fiiftest-z"$force"
@@ -238,7 +238,7 @@ cmd_add()if!gitsubmodule--helpercheck-name"$sm_name"then-die"$(eval_gettext"'$sm_name' is not a valid submodule name")"+die"fatal: $(eval_gettext"'$sm_name' is not a valid submodule name")"fi# perhaps the path exists and is already a git repo, else clone it
@@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option."gitconfigsubmodule."$sm_name".url"$realrepo"gitadd--no-warn-embedded-repo$force"$sm_path"||-die"$(eval_gettext"Failed to add submodule '\$sm_path'")"+die"fatal: $(eval_gettext"Failed to add submodule '\$sm_path'")"gitsubmodule--helperconfigsubmodule."$sm_name".path"$sm_path"&&gitsubmodule--helperconfigsubmodule."$sm_name".url"$repo"&&
@@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option."gitsubmodule--helperconfigsubmodule."$sm_name".branch"$branch"fi&&gitadd--force.gitmodules||-die"$(eval_gettext"Failed to register submodule '\$sm_path'")"+die"fatal: $(eval_gettext"Failed to register submodule '\$sm_path'")"# NEEDSWORK: In a multi-working-tree world, this needs to be# set in the per-worktree config.
@@ -565,7 +565,7 @@ cmd_update()elsesubsha1=$(sanitize_submodule_env;cd"$sm_path"&&gitrev-parse--verifyHEAD)||-die"$(eval_gettext"Unable to find current revision in submodule path '\$displaypath'")"+die"fatal: $(eval_gettext"Unable to find current revision in submodule path '\$displaypath'")"fiiftest-n"$remote"
@@ -575,12 +575,12 @@ cmd_update()then# Fetch remote before determining tracking $sha1fetch_in_submodule"$sm_path"$depth||-die"$(eval_gettext"Unable to fetch in submodule path '\$sm_path'")"+die"fatal: $(eval_gettext"Unable to fetch in submodule path '\$sm_path'")"firemote_name=$(sanitize_submodule_env;cd"$sm_path"&&gitsubmodule--helperprint-default-remote)sha1=$(sanitize_submodule_env;cd"$sm_path"&&gitrev-parse--verify"${remote_name}/${branch}")||-die"$(eval_gettext"Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"+die"fatal: $(eval_gettext"Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"fiiftest"$subsha1"!="$sha1"||test-n"$force"
@@ -604,36 +604,36 @@ cmd_update()# not be reachable from any of the refsis_tip_reachable"$sm_path""$sha1"||fetch_in_submodule"$sm_path""$depth""$sha1"||-die"$(eval_gettext"Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"+die"fatal: $(eval_gettext"Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"fimust_die_on_failure=case"$update_module"incheckout)command="git checkout $subforce -q"-die_msg="$(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"+die_msg="fatal: $(eval_gettext"Unable to checkout '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': checked out '\$sha1'")";;rebase)command="git rebase ${GIT_QUIET:+--quiet}"-die_msg="$(eval_gettext"Unable to rebase '\$sha1' in submodule path '\$displaypath'")"+die_msg="fatal: $(eval_gettext"Unable to rebase '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': rebased into '\$sha1'")"must_die_on_failure=yes;;merge)command="git merge ${GIT_QUIET:+--quiet}"-die_msg="$(eval_gettext"Unable to merge '\$sha1' in submodule path '\$displaypath'")"+die_msg="fatal: $(eval_gettext"Unable to merge '\$sha1' in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': merged in '\$sha1'")"must_die_on_failure=yes;;!*)command="${update_module#!}"-die_msg="$(eval_gettext"Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"+die_msg="fatal: $(eval_gettext"Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"say_msg="$(eval_gettext"Submodule path '\$displaypath': '\$command \$sha1'")"must_die_on_failure=yes;;*)-die"$(eval_gettext"Invalid update mode '$update_module' for submodule path '$path'")"+die"fatal: $(eval_gettext"Invalid update mode '$update_module' for submodule path '$path'")"esacif(sanitize_submodule_env;cd"$sm_path"&&$command"$sha1")
@@ -660,7 +660,7 @@ cmd_update()res=$?iftest$res-gt0then-die_msg="$(eval_gettext"Failed to recurse into submodule path '\$displaypath'")"+die_msg="fatal: $(eval_gettext"Failed to recurse into submodule path '\$displaypath'")"iftest$res-ne2thenerr="${err};$die_msg"
@@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' ' test_expect_success'add aborts on repository with no commits''cat>expect<<-\EOF&&-'"'repo-no-commits'"'doesnothaveacommitcheckedout+fatal:'"'repo-no-commits'"'doesnothaveacommitcheckedoutEOFgitinitrepo-no-commits&&test_must_failgitsubmoduleadd../a./repo-no-commits2>actual&&
@@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' ' test_expect_success'submodule add to path with tracked content fails''(cdaddtest&&-echo"'\''dir-tracked'\'' already exists in the index">expect&&+echo"fatal: '\''dir-tracked'\'' already exists in the index">expect&&mkdirdir-tracked&&test_commitfoodir-tracked/bar&&test_must_failgitsubmoduleadd"$submodurl"dir-tracked>actual2>&1&&
Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Suggested-by: Junio C Hamano <redacted>
---
builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
1 file changed, 128 insertions(+), 113 deletions(-)
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,free(error_strategy);}+staticintclone_submodule(structmodule_clone_data*clone_data)+{+char*p,*sm_gitdir;+char*sm_alternate=NULL,*error_strategy=NULL;+structstrbufsb=STRBUF_INIT;+structchild_processcp=CHILD_PROCESS_INIT;++strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),clone_data->name);+sm_gitdir=absolute_pathdup(sb.buf);+strbuf_reset(&sb);++if(!is_absolute_path(clone_data->path)){+strbuf_addf(&sb,"%s/%s",get_git_work_tree(),clone_data->path);+clone_data->path=strbuf_detach(&sb,NULL);+}else{+clone_data->path=xstrdup(clone_data->path);+}++if(validate_submodule_git_dir(sm_gitdir,clone_data->name)<0)+die(_("refusing to create/use '%s' in another submodule's "+"git dir"),sm_gitdir);++if(!file_exists(sm_gitdir)){+if(safe_create_leading_directories_const(sm_gitdir)<0)+die(_("could not create directory '%s'"),sm_gitdir);++prepare_possible_alternates(clone_data->name,&clone_data->reference);++strvec_push(&cp.args,"clone");+strvec_push(&cp.args,"--no-checkout");+if(clone_data->quiet)+strvec_push(&cp.args,"--quiet");+if(clone_data->progress)+strvec_push(&cp.args,"--progress");+if(clone_data->depth&&*(clone_data->depth))+strvec_pushl(&cp.args,"--depth",clone_data->depth,NULL);+if(clone_data->reference.nr){+structstring_list_item*item;+for_each_string_list_item(item,&clone_data->reference)+strvec_pushl(&cp.args,"--reference",+item->string,NULL);+}+if(clone_data->dissociate)+strvec_push(&cp.args,"--dissociate");+if(sm_gitdir&&*sm_gitdir)+strvec_pushl(&cp.args,"--separate-git-dir",sm_gitdir,NULL);+if(clone_data->single_branch>=0)+strvec_push(&cp.args,clone_data->single_branch?+"--single-branch":+"--no-single-branch");++strvec_push(&cp.args,"--");+strvec_push(&cp.args,clone_data->url);+strvec_push(&cp.args,clone_data->path);++cp.git_cmd=1;+prepare_submodule_repo_env(&cp.env_array);+cp.no_stdin=1;++if(run_command(&cp))+die(_("clone of '%s' into submodule path '%s' failed"),+clone_data->url,clone_data->path);+}else{+if(clone_data->require_init&&!access(clone_data->path,X_OK)&&+!is_empty_dir(clone_data->path))+die(_("directory not empty: '%s'"),clone_data->path);+if(safe_create_leading_directories_const(clone_data->path)<0)+die(_("could not create directory '%s'"),clone_data->path);+strbuf_addf(&sb,"%s/index",sm_gitdir);+unlink_or_warn(sb.buf);+strbuf_reset(&sb);+}++connect_work_tree_and_git_dir(clone_data->path,sm_gitdir,0);++p=git_pathdup_submodule(clone_data->path,"config");+if(!p)+die(_("could not get submodule directory for '%s'"),clone_data->path);++/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */+git_config_get_string("submodule.alternateLocation",&sm_alternate);+if(sm_alternate)+git_config_set_in_file(p,"submodule.alternateLocation",+sm_alternate);+git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);+if(error_strategy)+git_config_set_in_file(p,"submodule.alternateErrorStrategy",+error_strategy);++free(sm_alternate);+free(error_strategy);++strbuf_release(&sb);+free(sm_gitdir);+free(p);+return0;+}+staticintmodule_clone(intargc,constchar**argv,constchar*prefix){-constchar*name=NULL,*url=NULL,*depth=NULL;-intquiet=0;-intprogress=0;-char*p,*path=NULL,*sm_gitdir;-structstrbufsb=STRBUF_INIT;-structstring_listreference=STRING_LIST_INIT_NODUP;-intdissociate=0,require_init=0;-char*sm_alternate=NULL,*error_strategy=NULL;-intsingle_branch=-1;+intdissociate=0,quiet=0,progress=0,require_init=0;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;structoptionmodule_clone_options[]={-OPT_STRING(0,"prefix",&prefix,+OPT_STRING(0,"prefix",&clone_data.prefix,N_("path"),N_("alternative anchor for relative paths")),-OPT_STRING(0,"path",&path,+OPT_STRING(0,"path",&clone_data.path,N_("path"),N_("where the new submodule will be cloned to")),-OPT_STRING(0,"name",&name,+OPT_STRING(0,"name",&clone_data.name,N_("string"),N_("name of the new submodule")),-OPT_STRING(0,"url",&url,+OPT_STRING(0,"url",&clone_data.url,N_("string"),N_("url where to clone the submodule from")),-OPT_STRING_LIST(0,"reference",&reference,+OPT_STRING_LIST(0,"reference",&clone_data.reference,N_("repo"),N_("reference repository")),OPT_BOOL(0,"dissociate",&dissociate,N_("use --reference only while cloning")),-OPT_STRING(0,"depth",&depth,+OPT_STRING(0,"depth",&clone_data.depth,N_("string"),N_("depth for shallow clones")),OPT__QUIET(&quiet,"Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)N_("force cloning progress")),OPT_BOOL(0,"require-init",&require_init,N_("disallow cloning into non-empty directory")),-OPT_BOOL(0,"single-branch",&single_branch,+OPT_BOOL(0,"single-branch",&clone_data.single_branch,N_("clone only one branch, HEAD or --branch")),OPT_END()};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)argc=parse_options(argc,argv,prefix,module_clone_options,git_submodule_helper_usage,0);-if(argc||!url||!path||!*path)+clone_data.dissociate=!!dissociate;+clone_data.quiet=!!quiet;+clone_data.progress=!!progress;+clone_data.require_init=!!require_init;++if(argc||!clone_data.url||!clone_data.path||!*(clone_data.path))usage_with_options(git_submodule_helper_usage,module_clone_options);-strbuf_addf(&sb,"%s/modules/%s",get_git_dir(),name);-sm_gitdir=absolute_pathdup(sb.buf);-strbuf_reset(&sb);--if(!is_absolute_path(path)){-strbuf_addf(&sb,"%s/%s",get_git_work_tree(),path);-path=strbuf_detach(&sb,NULL);-}else-path=xstrdup(path);--if(validate_submodule_git_dir(sm_gitdir,name)<0)-die(_("refusing to create/use '%s' in another submodule's "-"git dir"),sm_gitdir);--if(!file_exists(sm_gitdir)){-if(safe_create_leading_directories_const(sm_gitdir)<0)-die(_("could not create directory '%s'"),sm_gitdir);--prepare_possible_alternates(name,&reference);--if(clone_submodule(path,sm_gitdir,url,depth,&reference,dissociate,-quiet,progress,single_branch))-die(_("clone of '%s' into submodule path '%s' failed"),-url,path);-}else{-if(require_init&&!access(path,X_OK)&&!is_empty_dir(path))-die(_("directory not empty: '%s'"),path);-if(safe_create_leading_directories_const(path)<0)-die(_("could not create directory '%s'"),path);-strbuf_addf(&sb,"%s/index",sm_gitdir);-unlink_or_warn(sb.buf);-strbuf_reset(&sb);-}--connect_work_tree_and_git_dir(path,sm_gitdir,0);--p=git_pathdup_submodule(path,"config");-if(!p)-die(_("could not get submodule directory for '%s'"),path);--/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */-git_config_get_string("submodule.alternateLocation",&sm_alternate);-if(sm_alternate)-git_config_set_in_file(p,"submodule.alternateLocation",-sm_alternate);-git_config_get_string("submodule.alternateErrorStrategy",&error_strategy);-if(error_strategy)-git_config_set_in_file(p,"submodule.alternateErrorStrategy",-error_strategy);--free(sm_alternate);-free(error_strategy);--strbuf_release(&sb);-free(sm_gitdir);-free(path);-free(p);+clone_submodule(&clone_data);return0;}
Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.
This is meant to be a faithful conversion that leaves the behaviour of
'cmd_add()' script unchanged.
Signed-off-by: Atharva Raykar <redacted>
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Shourya Shukla [off-list ref]
Based-on-patch-by: Prathamesh Chavan [off-list ref]
Helped-by: Đoàn Trần Công Danh [off-list ref]
---
builtin/submodule--helper.c | 177 ++++++++++++++++++++++++++++++++++++
git-submodule.sh | 38 +-------
2 files changed, 178 insertions(+), 37 deletions(-)
@@ -2760,6 +2760,182 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)return!!ret;}+structadd_data{+constchar*prefix;+constchar*branch;+constchar*reference_path;+constchar*sm_path;+constchar*sm_name;+constchar*repo;+constchar*realrepo;+intdepth;+unsignedintforce:1;+unsignedintquiet:1;+unsignedintprogress:1;+unsignedintdissociate:1;+};+#define ADD_DATA_INIT { .depth = -1 }++staticvoidshow_fetch_remotes(FILE*output,constchar*sm_name,constchar*git_dir_path)+{+structchild_processcp_remote=CHILD_PROCESS_INIT;+structstrbufsb_remote_out=STRBUF_INIT;++cp_remote.git_cmd=1;+strvec_pushf(&cp_remote.env_array,+"GIT_DIR=%s",git_dir_path);+strvec_push(&cp_remote.env_array,"GIT_WORK_TREE=.");+strvec_pushl(&cp_remote.args,"remote","-v",NULL);+if(!capture_command(&cp_remote,&sb_remote_out,0)){+char*next_line;+char*line=sb_remote_out.buf;+while((next_line=strchr(line,'\n'))!=NULL){+size_tlen=next_line-line;+if(strip_suffix_mem(line,&len," (fetch)"))+fprintf(output," %.*s\n",(int)len,line);+line=next_line+1;+}+}++strbuf_release(&sb_remote_out);+}++staticintadd_submodule(conststructadd_data*add_data)+{+char*submod_gitdir_path;+structmodule_clone_dataclone_data=MODULE_CLONE_DATA_INIT;++/* perhaps the path already exists and is already a git repo, else clone it */+if(is_directory(add_data->sm_path)){+structstrbufsm_path=STRBUF_INIT;+strbuf_addstr(&sm_path,add_data->sm_path);+submod_gitdir_path=xstrfmt("%s/.git",add_data->sm_path);+if(is_nonbare_repository_dir(&sm_path))+printf(_("Adding existing repo at '%s' to the index\n"),+add_data->sm_path);+else+die(_("'%s' already exists and is not a valid git repo"),+add_data->sm_path);+strbuf_release(&sm_path);+free(submod_gitdir_path);+}else{+structchild_processcp=CHILD_PROCESS_INIT;+submod_gitdir_path=xstrfmt(".git/modules/%s",add_data->sm_name);++if(is_directory(submod_gitdir_path)){+if(!add_data->force){+fprintf(stderr,_("A git directory for '%s' is found "+"locally with remote(s):"),+add_data->sm_name);+show_fetch_remotes(stderr,add_data->sm_name,+submod_gitdir_path);+free(submod_gitdir_path);+die(_("If you want to reuse this local git "+"directory instead of cloning again from\n"+" %s\n"+"use the '--force' option. If the local git "+"directory is not the correct repo\n"+"or if you are unsure what this means, choose "+"another name with the '--name' option.\n"),+add_data->realrepo);+}else{+printf(_("Reactivating local git directory for "+"submodule '%s'\n"),add_data->sm_name);+}+}+free(submod_gitdir_path);++clone_data.prefix=add_data->prefix;+clone_data.path=add_data->sm_path;+clone_data.name=add_data->sm_name;+clone_data.url=add_data->realrepo;+clone_data.quiet=add_data->quiet;+clone_data.progress=add_data->progress;+if(add_data->reference_path)+string_list_append(&clone_data.reference,+xstrdup(add_data->reference_path));+clone_data.dissociate=add_data->dissociate;+if(add_data->depth>=0)+clone_data.depth=xstrfmt("%d",add_data->depth);++if(clone_submodule(&clone_data))+return-1;++prepare_submodule_repo_env(&cp.env_array);+cp.git_cmd=1;+cp.dir=add_data->sm_path;+strvec_pushl(&cp.args,"checkout","-f","-q",NULL);++if(add_data->branch){+strvec_pushl(&cp.args,"-B",add_data->branch,NULL);+strvec_pushf(&cp.args,"origin/%s",add_data->branch);+}++if(run_command(&cp))+die(_("unable to checkout submodule '%s'"),add_data->sm_path);+}+return0;+}++staticintadd_clone(intargc,constchar**argv,constchar*prefix)+{+intforce=0,quiet=0,dissociate=0,progress=0;+structadd_dataadd_data=ADD_DATA_INIT;++structoptionoptions[]={+OPT_STRING('b',"branch",&add_data.branch,+N_("branch"),+N_("branch of repository to checkout on cloning")),+OPT_STRING(0,"prefix",&prefix,+N_("path"),+N_("alternative anchor for relative paths")),+OPT_STRING(0,"path",&add_data.sm_path,+N_("path"),+N_("where the new submodule will be cloned to")),+OPT_STRING(0,"name",&add_data.sm_name,+N_("string"),+N_("name of the new submodule")),+OPT_STRING(0,"url",&add_data.realrepo,+N_("string"),+N_("url where to clone the submodule from")),+OPT_STRING(0,"reference",&add_data.reference_path,+N_("repo"),+N_("reference repository")),+OPT_BOOL(0,"dissociate",&dissociate,+N_("use --reference only while cloning")),+OPT_INTEGER(0,"depth",&add_data.depth,+N_("depth for shallow clones")),+OPT_BOOL(0,"progress",&progress,+N_("force cloning progress")),+OPT__FORCE(&force,N_("allow adding an otherwise ignored submodule path"),+PARSE_OPT_NOCOMPLETE),+OPT__QUIET(&quiet,"suppress output for cloning a submodule"),+OPT_END()+};++constchar*constusage[]={+N_("git submodule--helper add-clone [<options>...] "+"--url <url> --path <path> --name <name>"),+NULL+};++argc=parse_options(argc,argv,prefix,options,usage,0);++if(argc!=0)+usage_with_options(usage,options);++add_data.prefix=prefix;+add_data.progress=!!progress;+add_data.dissociate=!!dissociate;+add_data.force=!!force;+add_data.quiet=!!quiet;++if(add_submodule(&add_data))+return1;++return0;+}+#define SUPPORT_SUPER_PREFIX (1<<0)structcmd_struct{
@@ -241,43 +241,7 @@ cmd_add()die"fatal: $(eval_gettext"'$sm_name' is not a valid submodule name")"fi-# perhaps the path exists and is already a git repo, else clone it-iftest-e"$sm_path"-then-iftest-d"$sm_path"/.git||test-f"$sm_path"/.git-then-eval_gettextln"Adding existing repo at '\$sm_path' to the index"-else-die"$(eval_gettext"'\$sm_path' already exists and is not a valid git repo")"-fi--else-iftest-d".git/modules/$sm_name"-then-iftest-z"$force"-then-eval_gettextln>&2"A git directory for '\$sm_name' is found locally with remote(s):"-GIT_DIR=".git/modules/$sm_name"GIT_WORK_TREE=.gitremote-v|grep'(fetch)'|sed-es,^," ",-es,' (fetch)',,>&2-die"$(eval_gettextln"\-Ifyouwanttoreusethislocalgitdirectoryinsteadofcloningagainfrom-\$realrepo-usethe'--force'option.Ifthelocalgitdirectoryisnotthecorrectrepo-oryouareunsurewhatthismeanschooseanothernamewiththe'--name'option.")"-else-eval_gettextln"Reactivating local git directory for submodule '\$sm_name'."-fi-fi-gitsubmodule--helperclone${GIT_QUIET:+--quiet}${progress:+"--progress"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exit-(-sanitize_submodule_env-cd"$sm_path"&&-# ash fails to wordsplit ${branch:+-b "$branch"...}-case"$branch"in-'')gitcheckout-f-q;;-?*)gitcheckout-f-q-B"$branch""origin/$branch";;-esac-)||die"$(eval_gettext"Unable to checkout submodule '\$sm_path'")"-fi+gitsubmodule--helperadd-clone${GIT_QUIET:+--quiet}${force:+"--force"}${progress:+"--progress"}${branch:+--branch "$branch"}--prefix"$wt_prefix"--path"$sm_path"--name"$sm_name"--url"$realrepo"${reference:+"$reference"}${dissociate:+"--dissociate"}${depth:+"$depth"}||exitgitconfigsubmodule."$sm_name".url"$realrepo"gitadd--no-warn-embedded-repo$force"$sm_path"||
On 09-Jul-2021, at 20:22, Đoàn Trần Công Danh [off-list ref] wrote:
On 2021-07-08 15:25:31+0530, Atharva Raykar [off-list ref] wrote:
quoted
The standard `die()` function that is used in C code prefixes all the
messages passed to it with 'fatal: '. This does not happen with the
`die` used in 'git-submodule.sh'.
Let's prefix each of the shell die messages with 'fatal: ' so that when
they are converted to C code, the error messages stay the same as before
the conversion.
That sounds good.
quoted
--- a/git-submodule.sh+++ b/git-submodule.sh
@@ -147,7 +147,7 @@ cmd_add()
if ! git submodule--helper config --check-writeable >/dev/null 2>&1
then
- die "$(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
+ die "$(eval_gettext "fatal: please make sure that the .gitmodules file is in the working tree")"
Except that, "fatal: " isn't subjected to translation. And this will
create new translatable item for translator. Perhaps:
- die "$(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
+ die "fatal: $(eval_gettext "please make sure that the .gitmodules file is in the working tree")"
Okay, I have made the change. I was wondering if there any specific
reason as to why 'fatal' should not be translated? Is it because
an intermediate change like this should not create more work for
translators?
Hi Atharva,
On 10 ஜூலை, 2021 பிற்பகல் 1:22:16 IST, Atharva Raykar [off-list ref] wrote:
quoted
On 09-Jul-2021, at 20:22, Đoàn Trần Công Danh [off-list ref]
wrote:
quoted
On 2021-07-08 15:25:31+0530, Atharva Raykar [off-list ref]
wrote:
quoted
quoted
The standard `die()` function that is used in C code prefixes all
the
quoted
quoted
messages passed to it with 'fatal: '. This does not happen with the
`die` used in 'git-submodule.sh'.
Let's prefix each of the shell die messages with 'fatal: ' so that
when
quoted
quoted
they are converted to C code, the error messages stay the same as
before
quoted
quoted
the conversion.
That sounds good.
quoted
--- a/git-submodule.sh+++ b/git-submodule.sh
@@ -147,7 +147,7 @@ cmd_add()
if ! git submodule--helper config --check-writeable >/dev/null 2>&1
then
- die "$(eval_gettext "please make sure that the .gitmodules file
is in the working tree")"
quoted
quoted
+ die "$(eval_gettext "fatal: please make sure that the
.gitmodules file is in the working tree")"
quoted
Except that, "fatal: " isn't subjected to translation. And this will
create new translatable item for translator. Perhaps:
- die "$(eval_gettext "please make sure that the .gitmodules file
is in the working tree")"
quoted
+ die "fatal: $(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
Okay, I have made the change. I was wondering if there any specific
reason as to why 'fatal' should not be translated? Is it because
an intermediate change like this should not create more work for
translators?
Yes. That's likely the intention.
--
Sivaraam
Sent from my Android device with K-9 Mail. Please excuse my brevity.
The sm_name parameter is not used here. I don't think it's a bug; we
just don't need it (there's a message that mentions the name, but it
happens right before we call the function). Maybe this should go on top
of ar/submodule-add?
-- >8 --
Subject: submodule: drop unused sm_name parameter from show_fetch_remotes()
This parameter has not been used since the function was introduced in
8c8195e9c3 (submodule--helper: introduce add-clone subcommand,
2021-07-10).
Signed-off-by: Jeff King <redacted>
---
builtin/submodule--helper.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
@@ -2833,8 +2833,7 @@ static int add_submodule(const struct add_data *add_data)fprintf(stderr,_("A git directory for '%s' is found ""locally with remote(s):"),add_data->sm_name);-show_fetch_remotes(stderr,add_data->sm_name,-submod_gitdir_path);+show_fetch_remotes(stderr,submod_gitdir_path);free(submod_gitdir_path);die(_("If you want to reuse this local git ""directory instead of cloning again from\n"
The sm_name parameter is not used here. I don't think it's a bug; we
just don't need it (there's a message that mentions the name, but it
happens right before we call the function). Maybe this should go on top
of ar/submodule-add?
-- >8 --
Subject: submodule: drop unused sm_name parameter from show_fetch_remotes()
This parameter has not been used since the function was introduced in
8c8195e9c3 (submodule--helper: introduce add-clone subcommand,
2021-07-10).
Signed-off-by: Jeff King <redacted>
---
builtin/submodule--helper.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
@@ -2833,8 +2833,7 @@ static int add_submodule(const struct add_data *add_data)
fprintf(stderr, _("A git directory for '%s' is found "
"locally with remote(s):"),
add_data->sm_name);
- show_fetch_remotes(stderr, add_data->sm_name,
- submod_gitdir_path);
+ show_fetch_remotes(stderr, submod_gitdir_path);
free(submod_gitdir_path);
die(_("If you want to reuse this local git "
"directory instead of cloning again from\n"
--
2.32.0.784.g92e169d3d7
Yes, this is definitely an oversight on my part, and it looks like this
topic has already made it to 'next'.
Thanks for the fix.
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
After the refactoring the error started appearing like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.
--- END OF OUTPUT ---
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
Even with this patch, the error message is still not fully consistent with the one that
used to be printed before the refactoring. Here's the diff:
3c3
< If you want to reuse this local git directory instead of cloning again from
---
fatal: If you want to reuse this local git directory instead of cloning again from
6c6
< or you are unsure what this means choose another name with the '--name' option.
---
or if you are unsure what this means, choose another name with the '--name' option.
The first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix
made sense in other cases, I wonder if it's helpful in this case as the message being
printed is an informative one. Should we avoid using 'die' to print this message?
The second part of the diff shows that there's some small grammatcial tweaks in the last
line. While I appreciate the intention, I'm not very sure if this change is a strict
improvement. I wonder about this as the original sounded good enough to me and thus it
feels like the change in message is triggering unnecesssary translation work. Should
we avoid the change? Or does it actually seem like an improvement to the message?
builtin/submodule--helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -2824,7 +2824,7 @@ static int add_submodule(const struct add_data *add_data)if(is_directory(submod_gitdir_path)){if(!add_data->force){fprintf(stderr,_("A git directory for '%s' is found "-"locally with remote(s):"),+"locally with remote(s):\n"),add_data->sm_name);show_fetch_remotes(stderr,add_data->sm_name,submod_gitdir_path);
@@ -2835,7 +2835,7 @@ static int add_submodule(const struct add_data *add_data)"use the '--force' option. If the local git ""directory is not the correct repo\n""or if you are unsure what this means, choose "-"another name with the '--name' option.\n"),+"another name with the '--name' option."),add_data->realrepo);}else{printf(_("Reactivating local git directory for "
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
[...]
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
Thanks for catching this and sending a patch!
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
Even with this patch, the error message is still not fully consistent with the one that
used to be printed before the refactoring. Here's the diff:
3c3
< If you want to reuse this local git directory instead of cloning again from
---
quoted
fatal: If you want to reuse this local git directory instead of cloning again from
6c6
< or you are unsure what this means choose another name with the '--name' option.
---
quoted
or if you are unsure what this means, choose another name with the '--name' option.
The first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix
made sense in other cases, I wonder if it's helpful in this case as the message being
printed is an informative one. Should we avoid using 'die' to print this message?
I had initially implemented that message as an fprintf() with return for
the same reason, but Junio suggested we die() instead to keep the
conversion more faithful to the original [1].
Although now that I think of it, it feels like a tradeoff between
faithfulness to the original code and faithfulness to the original
behaviour. I also think this change is fairly inconsequential and easily
reversible so I am fine with it being done either way.
[1] https://lore.kernel.org/git/xmqqk0n03k84.fsf@gitster.g/
The second part of the diff shows that there's some small grammatcial tweaks in the last
line. While I appreciate the intention, I'm not very sure if this change is a strict
improvement. I wonder about this as the original sounded good enough to me and thus it
feels like the change in message is triggering unnecesssary translation work. Should
we avoid the change? Or does it actually seem like an improvement to the message?
I don't think that extra 'if' was intended. I think it's better to avoid
the change I inadvertently introduced.
@@ -2824,7 +2824,7 @@ static int add_submodule(const struct add_data *add_data)if(is_directory(submod_gitdir_path)){if(!add_data->force){fprintf(stderr,_("A git directory for '%s' is found "-"locally with remote(s):"),+"locally with remote(s):\n"),add_data->sm_name);show_fetch_remotes(stderr,add_data->sm_name,submod_gitdir_path);
@@ -2835,7 +2835,7 @@ static int add_submodule(const struct add_data *add_data)"use the '--force' option. If the local git ""directory is not the correct repo\n""or if you are unsure what this means, choose "-"another name with the '--name' option.\n"),+"another name with the '--name' option."),add_data->realrepo);}else{printf(_("Reactivating local git directory for "
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
[...]
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
Thanks for catching this and sending a patch!
quoted
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
Even with this patch, the error message is still not fully consistent with the one that
used to be printed before the refactoring. Here's the diff:
3c3
< If you want to reuse this local git directory instead of cloning again from
---
quoted
fatal: If you want to reuse this local git directory instead of cloning again from
6c6
< or you are unsure what this means choose another name with the '--name' option.
---
quoted
or if you are unsure what this means, choose another name with the '--name' option.
The first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix
made sense in other cases, I wonder if it's helpful in this case as the message being
printed is an informative one. Should we avoid using 'die' to print this message?
I had initially implemented that message as an fprintf() with return for
the same reason, but Junio suggested we die() instead to keep the
conversion more faithful to the original [1].
Although now that I think of it, it feels like a tradeoff between
faithfulness to the original code and faithfulness to the original
behaviour. I also think this change is fairly inconsequential and easily
reversible so I am fine with it being done either way.
[1] https://lore.kernel.org/git/xmqqk0n03k84.fsf@gitster.g/
I remember this discussion. But ...
A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
... I'm not able to understand how the 'fatal: ' prefix in the _middle_ of this
information message would help the reader in any way :-/
If it had occurred at the beginning of the message that would've been different
and likely useful too. I'll try sending a re-roll with a new change that moves
the fatal to the beginning of the message and see how it goes.
quoted
The second part of the diff shows that there's some small grammatcial tweaks in the last
line. While I appreciate the intention, I'm not very sure if this change is a strict
improvement. I wonder about this as the original sounded good enough to me and thus it
feels like the change in message is triggering unnecesssary translation work. Should
we avoid the change? Or does it actually seem like an improvement to the message?
I don't think that extra 'if' was intended. I think it's better to avoid
the change I inadvertently introduced.
I guess I spoke too soon. Regardless of the additional "... if ..." in the message,
the conversion might introduce translation work as the variable substitution varies
between shell and C. There isn't much we could do about this. I'm not sure if
translators use any strategy to avoid redundant work during conversions like these.
Jiang Xin (Cc-ed) might be able to shed some light.
That said, I'll still revert the extra 'if' as you say it was introduced inadvertently.
--
Sivaraam
Hi all,
Finally got a chance to send the v2 of this one.
Changes in v2:
- Removed the 'if' from the message as Atharva mentioned that it was
added inadvertently
- Moved 'fatal:' prefix from middle of the message to the beginning.
- Expanded the commit message to also mention the output after the change
For reference, the v1 could be found here:
https://public-inbox.org/git/20210805192803.679948-1-kaartic.sivaraam@gmail.com/
... and the range-diff against v1 could be found below. Reviews appreciated.
--
Sivaraam
Kaartic Sivaraam (1):
submodule--helper: fix incorrect newlines in an error message
builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
Range-diff against v1:
1: c00617bc03 ! 1: c6daed7a92 submodule--helper: fix incorrect newlines in an error message
@@ Commit message
After the refactoring the error started appearing like so:
- --- START OF OUTPUT ---
+ --- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
@@ Commit message
Make the error message consistent with the error message that used to be
printed before the refactoring.
+ This also moves the 'fatal:' prefix that appears in the middle of the
+ error message to the first line as it would more appropriate to have
+ it in the first line. The output after the change would look like:
+
+ --- START OF OUTPUT ---
+ $ git submodule add ../sub/ subm
+ fatal: A git directory for 'subm' is found locally with remote(s):
+ origin /me/git-repos-for-test/sub
+ If you want to reuse this local git directory instead of cloning again from
+ /me/git-repos-for-test/sub
+ use the '--force' option. If the local git directory is not the correct repo
+ or you are unsure what this means choose another name with the '--name' option.
+ --- END OF OUTPUT ---
+
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
## builtin/submodule--helper.c ##
+@@ builtin/submodule--helper.c: struct add_data {
+ };
+ #define ADD_DATA_INIT { .depth = -1 }
+
+-static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
++static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)
+ {
+ struct child_process cp_remote = CHILD_PROCESS_INIT;
+ struct strbuf sb_remote_out = STRBUF_INIT;
+@@ builtin/submodule--helper.c: static void show_fetch_remotes(FILE *output, const char *sm_name, const char *gi
+ while ((next_line = strchr(line, '\n')) != NULL) {
+ size_t len = next_line - line;
+ if (strip_suffix_mem(line, &len, " (fetch)"))
+- fprintf(output, " %.*s\n", (int)len, line);
++ strbuf_addf(msg, " %.*s\n", (int)len, line);
+ line = next_line + 1;
+ }
+ }
@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
+
if (is_directory(submod_gitdir_path)) {
if (!add_data->force) {
- fprintf(stderr, _("A git directory for '%s' is found "
+- fprintf(stderr, _("A git directory for '%s' is found "
- "locally with remote(s):"),
-+ "locally with remote(s):\n"),
- add_data->sm_name);
- show_fetch_remotes(stderr, add_data->sm_name,
+- add_data->sm_name);
+- show_fetch_remotes(stderr, add_data->sm_name,
++ struct strbuf msg = STRBUF_INIT;
++ char *die_msg;
++
++ strbuf_addf(&msg, _("A git directory for '%s' is found "
++ "locally with remote(s):\n"),
++ add_data->sm_name);
++
++ show_fetch_remotes(&msg, add_data->sm_name,
submod_gitdir_path);
-@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
- "use the '--force' option. If the local git "
- "directory is not the correct repo\n"
- "or if you are unsure what this means, choose "
+ free(submod_gitdir_path);
+- die(_("If you want to reuse this local git "
+- "directory instead of cloning again from\n"
+- " %s\n"
+- "use the '--force' option. If the local git "
+- "directory is not the correct repo\n"
+- "or if you are unsure what this means, choose "
- "another name with the '--name' option.\n"),
-+ "another name with the '--name' option."),
- add_data->realrepo);
+- add_data->realrepo);
++
++ strbuf_addf(&msg, _("If you want to reuse this local git "
++ "directory instead of cloning again from\n"
++ " %s\n"
++ "use the '--force' option. If the local git "
++ "directory is not the correct repo\n"
++ "or you are unsure what this means choose "
++ "another name with the '--name' option."),
++ add_data->realrepo);
++
++ die_msg = strbuf_detach(&msg, NULL);
++ die("%s", die_msg);
} else {
printf(_("Reactivating local git directory for "
+ "submodule '%s'\n"), add_data->sm_name);
--
2.32.0.385.gc00617bc03.dirty
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
After the refactoring the error started appearing like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.
--- END OF OUTPUT ---
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
This also moves the 'fatal:' prefix that appears in the middle of the
error message to the first line as it would more appropriate to have
it in the first line. The output after the change would look like:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
fatal: A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
@@ -2823,20 +2823,28 @@ static int add_submodule(const struct add_data *add_data)if(is_directory(submod_gitdir_path)){if(!add_data->force){-fprintf(stderr,_("A git directory for '%s' is found "-"locally with remote(s):"),-add_data->sm_name);-show_fetch_remotes(stderr,add_data->sm_name,+structstrbufmsg=STRBUF_INIT;+char*die_msg;++strbuf_addf(&msg,_("A git directory for '%s' is found "+"locally with remote(s):\n"),+add_data->sm_name);++show_fetch_remotes(&msg,add_data->sm_name,submod_gitdir_path);free(submod_gitdir_path);-die(_("If you want to reuse this local git "-"directory instead of cloning again from\n"-" %s\n"-"use the '--force' option. If the local git "-"directory is not the correct repo\n"-"or if you are unsure what this means, choose "-"another name with the '--name' option.\n"),-add_data->realrepo);++strbuf_addf(&msg,_("If you want to reuse this local git "+"directory instead of cloning again from\n"+" %s\n"+"use the '--force' option. If the local git "+"directory is not the correct repo\n"+"or you are unsure what this means choose "+"another name with the '--name' option."),+add_data->realrepo);++die_msg=strbuf_detach(&msg,NULL);+die("%s",die_msg);}else{printf(_("Reactivating local git directory for ""submodule '%s'\n"),add_data->sm_name);
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
After the refactoring the error started appearing like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.
--- END OF OUTPUT ---
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
This also moves the 'fatal:' prefix that appears in the middle of the
error message to the first line as it would more appropriate to have
it in the first line. The output after the change would look like:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
fatal: A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)
I like the change from using a strbuf instead of passing the output
stream and printing to it. But maybe we should rename this function, now
that it doesn't really 'show' anything? Probably something like
'append_fetch_remotes()'?
@@ -2823,20 +2823,28 @@ static int add_submodule(const struct add_data *add_data) if (is_directory(submod_gitdir_path)) { if (!add_data->force) {- fprintf(stderr, _("A git directory for '%s' is found "- "locally with remote(s):"),- add_data->sm_name);- show_fetch_remotes(stderr, add_data->sm_name,+ struct strbuf msg = STRBUF_INIT;+ char *die_msg;++ strbuf_addf(&msg, _("A git directory for '%s' is found "+ "locally with remote(s):\n"),+ add_data->sm_name);++ show_fetch_remotes(&msg, add_data->sm_name, submod_gitdir_path); free(submod_gitdir_path);- die(_("If you want to reuse this local git "- "directory instead of cloning again from\n"- " %s\n"- "use the '--force' option. If the local git "- "directory is not the correct repo\n"- "or if you are unsure what this means, choose "- "another name with the '--name' option.\n"),- add_data->realrepo);++ strbuf_addf(&msg, _("If you want to reuse this local git "+ "directory instead of cloning again from\n"+ " %s\n"+ "use the '--force' option. If the local git "+ "directory is not the correct repo\n"+ "or you are unsure what this means choose "+ "another name with the '--name' option."),+ add_data->realrepo);++ die_msg = strbuf_detach(&msg, NULL);+ die("%s", die_msg); } else { printf(_("Reactivating local git directory for " "submodule '%s'\n"), add_data->sm_name);
Other than that this patch is an improvement. Thanks for fixing this!
I like the change from using a strbuf instead of passing the output
stream and printing to it. But maybe we should rename this function, now
that it doesn't really 'show' anything? Probably something like
'append_fetch_remotes()'?
That's a good point. I've taken your suggestion into account in this v3.
Find the details of the v3 of this patch below.
Changes since v2:
- Renamed the helper function name to be more appropriate, upon suggestion.
Also, I rebased my local branch over the latest 'master'. So, this should apply
cleanly over 'master'.
For reference, the v2 could be found here:
https://public-inbox.org/git/20210918193116.310575-1-kaartic.sivaraam@gmail.com/
... and the range-diff against v2 could be found below.
--
Sivaraam
Kaartic Sivaraam (1):
submodule--helper: fix incorrect newlines in an error message
builtin/submodule--helper.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
Range-diff against v2:
1: 95cbe38be3 ! 1: 7c4887ccf5 submodule--helper: fix incorrect newlines in an error message
@@ builtin/submodule--helper.c: struct add_data {
#define ADD_DATA_INIT { .depth = -1 }
-static void show_fetch_remotes(FILE *output, const char *git_dir_path)
-+static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)
++static void append_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)
{
struct child_process cp_remote = CHILD_PROCESS_INIT;
struct strbuf sb_remote_out = STRBUF_INIT;
@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add
+ "locally with remote(s):\n"),
+ add_data->sm_name);
+
-+ show_fetch_remotes(&msg, add_data->sm_name,
-+ submod_gitdir_path);
++ append_fetch_remotes(&msg, add_data->sm_name,
++ submod_gitdir_path);
free(submod_gitdir_path);
- die(_("If you want to reuse this local git "
- "directory instead of cloning again from\n"
--
2.33.1.1058.gd3b4e01def
A refactoring[1] done as part of the recent conversion of
'git submodule add' to builtin, changed the error message
shown when a Git directory already exists locally for a submodule
name. Before the refactoring, the error used to appear like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
After the refactoring the error started appearing like so:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub
fatal: If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.
--- END OF OUTPUT ---
As one could observe the remote information is printed along with the
first line rather than on its own line. Also, there's an additional
newline following output.
Make the error message consistent with the error message that used to be
printed before the refactoring.
This also moves the 'fatal:' prefix that appears in the middle of the
error message to the first line as it would more appropriate to have
it in the first line. The output after the change would look like:
--- START OF OUTPUT ---
$ git submodule add ../sub/ subm
fatal: A git directory for 'subm' is found locally with remote(s):
origin /me/git-repos-for-test/sub
If you want to reuse this local git directory instead of cloning again from
/me/git-repos-for-test/sub
use the '--force' option. If the local git directory is not the correct repo
or you are unsure what this means choose another name with the '--name' option.
--- END OF OUTPUT ---
[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t
Signed-off-by: Kaartic Sivaraam <redacted>
---
builtin/submodule--helper.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
@@ -3047,19 +3047,28 @@ static int add_submodule(const struct add_data *add_data)if(is_directory(submod_gitdir_path)){if(!add_data->force){-fprintf(stderr,_("A git directory for '%s' is found "-"locally with remote(s):"),-add_data->sm_name);-show_fetch_remotes(stderr,submod_gitdir_path);+structstrbufmsg=STRBUF_INIT;+char*die_msg;++strbuf_addf(&msg,_("A git directory for '%s' is found "+"locally with remote(s):\n"),+add_data->sm_name);++append_fetch_remotes(&msg,add_data->sm_name,+submod_gitdir_path);free(submod_gitdir_path);-die(_("If you want to reuse this local git "-"directory instead of cloning again from\n"-" %s\n"-"use the '--force' option. If the local git "-"directory is not the correct repo\n"-"or if you are unsure what this means, choose "-"another name with the '--name' option.\n"),-add_data->realrepo);++strbuf_addf(&msg,_("If you want to reuse this local git "+"directory instead of cloning again from\n"+" %s\n"+"use the '--force' option. If the local git "+"directory is not the correct repo\n"+"or you are unsure what this means choose "+"another name with the '--name' option."),+add_data->realrepo);++die_msg=strbuf_detach(&msg,NULL);+die("%s",die_msg);}else{printf(_("Reactivating local git directory for ""submodule '%s'\n"),add_data->sm_name);