{"thread":{"id":"45614","subject":"[GSoC][PATCH v1] Disallow git commands from within unpopulated submodules","startedAt":"2017-04-06T06:03:06Z","lastAt":"2017-04-06T20:48:51Z","messageCount":4,"participants":["Prathamesh Chavan","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316255","messageId":"20170406060053.4453-1-pc44800@gmail.com","threadId":"45614","inReplyTo":null,"subject":"[GSoC][PATCH v1] Disallow git commands from within unpopulated submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-04-06T06:00:53Z","receivedAt":"2017-04-06T06:03:06Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"The main motivations for disallowing git commands within an\nunpopulated submodule are:\n\nWhenever we run \"git -C status\" within an unpopulated submodule, it\nfalls back to the superproject. This occurs since there is no .git\nfile in the submodule directory. So superproject's status gets displayed.\nAlso, the user's intention is not clear behind running the command\nin an unpopulated submodule. Hence we prefer to error out.\n\nWhen we run the command \"git -C sub add .\" within a submodule, the\nresults observed are:\n\nIn the case of the populated submodule, it acts like running “git add .“\ninside the submodule. This is uncontroversial and runs as expected.\n\nIn the case of the unpopulated submodule, the user's intention behind\nentering the above command is unclear. He may have intended to add\nthe submodule to the superproject or to add all files inside the\nsub/ directory to the submodule or superproject. Hence we’ll prefer\nto error out in these case.\n\nEventually, we use a check_prefix_inside_submodule to see check if the\npath is inside an unpopulated submodule. If it is, then we report the\nuser about the unpopulated submodule.\n\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\n\nSince this patch effectively uses RUN_SETUP, builtin commands like\n'diff' and other non-builtin commands are not filtered.\nFor such cases, I think, we need to handle them separately.\n\nAlso since currently, git-submodule is not a builtin command, the\ncommand for initializing and updating the submodule doesn't return an\nerror message, but once it is converted to builtin, we need to handle\nits case explicitly.\n\nThe build report of this patch is available on:\nhttps://travis-ci.org/pratham-pc/git/builds/219030999\n\nAlso, the above patch was initially my GSoC project topic, but I changed\nit later on and added these bug fixes to my wishlist of the proposal.\n\n builtin/submodule--helper.c      |  4 ----\n git.c                            |  3 +++\n submodule.c                      | 45 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                      |  6 ++++++\n t/t6134-pathspec-in-submodule.sh |  2 +-\n 5 files changed, 55 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 15a5430c0..4f7c7d7b8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -219,10 +219,6 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-struct module_list {\n-\tconst struct cache_entry **entries;\n-\tint alloc, nr;\n-};\n #define MODULE_LIST_INIT { NULL, 0, 0 }\n \n static int module_list_compute(int argc, const char **argv,\ndiff --git a/git.c b/git.c\nindex 33f52acbc..eefe3fb01 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"help.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--help] [-C <path>] [-c name=value]\\n\"\n@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tif (prefix)\n \t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n+\tif (prefix)\n+\t\tcheck_prefix_inside_submodule(prefix);\n \n \tif (!help && p->option & NEED_WORK_TREE)\n \t\tsetup_work_tree();\ndiff --git a/submodule.c b/submodule.c\nindex 0a2831d84..d2c3023bf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -545,6 +545,51 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+#define MODULE_LIST_INIT { NULL, 0, 0 }\n+\n+void check_prefix_inside_submodule(const char *prefix)\n+{\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint i;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\n+\t\tALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n+\t\tlist.entries[list.nr++] = ce;\n+\t\twhile (i + 1 < active_nr &&\n+\t\t\t!strcmp(ce->name, active_cache[i + 1]->name))\n+\t\t\t /*\n+\t\t\t  * Skip entries with the same name in different stages\n+\t\t\t  * to make sure an entry is returned only once.\n+\t\t\t  */\n+\t\t\ti++;\n+\t}\n+\n+\tfor(i = 0; i < list.nr; i++) {\n+\t\tif(strlen((*list.entries[i]).name) ==  strlen(prefix)) {\n+\t\t\tif (!strcmp((*list.entries[i]).name, prefix)) {\n+\t\t\t\t/* This case cannot happen because */\n+\t\t\t\tdie(\"BUG: prefixes end with '/', but we do not record ending slashes in the index\");\n+\t\t\t}\n+\t\t}\n+\t\telse if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {\n+\t\t\tconst char *out = NULL;\n+\t\t\tif(skip_prefix(prefix, (*list.entries[i]).name, &out)) {\n+\t\t\t\tif(strlen(out) == 1 && out[0] == '/')\n+\t\t\t\t\tdie(_(\"command from inside unpopulated submodule '%s' not supported.\"), (*list.entries[i]).name);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t      int flags, void *cb_data)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 05ab674f0..5e41e5afc 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,6 +31,12 @@ struct submodule_update_strategy {\n };\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n+struct module_list {\n+\tconst struct cache_entry **entries;\n+\tint alloc, nr;\n+};\n+\n+extern void check_prefix_inside_submodule(const char *prefix);\n extern int is_staging_gitmodules_ok(void);\n extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n extern int remove_path_from_gitmodules(const char *path);\ndiff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh\nindex fd401ca60..086cc4c47 100755\n--- a/t/t6134-pathspec-in-submodule.sh\n+++ b/t/t6134-pathspec-in-submodule.sh\n@@ -25,7 +25,7 @@ test_expect_success 'error message for path inside submodule' '\n '\n \n cat <<EOF >expect\n-fatal: Pathspec '.' is in submodule 'sub'\n+fatal: command from inside unpopulated submodule 'sub' not supported.\n EOF\n \n test_expect_success 'error message for path inside submodule from within submodule' '\n-- \n2.11.0\n\n"},{"id":"316256","messageId":"20170406061107.6849-1-pc44800@gmail.com","threadId":"45614","inReplyTo":"20170406060053.4453-1-pc44800@gmail.com","subject":"[GSoC][PATCH v1] Disallow git commands from within unpopulated submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-04-06T06:11:07Z","receivedAt":"2017-04-06T06:13:04Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"The main motivations for disallowing git commands within an\nunpopulated submodule are:\n\nWhenever we run \"git -C status\" within an unpopulated submodule, it\nfalls back to the superproject. This occurs since there is no .git\nfile in the submodule directory. So superproject's status gets displayed.\nAlso, the user's intention is not clear behind running the command\nin an unpopulated submodule. Hence we prefer to error out.\n\nWhen we run the command \"git -C sub add .\" within a submodule, the\nresults observed are:\n\nIn the case of the populated submodule, it acts like running “git add .“\ninside the submodule. This is uncontroversial and runs as expected.\n\nIn the case of the unpopulated submodule, the user's intention behind\nentering the above command is unclear. He may have intended to add\nthe submodule to the superproject or to add all files inside the\nsub/ directory to the submodule or superproject. Hence we’ll prefer\nto error out in these case.\n\nEventually, we use a check_prefix_inside_submodule to see check if the\npath is inside an unpopulated submodule. If it is, then we report the\nuser about the unpopulated submodule.\n\nSigned-off-by: Prathamesh Chavan <pc44800@gmail.com>\n---\n\nSince this patch effectively uses RUN_SETUP, builtin commands like\n'diff' and other non-builtin commands are not filtered.\nFor such cases, I think, we need to handle them separately.\n\nAlso since currently, git-submodule is not a builtin command, the\ncommand for initializing and updating the submodule doesn't return an\nerror message, but once it is converted to builtin, we need to handle\nits case explicitly.\n\nThe build report of this patch is available on:\nhttps://travis-ci.org/pratham-pc/git/builds/219030999\n\nAlso, the above patch was initially my GSoC project topic, but I changed\nit later on and added these bug fixes to my wishlist of the proposal.\n\n(Had to send the patch again since a typo occurred while sending previous the mail)\n\n builtin/submodule--helper.c      |  4 ----\n git.c                            |  3 +++\n submodule.c                      | 45 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                      |  6 ++++++\n t/t6134-pathspec-in-submodule.sh |  2 +-\n 5 files changed, 55 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 15a5430c0..4f7c7d7b8 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -219,10 +219,6 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr\n \treturn 0;\n }\n \n-struct module_list {\n-\tconst struct cache_entry **entries;\n-\tint alloc, nr;\n-};\n #define MODULE_LIST_INIT { NULL, 0, 0 }\n \n static int module_list_compute(int argc, const char **argv,\ndiff --git a/git.c b/git.c\nindex 33f52acbc..eefe3fb01 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"help.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--help] [-C <path>] [-c name=value]\\n\"\n@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tif (prefix)\n \t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n+\tif (prefix)\n+\t\tcheck_prefix_inside_submodule(prefix);\n \n \tif (!help && p->option & NEED_WORK_TREE)\n \t\tsetup_work_tree();\ndiff --git a/submodule.c b/submodule.c\nindex 0a2831d84..d2c3023bf 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -545,6 +545,51 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+#define MODULE_LIST_INIT { NULL, 0, 0 }\n+\n+void check_prefix_inside_submodule(const char *prefix)\n+{\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tint i;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\n+\t\tALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n+\t\tlist.entries[list.nr++] = ce;\n+\t\twhile (i + 1 < active_nr &&\n+\t\t\t!strcmp(ce->name, active_cache[i + 1]->name))\n+\t\t\t /*\n+\t\t\t  * Skip entries with the same name in different stages\n+\t\t\t  * to make sure an entry is returned only once.\n+\t\t\t  */\n+\t\t\ti++;\n+\t}\n+\n+\tfor(i = 0; i < list.nr; i++) {\n+\t\tif(strlen((*list.entries[i]).name) ==  strlen(prefix)) {\n+\t\t\tif (!strcmp((*list.entries[i]).name, prefix)) {\n+\t\t\t\t/* This case cannot happen because */\n+\t\t\t\tdie(\"BUG: prefixes end with '/', but we do not record ending slashes in the index\");\n+\t\t\t}\n+\t\t}\n+\t\telse if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {\n+\t\t\tconst char *out = NULL;\n+\t\t\tif(skip_prefix(prefix, (*list.entries[i]).name, &out)) {\n+\t\t\t\tif(strlen(out) == 1 && out[0] == '/')\n+\t\t\t\t\tdie(_(\"command from inside unpopulated submodule '%s' not supported.\"), (*list.entries[i]).name);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t      int flags, void *cb_data)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 05ab674f0..5e41e5afc 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -31,6 +31,12 @@ struct submodule_update_strategy {\n };\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n+struct module_list {\n+\tconst struct cache_entry **entries;\n+\tint alloc, nr;\n+};\n+\n+extern void check_prefix_inside_submodule(const char *prefix);\n extern int is_staging_gitmodules_ok(void);\n extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n extern int remove_path_from_gitmodules(const char *path);\ndiff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh\nindex fd401ca60..086cc4c47 100755\n--- a/t/t6134-pathspec-in-submodule.sh\n+++ b/t/t6134-pathspec-in-submodule.sh\n@@ -25,7 +25,7 @@ test_expect_success 'error message for path inside submodule' '\n '\n \n cat <<EOF >expect\n-fatal: Pathspec '.' is in submodule 'sub'\n+fatal: command from inside unpopulated submodule 'sub' not supported.\n EOF\n \n test_expect_success 'error message for path inside submodule from within submodule' '\n-- \n2.11.0\n\n"},{"id":"316295","messageId":"CAGZ79kYULEaVYFX6_HGJi=CEA1E3Z8i4sKw+8bMaVsqiyWsuGw@mail.gmail.com","threadId":"45614","inReplyTo":"20170406060053.4453-1-pc44800@gmail.com","subject":"Re: [GSoC][PATCH v1] Disallow git commands from within unpopulated submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-06T18:15:43Z","receivedAt":"2017-04-06T18:15:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Apr 5, 2017 at 11:00 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> The main motivations for disallowing git commands within an\n> unpopulated submodule are:\n>\n> Whenever we run \"git -C status\" within an unpopulated submodule, it\n> falls back to the superproject. This occurs since there is no .git\n> file in the submodule directory. So superproject's status gets displayed.\n> Also, the user's intention is not clear behind running the command\n> in an unpopulated submodule. Hence we prefer to error out.\n>\n> When we run the command \"git -C sub add .\" within a submodule, the\n> results observed are:\n>\n> In the case of the populated submodule, it acts like running “git add .“\n> inside the submodule. This is uncontroversial and runs as expected.\n>\n> In the case of the unpopulated submodule, the user's intention behind\n> entering the above command is unclear. He may have intended to add\n> the submodule to the superproject or to add all files inside the\n> sub/ directory to the submodule or superproject. Hence we’ll prefer\n> to error out in these case.\n>\n> Eventually, we use a check_prefix_inside_submodule to see check if the\n> path is inside an unpopulated submodule. If it is, then we report the\n> user about the unpopulated submodule.\n>\n> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>\n> ---\n>\n> Since this patch effectively uses RUN_SETUP, builtin commands like\n> 'diff' and other non-builtin commands are not filtered.\n> For such cases, I think, we need to handle them separately.\n>\n> Also since currently, git-submodule is not a builtin command, the\n> command for initializing and updating the submodule doesn't return an\n> error message, but once it is converted to builtin, we need to handle\n> its case explicitly.\n>\n> The build report of this patch is available on:\n> https://travis-ci.org/pratham-pc/git/builds/219030999\n>\n> Also, the above patch was initially my GSoC project topic, but I changed\n> it later on and added these bug fixes to my wishlist of the proposal.\n\nThanks for picking this topic up. :)\n\nA couple of weeks back I floated a similar proposal of a patch[1], but\nas far as I remember Peff hinted that it is a bad UI to do it on such a\ngeneric early level[2]. And you also mention here that we'd not affect\ngit-diff or other commands that do not have RUN_SETUP set.\n\n[1] https://public-inbox.org/git/20170119193023.26837-1-sbeller@google.com/\n[2] from the same thread as [1]:\n  https://public-inbox.org/git/20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net/\n\nAnd I agree with Peff here that this high level catching is not the best way to\ngo. Rather we'd have to go through each command, e.g. in git-status I could\nimagine it could look like: (white space mangled):\n\n    diff --git a/builtin/commit.c b/builtin/commit.c\n    index 4e288bc513..e3c44d4ac4 100644\n    --- a/builtin/commit.c\n    +++ b/builtin/commit.c\n    @@ -1328,6 +1328,23 @@ static int git_status_config(const char *k,\nconst char *v, void *cb)\n         return git_diff_ui_config(k, v, NULL);\n     }\n\n    +static void print_warning_inside_submodule(int status_format,\nconst char *prefix)\n    +{\n    +    switch (status_format) {\n    +    case STATUS_FORMAT_UNSPECIFIED: /* fall through */\n    +    case STATUS_FORMAT_NONE:     /* fall through */\n    +    case STATUS_FORMAT_LONG:\n    +        printf(_(\"\\n\\nWARNING: \\n\\nIn uninitialized submodule\n'%s'\\n\\n\\n\"), prefix);\n    +    case STATUS_FORMAT_SHORT:\n    +        printf(_(\"WARNING: In uninitialized submodule '%s'\\n\"), prefix);\n    +    case STATUS_FORMAT_PORCELAIN:\n    +        /* cannot encode the warning in porcelain v1. */\n    +        break;\n    +    case STATUS_FORMAT_PORCELAIN_V2:\n    +        printf(\"# WARNING prefix in submodule\\n\");\n    +    }\n    +}\n    +\n     int cmd_status(int argc, const char **argv, const char *prefix)\n     {\n         static struct wt_status s;\n    @@ -1380,6 +1397,9 @@ int cmd_status(int argc, const char **argv,\nconst char *prefix)\n         read_cache_preload(&s.pathspec);\n         refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n&s.pathspec, NULL, NULL);\n\n    +    if (prefix && check_prefix_inside_submodule(prefix))\n    +        print_warning_inside_submodule(status_format, prefix);\n    +\n         fd = hold_locked_index(&index_lock, 0);\n\n         s.is_initial = get_sha1(s.reference, oid.hash) ? 1 : 0;\n\n>\n> +#define MODULE_LIST_INIT { NULL, 0, 0 }\n\nI'd keep this initializer macro near the definition, i.e. in the header\n(compare to STRBUF_INIT or STRING_LIST_INIT_* for example),\nas this can then be used where ever we can use the data structure.\n\n> +void check_prefix_inside_submodule(const char *prefix)\n\nI think we'll need this function in returning way, i.e.\n\n    int check_prefix_inside_submodule(const char *prefix)\n    {\n        ... do check ...\n        return result;\n    }\n\n> +{\n> +       struct module_list list = MODULE_LIST_INIT;\n> +       int i;\n> +\n> +       if (read_cache() < 0)\n> +               die(_(\"index file corrupt\"));\n> +\n> +       for (i = 0; i < active_nr; i++) {\n> +               const struct cache_entry *ce = active_cache[i];\n> +\n> +               if (!S_ISGITLINK(ce->ce_mode))\n> +                               continue;\n> +\n> +               ALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n> +               list.entries[list.nr++] = ce;\n> +               while (i + 1 < active_nr &&\n> +                       !strcmp(ce->name, active_cache[i + 1]->name))\n> +                        /*\n> +                         * Skip entries with the same name in different stages\n> +                         * to make sure an entry is returned only once.\n> +                         */\n> +                       i++;\n> +       }\n\nThe code up to here seems to be partially duplicate of\nsubmodule--helper.c#module_list_compute().\n\nAt first I tried coming up with a nice code deduplication (i.e. put\nthe essential parts as a function somewhere), but I think this doesn't\nmake sense from an algorithmic point of view, because this runs in O(n)\nof active_cache.\n\nactive_cache is sorted by (1st) alphabet and (2nd) the index stage.\nThe second sorting is why we have the while loop with the comment\nin the code.\n\nThe problem we are trying to solve here, is \"Does the active_index contain\nprefix?\", which can be done in O(log n) with a binary search, because\nactive_index is sorted by alphabet.\n\nSo I am not sure how much code we can reuse here. nevertheless:\n\n> +       for(i = 0; i < list.nr; i++) {\n\nStyle nit: We prefer a whitespace between a control statement\n(for, if, while) and the opening parens, i.e. \"for (i = ..\"\n\n> +               if(strlen((*list.entries[i]).name) ==  strlen(prefix)) {\n\n(*list.entries[i]).name\n\ncan be simplified. The dereference *, combined with an access\nof the struct member can be done as ->.\n(*foo).bar is equal to foo->bar.\n\n> +               }\n> +               else if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {\n\nThe Git coding style prefers to have the closing brace and the else\non the same line:\n\n        ..\n    } else if (..) {\n        ..\n\n\n> +                       const char *out = NULL;\n> +                       if(skip_prefix(prefix, (*list.entries[i]).name, &out)) {\n> +                               if(strlen(out) == 1 && out[0] == '/')\n\nThe strlen operation can take quite long potentially (O(n) with n the\nlength of out).\nSo you could put the cheaper operation first, or the following would\ncheck the same\nwithout having to compute the length:\n\n    if (out && out[0] == '/' && !out + 1)\n        ..\n\n> +                                       die(_(\"command from inside unpopulated submodule '%s' not supported.\"), (*list.entries[i]).name);\n\nOnce we have this function inside each command, we can be more precise\nthe error message. :)\n\nThanks,\nStefan\n"},{"id":"316308","messageId":"CAME+mvXbhW9Ya=FZEA8AmWihdn5ROVx6zAbsNYCEUEBY9p-wVQ@mail.gmail.com","threadId":"45614","inReplyTo":"CAGZ79kYULEaVYFX6_HGJi=CEA1E3Z8i4sKw+8bMaVsqiyWsuGw@mail.gmail.com","subject":"Re: [GSoC][PATCH v1] Disallow git commands from within unpopulated submodules","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-04-06T20:48:45Z","receivedAt":"2017-04-06T20:48:51Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"> A couple of weeks back I floated a similar proposal of a patch[1], but\n> as far as I remember Peff hinted that it is a bad UI to do it on such a\n> generic early level[2]. And you also mention here that we'd not affect\n> git-diff or other commands that do not have RUN_SETUP set.\n>\n> [1] https://public-inbox.org/git/20170119193023.26837-1-sbeller@google.com/\n> [2] from the same thread as [1]:\n>   https://public-inbox.org/git/20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net/\n>\n> And I agree with Peff here that this high level catching is not the best way to\n> go. Rather we'd have to go through each command, e.g. in git-status I could\n> imagine it could look like: (white space mangled):\n>\n>     diff --git a/builtin/commit.c b/builtin/commit.c\n>     index 4e288bc513..e3c44d4ac4 100644\n>     --- a/builtin/commit.c\n>     +++ b/builtin/commit.c\n>     @@ -1328,6 +1328,23 @@ static int git_status_config(const char *k,\n> const char *v, void *cb)\n>          return git_diff_ui_config(k, v, NULL);\n>      }\n>\n>     +static void print_warning_inside_submodule(int status_format,\n> const char *prefix)\n>     +{\n>     +    switch (status_format) {\n>     +    case STATUS_FORMAT_UNSPECIFIED: /* fall through */\n>     +    case STATUS_FORMAT_NONE:     /* fall through */\n>     +    case STATUS_FORMAT_LONG:\n>     +        printf(_(\"\\n\\nWARNING: \\n\\nIn uninitialized submodule\n> '%s'\\n\\n\\n\"), prefix);\n>     +    case STATUS_FORMAT_SHORT:\n>     +        printf(_(\"WARNING: In uninitialized submodule '%s'\\n\"), prefix);\n>     +    case STATUS_FORMAT_PORCELAIN:\n>     +        /* cannot encode the warning in porcelain v1. */\n>     +        break;\n>     +    case STATUS_FORMAT_PORCELAIN_V2:\n>     +        printf(\"# WARNING prefix in submodule\\n\");\n>     +    }\n>     +}\n>     +\n>      int cmd_status(int argc, const char **argv, const char *prefix)\n>      {\n>          static struct wt_status s;\n>     @@ -1380,6 +1397,9 @@ int cmd_status(int argc, const char **argv,\n> const char *prefix)\n>          read_cache_preload(&s.pathspec);\n>          refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n> &s.pathspec, NULL, NULL);\n>\n>     +    if (prefix && check_prefix_inside_submodule(prefix))\n>     +        print_warning_inside_submodule(status_format, prefix);\n>     +\n>          fd = hold_locked_index(&index_lock, 0);\n>\n>          s.is_initial = get_sha1(s.reference, oid.hash) ? 1 : 0;\n>\nYes, even after finishing my patch and finding out that it doesn't work\nfor various commands, high level catching doesn't seems correct. Also\nafter you mention in the status command itself, as we don't expect it to\nerror out in every case, hence making the change in every command\nindividually will be an appropriate choice. Also in this way, we can also\ninclude the case of git-diff and also for the non-bultin commands.\n\n>>\n>> +#define MODULE_LIST_INIT { NULL, 0, 0 }\n>\n> I'd keep this initializer macro near the definition, i.e. in the header\n> (compare to STRBUF_INIT or STRING_LIST_INIT_* for example),\n> as this can then be used where ever we can use the data structure.\n>\nI was a little confused about its location too. But I agree with you\nthat as it can be used where ever we can later, keeping it in\nheader will be more appropriate.\n\n>> +void check_prefix_inside_submodule(const char *prefix)\n>\n> I think we'll need this function in returning way, i.e.\n>\n>     int check_prefix_inside_submodule(const char *prefix)\n>     {\n>         ... do check ...\n>         return result;\n>     }\n>\n>> +{\n>> +       struct module_list list = MODULE_LIST_INIT;\n>> +       int i;\n>> +\n>> +       if (read_cache() < 0)\n>> +               die(_(\"index file corrupt\"));\n>> +\n>> +       for (i = 0; i < active_nr; i++) {\n>> +               const struct cache_entry *ce = active_cache[i];\n>> +\n>> +               if (!S_ISGITLINK(ce->ce_mode))\n>> +                               continue;\n>> +\n>> +               ALLOC_GROW(list.entries, list.nr + 1, list.alloc);\n>> +               list.entries[list.nr++] = ce;\n>> +               while (i + 1 < active_nr &&\n>> +                       !strcmp(ce->name, active_cache[i + 1]->name))\n>> +                        /*\n>> +                         * Skip entries with the same name in different stages\n>> +                         * to make sure an entry is returned only once.\n>> +                         */\n>> +                       i++;\n>> +       }\n>\n> The code up to here seems to be partially duplicate of\n> submodule--helper.c#module_list_compute().\n>\n> At first I tried coming up with a nice code deduplication (i.e. put\n> the essential parts as a function somewhere), but I think this doesn't\n> make sense from an algorithmic point of view, because this runs in O(n)\n> of active_cache.\n>\n> active_cache is sorted by (1st) alphabet and (2nd) the index stage.\n> The second sorting is why we have the while loop with the comment\n> in the code.\n\nSorry, I was unaware about the active_cache being sorted, hence went\nfor linear search.\n\n>\n> The problem we are trying to solve here, is \"Does the active_index contain\n> prefix?\", which can be done in O(log n) with a binary search, because\n> active_index is sorted by alphabet.\n>\n> So I am not sure how much code we can reuse here. nevertheless:\n>\n>> +       for(i = 0; i < list.nr; i++) {\n>\n> Style nit: We prefer a whitespace between a control statement\n> (for, if, while) and the opening parens, i.e. \"for (i = ..\"\n>\n>> +               if(strlen((*list.entries[i]).name) ==  strlen(prefix)) {\n>\n> (*list.entries[i]).name\n>\n> can be simplified. The dereference *, combined with an access\n> of the struct member can be done as ->.\n> (*foo).bar is equal to foo->bar.\n>\n>> +               }\n>> +               else if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {\n>\n> The Git coding style prefers to have the closing brace and the else\n> on the same line:\n>\n>         ..\n>     } else if (..) {\n>         ..\n>\n\nSorry for being sloppy with the coding style. In all my future patches\nI'll make sure I learn from all the mistakes I made so far and also\nbe more careful.\n\n>\n>> +                       const char *out = NULL;\n>> +                       if(skip_prefix(prefix, (*list.entries[i]).name, &out)) {\n>> +                               if(strlen(out) == 1 && out[0] == '/')\n>\n> The strlen operation can take quite long potentially (O(n) with n the\n> length of out).\n> So you could put the cheaper operation first, or the following would\n> check the same\n> without having to compute the length:\n>\n>     if (out && out[0] == '/' && !out + 1)\n>         ..\n>\n>> +                                       die(_(\"command from inside unpopulated submodule '%s' not supported.\"), (*list.entries[i]).name);\n>\n> Once we have this function inside each command, we can be more precise\n> the error message. :)\n\nThanks for all the advices on this patch. I'll implement the above changes\nin a new patch series.\nThe approach that I'll take will be as follows:\n1. Create function check_prefix_inside_submodule which returns value\n   accordingly. Also this function is implemented by checking the active_cache\n   list and checking if it prefix is present in active_cache[i]->name,\n   using binary search.\n   Also is there some other way which is more quicker than searching\n   for the prefix in the cache?\n2. Individually call this in each command after the git environment is set\n   and options are parsed.\n3. Apply this change for appropriate options only, as suggested above for\n   the status command.\nThis will ensure more accuracy.\nI have also mentioned to work on this BUG in my git proposal as well, but I\nkept it in my wishlist section. Hence, I'll continue to work on this as my time\npermits. Currently, I'm also working on converting the git-submodule\nsubcommand 'foreach' from script to builtin, by first converting it\nto a function in submodule--helper.c and then later converting it\nto builtin.\n\nThanks,\nPrathamesh Chavan\n"}]}