Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 3, 2015, 22:07 UTC
- Message-ID
- <xmqq8u8ni2bv.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1441230146-26921-4-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 30 quoted lines
> @@ -119,6 +120,140 @@ 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 && *depth)
> + argv_array_pushl(&cp.args, "--depth", depth, NULL);
> + if (reference && *reference)
> + argv_array_pushl(&cp.args, "--reference", reference, NULL);
> + if (gitdir && *gitdir)
> + argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
> +
> + 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;Output from "git clone" is not shown, regardless of --quiet option?
Show 47 quoted lines
> + 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;
> + char *sm_gitdir, *cwd, *p;
> + struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + struct option module_clone_options[] = {
> + OPT_STRING(0, "prefix", &prefix,
> + 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__QUIET(&quiet, "Suppress output for cloning a submodule"),
> + OPT_END()
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
> + "[--reference <repository>] [--name <name>] [--url <url>]"
> + "[--depth <depth>] [--] [<path>...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_clone_options,
> + git_submodule_helper_usage, 0);
> +
> + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);The original says
base_name=$(dirname "$name")
before doing this %s/modules/%s concatenation. I do not think we intended to allow a slash in submodule name, so this difference may be showing that the original was doing an unnecessary thing.
Show 14 quoted lines
> + sm_gitdir = strbuf_detach(&sb, NULL);
> +
> + if (!file_exists(sm_gitdir)) {
> + if (safe_create_leading_directories_const(sm_gitdir) < 0)
> + die(_("could not create directory '%s'"), sm_gitdir);
> + if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))
> + die(_("clone of '%s' into submodule path '%s' failed"),
> + url, path);
> + } else {
> + if (safe_create_leading_directories_const(path) < 0)
> + die(_("could not create directory '%s'"), path);
> + strbuf_addf(&sb, "%s/index", sm_gitdir);
> + if (unlink(sb.buf) < 0)
> + die_errno(_("failed to delete '%s'"), sm_gitdir);The original says "we do not care if it failed" with
rm -f "$gitdir/index"
I think the intention of the original is "we do not care if it failed because it did not exist." in which case unconditional die_errno() here may be something we do not want?
Show 24 quoted lines
> + strbuf_reset(&sb);
> + }
> +
> + /* Write a .git file in the submodule to redirect to the superproject. */
> + if (safe_create_leading_directories_const(path) < 0)
> + die(_("could not create directory '%s'"), path);
> +
> + if (path && *path)
> + strbuf_addf(&sb, "%s/.git", path);
> + else
> + strbuf_addf(&sb, ".git");
> +
> + if (safe_create_leading_directories_const(sb.buf) < 0)
> + die(_("could not create leading directories of '%s'"), sb.buf);
> + submodule_dot_git = fopen(sb.buf, "w");
> + if (!submodule_dot_git)
> + die_errno(_("cannot open file '%s'"), sb.buf);
> +
> + 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);
> + strbuf_reset(&rel_path);The original seems to go quite a length to make sure symbolic links do not fool the comparison between $gitdir and $sm_path, and also it makes sure one is not a subpath of the other. Do we need the same level of carefulness, or is textual relative_path() enough?
Show 24 quoted lines
> + cwd = xgetcwd();
> + /* Redirect the worktree of the submodule in the superproject's config */
> + if (!is_absolute_path(sm_gitdir)) {
> + strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir);
> + free(sm_gitdir);
> + sm_gitdir = strbuf_detach(&sb, NULL);
> + }
> +
> + strbuf_addf(&sb, "%s/%s", cwd, 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);
> + strbuf_release(&rel_path);
> + free(sm_gitdir);
> + free(cwd);
> + free(p);
> + return 0;
> +}
>
> struct cmd_struct {
> const char *cmd;Show 10 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh > index 2be8da2..7cfdc2c 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -301,7 +227,7 @@ cmd_add() > shift > ;; > --depth=*) > - depth=$1 > + depth="$1"
This seems to be an unrelated change.
Otherwise looks quite straight-forward.
Thanks.