Re: [GSoC][PATCH/RFC v3 3/3] credential-cache: only use user_socket if a socket

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2017, 06:10 UTC
Message-ID: <xmqqo9x4jsmi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq8to8lj59.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 27 quoted lines

> Devin Lehmacher <lehmacdj@gmail.com> writes:
>
>> diff --git a/credential-cache.c b/credential-cache.c
>> index db1343b46..63236adc2 100644
>> --- a/credential-cache.c
>> +++ b/credential-cache.c
>> @@ -83,12 +83,18 @@ static void do_cache(const char *socket, const char *action, int timeout,
>>  	strbuf_release(&buf);
>>  }
>>  
>> +static int is_socket(char *path) {
>> +	struct stat sb;
>> +	int ret = lstat(path, &sb);
>> +	return ret && S_IFSOCK(sb.st_mode);
>> +}
>> +
>>  static char *get_socket_path(void) {
>>  	char *home_socket;
>>  
>>  	home_socket = expand_user_path("~/.git-credential-cache/socket");
>>  	if (home_socket) {
>> -		if (file_exists(home_socket))
>> +		if (is_socket(home_socket))
>
> This should be done as part of 2/3, no?  It does not make sense to
> add 2/3 and then immediately say "oops, the check in 2/3 is wrong,
> and let's update it like so".

Also I think you would want to use S_ISFIFO() and/or S_ISSOCK() macros (I do not offhand recall which one credential cache daemon uses), not the S_IFxxx constant.

Perhaps

   1/3 - add xdg_cache similar to xdg_config

   2/3 - add is_socket()

   3/3 - use xdg_cache location for socket if traditional location
         is not in use

would be a better logical ordering of the patches.

Having said that, I do not think ~/.git-credential-cache/socket is the right thing to test. If the ~/.git-credential-cache directory already exists, it is likely that the user has a set-up that works well with "socket" inside it, and it is safer to keep using that location.

On the other hand, if that directory does not exist, we know it is safe to use whatever new location---after all, the lack of the directory tells us that the user has never used the traditional location successfully ;-)

So is_socket() may not even be needed, in which case this will be a two-patch series:

   1/2 - add xdg_cache similar to xdg_config
   2/2 - use xdg_cache location if ~/.git-credential-cache/ is not there