Re: [PATCH] submodule update --init: displays correct path from submodule

[David Turner <novalis@novalis.org>]

(for the test)
Signed-off-by: David Turner <dturner@twosigma.com>

TIL: super-prefix!


On Fri, 2017-01-06 at 16:19 -0800, Stefan Beller wrote:
> In the submodule helper we did not correctly handled the display path
> for initializing submodules when both the submodule is inside a
> subdirectory as well as the command being invoked from a subdirectory
> (as viewed from the superproject).
> 
> This was broken in 3604242f080, which was written at a time where
> there was no super-prefix available, so we abused the --prefix option
> for the same purpose and could get only one case right (the call from
> within a subdirectory, not the submodule being in a subdirectory).
> 
> Test-provided-by: David Turner <novalis@novalis.org>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>   applies on sb/submodule-embed-gitdir as that contains 89c862655
>   (submodule helper: support super prefix)
> 
>  builtin/submodule--helper.c | 13 +++++++------
>  git-submodule.sh            |  2 +-
>  t/t7406-submodule-update.sh | 17 +++++++++++++++++
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 242d9911a6..7b3f9fc293 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -305,32 +305,36 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  
>  		utf8_fprintf(stdout, "%s\n", ce->name);
>  	}
>  	return 0;
>  }
>  
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>  	const struct submodule *sub;
>  	struct strbuf sb = STRBUF_INIT;
>  	char *upd = NULL, *url = NULL, *displaypath;
>  
>  	/* Only loads from .gitmodules, no overlay with .git/config */
>  	gitmodules_config();
>  
> -	if (prefix) {
> -		strbuf_addf(&sb, "%s%s", prefix, path);
> +	if (prefix && get_super_prefix())
> +		die("BUG: cannot have prefix and superprefix");
> +	else if (prefix)
> +		displaypath = xstrdup(relative_path(path, prefix, &sb));
> +	else if (get_super_prefix()) {
> +		strbuf_addf(&sb, "%s%s", get_super_prefix(), path);
>  		displaypath = strbuf_detach(&sb, NULL);
>  	} else
>  		displaypath = xstrdup(path);
>  
>  	sub = submodule_from_path(null_sha1, path);
>  
>  	if (!sub)
>  		die(_("No url found for submodule path '%s' in .gitmodules"),
>  			displaypath);
>  
>  	/*
>  	 * Copy url setting when it is not set yet.
>  	 * To look up the url in .git/config, we must not fall back to
>  	 * .gitmodules, so look it up directly.
>  	 */
> @@ -391,33 +395,30 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  	}
>  	strbuf_release(&sb);
>  	free(displaypath);
>  	free(url);
>  	free(upd);
>  }
>  
>  static int module_init(int argc, const char **argv, const char *prefix)
>  {
>  	struct pathspec pathspec;
>  	struct module_list list = MODULE_LIST_INIT;
>  	int quiet = 0;
>  	int i;
>  
>  	struct option module_init_options[] = {
> -		OPT_STRING(0, "prefix", &prefix,
> -			   N_("path"),
> -			   N_("alternative anchor for relative paths")),
>  		OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
>  		OPT_END()
>  	};
>  
>  	const char *const git_submodule_helper_usage[] = {
>  		N_("git submodule--helper init [<path>]"),
>  		NULL
>  	};
>  
>  	argc = parse_options(argc, argv, prefix, module_init_options,
>  			     git_submodule_helper_usage, 0);
>  
>  	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
>  		return 1;
>  
> @@ -1117,31 +1118,31 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  
>  struct cmd_struct {
>  	const char *cmd;
>  	int (*fn)(int, const char **, const char *);
>  	unsigned option;
>  };
>  
>  static struct cmd_struct commands[] = {
>  	{"list", module_list, 0},
>  	{"name", module_name, 0},
>  	{"clone", module_clone, 0},
>  	{"update-clone", update_clone, 0},
>  	{"relative-path", resolve_relative_path, 0},
>  	{"resolve-relative-url", resolve_relative_url, 0},
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
> -	{"init", module_init, 0},
> +	{"init", module_init, SUPPORT_SUPER_PREFIX},
>  	{"remote-branch", resolve_remote_submodule_branch, 0},
>  	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
>  };
>  
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
>  	if (argc < 2)
>  		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)) {
>  			if (get_super_prefix() &&
>  			    !(commands[i].option & SUPPORT_SUPER_PREFIX))
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9285b5c43d..4e47ff8ad8 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -362,31 +362,31 @@ cmd_init()
>  			;;
>  		--)
>  			shift
>  			break
>  			;;
>  		-*)
>  			usage
>  			;;
>  		*)
>  			break
>  			;;
>  		esac
>  		shift
>  	done
>  
> -	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper init ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} "$@"
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper init ${GIT_QUIET:+--quiet}  "$@"
>  }
>  
>  #
>  # Unregister submodules from .git/config and remove their work tree
>  #
>  cmd_deinit()
>  {
>  	# parse $args after "submodule ... deinit".
>  	deinit_all=
>  	while test $# -ne 0
>  	do
>  		case "$1" in
>  		-f|--force)
>  			force=$1
>  			;;
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 64f322c4cc..725bbed1f8 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -128,30 +128,47 @@ done.
>  Cloning into '$pwd/recursivesuper/super/submodule'...
>  done.
>  EOF
>  
>  test_expect_success 'submodule update --init --recursive from subdirectory' '
>  	git -C recursivesuper/super reset --hard HEAD^ &&
>  	(cd recursivesuper &&
>  	 mkdir tmp &&
>  	 cd tmp &&
>  	 git submodule update --init --recursive ../super >../../actual 2>../../actual2
>  	) &&
>  	test_i18ncmp expect actual &&
>  	test_i18ncmp expect2 actual2
>  '
>  
> +cat <<EOF >expect2
> +Submodule 'foo/sub' ($pwd/withsubs/../rebasing) registered for path 'sub'
> +EOF
> +
> +test_expect_success 'submodule update --init from and of subdirectory' '
> +	git init withsubs &&
> +	(cd withsubs &&
> +	 mkdir foo &&
> +	 git submodule add "$(pwd)/../rebasing" foo/sub &&
> +	 (cd foo &&
> +	  git submodule deinit -f sub &&
> +	  git submodule update --init sub 2>../../actual2
> +	 )
> +	) &&
> +	test_i18ncmp expect2 actual2
> +'
> +
>  apos="'";
>  test_expect_success 'submodule update does not fetch already present commits' '
>  	(cd submodule &&
>  	  echo line3 >> file &&
>  	  git add file &&
>  	  test_tick &&
>  	  git commit -m "upstream line3"
>  	) &&
>  	(cd super/submodule &&
>  	  head=$(git rev-parse --verify HEAD) &&
>  	  echo "Submodule path ${apos}submodule$apos: checked out $apos$head$apos" > ../../expected &&
>  	  git reset --hard HEAD~1
>  	) &&
>  	(cd super &&
>  	  git submodule update > ../actual 2> ../actual.err