Re: [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c

From: Eric Sunshine <sunshine@sunshineco.com>
To: Rafael Silva <rafaeloliveira.cs@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c
Date: Wed, 6 Jan 2021 00:58:54 -0500	[thread overview]
Message-ID: <CAPig+cThjBHn6AKaLvH5utD7rXMoj+gBXi-GVrfgoQ99v_38dg@mail.gmail.com> (raw)
In-Reply-To: <20210104162128.95281-2-rafaeloliveira.cs@gmail.com>

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> worktree: move should_prune_worktree() to worktree.c

The subject is a bit confusing since this function already lives in
`worktree.c` (albeit within the `builtin` directory). An alternate way
to word this might be:

    worktree: libify and publish should_prune_worktree()

or just:

    worktree: libify should_prune_worktree()

> As part of teaching "git worktree list" to annotate worktree that is a
> candidate for pruning, let's move should_prune_worktree() from
> builtin/worktree.c to worktree.c in order to make part of the worktree
> public API.

Good.

> This function will be used by another API function, implemented in the
> next patch that will accept a pointer to "worktree" structure directly
> making it easier to implement the "prunable" annotations.

Okay, though I'm not sure this paragraph adds value since the reader
understands implicitly from the first paragraph that this is the
reason this patch is libifying the function. (I'd probably drop this
paragraph if I was composing the message.)

> should_prune_worktree() knows how to select the given worktree for pruning
> based on an expiration date, however the expiration value is stored on a
> global variable and it is not local to the function. In order to move the
> function, teach should_prune_worktree() to take the expiration date as an
> argument.

Rather than "on a global variable", it would be more accurate to say
"static file-scope variable".

> diff --git a/worktree.c b/worktree.c
> @@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path,
> +/*
> + * Return true if worktree entry should be pruned, along with the reason for
> + * pruning. Otherwise, return false and the worktree's path, or NULL if it
> + * cannot be determined. Caller is responsible for freeing returned path.
> + */
> +int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)

The function documentation which was moved here (worktree.c) from
builtin/worktree.c is duplicated in the header worktree.h. Having it
in the header is good. Having it here in the source file doesn't
necessarily hurt, but I worry about it becoming out-of-sync with the
copy in the header. For that reason, it might make sense to keep it
only in the header.

All of the above review comments are very minor and several of them
are subjective, thus not necessarily worth a re-roll.