Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> The patch series may be a little bit more pleasant to read if you renamed
> git_open_noatime() to git_open() first, in a separate commit.
Possibly.
If this were "we added a new parameter at the same time and each
calling site of the renamed function with an extra parameter chooses
to pass different value to it", then keeping the rename and the
interface enhancement as two separate steps would help a lot.
But this one only renames and updates the internal implementation,
without changing what the calling sites need to do. I am OK with
having them together in a single patch like the one posted.
>> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
>> if (fd >= 0)
>> return fd;
>>
>> - /* Might the failure be due to O_NOATIME? */
>> - if (errno != ENOENT && sha1_file_open_flag) {
>> - sha1_file_open_flag = 0;
>> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
>> + if (O_CLOEXEC && errno == EINVAL &&
>> + (sha1_file_open_flag & O_CLOEXEC)) {
>> + sha1_file_open_flag &= ~O_CLOEXEC;
>
> How about
>
> if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
> sha1_file_open_flag &= ~O_CLOEXEC;
>
> instead? It is shorter and should be just as easily optimized out by a
> C compiler if O_CLOEXEC was defined as 0.
Yup, I think that makes things easier to read for humans, too.