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

Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Aug 31, 2015, 22:35 UTC
Message-ID
<CAPig+cQVe+qmTsqqdnTXDABxNkQoeEqz07pP2pYbY_n11XMDWA@mail.gmail.com>
In-Reply-To
<1441048767-29729-4-git-send-email-sbeller@google.com>
On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller <sbeller@google.com> wrote:
> `module_clone` is part of the update command,
> which I want to convert to C next.
Hmm, place commentary below "---".
Show 22 quoted lines
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c8f7e0c..d29499c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static int clone_submodule(const char *path, const char *gitdir, const char *url,
> +                          const char *depth, const char *reference, int quiet)
> +{
> +       struct child_process cp;
> +       child_process_init(&cp);
> +
> +       argv_array_push(&cp.args, "clone");
> +       argv_array_push(&cp.args, "--no-checkout");
> +       if (quiet)
> +               argv_array_push(&cp.args, "--quiet");
> +       if (depth && strcmp(depth, "")) {
More idiomatic:
    if (depth && *depth) {
> +               argv_array_push(&cp.args, "--depth");
> +               argv_array_push(&cp.args, depth);
These would be easier to read if you consolidate them with argv_array_pushl():
    argv_array_pushl(&cp.args, "--depth", depth, NULL);
and it would allow you to drop the noisy braces.
> +       }
> +       if (reference && strcmp(reference, "")) {
    if (reference && *reference) {
> +               argv_array_push(&cp.args, "--reference");
> +               argv_array_push(&cp.args, reference);
    argv_array_pushl(&cp.args, "--reference", reference, NULL);
> +       }
> +       if (gitdir) {

Why is this case different than the others, in that it doesn't check for the zero-length string ""?

> +               argv_array_push(&cp.args, "--separate-git-dir");
> +               argv_array_push(&cp.args, gitdir);
    argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
Show 21 quoted lines
> +       }
> +       argv_array_push(&cp.args, url);
> +       argv_array_push(&cp.args, path);
> +
> +       cp.git_cmd = 1;
> +       cp.env = local_repo_env;
> +
> +       cp.no_stdin = 1;
> +       cp.no_stdout = 1;
> +       cp.no_stderr = 1;
> +
> +       return run_command(&cp);
> +}
> +
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> +       const char *path = NULL, *name = NULL, *url = NULL;
> +       const char *reference = NULL, *depth = NULL;
> +       int quiet = 0;
> +       FILE *submodule_dot_git;
> +       const char *sm_gitdir, *p;

Why is 'sm_gitdir' declared const even though it's always pointing at memory which this function owns and needs to free? If you drop the 'const', then you won't have to cast it to non-const in a couple places below.

Show 26 quoted lines
> +       struct strbuf rel_path = STRBUF_INIT;
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       struct option module_update_options[] = {
> +               OPT_STRING(0, "prefix", &alternative_path,
> +                          N_("path"),
> +                          N_("alternative anchor for relative paths")),
> +               OPT_STRING(0, "path", &path,
> +                          N_("path"),
> +                          N_("where the new submodule will be cloned to")),
> +               OPT_STRING(0, "name", &name,
> +                          N_("string"),
> +                          N_("name of the new submodule")),
> +               OPT_STRING(0, "url", &url,
> +                          N_("string"),
> +                          N_("url where to clone the submodule from")),
> +               OPT_STRING(0, "reference", &reference,
> +                          N_("string"),
> +                          N_("reference repository")),
> +               OPT_STRING(0, "depth", &depth,
> +                          N_("string"),
> +                          N_("depth for shallow clones")),
> +               OPT_END()
> +       };
> +
> +       static const char * const git_submodule_helper_usage[] = {
You can drop this 'static'.
Show 11 quoted lines
> +               N_("git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]"
> +                  "[-f|--force] [--rebase|--merge] [--reference <repository>]"
> +                  "[--depth <depth>] [--recursive] [--] [<path>...]"),
> +               NULL
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, module_update_options,
> +                            git_submodule_helper_usage, 0);
> +
> +       if (getenv("GIT_QUIET"))
> +               quiet = 1;

I understand that you're simply replicating the behavior of the shell code, but this is "yuck".

'module_clone' is only called from two places in git-submodule.sh, so how about a cleanup patch which makes 'module_clone' accept a --quiet flag and have the two callers pass it explicitly? Finally, have this C replacement accept --quiet as a proper command-line option rather than via this hidden backdoor method.

Show 5 quoted lines
> +       strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> +       sm_gitdir = strbuf_detach(&sb, NULL);
> +
> +       if (!file_exists(sm_gitdir)) {
> +               safe_create_leading_directories_const(sm_gitdir);

Why is it that the return value of safe_create_leading_directories_const() is sometimes checked in this function and sometimes not?

Show 6 quoted lines
> +               if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))
> +                       die(N_("Clone of '%s' into submodule path '%s' failed"),
> +                           url, path);
> +       } else {
> +               safe_create_leading_directories_const(path);
> +               unlink(sm_gitdir);
Check the return value of unlink()?
> +       }
> +
> +       /* Write a .git file in the submodule to redirect to the superproject. */
> +       if (alternative_path && !strcmp(alternative_path, "")) {
Maybe?
    if (alternative_path && !*alternative_path) {
Show 12 quoted lines
> +               p = relative_path(path, alternative_path, &sb);
> +               strbuf_reset(&sb);
> +       } else
> +               p = path;
> +
> +       if (safe_create_leading_directories_const(p) < 0)
> +               die("Could not create directory '%s'", p);
> +
> +       strbuf_addf(&sb, "%s/.git", p);
> +
> +       if (safe_create_leading_directories_const(sb.buf) < 0)
> +               die(_("could not create leading directories of '%s'"), sb.buf);
Why the mix of capitalized and lowercase-only error messages in this function?

Also, the original shell code consistently uses eval_gettext(), so _("...") seems warranted.

Show 11 quoted lines
> +       submodule_dot_git = fopen(sb.buf, "w");
> +       if (!submodule_dot_git)
> +               die ("Cannot open file '%s': %s", sb.buf, strerror(errno));
> +
> +       fprintf(submodule_dot_git, "gitdir: %s\n",
> +               relative_path(sm_gitdir, path, &rel_path));
> +       if (fclose(submodule_dot_git))
> +               die("Could not close file %s", sb.buf);
> +       strbuf_reset(&sb);
> +
> +       /* Redirect the worktree of the submodule in the superprojects config */
s/superprojects/superproject's/
Show 7 quoted lines
> +       if (!is_absolute_path(sm_gitdir)) {
> +               char *s = (char*)sm_gitdir;
> +               if (strbuf_getcwd(&sb))
> +                       die_errno("unable to get current working directory");
> +               strbuf_addf(&sb, "/%s", sm_gitdir);
> +               sm_gitdir = strbuf_detach(&sb, NULL);
> +               free(s);
Why squirrel-away 'sm_gitdir' in 's' unnecessarily? Equivalent:
    if (strbuf_getcwd(&sb))
        die_errno(...);
    strbuf_addf(&sb, "/%s, sm_gitdir);
    free(sm_gitdir);
    sm_gitdir = strbuf_detach(&sb, NULL);
> +       }
> +
> +       if (strbuf_getcwd(&sb))
> +               die_errno("unable to get current working directory");

The conditional block just above here also gets 'cwd'. If you move this code above the !is_absolute_path(sm_gitdir) conditional block, then you can avoid the duplication.

Show 34 quoted lines
> +       strbuf_addf(&sb, "/%s", path);
> +
> +       p = git_pathdup_submodule(path, "config");
> +       if (!p)
> +               die("Could not get submodule directory for '%s'", path);
> +       git_config_set_in_file(p, "core.worktree",
> +                              relative_path(sb.buf, sm_gitdir, &rel_path));
> +       strbuf_release(&sb);
> +       free((char *)sm_gitdir);
> +       return 0;
> +}
> +
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>         if (argc < 2)
> @@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>         if (!strcmp(argv[1], "module_name"))
>                 return module_name(argc - 2, argv + 2, prefix);
>
> +       if (!strcmp(argv[1], "module_clone"))
> +               return module_clone(argc - 1, argv + 1, prefix);
> +
>  usage:
> -       usage("git submodule--helper [module_list | module_name]\n");
> +       usage("git submodule--helper [module_list | module_name | "
> +             "module_clone]\n");
>  }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e6ff38d..fb5155e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -178,80 +178,6 @@ get_submodule_config () {
>         printf '%s' "${value:-$default}"
>  }
Previous: Stefan BellerNext: Stefan Beller
Message 9 of 21 in “[PATCHv2 0/3] submodule--helper: Have some refactoring only patches first”
  1. Stefan BellerAug 31, 2015
  2. 1/3 submodule: implement `module_list` as a builtin helperStefan Beller, Aug 31, 2015
  3. Eric SunshineAug 31, 2015
  4. 2/3 submodule: implement `module_name` as a builtin helperStefan Beller, Aug 31, 2015
  5. Junio C HamanoAug 31, 2015
  6. Stefan BellerAug 31, 2015
  7. Eric SunshineAug 31, 2015
  8. 3/3 submodule: implement `module_clone` as a builtin helperStefan Beller, Aug 31, 2015
  9. Eric SunshineAug 31, 2015
  10. Stefan BellerSep 1, 2015
  11. Eric SunshineSep 1, 2015
  12. Junio C HamanoAug 31, 2015
  13. 0/3 submodule--helper: Have some refactoring only patches firstStefan Beller, Sep 1, 2015
  14. 1/3 submodule: implement `module-list` as a builtin helperStefan Beller, Sep 1, 2015
  15. Eric SunshineSep 1, 2015
  16. 2/3 submodule: implement `module-name` as a builtin helperStefan Beller, Sep 1, 2015
  17. Eric SunshineSep 1, 2015
  18. Stefan BellerSep 1, 2015
  19. 3/3 submodule: implement `module-clone` as a builtin helperStefan Beller, Sep 1, 2015
  20. Eric SunshineSep 1, 2015
  21. Eric SunshineSep 1, 2015

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.