git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Christian Couder <christian.couder@gmail.com>
Date
Jun 1, 2021, 22:10 UTC
Message-ID
<CAP8UFD3SMghGb0y0jKuLScrKqqHgZFDxW1c97MwoEz+1hXt1hA@mail.gmail.com>
In-Reply-To
<20210528081224.69163-1-raykar.ath@gmail.com>
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"
Show 5 quoted lines
> 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.
Show 16 quoted lines
> 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.

Show 61 quoted lines
> [1] https://lore.kernel.org/git/20201214231939.644175-1-periperidip@gmail.com/
>
>  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.

Show 20 quoted lines
> +                       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?
Show 50 quoted lines
> +               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.

Show 7 quoted lines
> +               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.

Show 66 quoted lines
> +               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
> +       };
Previous: Atharva RaykarNext: Atharva Raykar
Message 2 of 11 in “[GSoC] submodule: introduce add-clone helper for submodule add”
  1. [GSoC] submodule: introduce add-clone helper for submodule addAtharva Raykar, May 28, 2021
  2. Christian CouderJun 1, 2021
  3. Atharva RaykarJun 2, 2021
  4. Atharva RaykarJun 2, 2021
  5. [GSoC] submodule--helper: introduce add-clone subcommandAtharva Raykar, Jun 2, 2021
  6. Christian CouderJun 4, 2021
  7. Atharva RaykarJun 4, 2021
  8. [GSoC] submodule--helper: introduce add-clone subcommandAtharva Raykar, Jun 4, 2021
  9. Atharva RaykarJun 4, 2021
  10. Shourya ShuklaJun 4, 2021
  11. Atharva RaykarJun 4, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.