From: Jeff King <peff@peff.net>
To: Colin Hinton <colinlewishinton@gmail.com>
Cc: Patrick Steinhardt <ps@pks.im>, git@vger.kernel.org
Subject: Re: [PATCH] chdir-notify.h: Removed unused param 'name'
Date: Fri, 14 Aug 2026 16:31:00 -0400 [thread overview]
Message-ID: <20260814203100.GB2575854@coredump.intra.peff.net> (raw)
In-Reply-To: <20260814193849.1538-1-colinlewishinton@gmail.com>
On Fri, Aug 14, 2026 at 12:38:49PM -0700, Colin Hinton wrote:
> the `name` parameter in `chdir_notify_entry` is only set to NULL.
> Dropped to simplify logic in `chdir_notify_unregister`
I think this makes sense. It is often helpful in cleanup patches to
summarize the history in the commit message. And then we can be sure
that the unused field is OK to be removed, and it is not simply a bug
that nobody is passing in the value.
It looks like these became NULL in the callers across several functions,
like 1f43ff2c7e (refs: unregister reference stores from "chdir_notify",
2026-06-25) and 0de2467e6c (odb/source-packed: start converting to a
proper `struct odb_source`, 2026-06-17). With hunks like this:
- chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
- chdir_notify_reparent("files-backend $GIT_COMMONDIR",
- &refs->gitcommondir);
+ chdir_notify_register(NULL, files_ref_store_reparent, refs);
None of those indicate why they drop the descriptive names in favor of
NULL.
I think ultimately the reason is that the only user of the "name" field
was chdir_notify_reparent(), when it produced trace output. That went
away in 5bf546755c (chdir-notify: drop unused `chdir_notify_reparent()`,
2026-06-25).
So those other patches were preparing for that world, though I think the
ordering is somewhat confusing (and I won't be surprised if there was
some intermediate state where turning on tracing might have caused a
segfault).
So AFAICT the patch itself is good, but it might be nice to give some
explanation in the commit message.
-Peff
next prev parent reply other threads:[~2026-08-14 20:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-14 19:38 [PATCH] chdir-notify.h: Removed unused param 'name' Colin Hinton
2026-08-14 20:31 ` Jeff King [this message]
2026-08-14 20:42 ` Junio C Hamano
2026-08-14 21:42 ` [PATCH v2] " Colin Hinton
2026-08-15 5:12 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260814203100.GB2575854@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=colinlewishinton@gmail.com \
--cc=git@vger.kernel.org \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox