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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 6, 2024, 23:24 UTC
Message-ID: <CAPig+cSqWomvpgAGUCCaxHG9UTy+JecRndM9ynwdBgUoLYAXTw@mail.gmail.com>
In-Reply-To: <VKrLGkXtD_CWHlQJRx_JqPn9sFsjKL88fRnGvA2UcV_gvQxyv8kmIx9M6BkPRfAx9paB7KrjkL9XAE8P2P2EPJnciBN4F6LAAc176NzLh6U=@pm.me>

On Sun, Oct 6, 2024 at 7:03 PM Caleb White <cdwhite3@pm.me> wrote:

Show 13 quoted lines

> On Sunday, October 6th, 2024 at 06:24, Eric Sunshine <sunshine@sunshineco.com> 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.

Reviewers are a limited resource on this project[1], so it's ideal if submissions can be as reviewer-friendly as possible. Extraneous patches, unnecessary or unrelated changes to surrounding code, etc. all make a patch series more onerous to review, thus are best avoided. (This concern prompted all the review comments I left on this patch.) So, let's drop this patch since it adds no value to either this series or to the existing codebase. If someone needs such a change later on, they can resurrect the change.

[1] There are far more people submitting patches to the project than reviewing them. For instance, according to Junio's latest "What's Cooking" report[2], the patch I submitted[3] a couple weeks ago to fix "git worktree repair" to handle manual copy operations is still awaiting review. (Since you've now been living in the worktree code a bit and have had to digest the "repair" logic, perhaps you'd be interested in reviewing that patch?)

[2]: see the "es/worktree-repair-copied" entry at https://lore.kernel.org/git/xmqq7cancmoj.fsf@gitster.g/

[3]: https://lore.kernel.org/git/20240923075416.54289-1-ericsunshine@charter.net/

Show 13 quoted lines

> > 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.

Since this project is still following older style rules about declaring all variables up-front, it's not unusual to declare variables which don't necessarily get used in some code path (even though it might feel weird for someone who habitually declares variables only as and where needed).