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

On Thu, 7 Jan 2021 at 03:08, Derrick Stolee [off-list ref] wrote:

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

quoted

Junio C Hamano [off-list ref] writes:
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".

To be perfectly honest, I just grepped around. I just tried your
suggestion and it seems like I really did catch everyone who looks at
`->tempfile` or `.tempfile`.

I could add a remark in the commit message of the last patch along the
lines of "After this commit, renaming the `tempfile` field only triggers
compilation errors in lockfile.[ch] and this one instance that we're
intentionally leaving here.".

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.

Right, so outside of lockfile.[ch], that's the one spot where I needed
to do s/tempfile/tmpfile/ to be able to build again after trying out
Junio's idea. (We could have `get_lock_file_tempfile()` but it feels
funny to me to have an API for leaking the implementation. Just having
one user who needs to access the internal tempfile also sort of argues
for punting on introducing such a function, to me at least.)

I did on purpose avoid doing the same "don't peek" for tempfiles,
because I sort of assumed it would be a much deeper rabbit hole. Except
for in the last patch: In read-cache.c, the use of tempfiles/lock files
is intertwined/entangled enough that I felt silly modifying the spots
where we use lock files without doing the exact same cleanups to
tempfiles.

I just tried quickly renaming `fd` of `struct tempfile`. There aren't
*too* many hits, but from a quick glance it seems like some of them
might want a more informed rewrite of the logic to express intent even
better. For example, rather than grabbing the `->fd` using the "proper"
function and handling it sort of as a boolean, it might express intent
even better to not grab it at all and instead explicitly ask "do we
hold the lock?".

I'm tempted to promise I'll try a similar round of cleanups for tempfile
users, rather than rerolling and turning ~5 patches in v1 into ~10 in
v2, especially when those might be slightly more invasive. Hmm?

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.

I've never used coccinelle, but I'm guessing it might be a decent fit
for the problem. Either by encoding all 10(?) "don't do this, do that"
or if it's somehow possible to more generally say "don't look at
lockfile{.,->}tempfile". We'll obviously need some allowlisting as well.

Thanks both for your comments.

Martin