Re: [PATCH 1/2] builtin/mv: remove get_pathspec()

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:
>  builtin/mv: remove get_pathspec()

Misleading. Perhaps rephrase as:

    mv: drop dependency upon deprecated get_pathspec

> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so
> reimplement `get_pathspec` literally in builtin/mv.c

Curious. Since this is just moving code around, rather than doing the
actual work to complete the final step as stated by the NEEDSWORK
comment, isn't it just moving the "problem" from one location to
another? Is it worth the code churn?

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/mv.c b/builtin/mv.c
> index d1d4316..99e9b3c 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -10,6 +10,7 @@
>  #include "string-list.h"
>  #include "parse-options.h"
>  #include "submodule.h"
> +#include "pathspec.h"
>
>  static const char * const builtin_mv_usage[] = {
>         N_("git mv [<options>] <source>... <destination>"),
> @@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {
>  #define KEEP_TRAILING_SLASH 2
>
>  static const char **internal_copy_pathspec(const char *prefix,
> -                                          const char **pathspec,
> +                                          const char **argv,

What is this change about? It doesn't seem to be related to anything
else in the patch or to its stated purpose, and makes the argument's
purpose less clear, so it's not obvious why it is a good change.

>                                            int count, unsigned flags)
>  {
>         int i;
> +       struct pathspec ps;
>         const char **result = xmalloc((count + 1) * sizeof(const char *));
> -       memcpy(result, pathspec, count * sizeof(const char *));
> +       memcpy(result, argv, count * sizeof(const char *));
>         result[count] = NULL;
> +
> +       /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */
>         for (i = 0; i < count; i++) {
>                 int length = strlen(result[i]);
>                 int to_copy = length;
> @@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char *prefix,
>                                 result[i] = it;
>                 }
>         }
> -       return get_pathspec(prefix, result);
> +
> +       parse_pathspec(&ps,
> +                      PATHSPEC_ALL_MAGIC &
> +                      ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> +                      PATHSPEC_PREFER_CWD,
> +                      prefix, result);
> +       return ps._raw;
>  }
>
>  static const char *add_slash(const char *path)
> --
> 2.5.0.239.g9728e1d.dirty