Re: [PATCH v2 4/4] worktree: prevent null pointer dereference

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
> If worktrees is NULL, free_worktrees() should return immediately to
> prevent a null pointer dereference.
>
> Signed-off-by: Caleb White <cdwhite3@pm.me>

Critical questions: It is not clear why this patch is needed,
especially coming at the end of the series. Is there some code in a
patch earlier in the series which might call free_worktrees() with
NULL? If so, then this patch should come before that patch. If not,
then why do we need this patch at all?

Devil's advocate question: Why is it the responsibility of
free_worktrees() to check this condition as opposed to being the
caller's responsibility?

The commit message should explain the need for this patch and answer
these questions, not just say what change is being made.

> diff --git a/worktree.c b/worktree.c
> @@ -28,8 +28,9 @@ void free_worktree(struct worktree *worktree)
>  void free_worktrees(struct worktree **worktrees)
>  {
> -       int i = 0;
> -       for (i = 0; worktrees[i]; i++)
> +       if  (!worktrees)
> +               return;
> +       for (int i = 0; worktrees[i]; i++)
>                 free_worktree(worktrees[i]);

Although it's true that this project has recently started allowing
declaration of the variable in the `for` statement, that change is
unrelated to the stated purpose in the commit message. True, it's a
minor thing in this case, but it causes a hiccup for reviewers when
unrelated changes are piggybacked like this with the "real" change
since it adds noise which obscures what the reviewer should really be
focusing on.