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

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

Junio C Hamano [off-list ref] writes:

quoted

Derrick Stolee [off-list ref] writes:

quoted

On 1/5/2021 2:23 PM, Martin Ågren wrote:

quoted

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/ (local)

Thanks for being diligent and keeping the code clean.

This series is good-to-go.

Reviewed-by: Derrick Stolee <redacted>

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