Re: [PATCH][GSoC] submodule: introduce add-clone helper for submodule add

> 
> On Fri, May 28, 2021 at 10:13 AM Atharva Raykar <raykar.ath@gmail.com> 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@gmail.com>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
>> Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
>> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
>> ---
>> 
>> 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.
> 
>> [...]
>> +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.

>> [...]
>> +       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.