[GSoC] Update: Week 5

[GSoC] Update: Week 5

[Prathamesh Chavan]

SUMMARY OF MY PROJECT:

Git submodule subcommands are currently implemented by using shell script
'git-submodule.sh'. There are several reasons why we'll prefer not to
use the shell script. My project intends to convert the subcommands into
C code, thus making them builtins. This will increase Git's portability
and hence the efficiency of working with the git-submodule commands.
Link to the complete proposal: [1]

Mentors:
Stefan Beller <sbeller@google.com>
Christian Couder <christian.couder@gmail.com>

UPDATES:

Following are the updates about my ongoing project:

1. sync and status: The patches were discussed with the mentors
   and after that, are being posted with this patch.

2. deinit: The patch is finally debugged, and is ready to be
   discussed. It is also attached with this update.

3. summary: While porting the subcommand, I underwent certain
   issues. After getting them clarified from my mentors, I
   have resumed working on it. I'm aware of the time I have
   taken for porting this subcommand is more than the previous
   ones. Hence will try my best to finish this in this week.

4. foreach: As stated in the previous update, the subcommand was
   ported without resolving the bug, and simply translating the
   present code, and adding a NEEDSWORK tag to the comment for
   mentioning the reported bug as well.
   But as communicating between child_process is still an issue
   and so there was no simple was to current carry out the
   porting. And hence, a hack was used instead. But after
   discussing it, instead using the repository-object patch
   series will help to resolve these issues in this situation.

PLAN FOR WEEK-6 (20 June 2017 to 26 June 2017):

1. summary: Mostly I'll be working on this and post the patch
   for discussion as soon as possible.

2. foreach: As it was decided that unblock the conversion of
   this submodule subcommand, the original cmd_foreach was
   ported without including the BUG-FIX patch here.
   Hence, for this week I will try to utilize the
   'repository-object' series by Brandon Williams.

3. deinit: I will be working on improvising this patch as it was
   recently debugged and posted for discussion.

[1]: https://docs.google.com/document/d/1krxVLooWl--75Pot3dazhfygR3wCUUWZWzTXtK1L-xU/

Thanks,
Prathamesh Chavan

[GSoC][PATCH 1/6] dir: create function count_slashes

[Prathamesh Chavan]

Similar functions exist in apply.c and builtin/show-branch.c for
counting the number of slashes in a string. Also in the later
patches, we introduce a third caller for the same. Hence, we unify
it now by cleaning the existing functions and declaring a common
function count_slashes in dir.h and implementing it in dir.c to
remove this code duplication.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
The complete build report of this is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: All-patch-series
Build #111

 apply.c               | 11 -----------
 builtin/show-branch.c | 13 +++----------
 dir.c                 |  9 +++++++++
 dir.h                 |  3 +++
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/apply.c b/apply.c
index c49cef063..121e53406 100644
--- a/apply.c
+++ b/apply.c
@@ -762,17 +762,6 @@ static char *find_name_traditional(struct apply_state *state,
 	return find_name_common(state, line, def, p_value, line + len, 0);
 }
 
-static int count_slashes(const char *cp)
-{
-	int cnt = 0;
-	char ch;
-
-	while ((ch = *cp++))
-		if (ch == '/')
-			cnt++;
-	return cnt;
-}
-
 /*
  * Given the string after "--- " or "+++ ", guess the appropriate
  * p_value for the given patch.
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4a6cc6f49..3636a0559 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -5,6 +5,7 @@
 #include "color.h"
 #include "argv-array.h"
 #include "parse-options.h"
+#include "dir.h"
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
@@ -421,14 +422,6 @@ static int append_tag_ref(const char *refname, const struct object_id *oid,
 
 static const char *match_ref_pattern = NULL;
 static int match_ref_slash = 0;
-static int count_slash(const char *s)
-{
-	int cnt = 0;
-	while (*s)
-		if (*s++ == '/')
-			cnt++;
-	return cnt;
-}
 
 static int append_matching_ref(const char *refname, const struct object_id *oid,
 			       int flag, void *cb_data)
@@ -438,7 +431,7 @@ static int append_matching_ref(const char *refname, const struct object_id *oid,
 	 * refs/tags/v0.99.9a and friends.
 	 */
 	const char *tail;
-	int slash = count_slash(refname);
+	int slash = count_slashes(refname);
 	for (tail = refname; *tail && match_ref_slash < slash; )
 		if (*tail++ == '/')
 			slash--;
@@ -529,7 +522,7 @@ static void append_one_rev(const char *av)
 		int saved_matches = ref_name_cnt;
 
 		match_ref_pattern = av;
-		match_ref_slash = count_slash(av);
+		match_ref_slash = count_slashes(av);
 		for_each_ref(append_matching_ref, NULL);
 		if (saved_matches == ref_name_cnt &&
 		    ref_name_cnt < MAX_REVS)
diff --git a/dir.c b/dir.c
index 9efcf1eab..4a953c16a 100644
--- a/dir.c
+++ b/dir.c
@@ -52,6 +52,15 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
 static int get_dtype(struct dirent *de, struct index_state *istate,
 		     const char *path, int len);
 
+int count_slashes(const char *s)
+{
+	int cnt = 0;
+	while (*s)
+		if (*s++ == '/')
+			cnt++;
+	return cnt;
+}
+
 int fspathcmp(const char *a, const char *b)
 {
 	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
diff --git a/dir.h b/dir.h
index a89c13e27..e3717055d 100644
--- a/dir.h
+++ b/dir.h
@@ -197,6 +197,9 @@ struct dir_struct {
 	unsigned unmanaged_exclude_files;
 };
 
+/*Count the number of slashes for string s*/
+extern int count_slashes(const char *s);
+
 /*
  * The ordering of these constants is significant, with
  * higher-numbered match types signifying "closer" (i.e. more
-- 
2.13.0

[GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_list

[Prathamesh Chavan]

Functions get_submodule_displaypath and for_each_submodule_list
for using them in the later patches, related to porting submodule
subcommands from shell to C.
These new functions are also used in ported submodule subcommand
init

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 69 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8cc648d85..f7adca95b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,9 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
+				      void *cb_data);
+
 static char *get_default_remote(void)
 {
 	char *dest = NULL, *ret;
@@ -219,6 +222,27 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+	const char *super_prefix = get_super_prefix();
+
+	if (prefix && super_prefix) {
+		BUG("cannot have prefix '%s' and superprefix '%s'",
+		    prefix, super_prefix);
+	} else if (prefix) {
+		struct strbuf sb = STRBUF_INIT;
+		char *displaypath = xstrdup(relative_path(path, prefix, &sb));
+		strbuf_release(&sb);
+		return displaypath;
+	} else if (super_prefix) {
+		int len = strlen(super_prefix);
+		const char *format = is_dir_sep(super_prefix[len-1]) ? "%s%s" : "%s/%s";
+		return xstrfmt(format, super_prefix, path);
+	} else {
+		return xstrdup(path);
+	}
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -330,26 +354,30 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_submodule_list(const struct module_list list,
+				    submodule_list_func_t fn, void *cb_data)
 {
+	int i;
+	for (i = 0; i < list.nr; i++)
+		fn(list.entries[i], cb_data);
+}
+
+struct init_cb {
+	const char *prefix;
+	unsigned int quiet: 1;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct init_cb *info = cb_data;
 	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 && 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);
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
 
-	sub = submodule_from_path(null_sha1, path);
+	sub = submodule_from_path(null_sha1, list_item->name);
 
 	if (!sub)
 		die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -361,7 +389,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 	 *
 	 * Set active flag for the submodule being initialized
 	 */
-	if (!is_submodule_initialized(path)) {
+	if (!is_submodule_initialized(list_item->name)) {
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "submodule.%s.active", sub->name);
 		git_config_set_gently(sb.buf, "true");
@@ -404,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 		if (git_config_set_gently(sb.buf, url))
 			die(_("Failed to register url for submodule path '%s'"),
 			    displaypath);
-		if (!quiet)
+		if (!info->quiet)
 			fprintf(stderr,
 				_("Submodule '%s' (%s) registered for path '%s'\n"),
 				sub->name, url, displaypath);
@@ -433,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
 
 static int module_init(int argc, const char **argv, const char *prefix)
 {
+	struct init_cb info = INIT_CB_INIT;
 	struct pathspec pathspec;
 	struct module_list list = MODULE_LIST_INIT;
 	int quiet = 0;
-	int i;
 
 	struct option module_init_options[] = {
 		OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
@@ -461,8 +489,11 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	if (!argc && git_config_get_value_multi("submodule.active"))
 		module_list_active(&list);
 
-	for (i = 0; i < list.nr; i++)
-		init_submodule(list.entries[i]->name, prefix, quiet);
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+
+	gitmodules_config();
+	for_each_submodule_list(list, init_submodule, &info);
 
 	return 0;
 }
-- 
2.13.0

[GSoC][PATCH 3/6] submodule: port set_name_rev from shell to C

[Prathamesh Chavan]

Since later on we want to port submodule subcommand status, and since
set_name_rev is part of cmd_status, hence this function is ported. It
has been ported to function print_name_rev in C, which calls get_name_rev
to get the revname, and after formatting it, print_name_rev prints it.
And hence in this way, the command `git submodule--helper print-name-rev
"sm_path" "sha1"` sets value of revname in git-submodule.sh

The function get_name_rev returns the stdout of the git describe
commands. Since there are four different git-describe commands used for
generating the name rev, four child_process are introduced, each successive
child process running only when previous has no stdout. The order of these
four git-describe commands is maintained the same as it was in the function
set_name_rev() in shell script.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 16 ++---------
 2 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f7adca95b..6fd861e42 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -243,6 +243,74 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)
 	}
 }
 
+enum describe_step {
+	step_bare,
+	step_tags,
+	step_contains,
+	step_all_always,
+	step_end
+};
+
+static char *get_name_rev(const char *sub_path, const char* object_id)
+{
+	struct strbuf sb = STRBUF_INIT;
+	enum describe_step cur_step;
+
+	for (cur_step = step_bare; cur_step < step_end; cur_step++) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.dir = sub_path;
+		cp.git_cmd = 1;
+		cp.no_stderr = 1;
+
+		switch (cur_step) {
+			case step_bare:
+				argv_array_pushl(&cp.args, "describe",
+						 object_id, NULL);
+				break;
+			case step_tags:	
+				argv_array_pushl(&cp.args, "describe",
+						 "--tags", object_id, NULL);
+				break;
+			case step_contains:
+				argv_array_pushl(&cp.args, "describe",
+						 "--contains", object_id,
+						 NULL);
+				break;
+			case step_all_always:
+				argv_array_pushl(&cp.args, "describe",
+						 "--all", "--always",
+						 object_id, NULL);
+				break;
+			default:
+				BUG("unknown describe step '%d'", cur_step);
+		}
+
+		if (!capture_command(&cp, &sb, 0) && sb.len) {
+			strbuf_strip_suffix(&sb, "\n");
+			return strbuf_detach(&sb, NULL);
+		}
+
+	}
+
+	strbuf_release(&sb);
+	return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+	char *namerev;
+	if (argc != 3)
+		die("print-name-rev only accepts two arguments: <path> <sha1>");
+
+	namerev = get_name_rev(argv[1], argv[2]);
+	if (namerev && namerev[0])
+		printf(" (%s)", namerev);
+	printf("\n");
+
+	return 0;
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -1242,6 +1310,7 @@ static struct cmd_struct commands[] = {
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url", resolve_relative_url, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
+	{"print-name-rev", print_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index c0d0e9a4c..091051891 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -758,18 +758,6 @@ cmd_update()
 	}
 }
 
-set_name_rev () {
-	revname=$( (
-		sanitize_submodule_env
-		cd "$1" && {
-			git describe "$2" 2>/dev/null ||
-			git describe --tags "$2" 2>/dev/null ||
-			git describe --contains "$2" 2>/dev/null ||
-			git describe --all --always "$2"
-		}
-	) )
-	test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1041,14 +1029,14 @@ cmd_status()
 		fi
 		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
 		then
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
 			say " $sha1 $displaypath$revname"
 		else
 			if test -z "$cached"
 			then
 				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
 			fi
-			set_name_rev "$sm_path" "$sha1"
+			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
 			say "+$sha1 $displaypath$revname"
 		fi
 
-- 
2.13.0

[GSoC][PATCH 4/6] submodule: port submodule subcommand status

[Prathamesh Chavan]

The mechanism used for porting submodule subcommand 'status'
is similar to that used for subcommand 'foreach'.
The function cmd_status from git-submodule is ported to three
functions in the builtin submodule--helper namely: module_status,
for_each_submodule_list and status_submodule.

print_status is also introduced for handling the output of
the subcommand and also to reduce the code size.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 152 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  49 +-------------
 2 files changed, 153 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6fd861e42..78b21ab22 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -566,6 +566,157 @@ static int module_init(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct status_cb {
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+	unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+			 char *sub_sha1, char *displaypath)
+{
+	if (info->quiet)
+		return;
+
+	printf("%c%s %s", state, sub_sha1, displaypath);
+
+	if (state == ' ' || state == '+') {
+		struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+		argv_array_pushl(&name_rev_args, "print-name-rev",
+				 path, sub_sha1, NULL);
+		print_name_rev(name_rev_args.argc, name_rev_args.argv,
+			       info->prefix);
+	} else {
+		printf("\n");
+	}
+}
+
+static void status_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct status_cb *info = cb_data;
+	char *sub_sha1 = xstrdup(oid_to_hex(&list_item->oid));
+	char *displaypath;
+	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+	if (!submodule_from_path(null_sha1, list_item->name))
+		die(_("no submodule mapping found in .gitmodules for path '%s'"),
+		      list_item->name);
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	if (list_item->ce_flags) {
+		print_status(info, 'U', list_item->name,
+			     sha1_to_hex(null_sha1), displaypath);
+		goto cleanup;
+	}
+
+	if (!is_submodule_initialized(list_item->name)) {
+		print_status(info, '-', list_item->name, sub_sha1, displaypath);
+		goto cleanup;
+	}
+
+	argv_array_pushl(&diff_files_args, "diff-files",
+			 "--ignore-submodules=dirty", "--quiet", "--",
+			 list_item->name, NULL);
+
+	if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+			    info->prefix)) {
+		print_status(info, ' ', list_item->name, sub_sha1, displaypath);
+	} else {
+		if (!info->cached) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf sb = STRBUF_INIT;
+
+			prepare_submodule_repo_env(&cp.env_array);
+			cp.git_cmd = 1;
+			cp.dir = list_item->name;
+
+			argv_array_pushl(&cp.args, "rev-parse",
+					 "--verify", "HEAD", NULL);
+
+			if (capture_command(&cp, &sb, 0))
+				die(_("could not run 'git rev-parse --verify"
+				      "HEAD' in submodule %s"),
+				      list_item->name);
+
+			strbuf_strip_suffix(&sb, "\n");
+			print_status(info, '+', list_item->name, sb.buf,
+				     displaypath);
+			strbuf_release(&sb);
+		} else {
+			print_status(info, '+', list_item->name, sub_sha1,
+				     displaypath);
+		}
+	}
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
+				 "submodule--helper", "status", "--recursive",
+				 NULL);
+
+		if (info->cached)
+			argv_array_push(&cpr.args, "--cached");
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      list_item->name);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_sha1);
+}
+
+static int module_status(int argc, const char **argv, const char *prefix)
+{
+	struct status_cb info = STATUS_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int cached = 0;
+	int recursive = 0;
+
+	struct option module_status_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
+		OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_status_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.recursive = !!recursive;
+	info.cached = !!cached;
+
+	gitmodules_config();
+	for_each_submodule_list(list, status_submodule, &info);
+
+	return 0;
+}
+
 static int module_name(int argc, const char **argv, const char *prefix)
 {
 	const struct submodule *sub;
@@ -1312,6 +1463,7 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"print-name-rev", print_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
+	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 091051891..a24b1b91b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1004,54 +1004,7 @@ cmd_status()
 		shift
 	done
 
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-		if test "$stage" = U
-		then
-			say "U$sha1 $displaypath"
-			continue
-		fi
-		if ! git submodule--helper is-active "$sm_path" ||
-		{
-			! test -d "$sm_path"/.git &&
-			! test -f "$sm_path"/.git
-		}
-		then
-			say "-$sha1 $displaypath"
-			continue;
-		fi
-		if git diff-files --ignore-submodules=dirty --quiet -- "$sm_path"
-		then
-			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
-			say " $sha1 $displaypath$revname"
-		else
-			if test -z "$cached"
-			then
-				sha1=$(sanitize_submodule_env; cd "$sm_path" && git rev-parse --verify HEAD)
-			fi
-			revname=$(git submodule--helper print-name-rev "$sm_path" "$sha1")
-			say "+$sha1 $displaypath$revname"
-		fi
-
-		if test -n "$recursive"
-		then
-			(
-				prefix="$displaypath/"
-				sanitize_submodule_env
-				wt_prefix=
-				cd "$sm_path" &&
-				eval cmd_status
-			) ||
-			die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
 }
 #
 # Sync remote urls for submodules
-- 
2.13.0

[GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to C

[Prathamesh Chavan]

The mechanism used for porting the submodule subcommand 'sync' is
similar to that of 'foreach', where we split the function cmd_sync
from shell into three functions in C, module_sync,
for_each_submodule_list and sync_submodule.

print_default_remote is introduced as a submodule--helper
subcommand for getting the default remote as stdout.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 180 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  56 +-------------
 2 files changed, 181 insertions(+), 55 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 78b21ab22..e10cac462 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -43,6 +43,20 @@ static char *get_default_remote(void)
 	return ret;
 }
 
+static int print_default_remote(int argc, const char **argv, const char *prefix)
+{
+	const char *remote;
+
+	if (argc != 1)
+		die(_("submodule--helper print-default-remote takes no arguments"));
+
+	remote = get_default_remote();
+	if (remote)
+		puts(remote);
+
+	return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
 	return str[0] == '.' && is_dir_sep(str[1]);
@@ -311,6 +325,25 @@ static int print_name_rev(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static char *get_up_path(const char *path)
+{
+	int i = count_slashes(path);
+	struct strbuf sb = STRBUF_INIT;
+
+	while (i--)
+		strbuf_addstr(&sb, "../");
+
+	/*
+	 *Check if 'path' ends with slash or not
+	 *for having the same output for dir/sub_dir
+	 *and dir/sub_dir/
+	 */
+	if (!is_dir_sep(path[i - 1]))
+		strbuf_addstr(&sb, "../");
+
+	return strbuf_detach(&sb, NULL);
+}
+
 struct module_list {
 	const struct cache_entry **entries;
 	int alloc, nr;
@@ -736,6 +769,151 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct sync_cb {
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int recursive: 1;
+};
+#define SYNC_CB_INIT { NULL, 0, 0 }
+
+static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+	struct sync_cb *info = cb_data;
+	const struct submodule *sub;
+	char *sub_key, *remote_key;
+	char *url, *sub_origin_url, *super_config_url, *displaypath;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	if (!is_submodule_initialized(list_item->name))
+		return;
+
+	sub = submodule_from_path(null_sha1, list_item->name);
+
+	if (!sub->url)
+		die(_("no url found for submodule path '%s' in .gitmodules"),
+		      list_item->name);
+
+	url = xstrdup(sub->url);
+
+	if (starts_with_dot_dot_slash(url) || starts_with_dot_slash(url)) {
+		char *remote_url, *up_path;
+		char *remote = get_default_remote();
+		char *remote_key = xstrfmt("remote.%s.url", remote);
+		free(remote);
+
+		if (git_config_get_string(remote_key, &remote_url))
+			remote_url = xgetcwd();
+		up_path = get_up_path(list_item->name);
+		sub_origin_url = relative_url(remote_url, url, up_path);
+		super_config_url = relative_url(remote_url, url, NULL);
+		free(remote_key);
+		free(up_path);
+		free(remote_url);
+	} else {
+		sub_origin_url = xstrdup(url);
+		super_config_url = xstrdup(url);
+	}
+
+	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+	if (!info->quiet)
+		printf(_("Synchronizing submodule url for '%s'\n"),
+			 displaypath);
+
+	sub_key = xstrfmt("submodule.%s.url", sub->name);
+	if (git_config_set_gently(sub_key, super_config_url))
+		die(_("failed to register url for submodule path '%s'"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(list_item->name, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.dir = list_item->name;
+	argv_array_pushl(&cp.args, "submodule--helper",
+			 "print-default-remote", NULL);
+	if (capture_command(&cp, &sb, 0))
+		die(_("failed to get the default remote for submodule '%s'"),
+		      list_item->name);
+
+	strbuf_strip_suffix(&sb, "\n");
+	remote_key = xstrfmt("remote.%s.url", sb.buf);
+	strbuf_release(&sb);
+
+	child_process_init(&cp);
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.dir = list_item->name;
+	argv_array_pushl(&cp.args, "config", remote_key, sub_origin_url, NULL);
+	if (run_command(&cp))
+		die(_("failed to update remote for submodule '%s'"),
+		      list_item->name);
+
+	if (info->recursive) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = list_item->name;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
+				 "submodule--helper", "sync", "--recursive",
+				 NULL);
+
+		if (info->quiet)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      list_item->name);
+	}
+
+cleanup:
+	free(sub_key);
+	free(url);
+	free(super_config_url);
+	free(displaypath);
+	free(sub_origin_url);
+}
+
+static int module_sync(int argc, const char **argv, const char *prefix)
+{
+	struct sync_cb info = SYNC_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int recursive = 0;
+
+	struct option module_sync_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output of synchronizing submodule url")),
+		OPT_BOOL(0, "recursive", &recursive,
+			N_("Recurse into nested submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_sync_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.recursive = !!recursive;
+
+	gitmodules_config();
+	for_each_submodule_list(list, sync_submodule, &info);
+
+	return 0;
+}
+
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
 			   const char *depth, struct string_list *reference,
 			   int quiet, int progress)
@@ -1464,6 +1642,8 @@ static struct cmd_struct commands[] = {
 	{"print-name-rev", print_name_rev, 0},
 	{"init", module_init, SUPPORT_SUPER_PREFIX},
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
+	{"print-default-remote", print_default_remote, 0},
+	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index a24b1b91b..33b4b7306 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,63 +1036,9 @@ cmd_sync()
 			;;
 		esac
 	done
-	cd_to_toplevel
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-
-		# skip inactive submodules
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			continue
-		fi
-
-		name=$(git submodule--helper name "$sm_path")
-		url=$(git config -f .gitmodules --get submodule."$name".url)
-
-		# Possibly a url relative to parent
-		case "$url" in
-		./*|../*)
-			# rewrite foo/bar as ../.. to find path from
-			# submodule work tree to superproject work tree
-			up_path="$(printf '%s\n' "$sm_path" | sed "s/[^/][^/]*/../g")" &&
-			# guarantee a trailing /
-			up_path=${up_path%/}/ &&
-			# path from submodule work tree to submodule origin repo
-			sub_origin_url=$(git submodule--helper resolve-relative-url "$url" "$up_path") &&
-			# path from superproject work tree to submodule origin repo
-			super_config_url=$(git submodule--helper resolve-relative-url "$url") || exit
-			;;
-		*)
-			sub_origin_url="$url"
-			super_config_url="$url"
-			;;
-		esac
 
-		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
-		say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
-		git config submodule."$name".url "$super_config_url"
-
-		if test -e "$sm_path"/.git
-		then
-		(
-			sanitize_submodule_env
-			cd "$sm_path"
-			remote=$(get_default_remote)
-			git config remote."$remote".url "$sub_origin_url"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 
-			if test -n "$recursive"
-			then
-				prefix="$prefix$sm_path/"
-				eval cmd_sync
-			fi
-		)
-		fi
-	done
 }
 
 cmd_absorbgitdirs()
-- 
2.13.0

[GSoC][PATCH 6/6] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into three
functions: module_deinit, for_each_submodule_list and
deinit_submodule.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 140 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  55 +----------------
 2 files changed, 141 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e10cac462..f029f5fae 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -750,6 +750,145 @@ static int module_status(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct deinit_cb {
+	const char *prefix;
+	unsigned int quiet: 1;
+	unsigned int force: 1;
+	unsigned int all: 1;
+};
+#define DEINIT_CB_INIT { NULL, 0, 0, 0 }
+
+static void deinit_submodule(const struct cache_entry *list_item,
+			     void *cb_data)
+{
+	struct deinit_cb *info = cb_data;
+	const struct submodule *sub;
+	char *displaypath = NULL;
+	struct child_process cp_config = CHILD_PROCESS_INIT;
+	struct strbuf sb_config = STRBUF_INIT;
+	char *sm_path = xstrdup(list_item->name);
+	char *sub_git_dir = xstrfmt("%s/.git", sm_path);
+
+	sub = submodule_from_path(null_sha1, sm_path);
+
+	if (!sub->name)
+		goto cleanup;
+
+	displaypath = get_submodule_displaypath(sm_path, info->prefix);
+
+	/* remove the submodule work tree (unless the user already did it) */
+	if (is_directory(sm_path)) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+
+		/* protect submodules containing a .git directory */
+		if (is_git_directory(sub_git_dir))
+			die(_("Submodule work tree '%s' contains a .git "
+			      "directory use 'rm -rf' if you really want "
+			      "to remove it including all of its history"),
+			      displaypath);
+
+		if (!info->force) {
+			struct child_process cp_rm = CHILD_PROCESS_INIT;
+			cp_rm.git_cmd = 1;
+			argv_array_pushl(&cp_rm.args, "rm", "-qn", sm_path,
+					 NULL);
+
+			/* list_item->name is changed by cmd_rm() below */
+			if (run_command(&cp_rm))
+				die(_("Submodule work tree '%s' contains local "
+				      "modifications; use '-f' to discard them"),
+				      displaypath);
+		}
+
+		cp.use_shell = 1;
+		argv_array_pushl(&cp.args, "rm", "-rf", sm_path, NULL);
+		if (!run_command(&cp)) {
+			if (!info->quiet)
+				printf(_("Cleared directory '%s'\n"),
+					 displaypath);
+		} else {
+			if (!info->quiet)
+				printf(_("Could not remove submodule work tree '%s'\n"),
+					 displaypath);
+		}
+	}
+
+	if (mkdir(sm_path, 0700))
+		die(_("could not create empty submodule directory %s"),
+		      displaypath);
+
+	cp_config.git_cmd = 1;
+	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
+	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
+
+	/* remove the .git/config entries (unless the user already did it) */
+	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
+		char *sub_key = xstrfmt("submodule.%s", sub->name);
+		/*
+		 * remove the whole section so we have a clean state when
+		 * the user later decides to init this submodule again
+		 */
+		git_config_rename_section_in_file(NULL, sub_key, NULL);
+		if (!info->quiet)
+			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
+				 sub->name, sub->url, displaypath);
+		free(sub_key);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_git_dir);
+	free(sm_path);
+	strbuf_release(&sb_config);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+	struct deinit_cb info = DEINIT_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int force = 0;
+	int all = 0;
+
+	struct option module_deinit_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT__FORCE(&force, N_("Remove submodule working trees even if they contain local changes")),
+		OPT_BOOL(0, "all", &all, N_("Unregister all submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_deinit_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.prefix = prefix;
+	info.quiet = !!quiet;
+	info.all = !!all;
+	info.force = !!force;
+
+	if (all && argc) {
+		error("pathspec and --all are incompatible");
+		usage_with_options(git_submodule_helper_usage,
+				   module_deinit_options);
+	}
+
+	if (!argc && !all)
+		die(_("Use '--all' if you really want to deinitialize all submodules"));
+
+	gitmodules_config();
+	for_each_submodule_list(list, deinit_submodule, &info);
+
+	return 0;
+}
+
 static int module_name(int argc, const char **argv, const char *prefix)
 {
 	const struct submodule *sub;
@@ -1644,6 +1783,7 @@ static struct cmd_struct commands[] = {
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
+	{"deinit", module_deinit, SUPPORT_SUPER_PREFIX},
 	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
diff --git a/git-submodule.sh b/git-submodule.sh
index 33b4b7306..2b2c6f5da 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -427,60 +427,7 @@ cmd_deinit()
 		shift
 	done
 
-	if test -n "$deinit_all" && test "$#" -ne 0
-	then
-		echo >&2 "$(eval_gettext "pathspec and --all are incompatible")"
-		usage
-	fi
-	if test $# = 0 && test -z "$deinit_all"
-	then
-		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
-	fi
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-
-		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
-
-		# Remove the submodule work tree (unless the user already did it)
-		if test -d "$sm_path"
-		then
-			# Protect submodules containing a .git directory
-			if test -d "$sm_path/.git"
-			then
-				die "$(eval_gettext "\
-Submodule work tree '\$displaypath' contains a .git directory
-(use 'rm -rf' if you really want to remove it including all of its history)")"
-			fi
-
-			if test -z "$force"
-			then
-				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
-			fi
-			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
-		fi
-
-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
-
-		# Remove the .git/config entries (unless the user already did it)
-		if test -n "$(git config --get-regexp submodule."$name\.")"
-		then
-			# Remove the whole section so we have a clean state when
-			# the user later decides to init this submodule again
-			url=$(git config submodule."$name".url)
-			git config --remove-section submodule."$name" 2>/dev/null &&
-			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
 }
 
 is_tip_reachable () (
-- 
2.13.0

Re: [GSoC] Update: Week 5

[Andrew Ardill]

On 20 June 2017 at 07:41, Prathamesh Chavan <pc44800@gmail.com> wrote:

>    But as communicating between child_process is still an issue
>    and so there was no simple was to current carry out the
>    porting. And hence, a hack was used instead. But after
>    discussing it, instead using the repository-object patch
>    series will help to resolve these issues in this situation.

Just wondering, does that mean that your patch series is dependent on
the repository-object one? I saw some discussion around it recently
but couldn't see it in the latest whats cooking so maybe I missed what
has happened to it.

Really enjoying your updates, by the way, they are very clear and show
what looks like great progress!

Regards,

Andrew Ardill

Re: [GSoC] Update: Week 5

[Brandon Williams]

On 06/20, Andrew Ardill wrote:
> On 20 June 2017 at 07:41, Prathamesh Chavan <pc44800@gmail.com> wrote:
> 
> >    But as communicating between child_process is still an issue
> >    and so there was no simple was to current carry out the
> >    porting. And hence, a hack was used instead. But after
> >    discussing it, instead using the repository-object patch
> >    series will help to resolve these issues in this situation.
> 
> Just wondering, does that mean that your patch series is dependent on
> the repository-object one? I saw some discussion around it recently
> but couldn't see it in the latest whats cooking so maybe I missed what
> has happened to it.
> 

Due to some of the discussion (and finding a bug with how git_path works
with worktrees) I decided to break the series up into smaller bits since
the original series was 30+ patches.  Once 'bw/config-h' and
'bw/ls-files-sans-the-index' have been merged into next I'll probably
send out v3 of the repository object series.

-- 
Brandon Williams

Re: [GSoC][PATCH 1/6] dir: create function count_slashes

[Stefan Beller]

On Mon, Jun 19, 2017 at 2:50 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> Similar functions exist in apply.c and builtin/show-branch.c for
> counting the number of slashes in a string. Also in the later
> patches, we introduce a third caller for the same. Hence, we unify
> it now by cleaning the existing functions and declaring a common
> function count_slashes in dir.h and implementing it in dir.c to
> remove this code duplication.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> The complete build report of this is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: All-patch-series
> Build #111

This patch was sent separately a couple days ago, see
the latest "What's cooking in git.git" (Jun 2017, #05; Mon, 19)
https://public-inbox.org/git/xmqqh8zbspm7.fsf@gitster.mtv.corp.google.com/

  * pc/dir-count-slashes (2017-06-12) 1 commit
    (merged to 'next' on 2017-06-19 at 57351a2771)
   + dir: create function count_slashes()

   Three instances of the same helper function have been consolidated
   to one.

   Will merge to 'master'.

so if you pull Junios git and rebase on top of his master branch
this should be already included there. (In that case there is no need
for you to carry this patch. It is just cumbersome for you and might
confuse Junio, which patches exactly to apply)

Thanks,
Stefan

Re: [GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to C

[Stefan Beller]

On Mon, Jun 19, 2017 at 2:50 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> The mechanism used for porting the submodule subcommand 'sync' is
> similar to that of 'foreach', where we split the function cmd_sync
> from shell into three functions in C, module_sync,
> for_each_submodule_list and sync_submodule.
>
> print_default_remote is introduced as a submodule--helper
> subcommand for getting the default remote as stdout.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---

Up to this patch, all other patches look good to me,
here I stumbled upon a small nit.


>  builtin/submodule--helper.c | 180 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  56 +-------------
>  2 files changed, 181 insertions(+), 55 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 78b21ab22..e10cac462 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -43,6 +43,20 @@ static char *get_default_remote(void)
>         return ret;
>  }
>
> +static int print_default_remote(int argc, const char **argv, const char *prefix)
> +{
> +       const char *remote;
> +
> +       if (argc != 1)
> +               die(_("submodule--helper print-default-remote takes no arguments"));
> +
> +       remote = get_default_remote();
> +       if (remote)
> +               puts(remote);
> +
> +       return 0;
> +}
> +
>  static int starts_with_dot_slash(const char *str)
>  {
>         return str[0] == '.' && is_dir_sep(str[1]);
> @@ -311,6 +325,25 @@ static int print_name_rev(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static char *get_up_path(const char *path)
> +{
> +       int i = count_slashes(path);
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       while (i--)
> +               strbuf_addstr(&sb, "../");
> +
> +       /*
> +        *Check if 'path' ends with slash or not
> +        *for having the same output for dir/sub_dir
> +        *and dir/sub_dir/
> +        */
> +       if (!is_dir_sep(path[i - 1]))
> +               strbuf_addstr(&sb, "../");
> +
> +       return strbuf_detach(&sb, NULL);
> +}
> +
>  struct module_list {
>         const struct cache_entry **entries;
>         int alloc, nr;
> @@ -736,6 +769,151 @@ static int module_name(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +struct sync_cb {
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define SYNC_CB_INIT { NULL, 0, 0 }
> +
> +static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +       struct sync_cb *info = cb_data;
> +       const struct submodule *sub;
> +       char *sub_key, *remote_key;
> +       char *url, *sub_origin_url, *super_config_url, *displaypath;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +
> +       if (!is_submodule_initialized(list_item->name))
> +               return;
> +
> +       sub = submodule_from_path(null_sha1, list_item->name);
> +
> +       if (!sub->url)

'sub' can be NULL as well, which when used to obtain the ->url
will crash. So we'd rather want to have (!sub || !sub->url).

I looked through other use cases, others only need (!sub), so this
thought did not hint at other bugs in the code base.


> +               die(_("no url found for submodule path '%s' in .gitmodules"),
> +                     list_item->name);
> +
> +       url = xstrdup(sub->url);

Why do we need to duplicate the url here? As we are not modifying it
(read: I did not spot the url modification), we could just use sub->url
instead, saving a variable.

Re: [GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_list

[Brandon Williams]

On 06/20, Prathamesh Chavan wrote:
> Functions get_submodule_displaypath and for_each_submodule_list
> for using them in the later patches, related to porting submodule
> subcommands from shell to C.
> These new functions are also used in ported submodule subcommand
> init

I didn't see anything wrong with these patches, but one small nit is
that this one patch is changing two different things, adding
'get_submodule_displaypath' and 'for_each_submodule_list'.  Logically
you could break this patch into two different parts, first introducing
one and then the other.

I'm not saying you need to re-do this patch though (I don't have a super
strong opinion though others might) just wanted to point it out for the
future.

> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 69 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 8cc648d85..f7adca95b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,9 @@
>  #include "refs.h"
>  #include "connect.h"
>  
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
> +				      void *cb_data);
> +
>  static char *get_default_remote(void)
>  {
>  	char *dest = NULL, *ret;
> @@ -219,6 +222,27 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>  	return 0;
>  }
>  
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> +	const char *super_prefix = get_super_prefix();
> +
> +	if (prefix && super_prefix) {
> +		BUG("cannot have prefix '%s' and superprefix '%s'",
> +		    prefix, super_prefix);
> +	} else if (prefix) {
> +		struct strbuf sb = STRBUF_INIT;
> +		char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> +		strbuf_release(&sb);
> +		return displaypath;
> +	} else if (super_prefix) {
> +		int len = strlen(super_prefix);
> +		const char *format = is_dir_sep(super_prefix[len-1]) ? "%s%s" : "%s/%s";
> +		return xstrfmt(format, super_prefix, path);
> +	} else {
> +		return xstrdup(path);
> +	}
> +}
> +
>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -330,26 +354,30 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> -static void init_submodule(const char *path, const char *prefix, int quiet)
> +static void for_each_submodule_list(const struct module_list list,
> +				    submodule_list_func_t fn, void *cb_data)
>  {
> +	int i;
> +	for (i = 0; i < list.nr; i++)
> +		fn(list.entries[i], cb_data);
> +}
> +
> +struct init_cb {
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +};
> +#define INIT_CB_INIT { NULL, 0 }
> +
> +static void init_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct init_cb *info = cb_data;
>  	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 && 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);
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
>  
> -	sub = submodule_from_path(null_sha1, path);
> +	sub = submodule_from_path(null_sha1, list_item->name);
>  
>  	if (!sub)
>  		die(_("No url found for submodule path '%s' in .gitmodules"),
> @@ -361,7 +389,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  	 *
>  	 * Set active flag for the submodule being initialized
>  	 */
> -	if (!is_submodule_initialized(path)) {
> +	if (!is_submodule_initialized(list_item->name)) {
>  		strbuf_reset(&sb);
>  		strbuf_addf(&sb, "submodule.%s.active", sub->name);
>  		git_config_set_gently(sb.buf, "true");
> @@ -404,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  		if (git_config_set_gently(sb.buf, url))
>  			die(_("Failed to register url for submodule path '%s'"),
>  			    displaypath);
> -		if (!quiet)
> +		if (!info->quiet)
>  			fprintf(stderr,
>  				_("Submodule '%s' (%s) registered for path '%s'\n"),
>  				sub->name, url, displaypath);
> @@ -433,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  
>  static int module_init(int argc, const char **argv, const char *prefix)
>  {
> +	struct init_cb info = INIT_CB_INIT;
>  	struct pathspec pathspec;
>  	struct module_list list = MODULE_LIST_INIT;
>  	int quiet = 0;
> -	int i;
>  
>  	struct option module_init_options[] = {
>  		OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> @@ -461,8 +489,11 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  	if (!argc && git_config_get_value_multi("submodule.active"))
>  		module_list_active(&list);
>  
> -	for (i = 0; i < list.nr; i++)
> -		init_submodule(list.entries[i]->name, prefix, quiet);
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +
> +	gitmodules_config();
> +	for_each_submodule_list(list, init_submodule, &info);
>  
>  	return 0;
>  }
> -- 
> 2.13.0
> 

-- 
Brandon Williams

Re: [GSoC][PATCH 4/6] submodule: port submodule subcommand status

[Brandon Williams]

On 06/20, Prathamesh Chavan wrote:
> The mechanism used for porting submodule subcommand 'status'
> is similar to that used for subcommand 'foreach'.

nit: since 'foreach' is stalled atm it may be confusing to reference
that change when it hasn't been merged in yet.

> The function cmd_status from git-submodule is ported to three
> functions in the builtin submodule--helper namely: module_status,
> for_each_submodule_list and status_submodule.
> 
> print_status is also introduced for handling the output of
> the subcommand and also to reduce the code size.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>

-- 
Brandon Williams

Re: [GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to C

[Christian Couder]

On Mon, Jun 19, 2017 at 11:50 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:

> +static char *get_up_path(const char *path)
> +{
> +       int i = count_slashes(path);
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       while (i--)
> +               strbuf_addstr(&sb, "../");
> +
> +       /*
> +        *Check if 'path' ends with slash or not
> +        *for having the same output for dir/sub_dir
> +        *and dir/sub_dir/
> +        */
> +       if (!is_dir_sep(path[i - 1]))

i is always 0 here, as we decrease it until it gets to 0 above.

> +               strbuf_addstr(&sb, "../");
> +
> +       return strbuf_detach(&sb, NULL);
> +}

Re: [GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_list

[Christian Couder]

On Mon, Jun 19, 2017 at 11:50 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:

> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> +       const char *super_prefix = get_super_prefix();
> +
> +       if (prefix && super_prefix) {
> +               BUG("cannot have prefix '%s' and superprefix '%s'",
> +                   prefix, super_prefix);
> +       } else if (prefix) {
> +               struct strbuf sb = STRBUF_INIT;
> +               char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> +               strbuf_release(&sb);
> +               return displaypath;
> +       } else if (super_prefix) {
> +               int len = strlen(super_prefix);
> +               const char *format = is_dir_sep(super_prefix[len-1]) ? "%s%s" : "%s/%s";

Style nit: please add spaces around "-", so "len - 1" instead of "len-1".

> +               return xstrfmt(format, super_prefix, path);
> +       } else {
> +               return xstrdup(path);
> +       }
> +}

Re: [GSoC] Update: Week 5

[Prathamesh Chavan]

On Tue, Jun 20, 2017 at 5:31 AM, Andrew Ardill <andrew.ardill@gmail.com> wrote:
> On 20 June 2017 at 07:41, Prathamesh Chavan <pc44800@gmail.com> wrote:
>
>>    But as communicating between child_process is still an issue
>>    and so there was no simple was to current carry out the
>>    porting. And hence, a hack was used instead. But after
>>    discussing it, instead using the repository-object patch
>>    series will help to resolve these issues in this situation.
>
> Just wondering, does that mean that your patch series is dependent on
> the repository-object one? I saw some discussion around it recently
> but couldn't see it in the latest whats cooking so maybe I missed what
> has happened to it.

Sorry for such a late reply. In this update, and even in the latest update[1],
the patches aren't dependent on the 'repository-object' series.
But there are certain issues encountered which I aim to resolve
using them.

>
> Really enjoying your updates, by the way, they are very clear and show
> what looks like great progress!

Thanks a lot for this, and I hope to keep improving it. :)

Thanks,
Prathamesh Chavan

[1]: https://public-inbox.org/git/CAME+mvUrr8EA-6jbCZdpB7dMZ5CN3RyY7yoRoUBoiZw=sH6Ysw@mail.gmail.com/