On 9/19/22 12:42 PM, Junio C Hamano wrote:
Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> writes:
On 9/16/22 4:11 PM, Junio C Hamano wrote:
"Eric DeCosta via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
+const char *fsmonitor_ipc__get_path(struct repository *r)
+{
+ static const char *ipc_path;
+ SHA_CTX sha1ctx;
+ char *sock_dir;
+ struct strbuf ipc_file = STRBUF_INIT;
+ unsigned char hash[SHA_DIGEST_LENGTH];
+
+ if (ipc_path)
+ return ipc_path;
+
+ if (!r)
+ r = the_repository;
I'd prefer not to see this "NULL means the_repository". It would be
a different story if the caller does not necessarily have a ready
access to the_repository, but it is a global, so the caller can pass
the_repository and be more explicit. Giving two ways to the caller
to express same thing is not a good idea.
Thanks.
To be fair, I added several "if (!r) r = the_repository;" statements
to the original public FSMonitor routines. There were obscure cases
where tests would sometimes randomly fail because "r" wasn't completely
passed down via some hard to isolate call stack. Offlist, AEvar told me
that he managed to isolate it and has a fix.
So eventually, we'll be able to get rid of all of these direct
references to "the_repository" and properly assume that "r" is
always passed down.
But for now, I think we should let this stay for safety.
I wouldn't call "sweeping a breakage under the rug" a "safety",
though. If the caller cannot decide which repository instance is
the right thing to pass, or the caller does not yet have a good one
to pass when making a call down the codepath, how can it be safer to
use the_repository that may or may not be the appropriate one than
noticing the problem by dying and stopping the spread of damage?
Aren't we in the middle of a transition from always
using the global "the_repository" to a passed "r" variable?
We're getting closer to being able to hide the the global
symbol, but we're not there yet, right?
I'm thinking that at as long as the global exists, we are not
safe to have multiple "struct repository" instances, right?
All I'm saying is that there are obscure/edge code paths where
a valid "r" is not being passed down. Or, more likely, someone
has an "istate" that doesn't yet have "istate->repo" set at the
point of the call, so they might be passing "istate->repo", but
it is null.
Tracking down these nulls is important, but shouldn't it be
independent of this one?
Jeff
