{"thread":{"id":"42407","subject":"[PATCH] submodule operations: tighten pathspec errors","startedAt":"2016-05-21T01:21:29Z","lastAt":"2016-06-06T19:31:22Z","messageCount":6,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287145","messageId":"1463793689-19496-1-git-send-email-sbeller@google.com","threadId":"42407","inReplyTo":null,"subject":"[PATCH] submodule operations: tighten pathspec errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-21T01:21:29Z","receivedAt":"2016-05-21T01:21:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"when the pathspec did not match any path, error out.\nAdd the `--error-unmatch` switch to use the old behavior.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis was taken from the Left Over Bits:\n   git submodule $cmd $pathspec may want to error out when the $pathspec does\n   not match any submodules. There must be an --unmatch-ok option to override\n   this safety, though. Cf. $gmane/289535\n   \nIt's a first initial version with no tests (and probably conflicting with\nsome topics in flight), but I was curious how involved this issue actually is,\nso I took a stab at implementing it.\n\nI was debating if inside git-submodules.sh we want to pass around a variable\nor instead export an environment variable GIT_SUBMODULE_UNMATCH, such that we\ndo not give it as an argument to the submodule--helper builtin.\n\n(This version is developed on top of the sb/submodule-deinit-all)\n   \n Documentation/git-submodule.txt | 15 ++++++++++-----\n builtin/submodule--helper.c     | 19 +++++++++++--------\n git-submodule.sh                | 41 ++++++++++++++++++++++++++++++-----------\n 3 files changed, 51 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex ad85183..ceacc02 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -11,16 +11,16 @@ SYNOPSIS\n [verse]\n 'git submodule' [--quiet] add [-b <branch>] [-f|--force] [--name <name>]\n \t      [--reference <repository>] [--depth <depth>] [--] <repository> [<path>]\n-'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n-'git submodule' [--quiet] init [--] [<path>...]\n-'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)\n-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]\n+'git submodule' [--quiet] status [--error-unmatch] [--cached] [--recursive] [--] [<path>...]\n+'git submodule' [--quiet] init [--error-unmatch] [--] [<path>...]\n+'git submodule' [--quiet] deinit [--error-unmatch] [-f|--force] (--all|[--] <path>...)\n+'git submodule' [--quiet] update [--error-unmatch] [--init] [--remote] [-N|--no-fetch]\n \t      [-f|--force] [--rebase|--merge] [--reference <repository>]\n \t      [--depth <depth>] [--recursive] [--] [<path>...]\n 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]\n \t      [commit] [--] [<path>...]\n 'git submodule' [--quiet] foreach [--recursive] <command>\n-'git submodule' [--quiet] sync [--recursive] [--] [<path>...]\n+'git submodule' [--quiet] sync [--error-unmatch] [--recursive] [--] [<path>...]\n \n \n DESCRIPTION\n@@ -260,6 +260,11 @@ OPTIONS\n \tThe name of the branch is recorded as `submodule.<name>.branch` in\n \t`.gitmodules` for `update --remote`.\n \n+--error-unmatch::\n+\tIf the pathspec included a specification that did not match,\n+\tthe usual operation is to error out. This switch suppresses\n+\terror reporting and continues the operation.\n+\n -f::\n --force::\n \tThis option is only valid for add, deinit and update commands.\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5295b72..91c49ec 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -19,7 +19,8 @@ struct module_list {\n static int module_list_compute(int argc, const char **argv,\n \t\t\t       const char *prefix,\n \t\t\t       struct pathspec *pathspec,\n-\t\t\t       struct module_list *list)\n+\t\t\t       struct module_list *list,\n+\t\t\t       int unmatch)\n {\n \tint i, result = 0;\n \tchar *ps_matched = NULL;\n@@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tconst struct cache_entry *ce = active_cache[i];\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\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 \n \t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n@@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,\n \t\t\ti++;\n \t}\n \n-\tif (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n+\tif (!unmatch &&\n+\t    ps_matched &&\n+\t    report_path_error(ps_matched, pathspec, prefix))\n \t\tresult = -1;\n \n \tfree(ps_matched);\n@@ -63,11 +65,12 @@ static int module_list_compute(int argc, const char **argv,\n \n static int module_list(int argc, const char **argv, const char *prefix)\n {\n-\tint i;\n+\tint i, unmatch = 0;\n \tstruct pathspec pathspec;\n \tstruct module_list list = MODULE_LIST_INIT;\n \n \tstruct option module_list_options[] = {\n+\t\tOPT_BOOL(0, \"unmatch\", &unmatch, N_(\"Do not report error if no matches are found\")),\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n \t\t\t   N_(\"path\"),\n \t\t\t   N_(\"alternative anchor for relative paths\")),\n@@ -82,7 +85,7 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_list_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n-\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list, unmatch) < 0) {\n \t\tprintf(\"#unmatched\\n\");\n \t\treturn 1;\n \t}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex fb68f1f..f10e10a 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -6,13 +6,13 @@\n \n dashless=$(basename \"$0\" | sed -e 's/-/ /')\n USAGE=\"[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]\n-   or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]\n-   or: $dashless [--quiet] init [--] [<path>...]\n-   or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)\n-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]\n+   or: $dashless [--quiet] status [--error-unmatch] [--cached] [--recursive] [--] [<path>...]\n+   or: $dashless [--quiet] init [--error-unmatch] [--] [<path>...]\n+   or: $dashless [--quiet] deinit [--error-unmatch] [-f|--force] (--all| [--] <path>...)\n+   or: $dashless [--quiet] update [--error-unmatch] [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference <repository>] [--recursive] [--] [<path>...]\n    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]\n    or: $dashless [--quiet] foreach [--recursive] <command>\n-   or: $dashless [--quiet] sync [--recursive] [--] [<path>...]\"\n+   or: $dashless [--quiet] sync [--error-unmatch] [--recursive] [--] [<path>...]\"\n OPTIONS_SPEC=\n SUBDIRECTORY_OK=Yes\n . git-sh-setup\n@@ -391,6 +391,9 @@ cmd_foreach()\n \t\t--recursive)\n \t\t\trecursive=1\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\t;;\n \t\t-*)\n \t\t\tusage\n \t\t\t;;\n@@ -407,7 +410,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\"|\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -453,6 +456,9 @@ cmd_init()\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -467,7 +473,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -534,6 +540,9 @@ cmd_deinit()\n \t\t--all)\n \t\t\tdeinit_all=t\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -558,7 +567,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '--all' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -661,6 +670,9 @@ cmd_update()\n \t\t--recursive)\n \t\t\trecursive=1\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\t;;\n \t\t--checkout)\n \t\t\tupdate=\"checkout\"\n \t\t\t;;\n@@ -692,7 +704,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1115,6 +1127,9 @@ cmd_status()\n \t\t--recursive)\n \t\t\trecursive=1\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -1129,7 +1144,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1193,6 +1208,10 @@ cmd_sync()\n \t\t\trecursive=1\n \t\t\tshift\n \t\t\t;;\n+\t\t--error-unmatch)\n+\t\t\tunmatch=1\n+\t\t\tshift\n+\t\t\t;;\n \t\t--)\n \t\t\tshift\n \t\t\tbreak\n@@ -1206,7 +1225,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-- \n2.8.0.2.gaa9b48a.dirty\n"},{"id":"287609","messageId":"xmqqd1o8vbc4.fsf@gitster.mtv.corp.google.com","threadId":"42407","inReplyTo":"1463793689-19496-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule operations: tighten pathspec errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-26T20:00:27Z","receivedAt":"2016-05-26T20:00:27Z","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> It's a first initial version with no tests (and probably conflicting with\n> some topics in flight), but I was curious how involved this issue actually is,\n> so I took a stab at implementing it.\n\nI take it to mean \"This is s/PATCH/RFC/\".\n\n> +--error-unmatch::\n> +\tIf the pathspec included a specification that did not match,\n> +\tthe usual operation is to error out. This switch suppresses\n> +\terror reporting and continues the operation.\n\nThe behaviour described is a total opposite of the option with the\nsame name \"ls-files\" has, no?\n\nIf there were no default, --error-unmatch would make an unmatching\npathspec an error and --no-error-unmatch would make it a non-error.\n\nIf the default is to error out, there is no need for --error-unmatch\nto exist, but you do want --no-error-unmatch aka --unmatch-ok.\n\nIf the default is not to error out, --error-unmatch should make it\nnotice and turn it into an error.\n\nI am guessing that you were debating yourself which should be the\ndefault and the patch ended up in an inconsistent state, the\ndescription assuming a more strict default, while the option name\nassuming a less strict default.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 5295b72..91c49ec 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -19,7 +19,8 @@ struct module_list {\n>  static int module_list_compute(int argc, const char **argv,\n>  \t\t\t       const char *prefix,\n>  \t\t\t       struct pathspec *pathspec,\n> -\t\t\t       struct module_list *list)\n> +\t\t\t       struct module_list *list,\n> +\t\t\t       int unmatch)\n\nWhat is \"unmatch\"?  \"Catch unmatch errors, please?\"  \"Do not check\nand report unmatch errors?\"\n\nMy cursory read of a few hunks below tells me that you meant the\nlatter, i.e. \"unmatch_ok\".\n\n> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,\n>  \n>  \tfor (i = 0; i < active_nr; i++) {\n>  \t\tconst struct cache_entry *ce = active_cache[i];\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\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\nOK, this is the crucial bit in this patch. pathspec matches are now\ndone only against gitlinks, so any unmatch is \"pattern or path\ngiven, but there was no such submodule\".\n\n> @@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,\n>  \t\t\ti++;\n>  \t}\n>  \n> -\tif (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n> +\tif (!unmatch &&\n> +\t    ps_matched &&\n> +\t    report_path_error(ps_matched, pathspec, prefix))\n>  \t\tresult = -1;\n\nIf unmatch is not true, then check if ps_matched records \"aw, this\npathspec element did not get used\" and complain.  If unmatch is\ntrue, we do not do that.\n\nWhich confirms my earlier \"'unmatch' here means 'unmatch_ok'\".\n\nIt is tempting to update report_path_error() return \"OK\" when its\nfirst parameter is NULL.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index fb68f1f..f10e10a 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -391,6 +391,9 @@ cmd_foreach()\n>  \t\t--recursive)\n>  \t\t\trecursive=1\n>  \t\t\t;;\n> +\t\t--error-unmatch)\n> +\t\t\tunmatch=1\n> +\t\t\t;;\n\nSo \"--error-unmatch\" does pass \"--unmatch\" which is \"please ignore\nunmatch errors\".  That is a bit strange (see above).\n\n> @@ -407,7 +410,7 @@ cmd_foreach()\n>  \t# command in the subshell (and a recursive call to this function)\n>  \texec 3<&0\n>  \n> -\tgit submodule--helper list --prefix \"$wt_prefix\"|\n> +\tgit submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\"|\n\nFor this to work, somebody must ensure that the variable unmatch is\neither unset or set to empty unless the user gave --error-unmatch to\nus.  There is a block of empty assignments hear the beginning of\nthis file for that very purpose, i.e. resetting a stray environment\nvariable that could be in user's environment.\n\nThe patch itself does not look too bad.  I do not have an opinion on\nwhich one should be the default, and I certainly would understand if\nyou want to keep the default loose (i.e. not complaining) with an\noptional error checking, but whichever default you choose, the\noption and variable names need to be clarified to avoid confusion.\n"},{"id":"288085","messageId":"CAGZ79kbdfEJ1iSpOJ=HfHP=EvVxB9Sv+5Zk+goLSOJphh8ZZ+w@mail.gmail.com","threadId":"42407","inReplyTo":"xmqqd1o8vbc4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule operations: tighten pathspec errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-01T20:55:09Z","receivedAt":"2016-06-01T20:55:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> It's a first initial version with no tests (and probably conflicting with\n>> some topics in flight), but I was curious how involved this issue actually is,\n>> so I took a stab at implementing it.\n>\n> I take it to mean \"This is s/PATCH/RFC/\".\n>\n>> +--error-unmatch::\n>> +     If the pathspec included a specification that did not match,\n>> +     the usual operation is to error out. This switch suppresses\n>> +     error reporting and continues the operation.\n>\n> The behaviour described is a total opposite of the option with the\n> same name \"ls-files\" has, no?\n>\n> If there were no default, --error-unmatch would make an unmatching\n> pathspec an error and --no-error-unmatch would make it a non-error.\n>\n> If the default is to error out, there is no need for --error-unmatch\n> to exist, but you do want --no-error-unmatch aka --unmatch-ok.\n>\n> If the default is not to error out, --error-unmatch should make it\n> notice and turn it into an error.\n>\n> I am guessing that you were debating yourself which should be the\n> default and the patch ended up in an inconsistent state, the\n> description assuming a more strict default, while the option name\n> assuming a less strict default.\n\nYes.\n\n>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 5295b72..91c49ec 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -19,7 +19,8 @@ struct module_list {\n>>  static int module_list_compute(int argc, const char **argv,\n>>                              const char *prefix,\n>>                              struct pathspec *pathspec,\n>> -                            struct module_list *list)\n>> +                            struct module_list *list,\n>> +                            int unmatch)\n>\n> What is \"unmatch\"?  \"Catch unmatch errors, please?\"  \"Do not check\n> and report unmatch errors?\"\n>\n> My cursory read of a few hunks below tells me that you meant the\n> latter, i.e. \"unmatch_ok\".\n>\n>> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,\n>>\n>>       for (i = 0; i < active_nr; i++) {\n>>               const struct cache_entry *ce = active_cache[i];\n>> -\n>> -             if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>> -                                 0, ps_matched, 1) ||\n>> -                 !S_ISGITLINK(ce->ce_mode))\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> OK, this is the crucial bit in this patch. pathspec matches are now\n> done only against gitlinks, so any unmatch is \"pattern or path\n> given, but there was no such submodule\".\n\nright.\n\n>\n>> @@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,\n>>                       i++;\n>>       }\n>>\n>> -     if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n>> +     if (!unmatch &&\n>> +         ps_matched &&\n>> +         report_path_error(ps_matched, pathspec, prefix))\n>>               result = -1;\n>\n> If unmatch is not true, then check if ps_matched records \"aw, this\n> pathspec element did not get used\" and complain.  If unmatch is\n> true, we do not do that.\n>\n> Which confirms my earlier \"'unmatch' here means 'unmatch_ok'\".\n>\n> It is tempting to update report_path_error() return \"OK\" when its\n> first parameter is NULL.\n\nsuch that we can do a\n\n    if (report_path_error(unmatch_ok ? NULL : ps_matched, pathspec, prefix)))\n        result = -1;\n\nThat looks good and inside of report_path_error we would only need a\n\n    if (!ps_matched)\n        return 0;\n\nat the beginning.\n\n>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index fb68f1f..f10e10a 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -391,6 +391,9 @@ cmd_foreach()\n>>               --recursive)\n>>                       recursive=1\n>>                       ;;\n>> +             --error-unmatch)\n>> +                     unmatch=1\n>> +                     ;;\n>\n> So \"--error-unmatch\" does pass \"--unmatch\" which is \"please ignore\n> unmatch errors\".  That is a bit strange (see above).\n>\n>> @@ -407,7 +410,7 @@ cmd_foreach()\n>>       # command in the subshell (and a recursive call to this function)\n>>       exec 3<&0\n>>\n>> -     git submodule--helper list --prefix \"$wt_prefix\"|\n>> +     git submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\"|\n>\n> For this to work, somebody must ensure that the variable unmatch is\n> either unset or set to empty unless the user gave --error-unmatch to\n> us.  There is a block of empty assignments hear the beginning of\n> this file for that very purpose, i.e. resetting a stray environment\n> variable that could be in user's environment.\n>\n> The patch itself does not look too bad.  I do not have an opinion on\n> which one should be the default, and I certainly would understand if\n> you want to keep the default loose (i.e. not complaining) with an\n> optional error checking, but whichever default you choose, the\n> option and variable names need to be clarified to avoid confusion.\n\nOk I'll fix the variable names; I think for consistency with e.g.\nls-files --error-unmatch\nwe would want to be loose by default and strict on that option.\n\nThanks,\nStefan\n\n>\n"},{"id":"288088","messageId":"xmqqy46owr0n.fsf@gitster.mtv.corp.google.com","threadId":"42407","inReplyTo":"CAGZ79kbdfEJ1iSpOJ=HfHP=EvVxB9Sv+5Zk+goLSOJphh8ZZ+w@mail.gmail.com","subject":"Re: [PATCH] submodule operations: tighten pathspec errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T21:14:32Z","receivedAt":"2016-06-01T21:14:32Z","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> On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>>> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,\n>>>\n>>>       for (i = 0; i < active_nr; i++) {\n>>>               const struct cache_entry *ce = active_cache[i];\n>>> -\n>>> -             if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>>> -                                 0, ps_matched, 1) ||\n>>> -                 !S_ISGITLINK(ce->ce_mode))\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>> OK, this is the crucial bit in this patch. pathspec matches are now\n>> done only against gitlinks, so any unmatch is \"pattern or path\n>> given, but there was no such submodule\".\n>\n> right.\n\nThat changes the semantics, and its user visible effect may deserve\nto be in the documentation, no?\n\nIt used to be that \"git submodule--helper list COPYING\" did not\ncomplain, but with this change, it would.  Which may be a good\nchange, but \"git submodule--helper list sub*\" where most but not all\nof glob expansion done by the shell are submodule directories may be\na common thing people may do, and it may be irritating to see the\nunmatch complaints.  I dunno.\n\nWhen we know we are not going to complain (i.e. --unmatch-ok option\nis given from the command line), I think it is perfectly fine (and\nit is even preferrable) to swap the order of the check.  The mode\ncheck done with S_ISGITLINK() is much cheaper and it is likely to\nyield true much less often, which in turn would allow us to make\nfewer calls to more expensive match_pathspec().\n\nBut when we want to diagnose typo (i.e. --unmatch-ok was not given),\nwe may want to preserve the current order, so that the \"sub*\"\nexample in a few paragraphs ago would not irritate the users.\n\n>> It is tempting to update report_path_error() return \"OK\" when its\n>> first parameter is NULL.\n>\n> such that we can do a\n>\n>     if (report_path_error(unmatch_ok ? NULL : ps_matched, pathspec, prefix)))\n>         result = -1;\n\nI actually was thinking about setting ps_matched to NULL so that\nboth match_pathspec() and report_path_error() would get NULL.\nmatch_pathspec() has to do _more_ work when ps_matched[] aka seen[]\nis given.\n\n>>> @@ -407,7 +410,7 @@ cmd_foreach()\n>>>       # command in the subshell (and a recursive call to this function)\n>>>       exec 3<&0\n>>>\n>>> -     git submodule--helper list --prefix \"$wt_prefix\"|\n>>> +     git submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\"|\n>>\n>> For this to work, somebody must ensure that the variable unmatch is\n>> either unset or set to empty unless the user gave --error-unmatch to\n>> us.  There is a block of empty assignments hear the beginning of\n>> this file for that very purpose, i.e. resetting a stray environment\n>> variable that could be in user's environment.\n>>\n>> The patch itself does not look too bad.  I do not have an opinion on\n>> which one should be the default, and I certainly would understand if\n>> you want to keep the default loose (i.e. not complaining) with an\n>> optional error checking, but whichever default you choose, the\n>> option and variable names need to be clarified to avoid confusion.\n>\n> Ok I'll fix the variable names; I think for consistency with e.g.\n> ls-files --error-unmatch\n> we would want to be loose by default and strict on that option.\n\nI do not think the \"typo-prevention\" safety measure should suddenly\nbe turned into opt-in; I'd suggest \"--unmatch-ok\" instead.\n"},{"id":"288089","messageId":"xmqqtwhcwqqh.fsf@gitster.mtv.corp.google.com","threadId":"42407","inReplyTo":"xmqqy46owr0n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule operations: tighten pathspec errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-01T21:20:38Z","receivedAt":"2016-06-01T21:20:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Ok I'll fix the variable names; I think for consistency with e.g.\n>> ls-files --error-unmatch\n>> we would want to be loose by default and strict on that option.\n>\n> I do not think the \"typo-prevention\" safety measure should suddenly\n> be turned into opt-in; I'd suggest \"--unmatch-ok\" instead.\n\nSorry, I should have grepped before hitting \"send\".  I think this is\nin line with \"git rm --ignore-unmatch\".  We complain by default, but\nthe user can choose to squelch.\n"},{"id":"288476","messageId":"CAGZ79kY+K4-hbzL-KGgsyTHvM0BoXBLawvj2GmWJ9tbTuiSGbg@mail.gmail.com","threadId":"42407","inReplyTo":"xmqqy46owr0n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule operations: tighten pathspec errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-06T19:31:22Z","receivedAt":"2016-06-06T19:31:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 1, 2016 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Thu, May 26, 2016 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>>> @@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,\n>>>>\n>>>>       for (i = 0; i < active_nr; i++) {\n>>>>               const struct cache_entry *ce = active_cache[i];\n>>>> -\n>>>> -             if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>>>> -                                 0, ps_matched, 1) ||\n>>>> -                 !S_ISGITLINK(ce->ce_mode))\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>>> OK, this is the crucial bit in this patch. pathspec matches are now\n>>> done only against gitlinks, so any unmatch is \"pattern or path\n>>> given, but there was no such submodule\".\n>>\n>> right.\n>\n> That changes the semantics, and its user visible effect may deserve\n> to be in the documentation, no?\n>\n> It used to be that \"git submodule--helper list COPYING\" did not\n> complain, but with this change, it would.  Which may be a good\n> change, but \"git submodule--helper list sub*\" where most but not all\n> of glob expansion done by the shell are submodule directories may be\n> a common thing people may do, and it may be irritating to see the\n> unmatch complaints.  I dunno.\n\nI fixed all the variable names and was confident with what I had,\nbut this is an issue indeed as the error message is plain wrong:\n    git$ git submodule--helper list sub*\n    error: pathspec 'submodule.c' did not match any file(s) known to git.\n    error: pathspec 'submodule-config.c' did not match any file(s) known to git.\n    error: pathspec 'submodule-config.h' did not match any file(s) known to git.\n    error: pathspec 'submodule-config.o' did not match any file(s) known to git.\n    error: pathspec 'submodule.h' did not match any file(s) known to git.\n    error: pathspec 'submodule_multiple_references' did not match any\nfile(s) known to git.\n    error: pathspec 'submodule.o' did not match any file(s) known to git.\n    error: pathspec 'submodulespec.o' did not match any file(s) known to git.\n\n(I realize I have some stray object files laying around)\n\nSo I do not think we can do\n\n-               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n-                                   0, ps_matched, 1) ||\n-                   !S_ISGITLINK(ce->ce_mode))\n+               if (!S_ISGITLINK(ce->ce_mode) ||\n+                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n+                                   0, ps_matched, 1))\n\nor we need to have custom error reporting, which checks if any file\nstill matches the pathspec (and ignore non gitlinks)\n\n>\n> When we know we are not going to complain (i.e. --unmatch-ok option\n\nI'd rather go with ignore-unmatch as git-rm does use that.\n\n> is given from the command line), I think it is perfectly fine (and\n> it is even preferrable) to swap the order of the check.  The mode\n> check done with S_ISGITLINK() is much cheaper and it is likely to\n> yield true much less often, which in turn would allow us to make\n> fewer calls to more expensive match_pathspec().\n\nAs said, in that case we would then need a\n    pathspec_mark_ps_matched(ce->name, ce_namelen(ce), ps_matched)\n\n>\n> But when we want to diagnose typo (i.e. --unmatch-ok was not given),\n> we may want to preserve the current order, so that the \"sub*\"\n> example in a few paragraphs ago would not irritate the users.\n\nI see.\n\n     if (!S_ISGITLINK(ce->ce_mode)) {\n        pathspec_mark_ps_matched(...);\n        continue;\n    } else {\n        if ( !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n                                     0, ps_matched, 1))\n        continue;\n    }\n    ...\n\ndoesn't look very appealing, so I guess we'd just keep the current behavior\nof\n-               if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n-                                   0, ps_matched, 1) ||\n-                   !S_ISGITLINK(ce->ce_mode))\n\n\n>\n>>> It is tempting to update report_path_error() return \"OK\" when its\n>>> first parameter is NULL.\n>>\n>> such that we can do a\n>>\n>>     if (report_path_error(unmatch_ok ? NULL : ps_matched, pathspec, prefix)))\n>>         result = -1;\n>\n> I actually was thinking about setting ps_matched to NULL so that\n> both match_pathspec() and report_path_error() would get NULL.\n> match_pathspec() has to do _more_ work when ps_matched[] aka seen[]\n> is given.\n\nYes for one call; No from the birds eye view;\nthe first lines that use the seen[]:\n\n    if (seen && seen[i] == MATCHED_EXACTLY)\n        continue;\n\nso if we have  `ps->nr > 0` then it is beneficial to have a\nseen array, I think?\n\n>\n>>>> @@ -407,7 +410,7 @@ cmd_foreach()\n>>>>       # command in the subshell (and a recursive call to this function)\n>>>>       exec 3<&0\n>>>>\n>>>> -     git submodule--helper list --prefix \"$wt_prefix\"|\n>>>> +     git submodule--helper list ${unmatch:+--unmatch} --prefix \"$wt_prefix\"|\n>>>\n>>> For this to work, somebody must ensure that the variable unmatch is\n>>> either unset or set to empty unless the user gave --error-unmatch to\n>>> us.  There is a block of empty assignments hear the beginning of\n>>> this file for that very purpose, i.e. resetting a stray environment\n>>> variable that could be in user's environment.\n\ndone.\n\n>>>\n>>> The patch itself does not look too bad.  I do not have an opinion on\n>>> which one should be the default, and I certainly would understand if\n>>> you want to keep the default loose (i.e. not complaining) with an\n>>> optional error checking, but whichever default you choose, the\n>>> option and variable names need to be clarified to avoid confusion.\n>>\n>> Ok I'll fix the variable names; I think for consistency with e.g.\n>> ls-files --error-unmatch\n>> we would want to be loose by default and strict on that option.\n>\n> I do not think the \"typo-prevention\" safety measure should suddenly\n> be turned into opt-in; I'd suggest \"--unmatch-ok\" instead.\n\n--ignore-unmatch has the same meaning and is taken by git-rm already?\n"}]}