From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 22 Apr 2016 22:45:03 +0000 (-0700)
Subject: Merge branch 'sb/submodule-helper-clone-regression-fix'
X-Git-Tag: v2.9.0-rc0~112
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/deef3cdc08eb16354d16da0813585d60d5f34412?ds=inline;hp=-c

Merge branch 'sb/submodule-helper-clone-regression-fix'

A partial rewrite of "git submodule" in the 2.7 timeframe changed
the way the gitdir: pointer in the submodules point at the real
repository location to use absolute paths by accident.  This has
been corrected.

* sb/submodule-helper-clone-regression-fix:
submodule--helper, module_clone: catch fprintf failure
submodule--helper: do not borrow absolute_path() result for too long
submodule--helper, module_clone: always operate on absolute paths
submodule--helper clone: create the submodule path just once
submodule--helper: fix potential NULL-dereference
recursive submodules: test for relative paths
---

deef3cdc08eb16354d16da0813585d60d5f34412
diff --combined builtin/submodule--helper.c
index d36e8a0ec4,b3a60f56c3..3bd6883eff
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@@ -22,12 -22,17 +22,12 @@@ static int module_list_compute(int argc
  			       struct module_list *list)
  {
  	int i, result = 0;
 -	char *max_prefix, *ps_matched = NULL;
 -	int max_prefix_len;
 +	char *ps_matched = NULL;
  	parse_pathspec(pathspec, 0,
  		       PATHSPEC_PREFER_FULL |
  		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
  		       prefix, argv);
  
 -	/* Find common prefix for all pathspec's */
 -	max_prefix = common_prefix(pathspec);
 -	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 -
  	if (pathspec->nr)
  		ps_matched = xcalloc(pathspec->nr, 1);
  
@@@ -37,9 -42,9 +37,9 @@@
  	for (i = 0; i < active_nr; i++) {
  		const struct cache_entry *ce = active_cache[i];
  
 -		if (!S_ISGITLINK(ce->ce_mode) ||
 -		    !match_pathspec(pathspec, ce->name, ce_namelen(ce),
 -				    max_prefix_len, ps_matched, 1))
 +		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
 +				    0, ps_matched, 1) ||
 +		    !S_ISGITLINK(ce->ce_mode))
  			continue;
  
  		ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
@@@ -52,6 -57,7 +52,6 @@@
  			 */
  			i++;
  	}
 -	free(max_prefix);
  
  	if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
  		result = -1;
@@@ -118,55 -124,6 +118,55 @@@ static int module_name(int argc, const 
  
  	return 0;
  }
 +
 +/*
 + * Rules to sanitize configuration variables that are Ok to be passed into
 + * submodule operations from the parent project using "-c". Should only
 + * include keys which are both (a) safe and (b) necessary for proper
 + * operation.
 + */
 +static int submodule_config_ok(const char *var)
 +{
 +	if (starts_with(var, "credential."))
 +		return 1;
 +	return 0;
 +}
 +
 +static int sanitize_submodule_config(const char *var, const char *value, void *data)
 +{
 +	struct strbuf *out = data;
 +
 +	if (submodule_config_ok(var)) {
 +		if (out->len)
 +			strbuf_addch(out, ' ');
 +
 +		if (value)
 +			sq_quotef(out, "%s=%s", var, value);
 +		else
 +			sq_quote_buf(out, var);
 +	}
 +
 +	return 0;
 +}
 +
 +static void prepare_submodule_repo_env(struct argv_array *out)
 +{
 +	const char * const *var;
 +
 +	for (var = local_repo_env; *var; var++) {
 +		if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
 +			struct strbuf sanitized_config = STRBUF_INIT;
 +			git_config_from_parameters(sanitize_submodule_config,
 +						   &sanitized_config);
 +			argv_array_pushf(out, "%s=%s", *var, sanitized_config.buf);
 +			strbuf_release(&sanitized_config);
 +		} else {
 +			argv_array_push(out, *var);
 +		}
 +	}
 +
 +}
 +
  static int clone_submodule(const char *path, const char *gitdir, const char *url,
  			   const char *depth, const char *reference, int quiet)
  {
@@@ -188,7 -145,7 +188,7 @@@
  	argv_array_push(&cp.args, path);
  
  	cp.git_cmd = 1;
 -	cp.env = local_repo_env;
 +	prepare_submodule_repo_env(&cp.env_array);
  	cp.no_stdin = 1;
  
  	return run_command(&cp);
@@@ -196,11 -153,11 +196,11 @@@
  
  static int module_clone(int argc, const char **argv, const char *prefix)
  {
- 	const char *path = NULL, *name = NULL, *url = NULL;
+ 	const char *name = NULL, *url = NULL;
  	const char *reference = NULL, *depth = NULL;
  	int quiet = 0;
  	FILE *submodule_dot_git;
- 	char *sm_gitdir, *cwd, *p;
+ 	char *p, *path = NULL, *sm_gitdir;
  	struct strbuf rel_path = STRBUF_INIT;
  	struct strbuf sb = STRBUF_INIT;
  
@@@ -229,20 -186,26 +229,27 @@@
  
  	const char *const git_submodule_helper_usage[] = {
  		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
 -		   "[--reference <repository>] [--name <name>] [--url <url>]"
 -		   "[--depth <depth>] [--] [<path>...]"),
 +		   "[--reference <repository>] [--name <name>] [--depth <depth>] "
 +		   "--url <url> --path <path>"),
  		NULL
  	};
  
  	argc = parse_options(argc, argv, prefix, module_clone_options,
  			     git_submodule_helper_usage, 0);
  
- 	if (argc || !url || !path)
 -	if (!path || !*path)
 -		die(_("submodule--helper: unspecified or empty --path"));
++	if (argc || !url || !path || !*path)
 +		usage_with_options(git_submodule_helper_usage,
 +				   module_clone_options);
  
  	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
- 	sm_gitdir = strbuf_detach(&sb, NULL);
+ 	sm_gitdir = xstrdup(absolute_path(sb.buf));
+ 	strbuf_reset(&sb);
+ 
+ 	if (!is_absolute_path(path)) {
+ 		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
+ 		path = strbuf_detach(&sb, NULL);
+ 	} else
+ 		path = xstrdup(path);
  
  	if (!file_exists(sm_gitdir)) {
  		if (safe_create_leading_directories_const(sm_gitdir) < 0)
@@@ -259,316 -222,34 +266,301 @@@
  	}
  
  	/* 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_addstr(&sb, ".git");
- 
+ 	strbuf_addf(&sb, "%s/.git", path);
  	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));
+ 	fprintf_or_die(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);
  
- 	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));
+ 			       relative_path(path, sm_gitdir, &rel_path));
  	strbuf_release(&sb);
  	strbuf_release(&rel_path);
  	free(sm_gitdir);
- 	free(cwd);
+ 	free(path);
  	free(p);
  	return 0;
  }
  
 +static int module_sanitize_config(int argc, const char **argv, const char *prefix)
 +{
 +	struct strbuf sanitized_config = STRBUF_INIT;
 +
 +	if (argc > 1)
 +		usage(_("git submodule--helper sanitize-config"));
 +
 +	git_config_from_parameters(sanitize_submodule_config, &sanitized_config);
 +	if (sanitized_config.len)
 +		printf("%s\n", sanitized_config.buf);
 +
 +	strbuf_release(&sanitized_config);
 +
 +	return 0;
 +}
 +
 +struct submodule_update_clone {
 +	/* index into 'list', the list of submodules to look into for cloning */
 +	int current;
 +	struct module_list list;
 +	unsigned warn_if_uninitialized : 1;
 +
 +	/* update parameter passed via commandline */
 +	struct submodule_update_strategy update;
 +
 +	/* configuration parameters which are passed on to the children */
 +	int quiet;
 +	const char *reference;
 +	const char *depth;
 +	const char *recursive_prefix;
 +	const char *prefix;
 +
 +	/* Machine-readable status lines to be consumed by git-submodule.sh */
 +	struct string_list projectlines;
 +
 +	/* If we want to stop as fast as possible and return an error */
 +	unsigned quickstop : 1;
 +};
 +#define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
 +	SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
 +	STRING_LIST_INIT_DUP, 0}
 +
 +/**
 + * Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to
 + * run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise.
 + */
 +static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 +					   struct child_process *child,
 +					   struct submodule_update_clone *suc,
 +					   struct strbuf *out)
 +{
 +	const struct submodule *sub = NULL;
 +	struct strbuf displaypath_sb = STRBUF_INIT;
 +	struct strbuf sb = STRBUF_INIT;
 +	const char *displaypath = NULL;
 +	char *url = NULL;
 +	int needs_cloning = 0;
 +
 +	if (ce_stage(ce)) {
 +		if (suc->recursive_prefix)
 +			strbuf_addf(&sb, "%s/%s", suc->recursive_prefix, ce->name);
 +		else
 +			strbuf_addf(&sb, "%s", ce->name);
 +		strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
 +		strbuf_addch(out, '\n');
 +		goto cleanup;
 +	}
 +
 +	sub = submodule_from_path(null_sha1, ce->name);
 +
 +	if (suc->recursive_prefix)
 +		displaypath = relative_path(suc->recursive_prefix,
 +					    ce->name, &displaypath_sb);
 +	else
 +		displaypath = ce->name;
 +
 +	if (suc->update.type == SM_UPDATE_NONE
 +	    || (suc->update.type == SM_UPDATE_UNSPECIFIED
 +		&& sub->update_strategy.type == SM_UPDATE_NONE)) {
 +		strbuf_addf(out, _("Skipping submodule '%s'"), displaypath);
 +		strbuf_addch(out, '\n');
 +		goto cleanup;
 +	}
 +
 +	/*
 +	 * Looking up the url in .git/config.
 +	 * We must not fall back to .gitmodules as we only want
 +	 * to process configured submodules.
 +	 */
 +	strbuf_reset(&sb);
 +	strbuf_addf(&sb, "submodule.%s.url", sub->name);
 +	git_config_get_string(sb.buf, &url);
 +	if (!url) {
 +		/*
 +		 * Only mention uninitialized submodules when their
 +		 * path have been specified
 +		 */
 +		if (suc->warn_if_uninitialized) {
 +			strbuf_addf(out,
 +				_("Submodule path '%s' not initialized"),
 +				displaypath);
 +			strbuf_addch(out, '\n');
 +			strbuf_addstr(out,
 +				_("Maybe you want to use 'update --init'?"));
 +			strbuf_addch(out, '\n');
 +		}
 +		goto cleanup;
 +	}
 +
 +	strbuf_reset(&sb);
 +	strbuf_addf(&sb, "%s/.git", ce->name);
 +	needs_cloning = !file_exists(sb.buf);
 +
 +	strbuf_reset(&sb);
 +	strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
 +			sha1_to_hex(ce->sha1), ce_stage(ce),
 +			needs_cloning, ce->name);
 +	string_list_append(&suc->projectlines, sb.buf);
 +
 +	if (!needs_cloning)
 +		goto cleanup;
 +
 +	child->git_cmd = 1;
 +	child->no_stdin = 1;
 +	child->stdout_to_stderr = 1;
 +	child->err = -1;
 +	argv_array_push(&child->args, "submodule--helper");
 +	argv_array_push(&child->args, "clone");
 +	if (suc->quiet)
 +		argv_array_push(&child->args, "--quiet");
 +	if (suc->prefix)
 +		argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL);
 +	argv_array_pushl(&child->args, "--path", sub->path, NULL);
 +	argv_array_pushl(&child->args, "--name", sub->name, NULL);
 +	argv_array_pushl(&child->args, "--url", url, NULL);
 +	if (suc->reference)
 +		argv_array_push(&child->args, suc->reference);
 +	if (suc->depth)
 +		argv_array_push(&child->args, suc->depth);
 +
 +cleanup:
 +	free(url);
 +	strbuf_reset(&displaypath_sb);
 +	strbuf_reset(&sb);
 +
 +	return needs_cloning;
 +}
 +
 +static int update_clone_get_next_task(struct child_process *child,
 +				      struct strbuf *err,
 +				      void *suc_cb,
 +				      void **void_task_cb)
 +{
 +	struct submodule_update_clone *suc = suc_cb;
 +
 +	for (; suc->current < suc->list.nr; suc->current++) {
 +		const struct cache_entry *ce = suc->list.entries[suc->current];
 +		if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
 +			suc->current++;
 +			return 1;
 +		}
 +	}
 +	return 0;
 +}
 +
 +static int update_clone_start_failure(struct strbuf *err,
 +				      void *suc_cb,
 +				      void *void_task_cb)
 +{
 +	struct submodule_update_clone *suc = suc_cb;
 +	suc->quickstop = 1;
 +	return 1;
 +}
 +
 +static int update_clone_task_finished(int result,
 +				      struct strbuf *err,
 +				      void *suc_cb,
 +				      void *void_task_cb)
 +{
 +	struct submodule_update_clone *suc = suc_cb;
 +
 +	if (!result)
 +		return 0;
 +
 +	suc->quickstop = 1;
 +	return 1;
 +}
 +
 +static int update_clone(int argc, const char **argv, const char *prefix)
 +{
 +	const char *update = NULL;
 +	int max_jobs = -1;
 +	struct string_list_item *item;
 +	struct pathspec pathspec;
 +	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
 +
 +	struct option module_update_clone_options[] = {
 +		OPT_STRING(0, "prefix", &prefix,
 +			   N_("path"),
 +			   N_("path into the working tree")),
 +		OPT_STRING(0, "recursive-prefix", &suc.recursive_prefix,
 +			   N_("path"),
 +			   N_("path into the working tree, across nested "
 +			      "submodule boundaries")),
 +		OPT_STRING(0, "update", &update,
 +			   N_("string"),
 +			   N_("rebase, merge, checkout or none")),
 +		OPT_STRING(0, "reference", &suc.reference, N_("repo"),
 +			   N_("reference repository")),
 +		OPT_STRING(0, "depth", &suc.depth, "<depth>",
 +			   N_("Create a shallow clone truncated to the "
 +			      "specified number of revisions")),
 +		OPT_INTEGER('j', "jobs", &max_jobs,
 +			    N_("parallel jobs")),
 +		OPT__QUIET(&suc.quiet, N_("don't print cloning progress")),
 +		OPT_END()
 +	};
 +
 +	const char *const git_submodule_helper_usage[] = {
 +		N_("git submodule--helper update_clone [--prefix=<path>] [<path>...]"),
 +		NULL
 +	};
 +	suc.prefix = prefix;
 +
 +	argc = parse_options(argc, argv, prefix, module_update_clone_options,
 +			     git_submodule_helper_usage, 0);
 +
 +	if (update)
 +		if (parse_submodule_update_strategy(update, &suc.update) < 0)
 +			die(_("bad value for update parameter"));
 +
 +	if (module_list_compute(argc, argv, prefix, &pathspec, &suc.list) < 0)
 +		return 1;
 +
 +	if (pathspec.nr)
 +		suc.warn_if_uninitialized = 1;
 +
 +	/* Overlay the parsed .gitmodules file with .git/config */
 +	gitmodules_config();
 +	git_config(submodule_config, NULL);
 +
 +	if (max_jobs < 0)
 +		max_jobs = parallel_submodules();
 +
 +	run_processes_parallel(max_jobs,
 +			       update_clone_get_next_task,
 +			       update_clone_start_failure,
 +			       update_clone_task_finished,
 +			       &suc);
 +
 +	/*
 +	 * We saved the output and put it out all at once now.
 +	 * That means:
 +	 * - the listener does not have to interleave their (checkout)
 +	 *   work with our fetching.  The writes involved in a
 +	 *   checkout involve more straightforward sequential I/O.
 +	 * - the listener can avoid doing any work if fetching failed.
 +	 */
 +	if (suc.quickstop)
 +		return 1;
 +
 +	for_each_string_list_item(item, &suc.projectlines)
 +		utf8_fprintf(stdout, "%s", item->string);
 +
 +	return 0;
 +}
 +
  struct cmd_struct {
  	const char *cmd;
  	int (*fn)(int, const char **, const char *);
@@@ -578,21 -259,19 +570,21 @@@ static struct cmd_struct commands[] = 
  	{"list", module_list},
  	{"name", module_name},
  	{"clone", module_clone},
 +	{"sanitize-config", module_sanitize_config},
 +	{"update-clone", update_clone}
  };
  
  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
  {
  	int i;
  	if (argc < 2)
 -		die(_("fatal: submodule--helper subcommand must be "
 +		die(_("submodule--helper subcommand must be "
  		      "called with a subcommand"));
  
  	for (i = 0; i < ARRAY_SIZE(commands); i++)
  		if (!strcmp(argv[1], commands[i].cmd))
  			return commands[i].fn(argc - 1, argv + 1, prefix);
  
 -	die(_("fatal: '%s' is not a valid submodule--helper "
 +	die(_("'%s' is not a valid submodule--helper "
  	      "subcommand"), argv[1]);
  }
diff --combined t/t7400-submodule-basic.sh
index 17d7a98207,ea3fabbed8..f99f674ac7
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@@ -462,7 -462,7 +462,7 @@@ test_expect_success 'update --init' 
  	git config --remove-section submodule.example &&
  	test_must_fail git config submodule.example.url &&
  
 -	git submodule update init > update.out &&
 +	git submodule update init 2> update.out &&
  	cat update.out &&
  	test_i18ngrep "not initialized" update.out &&
  	test_must_fail git rev-parse --resolve-git-dir init/.git &&
@@@ -480,7 -480,7 +480,7 @@@ test_expect_success 'update --init fro
  	mkdir -p sub &&
  	(
  		cd sub &&
 -		git submodule update ../init >update.out &&
 +		git submodule update ../init 2>update.out &&
  		cat update.out &&
  		test_i18ngrep "not initialized" update.out &&
  		test_must_fail git rev-parse --resolve-git-dir ../init/.git &&
@@@ -816,6 -816,47 +816,47 @@@ test_expect_success 'submodule add --na
  		git config submodule.repo_new.url >actual &&
  		test_cmp expect actual
  	)
+ '
+ 
+ test_expect_success 'recursive relative submodules stay relative' '
+ 	test_when_finished "rm -rf super clone2 subsub sub3" &&
+ 	mkdir subsub &&
+ 	(
+ 		cd subsub &&
+ 		git init &&
+ 		>t &&
+ 		git add t &&
+ 		git commit -m "initial commit"
+ 	) &&
+ 	mkdir sub3 &&
+ 	(
+ 		cd sub3 &&
+ 		git init &&
+ 		>t &&
+ 		git add t &&
+ 		git commit -m "initial commit" &&
+ 		git submodule add ../subsub dirdir/subsub &&
+ 		git commit -m "add submodule subsub"
+ 	) &&
+ 	mkdir super &&
+ 	(
+ 		cd super &&
+ 		git init &&
+ 		>t &&
+ 		git add t &&
+ 		git commit -m "initial commit" &&
+ 		git submodule add ../sub3 &&
+ 		git commit -m "add submodule sub"
+ 	) &&
+ 	git clone super clone2 &&
+ 	(
+ 		cd clone2 &&
+ 		git submodule update --init --recursive &&
+ 		echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
+ 		echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect
+ 	) &&
+ 	test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
+ 	test_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git
  '
  
  test_expect_success 'submodule add with an existing name fails unless forced' '
@@@ -849,19 -890,6 +890,19 @@@ test_expect_success 'set up a second su
  	git commit -m "submodule example2 added"
  '
  
 +test_expect_success 'submodule deinit works on repository without submodules' '
 +	test_when_finished "rm -rf newdirectory" &&
 +	mkdir newdirectory &&
 +	(
 +		cd newdirectory &&
 +		git init &&
 +		>file &&
 +		git add file &&
 +		git commit -m "repo should not be empty"
 +		git submodule deinit .
 +	)
 +'
 +
  test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '
  	git config submodule.example.foo bar &&
  	git config submodule.example2.frotz nitfol &&
@@@ -1012,30 -1040,5 +1053,30 @@@ test_expect_success 'submodule add clon
  	)
  '
  
 +test_expect_success 'submodule helper list is not confused by common prefixes' '
 +	mkdir -p dir1/b &&
 +	(
 +		cd dir1/b &&
 +		git init &&
 +		echo hi >testfile2 &&
 +		git add . &&
 +		git commit -m "test1"
 +	) &&
 +	mkdir -p dir2/b &&
 +	(
 +		cd dir2/b &&
 +		git init &&
 +		echo hello >testfile1 &&
 +		git add .  &&
 +		git commit -m "test2"
 +	) &&
 +	git submodule add /dir1/b dir1/b &&
 +	git submodule add /dir2/b dir2/b &&
 +	git commit -m "first submodule commit" &&
 +	git submodule--helper list dir1/b |cut -c51- >actual &&
 +	echo "dir1/b" >expect &&
 +	test_cmp expect actual
 +'
 +
  
  test_done