On Sunday, October 6th, 2024 at 06:24, Eric Sunshine [off-list ref] wrote: 

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?

When I was working through different solution for the 3rd patch, I
encountered this issue and this was the fix for that (granted, I
could've handled this on the caller side). It turned out that I had
to go a different direction and this was no longer needed, but I
figured it wouldn't hurt to leave this in which is why it's the
last patch. However, I can just drop this if you want.

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

Sometimes it's cleaner to write a check once on the callee side
instead of all callers having to duplicate the same check. This
also follows the same pattern as free_worktree():

void free_worktree(struct worktree *worktree)
{
	if (!worktree)
		return;

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

Will do (unless we decide to drop this).

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.

I didn't change this originally, but then the build process threw errors
that I had code before declarations (because I placed the if condition at
the start of the function). I could've moved the if condition past the
declaration, but it seemed weird to need to declare a variable if the
function is just going to immediately return due to a NULL pointer.

If we decide to keep this patch then I can keep the separate declaration.