Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> writes:

> @@ -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?

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

> +	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?

> +		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?

> +	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;


> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]