{"thread":{"id":"46637","subject":"[GSoC][PATCH 1/4] submodule--helper: introduce get_submodule_displaypath()","startedAt":"2017-08-21T10:45:29Z","lastAt":"2017-10-07T09:35:39Z","messageCount":59,"participants":["Prathamesh Chavan","Heiko Voigt","Junio C Hamano","Stefan Beller","Han-Wen Nienhuys","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"326863","messageId":"20170821161515.23775-1-pc44800@gmail.com","threadId":"46637","inReplyTo":null,"subject":"[GSoC][PATCH 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-21T16:15:12Z","receivedAt":"2017-08-21T10:45:29Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nAs said in the previous update,\na short patch series is floated for the maintainer's review,\nand is consisting of the following changes:\n* introduce function get_submodule_displaypath()\n* introduce function for_each_submodule_list()\n* port function set_name_rev() from shell to C\n* port submodule subcommand 'status' from shell to C\n\nThe complete build report for the above series of patches is available\nat:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: week-14-1\nBuild #158\nThe above changes are also push on github and are available at:\nhttps://github.com/pratham-pc/git/commits/week-14-1\n\nThe above changes were based on master branch.\nBut along with the above changes, the same series was also applied\nto a separate branch based on the 'next' branch. The changes were\napplicable without any additional changes to the patches.\nComplete build report of that is also available at:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: week-14-1-next\nBuild #159\nThe above changes are also push on github and are available at:\nhttps://github.com/pratham-pc/git/commits/week-14-1-next\n\n builtin/submodule--helper.c | 33 ++++++++++++++++++++++-----------\n 1 file changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 84562ec83..dcdbde963 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,27 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = is_dir_sep(super_prefix[len - 1]) ? \"%s%s\" : \"%s/%s\";\n+\t\treturn xstrfmt(format, super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n \t/* Only loads from .gitmodules, no overlay with .git/config */\n \tgitmodules_config();\n-\n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t}\n-- \n2.13.0\n\n"},{"id":"326864","messageId":"20170821161515.23775-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170821161515.23775-1-pc44800@gmail.com","subject":"[GSoC][PATCH 2/4] submodule--helper: introduce for_each_submodule_list()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-21T16:15:13Z","receivedAt":"2017-08-21T10:45:39Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_submodule_list() and\nreplace a loop in module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++----------\n 1 file changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex dcdbde963..7803457ba 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,9 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -352,17 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_submodule_list(const struct module_list list,\n+\t\t\t\t submodule_list_func_t fn, void *cb_data)\n {\n+\tint i;\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tfn(list.entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct init_cb *info = cb_data;\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* Only loads from .gitmodules, no overlay with .git/config */\n-\tgitmodules_config();\n-\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n \n-\tsub = submodule_from_path(&null_oid, path);\n+\tsub = submodule_from_path(&null_oid, list_item->name);\n \n \tif (!sub)\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n@@ -374,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t *\n \t * Set active flag for the submodule being initialized\n \t */\n-\tif (!is_submodule_active(the_repository, path)) {\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t}\n@@ -416,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!info->quiet)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -445,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -473,8 +489,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\n+\tgitmodules_config();\n+\tfor_each_submodule_list(list, init_submodule, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"326865","messageId":"20170821161515.23775-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170821161515.23775-1-pc44800@gmail.com","subject":"[GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-21T16:15:14Z","receivedAt":"2017-08-21T10:45:47Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Function set_name_rev() is ported from git-submodule to the\nsubmodule--helper builtin. The function get_name_rev() generates the\nvalue of the revision name as required, and the function\nprint_name_rev() handles the formating and printing of the obtained\nrevision name.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 16 ++----------\n 2 files changed, 65 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7803457ba..a4bff3f38 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -244,6 +244,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *get_name_rev(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n+static int print_name_rev(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *namerev;\n+\tif (argc != 3)\n+\t\tdie(\"print-name-rev only accepts two arguments: \");\n+\n+\tnamerev = get_name_rev(argv[1], argv[2]);\n+\tif (namerev && namerev[0])\n+\t\tprintf(\" (%s)\", namerev);\n+\tprintf(\"\\n\");\n+\n+\tfree(namerev);\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"print-name-rev\", print_name_rev, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e131760ee..e988167e0 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -759,18 +759,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1042,14 +1030,14 @@ cmd_status()\n \t\tfi\n \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n \t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n \t\t\tsay \" $sha1 $displaypath$revname\"\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.13.0\n\n"},{"id":"326866","messageId":"20170821161515.23775-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170821161515.23775-1-pc44800@gmail.com","subject":"[GSoC][PATCH 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-21T16:15:15Z","receivedAt":"2017-08-21T10:45:55Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nthree functions: module_status(), submodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_submodule_list() looping through the\nlist obtained.\n\nThen for_each_submodule_list() calls submodule_status() for each of the\nsubmodule in its list. The function submodule_status() is responsible\nfor generating the status each submodule it is called for, and\nthen calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nThe patch below underwent some changes after its previous version.\n\nIn the shell script, the submodules, which had merge-conflicts are\nidentified by checking if the $stage variable has the value 'U'\nTill the last patch series, this was done by checking if ce_flags have\na non-zero value. In this new patch series, this was changed and\ninstead ce_stage() was called on the list_item(cache_entry), and then\nwe check whether it has a non-zero value.\n\n builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 49 +-------------\n 2 files changed, 157 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a4bff3f38..831370d6e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -560,6 +560,161 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+\tunsigned int cached: 1;\n+};\n+#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n+\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+') {\n+\t\tstruct argv_array name_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushl(&name_rev_args, \"print-name-rev\",\n+\t\t\t\t path, oid_to_hex(oid), NULL);\n+\t\tprint_name_rev(name_rev_args.argc, name_rev_args.argv,\n+\t\t\t info->prefix);\n+\n+\t\targv_array_clear(&name_rev_args);\n+\t} else {\n+\t\tprintf(\"\\n\");\n+\t}\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(&null_oid, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (ce_stage(list_item)) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t list_item->name, NULL);\n+\n+\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n+\t\t\t info->prefix)) {\n+\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (head_ref_submodule(list_item->name,\n+\t\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t\t \"submodule '%s'\"), list_item->name);\n+\n+\t\t\tprint_status(info, '+', list_item->name, &oid,\n+\t\t\t\t displaypath);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name,\n+\t\t\t\t &list_item->oid, displaypath);\n+\t\t}\n+\t}\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = list_item->name;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_pushl(&cpr.args, \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"status\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->cached)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"),\n+\t\t\t list_item->name);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint cached = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] []\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\tinfo.cached = !!cached;\n+\n+\tgitmodules_config();\n+\tfor_each_submodule_list(list, status_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1306,6 +1461,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"print-name-rev\", print_name_rev, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e988167e0..51b057d82 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1005,54 +1005,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"326886","messageId":"20170821164730.GC1618@book.hvoigt.net","threadId":"46637","inReplyTo":"20170821161515.23775-3-pc44800@gmail.com","subject":"Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-21T16:47:30Z","receivedAt":"2017-08-21T16:47:40Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Aug 21, 2017 at 09:45:14PM +0530, Prathamesh Chavan wrote:\n> Function set_name_rev() is ported from git-submodule to the\n> submodule--helper builtin. The function get_name_rev() generates the\n> value of the revision name as required, and the function\n> print_name_rev() handles the formating and printing of the obtained\n> revision name.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 16 ++----------\n> 2 files changed, 65 insertions(+), 14 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 7803457ba..a4bff3f38 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n\n[...]\n\n> @@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {\n> \t{\"relative-path\", resolve_relative_path, 0},\n> \t{\"resolve-relative-url\", resolve_relative_url, 0},\n> \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n> +\t{\"print-name-rev\", print_name_rev, 0},\n\nI see the function in git-submodule.sh was named kind of reverse. How\nabout we do it more naturally here and call this 'rev-name' instead?\nThat makes is more clear to me and is also similar to the used variable\nname 'revname'.\n\nI would also prefix it differently like 'get' or 'calculate' instead of\n'print' since it tries to find a name and is not just a simple lookup.\n\nSo in summary I would prefer something like 'get-rev-name' as a name for\nthe subcommand here.\n\n> \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n> \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n> \t{\"push-check\", push_check, 0},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index e131760ee..e988167e0 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -759,18 +759,6 @@ cmd_update()\n> \t}\n> }\n> \n> -set_name_rev () {\n> -\trevname=$( (\n> -\t\tsanitize_submodule_env\n> -\t\tcd \"$1\" && {\n> -\t\t\tgit describe \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --all --always \"$2\"\n> -\t\t}\n> -\t) )\n> -\ttest -z \"$revname\" || revname=\" ($revname)\"\n> -}\n> #\n> # Show commit summary for submodules in index or working tree\n> #\n> @@ -1042,14 +1030,14 @@ cmd_status()\n> \t\tfi\n> \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n> \t\tthen\n> -\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n> +\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n> \t\t\tsay \" $sha1 $displaypath$revname\"\n> \t\telse\n> \t\t\tif test -z \"$cached\"\n> \t\t\tthen\n> \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n> \t\t\tfi\n> -\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n> +\t\t\trevname=$(git submodule--helper print-name-rev \"$sm_path\" \"$sha1\")\n> \t\t\tsay \"+$sha1 $displaypath$revname\"\n> \t\tfi\n> \n> -- \n> 2.13.0\n> \n"},{"id":"326890","messageId":"CAME+mvWTNSUkK5zKTTAEU2SGxM9TgjjBywJ_FTLW_wLHRT=iAw@mail.gmail.com","threadId":"46637","inReplyTo":"20170821164730.GC1618@book.hvoigt.net","subject":"Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-21T17:24:06Z","receivedAt":"2017-08-21T17:24:13Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"On Mon, Aug 21, 2017 at 10:17 PM, Heiko Voigt wrote:\n> On Mon, Aug 21, 2017 at 09:45:14PM +0530, Prathamesh Chavan wrote:\n>> Function set_name_rev() is ported from git-submodule to the\n>> submodule--helper builtin. The function get_name_rev() generates the\n>> value of the revision name as required, and the function\n>> print_name_rev() handles the formating and printing of the obtained\n>> revision name.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Stefan Beller \n>> Signed-off-by: Prathamesh Chavan \n>> ---\n>> builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n>> git-submodule.sh | 16 ++----------\n>> 2 files changed, 65 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 7803457ba..a4bff3f38 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>\n> [...]\n>\n>> @@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {\n>> {\"relative-path\", resolve_relative_path, 0},\n>> {\"resolve-relative-url\", resolve_relative_url, 0},\n>> {\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n>> + {\"print-name-rev\", print_name_rev, 0},\n>\n> I see the function in git-submodule.sh was named kind of reverse. How\n> about we do it more naturally here and call this 'rev-name' instead?\n> That makes is more clear to me and is also similar to the used variable\n> name 'revname'.\n\nThe functions 'print_name_rev()' and 'get_name_rev()' are the ported\nC functions of the function 'set_name_rev()'\nTheir names were assigned so due to the existing function's name,\nand hence to faithfully port the functions.\n\nBut, thanks for the above suggestion, and also for reviewing\nthe patch. I will update the names as print_rev_name() and\nget_rev_name() respectively.\n>\n> I would also prefix it differently like 'get' or 'calculate' instead of\n> 'print' since it tries to find a name and is not just a simple lookup.\n\nThe former function from the shell script, 'set_name_rev()' is split\ninto two functions, namely: 'print_name_rev()' and 'get_name_rev()'\nThe function print_name_rev() is just the front_end of the function,\nand exists to printf the return value of the get_name_rev() function\nis the required format.\nCalculation of the value is actually done by the function\nget_name_rev().\nHence, I named the functions the way they are.\n\nThanks,\nPrathamesh Chavan\n"},{"id":"326986","messageId":"xmqqinhf1bjf.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170821161515.23775-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-22T22:29:40Z","receivedAt":"2017-08-22T22:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> Introduce function get_submodule_displaypath() to replace the code\n> occurring in submodule_init() for generating displaypath of the\n> submodule with a call to it.\n\nLooks like a quite straight-forward refactoring.\n\n> +static char *get_submodule_displaypath(const char *path, const char *prefix)\n> +{\n> +\tconst char *super_prefix = get_super_prefix();\n> +\n> +\tif (prefix && super_prefix) {\n> +\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n> +\t\t prefix, super_prefix);\n> +\t} else if (prefix) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn displaypath;\n\nUse of xstrdup() would waste one extra allocation and copy, no? In\nother words, wouldn't this do the same thing?\n\n\t... {\n\t\tstruct strbuf sb = STRBUF_INIT;\n\n\t\trelative_path(path, prefix, &sb);\n\t return strbuf_detach(&sb, NULL);\n\n> @@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t * Set active flag for the submodule being initialized\n> \t */\n> \tif (!is_submodule_active(the_repository, path)) {\n> -\t\tstrbuf_reset(&sb);\n> \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n> \t\tgit_config_set_gently(sb.buf, \"true\");\n> \t}\n\nThis is because sb will stay untouched with the use of the new\nhelper. With the way this single strbuf is reused throughout this\nfunction, I cannot help wondering if the prevailing pattern of\nresetting and then using is a mistake. If the strbuf is mostly used\nas a scratchpad, wouldn't it make more sense to use it and then\nclean up when you are done with it?\n\nI.e. something along this line that shows only two uses...\n\n builtin/submodule--helper.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0ff9dd0b85..84ded4b2e9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -363,9 +363,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -373,7 +373,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -410,9 +409,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n\n\n"},{"id":"326987","messageId":"xmqqefs31b73.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170821161515.23775-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH 2/4] submodule--helper: introduce for_each_submodule_list()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-22T22:37:04Z","receivedAt":"2017-08-22T22:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> -static void init_submodule(const char *path, const char *prefix, int quiet)\n> +static void for_each_submodule_list(const struct module_list list,\n> +\t\t\t\t submodule_list_func_t fn, void *cb_data)\n\nIt may not be wrong per-se, but can't this just be for_each_submodule()?\n\nYour \"justification\" may be that this makes it clear that you are\niterating over module_list and not other kind of group of\nsubmodules, but I would say the design of the subsystem is broken if\nsome places use a list of submodules while some other places use an\narray of submodules to represent a group of submodules. Especially\nwhen there is a dedicated type to hold a group of submodules,\ni.e. struct module-list, that type should be used consistently\nthroughout the subsystem and API, no?\n\n> {\n> +\tint i;\n> +\tfor (i = 0; i < list.nr; i++)\n> +\t\tfn(list.entries[i], cb_data);\n> +}\n\nAlso, did you really want to pass the structure by value? At least\nin C, it is more customary to pass these things by pointer, i.e.\n\n\tfor_each_submodule(struct module_list *list,\n\t\t\t for_each_submodule_fn fn,\n\t\t\t void *cb_data)\n\t{\n\t\tfor (i = 0; i < list->nr; i++)\n\t\t\t...\n\nOtherwise you'd be making a copy on stack unnecessarily (ok, \"const\"\nmight hint a smart compiler to turn this inefficient code to pass it\nby pointer, but I do not think it is a particulary good to rely on\nsuch things).\n\n"},{"id":"327059","messageId":"20170823181506.8557-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"xmqqinhf1bjf.fsf@gitster.mtv.corp.google.com","subject":"[GSoC][PATCH v2 0/4] submodule: Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-23T18:15:02Z","receivedAt":"2017-08-23T18:19:22Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Changes in v2:\n\n* In the function get_submodule_displaypath(), I tried avoiding the extra\n memory allocation, but it rather invoked test 5 from\n t7406-submodule-update. The details of the test failiure can be seen\n in the build report as well. (Build #162)\n This failure occured as even though the function relative_path(),\n returned the value correctly, NULL was stored in sb.buf\n Hence currently the function is still using the old way of\n allocating extra memory and releasing the strbuf first, and return\n the extra allocated memory.\n* It was observed that strbuf_reset was being done just before the strbuf\n was being reused. It made more sense to reset them just after their use \n instead. Hence, these function calls of strbuf_reset() were moved\n accordingly.\n (The above change was limited to the function init_submodule() only,\n since already there was a deletion of one such funciton call,\n and IMO, it won't be suitable to carryout the operation for the complete\n file in the same commit.)\n* The function name for_each_submodule_list() was changed to\n for_each_submodule().\n* Instead of passing the complete list to the function for_each_submodule(),\n only its pointer is passed to avoid the compiler from making copy of the\n list, and to make the code more efficient.\n* The function names of print_name_rev() and get_name_rev() were changed\n to get_rev_name() and compute_rev_name(). Now the function get_rev_name()\n acts as the front end of the subcommand and calls the function\n compute_rev_name() for generating and receiving the value of revision name.\n* The above change was also accompanied by the change in names of some\n variables used internally in the functions.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/week-14-1\n\nAnd the build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: week-14-1\nBuild #163\n\nPrathamesh Chavan (4):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_submodule()\n submodule: port set_name_rev() from shell to C\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 294 ++++++++++++++++++++++++++++++++++++++++----\n git-submodule.sh | 61 +--------\n 2 files changed, 273 insertions(+), 82 deletions(-)\n\n-- \n2.13.0\n\n"},{"id":"327060","messageId":"20170823181506.8557-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170823181506.8557-1-pc44800@gmail.com","subject":"[GSoC][PATCH v2 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-23T18:15:03Z","receivedAt":"2017-08-23T18:19:33Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 38 +++++++++++++++++++++++++-------------\n 1 file changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 84562ec83..e666f84ba 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = is_dir_sep(super_prefix[len - 1]) ? \"%s%s\" : \"%s/%s\";\n+\n+\t\treturn xstrfmt(format, super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n \t/* Only loads from .gitmodules, no overlay with .git/config */\n \tgitmodules_config();\n-\n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.13.0\n\n"},{"id":"327061","messageId":"20170823181506.8557-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170823181506.8557-1-pc44800@gmail.com","subject":"[GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-23T18:15:04Z","receivedAt":"2017-08-23T18:19:43Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++----------\n 1 file changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e666f84ba..847fba854 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,9 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_submodule(const struct module_list *list,\n+\t\t\t submodule_list_func_t fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n {\n+\tstruct init_cb *info = cb_data;\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* Only loads from .gitmodules, no overlay with .git/config */\n-\tgitmodules_config();\n-\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n \n-\tsub = submodule_from_path(&null_oid, path);\n+\tsub = submodule_from_path(&null_oid, list_item->name);\n \n \tif (!sub)\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t *\n \t * Set active flag for the submodule being initialized\n \t */\n-\tif (!is_submodule_active(the_repository, path)) {\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t\tstrbuf_reset(&sb);\n@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!info->quiet)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\n+\tgitmodules_config();\n+\tfor_each_submodule(&list, init_submodule, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"327062","messageId":"20170823181506.8557-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170823181506.8557-1-pc44800@gmail.com","subject":"[GSoC][PATCH v2 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-23T18:15:05Z","receivedAt":"2017-08-23T18:19:51Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Function set_name_rev() is ported from git-submodule to the\nsubmodule--helper builtin. The function compute_rev_name() generates the\nvalue of the revision name as required.\nThe function get_rev_name() calls compute_rev_name() and receives the\nrevision name, and later handles its formating and printing.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 16 ++----------\n 2 files changed, 65 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 847fba854..6ae93ce38 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n+static int get_rev_name(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *revname;\n+\tif (argc != 3)\n+\t\tdie(\"get-rev-name only accepts two arguments: \");\n+\n+\trevname = compute_rev_name(argv[1], argv[2]);\n+\tif (revname && revname[0])\n+\t\tprintf(\" (%s)\", revname);\n+\tprintf(\"\\n\");\n+\n+\tfree(revname);\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e131760ee..91f043ec6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -759,18 +759,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1042,14 +1030,14 @@ cmd_status()\n \t\tfi\n \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n \t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \" $sha1 $displaypath$revname\"\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.13.0\n\n"},{"id":"327063","messageId":"20170823181506.8557-5-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170823181506.8557-1-pc44800@gmail.com","subject":"[GSoC][PATCH v2 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-23T18:15:06Z","receivedAt":"2017-08-23T18:19:59Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nthree functions: module_status(), submodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_submodule() looping through the\nlist obtained.\n\nThen for_each_submodule() calls submodule_status() for each of the\nsubmodule in its list. The function submodule_status() is responsible\nfor generating the status each submodule it is called for, and\nthen calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 49 +-------------\n 2 files changed, 157 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6ae93ce38..933073251 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -561,6 +561,161 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+\tunsigned int cached: 1;\n+};\n+#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n+\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+') {\n+\t\tstruct argv_array get_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushl(&get_rev_args, \"get-rev-name\",\n+\t\t\t\t path, oid_to_hex(oid), NULL);\n+\t\tget_rev_name(get_rev_args.argc, get_rev_args.argv,\n+\t\t\t info->prefix);\n+\n+\t\targv_array_clear(&get_rev_args);\n+\t} else {\n+\t\tprintf(\"\\n\");\n+\t}\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(&null_oid, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (ce_stage(list_item)) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t list_item->name, NULL);\n+\n+\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n+\t\t\t info->prefix)) {\n+\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (head_ref_submodule(list_item->name,\n+\t\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t\t \"submodule '%s'\"), list_item->name);\n+\n+\t\t\tprint_status(info, '+', list_item->name, &oid,\n+\t\t\t\t displaypath);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name,\n+\t\t\t\t &list_item->oid, displaypath);\n+\t\t}\n+\t}\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = list_item->name;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_pushl(&cpr.args, \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"status\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->cached)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"),\n+\t\t\t list_item->name);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint cached = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] []\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\tinfo.cached = !!cached;\n+\n+\tgitmodules_config();\n+\tfor_each_submodule(&list, status_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1307,6 +1462,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 91f043ec6..51b057d82 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1005,54 +1005,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"327067","messageId":"xmqqbmn6yu5u.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170823181506.8557-3-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T19:13:17Z","receivedAt":"2017-08-23T19:13:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,\n> +\t\t\t\t void *cb_data);\n> +\n> static char *get_default_remote(void)\n> {\n> \tchar *dest = NULL, *ret;\n> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> -static void init_submodule(const char *path, const char *prefix, int quiet)\n> +static void for_each_submodule(const struct module_list *list,\n> +\t\t\t submodule_list_func_t fn, void *cb_data)\n\nIn the output from\n\n\t$ git grep for_each \\*.h\n\nwe find that the convention is that an interator over a group of X\nis for_each_X, the callback function that is given to for_each_X is\nof type each_X_fn. An interator over a subset of group of X that\nhas trait Y, for_each_Y_X() iterates and calls back a function of\ntype each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).\n\nI do not offhand think of a reason why the above code need to\ndeviate from that pattern.\n\n\n"},{"id":"327070","messageId":"CAGZ79kbTHkcK8-rwpJbwy-v3NcfvVq=TbvVRG189bq4S9w14GA@mail.gmail.com","threadId":"46637","inReplyTo":"xmqqbmn6yu5u.fsf@gitster.mtv.corp.google.com","subject":"Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-23T19:31:13Z","receivedAt":"2017-08-23T19:31:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 23, 2017 at 12:13 PM, Junio C Hamano wrote:\n> Prathamesh Chavan writes:\n>\n>> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,\n>> + void *cb_data);\n>> +\n>> static char *get_default_remote(void)\n>> {\n>> char *dest = NULL, *ret;\n>> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>> return 0;\n>> }\n>>\n>> -static void init_submodule(const char *path, const char *prefix, int quiet)\n>> +static void for_each_submodule(const struct module_list *list,\n>> + submodule_list_func_t fn, void *cb_data)\n>\n> In the output from\n>\n> $ git grep for_each \\*.h\n>\n> we find that the convention is that an interator over a group of X\n> is for_each_X,\n\n... which this is...\n\n> the callback function that is given to for_each_X is\n> of type each_X_fn.\n\nSo you suggest s/submodule_list_func_t/each_submodule_fn/\n\n> An interator over a subset of group of X that\n> has trait Y, for_each_Y_X() iterates and calls back a function of\n> type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).\n\nThis reads as a suggestion for for_each_listed_submodule\nas the name.\n\n> I do not offhand think of a reason why the above code need to\n> deviate from that pattern.\n"},{"id":"327075","messageId":"xmqq7exuysc7.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"CAGZ79kbTHkcK8-rwpJbwy-v3NcfvVq=TbvVRG189bq4S9w14GA@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T19:52:40Z","receivedAt":"2017-08-23T19:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Wed, Aug 23, 2017 at 12:13 PM, Junio C Hamano wrote:\n>> Prathamesh Chavan writes:\n>>\n>>> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,\n>>> + void *cb_data);\n>>> +\n>>> static char *get_default_remote(void)\n>>> {\n>>> char *dest = NULL, *ret;\n>>> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>>> return 0;\n>>> }\n>>>\n>>> -static void init_submodule(const char *path, const char *prefix, int quiet)\n>>> +static void for_each_submodule(const struct module_list *list,\n>>> + submodule_list_func_t fn, void *cb_data)\n>>\n>> In the output from\n>>\n>> $ git grep for_each \\*.h\n>>\n>> we find that the convention is that an interator over a group of X\n>> is for_each_X,\n>\n> ... which this is...\n>\n>> the callback function that is given to for_each_X is\n>> of type each_X_fn.\n>\n> So you suggest s/submodule_list_func_t/each_submodule_fn/\n\nIt's not _I_ suggest---the remainder of the codebase screams that\nthe above name is wrong ;-). Didn't it for the mentors while they\nwere reading this code?\n\n>> An interator over a subset of group of X that\n>> has trait Y, for_each_Y_X() iterates and calls back a function of\n>> type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).\n>\n> This reads as a suggestion for for_each_listed_submodule\n> as the name.\n\nIf you need to have two ways to iterate over them, i.e. (1) over all\nsubmodules and (2) over only the listed ones (whatever that means),\nthen yes, for_each_listed_submodule() would be a good name for the\nlatter which would be a complement to for_each_submodule() that is\nthe former.\n\n>\n>> I do not offhand think of a reason why the above code need to\n>> deviate from that pattern.\n"},{"id":"327151","messageId":"20170824195051.30900-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"xmqq7exuysc7.fsf@gitster.mtv.corp.google.com","subject":"[GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-24T19:50:47Z","receivedAt":"2017-08-24T19:51:33Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Changes in v3:\n\n* The name of the iterator function for_each_submodule() was changed\n to the for_each_listed_submodule(), as the function fits the naming\n pattern for_each_Y_X(), as here we iterate over group of listed\n submodules (X) which are listed (Y) by the function module_list_compute()\n* The name of the call back function type for the above pattern\n for_each_Y_X() is each_X_fn. Hence, this pattern was followed and\n the name of the call back function type for for_each_listed_submodule()\n was changed from submodule_list_func_t to each_submodule_fn.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/week-14-1\n\nAnd its build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: week-14-1\nBuild #164\n\nPrathamesh Chavan (4):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_listed_submodule()\n submodule: port set_name_rev() from shell to C\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 294 ++++++++++++++++++++++++++++++++++++++++----\n git-submodule.sh | 61 +--------\n 2 files changed, 273 insertions(+), 82 deletions(-)\n\n-- \n2.13.0\n\n"},{"id":"327152","messageId":"20170824195051.30900-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170824195051.30900-1-pc44800@gmail.com","subject":"[GSoC][PATCH v3 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-24T19:50:48Z","receivedAt":"2017-08-24T19:51:36Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 38 +++++++++++++++++++++++++-------------\n 1 file changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 84562ec83..e666f84ba 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = is_dir_sep(super_prefix[len - 1]) ? \"%s%s\" : \"%s/%s\";\n+\n+\t\treturn xstrfmt(format, super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n \t/* Only loads from .gitmodules, no overlay with .git/config */\n \tgitmodules_config();\n-\n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.13.0\n\n"},{"id":"327153","messageId":"20170824195051.30900-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170824195051.30900-1-pc44800@gmail.com","subject":"[GSoC][PATCH v3 2/4] submodule--helper: introduce for_each_listed_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-24T19:50:49Z","receivedAt":"2017-08-24T19:51:45Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_listed_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++----------\n 1 file changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e666f84ba..8cd81b144 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,9 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_listed_submodule(const struct module_list *list,\n+\t\t\t\t each_submodule_fn fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n {\n+\tstruct init_cb *info = cb_data;\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\t/* Only loads from .gitmodules, no overlay with .git/config */\n-\tgitmodules_config();\n-\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n \n-\tsub = submodule_from_path(&null_oid, path);\n+\tsub = submodule_from_path(&null_oid, list_item->name);\n \n \tif (!sub)\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t *\n \t * Set active flag for the submodule being initialized\n \t */\n-\tif (!is_submodule_active(the_repository, path)) {\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t\tstrbuf_reset(&sb);\n@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!info->quiet)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\n+\tgitmodules_config();\n+\tfor_each_listed_submodule(&list, init_submodule, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"327154","messageId":"20170824195051.30900-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170824195051.30900-1-pc44800@gmail.com","subject":"[GSoC][PATCH v3 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-24T19:50:50Z","receivedAt":"2017-08-24T19:51:53Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Function set_name_rev() is ported from git-submodule to the\nsubmodule--helper builtin. The function compute_rev_name() generates the\nvalue of the revision name as required.\nThe function get_rev_name() calls compute_rev_name() and receives the\nrevision name, and later handles its formating and printing.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 16 ++----------\n 2 files changed, 65 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 8cd81b144..6ea6408c2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n+static int get_rev_name(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *revname;\n+\tif (argc != 3)\n+\t\tdie(\"get-rev-name only accepts two arguments: \");\n+\n+\trevname = compute_rev_name(argv[1], argv[2]);\n+\tif (revname && revname[0])\n+\t\tprintf(\" (%s)\", revname);\n+\tprintf(\"\\n\");\n+\n+\tfree(revname);\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e131760ee..91f043ec6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -759,18 +759,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1042,14 +1030,14 @@ cmd_status()\n \t\tfi\n \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n \t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \" $sha1 $displaypath$revname\"\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.13.0\n\n"},{"id":"327155","messageId":"20170824195051.30900-5-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170824195051.30900-1-pc44800@gmail.com","subject":"[GSoC][PATCH v3 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-24T19:50:51Z","receivedAt":"2017-08-24T19:52:04Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nthree functions: module_status(), submodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_listed_submodule() looping through the\nlist obtained.\n\nThen for_each_listed_submodule() calls submodule_status() for each of the\nsubmodule in its list. The function submodule_status() is responsible\nfor generating the status each submodule it is called for, and\nthen calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 156 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 49 +-------------\n 2 files changed, 157 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6ea6408c2..577494e31 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -561,6 +561,161 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+\tunsigned int cached: 1;\n+};\n+#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n+\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+') {\n+\t\tstruct argv_array get_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushl(&get_rev_args, \"get-rev-name\",\n+\t\t\t\t path, oid_to_hex(oid), NULL);\n+\t\tget_rev_name(get_rev_args.argc, get_rev_args.argv,\n+\t\t\t info->prefix);\n+\n+\t\targv_array_clear(&get_rev_args);\n+\t} else {\n+\t\tprintf(\"\\n\");\n+\t}\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(&null_oid, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (ce_stage(list_item)) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t list_item->name, NULL);\n+\n+\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n+\t\t\t info->prefix)) {\n+\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (head_ref_submodule(list_item->name,\n+\t\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t\t \"submodule '%s'\"), list_item->name);\n+\n+\t\t\tprint_status(info, '+', list_item->name, &oid,\n+\t\t\t\t displaypath);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name,\n+\t\t\t\t &list_item->oid, displaypath);\n+\t\t}\n+\t}\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = list_item->name;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_pushl(&cpr.args, \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"status\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->cached)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"),\n+\t\t\t list_item->name);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint cached = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] []\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\tinfo.cached = !!cached;\n+\n+\tgitmodules_config();\n+\tfor_each_listed_submodule(&list, status_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1307,6 +1462,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 91f043ec6..51b057d82 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1005,54 +1005,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"327210","messageId":"xmqqlgm7scpm.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170824195051.30900-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T18:51:17Z","receivedAt":"2017-08-25T18:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. I'll try to queue these before I'll go offline.\n\nMentors may want to help the student further in adjusting the patch\nseries to the more recent codebase; unfortunately the area the GSoC\nproject touches is a bit fluid these days. I resolved the conflicts\nwith nd/pune-in-worktree and bw/submodule-config-cleanup topics so\nthat the result would compile, but I am not sure if the resolution\nis correct (e.g. there may be a new leak I introduced while doing\nso).\n\nThanks.\n\n\n"},{"id":"327215","messageId":"CAGZ79kZwpu9YXr5gSXWQBmqhXFB7+2rFWh6zVtRoiyQORkUsdg@mail.gmail.com","threadId":"46637","inReplyTo":"xmqqlgm7scpm.fsf@gitster.mtv.corp.google.com","subject":"Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-25T19:15:24Z","receivedAt":"2017-08-25T19:15:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 25, 2017 at 11:51 AM, Junio C Hamano wrote:\n> Thanks. I'll try to queue these before I'll go offline.\n>\n> Mentors may want to help the student further in adjusting the patch\n> series to the more recent codebase; unfortunately the area the GSoC\n> project touches is a bit fluid these days. I resolved the conflicts\n> with nd/pune-in-worktree and bw/submodule-config-cleanup topics so\n> that the result would compile, but I am not sure if the resolution\n> is correct (e.g. there may be a new leak I introduced while doing\n> so).\n>\n> Thanks.\n\nOk, noted.\n\nPresumably I'll review your dirty merge then for this series?\n(And later parts might go on top of the new dirty merge)\n"},{"id":"327219","messageId":"xmqqd17js80d.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"CAGZ79kZwpu9YXr5gSXWQBmqhXFB7+2rFWh6zVtRoiyQORkUsdg@mail.gmail.com","subject":"Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T20:32:50Z","receivedAt":"2017-08-25T20:33:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, Aug 25, 2017 at 11:51 AM, Junio C Hamano wrote:\n>> Thanks. I'll try to queue these before I'll go offline.\n>>\n>> Mentors may want to help the student further in adjusting the patch\n>> series to the more recent codebase; unfortunately the area the GSoC\n>> project touches is a bit fluid these days. I resolved the conflicts\n>> with nd/pune-in-worktree and bw/submodule-config-cleanup topics so\n>> that the result would compile, but I am not sure if the resolution\n>> is correct (e.g. there may be a new leak I introduced while doing\n>> so).\n>>\n>> Thanks.\n>\n> Ok, noted.\n>\n> Presumably I'll review your dirty merge then for this series?\n> (And later parts might go on top of the new dirty merge)\n\nIn my mind, GSoC mentors are there to help the student improve as a\ndeveloper, more than they are to help improving the immediate output\nof the student. So in that sense, I was hoping that you'd teach how\nto work well together with other developers, when one's topic has\ninteractions with topics by others. Making sure an inevitable evil\nmerge is made correctly is of course needed for the current topic,\nbut a more important skill to learn is to avoid the need to ask an\nevil merge to be made by the integrator in the first place.\n\n\n"},{"id":"327258","messageId":"CAME+mvVUtt5DU0wgL4nDTvuXsFrxnQ-8Obnf6VrDGdxsikfXEQ@mail.gmail.com","threadId":"46637","inReplyTo":"xmqqlgm7scpm.fsf@gitster.mtv.corp.google.com","subject":"Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-27T11:50:01Z","receivedAt":"2017-08-27T11:57:19Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"On Sat, Aug 26, 2017 at 12:21 AM, Junio C Hamano wrote:\n> Thanks. I'll try to queue these before I'll go offline.\n>\n> Mentors may want to help the student further in adjusting the patch\n> series to the more recent codebase; unfortunately the area the GSoC\n> project touches is a bit fluid these days. I resolved the conflicts\n> with nd/pune-in-worktree and bw/submodule-config-cleanup topics so\n> that the result would compile, but I am not sure if the resolution\n> is correct (e.g. there may be a new leak I introduced while doing\n> so).\n>\n\nThanks.\nI'll see the dirty merges and will resend the whole series after reviewing\nthe dirty merge and sending a new one with/without changes as required.\n\nThanks,\nPrathamesh Chavan\n"},{"id":"327276","messageId":"20170828115558.28297-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"xmqqlgm7scpm.fsf@gitster.mtv.corp.google.com","subject":"[GSoC][PATCH v4 0/4] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-28T11:55:54Z","receivedAt":"2017-08-28T11:56:26Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Changes in v4:\n* The patches were adjusted to recent codebase.\n Also, the gitmodules_config() was call was removed\n from the functions module_init() and module_status()\n which was essential after the merge of the branch\n bw/submodule-config-cleanup.\n Since it was mentioned earlier, I even tried basing\n the patch series on the branch nd/prune-in-worktree.\n Conflicts occured while doing so, since the later\n branch is based on the master branch with the HEAD\n pointing to the commit: The fourth batch post 2.14\n I think there won't be any conflicts, if that is changed.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/patch-series-1\n\nAnd its build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: patch-series-1\nBuild #167\n\nThe above changes were based on master branch.\n\nAnother branch, similar to the above, was created, but was based\non the 'next' branch.\n\nComplete build report of that is also available at:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: patch-series-1-next\nBuild #168\n\nThe above changes are also push on github and are available at:\nhttps://github.com/pratham-pc/git/commits/patch-series-1-next\n\nPrathamesh Chavan (4):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_listed_submodule()\n submodule: port set_name_rev() from shell to C\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 289 +++++++++++++++++++++++++++++++++++++++++---\n git-submodule.sh | 61 +---------\n 2 files changed, 271 insertions(+), 79 deletions(-)\n\n-- \n2.13.0\n\n"},{"id":"327277","messageId":"20170828115558.28297-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170828115558.28297-1-pc44800@gmail.com","subject":"[GSoC][PATCH v4 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-28T11:55:55Z","receivedAt":"2017-08-28T11:56:30Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++++------------\n 1 file changed, 25 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 818fe74f0..e25854371 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = is_dir_sep(super_prefix[len - 1]) ? \"%s%s\" : \"%s/%s\";\n+\n+\t\treturn xstrfmt(format, super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -335,15 +357,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -358,9 +372,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -368,7 +382,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -405,9 +418,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.13.0\n\n"},{"id":"327278","messageId":"20170828115558.28297-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170828115558.28297-1-pc44800@gmail.com","subject":"[GSoC][PATCH v4 2/4] submodule--helper: introduce for_each_listed_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-28T11:55:56Z","receivedAt":"2017-08-28T11:56:33Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_listed_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++--------\n 1 file changed, 28 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e25854371..ea99d8e39 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,9 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -351,15 +354,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_listed_submodule(const struct module_list *list,\n+\t\t\t\t each_submodule_fn fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n {\n+\tstruct init_cb *info = cb_data;\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n \n-\tsub = submodule_from_path(&null_oid, path);\n+\tsub = submodule_from_path(&null_oid, list_item->name);\n \n \tif (!sub)\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n@@ -371,7 +389,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t *\n \t * Set active flag for the submodule being initialized\n \t */\n-\tif (!is_submodule_active(the_repository, path)) {\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t\tstrbuf_reset(&sb);\n@@ -413,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!info->quiet)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -442,10 +460,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -470,8 +488,10 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\n+\tfor_each_listed_submodule(&list, init_submodule, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"327279","messageId":"20170828115558.28297-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170828115558.28297-1-pc44800@gmail.com","subject":"[GSoC][PATCH v4 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-28T11:55:57Z","receivedAt":"2017-08-28T11:56:36Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Function set_name_rev() is ported from git-submodule to the\nsubmodule--helper builtin. The function compute_rev_name() generates the\nvalue of the revision name as required.\nThe function get_rev_name() calls compute_rev_name() and receives the\nrevision name, and later handles its formating and printing.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 16 ++----------\n 2 files changed, 65 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ea99d8e39..85df11129 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n+static int get_rev_name(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *revname;\n+\tif (argc != 3)\n+\t\tdie(\"get-rev-name only accepts two arguments: \");\n+\n+\trevname = compute_rev_name(argv[1], argv[2]);\n+\tif (revname && revname[0])\n+\t\tprintf(\" (%s)\", revname);\n+\tprintf(\"\\n\");\n+\n+\tfree(revname);\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1292,6 +1354,7 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 66d1ae8ef..5211361c5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -758,18 +758,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1041,14 +1029,14 @@ cmd_status()\n \t\tfi\n \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n \t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \" $sha1 $displaypath$revname\"\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.13.0\n\n"},{"id":"327280","messageId":"20170828115558.28297-5-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170828115558.28297-1-pc44800@gmail.com","subject":"[GSoC][PATCH v4 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-08-28T11:55:58Z","receivedAt":"2017-08-28T11:56:42Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nthree functions: module_status(), submodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_listed_submodule() looping through the\nlist obtained.\n\nThen for_each_listed_submodule() calls submodule_status() for each of the\nsubmodule in its list. The function submodule_status() is responsible\nfor generating the status each submodule it is called for, and\nthen calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 155 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 49 +-------------\n 2 files changed, 156 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85df11129..abf5c126a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -558,6 +558,160 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+\tunsigned int cached: 1;\n+};\n+#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n+\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+') {\n+\t\tstruct argv_array get_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushl(&get_rev_args, \"get-rev-name\",\n+\t\t\t\t path, oid_to_hex(oid), NULL);\n+\t\tget_rev_name(get_rev_args.argc, get_rev_args.argv,\n+\t\t\t info->prefix);\n+\n+\t\targv_array_clear(&get_rev_args);\n+\t} else {\n+\t\tprintf(\"\\n\");\n+\t}\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(&null_oid, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (ce_stage(list_item)) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t list_item->name, NULL);\n+\n+\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n+\t\t\t info->prefix)) {\n+\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (head_ref_submodule(list_item->name,\n+\t\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t\t \"submodule '%s'\"), list_item->name);\n+\n+\t\t\tprint_status(info, '+', list_item->name, &oid,\n+\t\t\t\t displaypath);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name,\n+\t\t\t\t &list_item->oid, displaypath);\n+\t\t}\n+\t}\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = list_item->name;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_pushl(&cpr.args, \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"status\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->cached)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"),\n+\t\t\t list_item->name);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint cached = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] []\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\tinfo.cached = !!cached;\n+\n+\tfor_each_listed_submodule(&list, status_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1356,6 +1510,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5211361c5..156255a9e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1004,54 +1004,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"328569","messageId":"20170921150626.4979-1-hanwen@google.com","threadId":"46637","inReplyTo":"20170828115558.28297-2-pc44800@gmail.com","subject":"[GSoC][PATCH v4 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2017-09-21T15:06:26Z","receivedAt":"2017-09-21T15:06:43Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"LGTM with nits.\n\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n\nthis could do with a comment\n\n /* the result should be freed by the caller. */\n\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = is_dir_sep(super_prefix[len - 1]) ? \"%s%s\" : \"%s/%s\";\n\nwhat if len == 0? The handling of '/' looks like a change from the original.\n"},{"id":"328570","messageId":"20170921153155.8544-1-hanwen@google.com","threadId":"46637","inReplyTo":"20170828115558.28297-4-pc44800@gmail.com","subject":"[GSoC][PATCH v4 3/4] submodule: port set_name_rev() from shell to C","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2017-09-21T15:31:55Z","receivedAt":"2017-09-21T15:32:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"LGTM with nits\n\ncommit message:\n\n\"revision name, and later handles its formating and printing.\"\n\ntypo: formatting\n\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n\nyou discard all output if these commands fail, so if the argument is a\nnot a submodule, or the other is not a sha1, it will just print\nnothing without error message. Maybe that is OK, though? I don't see\ndocumentation for these commands, so maybe this is not meant to be\nusable?\n"},{"id":"328572","messageId":"20170921161059.11750-1-hanwen@google.com","threadId":"46637","inReplyTo":"20170828115558.28297-5-pc44800@gmail.com","subject":"[GSoC][PATCH v4 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2017-09-21T16:10:59Z","receivedAt":"2017-09-21T16:11:07Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] []\"),\n+\t\tNULL\n\nthe manpage over here says\n\n git submodule [--quiet] status [--cached] [--recursive] [--] [...]\n\nie. multiple path arguments. Should this usage string be tweaked?\n\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n\ncould do with a comment. What are the options for the `state` char?\n\n+\tif (state == ' ' || state == '+') {\n+\t\tstruct argv_array get_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushl(&get_rev_args, \"get-rev-name\",\n+\t\t\t\t path, oid_to_hex(oid), NULL);\n+\t\tget_rev_name(get_rev_args.argc, get_rev_args.argv,\n+\t\t\t info->prefix);\n\nsince you're not really subprocessing, can't you simply have a\n\n do_print_rev_name(char *path, char *sha) {\n ..\n printf(\"\\n\");\n }\n\nand call that directly? Or call compute_rev_name directly. Then you\ndon't have to do argv setup here.\n\nAlso, the name get_rev_name() is a little unfortunate, since it\ndoesn't return a name, but rather prints it. Maybe the functions\nimplementing helper commands could be named like:\n\n command_get_rev_name\n\nor similar.\n"},{"id":"328766","messageId":"20170924120858.26813-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170921161059.11750-1-hanwen@google.com","subject":"[PATCH v5 0/4] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-24T12:08:54Z","receivedAt":"2017-09-24T12:09:21Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Changes in v5:\n* in print_status() function, we now call compute_rev_name() directly\n instead of doing the argv setup and calling get_rev_name() function.\n* Since we no longer use the helper function get_rev_name(), we have\n removed it from the code base after porting submodule subcommand\n 'status'.\n* function get_submodule_displaypath() has been modified, and now handles\n the case of super_prefix being non-null and of length zero.\n* I have still kept the name of the function get_rev_name() unchanged,\n since in the function cmd_status(), it is used to get the value of\n the variable revname. And in the next commit since we do not require\n the function anymore, we reomve it from the code base.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/patch-series-1\n\nAnd its build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: patch-series-1\nBuild #179\n\nThanks, Han-Wen Nienhuys for reviewing the previous patch series.\n\nPrathamesh Chavan (4):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_listed_submodule()\n submodule: port set_name_rev() from shell to C\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 265 ++++++++++++++++++++++++++++++++++++++++----\n git-submodule.sh | 61 +---------\n 2 files changed, 247 insertions(+), 79 deletions(-)\n\n-- \n2.13.0\n\n"},{"id":"328767","messageId":"20170924120858.26813-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170924120858.26813-1-pc44800@gmail.com","subject":"[PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-24T12:08:55Z","receivedAt":"2017-09-24T12:09:25Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 38 ++++++++++++++++++++++++++------------\n 1 file changed, 26 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 818fe74f0..d24ac9028 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,29 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\tint len = strlen(super_prefix);\n+\t\tconst char *format = (len > 0 && is_dir_sep(super_prefix[len - 1])) ? \"%s%s\" : \"%s/%s\";\n+\n+\t\treturn xstrfmt(format, super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -335,15 +358,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -358,9 +373,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -368,7 +383,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -405,9 +419,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.13.0\n\n"},{"id":"328768","messageId":"20170924120858.26813-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170924120858.26813-1-pc44800@gmail.com","subject":"[PATCH v5 2/4] submodule--helper: introduce for_each_listed_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-24T12:08:56Z","receivedAt":"2017-09-24T12:09:27Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_listed_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++--------\n 1 file changed, 28 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d24ac9028..d12790b5c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,9 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -352,15 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_listed_submodule(const struct module_list *list,\n+\t\t\t\t each_submodule_fn fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n {\n+\tstruct init_cb *info = cb_data;\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n \n-\tsub = submodule_from_path(&null_oid, path);\n+\tsub = submodule_from_path(&null_oid, list_item->name);\n \n \tif (!sub)\n \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n@@ -372,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t *\n \t * Set active flag for the submodule being initialized\n \t */\n-\tif (!is_submodule_active(the_repository, path)) {\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n \t\tstrbuf_reset(&sb);\n@@ -414,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!info->quiet)\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -443,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -471,8 +489,10 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\n+\tfor_each_listed_submodule(&list, init_submodule, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"328769","messageId":"20170924120858.26813-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170924120858.26813-1-pc44800@gmail.com","subject":"[PATCH v5 3/4] submodule: port set_name_rev() from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-24T12:08:57Z","receivedAt":"2017-09-24T12:09:29Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Function set_name_rev() is ported from git-submodule to the\nsubmodule--helper builtin. The function compute_rev_name() generates the\nvalue of the revision name as required.\nThe function get_rev_name() calls compute_rev_name() and receives the\nrevision name, and later handles its formatting and printing.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 63 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 16 ++----------\n 2 files changed, 65 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d12790b5c..7ca8e8153 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -246,6 +246,68 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n+static int get_rev_name(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *revname;\n+\tif (argc != 3)\n+\t\tdie(\"get-rev-name only accepts two arguments: \");\n+\n+\trevname = compute_rev_name(argv[1], argv[2]);\n+\tif (revname && revname[0])\n+\t\tprintf(\" (%s)\", revname);\n+\tprintf(\"\\n\");\n+\n+\tfree(revname);\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1293,6 +1355,7 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 66d1ae8ef..5211361c5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -758,18 +758,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1041,14 +1029,14 @@ cmd_status()\n \t\tfi\n \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n \t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \" $sha1 $displaypath$revname\"\n \t\telse\n \t\t\tif test -z \"$cached\"\n \t\t\tthen\n \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n \t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n+\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.13.0\n\n"},{"id":"328770","messageId":"20170924120858.26813-5-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170924120858.26813-1-pc44800@gmail.com","subject":"[PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-24T12:08:58Z","receivedAt":"2017-09-24T12:09:34Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nthree functions: module_status(), submodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_listed_submodule() looping through the\nlist obtained.\n\nThen for_each_listed_submodule() calls submodule_status() for each of the\nsubmodule in its list. The function submodule_status() is responsible\nfor generating the status each submodule it is called for, and\nthen calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nHelper function get_rev_name() removed after porting the above subcommand\nas it is no longer used.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 162 +++++++++++++++++++++++++++++++++++++++-----\n git-submodule.sh | 49 +-------------\n 2 files changed, 147 insertions(+), 64 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 7ca8e8153..8876a4a08 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -293,21 +293,6 @@ static char *compute_rev_name(const char *sub_path, const char* object_id)\n \treturn NULL;\n }\n \n-static int get_rev_name(int argc, const char **argv, const char *prefix)\n-{\n-\tchar *revname;\n-\tif (argc != 3)\n-\t\tdie(\"get-rev-name only accepts two arguments: \");\n-\n-\trevname = compute_rev_name(argv[1], argv[2]);\n-\tif (revname && revname[0])\n-\t\tprintf(\" (%s)\", revname);\n-\tprintf(\"\\n\");\n-\n-\tfree(revname);\n-\treturn 0;\n-}\n-\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -559,6 +544,151 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+\tunsigned int cached: 1;\n+};\n+#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n+\n+static void print_status(struct status_cb *info, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+')\n+\t\tprintf(\" (%s)\", compute_rev_name(path, oid_to_hex(oid)));\n+\n+\tprintf(\"\\n\");\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(&null_oid, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (ce_stage(list_item)) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t list_item->name, NULL);\n+\n+\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n+\t\t\t info->prefix)) {\n+\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n+\t\t\t displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (refs_head_ref(get_submodule_ref_store(list_item->name), handle_submodule_head_ref, &oid))\n+\t\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t\t \"submodule '%s'\"), list_item->name);\n+\n+\t\t\tprint_status(info, '+', list_item->name, &oid,\n+\t\t\t\t displaypath);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name,\n+\t\t\t\t &list_item->oid, displaypath);\n+\t\t}\n+\t}\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = list_item->name;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_pushl(&cpr.args, \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"status\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->cached)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"),\n+\t\t\t list_item->name);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint cached = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\tinfo.cached = !!cached;\n+\n+\tfor_each_listed_submodule(&list, status_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1355,8 +1485,8 @@ static struct cmd_struct commands[] = {\n \t{\"relative-path\", resolve_relative_path, 0},\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n-\t{\"get-rev-name\", get_rev_name, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5211361c5..156255a9e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1004,54 +1004,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"328782","messageId":"xmqqshfbfo21.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170924120858.26813-2-pc44800@gmail.com","subject":"Re: [PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T03:35:18Z","receivedAt":"2017-09-25T03:35:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> Introduce function get_submodule_displaypath() to replace the code\n> occurring in submodule_init() for generating displaypath of the\n> submodule with a call to it.\n>\n> This new function will also be used in other parts of the system\n> in later patches.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> builtin/submodule--helper.c | 38 ++++++++++++++++++++++++++------------\n> 1 file changed, 26 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 818fe74f0..d24ac9028 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -220,6 +220,29 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n> \treturn 0;\n> }\n> \n> +/* the result should be freed by the caller. */\n> +static char *get_submodule_displaypath(const char *path, const char *prefix)\n> +{\n> +\tconst char *super_prefix = get_super_prefix();\n> +\n> +\tif (prefix && super_prefix) {\n> +\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n> +\t\t prefix, super_prefix);\n> +\t} else if (prefix) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n> +\t\tstrbuf_release(&sb);\n> +\t\treturn displaypath;\n> +\t} else if (super_prefix) {\n> +\t\tint len = strlen(super_prefix);\n> +\t\tconst char *format = (len > 0 && is_dir_sep(super_prefix[len - 1])) ? \"%s%s\" : \"%s/%s\";\n> +\n> +\t\treturn xstrfmt(format, super_prefix, path);\n> +\t} else {\n> +\t\treturn xstrdup(path);\n> +\t}\n> +}\n\nLooks like a fairly faithful rewrite of the original below, with a\nreasonable clean-up (e.g. use of xstrfmt() instead of addf()).\n\nOne thing I noticed is that future callers of this function, unlike\ninit_submodule() which is its original caller, are allowed to have\nsuper-prefix that does not end in a slash, because the helper\nautomatically supplies one when it is missing.\n\nI am not sure the added leniency is desirable [*1*], but in any\ncase, the added leniency deserves a mention in the log message, I\nwould think.\n\n\n[Footnote]\n\n*1* Often it is harder to introduce bugs when the internal rules are\nstricter, e.g. \"prefix must end with slash\", than when they are\nlooser, e.g. \"prefix may or may not end with slash\", because\nallowing different codepaths to have the same thing in different\nrepresentations would hide bugs that is only uncovered when these\ncodepaths eventually meet (e.g. one codepath has a path with and the\nother without trailing slash and they both think that is the\nprefix---then they use strcmp() to see if they have the same prefix,\nwhich would be a bug, which may not be noticed for a long time).\n\n> struct module_list {\n> \tconst struct cache_entry **entries;\n> \tint alloc, nr;\n> @@ -335,15 +358,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tchar *upd = NULL, *url = NULL, *displaypath;\n> \n> -\tif (prefix && get_super_prefix())\n> -\t\tdie(\"BUG: cannot have prefix and superprefix\");\n> -\telse if (prefix)\n> -\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n> -\telse if (get_super_prefix()) {\n> -\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n> -\t\tdisplaypath = strbuf_detach(&sb, NULL);\n> -\t} else\n> -\t\tdisplaypath = xstrdup(path);\n> +\tdisplaypath = get_submodule_displaypath(path, prefix);\n> \n> \tsub = submodule_from_path(&null_oid, path);\n> \n> @@ -358,9 +373,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t * Set active flag for the submodule being initialized\n> \t */\n> \tif (!is_submodule_active(the_repository, path)) {\n> -\t\tstrbuf_reset(&sb);\n> \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n> \t\tgit_config_set_gently(sb.buf, \"true\");\n> +\t\tstrbuf_reset(&sb);\n> \t}\n> \n> \t/*\n> @@ -368,7 +383,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t * To look up the url in .git/config, we must not fall back to\n> \t * .gitmodules, so look it up directly.\n> \t */\n> -\tstrbuf_reset(&sb);\n> \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n> \tif (git_config_get_string(sb.buf, &url)) {\n> \t\tif (!sub->url)\n> @@ -405,9 +419,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n> \t\t\t\tsub->name, url, displaypath);\n> \t}\n> +\tstrbuf_reset(&sb);\n> \n> \t/* Copy \"update\" setting when it is not set yet */\n> -\tstrbuf_reset(&sb);\n> \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n> \tif (git_config_get_string(sb.buf, &upd) &&\n> \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n"},{"id":"328783","messageId":"xmqqmv5jfnod.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170924120858.26813-3-pc44800@gmail.com","subject":"Re: [PATCH v5 2/4] submodule--helper: introduce for_each_listed_submodule()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T03:43:30Z","receivedAt":"2017-09-25T03:43:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> Introduce function for_each_listed_submodule() and replace a loop\n> in module_init() with a call to it.\n>\n> The new function will also be used in other parts of the\n> system in later patches.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> builtin/submodule--helper.c | 36 ++++++++++++++++++++++++++++--------\n> 1 file changed, 28 insertions(+), 8 deletions(-)\n\nThis looks more or less perfectly done, but I say this basing it on\nan assumption that nobody ever would want to initialize a single\nsubmodule without going through module_init() interface.\n\nIf there will be a caller that would have made a direct call to\ninit_submodule() if this patch did not exist, then I think this\npatch is better done by keeping the external interface to the\ninit_submodule() function intact, and introducing an intermediate\nhelper init_submodule_cb() function that is to be used as the\ncallback used in for_each_listed_submodule() API. In general, we\nshould make sure that these callback functions that take \"void\n*cb_data\" parameters are called _only_ by these iterators that\ndefined the callback (in this case, for_each_listed_submodule())\nand not other functions.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d24ac9028..d12790b5c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -14,6 +14,9 @@\n> #include \"refs.h\"\n> #include \"connect.h\"\n> \n> +typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n> +\t\t\t\t void *cb_data);\n> +\n> static char *get_default_remote(void)\n> {\n> \tchar *dest = NULL, *ret;\n> @@ -352,15 +355,30 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> -static void init_submodule(const char *path, const char *prefix, int quiet)\n> +static void for_each_listed_submodule(const struct module_list *list,\n> +\t\t\t\t each_submodule_fn fn, void *cb_data)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < list->nr; i++)\n> +\t\tfn(list->entries[i], cb_data);\n> +}\n> +\n> +struct init_cb {\n> +\tconst char *prefix;\n> +\tunsigned int quiet: 1;\n> +};\n> +#define INIT_CB_INIT { NULL, 0 }\n> +\n> +static void init_submodule(const struct cache_entry *list_item, void *cb_data)\n> {\n> +\tstruct init_cb *info = cb_data;\n> \tconst struct submodule *sub;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tchar *upd = NULL, *url = NULL, *displaypath;\n> \n> -\tdisplaypath = get_submodule_displaypath(path, prefix);\n> +\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n> \n> -\tsub = submodule_from_path(&null_oid, path);\n> +\tsub = submodule_from_path(&null_oid, list_item->name);\n> \n> \tif (!sub)\n> \t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> @@ -372,7 +390,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t *\n> \t * Set active flag for the submodule being initialized\n> \t */\n> -\tif (!is_submodule_active(the_repository, path)) {\n> +\tif (!is_submodule_active(the_repository, list_item->name)) {\n> \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n> \t\tgit_config_set_gently(sb.buf, \"true\");\n> \t\tstrbuf_reset(&sb);\n> @@ -414,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \t\tif (git_config_set_gently(sb.buf, url))\n> \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n> \t\t\t displaypath);\n> -\t\tif (!quiet)\n> +\t\tif (!info->quiet)\n> \t\t\tfprintf(stderr,\n> \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n> \t\t\t\tsub->name, url, displaypath);\n> @@ -443,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> \n> static int module_init(int argc, const char **argv, const char *prefix)\n> {\n> +\tstruct init_cb info = INIT_CB_INIT;\n> \tstruct pathspec pathspec;\n> \tstruct module_list list = MODULE_LIST_INIT;\n> \tint quiet = 0;\n> -\tint i;\n> \n> \tstruct option module_init_options[] = {\n> \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n> @@ -471,8 +489,10 @@ static int module_init(int argc, const char **argv, const char *prefix)\n> \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n> \t\tmodule_list_active(&list);\n> \n> -\tfor (i = 0; i < list.nr; i++)\n> -\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n> +\tinfo.prefix = prefix;\n> +\tinfo.quiet = !!quiet;\n> +\n> +\tfor_each_listed_submodule(&list, init_submodule, &info);\n> \n> \treturn 0;\n> }\n"},{"id":"328784","messageId":"xmqqfubbfnan.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170924120858.26813-4-pc44800@gmail.com","subject":"Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T03:51:44Z","receivedAt":"2017-09-25T03:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> +static char *compute_rev_name(const char *sub_path, const char* object_id)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char ***d;\n> +\n> +\tstatic const char *describe_bare[] = {\n> +\t\tNULL\n> +\t};\n> +...\n> +\tstatic const char **describe_argv[] = {\n> +\t\tdescribe_bare, describe_tags, describe_contains,\n> +\t\tdescribe_all_always, NULL\n> +\t};\n> +\n> +\tfor (d = describe_argv; *d; d++) {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tprepare_submodule_repo_env(&cp.env_array);\n> +\t\tcp.dir = sub_path;\n> +\t\tcp.git_cmd = 1;\n> +\t\tcp.no_stderr = 1;\n> +\n> +\t\targv_array_push(&cp.args, \"describe\");\n> +\t\targv_array_pushv(&cp.args, *d);\n> +\t\targv_array_push(&cp.args, object_id);\n\nNicely done.\n\n> +\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n\nObservation: even if the command did not fail, if it returned an\nempty string, then you would reject that result and loop on here.\nThis is different from the original scripted version, but it should\nbe OK.\n\n> +\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n> +\t\t\treturn strbuf_detach(&sb, NULL);\n> +\t\t}\n> +\t}\n> +\n> +\tstrbuf_release(&sb);\n> +\treturn NULL;\n\nObservation: and if you do not find any non-empty-string answer, you\nreturn NULL.\n\n> +}\n> +\n> +static int get_rev_name(int argc, const char **argv, const char *prefix)\n> +{\n> +\tchar *revname;\n> +\tif (argc != 3)\n> +\t\tdie(\"get-rev-name only accepts two arguments: \");\n> +\n> +\trevname = compute_rev_name(argv[1], argv[2]);\n> +\tif (revname && revname[0])\n> +\t\tprintf(\" (%s)\", revname);\n\nSo, while it is not wrong per-se, I do not think we need to check\nrevname[0] here. The helper never returns a non-NULL pointer that\npoints at an empty string, right?\n\nOn the other hand, if we dropped the \"&& sb.len\" check in the helper\nfunction to be more faithful to the original, then we must check\nrevname[0] for an empty string.\n\n> +\tprintf(\"\\n\");\n> +\n> +\tfree(revname);\n> +\treturn 0;\n> +}\n> +\n> struct module_list {\n> \tconst struct cache_entry **entries;\n> \tint alloc, nr;\n> @@ -1293,6 +1355,7 @@ static struct cmd_struct commands[] = {\n> \t{\"relative-path\", resolve_relative_path, 0},\n> \t{\"resolve-relative-url\", resolve_relative_url, 0},\n> \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n> +\t{\"get-rev-name\", get_rev_name, 0},\n> \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n> \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n> \t{\"push-check\", push_check, 0},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 66d1ae8ef..5211361c5 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -758,18 +758,6 @@ cmd_update()\n> \t}\n> }\n> \n> -set_name_rev () {\n> -\trevname=$( (\n> -\t\tsanitize_submodule_env\n> -\t\tcd \"$1\" && {\n> -\t\t\tgit describe \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n> -\t\t\tgit describe --all --always \"$2\"\n> -\t\t}\n> -\t) )\n> -\ttest -z \"$revname\" || revname=\" ($revname)\"\n> -}\n> #\n> # Show commit summary for submodules in index or working tree\n> #\n> @@ -1041,14 +1029,14 @@ cmd_status()\n> \t\tfi\n> \t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n> \t\tthen\n> -\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n> +\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n> \t\t\tsay \" $sha1 $displaypath$revname\"\n> \t\telse\n> \t\t\tif test -z \"$cached\"\n> \t\t\tthen\n> \t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n> \t\t\tfi\n> -\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n> +\t\t\trevname=$(git submodule--helper get-rev-name \"$sm_path\" \"$sha1\")\n> \t\t\tsay \"+$sha1 $displaypath$revname\"\n> \t\tfi\n"},{"id":"328785","messageId":"xmqq8th3fn4u.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"xmqqfubbfnan.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T03:55:13Z","receivedAt":"2017-09-25T03:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nicely done.\n>\n>> +\t\tif (!capture_command(&cp, &sb, 0) && sb.len) {\n> ...\n> So, while it is not wrong per-se, I do not think we need to check\n> revname[0] here. The helper never returns a non-NULL pointer that\n> points at an empty string, right?\n>\n> On the other hand, if we dropped the \"&& sb.len\" check in the helper\n> function to be more faithful to the original, then we must check\n> revname[0] for an empty string.\n\nAh, ignore all of the above. This will all be discarded in the next\nstep [4/4], as far as I can tell. Perhaps we should drop this step\nand get directly to it, making the result a three-patch series\ninstead, then, no?\n\n"},{"id":"328789","messageId":"xmqq4lrrfjt9.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170924120858.26813-5-pc44800@gmail.com","subject":"Re: [PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T05:06:58Z","receivedAt":"2017-09-25T05:07:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> This aims to make git-submodule 'status' a built-in. Hence, the function\n> cmd_status() is ported from shell to C. This is done by introducing\n> three functions: module_status(), submodule_status() and print_status().\n\nWell then it does not just aim to, but it does, doesn't it ;-)?\n\n> @@ -559,6 +544,151 @@ static int module_init(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +struct status_cb {\n> +\tconst char *prefix;\n> +\tunsigned int quiet: 1;\n> +\tunsigned int recursive: 1;\n> +\tunsigned int cached: 1;\n> +};\n> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }\n> +\n> +static void print_status(struct status_cb *info, char state, const char *path,\n> +\t\t\t const struct object_id *oid, const char *displaypath)\n> +{\n> +\tif (info->quiet)\n> +\t\treturn;\n> +\n> +\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n> +\n> +\tif (state == ' ' || state == '+')\n> +\t\tprintf(\" (%s)\", compute_rev_name(path, oid_to_hex(oid)));\n> +\n> +\tprintf(\"\\n\");\n> +}\n> +\n> +static int handle_submodule_head_ref(const char *refname,\n> +\t\t\t\t const struct object_id *oid, int flags,\n> +\t\t\t\t void *cb_data)\n> +{\n> +\tstruct object_id *output = cb_data;\n> +\tif (oid)\n> +\t\toidcpy(output, oid);\n> +\n> +\treturn 0;\n> +}\n\nAll of the above look sensible.\n\n> +static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n> +{\n> +\tstruct status_cb *info = cb_data;\n> +\tchar *displaypath;\n> +\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n> +\n> +\tif (!submodule_from_path(&null_oid, list_item->name))\n> +\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n> +\t\t list_item->name);\n> +\n> +\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n> +\n> +\tif (ce_stage(list_item)) {\n> +\t\tprint_status(info, 'U', list_item->name,\n> +\t\t\t &null_oid, displaypath);\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\tif (!is_submodule_active(the_repository, list_item->name)) {\n> +\t\tprint_status(info, '-', list_item->name, &list_item->oid,\n> +\t\t\t displaypath);\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\targv_array_pushl(&diff_files_args, \"diff-files\",\n> +\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n> +\t\t\t list_item->name, NULL);\n> +\n> +\tif (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,\n> +\t\t\t info->prefix)) {\n\nCalling any cmd_foo() from places other than git.c, especially if it\nis done more than once, is a source of bug. I didn't check closely\nif cmd_diff_files() currently has any of the potential problems, but\nthese functions are free to (1) modify global or file-scope static\nvariables, assuming that they will not be called again, leaving\nthese variables in a state different from the original and making\ncmd_foo() unsuitable to be called twice, and (2) exit instead of\nreturn at the end, among other things.\n\nThe functions in diff-lib.c (I think run_diff_files() for this\napplication) are designed to be called multiple times as library-ish\nhelpers; this caller should be using them instead.\n\n> +\t\tprint_status(info, ' ', list_item->name, &list_item->oid,\n> +\t\t\t displaypath);\n> +\t} else {\n> +\t\tif (!info->cached) {\n\nBy using \"else if (!info->cached) {\" here, you can reduce the\nnesting level, perhaps?\n\n> +static int module_status(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct status_cb info = STATUS_CB_INIT;\n> +\tstruct pathspec pathspec;\n> +\tstruct module_list list = MODULE_LIST_INIT;\n> +\tint quiet = 0;\n> +\tint cached = 0;\n> +\tint recursive = 0;\n> +\n> +\tstruct option module_status_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n> +\t\tOPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n> +\t\tOPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const git_submodule_helper_usage[] = {\n> +\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] [...]\"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_status_options,\n> +\t\t\t git_submodule_helper_usage, 0);\n> +\n> +\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n> +\t\treturn 1;\n> +\n> +\tinfo.prefix = prefix;\n> +\tinfo.quiet = !!quiet;\n> +\tinfo.recursive = !!recursive;\n> +\tinfo.cached = !!cached;\n> +\n> +\tfor_each_listed_submodule(&list, status_submodule, &info);\n> +\n> +\treturn 0;\n> +}\n\nThe same comment as [2/4] on \"init_submodule()? don't you want\ninit_submodule_cb() instead?\" applies to \"status_submodule()\". I\nsuspect that in the future we would want more direct calls to show\nthe status of one single submodule, without indirectly controlling\nwhat is chosen via the &list interface, and if that is the case,\nyou'd want the interface into the leaf level helper function to be a\nmore direct one that is not based on \"void *cb_data\", with a thin\nwrapper around it that is to be used as the callback function by\nfor_each_listed_submodule().\n\nOther than that, looks very cleanly done.\n\nThanks.\n"},{"id":"329166","messageId":"20170929094453.4499-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"xmqq4lrrfjt9.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v6 0/3] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-29T09:44:50Z","receivedAt":"2017-09-29T09:45:09Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"changes in v6:\n\n* The function get_submodule_displaypath() was modified for the case\n when get_super_prefix() returns a non-null value. The condition to check\n if the super-prefix ends with a '/' is removed. To accomodate this change\n appropriate value of super_prefix is passed instead in the recursive calls\n of init_submodule() and status_submodule().\n\n* To accomodate the possiblity of a direct call to the function\n init_submodule(), a callback function init_submodule_cb() is introduced\n which takes cache_entry and init_cb structures as input params, and\n calls init_submodule() with parameters which are more appropriate\n for a direct call of this function.\n\n* Similar changes were even done for status_submodule(). But as it was\n observed that the number of params increased a lot due to flags\n like quiet, recursive, cached, etc, and keeping in mind the future\n subcommand's ported functions as well, a single unsigned int called\n cb_flags was introduced to store all of these flags, instead of having\n parameter for each one.\n\n* Patches [3/4] and [4/4] from the previous series were merged as a single\n step.\n\n* Call to function cmd_diff_files was avoided in the function status_submodule()\n and instead used the function run_diff_files() for the same purpose.\n\nSince there were many changes the patches required, I took more time on\nmaking these changes. Thank you, Junio for the last reviews. They\nhelped a lot for improving the patch series.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/patch-series-1\n\nAnd its build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: patch-series-1\nBuild #184\n\nPrathamesh Chavan (3):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_listed_submodule()\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 281 +++++++++++++++++++++++++++++++++++++++++---\n git-submodule.sh | 61 +---------\n 2 files changed, 265 insertions(+), 77 deletions(-)\n\n-- \n2.13.0\n\n"},{"id":"329167","messageId":"20170929094453.4499-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170929094453.4499-1-pc44800@gmail.com","subject":"[PATCH v6 1/3] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-29T09:44:51Z","receivedAt":"2017-09-29T09:45:11Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 818fe74f0..cdae54426 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -220,6 +220,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\treturn xstrfmt(\"%s%s\", super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -335,15 +355,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -358,9 +370,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -368,7 +380,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -405,9 +416,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.13.0\n\n"},{"id":"329168","messageId":"20170929094453.4499-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170929094453.4499-1-pc44800@gmail.com","subject":"[PATCH v6 2/3] submodule--helper: introduce for_each_listed_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-29T09:44:52Z","receivedAt":"2017-09-29T09:45:17Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_listed_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 39 ++++++++++++++++++++++++++++++++++-----\n 1 file changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex cdae54426..20a1ef868 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,11 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+#define CB_OPT_QUIET\t\t(1<<0)\n+\n+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -349,7 +354,22 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_listed_submodule(const struct module_list *list,\n+\t\t\t\t each_submodule_fn fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int cb_flags;\n+};\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const char *path, const char *prefix,\n+\t\t\t unsigned int cb_flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -411,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!(cb_flags & CB_OPT_QUIET))\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -438,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tfree(upd);\n }\n \n+static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct init_cb *info = cb_data;\n+\tinit_submodule(list_item->name, info->prefix, info->cb_flags);\n+}\n+\n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -468,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tif (quiet)\n+\t\tinfo.cb_flags |= CB_OPT_QUIET;\n+\n+\tfor_each_listed_submodule(&list, init_submodule_cb, &info);\n \n \treturn 0;\n }\n-- \n2.13.0\n\n"},{"id":"329169","messageId":"20170929094453.4499-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20170929094453.4499-1-pc44800@gmail.com","subject":"[PATCH v6 3/3] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-09-29T09:44:53Z","receivedAt":"2017-09-29T09:45:19Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nfour functions: module_status(), submodule_status_cb(),\nsubmodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_listed_submodule() looping through the\nlist obtained.\n\nThen for_each_listed_submodule() calls submodule_status_cb() for each of\nthe submodule in its list. The function submodule_status_cb() calls\nsubmodule_status() after passing appropriate arguments to the funciton.\nFunction submodule_status() is responsible for generating the status\neach submodule it is called for, and then calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nFunction set_name_rev() is also ported from git-submodule to the\nsubmodule--helper builtin function compute_rev_name(), which now\ngenerates the value of the revision name as required.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 207 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 61 +------------\n 2 files changed, 208 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 20a1ef868..50d38fc20 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -13,8 +13,13 @@\n #include \"remote.h\"\n #include \"refs.h\"\n #include \"connect.h\"\n+#include \"revision.h\"\n+#include \"diffcore.h\"\n+#include \"diff.h\"\n \n #define CB_OPT_QUIET\t\t(1<<0)\n+#define CB_OPT_CACHED\t\t(1<<1)\n+#define CB_OPT_RECURSIVE\t(1<<2)\n \n typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \t\t\t\t void *cb_data);\n@@ -245,6 +250,53 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = {\n+\t\tNULL\n+\t};\n+\n+\tstatic const char *describe_tags[] = {\n+\t\t\"--tags\", NULL\n+\t};\n+\n+\tstatic const char *describe_contains[] = {\n+\t\t\"--contains\", NULL\n+\t};\n+\n+\tstatic const char *describe_all_always[] = {\n+\t\t\"--all\", \"--always\", NULL\n+\t};\n+\n+\tstatic const char **describe_argv[] = {\n+\t\tdescribe_bare, describe_tags, describe_contains,\n+\t\tdescribe_all_always, NULL\n+\t};\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0)) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -503,6 +555,160 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int cb_flags;\n+};\n+#define STATUS_CB_INIT { NULL, 0 }\n+\n+static void print_status(unsigned int flags, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (flags & CB_OPT_QUIET)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+')\n+\t\tprintf(\" (%s)\", compute_rev_name(path, oid_to_hex(oid)));\n+\n+\tprintf(\"\\n\");\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const char *path, const struct object_id *ce_oid,\n+\t\t\t unsigned int ce_flags, const char *prefix,\n+\t\t\t unsigned int cb_flags)\n+{\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\tstruct rev_info rev;\n+\tint diff_files_result;\n+\n+\tif (!submodule_from_path(&null_oid, path))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t path);\n+\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\n+\tif ((CE_STAGEMASK & ce_flags) >> CE_STAGESHIFT) {\n+\t\tprint_status(cb_flags, 'U', path, &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, path)) {\n+\t\tprint_status(cb_flags, '-', path, ce_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t path, NULL);\n+\n+\tgit_config(git_diff_basic_config, NULL);\n+\tinit_revisions(&rev, prefix);\n+\trev.abbrev = 0;\n+\tprecompose_argv(diff_files_args.argc, diff_files_args.argv);\n+\tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n+\t\t\t\t\t diff_files_args.argv,\n+\t\t\t\t\t &rev, NULL);\n+\tdiff_files_result = run_diff_files(&rev, 0);\n+\n+\tif (!diff_result_code(&rev.diffopt, diff_files_result)) {\n+\t\tprint_status(cb_flags, ' ', path, ce_oid,\n+\t\t\t displaypath);\n+\t} else if (!(cb_flags & CB_OPT_CACHED)) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (refs_head_ref(get_submodule_ref_store(path),\n+\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t \"submodule '%s'\"), path);\n+\n+\t\tprint_status(cb_flags, '+', path, &oid, displaypath);\n+\t} else {\n+\t\tprint_status(cb_flags, '+', path, ce_oid, displaypath);\n+\t}\n+\n+\tif (cb_flags & CB_OPT_RECURSIVE) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = path;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_push(&cpr.args, \"--super-prefix\");\n+\t\targv_array_pushf(&cpr.args, \"%s/\", displaypath);\n+\t\targv_array_pushl(&cpr.args, \"submodule--helper\", \"status\",\n+\t\t\t\t \"--recursive\", NULL);\n+\n+\t\tif (cb_flags & CB_OPT_CACHED)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (cb_flags & CB_OPT_QUIET)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"), path);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static void status_submodule_cb(const struct cache_entry *list_item,\n+\t\t\t\tvoid *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tstatus_submodule(list_item->name, &list_item->oid, list_item->ce_flags,\n+\t\t\t info->prefix, info->cb_flags);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BIT(0, \"cached\", &info.cb_flags, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\"), CB_OPT_CACHED),\n+\t\tOPT_BIT(0, \"recursive\", &info.cb_flags, N_(\"recurse into nested submodules\"), CB_OPT_RECURSIVE),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tif (quiet)\n+\t\tinfo.cb_flags |= CB_OPT_QUIET;\n+\n+\tfor_each_listed_submodule(&list, status_submodule_cb, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1300,6 +1506,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 66d1ae8ef..156255a9e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -758,18 +758,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1016,54 +1004,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.13.0\n\n"},{"id":"329364","messageId":"xmqqo9pqs7qm.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170929094453.4499-1-pc44800@gmail.com","subject":"Re: [PATCH v6 0/3] Incremental rewrite of git-submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T00:39:45Z","receivedAt":"2017-10-02T00:39:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> * The function get_submodule_displaypath() was modified for the case\n> when get_super_prefix() returns a non-null value. The condition to check\n> if the super-prefix ends with a '/' is removed. To accomodate this change\n> appropriate value of super_prefix is passed instead in the recursive calls\n> of init_submodule() and status_submodule().\n\nOK.\n\n> * To accomodate the possiblity of a direct call to the function\n> init_submodule(), a callback function init_submodule_cb() is introduced\n> which takes cache_entry and init_cb structures as input params, and\n> calls init_submodule() with parameters which are more appropriate\n> for a direct call of this function.\n\nOK.\n\n> * Similar changes were even done for status_submodule(). But as it was\n> observed that the number of params increased a lot due to flags\n> like quiet, recursive, cached, etc, and keeping in mind the future\n> subcommand's ported functions as well, a single unsigned int called\n> cb_flags was introduced to store all of these flags, instead of having\n> parameter for each one.\n\nUse of a flag word instead of many bit parameter is a very good\nidea. I do not think it is brilliant to call a field in a structure\nthat is used as an interface to the callback interface cb_flags,\nthough, especially when it is already clear that it is about the\ncallback from the name the structure. Just name them \"flags\".\n\nI may or may not leave more detailed comments on individual patches.\n\nThanks.\n"},{"id":"329365","messageId":"xmqqk20es7i0.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170929094453.4499-2-pc44800@gmail.com","subject":"Re: [PATCH v6 1/3] submodule--helper: introduce get_submodule_displaypath()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T00:44:55Z","receivedAt":"2017-10-02T00:45:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This step looks good to me. Thanks.\n"},{"id":"329366","messageId":"xmqq8tgus6zw.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170929094453.4499-3-pc44800@gmail.com","subject":"Re: [PATCH v6 2/3] submodule--helper: introduce for_each_listed_submodule()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T00:55:47Z","receivedAt":"2017-10-02T00:55:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> Introduce function for_each_listed_submodule() and replace a loop\n> in module_init() with a call to it.\n>\n> The new function will also be used in other parts of the\n> system in later patches.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> builtin/submodule--helper.c | 39 ++++++++++++++++++++++++++++++++++-----\n> 1 file changed, 34 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index cdae54426..20a1ef868 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -14,6 +14,11 @@\n> #include \"refs.h\"\n> #include \"connect.h\"\n> \n> +#define CB_OPT_QUIET\t\t(1<<0)\n\nIs the purpose of this bit to make the callback quiet? I do not\nthink so. Is there a reason why we cannot call it just OPT_QUIET or\nsomething instead?\n\nWhen the set of functions that pay attention to these flags include\nboth ones that are callable for a single submodule and ones meant as\ncallbacks for for-each interface, having to flip bit whose name\nscreams \"CallBack!\" in a caller of a single-short version feels very\nwrong.\n\n\"make style\" tells me to format the above like so:\n\n\t#define OPT_QUIET (1 << 0)\n\nand I think I agree.\n\n> @@ -349,7 +354,22 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> -static void init_submodule(const char *path, const char *prefix, int quiet)\n> +static void for_each_listed_submodule(const struct module_list *list,\n> +\t\t\t\t each_submodule_fn fn, void *cb_data)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < list->nr; i++)\n> +\t\tfn(list->entries[i], cb_data);\n> +}\n\nGood.\n\n> +struct init_cb {\n\nI take it is a short-hand for \"submodule init callback\"? As long as\nthe name stays inside this file, I think we are OK.\n\n> +\tconst char *prefix;\n> +\tunsigned int cb_flags;\n\nCall this just \"flags\"; call-back ness is plenty clear from the fact\nthat it lives in a structure meant as a callback interface already.\n\n> +};\n\nBlank line here?\n\n> +#define INIT_CB_INIT { NULL, 0 }\n> +\n> +static void init_submodule(const char *path, const char *prefix,\n> +\t\t\t unsigned int cb_flags)\n\nCall this also \"flags\"; a direct caller of this function that wants\nto initialize a single submodule without going thru the for-each\ncallback interface would not be passing \"callback flags\"--they are\njust passing a set of flags.\n"},{"id":"329367","messageId":"xmqqy3ouqruh.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20170929094453.4499-4-pc44800@gmail.com","subject":"Re: [PATCH v6 3/3] submodule: port submodule subcommand 'status' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T01:08:22Z","receivedAt":"2017-10-02T01:08:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> \n> #define CB_OPT_QUIET\t\t(1<<0)\n> +#define CB_OPT_CACHED\t\t(1<<1)\n> +#define CB_OPT_RECURSIVE\t(1<<2)\n\nSame comments on both naming and formatting.\n\n> @@ -245,6 +250,53 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n> \t}\n> }\n> \n> +static char *compute_rev_name(const char *sub_path, const char* object_id)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char ***d;\n> +\n> +\tstatic const char *describe_bare[] = {\n> +\t\tNULL\n> +\t};\n> +\n> +\tstatic const char *describe_tags[] = {\n> +\t\t\"--tags\", NULL\n> +\t};\n> +\n> +\tstatic const char *describe_contains[] = {\n> +\t\t\"--contains\", NULL\n> +\t};\n> +\n> +\tstatic const char *describe_all_always[] = {\n> +\t\t\"--all\", \"--always\", NULL\n> +\t};\n> +\n> +\tstatic const char **describe_argv[] = {\n> +\t\tdescribe_bare, describe_tags, describe_contains,\n> +\t\tdescribe_all_always, NULL\n> +\t};\n\n\"make style\" seems to suggest a lot more compact version to be used\nfor the above, and I tend to agree with its diagnosis.\n\n> @@ -503,6 +555,160 @@ static int module_init(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +struct status_cb {\n> +\tconst char *prefix;\n> +\tunsigned int cb_flags;\n> +};\n> +#define STATUS_CB_INIT { NULL, 0 }\n\nSame three comments as the previous \"init_cb\" patch apply.\n\n> +\targv_array_pushl(&diff_files_args, \"diff-files\",\n> +\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n> +\t\t\t path, NULL);\n> +\n> +\tgit_config(git_diff_basic_config, NULL);\n\nShould this be called every time? The config file is not changing,\nno?\n\n> +\tinit_revisions(&rev, prefix);\n> +\trev.abbrev = 0;\n\nThis part looks OK.\n\n> +\tprecompose_argv(diff_files_args.argc, diff_files_args.argv);\n\nI do not think this is correct. We certainly did not get the path\nargument (i.e. args.argv) from the command line of macOS X box and\nthe correction for UTF-8 canonicalization should not be necessary.\nEven if we did get path from the command line, I think the UTF-8\ncorrection should have been done for us for any command (like \"git\nsubmodule--helper\") that uses parse-optoins API already.\n\nJust dropping the line should be sufficient to correct this, I think.\n\nThe remainder of the patch looked more-or-less OK, but I'd revisit\nit later to make sure.\n\nThanks.\n"},{"id":"329881","messageId":"20171006132415.2876-1-pc44800@gmail.com","threadId":"46637","inReplyTo":"xmqqy3ouqruh.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v7 0/3] Incremental rewrite of git-submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-10-06T13:24:12Z","receivedAt":"2017-10-06T13:24:32Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Changes in v7:\n\n* Instead of using cb_flags in the callback data's struct, 'flags' is used.\n\n* Similar changes were applied to the CB_OPT_QUIET and other bits.\n\n* The function compute_rev_name() was formatted in accordance with the \"make\n style\", into a compact version.\n\n* Call to precompose_argv() in the function status_submodule() was dropped\n as the call was unnecessary.\n\nAs before you can find this series at: \nhttps://github.com/pratham-pc/git/commits/patch-series-1\n\nAnd its build report is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: patch-series-1\nBuild #190\n\nThe above changes were based on master branch.\n\nAnother branch, similar to the above, was created, but was based\non the 'next' branch.\nComplete build report of that is also available at:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: patch-series-1-next\nBuild #189\nThe above changes are also push on github and are available at:\nhttps://github.com/pratham-pc/git/commits/patch-series-1-next\n\nPrathamesh Chavan (3):\n submodule--helper: introduce get_submodule_displaypath()\n submodule--helper: introduce for_each_listed_submodule()\n submodule: port submodule subcommand 'status' from shell to C\n\n builtin/submodule--helper.c | 273 +++++++++++++++++++++++++++++++++++++++++---\n git-submodule.sh | 61 +---------\n 2 files changed, 257 insertions(+), 77 deletions(-)\n\n-- \n2.14.2\n\n"},{"id":"329882","messageId":"20171006132415.2876-2-pc44800@gmail.com","threadId":"46637","inReplyTo":"20171006132415.2876-1-pc44800@gmail.com","subject":"[PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-10-06T13:24:13Z","receivedAt":"2017-10-06T13:24:35Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function get_submodule_displaypath() to replace the code\noccurring in submodule_init() for generating displaypath of the\nsubmodule with a call to it.\n\nThis new function will also be used in other parts of the system\nin later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 06ed02f99..56c1c52e2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+/* the result should be freed by the caller. */\n+static char *get_submodule_displaypath(const char *path, const char *prefix)\n+{\n+\tconst char *super_prefix = get_super_prefix();\n+\n+\tif (prefix && super_prefix) {\n+\t\tBUG(\"cannot have prefix '%s' and superprefix '%s'\",\n+\t\t prefix, super_prefix);\n+\t} else if (prefix) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tchar *displaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t\tstrbuf_release(&sb);\n+\t\treturn displaypath;\n+\t} else if (super_prefix) {\n+\t\treturn xstrfmt(\"%s%s\", super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *upd = NULL, *url = NULL, *displaypath;\n \n-\tif (prefix && get_super_prefix())\n-\t\tdie(\"BUG: cannot have prefix and superprefix\");\n-\telse if (prefix)\n-\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n-\telse if (get_super_prefix()) {\n-\t\tstrbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n-\t\tdisplaypath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tdisplaypath = xstrdup(path);\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n \n \tsub = submodule_from_path(&null_oid, path);\n \n@@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * Set active flag for the submodule being initialized\n \t */\n \tif (!is_submodule_active(the_repository, path)) {\n-\t\tstrbuf_reset(&sb);\n \t\tstrbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n \t\tgit_config_set_gently(sb.buf, \"true\");\n+\t\tstrbuf_reset(&sb);\n \t}\n \n \t/*\n@@ -367,7 +379,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t * To look up the url in .git/config, we must not fall back to\n \t * .gitmodules, so look it up directly.\n \t */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n \tif (git_config_get_string(sb.buf, &url)) {\n \t\tif (!sub->url)\n@@ -404,9 +415,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n \t}\n+\tstrbuf_reset(&sb);\n \n \t/* Copy \"update\" setting when it is not set yet */\n-\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n \tif (git_config_get_string(sb.buf, &upd) &&\n \t sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n-- \n2.14.2\n\n"},{"id":"329883","messageId":"20171006132415.2876-3-pc44800@gmail.com","threadId":"46637","inReplyTo":"20171006132415.2876-1-pc44800@gmail.com","subject":"[PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-10-06T13:24:14Z","receivedAt":"2017-10-06T13:24:38Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Introduce function for_each_listed_submodule() and replace a loop\nin module_init() with a call to it.\n\nThe new function will also be used in other parts of the\nsystem in later patches.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 40 +++++++++++++++++++++++++++++++++++-----\n 1 file changed, 35 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 56c1c52e2..29e3fde16 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -14,6 +14,11 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+#define OPT_QUIET (1 << 0)\n+\n+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static void init_submodule(const char *path, const char *prefix, int quiet)\n+static void for_each_listed_submodule(const struct module_list *list,\n+\t\t\t\t each_submodule_fn fn, void *cb_data)\n+{\n+\tint i;\n+\tfor (i = 0; i < list->nr; i++)\n+\t\tfn(list->entries[i], cb_data);\n+}\n+\n+struct init_cb {\n+\tconst char *prefix;\n+\tunsigned int flags;\n+};\n+\n+#define INIT_CB_INIT { NULL, 0 }\n+\n+static void init_submodule(const char *path, const char *prefix,\n+\t\t\t unsigned int flags)\n {\n \tconst struct submodule *sub;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \t\tif (git_config_set_gently(sb.buf, url))\n \t\t\tdie(_(\"Failed to register url for submodule path '%s'\"),\n \t\t\t displaypath);\n-\t\tif (!quiet)\n+\t\tif (!(flags & OPT_QUIET))\n \t\t\tfprintf(stderr,\n \t\t\t\t_(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n \t\t\t\tsub->name, url, displaypath);\n@@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n \tfree(upd);\n }\n \n+static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct init_cb *info = cb_data;\n+\tinit_submodule(list_item->name, info->prefix, info->flags);\n+}\n+\n static int module_init(int argc, const char **argv, const char *prefix)\n {\n+\tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \tint quiet = 0;\n-\tint i;\n \n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n@@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n \t\tmodule_list_active(&list);\n \n-\tfor (i = 0; i < list.nr; i++)\n-\t\tinit_submodule(list.entries[i]->name, prefix, quiet);\n+\tinfo.prefix = prefix;\n+\tif (quiet)\n+\t\tinfo.flags |= OPT_QUIET;\n+\n+\tfor_each_listed_submodule(&list, init_submodule_cb, &info);\n \n \treturn 0;\n }\n-- \n2.14.2\n\n"},{"id":"329884","messageId":"20171006132415.2876-4-pc44800@gmail.com","threadId":"46637","inReplyTo":"20171006132415.2876-1-pc44800@gmail.com","subject":"[PATCH v7 3/3] submodule: port submodule subcommand 'status' from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-10-06T13:24:15Z","receivedAt":"2017-10-06T13:24:42Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule 'status' a built-in. Hence, the function\ncmd_status() is ported from shell to C. This is done by introducing\nfour functions: module_status(), submodule_status_cb(),\nsubmodule_status() and print_status().\n\nThe function module_status() acts as the front-end of the subcommand.\nIt parses subcommand's options and then calls the function\nmodule_list_compute() for computing the list of submodules. Then\nthis functions calls for_each_listed_submodule() looping through the\nlist obtained.\n\nThen for_each_listed_submodule() calls submodule_status_cb() for each of\nthe submodule in its list. The function submodule_status_cb() calls\nsubmodule_status() after passing appropriate arguments to the funciton.\nFunction submodule_status() is responsible for generating the status\neach submodule it is called for, and then calls print_status().\n\nFinally, the function print_status() handles the printing of submodule's\nstatus.\n\nFunction set_name_rev() is also ported from git-submodule to the\nsubmodule--helper builtin function compute_rev_name(), which now\ngenerates the value of the revision name as required.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n builtin/submodule--helper.c | 198 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 61 +-------------\n 2 files changed, 199 insertions(+), 60 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 29e3fde16..d366e8e7b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -13,8 +13,13 @@\n #include \"remote.h\"\n #include \"refs.h\"\n #include \"connect.h\"\n+#include \"revision.h\"\n+#include \"diffcore.h\"\n+#include \"diff.h\"\n \n #define OPT_QUIET (1 << 0)\n+#define OPT_CACHED (1 << 1)\n+#define OPT_RECURSIVE (1 << 2)\n \n typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \t\t\t\t void *cb_data);\n@@ -244,6 +249,44 @@ static char *get_submodule_displaypath(const char *path, const char *prefix)\n \t}\n }\n \n+static char *compute_rev_name(const char *sub_path, const char* object_id)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tconst char ***d;\n+\n+\tstatic const char *describe_bare[] = { NULL };\n+\n+\tstatic const char *describe_tags[] = { \"--tags\", NULL };\n+\n+\tstatic const char *describe_contains[] = { \"--contains\", NULL };\n+\n+\tstatic const char *describe_all_always[] = { \"--all\", \"--always\", NULL };\n+\n+\tstatic const char **describe_argv[] = { describe_bare, describe_tags,\n+\t\t\t\t\t\tdescribe_contains,\n+\t\t\t\t\t\tdescribe_all_always, NULL };\n+\n+\tfor (d = describe_argv; *d; d++) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = sub_path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stderr = 1;\n+\n+\t\targv_array_push(&cp.args, \"describe\");\n+\t\targv_array_pushv(&cp.args, *d);\n+\t\targv_array_push(&cp.args, object_id);\n+\n+\t\tif (!capture_command(&cp, &sb, 0)) {\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\treturn strbuf_detach(&sb, NULL);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb);\n+\treturn NULL;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -503,6 +546,160 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct status_cb {\n+\tconst char *prefix;\n+\tunsigned int flags;\n+};\n+\n+#define STATUS_CB_INIT { NULL, 0 }\n+\n+static void print_status(unsigned int flags, char state, const char *path,\n+\t\t\t const struct object_id *oid, const char *displaypath)\n+{\n+\tif (flags & OPT_QUIET)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, oid_to_hex(oid), displaypath);\n+\n+\tif (state == ' ' || state == '+')\n+\t\tprintf(\" (%s)\", compute_rev_name(path, oid_to_hex(oid)));\n+\n+\tprintf(\"\\n\");\n+}\n+\n+static int handle_submodule_head_ref(const char *refname,\n+\t\t\t\t const struct object_id *oid, int flags,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct object_id *output = cb_data;\n+\tif (oid)\n+\t\toidcpy(output, oid);\n+\n+\treturn 0;\n+}\n+\n+static void status_submodule(const char *path, const struct object_id *ce_oid,\n+\t\t\t unsigned int ce_flags, const char *prefix,\n+\t\t\t unsigned int flags)\n+{\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\tstruct rev_info rev;\n+\tint diff_files_result;\n+\n+\tif (!submodule_from_path(&null_oid, path))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t path);\n+\n+\tdisplaypath = get_submodule_displaypath(path, prefix);\n+\n+\tif ((CE_STAGEMASK & ce_flags) >> CE_STAGESHIFT) {\n+\t\tprint_status(flags, 'U', path, &null_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_active(the_repository, path)) {\n+\t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\targv_array_pushl(&diff_files_args, \"diff-files\",\n+\t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n+\t\t\t path, NULL);\n+\n+\tgit_config(git_diff_basic_config, NULL);\n+\tinit_revisions(&rev, prefix);\n+\trev.abbrev = 0;\n+\tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n+\t\t\t\t\t diff_files_args.argv,\n+\t\t\t\t\t &rev, NULL);\n+\tdiff_files_result = run_diff_files(&rev, 0);\n+\n+\tif (!diff_result_code(&rev.diffopt, diff_files_result)) {\n+\t\tprint_status(flags, ' ', path, ce_oid,\n+\t\t\t displaypath);\n+\t} else if (!(flags & OPT_CACHED)) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (refs_head_ref(get_submodule_ref_store(path),\n+\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\t\tdie(_(\"could not resolve HEAD ref inside the\"\n+\t\t\t \"submodule '%s'\"), path);\n+\n+\t\tprint_status(flags, '+', path, &oid, displaypath);\n+\t} else {\n+\t\tprint_status(flags, '+', path, ce_oid, displaypath);\n+\t}\n+\n+\tif (flags & OPT_RECURSIVE) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.git_cmd = 1;\n+\t\tcpr.dir = path;\n+\t\tprepare_submodule_repo_env(&cpr.env_array);\n+\n+\t\targv_array_push(&cpr.args, \"--super-prefix\");\n+\t\targv_array_pushf(&cpr.args, \"%s/\", displaypath);\n+\t\targv_array_pushl(&cpr.args, \"submodule--helper\", \"status\",\n+\t\t\t\t \"--recursive\", NULL);\n+\n+\t\tif (flags & OPT_CACHED)\n+\t\t\targv_array_push(&cpr.args, \"--cached\");\n+\n+\t\tif (flags & OPT_QUIET)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"failed to recurse into submodule '%s'\"), path);\n+\t}\n+\n+cleanup:\n+\targv_array_clear(&diff_files_args);\n+\tfree(displaypath);\n+}\n+\n+static void status_submodule_cb(const struct cache_entry *list_item,\n+\t\t\t\tvoid *cb_data)\n+{\n+\tstruct status_cb *info = cb_data;\n+\tstatus_submodule(list_item->name, &list_item->oid, list_item->ce_flags,\n+\t\t\t info->prefix, info->flags);\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct status_cb info = STATUS_CB_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\n+\tstruct option module_status_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress submodule status output\")),\n+\t\tOPT_BIT(0, \"cached\", &info.flags, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\"), OPT_CACHED),\n+\t\tOPT_BIT(0, \"recursive\", &info.flags, N_(\"recurse into nested submodules\"), OPT_RECURSIVE),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule status [--quiet] [--cached] [--recursive] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_status_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tinfo.prefix = prefix;\n+\tif (quiet)\n+\t\tinfo.flags |= OPT_QUIET;\n+\n+\tfor_each_listed_submodule(&list, status_submodule_cb, &info);\n+\n+\treturn 0;\n+}\n+\n static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n@@ -1300,6 +1497,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n+\t{\"status\", module_status, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 66d1ae8ef..156255a9e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -758,18 +758,6 @@ cmd_update()\n \t}\n }\n \n-set_name_rev () {\n-\trevname=$( (\n-\t\tsanitize_submodule_env\n-\t\tcd \"$1\" && {\n-\t\t\tgit describe \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --tags \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --contains \"$2\" 2>/dev/null ||\n-\t\t\tgit describe --all --always \"$2\"\n-\t\t}\n-\t) )\n-\ttest -z \"$revname\" || revname=\" ($revname)\"\n-}\n #\n # Show commit summary for submodules in index or working tree\n #\n@@ -1016,54 +1004,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read -r mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n-\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\tif test \"$stage\" = U\n-\t\tthen\n-\t\t\tsay \"U$sha1 $displaypath\"\n-\t\t\tcontinue\n-\t\tfi\n-\t\tif ! git submodule--helper is-active \"$sm_path\" ||\n-\t\t{\n-\t\t\t! test -d \"$sm_path\"/.git &&\n-\t\t\t! test -f \"$sm_path\"/.git\n-\t\t}\n-\t\tthen\n-\t\t\tsay \"-$sha1 $displaypath\"\n-\t\t\tcontinue;\n-\t\tfi\n-\t\tif git diff-files --ignore-submodules=dirty --quiet -- \"$sm_path\"\n-\t\tthen\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n-\t\t\tsay \" $sha1 $displaypath$revname\"\n-\t\telse\n-\t\t\tif test -z \"$cached\"\n-\t\t\tthen\n-\t\t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" && git rev-parse --verify HEAD)\n-\t\t\tfi\n-\t\t\tset_name_rev \"$sm_path\" \"$sha1\"\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n-\n-\t\tif test -n \"$recursive\"\n-\t\tthen\n-\t\t\t(\n-\t\t\t\tprefix=\"$displaypath/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\twt_prefix=\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\teval cmd_status\n-\t\t\t) ||\n-\t\t\tdie \"$(eval_gettext \"Failed to recurse into submodule path '\\$sm_path'\")\"\n-\t\tfi\n-\tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n }\n #\n # Sync remote urls for submodules\n-- \n2.14.2\n\n"},{"id":"329938","messageId":"CAPig+cTEMH=RVfqekuP-oWOoRmNWEdvdFZz4bOdS321oND1Ypg@mail.gmail.com","threadId":"46637","inReplyTo":"20171006132415.2876-2-pc44800@gmail.com","subject":"Re: [PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-06T21:12:52Z","receivedAt":"2017-10-06T21:12:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"I didn't find a URL in the cover letter pointing at previous\niterations of this patch series and related discussions, so forgive me\nif comments below merely repeat what was said earlier...\n\nOn Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan wrote:\n> Introduce function get_submodule_displaypath() to replace the code\n> occurring in submodule_init() for generating displaypath of the\n> submodule with a call to it.\n>\n> This new function will also be used in other parts of the system\n> in later patches.\n>\n> Signed-off-by: Prathamesh Chavan \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n> +/* the result should be freed by the caller. */\n> +static char *get_submodule_displaypath(const char *path, const char *prefix)\n> +{\n> + const char *super_prefix = get_super_prefix();\n> +\n> + if (prefix && super_prefix) {\n> + BUG(\"cannot have prefix '%s' and superprefix '%s'\",\n> + prefix, super_prefix);\n> + } else if (prefix) {\n> + struct strbuf sb = STRBUF_INIT;\n> + char *displaypath = xstrdup(relative_path(path, prefix, &sb));\n> + strbuf_release(&sb);\n> + return displaypath;\n> + } else if (super_prefix) {\n> + return xstrfmt(\"%s%s\", super_prefix, path);\n> + } else {\n> + return xstrdup(path);\n> + }\n> +}\n\nAt first glance, this appears to be a simple code-movement patch which\nshouldn't require deep inspection by a reviewer, however, upon closer\nexamination, it turns out that it is doing rather more than that,\nwhich increases reviewer burden, especially since these additional\nchanges are not mentioned in the commit message. At a minimum, it\nincludes these changes:\n\n* factors out calls to get_super_prefix()\n* adds extra context to the \"BUG\" message\n* changes die(\"BUG...\") to BUG(...)\n* allocates/releases a strbuf\n* changes assignments to returns\n\nThe final two are obvious necessary (or clarifying) changes which a\nreviewer would expect to see in a patch which factors code out to its\nown function; the others not so.\n\nThis isn't to say that the other changes are not reasonable -- they\nare -- but if one of your goals is to make the patches easy for\nreviewers to digest, then you should make the changes as obvious as\npossible for reviewers to spot. One way would be to mention in the\ncommit message that you're taking the opportunity to also make these\nparticular cleanups to the code. A more common approach is to place\nthe various cleanups in preparatory patches before this one, with one\ncleanup per patch. I'd prefer to see the latter (if my opinion carries\nany weight).\n\nMore below...\n\n> @@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> struct strbuf sb = STRBUF_INIT;\n> char *upd = NULL, *url = NULL, *displaypath;\n>\n> - if (prefix && get_super_prefix())\n> - die(\"BUG: cannot have prefix and superprefix\");\n> - else if (prefix)\n> - displaypath = xstrdup(relative_path(path, prefix, &sb));\n> - else if (get_super_prefix()) {\n> - strbuf_addf(&sb, \"%s%s\", get_super_prefix(), path);\n> - displaypath = strbuf_detach(&sb, NULL);\n> - } else\n> - displaypath = xstrdup(path);\n> + displaypath = get_submodule_displaypath(path, prefix);\n>\n> sub = submodule_from_path(&null_oid, path);\n>\n> @@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> * Set active flag for the submodule being initialized\n> */\n> if (!is_submodule_active(the_repository, path)) {\n> - strbuf_reset(&sb);\n> strbuf_addf(&sb, \"submodule.%s.active\", sub->name);\n> git_config_set_gently(sb.buf, \"true\");\n> + strbuf_reset(&sb);\n\nThis strbuf_reset() movement, and those below, are pretty much just\n\"noise\" changes. They add extra burden to the review process without\nreally improving the code. The reason they add to reviewer burden is\nthat they do not seem to be related to the intention stated in the\ncommit message, so the reviewer must spend extra time trying to\nunderstand their purpose and correctness.\n\nMore serious, though, is that these strbuf_reset() movements may\nactually increase the burden on someone changing the code in the\nfuture. Presumably, your reason for making these changes is that you\nreviewed the code after factoring out the get_submodule_displaypath()\nlogic and discovered that the strbuf was no longer touched before this\npoint, therefore resetting it before strbuf_addf() is unnecessary.\nWhile this may be true today, it may not be so in the future. If\nsomeone comes along and adds code above this point which does touch\nthe strbuf, then these code movements either need to be reverted by\nthat person (more noise) or that person needs to remember to add a\nstrbuf_reset() at the end of the new code.\n\nMoreover, it's somewhat easier to reason about the strbuf_reset()'s\nand the corresponding strbuf_addf()'s when they are kept together, as\nin the original code, so, for that reason alone, one could argue that\nmoving the strbuf_reset()'s does not really improve the code.\n\nI'd suggest dropping these changes in the re-roll.\n\n> }\n>\n> /*\n> @@ -367,7 +379,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> * To look up the url in .git/config, we must not fall back to\n> * .gitmodules, so look it up directly.\n> */\n> - strbuf_reset(&sb);\n> strbuf_addf(&sb, \"submodule.%s.url\", sub->name);\n> if (git_config_get_string(sb.buf, &url)) {\n> if (!sub->url)\n> @@ -404,9 +415,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> _(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n> sub->name, url, displaypath);\n> }\n> + strbuf_reset(&sb);\n>\n> /* Copy \"update\" setting when it is not set yet */\n> - strbuf_reset(&sb);\n> strbuf_addf(&sb, \"submodule.%s.update\", sub->name);\n> if (git_config_get_string(sb.buf, &upd) &&\n> sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {\n> --\n> 2.14.2\n"},{"id":"329939","messageId":"CAPig+cT31XM9nW7sytukbQQ_O_15np6oepazKJaoNuHey+kiBA@mail.gmail.com","threadId":"46637","inReplyTo":"20171006132415.2876-3-pc44800@gmail.com","subject":"Re: [PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-06T21:56:04Z","receivedAt":"2017-10-06T21:56:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Same disclaimer as in my review of patch 1/3: I didn't see a URL in\nthe cover letter pointing at discussions of earlier iterations, so\nbelow comments may be at odds with what went on previously...\n\nOn Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan wrote:\n> Introduce function for_each_listed_submodule() and replace a loop\n> in module_init() with a call to it.\n>\n> The new function will also be used in other parts of the\n> system in later patches.\n>\n> Signed-off-by: Prathamesh Chavan \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -14,6 +14,11 @@\n> #include \"refs.h\"\n> #include \"connect.h\"\n>\n> +#define OPT_QUIET (1 << 0)\n> +\n> +typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n> + void *cb_data);\n\nWhat is the reason for having the definition of 'each_submodule_fn' so\nfar removed textually from its first reference by\nfor_each_listed_submodule() below?\n\n> static char *get_default_remote(void)\n> {\n> char *dest = NULL, *ret;\n> @@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> return 0;\n> }\n>\n> -static void init_submodule(const char *path, const char *prefix, int quiet)\n> +static void for_each_listed_submodule(const struct module_list *list,\n> + each_submodule_fn fn, void *cb_data)\n> +{\n> + int i;\n> + for (i = 0; i < list->nr; i++)\n> + fn(list->entries[i], cb_data);\n> +}\n\nI'm very curious about the justification for introducing a for-each\nfunction for what amounts to the simplest sort of loop possible: a\ncanonical for-loop with a one-line body. I could easily understand the\ndesire for such a function if either the loop conditions or the body\nof the loop, or both, were complex, but this does not seem to be the\ncase. Even the callers of this new function, in this patch and in 3/3,\nare as simple as possible: one-liners (simple function calls).\n\nAlthough this sort of for-each function can, at times, be helpful,\nthere are costs: extra boilerplate and increased complexity for\nclients since it requires callback functions and (optionally) callback\ndata. The separation of logic into a callback function can make code\nmore difficult to reason about than when it is simply the body of a\nfor-loop.\n\nSo, unless the plan for the future is that this for-each function will\nhave considerable additional functionality baked into it, I'm having a\ndifficult time understanding why this change is desirable.\n\n> +struct init_cb {\n> + const char *prefix;\n> + unsigned int flags;\n> +};\n> +\n> +#define INIT_CB_INIT { NULL, 0 }\n\nWhy are these definitions so far removed from init_submodule_cb() below?\n\n> +static void init_submodule(const char *path, const char *prefix,\n> + unsigned int flags)\n> {\n> const struct submodule *sub;\n> struct strbuf sb = STRBUF_INIT;\n> @@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> if (git_config_set_gently(sb.buf, url))\n> die(_(\"Failed to register url for submodule path '%s'\"),\n> displaypath);\n> - if (!quiet)\n> + if (!(flags & OPT_QUIET))\n\nThis change of having init_submodule() accept a 'flags' argument,\nrather than a single boolean, increases reviewer burden, since the\nreviewer is forced to puzzle out how this change relates to the stated\nintention of the patch since it is not mentioned at all by the commit\nmessage.\n\nIt's also conceptually unrelated to the introduction of a for-each\nfunction, thus should be instead be done by a separate preparatory\npatch.\n\n> fprintf(stderr,\n> _(\"Submodule '%s' (%s) registered for path '%s'\\n\"),\n> sub->name, url, displaypath);\n> @@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char *prefix, int quiet)\n> free(upd);\n> }\n>\n> +static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)\n> +{\n> + struct init_cb *info = cb_data;\n> + init_submodule(list_item->name, info->prefix, info->flags);\n> +}\n> +\n> static int module_init(int argc, const char **argv, const char *prefix)\n> {\n> + struct init_cb info = INIT_CB_INIT;\n> struct pathspec pathspec;\n> struct module_list list = MODULE_LIST_INIT;\n> int quiet = 0;\n> - int i;\n>\n> struct option module_init_options[] = {\n> OPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n> @@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, const char *prefix)\n> if (!argc && git_config_get_value_multi(\"submodule.active\"))\n> module_list_active(&list);\n>\n> - for (i = 0; i < list.nr; i++)\n> - init_submodule(list.entries[i]->name, prefix, quiet);\n> + info.prefix = prefix;\n> + if (quiet)\n> + info.flags |= OPT_QUIET;\n> +\n> + for_each_listed_submodule(&list, init_submodule_cb, &info);\n>\n> return 0;\n> }\n> --\n> 2.14.2\n"},{"id":"329960","messageId":"xmqqshevtk77.fsf@gitster.mtv.corp.google.com","threadId":"46637","inReplyTo":"20171006132415.2876-1-pc44800@gmail.com","subject":"Re: [PATCH v7 0/3] Incremental rewrite of git-submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T08:51:08Z","receivedAt":"2017-10-07T08:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Prathamesh Chavan writes:\n\n> Changes in v7:\n\nFWIW, the previous one is at\n\n https://public-inbox.org/git/20170929094453.4499-1-pc44800@gmail.com\n\nAlternatively, the References link can be followed back from the\ncover letter to go back to quite an early iteration of the series.\n\nHope that helps ;-)\n\n\n\n"},{"id":"329961","messageId":"CAPig+cSCRyfs5_6kbcy87rAVBZwg0zYs3JGkzxAO5MMmznfEpg@mail.gmail.com","threadId":"46637","inReplyTo":"xmqqshevtk77.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v7 0/3] Incremental rewrite of git-submodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-07T09:35:33Z","receivedAt":"2017-10-07T09:35:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 7, 2017 at 4:51 AM, Junio C Hamano wrote:\n> FWIW, the previous one is at\n> https://public-inbox.org/git/20170929094453.4499-1-pc44800@gmail.com\n> Hope that helps ;-)\n\nThanks, it does help.\n\nHaving scanned discussions of previous versions, I see that some of my\ncomments do indeed overlap (and sometimes are at odds) with comments\nfrom other reviewers.\n"}]}