{"thread":{"id":"54826","subject":"[PATCH v3 0/3] submodule: port subcommand add from shell to C","startedAt":"2020-12-14T23:20:52Z","lastAt":"2020-12-22T23:43:20Z","messageCount":10,"participants":["Shourya Shukla","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"412219","messageId":"20201214231939.644175-1-periperidip@gmail.com","threadId":"54826","inReplyTo":null,"subject":"[PATCH v3 0/3] submodule: port subcommand add from shell to C","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2020-12-14T23:19:36Z","receivedAt":"2020-12-14T23:20:52Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Greetings,\n\nThis is the v3 of the patch series with the same title. You may view\nthe v2 here:\nhttps://lore.kernel.org/git/20201007074538.25891-1-shouryashukla.oo@gmail.com/\n\nI have applied almost all of the changes asked before, except a few\nwhich confused me a little. It would be great if I could get some help\nabout them:\n\n    1. In this mail: https://lore.kernel.org/git/xmqqo8ldznjx.fsf@gitster.c.googlers.com/\n       Junio asked me to accomodate for a merge in progress since\n       'cache_pos < 0' does not necessarily mean that the path exists.\n       I was wondering which function would be the most appropriate for\n       the if-statements:\n            if (!force) {\n\t\t        if (cache_pos >= 0 || dir_in_cache)\n            }\n       I was thinking of going with 'read_cache_unmerged()'. I thought\n       of this by seeing what is triggered in case of a merge conflict\n       and cam across this. What is your opinion on this?\n\n    2. In this mail: https://lore.kernel.org/git/xmqqimbky6st.fsf@gitster.c.googlers.com/\n       In this section:\n            /* strip trailing '/' */\n\t        if (is_dir_sep(sm_path[strlen(sm_path) -1]))\n\t\t        sm_path[strlen(sm_path) - 1] = '\\0';\n\n       Junio makes a reasonable argument that we need to make sure that\n       multiple trailing slashes are eliminated but my code only takes\n       care of a single trailing slash. I was looking into the code of\n       'normalize_path_copy()' and saw that the function it essentially\n       calls: 'normalize_path_copy_len()' does not perform anything on\n       the trailing slashes and this behaviour is mentioned as a\n       NEEDSWORK.\n\n       I was thinking of correcting the above function instead of\n       putting in an extra loop. Is this feasible?\n\n    3. In the following segment:\n        /*\n         * NEEDSWORK: In a multi-working-tree world, this needs to be\n         * set in the per-worktree config.\n         */\n        if (!git_config_get_string(\"submodule.active\", &var) && var) {\n\n        There was a comment: \"What if this were a valueless true\n        (\"[submodule] active\\n\" without \"= true\")?  Wouldn't get_string()\n        fail?\"\n\n        I was under the impression that even if the above failed, it\n        will not really affect the big picture since at the we will set\n        'submodule.name.active\" as true irrespective of the above value.\n        Is this correct?\n\nFeedback and reviews are appreciated.\n\nRegards,\nShourya Shukla\n\nShourya Shukla (3):\n  dir: change the scope of function 'directory_exists_in_index()'\n  submodule: port submodule subcommand 'add' from shell to C\n  t7400: add test to check 'submodule add' for tracked paths\n\n builtin/submodule--helper.c | 410 +++++++++++++++++++++++++++++++++++-\n dir.c                       |  30 ++-\n dir.h                       |   9 +\n git-submodule.sh            | 161 +-------------\n t/t7400-submodule-basic.sh  |  13 +-\n 5 files changed, 443 insertions(+), 180 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"412220","messageId":"20201214231939.644175-2-periperidip@gmail.com","threadId":"54826","inReplyTo":"20201214231939.644175-1-periperidip@gmail.com","subject":"[PATCH v3 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2020-12-14T23:19:37Z","receivedAt":"2020-12-14T23:21:23Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Change the scope of the function 'directory_exists_in_index()' as well\nas declare it in 'dir.h'.\n\nSince the return type of the function is the enumerator 'exist_status',\nchange its scope as well and declare it in 'dir.h'. While at it, rename\nthe members of the aforementioned enum so as to avoid any naming clashes\nor confusions later on.\n\nHelped-by: Christian Couder <christian.couder@gmail.com>\nHelped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n builtin/submodule--helper.c | 408 ++++++++++++++++++++++++++++++++++++\n dir.c                       |  30 ++-\n dir.h                       |   9 +\n 3 files changed, 429 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c30896c897..4dfad35d77 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2744,6 +2744,414 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \treturn !!ret;\n }\n \n+struct add_data {\n+\tconst char *prefix;\n+\tconst char *branch;\n+\tconst char *reference_path;\n+\tconst char *sm_path;\n+\tconst char *sm_name;\n+\tconst char *repo;\n+\tconst char *realrepo;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+};\n+#define ADD_DATA_INIT { 0 }\n+\n+/*\n+ * Guess the directory name from the repository URL by performing the\n+ * operations below in the following order:\n+ *\n+ * - If the URL ends with '/', remove that.\n+ *\n+ * - If the result of the above ends with zero or more ':', followed\n+ *  by zero or more '/', followed by \".git\", drop the matching part.\n+ *\n+ * - If the result of the above has '/' or ':' in it, remove everything\n+ *  before it and '/' or ':' itself.\n+ */\n+static char *guess_dir_name(const char *repo)\n+{\n+\tconst char *start, *end;\n+\n+\tstart = repo;\n+\tend = repo + strlen(repo);\n+\n+\t/* remove the trailing '/' */\n+\tif (repo < end - 1 && end[-1] == '/')\n+\t\tend--;\n+\n+\t/* remove the trailing ':', '/' and '.git' */\n+\tif (repo < end - 4 && !memcmp(\".git\", end - 4, 4)) {\n+\t\tend -= 4;\n+\t\twhile (repo < end - 1 && end[-1] == '/')\n+\t\t\tend--;\n+\t\twhile (repo < end - 1 && end[-1] == ':')\n+\t\t\tend--;\n+\t}\n+\n+\t/* find the last ':' or '/' */\n+\tfor (start = end - 1; repo <= start; start--) {\n+\t\tif (*start == '/' || *start == ':')\n+\t\t\tbreak;\n+\t}\n+\t/* exclude '/' or ':' itself */\n+\tstart++;\n+\n+\treturn xmemdupz(start, end - start);\n+}\n+\n+static int can_create_submodule(unsigned int force, const char *path)\n+{\n+\tint cache_pos, dir_in_cache = 0;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tcache_pos = cache_name_pos(path, strlen(path));\n+\tif(cache_pos < 0 &&\n+\t   directory_exists_in_index(&the_index, path, strlen(path)) == is_cache_directory)\n+\t\tdir_in_cache = 1;\n+\n+\tif (!force) {\n+\t\tif (cache_pos >= 0 || dir_in_cache)\n+\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n+\t} else {\n+\t\tstruct cache_entry *ce = NULL;\n+\t\tif (cache_pos >= 0)\n+\t\t\tce = the_index.cache[cache_pos];\n+\t\tif (dir_in_cache || (ce && !S_ISGITLINK(ce->ce_mode)))\n+\t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n+\t\t\t      \"submodule\"), path);\n+\t}\n+\treturn 0;\n+}\n+\n+static const char *parse_token(const char *cp, int *len)\n+{\n+\tconst char *p = cp, *start, *end;\n+\tchar *str;\n+\n+\tstart = p;\n+\twhile (*p != ' ')\n+\t\tp++;\n+\tend = p;\n+\tstr = xstrndup(start, end - start);\n+\n+\twhile(*p == ' ')\n+\t\tp++;\n+\n+\treturn str;\n+}\n+\n+static void report_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir)\n+{\n+\tstruct child_process cp_rem = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb_rem = STRBUF_INIT;\n+\n+\tcp_rem.git_cmd = 1;\n+\tfprintf(stderr, _(\"A git directory for '%s' is \"\n+\t\t\"found locally with remote(s):\\n\"), sm_name);\n+\tstrvec_pushf(&cp_rem.env_array,\n+\t\t     \"GIT_DIR=%s\", git_dir);\n+\tstrvec_push(&cp_rem.env_array, \"GIT_WORK_TREE=.\");\n+\tstrvec_pushl(&cp_rem.args, \"remote\", \"-v\", NULL);\n+\tif (!capture_command(&cp_rem, &sb_rem, 0)) {\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < sb_rem.len; i++) {\n+\t\t\tchar *start = sb_rem.buf + i, *end = start;\n+\t\t\tconst char *name = start, *url, *tail;\n+\t\t\tint namelen, urllen;\n+\n+\t\t\twhile (sb_rem.buf[i++] != '\\n')\n+\t\t\t\tend++;\n+\t\t\turl = parse_token(name, &namelen);\n+\t\t\ttail = parse_token(url, &urllen);\n+\t\t\tif (!memcmp(tail, \"(fetch)\", 7))\n+\t\t\t\tfprintf(stderr, \"  %s\\t%s\\n\", name, url);\n+\t\t\tstart = *end ? end + 1 : end;\n+\t\t}\n+\t}\n+}\n+\n+static int add_submodule(struct add_data *info)\n+{\n+\t/* perhaps the path exists and is already a git repo, else clone it */\n+\tif (is_directory(info->sm_path)) {\n+\t\tchar *sub_git_path = xstrfmt(\"%s/.git\", info->sm_path);\n+\t\tif (is_directory(sub_git_path) || file_exists(sub_git_path))\n+\t\t\tprintf(_(\"Adding existing repo at '%s' to the index\\n\"),\n+\t\t\t\t info->sm_path);\n+\t\telse\n+\t\t\tdie(_(\"'%s' already exists and is not a valid git repo\"),\n+\t\t\t      info->sm_path);\n+\t\tfree(sub_git_path);\n+\t} else {\n+\t\tstruct strvec clone_args = STRVEC_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tchar *submodule_git_dir = xstrfmt(\".git/modules/%s\", info->sm_name);\n+\n+\t\tif (is_directory(submodule_git_dir)) {\n+\t\t\tif (!info->force) {\n+\t\t\t\treport_fetch_remotes(stderr, info->sm_name,\n+\t\t\t\t\t\t     submodule_git_dir);\n+\t\t\t\terror(_(\"If you want to reuse this local git \"\n+\t\t\t\t      \"directory instead of cloning again from\\n \"\n+\t\t\t\t      \"  %s\\n\"\n+\t\t\t\t      \"use the '--force' option. If the local \"\n+\t\t\t\t      \"git directory is not the correct repo\\n\"\n+\t\t\t\t      \"or you are unsure what this means choose \"\n+\t\t\t\t      \"another name with the '--name' option.\"),\n+\t\t\t\t      info->realrepo);\n+\t\t\t\treturn 1;\n+\t\t\t} else {\n+\t\t\t\tprintf(_(\"Reactivating local git directory for \"\n+\t\t\t\t\t \"submodule '%s'.\"), info->sm_path);\n+\t\t\t}\n+\t\t}\n+\t\tfree(submodule_git_dir);\n+\n+\t\tstrvec_push(&clone_args, \"clone\");\n+\n+\t\tif (info->quiet)\n+\t\t\tstrvec_push(&clone_args, \"--quiet\");\n+\n+\t\tif (info->progress)\n+\t\t\tstrvec_push(&clone_args, \"--progress\");\n+\n+\t\tif (info->prefix)\n+\t\t\tstrvec_pushl(&clone_args, \"--prefix\", info->prefix, NULL);\n+\t\tstrvec_pushl(&clone_args, \"--path\", info->sm_path, \"--name\",\n+\t\t\t     info->sm_name, \"--url\", info->realrepo, NULL);\n+\t\tif (info->reference_path)\n+\t\t\tstrvec_pushl(&clone_args, \"--reference\",\n+\t\t\t\t     info->reference_path, NULL);\n+\t\tif (info->dissociate)\n+\t\t\tstrvec_push(&clone_args, \"--dissociate\");\n+\n+\t\tif (info->depth >= 0)\n+\t\t\tstrvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n+\n+\t\tif (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n+\t\t\tstrvec_clear(&clone_args);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.git_cmd = 1;\n+\t\tcp.dir = info->sm_path;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-f\", \"-q\", NULL);\n+\n+\t\tif (info->branch) {\n+\t\t\tstrvec_pushl(&cp.args, \"-B\", info->branch, NULL);\n+\t\t\tstrvec_pushf(&cp.args, \"origin/%s\", info->branch);\n+\t\t}\n+\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"unable to checkout submodule '%s'\"), info->sm_path);\n+\t}\n+\treturn 0;\n+}\n+\n+static void config_added_submodule(struct add_data *info)\n+{\n+\tchar *key, *var = NULL;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct child_process cp2 = CHILD_PROCESS_INIT;\n+\n+\tkey = xstrfmt(\"submodule.%s.url\", info->sm_name);\n+\tgit_config_set_gently(key, info->realrepo);\n+\tfree(key);\n+\n+\tcp.git_cmd = 1;\n+\tstrvec_pushl(&cp.args, \"add\", \"--no-warn-embedded-repo\", NULL);\n+\tif (info->force)\n+\t\tstrvec_push(&cp.args, \"--force\");\n+\tstrvec_pushl(&cp.args, \"--\", info->sm_path, NULL);\n+\n+\tif (run_command(&cp))\n+\t\tdie(_(\"failed to add submodule '%s'\"), info->sm_path);\n+\n+\tkey = xstrfmt(\"submodule.%s.path\", info->sm_name);\n+\tconfig_set_in_gitmodules_file_gently(key, info->sm_path);\n+\tfree(key);\n+\tkey = xstrfmt(\"submodule.%s.url\", info->sm_name);\n+\tconfig_set_in_gitmodules_file_gently(key, info->repo);\n+\tfree(key);\n+\tkey = xstrfmt(\"submodule.%s.branch\", info->sm_name);\n+\tif (info->branch)\n+\t\tconfig_set_in_gitmodules_file_gently(key, info->branch);\n+\tfree(key);\n+\n+\tcp2.git_cmd = 1;\n+\tstrvec_pushl(&cp2.args, \"add\", \"--force\", NULL);\n+\tstrvec_pushl(&cp2.args, \"--\", \".gitmodules\", NULL);\n+\n+\tif (run_command(&cp2))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), info->sm_path);\n+\n+\t/*\n+\t * NEEDSWORK: In a multi-working-tree world, this needs to be\n+\t * set in the per-worktree config.\n+\t */\n+\tif (!git_config_get_string(\"submodule.active\", &var) && var) {\n+\n+\t\t/*\n+\t\t * If the submodule being adding isn't already covered by the\n+\t\t * current configured pathspec, set the submodule's active flag\n+\t\t */\n+\t\tif (!is_submodule_active(the_repository, info->sm_path)) {\n+\t\t\tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n+\t\t\tgit_config_set_gently(key, \"true\");\n+\t\t\tfree(key);\n+\t\t}\n+\t} else {\n+\t\tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t}\n+}\n+\n+static int module_add(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *branch = NULL, *custom_name = NULL, *realrepo = NULL;\n+\tconst char *reference_path = NULL, *repo = NULL, *name = NULL;\n+\tchar *sm_path;\n+\tint force = 0, quiet = 0, depth = -1, progress = 0, dissociate = 0;\n+\tstruct add_data info = ADD_DATA_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING('b', \"branch\", &branch, N_(\"branch\"),\n+\t\t\t   N_(\"branch of repository to add as submodule\")),\n+\t\tOPT_BOOL('f', \"force\", &force, N_(\"allow adding an otherwise \"\n+\t\t\t\t\t\t  \"ignored submodule path\")),\n+\t\tOPT__QUIET(&quiet, N_(\"print only error messages\")),\n+\t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force cloning progress\")),\n+\t\tOPT_STRING(0, \"reference\", &reference_path, N_(\"repository\"),\n+\t\t\t   N_(\"reference repository\")),\n+\t\tOPT_BOOL(0, \"dissociate\", &dissociate, N_(\"borrow the objects from reference repositories\")),\n+\t\tOPT_STRING(0, \"name\", &custom_name, N_(\"name\"),\n+\t\t\t   N_(\"sets the submodule’s name to the given string \"\n+\t\t\t      \"instead of defaulting to its path\")),\n+\t\tOPT_INTEGER(0, \"depth\", &depth, N_(\"depth for shallow clones\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add [<options>] [--] [<path>]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (!is_writing_gitmodules_ok())\n+\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n+\tif (prefix && *prefix && reference_path &&\n+\t    !is_absolute_path(reference_path))\n+\t\treference_path = xstrfmt(\"%s%s\", prefix, reference_path);\n+\n+\tif (argc == 0 || argc > 2) {\n+\t\tusage_with_options(usage, options);\n+\t} else if (argc == 1) {\n+\t\trepo = argv[0];\n+\t\tsm_path = guess_dir_name(repo);\n+\t} else {\n+\t\trepo = argv[0];\n+\t\tsm_path = xstrdup(argv[1]);\n+\t}\n+\n+\tif (prefix && *prefix && !is_absolute_path(sm_path))\n+\t\tsm_path = xstrfmt(\"%s%s\", prefix, sm_path);\n+\n+\t/* assure repo is absolute or relative to parent */\n+\tif (starts_with_dot_dot_slash(repo) || starts_with_dot_slash(repo)) {\n+\t\tchar *remote = get_default_remote();\n+\t\tchar *remoteurl;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tif (prefix)\n+\t\t\tdie(_(\"relative path can only be used from the toplevel \"\n+\t\t\t      \"of the working tree\"));\n+\t\t/* dereference source url relative to parent's url */\n+\t\tstrbuf_addf(&sb, \"remote.%s.url\", remote);\n+\t\tif (git_config_get_string(sb.buf, &remoteurl))\n+\t\t\tremoteurl = xgetcwd();\n+\t\trealrepo = relative_url(remoteurl, repo, NULL);\n+\n+\t\tfree(remoteurl);\n+\t\tfree(remote);\n+\t} else if (is_dir_sep(repo[0]) || strchr(repo, ':')) {\n+\t\trealrepo = repo;\n+\t} else {\n+\t\tdie(_(\"repo URL: '%s' must be absolute or begin with ./|../\"),\n+\t\t      repo);\n+\t}\n+\n+\t/*\n+\t * normalize path:\n+\t * multiple //; leading ./; /./; /../;\n+\t */\n+\tnormalize_path_copy(sm_path, sm_path);\n+\t/* strip trailing '/' */\n+\tif (is_dir_sep(sm_path[strlen(sm_path) -1]))\n+\t\tsm_path[strlen(sm_path) - 1] = '\\0';\n+\n+\tif (can_create_submodule(force, sm_path))\n+\t\treturn 1;\n+\n+\tstrbuf_addstr(&sb, sm_path);\n+\tif (is_nonbare_repository_dir(&sb)) {\n+\t\tstruct object_id oid;\n+\t\tif (resolve_gitlink_ref(sm_path, \"HEAD\", &oid) < 0)\n+\t\t\tdie(_(\"'%s' does not have a commit checked out\"), sm_path);\n+\t}\n+\n+\tif (!force) {\n+\t\tint exit_code = -1;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stdout = 1;\n+\t\tstrvec_pushl(&cp.args, \"add\", \"--dry-run\", \"--ignore-missing\",\n+\t\t\t     \"--no-warn-embedded-repo\", sm_path, NULL);\n+\t\tif ((exit_code = pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))) {\n+\t\t\tstrbuf_complete_line(&sb);\n+\t\t\tfputs(sb.buf, stderr);\n+\t\t\treturn exit_code;\n+\t\t}\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tname = custom_name ? custom_name : sm_path;\n+\tif (check_submodule_name(name))\n+\t\tdie(_(\"'%s' is not a valid submodule name\"), name);\n+\n+\tinfo.prefix = prefix;\n+\tinfo.sm_name = name;\n+\tinfo.sm_path = sm_path;\n+\tinfo.repo = repo;\n+\tinfo.realrepo = realrepo;\n+\tinfo.reference_path = reference_path;\n+\tinfo.branch = branch;\n+\tinfo.depth = depth;\n+\tinfo.progress = !!progress;\n+\tinfo.dissociate = !!dissociate;\n+\tinfo.force = !!force;\n+\tinfo.quiet = !!quiet;\n+\n+\tif (add_submodule(&info))\n+\t\treturn 1;\n+\tconfig_added_submodule(&info);\n+\tfree(sm_path);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\ndiff --git a/dir.c b/dir.c\nindex d637461da5..f37de276f2 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1655,12 +1655,6 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \treturn dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);\n }\n \n-enum exist_status {\n-\tindex_nonexistent = 0,\n-\tindex_directory,\n-\tindex_gitdir\n-};\n-\n /*\n  * Do not use the alphabetically sorted index to look up\n  * the directory name; instead, use the case insensitive\n@@ -1672,13 +1666,13 @@ static enum exist_status directory_exists_in_index_icase(struct index_state *ist\n \tstruct cache_entry *ce;\n \n \tif (index_dir_exists(istate, dirname, len))\n-\t\treturn index_directory;\n+\t\treturn is_cache_directory;\n \n \tce = index_file_exists(istate, dirname, len, ignore_case);\n \tif (ce && S_ISGITLINK(ce->ce_mode))\n-\t\treturn index_gitdir;\n+\t\treturn is_cache_gitdir;\n \n-\treturn index_nonexistent;\n+\treturn is_cache_absent;\n }\n \n /*\n@@ -1688,8 +1682,8 @@ static enum exist_status directory_exists_in_index_icase(struct index_state *ist\n  * the files it contains) will sort with the '/' at the\n  * end.\n  */\n-static enum exist_status directory_exists_in_index(struct index_state *istate,\n-\t\t\t\t\t\t   const char *dirname, int len)\n+enum exist_status directory_exists_in_index(struct index_state *istate,\n+\t\t\t\t\t    const char *dirname, int len)\n {\n \tint pos;\n \n@@ -1709,11 +1703,11 @@ static enum exist_status directory_exists_in_index(struct index_state *istate,\n \t\tif (endchar > '/')\n \t\t\tbreak;\n \t\tif (endchar == '/')\n-\t\t\treturn index_directory;\n+\t\t\treturn is_cache_directory;\n \t\tif (!endchar && S_ISGITLINK(ce->ce_mode))\n-\t\t\treturn index_gitdir;\n+\t\t\treturn is_cache_gitdir;\n \t}\n-\treturn index_nonexistent;\n+\treturn is_cache_absent;\n }\n \n /*\n@@ -1767,11 +1761,11 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n \n-\tif (status == index_directory)\n+\tif (status == is_cache_directory)\n \t\treturn path_recurse;\n-\tif (status == index_gitdir)\n+\tif (status == is_cache_gitdir)\n \t\treturn path_none;\n-\tif (status != index_nonexistent)\n+\tif (status != is_cache_absent)\n \t\tBUG(\"Unhandled value for directory_exists_in_index: %d\\n\", status);\n \n \t/*\n@@ -2190,7 +2184,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \tif ((dir->flags & DIR_COLLECT_KILLED_ONLY) &&\n \t    (dtype == DT_DIR) &&\n \t    !has_path_in_index &&\n-\t    (directory_exists_in_index(istate, path->buf, path->len) == index_nonexistent))\n+\t    (directory_exists_in_index(istate, path->buf, path->len) == is_cache_absent))\n \t\treturn path_none;\n \n \texcluded = is_excluded(dir, istate, path->buf, &dtype);\ndiff --git a/dir.h b/dir.h\nindex a3c40dec51..af817a21b2 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -370,6 +370,15 @@ int read_directory(struct dir_struct *, struct index_state *istate,\n \t\t   const char *path, int len,\n \t\t   const struct pathspec *pathspec);\n \n+enum exist_status {\n+\tis_cache_absent = 0,\n+\tis_cache_directory,\n+\tis_cache_gitdir\n+};\n+\n+enum exist_status directory_exists_in_index(struct index_state *istate,\n+\t\t\t\t\t    const char *dirname, int len);\n+\n enum pattern_match_result {\n \tUNDECIDED = -1,\n \tNOT_MATCHED = 0,\n-- \n2.25.1\n\n"},{"id":"412221","messageId":"20201214231939.644175-3-periperidip@gmail.com","threadId":"54826","inReplyTo":"20201214231939.644175-1-periperidip@gmail.com","subject":"[PATCH v3 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2020-12-14T23:19:38Z","receivedAt":"2020-12-14T23:21:30Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Convert submodule subcommand 'add' to a builtin and call it via\n'git-submodule.sh'.\n\nAlso, since the command die()s out in case of absence of commits in the\nsubmodule, the keyword 'fatal' is prefixed in the error messages.\nTherefore, prepend the keyword in the expected output of test t7400.6.\n\nWhile at it, eliminate the extra preprocessor directive\n`#include \"dir.h\"` at the start of 'submodule--helper.c'.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Stefan Beller <stefanbeller@gmail.com>\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n builtin/submodule--helper.c |   2 +-\n git-submodule.sh            | 161 +-----------------------------------\n t/t7400-submodule-basic.sh  |   2 +-\n 3 files changed, 3 insertions(+), 162 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4dfad35d77..4f1d892b9a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -19,7 +19,6 @@\n #include \"diffcore.h\"\n #include \"diff.h\"\n #include \"object-store.h\"\n-#include \"dir.h\"\n #include \"advice.h\"\n \n #define OPT_QUIET (1 << 0)\n@@ -3185,6 +3184,7 @@ static struct cmd_struct commands[] = {\n \t{\"config\", module_config, 0},\n \t{\"set-url\", module_set_url, 0},\n \t{\"set-branch\", module_set_branch, 0},\n+\t{\"add\", module_add, SUPPORT_SUPER_PREFIX},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex eb90f18229..b586f9532d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -145,166 +145,7 @@ cmd_add()\n \t\tshift\n \tdone\n \n-\tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n-\tthen\n-\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n-\tfi\n-\n-\tif test -n \"$reference_path\"\n-\tthen\n-\t\tis_absolute_path \"$reference_path\" ||\n-\t\treference_path=\"$wt_prefix$reference_path\"\n-\n-\t\treference=\"--reference=$reference_path\"\n-\tfi\n-\n-\trepo=$1\n-\tsm_path=$2\n-\n-\tif test -z \"$sm_path\"; then\n-\t\tsm_path=$(printf '%s\\n' \"$repo\" |\n-\t\t\tsed -e 's|/$||' -e 's|:*/*\\.git$||' -e 's|.*[/:]||g')\n-\tfi\n-\n-\tif test -z \"$repo\" || test -z \"$sm_path\"; then\n-\t\tusage\n-\tfi\n-\n-\tis_absolute_path \"$sm_path\" || sm_path=\"$wt_prefix$sm_path\"\n-\n-\t# assure repo is absolute or relative to parent\n-\tcase \"$repo\" in\n-\t./*|../*)\n-\t\ttest -z \"$wt_prefix\" ||\n-\t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n-\n-\t\t# dereference source url relative to parent's url\n-\t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n-\t\t;;\n-\t*:*|/*)\n-\t\t# absolute url\n-\t\trealrepo=$repo\n-\t\t;;\n-\t*)\n-\t\tdie \"$(eval_gettext \"repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n-\t;;\n-\tesac\n-\n-\t# normalize path:\n-\t# multiple //; leading ./; /./; /../; trailing /\n-\tsm_path=$(printf '%s/\\n' \"$sm_path\" |\n-\t\tsed -e '\n-\t\t\ts|//*|/|g\n-\t\t\ts|^\\(\\./\\)*||\n-\t\t\ts|/\\(\\./\\)*|/|g\n-\t\t\t:start\n-\t\t\ts|\\([^/]*\\)/\\.\\./||\n-\t\t\ttstart\n-\t\t\ts|/*$||\n-\t\t')\n-\tif test -z \"$force\"\n-\tthen\n-\t\tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n-\telse\n-\t\tgit ls-files -s \"$sm_path\" | sane_grep -v \"^160000\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n-\tfi\n-\n-\tif test -d \"$sm_path\" &&\n-\t\ttest -z $(git -C \"$sm_path\" rev-parse --show-cdup 2>/dev/null)\n-\tthen\n-\t    git -C \"$sm_path\" rev-parse --verify -q HEAD >/dev/null ||\n-\t    die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n-\tfi\n-\n-\tif test -z \"$force\"\n-\tthen\n-\t    dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo \"$sm_path\" 2>&1 >/dev/null)\n-\t    res=$?\n-\t    if test $res -ne 0\n-\t    then\n-\t\t echo >&2 \"$dryerr\"\n-\t\t exit $res\n-\t    fi\n-\tfi\n-\n-\tif test -n \"$custom_name\"\n-\tthen\n-\t\tsm_name=\"$custom_name\"\n-\telse\n-\t\tsm_name=\"$sm_path\"\n-\tfi\n-\n-\tif ! git submodule--helper check-name \"$sm_name\"\n-\tthen\n-\t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n-\tfi\n-\n-\t# perhaps the path exists and is already a git repo, else clone it\n-\tif test -e \"$sm_path\"\n-\tthen\n-\t\tif test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\teval_gettextln \"Adding existing repo at '\\$sm_path' to the index\"\n-\t\telse\n-\t\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists and is not a valid git repo\")\"\n-\t\tfi\n-\n-\telse\n-\t\tif test -d \".git/modules/$sm_name\"\n-\t\tthen\n-\t\t\tif test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\teval_gettextln >&2 \"A git directory for '\\$sm_name' is found locally with remote(s):\"\n-\t\t\t\tGIT_DIR=\".git/modules/$sm_name\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\"  \", -e s,' (fetch)',, >&2\n-\t\t\t\tdie \"$(eval_gettextln \"\\\n-If you want to reuse this local git directory instead of cloning again from\n-  \\$realrepo\n-use the '--force' option. If the local git directory is not the correct repo\n-or you are unsure what this means choose another name with the '--name' option.\")\"\n-\t\t\telse\n-\t\t\t\teval_gettextln \"Reactivating local git directory for submodule '\\$sm_name'.\"\n-\t\t\tfi\n-\t\tfi\n-\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+\"--progress\"} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" ${reference:+\"$reference\"} ${dissociate:+\"--dissociate\"} ${depth:+\"$depth\"} || exit\n-\t\t(\n-\t\t\tsanitize_submodule_env\n-\t\t\tcd \"$sm_path\" &&\n-\t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n-\t\t\tcase \"$branch\" in\n-\t\t\t'') git checkout -f -q ;;\n-\t\t\t?*) git checkout -f -q -B \"$branch\" \"origin/$branch\" ;;\n-\t\t\tesac\n-\t\t) || die \"$(eval_gettext \"Unable to checkout submodule '\\$sm_path'\")\"\n-\tfi\n-\tgit config submodule.\"$sm_name\".url \"$realrepo\"\n-\n-\tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-\tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n-\n-\tgit submodule--helper config submodule.\"$sm_name\".path \"$sm_path\" &&\n-\tgit submodule--helper config submodule.\"$sm_name\".url \"$repo\" &&\n-\tif test -n \"$branch\"\n-\tthen\n-\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n-\tfi &&\n-\tgit add --force .gitmodules ||\n-\tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n-\n-\t# NEEDSWORK: In a multi-working-tree world, this needs to be\n-\t# set in the per-worktree config.\n-\tif git config --get submodule.active >/dev/null\n-\tthen\n-\t\t# If the submodule being adding isn't already covered by the\n-\t\t# current configured pathspec, set the submodule's active flag\n-\t\tif ! git submodule--helper is-active \"$sm_path\"\n-\t\tthen\n-\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\t\tfi\n-\telse\n-\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\tfi\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper add ${force:+--force} ${GIT_QUIET:+--quiet} ${progress:+--progress} ${branch:+--branch \"$branch\"} ${reference_path:+--reference \"$reference_path\"} ${dissociate:+--dissociate} ${custom_name:+--name \"$custom_name\"} ${depth:+\"$depth\"} -- \"$@\"\n }\n \n #\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex fec7e0299d..4ab8298385 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -48,7 +48,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n \n test_expect_success 'add aborts on repository with no commits' '\n \tcat >expect <<-\\EOF &&\n-\t'\"'repo-no-commits'\"' does not have a commit checked out\n+\tfatal: '\"'repo-no-commits'\"' does not have a commit checked out\n \tEOF\n \tgit init repo-no-commits &&\n \ttest_must_fail git submodule add ../a ./repo-no-commits 2>actual &&\n-- \n2.25.1\n\n"},{"id":"412222","messageId":"20201214231939.644175-4-periperidip@gmail.com","threadId":"54826","inReplyTo":"20201214231939.644175-1-periperidip@gmail.com","subject":"[PATCH v3 3/3] t7400: add test to check 'submodule add' for tracked paths","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2020-12-14T23:19:39Z","receivedAt":"2020-12-14T23:21:34Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Add test to check if 'git submodule add' works on paths which are\ntracked by Git.\n\nHelped-by: Christian Couder <christian.couder@gmail.com>\nHelped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 4ab8298385..d9317192e0 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -193,6 +193,17 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n \t)\n '\n \n+test_expect_success 'submodule add to path with tracked contents fails' '\n+\t(\n+\t\tcd addtest-ignore &&\n+\t\tmkdir track &&\n+\t\tgit add -f track &&\n+\t\tgit commit -m \"add tracked path\" &&\n+\t\t! git submodule add \"$submodurl\" submod >output 2>&1 &&\n+\t\ttest_file_not_empty output\n+\t)\n+'\n+\n test_expect_success 'submodule add to reconfigure existing submodule with --force' '\n \t(\n \t\tcd addtest-ignore &&\n-- \n2.25.1\n\n"},{"id":"412303","messageId":"xmqqlfdy7niy.fsf@gitster.c.googlers.com","threadId":"54826","inReplyTo":"20201214231939.644175-1-periperidip@gmail.com","subject":"Re: [PATCH v3 0/3] submodule: port subcommand add from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-15T21:44:05Z","receivedAt":"2020-12-15T21:45:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n>     3. In the following segment:\n>         /*\n>          * NEEDSWORK: In a multi-working-tree world, this needs to be\n>          * set in the per-worktree config.\n>          */\n>         if (!git_config_get_string(\"submodule.active\", &var) && var) {\n>\n>         There was a comment: \"What if this were a valueless true\n>         (\"[submodule] active\\n\" without \"= true\")?  Wouldn't get_string()\n>         fail?\"\n>\n>         I was under the impression that even if the above failed, it\n>         will not really affect the big picture since at the we will set\n>         'submodule.name.active\" as true irrespective of the above value.\n>         Is this correct?\n\nLet's see what kind of value the \"submodule.active\" variable is\nmeant to be set to.  Documentation/config/submodule.txt has this:\n\n    submodule.active::\n            A repeated field which contains a pathspec used to match against a\n            submodule's path to determine if the submodule is of interest to git\n            commands. See linkgit:gitsubmodules[7] for details.\n\nIt definitely is a string value, and making it a valueless true is\nan error in the configuration.  I wonder if we want to diagnose such\nan error, or can we just pretend we didn't see it and keep going?\n\nAlso the \"var\" (one of the values set for this multi-valued\nvariable) is never used in the body of the \"if\" statement.  The\nother user of \"submodule.active\" in module_init() seems to use\nconfig_get_value_multi() on it.  The new code may deserve a comment\nto explain why that is OK to (1) grab just a single value out of the\nmulti-valued variable, and (2) not even look at its value.\n"},{"id":"412490","messageId":"20201217141625.GA7638@konoha","threadId":"54826","inReplyTo":"xmqqlfdy7niy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 0/3] submodule: port subcommand add from shell to C","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2020-12-17T14:16:25Z","receivedAt":"2020-12-17T14:17:14Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 15/12 01:44, Junio C Hamano wrote:\n> Shourya Shukla <periperidip@gmail.com> writes:\n> \n> >     3. In the following segment:\n> >         /*\n> >          * NEEDSWORK: In a multi-working-tree world, this needs to be\n> >          * set in the per-worktree config.\n> >          */\n> >         if (!git_config_get_string(\"submodule.active\", &var) && var) {\n> >\n> >         There was a comment: \"What if this were a valueless true\n> >         (\"[submodule] active\\n\" without \"= true\")?  Wouldn't get_string()\n> >         fail?\"\n> >\n> >         I was under the impression that even if the above failed, it\n> >         will not really affect the big picture since at the we will set\n> >         'submodule.name.active\" as true irrespective of the above value.\n> >         Is this correct?\n> \n> Let's see what kind of value the \"submodule.active\" variable is\n> meant to be set to.  Documentation/config/submodule.txt has this:\n> \n>     submodule.active::\n>             A repeated field which contains a pathspec used to match against a\n>             submodule's path to determine if the submodule is of interest to git\n>             commands. See linkgit:gitsubmodules[7] for details.\n> \n> It definitely is a string value, and making it a valueless true is\n> an error in the configuration.\n\nI think that we did not _make_ it a valueless true. It was already there\nand we somehow managed to check it. If you mean that we should ensure\nthat we set it to \"true\" so that any such errors don't happen later on,\nthen that is a different thing.\n\n> I wonder if we want to diagnose such\n> an error, or can we just pretend we didn't see it and keep going?\n\nI guess we could pretend we did not see it since it isn't affecting the\nrun of the sub-command. If you think otherwise, please suggest.\n\n> Also the \"var\" (one of the values set for this multi-valued\n> variable) is never used in the body of the \"if\" statement.  The\n> other user of \"submodule.active\" in module_init() seems to use\n> config_get_value_multi() on it.  The new code may deserve a comment\n> to explain why that is OK to (1) grab just a single value out of the\n> multi-valued variable, and (2) not even look at its value.\n\nUnderstood. So a comment along the lines of:\n\n\t/*\n\t * Since we are fetching information only about one submodule,\n\t * we need not fetch a  list of submodules to check the activity\n\t * status of a single submodule.\n\t *\n\t * In case of a valueless true, i.e, '[submodule] active\\n'\n\t * without '= true', we need not worry about any errors since\n\t * irrespective of the above value, we will set\n\t * 'submodule.<name>.active' as true.\n\t */\n\nwill work? Also, could you please comment on the other two issues I\nmentioned in the cover letter so I might as well start work on v4 of\nthis patch?\n\nRegards,\nShourya Shukla\n\n"},{"id":"412507","messageId":"xmqqo8isxefz.fsf@gitster.c.googlers.com","threadId":"54826","inReplyTo":"20201217141625.GA7638@konoha","subject":"Re: [PATCH v3 0/3] submodule: port subcommand add from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-17T22:20:16Z","receivedAt":"2020-12-17T22:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> On 15/12 01:44, Junio C Hamano wrote:\n>> Shourya Shukla <periperidip@gmail.com> writes:\n>> \n>> >     3. In the following segment:\n>> >         /*\n>> >          * NEEDSWORK: In a multi-working-tree world, this needs to be\n>> >          * set in the per-worktree config.\n>> >          */\n>> >         if (!git_config_get_string(\"submodule.active\", &var) && var) {\n>> >\n>> >         There was a comment: \"What if this were a valueless true\n>> >         (\"[submodule] active\\n\" without \"= true\")?  Wouldn't get_string()\n>> >         fail?\"\n>> >\n>> >         I was under the impression that even if the above failed, it\n>> >         will not really affect the big picture since at the we will set\n>> >         'submodule.name.active\" as true irrespective of the above value.\n>> >         Is this correct?\n>> \n>> Let's see what kind of value the \"submodule.active\" variable is\n>> meant to be set to.  Documentation/config/submodule.txt has this:\n>> \n>>     submodule.active::\n>>             A repeated field which contains a pathspec used to match against a\n>>             submodule's path to determine if the submodule is of interest to git\n>>             commands. See linkgit:gitsubmodules[7] for details.\n>> \n>> It definitely is a string value, and making it a valueless true is\n>> an error in the configuration.\n>\n> I think that we did not _make_ it a valueless true. It was already there\n> and we somehow managed to check it. If you mean that we should ensure\n> that we set it to \"true\" so that any such errors don't happen later on,\n> then that is a different thing.\n\nLet me rephrase.  When a user has \"[submodule] active\" in his or her\nconfiguration file, it is a configuration error.  When Git reads\n\"submodule.active\" configuration variable to make a decision (like\nthe above code) and finds that the user has such an error, the user\nwould appreciate if the error is pointed out, so that it can be\ncorrected, rather than silently ignored.\n\n>> Also the \"var\" (one of the values set for this multi-valued\n>> variable) is never used in the body of the \"if\" statement.  The\n>> other user of \"submodule.active\" in module_init() seems to use\n>> config_get_value_multi() on it.  The new code may deserve a comment\n>> to explain why that is OK to (1) grab just a single value out of the\n>> multi-valued variable, and (2) not even look at its value.\n>\n> Understood. So a comment along the lines of:\n>\n> \t/*\n> \t * Since we are fetching information only about one submodule,\n> \t * we need not fetch a  list of submodules to check the activity\n> \t * status of a single submodule.\n\nMakes me wonder if I am getting the semantics of submodule.active\nvariable right.\n\nFrom the three-line description in the documentation (see above), I\nwould have guessed that if we have three values for\nsubmodule.active, e.g.\n\n\t[submodule]\n\t\tactive = $a\n\t\tactive = $b\n\t\tactive = $c\n\nthen when deciding if we want to see if a submodule at a $sm_path,\nwe'd see if $path matches any one of $a, $b, or $c and if it does,\nit is determined that the submodule is \"of interest to git\ncommands\".\n\nYes, we may be fetching information only about one submodule at\n$sm_path, but given the explanation of how the configuration\nvariable is designed to work, how can we _not_ fetch the list and\ncheck all of them?\n\nSo the comment above (for that matter, the one below that talks\nabout valuless true) does not make any sense to me, sorry.\n\n> \t * In case of a valueless true, i.e, '[submodule] active\\n'\n> \t * without '= true', we need not worry about any errors since\n> \t * irrespective of the above value, we will set\n> \t * 'submodule.<name>.active' as true.\n> \t */\n>\n> will work? \n\nThe real reason why it is OK to just check existence of submodule.active\nvariable without seeing any value of them is because the check is done\nto see if this call is needed at all:\n\n\tgit submodule--helper is-active \"$sm_path\"\n\nThis \"helper\" eventually calls submodule.c::is_submodule_active()\nthat does the real check---it gets the multi-valued submodule.active\nand checks them against the path to determine if the submodule is\n\"of interest\".\n\nOn the other hand, when we know submodule.active does not exist, all\nsubmodules are of interest when it comes to \"submodule add\".  That\nis how\n\n-\tif git config --get submodule.active >/dev/null\n-\tthen\n-\t\t# If the submodule being adding isn't already covered by the\n-\t\t# current configured pathspec, set the submodule's active flag\n-\t\tif ! git submodule--helper is-active \"$sm_path\"\n-\t\tthen\n-\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\t\tfi\n-\telse\n-\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\tfi\n\ntaken from your [2/3] that ignored the values of submodule.active is\n\"correct\"; we know that the real work is done elsewhere--we are only\nlearning if the is-active check is necessary.\n\nI think explaining why it works correctly to show future readers\nthat the code was written by folks who knew what they were doing\nwould be worth the effort to help future code evolution.\n\nThis, from your [1/3], is a faithful translation of the above,\nbut ...\n\n+\tif (!git_config_get_string(\"submodule.active\", &var) && var) {\n+\n+\t\t/*\n+\t\t * If the submodule being adding isn't already covered by the\n+\t\t * current configured pathspec, set the submodule's active flag\n+\t\t */\n+\t\tif (!is_submodule_active(the_repository, info->sm_path)) {\n+\t\t\tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n+\t\t\tgit_config_set_gently(key, \"true\");\n+\t\t\tfree(key);\n+\t\t}\n+\t} else {\n+\t\tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t}\n\n... by knowing why we check submodule.active but discard its value,\nfuture developers (read: I think this is outside the scope of this\nseries) can rewrite it like so to make it more readable and reduce\nthe repeated code to set submodule.$sm_name.active to true.\n\n        /*\n\t * If submodule.active does not exist, we will activate this\n\t * module unconditionally.\n\t *\n         * Otherwise, is_submodule_active() is asked to determine if\n         * the path currently is of interest; because it will obtain\n         * and iterate over this multi-valued variable by itself, we\n         * do not need its values we obtain from git_config_get_string()\n         * call here.  We are only checking if we need to ask the\n         * is_submodule_active() helper function.  We explicitly set\n\t * the submodule.$sm_name.active if submodule.active patterns\n\t * do not cover the path (i.e. is_submodule_active() says \"no\".\n         */\n\tif (git_config_get_string(\"submodule.active\", &var) ||\n\t    !is_submodule_active(the_repository, info->sm_path)) {\n\t\tkey = xstrfmt(...);\n\t\tgit_config_set_gently(key, \"true\");\n\t\tfree(key);\n\t}\n\nand a comment like this would help such readers, for example.\n\nBy the way, as you might have noticed, your [1/3] contains a lot of\nmaterial that ought to be part of [2/3], doesn't it?  [1/3] was\nsupposed to be just borrowing helper from dir.c but has the new\n\"add\" code implemented in the same patch.\n\n> Also, could you please comment on the other two issues I\n> mentioned in the cover letter so I might as well start work on v4 of\n> this patch?\n\nI'll leave the other two to other reviewers and mentors for now, but\nmay come back to them if I beat them.\n\nThanks.\n"},{"id":"412608","messageId":"nycvar.QRO.7.76.6.2012190104140.56@tvgsbejvaqbjf.bet","threadId":"54826","inReplyTo":"20201214231939.644175-2-periperidip@gmail.com","subject":"Re: [PATCH v3 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-19T00:08:11Z","receivedAt":"2020-12-19T00:09:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Tue, 15 Dec 2020, Shourya Shukla wrote:\n\n> Change the scope of the function 'directory_exists_in_index()' as well\n> as declare it in 'dir.h'.\n>\n> Since the return type of the function is the enumerator 'exist_status',\n> change its scope as well and declare it in 'dir.h'. While at it, rename\n> the members of the aforementioned enum so as to avoid any naming clashes\n> or confusions later on.\n\nThis makes it sound as if only existing code was adjusted, in a minimal\nway, but no new code was introduced. But that's not true:\n\n>\n> Helped-by: Christian Couder <christian.couder@gmail.com>\n> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Shourya Shukla <periperidip@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 408 ++++++++++++++++++++++++++++++++++++\n>  dir.c                       |  30 ++-\n>  dir.h                       |   9 +\n>  3 files changed, 429 insertions(+), 18 deletions(-)\n\nTons of new code there. And unfortunately...\n\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c30896c897..4dfad35d77 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2744,6 +2744,414 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n>  \treturn !!ret;\n>  }\n>\n> +struct add_data {\n> +\tconst char *prefix;\n> +\tconst char *branch;\n> +\tconst char *reference_path;\n> +\tconst char *sm_path;\n> +\tconst char *sm_name;\n> +\tconst char *repo;\n> +\tconst char *realrepo;\n> +\tint depth;\n> +\tunsigned int force: 1;\n> +\tunsigned int quiet: 1;\n> +\tunsigned int progress: 1;\n> +\tunsigned int dissociate: 1;\n> +};\n> +#define ADD_DATA_INIT { 0 }\n> +\n> +/*\n> + * Guess the directory name from the repository URL by performing the\n> + * operations below in the following order:\n> + *\n> + * - If the URL ends with '/', remove that.\n> + *\n> + * - If the result of the above ends with zero or more ':', followed\n> + *  by zero or more '/', followed by \".git\", drop the matching part.\n> + *\n> + * - If the result of the above has '/' or ':' in it, remove everything\n> + *  before it and '/' or ':' itself.\n> + */\n> +static char *guess_dir_name(const char *repo)\n> +{\n> +\tconst char *start, *end;\n> +\n> +\tstart = repo;\n> +\tend = repo + strlen(repo);\n> +\n> +\t/* remove the trailing '/' */\n> +\tif (repo < end - 1 && end[-1] == '/')\n> +\t\tend--;\n> +\n> +\t/* remove the trailing ':', '/' and '.git' */\n> +\tif (repo < end - 4 && !memcmp(\".git\", end - 4, 4)) {\n> +\t\tend -= 4;\n> +\t\twhile (repo < end - 1 && end[-1] == '/')\n> +\t\t\tend--;\n> +\t\twhile (repo < end - 1 && end[-1] == ':')\n> +\t\t\tend--;\n> +\t}\n> +\n> +\t/* find the last ':' or '/' */\n> +\tfor (start = end - 1; repo <= start; start--) {\n> +\t\tif (*start == '/' || *start == ':')\n> +\t\t\tbreak;\n> +\t}\n> +\t/* exclude '/' or ':' itself */\n> +\tstart++;\n> +\n> +\treturn xmemdupz(start, end - start);\n> +}\n> +\n> +static int can_create_submodule(unsigned int force, const char *path)\n> +{\n> +\tint cache_pos, dir_in_cache = 0;\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n> +\n> +\tcache_pos = cache_name_pos(path, strlen(path));\n> +\tif(cache_pos < 0 &&\n> +\t   directory_exists_in_index(&the_index, path, strlen(path)) == is_cache_directory)\n> +\t\tdir_in_cache = 1;\n> +\n> +\tif (!force) {\n> +\t\tif (cache_pos >= 0 || dir_in_cache)\n> +\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> +\t} else {\n> +\t\tstruct cache_entry *ce = NULL;\n> +\t\tif (cache_pos >= 0)\n> +\t\t\tce = the_index.cache[cache_pos];\n> +\t\tif (dir_in_cache || (ce && !S_ISGITLINK(ce->ce_mode)))\n> +\t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n> +\t\t\t      \"submodule\"), path);\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static const char *parse_token(const char *cp, int *len)\n> +{\n> +\tconst char *p = cp, *start, *end;\n> +\tchar *str;\n> +\n> +\tstart = p;\n> +\twhile (*p != ' ')\n> +\t\tp++;\n> +\tend = p;\n> +\tstr = xstrndup(start, end - start);\n> +\n> +\twhile(*p == ' ')\n> +\t\tp++;\n> +\n> +\treturn str;\n> +}\n\nThis function is not careful enough to avoid buffer overruns. It even\ntriggers a segmentation fault in our test suite:\nhttps://github.com/gitgitgadget/git/runs/1574891976?check_suite_focus=true#step:6:3152\n\nI need this to make it pass (only tested locally so far, but I trust you\nto take the baton from here):\n\n-- snipsnap --\nFrom c28c0cd3ac21d546394335957fbaa350ab287c3f Mon Sep 17 00:00:00 2001\nFrom: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate: Sat, 19 Dec 2020 01:02:04 +0100\nSubject: [PATCH] fixup??? dir: change the scope of function\n 'directory_exists_in_index()'\n\nThis fixes the segmentation fault reported in the linux-musl job of our\nCI builds. Valgrind has this to say about it:\n\n==32354==\n==32354== Process terminating with default action of signal 11 (SIGSEGV)\n==32354==  Access not within mapped region at address 0x5C73000\n==32354==    at 0x202F5A: parse_token (submodule--helper.c:2837)\n==32354==    by 0x20319B: report_fetch_remotes (submodule--helper.c:2871)\n==32354==    by 0x2033FD: add_submodule (submodule--helper.c:2898)\n==32354==    by 0x204612: module_add (submodule--helper.c:3146)\n==32354==    by 0x20478A: cmd_submodule__helper (submodule--helper.c:3202)\n==32354==    by 0x12655E: run_builtin (git.c:458)\n==32354==    by 0x1269B4: handle_builtin (git.c:712)\n==32354==    by 0x126C79: run_argv (git.c:779)\n==32354==    by 0x12715C: cmd_main (git.c:913)\n==32354==    by 0x2149A2: main (common-main.c:52)\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4f1d892b9a9..29a6f80b937 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2834,12 +2834,12 @@ static const char *parse_token(const char *cp, int *len)\n \tchar *str;\n\n \tstart = p;\n-\twhile (*p != ' ')\n+\twhile (*p && *p != ' ')\n \t\tp++;\n \tend = p;\n \tstr = xstrndup(start, end - start);\n\n-\twhile(*p == ' ')\n+\twhile(*p && *p == ' ')\n \t\tp++;\n\n \treturn str;\n--\n2.29.2.windows.1.1.g3464b98ce68\n\n"},{"id":"412612","messageId":"xmqqsg82tyeo.fsf@gitster.c.googlers.com","threadId":"54826","inReplyTo":"nycvar.QRO.7.76.6.2012190104140.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T00:47:11Z","receivedAt":"2020-12-19T00:48:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Shourya,\n>\n> On Tue, 15 Dec 2020, Shourya Shukla wrote:\n>\n>> Change the scope of the function 'directory_exists_in_index()' as well\n>> as declare it in 'dir.h'.\n>>\n>> Since the return type of the function is the enumerator 'exist_status',\n>> change its scope as well and declare it in 'dir.h'. While at it, rename\n>> the members of the aforementioned enum so as to avoid any naming clashes\n>> or confusions later on.\n>\n> This makes it sound as if only existing code was adjusted, in a minimal\n> way, but no new code was introduced. But that's not true:\n\nI noticed it last night, too---I suspect it was a mistake made while\nshuffling changes across steps with rebase -i.\n\n>> Helped-by: Christian Couder <christian.couder@gmail.com>\n>> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n>> Signed-off-by: Shourya Shukla <periperidip@gmail.com>\n>> ---\n>>  builtin/submodule--helper.c | 408 ++++++++++++++++++++++++++++++++++++\n>>  dir.c                       |  30 ++-\n>>  dir.h                       |   9 +\n>>  3 files changed, 429 insertions(+), 18 deletions(-)\n>\n> Tons of new code there. And unfortunately...\n\n>> +static const char *parse_token(const char *cp, int *len)\n>> +{\n>> +\tconst char *p = cp, *start, *end;\n>> +\tchar *str;\n>> +\n>> +\tstart = p;\n>> +\twhile (*p != ' ')\n>> +\t\tp++;\n>> +\tend = p;\n>> +\tstr = xstrndup(start, end - start);\n>> +\n>> +\twhile(*p == ' ')\n>> +\t\tp++;\n>> +\n>> +\treturn str;\n>> +}\n>\n> This function is not careful enough to avoid buffer overruns. It even\n> triggers a segmentation fault in our test suite:\n> https://github.com/gitgitgadget/git/runs/1574891976?check_suite_focus=true#step:6:3152\n\nI notice that len is not used at all ;-)\n\n> I need this to make it pass (only tested locally so far, but I trust you\n> to take the baton from here):\n>\n> -- snipsnap --\n> From c28c0cd3ac21d546394335957fbaa350ab287c3f Mon Sep 17 00:00:00 2001\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date: Sat, 19 Dec 2020 01:02:04 +0100\n> Subject: [PATCH] fixup??? dir: change the scope of function\n>  'directory_exists_in_index()'\n>\n> This fixes the segmentation fault reported in the linux-musl job of our\n> CI builds. Valgrind has this to say about it:\n>\n> ==32354==\n> ==32354== Process terminating with default action of signal 11 (SIGSEGV)\n> ==32354==  Access not within mapped region at address 0x5C73000\n> ==32354==    at 0x202F5A: parse_token (submodule--helper.c:2837)\n> ==32354==    by 0x20319B: report_fetch_remotes (submodule--helper.c:2871)\n> ==32354==    by 0x2033FD: add_submodule (submodule--helper.c:2898)\n> ==32354==    by 0x204612: module_add (submodule--helper.c:3146)\n> ==32354==    by 0x20478A: cmd_submodule__helper (submodule--helper.c:3202)\n> ==32354==    by 0x12655E: run_builtin (git.c:458)\n> ==32354==    by 0x1269B4: handle_builtin (git.c:712)\n> ==32354==    by 0x126C79: run_argv (git.c:779)\n> ==32354==    by 0x12715C: cmd_main (git.c:913)\n> ==32354==    by 0x2149A2: main (common-main.c:52)\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  builtin/submodule--helper.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 4f1d892b9a9..29a6f80b937 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2834,12 +2834,12 @@ static const char *parse_token(const char *cp, int *len)\n>  \tchar *str;\n>\n>  \tstart = p;\n> -\twhile (*p != ' ')\n> +\twhile (*p && *p != ' ')\n>  \t\tp++;\n>  \tend = p;\n>  \tstr = xstrndup(start, end - start);\n>\n> -\twhile(*p == ' ')\n> +\twhile(*p && *p == ' ')\n>  \t\tp++;\n>\n>  \treturn str;\n> --\n> 2.29.2.windows.1.1.g3464b98ce68\n"},{"id":"412873","messageId":"xmqqft3xflw7.fsf@gitster.c.googlers.com","threadId":"54826","inReplyTo":"20201214231939.644175-1-periperidip@gmail.com","subject":"Re: [PATCH v3 0/3] submodule: port subcommand add from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T23:42:32Z","receivedAt":"2020-12-22T23:43:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> Feedback and reviews are appreciated.\n>\n> Regards,\n> Shourya Shukla\n>\n> Shourya Shukla (3):\n>   dir: change the scope of function 'directory_exists_in_index()'\n>   submodule: port submodule subcommand 'add' from shell to C\n>   t7400: add test to check 'submodule add' for tracked paths\n\nSorry for not being a feedback nor a review, but we are seeing a\nsegfault from \"git submodule add\" when the topic is tested with the\nrest of 'seen':\n\n  https://github.com/git/git/runs/1597682274#step:6:3155\n\nIt seems that you need to be logged in to see the full CI output to\nthe line level when visiting the above URL.\n\nThanks.\n"}]}