Re: [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 18, 2021, 02:57 UTC
Message-ID: <CAPig+cQX27n6vpGP4m3S9paUnpNw5etLLAXr=WO9rC1KSSLMmQ@mail.gmail.com>
In-Reply-To: <20210117234244.95106-3-rafaeloliveira.cs@gmail.com>

On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:

Show 14 quoted lines

> Add worktree_prune_reason() to allow a caller to discover whether a
> worktree is prunable and the reason that it is, much like
> worktree_lock_reason() indicates whether a worktree is locked and the
> reason for the lock. As with worktree_lock_reason(), retrieve the
> prunable reason lazily and cache it in the `worktree` structure.
>
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
> +{
> +       struct strbuf reason = STRBUF_INIT;
> +       char *path;

The `path` variable is uninitialized...

> +       if (should_prune_worktree(wt->id, &reason, &path, expire))
> +               wt->prune_reason = strbuf_detach(&reason, NULL);

...but the very first thing should_prune_worktree() does is unconditionally set it to NULL, so it's either NULL or an allocated string regardless of the value returned by should_prune_worktree()...

> +       strbuf_release(&reason);
> +       free(path);

...which makes the behavior of `free(path)` deterministic. Good.

I'm on the fence about whether or not we should initialize `path` to NULL:

    char *path = NULL;

just to make it easier for the next person to reason about it without having to dig into the implementation of should_prune_worktree(). This is a really minor point, though, not worth a re-roll.