{"thread":{"id":"40002","subject":"[PATCH] builtin/mv: Get rid of the last caller of get_pathspec","startedAt":"2015-08-03T17:53:02Z","lastAt":"2015-08-06T20:59:48Z","messageCount":9,"participants":["Stefan Beller","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"267330","messageId":"1438624382-31248-1-git-send-email-sbeller@google.com","threadId":"40002","inReplyTo":null,"subject":"[PATCH] builtin/mv: Get rid of the last caller of get_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T17:53:02Z","receivedAt":"2015-08-03T17:53:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`get_pathspec` is deprecated and builtin/mv is its last caller, so getting\nrid of `get_pathspec` is rather easy. By getting rid of `get_pathspec`,\nthe documentation such as 'technical/api-setup.txt' becomes easier to read\nas the reader doesn't need to bear with the additional fact that\n`get_pathspec` is deprecated.\n\nThe code in 'builtin/mv' still requires some work to make it less ugly.\n\nCC: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nCC: Junio C Hamano <gitster@pobox.com>\nCC: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/technical/api-setup.txt |  2 --\n builtin/mv.c                          | 19 ++++++++++++++++---\n cache.h                               |  1 -\n pathspec.c                            | 30 ------------------------------\n 4 files changed, 16 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt\nindex 540e455..eb1fa98 100644\n--- a/Documentation/technical/api-setup.txt\n+++ b/Documentation/technical/api-setup.txt\n@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:\n \n - prefix and args come from cmd_* functions\n \n-get_pathspec() is obsolete and should never be used in new code.\n-\n parse_pathspec() helps catch unsupported features and reject them\n politely. At a lower level, different pathspec-related functions may\n not support the same set of features. Such pathspec-sensitive\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex d1d4316..b89d90a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -10,6 +10,7 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n #include \"submodule.h\"\n+#include \"pathspec.h\"\n \n static const char * const builtin_mv_usage[] = {\n \tN_(\"git mv [<options>] <source>... <destination>\"),\n@@ -20,13 +21,19 @@ static const char * const builtin_mv_usage[] = {\n #define KEEP_TRAILING_SLASH 2\n \n static const char **internal_copy_pathspec(const char *prefix,\n-\t\t\t\t\t   const char **pathspec,\n+\t\t\t\t\t   const char **argv,\n \t\t\t\t\t   int count, unsigned flags)\n {\n \tint i;\n+\tstruct pathspec ps;\n \tconst char **result = xmalloc((count + 1) * sizeof(const char *));\n-\tmemcpy(result, pathspec, count * sizeof(const char *));\n+\tmemcpy(result, argv, count * sizeof(const char *));\n \tresult[count] = NULL;\n+\n+\t/*\n+\t * NEEDSWORK: instead of preprocessing, pass the right flags to\n+\t * parse_pathspec below.\n+\t */\n \tfor (i = 0; i < count; i++) {\n \t\tint length = strlen(result[i]);\n \t\tint to_copy = length;\n@@ -42,7 +49,13 @@ static const char **internal_copy_pathspec(const char *prefix,\n \t\t\t\tresult[i] = it;\n \t\t}\n \t}\n-\treturn get_pathspec(prefix, result);\n+\n+\tparse_pathspec(&ps,\n+\t\t       PATHSPEC_ALL_MAGIC &\n+\t\t       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n+\t\t       PATHSPEC_PREFER_CWD,\n+\t\t       prefix, result);\n+\treturn ps._raw;\n }\n \n static const char *add_slash(const char *path)\ndiff --git a/cache.h b/cache.h\nindex 4f55466..d4e22e2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -452,7 +452,6 @@ extern void set_git_work_tree(const char *tree);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n-extern const char **get_pathspec(const char *prefix, const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/pathspec.c b/pathspec.c\nindex 9304ee3..b0e14e5 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -450,36 +450,6 @@ void parse_pathspec(struct pathspec *pathspec,\n \t}\n }\n \n-/*\n- * N.B. get_pathspec() is deprecated in favor of the \"struct pathspec\"\n- * based interface - see pathspec.c:parse_pathspec().\n- *\n- * Arguments:\n- *  - prefix - a path relative to the root of the working tree\n- *  - pathspec - a list of paths underneath the prefix path\n- *\n- * Iterates over pathspec, prepending each path with prefix,\n- * and return the resulting list.\n- *\n- * If pathspec is empty, return a singleton list containing prefix.\n- *\n- * If pathspec and prefix are both empty, return an empty list.\n- *\n- * This is typically used by built-in commands such as add.c, in order\n- * to normalize argv arguments provided to the built-in into a list of\n- * paths to process, all relative to the root of the working tree.\n- */\n-const char **get_pathspec(const char *prefix, const char **pathspec)\n-{\n-\tstruct pathspec ps;\n-\tparse_pathspec(&ps,\n-\t\t       PATHSPEC_ALL_MAGIC &\n-\t\t       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n-\t\t       PATHSPEC_PREFER_CWD,\n-\t\t       prefix, pathspec);\n-\treturn ps._raw;\n-}\n-\n void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n {\n \t*dst = *src;\n-- \n2.5.0.2.g6ffee06.dirty\n"},{"id":"267369","messageId":"CAPig+cT_YB=HY9dBo5eRNu4qvCqCXRwwXgpxCA8YjxagP+sHfA@mail.gmail.com","threadId":"40002","inReplyTo":"1438624382-31248-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] builtin/mv: Get rid of the last caller of get_pathspec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-03T22:48:16Z","receivedAt":"2015-08-03T22:48:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 3, 2015 at 1:53 PM, Stefan Beller <sbeller@google.com> wrote:\n> `get_pathspec` is deprecated and builtin/mv is its last caller, so getting\n> rid of `get_pathspec` is rather easy. By getting rid of `get_pathspec`,\n> the documentation such as 'technical/api-setup.txt' becomes easier to read\n> as the reader doesn't need to bear with the additional fact that\n> `get_pathspec` is deprecated.\n>\n> The code in 'builtin/mv' still requires some work to make it less ugly.\n\nPerhaps split this into two patches? One to free git-mv of\nget_pathspec() dependence, and the second to actually retire\nget_pathspec().\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n"},{"id":"267578","messageId":"1438885632-26470-1-git-send-email-sbeller@google.com","threadId":"40002","inReplyTo":"CAPig+cT_YB=HY9dBo5eRNu4qvCqCXRwwXgpxCA8YjxagP+sHfA@mail.gmail.com","subject":"[PATCH 0/2] Remove get_pathspec finally","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T18:27:10Z","receivedAt":"2015-08-06T18:27:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Remove the last caller of the get_pathspec function and the get_pathspec function\nitself.\n\nI stumbled into this as I was reading the documentation on pathspec, and the\nfirst sentence\n\n  get_pathspec() is obsolete and should never be used in new code.\n\nmade me wonder.\n\nThis replaces sb/remove-get-pathspec\n\nStefan Beller (2):\n  builtin/mv: remove get_pathspec()\n  pathspec: remove deprecated get_pathspec\n\n Documentation/technical/api-setup.txt |  2 --\n builtin/mv.c                          | 16 +++++++++++++---\n cache.h                               |  1 -\n pathspec.c                            | 36 -----------------------------------\n pathspec.h                            |  2 +-\n 5 files changed, 14 insertions(+), 43 deletions(-)\n\n-- \n2.5.0.239.g9728e1d.dirty\n"},{"id":"267579","messageId":"1438885632-26470-2-git-send-email-sbeller@google.com","threadId":"40002","inReplyTo":"1438885632-26470-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] builtin/mv: remove get_pathspec()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T18:27:11Z","receivedAt":"2015-08-06T18:27:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`get_pathspec` is deprecated and builtin/mv.c is its last caller, so\nreimplement `get_pathspec` literally in builtin/mv.c\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/mv.c | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex d1d4316..99e9b3c 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -10,6 +10,7 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n #include \"submodule.h\"\n+#include \"pathspec.h\"\n \n static const char * const builtin_mv_usage[] = {\n \tN_(\"git mv [<options>] <source>... <destination>\"),\n@@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {\n #define KEEP_TRAILING_SLASH 2\n \n static const char **internal_copy_pathspec(const char *prefix,\n-\t\t\t\t\t   const char **pathspec,\n+\t\t\t\t\t   const char **argv,\n \t\t\t\t\t   int count, unsigned flags)\n {\n \tint i;\n+\tstruct pathspec ps;\n \tconst char **result = xmalloc((count + 1) * sizeof(const char *));\n-\tmemcpy(result, pathspec, count * sizeof(const char *));\n+\tmemcpy(result, argv, count * sizeof(const char *));\n \tresult[count] = NULL;\n+\n+\t/* NEEDSWORK: Move these preprocessing steps into parse_pathspec */\n \tfor (i = 0; i < count; i++) {\n \t\tint length = strlen(result[i]);\n \t\tint to_copy = length;\n@@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char *prefix,\n \t\t\t\tresult[i] = it;\n \t\t}\n \t}\n-\treturn get_pathspec(prefix, result);\n+\n+\tparse_pathspec(&ps,\n+\t\t       PATHSPEC_ALL_MAGIC &\n+\t\t       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n+\t\t       PATHSPEC_PREFER_CWD,\n+\t\t       prefix, result);\n+\treturn ps._raw;\n }\n \n static const char *add_slash(const char *path)\n-- \n2.5.0.239.g9728e1d.dirty\n"},{"id":"267577","messageId":"1438885632-26470-3-git-send-email-sbeller@google.com","threadId":"40002","inReplyTo":"1438885632-26470-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] pathspec: remove deprecated get_pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T18:27:12Z","receivedAt":"2015-08-06T18:27:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The function `get_pathspec` is no longer used, so remove it.\n\nThe NEEDSWORK comment in pathspec.c is outdated as that happened in\n(fadf96aba, 2013-09-09, Merge branch 'nd/magic-pathspec')\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/technical/api-setup.txt |  2 --\n cache.h                               |  1 -\n pathspec.c                            | 36 -----------------------------------\n pathspec.h                            |  2 +-\n 4 files changed, 1 insertion(+), 40 deletions(-)\n\ndiff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt\nindex 540e455..eb1fa98 100644\n--- a/Documentation/technical/api-setup.txt\n+++ b/Documentation/technical/api-setup.txt\n@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:\n \n - prefix and args come from cmd_* functions\n \n-get_pathspec() is obsolete and should never be used in new code.\n-\n parse_pathspec() helps catch unsupported features and reject them\n politely. At a lower level, different pathspec-related functions may\n not support the same set of features. Such pathspec-sensitive\ndiff --git a/cache.h b/cache.h\nindex 4f55466..d4e22e2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -452,7 +452,6 @@ extern void set_git_work_tree(const char *tree);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n-extern const char **get_pathspec(const char *prefix, const char **pathspec);\n extern void setup_work_tree(void);\n extern const char *setup_git_directory_gently(int *);\n extern const char *setup_git_directory(void);\ndiff --git a/pathspec.c b/pathspec.c\nindex 9304ee3..abaec0a 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -94,12 +94,6 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n  *\n  * For now, we only parse the syntax and throw out anything other than\n  * \"top\" magic.\n- *\n- * NEEDSWORK: This needs to be rewritten when we start migrating\n- * get_pathspec() users to use the \"struct pathspec\" interface.  For\n- * example, a pathspec element may be marked as case-insensitive, but\n- * the prefix part must always match literally, and a single stupid\n- * string cannot express such a case.\n  */\n static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\t\t\tunsigned *p_short_magic,\n@@ -450,36 +444,6 @@ void parse_pathspec(struct pathspec *pathspec,\n \t}\n }\n \n-/*\n- * N.B. get_pathspec() is deprecated in favor of the \"struct pathspec\"\n- * based interface - see pathspec.c:parse_pathspec().\n- *\n- * Arguments:\n- *  - prefix - a path relative to the root of the working tree\n- *  - pathspec - a list of paths underneath the prefix path\n- *\n- * Iterates over pathspec, prepending each path with prefix,\n- * and return the resulting list.\n- *\n- * If pathspec is empty, return a singleton list containing prefix.\n- *\n- * If pathspec and prefix are both empty, return an empty list.\n- *\n- * This is typically used by built-in commands such as add.c, in order\n- * to normalize argv arguments provided to the built-in into a list of\n- * paths to process, all relative to the root of the working tree.\n- */\n-const char **get_pathspec(const char *prefix, const char **pathspec)\n-{\n-\tstruct pathspec ps;\n-\tparse_pathspec(&ps,\n-\t\t       PATHSPEC_ALL_MAGIC &\n-\t\t       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n-\t\t       PATHSPEC_PREFER_CWD,\n-\t\t       prefix, pathspec);\n-\treturn ps._raw;\n-}\n-\n void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n {\n \t*dst = *src;\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..b345df6 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -19,7 +19,7 @@\n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern satisfies GFNM_ONESTAR */\n \n struct pathspec {\n-\tconst char **_raw; /* get_pathspec() result, not freed by free_pathspec() */\n+\tconst char **_raw; /* not freed by free_pathspec() */\n \tint nr;\n \tunsigned int has_wildcard:1;\n \tunsigned int recursive:1;\n-- \n2.5.0.239.g9728e1d.dirty\n"},{"id":"267580","messageId":"CAPig+cRREzwRJV-az-p-r1E7JyDqDZ4GPvM1cQQzXXNyLjGPUQ@mail.gmail.com","threadId":"40002","inReplyTo":"1438885632-26470-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] builtin/mv: remove get_pathspec()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-06T18:46:06Z","receivedAt":"2015-08-06T18:46:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:\n>  builtin/mv: remove get_pathspec()\n\nMisleading. Perhaps rephrase as:\n\n    mv: drop dependency upon deprecated get_pathspec\n\n> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so\n> reimplement `get_pathspec` literally in builtin/mv.c\n\nCurious. Since this is just moving code around, rather than doing the\nactual work to complete the final step as stated by the NEEDSWORK\ncomment, isn't it just moving the \"problem\" from one location to\nanother? Is it worth the code churn?\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index d1d4316..99e9b3c 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -10,6 +10,7 @@\n>  #include \"string-list.h\"\n>  #include \"parse-options.h\"\n>  #include \"submodule.h\"\n> +#include \"pathspec.h\"\n>\n>  static const char * const builtin_mv_usage[] = {\n>         N_(\"git mv [<options>] <source>... <destination>\"),\n> @@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {\n>  #define KEEP_TRAILING_SLASH 2\n>\n>  static const char **internal_copy_pathspec(const char *prefix,\n> -                                          const char **pathspec,\n> +                                          const char **argv,\n\nWhat is this change about? It doesn't seem to be related to anything\nelse in the patch or to its stated purpose, and makes the argument's\npurpose less clear, so it's not obvious why it is a good change.\n\n>                                            int count, unsigned flags)\n>  {\n>         int i;\n> +       struct pathspec ps;\n>         const char **result = xmalloc((count + 1) * sizeof(const char *));\n> -       memcpy(result, pathspec, count * sizeof(const char *));\n> +       memcpy(result, argv, count * sizeof(const char *));\n>         result[count] = NULL;\n> +\n> +       /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */\n>         for (i = 0; i < count; i++) {\n>                 int length = strlen(result[i]);\n>                 int to_copy = length;\n> @@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char *prefix,\n>                                 result[i] = it;\n>                 }\n>         }\n> -       return get_pathspec(prefix, result);\n> +\n> +       parse_pathspec(&ps,\n> +                      PATHSPEC_ALL_MAGIC &\n> +                      ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n> +                      PATHSPEC_PREFER_CWD,\n> +                      prefix, result);\n> +       return ps._raw;\n>  }\n>\n>  static const char *add_slash(const char *path)\n> --\n> 2.5.0.239.g9728e1d.dirty\n"},{"id":"267582","messageId":"CAGZ79kZ7oS+AQgq91WRaFTCKGUnZj-cZB1YPkjQo9KAueqEN3w@mail.gmail.com","threadId":"40002","inReplyTo":"CAPig+cRREzwRJV-az-p-r1E7JyDqDZ4GPvM1cQQzXXNyLjGPUQ@mail.gmail.com","subject":"Re: [PATCH 1/2] builtin/mv: remove get_pathspec()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T18:58:41Z","receivedAt":"2015-08-06T18:58:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:\n>>  builtin/mv: remove get_pathspec()\n>\n> Misleading. Perhaps rephrase as:\n>\n>     mv: drop dependency upon deprecated get_pathspec\n>\n>> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so\n>> reimplement `get_pathspec` literally in builtin/mv.c\n>\n> Curious. Since this is just moving code around, rather than doing the\n> actual work to complete the final step as stated by the NEEDSWORK\n> comment, isn't it just moving the \"problem\" from one location to\n> another? Is it worth the code churn?\n\nYeah it is moving around the problem a bit. And the code churn is\nunfortunate. Though when I was reading the documentation on\npathspecs, literally the first sentence was \"Do not use get_pathspec,\nit is out dated\". And that was a sad taste for reading documentation.\n\nIt's ok to have such warnings in the docs, but as the first sentence as if\nthere was nothing more important than avoiding the out dated stuff? I\nmean I want to understand the actual code and how I can use it, right?\n\nAnd there are different approaches to solving the problem.\nI could have just reworded or even just rearranged the documentation.\n\nThe approach I take here includes a bit of code churn, but it moves the\nproblematic pieces all in one spot.\n\n>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>> diff --git a/builtin/mv.c b/builtin/mv.c\n>> index d1d4316..99e9b3c 100644\n>> --- a/builtin/mv.c\n>> +++ b/builtin/mv.c\n>> @@ -10,6 +10,7 @@\n>>  #include \"string-list.h\"\n>>  #include \"parse-options.h\"\n>>  #include \"submodule.h\"\n>> +#include \"pathspec.h\"\n>>\n>>  static const char * const builtin_mv_usage[] = {\n>>         N_(\"git mv [<options>] <source>... <destination>\"),\n>> @@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {\n>>  #define KEEP_TRAILING_SLASH 2\n>>\n>>  static const char **internal_copy_pathspec(const char *prefix,\n>> -                                          const char **pathspec,\n>> +                                          const char **argv,\n>\n> What is this change about? It doesn't seem to be related to anything\n> else in the patch or to its stated purpose, and makes the argument's\n> purpose less clear, so it's not obvious why it is a good change.\n>\n>>                                            int count, unsigned flags)\n>>  {\n>>         int i;\n>> +       struct pathspec ps;\n>>         const char **result = xmalloc((count + 1) * sizeof(const char *));\n>> -       memcpy(result, pathspec, count * sizeof(const char *));\n>> +       memcpy(result, argv, count * sizeof(const char *));\n>>         result[count] = NULL;\n>> +\n>> +       /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */\n>>         for (i = 0; i < count; i++) {\n>>                 int length = strlen(result[i]);\n>>                 int to_copy = length;\n>> @@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char *prefix,\n>>                                 result[i] = it;\n>>                 }\n>>         }\n>> -       return get_pathspec(prefix, result);\n>> +\n>> +       parse_pathspec(&ps,\n>> +                      PATHSPEC_ALL_MAGIC &\n>> +                      ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),\n>> +                      PATHSPEC_PREFER_CWD,\n>> +                      prefix, result);\n>> +       return ps._raw;\n>>  }\n>>\n>>  static const char *add_slash(const char *path)\n>> --\n>> 2.5.0.239.g9728e1d.dirty\n"},{"id":"267587","messageId":"CAPig+cTn=KLryVJEz7_u7nt2AzB1zzc5y40euk=oP3ZWaaYF3A@mail.gmail.com","threadId":"40002","inReplyTo":"CAGZ79kZ7oS+AQgq91WRaFTCKGUnZj-cZB1YPkjQo9KAueqEN3w@mail.gmail.com","subject":"Re: [PATCH 1/2] builtin/mv: remove get_pathspec()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-06T20:18:34Z","receivedAt":"2015-08-06T20:18:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 6, 2015 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:\n> On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:\n>>> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so\n>>> reimplement `get_pathspec` literally in builtin/mv.c\n>>\n>> Curious. Since this is just moving code around, rather than doing the\n>> actual work to complete the final step as stated by the NEEDSWORK\n>> comment, isn't it just moving the \"problem\" from one location to\n>> another? Is it worth the code churn?\n>\n> Yeah it is moving around the problem a bit. And the code churn is\n> unfortunate. Though when I was reading the documentation on\n> pathspecs, literally the first sentence was \"Do not use get_pathspec,\n> it is out dated\". And that was a sad taste for reading documentation.\n\nBy loudly warning you about deprecation and, more importantly,\npointing you at the accepted alternative, this documentation saves you\nfrom wasting time (both time spent reading and time spent going down a\ndead-end path). It would be a \"sad taste\" if it warned you quietly\nonly at the end of the documentation or not at all.\n\n> It's ok to have such warnings in the docs, but as the first sentence as if\n> there was nothing more important than avoiding the out dated stuff?\n\n>From a documentation standpoint, there is nothing more important than\nwarning you to avoid it since it is outdated and likely to go away in\nthe future.\n\n> I mean I want to understand the actual code and how I can use it, right?\n\nNo. It's deprecated and not meant for your use.\n\nIt's a different matter if you want to understand what the function\ndoes because you've encountered a call in existing code, but that case\nis covered by the existing documentation still being intact (that is,\nit wasn't removed when the deprecation notice was added).\n\n> And there are different approaches to solving the problem.\n> I could have just reworded or even just rearranged the documentation.\n\nThe documentation seems fine as-is.\n\n> The approach I take here includes a bit of code churn, but it moves the\n> problematic pieces all in one spot.\n\nIndeed, I had the \"localizing the problem to one spot\" argument in\nmind, and even wrote it as an answer to my own question, but deleted\nit before hitting \"Send\". The counterargument (aside from code churn)\nis, that by leaving it alone, it serves as a good reminder of the\n\"problem\" and is more likely to get noticed and (perhaps) fixed by\nsomeone than if it is hidden away in builtin/mv.c.\n"},{"id":"267589","messageId":"CAGZ79ka=Zw3DmvQHwQXCAd1rekR4AJSG8CDDu9thUmvDppVUZA@mail.gmail.com","threadId":"40002","inReplyTo":"CAPig+cTn=KLryVJEz7_u7nt2AzB1zzc5y40euk=oP3ZWaaYF3A@mail.gmail.com","subject":"Re: [PATCH 1/2] builtin/mv: remove get_pathspec()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T20:59:48Z","receivedAt":"2015-08-06T20:59:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Aug 6, 2015 at 1:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Aug 6, 2015 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:\n>> On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:\n>>>> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so\n>>>> reimplement `get_pathspec` literally in builtin/mv.c\n>>>\n>>> Curious. Since this is just moving code around, rather than doing the\n>>> actual work to complete the final step as stated by the NEEDSWORK\n>>> comment, isn't it just moving the \"problem\" from one location to\n>>> another? Is it worth the code churn?\n>>\n>> Yeah it is moving around the problem a bit. And the code churn is\n>> unfortunate. Though when I was reading the documentation on\n>> pathspecs, literally the first sentence was \"Do not use get_pathspec,\n>> it is out dated\". And that was a sad taste for reading documentation.\n>\n> By loudly warning you about deprecation and, more importantly,\n> pointing you at the accepted alternative, this documentation saves you\n> from wasting time (both time spent reading and time spent going down a\n> dead-end path). It would be a \"sad taste\" if it warned you quietly\n> only at the end of the documentation or not at all.\n>\n>> It's ok to have such warnings in the docs, but as the first sentence as if\n>> there was nothing more important than avoiding the out dated stuff?\n>\n> From a documentation standpoint, there is nothing more important than\n> warning you to avoid it since it is outdated and likely to go away in\n> the future.\n>\n>> I mean I want to understand the actual code and how I can use it, right?\n>\n> No. It's deprecated and not meant for your use.\n\nI need to be more precise:\nI mean I want to learn how to deal with <pathspec> things in code.\nAssume I know nothing of the history of that code and its deprecated\nearlier attempts. So I would expect a documentation like:\n\n    foo-bar(int arg): bars the foos with the specific thing equal `arg`\n\n    foobaz(int a, int b, int c): Note: this is outdated, bars the foos with ...\n\nin that order, so if I were to start reading from top to bottom, I would find\nthe current version first and could jump off starting writing code based on\nfoo-bar.\n\nIf I was reading code and wonder: \"What does foobaz exactly do? Oh\nI know! I'll consult the documentation\", then I would find the note about\nit being out dated and preferably its suggested new replacement.\n\nHowever I perceived the documentation I read more like this:\n\n    some words on foo-bar, a bit too shallow (a good overview,\n    but I'd like to know more details, so keep reading)\n\n    note on foobaz being outdated\n\n    more words on foo-bar\n\n>\n> It's a different matter if you want to understand what the function\n> does because you've encountered a call in existing code, but that case\n> is covered by the existing documentation still being intact (that is,\n> it wasn't removed when the deprecation notice was added).\n>\n>> And there are different approaches to solving the problem.\n>> I could have just reworded or even just rearranged the documentation.\n>\n> The documentation seems fine as-is.\n>\n>> The approach I take here includes a bit of code churn, but it moves the\n>> problematic pieces all in one spot.\n>\n> Indeed, I had the \"localizing the problem to one spot\" argument in\n> mind, and even wrote it as an answer to my own question, but deleted\n> it before hitting \"Send\". The counterargument (aside from code churn)\n> is, that by leaving it alone, it serves as a good reminder of the\n> \"problem\" and is more likely to get noticed and (perhaps) fixed by\n> someone than if it is hidden away in builtin/mv.c.\n\nHaving code just in one ugly spot however helps people in the other areas,\nwhere they can see the shiny new thing in action and don't have the mental\nburden of seeing a NEEDSWORK comment and thinking about that instead\nof the feature they want to add there.\n"}]}