On 20.12.24 16:44, Junio C Hamano wrote:
"Konrad Bucheli (PSI)" <konrad.bucheli@xxxxxx> 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 */
