{"thread":{"id":"54360","subject":"[PATCH v2 0/3] submodule: port subcommand add from shell to C","startedAt":"2020-10-07T07:45:53Z","lastAt":"2020-11-19T20:37:41Z","messageCount":16,"participants":["Shourya Shukla","Junio C Hamano","Jonathan Tan","Emily Shaffer","Josh Steadmon","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"407029","messageId":"20201007074538.25891-1-shouryashukla.oo@gmail.com","threadId":"54360","inReplyTo":null,"subject":"[PATCH v2 0/3] submodule: port subcommand add from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-07T07:45:35Z","receivedAt":"2020-10-07T07:45:53Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello all,\n\nThis is the v2 of the patch with the same title, delivered more than a\nmonth ago as a part of my GSoC. Link to v1:\nhttps://lore.kernel.org/git/20200824090359.403944-1-shouryashukla.oo@gmail.com/\n\nThe changelog is as follows:\n\n    1. Introduce PATCH[1/3](dir: change the scope of function\n       'directory_exists_in_index()', 2020-10-06). This was done since\n       the above mentioned function will be used in the patch that\n       follows.\n\n    2. There are multiple changes in this commit:\n\n            A. Improve the part which checks if the 'path' given as\n               argument exists or not. Implementing Kaartic's\n               suggestions on the patch, I had to make sure that the\n               case for checking if the path has tracked contents or\n               not also works.\n\n            B. Also, wrap the aforementioned segment in a function\n               since it became very long. The function is called\n               'check_sm_exists()'.\n\n            C. Also, use the function 'is_nonbare_repository_dir()'\n               instead of 'is_directory()' when trying to resolve\n               gitlink.\n\n            D. Append keyword 'fatal' in front of the expected output of\n               test t7400.6 since the command die()s out in case of\n               absence of commits in a submodule.\n\n            E. Remove the extra `#include \"dir.h\"` from\n               'submodule--helper.c'.\n\n    3. Introduce PATCH[3/3] (t7400: add test to check 'submodule add'\n       for tracked paths, 2020-10-07). Kaartic pointed out that a test\n       for path with tracked contents did not exist and hence it was\n       necessary to write one. Therefore, this commit introduces a new\n       test 't7400.18: submodule add to path with tracked contents\n       fails'.\n\nComments and feedback are appreciated. Sorry for the month long delay, I\nwas on a vacation.\n\nI am attaching a range-diff between v1 and v2 at the end of this mail.\n\nRegards,\nShourya Shukla\n-----\n\n-:  ---------- > 1:  bdac00494e dir: change the scope of function 'directory_exists_in_index()'\n1:  b08d81e179 ! 2:  3e20d0fe04 submodule: port submodule subcommand 'add' from shell to C\n    @@ Commit message\n         'git-submodule.sh'.\n\n         Also, since the command die()s out in case of absence of commits in the\n    -    submodule and exits with exit status 1 when we try adding a submodule\n    -    which is mentioned in .gitignore, the keyword 'fatal' is prefixed in the\n    -    error messages. Therefore, prepend the keyword in the expected outputs\n    -    of tests t7400.6 and t7400.16.\n    +    submodule, the keyword 'fatal' is prefixed in the error messages.\n    +    Therefore, prepend the keyword in the expected output of test t7400.6.\n    +\n    +    While at it, eliminate the extra preprocessor directive\n    +    `#include \"dir.h\"` at the start of 'submodule--helper.c'.\n\n         Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n         Mentored-by: Stefan Beller <stefanbeller@gmail.com>\n    @@ Commit message\n         Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n\n      ## builtin/submodule--helper.c ##\n    +@@\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     @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char **argv, const char *prefix)\n        return !!ret;\n      }\n    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *\n     +  free(url);\n     +}\n     +\n    ++static int check_sm_exists(unsigned int force, const char *path) {\n    ++\n    ++  int cache_pos, dir_in_cache = 0;\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 && (directory_exists_in_index(&the_index,\n    ++     path, strlen(path)) == index_directory))\n    ++          dir_in_cache = 1;\n    ++\n    ++  if (!force) {\n    ++          if (cache_pos >= 0 || dir_in_cache)\n    ++                  die(_(\"'%s' already exists in the index\"), path);\n    ++  } else {\n    ++          struct cache_entry *ce = NULL;\n    ++          if (cache_pos >= 0)\n    ++                  ce = the_index.cache[cache_pos];\n    ++          if (dir_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    ++  return 0;\n    ++}\n    ++\n     +static void modify_remote_v(struct strbuf *sb)\n     +{\n     +  int i;\n    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *\n     +  if (is_dir_sep(path[strlen(path) -1]))\n     +          path[strlen(path) - 1] = '\\0';\n     +\n    -+  if (!force) {\n    -+          if (is_directory(path) && submodule_from_path(the_repository, &null_oid, path))\n    -+                  die(_(\"'%s' already exists in the index\"), path);\n    -+  } else {\n    -+          int err;\n    -+          if (index_name_pos(&the_index, path, strlen(path)) >= 0 &&\n    -+              !is_submodule_populated_gently(path, &err))\n    -+                  die(_(\"'%s' already exists in the index and is not a \"\n    -+                        \"submodule\"), path);\n    -+  }\n    ++  if (check_sm_exists(force, path))\n    ++          return 1;\n     +\n     +  strbuf_addstr(&sb, path);\n    -+  if (is_directory(path)) {\n    ++  if (is_nonbare_repository_dir(&sb)) {\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    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *\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    ++          if (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0)) {\n    ++                  fprintf(stderr, _(\"%s\"), sb.buf);\n    ++                  return 1;\n    ++          }\n     +          strbuf_release(&sb);\n     +  }\n     +\n    @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule update aborts on miss\n        EOF\n        git init repo-no-commits &&\n        test_must_fail git submodule add ../a ./repo-no-commits 2>actual &&\n    -@@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add to .gitignored path fails' '\n    -   (\n    -           cd addtest-ignore &&\n    -           cat <<-\\EOF >expect &&\n    --          The following paths are ignored by one of your .gitignore files:\n    -+          fatal: The following paths are ignored by one of your .gitignore files:\n    -           submod\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    -           EOF\n    -           # Does not use test_commit due to the ignore\n    -           echo \"*\" > .gitignore &&\n-:  ---------- > 3:  98b05eb46d t7400: add test to check 'submodule add' for tracked paths\n-----\n\nPrathamesh Chavan (1):\n  submodule: port submodule subcommand 'add' from shell to C\n\nShourya Shukla (2):\n  dir: change the scope of function 'directory_exists_in_index()'\n  t7400: add test to check 'submodule add' for tracked paths\n\n builtin/submodule--helper.c | 391 +++++++++++++++++++++++++++++++++++-\n dir.c                       |  10 +-\n dir.h                       |   9 +\n git-submodule.sh            | 161 +--------------\n t/t7400-submodule-basic.sh  |  13 +-\n 5 files changed, 414 insertions(+), 170 deletions(-)\n\n-- \n2.28.0\n\n"},{"id":"407030","messageId":"20201007074538.25891-2-shouryashukla.oo@gmail.com","threadId":"54360","inReplyTo":"20201007074538.25891-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-07T07:45:36Z","receivedAt":"2020-10-07T07:45:56Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"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'.\n\nHelped-by: Christian Couder <christian.couder@gmail.com>\nHelped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n dir.c | 10 ++--------\n dir.h |  9 +++++++++\n 2 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 78387110e6..e67cf52fec 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@@ -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 \ndiff --git a/dir.h b/dir.h\nindex a3c40dec51..e46f240528 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+\tindex_nonexistent = 0,\n+\tindex_directory,\n+\tindex_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.28.0\n\n"},{"id":"407031","messageId":"20201007074538.25891-3-shouryashukla.oo@gmail.com","threadId":"54360","inReplyTo":"20201007074538.25891-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-07T07:45:37Z","receivedAt":"2020-10-07T07:46:01Z","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, 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 <shouryashukla.oo@gmail.com>\n---\n builtin/submodule--helper.c | 391 +++++++++++++++++++++++++++++++++++-\n git-submodule.sh            | 161 +--------------\n t/t7400-submodule-basic.sh  |   2 +-\n 3 files changed, 392 insertions(+), 162 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex de5ad73bb8..ec0a50d032 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@@ -2744,6 +2743,395 @@ 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 int check_sm_exists(unsigned int force, const char *path) {\n+\n+\tint cache_pos, dir_in_cache = 0;\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 && (directory_exists_in_index(&the_index,\n+\t   path, strlen(path)) == index_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 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 (check_sm_exists(force, path))\n+\t\treturn 1;\n+\n+\tstrbuf_addstr(&sb, path);\n+\tif (is_nonbare_repository_dir(&sb)) {\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\tfprintf(stderr, _(\"%s\"), sb.buf);\n+\t\t\treturn 1;\n+\t\t}\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@@ -2777,6 +3165,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 7ce52872b7..f1cbe4934a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -146,166 +146,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.28.0\n\n"},{"id":"407032","messageId":"20201007074538.25891-4-shouryashukla.oo@gmail.com","threadId":"54360","inReplyTo":"20201007074538.25891-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 3/3] t7400: add test to check 'submodule add' for tracked paths","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-07T07:45:38Z","receivedAt":"2020-10-07T07:46:04Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"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 <shouryashukla.oo@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.28.0\n\n"},{"id":"407079","messageId":"xmqq8sch3o8v.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"20201007074538.25891-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T18:05:52Z","receivedAt":"2020-10-07T18:06:04Z","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> diff --git a/dir.h b/dir.h\n> index a3c40dec51..e46f240528 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> +\tindex_nonexistent = 0,\n> +\tindex_directory,\n> +\tindex_gitdir\n> +};\n\nThese were adequate as private names used within the wall of dir.c,\nbut I doubt that they are named specific enough to stand out as\npublic symbols.\n\nUnlike say \"index_state\" (the name of a struct type), whose nature\nis quite global to any code that wants to access the in-core index,\n\"index_directory\" is *NOT* such a name.  It is only of interest to\nthose who want to see \"I have a directory name---does it appear as a\ndirectory in the index?\".  It is not even interesting to those who\nwant to ask similar and related questions like \"I have this\npathname---does it appear as anything in the index, and if so what\ntype of entry is it?\".  A worse part of this is that even if such a\nhelper function file_exists_in_index() were to be written, the\n\"exist_status\" enum won't be usable to return the answer that\nquestion, but yet the enum squats on a perfectly good name to\nexpress \"status\" for the whole class that it does not represent.\n\nSo, NAK.  We need to come up with a better name for these symbols if\nwe were to expose them to the outside world.  The only good name\nthis patch makes public is \"directory_exists_in_index()\", which is\nspecific enough.\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"},{"id":"407085","messageId":"xmqqtuv52877.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"20201007074538.25891-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T18:37:48Z","receivedAt":"2020-10-07T18:37:54Z","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> From: Prathamesh Chavan <pc44800@gmail.com>\n>\n> Convert submodule subcommand 'add' to a builtin and call it via\n> 'git-submodule.sh'.\n>\n> Also, since the command die()s out in case of absence of commits in the\n> submodule, the keyword 'fatal' is prefixed in the error messages.\n> Therefore, prepend the keyword in the expected output of test t7400.6.\n>\n> While at it, eliminate the extra preprocessor directive\n> `#include \"dir.h\"` at the start of 'submodule--helper.c'.\n>\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Stefan Beller <stefanbeller@gmail.com>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 391 +++++++++++++++++++++++++++++++++++-\n>  git-submodule.sh            | 161 +--------------\n>  t/t7400-submodule-basic.sh  |   2 +-\n>  3 files changed, 392 insertions(+), 162 deletions(-)\n\nWhoa.  That looks like a huge change.  Makes me wonder if we want\nthis split into multiple pieces, but let's read on.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index de5ad73bb8..ec0a50d032 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> @@ -2744,6 +2743,395 @@ 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\nThis is used in a context like this:\n\n\tstruct add_data data = ADD_DATA_INIT;\n\nIt is a tangent, but wouldn't\n\n\t#define ADD_DATA_INIT { 0 }\n\nbe a more appropriate way to express that there is nothing other\nthan the initialization to zero values going on?\n\n> +/*\n> + * Guess dir name from repository: strip leading '.*[/:]',\n> + * strip trailing '[:/]*.git'.\n> + */\n\nThe original also strips trailing '/'.  The original does these in\norder:\n\n - if $repo ends with '/', remove that.  The above description does\n   not mention it.\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   The above description sounds as if it will remove \":/:/:.git\"\n   from the end (and the code seems to have the same bug, as\n   after_slash_or_colon won't allow the code to know if the previous\n   character before \".git\" was slash or colon).\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 *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\nSo, this looks quite bogus and unnatural.  Checking for \".git\" at\nevery position in the string is meaningless.\n\nI wonder if something along the following (beware: not even compile\ntested or checked for off-by-ones) would be easier to follow and\nmore faithful conversion to the original.\n\n\tep = repo + strlen(repo);\n\n        /*\n         * eat trailing slashes - a conversion less faithful to\n         * the original may want to loop to cull duplicated trailing\n\t * slashes, but we can leave it as user-error for now.\n\t */\n\tif (repo < ep - 1 && ep[-1] == '/')\n\t\tep--;\n\n\t/* eat \":*/*\\.git\" at the tail */\n\tif (repo < ep - 4 && !memcmp(\".git\", ep - 4, 4)) {\n\t\tep -= 4;\n\t\twhile (repo < ep - 1 && ep[-1] == '/')\n\t\t\tep--;\n\t\twhile (repo < ep - 1 && ep[-1] == ':')\n\t\t\tep--;\n\t}\n\n\t/* find the last ':' or '/' */\n\tfor (sp = ep - 1; repo <= sp; sp--) {\n\t\tif (*sp == '/' || *sp == ':')\n\t\t\tbreak;\n\t}\n        sp++; /* exclude '/' or ':' itself */\n\n        /* sp point at the beginning, and ep points at the end */\n\treturn xmemdupz(sp, ep - sp);\n\n> +}\n\nThat's it for now; I didn't look at the remainder of this patch\nduring this sitting before I have to move on, but I may revisit the\nrest at some other time.\n\nThanks.\n"},{"id":"407109","messageId":"xmqqo8ldznjx.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"20201007074538.25891-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-07T22:19:46Z","receivedAt":"2020-10-07T22:19:56Z","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> +static void fprintf_submodule_remote(const char *str)\n\nThe fact that the helper happens to use fprintf() to do its job is\nmuch less important than it writes to the standard error stream.\nName it after what it does than how it does so.  Is there a word\nthat explains at a higher-level concept than \"print to stderr\" that\nthis function tries to achieve?  \n\nSame question for the name of the only caller of this function,\nmodify_remote_v().  That name does not mean anything to readers\nother than that it futz with output from \"remote -v\" command, which\nis the least interesting piece of information.  What does it try to\nachieve by using \"remote -v\"?  Can we name the function after that?\n\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> +\twhile(*p == ' ')\n> +\t\tp++;\n\nPerhaps make a small helper out of these seven lines, so that the\ncaller can say something like\n\n    p = str;\n    name = parse_token(&p);\n    url = parse_token(&p);\n\nThis one you should be able to do without any extra allocation,\nthough.  Just write a parse_token() that finds start and length,\nprepare \"char *name; int namelen\" and the same pair for URL,\nand then\n\n\tfprintf(stderr, \"  %.*s\\t%.*s\\n\",\n\t\tnamelen, name, urllen, url);\n\n> +\tfprintf(stderr, \"  %s\\t%s\\n\", name, url);\n> +\tfree(name);\n> +\tfree(url);\n> +}\n\n> +static int check_sm_exists(unsigned int force, const char *path) {\n> +\n> +\tint cache_pos, dir_in_cache = 0;\n\nHave a blank line here to separate decl and the first statement.\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 && (directory_exists_in_index(&the_index,\n> +\t   path, strlen(path)) == index_directory))\n> +\t\tdir_in_cache = 1;\n\nFunny line wrapping.  Try to cut long line at an operator as close\nto the root of the parse tree (in this case, &&) as possible, i.e.\n\n\tif (cache_pos < 0 &&\n\t    directory_exists_in_index(&the_index, path, strlen(path)) == index_directory)\n\nIt is OK to further wrap after == if the second line bothers you.\n\nA bigger question.  Can the path be a regular file but at a higher\nstage because we are in the middle of a conflicted merge?  We'd get\ncache_pos that is negative in that case, too, and we definitely would\nwant to say the path already exists in the index in such a case, but ...\n\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\n... the current code may not trigger this die() in such a case, no?\n\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\nLikewise here.  cache_pos < 0 does not automatically mean it does\nnot exist.  It tells you that it does not exist as a merged entry.\n\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static void modify_remote_v(struct strbuf *sb)\n\nThis roughly corresponds to this part of the original\n\n    grep '(fetch)' | sed -e s,^,\"  \", -e s,' (fetch)',,\n\nI actualy would suggest moving the \"git remote -v\" invocation and\ncapturing of its output to this helper function and name it after\nwhat it does, which seems to be \"show fetch remotes\" to me.\n\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\nThe original makes sure the 'fetch' appears inside \"()\" but this\ndoes not.  Any reason why we want to do it differently?\n\n> +\t\t\tfprintf_submodule_remote(xstrndup(start, end - start - 7));\n\nThe result of xstrndup() is leaking here.\n\nIn any case, with a helper function like parse_token() suggested\nbefore, you can get rid of the fprintf_submodule_remote() helper and\nopen code it here, without any temporary allocation and freeing.\nYou'd have the start of each line of \"git remote -v\" output (so you\nknow where it starts and it ends), and a parser that roughly does\nthis:\n\n\t/*\n\t * cp points at the current location, and ep points the\n\t * end of the buffer.  find the tail of the current string\n\t * and store its length in *len, skip over whitespaces and\n\t * return the location to be used as the new cp.\n\t */\n\tconst char *parse_token(char *cp, char *ep, int *len);\n\nand make the latter half of this function (former half would be\nspawning \"remote -v\" and capturing its output in sb) a loop whose\nbody may look like\n\n\t{\n\t\tchar *end, *name, *url, *tail;\n\t\tint namelen, urllen;\n\n\t\tend = strchrnul(start, '\\n');\n\t\tname = start;\n\t\turl = parse_token(name, end, &namelen);\n\t\ttail = parse_token(url, end, &urllen);\n\t\tif (!memcmp(tail, \"(fetch)\", 7))\n\t\t\tfprintf(stderr, ...); /* see above */\n\t\tstart = *end ? end + 1 : end;\n\t}\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\nAs I said, it would be a better organization to have a helper\nfunction that does what is done from here ...\n\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\n... up to here.  Can you say what the purpose of that helper\nfunction?  I'd say it is for a given git repository (specified by\nsubmodule_git_dir, which we will pass as the parameter to that\nhelper), report the names and URLs of fetch remotes defined in that\nrepository.  So, perhaps its signature might be:\n\n\tstatic void report_fetch_remotes(FILE *output, const char *git_dir);\n\nwhere we would make a call to it from here like so:\n\n\t\t\t\treport_fetch_remotes(stderr, submodule_git_dir);\n\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\nI think I've reviewed up to this point this time around.\n\nThanks.\n\n"},{"id":"407168","messageId":"xmqqimbky6st.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"20201007074538.25891-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-08T17:19:14Z","receivedAt":"2020-10-08T17:19:19Z","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> +static void config_added_submodule(struct add_data *info)\n> +{\n\nThis one I may take a look at later, but won't review in this\nmessage.\n\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\nChecking \"*prefix\" lets us avoid an unnecessary allocation, i.e.\n\n\tif (prefix && *prefix &&\n\t    reference_path && !is_absolute_path(reference_path))\n\n> +\t\treference_path = xstrfmt(\"%s%s\", prefix, reference_path);\n> +\n> +\tif (argc == 0 || argc > 2) {\n\nNice that you are checking excess args, which the original didn't do.\n\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\nWe've reviewed the function already.  Good.\n\n> +\t} else {\n> +\t\trepo = argv[0];\n> +\t\tpath = xstrdup(argv[1]);\n\nOK.  So after this if/else if/else cascade, path is an allocated\npiece of memory we could later free() whichever branch is taken.\n\n> +\t}\n> +\n> +\tif (!is_absolute_path(path) && prefix)\n> +\t\tpath = xstrfmt(\"%s%s\", prefix, path);\n\nThis also makes path freeable, but the original path is leaked.\n\n\tif (prefix && *prefix && !is_absolute_path(path)) {\n\t\tfree(path);\n\t\tpath = xstrfmt(...);\n\t}\n\nIs there a reason (does not have to be a strong reason) why we use\n'path', not 'sm_path', as the variable name that corresponds to\n$sm_path in the original, by the way?\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\nThis is 'git submodule--helper resolve-relative-url \"$repo\"' in the\noriginal.\n\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\nAnd this is copied-and-pasted from resolve_relative_url() function\nfound in builtins/submodule--helper.c.\n\nrelative_url() returns an allocated memory so we can free() realrepo\nif we took this branch.\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\nThis repo came from argv[0] so we cannot free realrepo if we took\nthis branch.  Are we willing to leak realrepo we obtained from the\nother branch?\n\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\nIt's nice that a handy (almost) equivalent helper is already\navailable ;-)\n\n> +\t/* strip trailing '/' */\n> +\tif (is_dir_sep(path[strlen(path) -1]))\n> +\t\tpath[strlen(path) - 1] = '\\0';\n\nThe original dealt with multiple trailing '/' but this one does not.\nShouldn't it loop starting at the end?\n\n> +\tif (check_sm_exists(force, path))\n> +\t\treturn 1;\n\nOK.  I think we reviewed the function.  Seeing it in the context of\nthe calling site makes us realize that it has a wrong name.  \"check\nsubmodule exists\" sounds as if we expect a submodule to exist at the\npath, and it is an error for a submodule not to be there, but that\nis not what this caller (which is the only caller of the helper)\nwants to check.  And more importantly, the helper reacts to anything\nsitting at the path, not just submoudle.\n\nSo what does the helper really do?  I think it checks if it is OK to\ncreate a submodule there.  IOW, \"exists\" part of the name is what\nmakes it a misnomer.  Perhaps \"can_create_submodule()\"?\n\n> +\tstrbuf_addstr(&sb, path);\n> +\tif (is_nonbare_repository_dir(&sb)) {\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\tfprintf(stderr, _(\"%s\"), sb.buf);\n\nSorry, but I cannot guess what this _(\"%s\") is trying to achieve.\nShouldn't it be\n\t\t\tstrbuf_complete_line(&sb);\n\t\t\tfputs(sb.buf, stderr);\ninstead?\n\n> +\t\t\treturn 1;\n\nThe original honors the exit code from the dry-run and relays it to\nthe user.  Is this a regression, or nobody care what exit status\nthey get as long as it is not zero?\n\n> +\t\t}\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\nI think we've reviewed this funciton already.\n\n> +\tconfig_added_submodule(&info);\n> +\n> +\tfree(path);\n\nLooking a bit uneven wrt to leak handling.\n\n> +\treturn 0;\n> +}\n\nThanks.\n"},{"id":"407197","messageId":"xmqqd01sugrg.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"20201007074538.25891-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T05:09:55Z","receivedAt":"2020-10-09T05:10:04Z","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> +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\nHmph, you lost me.  I think this ought to correspond to this part of\nthe original:\n\n\tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n\tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n\nI can see that adding \"--\" before $sm_path may be an improvement,\nbut why do we also add .gitmodules here, and ...\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... perform quite a lot of configuration writing, before actually\nspawning the \"git add\" and make sure it succeeds?  The original\nwon't futz with any of these .gitmodules entries if \"git add\" of the\n$sm_path fails and that is a good discipline to follow.\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\nWhat if this were a valueless true (\"[submodule] active\\n\" without\n\"= true\")?  Wouldn't get_string() fail?\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> + ...\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\nWhew.\n\nThis was way too big to be reviewed in a single sitting.  I do not\nknow offhand if there is a better way to structure the changes into\na more digestible pieces to help prevent reviewers from overlooking\npotential mistakes, though.\n\nThanks.\n\n"},{"id":"407320","messageId":"20201012101140.GA12637@konoha","threadId":"54360","inReplyTo":"xmqq8sch3o8v.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-12T10:11:40Z","receivedAt":"2020-10-12T10:11:50Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 07/10 11:05, Junio C Hamano wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \n> > diff --git a/dir.h b/dir.h\n> > index a3c40dec51..e46f240528 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> > +\tindex_nonexistent = 0,\n> > +\tindex_directory,\n> > +\tindex_gitdir\n> > +};\n> \n> These were adequate as private names used within the wall of dir.c,\n> but I doubt that they are named specific enough to stand out as\n> public symbols.\n> \n> Unlike say \"index_state\" (the name of a struct type), whose nature\n> is quite global to any code that wants to access the in-core index,\n> \"index_directory\" is *NOT* such a name.  It is only of interest to\n> those who want to see \"I have a directory name---does it appear as a\n> directory in the index?\".  It is not even interesting to those who\n> want to ask similar and related questions like \"I have this\n> pathname---does it appear as anything in the index, and if so what\n> type of entry is it?\".  A worse part of this is that even if such a\n> helper function file_exists_in_index() were to be written, the\n> \"exist_status\" enum won't be usable to return the answer that\n> question, but yet the enum squats on a perfectly good name to\n> express \"status\" for the whole class that it does not represent.\n> \n> So, NAK.  We need to come up with a better name for these symbols if\n> we were to expose them to the outside world.  The only good name\n> this patch makes public is \"directory_exists_in_index()\", which is\n> specific enough.\n\nUnderstandable. So, how would it be if the function I wrote in\n'submodule--helper.c' could instead be written here (in dir.c) and\nwould help to check if the path is there in the index or not. It could be anything\nranging from a filename to a SM. Since the return type will be an\ninteger, this PATCH 1/3 can be eliminated and we won't need to change\nthe scope of the 'directory_exists_in_index()' function and the enum can\nstay visible only in dir.c\n\nWhat are your thoughts on this?\n\n"},{"id":"410280","messageId":"20201118231331.716110-1-jonathantanmy@google.com","threadId":"54360","inReplyTo":"xmqqd01sugrg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-11-18T23:13:31Z","receivedAt":"2020-11-18T23:13:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Whew.\n> \n> This was way too big to be reviewed in a single sitting.  I do not\n> know offhand if there is a better way to structure the changes into\n> a more digestible pieces to help prevent reviewers from overlooking\n> potential mistakes, though.\n> \n> Thanks.\n\nI just took a look at this, and one thing that would have helped is if\nyou ported the end of the function first in a commit, and work your way\nbackwards (in one or more commits).\n\nAfter reading through the whole thing, I saw that this is mostly a\nstraightforward start-to-finish port (besides factoring out code into\nfunctions), but it would be much easier for reviewers to conceptualize\nand discuss the different parts if they were already divided.\n"},{"id":"410282","messageId":"20201118232557.GA3698950@google.com","threadId":"54360","inReplyTo":"20201007074538.25891-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-11-18T23:25:57Z","receivedAt":"2020-11-18T23:26:06Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Hi,\n\nOn Wed, Oct 07, 2020 at 01:15:36PM +0530, 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'.\n\nI don't have comments about the diff itself beyond what Junio mentioned\n- it's very simple. But I do think this commit message needs a rewrite.\n\nYour commit message summarizes the diff - which isn't useful, because\nthe diff itself is very simple. But what it fails to do is what I'm a\nlot more interested in, reading this change: *why* do you want to make\nthis function and enum reusable? I think you mention it in the cover\nletter, but it's not explained at all here.\n\nExplaining the motivation in the cover letter also would help us\nunderstand whether it is better to make the enum public, like your diff\nproposes, or to wrap or change the function and avoid exposing the enum,\nlike you suggested in reply to Junio's comment.\n\nLastly, saying something like \"This change is needed so that git commit\ncan sort ducks by feather length\" helps avoid\nhttps://en.wikipedia.org/wiki/XY_problem - that is, maybe we already\nhave another tool which is more appropriate, and which you missed; and\nknowing your motivation, someone can point you in that direction\ninstead.\n\nThe same comment holds true for your patch 3, as well.\n\nThanks for your effort on this series.\n\n - Emily\n"},{"id":"410316","messageId":"20201119000333.GI36751@google.com","threadId":"54360","inReplyTo":"20201007074538.25891-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 0/3] submodule: port subcommand add from shell to C","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-11-19T00:03:33Z","receivedAt":"2020-11-19T00:03:43Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Hi Shourya,\n\nThank you for this series! Please see the comments below:\n\n\nOn 2020.10.07 13:15, Shourya Shukla wrote:\n> Hello all,\n> \n> This is the v2 of the patch with the same title, delivered more than a\n> month ago as a part of my GSoC. Link to v1:\n> https://lore.kernel.org/git/20200824090359.403944-1-shouryashukla.oo@gmail.com/\n\nSince GSoC has ended for the year, I wanted to point out the\ngit-mentoring@googlegroups.com list, where you can find additional\nmentors if you like.\n\n> The changelog is as follows:\n> \n>     1. Introduce PATCH[1/3](dir: change the scope of function\n>        'directory_exists_in_index()', 2020-10-06). This was done since\n>        the above mentioned function will be used in the patch that\n>        follows.\n> \n>     2. There are multiple changes in this commit:\n> \n>             A. Improve the part which checks if the 'path' given as\n>                argument exists or not. Implementing Kaartic's\n>                suggestions on the patch, I had to make sure that the\n>                case for checking if the path has tracked contents or\n>                not also works.\n> \n>             B. Also, wrap the aforementioned segment in a function\n>                since it became very long. The function is called\n>                'check_sm_exists()'.\n> \n>             C. Also, use the function 'is_nonbare_repository_dir()'\n>                instead of 'is_directory()' when trying to resolve\n>                gitlink.\n> \n>             D. Append keyword 'fatal' in front of the expected output of\n>                test t7400.6 since the command die()s out in case of\n>                absence of commits in a submodule.\n> \n>             E. Remove the extra `#include \"dir.h\"` from\n>                'submodule--helper.c'.\n> \n>     3. Introduce PATCH[3/3] (t7400: add test to check 'submodule add'\n>        for tracked paths, 2020-10-07). Kaartic pointed out that a test\n>        for path with tracked contents did not exist and hence it was\n>        necessary to write one. Therefore, this commit introduces a new\n>        test 't7400.18: submodule add to path with tracked contents\n>        fails'.\n\nGenerally, we want to avoid describing in detail what the code does;\nhopefully, the code can speak for itself. It may be a better use of the\ncover letter to describe the motivation for the series as a whole.\nReviewers will not necessarily have background on what you want to\naccomplish. We came up with a few factors that might have inspired this\nchange, but we're not sure which you intended to address:\n\n* Increase efficiency by reducing the number of processes forked and the\n  use of the shell.\n\n* Make the submodule code easier to maintain (since the project probably\n  has more C experts than shell experts).\n\n* Improve the user experience with submodules by giving the\n  submodule-add code access to C internals, and vice versa.\n\nKnowing what you want to accomplish can make it easier for reviewers. Of\ncourse, you'll also want to include important context in your commit\nmessages as well, so that it's available in the history if future\ndebugging is necessary.\n\n\nThanks again for the series, and please feel free to follow up if you\nhave any questions\n-- Josh\n"},{"id":"410336","messageId":"871rgprdt1.fsf@evledraar.gmail.com","threadId":"54360","inReplyTo":"20201118231331.716110-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-19T07:44:26Z","receivedAt":"2020-11-19T07:44:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 19 2020, Jonathan Tan wrote:\n\n>> Whew.\n>> \n>> This was way too big to be reviewed in a single sitting.  I do not\n>> know offhand if there is a better way to structure the changes into\n>> a more digestible pieces to help prevent reviewers from overlooking\n>> potential mistakes, though.\n>> \n>> Thanks.\n>\n> I just took a look at this, and one thing that would have helped is if\n> you ported the end of the function first in a commit, and work your way\n> backwards (in one or more commits).\n>\n> After reading through the whole thing, I saw that this is mostly a\n> straightforward start-to-finish port (besides factoring out code into\n> functions), but it would be much easier for reviewers to conceptualize\n> and discuss the different parts if they were already divided.\n\nHaving done some minor changes to git-submodule.sh recently, I wondered\nif we weren't at the point where it would be a nice approach to invert\nthe C/sh helper relationship.\n\nI.e. write git-submodule.c, which would be the small entry point, it\nwould then mostly dispatch to a submodule--helper, which would in turn\nmostly dispatch to a new submodule--helper-sh (containing most of the\ncurrent git-submodule.sh code), which in turn would re-dispatch to the C\nsubmodule--helper (which as an aside, then sometimes calls itself via\nprocess invocation).\n\nIt's quite a bit of spaghetti code, but means that there's a straighter\npath to porting some of the setup code such as the \"--check-writeable\",\nis_absolute_path() etc. being changed at the start of the change here to\ngit-submodule.sh.\n"},{"id":"410351","messageId":"nycvar.QRO.7.76.6.2011191327320.56@tvgsbejvaqbjf.bet","threadId":"54360","inReplyTo":"871rgprdt1.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-19T12:38:16Z","receivedAt":"2020-11-19T12:38:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Thu, 19 Nov 2020, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Thu, Nov 19 2020, Jonathan Tan wrote:\n>\n> >> Whew.\n> >>\n> >> This was way too big to be reviewed in a single sitting.  I do not\n> >> know offhand if there is a better way to structure the changes into\n> >> a more digestible pieces to help prevent reviewers from overlooking\n> >> potential mistakes, though.\n> >>\n> >> Thanks.\n> >\n> > I just took a look at this, and one thing that would have helped is if\n> > you ported the end of the function first in a commit, and work your way\n> > backwards (in one or more commits).\n> >\n> > After reading through the whole thing, I saw that this is mostly a\n> > straightforward start-to-finish port (besides factoring out code into\n> > functions), but it would be much easier for reviewers to conceptualize\n> > and discuss the different parts if they were already divided.\n>\n> Having done some minor changes to git-submodule.sh recently, I wondered\n> if we weren't at the point where it would be a nice approach to invert\n> the C/sh helper relationship.\n>\n> I.e. write git-submodule.c, which would be the small entry point, it\n> would then mostly dispatch to a submodule--helper, which would in turn\n> mostly dispatch to a new submodule--helper-sh (containing most of the\n> current git-submodule.sh code), which in turn would re-dispatch to the C\n> submodule--helper (which as an aside, then sometimes calls itself via\n> process invocation).\n>\n> It's quite a bit of spaghetti code, but means that there's a straighter\n> path to porting some of the setup code such as the \"--check-writeable\",\n> is_absolute_path() etc. being changed at the start of the change here to\n> git-submodule.sh.\n\nLooking at\nhttps://github.com/gitgitgadget/git/blob/ss/submodule-add-in-c/git-submodule.sh,\nI see that while there are still 794 lines, most of it is just mostly\nredundant option parsing. The only function left to convert is\n`cmd_update()`, and the first half was already converted to C long ago,\nvia the `git submodule--helper update-clone` subcommand:\nhttps://github.com/gitgitgadget/git/blob/ss/submodule-add-in-c/git-submodule.sh#L269-L530\n\nAt this stage I suspect that having a little more patience would make more\nsense. After `cmd_update` is converted, we can simply finish the\nconversion, without having to keep the shell script around.\n\nCiao,\nDscho\n"},{"id":"410378","messageId":"xmqq8saxxeuq.fsf@gitster.c.googlers.com","threadId":"54360","inReplyTo":"nycvar.QRO.7.76.6.2011191327320.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-19T20:37:33Z","receivedAt":"2020-11-19T20:37:41Z","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> At this stage I suspect that having a little more patience would make more\n> sense. After `cmd_update` is converted, we can simply finish the\n> conversion, without having to keep the shell script around.\n\nThat matches how I view the current state.  We seem to be reviewer\nbandwidth limited and folks who took a look at this series to help\nmoving it forward certainly deserve a lot of gratitude.\n\nThanks.\n"}]}