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?
