{"thread":{"id":"41783","subject":"[PATCH] submodule helper: accept '.' for repositories with no submodules","startedAt":"2016-03-22T17:59:39Z","lastAt":"2016-03-22T22:50:46Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281459","messageId":"1458669579-32160-1-git-send-email-sbeller@google.com","threadId":"41783","inReplyTo":null,"subject":"[PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-22T17:59:39Z","receivedAt":"2016-03-22T17:59:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In 74703a1e4d (2015-09-02, submodule: rewrite `module_list` shell\nfunction in C), \"submodule deinit .\" was broken.\n\nThe original module_list accepted '.' as a pathspec just fine, as it\nwas using\n\n  git ls-files -z --error-unmatch --stage -- \"$@\" || { custom filtering}\n\nand git ls-files doesn't make a difference between \".\" and no arguments\nthere. When using the parse_pathspec function in C, this is a difference\nhowever, when no path matches.\n\n'submodule deinit' asks users to explicitely to give '.' instead of\nempty arguments to specify all submodules (as a safety measure?),\nso we have to support that as well.\n\nAdd a test case to prevent this error coming up again and fix this\nby special casing '.' in the new module_list to reduce the difference\nbetween the old and new module_list.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n This applies on v2.7.4\n \n I looked at alternatives of how to fix it, e.g.\n later in module_list to make an exception for calling\n        if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n             result = -1;\n but that is similarly ugly.\n\n builtin/submodule--helper.c |  8 ++++++++\n t/t7400-submodule-basic.sh  | 10 ++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed764c9..47e6839 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -23,6 +23,14 @@ static int module_list_compute(int argc, const char **argv,\n {\n \tint i, result = 0;\n \tchar *ps_matched = NULL;\n+\n+\t/*\n+\t * We need to treat a path spec of '.' the same as an empty\n+\t * path spec, because \"submodule deinit\" wants to be given '.'\n+\t * instead of an empty list.\n+\t */\n+\tif (argc == 1 && !strcmp(\".\", argv[0]))\n+\t\targv[0] = NULL;\n \tparse_pathspec(pathspec, 0,\n \t\t       PATHSPEC_PREFER_FULL |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex be82a75..fdf7105 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -849,6 +849,16 @@ test_expect_success 'set up a second submodule' '\n \tgit commit -m \"submodule example2 added\"\n '\n \n+test_expect_success 'submodule deinit -f . works on empty repository' '\n+\ttest_when_finished \"rm -rf newdirectory\" &&\n+\tmkdir newdirectory &&\n+\t(\n+\t\tcd newdirectory &&\n+\t\tgit init &&\n+\t\tgit submodule deinit .\n+\t)\n+'\n+\n test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' '\n \tgit config submodule.example.foo bar &&\n \tgit config submodule.example2.frotz nitfol &&\n-- \n2.7.4.1.g33fcf9d\n"},{"id":"281466","messageId":"xmqqy49agx98.fsf@gitster.mtv.corp.google.com","threadId":"41783","inReplyTo":"1458669579-32160-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T18:53:23Z","receivedAt":"2016-03-22T18:53:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> In 74703a1e4d (2015-09-02, submodule: rewrite `module_list` shell\n> function in C), \"submodule deinit .\" was broken.\n>\n> The original module_list accepted '.' as a pathspec just fine, as it\n> was using\n>\n>   git ls-files -z --error-unmatch --stage -- \"$@\" || { custom filtering}\n>\n> and git ls-files doesn't make a difference between \".\" and no arguments\n> there. When using the parse_pathspec function in C, this is a difference\n> however, when no path matches.\n\nIs that an accurate description of the issue?\n\nThe original (above) errors out if there is a pathspec that does not\nmatch any path in the index.  The C rewrite however instead does\nthis:\n\n\t\tif (!S_ISGITLINK(ce->ce_mode) ||\n\t\t    !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n\t\t\t\t    0, ps_matched, 1))\n\t\t\tcontinue;\n\nto error out if there is a pathspec that does not match any\nsubmodule path.  That is the root cause of the difference in\nbehaviour.\n\nSo if we were to aim for a faithful rewrite, perhaps swapping the\norder of the check, i.e.\n\n\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n\t\t\t\t    0, ps_matched, 1) ||\n\t\t    !S_ISGITLINK(ce->ce_mode))\n\t\t\tcontinue;\n\nNow, arguably, the behaviour of C rewrite makes more sense in that\nit would diagnose a pathspec that does not match a submodule as an\nerror, e.g.\n\n\t$ git submodule--helper list 'COPYIN*'\n\terror: pathspec 'COPYIN*' did not match any file(s) known to git.\n\t#unmatched\n\nThe error message _is_ wrong, but the end result is more helpful to\nthe user---the user thought there was a submodule that would match\nthat pathspec, and there isn't, so we suspect there was a typo and\ncautiously error out.\n\n\"submodule deinit .\" may have \"worked\" in the sense that you would\nhave at least one path in your tree and avoided this \"nothing\nmatches\" most of the time.  It would have still failed with the\nexactly same error if run in an empty repository, i.e.\n\n\t$ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n        $ git init\n        $ rungit v2.6.6 submodule deinit .\n        error: pathspec '.' did not match any file(s) known to git.\n\tDid you forget to 'git add'?\n\t$ >file && git add file\n        $ rungit v2.6.6 submodule deinit .\n\t$ echo $?\n\t0\n\nIn other words, \"Use '.' if you really want to\" is a faulty\nsuggestion.  There is no guarantee that it would match anything in\nthe old world order, and certainly there is no guarantee that it\nwould match any submodule in the new world order.\n\nWhen another person who is not Per Cederqvist realizes that the\nlogic that issues the faulty suggestion is because the command wants\nsome pathspec, she may try\n\n    $ git submodule deinit '*'\n\nand complain that it used to work but it no longer, even with the\nband-aid patch under discussion that special cases '.'.\n\nSo I dunno.  This is not only \"deinit\", but also the post rewrite\nversion catches\n\n\t$ git submodule init -- 'COPYIN*'\n\nas an error, which we probably would want to keep, so I am reluctant\nto suggest swapping the order of the check to do pathspec first and\nthen gitlink-ness (it has performance implications but correctness\nis a more important issue), but if we want to keep the backward\ncompatibility, that would be the best bug-to-bug compatible fix in\nthe shorter term.\n"},{"id":"281472","messageId":"CAGZ79kaMa3CyJ-Y9Xn9QShzTUGythos+pmKZXQ7m-2c0bF0u9A@mail.gmail.com","threadId":"41783","inReplyTo":"xmqqy49agx98.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-22T19:30:13Z","receivedAt":"2016-03-22T19:30:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 22, 2016 at 11:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> In 74703a1e4d (2015-09-02, submodule: rewrite `module_list` shell\n>> function in C), \"submodule deinit .\" was broken.\n>>\n>> The original module_list accepted '.' as a pathspec just fine, as it\n>> was using\n>>\n>>   git ls-files -z --error-unmatch --stage -- \"$@\" || { custom filtering}\n>>\n>> and git ls-files doesn't make a difference between \".\" and no arguments\n>> there. When using the parse_pathspec function in C, this is a difference\n>> however, when no path matches.\n>\n> Is that an accurate description of the issue?\n>\n> The original (above) errors out if there is a pathspec that does not\n> match any path in the index.  The C rewrite however instead does\n> this:\n>\n>                 if (!S_ISGITLINK(ce->ce_mode) ||\n>                     !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>                                     0, ps_matched, 1))\n>                         continue;\n>\n> to error out if there is a pathspec that does not match any\n> submodule path.  That is the root cause of the difference in\n> behaviour.\n>\n> So if we were to aim for a faithful rewrite, perhaps swapping the\n> order of the check, i.e.\n>\n>                 if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>                                     0, ps_matched, 1) ||\n>                     !S_ISGITLINK(ce->ce_mode))\n>                         continue;\n>\n> Now, arguably, the behaviour of C rewrite makes more sense in that\n> it would diagnose a pathspec that does not match a submodule as an\n> error, e.g.\n>\n>         $ git submodule--helper list 'COPYIN*'\n>         error: pathspec 'COPYIN*' did not match any file(s) known to git.\n>         #unmatched\n>\n> The error message _is_ wrong, but the end result is more helpful to\n> the user---the user thought there was a submodule that would match\n> that pathspec, and there isn't, so we suspect there was a typo and\n> cautiously error out.\n>\n> \"submodule deinit .\" may have \"worked\" in the sense that you would\n> have at least one path in your tree and avoided this \"nothing\n> matches\" most of the time.  It would have still failed with the\n> exactly same error if run in an empty repository, i.e.\n>\n>         $ E=/var/tmp/x/empty && rm -fr \"$E\" && mkdir -p \"$E\" && cd \"$E\"\n>         $ git init\n>         $ rungit v2.6.6 submodule deinit .\n>         error: pathspec '.' did not match any file(s) known to git.\n>         Did you forget to 'git add'?\n>         $ >file && git add file\n>         $ rungit v2.6.6 submodule deinit .\n>         $ echo $?\n>         0\n>\n> In other words, \"Use '.' if you really want to\" is a faulty\n> suggestion.  There is no guarantee that it would match anything in\n> the old world order, and certainly there is no guarantee that it\n> would match any submodule in the new world order.\n>\n> When another person who is not Per Cederqvist realizes that the\n> logic that issues the faulty suggestion is because the command wants\n> some pathspec, she may try\n>\n>     $ git submodule deinit '*'\n>\n> and complain that it used to work but it no longer, even with the\n> band-aid patch under discussion that special cases '.'.\n>\n> So I dunno.  This is not only \"deinit\", but also the post rewrite\n> version catches\n>\n>         $ git submodule init -- 'COPYIN*'\n>\n> as an error, which we probably would want to keep, so I am reluctant\n> to suggest swapping the order of the check to do pathspec first and\n> then gitlink-ness (it has performance implications but correctness\n> is a more important issue), but if we want to keep the backward\n> compatibility, that would be the best bug-to-bug compatible fix in\n> the shorter term.\n\nSo I have 2 goals in mind:\n* Git suggested to use '.' explicitly, so it should just work --even\nfor a completely\n  empty repository (see the test for it)\n* Eventually -- not in this patch, but a later patch targeted at\nmaster -- we want to\n  remove the recommendation to use '.', and allow no arguments or a\ndifferent argument\n  for \"all submodules\". git add uses '.' for it though, so '.' seems\nright and valid.\n  git add '*' is also valid.\n\nMaybe combine the second idea with a slight refactoring of\nparse_pathspec, such that you\ncan pass a callback function to parse_pathspec which can decided on\neach file if it is\na file to inspect. (i.e. for our usecase we'd check for ce_mode to be\nGITLINK, another\nhypothetical use case would be using parse_pathspec for finding all\nfiles with a certain\nproperty, e.g. finding all files ending in .c or files written in all\ncapital letters or such)\n\nThen you could do a conditional parse_pathspec over the partial\nrepository which matched\nthe additional filtering function.\n\n\nMaybe we can also special case the \"force\" argument only here for\nCedars use case.\n(\"git submodule deinit .\" complains because there are no further\nsubmodules, but -f solves\nthe complaint?)\n"},{"id":"281477","messageId":"xmqqoaa6gtuu.fsf@gitster.mtv.corp.google.com","threadId":"41783","inReplyTo":"CAGZ79kaMa3CyJ-Y9Xn9QShzTUGythos+pmKZXQ7m-2c0bF0u9A@mail.gmail.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T20:06:49Z","receivedAt":"2016-03-22T20:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Maybe we can also special case the \"force\" argument only here for\n> Cedars use case.  (\"git submodule deinit .\" complains because\n> there are no further submodules, but -f solves the complaint?)\n\nI think that would have been the most sensible thing to do when\nwe were adding \"git submodule deinit\", and it would be the most\nsensible behaviour in the longer term.\n\n\n> So I have 2 goals in mind:\n\n> * Git suggested to use '.' explicitly, so it should just work\n> --even for a completely empty repository (see the test for it)\n\nI actually view this as either a low-prio goal or even a\nnon-goal, as long as we have a sensible migration plan to stop\nsuggesting '.'.\n\n> * Eventually -- not in this patch, but a later patch targeted\n> at master -- we want to remove the recommendation to use '.',\n> and allow no arguments or a different argument for \"all\n> submodules\".\n\n> git add uses '.' for it though, so '.' seems right and valid.\n> git add '*' is also valid.\n\nPerhaps I am misunderstanding what you want.  Do you want a\nregular file that happens to match pathspec to prevent\nmodule_list from noticing \"pathspec did not match\" situation and\ngiving the user an error?  That would be the \"match-pathspec first\nand then mode-check\" behaviour, that is the same as v2.6.x\nseries [*1*].\n\n> Maybe combine the second idea with a slight refactoring of\n> parse_pathspec, such that you can pass a callback function to\n> parse_pathspec which can decided on each file if it is a file\n> to inspect. (i.e. for our usecase we'd check for ce_mode to be\n> GITLINK, another hypothetical use case would be using\n> parse_pathspec for finding all files with a certain property,\n> e.g. finding all files ending in .c or files written in all\n> capital letters or such)\n>\n> Then you could do a conditional parse_pathspec over the partial\n> repository which matched\n> the additional filtering function.\n>\n\nI do not think that buys us much.  You have already shown how to\nimplement \"filter first and then pathspec match\" if a caller\nwants to (which turned out to be a regression in this case, but\nthat is besides the point).\n\n\n\n[Footnote]\n\n*1* If not, then similaritly with \"git add\" does not have much\n    to do with what module_list() should do.  \"git add $pathspec\"\n    fails if there is nothing that matches the pathspec, but\n    \"module_list\" wants to complain if $pathspec does not match\n    any submodule.  And it is an accident that \"git add .\" in\n    an empty directory does not complain (indeed \"git add '*'\"\n    does complain because it sidesteps the accident).\n"},{"id":"281487","messageId":"CAGZ79ka=EJ-L+Zaqh0kWUdjmyUva3fEcbwJn18uxshe3R5ABBQ@mail.gmail.com","threadId":"41783","inReplyTo":"xmqqoaa6gtuu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-22T21:16:40Z","receivedAt":"2016-03-22T21:16:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 22, 2016 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Maybe we can also special case the \"force\" argument only here for\n>> Cedars use case.  (\"git submodule deinit .\" complains because\n>> there are no further submodules, but -f solves the complaint?)\n>\n> I think that would have been the most sensible thing to do when\n> we were adding \"git submodule deinit\", and it would be the most\n> sensible behaviour in the longer term.\n>\n>\n>> So I have 2 goals in mind:\n>\n>> * Git suggested to use '.' explicitly, so it should just work\n>> --even for a completely empty repository (see the test for it)\n>\n> I actually view this as either a low-prio goal or even a\n> non-goal, as long as we have a sensible migration plan to stop\n> suggesting '.'.\n>\n>> * Eventually -- not in this patch, but a later patch targeted\n>> at master -- we want to remove the recommendation to use '.',\n>> and allow no arguments or a different argument for \"all\n>> submodules\".\n>\n>> git add uses '.' for it though, so '.' seems right and valid.\n>> git add '*' is also valid.\n>\n> Perhaps I am misunderstanding what you want.  Do you want a\n> regular file that happens to match pathspec to prevent\n> module_list from noticing \"pathspec did not match\" situation and\n> giving the user an error?  That would be the \"match-pathspec first\n> and then mode-check\" behaviour, that is the same as v2.6.x\n> series [*1*].\n>\n>> Maybe combine the second idea with a slight refactoring of\n>> parse_pathspec, such that you can pass a callback function to\n>> parse_pathspec which can decided on each file if it is a file\n>> to inspect. (i.e. for our usecase we'd check for ce_mode to be\n>> GITLINK, another hypothetical use case would be using\n>> parse_pathspec for finding all files with a certain property,\n>> e.g. finding all files ending in .c or files written in all\n>> capital letters or such)\n>>\n>> Then you could do a conditional parse_pathspec over the partial\n>> repository which matched\n>> the additional filtering function.\n>>\n>\n> I do not think that buys us much.  You have already shown how to\n> implement \"filter first and then pathspec match\" if a caller\n> wants to (which turned out to be a regression in this case, but\n> that is besides the point).\n\nAnd by including this filtering into the pathspec machine we can pass\na flag DONT_BREAK_ON_NO_FILTER_RESULTS_WHEN_HAVING_OTHER_MATCHES\n(name for illustration purpose only ;) which is how I understand this\nregression?\n\n>\n>\n>\n> [Footnote]\n>\n> *1* If not, then similaritly with \"git add\" does not have much\n>     to do with what module_list() should do.  \"git add $pathspec\"\n>     fails if there is nothing that matches the pathspec, but\n>     \"module_list\" wants to complain if $pathspec does not match\n>     any submodule.  And it is an accident that \"git add .\" in\n>     an empty directory does not complain (indeed \"git add '*'\"\n>     does complain because it sidesteps the accident).\n>\n>\n"},{"id":"281493","messageId":"xmqqtwjyfb0s.fsf@gitster.mtv.corp.google.com","threadId":"41783","inReplyTo":"CAGZ79ka=EJ-L+Zaqh0kWUdjmyUva3fEcbwJn18uxshe3R5ABBQ@mail.gmail.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T21:38:59Z","receivedAt":"2016-03-22T21:38:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I do not think that buys us much.  You have already shown how to\n>> implement \"filter first and then pathspec match\" if a caller\n>> wants to (which turned out to be a regression in this case, but\n>> that is besides the point).\n>\n> And by including this filtering into the pathspec machine we can pass\n> a flag DONT_BREAK_ON_NO_FILTER_RESULTS_WHEN_HAVING_OTHER_MATCHES\n> (name for illustration purpose only ;) which is how I understand this\n> regression?\n\nBut you do not even need that if you fix the regression with\nsomething like this, no?  Do we need to add complexity to pathspec\nmachinery only to make it easier to misuse it and then add another\nDONT_BREAK_... band-aid to fix a bug that can come from such a\nmisuse?\n\n builtin/submodule--helper.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed764c9..740b57a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -37,10 +37,11 @@ static int module_list_compute(int argc, const char **argv,\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    !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n+\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n \t\t\t\t    0, ps_matched, 1))\n \t\t\tcontinue;\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n \n \t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n \t\tlist->entries[list->nr++] = ce;\n"},{"id":"281496","messageId":"CAGZ79kYNQ46Yett8xwtPL8Mi1TkitB1kuWbQDsR6bg-5dxckNA@mail.gmail.com","threadId":"41783","inReplyTo":"xmqqtwjyfb0s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-22T21:47:22Z","receivedAt":"2016-03-22T21:47:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 22, 2016 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>> I do not think that buys us much.  You have already shown how to\n>>> implement \"filter first and then pathspec match\" if a caller\n>>> wants to (which turned out to be a regression in this case, but\n>>> that is besides the point).\n>>\n>> And by including this filtering into the pathspec machine we can pass\n>> a flag DONT_BREAK_ON_NO_FILTER_RESULTS_WHEN_HAVING_OTHER_MATCHES\n>> (name for illustration purpose only ;) which is how I understand this\n>> regression?\n>\n> But you do not even need that if you fix the regression with\n> something like this, no?  Do we need to add complexity to pathspec\n> machinery only to make it easier to misuse it and then add another\n> DONT_BREAK_... band-aid to fix a bug that can come from such a\n> misuse?\n>\n>  builtin/submodule--helper.c | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ed764c9..740b57a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -37,10 +37,11 @@ static int module_list_compute(int argc, const char **argv,\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> -                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> +               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>                                     0, ps_matched, 1))\n>                         continue;\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\n\nThis patch is conceptually, what you said in the first message as\n\n> So I dunno.  This is not only \"deinit\", but also the post rewrite\n> version catches\n>\n>         $ git submodule init -- 'COPYIN*'\n>\n> as an error, which we probably would want to keep, so I am reluctant\n> to suggest swapping the order of the check to do pathspec first and\n> then gitlink-ness (it has performance implications but correctness\n> is a more important issue), but if we want to keep the backward\n> compatibility, that would be the best bug-to-bug compatible fix in\n> the shorter term.\n\nI was under the impression that we do not want to have this bugfix\n(at least long term) and then I tried to come up with an idea, which\nis both:\n* correct in this case\n* and catches the git submodule init -- 'COPYIN*' case as failure\n"},{"id":"281498","messageId":"xmqqpoumf9uy.fsf@gitster.mtv.corp.google.com","threadId":"41783","inReplyTo":"CAGZ79kYNQ46Yett8xwtPL8Mi1TkitB1kuWbQDsR6bg-5dxckNA@mail.gmail.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T22:04:05Z","receivedAt":"2016-03-22T22:04:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> I was under the impression that we do not want to have this bugfix\n> (at least long term) and then I tried to come up with an idea,\n> which is both:\n> * correct in this case\n> * and catches the git submodule init -- 'COPYIN*' case as failure\n\nSatisfying both has to be impossible, I am afraid, so coming up with\nan idea to do so may be futile.\n\nIf \"git submodule $subcmd -- 'COPYIN*'\" must fail (which I think is\na good property to have), then \"git submodule $subcmd -- .\" must\nfail if there is no submodule in the repository, I would think, if\nwe want to be consistent.  Both are complaining not just because\nthere is no path that match the given pathspec, but because there is\nno submodule that match the given pathspec.\n\nThat is why I said I was in favour of giving some _other_ way, other\nthan \"this is guaranteed to match some submodule\" pathspec (which\nfundamentally does not exist, because there are projects that do not\nhave any submodule in them), as a suggestion for \"deinit all\"\nsafety.\n\nA slight tangent.\n\nWe have --error-unmatch in ls-files to detect pathspec that did not\nhit anything, and our Porcelain commands treat a pathspec that does\nnot hit anything as an error, but there probably need to be\n\"--unmatch-ok\" option?  I.e.\n\n\t$ git add 'A*'\n        fatal: pathspec 'A*' did not match any files\n        $ git add --unmatch-ok 'A*'\n        ... nothing is added but we do not get an error ...\n\nThen the interaction may go like this:\n\n\t$ git submodule deinit\n        You need to limit submodules to deinit with some pathspec\n        $ git submodule deinit .\n        fatal: pathspec '.' did not match any submodule\n        $ git submodule deinit --unmatch-ok .\n        ... nothing deinited but we do not get an error ...\n\nFor this particular one,\n\n\t$ git submodule deinit --force\n\nmay be also OK, but there may be _other_ conditions that you may\nallow overriding in \"deinit\" operation (e.g. we may refuse to deinit\nwhen there are unsaved/unuploaded changes, and allow --force to\noverride it), so...\n"},{"id":"281499","messageId":"CAGZ79kYhK1dDQsxjRwhArXMi-0Gbd8ciYwdFnerT4fjvpthWCg@mail.gmail.com","threadId":"41783","inReplyTo":"xmqqpoumf9uy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-22T22:10:25Z","receivedAt":"2016-03-22T22:10:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 22, 2016 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> I was under the impression that we do not want to have this bugfix\n>> (at least long term) and then I tried to come up with an idea,\n>> which is both:\n>> * correct in this case\n>> * and catches the git submodule init -- 'COPYIN*' case as failure\n>\n> Satisfying both has to be impossible, I am afraid, so coming up with\n> an idea to do so may be futile.\n>\n> If \"git submodule $subcmd -- 'COPYIN*'\" must fail (which I think is\n> a good property to have), then \"git submodule $subcmd -- .\" must\n> fail if there is no submodule in the repository, I would think, if\n> we want to be consistent.  Both are complaining not just because\n> there is no path that match the given pathspec, but because there is\n> no submodule that match the given pathspec.\n>\n> That is why I said I was in favour of giving some _other_ way, other\n> than \"this is guaranteed to match some submodule\" pathspec (which\n> fundamentally does not exist, because there are projects that do not\n> have any submodule in them), as a suggestion for \"deinit all\"\n> safety.\n>\n> A slight tangent.\n>\n> We have --error-unmatch in ls-files to detect pathspec that did not\n> hit anything, and our Porcelain commands treat a pathspec that does\n> not hit anything as an error, but there probably need to be\n> \"--unmatch-ok\" option?  I.e.\n>\n>         $ git add 'A*'\n>         fatal: pathspec 'A*' did not match any files\n>         $ git add --unmatch-ok 'A*'\n>         ... nothing is added but we do not get an error ...\n>\n> Then the interaction may go like this:\n>\n>         $ git submodule deinit\n>         You need to limit submodules to deinit with some pathspec\n>         $ git submodule deinit .\n>         fatal: pathspec '.' did not match any submodule\n>         $ git submodule deinit --unmatch-ok .\n>         ... nothing deinited but we do not get an error ...\n>\n> For this particular one,\n>\n>         $ git submodule deinit --force\n>\n> may be also OK, but there may be _other_ conditions that you may\n> allow overriding in \"deinit\" operation (e.g. we may refuse to deinit\n> when there are unsaved/unuploaded changes, and allow --force to\n> override it), so...\n\nThat sounds better thought than my thinking.\nSo for now I would send the performance regressing flip of IS_GITLINK\nand match_pathspec targeting 2.7 and then add a --unmatch-ok switch\nfor 2.8 and later?\n"},{"id":"281509","messageId":"xmqqy49adt4p.fsf@gitster.mtv.corp.google.com","threadId":"41783","inReplyTo":"CAGZ79kYhK1dDQsxjRwhArXMi-0Gbd8ciYwdFnerT4fjvpthWCg@mail.gmail.com","subject":"Re: [PATCH] submodule helper: accept '.' for repositories with no submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T22:50:46Z","receivedAt":"2016-03-22T22:50:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So for now I would send the performance regressing flip of\n> IS_GITLINK and match_pathspec targeting 2.7 and then add a\n> --unmatch-ok switch for 2.8 and later?\n\n\"git submodule $subcommand -- COPYIN\\*\" that detects that there is\nno submodule that match the pathspec and errors out, which is what\nthe C rewrite does, is a new feature that was done by accident when\nwe should have been doing a faithful translation.\n\nI'd say bug-to-bug-compatible regression fix is appropriate for 2.8\nand below; the \"check ce-mode first and then pathspec match\" should\nbe done as a new feature after 2.8 final, and if --unmatch-ok is\nnecessary to make the feature work better, that, too, should come in\nthe same timeframe.\n"}]}