Hi Lars,
On Mon, 24 Oct 2016, larsxschneider@xxxxxxxxx wrote:
> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>
> All processes that the Git main process spawns inherit the open file
> descriptors of the main process. These leaked file descriptors can
> cause problems.
>
> Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
> descriptors. Since `git_open_noatime` does not describe the function
> properly anymore rename it to `git_open`.
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.
> @@ -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.
> continue;
> }
>
> + /* Might the failure be due to O_NOATIME? */
> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> + sha1_file_open_flag &= ~O_NOATIME;
> + continue;
> + }
I *think* the --patience diff option would have made that patch a little
more obvious.
Otherwise, the patch looks fine to me,
Dscho