On 1/6/2021 5:36 PM, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >> Derrick Stolee <stolee@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ >>> >>> Thanks for being diligent and keeping the code clean. >>> >>> This series is good-to-go. >>> >>> Reviewed-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> >> 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
