| Message ID | 20201007074538.25891-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | submodule: port subcommand add from shell to C | expand |
Hi Shourya, Thank you for this series! Please see the comments below: On 2020.10.07 13:15, Shourya Shukla wrote: > Hello all, > > This is the v2 of the patch with the same title, delivered more than a > month ago as a part of my GSoC. Link to v1: > https://lore.kernel.org/git/20200824090359.403944-1-shouryashukla.oo@gmail.com/ Since GSoC has ended for the year, I wanted to point out the git-mentoring@googlegroups.com list, where you can find additional mentors if you like. > The changelog is as follows: > > 1. Introduce PATCH[1/3](dir: change the scope of function > 'directory_exists_in_index()', 2020-10-06). This was done since > the above mentioned function will be used in the patch that > follows. > > 2. There are multiple changes in this commit: > > A. Improve the part which checks if the 'path' given as > argument exists or not. Implementing Kaartic's > suggestions on the patch, I had to make sure that the > case for checking if the path has tracked contents or > not also works. > > B. Also, wrap the aforementioned segment in a function > since it became very long. The function is called > 'check_sm_exists()'. > > C. Also, use the function 'is_nonbare_repository_dir()' > instead of 'is_directory()' when trying to resolve > gitlink. > > D. Append keyword 'fatal' in front of the expected output of > test t7400.6 since the command die()s out in case of > absence of commits in a submodule. > > E. Remove the extra `#include "dir.h"` from > 'submodule--helper.c'. > > 3. Introduce PATCH[3/3] (t7400: add test to check 'submodule add' > for tracked paths, 2020-10-07). Kaartic pointed out that a test > for path with tracked contents did not exist and hence it was > necessary to write one. Therefore, this commit introduces a new > test 't7400.18: submodule add to path with tracked contents > fails'. Generally, we want to avoid describing in detail what the code does; hopefully, the code can speak for itself. It may be a better use of the cover letter to describe the motivation for the series as a whole. Reviewers will not necessarily have background on what you want to accomplish. We came up with a few factors that might have inspired this change, but we're not sure which you intended to address: * Increase efficiency by reducing the number of processes forked and the use of the shell. * Make the submodule code easier to maintain (since the project probably has more C experts than shell experts). * Improve the user experience with submodules by giving the submodule-add code access to C internals, and vice versa. Knowing what you want to accomplish can make it easier for reviewers. Of course, you'll also want to include important context in your commit messages as well, so that it's available in the history if future debugging is necessary. Thanks again for the series, and please feel free to follow up if you have any questions -- Josh
Hello all, This is the v2 of the patch with the same title, delivered more than a month ago as a part of my GSoC. Link to v1: https://lore.kernel.org/git/20200824090359.403944-1-shouryashukla.oo@gmail.com/ The changelog is as follows: 1. Introduce PATCH[1/3](dir: change the scope of function 'directory_exists_in_index()', 2020-10-06). This was done since the above mentioned function will be used in the patch that follows. 2. There are multiple changes in this commit: A. Improve the part which checks if the 'path' given as argument exists or not. Implementing Kaartic's suggestions on the patch, I had to make sure that the case for checking if the path has tracked contents or not also works. B. Also, wrap the aforementioned segment in a function since it became very long. The function is called 'check_sm_exists()'. C. Also, use the function 'is_nonbare_repository_dir()' instead of 'is_directory()' when trying to resolve gitlink. D. Append keyword 'fatal' in front of the expected output of test t7400.6 since the command die()s out in case of absence of commits in a submodule. E. Remove the extra `#include "dir.h"` from 'submodule--helper.c'. 3. Introduce PATCH[3/3] (t7400: add test to check 'submodule add' for tracked paths, 2020-10-07). Kaartic pointed out that a test for path with tracked contents did not exist and hence it was necessary to write one. Therefore, this commit introduces a new test 't7400.18: submodule add to path with tracked contents fails'. Comments and feedback are appreciated. Sorry for the month long delay, I was on a vacation. I am attaching a range-diff between v1 and v2 at the end of this mail. Regards, Shourya Shukla ----- -: ---------- > 1: bdac00494e dir: change the scope of function 'directory_exists_in_index()' 1: b08d81e179 ! 2: 3e20d0fe04 submodule: port submodule subcommand 'add' from shell to C @@ Commit message 'git-submodule.sh'. Also, since the command die()s out in case of absence of commits in the - submodule and exits with exit status 1 when we try adding a submodule - which is mentioned in .gitignore, the keyword 'fatal' is prefixed in the - error messages. Therefore, prepend the keyword in the expected outputs - of tests t7400.6 and t7400.16. + submodule, the keyword 'fatal' is prefixed in the error messages. + Therefore, prepend the keyword in the expected output of test t7400.6. + + While at it, eliminate the extra preprocessor directive + `#include "dir.h"` at the start of 'submodule--helper.c'. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Stefan Beller <stefanbeller@gmail.com> @@ Commit message Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> ## builtin/submodule--helper.c ## +@@ + #include "diffcore.h" + #include "diff.h" + #include "object-store.h" +-#include "dir.h" + #include "advice.h" + + #define OPT_QUIET (1 << 0) @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char **argv, const char *prefix) return !!ret; } @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char * + free(url); +} + ++static int check_sm_exists(unsigned int force, const char *path) { ++ ++ int cache_pos, dir_in_cache = 0; ++ if (read_cache() < 0) ++ die(_("index file corrupt")); ++ ++ cache_pos = cache_name_pos(path, strlen(path)); ++ if(cache_pos < 0 && (directory_exists_in_index(&the_index, ++ path, strlen(path)) == index_directory)) ++ dir_in_cache = 1; ++ ++ if (!force) { ++ if (cache_pos >= 0 || dir_in_cache) ++ die(_("'%s' already exists in the index"), path); ++ } else { ++ struct cache_entry *ce = NULL; ++ if (cache_pos >= 0) ++ ce = the_index.cache[cache_pos]; ++ if (dir_in_cache || (ce && !S_ISGITLINK(ce->ce_mode))) ++ die(_("'%s' already exists in the index and is not a " ++ "submodule"), path); ++ } ++ return 0; ++} ++ +static void modify_remote_v(struct strbuf *sb) +{ + int i; @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char * + if (is_dir_sep(path[strlen(path) -1])) + path[strlen(path) - 1] = '\0'; + -+ if (!force) { -+ if (is_directory(path) && submodule_from_path(the_repository, &null_oid, path)) -+ die(_("'%s' already exists in the index"), path); -+ } else { -+ int err; -+ if (index_name_pos(&the_index, path, strlen(path)) >= 0 && -+ !is_submodule_populated_gently(path, &err)) -+ die(_("'%s' already exists in the index and is not a " -+ "submodule"), path); -+ } ++ if (check_sm_exists(force, path)) ++ return 1; + + strbuf_addstr(&sb, path); -+ if (is_directory(path)) { ++ if (is_nonbare_repository_dir(&sb)) { + struct object_id oid; + if (resolve_gitlink_ref(path, "HEAD", &oid) < 0) + die(_("'%s' does not have a commit checked out"), path); @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char * + cp.no_stdout = 1; + strvec_pushl(&cp.args, "add", "--dry-run", "--ignore-missing", + "--no-warn-embedded-repo", path, NULL); -+ if (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0)) -+ die(_("%s"), sb.buf); ++ if (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0)) { ++ fprintf(stderr, _("%s"), sb.buf); ++ return 1; ++ } + strbuf_release(&sb); + } + @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule update aborts on miss EOF git init repo-no-commits && test_must_fail git submodule add ../a ./repo-no-commits 2>actual && -@@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add to .gitignored path fails' ' - ( - cd addtest-ignore && - cat <<-\EOF >expect && -- The following paths are ignored by one of your .gitignore files: -+ fatal: The following paths are ignored by one of your .gitignore files: - submod - hint: Use -f if you really want to add them. - hint: Turn this message off by running - hint: "git config advice.addIgnoredFile false" -+ - EOF - # Does not use test_commit due to the ignore - echo "*" > .gitignore && -: ---------- > 3: 98b05eb46d t7400: add test to check 'submodule add' for tracked paths ----- Prathamesh Chavan (1): submodule: port submodule subcommand 'add' from shell to C Shourya Shukla (2): dir: change the scope of function 'directory_exists_in_index()' t7400: add test to check 'submodule add' for tracked paths builtin/submodule--helper.c | 391 +++++++++++++++++++++++++++++++++++- dir.c | 10 +- dir.h | 9 + git-submodule.sh | 161 +-------------- t/t7400-submodule-basic.sh | 13 +- 5 files changed, 414 insertions(+), 170 deletions(-)