From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 19 Sep 2022 12:31:04 GMT
Subject: Re: [PATCH v7 2/6] fsmonitor: relocate socket file if .git directory is remote
Message-ID: <5c50a3cf-c5ac-6b11-2548-8400e5574b32@jeffhostetler.com>
In-Reply-To: <xmqqy1ujf5a9.fsf@gitster.g>



On 9/16/22 4:11 PM, Junio C Hamano wrote:
> "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> 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.

Jeff