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

[Stefan Beller <sbeller@google.com>]

On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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?

Yeah it is moving around the problem a bit. And the code churn is
unfortunate. Though when I was reading the documentation on
pathspecs, literally the first sentence was "Do not use get_pathspec,
it is out dated". And that was a sad taste for reading documentation.

It's ok to have such warnings in the docs, but as the first sentence as if
there was nothing more important than avoiding the out dated stuff? I
mean I want to understand the actual code and how I can use it, right?

And there are different approaches to solving the problem.
I could have just reworded or even just rearranged the documentation.

The approach I take here includes a bit of code churn, but it moves the
problematic pieces all in one spot.

>
>> 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