Re: [PATCH 0/5] avoid peeking into `struct lock_file`

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 7, 2021, 02:08 UTC
Message-ID: <8ae92e79-ef13-3faf-2fc2-d4b107e73c36@gmail.com>
In-Reply-To: <xmqqim89pu9k.fsf@gitster.c.googlers.com>

On 1/6/2021 5:36 PM, Junio C Hamano wrote:

Show 34 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>> On 1/5/2021 2:23 PM, Martin Ågren wrote:
>>>> I made a comment in [1] about how we could avoid peeking into a `struct
>>>> lock_file` and instead use a helper function that we happen to have at
>>>> our disposal. I then grepped around a bit and found that we're pretty
>>>> good at avoiding such peeking at the moment, but that we could do
>>>> a bit better.
>>>>
>>>> Here's a series to avoid such `lk.tempfile.foo` in favor of
>>>> `get_lock_file_foo(&lk)`.
>>>>
>>>> [1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/
>>>
>>> Thanks for being diligent and keeping the code clean.
>>>
>>> This series is good-to-go.
>>>
>>> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
>>
>> Thanks, both.
> 
> I liked what I saw.  The code after these patches got certainly
> clearer.
> 
> But it was not quite clear what I was *NOT* seeing in these patches.
> 
> IOW, how extensive is the coverage of these patches?  If we renamed
> the .tempfile field to, say, .tmpfile in "struct lock_file" in
> "lockfile.h", for example, would "lockfile.[ch]" be the *only* files
> that need to be adjusted to make the code compile again?  The same
> question for various fields in "struct tempfile".

 
There was a note in patch 5 about how do_write_index() takes a
tempfile instead of a lockfile, because sometimes the index is
written without a lock.

I can't say that this is otherwise comprehensive. Definitely a step in the right direction.

I think the only way to enforce this is to make 'struct lock_file' anonymous in lockfile.h and actually defined in lockfile.c, assuming that's possible. It seems like external callers would only be able to declare a pointer to one, but without access to sizeof(struct lock_file) these callers would be severely limited.

Thanks, -Stolee