Re: [PATCH 5/7] worktree.c: add clear_worktree()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 11, 2016, 06:36 UTC
Message-ID: <CAPig+cS-tb_hD3k2XsTioJrkzaB3FxSnrxM6JMq5-og1ADnoRg@mail.gmail.com>
In-Reply-To: <20160510141553.22967-5-pclouds@gmail.com>

On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> The use case is keep some worktree and discard the rest of the worktree
> list.

So, you're saying that rather than a client freeing the entire worktree list like this:

    free_worktrees(worktrees);

it might instead want to keep some element ('n') and free all others plus the list itself, like this:

    struct worktree *keep = worktrees[n];
    struct worktree **p = worktrees;
    for (; *p; p++)
        if (*p != keep)
            clear_worktree(*p);
    free(worktrees);

Is that correct?

If so, then doesn't this require the client to have far too intimate knowledge of the internals of free_worktrees(). Or, was your idea that the client would simply leak the 'worktrees' array after freeing the unwanted elements?

In either case, a cleaner approach might be to provide a function for copying a worktree element. Perhaps:

    struct worktree *copy_worktree(const struct worktree *);

or something?

Show 50 quoted lines

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> index f4a4f38..335c58f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -5,14 +5,22 @@
>  #include "dir.h"
>  #include "wt-status.h"
>
> +void clear_worktree(struct worktree *wt)
> +{
> +       if (!wt)
> +               return;
> +       free(wt->path);
> +       free(wt->id);
> +       free(wt->head_ref);
> +       memset(wt, 0, sizeof(*wt));
> +}
> +
>  void free_worktrees(struct worktree **worktrees)
>  {
>         int i = 0;
>
>         for (i = 0; worktrees[i]; i++) {
> -               free(worktrees[i]->path);
> -               free(worktrees[i]->id);
> -               free(worktrees[i]->head_ref);
> +               clear_worktree(worktrees[i]);
>                 free(worktrees[i]);
>         }
>         free (worktrees);
> diff --git a/worktree.h b/worktree.h
> index 1394909..7430a4e 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -29,6 +29,11 @@ extern struct worktree **get_worktrees(void);
>   */
>  extern const char *get_worktree_git_dir(const struct worktree *wt);
>
> +/*
> + * Free up the memory for worktree
> + */
> +extern void clear_worktree(struct worktree *);
> +
>  /*
>   * Free up the memory for worktree(s)
>   */
> --
> 2.8.2.524.g6ff3d78