Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2016, 16:58 UTC
Message-ID: <xmqqwpgws6ur.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1610251220280.3264@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Show 19 quoted lines

>> @@ -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.