On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.Attachment:
signature.asc
Description: OpenPGP digital signature
