All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Port `git submodule init` from shell to C
@ 2016-02-12 23:39 Stefan Beller
  2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
  2016-02-12 23:39 ` [PATCH 2/2] submodule: port init " Stefan Beller
  0 siblings, 2 replies; 15+ messages in thread
From: Stefan Beller @ 2016-02-12 23:39 UTC (permalink / raw)
  To: git, gitster, jrnieder, Jens.Lehmann; +Cc: Stefan Beller

This applies on top of the just sent series which is going to replace
sb/submodule-parallel-update, replacing sb/submodule-init.

* Using enums for the submodule update strategy now.

Thanks,
Stefan

Stefan Beller (2):
  submodule: port resolve_relative_url from shell to C
  submodule: port init from shell to C

 builtin/submodule--helper.c | 315 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            | 118 +----------------
 submodule.c                 |  21 +++
 submodule.h                 |   1 +
 t/t0060-path-utils.sh       |  42 ++++++
 5 files changed, 382 insertions(+), 115 deletions(-)

-- 
2.7.1.292.g18a4ced.dirty

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 1/2] submodule: port resolve_relative_url from shell to C
  2016-02-12 23:39 [PATCH 0/2] Port `git submodule init` from shell to C Stefan Beller
@ 2016-02-12 23:39 ` Stefan Beller
  2016-02-18 20:23   ` Junio C Hamano
                     ` (2 more replies)
  2016-02-12 23:39 ` [PATCH 2/2] submodule: port init " Stefan Beller
  1 sibling, 3 replies; 15+ messages in thread
From: Stefan Beller @ 2016-02-12 23:39 UTC (permalink / raw)
  To: git, gitster, jrnieder, Jens.Lehmann; +Cc: Stefan Beller

Later on we want to automatically call `git submodule init` from
other commands, such that the users don't have to initialize the
submodule themselves.  As these other commands are written in C
already, we'd need the init functionality in C, too.  The
`resolve_relative_url` function is a large part of that init
functionality, so start by porting this function to C.

To create the tests in t0060, the function `resolve_relative_url`
was temporarily enhanced to write all inputs and output to disk
when running the test suite. The added tests in this patch are
a small selection thereof.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 208 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            |  81 +----------------
 t/t0060-path-utils.sh       |  42 +++++++++
 3 files changed, 253 insertions(+), 78 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 65bdc14..d1e9118 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -9,6 +9,210 @@
 #include "submodule-config.h"
 #include "string-list.h"
 #include "run-command.h"
+#include "remote.h"
+#include "refs.h"
+#include "connect.h"
+
+static char *get_default_remote(void)
+{
+	char *dest = NULL, *ret;
+	unsigned char sha1[20];
+	int flag = 0;
+	struct strbuf sb = STRBUF_INIT;
+	const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
+
+	if (!refname)
+		die("No such ref: HEAD");
+
+	/* detached HEAD */
+	if (!strcmp(refname, "HEAD"))
+		return xstrdup("origin");
+
+	if (!skip_prefix(refname, "refs/heads/", &refname))
+		die(_("Expecting a full ref name, got %s"), refname);
+
+	strbuf_addf(&sb, "branch.%s.remote", refname);
+	if (git_config_get_string(sb.buf, &dest))
+		ret = xstrdup("origin");
+	else
+		ret = dest;
+
+	strbuf_release(&sb);
+	return ret;
+}
+
+static int starts_with_dot_slash(const char *str)
+{
+	return str[0] == '.' && is_dir_sep(str[1]);
+}
+
+static int starts_with_dot_dot_slash(const char *str)
+{
+	return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
+}
+
+/*
+ * Returns 1 if it was the last chop before ':'.
+ */
+static int chop_last_dir(char **remoteurl, int is_relative)
+{
+	char *rfind = find_last_dir_sep(*remoteurl);
+	if (rfind) {
+		*rfind = '\0';
+		return 0;
+	}
+
+	rfind = strrchr(*remoteurl, ':');
+	if (rfind) {
+		*rfind = '\0';
+		return 1;
+	}
+
+	if (is_relative || !strcmp(".", *remoteurl))
+		die(_("cannot strip one component off url '%s'"),
+			*remoteurl);
+
+	free(*remoteurl);
+	*remoteurl = xstrdup(".");
+	return 0;
+}
+
+/*
+ * The `url` argument is the URL that navigates to the submodule origin
+ * repo. When relative, this URL is relative to the superproject origin
+ * URL repo. The `up_path` argument, if specified, is the relative
+ * path that navigates from the submodule working tree to the superproject
+ * working tree. Returns the origin URL of the submodule.
+ *
+ * Return either an absolute URL or filesystem path (if the superproject
+ * origin URL is an absolute URL or filesystem path, respectively) or a
+ * relative file system path (if the superproject origin URL is a relative
+ * file system path).
+ *
+ * When the output is a relative file system path, the path is either
+ * relative to the submodule working tree, if up_path is specified, or to
+ * the superproject working tree otherwise.
+ *
+ * NEEDSWORK: This works incorrectly on the domain and protocol part.
+ * remote_url      url              outcome          correct
+ * http://a.com/b  ../c             http://a.com/c   yes
+ * http://a.com/b  ../../c          http://c         no (domain should be kept)
+ * http://a.com/b  ../../../c       http:/c          no
+ * http://a.com/b  ../../../../c    http:c           no
+ * http://a.com/b  ../../../../../c    .:c           no
+ */
+static char *relative_url(const char *remote_url,
+				const char *url,
+				const char *up_path)
+{
+	int is_relative = 0;
+	int colonsep = 0;
+	char *out;
+	char *remoteurl = xstrdup(remote_url);
+	struct strbuf sb = STRBUF_INIT;
+	size_t len = strlen(remoteurl);
+
+	if (is_dir_sep(remoteurl[len]))
+		remoteurl[len] = '\0';
+
+	if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
+		is_relative = 0;
+	else {
+		is_relative = 1;
+		/*
+		 * Prepend a './' to ensure all relative
+		 * remoteurls start with './' or '../'
+		 */
+		if (!starts_with_dot_slash(remoteurl) &&
+		    !starts_with_dot_dot_slash(remoteurl)) {
+			strbuf_reset(&sb);
+			strbuf_addf(&sb, "./%s", remoteurl);
+			free(remoteurl);
+			remoteurl = strbuf_detach(&sb, NULL);
+		}
+	}
+	/*
+	 * When the url starts with '../', remove that and the
+	 * last directory in remoteurl.
+	 */
+	while (url) {
+		if (starts_with_dot_dot_slash(url)) {
+			url += 3;
+			colonsep |= chop_last_dir(&remoteurl, is_relative);
+		} else if (starts_with_dot_slash(url))
+			url += 2;
+		else
+			break;
+	}
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
+
+	if (starts_with_dot_slash(sb.buf))
+		out = xstrdup(sb.buf + 2);
+	else
+		out = xstrdup(sb.buf);
+	strbuf_reset(&sb);
+
+	free(remoteurl);
+	if (!up_path || !is_relative)
+		return out;
+
+	strbuf_addf(&sb, "%s%s", up_path, out);
+	free(out);
+	return strbuf_detach(&sb, NULL);
+}
+
+static int resolve_relative_url(int argc, const char **argv, const char *prefix)
+{
+	char *remoteurl = NULL;
+	char *remote = get_default_remote();
+	const char *up_path = NULL;
+	char *res;
+	const char *url;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (argc != 2 && argc != 3)
+		die("resolve-relative-url only accepts one or two arguments");
+
+	url = argv[1];
+	strbuf_addf(&sb, "remote.%s.url", remote);
+	free(remote);
+
+	if (git_config_get_string(sb.buf, &remoteurl))
+		/* the repository is its own authoritative upstream */
+		remoteurl = xgetcwd();
+
+	if (argc == 3)
+		up_path = argv[2];
+
+	res = relative_url(remoteurl, url, up_path);
+	puts(res);
+	free(res);
+	free(remoteurl);
+	return 0;
+}
+
+static int resolve_relative_url_test(int argc, const char **argv, const char *prefix)
+{
+	char *remoteurl, *res;
+	const char *up_path, *url;
+
+	if (argc != 4)
+		die("resolve-relative-url-test only accepts three arguments: <up_path> <remoteurl> <url>");
+
+	up_path = argv[1];
+	remoteurl = xstrdup(argv[2]);
+	url = argv[3];
+
+	if (!strcmp(up_path, "(null)"))
+		up_path = NULL;
+
+	res = relative_url(remoteurl, url, up_path);
+	puts(res);
+	free(res);
+	free(remoteurl);
+	return 0;
+}
 
 struct module_list {
 	const struct cache_entry **entries;
@@ -502,7 +706,9 @@ static struct cmd_struct commands[] = {
 	{"list", module_list},
 	{"name", module_name},
 	{"clone", module_clone},
-	{"update-clone", update_clone}
+	{"update-clone", update_clone},
+	{"resolve-relative-url", resolve_relative_url},
+	{"resolve-relative-url-test", resolve_relative_url_test},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 10c5af9..615ef9b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -46,79 +46,6 @@ prefix=
 custom_name=
 depth=
 
-# The function takes at most 2 arguments. The first argument is the
-# URL that navigates to the submodule origin repo. When relative, this URL
-# is relative to the superproject origin URL repo. The second up_path
-# argument, if specified, is the relative path that navigates
-# from the submodule working tree to the superproject working tree.
-#
-# The output of the function is the origin URL of the submodule.
-#
-# The output will either be an absolute URL or filesystem path (if the
-# superproject origin URL is an absolute URL or filesystem path,
-# respectively) or a relative file system path (if the superproject
-# origin URL is a relative file system path).
-#
-# When the output is a relative file system path, the path is either
-# relative to the submodule working tree, if up_path is specified, or to
-# the superproject working tree otherwise.
-resolve_relative_url ()
-{
-	remote=$(get_default_remote)
-	remoteurl=$(git config "remote.$remote.url") ||
-		remoteurl=$(pwd) # the repository is its own authoritative upstream
-	url="$1"
-	remoteurl=${remoteurl%/}
-	sep=/
-	up_path="$2"
-
-	case "$remoteurl" in
-	*:*|/*)
-		is_relative=
-		;;
-	./*|../*)
-		is_relative=t
-		;;
-	*)
-		is_relative=t
-		remoteurl="./$remoteurl"
-		;;
-	esac
-
-	while test -n "$url"
-	do
-		case "$url" in
-		../*)
-			url="${url#../}"
-			case "$remoteurl" in
-			*/*)
-				remoteurl="${remoteurl%/*}"
-				;;
-			*:*)
-				remoteurl="${remoteurl%:*}"
-				sep=:
-				;;
-			*)
-				if test -z "$is_relative" || test "." = "$remoteurl"
-				then
-					die "$(eval_gettext "cannot strip one component off url '\$remoteurl'")"
-				else
-					remoteurl=.
-				fi
-				;;
-			esac
-			;;
-		./*)
-			url="${url#./}"
-			;;
-		*)
-			break;;
-		esac
-	done
-	remoteurl="$remoteurl$sep${url%/}"
-	echo "${is_relative:+${up_path}}${remoteurl#./}"
-}
-
 # Resolve a path to be relative to another path.  This is intended for
 # converting submodule paths when git-submodule is run in a subdirectory
 # and only handles paths where the directory separator is '/'.
@@ -281,7 +208,7 @@ cmd_add()
 		die "$(gettext "Relative path can only be used from the toplevel of the working tree")"
 
 		# dereference source url relative to parent's url
-		realrepo=$(resolve_relative_url "$repo") || exit
+		realrepo=$(git submodule--helper resolve-relative-url "$repo") || exit
 		;;
 	*:*|/*)
 		# absolute url
@@ -485,7 +412,7 @@ cmd_init()
 			# Possibly a url relative to parent
 			case "$url" in
 			./*|../*)
-				url=$(resolve_relative_url "$url") || exit
+				url=$(git submodule--helper resolve-relative-url "$url") || exit
 				;;
 			esac
 			git config submodule."$name".url "$url" ||
@@ -1176,9 +1103,9 @@ cmd_sync()
 			# guarantee a trailing /
 			up_path=${up_path%/}/ &&
 			# path from submodule work tree to submodule origin repo
-			sub_origin_url=$(resolve_relative_url "$url" "$up_path") &&
+			sub_origin_url=$(git submodule--helper resolve-relative-url "$url" "$up_path") &&
 			# path from superproject work tree to submodule origin repo
-			super_config_url=$(resolve_relative_url "$url") || exit
+			super_config_url=$(git submodule--helper resolve-relative-url "$url") || exit
 			;;
 		*)
 			sub_origin_url="$url"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index f0152a7..0d2176b 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -19,6 +19,13 @@ relative_path() {
 	"test \"\$(test-path-utils relative_path '$1' '$2')\" = '$expected'"
 }
 
+test_submodule_relative_url() {
+	test_expect_success "test_submodule_relative_url: $1 $2 $3 => $4" "
+		actual=\$(git submodule--helper resolve-relative-url-test '$1' '$2' '$3') &&
+		test \"\$actual\" = '$4'
+	"
+}
+
 test_git_path() {
 	test_expect_success "git-path $1 $2 => $3" "
 		$1 git rev-parse --git-path $2 >actual &&
@@ -289,4 +296,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config
 test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs
 test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow
 
+test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
+test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
+test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
+test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
+test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
+test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/submodule"
+test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
+test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
+test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
+test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
+test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
+test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
+test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
+test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/subsuper_update_r" "../subsubsuper_update_r" "/u//trash directory.t7406-submodule-update/subsubsuper_update_r"
+test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/super_update_r2" "../subsuper_update_r" "/u//trash directory.t7406-submodule-update/subsuper_update_r"
+test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm/." "../." "/u/trash directory.t3600-rm/."
+test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm" "./." "/u/trash directory.t3600-rm/."
+test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
+test_submodule_relative_url "../" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
+test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic" "./å äö" "/u/trash directory.t7400-submodule-basic/å äö"
+test_submodule_relative_url "(null)" "/u/trash directory.t7403-submodule-sync/." "../submodule" "/u/trash directory.t7403-submodule-sync/submodule"
+test_submodule_relative_url "(null)" "/u/trash directory.t7407-submodule-foreach/submodule" "../submodule" "/u/trash directory.t7407-submodule-foreach/submodule"
+test_submodule_relative_url "(null)" "/u/trash directory.t7409-submodule-detached-worktree/home2/../remote" "../bundle1" "/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1"
+test_submodule_relative_url "(null)" "/u/trash directory.t7613-merge-submodule/submodule_update_repo" "./." "/u/trash directory.t7613-merge-submodule/submodule_update_repo/."
+test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
+test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
+test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
+test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
+test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
+test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" "helper:://hostname/subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" "ssh://hostname/subrepo"
+test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh://hostname:22/subrepo"
+test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" "user@host:path/to/subrepo"
+test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" "user@host:subrepo"
+
 test_done
-- 
2.7.1.292.g18a4ced.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 2/2] submodule: port init from shell to C
  2016-02-12 23:39 [PATCH 0/2] Port `git submodule init` from shell to C Stefan Beller
  2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
@ 2016-02-12 23:39 ` Stefan Beller
  2016-02-18 20:46   ` Junio C Hamano
  1 sibling, 1 reply; 15+ messages in thread
From: Stefan Beller @ 2016-02-12 23:39 UTC (permalink / raw)
  To: git, gitster, jrnieder, Jens.Lehmann; +Cc: Stefan Beller

By having the `init` functionality in C, we can reference it easier
from other parts in the code.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  39 +---------------
 submodule.c                 |  21 +++++++++
 submodule.h                 |   1 +
 4 files changed, 130 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d1e9118..30e623a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -214,6 +214,112 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+static void init_submodule(const char *path, const char *prefix, int quiet)
+{
+	const struct submodule *sub;
+	struct strbuf sb = STRBUF_INIT;
+	char *url = NULL;
+	const char *upd = NULL;
+	char *cwd = xgetcwd();
+	const char *displaypath = relative_path(cwd, prefix, &sb);
+
+	/* Only loads from .gitmodules, no overlay with .git/config */
+	gitmodules_config();
+
+	sub = submodule_from_path(null_sha1, path);
+
+	/*
+	 * 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.
+	 */
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (git_config_get_string(sb.buf, &url)) {
+		url = xstrdup(sub->url);
+
+		if (!url)
+			die(_("No url found for submodule path '%s' in .gitmodules"),
+				displaypath);
+
+		/* Possibly a url relative to parent */
+		if (starts_with_dot_dot_slash(url) ||
+		    starts_with_dot_slash(url)) {
+			char *remoteurl;
+			char *remote = get_default_remote();
+			struct strbuf remotesb = STRBUF_INIT;
+			strbuf_addf(&remotesb, "remote.%s.url", remote);
+			free(remote);
+
+			if (git_config_get_string(remotesb.buf, &remoteurl))
+				/*
+				 * The repository is its own
+				 * authoritative upstream
+				 */
+				remoteurl = xgetcwd();
+			url = relative_url(remoteurl, url, NULL);
+			strbuf_release(&remotesb);
+			free(remoteurl);
+		}
+
+		if (git_config_set(sb.buf, url))
+			die(_("Failed to register url for submodule path '%s'"),
+			    displaypath);
+		if (!quiet)
+			printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
+				sub->name, url, displaypath);
+	}
+
+	/* Copy "update" setting when it is not set yet */
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.update", sub->name);
+	if (git_config_get_string_const(sb.buf, &upd) &&
+	    sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
+		if (sub->update_strategy.type == SM_UPDATE_COMMAND) {
+			fprintf(stderr, _("warning: command update mode suggested for submodule '%s'\n"),
+				sub->name);
+			upd = "none";
+		} else
+			upd = submodule_strategy_to_string(&sub->update_strategy);
+
+		if (git_config_set(sb.buf, upd))
+			die(_("Failed to register update mode for submodule path '%s'"), displaypath);
+	}
+	strbuf_release(&sb);
+	free(cwd);
+	free(url);
+}
+
+static int module_init(int argc, const char **argv, const char *prefix)
+{
+	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, "Suppress output for initialzing 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 (argc == 0)
+		die(_("Pass at least one submodule"));
+
+	for (i = 0; i < argc; i++)
+		init_submodule(argv[i], prefix, quiet);
+
+	return 0;
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -709,6 +815,7 @@ static struct cmd_struct commands[] = {
 	{"update-clone", update_clone},
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
+	{"init", module_init}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 615ef9b..6fce0dc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -398,45 +398,8 @@ cmd_init()
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
-		name=$(git submodule--helper name "$sm_path") || exit
-
-		displaypath=$(relative_path "$sm_path")
-
-		# Copy url setting when it is not set yet
-		if test -z "$(git config "submodule.$name.url")"
-		then
-			url=$(git config -f .gitmodules submodule."$name".url)
-			test -z "$url" &&
-			die "$(eval_gettext "No url found for submodule path '\$displaypath' in .gitmodules")"
-
-			# Possibly a url relative to parent
-			case "$url" in
-			./*|../*)
-				url=$(git submodule--helper resolve-relative-url "$url") || exit
-				;;
-			esac
-			git config submodule."$name".url "$url" ||
-			die "$(eval_gettext "Failed to register url for submodule path '\$displaypath'")"
 
-			say "$(eval_gettext "Submodule '\$name' (\$url) registered for path '\$displaypath'")"
-		fi
-
-		# Copy "update" setting when it is not set yet
-		if upd="$(git config -f .gitmodules submodule."$name".update)" &&
-		   test -n "$upd" &&
-		   test -z "$(git config submodule."$name".update)"
-		then
-			case "$upd" in
-			checkout | rebase | merge | none)
-				;; # known modes of updating
-			*)
-				echo >&2 "warning: unknown update mode '$upd' suggested for submodule '$name'"
-				upd=none
-				;;
-			esac
-			git config submodule."$name".update "$upd" ||
-			die "$(eval_gettext "Failed to register update mode for submodule path '\$displaypath'")"
-		fi
+		git submodule--helper init ${GIT_QUIET:+--quiet} "$sm_path" || exit
 	done
 }
 
diff --git a/submodule.c b/submodule.c
index 263cb2a..37d6b8d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -235,6 +235,27 @@ int parse_submodule_update_strategy(const char *value,
 	return 0;
 }
 
+const char *submodule_strategy_to_string(const struct submodule_update_strategy *s)
+{
+	struct strbuf sb = STRBUF_INIT;
+	switch (s->type) {
+	case SM_UPDATE_CHECKOUT:
+		return "checkout";
+	case SM_UPDATE_MERGE:
+		return "merge";
+	case SM_UPDATE_REBASE:
+		return "rebase";
+	case SM_UPDATE_NONE:
+		return "none";
+	case SM_UPDATE_UNSPECIFIED:
+		return NULL;
+	case SM_UPDATE_COMMAND:
+		strbuf_addf(&sb, "!%s", s->command);
+		return strbuf_detach(&sb, 0);
+	}
+	return NULL;
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
diff --git a/submodule.h b/submodule.h
index 7ef3775..ff4c4f3 100644
--- a/submodule.h
+++ b/submodule.h
@@ -39,6 +39,7 @@ int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
 int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
+const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
-- 
2.7.1.292.g18a4ced.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C
  2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
@ 2016-02-18 20:23   ` Junio C Hamano
  2016-02-27  8:28   ` Duy Nguyen
  2016-03-02 17:21   ` Johannes Sixt
  2 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2016-02-18 20:23 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, jrnieder, Jens.Lehmann

Stefan Beller <sbeller@google.com> writes:

> +static int starts_with_dot_slash(const char *str)
> +{
> +	return str[0] == '.' && is_dir_sep(str[1]);
> +}
> +
> +static int starts_with_dot_dot_slash(const char *str)
> +{
> +	return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
> +}
> +
> +/*
> + * Returns 1 if it was the last chop before ':'.
> + */
> +static int chop_last_dir(char **remoteurl, int is_relative)
> +{
> +	char *rfind = find_last_dir_sep(*remoteurl);
> +	if (rfind) {
> +		*rfind = '\0';
> +		return 0;
> +	}
> +
> +	rfind = strrchr(*remoteurl, ':');
> +	if (rfind) {
> +		*rfind = '\0';
> +		return 1;
> +	}
> +
> +	if (is_relative || !strcmp(".", *remoteurl))
> +		die(_("cannot strip one component off url '%s'"),
> +			*remoteurl);
> +
> +	free(*remoteurl);
> +	*remoteurl = xstrdup(".");
> +	return 0;
> +}
> +
> +/*
> + * The `url` argument is the URL that navigates to the submodule origin
> + * repo. When relative, this URL is relative to the superproject origin
> + * URL repo. The `up_path` argument, if specified, is the relative
> + * path that navigates from the submodule working tree to the superproject
> + * working tree. Returns the origin URL of the submodule.
> + *
> + * Return either an absolute URL or filesystem path (if the superproject
> + * origin URL is an absolute URL or filesystem path, respectively) or a
> + * relative file system path (if the superproject origin URL is a relative
> + * file system path).
> + *
> + * When the output is a relative file system path, the path is either
> + * relative to the submodule working tree, if up_path is specified, or to
> + * the superproject working tree otherwise.
> + *
> + * NEEDSWORK: This works incorrectly on the domain and protocol part.
> + * remote_url      url              outcome          correct
> + * http://a.com/b  ../c             http://a.com/c   yes
> + * http://a.com/b  ../../c          http://c         no (domain should be kept)
> + * http://a.com/b  ../../../c       http:/c          no
> + * http://a.com/b  ../../../../c    http:c           no
> + * http://a.com/b  ../../../../../c    .:c           no
> + */

Not just "no" but we should say what the expected outcome is.  I
think all of them should error out?

Given how chop_last_dir() works, I think this function is broken
when a local part has a colon in its path component, too.  I do not
think the scripted version did any better on such a URL; just
another thing that NEEDSWORK comment should list as a thing to be
fixed in the future.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] submodule: port init from shell to C
  2016-02-12 23:39 ` [PATCH 2/2] submodule: port init " Stefan Beller
@ 2016-02-18 20:46   ` Junio C Hamano
  2016-02-18 23:15     ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2016-02-18 20:46 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, jrnieder, Jens.Lehmann

Stefan Beller <sbeller@google.com> writes:

> By having the `init` functionality in C, we can reference it easier
> from other parts in the code.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  39 +---------------
>  submodule.c                 |  21 +++++++++
>  submodule.h                 |   1 +
>  4 files changed, 130 insertions(+), 38 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d1e9118..30e623a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -214,6 +214,112 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>  	return 0;
>  }
>  
> +static void init_submodule(const char *path, const char *prefix, int quiet)
> +{
> +	const struct submodule *sub;
> +	struct strbuf sb = STRBUF_INIT;
> +	char *url = NULL;
> +	const char *upd = NULL;
> +	char *cwd = xgetcwd();
> +	const char *displaypath = relative_path(cwd, prefix, &sb);
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();

This feeds submodule_config() function with the contents of
".gitmodules".

> +	sub = submodule_from_path(null_sha1, path);
> +
> +	/*
> +	 * 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.
> +	 */
> +	strbuf_reset(&sb);
> +	strbuf_addf(&sb, "submodule.%s.url", sub->name);
> +	if (git_config_get_string(sb.buf, &url)) {
> +		url = xstrdup(sub->url);
> +		if (!url)
> +			die(_("No url found for submodule path '%s' in .gitmodules"),
> +				displaypath);

I am assuming that this corresponds to these lines in the original
scripted version:

		url=$(git config -f .gitmodules submodule."$name".url)
		test -z "$url" &&
		die "$(eval_gettext "No url found for submodule path...

but what makes git_config_get_string() to read from ".gitmodules"
file?  Doesn't it read from $GIT_DIR/config & ~/.gitconfig instead?

> +		/* Possibly a url relative to parent */
> +		if (starts_with_dot_dot_slash(url) ||
> +		    starts_with_dot_slash(url)) {
> +			char *remoteurl;
> +			char *remote = get_default_remote();
> +			struct strbuf remotesb = STRBUF_INIT;
> +			strbuf_addf(&remotesb, "remote.%s.url", remote);
> +			free(remote);
> +
> +			if (git_config_get_string(remotesb.buf, &remoteurl))
> +				/*
> +				 * The repository is its own
> +				 * authoritative upstream
> +				 */
> +				remoteurl = xgetcwd();
> +			url = relative_url(remoteurl, url, NULL);
> +			strbuf_release(&remotesb);
> +			free(remoteurl);

Does the code inside this block correspond to this single line in
the original?

	url=$(git submodule--helper resolve-relative-url "$url") || exit

It seems to be doing quite a different thing, though.

> +		}
> +
> +		if (git_config_set(sb.buf, url))
> +			die(_("Failed to register url for submodule path '%s'"),
> +			    displaypath);
> +		if (!quiet)
> +			printf(_("Submodule '%s' (%s) registered for path '%s'\n"),
> +				sub->name, url, displaypath);
> +	}
> +
> +	/* Copy "update" setting when it is not set yet */
> +	strbuf_reset(&sb);
> +	strbuf_addf(&sb, "submodule.%s.update", sub->name);
> +	if (git_config_get_string_const(sb.buf, &upd) &&



This part of the code is supposed to read from in-tree ".gitmodules"
and copy to $GIT_DIR/config (i.e. git_config_set() below), but
again, I am not sure what makes this read from ".gitmodules".

Puzzled.

> +	    sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
> +		if (sub->update_strategy.type == SM_UPDATE_COMMAND) {
> +			fprintf(stderr, _("warning: command update mode suggested for submodule '%s'\n"),
> +				sub->name);
> +			upd = "none";
> +		} else
> +			upd = submodule_strategy_to_string(&sub->update_strategy);
> +
> +		if (git_config_set(sb.buf, upd))
> +			die(_("Failed to register update mode for submodule path '%s'"), displaypath);
> +	}
> +	strbuf_release(&sb);
> +	free(cwd);
> +	free(url);
> +}

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] submodule: port init from shell to C
  2016-02-18 20:46   ` Junio C Hamano
@ 2016-02-18 23:15     ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2016-02-18 23:15 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, jrnieder, Jens.Lehmann

Junio C Hamano <gitster@pobox.com> writes:

>> +	strbuf_reset(&sb);
>> +	strbuf_addf(&sb, "submodule.%s.url", sub->name);
>> +	if (git_config_get_string(sb.buf, &url)) {
>> +		url = xstrdup(sub->url);
>> +		if (!url)
>> +			die(_("No url found for submodule path '%s' in .gitmodules"),
>> +				displaypath);
>
> I am assuming that this corresponds to these lines in the original
> scripted version:
>
> 		url=$(git config -f .gitmodules submodule."$name".url)
> 		test -z "$url" &&
> 		die "$(eval_gettext "No url found for submodule path...
>
> but what makes git_config_get_string() to read from ".gitmodules"
> file?  Doesn't it read from $GIT_DIR/config & ~/.gitconfig instead?

I am starting to suspect that reading of ".gitmodules" in the
context of "git submodule" command is a good use case for the
configset API.  It wants to read variables from ".gitmodules" and
the regular configuration file, and cares about where they come
from, illustrated by this codepath.  Read URL from .gitmodules, do
something to it, and update the regular configuration file.  Read
Update from .gitmodules, do some verification, and selectively store
it in the regular configuration file.  There may be cases where you
want to check if a variable is in the regular configuration file
(i.e. read from there), see its value in ".gitmodules", and
conditionally update the regular configuration file (i.e. write into
it).  The configset API was designed to help implement this kind of
thing in a clean manner (i.e. initialize one, add ".gitmodules"
file, and then configset_get_* on it, without affecting what you
read and write with the regular config_get_*/config_set_* to the
regular configuration file).

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C
  2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
  2016-02-18 20:23   ` Junio C Hamano
@ 2016-02-27  8:28   ` Duy Nguyen
  2016-03-02 17:21   ` Johannes Sixt
  2 siblings, 0 replies; 15+ messages in thread
From: Duy Nguyen @ 2016-02-27  8:28 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, gitster, jrnieder, Jens.Lehmann

On Fri, Feb 12, 2016 at 03:39:15PM -0800, Stefan Beller wrote:
> Later on we want to automatically call `git submodule init` from
> other commands, such that the users don't have to initialize the
> submodule themselves.  As these other commands are written in C
> already, we'd need the init functionality in C, too.  The
> `resolve_relative_url` function is a large part of that init
> functionality, so start by porting this function to C.
> 
> To create the tests in t0060, the function `resolve_relative_url`
> was temporarily enhanced to write all inputs and output to disk
> when running the test suite. The added tests in this patch are
> a small selection thereof.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/submodule--helper.c | 208 +++++++++++++++++++++++++++++++++++++++++++-

i18n fixes. Can you squash this patch in when it's re-rolled?

-- 8< --
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4a0fd7..a6e54fa 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -22,7 +22,7 @@ static char *get_default_remote(void)
 	const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
 
 	if (!refname)
-		die("No such ref: HEAD");
+		die(_("No such ref: %s"), "HEAD");
 
 	/* detached HEAD */
 	if (!strcmp(refname, "HEAD"))
-- 8< --

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C
  2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
  2016-02-18 20:23   ` Junio C Hamano
  2016-02-27  8:28   ` Duy Nguyen
@ 2016-03-02 17:21   ` Johannes Sixt
  2016-03-02 17:34     ` Stefan Beller
  2 siblings, 1 reply; 15+ messages in thread
From: Johannes Sixt @ 2016-03-02 17:21 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, gitster, jrnieder, Jens.Lehmann

Am 13.02.2016 um 00:39 schrieb Stefan Beller:
> @@ -289,4 +296,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config
>   test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs
>   test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow
>   
> +test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
> +test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
> +test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
> +test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
> +test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
> +test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/submodule"
> +test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
> +test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
> +test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
> +test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
> +test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
> +test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
> +test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
> +test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/subsuper_update_r" "../subsubsuper_update_r" "/u//trash directory.t7406-submodule-update/subsubsuper_update_r"
> +test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/super_update_r2" "../subsuper_update_r" "/u//trash directory.t7406-submodule-update/subsuper_update_r"
> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm/." "../." "/u/trash directory.t3600-rm/."
> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm" "./." "/u/trash directory.t3600-rm/."
> +test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
> +test_submodule_relative_url "../" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
> +test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic" "./å äö" "/u/trash directory.t7400-submodule-basic/å äö"
> +test_submodule_relative_url "(null)" "/u/trash directory.t7403-submodule-sync/." "../submodule" "/u/trash directory.t7403-submodule-sync/submodule"
> +test_submodule_relative_url "(null)" "/u/trash directory.t7407-submodule-foreach/submodule" "../submodule" "/u/trash directory.t7407-submodule-foreach/submodule"
> +test_submodule_relative_url "(null)" "/u/trash directory.t7409-submodule-detached-worktree/home2/../remote" "../bundle1" "/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1"
> +test_submodule_relative_url "(null)" "/u/trash directory.t7613-merge-submodule/submodule_update_repo" "./." "/u/trash directory.t7613-merge-submodule/submodule_update_repo/."

The tests with absolute paths all fail on Windows. The reason is that
git.exe sees mangled paths and 'git submodule--helper
resolve-relative-url-test' produces mangled paths (that begins with a
drive letter), whereas the test script expects POSIX paths. The pattern
I currently use to fix this is

test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"

(In our test scripts, $PWD is a POSIX style path and $(pwd) is a
Windows style path).

With this change, the penultimate case above still fails because the
'home2/..' gets lost somewhere in the actual output, which I still have
to debug.

The two cases beginning with '/u//' cannot be tested on Windows.
Are they important? Are the doubled slashes intentional?

> +test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
> +test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
> +test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> +test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
> +test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
> +test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" "helper:://hostname/subrepo"
> +test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" "ssh://hostname/subrepo"
> +test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh://hostname:22/subrepo"
> +test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" "user@host:path/to/subrepo"
> +test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" "user@host:subrepo"
> +
>   test_done
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C
  2016-03-02 17:21   ` Johannes Sixt
@ 2016-03-02 17:34     ` Stefan Beller
  0 siblings, 0 replies; 15+ messages in thread
From: Stefan Beller @ 2016-03-02 17:34 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git, Junio C Hamano, Jonathan Nieder, Jens Lehmann

On Wed, Mar 2, 2016 at 9:21 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 13.02.2016 um 00:39 schrieb Stefan Beller:
>> @@ -289,4 +296,39 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config
>>   test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs
>>   test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow
>>
>> +test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
>> +test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
>> +test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
>> +test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
>> +test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
>> +test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/submodule"
>> +test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
>> +test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
>> +test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
>> +test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
>> +test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
>> +test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
>> +test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
>> +test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/subsuper_update_r" "../subsubsuper_update_r" "/u//trash directory.t7406-submodule-update/subsubsuper_update_r"
>> +test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/super_update_r2" "../subsuper_update_r" "/u//trash directory.t7406-submodule-update/subsuper_update_r"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm/." "../." "/u/trash directory.t3600-rm/."
>> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm" "./." "/u/trash directory.t3600-rm/."
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
>> +test_submodule_relative_url "../" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic" "./å äö" "/u/trash directory.t7400-submodule-basic/å äö"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7403-submodule-sync/." "../submodule" "/u/trash directory.t7403-submodule-sync/submodule"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7407-submodule-foreach/submodule" "../submodule" "/u/trash directory.t7407-submodule-foreach/submodule"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7409-submodule-detached-worktree/home2/../remote" "../bundle1" "/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1"
>> +test_submodule_relative_url "(null)" "/u/trash directory.t7613-merge-submodule/submodule_update_repo" "./." "/u/trash directory.t7613-merge-submodule/submodule_update_repo/."
>
> The tests with absolute paths all fail on Windows. The reason is that
> git.exe sees mangled paths and 'git submodule--helper
> resolve-relative-url-test' produces mangled paths (that begins with a
> drive letter), whereas the test script expects POSIX paths. The pattern
> I currently use to fix this is
>
> test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
>
> (In our test scripts, $PWD is a POSIX style path and $(pwd) is a
> Windows style path).
>
> With this change, the penultimate case above still fails because the
> 'home2/..' gets lost somewhere in the actual output, which I still have
> to debug.
>
> The two cases beginning with '/u//' cannot be tested on Windows.
> Are they important? Are the doubled slashes intentional?

The way I got the test cases is by inserting a debug printf into the
shell script
and then running the test suite (as that ought to work correct and
cover everything).

And the double slash is intentional, somewhere in the test suite we have a case
where that occurs. It may be a calling error?

Thanks for looking into this on the Windows side :)

Thanks,
Stefan

>
>> +test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
>> +test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
>> +test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
>> +test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
>> +test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
>> +test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" "helper:://hostname/subrepo"
>> +test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" "ssh://hostname/subrepo"
>> +test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh://hostname:22/subrepo"
>> +test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" "user@host:path/to/subrepo"
>> +test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" "user@host:subrepo"
>> +
>>   test_done
>>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] Port `git submodule init` from shell to C
  2016-04-13  1:04 ` Junio C Hamano
@ 2016-04-13  1:41   ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2016-04-13  1:41 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, pclouds, j6t

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> This is a resend of origin/sb/submodule-init (and it still applies on 
>> top of submodule-parallel-update)
>
> Resend, or update?  There was some overlap with your recent series
> that fixes "prefix" thing, IIRC.

I've queued one possible resolution on 'pu' and pushed the result
out.  It sends $sm_path to the "helper init", but I think the
implementation of "helper init" now needs to do the equivalent of
"displaypath=$prefix$sm_path" that was done in your prefix fix
series, or something like that.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] Port `git submodule init` from shell to C
  2016-04-13  0:18 [PATCH 0/2] Port `git submodule init` " Stefan Beller
@ 2016-04-13  1:04 ` Junio C Hamano
  2016-04-13  1:41   ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2016-04-13  1:04 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, pclouds, j6t

Stefan Beller <sbeller@google.com> writes:

> This is a resend of origin/sb/submodule-init (and it still applies on 
> top of submodule-parallel-update)

Resend, or update?  There was some overlap with your recent series
that fixes "prefix" thing, IIRC.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 0/2] Port `git submodule init` from shell to C
@ 2016-04-13  0:18 Stefan Beller
  2016-04-13  1:04 ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Beller @ 2016-04-13  0:18 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, j6t, Stefan Beller

This is a resend of origin/sb/submodule-init (and it still applies on 
top of submodule-parallel-update)

I squashed a change which Michael Haggerty proposed in one of the
large cleanup series preparing for the new refs backend. (18/21)
As that is the only fix in that series touching submodules,
picking it up here would untangle sb/submodule-init from that series.

Thanks,
Stefan

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d942463..3078790 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -17,9 +17,8 @@ static char *get_default_remote(void)
 {
        char *dest = NULL, *ret;
        unsigned char sha1[20];
-       int flag = 0;
        struct strbuf sb = STRBUF_INIT;
-       const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
+       const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
 
        if (!refname)
                die(_("No such ref: %s"), "HEAD");


Stefan Beller (2):
  submodule: port resolve_relative_url from shell to C
  submodule: port init from shell to C

 builtin/submodule--helper.c | 316 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            | 118 +----------------
 submodule.c                 |  21 +++
 submodule.h                 |   1 +
 t/t0060-path-utils.sh       |  43 ++++++
 5 files changed, 384 insertions(+), 115 deletions(-)

-- 
2.5.0.264.gc776916.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 0/2] Port `git submodule init` from shell to C
@ 2016-03-15  0:15 Stefan Beller
  0 siblings, 0 replies; 15+ messages in thread
From: Stefan Beller @ 2016-03-15  0:15 UTC (permalink / raw)
  To: git; +Cc: gitster, jrnieder, j6t, pclouds, Stefan Beller

This applies on top of origin/sb/submodule-parallel-update.
By having the `init` functionality in C, we can reference it easier
from other parts in the code.

Thanks for the reviews a long time ago!

Junio,
  I fixed the NEEDSWORK comment for relative_url
  and thought about the memory allocation
Duy,
  I fixed the i18n issues introduced.
Johannes,
  I took $PWD instead of absolute paths. According to your last message, this
  may still break on Windows for 
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$PWD/home2/../bundle1"

There are however no further tests starting with /.

Thanks,
Stefan

Stefan Beller (2):
  submodule: port resolve_relative_url from shell to C
  submodule: port init from shell to C

 builtin/submodule--helper.c | 317 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            | 118 +----------------
 submodule.c                 |  21 +++
 submodule.h                 |   1 +
 t/t0060-path-utils.sh       |  43 ++++++
 5 files changed, 385 insertions(+), 115 deletions(-)

Diff to origin/sb/submodule-init:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 213af2e..d942463 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -22,7 +22,7 @@ static char *get_default_remote(void)
 	const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
 
 	if (!refname)
-		die("No such ref: HEAD");
+		die(_("No such ref: %s"), "HEAD");
 
 	/* detached HEAD */
 	if (!strcmp(refname, "HEAD"))
@@ -94,12 +94,14 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * the superproject working tree otherwise.
  *
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url      url              outcome          correct
- * http://a.com/b  ../c             http://a.com/c   yes
- * http://a.com/b  ../../c          http://c         no (domain should be kept)
- * http://a.com/b  ../../../c       http:/c          no
- * http://a.com/b  ../../../../c    http:c           no
- * http://a.com/b  ../../../../../c    .:c           no
+ * remote_url      url              outcome          expectation
+ * http://a.com/b  ../c             http://a.com/c   as is
+ * http://a.com/b  ../../c          http://c         error out
+ * http://a.com/b  ../../../c       http:/c          error out
+ * http://a.com/b  ../../../../c    http:c           error out
+ * http://a.com/b  ../../../../../c    .:c           error out
+ * NEEDSWORK: Given how chop_last_dir() works, this function is broken
+ * when a local part has a colon in its path component, too.
  */
 static char *relative_url(const char *remote_url,
 				const char *url,
@@ -146,6 +148,7 @@ static char *relative_url(const char *remote_url,
 	}
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
+	free(remoteurl);
 
 	if (starts_with_dot_slash(sb.buf))
 		out = xstrdup(sb.buf + 2);
@@ -153,7 +156,6 @@ static char *relative_url(const char *remote_url,
 		out = xstrdup(sb.buf);
 	strbuf_reset(&sb);
 
-	free(remoteurl);
 	if (!up_path || !is_relative)
 		return out;
 
@@ -299,7 +301,7 @@ static int module_init(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("alternative anchor for relative paths")),
-		OPT__QUIET(&quiet, "Suppress output for initialzing a submodule"),
+		OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
 		OPT_END()
 	};
 
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 8a1579c..579c1fa 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -293,35 +293,36 @@ test_git_path GIT_COMMON_DIR=bar config                   bar/config
 test_git_path GIT_COMMON_DIR=bar packed-refs              bar/packed-refs
 test_git_path GIT_COMMON_DIR=bar shallow                  bar/shallow
 
-test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
+test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
+test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
+test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/submodule"
+test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
+test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
+test_submodule_relative_url "../" "$PWD/addtest" "../repo" "$PWD/repo"
+test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
+test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
+
+test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
-test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
 test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
-test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/submodule"
 test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
-test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
 test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
-test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
-test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
-test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/subsuper_update_r" "../subsubsuper_update_r" "/u//trash directory.t7406-submodule-update/subsubsuper_update_r"
-test_submodule_relative_url "(null)" "/u//trash directory.t7406-submodule-update/super_update_r2" "../subsuper_update_r" "/u//trash directory.t7406-submodule-update/subsuper_update_r"
-test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm/." "../." "/u/trash directory.t3600-rm/."
-test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm" "./." "/u/trash directory.t3600-rm/."
-test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
-test_submodule_relative_url "../" "/u/trash directory.t7400-submodule-basic/addtest" "../repo" "/u/trash directory.t7400-submodule-basic/repo"
-test_submodule_relative_url "(null)" "/u/trash directory.t7400-submodule-basic" "./å äö" "/u/trash directory.t7400-submodule-basic/å äö"
-test_submodule_relative_url "(null)" "/u/trash directory.t7403-submodule-sync/." "../submodule" "/u/trash directory.t7403-submodule-sync/submodule"
-test_submodule_relative_url "(null)" "/u/trash directory.t7407-submodule-foreach/submodule" "../submodule" "/u/trash directory.t7407-submodule-foreach/submodule"
-test_submodule_relative_url "(null)" "/u/trash directory.t7409-submodule-detached-worktree/home2/../remote" "../bundle1" "/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1"
-test_submodule_relative_url "(null)" "/u/trash directory.t7613-merge-submodule/submodule_update_repo" "./." "/u/trash directory.t7613-merge-submodule/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$PWD/subsubsuper_update_r"
+test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$PWD/subsuper_update_r"
+test_submodule_relative_url "(null)" "$PWD/." "../." "$PWD/."
+test_submodule_relative_url "(null)" "$PWD" "./." "$PWD/."
+test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$PWD/repo"
+test_submodule_relative_url "(null)" "$PWD" "./å äö" "$PWD/å äö"
+test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$PWD/submodule"
+test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$PWD/submodule"
+test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$PWD/home2/../bundle1"
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$PWD/submodule_update_repo/."
 test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
 test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
-test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
-test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" "helper:://hostname/subrepo"
 test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" "ssh://hostname/subrepo"
 test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh://hostname:22/subrepo"


-- 
2.7.0.rc0.46.g8f16ed4.dirty

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] Port `git submodule init` from shell to C
  2016-01-15 23:37 Stefan Beller
@ 2016-01-19 23:17 ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2016-01-19 23:17 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, j6t, sunshine, Jens.Lehmann

I've queued these with a bit of tweak in their log message.

Thanks.

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 0/2] Port `git submodule init` from shell to C
@ 2016-01-15 23:37 Stefan Beller
  2016-01-19 23:17 ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Beller @ 2016-01-15 23:37 UTC (permalink / raw)
  To: git; +Cc: gitster, j6t, sunshine, Jens.Lehmann, Stefan Beller

The first patch is a reroll of "[PATCH] submodule: Port resolve_relative_url
from shell to C" and the second patch is new.

This applies on top of origin/sb/submodule-parallel-update, as it makes use
of the recorded update strategy in the submodule configuration.

Thanks,
Stefan

Stefan Beller (2):
  submodule: Port resolve_relative_url from shell to C
  submodule: Port init from shell to C

 builtin/submodule--helper.c | 330 +++++++++++++++++++++++++++++++++++++++++++-
 git-submodule.sh            | 118 +---------------
 t/t0060-path-utils.sh       |  42 ++++++
 3 files changed, 370 insertions(+), 120 deletions(-)

-- 
2.7.0.rc0.37.gb4a0220.dirty

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-04-13  1:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 23:39 [PATCH 0/2] Port `git submodule init` from shell to C Stefan Beller
2016-02-12 23:39 ` [PATCH 1/2] submodule: port resolve_relative_url " Stefan Beller
2016-02-18 20:23   ` Junio C Hamano
2016-02-27  8:28   ` Duy Nguyen
2016-03-02 17:21   ` Johannes Sixt
2016-03-02 17:34     ` Stefan Beller
2016-02-12 23:39 ` [PATCH 2/2] submodule: port init " Stefan Beller
2016-02-18 20:46   ` Junio C Hamano
2016-02-18 23:15     ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2016-04-13  0:18 [PATCH 0/2] Port `git submodule init` " Stefan Beller
2016-04-13  1:04 ` Junio C Hamano
2016-04-13  1:41   ` Junio C Hamano
2016-03-15  0:15 Stefan Beller
2016-01-15 23:37 Stefan Beller
2016-01-19 23:17 ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.