{"thread":{"id":"45748","subject":"[GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C","startedAt":"2017-04-19T17:06:36Z","lastAt":"2017-06-05T22:20:40Z","messageCount":48,"participants":["Prathamesh Chavan","Stefan Beller","Junio C Hamano","Ramsay Jones","Brandon Williams","Johannes Sixt","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317249","messageId":"20170419170513.16475-1-pc44800@gmail.com","threadId":"45748","inReplyTo":null,"subject":"[GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-04-19T17:05:13Z","receivedAt":"2017-04-19T17:06:36Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule foreach a builtin. This is the very\nfirst step taken in this direction. Hence, 'foreach' is ported to\nsubmodule--helper, and submodule--helper is called from git-submodule.sh.\nThe code is split up to have one function to obtain all the list of\nsubmodules and a calling function that takes care of running the command\nin that submodule, and recursively perform the same when --recursive is\nflagged.\n\nThe First function module_foreach first parses the options present in\nargv, and then with the help of read_cache, generates the list of\nsubmodules present in the current working tree. Traversing through the\nlist, foreach_submodule function is called for each entry.\n\nThe second function foreach_submodule, generates a submodule struct sub\nfor $name, $path values and then later prepends name=sub->name;\npath=sub-> path; and other value assignment to an argv_array structure.\nAlso the of submodule-foreach is appended to this structure\nand finally, using run_command_v_opt the commands are executed in a\nsingle but separate shell.\n\nThe second function also takes care of the recursive flag, by creating\na saperate argv_array structure and prepending \"--super-prefix displaypath\",\nand other required arguments to the structure and then appending the\ninput of submodule-foreach to the argument's array.\n\n\nSigned-off-by: Prathamesh Chavan \n---\n\nThe build report of this patch is available at: \nhttps://travis-ci.org/pratham-pc/git/builds/223573936\n\nClearly, there are still some tests which are failing. I have\nsubmitted this as RFC patch for getting suggestions on debugging these\nerrors and for reviewing the approach taken for porting submodule\n'foreach' subcommand to C.\n\nAlso, I have based my branch on gitster/jk/ls-files-recurse-submodules-fix,\nsince while using --super-prefix in recursively calling the foreach\ncommand, it produced results indicating that a --super-prefix can't\nbe used from a subdirectory:\n\n fatal: can't use --super-prefix from a subdirectory\n\nThe patch and the discussion related to it can be found at: \nhttps://public-inbox.org/git/20170412003911.1142-1-jacob.e.keller@intel.com/T/#u\n\nAlso, I would like to ask is there are any more changes required in my microproject for getting it merged.\nhttps://public-inbox.org/git/20170403213557.27724-1-pc44800@gmail.com/\n\n\n builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 40 +-----------\n 2 files changed, 154 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85aafe46a..276ed6025 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -487,6 +487,158 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void foreach_submodule(int argc, const char **argv, const char *path,\n+\t\t\t const unsigned char *sha1, const char *prefix,\n+\t\t\t int quiet, int recursive)\n+{\n+\tconst char *toplevel = xgetcwd();\n+\tconst struct submodule *sub;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf sub_sha1 = STRBUF_INIT;\n+\tstruct strbuf cmd = STRBUF_INIT;\n+\tchar *displaypath;\n+\tint i;\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+\t} else if (prefix) {\n+\t\tdisplaypath = xstrdup(relative_path(prefix, path, &sb));\n+\t} else 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+\t}\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\tstrbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);\n+\n+\tif (!chdir(path)) {\n+\t\tif (!access_or_warn(\".git\", R_OK, 0)) {\n+\t\t\tif (!quiet)\n+\t\t\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\n+\t\t\tif (argc == 1) {\n+\t\t\t\tstruct argv_array argcp1 = ARGV_ARRAY_INIT;\n+\n+\t\t\t\tstrbuf_addstr(&cmd, \"name=\");\n+\t\t\t\tstrbuf_addstr(&cmd, sub->name);\n+\t\t\t\tstrbuf_addstr(&cmd, \"; \");\n+\t\t\t\tstrbuf_addstr(&cmd, \"toplevel=\");\n+\t\t\t\tstrbuf_addstr(&cmd, toplevel);\n+\t\t\t\tstrbuf_addstr(&cmd, \"; \");\n+\t\t\t\tstrbuf_addstr(&cmd, \"sha1=\");\n+\t\t\t\tstrbuf_addstr(&cmd, sub_sha1.buf);\n+\t\t\t\tstrbuf_addstr(&cmd, \"; \");\n+\t\t\t\tstrbuf_addstr(&cmd, \"path=\");\n+\t\t\t\tstrbuf_addstr(&cmd, sub->path);\n+\t\t\t\tstrbuf_addstr(&cmd, \"; \");\n+\t\t\t\tstrbuf_addstr(&cmd, argv[0]);\n+\n+\t\t\t\targv_array_push(&argcp1, cmd.buf);\n+\t\t\t\trun_command_v_opt(argcp1.argv, RUN_USING_SHELL);\n+\t\t\t} else {\n+\t\t\t\trun_command_v_opt(argv, RUN_USING_SHELL);\n+\t\t\t}\n+\n+\t\t\tif (recursive) {\n+\t\t\t\tstruct argv_array argcp = ARGV_ARRAY_INIT;\n+\n+\t\t\t\targv_array_push(&argcp, \"git\");\n+\t\t\t\targv_array_push(&argcp, \"--super-prefix\");\n+\t\t\t\targv_array_push(&argcp, displaypath);\n+\t\t\t\targv_array_push(&argcp, \"submodule--helper\");\n+\n+\t\t\t\tif (quiet)\n+\t\t\t\t\targv_array_push(&argcp, \"--quiet\");\n+\t\t\t\targv_array_push(&argcp, \"foreach\");\n+\t\t\t\targv_array_push(&argcp, \"--recursive\");\n+\n+\t\t\t\tfor (i = 0; i < argc; i++)\n+\t\t\t\t\targv_array_push(&argcp, argv[i]);\n+\n+\t\t\t\trun_command_v_opt(argcp.argv, RUN_USING_SHELL);\n+\t\t\t}\n+\n+\t\t\tif (chdir(toplevel))\n+\t\t\t\tdie_errno(_(\"cannot chdir to %s\"), toplevel);\n+\t\t}\n+\t} else {\n+\t\tdie_errno(_(\"cannot chdir to %s\"), path);\n+\t}\n+\n+\tstrbuf_release(&cmd);\n+\tstrbuf_release(&sub_sha1);\n+\tstrbuf_release(&sb);\n+\tfree(displaypath);\n+\treturn;\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\tint i;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of Entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t N_(\"Traverse submodules ercursively and apply the command for all nested submodules\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper foreach [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\n+\t\tALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n+\t\tlist.entries[list.nr++] = ce;\n+\t\twhile (i + 1 < active_nr &&\n+\t\t\t!strcmp(ce->name, active_cache[i + 1]->name))\n+\t\t\t /*\n+\t\t\t * Skip entries with the same name in different stages\n+\t\t\t * to make sure an entry is returned only once.\n+\t\t\t */\n+\t\t\ti++;\n+\t}\n+\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\tif (prefix) {\n+\t\t\tconst char *out = NULL;\n+\t\t\tif (skip_prefix(prefix, list.entries[i]->name, &out)) {\n+\t\t\t\tif (out && out[0] == '/' && !out + 1) \n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\n+\t\tforeach_submodule(argc, argv, list.entries[i]->name,\n+\t\t\t\t list.entries[i]->oid.hash, prefix,\n+\t\t\t\t quiet, recursive);\n+\t}\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1168,6 +1320,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 6ec35e5fc..e2c2b40f4 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -321,46 +321,8 @@ cmd_foreach()\n \t\tesac\n \t\tshift\n \tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\ttoplevel=$(pwd)\n-\n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\n-\n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.11.0\n\n"},{"id":"317259","messageId":"CAGZ79kYmRe+NURkgxRQM2QsGQEqtp+oGas5H0ryfztx8s2chwA@mail.gmail.com","threadId":"45748","inReplyTo":"20170419170513.16475-1-pc44800@gmail.com","subject":"Re: [GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-19T18:08:11Z","receivedAt":"2017-04-19T18:08:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Apr 19, 2017 at 10:05 AM, Prathamesh Chavan wrote:\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n\ncool :)\n\n\n> The code is split up to have one function to obtain all the list of\n> submodules and a calling function that takes care of running the command\n> in that submodule, and recursively perform the same when --recursive is\n> flagged.\n>\n> The First function module_foreach first parses the options present in\n> argv, and then with the help of read_cache, generates the list of\n> submodules present in the current working tree. Traversing through the\n> list, foreach_submodule function is called for each entry.\n\nI wonder if we could re-use module_list here?\n\n> The second function foreach_submodule, generates a submodule struct sub\n> for $name, $path values and then later prepends name=sub->name;\n> path=sub-> path; and other value assignment to an argv_array structure.\n> Also the of submodule-foreach is appended to this structure\n> and finally, using run_command_v_opt the commands are executed in a\n> single but separate shell.\n\nAs noted below, I would use a struct child_process as that seems to make life\neasier here.\n\n\nWhen applying the patch git-am says:\nApplying: submodule: port subcommand foreach from shell to C\n.git/rebase-apply/patch:177: trailing whitespace.\n if (out && out[0] == '/' && !out + 1)\nwarning: 1 line adds whitespace errors.\n\n>\n> builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 40 +-----------\n\ncool. :)\n\n> +\n> + /* Only loads from .gitmodules, no overlay with .git/config */\n\nWhy would we not overlay the .gitmodules config with .git/config?\n\n> + gitmodules_config();\n> +\n> + if (prefix && get_super_prefix()) {\n> + die(\"BUG: cannot have prefix and superprefix\");\n> + } else if (prefix) {\n> + displaypath = xstrdup(relative_path(prefix, path, &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> + }\n> +\n> + sub = submodule_from_path(null_sha1, path);\n> +\n> + if (!sub)\n> + die(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> + displaypath);\n> + strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);\n> +\n\n> +\n> + if (argc == 1) {\n> + struct argv_array argcp1 = ARGV_ARRAY_INIT;\n\nOh the case of argc=1 is interesting. 1c4fb136db (submodule foreach:\nskip eval for more than one argument, 2013-09-27) explains why.\n\n\n\n> +\n> + strbuf_addstr(&cmd, \"name=\");\n> + strbuf_addstr(&cmd, sub->name);\n> + strbuf_addstr(&cmd, \"; \");\n> + strbuf_addstr(&cmd, \"toplevel=\");\n> + strbuf_addstr(&cmd, toplevel);\n> + strbuf_addstr(&cmd, \"; \");\n> + strbuf_addstr(&cmd, \"sha1=\");\n> + strbuf_addstr(&cmd, sub_sha1.buf);\n> + strbuf_addstr(&cmd, \"; \");\n> + strbuf_addstr(&cmd, \"path=\");\n> + strbuf_addstr(&cmd, sub->path);\n> + strbuf_addstr(&cmd, \"; \");\n> + strbuf_addstr(&cmd, argv[0]);\n\nInstead of prefixing the command with these variables, we can set them\nas environment variables; Then we do not have to add semicolons ourselves as the\nenvironment variable infrastructure does that for us.\n\n struct child_process cp = CHILD_PROCESS_INIT;\n argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n argv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n ...\n\n> +\n> + argv_array_push(&argcp1, cmd.buf);\n> + run_command_v_opt(argcp1.argv, RUN_USING_SHELL);\n\nOh, you use run_command_v_opt, which is a wrapper around the struct\nchild_process.\nI would suggest to use the struct child_process directly as then we\ncan set the environment\nourselves in an easier way. To set this flag we'd do\n\n cp.use_shell = 1;\n\n> + if (!chdir(path)) {\n> + if (!access_or_warn(\".git\", R_OK, 0)) {\n\nThe same applies to changing directories. Here in this code we chdir\n(path) and later\nchdir (toplevel), but this process doesn't need to change its\ndirectories but it can stay at\nthe toplevel directory. Only the child process needs chdir to the\ncorrect path, which can\nbe done via\n\n cp.dir = path;\n\n\n> + } else {\n> + run_command_v_opt(argv, RUN_USING_SHELL);\n> + }\n> +\n> + if (recursive) {\n> + struct argv_array argcp = ARGV_ARRAY_INIT;\n> +\n> + argv_array_push(&argcp, \"git\");\n> + argv_array_push(&argcp, \"--super-prefix\");\n> + argv_array_push(&argcp, displaypath);\n> + argv_array_push(&argcp, \"submodule--helper\");\n\nGood call, the recursing still needs to create a new child process of its own\ninstead of just calling the function recursively, as we do not have\naccess to the\nnested submodule data here.\n\n> +\n> + if (quiet)\n> + argv_array_push(&argcp, \"--quiet\");\n> + argv_array_push(&argcp, \"foreach\");\n> + argv_array_push(&argcp, \"--recursive\");\n> +\n> + for (i = 0; i < argc; i++)\n> + argv_array_push(&argcp, argv[i]);\n> +\n> + run_command_v_opt(argcp.argv, RUN_USING_SHELL);\n\nI'd also suggest to use the struct child_process directly here instead\nof a wrapper\nas then we have access to all the knobs ourselves.\n\n> +\n> + struct option module_foreach_options[] = {\n> + OPT__QUIET(&quiet, N_(\"Suppress output of Entering each submodule command\")),\n> + OPT_BOOL(0, \"recursive\", &recursive,\n> + N_(\"Traverse submodules ercursively and apply the command for all nested submodules\")),\n\ns/ercursively/recursively/\nAlso wording: Do you apply a command or do you run a command? Maybe in\na shorter version:\n N_(\"recurse into nested submodules\")\n\n\n> + const char *const git_submodule_helper_usage[] = {\n> + N_(\"git submodule--helper foreach [--recursive] \"),\n\nand [--quiet] as well\n\n\n> + for (i = 0; i < active_nr; i++) {\n> + const struct cache_entry *ce = active_cache[i];\n> +\n> + if (!S_ISGITLINK(ce->ce_mode))\n> + continue;\n> +\n> + ALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n> + list.entries[list.nr++] = ce;\n> + while (i + 1 < active_nr &&\n> + !strcmp(ce->name, active_cache[i + 1]->name))\n> + /*\n> + * Skip entries with the same name in different stages\n> + * to make sure an entry is returned only once.\n> + */\n> + i++;\n> + }\n> +\n> + for (i = 0; i < list.nr; i++) {\n> + if (prefix) {\n> + const char *out = NULL;\n> + if (skip_prefix(prefix, list.entries[i]->name, &out)) {\n> + if (out && out[0] == '/' && !out + 1)\n> + return 0;\n> + }\n> + }\n\nThe lines up to here are the functional equivalent of\n git submodule--helper list --prefix \"$wt_prefix\"\n\nWould it make sense to get the list via calling\n\n int argc = 0;\n char *argv = {NULL}; /* it has to be null terminated, I think */\n\n struct pathspec pathspec;\n /* no need to init this, as module_list_compute will do that via\n parse_pathspec */\n\n struct module_list list = MODULE_LIST_INIT\n\n if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n die(\"BUG: module_list_compute should not choke on empty pathspec\");\n\n for (i = 0; i < list.nr; i++)\n foreach_submodule(...);\n\n> static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> const char *depth, struct string_list *reference,\n> int quiet, int progress)\n> @@ -1168,6 +1320,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> + {\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n\nHaving SUPPORT_SUPER_PREFIX makes sense as we want to print out\npath from outside the repo for example.\n\n\n> + git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n\nI think we'd want to drop the \"quotes\" around the last $@ such that the\narguments are not passed in as one long string, but one by one.\n(Then the submodule--helper code can differentiate between the\nnumber of arguments, just as the shell code does below)\n\nThanks,\nStefan\n"},{"id":"317549","messageId":"20170422195804.18477-1-pc44800@gmail.com","threadId":"45748","inReplyTo":"CAGZ79kYmRe+NURkgxRQM2QsGQEqtp+oGas5H0ryfztx8s2chwA@mail.gmail.com","subject":"[GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-04-22T19:58:04Z","receivedAt":"2017-04-22T19:59:28Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule foreach a builtin. This is the very\nfirst step taken in this direction. Hence, 'foreach' is ported to\nsubmodule--helper, and submodule--helper is called from git-submodule.sh.\nThe code is split up to have one function to obtain all the list of\nsubmodules and a calling function that takes care of running the command\nin that submodule, and recursively perform the same when --recursive is\nflagged.\n\nThe First function module_foreach first parses the options present in\nargv, and then with the help of module_list_compute, generates the list of\nsubmodules present in the current working tree. Traversing through the\nlist, foreach_submodule function is called for each entry.\n\nThe second function foreach_submodule, generates a submodule struct sub\nfor $name, value and then later prepends name=sub->name; and other\nvalue assignment to the env argv_array structure of a child_process.\nAlso the of submodule-foreach is push to args argv_array\nstructure and finally, using run_command the commands are executed\nusing a shell.\n\nThe second function also takes care of the recursive flag, by creating\na saperate child_process structure and prepending \"--super-prefix displaypath\",\nto the args argv_array structure. Other required arguments and the\ninput of submodule-foreach is also appended to this argv_array.\n\nSigned-off-by: Prathamesh Chavan \n---\n\nIn this new version of patch following changes have been added:\n\nmodule_list_compute is used to generate the list of submodules present\nin the current working tree.\n\nInstead of using argv_array structure, a child_process structure\nis been used to set the env variable, and run the given command at the \nspecified path with the given arguments.\n\nIt was suggested that the quotes of \"$@\" be removed to avoid passing\nthe arguments as a single long string. But since quoted\n$@ doesn't pass the arguments as a single long string, but instead\npasses quoted arguments, in this patch I have kept \"$@\" unchanged.\nInformation related to this is available on:\nwww.gnu.org/software/bash/manual/html_node/Special-Parameters.html\n\nAlso, it was observed that just after removal of the quotes of \"$@\"\npresent in the git-submodule.sh file, there were a number of tests\nfrom t7407 which failed, and otherwise were passing with \"$@\".\n\nSome additional changes are also mode which weren't suggested.\n\nAdded the flag PARSE_OPT_KEEP_UNKNOWN in the parameters of\nparse_options to ignore any option passed as a part of \nand not as an option for git-submodule foreach.\n\nPreviously, display path showed incorrect output when the command\nwas executed from with a subdirectory. This occurred due to an incorrect\norder of parameter being passed and hence has been corrected.\n\nAdditional env variable $sm_path was added, since it was used in\ntest '\"submodule foreach\" from subdirectory' in t7407.\nI preferred adding this, instead of changing the test case, since\nin the case of git-submodule.sh, this env variable was accessible.\n\nI checked-out the commit 1c4fb136db (submodule foreach: skip eval for \nmore than one argument, 2013-09-27), which explains that why for \nthe case when argc>1, we do not use eval. But since, we are calling the\ncommand in a separate shell itself for all values of argc, hence IMO,\nthis case need not be considered separately.\n\nThe build report of this patch is available at: \ntravis-ci.org/pratham-pc/git/builds/224744614 \n \nThere are still 5 tests which are failing. I have submitted this\nas RFC patch for getting suggestions on debugging these\nerrors and for reviewing the approach taken for porting submodule\n'foreach' subcommand to C.\n \nI have observed that when we recursively run a command foreach\nsubmodule from a subdirectory, the $path variable as finally obtained\nby this patch differs with the $path variable as observed by the \nexisting git-submodule code for a nested submodule.\n\nI'll again mention that I have based my branch on\ngitster/jk/ls-files-recurse-submodules-fix, since while\nusing --super-prefix in recursively calling the foreach\ncommand, it produced results indicating that a --super-prefix can't\nbe used from a subdirectory:\n\n fatal: can't use --super-prefix from a subdirectory\n\nThe patch and the discussion related to it can be found at: \npublic-inbox.org/git/20170412003911.1142-1-jacob.e.keller@intel.com/T/#u\n\nAlso, in the function foreach_submodule, we call gitmodules_config()\nto read values from the worktree .gitmodules and then look up\nthe information (in this case only the sub->name) by using\nsubmodule_from_path funciton. Since we don't want to\noverwrite the null_sha1 entry, only loads from .gitmodules\nand avoid overlaying with .git/config.\n(also, since this whole process is required only to get the value\nog submodule's name, is there some other way by which we may obtain\nthe value so as to avoid this step?)\n\nAs currently finally exams are going on in my college, I was unable to\nwork on it much and the submission got delayed.\n\n\n builtin/submodule--helper.c | 130 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 40 +-------------\n 2 files changed, 131 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 85aafe46a..fba59c495 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -487,6 +487,135 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void foreach_submodule(int argc, const char **argv, const char *path,\n+\t\t\t const unsigned char *sha1, const char *prefix,\n+\t\t\t int quiet, int recursive)\n+{\n+\tconst char *toplevel = xgetcwd();\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf sub_sha1 = STRBUF_INIT;\n+\tstruct strbuf file = STRBUF_INIT;\n+\tchar* displaypath = NULL;\n+\tint i;\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+\t} else if (prefix) {\n+\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n+\t} else 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+\t}\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\tstrbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);\n+\n+\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+\targv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n+\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n+\targv_array_pushf(&cp.env_array, \"sha1=%s\", sub_sha1.buf);\n+\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+\tcp.use_shell = 1;\n+\tcp.dir = path;\n+\n+\tfor (i = 0; i < argc; i++)\n+\t\targv_array_push(&cp.args, argv[i]);\n+\n+\tstrbuf_addstr(&file, path);\n+\tstrbuf_addstr(&file, \"/.git\");\n+\n+\tif (!quiet && !access_or_warn(file.buf, R_OK, 0))\n+\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\n+\tif (!access_or_warn(file.buf, R_OK, 0))\n+\t\trun_command(&cp);\n+\n+\tif(recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.use_shell = 1;\n+\t\tcpr.dir = path;\n+\n+\t\targv_array_pushf(&cpr.args, \"git\");\n+\t\targv_array_pushf(&cpr.args, \"--super-prefix\");\n+\t\targv_array_push(&cpr.args, displaypath);\n+\t\targv_array_pushf(&cpr.args, \"submodule--helper\");\n+\n+\t\tif (quiet)\n+\t\t\targv_array_pushf(&cpr.args, \"--quiet\");\n+\n+\t\targv_array_pushf(&cpr.args, \"foreach\");\n+\t\targv_array_pushf(&cpr.args, \"--recursive\");\n+\n+\t\tfor (i = 0; i < argc; i++)\n+\t\t\targv_array_push(&cpr.args, argv[i]);\n+\n+\t\trun_command(&cpr);\n+\t}\n+\n+\tstrbuf_release(&file);\n+\tstrbuf_release(&sub_sha1);\n+\tstrbuf_release(&sb);\n+\tfree(displaypath);\n+\n+\treturn;\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\tint i;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t 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--helper [--quiet] foreach [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n+\t\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n+\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\tif (prefix) {\n+\t\t\tconst char *out = NULL;\n+\t\t\tif (skip_prefix(prefix, list.entries[i]->name, &out)) {\n+\t\t\t\tif (out && out[0] == '/' && !out + 1)\n+\t\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\n+\t\tforeach_submodule(argc, argv, list.entries[i]->name,\n+\t\t\t\t list.entries[i]->oid.hash, prefix,\n+\t\t\t\t quiet, recursive);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1168,6 +1297,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\n \t{\"init\", module_init, SUPPORT_SUPER_PREFIX},\n \t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 6ec35e5fc..e2c2b40f4 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -321,46 +321,8 @@ cmd_foreach()\n \t\tesac\n \t\tshift\n \tdone\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\ttoplevel=$(pwd)\n-\n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\n-\n-\t{\n-\t\tgit submodule--helper list --prefix \"$wt_prefix\" ||\n-\t\techo \"#unmatched\" $?\n-\t} |\n-\twhile read mode sha1 stage sm_path\n-\tdo\n-\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n-\t\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.11.0\n\n"},{"id":"317629","messageId":"xmqqfugy35lv.fsf@gitster.mtv.corp.google.com","threadId":"45748","inReplyTo":"20170422195804.18477-1-pc44800@gmail.com","subject":"Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-24T02:24:28Z","receivedAt":"2017-04-24T02:24:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> +static void foreach_submodule(int argc, const char **argv, const char *path,\n> +\t\t\t const unsigned char *sha1, const char *prefix,\n> +\t\t\t int quiet, int recursive)\n> +{\n\nI think that a reader would expect that a function whose name is\nforeach_something() to iterate over some collection and do something\non each of them, but this is not. It is \"do something on one thing\"\npart in a loop that exists elsewhere.\n\nDo not call such a helper \"foreach_\".\n\nWould it make more sense to use the \"struct object_id\" (instead of\nuchar[40]) interface for new functions like this?\n\n> +\tconst char *toplevel = xgetcwd();\n> +\tconst struct submodule *sub;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf sub_sha1 = STRBUF_INIT;\n> +\tstruct strbuf file = STRBUF_INIT;\n> +\tchar* displaypath = NULL;\n> +\tint i;\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> +\t} else if (prefix) {\n> +\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n\nAn extra SP after the last comma?\n\n> +\t} else 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> +\t}\n> +\n> +\tsub = submodule_from_path(null_sha1, path);\n> +\n> +\tif (!sub)\n> +\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> +\t\t displaypath);\n> +\tstrbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);\n\nWhy add-unique-abbrev if we do not want to abbreviate at all (with\nhardcoded constant 40)? IOW, shouldn't this be\n\n\tstrbuf_addstr(&sub_sha1, sha1_to_hex(sha1));\n\n> +\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> +\targv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n> +\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n\nWhy are we sending a value that will always be the same in two\nvariables? \"git submodule --help\" seems to list only name, path,\nsha1 and toplevel variables, and not sm_path. Is it a documentation\nbug, or are we clobbering a variable that the end user may be using\nfor other purposes in her foreach script?\n\n> +\targv_array_pushf(&cp.env_array, \"sha1=%s\", sub_sha1.buf);\n> +\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n> +\n> +\tcp.use_shell = 1;\n> +\tcp.dir = path;\n> +\n> +\tfor (i = 0; i < argc; i++)\n> +\t\targv_array_push(&cp.args, argv[i]);\n> +\n> +\tstrbuf_addstr(&file, path);\n> +\tstrbuf_addstr(&file, \"/.git\");\n> +\n> +\tif (!quiet && !access_or_warn(file.buf, R_OK, 0))\n> +\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n> +\n> +\tif (!access_or_warn(file.buf, R_OK, 0))\n> +\t\trun_command(&cp);\n> +\n> +\tif(recursive) {\n\nMissing SP after \"if\".\n\nMore importantly, if the earlier access-or-warn failed and we didn't\ndo the run-command for the path, does it even make sense to recurse\ninto it?\n\nThe original scripted version seems to refrain from recursing into\nit if the command fails in the submodule in the first place. This\ncode discards the exit status from run_command(), which looks wrong.\n\n> +\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n> +\n> +\t\tcpr.use_shell = 1;\n> +\t\tcpr.dir = path;\n> +\n> +\t\targv_array_pushf(&cpr.args, \"git\");\n> +\t\targv_array_pushf(&cpr.args, \"--super-prefix\");\n> +\t\targv_array_push(&cpr.args, displaypath);\n> +\t\targv_array_pushf(&cpr.args, \"submodule--helper\");\n> +\n> +\t\tif (quiet)\n> +\t\t\targv_array_pushf(&cpr.args, \"--quiet\");\n> +\n> +\t\targv_array_pushf(&cpr.args, \"foreach\");\n> +\t\targv_array_pushf(&cpr.args, \"--recursive\");\n> +\n> +\t\tfor (i = 0; i < argc; i++)\n> +\t\t\targv_array_push(&cpr.args, argv[i]);\n> +\n> +\t\trun_command(&cpr);\n\nSimilarly, the exit status of this invocation is discarded, which\nprobably is wrong. The original seems to pay attention to the\nfailure from the command invoked via \"foreach --recursive\" and stops\ninteration when it sees a failure.\n\n> +static int module_foreach(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct pathspec pathspec;\n> +\tstruct module_list list = MODULE_LIST_INIT;\n> +\tint quiet = 0;\n> +\tint recursive = 0;\n> +\tint i;\n> +\n> +\tstruct option module_foreach_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n> +\t\tOPT_BOOL(0, \"recursive\", &recursive,\n> +\t\t\t 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--helper [--quiet] foreach [--recursive] \"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_foreach_options,\n> +\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n\n> +\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n> +\t\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n\nThis line would fit within 80-col if it weren't overly indented, I\nwould think.\n\nOne suggestion.\n\nStart by updating \"git submodule foreach [--recursive]\" tests in t/\ndirectory before writing any more C code. Arrange to have two\nsubmodules, have your custom command that is run with foreach fail\nin the first submodule, and write a test that documents the\nbehaviour of the scripted version (e.g. your custom command should\nnot be even run inside the second submodule). Add a similar test\nwith two submodules, first of which has a nested submodule, and have\nyour custom command fail in one of these three places, and document\nthe behaviour of the scripted version (e.g. your custom command will\nrun for the first submodule, and if it fails there, does not run in\nthe nested submodule or in the second submodule; if your custom\ncommand succeeds in the first submodule, it goes to its nested\nsubmodule, and if your custom command fails there, the second\nsubmodule will not visited, etc.).\n\nAlso if it is not done in the existing tests, perhaps writing tests\nor two with submodules whose names are \"submodule 1\", \"submodule 2\",\netc. and making sure that there is no funny splitting of the arguments\nwould be a healthy thing to do.\n\nAfter getting that working, then start writing the above C code.\nThat would catch cases where your \"rewrite in C\" accidentally\nintroduces regressions.\n\nThanks.\n"},{"id":"317714","messageId":"CAGZ79kb1CR3qKOzByFC_wy7+Fh7cofFT1urhA06RuBK_3vGKmg@mail.gmail.com","threadId":"45748","inReplyTo":"20170422195804.18477-1-pc44800@gmail.com","subject":"Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-24T20:03:11Z","receivedAt":"2017-04-24T20:03:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> The First function module_foreach first parses the options present in\n\ns/First/first/\n\n> The second function also takes care of the recursive flag, by creating\n> a saperate child_process structure and prepending \"--super-prefix displaypath\",\n\n s/saperate/separate\n\n> Additional env variable $sm_path was added, since it was used in\n> test '\"submodule foreach\" from subdirectory' in t7407.\n> I preferred adding this, instead of changing the test case, since\n> in the case of git-submodule.sh, this env variable was accessible.\n\nMakes sense. Though this explanation could go into the commit message as well.\n\n> I checked-out the commit 1c4fb136db (submodule foreach: skip eval for\n> more than one argument, 2013-09-27), which explains that why for\n> the case when argc>1, we do not use eval. But since, we are calling the\n> command in a separate shell itself for all values of argc, hence IMO,\n> this case need not be considered separately.\n\nMakes sense. Though this explanation could go into the commit message as well.\n\n> I have observed that when we recursively run a command foreach\n> submodule from a subdirectory, the $path variable as finally obtained\n> by this patch differs with the $path variable as observed by the\n> existing git-submodule code for a nested submodule.\n\nUh, then the recuring is still a bit broken.\n\n> I'll again mention that I have based my branch on\n> gitster/jk/ls-files-recurse-submodules-fix, since while\n> using --super-prefix in recursively calling the foreach\n\nOops, missed that part. (Later in the reply I hint at the patch not\napplying correctly, disregard that part.)\n\n> Also, in the function foreach_submodule, we call gitmodules_config()\n> to read values from the worktree .gitmodules and then look up\n> the information (in this case only the sub->name) by using\n> submodule_from_path funciton. Since we don't want to\n> overwrite the null_sha1 entry, only loads from .gitmodules\n> and avoid overlaying with .git/config.\n\nThe null_sha1 entry is there nevertheless.\n\n> (also, since this whole process is required only to get the value\n> og submodule's name, is there some other way by which we may obtain\n> the value so as to avoid this step?)\n\nUnfortunately this is the only way to get the name. But that's right, for\njust getting the name we do not need the .git/config overlay as that\nis not supposed to overwrite name/path mappings.\n\n> As currently finally exams are going on in my college, I was unable to\n> work on it much and the submission got delayed.\n\nGood luck with the finals. :)\n\n+\n+ /* Only loads from .gitmodules, no overlay with .git/config */\n+ gitmodules_config();\n\nWe should do the config loading/parsing not\nin the inner looping function, but just once outside.\n\n+\n+ argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+ argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n+ argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n\nYou mention keeping 'sm_path' in the notes after the commit message. I would\nadd that part to the commit message, to explain why we have multiple variables\nthat have the same value. Maybe even a comment in the code:\n\n /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */\n .. sm_path ..\n\n+ argv_array_pushf(&cp.env_array, \"sha1=%s\", sub_sha1.buf);\n+ argv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+ cp.use_shell = 1;\n+ cp.dir = path;\n+\n+ for (i = 0; i < argc; i++)\n+ argv_array_push(&cp.args, argv[i]);\n+\n+ strbuf_addstr(&file, path);\n+ strbuf_addstr(&file, \"/.git\");\n\nThis could be oneline as:\n strbuf_addf(&file, \"%s/.git\", path);\n+\n+ if (!quiet && !access_or_warn(file.buf, R_OK, 0))\n+ printf(_(\"Entering '%s'\\n\"), displaypath);\n+\n+ if (!access_or_warn(file.buf, R_OK, 0))\n\nI do not think we'd want to warn about a missing .git file (which is what the\nor_warn part does).\n\nInstead I'd rather use the abstract function 'is_submodule_populated_gently'.\n\n+ run_command(&cp);\n+\n+ if(recursive) {\n+ struct child_process cpr = CHILD_PROCESS_INIT;\n+\n+ cpr.use_shell = 1;\n+ cpr.dir = path;\n+\n+ argv_array_pushf(&cpr.args, \"git\");\n+ argv_array_pushf(&cpr.args, \"--super-prefix\");\n+ argv_array_push(&cpr.args, displaypath);\n+ argv_array_pushf(&cpr.args, \"submodule--helper\");\n\nNo need for the 'f'ormat version of pusing strings if there is no formatting\ngoing on. An alternative would be to push them all at once:\n\n argv_array_pushl(&cpr.args, \"git\", \"--super-prefix\", displaypath,\n submodule--helper, NULL);\n\n(The 'l' version of pushing accepts an arbitrary number of arguments until the\nfinal NULL)\n\n\n+ if (quiet)\n+ argv_array_pushf(&cpr.args, \"--quiet\");\n+\n+ argv_array_pushf(&cpr.args, \"foreach\");\n+ argv_array_pushf(&cpr.args, \"--recursive\");\n\n\n+\n+ for (i = 0; i < argc; i++)\n+ argv_array_push(&cpr.args, argv[i]);\n+\n+ run_command(&cpr);\n\nI'd think we'd need to react to the exit code here (and above), and even\nstop early just as the shell version does. (To find out how it does so,\nI used gitk, and followed the error line\n die \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned\nnon-zero status.\")\"\nand right clicked on that line \"Show origin of that line\". There is a lot of\nnoise going on, e.g. i18n, path issues being fixed, and such, until you'd find\n19a31f9c1a (git-submodule - Add 'foreach' subcommand, 2008-08-10), which\noutlines more clearly its basic functionality:\n\n cmd_foreach()\n {\n git ls-files --stage | grep '^160000 ' |\n while read mode sha1 stage path\n do\n if test -e \"$path\"/.git\n then\n say \"Entering '$path'\"\n (cd \"$path\" && eval \"$@\") ||\n die \"Stopping at '$path'; script returned non-zero status.\"\n fi\n done\n }\n\nNote the && || operators, that act as\nhttps://en.wikipedia.org/wiki/Short-circuit_evaluation\n\n\nI do not think we would need the following block of code:\n\n+ if (prefix) {\n+ const char *out = NULL;\n+ if (skip_prefix(prefix, list.entries[i]->name, &out)) {\n+ if (out && out[0] == '/' && !out + 1)\n+ return 0;\n+ }\n+ }\n\nbecause 'module_list_compute' was given the 'prefix', such that the list only\ncontains list submodules within the 'prefix' area. However with the early\nreturn inside, I wonder what this code is about? (If this is not about\nscoping to the directory with prefix, maybe a comment could clarify?)\n\n+ foreach_submodule(argc, argv, list.entries[i]->name,\n+ list.entries[i]->oid.hash, prefix,\n+ quiet, recursive);\n\nAs Junio suggests, the name of foreach_submodule can be improved.\nAlso as I think further about it, we may actually want to have 3\n(instead of 2) functions:\n\n* The outer-most function is what we have here except the for-loop.\n (i.e. it is the front-end function for \"submodule--helper foreach\")\n* the actual looping function, as that can be re-used later from within\n C, then we would not need to spawn an extra submodule--helper, but\n could directly call the\n foreach_submodule(, , )\n* The inner-most function is what is currently named 'foreach_submodule'\n it \"does one thing\", in this case runa process in each submodule.\n\nAn example of such an approach is seen with the functions:\n 'for_each_string_list(list, string_list_each_func_t, cb_data);'\n (not the macro ending in _item)\n 'for_each_*' as found in cache.h\n\n int for_each_string_list(struct string_list *list,\n string_list_each_func_t fn, void *cb_data)\n {\n int i, ret = 0;\n for (i = 0; i < list->nr; i++)\n if ((ret = fn(&list->items[i], cb_data)))\n break;\n return ret;\n }\n\nThat way we'd have a nice helper function in the future to iterate\nover submodules with a given list.\n\n--\n\nThe following hunk changed in upstream, which is why I could not apply\nthe patch.\nThe colliding commit is cf9e55f494 (submodule: prevent backslash expantion in\nsubmodule names, 2017-04-07), which added just a \"-r\" to the \"while\nread mode...\"\n\nSo if you'd rebase your work on top of the latest master, this will produce\na conflict as well for you.\n\n+ git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix\n\"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet}\n${recursive:+--recursive} \"$@\"\n\n- toplevel=$(pwd)\n-\n- # dup stdin so that it can be restored when running the external\n- # command in the subshell (and a recursive call to this function)\n- exec 3<&0\n-\n..\n\nThanks,\nStefan\n"},{"id":"317728","messageId":"efe8e82d-a021-5fc4-492a-e6e0ab7d52d3@ramsayjones.plus.com","threadId":"45748","inReplyTo":"CAGZ79kb1CR3qKOzByFC_wy7+Fh7cofFT1urhA06RuBK_3vGKmg@mail.gmail.com","subject":"Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-04-24T22:11:55Z","receivedAt":"2017-04-24T22:12:02Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 24/04/17 21:03, Stefan Beller wrote:\n[snip]\n\n> +\n> + argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> + argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n> + argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> \n> You mention keeping 'sm_path' in the notes after the commit message. I would\n> add that part to the commit message, to explain why we have multiple variables\n> that have the same value. Maybe even a comment in the code:\n> \n> /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */\n> .. sm_path ..\n\nHmm, you need to be a bit careful with putting 'path' in the\nenvironment (if you then export it to sub-processes) on windows\n(cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked\nto remove $path altogether from the 'submodule-foreach api' in\nthat commit, but users and their scripts were already using it\n(so I couldn't just drop it, without some deprecation period).\nSo long as whatever was being 'eval'-ed in the script didn't\nexport $path, ...\n\nATB,\nRamsay Jones\n\n"},{"id":"317730","messageId":"CAGZ79kbU4LcRFHh5dM7i+L5DKpxKSrK-mZdYEeScrzVnYZsJmg@mail.gmail.com","threadId":"45748","inReplyTo":"efe8e82d-a021-5fc4-492a-e6e0ab7d52d3@ramsayjones.plus.com","subject":"Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-24T22:17:24Z","receivedAt":"2017-04-24T22:17:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 24, 2017 at 3:11 PM, Ramsay Jones\n wrote:\n>\n>\n> On 24/04/17 21:03, Stefan Beller wrote:\n> [snip]\n>\n>> +\n>> + argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n>> + argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n>> + argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n>>\n>> You mention keeping 'sm_path' in the notes after the commit message. I would\n>> add that part to the commit message, to explain why we have multiple variables\n>> that have the same value. Maybe even a comment in the code:\n>>\n>> /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */\n>> .. sm_path ..\n>\n> Hmm, you need to be a bit careful with putting 'path' in the\n> environment (if you then export it to sub-processes) on windows\n> (cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked\n> to remove $path altogether from the 'submodule-foreach api' in\n> that commit, but users and their scripts were already using it\n> (so I couldn't just drop it, without some deprecation period).\n> So long as whatever was being 'eval'-ed in the script didn't\n> export $path, ...\n>\n\nOh, I misread the comment\n\n # we make $path available to scripts ...\n path=$sm_path\n\nas it was such a casual friendly thing to say in that context.\nSo the *real* historic baggage is\n argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\nwhereas\n argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\nis considered the correct way to go.\n\nThanks,\nStefan\n"},{"id":"317732","messageId":"c3b724d8-bb9b-b63b-0105-8c1e22b120be@ramsayjones.plus.com","threadId":"45748","inReplyTo":"CAGZ79kbU4LcRFHh5dM7i+L5DKpxKSrK-mZdYEeScrzVnYZsJmg@mail.gmail.com","subject":"Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-04-24T22:43:51Z","receivedAt":"2017-04-24T22:44:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 24/04/17 23:17, Stefan Beller wrote:\n> On Mon, Apr 24, 2017 at 3:11 PM, Ramsay Jones\n> wrote:\n>>\n>>\n>> On 24/04/17 21:03, Stefan Beller wrote:\n>> [snip]\n>>\n>>> +\n>>> + argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n>>> + argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n>>> + argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n>>>\n>>> You mention keeping 'sm_path' in the notes after the commit message. I would\n>>> add that part to the commit message, to explain why we have multiple variables\n>>> that have the same value. Maybe even a comment in the code:\n>>>\n>>> /* Keep sm_path for historic reasons, see tests in 091a6eb0fee. */\n>>> .. sm_path ..\n>>\n>> Hmm, you need to be a bit careful with putting 'path' in the\n>> environment (if you then export it to sub-processes) on windows\n>> (cygwin, MinGW, GfW). See commit 64394e3ae9. I would have liked\n>> to remove $path altogether from the 'submodule-foreach api' in\n>> that commit, but users and their scripts were already using it\n>> (so I couldn't just drop it, without some deprecation period).\n>> So long as whatever was being 'eval'-ed in the script didn't\n>> export $path, ...\n>>\n> \n> Oh, I misread the comment\n> \n> # we make $path available to scripts ...\n> path=$sm_path\n> \n> as it was such a casual friendly thing to say in that context.\n> So the *real* historic baggage is\n> argv_array_pushf(&cp.env_array, \"path=%s\", displaypath);\n> whereas\n> argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> is considered the correct way to go.\n\nI have to admit that I didn't actually read the code in this\npatch. I just saw the subject line and the ass-backward comment\nabout $sm_path. ;-)\n\nMy intention was simply to warn: 'there be dragons'.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"319542","messageId":"20170512114404.10008-2-pc44800@gmail.com","threadId":"45748","inReplyTo":"20170512114404.10008-1-pc44800@gmail.com","subject":"[GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-12T11:44:04Z","receivedAt":"2017-05-12T18:45:42Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule foreach a builtin. This is the very\nfirst step taken in this direction. Hence, 'foreach' is ported to\nsubmodule--helper, and submodule--helper is called from git-submodule.sh.\nThe code is split up to have one function to obtain all the list of\nsubmodules. This function acts as the front-end of git-submodule foreach\nsubcommand. It calls the function for_each_submodule_list, which basically\nloops through the list and calls function fn, which in this case is\nruncommand_in_submodule. This third function is a calling function that\ntakes care of running the command in that submodule, and recursively\nperform the same when --recursive is flagged.\n\nThe first function module_foreach first parses the options present in\nargv, and 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\nlist, and calls function fn (which in case of submodule subcommand\nforeach is runcommand_in_submodule) is called for each entry.\n\nThe third function runcommand_in_submodule, generates a submodule struct sub\nfor $name value and then later prepends name=sub->name and other\nvalue assignments to the env argv_array structure of a child_process.\nAlso the of submodule-foreach is push to args argv_array\nstructure and finally, using run_command the commands are executed\nusing a shell.\n\nThe third function also takes care of the recursive flag, by creating\na separate child_process structure and prepending \"--super-prefix displaypath\",\nto the args argv_array structure. Other required arguments and the\ninput of submodule-foreach is also appended to this argv_array.\n\nThe commit 1c4fb136db (submodule foreach: skip eval for more than one\nargument, 2013-09-27), which explains that why for the case when argc>1,\nwe do not use eval. But since in this patch, we are calling the\ncommand in a separate shell itself for all values of argc, this case\nis not considered separately.\n\nBoth env variable $path and $sm_path were added since both are used in\ntests in t7407.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n\nIn this new version of patch, following changes were added:\n\nforeach_submodule was renamed to runcommand_in_submodule\n\nWe module_foreach into two parts, such that, module foreach\ngenerates the list of submodules present in the cwd. And later\ncalls for_each_submodule_list which basically loops through the\nlist of submodule and calls the passed function fn.\n\nAdditionally, this patch also pass all those test, which it\nfailed earlier.\n\nSince in the run-command API errors out when child\nprocess with no arguments is passed, for test #10\nfrom t7407-submodule-foreach, additional condition\nwas added in runcommand_in_submodule funciton to\nhandle this case instead of modifying the run-command.\n\nComplete build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds/\nbranch: foreach\nbuild #52\n\nIt can been seen that the patch fails in test #9\nof t7407-submodule-foreach, which is the newly added\ntest to that suite. The main reason of adding this test\nwas to bring the behavior of $path for the submodule\nforeach --recursive case.\n\nThe observation made was as follows:\n\nFor a project - super containing dir (not a submodule)\nand a submodule sub which contains another submodule\nsubsub. When we run a command from super/dir:\n\ngit submodule foreach \"echo \\$path-\\$sm_path\"\n\nactual results:\nEntering '../sub'\n../sub-../sub\nEntering '../sub/subsub'\n../subsub-../subsub\n\nported function's result:\nEntering '../sub'\nsub-../sub\nEntering '../sub/subsub'\nsubsub-../sub/subsub\n\nThis is occurring since in cmd_foreach of git-submodule.sh\nwhen we use to recurse, we call cmd_foreach\nand hence the process ran in the same shell.\nBecause of this, the variable $wt_prefix is set only once\nwhich is at the beginning of the submodule foreach execution.\nwt_prefix=$(git rev-parse --show-prefix)\n\nAnd since sm_path and path are set using $wt_prefix as :\nsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\npath=$sm_path\nIt differs with the value of displaypath as well.\n\nThis make the value of $path confusing and I also feel it\ndeviates from its documentation:\n$path is the name of the submodule directory relative\nto the superproject.\n\nBut since in refactoring the code, we wish to maintain the\ncode in same way, we need to pass wt_prefix on every\nrecursive call, which may result in complex C code.\nAnother option could be to first correct the $path value\nin git-submodule.sh and then port the updated cmd_foreach.\n\n builtin/submodule--helper.c | 145 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 39 +-----------\n 2 files changed, 146 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..1a5b26c22 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -13,6 +13,10 @@\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+\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -331,6 +335,15 @@ 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+\treturn;\n+}\n+\n static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -487,6 +500,137 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cb_foreach {\n+\tint argc;\n+\tconst char **argv;\n+\tconst char *prefix;\n+\tint quiet;\n+\tint recursive;\n+};\n+\n+static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct cb_foreach *info = cb_data;\n+\tconst char *toplevel = xgetcwd();\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf sub_sha1 = STRBUF_INIT;\n+\tchar* displaypath = NULL;\n+\tint i;\n+\n+\t/* Only loads from .gitmodules, no overlay with .git/config */\n+\tgitmodules_config();\n+\n+\tif (info->prefix && get_super_prefix()) {\n+\t\tdie(\"BUG: cannot have prefix '%s' and superprefix '%s'\",\n+\t\t info->prefix, get_super_prefix());\n+\t} else if (info->prefix) {\n+\t\tdisplaypath = xstrdup(relative_path(list_item->name, info->prefix, &sb));\n+\t} else if (get_super_prefix()) {\n+\t\tstrbuf_addf(&sb, \"%s/%s\", get_super_prefix(), list_item->name);\n+\t\tdisplaypath = strbuf_detach(&sb, NULL);\n+\t} else {\n+\t\tdisplaypath = xstrdup(list_item->name);\n+\t}\n+\n+\tsub = submodule_from_path(null_sha1, list_item->name);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\n+\tstrbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));\n+\n+\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n+\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name); \n+\targv_array_pushf(&cp.env_array, \"sha1=%s\", sub_sha1.buf);\n+\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+\tcp.use_shell = 1;\n+\tcp.dir = list_item->name;\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\targv_array_pushf(&cp.env_array, \"%s\", GIT_SUPER_PREFIX_ENVIRONMENT);\n+\n+\tfor (i = 0; i < info->argc; i++)\n+\t\targv_array_push(&cp.args, info->argv[i]);\n+\n+\tif (!is_submodule_populated_gently(list_item->name, NULL))\n+\t\treturn;\n+\t\n+\tif (!info->quiet)\n+\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\tif (info->argv[0] && run_command(&cp))\n+\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"), displaypath);\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.use_shell = 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, \"git\", \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", NULL);\n+\n+\t\targv_array_pushl(&cpr.args, \"foreach\", \"--recursive\", NULL);\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tfor (i = 0; i < info->argc; i++)\n+\t\t\targv_array_push(&cpr.args, info->argv[i]);\n+\n+\t\trun_command(&cpr);\n+\t}\n+\n+\tstrbuf_release(&sub_sha1);\n+\tstrbuf_release(&sb);\n+\tfree(displaypath);\n+\n+\treturn;\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct cb_foreach info;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\tmemset(&info, 0, sizeof(info));\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n+\t\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n+\n+\tinfo.argc = argc;\n+\tinfo.argv = argv;\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = quiet;\n+\tinfo.recursive = recursive;\n+\n+\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1212,6 +1356,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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..032fd2540 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -322,45 +322,8 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\ttoplevel=$(pwd)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.11.0\n\n"},{"id":"319551","messageId":"20170512114404.10008-1-pc44800@gmail.com","threadId":"45748","inReplyTo":"c3b724d8-bb9b-b63b-0105-8c1e22b120be@ramsayjones.plus.com","subject":"[GSoC][RFC/PATCH v3 1/2] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-12T11:44:03Z","receivedAt":"2017-05-12T18:46:04Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Additional test cases added to the submodule-foreach test suite\nto check the submodule foreach --recursive behavior from a\nsubdirectory as this was missing from the test suite.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-\n 1 file changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42..58a890e31 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -79,7 +79,6 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \t) &&\n \ttest_i18ncmp expect actual\n '\n-\n cat >expect <expect <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect < It can been seen that the patch fails in test #9\n> of t7407-submodule-foreach, which is the newly added\n> test to that suite. The main reason of adding this test\n> was to bring the behavior of $path for the submodule\n> foreach --recursive case.\n>\n> The observation made was as follows:\n>\n> For a project - super containing dir (not a submodule)\n> and a submodule sub which contains another submodule\n> subsub. When we run a command from super/dir:\n>\n> git submodule foreach \"echo \\$path-\\$sm_path\"\n>\n> actual results:\n> Entering '../sub'\n> ../sub-../sub\n> Entering '../sub/subsub'\n> ../subsub-../subsub\n>\n> ported function's result:\n> Entering '../sub'\n> sub-../sub\n> Entering '../sub/subsub'\n> subsub-../sub/subsub\n>\n> This is occurring since in cmd_foreach of git-submodule.sh\n> when we use to recurse, we call cmd_foreach\n> and hence the process ran in the same shell.\n> Because of this, the variable $wt_prefix is set only once\n> which is at the beginning of the submodule foreach execution.\n> wt_prefix=$(git rev-parse --show-prefix)\n>\n> And since sm_path and path are set using $wt_prefix as :\n> sm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> path=$sm_path\n> It differs with the value of displaypath as well.\n>\n> This make the value of $path confusing and I also feel it\n> deviates from its documentation:\n> $path is the name of the submodule directory relative\n> to the superproject.\n>\n> But since in refactoring the code, we wish to maintain the\n> code in same way, we need to pass wt_prefix on every\n> recursive call, which may result in complex C code.\n> Another option could be to first correct the $path value\n> in git-submodule.sh and then port the updated cmd_foreach.\n>\n\nWe discussed this patch off list before and I think the code is fine,\ntwo minor nits below. But this observation is something we'd want\nto talk about here on list. Sorry for dropping the ball for 3 days.\n\nIf we do not apply patch 1, the test suite passes, as far as I observe\nthe situation and patch 1 introduces a test that passes the shell\nforeach, but breaks here, IIUC.\n\nSpecifically the $sm_path breaks the test in the sense that it is\nactually correct now for nested submodules when the command\nin the superproject is run from within a directory.\n\nI think this is a rare bug, that we can just fix along the way, i.e.\nmention in the commit message that we fix it and adapt the test\nsuch that it passes with the rewrite in C, here.\n\n> + argv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n\ngit-am claims there are trailing white spaces.\n$ git am \nApplying: submodule: port subcommand foreach from shell to C\n.git/rebase-apply/patch:158: trailing whitespace.\nargv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n.git/rebase-apply/patch:172: trailing whitespace.\nwarning: 2 lines add whitespace errors.\n\nNot sure which editor you use, but I could configure my editor to strip\ntrailing whitespaces on save, and had never any issues with them\nsince.\n\n> + struct cb_foreach info;\n..\n> +\n> + memset(&info, 0, sizeof(info));\n\nInstead of this memset, you could also have a #define CB_FOREACH_INIT\nsimilar to e.g. MODULE_LIST_INIT, STRBUF_INIT, STRING_LIST_INIT and so on.\nThis memset works, too.\n\nThanks,\nStefan\n"},{"id":"319814","messageId":"20170515183405.GA79147@google.com","threadId":"45748","inReplyTo":"20170512114404.10008-2-pc44800@gmail.com","subject":"Re: [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-15T18:34:05Z","receivedAt":"2017-05-15T18:34:14Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/12, Prathamesh Chavan wrote:\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n> The code is split up to have one function to obtain all the list of\n> submodules. This function acts as the front-end of git-submodule foreach\n> subcommand. It calls the function for_each_submodule_list, which basically\n> loops through the list and calls function fn, which in this case is\n> runcommand_in_submodule. This third function is a calling function that\n> takes care of running the command in that submodule, and recursively\n> perform the same when --recursive is flagged.\n> \n> The first function module_foreach first parses the options present in\n> argv, 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\n> list, and calls function fn (which in case of submodule subcommand\n> foreach is runcommand_in_submodule) is called for each entry.\n> \n> The third function runcommand_in_submodule, generates a submodule struct sub\n> for $name value and then later prepends name=sub->name and other\n> value assignments to the env argv_array structure of a child_process.\n> Also the of submodule-foreach is push to args argv_array\n> structure and finally, using run_command the commands are executed\n> using a shell.\n> \n> The third function also takes care of the recursive flag, by creating\n> a separate child_process structure and prepending \"--super-prefix displaypath\",\n> to the args argv_array structure. Other required arguments and the\n> input of submodule-foreach is also appended to this argv_array.\n> \n> The commit 1c4fb136db (submodule foreach: skip eval for more than one\n> argument, 2013-09-27), which explains that why for the case when argc>1,\n> we do not use eval. But since in this patch, we are calling the\n> command in a separate shell itself for all values of argc, this case\n> is not considered separately.\n> \n> Both env variable $path and $sm_path were added since both are used in\n> tests in t7407.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> \n> In this new version of patch, following changes were added:\n> \n> foreach_submodule was renamed to runcommand_in_submodule\n> \n> We module_foreach into two parts, such that, module foreach\n> generates the list of submodules present in the cwd. And later\n> calls for_each_submodule_list which basically loops through the\n> list of submodule and calls the passed function fn.\n> \n> Additionally, this patch also pass all those test, which it\n> failed earlier.\n> \n> Since in the run-command API errors out when child\n> process with no arguments is passed, for test #10\n> from t7407-submodule-foreach, additional condition\n> was added in runcommand_in_submodule funciton to\n> handle this case instead of modifying the run-command.\n> \n> Complete build report is available at:\n> https://travis-ci.org/pratham-pc/git/builds/\n> branch: foreach\n> build #52\n> \n> It can been seen that the patch fails in test #9\n> of t7407-submodule-foreach, which is the newly added\n> test to that suite. The main reason of adding this test\n> was to bring the behavior of $path for the submodule\n> foreach --recursive case.\n> \n> The observation made was as follows:\n> \n> For a project - super containing dir (not a submodule)\n> and a submodule sub which contains another submodule\n> subsub. When we run a command from super/dir:\n> \n> git submodule foreach \"echo \\$path-\\$sm_path\"\n> \n> actual results:\n> Entering '../sub'\n> ../sub-../sub\n> Entering '../sub/subsub'\n> ../subsub-../subsub\n> \n> ported function's result:\n> Entering '../sub'\n> sub-../sub\n> Entering '../sub/subsub'\n> subsub-../sub/subsub\n> \n> This is occurring since in cmd_foreach of git-submodule.sh\n> when we use to recurse, we call cmd_foreach\n> and hence the process ran in the same shell.\n> Because of this, the variable $wt_prefix is set only once\n> which is at the beginning of the submodule foreach execution.\n> wt_prefix=$(git rev-parse --show-prefix)\n> \n> And since sm_path and path are set using $wt_prefix as :\n> sm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> path=$sm_path\n> It differs with the value of displaypath as well.\n> \n> This make the value of $path confusing and I also feel it\n> deviates from its documentation:\n> $path is the name of the submodule directory relative\n> to the superproject.\n> \n> But since in refactoring the code, we wish to maintain the\n> code in same way, we need to pass wt_prefix on every\n> recursive call, which may result in complex C code.\n> Another option could be to first correct the $path value\n> in git-submodule.sh and then port the updated cmd_foreach.\n> \n> builtin/submodule--helper.c | 145 ++++++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 39 +-----------\n> 2 files changed, 146 insertions(+), 38 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 566a5b6a6..1a5b26c22 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -13,6 +13,10 @@\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> +\n> +\n\nnit: You could probably get rid of 2 of these empty lines.\n\n> static char *get_default_remote(void)\n> {\n> \tchar *dest = NULL, *ret;\n> @@ -331,6 +335,15 @@ 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> +\treturn;\n> +}\n> +\n> static void init_submodule(const char *path, const char *prefix, int quiet)\n> {\n> \tconst struct submodule *sub;\n> @@ -487,6 +500,137 @@ static int module_name(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +struct cb_foreach {\n> +\tint argc;\n> +\tconst char **argv;\n> +\tconst char *prefix;\n> +\tint quiet;\n> +\tint recursive;\n> +};\n> +\n> +static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)\n> +{\n> +\tstruct cb_foreach *info = cb_data;\n> +\tconst char *toplevel = xgetcwd();\n\nxgetcwd() returns a 'char *' so you own the memory for the string that\nwas returned. This declaration should be changed to 'char *' and be\nfreed at the end before returning from this function.\n\n> +\tconst struct submodule *sub;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf sub_sha1 = STRBUF_INIT;\n> +\tchar* displaypath = NULL;\n> +\tint i;\n> +\n> +\t/* Only loads from .gitmodules, no overlay with .git/config */\n> +\tgitmodules_config();\n> +\n> +\tif (info->prefix && get_super_prefix()) {\n> +\t\tdie(\"BUG: cannot have prefix '%s' and superprefix '%s'\",\n> +\t\t info->prefix, get_super_prefix());\n> +\t} else if (info->prefix) {\n> +\t\tdisplaypath = xstrdup(relative_path(list_item->name, info->prefix, &sb));\n\nI would move the declaration of the strbuf to be inside this if block\nsince its not really used else where (except in the next if block but\nI'll get to that).\n\n> +\t} else if (get_super_prefix()) {\n> +\t\tstrbuf_addf(&sb, \"%s/%s\", get_super_prefix(), list_item->name);\n> +\t\tdisplaypath = strbuf_detach(&sb, NULL);\n\nYou don't really need to use a strbuf for this, you could just use\nxfmtstr() instead and avoid needing to call detach. Also with the\nnumber of times you call 'get_super_prefix()' it may be worth while to\nhave a local 'const char *super_prefix' which you assign a single time\nand reuse so that you don't waste time calling that function over and\nover again.\n\n> +\t} else {\n> +\t\tdisplaypath = xstrdup(list_item->name);\n> +\t}\n> +\n> +\tsub = submodule_from_path(null_sha1, list_item->name);\n> +\n> +\tif (!sub)\n> +\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> +\t\t displaypath);\n> +\n> +\tstrbuf_addstr(&sub_sha1, oid_to_hex(&list_item->oid));\n> +\n> +\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> +\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> +\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name); \n\nTrailing white space ^ here\n\n> +\targv_array_pushf(&cp.env_array, \"sha1=%s\", sub_sha1.buf);\n\nYou can remove the 'sub_sha1' strbuf. Since you are just using it this\nonce it makes more sense just substitute 'sub_sha1.buf' for the call to\n'oid_to_hex()'\n\n> +\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n> +\n> +\tcp.use_shell = 1;\n> +\tcp.dir = list_item->name;\n> +\tprepare_submodule_repo_env(&cp.env_array);\n\nI would probably hoist this call up before you start setting 'name',\n'sm_path', 'path', etc. That way you are certain that the call to\nprepare_submodule_repo_env() doesn't mess with anything you've already\nset. Now in this context it doesn't and you would be fine but this is\njust to future proof any changes to that function.\n\n> +\targv_array_pushf(&cp.env_array, \"%s\", GIT_SUPER_PREFIX_ENVIRONMENT);\n\nI assume you are doing this to ensure that the super prefix environment\nvar is cleared in the child process? Though you shouldn't have to\nbecause this env var is covered in 'local_repo_env' and should already\nbe taken care of my the call to 'prepare_submodule_repo_env()'\n\n> +\n> +\tfor (i = 0; i < info->argc; i++)\n> +\t\targv_array_push(&cp.args, info->argv[i]);\n> +\n> +\tif (!is_submodule_populated_gently(list_item->name, NULL))\n> +\t\treturn;\n> +\t\n\nAnd trailing white space here ^\n\n> +\tif (!info->quiet)\n> +\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n> +\tif (info->argv[0] && run_command(&cp))\n> +\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"), displaypath);\n> +\n> +\tif (info->recursive) {\n> +\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n\nYou don't need to use a new child_process struct here, you could reuse\n'cp' since the call to run_command will free all the memory at the end\nof the call....oh but you're not unconditionally running run_command\nbecause of the check to argv[0]...\n\nMaybe we want to do an early return if argv[0] is empty? Not sure how\nthat would break tests though...\n\n> +\n> +\t\tcpr.use_shell = 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, \"git\", \"--super-prefix\", displaypath,\n> +\t\t\t\t \"submodule--helper\", NULL);\n> +\n> +\t\targv_array_pushl(&cpr.args, \"foreach\", \"--recursive\", NULL);\n\nsince you're already using pushl, just combine these two function calls.\n\n> +\n> +\t\tif (info->quiet)\n> +\t\t\targv_array_push(&cpr.args, \"--quiet\");\n> +\n> +\t\tfor (i = 0; i < info->argc; i++)\n> +\t\t\targv_array_push(&cpr.args, info->argv[i]);\n> +\n> +\t\trun_command(&cpr);\n\nAre you not going to do anything if the exit status was non-zero?\n\n> +\t}\n> +\n> +\tstrbuf_release(&sub_sha1);\n> +\tstrbuf_release(&sb);\n> +\tfree(displaypath);\n> +\n> +\treturn;\n\nYou don't need this return here.\n\n> +}\n> +\n> +static int module_foreach(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct cb_foreach info;\n> +\tstruct pathspec pathspec;\n> +\tstruct module_list list = MODULE_LIST_INIT;\n> +\tint quiet = 0;\n> +\tint recursive = 0;\n> +\n> +\tstruct option module_foreach_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n> +\t\tOPT_BOOL(0, \"recursive\", &recursive,\n> +\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n> +\t\tNULL\n> +\t};\n> +\n> +\tmemset(&info, 0, sizeof(info));\n> +\n> +\targc = parse_options(argc, argv, prefix, module_foreach_options,\n> +\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n> +\n> +\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n> +\t\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n\nnit: Indention looks off here.\n\n> +\n> +\tinfo.argc = argc;\n> +\tinfo.argv = argv;\n> +\tinfo.prefix = prefix;\n> +\tinfo.quiet = quiet;\n> +\tinfo.recursive = recursive;\n> +\n> +\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> \t\t\t const char *depth, struct string_list *reference,\n> \t\t\t int quiet, int progress)\n> @@ -1212,6 +1356,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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..032fd2540 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -322,45 +322,8 @@ cmd_foreach()\n> \t\tshift\n> \tdone\n> \n> -\ttoplevel=$(pwd)\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n> \n> -\t# dup stdin so that it can be restored when running the external\n> -\t# command in the subshell (and a recursive call to this function)\n> -\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n> -\t\tthen\n> -\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n> -\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n> -\t\t\tname=$(git submodule--helper name \"$sm_path\")\n> -\t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> -\t\t\t\tsanitize_submodule_env\n> -\t\t\t\tcd \"$sm_path\" &&\n> -\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> -\t\t\t\t# we make $path available to scripts ...\n> -\t\t\t\tpath=$sm_path &&\n> -\t\t\t\tif test $# -eq 1\n> -\t\t\t\tthen\n> -\t\t\t\t\teval \"$1\"\n> -\t\t\t\telse\n> -\t\t\t\t\t\"$@\"\n> -\t\t\t\tfi &&\n> -\t\t\t\tif test -n \"$recursive\"\n> -\t\t\t\tthen\n> -\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n> -\t\t\t\tfi\n> -\t\t\t) <&3 3<&- ||\n> -\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n> -\t\tfi\n> -\tdone\n> }\n> \n> #\n> -- \n> 2.11.0\n> \n\n-- \nBrandon Williams\n"},{"id":"320373","messageId":"20170521125814.26255-1-pc44800@gmail.com","threadId":"45748","inReplyTo":"20170515183405.GA79147@google.com","subject":"[GSoC][PATCH v4 1/2] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-21T12:58:13Z","receivedAt":"2017-05-21T12:58:40Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Additional test cases added to the submodule-foreach test suite\nto check the submodule foreach --recursive behavior from a\nsubdirectory as this was missing from the test suite.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nIt was observed that after porting the submodule subcommand to\nC, it passed all the test from the existing test-suite.\nBut since there was some observation made, where the output of\nthe orignal submodule foreach subcommand wasn't matching to that\nof the newly ported function, this test has been added.\n\nAfter which, it can been seen that the patch fails in test #9\nof t7407-submodule-foreach, which is the newly added\ntest to that suite. The main reason of adding this test\nwas to bring the behavior of $path for the submodule\nforeach --recursive case.\n\nThe observation made was as follows:\n\nFor a project - super containing dir (not a submodule)\nand a submodule sub which contains another submodule\nsubsub. When we run a command from super/dir:\n\ngit submodule foreach \"echo \\$path-\\$sm_path\"\n\nactual results:\nEntering '../sub'\n../sub-../sub\nEntering '../sub/subsub'\n../subsub-../subsub\n\nported function's result:\nEntering '../sub'\nsub-../sub\nEntering '../sub/subsub'\nsubsub-../sub/subsub\n\nThis is occurring since in cmd_foreach of git-submodule.sh\nwhen we use to recurse, we call cmd_foreach\nand hence the process ran in the same shell.\nBecause of this, the variable $wt_prefix is set only once\nwhich is at the beginning of the submodule foreach execution.\nwt_prefix=$(git rev-parse --show-prefix)\n\nAnd since sm_path and path are set using $wt_prefix as :\nsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\npath=$sm_path\nIt differs with the value of displaypath as well.\n\nThis make the value of $path confusing and I also feel it\ndeviates from its documentation:\n$path is the name of the submodule directory relative\nto the superproject.\n\nBut since in refactoring the code, we wish to maintain the\ncode in same way, we need to pass wt_prefix on every\nrecursive call, which may result in complex C code.\nAnother option could be to first correct the $path value\nin git-submodule.sh and then port the updated cmd_foreach.\n\n t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-\n 1 file changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42..58a890e31 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -79,7 +79,6 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \t) &&\n \ttest_i18ncmp expect actual\n '\n-\n cat >expect <expect <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect <name; and other\nvalue assignment to the env argv_array structure of a child_process.\nAlso the of submodule-foreach is push to args argv_array\nstructure and finally, using run_command the commands are executed\nusing a shell.\n\nThe third function also takes care of the recursive flag, by creating\na separate child_process structure and prepending \"--super-prefix displaypath\",\nto the args argv_array structure. Other required arguments and the\ninput of submodule-foreach is also appended to this argv_array.\n\nThe commit 1c4fb136db (submodule foreach: skip eval for more than one\nargument, 2013-09-27), which explains that why for the case when argc>1,\nwe do not use eval. But since in this patch, we are calling the\ncommand in a separate shell itself for all values of argc, this case\nis not considered separately.\n\nBoth env variable $path and $sm_path were added since both are used in\ntests in t7407.\n\nHelped-by: Brandon Williams \nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nThis series of patch is based on gitster/jk/bug-to-abort for untilizing its\nBUG() macro.\n\nIn this new version of patch, a new function\nget_submodule_displaypath is introduced, which is the same one\nas that in the patch series for porting of submodule subcommand\nstatus. I had to again introduce this in this patch as well as\nI am working on two separate branches for parting of each function.\nAlso, the function for_each_submodule_list repeats for the same\nreason.\n\nI have pushed this work on Github at:\nhttps://github.com/pratham-pc/git/commits/foreach\n\nIts build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: foreach\nBuild #67\n\nI have also made some changes in git-submodule.sh for correcting\nthe $path variable. And hence made the corresponding changes in\nthe new test introduced in t7407-submodule-foreach as well.\nI have push this work at:\nhttps://github.com/pratham-pc/git/commits/foreach-bug-fixed\n\nIts build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds/\nBranch: foreach-bug-fixed\nBuild #66\n\n builtin/submodule--helper.c | 142 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 39 +-----------\n 2 files changed, 143 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..4e19beaff 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 struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -331,6 +350,15 @@ 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+\treturn;\n+}\n+\n static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -487,6 +515,119 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cb_foreach {\n+\tint argc;\n+\tconst char **argv;\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+};\n+#define CB_FOREACH_INIT { 0, NULL, 0, 0 }\n+\n+static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)\n+{\n+\tstruct cb_foreach *info = cb_data;\n+\tchar *toplevel = xgetcwd();\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tchar* displaypath = NULL;\n+\tint i;\n+\n+\t/* Only loads from .gitmodules, no overlay with .git/config */\n+\tgitmodules_config();\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tsub = submodule_from_path(null_sha1, list_item->name);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.use_shell = 1;\n+\tcp.dir = list_item->name;\n+\n+\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n+\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n+\targv_array_pushf(&cp.env_array, \"sha1=%s\", oid_to_hex(&list_item->oid));\n+\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+\tfor (i = 0; i < info->argc; i++)\n+\t\targv_array_push(&cp.args, info->argv[i]);\n+\n+\tif (!is_submodule_populated_gently(list_item->name, NULL))\n+\t\treturn;\n+\n+\tif (!info->quiet)\n+\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\tif (info->argv[0] && run_command(&cp))\n+\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"), displaypath);\n+\n+\tif (info->recursive) {\n+\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n+\n+\t\tcpr.use_shell = 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, \"git\", \"--super-prefix\", displaypath,\n+\t\t\t\t \"submodule--helper\", \"foreach\", \"--recursive\", NULL);\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tfor (i = 0; i < info->argc; i++)\n+\t\t\targv_array_push(&cpr.args, info->argv[i]);\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"run_command returned non-zero status while \\\n+\t\t\t recuring in the nested submodules of %s\\n.\"),\n+\t\t\t displaypath);\n+\t}\n+\n+\tfree(displaypath);\n+\tfree(toplevel);\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct cb_foreach info = CB_FOREACH_INIT;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n+\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n+\n+\tinfo.argc = argc;\n+\tinfo.argv = argv;\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\n+\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1212,6 +1353,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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..032fd2540 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -322,45 +322,8 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\ttoplevel=$(pwd)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.11.0\n\n"},{"id":"320477","messageId":"CAGZ79kbmQ5_Vb8BSoJkA74-+e0FoKwz=iJkSk4sdSnc46s+qUw@mail.gmail.com","threadId":"45748","inReplyTo":"20170521125814.26255-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-22T20:04:42Z","receivedAt":"2017-05-22T20:04:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan wrote:\n\n> I have also made some changes in git-submodule.sh for correcting\n> the $path variable. And hence made the corresponding changes in\n> the new test introduced in t7407-submodule-foreach as well.\n> I have push this work at:\n> https://github.com/pratham-pc/git/commits/foreach-bug-fixed\n\nThis one seems to pass the test suite by having the bug fixed.\n(The patches posted here seems to be\nhttps://github.com/pratham-pc/git/commits/foreach\nwhich does not pass tests? These two series seem to only differ in\nthe bug fix commit, which I think is a good idea to include, as then we\nhave a bug fixed and the tests pass.)\n\n> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)\n..\n> + return;\n\nno need for an explicit return in a void function.\n\n> +struct cb_foreach {\n> + int argc;\n> + const char **argv;\n> + const char *prefix;\n> + unsigned int quiet: 1;\n> + unsigned int recursive: 1;\n> +};\n> +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }\n\nThis static initializer doesn't quite match the struct,\n(I would expect two NULLs as we have two const char pointers).\n\n> +\n> + info.argc = argc;\n> + info.argv = argv;\n> + info.prefix = prefix;\n> + info.quiet = !!quiet;\n> + info.recursive = !!recursive;\n\nas you assign all fields of the struct yourself, you could also omit the\nstatic initialization via _INIT above.\n\n\nApart from these two minor nits the code looks good to me.\nHowever we'd really want to have the bug fix patch as well.\n(At the time of submission of a patch we should not be aware\nof any tests failing, which we are without said bug fix patch)\n\nThanks,\nStefan\n"},{"id":"320571","messageId":"20170523190617.GE115919@google.com","threadId":"45748","inReplyTo":"20170521125814.26255-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v4 1/2] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-23T19:06:17Z","receivedAt":"2017-05-23T19:06:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/21, Prathamesh Chavan wrote:\n> Additional test cases added to the submodule-foreach test suite\n> to check the submodule foreach --recursive behavior from a\n> subdirectory as this was missing from the test suite.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> It was observed that after porting the submodule subcommand to\n> C, it passed all the test from the existing test-suite.\n> But since there was some observation made, where the output of\n> the orignal submodule foreach subcommand wasn't matching to that\n> of the newly ported function, this test has been added.\n> \n> After which, it can been seen that the patch fails in test #9\n> of t7407-submodule-foreach, which is the newly added\n> test to that suite. The main reason of adding this test\n> was to bring the behavior of $path for the submodule\n> foreach --recursive case.\n> \n> The observation made was as follows:\n> \n> For a project - super containing dir (not a submodule)\n> and a submodule sub which contains another submodule\n> subsub. When we run a command from super/dir:\n> \n> git submodule foreach \"echo \\$path-\\$sm_path\"\n> \n> actual results:\n> Entering '../sub'\n> ../sub-../sub\n> Entering '../sub/subsub'\n> ../subsub-../subsub\n> \n> ported function's result:\n> Entering '../sub'\n> sub-../sub\n> Entering '../sub/subsub'\n> subsub-../sub/subsub\n> \n> This is occurring since in cmd_foreach of git-submodule.sh\n> when we use to recurse, we call cmd_foreach\n> and hence the process ran in the same shell.\n> Because of this, the variable $wt_prefix is set only once\n> which is at the beginning of the submodule foreach execution.\n> wt_prefix=$(git rev-parse --show-prefix)\n> \n> And since sm_path and path are set using $wt_prefix as :\n> sm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> path=$sm_path\n> It differs with the value of displaypath as well.\n> \n> This make the value of $path confusing and I also feel it\n> deviates from its documentation:\n> $path is the name of the submodule directory relative\n> to the superproject.\n> \n> But since in refactoring the code, we wish to maintain the\n> code in same way, we need to pass wt_prefix on every\n> recursive call, which may result in complex C code.\n> Another option could be to first correct the $path value\n> in git-submodule.sh and then port the updated cmd_foreach.\n> \n> t/t7407-submodule-foreach.sh | 35 ++++++++++++++++++++++++++++++++++-\n> 1 file changed, 34 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6ba5daf42..58a890e31 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -79,7 +79,6 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n> \t) &&\n> \ttest_i18ncmp expect actual\n> '\n> -\n\nThe removal of this line seems unrelated to the rest of this patch. Was\nthis intended?\n\n> cat >expect < Entering '../sub1'\n> $pwd/clone-foo1-../sub1-$sub1sha1\n> @@ -197,6 +196,40 @@ test_expect_success 'test messages from \"foreach --recursive\" from subdirectory'\n> \ttest_i18ncmp expect actual\n> '\n> \n> +sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)\n> +sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)\n> +sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)\n> +nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)\n> +nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)\n> +nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)\n> +submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)\n> +\n> +cat >expect < +Entering '../nested1'\n> +$pwd/clone2-nested1-../nested1-$nested1sha1\n> +Entering '../nested1/nested2'\n> +$pwd/clone2/nested1-nested2-../nested2-$nested2sha1\n> +Entering '../nested1/nested2/nested3'\n> +$pwd/clone2/nested1/nested2-nested3-../nested3-$nested3sha1\n> +Entering '../nested1/nested2/nested3/submodule'\n> +$pwd/clone2/nested1/nested2/nested3-submodule-../submodule-$submodulesha1\n> +Entering '../sub1'\n> +$pwd/clone2-foo1-../sub1-$sub1sha1\n> +Entering '../sub2'\n> +$pwd/clone2-foo2-../sub2-$sub2sha1\n> +Entering '../sub3'\n> +$pwd/clone2-foo3-../sub3-$sub3sha1\n> +EOF\n> +\n> +test_expect_success 'test \"submodule foreach --recursive\" from subdirectory' '\n> +\t(\n> +\t\tcd clone2 &&\n> +\t\tcd untracked &&\n> +\t\tgit submodule foreach --recursive \"echo \\$toplevel-\\$name-\\$sm_path-\\$sha1\" >../../actual\n> +\t) &&\n> +\ttest_i18ncmp expect actual\n> +'\n> +\n> cat > expect < nested1-nested1\n> nested2-nested2\n> -- \n> 2.11.0\n> \n\n-- \nBrandon Williams\n"},{"id":"320573","messageId":"20170523190928.GF115919@google.com","threadId":"45748","inReplyTo":"CAGZ79kbmQ5_Vb8BSoJkA74-+e0FoKwz=iJkSk4sdSnc46s+qUw@mail.gmail.com","subject":"Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-23T19:09:28Z","receivedAt":"2017-05-23T19:09:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/22, Stefan Beller wrote:\n> On Sun, May 21, 2017 at 5:58 AM, Prathamesh Chavan wrote:\n> \n> > I have also made some changes in git-submodule.sh for correcting\n> > the $path variable. And hence made the corresponding changes in\n> > the new test introduced in t7407-submodule-foreach as well.\n> > I have push this work at:\n> > https://github.com/pratham-pc/git/commits/foreach-bug-fixed\n> \n> This one seems to pass the test suite by having the bug fixed.\n> (The patches posted here seems to be\n> https://github.com/pratham-pc/git/commits/foreach\n> which does not pass tests? These two series seem to only differ in\n> the bug fix commit, which I think is a good idea to include, as then we\n> have a bug fixed and the tests pass.)\n> \n> > +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)\n> ..\n> > + return;\n> \n> no need for an explicit return in a void function.\n> \n> > +struct cb_foreach {\n> > + int argc;\n> > + const char **argv;\n> > + const char *prefix;\n> > + unsigned int quiet: 1;\n> > + unsigned int recursive: 1;\n> > +};\n> > +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }\n> \n> This static initializer doesn't quite match the struct,\n> (I would expect two NULLs as we have two const char pointers).\n\nIf we ever move to a new version of C, these initializers would be much\nmore readable as we could assign values to the fields themselves. But\nthat is unrelated to this change.\n\n> \n> > +\n> > + info.argc = argc;\n> > + info.argv = argv;\n> > + info.prefix = prefix;\n> > + info.quiet = !!quiet;\n> > + info.recursive = !!recursive;\n> \n> as you assign all fields of the struct yourself, you could also omit the\n> static initialization via _INIT above.\n> \n> \n> Apart from these two minor nits the code looks good to me.\n> However we'd really want to have the bug fix patch as well.\n> (At the time of submission of a patch we should not be aware\n> of any tests failing, which we are without said bug fix patch)\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"320590","messageId":"20170523193627.GG115919@google.com","threadId":"45748","inReplyTo":"20170521125814.26255-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-23T19:36:27Z","receivedAt":"2017-05-23T19:36:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/21, Prathamesh Chavan wrote:\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n> The code is split up to have one function to obtain all the list of\n> submodules. This function acts as the front-end of git-submodule foreach\n> subcommand. It calls the function for_each_submodule_list, which basically\n> loops through the list and calls function fn, which in this case is\n> runcommand_in_submodule. This third function is a calling function that\n> takes care of running the command in that submodule, and recursively\n> perform the same when --recursive is flagged.\n> \n> The first function module_foreach first parses the options present in\n> argv, 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\n> list, and calls function fn (which in case of submodule subcommand\n> foreach is runcommand_in_submodule) is called for each entry.\n> \n> The third function runcommand_in_submodule, generates a submodule struct sub\n> for $name, value and then later prepends name=sub->name; and other\n> value assignment to the env argv_array structure of a child_process.\n> Also the of submodule-foreach is push to args argv_array\n> structure and finally, using run_command the commands are executed\n> using a shell.\n> \n> The third function also takes care of the recursive flag, by creating\n> a separate child_process structure and prepending \"--super-prefix displaypath\",\n> to the args argv_array structure. Other required arguments and the\n> input of submodule-foreach is also appended to this argv_array.\n> \n> The commit 1c4fb136db (submodule foreach: skip eval for more than one\n> argument, 2013-09-27), which explains that why for the case when argc>1,\n> we do not use eval. But since in this patch, we are calling the\n> command in a separate shell itself for all values of argc, this case\n> is not considered separately.\n> \n> Both env variable $path and $sm_path were added since both are used in\n> tests in t7407.\n> \n> Helped-by: Brandon Williams \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> This series of patch is based on gitster/jk/bug-to-abort for untilizing its\n> BUG() macro.\n> \n> In this new version of patch, a new function\n> get_submodule_displaypath is introduced, which is the same one\n> as that in the patch series for porting of submodule subcommand\n> status. I had to again introduce this in this patch as well as\n> I am working on two separate branches for parting of each function.\n> Also, the function for_each_submodule_list repeats for the same\n> reason.\n> \n> I have pushed this work on Github at:\n> https://github.com/pratham-pc/git/commits/foreach\n> \n> Its build report is available at:\n> https://travis-ci.org/pratham-pc/git/builds/\n> Branch: foreach\n> Build #67\n> \n> I have also made some changes in git-submodule.sh for correcting\n> the $path variable. And hence made the corresponding changes in\n> the new test introduced in t7407-submodule-foreach as well.\n> I have push this work at:\n> https://github.com/pratham-pc/git/commits/foreach-bug-fixed\n> \n> Its build report is available at:\n> https://travis-ci.org/pratham-pc/git/builds/\n> Branch: foreach-bug-fixed\n> Build #66\n> \n> builtin/submodule--helper.c | 142 ++++++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 39 +-----------\n> 2 files changed, 143 insertions(+), 38 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 566a5b6a6..4e19beaff 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\nYou have a potential memory leak here, you need to release the strbuf\nbefore returning.\n\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> @@ -331,6 +350,15 @@ 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\nnit: You could probably break this line so its not longer than 80 chars.\n\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < list.nr; i++)\n> +\t\tfn(list.entries[i], cb_data);\n> +\n> +\treturn;\n\nNo return needed.\n\n> +}\n\nsmall nit, and not that important, but could this function potentially\nbe moved closer to where it is used? What as the rational for placing it\nhere?\n\n> +\n> static void init_submodule(const char *path, const char *prefix, int quiet)\n> {\n> \tconst struct submodule *sub;\n> @@ -487,6 +515,119 @@ static int module_name(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +struct cb_foreach {\n> +\tint argc;\n> +\tconst char **argv;\n> +\tconst char *prefix;\n> +\tunsigned int quiet: 1;\n> +\tunsigned int recursive: 1;\n> +};\n> +#define CB_FOREACH_INIT { 0, NULL, 0, 0 }\n\nNeed an extra NULL as Stefan pointed out:\n { 0, NULL, NULL, 0, 0 }\n\n> +\n> +static void runcommand_in_submodule(const struct cache_entry *list_item, void *cb_data)\n> +{\n> +\tstruct cb_foreach *info = cb_data;\n> +\tchar *toplevel = xgetcwd();\n> +\tconst struct submodule *sub;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tchar* displaypath = NULL;\n> +\tint i;\n> +\n> +\t/* Only loads from .gitmodules, no overlay with .git/config */\n> +\tgitmodules_config();\n> +\n> +\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n> +\n> +\tsub = submodule_from_path(null_sha1, list_item->name);\n> +\n> +\tif (!sub)\n> +\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> +\t\t displaypath);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\tcp.use_shell = 1;\n> +\tcp.dir = list_item->name;\n> +\n> +\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> +\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> +\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n> +\targv_array_pushf(&cp.env_array, \"sha1=%s\", oid_to_hex(&list_item->oid));\n> +\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n> +\n> +\tfor (i = 0; i < info->argc; i++)\n> +\t\targv_array_push(&cp.args, info->argv[i]);\n> +\n> +\tif (!is_submodule_populated_gently(list_item->name, NULL))\n> +\t\treturn;\n\nThis check needs to be hoisted up probably before calculating the\ndisplay path, otherwise you have a bunch of memory leaks that need to be\nplugged. Something like:\n\n +\tsub = submodule_from_path(null_sha1, list_item->name);\n +\n +\tif (!sub)\n +\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n +\t\t displaypath);\n +\n +\tif (!is_submodule_populated_gently(list_item->name, NULL))\n +\t\treturn;\n +\n +\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n\n> +\n> +\tif (!info->quiet)\n> +\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n> +\tif (info->argv[0] && run_command(&cp))\n> +\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"), displaypath);\n> +\n> +\tif (info->recursive) {\n> +\t\tstruct child_process cpr = CHILD_PROCESS_INIT;\n> +\n> +\t\tcpr.use_shell = 1;\n\nYou can set .git_cmd = 1 instead.\n\n> +\t\tcpr.dir = list_item->name;\n> +\t\tprepare_submodule_repo_env(&cpr.env_array);\n> +\n> +\t\targv_array_pushl(&cpr.args, \"git\", \"--super-prefix\", displaypath,\n\nAnd then you don't need to include \"git\" here.\n\n> +\t\t\t\t \"submodule--helper\", \"foreach\", \"--recursive\", NULL);\n> +\n> +\t\tif (info->quiet)\n> +\t\t\targv_array_push(&cpr.args, \"--quiet\");\n> +\n> +\t\tfor (i = 0; i < info->argc; i++)\n> +\t\t\targv_array_push(&cpr.args, info->argv[i]);\n> +\n> +\t\tif (run_command(&cpr))\n> +\t\t\tdie(_(\"run_command returned non-zero status while \\\n> +\t\t\t recuring in the nested submodules of %s\\n.\"),\n\nIf you're going to split these two lines up then it may make more sense\nto use concatenation instead of a continuation '\\'. I'm not sure how\nthe spaces at the beginning of the line will look when printed.\nSomething like this:\n\n +\t\t\tdie(_(\"run_command returned non-zero status while\"\n +\t\t\t \"recursing in the nested submodules of %s\\n.\"),\n\n\nAlso s/recuring/recursing\n\n> +\t\t\t displaypath);\n> +\t}\n> +\n> +\tfree(displaypath);\n> +\tfree(toplevel);\n> +}\n> +\n> +static int module_foreach(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct cb_foreach info = CB_FOREACH_INIT;\n> +\tstruct pathspec pathspec;\n> +\tstruct module_list list = MODULE_LIST_INIT;\n> +\tint quiet = 0;\n> +\tint recursive = 0;\n> +\n> +\tstruct option module_foreach_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n> +\t\tOPT_BOOL(0, \"recursive\", &recursive,\n> +\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_foreach_options,\n> +\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n> +\n> +\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n> +\t\tdie(\"BUG: module_list_compute should not choke on empty pathspec\");\n\nYou mentioned using the 'BUG()' function call, and you use it up above,\nso why not use it here too.\n\n> +\n> +\tinfo.argc = argc;\n> +\tinfo.argv = argv;\n> +\tinfo.prefix = prefix;\n> +\tinfo.quiet = !!quiet;\n> +\tinfo.recursive = !!recursive;\n\nIf these values are boolean why do we need to do the extra '!!'?\n\n> +\n> +\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> \t\t\t const char *depth, struct string_list *reference,\n> \t\t\t int quiet, int progress)\n> @@ -1212,6 +1353,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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..032fd2540 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -322,45 +322,8 @@ cmd_foreach()\n> \t\tshift\n> \tdone\n> \n> -\ttoplevel=$(pwd)\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n> \n> -\t# dup stdin so that it can be restored when running the external\n> -\t# command in the subshell (and a recursive call to this function)\n> -\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n> -\t\tthen\n> -\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n> -\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n> -\t\t\tname=$(git submodule--helper name \"$sm_path\")\n> -\t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> -\t\t\t\tsanitize_submodule_env\n> -\t\t\t\tcd \"$sm_path\" &&\n> -\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> -\t\t\t\t# we make $path available to scripts ...\n> -\t\t\t\tpath=$sm_path &&\n> -\t\t\t\tif test $# -eq 1\n> -\t\t\t\tthen\n> -\t\t\t\t\teval \"$1\"\n> -\t\t\t\telse\n> -\t\t\t\t\t\"$@\"\n> -\t\t\t\tfi &&\n> -\t\t\t\tif test -n \"$recursive\"\n> -\t\t\t\tthen\n> -\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n> -\t\t\t\tfi\n> -\t\t\t) <&3 3<&- ||\n> -\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n> -\t\tfi\n> -\tdone\n> }\n> \n> #\n> -- \n> 2.11.0\n> \n\n-- \nBrandon Williams\n"},{"id":"320596","messageId":"CAGZ79kYPUO34YUVR_u4sRuYz+Geo=wxwNEfCnyx+NQWQCQTkaQ@mail.gmail.com","threadId":"45748","inReplyTo":"20170523193627.GG115919@google.com","subject":"Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-23T20:57:25Z","receivedAt":"2017-05-23T20:59:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 23, 2017 at 12:36 PM, Brandon Williams wrote:\n>\n> You can set .git_cmd = 1 instead.\n>\n>> + cpr.dir = list_item->name;\n>> + prepare_submodule_repo_env(&cpr.env_array);\n>> +\n>> + argv_array_pushl(&cpr.args, \"git\", \"--super-prefix\", displaypath,\n>\n> And then you don't need to include \"git\" here.\n\neven if git_cmd = 1 is set, you'd need a first dummy argument?\ncf. find_unpushed_submodules, See comment in 9cfa1c260f\n(serialize collection of refs that contain submodule changes, 2016-11-16)\n\n>> +\n>> + info.argc = argc;\n>> + info.argv = argv;\n>> + info.prefix = prefix;\n>> + info.quiet = !!quiet;\n>> + info.recursive = !!recursive;\n>\n> If these values are boolean why do we need to do the extra '!!'?\n\nActually that was my advice. As we only have a limited space in a single\nbit, strange things happen when you were to do:\n\n quiet = 2; /* be extra quiet */\n info.quiet = quiet;\n\nThis is not the case here, but other commands have evolved over time\nto first take a OPT_BOOL, and then in a later patch an OPT_INT.\n(some commands take a \"-v -v -v\")\n\nAnd by having the double negative we'd have some defensive programming\nright here. (To prove I am not telling crazy stories, $ git log -S \\!\\!)\n"},{"id":"320597","messageId":"20170523210525.GH115919@google.com","threadId":"45748","inReplyTo":"CAGZ79kYPUO34YUVR_u4sRuYz+Geo=wxwNEfCnyx+NQWQCQTkaQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-23T21:05:25Z","receivedAt":"2017-05-23T21:05:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/23, Stefan Beller wrote:\n> On Tue, May 23, 2017 at 12:36 PM, Brandon Williams wrote:\n> >\n> > You can set .git_cmd = 1 instead.\n> >\n> >> + cpr.dir = list_item->name;\n> >> + prepare_submodule_repo_env(&cpr.env_array);\n> >> +\n> >> + argv_array_pushl(&cpr.args, \"git\", \"--super-prefix\", displaypath,\n> >\n> > And then you don't need to include \"git\" here.\n> \n> even if git_cmd = 1 is set, you'd need a first dummy argument?\n> cf. find_unpushed_submodules, See comment in 9cfa1c260f\n> (serialize collection of refs that contain submodule changes, 2016-11-16)\n\nDifferent subsystem, you don't need a dummy first argument. The\nrevision walking code does (for some reason) need a dummy first\nargument.\n\n> \n> >> +\n> >> + info.argc = argc;\n> >> + info.argv = argv;\n> >> + info.prefix = prefix;\n> >> + info.quiet = !!quiet;\n> >> + info.recursive = !!recursive;\n> >\n> > If these values are boolean why do we need to do the extra '!!'?\n> \n> Actually that was my advice. As we only have a limited space in a single\n> bit, strange things happen when you were to do:\n> \n> quiet = 2; /* be extra quiet */\n> info.quiet = quiet;\n> \n> This is not the case here, but other commands have evolved over time\n> to first take a OPT_BOOL, and then in a later patch an OPT_INT.\n> (some commands take a \"-v -v -v\")\n> \n> And by having the double negative we'd have some defensive programming\n> right here. (To prove I am not telling crazy stories, $ git log -S \\!\\!)\n\nAll good, I didn't notice that they were bit fields.\n\n-- \nBrandon Williams\n"},{"id":"320817","messageId":"20170526151713.10974-1-pc44800@gmail.com","threadId":"45748","inReplyTo":"20170521125814.26255-2-pc44800@gmail.com","subject":"[GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-26T15:17:11Z","receivedAt":"2017-05-26T15:24:05Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"According to the documentation about git-submodule foreach subcommand's\n$path variable:\n$path is the name of the submodule directory relative to the superproject\n\nBut it was observed when the value of the $path value deviates from this\nfor the nested submodules when the is run from a subdirectory.\nThis patch aims for its correction.\n\nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nThis series of patch is based on gitster/jk/bug-to-abort for untilizing its \nBUG() macro.\n\nThe observation made was as follows:\nFor a project - super containing dir (not a submodule) and a submodule sub \nwhich contains another submodule subsub. When we run a command from super/dir:\n\ngit submodule foreach \"echo \\$path-\\$sm_path\"\n\nactual results:\nEntering '../sub'\n../sub-../sub\nEntering '../sub/subsub'\n../subsub-../subsub\n\nexpected result wrt documentation and current test suite:\nEntering '../sub'\nsub-../sub\nEntering '../sub/subsub'\nsubsub-../sub/subsub\n\nThis make the value of $path confusing and I also feel it deviates from its \ndocumentation:\n$path is the name of the submodule directory relative to the superproject.\nHence, this patch corrects the value assigned to the $path and $sm_path.\n\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c0d0e9a4c..ea6f56337 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -344,9 +344,9 @@ cmd_foreach()\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n \t\t\t\t# we make $path available to scripts ...\n \t\t\t\tpath=$sm_path &&\n+\t\t\t\tsm_path=$displaypath &&\n \t\t\t\tif test $# -eq 1\n \t\t\t\tthen\n \t\t\t\t\teval \"$1\"\n-- \n2.11.0\n\n"},{"id":"320818","messageId":"20170526151713.10974-3-pc44800@gmail.com","threadId":"45748","inReplyTo":"20170526151713.10974-1-pc44800@gmail.com","subject":"[GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-26T15:17:13Z","receivedAt":"2017-05-26T15:25:29Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"This aims to make git-submodule foreach a builtin. This is the very\nfirst step taken in this direction. Hence, 'foreach' is ported to\nsubmodule--helper, and submodule--helper is called from git-submodule.sh.\nThe code is split up to have one function to obtain all the list of\nsubmodules. This function acts as the front-end of git-submodule foreach\nsubcommand. It calls the function for_each_submodule_list, which basically\nloops through the list and calls function fn, which in this case is\nruncommand_in_submodule. This third function is a calling function that\ntakes care of running the command in that submodule, and recursively\nperform the same when --recursive is flagged.\n\nThe first function module_foreach first parses the options present in\nargv, and 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\nlist, and calls function fn (which in case of submodule subcommand\nforeach is runcommand_in_submodule) is called for each entry.\n\nThe third function runcommand_in_submodule, generates a submodule struct sub\nfor $name, value and then later prepends name=sub->name; and other\nvalue assignment to the env argv_array structure of a child_process.\nAlso the of submodule-foreach is push to args argv_array\nstructure and finally, using run_command the commands are executed\nusing a shell.\n\nThe third function also takes care of the recursive flag, by creating\na separate child_process structure and prepending \"--super-prefix displaypath\",\nto the args argv_array structure. Other required arguments and the\ninput of submodule-foreach is also appended to this argv_array.\n\nThe commit 1c4fb136db (submodule foreach: skip eval for more than one\nargument, 2013-09-27), which explains that why for the case when argc>1,\nwe do not use eval. But since in this patch, we are calling the\ncommand in a separate shell itself for all values of argc, this case\nis not considered separately.\n\nBoth env variable $path and $sm_path were added since both are used in\ntests in t7407.\n\nHelped-by: Brandon Williams \nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nThese series of patches passes the complete test suite.\nIts build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: submodule-foreach\nBuild #71\n\n builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 39 +-----------\n 2 files changed, 149 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..343b6269c 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,26 @@ 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;\n+\t\tdisplaypath = 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@@ -331,6 +353,14 @@ 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,\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 static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cb_foreach {\n+\tint argc;\n+\tconst char **argv;\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+};\n+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }\n+\n+static void runcommand_in_submodule(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct cb_foreach *info = cb_data;\n+\tchar *toplevel = xgetcwd();\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tchar* displaypath = NULL;\n+\tint i;\n+\n+\t/* Only loads from .gitmodules, no overlay with .git/config */\n+\tgitmodules_config();\n+\n+\tsub = submodule_from_path(null_sha1, list_item->name);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\n+\tif (!is_submodule_populated_gently(list_item->name, NULL))\n+\t\treturn;\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.use_shell = 1;\n+\tcp.dir = list_item->name;\n+\n+\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n+\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n+\targv_array_pushf(&cp.env_array, \"sha1=%s\", oid_to_hex(&list_item->oid));\n+\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+\tfor (i = 0; i < info->argc; i++)\n+\t\targv_array_push(&cp.args, info->argv[i]);\n+\n+\tif (!info->quiet)\n+\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\n+\tif (info->argv[0] && run_command(&cp))\n+\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n+\t\t displaypath);\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\", \"foreach\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tfor (i = 0; i < info->argc; i++)\n+\t\t\targv_array_push(&cpr.args, info->argv[i]);\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"run_command returned non-zero status while\"\n+\t\t\t \"recursing in the nested submodules of %s\\n.\"),\n+\t\t\t displaypath);\n+\t}\n+\n+\tfree(displaypath);\n+\tfree(toplevel);\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct cb_foreach info;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n+\t\tBUG(\"module_list_compute should not choke on empty pathspec\");\n+\n+\tinfo.argc = argc;\n+\tinfo.argv = argv;\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = quiet;\n+\tinfo.recursive = recursive;\n+\n+\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1212,6 +1359,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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 ea6f56337..032fd2540 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -322,45 +322,8 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\ttoplevel=$(pwd)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tsm_path=$displaypath &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.11.0\n\n"},{"id":"320819","messageId":"20170526151713.10974-2-pc44800@gmail.com","threadId":"45748","inReplyTo":"20170526151713.10974-1-pc44800@gmail.com","subject":"[GSoC][PATCH v5 2/3] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-05-26T15:17:12Z","receivedAt":"2017-05-26T15:25:36Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Additional test cases added to the submodule-foreach test suite\nto check the submodule foreach --recursive behavior from a\nsubdirectory as this was missing from the test suite.\n\nHelped-by: Brandon Williams \nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nAdditional test added to check the bug fixed in the [PATCH v5 1/3] of\nthis patch series.\n\n t/t7407-submodule-foreach.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42..1c8d132d8 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -197,6 +197,40 @@ test_expect_success 'test messages from \"foreach --recursive\" from subdirectory'\n \ttest_i18ncmp expect actual\n '\n \n+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)\n+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)\n+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)\n+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)\n+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)\n+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)\n+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)\n+\n+cat >expect <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect < wrote:\n> This aims to make git-submodule foreach a builtin.\n\nCool. I reviewed the code and only have one minor nit.\n\n> +static void runcommand_in_submodule(const struct cache_entry *list_item,\n> + void *cb_data)\n> +{\n\n> + /* Only loads from .gitmodules, no overlay with .git/config */\n> + gitmodules_config();\n\nPerformance nit: We only need to load the gitmodules file once instead\nof foreach submodule separately, so we could move this to module_foreach().\n\nThanks,\nStefan\n"},{"id":"320822","messageId":"CAGZ79kY4s_WtEJfyFStchFvr7ka659zYjv6H3vy9GsCX2sKbdg@mail.gmail.com","threadId":"45748","inReplyTo":"20170526151713.10974-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v5 2/3] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-26T16:19:10Z","receivedAt":"2017-05-26T16:19:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 26, 2017 at 8:17 AM, Prathamesh Chavan wrote:\n> Additional test cases added to the submodule-foreach test suite\n> to check the submodule foreach --recursive behavior from a\n> subdirectory as this was missing from the test suite.\n\nAs this demonstrates the fixture of the first patch,\nthis could be squashed into the first commit.\n\nReason: When someone is looking at that first commit, they\nmay wonder if there is no test that demonstrates the fix. (as fixing\na bug with no test is bad style. ;) And given the data structures of\nGit it is only easy to find the previous commit, but hard to find the\nnext commit (this one) later on.\n\nI think with only the minor nit in patch 3, the foreach is tackled. :)\n\nThanks,\nStefan\n"},{"id":"320824","messageId":"01e8c552-fd3a-ee05-1ff1-3b3ea0f7feeb@ramsayjones.plus.com","threadId":"45748","inReplyTo":"20170526151713.10974-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-26T16:31:06Z","receivedAt":"2017-05-26T16:31:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 26/05/17 16:17, Prathamesh Chavan wrote:\n> According to the documentation about git-submodule foreach subcommand's\n> $path variable:\n> $path is the name of the submodule directory relative to the superproject\n> \n> But it was observed when the value of the $path value deviates from this\n> for the nested submodules when the is run from a subdirectory.\n> This patch aims for its correction.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> This series of patch is based on gitster/jk/bug-to-abort for untilizing its \n> BUG() macro.\n> \n> The observation made was as follows:\n> For a project - super containing dir (not a submodule) and a submodule sub \n> which contains another submodule subsub. When we run a command from super/dir:\n> \n> git submodule foreach \"echo \\$path-\\$sm_path\"\n> \n> actual results:\n> Entering '../sub'\n> ../sub-../sub\n> Entering '../sub/subsub'\n> ../subsub-../subsub\n> \n> expected result wrt documentation and current test suite:\n> Entering '../sub'\n> sub-../sub\n> Entering '../sub/subsub'\n> subsub-../sub/subsub\n> \n> This make the value of $path confusing and I also feel it deviates from its \n> documentation:\n> $path is the name of the submodule directory relative to the superproject.\n> Hence, this patch corrects the value assigned to the $path and $sm_path.\n> \n> git-submodule.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index c0d0e9a4c..ea6f56337 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -344,9 +344,9 @@ cmd_foreach()\n> \t\t\t\tprefix=\"$prefix$sm_path/\"\n> \t\t\t\tsanitize_submodule_env\n> \t\t\t\tcd \"$sm_path\" &&\n> -\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n> \t\t\t\t# we make $path available to scripts ...\n> \t\t\t\tpath=$sm_path &&\n> +\t\t\t\tsm_path=$displaypath &&\n> \t\t\t\tif test $# -eq 1\n> \t\t\t\tthen\n> \t\t\t\t\teval \"$1\"\n> \n\nHmm, I'm not sure which documentation you are referring to, but if\n$path != $sm_path then something is wrong. (unless their definition\nhas changed, of course). commit 091a6eb0fe may have muddied the water\na little by using $sm_path in the test in t7407, since (as far as I\nknow) $path is the user-facing variable (NOT $sm_path).\n\nATB,\nRamsay Jones\n\n\n"},{"id":"320825","messageId":"20170526163319.GA4592@google.com","threadId":"45748","inReplyTo":"20170526151713.10974-2-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v5 2/3] t7407: test \"submodule foreach --recursive\" from subdirectory added","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-26T16:33:19Z","receivedAt":"2017-05-26T16:33:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/26, Prathamesh Chavan wrote:\n> Additional test cases added to the submodule-foreach test suite\n> to check the submodule foreach --recursive behavior from a\n> subdirectory as this was missing from the test suite.\n> \n> Helped-by: Brandon Williams \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> Additional test added to check the bug fixed in the [PATCH v5 1/3] of\n> this patch series.\n> \n> t/t7407-submodule-foreach.sh | 34 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n> \n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6ba5daf42..1c8d132d8 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -197,6 +197,40 @@ test_expect_success 'test messages from \"foreach --recursive\" from subdirectory'\n> \ttest_i18ncmp expect actual\n> '\n> \n> +sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)\n> +sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)\n> +sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)\n> +nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)\n> +nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)\n> +nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)\n> +submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)\n> +\n> +cat >expect < +Entering '../nested1'\n> +$pwd/clone2-nested1-../nested1-$nested1sha1\n> +Entering '../nested1/nested2'\n> +$pwd/clone2/nested1-nested2-../nested1/nested2-$nested2sha1\n> +Entering '../nested1/nested2/nested3'\n> +$pwd/clone2/nested1/nested2-nested3-../nested1/nested2/nested3-$nested3sha1\n> +Entering '../nested1/nested2/nested3/submodule'\n> +$pwd/clone2/nested1/nested2/nested3-submodule-../nested1/nested2/nested3/submodule-$submodulesha1\n> +Entering '../sub1'\n> +$pwd/clone2-foo1-../sub1-$sub1sha1\n> +Entering '../sub2'\n> +$pwd/clone2-foo2-../sub2-$sub2sha1\n> +Entering '../sub3'\n> +$pwd/clone2-foo3-../sub3-$sub3sha1\n> +EOF\n> +\n> +test_expect_success 'test \"submodule foreach --recursive\" from subdirectory' '\n> +\t(\n> +\t\tcd clone2 &&\n> +\t\tcd untracked &&\n> +\t\tgit submodule foreach --recursive \"echo \\$toplevel-\\$name-\\$sm_path-\\$sha1\" >../../actual\n> +\t) &&\n\nsmall nit: You can either merge the two cd commands to 'cd clone2/untracked' or\nbetter you can even avoid the subshell entirely by doing the following:\n\n git -C clone2/untracked submodule foreach --recursive \\\n \"echo \\$toplevel-\\$name-\\$sm_path-\\$sha1\" >actual\n\nOr something akin to that.\n\n> +\ttest_i18ncmp expect actual\n> +'\n> +\n> cat > expect < nested1-nested1\n> nested2-nested2\n> -- \n> 2.11.0\n> \n\n-- \nBrandon Williams\n"},{"id":"320826","messageId":"20170526164459.GB4592@google.com","threadId":"45748","inReplyTo":"20170526151713.10974-3-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-26T16:44:59Z","receivedAt":"2017-05-26T16:45:07Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/26, Prathamesh Chavan wrote:\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n> The code is split up to have one function to obtain all the list of\n> submodules. This function acts as the front-end of git-submodule foreach\n> subcommand. It calls the function for_each_submodule_list, which basically\n> loops through the list and calls function fn, which in this case is\n> runcommand_in_submodule. This third function is a calling function that\n> takes care of running the command in that submodule, and recursively\n> perform the same when --recursive is flagged.\n> \n> The first function module_foreach first parses the options present in\n> argv, 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\n> list, and calls function fn (which in case of submodule subcommand\n> foreach is runcommand_in_submodule) is called for each entry.\n> \n> The third function runcommand_in_submodule, generates a submodule struct sub\n> for $name, value and then later prepends name=sub->name; and other\n> value assignment to the env argv_array structure of a child_process.\n> Also the of submodule-foreach is push to args argv_array\n> structure and finally, using run_command the commands are executed\n> using a shell.\n> \n> The third function also takes care of the recursive flag, by creating\n> a separate child_process structure and prepending \"--super-prefix displaypath\",\n> to the args argv_array structure. Other required arguments and the\n> input of submodule-foreach is also appended to this argv_array.\n> \n> The commit 1c4fb136db (submodule foreach: skip eval for more than one\n> argument, 2013-09-27), which explains that why for the case when argc>1,\n> we do not use eval. But since in this patch, we are calling the\n> command in a separate shell itself for all values of argc, this case\n> is not considered separately.\n> \n> Both env variable $path and $sm_path were added since both are used in\n> tests in t7407.\n> \n> Helped-by: Brandon Williams \n> Mentored-by: Christian Couder \n> Mentored-by: Stefan Beller \n> Signed-off-by: Prathamesh Chavan \n> ---\n> These series of patches passes the complete test suite.\n> Its build report is available at:\n> https://travis-ci.org/pratham-pc/git/builds\n> Branch: submodule-foreach\n> Build #71\n> \n> builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 39 +-----------\n> 2 files changed, 149 insertions(+), 38 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 566a5b6a6..343b6269c 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,26 @@ 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;\n> +\t\tdisplaypath = xstrdup(relative_path(path, prefix, &sb));\n\nThese can probably go on the same line:\n \n char *displaypath = xstrdup(relative_path(path, prefix, &sb));\n\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> @@ -331,6 +353,14 @@ 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,\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> static void init_submodule(const char *path, const char *prefix, int quiet)\n> {\n> \tconst struct submodule *sub;\n> @@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +struct cb_foreach {\n> +\tint argc;\n> +\tconst char **argv;\n> +\tconst char *prefix;\n> +\tunsigned int quiet: 1;\n> +\tunsigned int recursive: 1;\n> +};\n> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }\n> +\n> +static void runcommand_in_submodule(const struct cache_entry *list_item,\n> +\t\t\t\t void *cb_data)\n> +{\n> +\tstruct cb_foreach *info = cb_data;\n> +\tchar *toplevel = xgetcwd();\n> +\tconst struct submodule *sub;\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tchar* displaypath = NULL;\n> +\tint i;\n> +\n> +\t/* Only loads from .gitmodules, no overlay with .git/config */\n> +\tgitmodules_config();\n> +\n> +\tsub = submodule_from_path(null_sha1, list_item->name);\n> +\n> +\tif (!sub)\n> +\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n> +\t\t displaypath);\n> +\n> +\tif (!is_submodule_populated_gently(list_item->name, NULL))\n> +\t\treturn;\n\nI missed one other memory leak from the last round. You should probably\ncall xgetcwd() to fill 'toplevel' here to avoid leaking the memory if\nyou do an early return.\n\n> +\n> +\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n> +\n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\tcp.use_shell = 1;\n> +\tcp.dir = list_item->name;\n> +\n> +\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> +\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> +\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n> +\targv_array_pushf(&cp.env_array, \"sha1=%s\", oid_to_hex(&list_item->oid));\n> +\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n> +\n> +\tfor (i = 0; i < info->argc; i++)\n> +\t\targv_array_push(&cp.args, info->argv[i]);\n> +\n> +\tif (!info->quiet)\n> +\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n> +\n> +\tif (info->argv[0] && run_command(&cp))\n> +\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n> +\t\t displaypath);\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\", \"foreach\", \"--recursive\",\n> +\t\t\t\t NULL);\n> +\n> +\t\tif (info->quiet)\n> +\t\t\targv_array_push(&cpr.args, \"--quiet\");\n> +\n> +\t\tfor (i = 0; i < info->argc; i++)\n> +\t\t\targv_array_push(&cpr.args, info->argv[i]);\n> +\n> +\t\tif (run_command(&cpr))\n> +\t\t\tdie(_(\"run_command returned non-zero status while\"\n> +\t\t\t \"recursing in the nested submodules of %s\\n.\"),\n> +\t\t\t displaypath);\n> +\t}\n> +\n> +\tfree(displaypath);\n> +\tfree(toplevel);\n> +}\n> +\n> +static int module_foreach(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct cb_foreach info;\n> +\tstruct pathspec pathspec;\n> +\tstruct module_list list = MODULE_LIST_INIT;\n> +\tint quiet = 0;\n> +\tint recursive = 0;\n> +\n> +\tstruct option module_foreach_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n> +\t\tOPT_BOOL(0, \"recursive\", &recursive,\n> +\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_foreach_options,\n> +\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n> +\n> +\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n> +\t\tBUG(\"module_list_compute should not choke on empty pathspec\");\n> +\n> +\tinfo.argc = argc;\n> +\tinfo.argv = argv;\n> +\tinfo.prefix = prefix;\n> +\tinfo.quiet = quiet;\n> +\tinfo.recursive = recursive;\n> +\n> +\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> \t\t\t const char *depth, struct string_list *reference,\n> \t\t\t int quiet, int progress)\n> @@ -1212,6 +1359,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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 ea6f56337..032fd2540 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -322,45 +322,8 @@ cmd_foreach()\n> \t\tshift\n> \tdone\n> \n> -\ttoplevel=$(pwd)\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n> \n> -\t# dup stdin so that it can be restored when running the external\n> -\t# command in the subshell (and a recursive call to this function)\n> -\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n> -\t\tthen\n> -\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n> -\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n> -\t\t\tname=$(git submodule--helper name \"$sm_path\")\n> -\t\t\t(\n> -\t\t\t\tprefix=\"$prefix$sm_path/\"\n> -\t\t\t\tsanitize_submodule_env\n> -\t\t\t\tcd \"$sm_path\" &&\n> -\t\t\t\t# we make $path available to scripts ...\n> -\t\t\t\tpath=$sm_path &&\n> -\t\t\t\tsm_path=$displaypath &&\n> -\t\t\t\tif test $# -eq 1\n> -\t\t\t\tthen\n> -\t\t\t\t\teval \"$1\"\n> -\t\t\t\telse\n> -\t\t\t\t\t\"$@\"\n> -\t\t\t\tfi &&\n> -\t\t\t\tif test -n \"$recursive\"\n> -\t\t\t\tthen\n> -\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n> -\t\t\t\tfi\n> -\t\t\t) <&3 3<&- ||\n> -\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n> -\t\tfi\n> -\tdone\n> }\n> \n> #\n> -- \n> 2.11.0\n> \n\nLooking good!\n\n-- \nBrandon Williams\n"},{"id":"320827","messageId":"CAGZ79kYbi5QxWAsxdfPkuWEyMt9Qg753sm0vExsKaWyksDVw+Q@mail.gmail.com","threadId":"45748","inReplyTo":"01e8c552-fd3a-ee05-1ff1-3b3ea0f7feeb@ramsayjones.plus.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-26T17:07:30Z","receivedAt":"2017-05-26T17:07:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones\n wrote:\n>\n>\n> On 26/05/17 16:17, Prathamesh Chavan wrote:\n>> According to the documentation about git-submodule foreach subcommand's\n>> $path variable:\n>> $path is the name of the submodule directory relative to the superproject\n>>\n>> But it was observed when the value of the $path value deviates from this\n>> for the nested submodules when the is run from a subdirectory.\n>> This patch aims for its correction.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Stefan Beller \n>> Signed-off-by: Prathamesh Chavan \n>> ---\n>> This series of patch is based on gitster/jk/bug-to-abort for untilizing its\n>> BUG() macro.\n>>\n>> The observation made was as follows:\n>> For a project - super containing dir (not a submodule) and a submodule sub\n>> which contains another submodule subsub. When we run a command from super/dir:\n>>\n>> git submodule foreach \"echo \\$path-\\$sm_path\"\n>>\n>> actual results:\n>> Entering '../sub'\n>> ../sub-../sub\n>> Entering '../sub/subsub'\n>> ../subsub-../subsub\n>>\n>> expected result wrt documentation and current test suite:\n>> Entering '../sub'\n>> sub-../sub\n>> Entering '../sub/subsub'\n>> subsub-../sub/subsub\n>>\n>> This make the value of $path confusing and I also feel it deviates from its\n>> documentation:\n>> $path is the name of the submodule directory relative to the superproject.\n>> Hence, this patch corrects the value assigned to the $path and $sm_path.\n>>\n>> git-submodule.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index c0d0e9a4c..ea6f56337 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -344,9 +344,9 @@ cmd_foreach()\n>> prefix=\"$prefix$sm_path/\"\n>> sanitize_submodule_env\n>> cd \"$sm_path\" &&\n>> - sm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n>> # we make $path available to scripts ...\n>> path=$sm_path &&\n>> + sm_path=$displaypath &&\n>> if test $# -eq 1\n>> then\n>> eval \"$1\"\n>>\n>\n> Hmm, I'm not sure which documentation you are referring to,\n\nQuite likely our fine manual pages. ;)\n\n foreach [--recursive] \n Evaluates an arbitrary shell command in each checked out submodule.\n The command has access to the variables $name, $path, $sha1 and\n $toplevel: $name is the name of the relevant submodule section in\n .gitmodules, $path is the name of the submodule directory relative\n to the superproject, $sha1 is the commit as recorded in the\n superproject, and $toplevel is the absolute path to the top-level\n of the superproject. Any submodules defined in the superproject but\n not checked out are ignored by this command. Unless given --quiet,\n foreach prints the name of each submodule before evaluating the\n command. If --recursive is given, submodules are traversed\n recursively (i.e. the given shell command is evaluated in nested\n submodules as well). A non-zero return from the command in any\n submodule causes the processing to terminate. This can be\n overridden by adding || : to the end of the command.\n\nAs $path is documented and $sm_path is not, we should care about\n$path first to be correct and either fix the documentation or the implementation\nsuch that we have a consistent world view. :)\n\n> but if\n> $path != $sm_path then something is wrong. (unless their definition\n> has changed, of course).\n\nI would lean in doing so (changing their definition):\n\n $path (as documented) is the name of the submodule directory\n relative to the direct superproject (so in nested submodules you\n go up only one level).\n\n$sm_path on the other hand is not documented at all and yields\nnon-sense results in corner cases.\n\nWith this patch it becomes less non-sensey and could be documented as:\n\n $sm_path is the relative path from the current working directory\n to the submodule (ignoring relations to the superproject or nesting\n of submodules). This documentation also fits into the narrative of\n the test in t7407.\n\nThanks,\nStefan\n"},{"id":"320852","messageId":"41781ba1-2284-cc5d-8736-bb87d580f5bf@ramsayjones.plus.com","threadId":"45748","inReplyTo":"83801d05-e3a9-b7e4-8f29-cd8172257dac@kdbg.org","subject":"Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-27T01:20:40Z","receivedAt":"2017-05-27T01:22:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 26/05/17 22:54, Johannes Sixt wrote:\n> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:\n>> + argv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n> \n> Not good! On Windows, environment variables are case insensitive. The environment variable \"path\" has a very special purpose, although it is generally spelled \"PATH\" (actually \"Path\" on Windows).\n> \n> Lowercase \"path\" may have worked as long as it was only used in a shell script (and perhaps only by lucky coincidence), but this I can pretty much guarantee to fail. (I haven't tested it, though.)\n> \n> The correct fix can only be to rename this variable here and in shell scripts that need the value that is set here.\n\nYeah, I already pointed to commit 64394e3ae9 (but it seems not\nto have registered!), but ...\n\nI tried provoking a failure on cygwin, and I couldn't get it to fail!\nSince Johannes told me about Gfw fork of the msys-runtime, I didn't\neven attempt to try and provoke a failure on MSYS2/MinGW.\n\nSo, maybe it's fixed (no I'm not convinced either) ...\n\nATB,\nRamsay Jones\n\n\n"},{"id":"320855","messageId":"20170526220319.GC4592@google.com","threadId":"45748","inReplyTo":"83801d05-e3a9-b7e4-8f29-cd8172257dac@kdbg.org","subject":"Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-26T22:03:19Z","receivedAt":"2017-05-27T01:48:54Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/26, Johannes Sixt wrote:\n> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:\n> >+\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n> \n> Not good! On Windows, environment variables are case insensitive.\n> The environment variable \"path\" has a very special purpose, although\n> it is generally spelled \"PATH\" (actually \"Path\" on Windows).\n> \n> Lowercase \"path\" may have worked as long as it was only used in a\n> shell script (and perhaps only by lucky coincidence), but this I can\n> pretty much guarantee to fail. (I haven't tested it, though.)\n> \n> The correct fix can only be to rename this variable here and in\n> shell scripts that need the value that is set here.\n> \n> -- Hannes\n\nNice catch, the only reason why this would have worked before was\nbecause it was a shell script before...\n\n-- \nBrandon Williams\n"},{"id":"320856","messageId":"83801d05-e3a9-b7e4-8f29-cd8172257dac@kdbg.org","threadId":"45748","inReplyTo":"20170526151713.10974-3-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-26T21:54:56Z","receivedAt":"2017-05-27T01:49:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:\n> +\targv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n\nNot good! On Windows, environment variables are case insensitive. The \nenvironment variable \"path\" has a very special purpose, although it is \ngenerally spelled \"PATH\" (actually \"Path\" on Windows).\n\nLowercase \"path\" may have worked as long as it was only used in a shell \nscript (and perhaps only by lucky coincidence), but this I can pretty \nmuch guarantee to fail. (I haven't tested it, though.)\n\nThe correct fix can only be to rename this variable here and in shell \nscripts that need the value that is set here.\n\n-- Hannes\n"},{"id":"320858","messageId":"24ebb17b-4324-c6ef-7e3a-5576cda3b595@ramsayjones.plus.com","threadId":"45748","inReplyTo":"CAGZ79kYbi5QxWAsxdfPkuWEyMt9Qg753sm0vExsKaWyksDVw+Q@mail.gmail.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-27T01:10:54Z","receivedAt":"2017-05-27T01:51:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 26/05/17 18:07, Stefan Beller wrote:\n> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones\n> wrote:\n>> Hmm, I'm not sure which documentation you are referring to,\n> \n> Quite likely our fine manual pages. ;)\n> \n> foreach [--recursive] \n> Evaluates an arbitrary shell command in each checked out submodule.\n> The command has access to the variables $name, $path, $sha1 and\n> $toplevel: $name is the name of the relevant submodule section in\n> .gitmodules, $path is the name of the submodule directory relative\n> to the superproject, $sha1 is the commit as recorded in the\n> superproject, and $toplevel is the absolute path to the top-level\n> of the superproject. Any submodules defined in the superproject but\n> not checked out are ignored by this command. Unless given --quiet,\n> foreach prints the name of each submodule before evaluating the\n> command. If --recursive is given, submodules are traversed\n> recursively (i.e. the given shell command is evaluated in nested\n> submodules as well). A non-zero return from the command in any\n> submodule causes the processing to terminate. This can be\n> overridden by adding || : to the end of the command.\n\nI suspected as much, but I was wondering specifically if $sm_path\nhad been documented anywhere. I didn't think so, but ...\n\n> As $path is documented and $sm_path is not, we should care about\n> $path first to be correct and either fix the documentation or the implementation\n> such that we have a consistent world view. :)\n\nSure, but what is that world view? :-D\n\nI suspect that commit 091a6eb0fe did not intend (should not have)\nused $sm_path in that test. If we were to 'fix' that test, would\nit still work?\n\nBack in 2012, the submodule list was generated by filtering the\noutput of 'git ls-files --error-unmatch --stage --'; but I don't\nrecall if (at that time) git-ls-files required being at the top\nof the working tree, or if it would execute fine in a sub-directory.\nSo, it's possible that the documentation of $path was wrong all along.\n;-)\n\nAt that time, by definition, $path == $sm_path. However, you know this\nstuff much better than me (I don't use git-submodule), so ...\n\n>> but if\n>> $path != $sm_path then something is wrong. (unless their definition\n>> has changed, of course).\n> \n> I would lean in doing so (changing their definition):\n> \n> $path (as documented) is the name of the submodule directory\n> relative to the direct superproject (so in nested submodules you\n> go up only one level).\n> \n> $sm_path on the other hand is not documented at all and yields\n> non-sense results in corner cases.\n\nHmm, at what point did '$sm_path yields non-sense results' start\nbeing the case? (perhaps the corner cases need to be fixed first).\n\n> With this patch it becomes less non-sensey and could be documented as:\n> \n> $sm_path is the relative path from the current working directory\n> to the submodule (ignoring relations to the superproject or nesting\n> of submodules). \n\nOK.\n\n> This documentation also fits into the narrative of\n> the test in t7407.\n\nHmm, does it?\n\nATB,\nRamsay Jones\n\n\n"},{"id":"320869","messageId":"ef0b2850-ad3b-e33d-37b7-ece9a305a7a4@ramsayjones.plus.com","threadId":"45748","inReplyTo":"41781ba1-2284-cc5d-8736-bb87d580f5bf@ramsayjones.plus.com","subject":"Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-27T14:06:51Z","receivedAt":"2017-05-27T14:07:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/05/17 02:20, Ramsay Jones wrote:\n> \n> \n> On 26/05/17 22:54, Johannes Sixt wrote:\n>> Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:\n>>> + argv_array_pushf(&cp.env_array, \"path=%s\", list_item->name);\n>>\n>> Not good! On Windows, environment variables are case insensitive. The environment variable \"path\" has a very special purpose, although it is generally spelled \"PATH\" (actually \"Path\" on Windows).\n>>\n>> Lowercase \"path\" may have worked as long as it was only used in a shell script (and perhaps only by lucky coincidence), but this I can pretty much guarantee to fail. (I haven't tested it, though.)\n>>\n>> The correct fix can only be to rename this variable here and in shell scripts that need the value that is set here.\n> \n> Yeah, I already pointed to commit 64394e3ae9 (but it seems not\n> to have registered!), but ...\n> \n> I tried provoking a failure on cygwin, and I couldn't get it to fail!\n\nTo be more explicit, last Sunday I hacked into t7407 to show an\nexample failure on cygwin (see patch below), but it passes on both\nLinux (expected) and cygwin! :( Perhaps you can see what I'm doing\nwrong?\n\nATB,\nRamsay Jones\n\n-- >8 --\nDate: Sun, 21 May 2017 16:23:58 +0100\nSubject: [PATCH] submodule: foreach $path munging on cygwin\n\n---\n t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42..c2d66bab7 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -68,17 +68,36 @@ Entering 'sub3'\n $pwd/clone-foo3-sub3-$sub3sha1\n EOF\n \n+cat >expect-func <>\n+Entering 'sub3'\n+running from TRASH\n+path is <>\n+EOF\n+\n test_expect_success 'test basic \"submodule foreach\" usage' '\n+\tPATH=\"$PWD:$PATH\" &&\n+\twrite_script foreach-func <<-\\EOF &&\n+\techo \"running from TRASH\"\n+\techo \"path is <<$1>>\"\n+\tEOF\n \tgit clone super clone &&\n \t(\n \t\tcd clone &&\n \t\tgit submodule update --init -- sub1 sub3 &&\n \t\tgit submodule foreach \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual &&\n+\t\tgit submodule foreach \"foreach-func \\$path\" > ../actual-func1 &&\n+\t\tgit submodule foreach \"export path; foreach-func \\$path\" > ../actual-func2 &&\n \t\tgit config foo.bar zar &&\n \t\tgit submodule foreach \"git config --file \\\"\\$toplevel/.git/config\\\" foo.bar\"\n \t) &&\n+\ttest_i18ncmp expect-func actual-func1 &&\n+\ttest_i18ncmp expect-func actual-func2 &&\n \ttest_i18ncmp expect actual\n '\n+test_done\n \n cat >expect < To be more explicit, last Sunday I hacked into t7407 to show an\n> example failure on cygwin (see patch below), but it passes on both\n> Linux (expected) and cygwin! :( Perhaps you can see what I'm doing\n> wrong?\n\nAs long as the git.exe you are using here is Cygwin's, Windows \nconventions do not apply. I think, the environment is transfered across \nthe exec boundaries using Cygwin's own tools, and Windows's \ncase-insensitive environment does not enter the game at all. But that's \njust a shot in the dark.\n\n> \n> ATB,\n> Ramsay Jones\n> \n> -- >8 --\n> Date: Sun, 21 May 2017 16:23:58 +0100\n> Subject: [PATCH] submodule: foreach $path munging on cygwin\n> \n> ---\n> t/t7407-submodule-foreach.sh | 19 +++++++++++++++++++\n> 1 file changed, 19 insertions(+)\n> \n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 6ba5daf42..c2d66bab7 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -68,17 +68,36 @@ Entering 'sub3'\n> $pwd/clone-foo3-sub3-$sub3sha1\n> EOF\n> \n> +cat >expect-func < +Entering 'sub1'\n> +running from TRASH\n> +path is <>\n> +Entering 'sub3'\n> +running from TRASH\n> +path is <>\n> +EOF\n> +\n> test_expect_success 'test basic \"submodule foreach\" usage' '\n> +\tPATH=\"$PWD:$PATH\" &&\n> +\twrite_script foreach-func <<-\\EOF &&\n> +\techo \"running from TRASH\"\n> +\techo \"path is <<$1>>\"\n> +\tEOF\n> \tgit clone super clone &&\n> \t(\n> \t\tcd clone &&\n> \t\tgit submodule update --init -- sub1 sub3 &&\n> \t\tgit submodule foreach \"echo \\$toplevel-\\$name-\\$path-\\$sha1\" > ../actual &&\n> +\t\tgit submodule foreach \"foreach-func \\$path\" > ../actual-func1 &&\n> +\t\tgit submodule foreach \"export path; foreach-func \\$path\" > ../actual-func2 &&\n> \t\tgit config foo.bar zar &&\n> \t\tgit submodule foreach \"git config --file \\\"\\$toplevel/.git/config\\\" foo.bar\"\n> \t) &&\n> +\ttest_i18ncmp expect-func actual-func1 &&\n> +\ttest_i18ncmp expect-func actual-func2 &&\n> \ttest_i18ncmp expect actual\n> '\n> +test_done\n> \n> cat >expect < Entering '../sub1'\n> \n\n"},{"id":"321085","messageId":"CAGZ79kYc9wx23N9quxhuYAf2H1Rm3tGNg_0Ydz0KO4qPH-Kz5w@mail.gmail.com","threadId":"45748","inReplyTo":"24ebb17b-4324-c6ef-7e3a-5576cda3b595@ramsayjones.plus.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-30T21:53:47Z","receivedAt":"2017-05-30T21:54:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones\n wrote:\n>\n>\n> On 26/05/17 18:07, Stefan Beller wrote:\n>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones\n>> wrote:\n>>> Hmm, I'm not sure which documentation you are referring to,\n>>\n>> Quite likely our fine manual pages. ;)\n>>\n>> foreach [--recursive] \n>> Evaluates an arbitrary shell command in each checked out submodule.\n>> The command has access to the variables $name, $path, $sha1 and\n>> $toplevel: $name is the name of the relevant submodule section in\n>> .gitmodules, $path is the name of the submodule directory relative\n>> to the superproject, $sha1 is the commit as recorded in the\n>> superproject, and $toplevel is the absolute path to the top-level\n>> of the superproject. Any submodules defined in the superproject but\n>> not checked out are ignored by this command. Unless given --quiet,\n>> foreach prints the name of each submodule before evaluating the\n>> command. If --recursive is given, submodules are traversed\n>> recursively (i.e. the given shell command is evaluated in nested\n>> submodules as well). A non-zero return from the command in any\n>> submodule causes the processing to terminate. This can be\n>> overridden by adding || : to the end of the command.\n>\n> I suspected as much, but I was wondering specifically if $sm_path\n> had been documented anywhere. I didn't think so, but ...\n>\n>> As $path is documented and $sm_path is not, we should care about\n>> $path first to be correct and either fix the documentation or the implementation\n>> such that we have a consistent world view. :)\n>\n> Sure, but what is that world view? :-D\n>\n> I suspect that commit 091a6eb0fe did not intend (should not have)\n> used $sm_path in that test. If we were to 'fix' that test, would\n> it still work?\n>\n> Back in 2012, the submodule list was generated by filtering the\n> output of 'git ls-files --error-unmatch --stage --'; but I don't\n> recall if (at that time) git-ls-files required being at the top\n> of the working tree, or if it would execute fine in a sub-directory.\n> So, it's possible that the documentation of $path was wrong all along.\n> ;-)\n>\n> At that time, by definition, $path == $sm_path. However, you know this\n> stuff much better than me (I don't use git-submodule), so ...\n\nDon't take that stance. Sometimes I shoot quickly from the hip without\nconsidering consequences (Figuratively).\n\nIn a foreach command I can see value both in the 'displaypath'\n(what sm_path would become here) and the 'submodule path'\nfrom the superproject. The naming of 'sm_path' hints at that it ought\nto be the 'submodule path'.\n\n>>\n>> $path (as documented) is the name of the submodule directory\n>> relative to the direct superproject (so in nested submodules you\n>> go up only one level).\n>>\n>> $sm_path on the other hand is not documented at all and yields\n>> non-sense results in corner cases.\n>\n> Hmm, at what point did '$sm_path yields non-sense results' start\n> being the case? (perhaps the corner cases need to be fixed first).\n\nWell the corner case is described in the patchs notes.\nSo that patch would fix it to be consistent with the new world view\n(that I have in mind) as I do not know about the 2012 ideas how submodules\nought to behave correctly.\n\n>> With this patch it becomes less non-sensey and could be documented as:\n>>\n>> $sm_path is the relative path from the current working directory\n>> to the submodule (ignoring relations to the superproject or nesting\n>> of submodules).\n>\n> OK.\n>\n>> This documentation also fits into the narrative of\n>> the test in t7407.\n>\n> Hmm, does it?\n\nAfter rereading that test, I would think so?\n\nThanks for keeping discussing this.\n\nSo maybe we want to\n* keep path=sm_path\n* fix the documentation via s/$path/$sm_path/g in that section quoted above.\n* Introduce a new variable sm_display_path that contains what this patch\n proposes sm_path to be.\n* fix the test in t7407 by checking both sm_path (fixed) as well\n as sm_display_path (what is currently recorded in sm_path)\n---\nIn the next patch:\n* Additionally in the rewrite in C, we would do an\n\n #ifndef WINDOWS /* need to lookup the exact macro */\n argv_array_push(env_vars, \"path=%s\", sm_path);\n #endif\n\nsuch that Windows users are forced to migrate to sm_path\nas path/Path is case sensitive there. sm_path being documented\nvalue, so it should work fine?\n\nThanks,\nStefan\n"},{"id":"321091","messageId":"56bcf006-06f1-1851-87de-5697f988cb08@ramsayjones.plus.com","threadId":"45748","inReplyTo":"CAGZ79kYc9wx23N9quxhuYAf2H1Rm3tGNg_0Ydz0KO4qPH-Kz5w@mail.gmail.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-30T23:07:33Z","receivedAt":"2017-05-30T23:07:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/05/17 22:53, Stefan Beller wrote:\n> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones\n> wrote:\n>> On 26/05/17 18:07, Stefan Beller wrote:\n>>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones\n>>> wrote:\n\n>> Back in 2012, the submodule list was generated by filtering the\n>> output of 'git ls-files --error-unmatch --stage --'; but I don't\n>> recall if (at that time) git-ls-files required being at the top\n>> of the working tree, or if it would execute fine in a sub-directory.\n>> So, it's possible that the documentation of $path was wrong all along.\n>> ;-)\n>>\n>> At that time, by definition, $path == $sm_path. However, you know this\n>> stuff much better than me (I don't use git-submodule), so ...\n> \n> Don't take that stance. Sometimes I shoot quickly from the hip without\n> considering consequences (Figuratively).\n> \n> In a foreach command I can see value both in the 'displaypath'\n> (what sm_path would become here) and the 'submodule path'\n> from the superproject. The naming of 'sm_path' hints at that it ought\n> to be the 'submodule path'.\n\nWell, since I introduced it, I can confidently proclaim that it is\nindeed the 'submodule path'. :-D\n\nAs I said above, I can't remember how git-ls-files worked back then,\nbut it seems that I thought of it as the path to the submodule from\nthe root of the working tree. Again, by definition, $sm_path == $path\n(as documented). Of course, that may have changed since then.\n\n>>> With this patch it becomes less non-sensey and could be documented as:\n>>>\n>>> $sm_path is the relative path from the current working directory\n>>> to the submodule (ignoring relations to the superproject or nesting\n>>> of submodules).\n>>\n>> OK.\n>>\n>>> This documentation also fits into the narrative of\n>>> the test in t7407.\n>>\n>> Hmm, does it?\n> \n> After rereading that test, I would think so?\n\nReally? So, if it differs from $path, then something changed between\ncommit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that\ncommit/test, so take what I say with a pinch of salt ...\n\n> Thanks for keeping discussing this.\n> \n> So maybe we want to\n> * keep path=sm_path\n\nAs I said in commit 64394e3ae9, $path was part of the API, so I could\nnot just rename it, without a deprecation period, etc ... So, I was\n'crossing my fingers' that nobody would export $path in their user\nscripts (not very likely, after all).\n\n> * fix the documentation via s/$path/$sm_path/g in that section quoted above.\n\nSo, \"$path is the name of the submodule directory relative to the\nsuperproject\", as currently documented in the man page, yes?\n\nSo, $sm_path == $path, at least for some period?\n\n> * Introduce a new variable sm_display_path that contains what this patch\n> proposes sm_path to be.\n\nSo, this would be the path from the cwd to the submodule, yes?\n\n> * fix the test in t7407 by checking both sm_path (fixed) as well\n> as sm_display_path (what is currently recorded in sm_path)\n\nHmm, ...\n\n> In the next patch:\n> * Additionally in the rewrite in C, we would do an\n> \n> #ifndef WINDOWS /* need to lookup the exact macro */\n> argv_array_push(env_vars, \"path=%s\", sm_path);\n> #endif\n> \n> such that Windows users are forced to migrate to sm_path\n> as path/Path is case sensitive there. sm_path being documented\n> value, so it should work fine?\n\nWell, as you saw in a separate thread, I can no longer get\ncygwin to fail, so something (probably in the cygwin runtime)\nhas changed since 2012 to make this work now, despite the\ncase insensitive win32 environment block. (This may also be\ntrue of MSYS2, but I haven't tested it).\n\nI have not tested this on MYSY2/MinGW/Git-for-windows, but\nJohannes Sixt was concerned about this, so I guess it may\nstill be a problem there.\n\nI don't know how windows folks will feel about simply\nremoving $path, ...\n\n\nATB,\nRamsay Jones\n\n"},{"id":"321097","messageId":"CAGZ79kYMA6Me_ZnBZQitW7ZSJ0kfvb-LPnH=1gTwhRN-KOe5pA@mail.gmail.com","threadId":"45748","inReplyTo":"56bcf006-06f1-1851-87de-5697f988cb08@ramsayjones.plus.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-30T23:29:42Z","receivedAt":"2017-05-30T23:29:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 30, 2017 at 4:07 PM, Ramsay Jones\n wrote:\n>\n>\n> On 30/05/17 22:53, Stefan Beller wrote:\n>> On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones\n>> wrote:\n>>> On 26/05/17 18:07, Stefan Beller wrote:\n>>>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones\n>>>> wrote:\n>\n>>> Back in 2012, the submodule list was generated by filtering the\n>>> output of 'git ls-files --error-unmatch --stage --'; but I don't\n>>> recall if (at that time) git-ls-files required being at the top\n>>> of the working tree, or if it would execute fine in a sub-directory.\n>>> So, it's possible that the documentation of $path was wrong all along.\n>>> ;-)\n>>>\n>>> At that time, by definition, $path == $sm_path. However, you know this\n>>> stuff much better than me (I don't use git-submodule), so ...\n>>\n>> Don't take that stance. Sometimes I shoot quickly from the hip without\n>> considering consequences (Figuratively).\n>>\n>> In a foreach command I can see value both in the 'displaypath'\n>> (what sm_path would become here) and the 'submodule path'\n>> from the superproject. The naming of 'sm_path' hints at that it ought\n>> to be the 'submodule path'.\n>\n> Well, since I introduced it, I can confidently proclaim that it is\n> indeed the 'submodule path'. :-D\n\nok. :)\n\n> As I said above, I can't remember how git-ls-files worked back then,\n> but it seems that I thought of it as the path to the submodule from\n> the root of the working tree. Again, by definition, $sm_path == $path\n> (as documented). Of course, that may have changed since then.\n\nDocumented in 64394e3 (git-submodule.sh: Don't use $path variable in\neval_gettext string, by yourself)\n\nWhat I intended to say above was \"documented to the end user\",\nand I do not count our commit messages as such. The end user facing\ndocumentation only talks about path, not mentioning sm_path.\n\n>>>> With this patch it becomes less non-sensey and could be documented as:\n>>>>\n>>>> $sm_path is the relative path from the current working directory\n>>>> to the submodule (ignoring relations to the superproject or nesting\n>>>> of submodules).\n>>>\n>>> OK.\n>>>\n>>>> This documentation also fits into the narrative of\n>>>> the test in t7407.\n>>>\n>>> Hmm, does it?\n>>\n>> After rereading that test, I would think so?\n>\n> Really? So, if it differs from $path, then something changed between\n> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that\n> commit/test, so take what I say with a pinch of salt ...\n\nWell yes. I am specifically reading 091a6eb0fe, the changes to t7407.\n\nIn that test sm_path contains the relative path from $PWD to the\nsubmodule. (It does NOT: \"$[sm_]path is the name of the submodule\ndirectory relative to the superproject\" as documented but rather\n... relative to the $PWD)\n\n>\n>> Thanks for keeping discussing this.\n>>\n>> So maybe we want to\n>> * keep path=sm_path\n>\n> As I said in commit 64394e3ae9, $path was part of the API, so I could\n> not just rename it, without a deprecation period, etc ... So, I was\n> 'crossing my fingers' that nobody would export $path in their user\n> scripts (not very likely, after all).\n\nOk. So another approach to get away in the C conversion:\n* export the sm_path as all other environment variables\n* for \"$path\" we do not export it into the environment, but\n prefix the command with it, i.e. we'd ask our shell to run\n \"path=%s; %s\", sm_path, argv[0]\n to preserve the historic behavior.\n\n>\n>> * fix the documentation via s/$path/$sm_path/g in that section quoted above.\n>\n> So, \"$path is the name of the submodule directory relative to the\n> superproject\", as currently documented in the man page, yes?\n\nNo, the documentation does not match reality. The reality is that\nboth sm_path as well as path give the display path.\n\n> So, $sm_path == $path, at least for some period?\n\nyes that is current reality.\n\n>\n>> * Introduce a new variable sm_display_path that contains what this patch\n>> proposes sm_path to be.\n>\n> So, this would be the path from the cwd to the submodule, yes?\n\nyes.\n\n>\n> I don't know how windows folks will feel about simply\n> removing $path, ...\n\nI agree that this is a bad idea now. As said above we'd\njust not export the path as an env variable and should be\n\"fine\" in the sense that we do not break historical expectations,\nbut have to deal with slightly messy code.\n\nJust digging further, there was ea2fa1040d (submodule foreach:\ncorrect path display in recursive submodules, 2016-03-29), which\nis tangent to the issue of whether we think it is a display path\nor the superproject->submodule path.\n\nThanks,\nStefan\n"},{"id":"321102","messageId":"6e1db1f4-abbe-199e-a206-a56d83fac4ff@ramsayjones.plus.com","threadId":"45748","inReplyTo":"CAGZ79kYMA6Me_ZnBZQitW7ZSJ0kfvb-LPnH=1gTwhRN-KOe5pA@mail.gmail.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-31T00:13:28Z","receivedAt":"2017-05-31T00:13:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 31/05/17 00:29, Stefan Beller wrote:\n \n>> As I said above, I can't remember how git-ls-files worked back then,\n>> but it seems that I thought of it as the path to the submodule from\n>> the root of the working tree. Again, by definition, $sm_path == $path\n>> (as documented). Of course, that may have changed since then.\n> \n> Documented in 64394e3 (git-submodule.sh: Don't use $path variable in\n> eval_gettext string, by yourself)\n> \n> What I intended to say above was \"documented to the end user\",\n> and I do not count our commit messages as such. The end user facing\n> documentation only talks about path, not mentioning sm_path.\n\nCorrect, and that is exactly what I was saying. ie. $path as\n'documented to the end user'. (again by _definition_ $sm_path\n_is_ $path).\n\n>>> After rereading that test, I would think so?\n>>\n>> Really? So, if it differs from $path, then something changed between\n>> commit 64394e3ae9 and commit 091a6eb0fe. I haven't really read that\n>> commit/test, so take what I say with a pinch of salt ...\n> \n> Well yes. I am specifically reading 091a6eb0fe, the changes to t7407.\n> \n> In that test sm_path contains the relative path from $PWD to the\n> submodule. (It does NOT: \"$[sm_]path is the name of the submodule\n> directory relative to the superproject\" as documented but rather\n> ... relative to the $PWD)\n\nIn that case, the current user documentation does not agree with\nthe current implementation, yes?\n\nSo, was the user documentation always wrong? (did git-ls-files work\nfrom a sub-directory, limiting its output to the cwd, or did it\nchdir() to the top of the worktree first?).\n\n>> As I said in commit 64394e3ae9, $path was part of the API, so I could\n>> not just rename it, without a deprecation period, etc ... So, I was\n>> 'crossing my fingers' that nobody would export $path in their user\n>> scripts (not very likely, after all).\n> \n> Ok. So another approach to get away in the C conversion:\n> * export the sm_path as all other environment variables\n> * for \"$path\" we do not export it into the environment, but\n> prefix the command with it, i.e. we'd ask our shell to run\n> \"path=%s; %s\", sm_path, argv[0]\n> to preserve the historic behavior.\n\nYes, that would probably work.\n\nATB,\nRamsay Jones\n\n"},{"id":"321104","messageId":"48149a4a-c7e3-d70e-7894-369681587372@ramsayjones.plus.com","threadId":"45748","inReplyTo":"6e1db1f4-abbe-199e-a206-a56d83fac4ff@ramsayjones.plus.com","subject":"Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-31T00:48:43Z","receivedAt":"2017-05-31T00:48:51Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 31/05/17 01:13, Ramsay Jones wrote:\n\n> On 31/05/17 00:29, Stefan Beller wrote:\n> \n>> In that test sm_path contains the relative path from $PWD to the\n>> submodule. (It does NOT: \"$[sm_]path is the name of the submodule\n>> directory relative to the superproject\" as documented but rather\n>> ... relative to the $PWD)\n> \n> In that case, the current user documentation does not agree with\n> the current implementation, yes?\n> \n> So, was the user documentation always wrong? (did git-ls-files work\n> from a sub-directory, limiting its output to the cwd, or did it\n> chdir() to the top of the worktree first?).\n\nTo answer my own question, I rebuilt git and tried directly:\n\n $ pwd\n /home/ramsay/git\n $ \n $ ./git version\n git version 1.7.10.1.g64394\n $ \n $ cd git_remote_helpers/\n $ ../git-ls-files --error-unmatch --stage --\n 100644 2247d5f95a7193c7221b9464debe167763b4fae3 0\t.gitignore\n 100644 74b05dc91e42414147d5f3dc7b4fc66fb86c0eca 0\tMakefile\n 100644 00f69cbeda277b24e8ab35cb7db2c25cc0cc122e 0\t__init__.py\n 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tgit/__init__.py\n 100644 9ee5f96d4ce313f4f94505ff65b560943bfd21cb 0\tgit/exporter.py\n 100644 007a1bfdf37d231470f69d9d0cffa46e80127f34 0\tgit/git.py\n 100644 5c6b595e16665bc508625ab0e96c95776bacba1a 0\tgit/importer.py\n 100644 e70025095dcfb31d3944e72ac1f83dd7d4109103 0\tgit/non_local.py\n 100644 acbf8d7785e2253777456f8910e2352992dda474 0\tgit/repo.py\n 100644 4bff8878d14ccaf02c552073ef55d519df0b4cad 0\tsetup.cfg\n 100644 4d434b65cbf5c42a455d5cd3bced030bfb51a245 0\tsetup.py\n 100644 fbbb01b14619c1d2ed6bcc8f304f019fbe98697f 0\tutil.py\n $ \n \nHmm, so it looks like $path was always relative to cwd!\n\n(got to get some sleep now ...)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"321401","messageId":"20170602112428.11131-1-pc44800@gmail.com","threadId":"45748","inReplyTo":"48149a4a-c7e3-d70e-7894-369681587372@ramsayjones.plus.com","subject":"[GSoC][PATCH v6 1/2] submodule: fix buggy $path and $sm_path variable's value","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-06-02T11:24:27Z","receivedAt":"2017-06-02T11:24:47Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"According to the documentation about git-submodule foreach subcommand's\n$path variable:\n$path is the name of the submodule directory relative to the superproject\n\nBut it was observed when the value of the $path value deviates from this\nfor the nested submodules when the is run from a subdirectory.\nThis patch aims for its correction.\n\nAdditional test cases added to the submodule-foreach test suite in t7407,\nto check the submodule foreach --recursive behavior from a subdirectory\nas this was missing from the test suite.\n\nHelped-by: Brandon Williams \nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\n git-submodule.sh | 2 +-\n t/t7407-submodule-foreach.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c0d0e9a4c..ea6f56337 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -344,9 +344,9 @@ cmd_foreach()\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n \t\t\t\t# we make $path available to scripts ...\n \t\t\t\tpath=$sm_path &&\n+\t\t\t\tsm_path=$displaypath &&\n \t\t\t\tif test $# -eq 1\n \t\t\t\tthen\n \t\t\t\t\teval \"$1\"\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42..f4e07eee3 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -197,6 +197,39 @@ test_expect_success 'test messages from \"foreach --recursive\" from subdirectory'\n \ttest_i18ncmp expect actual\n '\n \n+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)\n+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)\n+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)\n+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)\n+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)\n+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)\n+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)\n+\n+cat >expect <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect <name; and other\nvalue assignment to the env argv_array structure of a child_process.\nAlso the of submodule-foreach is push to args argv_array\nstructure and finally, using run_command the commands are executed\nusing a shell.\n\nThe third function also takes care of the recursive flag, by creating\na separate child_process structure and prepending \"--super-prefix displaypath\",\nto the args argv_array structure. Other required arguments and the\ninput of submodule-foreach is also appended to this argv_array.\n\nHelped-by: Brandon Williams \nMentored-by: Christian Couder \nMentored-by: Stefan Beller \nSigned-off-by: Prathamesh Chavan \n---\nIn this new version of, we avoid exporting the $path variable to\nthe environment, but instead prefix the with it.\nIn this way, we avoid the issue it creates with the env variable\n$PATH in windows.\n\nOther than that, additionally the case of no. of arugments in \nbeing equal to 1 is also considered separetly.\nTHe reason of having this change in the shell script was given in the\ncommit 1c4fb136db.\nAccording to my understanding, eval \"$1\" executes $1 in same shell,\nwhereas \"$@\" gets executed in a separate shell, which doesn't allow\n\"$@\" to access the env variables $name, $path, etc.\nHence, to keep the ported function similar, this condition is also\nadded. \n\nApart from this, other suggested changes are also implemented.\n\nComplete build report is available at:\nhttps://travis-ci.org/pratham-pc/git/builds\nBranch: submodule-foreach\nBuild #87\n \n builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 39 +----------\n 2 files changed, 154 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 566a5b6a6..d08a02ad9 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 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 struct module_list {\n \tconst struct cache_entry **entries;\n \tint alloc, nr;\n@@ -331,6 +353,14 @@ 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,\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 static void init_submodule(const char *path, const char *prefix, int quiet)\n {\n \tconst struct submodule *sub;\n@@ -487,6 +517,128 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cb_foreach {\n+\tint argc;\n+\tconst char **argv;\n+\tconst char *prefix;\n+\tunsigned int quiet: 1;\n+\tunsigned int recursive: 1;\n+};\n+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }\n+\n+static void runcommand_in_submodule(const struct cache_entry *list_item,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct cb_foreach *info = cb_data;\n+\tchar *toplevel;\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tchar *displaypath;\n+\tint i;\n+\n+\tsub = submodule_from_path(null_sha1, list_item->name);\n+\n+\tdisplaypath = get_submodule_displaypath(list_item->name, info->prefix);\n+\n+\tif (!sub)\n+\t\tdie(_(\"No url found for submodule path '%s' in .gitmodules\"),\n+\t\t displaypath);\n+\n+\tif (!is_submodule_populated_gently(list_item->name, NULL))\n+\t\treturn;\n+\n+\ttoplevel = xgetcwd();\n+\n+\tprepare_submodule_repo_env(&cp.env_array);\n+\tcp.use_shell = 1;\n+\tcp.dir = list_item->name;\n+\n+\tif (info->argc == 1) {\n+\t\targv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n+\t\targv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n+\t\targv_array_pushf(&cp.env_array, \"sha1=%s\",\n+\t\t\t\t oid_to_hex(&list_item->oid));\n+\t\targv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n+\n+\t\targv_array_pushf(&cp.args, \"path=%s; %s\", list_item->name,\n+\t\t\t\t info->argv[0]);\n+\t} else {\n+\t\tfor (i = 0; i < info->argc; i++)\n+\t\t\targv_array_push(&cp.args, info->argv[i]);\n+\t}\n+\n+\tif (!info->quiet)\n+\t\tprintf(_(\"Entering '%s'\\n\"), displaypath);\n+\n+\tif (info->argv[0] && run_command(&cp))\n+\t\tdie(_(\"run_command returned non-zero status for %s\\n.\"),\n+\t\t displaypath);\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\", \"foreach\", \"--recursive\",\n+\t\t\t\t NULL);\n+\n+\t\tif (info->quiet)\n+\t\t\targv_array_push(&cpr.args, \"--quiet\");\n+\n+\t\tfor (i = 0; i < info->argc; i++)\n+\t\t\targv_array_push(&cpr.args, info->argv[i]);\n+\n+\t\tif (run_command(&cpr))\n+\t\t\tdie(_(\"run_command returned non-zero status while\"\n+\t\t\t \"recursing in the nested submodules of %s\\n.\"),\n+\t\t\t displaypath);\n+\t}\n+\n+\tfree(displaypath);\n+\tfree(toplevel);\n+}\n+\n+static int module_foreach(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct cb_foreach info;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint quiet = 0;\n+\tint recursive = 0;\n+\n+\tstruct option module_foreach_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output of entering each submodule command\")),\n+\t\tOPT_BOOL(0, \"recursive\", &recursive,\n+\t\t\t 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--helper foreach [--quiet] [--recursive] \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_foreach_options,\n+\t\t\t git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)\n+\t\tBUG(\"module_list_compute should not choke on empty pathspec\");\n+\n+\tinfo.argc = argc;\n+\tinfo.argv = argv;\n+\tinfo.prefix = prefix;\n+\tinfo.quiet = !!quiet;\n+\tinfo.recursive = !!recursive;\n+\n+\tgitmodules_config();\n+\tfor_each_submodule_list(list, runcommand_in_submodule, &info);\n+\n+\treturn 0;\n+}\n+\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t const char *depth, struct string_list *reference,\n \t\t\t int quiet, int progress)\n@@ -1212,6 +1364,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{\"foreach\", module_foreach, SUPPORT_SUPER_PREFIX},\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 ea6f56337..032fd2540 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -322,45 +322,8 @@ cmd_foreach()\n \t\tshift\n \tdone\n \n-\ttoplevel=$(pwd)\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} \"$@\"\n \n-\t# dup stdin so that it can be restored when running the external\n-\t# command in the subshell (and a recursive call to this function)\n-\texec 3<&0\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\tif test -e \"$sm_path\"/.git\n-\t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n-\t\t\tname=$(git submodule--helper name \"$sm_path\")\n-\t\t\t(\n-\t\t\t\tprefix=\"$prefix$sm_path/\"\n-\t\t\t\tsanitize_submodule_env\n-\t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\t# we make $path available to scripts ...\n-\t\t\t\tpath=$sm_path &&\n-\t\t\t\tsm_path=$displaypath &&\n-\t\t\t\tif test $# -eq 1\n-\t\t\t\tthen\n-\t\t\t\t\teval \"$1\"\n-\t\t\t\telse\n-\t\t\t\t\t\"$@\"\n-\t\t\t\tfi &&\n-\t\t\t\tif test -n \"$recursive\"\n-\t\t\t\tthen\n-\t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n-\t\t\t\tfi\n-\t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n-\t\tfi\n-\tdone\n }\n \n #\n-- \n2.13.0\n\n"},{"id":"321468","messageId":"20170603003710.5558-1-sbeller@google.com","threadId":"45748","inReplyTo":"20170515183405.GA79147@google.com","subject":"[PATCH] submodule foreach: correct $sm_path in nested submodules from a dir","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-03T00:37:10Z","receivedAt":"2017-06-03T00:37:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When running 'git submodule foreach' from a subdirectory of your\nrepository, nested submodules get a bogus value for $sm_path:\nFor a submodule 'sub' that contains a nested submodule 'nested',\nrunning 'git -C dir submodule foreach echo $path' would report\npath='../nested' for the nested submodule. The first part '../' is\nderived from the logic computing the relative path from $pwd to the\nroot of the superproject. The second part is the submodule path inside\nthe submodule. This value is of little use and is hard to document.\n\nThere are two different possible solutions that have more value:\n(a) The path value is documented as the path from the toplevel of the\n superproject to the mount point of the submodule.\n In this case we would want to have path='sub/nested'.\n\n(b) As Ramsay noticed the documented value is wrong. For the non-nested\n case the path is equal to the relative path from $pwd to the\n submodules working directory. When following this model,\n the expected value would be path='../sub/nested'.\n\nThe behavior for (b) was introduced in 091a6eb0fe (submodule: drop the\ntop-level requirement, 2013-06-16) the intent for $path seemed to be\nrelative to $cwd to the submodule worktree, but that did not work for\nnested submodules, as the intermittent submodules were not included in\nthe path.\n\nIf we were to fix the meaning of the $path using (a) such that \"path\"\nis \"the path from the toplevel of the superproject to the mount point\nof the submodule\", we would break any existing submodule user that runs\nforeach from non-root of the superproject as the non-nested submodule\n'../sub' would change its path to 'sub'.\n\nIf we would fix the meaning of the $path using (b), such that \"path\"\nis \"the relative path from $pwd to the submodule\", then we would break\nany user that uses nested submodules (even from the root directory) as\nthe 'nested' would become 'sub/nested'.\n\nBoth groups can be found in the wild. The author has no data if one group\noutweighs the other by large margin, and offending each one seems equally\nbad at first. However in the authors imagination it is better to go with\n(a) as running from a sub directory sounds like it is carried out\nby a human rather than by some automation task. With a human on\nthe keyboard the feedback loop is short and the changed behavior can be\nadapted to quickly unlike some automation that can break silently.\n\nTo ameliorate the situation, perform these changes\n* Document 'sm_path' instead of 'path'.\n As using a variable '$path' may be harmful to users due to\n capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't\n use $path variable in eval_gettext string, 2012-04-17). Adjust\n the documentation to advocate for using $sm_path, which contains\n the same value. We still make the 'path' variable available,\n though not documented.\n\n* Clarify the 'toplevel' variable documentation.\n It does not contain the topmost superproject as the author assumed,\n but the direct superproject, such that $toplevel/$sm_path is the\n actual absolute path of the submodule.\n\n* The variable '$displaypath' was accessible but undocumented.\n Rename it '$displaypath' to '$dpath'. Document what it contains.\n Users that are broken by the behavior change of 'sm_path' introduced\n in this commit, can switch from '$path' to '$dpath'.\n\nDiscussed-with: Ramsay Jones \nSigned-off-by: Prathamesh Chavan \nSigned-off-by: Stefan Beller \n---\n\nRamsay,\nI would think this would be least offensive to all parties involved.\nWith my current understanding of the situation\nI think this is the best fix for now.\n\nPrathamesh, sorry for the missleading suggestions earlier\non how to approach this bug. Let's see how the discussion turns out on this\none before rebasing the rewrite in C on this one.\n\nThanks,\nStefan\n\n Documentation/git-submodule.txt | 32 ++++++++++++++++++--------------\n git-submodule.sh | 7 +++----\n t/t7407-submodule-foreach.sh | 39 ++++++++++++++++++++++++++++++++++++---\n 3 files changed, 57 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex 74bc6200d5..52e3ef1325 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -218,20 +218,24 @@ information too.\n \n foreach [--recursive] ::\n \tEvaluates an arbitrary shell command in each checked out submodule.\n-\tThe command has access to the variables $name, $path, $sha1 and\n-\t$toplevel:\n-\t$name is the name of the relevant submodule section in .gitmodules,\n-\t$path is the name of the submodule directory relative to the\n-\tsuperproject, $sha1 is the commit as recorded in the superproject,\n-\tand $toplevel is the absolute path to the top-level of the superproject.\n-\tAny submodules defined in the superproject but not checked out are\n-\tignored by this command. Unless given `--quiet`, foreach prints the name\n-\tof each submodule before evaluating the command.\n-\tIf `--recursive` is given, submodules are traversed recursively (i.e.\n-\tthe given shell command is evaluated in nested submodules as well).\n-\tA non-zero return from the command in any submodule causes\n-\tthe processing to terminate. This can be overridden by adding '|| :'\n-\tto the end of the command.\n+\tThe command has access to the following variables:\n++\n+* `$name` is the name of the relevant submodule section in .gitmodules,\n+* `$sha1` is the commit as recorded in the superproject.\n+* `$sm_path` is the path recorded in the superproject.\n+* `$toplevel` is the absolute path to its superproject, such that\n+ `$toplevel/$sm_path` is the absolute path of the submodule.\n+* `$dpath` contains the relative path from the current working directory\n+ to the submodules root directory.\n++\n+Any submodules defined in the superproject but not checked out are\n+ignored by this command. Unless given `--quiet`, foreach prints the name\n+of each submodule before evaluating the command.\n+If `--recursive` is given, submodules are traversed recursively (i.e.\n+the given shell command is evaluated in nested submodules as well).\n+A non-zero return from the command in any submodule causes\n+the processing to terminate. This can be overridden by adding '|| :'\n+to the end of the command.\n +\n As an example, the command below will show the path and currently\n checked out commit for each submodule:\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex c0d0e9a4c6..8133640fa1 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -337,14 +337,13 @@ cmd_foreach()\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n-\t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n-\t\t\tsay \"$(eval_gettext \"Entering '\\$displaypath'\")\"\n+\t\t\tdpath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n+\t\t\tsay \"$(eval_gettext \"Entering '\\$dpath'\")\"\n \t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tsanitize_submodule_env\n \t\t\t\tcd \"$sm_path\" &&\n-\t\t\t\tsm_path=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\") &&\n \t\t\t\t# we make $path available to scripts ...\n \t\t\t\tpath=$sm_path &&\n \t\t\t\tif test $# -eq 1\n@@ -358,7 +357,7 @@ cmd_foreach()\n \t\t\t\t\tcmd_foreach \"--recursive\" \"$@\"\n \t\t\t\tfi\n \t\t\t) <&3 3<&- ||\n-\t\t\tdie \"$(eval_gettext \"Stopping at '\\$displaypath'; script returned non-zero status.\")\"\n+\t\t\tdie \"$(eval_gettext \"Stopping at '\\$dpath'; script returned non-zero status.\")\"\n \t\tfi\n \tdone\n }\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 6ba5daf42e..a082ca75aa 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -82,16 +82,16 @@ test_expect_success 'test basic \"submodule foreach\" usage' '\n \n cat >expect <../../actual\n+\t\tgit submodule foreach \"echo \\$toplevel-\\$name-\\$sm_path-\\$dpath-\\$sha1\" >../../actual\n \t) &&\n \ttest_i18ncmp expect actual\n '\n@@ -197,6 +197,39 @@ test_expect_success 'test messages from \"foreach --recursive\" from subdirectory'\n \ttest_i18ncmp expect actual\n '\n \n+sub1sha1=$(cd clone2/sub1 && git rev-parse HEAD)\n+sub2sha1=$(cd clone2/sub2 && git rev-parse HEAD)\n+sub3sha1=$(cd clone2/sub3 && git rev-parse HEAD)\n+nested1sha1=$(cd clone2/nested1 && git rev-parse HEAD)\n+nested2sha1=$(cd clone2/nested1/nested2 && git rev-parse HEAD)\n+nested3sha1=$(cd clone2/nested1/nested2/nested3 && git rev-parse HEAD)\n+submodulesha1=$(cd clone2/nested1/nested2/nested3/submodule && git rev-parse HEAD)\n+\n+cat >expect <../../actual\n+\t) &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat > expect < wrote:\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n> The code is split up to have one function to obtain all the list of\n> submodules. This function acts as the front-end of git-submodule foreach\n> subcommand. It calls the function for_each_submodule_list, which basically\n> loops through the list and calls function fn, which in this case is\n> runcommand_in_submodule. This third function is a calling function that\n> takes care of running the command in that submodule, and recursively\n> perform the same when --recursive is flagged.\n>\n> The first function module_foreach first parses the options present in\n> argv, 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\n> list, and calls function fn (which in case of submodule subcommand\n> foreach is runcommand_in_submodule) is called for each entry.\n>\n> The third function runcommand_in_submodule, generates a submodule struct sub\n> for $name, value and then later prepends name=sub->name; and other\n> value assignment to the env argv_array structure of a child_process.\n> Also the of submodule-foreach is push to args argv_array\n> structure and finally, using run_command the commands are executed\n> using a shell.\n>\n> The third function also takes care of the recursive flag, by creating\n> a separate child_process structure and prepending \"--super-prefix displaypath\",\n> to the args argv_array structure. Other required arguments and the\n> input of submodule-foreach is also appended to this argv_array.\n>\n\nIs the commit message still accurate?\nYou describe the changes between the versions below the --- line,\nthat is not recorded in the permanent commit history.\n\nIn the commit message is less important to write \"what\" is happening,\nbecause that can easily be read from the patch/commit itself, but rather\n\"why\" things happen, such as design choices, maybe:\n\n This aims to make git-submodule foreach a builtin. This is the very\n first step taken in this direction. Hence, 'foreach' is ported to\n submodule--helper, and submodule--helper is called from git-submodule.sh.\n\n We'll introduce 3 functions, one that is exposed to the command line\n and handles command line arguments, one to iterate over a set of\n submodules, and finally one to execute an arbitrary shell command\n in the submodule.\n\n Attention must be paid to the 'path' variable, see 64394e3ae9\n (git-submodule.sh: Don't use $path variable in eval_gettext string,\n 2012-04-17) details. The path varialbe is not exposed into the environment\n of the invoked shell, but we just give \"path=%s;\" as the first argument.\n\n We do not need to condition on the number of arguments as in 1c4fb136db\n (submodule foreach: skip eval for more than one argument, 2013-09-27)\n as we will run exactly one shell in the submodules directory.\n\n Sign-off-...\n\n>\n> Other than that, additionally the case of no. of arugments in \n> being equal to 1 is also considered separetly.\n> THe reason of having this change in the shell script was given in the\n> commit 1c4fb136db.\n> According to my understanding, eval \"$1\" executes $1 in same shell,\n> whereas \"$@\" gets executed in a separate shell, which doesn't allow\n> \"$@\" to access the env variables $name, $path, etc.\n> Hence, to keep the ported function similar, this condition is also\n> added.\n\nThis paragraph would be a good candidate for the commit message, too.\nHowever as we rewrite it in C, we will spawn exactly one shell no matter\nhow many arguments we have (well for 0 we have no shell, but for 1 or more\narguments we'll spawn exactly one shell?)\n\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\nNote to self (or any other that is interested in long term clean code):\n I have seen this pattern a couple of times, a strbuf just to appease\n the argument list of relative_path.\n (init_submodule, prepare_to_clone_next_submodule,\n resolve_relative_path in submodule--helper\n cmd_rev_parse in builtin/rev-parse\n connect_work_tree_and_git_dir in dir.c\n write_name_quoted_relative in quote.c\n get_superproject_working_tree in submodule.c\n cmd_main in test-path-utils;\n actually all uses of this function :( )\n We should really make a relative_path function that can work\n without the need of a strbuf, maybe just wrap the 3 lines into a new\n function, or remove the strbuf from the argument list.\n (The potential memleak is horrible to fix though. But as seen here\n we could just always return an allocated string and\n mandate the caller to free it)\n\n> +struct cb_foreach {\n> + int argc;\n> + const char **argv;\n> + const char *prefix;\n> + unsigned int quiet: 1;\n> + unsigned int recursive: 1;\n> +};\n> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }\n> +\n> +static void runcommand_in_submodule(const struct cache_entry *list_item,\n> + void *cb_data)\n\nAs we only ever use list_item->name, we could also change\nthe argument list to take a \"const char *[submodule_]path\".\n\n> + prepare_submodule_repo_env(&cp.env_array);\n> + cp.use_shell = 1;\n> + cp.dir = list_item->name;\n> +\n> + if (info->argc == 1) {\n> + argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> + argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n> + argv_array_pushf(&cp.env_array, \"sha1=%s\",\n> + oid_to_hex(&list_item->oid));\n> + argv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n> +\n> + argv_array_pushf(&cp.args, \"path=%s; %s\", list_item->name,\n> + info->argv[0]);\n\nIn the argc != 1 case we also want to have the env_array filled with\nuseful variables. (It seems we do not have tests for that?)\nTo test for that we'd need a test similar as in 1c4fb136d,\njust with\n git submodule foreach echo \\$sm_path \\$dpath\nfor example.\n\nSo I think you can move the pushes to the env array outside this condition.\nAs we set cp.use_shell unconditionally, we do not need to construct\nthe first argument specially with path preset, but instead we can just prepend\nit in the array:\n\n argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n ... // more env stuff\n\n argv_array_pushf(&cp.args, \"path=%s;\", list_item->name);\n for (i = 0; i < info->argc; i++)\n argv_array_push(&cp.args, info->argv[i]);\n\nshould do?\n\n\n> +\n> + if (info->argv[0] && run_command(&cp))\n> + die(_(\"run_command returned non-zero status for %s\\n.\"),\n\nThis would rather be\n die(_(\"Stopping at '%s'; script returned non-zero status.\"), displaypath);\nto imitate the shell version faithfully?\n\n\n> +\n> + if (run_command(&cpr))\n> + die(_(\"run_command returned non-zero status while\"\n> + \"recursing in the nested submodules of %s\\n.\"),\n> + displaypath);\n\nsame here. As the inner process would have already printed the \"Stopping...\"\nwe would not need to repeat it, though.\n\nSo maybe\n\n /* no need to report error, child does: */\n run_command(&cpr);\n\nThe rest below looks good. :)\n\nThanks,\nStefan\n"},{"id":"321479","messageId":"0a0ad045-0c58-aaaf-d0ac-a4fa17570356@ramsayjones.plus.com","threadId":"45748","inReplyTo":"20170603003710.5558-1-sbeller@google.com","subject":"Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-06-03T14:07:17Z","receivedAt":"2017-06-03T14:07:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/17 01:37, Stefan Beller wrote:\n> When running 'git submodule foreach' from a subdirectory of your\n> repository, nested submodules get a bogus value for $sm_path:\n> For a submodule 'sub' that contains a nested submodule 'nested',\n> running 'git -C dir submodule foreach echo $path' would report\n> path='../nested' for the nested submodule. The first part '../' is\n> derived from the logic computing the relative path from $pwd to the\n> root of the superproject. The second part is the submodule path inside\n> the submodule. This value is of little use and is hard to document.\n> \n> There are two different possible solutions that have more value:\n> (a) The path value is documented as the path from the toplevel of the\n> superproject to the mount point of the submodule.\n> In this case we would want to have path='sub/nested'.\n> \n> (b) As Ramsay noticed the documented value is wrong. For the non-nested\n> case the path is equal to the relative path from $pwd to the\n> submodules working directory. When following this model,\n> the expected value would be path='../sub/nested'.\n> \n> The behavior for (b) was introduced in 091a6eb0fe (submodule: drop the\n\nAh, so prior to 091a6eb0fe the documentation of $path was actually\ncorrect - you could not run the command from a sub-directory, so\n$path _was_ the 'name of the submodule directory relative to the\nsuperproject'. (The fact that git-ls-files worked from a subdirectory\nis not in the least relevant - it never was!) ;-)\n\n> top-level requirement, 2013-06-16) the intent for $path seemed to be\n> relative to $cwd to the submodule worktree, but that did not work for\n> nested submodules, as the intermittent submodules were not included in\n ^^^^^^^^^^^^\nintermediate?\n\nHmm, so nobody noticed the change in behaviour (and, of course, that\nthe documentation hadn't been updated to match) since 2013!\n\n> the path.\n> \n> If we were to fix the meaning of the $path using (a) such that \"path\"\n> is \"the path from the toplevel of the superproject to the mount point\n> of the submodule\", we would break any existing submodule user that runs\n> foreach from non-root of the superproject as the non-nested submodule\n> '../sub' would change its path to 'sub'.\n> \n> If we would fix the meaning of the $path using (b), such that \"path\"\n> is \"the relative path from $pwd to the submodule\", then we would break\n> any user that uses nested submodules (even from the root directory) as\n> the 'nested' would become 'sub/nested'.\n> \n> Both groups can be found in the wild. The author has no data if one group\n> outweighs the other by large margin, and offending each one seems equally\n> bad at first. However in the authors imagination it is better to go with\n> (a) as running from a sub directory sounds like it is carried out\n> by a human rather than by some automation task. With a human on\n> the keyboard the feedback loop is short and the changed behavior can be\n> adapted to quickly unlike some automation that can break silently.\n\nI do not use submodules (I have absolutely no interest in them, except\nin a general wish to improve git sense). So, I have no idea what kind\nof impact either change will have. However, FWIW, I agree with your\nreasoning here. (Not that I actually get a vote, but I vote for a!)\n\n> To ameliorate the situation, perform these changes\n> * Document 'sm_path' instead of 'path'.\n> As using a variable '$path' may be harmful to users due to\n> capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't\n> use $path variable in eval_gettext string, 2012-04-17). Adjust\n> the documentation to advocate for using $sm_path, which contains\n> the same value. We still make the 'path' variable available,\n> though not documented.\n> \n> * Clarify the 'toplevel' variable documentation.\n> It does not contain the topmost superproject as the author assumed,\n> but the direct superproject, such that $toplevel/$sm_path is the\n> actual absolute path of the submodule.\n> \n> * The variable '$displaypath' was accessible but undocumented.\n> Rename it '$displaypath' to '$dpath'. Document what it contains.\n> Users that are broken by the behavior change of 'sm_path' introduced\n> in this commit, can switch from '$path' to '$dpath'.\n> \n> Discussed-with: Ramsay Jones \n> Signed-off-by: Prathamesh Chavan \n> Signed-off-by: Stefan Beller \n> ---\n> \n\n Documentation/git-submodule.txt | 32 ++++++++++++++++++--------------\n> git-submodule.sh | 7 +++----\n> t/t7407-submodule-foreach.sh | 39 ++++++++++++++++++++++++++++++++++++---\n> 3 files changed, 57 insertions(+), 21 deletions(-)\n> \n> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\n> index 74bc6200d5..52e3ef1325 100644\n> --- a/Documentation/git-submodule.txt\n> +++ b/Documentation/git-submodule.txt\n> @@ -218,20 +218,24 @@ information too.\n> \n> foreach [--recursive] ::\n> \tEvaluates an arbitrary shell command in each checked out submodule.\n> -\tThe command has access to the variables $name, $path, $sha1 and\n> -\t$toplevel:\n> -\t$name is the name of the relevant submodule section in .gitmodules,\n> -\t$path is the name of the submodule directory relative to the\n> -\tsuperproject, $sha1 is the commit as recorded in the superproject,\n> -\tand $toplevel is the absolute path to the top-level of the superproject.\n> -\tAny submodules defined in the superproject but not checked out are\n> -\tignored by this command. Unless given `--quiet`, foreach prints the name\n> -\tof each submodule before evaluating the command.\n> -\tIf `--recursive` is given, submodules are traversed recursively (i.e.\n> -\tthe given shell command is evaluated in nested submodules as well).\n> -\tA non-zero return from the command in any submodule causes\n> -\tthe processing to terminate. This can be overridden by adding '|| :'\n> -\tto the end of the command.\n> +\tThe command has access to the following variables:\n> ++\n> +* `$name` is the name of the relevant submodule section in .gitmodules,\n> +* `$sha1` is the commit as recorded in the superproject.\n> +* `$sm_path` is the path recorded in the superproject.\n\nJust to be sure, the 'path recorded in the superproject' means the\nsame thing as the 'name of the submodule directory relative to the\nsuperproject'. Yes?\n\n> +* `$toplevel` is the absolute path to its superproject, such that\n> + `$toplevel/$sm_path` is the absolute path of the submodule.\n> +* `$dpath` contains the relative path from the current working directory\n> + to the submodules root directory.\n\nSubject to the above, this all looks good to me. (I can't comment\non the implementation - I just assume that it correctly implements\nthe above).\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n"},{"id":"321507","messageId":"CAME+mvUSGAFbN5j-_hv7QpAS57hq4wgH+yZ7XJMPuyQN1gALaA@mail.gmail.com","threadId":"45748","inReplyTo":"CAGZ79kbDpyO5uRCPnhCWmaFKhyLEUN7mYO9PhU3xF0f=W35ofw@mail.gmail.com","subject":"Re: [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-06-04T10:32:29Z","receivedAt":"2017-06-04T10:32:48Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"On Sat, Jun 3, 2017 at 7:43 AM, Stefan Beller wrote:\n> On Fri, Jun 2, 2017 at 4:24 AM, Prathamesh Chavan wrote:\n>> This aims to make git-submodule foreach a builtin. This is the very\n>> first step taken in this direction. Hence, 'foreach' is ported to\n>> submodule--helper, and submodule--helper is called from git-submodule.sh.\n>> The code is split up to have one function to obtain all the list of\n>> submodules. This function acts as the front-end of git-submodule foreach\n>> subcommand. It calls the function for_each_submodule_list, which basically\n>> loops through the list and calls function fn, which in this case is\n>> runcommand_in_submodule. This third function is a calling function that\n>> takes care of running the command in that submodule, and recursively\n>> perform the same when --recursive is flagged.\n>>\n>> The first function module_foreach first parses the options present in\n>> argv, 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\n>> list, and calls function fn (which in case of submodule subcommand\n>> foreach is runcommand_in_submodule) is called for each entry.\n>>\n>> The third function runcommand_in_submodule, generates a submodule struct sub\n>> for $name, value and then later prepends name=sub->name; and other\n>> value assignment to the env argv_array structure of a child_process.\n>> Also the of submodule-foreach is push to args argv_array\n>> structure and finally, using run_command the commands are executed\n>> using a shell.\n>>\n>> The third function also takes care of the recursive flag, by creating\n>> a separate child_process structure and prepending \"--super-prefix displaypath\",\n>> to the args argv_array structure. Other required arguments and the\n>> input of submodule-foreach is also appended to this argv_array.\n>>\n>\n> Is the commit message still accurate?\n> You describe the changes between the versions below the --- line,\n> that is not recorded in the permanent commit history.\n>\n> In the commit message is less important to write \"what\" is happening,\n> because that can easily be read from the patch/commit itself, but rather\n> \"why\" things happen, such as design choices, maybe:\n>\n> This aims to make git-submodule foreach a builtin. This is the very\n> first step taken in this direction. Hence, 'foreach' is ported to\n> submodule--helper, and submodule--helper is called from git-submodule.sh.\n>\n> We'll introduce 3 functions, one that is exposed to the command line\n> and handles command line arguments, one to iterate over a set of\n> submodules, and finally one to execute an arbitrary shell command\n> in the submodule.\n>\n> Attention must be paid to the 'path' variable, see 64394e3ae9\n> (git-submodule.sh: Don't use $path variable in eval_gettext string,\n> 2012-04-17) details. The path varialbe is not exposed into the environment\n> of the invoked shell, but we just give \"path=%s;\" as the first argument.\n>\n> We do not need to condition on the number of arguments as in 1c4fb136db\n> (submodule foreach: skip eval for more than one argument, 2013-09-27)\n> as we will run exactly one shell in the submodules directory.\n>\n> Sign-off-...\n>\n>>\n>> Other than that, additionally the case of no. of arugments in \n>> being equal to 1 is also considered separetly.\n>> THe reason of having this change in the shell script was given in the\n>> commit 1c4fb136db.\n>> According to my understanding, eval \"$1\" executes $1 in same shell,\n>> whereas \"$@\" gets executed in a separate shell, which doesn't allow\n>> \"$@\" to access the env variables $name, $path, etc.\n>> Hence, to keep the ported function similar, this condition is also\n>> added.\n>\n> This paragraph would be a good candidate for the commit message, too.\n> However as we rewrite it in C, we will spawn exactly one shell no matter\n> how many arguments we have (well for 0 we have no shell, but for 1 or more\n> arguments we'll spawn exactly one shell?)\n>\n\nI was trying to explaing the condition of the code in git-submodule.sh,\nbefore porting. To be more clear, I meant that when we run the command\neval \"$1\", it runs in the same shell in which the cmd_foreach has been running,\nunlike in the case of \"$@\", in which case, the command in executed in a separate\nshell.\n\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>\n> Note to self (or any other that is interested in long term clean code):\n> I have seen this pattern a couple of times, a strbuf just to appease\n> the argument list of relative_path.\n> (init_submodule, prepare_to_clone_next_submodule,\n> resolve_relative_path in submodule--helper\n> cmd_rev_parse in builtin/rev-parse\n> connect_work_tree_and_git_dir in dir.c\n> write_name_quoted_relative in quote.c\n> get_superproject_working_tree in submodule.c\n> cmd_main in test-path-utils;\n> actually all uses of this function :( )\n> We should really make a relative_path function that can work\n> without the need of a strbuf, maybe just wrap the 3 lines into a new\n> function, or remove the strbuf from the argument list.\n> (The potential memleak is horrible to fix though. But as seen here\n> we could just always return an allocated string and\n> mandate the caller to free it)\n>\n>> +struct cb_foreach {\n>> + int argc;\n>> + const char **argv;\n>> + const char *prefix;\n>> + unsigned int quiet: 1;\n>> + unsigned int recursive: 1;\n>> +};\n>> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }\n>> +\n>> +static void runcommand_in_submodule(const struct cache_entry *list_item,\n>> + void *cb_data)\n>\n> As we only ever use list_item->name, we could also change\n> the argument list to take a \"const char *[submodule_]path\".\n>\n>> + prepare_submodule_repo_env(&cp.env_array);\n>> + cp.use_shell = 1;\n>> + cp.dir = list_item->name;\n>> +\n>> + if (info->argc == 1) {\n>> + argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n>> + argv_array_pushf(&cp.env_array, \"sm_path=%s\", displaypath);\n>> + argv_array_pushf(&cp.env_array, \"sha1=%s\",\n>> + oid_to_hex(&list_item->oid));\n>> + argv_array_pushf(&cp.env_array, \"toplevel=%s\", toplevel);\n>> +\n>> + argv_array_pushf(&cp.args, \"path=%s; %s\", list_item->name,\n>> + info->argv[0]);\n>\n> In the argc != 1 case we also want to have the env_array filled with\n> useful variables. (It seems we do not have tests for that?)\n> To test for that we'd need a test similar as in 1c4fb136d,\n> just with\n> git submodule foreach echo \\$sm_path \\$dpath\n> for example.\n>\n> So I think you can move the pushes to the env array outside this condition.\n> As we set cp.use_shell unconditionally, we do not need to construct\n> the first argument specially with path preset, but instead we can just prepend\n> it in the array:\n>\n> argv_array_pushf(&cp.env_array, \"name=%s\", sub->name);\n> ... // more env stuff\n>\n> argv_array_pushf(&cp.args, \"path=%s;\", list_item->name);\n> for (i = 0; i < info->argc; i++)\n> argv_array_push(&cp.args, info->argv[i]);\n>\n> should do?\n>\n>\nYes, even I think this is what should be done. But for the given code,\n\nif test $# -eq 1\nthen\n eval \"$1\"\nelse\n \"$@\"\nfi &&\n\nin case when $# is not equal to 1,\nthe \"$@\" gets executed in a separate shell and hence could not\naccess the $name, $path, $toplevel, $sha1 variables.\nInfact, it can be observed, that the output of the \necho \\$name is something bogus and not the submodule's name.\n\nAlso, in the given test suite, the env variables are used only in those\ncases where no. of arguments is one.\n\nHence, to keep the ported code similar to the privious one, such an\nadditional case of argc == 1 needs to be considered.\n\nThanks,\nPrathamesh Chavan\n"},{"id":"321509","messageId":"1fc84ffc-aa8d-5da4-bae1-8db9865f6ce5@ramsayjones.plus.com","threadId":"45748","inReplyTo":"0a0ad045-0c58-aaaf-d0ac-a4fa17570356@ramsayjones.plus.com","subject":"Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-06-04T15:05:12Z","receivedAt":"2017-06-04T15:05:20Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/06/17 15:07, Ramsay Jones wrote:\n[snip]\n\n>> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\n>> index 74bc6200d5..52e3ef1325 100644\n>> --- a/Documentation/git-submodule.txt\n>> +++ b/Documentation/git-submodule.txt\n>> @@ -218,20 +218,24 @@ information too.\n>> \n>> foreach [--recursive] ::\n>> \tEvaluates an arbitrary shell command in each checked out submodule.\n>> -\tThe command has access to the variables $name, $path, $sha1 and\n>> -\t$toplevel:\n>> -\t$name is the name of the relevant submodule section in .gitmodules,\n>> -\t$path is the name of the submodule directory relative to the\n>> -\tsuperproject, $sha1 is the commit as recorded in the superproject,\n>> -\tand $toplevel is the absolute path to the top-level of the superproject.\n>> -\tAny submodules defined in the superproject but not checked out are\n>> -\tignored by this command. Unless given `--quiet`, foreach prints the name\n>> -\tof each submodule before evaluating the command.\n>> -\tIf `--recursive` is given, submodules are traversed recursively (i.e.\n>> -\tthe given shell command is evaluated in nested submodules as well).\n>> -\tA non-zero return from the command in any submodule causes\n>> -\tthe processing to terminate. This can be overridden by adding '|| :'\n>> -\tto the end of the command.\n>> +\tThe command has access to the following variables:\n>> ++\n>> +* `$name` is the name of the relevant submodule section in .gitmodules,\n>> +* `$sha1` is the commit as recorded in the superproject.\n>> +* `$sm_path` is the path recorded in the superproject.\n> \n> Just to be sure, the 'path recorded in the superproject' means the\n> same thing as the 'name of the submodule directory relative to the\n> superproject'. Yes?\n> \n>> +* `$toplevel` is the absolute path to its superproject, such that\n>> + `$toplevel/$sm_path` is the absolute path of the submodule.\n>> +* `$dpath` contains the relative path from the current working directory\n>> + to the submodules root directory.\n\nBTW, I have not given any thought to what happens to these\nvariables/paths if you run '--recursive'. I assume the new\n'recursed' submodule takes on the role of the 'superproject'\nin the definitions above. Hmm, but what does $dpath show?\n(having 'recursed', is the cwd now at the top level of the\n'new' superproject?)\n\nHmm, you have changed several command to recurse into\nsubmodule(s) (I haven't followed that effort closely), so\nI suppose it should have some measure of consistency with\nthose commands. (whatever that means ;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"321569","messageId":"20170605222031.GB21733@aiede.mtv.corp.google.com","threadId":"45748","inReplyTo":"20170603003710.5558-1-sbeller@google.com","subject":"Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-06-05T22:20:31Z","receivedAt":"2017-06-05T22:20:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThis patch seems to aim to do multiple things. Initial thoughts:\n\nStefan Beller wrote:\n\n[...]\n> To ameliorate the situation, perform these changes\n> * Document 'sm_path' instead of 'path'.\n> As using a variable '$path' may be harmful to users due to\n> capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't\n> use $path variable in eval_gettext string, 2012-04-17). Adjust\n> the documentation to advocate for using $sm_path, which contains\n> the same value. We still make the 'path' variable available,\n> though not documented.\n\nMaking sm_path part of the public API as described here sounds like a\ngood idea (as a separate patch), to avoid conflicting with $PATH on\nWindows. It's convenient that scripts have access to the private\nvariable 'sm_path'. The 'path' variable would still need to be\ndocumented as a deprecated synonym so people working with existing\nscripts can know how to update them.\n\n> * Clarify the 'toplevel' variable documentation.\n> It does not contain the topmost superproject as the author assumed,\n> but the direct superproject, such that $toplevel/$sm_path is the\n> actual absolute path of the submodule.\n\nThis is very confusing. I suspect it's a bug. Can we make 'toplevel'\npoint to the topmost superproject (as a separate path)?\n\n> * The variable '$displaypath' was accessible but undocumented.\n> Rename it '$displaypath' to '$dpath'. Document what it contains.\n> Users that are broken by the behavior change of 'sm_path' introduced\n> in this commit, can switch from '$path' to '$dpath'.\n\nWhat does dpath stand for? Renaming the variable to $dpath means that\nscripts trying to adapt to this change would not work with previous\nversions of git. Would it make sense to use $displaypath for this for\ncompatibility?\n\nWhat is the intent behind the sm_path behavior change in this patch?\nStepping back, what kind of scripts is this interface meant to support\n(e.g., what is an example script that used this interface that would\nbe affected), and is there a straightforward way to support those use\ncases without breaking existing scripts except where necessary?\n\nTo summarize, the patch leaves me a bit confused. I think it would be\nbest to have multiple patches that solve one problem at a time, which\nwould hopefully make the story clearer.\n\nThanks and hope that helps,\nJonathan\n"}]}