{"thread":{"id":"46015","subject":"[GSoC][PATCH v1 1/2] submodule: port set_name_rev from shell to C","startedAt":"2017-05-21T12:27:32Z","lastAt":"2017-06-06T04:25:04Z","messageCount":9,"participants":["Prathamesh Chavan","Stefan Beller","Brandon Williams","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"320371","messageId":"20170521122711.22021-1-pc44800@gmail.com","threadId":"46015","inReplyTo":null,"subject":"[GSoC][PATCH v1 1/2] submodule: port set_name_rev from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-21T12:27:10Z","receivedAt":"2017-05-21T12:27:32Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Since later on we want to port submodule subcommand status, and since\nset_name_rev is part of cmd_status, hence this function is ported. It\nhas been ported to function set_name_rev in C, which calls get_name_rev\nto get the revname, and after formatting it, set_name_prints it. And\nhence in this way, the command `git submodule--helper set-name-rev\n\"sm_path\" \"sha1\"` sets value of revname in git-submodule.sh\n\nThe function get_name_rev returns the stdout of the git describe\ncommands. Since there are four different git-describe commands used for\ngenerating the name rev, four child_process are introduced, each successive\nchild process running only when previous has no stdout. The order of these\nfour git-describe commands is maintained the same as it was in the function\nset_name_rev() before porting.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\nThis series of patch is based on gitster/jk/bug-to-abort for untilizing its\nBUG() macro.\n\nSince submodule subcommand used set_name_rev function, first this function\nwas ported before porting the subcommand to C.\n\nComplete build report for this patch is available at:\nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: status\nBuild #64\n\nAlso, I have updated my Github and pushed this work. It can be seen at:\nhttps://github.com/pratham-pc/git/commits/status\n\n builtin/submodule--helper.c | 65 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 16 ++---------\n 2 files changed, 67 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..5f0ddd8ad 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -219,6 +219,70 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+enum describe_step {\n+\tstep_bare = 0,\n+\tstep_tags,\n+\tstep_contains,\n+\tstep_all_always,\n+\tstep_end\n+};\n+\n+static char *get_name_rev(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct child_process cp;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tenum describe_step cur_step;\n+\n+\tfor (cur_step = step_bare; cur_step < step_end; cur_step++) {\n+\t\tchild_process_init(&cp);\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = argv[1];\n+\t\tcp.no_stderr = 1;\n+\n+\t\tswitch (cur_step) {\n+\t\t\tcase step_bare:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\", argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_tags:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\", \"--tags\",\n+\t\t\t\t\t\t argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_contains:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n+\t\t\t\t\t\t \"--contains\", argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_all_always:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\", \"--all\",\n+\t\t\t\t\t\t \"--always\", argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tBUG(\"unknown describe step '%d'\", cur_step);\n+\t\t}\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+\n+static int set_name_rev(int argc, const char **argv, const char *prefix)\n+{\n+\tchar *namerev;\n+\tif (argc != 3)\n+\t\tdie(\"set-name-rev only accepts two arguments: <path> <sha1>\");\n+\n+\tnamerev = get_name_rev(argc, argv, prefix);\n+\tif (namerev[0])\n+\t\tprintf(\" (%s)\\n\", namerev);\n+\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1212,6 +1276,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{\"set-name-rev\", set_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 c0d0e9a4c..b6eb5bcce 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 set-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 set-name-rev \"$sm_path\" \"$sha1\")\n \t\t\tsay \"+$sha1 $displaypath$revname\"\n \t\tfi\n \n-- \n2.11.0\n\n"},{"id":"320372","messageId":"20170521122711.22021-2-pc44800@gmail.com","threadId":"46015","inReplyTo":"20170521122711.22021-1-pc44800@gmail.com","subject":"[GSoC][PATCH v1 2/2] submodule: port submodule subcommand status","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-21T12:27:11Z","receivedAt":"2017-05-21T12:27:36Z","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 builtin. 'status' is ported\nto submodule--helper, and submodule--helper is called from\ngit-submodule.sh.\n\nFor the purpose of porting cmd_status, the code is split up such that\none function obtains all the list of submodules, acting as the\nfront-end of git-submodule status. This function later calls the\nsecond function for_each_submodule_list,it which basically loops\nthrough the list of submodules and calls function fn, which in this\ncase is status_submodule. The third function, status submodule returns\nthe status of submodule and also takes care of the recursive flag.\n\nThe first function module_status parses the options present in argv,\nand then with the help of module_list_compute, generates the list of\nsubmodules present in the current working tree.\n\nThe second function for_each_submodule_list traverses through the list,\nand calls function fn (which in the case of submodule subcommand\nforeach is status_submodule) is called for each entry.\n\nThe third function status_foreach checks for the various conditions,\nand prints the status of the submodule accordingly. Also, this\nfunction takes care of the recursive flag by creating a separate\nchild_process and running it inside the submodule.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\nA new function, get_submodule_displaypath is also introduced for getting\nthe displaypath of the submodule while taking care of its prefix and\nsuperprefix.\n\n builtin/submodule--helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  48 +------------\n 2 files changed, 163 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5f0ddd8ad..7c040a375 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -13,6 +13,8 @@\n #include \"refs.h\"\n #include \"connect.h\"\n \n+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -219,6 +221,23 @@ 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\treturn xstrdup(relative_path(path, prefix, &sb));\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 enum describe_step {\n \tstep_bare = 0,\n \tstep_tags,\n@@ -395,6 +414,13 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void for_each_submodule_list(const struct module_list list, 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 static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -532,6 +558,141 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cb_status {\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int cached: 1;\n+\tunsigned int recursive: 1;\n+};\n+#define CB_STATUS_INIT { NULL, 0, 0, 0 }\n+\n+static void status_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct cb_status *info = cb_data;\n+\tstruct strbuf sub_sha1 = STRBUF_INIT;\n+\tchar* displaypath = NULL;\n+\tstruct argv_array diff_files_arg = ARGV_ARRAY_INIT;\n+\n+\targv_array_pushl(&diff_files_arg, \"diff-files\", \"--ignore-submodules=dirty\",\n+\t\t\t\"--quiet\", \"--\", list_item->name, NULL);\n+\n+\tgitmodules_config();\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (!submodule_from_path(null_sha1, list_item->name))\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"), list_item->name);\n+\n+\tstrbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));\n+\n+\tif (list_item->ce_flags) {\n+\t\tprintf(_(\"U%s %s\\n\"), sha1_to_hex(null_sha1), displaypath);\n+\t\treturn;\n+\t}\n+\n+\tif (!is_submodule_initialized(list_item->name)) {\n+\t\tprintf(_(\"-%s %s\\n\"), sub_sha1.buf, displaypath);\n+\t\treturn;\n+\t}\n+\n+\tif (!cmd_diff_files(diff_files_arg.argc, diff_files_arg.argv , info->prefix)) {\n+\t\tstruct argv_array name_rev_args = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&name_rev_args, \"set-name-rev\", list_item->name,\n+\t\t\t\tsub_sha1.buf, NULL);\n+\t\tprintf(_(\" %s %s\"), sub_sha1.buf, displaypath);\n+\t\tset_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);\n+\t} else {\n+\t\tstruct argv_array name_rev_args = ARGV_ARRAY_INIT;\n+\n+\t\tif (info->cached == 0) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\t\tcp.dir = list_item->name;\n+\n+\t\t\targv_array_pushl(&cp.args, \"git\", \"rev-parse\", \"--verify\", \"HEAD\", NULL);\n+\n+\t\t\tif (capture_command(&cp, &sb, 0))\n+\t\t\t\tdie(_(\"Could not run 'git rev-parse --verify HEAD' in submodule %s\"), list_item->name);\n+\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\targv_array_pushl(&name_rev_args, \"set-name-rev\", list_item->name,\n+\t\t\t\t\tsb.buf, NULL);\n+\n+\t\t\tprintf(_(\"+%s %s\"), sb.buf, displaypath);\n+\t\t\tset_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);\n+\n+\t\t\tstrbuf_release(&sb);\n+\t\t} else {\n+\n+\t\t\targv_array_pushl(&name_rev_args, \"set-name-rev\", list_item->name,\n+\t\t\t\t\tsub_sha1.buf, NULL);\n+\n+\t\t\tprintf(_(\"+%s %s\"), sub_sha1.buf, displaypath);\n+\t\t\tset_name_rev(name_rev_args.argc, name_rev_args.argv, info->prefix);\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\", 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 path %s\"), list_item->name);\n+\t}\n+\n+}\n+\n+static int module_status(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct cb_status info = CB_STATUS_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 output for initializing a submodule\")),\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] [<path>]\"),\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.cached = !!cached;\n+\tinfo.recursive = !!recursive;\n+\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@@ -1278,6 +1439,7 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"set-name-rev\", set_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 b6eb5bcce..a142f9c04 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1004,54 +1004,8 @@ 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 set-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 set-name-rev \"$sm_path\" \"$sha1\")\n-\t\t\tsay \"+$sha1 $displaypath$revname\"\n-\t\tfi\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\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 }\n #\n # Sync remote urls for submodules\n-- \n2.11.0\n\n"},{"id":"320483","messageId":"CAGZ79kYeJoVGRFyeGsXevo2JmDMoxf=tJubWcy5Qt==3QK=Hjg@mail.gmail.com","threadId":"46015","inReplyTo":"20170521122711.22021-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v1 2/2] submodule: port submodule subcommand status","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-22T21:28:30Z","receivedAt":"2017-05-22T21:28:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 21, 2017 at 5:27 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> This aims to make git-submodule status a builtin. 'status' is ported\n> to submodule--helper, and submodule--helper is called from\n> git-submodule.sh.\n>\n> For the purpose of porting cmd_status, the code is split up such that\n> one function obtains all the list of submodules, acting as the\n> front-end of git-submodule status. This function later calls the\n> second function for_each_submodule_list,it which basically loops\n> through the list of submodules and calls function fn, which in this\n> case is status_submodule. The third function, status submodule returns\n> the status of submodule and also takes care of the recursive flag.\n>\n> The first function module_status parses the options present in argv,\n> and then with the help of module_list_compute, generates the list of\n> submodules present in the current working tree.\n>\n> The second function for_each_submodule_list traverses through the list,\n> and calls function fn (which in the case of submodule subcommand\n> foreach is status_submodule) is called for each entry.\n>\n> The third function status_foreach checks for the various conditions,\n> and prints the status of the submodule accordingly. Also, this\n> function takes care of the recursive flag by creating a separate\n> child_process and running it inside the submodule.\n>\n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> ---\n> A new function, get_submodule_displaypath is also introduced for getting\n> the displaypath of the submodule while taking care of its prefix and\n> superprefix.\n>\n>  builtin/submodule--helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            |  48 +------------\n>  2 files changed, 163 insertions(+), 47 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 5f0ddd8ad..7c040a375 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -13,6 +13,8 @@\n>  #include \"refs.h\"\n>  #include \"connect.h\"\n>\n> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);\n> +\n>  static char *get_default_remote(void)\n>  {\n>         char *dest = NULL, *ret;\n> @@ -219,6 +221,23 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n>         return 0;\n>  }\n>\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> +               return xstrdup(relative_path(path, prefix, &sb));\n> +       } else if (super_prefix) {\n> +               return xstrfmt(\"%s/%s\", super_prefix, path);\n> +       } else {\n> +               return xstrdup(path);\n> +       }\n> +}\n> +\n>  enum describe_step {\n>         step_bare = 0,\n>         step_tags,\n> @@ -395,6 +414,13 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>         return 0;\n>  }\n>\n> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t 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\nUp to here it looks like the patch in\nhttps://public-inbox.org/git/20170521125814.26255-2-pc44800@gmail.com/\n(without the nit of having an extra void return)\n\nMaybe it is worth it to combine the two patch series, such that we'd need\nto review the common parts only once?\n\n> +\n>  static void init_submodule(const char *path, const char *prefix, int quiet)\n>  {\n>         const struct submodule *sub;\n> @@ -532,6 +558,141 @@ static int module_init(int argc, const char **argv, const char *prefix)\n>         return 0;\n>  }\n>\n> +struct cb_status {\n> +       const char *prefix;\n> +       unsigned int quiet: 1;\n> +       unsigned int cached: 1;\n> +       unsigned int recursive: 1;\n> +};\n> +#define CB_STATUS_INIT { NULL, 0, 0, 0 }\n\n\n\n> +\n> +               if (run_command(&cpr))\n> +                       die(_(\"Failed to recurse into submodule path %s\"), list_item->name);\n\nI thought this is a badly worded error message, but it turns out it\nis just as in the shell code, which is good for a direct translation.\n\nMaybe we can adapt the error message in a later follow up to be more\naligned to other submodule error messages. (dropping \"path\" and putting\nsingle quotes around %s, also un-capitalize the first letter)\n\n\n> +static int module_status(int argc, const char **argv, const char *prefix)\n> +{\n> +       struct cb_status info = CB_STATUS_INIT;\n> +       struct pathspec pathspec;\n> +       struct module_list list = MODULE_LIST_INIT;\n> +       int quiet = 0;\n> +       int cached = 0;\n> +       int recursive = 0;\n> +\n> +       struct option module_status_options[] = {\n> +               OPT__QUIET(&quiet, N_(\"Suppress output for initializing a submodule\")),\n> +               OPT_BOOL(0, \"cached\", &cached, N_(\"Use commit stored in the index instead of the one stored in the submodule HEAD\")),\n> +               OPT_BOOL(0, \"recursive\", &recursive, N_(\"Recurse into nested submodules\")),\n> +               OPT_END(),\n> +       };\n> +\n> +       const char *const git_submodule_helper_usage[] = {\n> +               N_(\"git submodule status [--quiet] [--cached] [--recursive] [<path>]\"),\n> +               NULL\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, module_status_options,\n> +                            git_submodule_helper_usage, 0);\n> +\n> +       if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n> +               return 1;\n> +\n> +       info.prefix = prefix;\n> +       info.quiet = !!quiet;\n> +       info.cached = !!cached;\n> +       info.recursive = !!recursive;\n> +\n> +       for_each_submodule_list(list, status_submodule, &info);\n> +\n> +       return 0;\n> +}\n\nThis function looks good. Though my gut reaction was to suggest to\nadd another layer of abstraction. Then I checked wt-status.c, but that\nmakes use of \"submodule summary\" and not \"submodule status\". So all is good.\n\n\n> +       git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n\nI'd think we would not need to pass down superprefix here as we do not call\n\"submodule status\" in a recursive way. The recursion works on\nthe submodule helper itself, so we could simplify it to just\n\n    git ${wt_prefix:+-C \"$wt_prefix\"} submodule--helper status\n${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive}\n\"$@\"\n\nAnother idea that I just had:\nMaybe we could drop --cached, --recursive as well,\nas they are just command line options, which could\nbe just part of \"$@\".\n\nFor --quiet this is a bit more complicated as it may come in\nvia an environment variable (which we could also check for\nin C in theory. I know I omitted that when writing some\nsubmodule--helper code a couple months ago, but the reason\nescaped me)\n\nThanks,\nStefan\n"},{"id":"321555","messageId":"20170605202529.22959-1-pc44800@gmail.com","threadId":"46015","inReplyTo":"CAGZ79kYeJoVGRFyeGsXevo2JmDMoxf=tJubWcy5Qt==3QK=Hjg@mail.gmail.com","subject":"[GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-06-05T20:25:28Z","receivedAt":"2017-06-05T20:26:25Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Since later on we want to port submodule subcommand status, and since\nset_name_rev is part of cmd_status, hence this function is ported. It\nhas been ported to function print_name_rev in C, which calls get_name_rev\nto get the revname, and after formatting it, print_name_rev prints it.\nAnd hence in this way, the command `git submodule--helper print-name-rev\n\"sm_path\" \"sha1\"` sets value of revname in git-submodule.sh\n\nThe function get_name_rev returns the stdout of the git describe\ncommands. Since there are four different git-describe commands used for\ngenerating the name rev, four child_process are introduced, each successive\nchild process running only when previous has no stdout. The order of these\nfour git-describe commands is maintained the same as it was in the function\nset_name_rev() in shell script.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\n builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 16 ++---------\n 2 files changed, 69 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..3022118d1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n+enum describe_step {\n+\tstep_bare = 0,\n+\tstep_tags,\n+\tstep_contains,\n+\tstep_all_always,\n+\tstep_end\n+};\n+\n+static char *get_name_rev(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct child_process cp;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tenum describe_step cur_step;\n+\n+\tfor (cur_step = step_bare; cur_step < step_end; cur_step++) {\n+\t\tchild_process_init(&cp);\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.dir = argv[1];\n+\t\tcp.no_stderr = 1;\n+\n+\t\tswitch (cur_step) {\n+\t\t\tcase step_bare:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n+\t\t\t\t\t\t argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_tags:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n+\t\t\t\t\t\t \"--tags\", argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_contains:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n+\t\t\t\t\t\t \"--contains\", argv[2], NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase step_all_always:\n+\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n+\t\t\t\t\t\t \"--all\", \"--always\", argv[2],\n+\t\t\t\t\t\t NULL);\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tBUG(\"unknown describe step '%d'\", cur_step);\n+\t\t}\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: <path> <sha1>\");\n+\n+\tnamerev = get_name_rev(argc, argv, prefix);\n+\tif (namerev && namerev[0])\n+\t\tprintf(\" (%s)\", namerev);\n+\tprintf(\"\\n\");\n+\n+\treturn 0;\n+}\n+\n struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -1212,6 +1278,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 c0d0e9a4c..091051891 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 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":"321556","messageId":"20170605202529.22959-2-pc44800@gmail.com","threadId":"46015","inReplyTo":"20170605202529.22959-1-pc44800@gmail.com","subject":"[GSoC][PATCH v2 2/2] submodule: port submodule subcommand status","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-06-05T20:25:29Z","receivedAt":"2017-06-05T20:26:32Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule subcommand status a builtin. Here\n'status' is ported to submodule--helper, and submodule--helper is\ncalled from git-submodule.sh.\n\nFor the purpose of porting cmd_status, the code is split up such that\none function obtains all the list of submodules, acting as the front-end\nof git-submodule status. This function later calls the second function\nfor_each_submodule_list,it which basically loops through the list of\nsubmodules and calls function fn, which in this case is status_submodule.\nThe third function, status submodule returns the status of submodule and\nalso takes care of the recursive flag.\n\nThe first function module_status parses the options present in argv,\nand then with the help of module_list_compute, generates the list of\nsubmodules present in the current working tree.\n\nThe second function for_each_submodule_list traverses through the list,\nand calls function fn (which in the case of submodule subcommand status\nis status_submodule) is called for each entry.\n\nThe third function status_submodule checks for the various conditions,\nand prints the status of the submodule accordingly. Also, this function\ntakes care of the recursive flag by creating a separate child_process\nand running it inside the submodule. The function print_status handles the\nprinting of submodule's status.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\nIn this new version of patch, function print_status is introduced.\n\nThe functions for_each_submodule_list and get_submodule_displaypath\nare found to be the same as those in the ported submodule subcommand\nforeach's patches. The reason for doing so is to keep both the patches\nindependant and on separate branches. Also this patch is build on\nthe branch gitster/jk/bug-to-abort for utilizing its BUG() macro.\n \nComplete build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: submodule-status-new\nBuild #91\n\n builtin/submodule--helper.c | 181 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  49 +-----------\n 2 files changed, 182 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3022118d1..85da05550 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -13,6 +13,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@@ -219,6 +222,25 @@ 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\treturn xstrfmt(\"%s/%s\", super_prefix, path);\n+\t} else {\n+\t\treturn xstrdup(path);\n+\t}\n+}\n+\n enum describe_step {\n \tstep_bare = 0,\n \tstep_tags,\n@@ -397,6 +419,13 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void for_each_submodule_list(const struct module_list list, 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 static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -534,6 +563,157 @@ 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 char *sub_sha1, char *displaypath)\n+{\n+\tif (info->quiet)\n+\t\treturn;\n+\n+\tprintf(\"%c%s %s\", state, sub_sha1, 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, sub_sha1, NULL);\n+\t\tprint_name_rev(name_rev_args.argc, name_rev_args.argv,\n+\t\t\t       info->prefix);\n+\t} else {\n+\t\tprintf(\"\\n\");\n+\t}\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 *sub_sha1 = xstrdup(oid_to_hex(&list_item->oid));\n+\tchar *displaypath;\n+\tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n+\n+\tif (!submodule_from_path(null_sha1, 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 (list_item->ce_flags) {\n+\t\tprint_status(info, 'U', list_item->name,\n+\t\t\t     sha1_to_hex(null_sha1), displaypath);\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!is_submodule_initialized(list_item->name)) {\n+\t\tprint_status(info, '-', list_item->name, sub_sha1, 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, sub_sha1, displaypath);\n+\t} else {\n+\t\tif (!info->cached) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.dir = list_item->name;\n+\n+\t\t\targv_array_pushl(&cp.args, \"rev-parse\",\n+\t\t\t\t\t \"--verify\", \"HEAD\", NULL);\n+\n+\t\t\tif (capture_command(&cp, &sb, 0))\n+\t\t\t\tdie(_(\"could not run 'git rev-parse --verify\"\n+\t\t\t\t      \"HEAD' in submodule %s\"),\n+\t\t\t\t      list_item->name);\n+\n+\t\t\tstrbuf_strip_suffix(&sb, \"\\n\");\n+\t\t\tprint_status(info, '+', list_item->name, sb.buf,\n+\t\t\t\t     displaypath);\n+\t\t\tstrbuf_release(&sb);\n+\t\t} else {\n+\t\t\tprint_status(info, '+', list_item->name, sub_sha1,\n+\t\t\t\t     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+\tfree(displaypath);\n+\tfree(sub_sha1);\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] [<path>]\"),\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@@ -1280,6 +1460,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 091051891..a24b1b91b 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 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":"321572","messageId":"CAGZ79kZv_Jw1n6z=5sDOtiJecyJmQ-NEzYN6kPL_uCCHTKE0bg@mail.gmail.com","threadId":"46015","inReplyTo":"20170605202529.22959-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-05T22:50:22Z","receivedAt":"2017-06-05T22:50:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 5, 2017 at 1:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> Since later on we want to port submodule subcommand status, and since\n> set_name_rev is part of cmd_status, hence this function is ported. It\n> has been ported to function print_name_rev in C, which calls get_name_rev\n> to get the revname, and after formatting it, print_name_rev prints it.\n> And hence in this way, the command `git submodule--helper print-name-rev\n> \"sm_path\" \"sha1\"` sets value of revname in git-submodule.sh\n>\n> The function get_name_rev returns the stdout of the git describe\n> commands. Since there are four different git-describe commands used for\n> generating the name rev, four child_process are introduced, each successive\n> child process running only when previous has no stdout. The order of these\n> four git-describe commands is maintained the same as it was in the function\n> set_name_rev() in shell script.\n>\n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 16 ++---------\n>  2 files changed, 69 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 566a5b6a6..3022118d1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n>         return 0;\n>  }\n>\n> +enum describe_step {\n> +       step_bare = 0,\n\nDo we rely on step_bare to be equal to 0?\n(This is the hint I am reading from '=0' here.\nIf we do not, please omit.)\n\n> +       step_tags,\n> +       step_contains,\n> +       step_all_always,\n> +       step_end\n> +};\n> +\n> +static char *get_name_rev(int argc, const char **argv, const char *prefix)\n\nSo we split up the functionality into two functions.\nget_name_rev, which does the heavy lifting work, and\nprint_name_rev, that is a wrapper around having to deal with\ngoing from shell to C and back.\n\nOne of C strength' compared to shell is type safety,\nso maybe we can tighten the contract that get_name_rev\noffers to its callers and make it\n\n  get_name_rev(const char *sub_path, const char *object_id / sha1)\n\nand then have print_name_rev call it via\n\n  get_name_rev (argv[1], argv[2])\n\n(which coincidentally is right after checking for\nargc != 3, which reinforces that the contract of the\nwrapper is \"just making sure we have valid input\" and\nthis function \"just does heavy lifting, assuming input\nis valid\".\n\n> +{\n> +       struct child_process cp;\n> +       struct strbuf sb = STRBUF_INIT;\n> +       enum describe_step cur_step;\n> +\n> +       for (cur_step = step_bare; cur_step < step_end; cur_step++) {\n> +               child_process_init(&cp);\n\n(minor nit, personal opinion, feel free to ignore:)\nAlternatively, you could declare cp inside the loop assigned to\nCHILD_PROCESS_INIT.\nSame for strbuf sb as well, such that you only declare the iterator\nvariable outside the loop.\n\n> +               prepare_submodule_repo_env(&cp.env_array);\n> +               cp.dir = argv[1];\n> +               cp.no_stderr = 1;\n\ncp.git_cmd = 1; as well?\n\nThanks,\nStefan\n"},{"id":"321574","messageId":"CAGZ79kZW=B1kd91y6VCushz5tqk1jYW=WzHheHi1pSiMWMQ4iQ@mail.gmail.com","threadId":"46015","inReplyTo":"20170605202529.22959-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v2 2/2] submodule: port submodule subcommand status","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-05T23:12:29Z","receivedAt":"2017-06-05T23:12:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 5, 2017 at 1:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> This aims to make git-submodule subcommand status a builtin. Here\n> 'status' is ported to submodule--helper, and submodule--helper is\n> called from git-submodule.sh.\n>\n> For the purpose of porting cmd_status, the code is split up such that\n> one function obtains all the list of submodules, acting as the front-end\n> of git-submodule status. This function later calls the second function\n> for_each_submodule_list,it which basically loops through the list of\n> submodules and calls function fn, which in this case is status_submodule.\n> The third function, status submodule returns the status of submodule and\n> also takes care of the recursive flag.\n>\n> The first function module_status parses the options present in argv,\n> and then with the help of module_list_compute, generates the list of\n> submodules present in the current working tree.\n>\n> The second function for_each_submodule_list traverses through the list,\n> and calls function fn (which in the case of submodule subcommand status\n> is status_submodule) is called for each entry.\n>\n> The third function status_submodule checks for the various conditions,\n> and prints the status of the submodule accordingly. Also, this function\n> takes care of the recursive flag by creating a separate child_process\n> and running it inside the submodule. The function print_status handles the\n> printing of submodule's status.\n>\n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> ---\n> In this new version of patch, function print_status is introduced.\n>\n> The functions for_each_submodule_list and get_submodule_displaypath\n> are found to be the same as those in the ported submodule subcommand\n> foreach's patches. The reason for doing so is to keep both the patches\n> independant and on separate branches.\n\n\nMaybe keep it even in a separate patch, such that\nthe status series becomes:\n  patch 1: introduce for_each_submodule_list and get_submodule_displaypath\n  patch 2: port print_name_rev\n  patch 3: port status\n\nwhereas the foreach series (and other series later) could\nre-use patch 1, and build on top of it.\n\nFor reviewing patches, it is fine to have the\nget_submodule_displaypath is both series, though for applying\npatches it for less complication/deduplication from the maintainer\nI would think.\n\n> +\n> +static void print_status(struct status_cb *info, char state, const char *path,\n> +                        char *sub_sha1, char *displaypath)\n> +{\n> +       if (info->quiet)\n> +               return;\n> +\n> +       printf(\"%c%s %s\", state, sub_sha1, displaypath);\n> +\n> +       if (state == ' ' || state == '+') {\n> +               struct argv_array name_rev_args = ARGV_ARRAY_INIT;\n> +\n> +               argv_array_pushl(&name_rev_args, \"print-name-rev\",\n> +                                path, sub_sha1, NULL);\n> +               print_name_rev(name_rev_args.argc, name_rev_args.argv,\n> +                              info->prefix);\n\n... with the suggestion given in the print_name_rev patch, this would\nbecome a one liner. :)\n\n\nThe rest looks good to me :)\n"},{"id":"321577","messageId":"20170605232059.GF40426@google.com","threadId":"46015","inReplyTo":"20170605202529.22959-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v2 1/2] submodule: port set_name_rev from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-05T23:20:59Z","receivedAt":"2017-06-05T23:21:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/06, Prathamesh Chavan wrote:\n> Since later on we want to port submodule subcommand status, and since\n> set_name_rev is part of cmd_status, hence this function is ported. It\n> has been ported to function print_name_rev in C, which calls get_name_rev\n> to get the revname, and after formatting it, print_name_rev prints it.\n> And hence in this way, the command `git submodule--helper print-name-rev\n> \"sm_path\" \"sha1\"` sets value of revname in git-submodule.sh\n> \n> The function get_name_rev returns the stdout of the git describe\n> commands. Since there are four different git-describe commands used for\n> generating the name rev, four child_process are introduced, each successive\n> child process running only when previous has no stdout. The order of these\n> four git-describe commands is maintained the same as it was in the function\n> set_name_rev() in shell script.\n> \n> Mentored-by: Christian Couder <christian.couder@gmail.com>\n> Mentored-by: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 16 ++---------\n>  2 files changed, 69 insertions(+), 14 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 566a5b6a6..3022118d1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -219,6 +219,72 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n>  \treturn 0;\n>  }\n>  \n> +enum describe_step {\n> +\tstep_bare = 0,\n> +\tstep_tags,\n> +\tstep_contains,\n> +\tstep_all_always,\n> +\tstep_end\n> +};\n> +\n> +static char *get_name_rev(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct child_process cp;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tenum describe_step cur_step;\n> +\n> +\tfor (cur_step = step_bare; cur_step < step_end; cur_step++) {\n> +\t\tchild_process_init(&cp);\n> +\t\tprepare_submodule_repo_env(&cp.env_array);\n> +\t\tcp.dir = argv[1];\n> +\t\tcp.no_stderr = 1;\n\nset cp.git = 1 so that you can avoid pushing \"git\" onto the arg array.\n\n> +\n> +\t\tswitch (cur_step) {\n> +\t\t\tcase step_bare:\n> +\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n> +\t\t\t\t\t\t argv[2], NULL);\n> +\t\t\t\tbreak;\n> +\t\t\tcase step_tags:\n> +\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n> +\t\t\t\t\t\t \"--tags\", argv[2], NULL);\n> +\t\t\t\tbreak;\n> +\t\t\tcase step_contains:\n> +\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n> +\t\t\t\t\t\t \"--contains\", argv[2], NULL);\n> +\t\t\t\tbreak;\n> +\t\t\tcase step_all_always:\n> +\t\t\t\targv_array_pushl(&cp.args, \"git\", \"describe\",\n> +\t\t\t\t\t\t \"--all\", \"--always\", argv[2],\n> +\t\t\t\t\t\t NULL);\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tBUG(\"unknown describe step '%d'\", cur_step);\n> +\t\t}\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: <path> <sha1>\");\n> +\n> +\tnamerev = get_name_rev(argc, argv, prefix);\n> +\tif (namerev && namerev[0])\n> +\t\tprintf(\" (%s)\", namerev);\n> +\tprintf(\"\\n\");\n> +\n> +\treturn 0;\n> +}\n> +\n>  struct module_list {\n>  \tconst struct cache_entry **entries;\n>  \tint alloc, nr;\n> @@ -1212,6 +1278,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},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index c0d0e9a4c..091051891 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 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\n-- \nBrandon Williams\n"},{"id":"321595","messageId":"CAP8UFD0ZRW0N-WZfQB0d7+c+LQD3Jkcpcj44kWhnDaEOd42wCg@mail.gmail.com","threadId":"46015","inReplyTo":"20170605202529.22959-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v2 2/2] submodule: port submodule subcommand status","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-06-06T04:24:58Z","receivedAt":"2017-06-06T04:25:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 5, 2017 at 10:25 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n\n> ---\n> In this new version of patch, function print_status is introduced.\n>\n> The functions for_each_submodule_list and get_submodule_displaypath\n> are found to be the same as those in the ported submodule subcommand\n> foreach's patches. The reason for doing so is to keep both the patches\n> independant and on separate branches. Also this patch is build on\n> the branch gitster/jk/bug-to-abort for utilizing its BUG() macro.\n\nThe BUG() macro is now in master.\n"}]}