{"thread":{"id":"54121","subject":"[GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","startedAt":"2020-08-24T09:04:53Z","lastAt":"2020-09-03T08:46:30Z","messageCount":12,"participants":["Shourya Shukla","Junio C Hamano","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404309","messageId":"20200824090359.403944-1-shouryashukla.oo@gmail.com","threadId":"54121","inReplyTo":null,"subject":"[GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-24T09:03:59Z","receivedAt":"2020-08-24T09:04:53Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"From: Prathamesh Chavan <pc44800@gmail.com>\n\nConvert 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 and exits with exit status 1 when we try adding a submodule\nwhich is mentioned in .gitignore, the keyword 'fatal' is prefixed in the\nerror messages. Therefore, prepend the keyword in the expected outputs\nof tests t7400.6 and t7400.16.\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 <shouryashukla.oo@gmail.com>\n---\nThis is my port of 'git submodule add' from shell to C. I had some\nconfusion regarding this segment in the shell script:\n\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\nThis is what I have done in C:\n\n\tif (!force) {\n\t\tif (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n\t} else {\n\t\tint err;\n\t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n\t\t    !is_submodule_populated_gently(path, &err))\n\t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n\t\t\t      \"submodule\"), path);\n\t}\n\nIs this part correct? I am not very sure about this. This particular\npart is not covered in any test or test script, so, I do not have a\nsolid method of knowing the correctness of this segment.\nFeedback and reviews are appreciated.\n---\n\n builtin/submodule--helper.c | 371 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 161 +---------------\n t/t7400-submodule-basic.sh  |   5 +-\n 3 files changed, 375 insertions(+), 162 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex df135abbf1..b8189822a3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2320,6 +2320,376 @@ 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 { NULL, NULL, NULL, NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0 }\n+\n+/*\n+ * Guess dir name from repository: strip leading '.*[/:]',\n+ * strip trailing '[:/]*.git'.\n+ */\n+static char *guess_dir_name(const char *repo)\n+{\n+\tconst char *p, *start, *end, *limit;\n+\tint after_slash_or_colon;\n+\n+\tafter_slash_or_colon = 0;\n+\tlimit = repo + strlen(repo);\n+\tstart = repo;\n+\tend = limit;\n+\tfor (p = repo; p < limit; p++) {\n+\t\tif (starts_with(p, \".git\")) {\n+\t\t\t/* strip trailing '[:/]*.git' */\n+\t\t\tif (!after_slash_or_colon)\n+\t\t\t\tend = p;\n+\t\t\tp += 3;\n+\t\t} else if (*p == '/' || *p == ':') {\n+\t\t\t/* strip leading '.*[/:]' */\n+\t\t\tif (end == limit)\n+\t\t\t\tend = p;\n+\t\t\tafter_slash_or_colon = 1;\n+\t\t} else if (after_slash_or_colon) {\n+\t\t\tstart = p;\n+\t\t\tend = limit;\n+\t\t\tafter_slash_or_colon = 0;\n+\t\t}\n+\t}\n+\treturn xstrndup(start, end - start);\n+}\n+\n+static void fprintf_submodule_remote(const char *str)\n+{\n+\tconst char *p = str;\n+\tconst char *start;\n+\tconst char *end;\n+\tchar *name, *url;\n+\n+\tstart = p;\n+\twhile (*p != ' ')\n+\t\tp++;\n+\tend = p;\n+\tname = xstrndup(start, end - start);\n+\n+\twhile(*p == ' ')\n+\t\tp++;\n+\tstart = p;\n+\twhile (*p != ' ')\n+\t\tp++;\n+\tend = p;\n+\turl = xstrndup(start, end - start);\n+\n+\tfprintf(stderr, \"  %s\\t%s\\n\", name, url);\n+\tfree(name);\n+\tfree(url);\n+}\n+\n+static void modify_remote_v(struct strbuf *sb)\n+{\n+\tint i;\n+\tfor (i = 0; i < sb->len; i++) {\n+\t\tconst char *start = sb->buf + i;\n+\t\tconst char *end = start;\n+\t\twhile (sb->buf[i++] != '\\n')\n+\t\t\tend++;\n+\t\tif (!strcmp(\"fetch\", xstrndup(end - 6, 5)))\n+\t\t\tfprintf_submodule_remote(xstrndup(start, end - start - 7));\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\tstruct child_process cp_rem = CHILD_PROCESS_INIT;\n+\t\t\t\tstruct strbuf sb_rem = STRBUF_INIT;\n+\t\t\t\tcp_rem.git_cmd = 1;\n+\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is \"\n+\t\t\t\t\t\"found locally with remote(s):\\n\"),\n+\t\t\t\t\tinfo->sm_name);\n+\t\t\t\tstrvec_pushf(&cp_rem.env_array,\n+\t\t\t\t\t     \"GIT_DIR=%s\", submodule_git_dir);\n+\t\t\t\tstrvec_push(&cp_rem.env_array, \"GIT_WORK_TREE=.\");\n+\t\t\t\tstrvec_pushl(&cp_rem.args, \"remote\", \"-v\", NULL);\n+\t\t\t\tif (!capture_command(&cp_rem, &sb_rem, 0)) {\n+\t\t\t\t\tmodify_remote_v(&sb_rem);\n+\t\t\t\t}\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+\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, \".gitmodules\", NULL);\n+\n+\tkey = xstrfmt(\"submodule.%s.path\", info->sm_name);\n+\tgit_config_set_in_file_gently(\".gitmodules\", key, info->sm_path);\n+\tfree(key);\n+\tkey = xstrfmt(\"submodule.%s.url\", info->sm_name);\n+\tgit_config_set_in_file_gently(\".gitmodules\", key, info->repo);\n+\tfree(key);\n+\tkey = xstrfmt(\"submodule.%s.branch\", info->sm_name);\n+\tif (info->branch)\n+\t\tgit_config_set_in_file_gently(\".gitmodules\", key, info->branch);\n+\tfree(key);\n+\n+\tif (run_command(&cp))\n+\t\tdie(_(\"failed to add 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 *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 (reference_path && !is_absolute_path(reference_path) && prefix)\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\tpath = guess_dir_name(repo);\n+\t} else {\n+\t\trepo = argv[0];\n+\t\tpath = xstrdup(argv[1]);\n+\t}\n+\n+\tif (!is_absolute_path(path) && prefix)\n+\t\tpath = xstrfmt(\"%s%s\", prefix, 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(path, path);\n+\t/* strip trailing '/' */\n+\tif (is_dir_sep(path[strlen(path) -1]))\n+\t\tpath[strlen(path) - 1] = '\\0';\n+\n+\tif (!force) {\n+\t\tif (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n+\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n+\t} else {\n+\t\tint err;\n+\t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n+\t\t    !is_submodule_populated_gently(path, &err))\n+\t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n+\t\t\t      \"submodule\"), path);\n+\t}\n+\n+\tstrbuf_addstr(&sb, path);\n+\tif (is_directory(path)) {\n+\t\tstruct object_id oid;\n+\t\tif (resolve_gitlink_ref(path, \"HEAD\", &oid) < 0)\n+\t\t\tdie(_(\"'%s' does not have a commit checked out\"), path);\n+\t}\n+\n+\tif (!force) {\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\", path, NULL);\n+\t\tif (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))\n+\t\t\tdie(_(\"%s\"), sb.buf);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tname = custom_name ? custom_name : 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 = 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+\n+\tfree(path);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2352,6 +2722,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 43eb6051d2..434db338a4 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -171,166 +171,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..f26edddbb8 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@@ -171,11 +171,12 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t(\n \t\tcd addtest-ignore &&\n \t\tcat <<-\\EOF >expect &&\n-\t\tThe following paths are ignored by one of your .gitignore files:\n+\t\tfatal: The following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n \t\thint: Use -f if you really want to add them.\n \t\thint: Turn this message off by running\n \t\thint: \"git config advice.addIgnoredFile false\"\n+\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\n-- \n2.28.0\n\n"},{"id":"404348","messageId":"xmqq8se36gev.fsf@gitster.c.googlers.com","threadId":"54121","inReplyTo":"20200824090359.403944-1-shouryashukla.oo@gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-24T18:35:20Z","receivedAt":"2020-08-24T18:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n\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\nHmph.  So,\n\n - if we are not being 'force'd, we see if there is anything in the\n   index for the path and error out, whether it is a gitlink or not.\n\n - if there is 'force' option, we see what the given path is in the\n   index, and if it is already a gitlink, then die.  That sort of\n   makes sense, as long as the remainder of the code deals with the\n   path that is not a submodule in a sensible way.\n\n> This is what I have done in C:\n>\n> \tif (!force) {\n> \t\tif (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n> \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n\nThe shell version would error out with anything in the index, so I'd\nexpect that a faithful conversion would not call is_directory() nor\nsubmodule_from_path() at all---it would just look path up in the_index\nand complains if anything is found.  For example, the quoted part in\nthe original above is what gives the error message when I do\n\n\t$ git submodule add ./Makefile\n\t'Makefile' already exists in the index.\n\nI think.  And the above code won't trigger the \"already exists\" at\nall because 'path' is not a directory.\n\n> \t} else {\n> \t\tint err;\n> \t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n> \t\t    !is_submodule_populated_gently(path, &err))\n> \t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n> \t\t\t      \"submodule\"), path);\n\nLikewise.  The above does much more than the original.\n\nThe original was checking if the found cache entry has 160000 mode\nbit, so the second test would not be is_submodule_populated_gently()\nbut more like !S_ISGITLINK(ce->ce_mode)\n\nNow it is a different question if the original is correct to begin\nwith ;-).  \n\n> \t}\n>\n> Is this part correct? I am not very sure about this. This particular\n> part is not covered in any test or test script, so, I do not have a\n> solid method of knowing the correctness of this segment.\n> Feedback and reviews are appreciated.\n"},{"id":"404371","messageId":"43337924c09119d43c74fdad3f00d4dab76edb51.camel@gmail.com","threadId":"54121","inReplyTo":"xmqq8se36gev.fsf@gitster.c.googlers.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-24T20:30:16Z","receivedAt":"2020-08-24T20:30:30Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Mon, 2020-08-24 at 11:35 -0700, Junio C Hamano wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \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> Hmph.  So,\n> \n>  - if we are not being 'force'd, we see if there is anything in the\n>    index for the path and error out, whether it is a gitlink or not.\n> \n\nRight.\n\n>  - if there is 'force' option, we see what the given path is in the\n>    index, and if it is already a gitlink, then die.  That sort of\n>    makes sense, as long as the remainder of the code deals with the\n>    path that is not a submodule in a sensible way.\n> \n\nWith `force, I think it's the opposite of what you describe. That is:\n\n    - if there is 'force' option, we see what the given path is in the\n      index, and if it is **not** already a gitlink, then die. \n\nNote the `-v` passed to sane_grep.\n\n> > This is what I have done in C:\n> > \n> > \tif (!force) {\n> > \t\tif (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n> > \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> \n> The shell version would error out with anything in the index, so I'd\n> expect that a faithful conversion would not call is_directory() nor\n> submodule_from_path() at all---it would just look path up in the_index\n> and complains if anything is found.  For example, the quoted part in\n> the original above is what gives the error message when I do\n> \n> \t$ git submodule add ./Makefile\n> \t'Makefile' already exists in the index.\n> \n> I think.  And the above code won't trigger the \"already exists\" at\n> all because 'path' is not a directory.\n> \n> > \t} else {\n> > \t\tint err;\n> > \t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n> > \t\t    !is_submodule_populated_gently(path, &err))\n> > \t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n> > \t\t\t      \"submodule\"), path);\n> \n> Likewise.  The above does much more than the original.\n> \n> The original was checking if the found cache entry has 160000 mode\n> bit, so the second test would not be is_submodule_populated_gently()\n> but more like !S_ISGITLINK(ce->ce_mode)\n> \n\nYeah, the C version does need a more proper check in both cases.\n\n\n> Now it is a different question if the original is correct to begin\n> with ;-).  \n> \n\nBy looking at commit message of 619acfc78c (submodule add: extend force\nflag to add existing repos, 2016-10-06), I'm assuming it's correct.\nThere are chances I might be missing something, though.\n\nSpeaking of correctness, I'm surprised how the port passed the\nfollowing test t7400.63 despite the incorrect check.\n\n-- 8< --\n$ ./t7400-submodule-basic.sh\n... snip ...\nok 62 - add submodules without specifying an explicit path\nok 63 - add should fail when path is used by a file\nok 64 - add should fail when path is used by an existing directory\n... snip ...\n-- >8 --\n\nMost likely it passed because it slipped through the incorrect check\nand failed later in the code[1]. That's not good, of course.\n\n> > \t}\n> > \n> > Is this part correct? I am not very sure about this. This particular\n> > part is not covered in any test or test script, so, I do not have a\n> > solid method of knowing the correctness of this segment.\n> > Feedback and reviews are appreciated.\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\tstruct child_process cp_rem = CHILD_PROCESS_INIT;\n> +\t\t\t\tstruct strbuf sb_rem = STRBUF_INIT;\n> +\t\t\t\tcp_rem.git_cmd = 1;\n> +\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is \"\n> +\t\t\t\t\t\"found locally with remote(s):\\n\"),\n> +\t\t\t\t\tinfo->sm_name);\n> +\t\t\t\tstrvec_pushf(&cp_rem.env_array,\n> +\t\t\t\t\t     \"GIT_DIR=%s\", submodule_git_dir);\n> +\t\t\t\tstrvec_push(&cp_rem.env_array, \"GIT_WORK_TREE=.\");\n> +\t\t\t\tstrvec_pushl(&cp_rem.args, \"remote\", \"-v\", NULL);\n> +\t\t\t\tif (!capture_command(&cp_rem, &sb_rem, 0)) {\n> +\t\t\t\t\tmodify_remote_v(&sb_rem);\n> +\t\t\t\t}\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\nThis part results in a difference in error message in shell and C \nversions.\n\n-- 8< --\n$ # Shell version\n$ git submodule add ../subm1 sub\nA git directory for 'sub' is found locally with remote(s):\n  origin        /me/subm1\nIf you want to reuse this local git directory instead of cloning again from\n  /me/subm1\nuse the '--force' option. If the local git directory is not the correct repo\nor you are unsure what this means choose another name with the '--name' option.\n$\n$ # C version\n$ git submodule add ../subm1 sub\nA git directory for 'sub' is found locally with remote(s):\n  origin        /me/subm1\nerror: If you want to reuse this local git directory instead of cloning again from\n   /me/subm1\nuse the '--force' option. If the local git directory is not the correct repo\nor you are unsure what this means choose another name with the '--name' option.\n-- >8 --\n\nNote how the third line is oddly prefixed by a `error` unlike the rest\nof the lines. It would be nice if we could weed out that inconsistency.\nWe could probably use `advise()` for printing the last four lines and\n`error()` for the lines above them.\n\n\nFootnote\n---\n[1]: Looks like not checking for the error message when a command fails\n     has it's own downsides x-(\n\n--\nSivaraam\n\n\n"},{"id":"404374","messageId":"xmqq1rjv4vrb.fsf@gitster.c.googlers.com","threadId":"54121","inReplyTo":"43337924c09119d43c74fdad3f00d4dab76edb51.camel@gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-24T20:46:48Z","receivedAt":"2020-08-24T20:46:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:\n\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>> Hmph.  So,\n>> \n>>  - if we are not being 'force'd, we see if there is anything in the\n>>    index for the path and error out, whether it is a gitlink or not.\n>> \n>\n> Right.\n>\n>>  - if there is 'force' option, we see what the given path is in the\n>>    index, and if it is already a gitlink, then die.  That sort of\n>>    makes sense, as long as the remainder of the code deals with the\n>>    path that is not a submodule in a sensible way.\n>> \n>\n> With `force, I think it's the opposite of what you describe. That is:\n>\n>     - if there is 'force' option, we see what the given path is in the\n>       index, and if it is **not** already a gitlink, then die. \n>\n> Note the `-v` passed to sane_grep.\n\nThanks.\n\nYeah, \"-v ^160000\" passes (i.e. detects an error) if the path exists\nand it is anything but gitlink, so missing path is OK (no input to\ngrep, and grep won't see a gitlink), a blob is not OK (grep sees\nsomething that is not a gitlink), and a gitlink is not OK.\n\nIf $sm_path is a directory with tracked contents, ls-files would\ngive multiple entries, and some of which may or may not be a\ngitlink, but most of them would not be, so it is likely that grep\nwould find one entry that is not gitlink and error out.  Which is a\ngood thing to do.\n\n>> > \t} else {\n>> > \t\tint err;\n>> > \t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n>> > \t\t    !is_submodule_populated_gently(path, &err))\n>> > \t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n>> > \t\t\t      \"submodule\"), path);\n>> \n>> Likewise.  The above does much more than the original.\n>> \n>> The original was checking if the found cache entry has 160000 mode\n>> bit, so the second test would not be is_submodule_populated_gently()\n>> but more like !S_ISGITLINK(ce->ce_mode)\n>\n> Yeah, the C version does need a more proper check in both cases.\n\nEspecially, the case where $sm_path is a directory with tracked\ncontents in it would need a careful examination.\n\nThanks.\n"},{"id":"404503","messageId":"20200826091502.GA29471@konoha","threadId":"54121","inReplyTo":"xmqq8se36gev.fsf@gitster.c.googlers.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-26T09:15:02Z","receivedAt":"2020-08-26T09:15:58Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 24/08 11:35, Junio C Hamano wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \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> Hmph.  So,\n> \n>  - if we are not being 'force'd, we see if there is anything in the\n>    index for the path and error out, whether it is a gitlink or not.\n> \n>  - if there is 'force' option, we see what the given path is in the\n>    index, and if it is already a gitlink, then die.  That sort of\n>    makes sense, as long as the remainder of the code deals with the\n>    path that is not a submodule in a sensible way.\n> \n> > This is what I have done in C:\n> >\n> > \tif (!force) {\n> > \t\tif (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n> > \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> \n> The shell version would error out with anything in the index, so I'd\n> expect that a faithful conversion would not call is_directory() nor\n> submodule_from_path() at all---it would just look path up in the_index\n> and complains if anything is found.  For example, the quoted part in\n> the original above is what gives the error message when I do\n> \n> \t$ git submodule add ./Makefile\n> \t'Makefile' already exists in the index.\n> \n> I think.  And the above code won't trigger the \"already exists\" at\n> all because 'path' is not a directory.\n\nAlright. That is correct. I tried to use a multitude of functions but\ndid not find luck with any of them. The functions I tried:\n\n    - index_path() to check if the path is in the index. For some\n      reason, it switched to the 'default' case and return the\n      'unsupported file type' error.\n\n    - A combination of doing an OR with index_file_exists() and\n      index_dir_exists(). Still no luck. t7406.43 fails.\n\n    - Using index_name_pos() along with the above two functions. Again a\n      failure in the same test.\n\nI feel that index_name_pos() should suffice this task but it fails in\nt7406.43. The SM is in index since 'git ls-files --error-unmatch s1'\ndoes return 's1' (s1 is the submodule). What am I missing here?\n\n> > \t} else {\n> > \t\tint err;\n> > \t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n> > \t\t    !is_submodule_populated_gently(path, &err))\n> > \t\t\tdie(_(\"'%s' already exists in the index and is not a \"\n> > \t\t\t      \"submodule\"), path);\n>\n> Likewise.  The above does much more than the original.\n>\n> The original was checking if the found cache entry has 160000 mode\n> bit, so the second test would not be is_submodule_populated_gently()\n> but more like !S_ISGITLINK(ce->ce_mode)\n\nUsing this results in failure of t7506.[33-40]. I implemented this in\ntwo ways:\n\n    1. Use stat() to initialise the stat st corresponding to the 'path'.\n       Then do a '!S_ISGITLINK(st.st_mode)'.\n\n    2. Run a for loop:\n\t\tfor (i = 0; i < active_nr; i++) {\n\t\tconst struct cache_entry *ce = active_cache[i];\n\n\t\tif (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n\t\t    !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        }\n\n        Still the tests failed. What is meant by 'active_nr' BTW? I am\n        not aware of this term.\n\nWhere am I going wrong for both the if-cases?\n\n"},{"id":"404504","messageId":"CAP6+3T2FbjKc35QYiDmaezzKbkrxEOcBqzirm032_tTU2foZ=Q@mail.gmail.com","threadId":"54121","inReplyTo":"43337924c09119d43c74fdad3f00d4dab76edb51.camel@gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-26T09:27:41Z","receivedAt":"2020-08-26T09:27:46Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 8/25/20, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:\n> On Mon, 2020-08-24 at 11:35 -0700, Junio C Hamano wrote:\n>> Now it is a different question if the original is correct to begin\n>> with ;-).\n>>\n>\n> By looking at commit message of 619acfc78c (submodule add: extend force\n> flag to add existing repos, 2016-10-06), I'm assuming it's correct.\n> There are chances I might be missing something, though.\n>\n> Speaking of correctness, I'm surprised how the port passed the\n> following test t7400.63 despite the incorrect check.\n>\n> -- 8< --\n> $ ./t7400-submodule-basic.sh\n> ... snip ...\n> ok 62 - add submodules without specifying an explicit path\n> ok 63 - add should fail when path is used by a file\n> ok 64 - add should fail when path is used by an existing directory\n> ... snip ...\n> -- >8 --\n>\n> Most likely it passed because it slipped through the incorrect check\n> and failed later in the code[1]. That's not good, of course.\n>\n>> > \t}\n>> >\n>> > Is this part correct? I am not very sure about this. This particular\n>> > part is not covered in any test or test script, so, I do not have a\n>> > solid method of knowing the correctness of this segment.\n>> > Feedback and reviews are appreciated.\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\tstruct child_process cp_rem = CHILD_PROCESS_INIT;\n>> +\t\t\t\tstruct strbuf sb_rem = STRBUF_INIT;\n>> +\t\t\t\tcp_rem.git_cmd = 1;\n>> +\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is \"\n>> +\t\t\t\t\t\"found locally with remote(s):\\n\"),\n>> +\t\t\t\t\tinfo->sm_name);\n>> +\t\t\t\tstrvec_pushf(&cp_rem.env_array,\n>> +\t\t\t\t\t     \"GIT_DIR=%s\", submodule_git_dir);\n>> +\t\t\t\tstrvec_push(&cp_rem.env_array, \"GIT_WORK_TREE=.\");\n>> +\t\t\t\tstrvec_pushl(&cp_rem.args, \"remote\", \"-v\", NULL);\n>> +\t\t\t\tif (!capture_command(&cp_rem, &sb_rem, 0)) {\n>> +\t\t\t\t\tmodify_remote_v(&sb_rem);\n>> +\t\t\t\t}\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> This part results in a difference in error message in shell and C\n> versions.\n>\n> -- 8< --\n> $ # Shell version\n> $ git submodule add ../subm1 sub\n> A git directory for 'sub' is found locally with remote(s):\n>   origin        /me/subm1\n> If you want to reuse this local git directory instead of cloning again from\n>   /me/subm1\n> use the '--force' option. If the local git directory is not the correct\n> repo\n> or you are unsure what this means choose another name with the '--name'\n> option.\n> $\n> $ # C version\n> $ git submodule add ../subm1 sub\n> A git directory for 'sub' is found locally with remote(s):\n>   origin        /me/subm1\n> error: If you want to reuse this local git directory instead of cloning\n> again from\n>    /me/subm1\n> use the '--force' option. If the local git directory is not the correct\n> repo\n> or you are unsure what this means choose another name with the '--name'\n> option.\n> -- >8 --\n>\n> Note how the third line is oddly prefixed by a `error` unlike the rest\n> of the lines. It would be nice if we could weed out that inconsistency.\n> We could probably use `advise()` for printing the last four lines and\n> `error()` for the lines above them.\n\nUnderstood. I will correct this part. BTW, you surely are talking\nabout error() on\nthe first 2 lines? I think fprintf(stderr, _()) is OK for them otherwise they\nwill be prefixed by 'error:' which will not be in line with the shell version.\n"},{"id":"404508","messageId":"5899bf6e-f61f-8a19-196d-d38d611dc037@gmail.com","threadId":"54121","inReplyTo":"CAP6+3T2FbjKc35QYiDmaezzKbkrxEOcBqzirm032_tTU2foZ=Q@mail.gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-26T10:54:39Z","receivedAt":"2020-08-26T10:54:48Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 26-08-2020 14:57, Shourya Shukla wrote:\n> On 8/25/20, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:\n>>\n>> This part results in a difference in error message in shell and C\n>> versions.\n>>\n>> -- 8< --\n>> $ # Shell version\n>> $ git submodule add ../subm1 sub\n>> A git directory for 'sub' is found locally with remote(s):\n>>   origin        /me/subm1\n>> If you want to reuse this local git directory instead of cloning again from\n>>   /me/subm1\n>> use the '--force' option. If the local git directory is not the correct\n>> repo\n>> or you are unsure what this means choose another name with the '--name'\n>> option.\n>> $\n>> $ # C version\n>> $ git submodule add ../subm1 sub\n>> A git directory for 'sub' is found locally with remote(s):\n>>   origin        /me/subm1\n>> error: If you want to reuse this local git directory instead of cloning\n>> again from\n>>    /me/subm1\n>> use the '--force' option. If the local git directory is not the correct\n>> repo\n>> or you are unsure what this means choose another name with the '--name'\n>> option.\n>> -- >8 --\n>>\n>> Note how the third line is oddly prefixed by a `error` unlike the rest\n>> of the lines. It would be nice if we could weed out that inconsistency.\n>> We could probably use `advise()` for printing the last four lines and\n>> `error()` for the lines above them.\n> \n> Understood. I will correct this part. BTW, you surely are talking\n> about error() on\n> the first 2 lines? I think fprintf(stderr, _()) is OK for them otherwise they\n> will be prefixed by 'error:' which will not be in line with the shell version.\n> \n\nYes. It's better to prefix them with `error` because well... it is an\nerror. I realize the shell version didn't explicitly do this but that\ndoesn't necessarily mean the error message was helpful without the\nprefix. AFAIK, many Git commands prefix their error messages with\n`fatal` or `error` which makes it easy to distinguish error messages\nfrom actual program output in the terminal. So, it's good to do the same\nhere.\n\n-- \nSivaraam\n"},{"id":"404771","messageId":"ce151a1408291bb0991ce89459e36ee13ccdfa52.camel@gmail.com","threadId":"54121","inReplyTo":"20200826091502.GA29471@konoha","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-30T19:58:53Z","receivedAt":"2020-08-30T19:59:10Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wed, 2020-08-26 at 14:45 +0530, Shourya Shukla wrote:\n> On 24/08 11:35, Junio C Hamano wrote:\n> > Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> > \n> > The shell version would error out with anything in the index, so I'd\n> > expect that a faithful conversion would not call is_directory() nor\n> > submodule_from_path() at all---it would just look path up in the_index\n> > and complains if anything is found.  For example, the quoted part in\n> > the original above is what gives the error message when I do\n> > \n> > \t$ git submodule add ./Makefile\n> > \t'Makefile' already exists in the index.\n> > \n> > I think.  And the above code won't trigger the \"already exists\" at\n> > all because 'path' is not a directory.\n> \n> Alright. That is correct. I tried to use a multitude of functions but\n> did not find luck with any of them. The functions I tried:\n> \n\nIt would've been nice to see the actual code you tried so that it's\neasier for others to more easily identify if you're using the wrong\nfunction or using the correct function in the wrong way.\n\n>     - index_path() to check if the path is in the index. For some\n>       reason, it switched to the 'default' case and return the\n>       'unsupported file type' error.\n> \n>     - A combination of doing an OR with index_file_exists() and\n>       index_dir_exists(). Still no luck. t7406.43 fails.\n> \n>     - Using index_name_pos() along with the above two functions. Again a\n>       failure in the same test.\n> \n> I feel that index_name_pos() should suffice this task but it fails in\n> t7406.43. The SM is in index since 'git ls-files --error-unmatch s1'\n> does return 's1' (s1 is the submodule). What am I missing here?\n> \n\nYou're likely missing the fact that you should call `read_cache` before\nusing `index_name_pos` or the likes of it.\n\nFor instance, the following works without issues for most cases (more\non that below):\n\n        if (read_cache() < 0)\n                die(_(\"index file corrupt\"));\n\n        cache_pos = cache_name_pos(path, strlen(path));\n        if (cache_pos >= 0) {\n                if (!force) {\n                        die(_(\"'%s' already exists in the index\"),\npath);\n                }\n                else {\n                        struct cache_entry *ce = the_index.cache[cache_pos];\n\n                        if (!S_ISGITLINK(ce->ce_mode))\n                                die(_(\"'%s' already exists in the index and is not a \"\n                                      \"submodule\"), path);\n                }\n        }\n\nThis is more close to what the shell version did but misses one case\nwhich might or might not be covered by the test suite[1]. The case when\npath is a directory that has tracked contents. In the shell version we\nwould get:\n\n   $ git submodule add ../git-crypt/ builtin\n   'builtin' already exists in the index\n   $ git submodule add --force ../git-crypt/ builtin\n   'builtin' already exists in the index and is not a submodule\n\n   In the C version with the above snippet we get:\n\n   $ git submodule add --force ../git-crypt/ builtin\n   fatal: 'builtin' does not have a commit checked out\n   $ git submodule add ../git-crypt/ builtin\n   fatal: 'builtin' does not have a commit checked out\n\n   That's not appropriate and should be fixed. I believe we could do\n   something with `cache_dir_exists` to fix this.\n\n\n   Footnote\n   ===\n\n   [1]: If it's not covered already, it might be a good idea to add a test\n   for the above case.\n\n   --\n   Sivaraam\n\n\n"},{"id":"404813","messageId":"20200831130448.GA119147@konoha","threadId":"54121","inReplyTo":"ce151a1408291bb0991ce89459e36ee13ccdfa52.camel@gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-31T13:04:48Z","receivedAt":"2020-08-31T13:12:01Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 31/08 01:28, Kaartic Sivaraam wrote:\n> On Wed, 2020-08-26 at 14:45 +0530, Shourya Shukla wrote:\n> > On 24/08 11:35, Junio C Hamano wrote:\n> > > Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> > > \n> > > The shell version would error out with anything in the index, so I'd\n> > > expect that a faithful conversion would not call is_directory() nor\n> > > submodule_from_path() at all---it would just look path up in the_index\n> > > and complains if anything is found.  For example, the quoted part in\n> > > the original above is what gives the error message when I do\n> > > \n> > > \t$ git submodule add ./Makefile\n> > > \t'Makefile' already exists in the index.\n> > > \n> > > I think.  And the above code won't trigger the \"already exists\" at\n> > > all because 'path' is not a directory.\n> > \n> > Alright. That is correct. I tried to use a multitude of functions but\n> > did not find luck with any of them. The functions I tried:\n> > \n> \n> It would've been nice to see the actual code you tried so that it's\n> easier for others to more easily identify if you're using the wrong\n> function or using the correct function in the wrong way.\n\nYeah, that is my fault. I will tag along below.\n\n> >     - index_path() to check if the path is in the index. For some\n> >       reason, it switched to the 'default' case and return the\n> >       'unsupported file type' error.\n> > \n> >     - A combination of doing an OR with index_file_exists() and\n> >       index_dir_exists(). Still no luck. t7406.43 fails.\n> > \n> >     - Using index_name_pos() along with the above two functions. Again a\n> >       failure in the same test.\n> > \n> > I feel that index_name_pos() should suffice this task but it fails in\n> > t7406.43. The SM is in index since 'git ls-files --error-unmatch s1'\n> > does return 's1' (s1 is the submodule). What am I missing here?\n> > \n> \n> You're likely missing the fact that you should call `read_cache` before\n> using `index_name_pos` or the likes of it.\n\nAlright, called it.\n\n> For instance, the following works without issues for most cases (more\n> on that below):\n> \n>         if (read_cache() < 0)\n>                 die(_(\"index file corrupt\"));\n> \n>         cache_pos = cache_name_pos(path, strlen(path));\n>         if (cache_pos >= 0) {\n>                 if (!force) {\n>                         die(_(\"'%s' already exists in the index\"),\n> path);\n>                 }\n>                 else {\n>                         struct cache_entry *ce = the_index.cache[cache_pos];\n> \n>                         if (!S_ISGITLINK(ce->ce_mode))\n>                                 die(_(\"'%s' already exists in the index and is not a \"\n>                                       \"submodule\"), path);\n>                 }\n>         }\n\nI actually did this only using 'index_*()' functions. But made a very\nvery very silly mistake:\nI did a sizeof() instead of strlen() and I did not notice this until\nI saw what you did. IDK how I made this mistake.\n\nThis is what I have done finally:\n---\n\tif (read_cache() < 0)\n\t\tdie(_(\"index file corrupt\"));\n\n\tif (!force) {\n\t\tif (cache_file_exists(path, strlen(path), ignore_case) ||\n\t\t    cache_dir_exists(path, strlen(path)))\n\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n\t} else {\n\t\tint cache_pos = cache_name_pos(path, strlen(path));\n\t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n\t\tif (cache_pos >= 0 && !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---\n\nI did not put the 'cache_pos >= 0' at the start since I thought that it\nwill unnecessarily increase an indentation level. Since we are using\n'cache_{file,dir}_exists' in the first check and 'cache_name_pos()' in\nthe second, the placement of check at another indentation level would be\nunnecessary. What do you think about this?\n\n> This is more close to what the shell version did but misses one case\n> which might or might not be covered by the test suite[1]. The case when\n> path is a directory that has tracked contents. In the shell version we\n> would get:\n> \n>    $ git submodule add ../git-crypt/ builtin\n>    'builtin' already exists in the index\n>    $ git submodule add --force ../git-crypt/ builtin\n>    'builtin' already exists in the index and is not a submodule\n> \n>    In the C version with the above snippet we get:\n> \n>    $ git submodule add --force ../git-crypt/ builtin\n>    fatal: 'builtin' does not have a commit checked out\n>    $ git submodule add ../git-crypt/ builtin\n>    fatal: 'builtin' does not have a commit checked out\n> \n>    That's not appropriate and should be fixed. I believe we could do\n>    something with `cache_dir_exists` to fix this.\n> \n> \n>    Footnote\n>    ===\n> \n>    [1]: If it's not covered already, it might be a good idea to add a test\n>    for the above case.\n\nLike Junio said, we do not care if it is a file or a directory of any\nsorts, we will give the error if it already exists. Therefore, even if\nit is an untracked or a tracked one, it should not matter to us. Hence\ntesting for it may not be necessary is what I feel. Why should we test\nit?\n\n"},{"id":"404881","messageId":"31e40c63bbac03d261ac6f46a0d2f6ae90a21038.camel@gmail.com","threadId":"54121","inReplyTo":"20200831130448.GA119147@konoha","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-09-01T20:35:22Z","receivedAt":"2020-09-01T20:35:35Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Mon, 2020-08-31 at 18:34 +0530, Shourya Shukla wrote:\n> On 31/08 01:28, Kaartic Sivaraam wrote:\n> \n> This is what I have done finally:\n> ---\n> \tif (read_cache() < 0)\n> \t\tdie(_(\"index file corrupt\"));\n> \n> \tif (!force) {\n> \t\tif (cache_file_exists(path, strlen(path), ignore_case) ||\n> \t\t    cache_dir_exists(path, strlen(path)))\n> \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> \t} else {\n> \t\tint cache_pos = cache_name_pos(path, strlen(path));\n> \t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n> \t\tif (cache_pos >= 0 && !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> ---\n> \n> I did not put the 'cache_pos >= 0' at the start since I thought that it\n> will unnecessarily increase an indentation level. Since we are using\n> 'cache_{file,dir}_exists' in the first check and 'cache_name_pos()' in\n> the second, the placement of check at another indentation level would be\n> unnecessary. What do you think about this?\n> \n\nInterestingly. 'cache_dir_exists' seems to work as expected only when\nthe global ignore_case whose value seems to depend on core.ignorecase.\nSo, we can't just rely on 'cache_dir_exists to identify a directory\nthat has tracked contents. Apparently, the 'directory_exists_in_index'\nin 'dir.c' seems to have the code that we want here (which is also the\nonly user of 'index_dir_exists'; the function for which\n'cache_dir_exists' is a convenience wrapper.\n\nThe best idea I could think of is to expose that method and re-use it\nhere. Given that my kowledge about index and caching is primitive, I'm\nnot sure if there's a better approach. If others have a better idea for\nhandling this directory case, do enlighten us.\n\n> > This is more close to what the shell version did but misses one case\n> > which might or might not be covered by the test suite[1]. The case when\n> > path is a directory that has tracked contents. In the shell version we\n> > would get:\n> > \n> >    $ git submodule add ../git-crypt/ builtin\n> >    'builtin' already exists in the index\n> >    $ git submodule add --force ../git-crypt/ builtin\n> >    'builtin' already exists in the index and is not a submodule\n> > \n> >    In the C version with the above snippet we get:\n> > \n> >    $ git submodule add --force ../git-crypt/ builtin\n> >    fatal: 'builtin' does not have a commit checked out\n> >    $ git submodule add ../git-crypt/ builtin\n> >    fatal: 'builtin' does not have a commit checked out\n> > \n> >    That's not appropriate and should be fixed. I believe we could do\n> >    something with `cache_dir_exists` to fix this.\n> > \n> > \n> >    Footnote\n> >    ===\n> > \n> >    [1]: If it's not covered already, it might be a good idea to add a test\n> >    for the above case.\n> \n> Like Junio said, we do not care if it is a file or a directory of any\n> sorts, we will give the error if it already exists. Therefore, even if\n> it is an untracked or a tracked one, it should not matter to us. Hence\n> testing for it may not be necessary is what I feel. Why should we test\n> it?\n\nI'm guessing you misunderstood. A few things:\n\n- We only care about tracked contents for the case in hand.\n\n- Identifying whether a given path corresponds to a directory\n  which has tracked contents is tricky. Neither 'cache_name_pos'\n  nor 'cache_file_exists' handle this. 'cache_dir_exists' is also\n  not very useful as mentioned above.\n\nSo, we do have to take care when handling that case as Junio pointed\nout.\n\n-- \nSivaraam\n\n\n"},{"id":"404914","messageId":"20200902120422.GA28650@konoha","threadId":"54121","inReplyTo":"31e40c63bbac03d261ac6f46a0d2f6ae90a21038.camel@gmail.com","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-09-02T12:04:22Z","receivedAt":"2020-09-02T12:04:38Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 02/09 02:05, Kaartic Sivaraam wrote:\n> On Mon, 2020-08-31 at 18:34 +0530, Shourya Shukla wrote:\n> > On 31/08 01:28, Kaartic Sivaraam wrote:\n> > \n> > This is what I have done finally:\n> > ---\n> > \tif (read_cache() < 0)\n> > \t\tdie(_(\"index file corrupt\"));\n> > \n> > \tif (!force) {\n> > \t\tif (cache_file_exists(path, strlen(path), ignore_case) ||\n> > \t\t    cache_dir_exists(path, strlen(path)))\n> > \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> > \t} else {\n> > \t\tint cache_pos = cache_name_pos(path, strlen(path));\n> > \t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n> > \t\tif (cache_pos >= 0 && !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> > ---\n> > \n> > I did not put the 'cache_pos >= 0' at the start since I thought that it\n> > will unnecessarily increase an indentation level. Since we are using\n> > 'cache_{file,dir}_exists' in the first check and 'cache_name_pos()' in\n> > the second, the placement of check at another indentation level would be\n> > unnecessary. What do you think about this?\n> > \n> \n> Interestingly. 'cache_dir_exists' seems to work as expected only when\n> the global ignore_case whose value seems to depend on core.ignorecase.\n> So, we can't just rely on 'cache_dir_exists to identify a directory\n> that has tracked contents. Apparently, the 'directory_exists_in_index'\n> in 'dir.c' seems to have the code that we want here (which is also the\n> only user of 'index_dir_exists'; the function for which\n> 'cache_dir_exists' is a convenience wrapper.\n\nI think both 'cache_{dir,file}_exists()' depend on 'core.ignorecase'\nthough I am not able to confirm this for 'cache_dir_exists()'. Where\nexactly does this happen for the function? The function you mention\nseems perfect to me, though, we will also have to make the enum\n'exist_status' visible. Will that be fine? The final output will be:\n---\n\tif (!force) {\n\t\tif (directory_exists_in_index(&the_index, path, strlen(path)))\n\t\t\tdie(_(\"'%s' already exists in the index\"), path);\n\t} else {\n\t\tint cache_pos = cache_name_pos(path, strlen(path));\n\t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n\t\tif (cache_pos >= 0 && !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---\n\n\nAnd obviously an extra commit changing the visibility of the function\nand the enum.\n \n> > > This is more close to what the shell version did but misses one case\n> > > which might or might not be covered by the test suite[1]. The case when\n> > > path is a directory that has tracked contents. In the shell version we\n> > > would get:\n> > > \n> > >    $ git submodule add ../git-crypt/ builtin\n> > >    'builtin' already exists in the index\n> > >    $ git submodule add --force ../git-crypt/ builtin\n> > >    'builtin' already exists in the index and is not a submodule\n> > > \n> > >    In the C version with the above snippet we get:\n> > > \n> > >    $ git submodule add --force ../git-crypt/ builtin\n> > >    fatal: 'builtin' does not have a commit checked out\n> > >    $ git submodule add ../git-crypt/ builtin\n> > >    fatal: 'builtin' does not have a commit checked out\n> > > \n> > >    That's not appropriate and should be fixed. I believe we could do\n> > >    something with `cache_dir_exists` to fix this.\n> > > \n> > > \n> > >    Footnote\n> > >    ===\n> > > \n> > >    [1]: If it's not covered already, it might be a good idea to add a test\n> > >    for the above case.\n> > \n> > Like Junio said, we do not care if it is a file or a directory of any\n> > sorts, we will give the error if it already exists. Therefore, even if\n> > it is an untracked or a tracked one, it should not matter to us. Hence\n> > testing for it may not be necessary is what I feel. Why should we test\n> > it?\n> \n> I'm guessing you misunderstood. A few things:\n> \n> - We only care about tracked contents for the case in hand.\n> \n> - Identifying whether a given path corresponds to a directory\n>   which has tracked contents is tricky. Neither 'cache_name_pos'\n>   nor 'cache_file_exists' handle this. 'cache_dir_exists' is also\n>   not very useful as mentioned above.\n> \n> So, we do have to take care when handling that case as Junio pointed\n> out.\n\nI still do not understand this case. Let's say this was our\nsuperproject:\n\n.gitmodules .git/ a.txt dir1/\n\nAnd we did:\n    $ git submodule add <url> dir1/\n\nNow, at this point, how does it matter if 'dir1/' has tracked content or\nnot right? A directory exists with that name and now we do not add the\nSM to that path.\n\n"},{"id":"404960","messageId":"dba90fee82a709538b9bff015e56a3c4834a42ca.camel@gmail.com","threadId":"54121","inReplyTo":"20200902120422.GA28650@konoha","subject":"Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-09-03T08:46:17Z","receivedAt":"2020-09-03T08:46:30Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"+Cc: Elijah Newren, Martin Ågren\n\nOn Wed, 2020-09-02 at 17:34 +0530, Shourya Shukla wrote:\n> On 02/09 02:05, Kaartic Sivaraam wrote:\n> > On Mon, 2020-08-31 at 18:34 +0530, Shourya Shukla wrote:\n> > > On 31/08 01:28, Kaartic Sivaraam wrote:\n> > > \n> > > This is what I have done finally:\n> > > ---\n> > > \tif (read_cache() < 0)\n> > > \t\tdie(_(\"index file corrupt\"));\n> > > \n> > > \tif (!force) {\n> > > \t\tif (cache_file_exists(path, strlen(path), ignore_case) ||\n> > > \t\t    cache_dir_exists(path, strlen(path)))\n> > > \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> > > \t} else {\n> > > \t\tint cache_pos = cache_name_pos(path, strlen(path));\n> > > \t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n> > > \t\tif (cache_pos >= 0 && !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> > > ---\n> > > \n> > > I did not put the 'cache_pos >= 0' at the start since I thought that it\n> > > will unnecessarily increase an indentation level. Since we are using\n> > > 'cache_{file,dir}_exists' in the first check and 'cache_name_pos()' in\n> > > the second, the placement of check at another indentation level would be\n> > > unnecessary. What do you think about this?\n> > > \n> > \n> > Interestingly. 'cache_dir_exists' seems to work as expected only when\n> > the global ignore_case whose value seems to depend on core.ignorecase.\n> > So, we can't just rely on 'cache_dir_exists to identify a directory\n> > that has tracked contents. Apparently, the 'directory_exists_in_index'\n> > in 'dir.c' seems to have the code that we want here (which is also the\n> > only user of 'index_dir_exists'; the function for which\n> > 'cache_dir_exists' is a convenience wrapper.\n> \n> I think both 'cache_{dir,file}_exists()' depend on 'core.ignorecase'\n> though I am not able to confirm this for 'cache_dir_exists()'. Where\n> exactly does this happen for the function?\n\nAs you can see in 'name-hash.c', 'index_file_exists' and there by\n'cache_dir_exists' work using the 'name_hash' stored in the index. If\nyou look at the flow of 'lazy_init_name_hash', you'll see how\n'name_hash' gets initialized and populated despite the value of\n'ignore_case'. OTOH, dir_hash is populted only when 'ignore_case' is\ntrue. So, it seems to be that only 'cache_dir_exists' depends on the\nvalue of 'ignore_case'.\n\n>  The function you mention\n> seems perfect to me, though, we will also have to make the enum\n> 'exist_status' visible. Will that be fine?\n\nTo me that appears to be the only way forward other than spawning a\ncall to ls-files as was done in one of the earlier versions tat was not\nsent to the list. Anyways, I'm not the best person to answer this\nquestion. So, I've CC-ed a couple of people who might be able to shed\nsome light for us.\n\n>  The final output will be:\n> ---\n> \tif (!force) {\n> \t\tif (directory_exists_in_index(&the_index, path, strlen(path)))\n> \t\t\tdie(_(\"'%s' already exists in the index\"), path);\n> \t} else {\n> \t\tint cache_pos = cache_name_pos(path, strlen(path));\n> \t\tstruct cache_entry *ce = the_index.cache[cache_pos];\n> \t\tif (cache_pos >= 0 && !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> ---\n> \n> \n\nThe above doesn't handle all cases. In particular, we want to handle\nthe case of tracked files when `force` is not given\n(directory_exists_in_index certainly doesn't handle that). We also need\nto handle directories with tracked contents when force is given (we\nalready know cache_name_pos is not sufficient to handle them). So, I\nthink we would want something along the lines of the following:\n\n        if (read_cache() < 0)\n                die(_(\"index file corrupt\"));\n\n        cache_pos = cache_name_pos(path, strlen(path));\n        if (cache_pos < 0 &&\n            directory_exists_in_index(&the_index, path, strlen(path)) == index_directory) {\n                directory_in_cache = 1;\n        }\n\n        if (!force) {\n               if (cache_pos >= 0 || directory_in_cache)\n                        die(_(\"'%s' already exists in the index\"), path);\n        }\n        else {\n                struct cache_entry *ce = NULL;\n                if (cache_pos >= 0)\n                {\n                        ce = the_index.cache[cache_pos];\n                }\n\n                if (directory_in_cache || (ce && !S_ISGITLINK(ce->ce_mode))) {\n                        die(_(\"'%s' already exists in the index and is not a \"\n                              \"submodule\"), path);\n                }\n        }\n\nAfter seeing this, I'm starting to think it's better have this in a\nseparate helper function instead of making the `module_add` function\neven more longer than it already is.\n\n> And obviously an extra commit changing the visibility of the function\n> and the enum.\n>  \n> > > > This is more close to what the shell version did but misses one case\n> > > > which might or might not be covered by the test suite[1]. The case when\n> > > > path is a directory that has tracked contents. In the shell version we\n> > > > would get:\n> > > > \n> > > >    $ git submodule add ../git-crypt/ builtin\n> > > >    'builtin' already exists in the index\n> > > >    $ git submodule add --force ../git-crypt/ builtin\n> > > >    'builtin' already exists in the index and is not a submodule\n> > > > \n> > > >    In the C version with the above snippet we get:\n> > > > \n> > > >    $ git submodule add --force ../git-crypt/ builtin\n> > > >    fatal: 'builtin' does not have a commit checked out\n> > > >    $ git submodule add ../git-crypt/ builtin\n> > > >    fatal: 'builtin' does not have a commit checked out\n> > > > \n> > > >    That's not appropriate and should be fixed. I believe we could do\n> > > >    something with `cache_dir_exists` to fix this.\n> > > > \n> > > > \n> > > >    Footnote\n> > > >    ===\n> > > > \n> > > >    [1]: If it's not covered already, it might be a good idea to add a test\n> > > >    for the above case.\n> > > \n> > > Like Junio said, we do not care if it is a file or a directory of any\n> > > sorts, we will give the error if it already exists. Therefore, even if\n> > > it is an untracked or a tracked one, it should not matter to us. Hence\n> > > testing for it may not be necessary is what I feel. Why should we test\n> > > it?\n> > \n> > I'm guessing you misunderstood. A few things:\n> > \n> > - We only care about tracked contents for the case in hand.\n> > \n> > - Identifying whether a given path corresponds to a directory\n> >   which has tracked contents is tricky. Neither 'cache_name_pos'\n> >   nor 'cache_file_exists' handle this. 'cache_dir_exists' is also\n> >   not very useful as mentioned above.\n> > \n> > So, we do have to take care when handling that case as Junio pointed\n> > out.\n> \n> I still do not understand this case. Let's say this was our\n> superproject:\n> \n> .gitmodules .git/ a.txt dir1/\n> \n> And we did:\n>     $ git submodule add <url> dir1/\n> \n> Now, at this point, how does it matter if 'dir1/' has tracked content or\n> not right? A directory exists with that name and now we do not add the\n> SM to that path.\n> \n\nI'm guessing you're looking at it in a more general sense of the\ncommand workflow. I was speaking only about the following snippet of\nthe shell script which we're trying to emulate now:\n\n        if test -z \"$force\"\n        then\n                git ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n                die \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n        else\n                git ls-files -s \"$sm_path\" | sane_grep -v \"^160000\" > /dev/null 2>&1 &&\n                die \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n        fi\n\nWhen sm_path is an empty directory or a directory that has no tracked\ncontents the 'ls-files' command would fail and we apparently will *not*\nget an error stating the path already exists in the index. The command\nmight fail in a later part of the code but that's not what I'm talking\nabout.\n\nA few other things I noticed:\n\n> +       strbuf_addstr(&sb, path);\n> +       if (is_directory(path)) {\n\nI remember mentioning to you that the 'is_directory' check is\nsufficient here and the 'is_nonbare_repository_dir' is not necessary\nhere as 'resolve_gitlink_ref' already takes care of it. Unfortunately,\nlooks like without the 'is_nonbare_repository_dir' check we get the\nfollowing unhelpful error message when the path is a directory that\n_exists_ and is ignored in .gitignore:\n\n   $ git submodule add ../git-crypt/ Debug\n   fatal: 'Debug' does not have a commit checked out\n\n   The shell version did not have this problem and gave the following\n   appropriate error message:\n\n   $ git submodule add ../git-crypt/ Debug\n   The following paths are ignored by one of your .gitignore files:\n   Debug\n   hint: Use -f if you really want to add them.\n   hint: Turn this message off by running\n   hint: \"git config advice.addIgnoredFile false\"\n\n      So, we should check whether the given directory is a non-bare\n      repository before calling 'resolve_gitlink_ref' to be consistent with\n      what the shell version does.\n\n      For the note, this isn't caught by the 'submodule add to .gitignored\n      path fails' in t7400 as the corresponding directory doesn't exist\n      there. So, our 'is_directory' check fails and we don't call\n      'resolve_gitlink_ref'.\n\n      > +               struct object_id oid;\n> +               if (resolve_gitlink_ref(path, \"HEAD\", &oid) < 0)\n> +                       die(_(\"'%s' does not have a commit checked out\"), path);\n> +       }\n> +\n> +       if (!force) {\n> +               struct strbuf sb = STRBUF_INIT;\n> +               struct child_process cp = CHILD_PROCESS_INIT;\n> +               cp.git_cmd = 1;\n> +               cp.no_stdout = 1;\n> +               strvec_pushl(&cp.args, \"add\", \"--dry-run\", \"--ignore-missing\",\n> +                            \"--no-warn-embedded-repo\", path, NULL);\n> +               if (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))\n> +                       die(_(\"%s\"), sb.buf);\n\nUsing 'die' to print an already formatted error message of a command\nresults in an additional newline which looks ugly. For reference, here\nare the output from the shell and C versions of the command:\n\n-- 8< --\n$ # Shell version\n$ git submodule add ../parent/ submod\nThe following paths are ignored by one of your .gitignore files:\nsubmod\nhint: Use -f if you really want to add them.\nhint: Turn this message off by running\nhint: \"git config advice.addIgnoredFile false\"\n$ # C version\n$ git submodule add ../parent/ submod\nfatal: The following paths are ignored by one of your .gitignore files:\nsubmod\nhint: Use -f if you really want to add them.\nhint: Turn this message off by running\nhint: \"git config advice.addIgnoredFile false\"\n\n$\n-- >8 --\n\nSo, it would be nice if we use 'fprintf(stderr, ...)' or something like\nthat so that we don't get the additional newline.\n\n> +               strbuf_release(&sb);\n> +       }\n> \n\n-- \nSivaraam\n\n\n"}]}