Re: [PATCH 01/17] mv: convert to using pathspec struct interface

From: Duy Nguyen <pclouds@gmail.com>
Date: Dec 7, 2016, 10:07 UTC
Message-ID: <CACsJy8Bm+Nif_PL1=BzTYLKGrnz94rjKOB=_PXz6OB47Js6v2A@mail.gmail.com>
In-Reply-To: <1481061106-117775-2-git-send-email-bmwill@google.com>

On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:

Show 14 quoted lines

> Convert the 'internal_copy_pathspec()' function to use the pathspec
> struct interface from using the deprecated 'get_pathspec()' interface.
>
> In addition to this, fix a memory leak caused by only duplicating some
> of the pathspec elements.  Instead always duplicate all of the the
> pathspec elements as an intermediate step (with modificationed based on
> the passed in flags).  This way the intermediate strings can then be
> freed prior to duplicating the result of parse_pathspec (which contains
> each of the elements with the prefix prepended).
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)

Stefan did something similar last year [1] but I couldn't find why it did not get merged. Not sure if the reasons are still relevant or not...

[1] http://public-inbox.org/git/%3C1438885632-26470-1-git-send-email-sbeller@google.com%3E/

Show 61 quoted lines

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 2f43877..4df4a12 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2006 Johannes Schindelin
>   */
>  #include "builtin.h"
> +#include "pathspec.h"
>  #include "lockfile.h"
>  #include "dir.h"
>  #include "cache-tree.h"
> @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
>  {
>         int i;
>         const char **result;
> +       struct pathspec ps;
>         ALLOC_ARRAY(result, count + 1);
> -       COPY_ARRAY(result, pathspec, count);
> -       result[count] = NULL;
> +
> +       /* Create an intermediate copy of the pathspec based on the flags */
>         for (i = 0; i < count; i++) {
> -               int length = strlen(result[i]);
> +               int length = strlen(pathspec[i]);
>                 int to_copy = length;
> +               char *it;
>                 while (!(flags & KEEP_TRAILING_SLASH) &&
> -                      to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
> +                      to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
>                         to_copy--;
> -               if (to_copy != length || flags & DUP_BASENAME) {
> -                       char *it = xmemdupz(result[i], to_copy);
> -                       if (flags & DUP_BASENAME) {
> -                               result[i] = xstrdup(basename(it));
> -                               free(it);
> -                       } else
> -                               result[i] = it;
> -               }
> +
> +               it = xmemdupz(pathspec[i], to_copy);
> +               if (flags & DUP_BASENAME) {
> +                       result[i] = xstrdup(basename(it));
> +                       free(it);
> +               } else
> +                       result[i] = it;
> +       }
> +       result[count] = NULL;
> +
> +       parse_pathspec(&ps,
> +                      PATHSPEC_ALL_MAGIC &
> +                      ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> +                      PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> +                      prefix, result);
> +       assert(count == ps.nr);
> +
> +       /* Copy the pathspec and free the old intermediate strings */
> +       for (i = 0; i < count; i++) {
> +               const char *match = xstrdup(ps.items[i].match);
> +               free((char *) result[i]);
> +               result[i] = match;

Sigh.. it looks so weird that we do all the parsing (in a _copy_ pathspec function) then remove struct pathspec and return the plain string. I guess we can't do anything more until we rework cmd_mv code to handle pathspec natively.

At the least I think we should rename this function to something else. But if you have time I really wish we could kill this function. I haven't stared at cmd_mv() long and hard, but it looks to me that we combining two separate functionalities in the same function here.

If "mv" takes n arguments, then the first <n-1> arguments may be pathspec, the last one is always a plain path. The "dest_path = internal_copy_pathspec..." could be as simple as "dest_path = prefix_path(argv[argc - 1])". the special treatment for this last argument [1] can live here. Then, we can do parse_pathspec for the <n-1> arguments in cmd_mv(). It's still far from perfect, because cmd_mv can't handle pathspec properly, but it reduces the messy mess in internal_copy_pathspec a bit, I hope.

[1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)

Show 11 quoted lines

>         }
> -       return get_pathspec(prefix, result);
> +
> +       clear_pathspec(&ps);
> +       return result;
>  }
>
>  static const char *add_slash(const char *path)
> --
> 2.8.0.rc3.226.g39d4020
>

-- 
Duy