On Sun, Oct 6, 2024 at 7:03 PM Caleb White [off-list ref] wrote:

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

quoted

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/ (local)

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

quoted

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