From: Konrad Bucheli (PSI) <konrad.bucheli@psi.ch>
Date: Fri, 20 Dec 2024 15:50:35 GMT
Subject: Re: chmod failure on GVFS mounted CIFS share
Message-ID: <8af7ff11-c5bb-464b-894f-f1fb30df50e0@psi.ch>
In-Reply-To: <xmqq8qsafjfy.fsf@gitster.g>



On 20.12.24 16:44, Junio C Hamano wrote:
> "Konrad Bucheli (PSI)" <konrad.bucheli@psi.ch> writes:
> 
>> I have another idea: there is no need for a chmod if both the config
>> file and the lock file already have he same mode. Which is the case if
>> the filesystem has no proper chmod support.
> 
> And for majority of people who have working chmod(), would it mean
> one extra and unnecessary system call?
> 

I do not have stats, but I guess the chmod call would be needed very 
rarely as most of the time both files have default permissions. I do not 
know which call is more expensive.

> Instead, how about doing the chmod() first, like we have always done,
> but after seeing it fail, check with lstat() to see if the modes are
> already in the desired state and refrain from complaining if that is
> the case?  That way, we'll incur extra overhead only in the error
> code path, which is the usual pattern we would prefer to do things.
> 
> So instead of removing this part, ...
> 
>> -		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> -			error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
> 
> ... you'd do an extra lstat() on the lock file (so you can move the
> st_lock inside this block, narrowing its scope) before calling
> error_errno(), and only after finding out that st_lock cannot
> somehow be obtained or the resulting mode is different, you call
> error_errno() and arrange an error to be returned, all inside the
> if(){} block of the original.
> 
> Wouldn't it work even better, I wonder?

If you think that is the way to go, I will adapt the patch.

> 
> Thanks.
> 
>> +		if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
>> +			error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
>>   			ret = CONFIG_NO_WRITE;
>>   			goto out_free;
>>   		}
>>   
>> +		if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
>> +			if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> +				error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
>> +				ret = CONFIG_NO_WRITE;
>> +				goto out_free;
>> +			}
>> +		}
>> +
>>   		if (store.seen_nr == 0) {
>>   			if (!store.seen_alloc) {
>>   				/* Did not see key nor section */
>