Re: chmod failure on GVFS mounted CIFS share
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 20, 2024, 15:44 UTC
- Message-ID
- <xmqq8qsafjfy.fsf@gitster.g>
- In-Reply-To
- <c287fbd7-eb08-45f1-953b-5afd4fe41f9f@psi.ch>
"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?
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?
Thanks.
Show 17 quoted lines
> + 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 */