On Fri, May 28, 2021 at 10:13 AM Atharva Raykar <raykar.ath@xxxxxxxxx> 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 <raykar.ath@xxxxxxxxx>
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Shourya Shukla <shouryashukla.oo@xxxxxxxxx>
> Based-on-patch-by: Shourya Shukla <shouryashukla.oo@xxxxxxxxx>
> Based-on-patch-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
> ---
>
> 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.
> [1] https://lore.kernel.org/git/20201214231939.644175-1-periperidip@xxxxxxxxx/
>
> 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.
> + 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.
> + 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?
> + 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.
> + 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.
> + 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
> + };
