Re: [PATCH][GSoC] submodule: introduce add-clone helper for submodule add
- From
- Christian Couder <hidden>
- Date
- 2021-06-01 22:10:29
- Reply to
- Atharva Raykar, 2021-05-28
On Fri, May 28, 2021 at 10:13 AM Atharva Raykar [off-list ref] wrote:
Convert the the shell code that performs the cloning of the repository that is
s/the the/the/
to be added, and checks out to the appropriate branch.
Something a bit more explicit might make things easier to understand. For example: "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." Then a simpler title could be: "submodule--helper: introduce add-clone subcommand"
This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.Good.
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] --- This is part of a series of changes that will result in all of 'submodule add' being converted to C, which is a more familiar language for Git developers, and paves the way to improve performance and portability. I have made this patch based on Shourya's patch[1]. I have decided to send the changes in smaller, more reviewable parts. The add-clone subcommand of submodule--helper is an intermediate change, while I work on translating all of the code. So in the next few patches, this helper subcommand is likely to be removed as its functionality would be invoked from the C code itself.
It might be a good idea to let us know how many such new subcommands you'd like to introduce before removing them. Anyway I think it's a good idea to send changes in smaller, more easily reviewable parts. Hopefully this way more work will end up being merged.
quoted hunk ↗ jump to hunk
[1] https://lore.kernel.org/git/20201214231939.644175-1-periperidip@gmail.com/ (local) builtin/submodule--helper.c | 221 ++++++++++++++++++++++++++++++++++++ git-submodule.sh | 38 +------ 2 files changed, 222 insertions(+), 37 deletions(-)diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d55f6262e9..39a844b0b1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c@@ -2745,6 +2745,226 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) return !!ret; } +struct add_data { + const char *prefix; + const char *branch; + const char *reference_path; + const char *sm_path; + const char *sm_name; + const char *repo; + const char *realrepo; + int depth; + unsigned int force: 1; + unsigned int quiet: 1; + unsigned int progress: 1; + unsigned int dissociate: 1; +}; +#define ADD_DATA_INIT { 0 } + +static char *parse_token(char **begin, const char *end) +{ + int size; + char *token, *pos = *begin; + while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n')) + pos++; + size = pos - *begin; + token = xstrndup(*begin, size); + *begin = pos + 1; + return token; +} + +static char *get_next_line(char *const begin, const char *const end) +{ + char *pos = begin; + while (pos != end && *pos++ != '\n'); + return pos; +} + +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path) +{ + struct child_process cp_remote = CHILD_PROCESS_INIT; + struct strbuf sb_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, *name, *url, *tail;
Maybe name, url and tail could be declared in the while loop below
where they are used.
quoted hunk ↗ jump to hunk
+ char *begin = sb_remote_out.buf; + char *end = sb_remote_out.buf + sb_remote_out.len; + while (begin != end && + (next_line = get_next_line(begin, end))) {
It would be nice if the above 2 lines could be reduced into just one
line. Maybe renaming "next_line" to just "line" could help with that.
quoted hunk ↗ jump to hunk
+ name = parse_token(&begin, next_line); + url = parse_token(&begin, next_line); + tail = parse_token(&begin, next_line); + if (!memcmp(tail, "(fetch)", 7)) + fprintf(output, " %s\t%s\n", name, url); + free(url); + free(name); + free(tail); + } + } + + strbuf_release(&sb_remote_out); +} + +static int add_submodule(const struct add_data *info) +{ + char *submod_gitdir_path; + /* perhaps the path already exists and is already a git repo, else clone it */ + if (is_directory(info->sm_path)) { + printf("sm_path=%s\n", info->sm_path);
Is this a leftover debug statement?
quoted hunk ↗ jump to hunk
+ submod_gitdir_path = xstrfmt("%s/.git", info->sm_path); + if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path)) + printf(_("Adding existing path at '%s' to index\n"), + info->sm_path); + else + die(_("'%s' already exists and is not a valid git repo"), + info->sm_path); + free(submod_gitdir_path); + } else { + struct strvec clone_args = STRVEC_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + submod_gitdir_path = xstrfmt(".git/modules/%s", info->sm_name); + + if (is_directory(submod_gitdir_path)) { + if (!info->force) { + error(_("A git directory for '%s' is found " + "locally with remote(s):"), info->sm_name); + show_fetch_remotes(stderr, info->sm_name, + submod_gitdir_path); + fprintf(stderr, + _("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"), + info->realrepo); + free(submod_gitdir_path); + return 1; + } else { + printf(_("Reactivating local git directory for " + "submodule '%s'\n"), info->sm_name); + } + } + free(submod_gitdir_path); + + strvec_push(&clone_args, "clone"); + + if (info->quiet) + strvec_push(&clone_args, "--quiet"); + + if (info->progress) + strvec_push(&clone_args, "--progress"); + + if (info->prefix) + strvec_pushl(&clone_args, "--prefix", info->prefix, NULL); + + strvec_pushl(&clone_args, "--path", info->sm_path, "--name", + info->sm_name, "--url", info->realrepo, NULL);
Maybe this unconditional strvec_pushl(...) could be squashed into the
strvec_push(&clone_args, "clone") above.
quoted hunk ↗ jump to hunk
+ if (info->reference_path) + strvec_pushl(&clone_args, "--reference", + info->reference_path, NULL); + + if (info->dissociate) + strvec_push(&clone_args, "--dissociate"); +
Blank lines since the above strvec_push(&clone_args, "clone") could
perhaps be removed.
quoted hunk ↗ jump to hunk
+ if (info->depth >= 0) + strvec_pushf(&clone_args, "--depth=%d", info->depth); + + if (module_clone(clone_args.nr, clone_args.v, info->prefix)) { + strvec_clear(&clone_args); + return -1; + } + strvec_clear(&clone_args); + + prepare_submodule_repo_env(&cp.env_array); + cp.git_cmd = 1; + cp.dir = info->sm_path; + strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL); + + if (info->branch) { + strvec_pushl(&cp.args, "-B", info->branch, NULL); + strvec_pushf(&cp.args, "origin/%s", info->branch); + } + + if (run_command(&cp)) + die(_("unable to checkout submodule '%s'"), info->sm_path); + } + return 0; +} + +static int add_clone(int argc, const char **argv, const char *prefix) +{ + const char *branch = NULL, *sm_path = NULL; + const char *wt_prefix = NULL, *realrepo = NULL; + const char *reference = NULL, *sm_name = NULL; + int force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0; + struct add_data info = ADD_DATA_INIT; + + struct option options[] = { + OPT_STRING('b', "branch", &branch, + N_("branch"), + N_("branch of repository to checkout on cloning")), + OPT_STRING(0, "prefix", &wt_prefix, + N_("path"), + N_("alternative anchor for relative paths")), + OPT_STRING(0, "path", &sm_path, + N_("path"), + N_("where the new submodule will be cloned to")), + OPT_STRING(0, "name", &sm_name, + N_("string"), + N_("name of the new submodule")), + OPT_STRING(0, "url", &realrepo, + N_("string"), + N_("url where to clone the submodule from")), + OPT_STRING(0, "reference", &reference, + N_("repo"), + N_("reference repository")), + OPT_BOOL(0, "dissociate", &dissociate, + N_("use --reference only while cloning")), + OPT_INTEGER(0, "depth", &depth, + N_("depth for shallow clones")), + OPT_BOOL(0, "progress", &progress, + N_("force cloning progress")), + OPT_BOOL('f', "force", &force, + N_("allow adding an otherwise ignored submodule path")), + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), + OPT_END() + }; + + const char *const usage[] = { + N_("git submodule--helper clone [--prefix=<path>] [--quiet] [--force] "
s/clone/add-clone/
+ "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]" + "--url <url> --path <path> --name <name>"),
The --progress and --dissociate options seem to be missing.
+ NULL + };