{"thread":{"id":"54941","subject":"[PATCH 0/5] avoid peeking into `struct lock_file`","startedAt":"2021-01-05T19:26:02Z","lastAt":"2021-01-07T18:18:16Z","messageCount":13,"participants":["Martin Ågren","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"413477","messageId":"cover.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":null,"subject":"[PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:45Z","receivedAt":"2021-01-05T19:26:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"I made a comment in [1] about how we could avoid peeking into a `struct\nlock_file` and instead use a helper function that we happen to have at\nour disposal. I then grepped around a bit and found that we're pretty\ngood at avoiding such peeking at the moment, but that we could do\na bit better.\n\nHere's a series to avoid such `lk.tempfile.foo` in favor of\n`get_lock_file_foo(&lk)`.\n\n[1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/\n\n\nMartin Ågren (5):\n builtin/gc: don't peek into `struct lock_file`\n commit-graph: don't peek into `struct lock_file`\n midx: don't peek into `struct lock_file`\n refs/files-backend: don't peek into `struct lock_file`\n read-cache: try not to peek into `struct {lock_,temp}file`\n\n builtin/gc.c | 6 +++---\n commit-graph.c | 6 +++---\n midx.c | 2 +-\n read-cache.c | 12 ++++++------\n refs/files-backend.c | 4 ++--\n 5 files changed, 15 insertions(+), 15 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"413478","messageId":"a3510f3969d26d26ade136b27a7b9dd1d9cc2976.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"[PATCH 4/5] refs/files-backend: don't peek into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:49Z","receivedAt":"2021-01-05T19:26:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commits, avoid peeking into the `struct\nlock_file`. Use the lock file API instead. Note how we obtain the path\nto the lock file if `fdopen_lock_file()` failed and that this is not a\nproblem: as documented in lockfile.h, failure to \"fdopen\" does not roll\nback the lock file and we're free to, e.g., query it for its path.\n\nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 04e85e7002..4fdc68810b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1824,12 +1824,12 @@ static int create_symref_locked(struct files_ref_store *refs,\n \n \tif (!fdopen_lock_file(&lock->lk, \"w\"))\n \t\treturn error(\"unable to fdopen %s: %s\",\n-\t\t\t lock->lk.tempfile->filename.buf, strerror(errno));\n+\t\t\t get_lock_file_path(&lock->lk), strerror(errno));\n \n \tupdate_symref_reflog(refs, lock, refname, target, logmsg);\n \n \t/* no error check; commit_ref will check ferror */\n-\tfprintf(lock->lk.tempfile->fp, \"ref: %s\\n\", target);\n+\tfprintf(get_lock_file_fp(&lock->lk), \"ref: %s\\n\", target);\n \tif (commit_ref(lock) < 0)\n \t\treturn error(\"unable to write symref for %s: %s\", refname,\n \t\t\t strerror(errno));\n-- \n2.30.0\n\n"},{"id":"413479","messageId":"6dfb457b2a3187546eb4b4d90679348f2051a57d.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"[PATCH 3/5] midx: don't peek into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:48Z","receivedAt":"2021-01-05T19:26:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commits, avoid peeking into the `struct\nlock_file`. Use the lock file API instead.\n\nThe two functions we're calling here double-check that the tempfile is\nindeed \"active\", which is arguably overkill considering how we took the\nlock on the line immediately above. More importantly, this future-proofs\nus against, e.g., other code appearing between these two lines or the\nlock file and/or tempfile internals changing.\n\nSigned-off-by: Martin Ågren \n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 79c282b070..9d41b9c271 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -918,7 +918,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \t\t\t\t\t(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);\n \n \thold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);\n-\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n+\tf = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));\n \tFREE_AND_NULL(midx_name);\n \n \tif (packs.m)\n-- \n2.30.0\n\n"},{"id":"413480","messageId":"7a91469c7c791d5faeabc601775a3b535cbfc26e.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"[PATCH 2/5] commit-graph: don't peek into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:47Z","receivedAt":"2021-01-05T19:26:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commit, avoid peeking into the `struct\nlock_file`. Use the lock file API instead.\n\nSigned-off-by: Martin Ågren \n---\n commit-graph.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 06f8dc1d89..031641014f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1694,8 +1694,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t} else {\n \t\thold_lock_file_for_update_mode(&lk, ctx->graph_name,\n \t\t\t\t\t LOCK_DIE_ON_ERROR, 0444);\n-\t\tfd = lk.tempfile->fd;\n-\t\tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n+\t\tfd = get_lock_file_fd(&lk);\n+\t\tf = hashfd(fd, get_lock_file_path(&lk));\n \t}\n \n \tchunks[0].id = GRAPH_CHUNKID_OIDFANOUT;\n@@ -1833,7 +1833,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\tresult = rename(ctx->graph_name, final_graph_name);\n \n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++)\n-\t\t\tfprintf(lk.tempfile->fp, \"%s\\n\", ctx->commit_graph_hash_after[i]);\n+\t\t\tfprintf(get_lock_file_fp(&lk), \"%s\\n\", ctx->commit_graph_hash_after[i]);\n \n \t\tif (result) {\n \t\t\terror(_(\"failed to rename temporary commit-graph file\"));\n-- \n2.30.0\n\n"},{"id":"413481","messageId":"bbf24f7677eed41b80562d153b35be314cfb2750.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"[PATCH 1/5] builtin/gc: don't peek into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:46Z","receivedAt":"2021-01-05T19:26:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"A `struct lock_file` is pretty much just a wrapper around a tempfile.\nBut it's easy enough to avoid relying on this. Use the wrappers that the\nlock file API provides rather than peeking at the temp file or even into\n*its* internals.\n\nSigned-off-by: Martin Ågren \n---\n builtin/gc.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 4c24f41852..64f2b52d6e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -92,7 +92,7 @@ static void process_log_file(void)\n \t\t */\n \t\tint saved_errno = errno;\n \t\tfprintf(stderr, _(\"Failed to fstat %s: %s\"),\n-\t\t\tget_tempfile_path(log_lock.tempfile),\n+\t\t\tget_lock_file_path(&log_lock),\n \t\t\tstrerror(saved_errno));\n \t\tfflush(stderr);\n \t\tcommit_lock_file(&log_lock);\n@@ -1518,7 +1518,7 @@ static int update_background_schedule(int run_maintenance)\n \tstrvec_split(&crontab_list.args, crontab_name);\n \tstrvec_push(&crontab_list.args, \"-l\");\n \tcrontab_list.in = -1;\n-\tcrontab_list.out = dup(lk.tempfile->fd);\n+\tcrontab_list.out = dup(get_lock_file_fd(&lk));\n \tcrontab_list.git_cmd = 0;\n \n \tif (start_command(&crontab_list)) {\n@@ -1533,7 +1533,7 @@ static int update_background_schedule(int run_maintenance)\n \t * Read from the .lock file, filtering out the old\n \t * schedule while appending the new schedule.\n \t */\n-\tcron_list = fdopen(lk.tempfile->fd, \"r\");\n+\tcron_list = fdopen(get_lock_file_fd(&lk), \"r\");\n \trewind(cron_list);\n \n \tstrvec_split(&crontab_edit.args, crontab_name);\n-- \n2.30.0\n\n"},{"id":"413482","messageId":"bb2327c7dca20fa563b18a9ec74180d7c595ca24.1609874026.git.martin.agren@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"[PATCH 5/5] read-cache: try not to peek into `struct {lock_,temp}file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-05T19:23:50Z","receivedAt":"2021-01-05T19:26:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Similar to the previous commits, try to avoid peeking into the `struct\nlock_file`. We also have some `struct tempfile`s -- let's avoid looking\ninto those as well.\n\nNote that `do_write_index()` takes a tempfile and that when we call it,\nwe either have a tempfile which we can easily hand down, or we have a\nlock file, from which we need to somehow obtain the internal tempfile.\nSo we need to leave that one instance of peeking-into. Nevertheless,\nthis commit leaves us not relying on exactly how the path of the\ntempfile / lock file is stored internally.\n\nSigned-off-by: Martin Ågren \n---\n read-cache.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex ecf6f68994..29144cf879 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3014,10 +3014,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tif (ce_flush(&c, newfd, istate->oid.hash))\n \t\treturn -1;\n \tif (close_tempfile_gently(tempfile)) {\n-\t\terror(_(\"could not close '%s'\"), tempfile->filename.buf);\n+\t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n \t\treturn -1;\n \t}\n-\tif (stat(tempfile->filename.buf, &st))\n+\tif (stat(get_tempfile_path(tempfile), &st))\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n@@ -3058,10 +3058,10 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \t * that is associated with the given \"istate\".\n \t */\n \ttrace2_region_enter_printf(\"index\", \"do_write_index\", the_repository,\n-\t\t\t\t \"%s\", lock->tempfile->filename.buf);\n+\t\t\t\t \"%s\", get_lock_file_path(lock));\n \tret = do_write_index(istate, lock->tempfile, 0);\n \ttrace2_region_leave_printf(\"index\", \"do_write_index\", the_repository,\n-\t\t\t\t \"%s\", lock->tempfile->filename.buf);\n+\t\t\t\t \"%s\", get_lock_file_path(lock));\n \n \tif (ret)\n \t\treturn ret;\n@@ -3158,10 +3158,10 @@ static int write_shared_index(struct index_state *istate,\n \tmove_cache_to_base_index(istate);\n \n \ttrace2_region_enter_printf(\"index\", \"shared/do_write_index\",\n-\t\t\t\t the_repository, \"%s\", (*temp)->filename.buf);\n+\t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n \tret = do_write_index(si->base, *temp, 1);\n \ttrace2_region_leave_printf(\"index\", \"shared/do_write_index\",\n-\t\t\t\t the_repository, \"%s\", (*temp)->filename.buf);\n+\t\t\t\t the_repository, \"%s\", get_tempfile_path(*temp));\n \n \tif (ret)\n \t\treturn ret;\n-- \n2.30.0\n\n"},{"id":"413523","messageId":"a401a6a7-fc15-9f26-2345-651964cf7b5d@gmail.com","threadId":"54941","inReplyTo":"cover.1609874026.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-06T02:16:01Z","receivedAt":"2021-01-06T02:16:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/5/2021 2:23 PM, Martin Ågren wrote:\n> I made a comment in [1] about how we could avoid peeking into a `struct\n> lock_file` and instead use a helper function that we happen to have at\n> our disposal. I then grepped around a bit and found that we're pretty\n> good at avoiding such peeking at the moment, but that we could do\n> a bit better.\n> \n> Here's a series to avoid such `lk.tempfile.foo` in favor of\n> `get_lock_file_foo(&lk)`.\n> \n> [1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/\n\nThanks for being diligent and keeping the code clean.\n\nThis series is good-to-go.\n\nReviewed-by: Derrick Stolee \n"},{"id":"413572","messageId":"xmqq5z4as2j9.fsf@gitster.c.googlers.com","threadId":"54941","inReplyTo":"a401a6a7-fc15-9f26-2345-651964cf7b5d@gmail.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T11:55:22Z","receivedAt":"2021-01-06T11:56:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 1/5/2021 2:23 PM, Martin Ågren wrote:\n>> I made a comment in [1] about how we could avoid peeking into a `struct\n>> lock_file` and instead use a helper function that we happen to have at\n>> our disposal. I then grepped around a bit and found that we're pretty\n>> good at avoiding such peeking at the moment, but that we could do\n>> a bit better.\n>> \n>> Here's a series to avoid such `lk.tempfile.foo` in favor of\n>> `get_lock_file_foo(&lk)`.\n>> \n>> [1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/\n>\n> Thanks for being diligent and keeping the code clean.\n>\n> This series is good-to-go.\n>\n> Reviewed-by: Derrick Stolee \n\nThanks, both.\n"},{"id":"413617","messageId":"xmqqim89pu9k.fsf@gitster.c.googlers.com","threadId":"54941","inReplyTo":"xmqq5z4as2j9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T22:36:55Z","receivedAt":"2021-01-06T22:37:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Derrick Stolee writes:\n>\n>> On 1/5/2021 2:23 PM, Martin Ågren wrote:\n>>> I made a comment in [1] about how we could avoid peeking into a `struct\n>>> lock_file` and instead use a helper function that we happen to have at\n>>> our disposal. I then grepped around a bit and found that we're pretty\n>>> good at avoiding such peeking at the moment, but that we could do\n>>> a bit better.\n>>> \n>>> Here's a series to avoid such `lk.tempfile.foo` in favor of\n>>> `get_lock_file_foo(&lk)`.\n>>> \n>>> [1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/\n>>\n>> Thanks for being diligent and keeping the code clean.\n>>\n>> This series is good-to-go.\n>>\n>> Reviewed-by: Derrick Stolee \n>\n> Thanks, both.\n\nI liked what I saw. The code after these patches got certainly\nclearer.\n\nBut it was not quite clear what I was *NOT* seeing in these patches.\n\nIOW, how extensive is the coverage of these patches? If we renamed\nthe .tempfile field to, say, .tmpfile in \"struct lock_file\" in\n\"lockfile.h\", for example, would \"lockfile.[ch]\" be the *only* files\nthat need to be adjusted to make the code compile again? The same\nquestion for various fields in \"struct tempfile\".\n\n"},{"id":"413635","messageId":"8ae92e79-ef13-3faf-2fc2-d4b107e73c36@gmail.com","threadId":"54941","inReplyTo":"xmqqim89pu9k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-07T02:08:57Z","receivedAt":"2021-01-07T02:09:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/6/2021 5:36 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Derrick Stolee writes:\n>>\n>>> On 1/5/2021 2:23 PM, Martin Ågren wrote:\n>>>> I made a comment in [1] about how we could avoid peeking into a `struct\n>>>> lock_file` and instead use a helper function that we happen to have at\n>>>> our disposal. I then grepped around a bit and found that we're pretty\n>>>> good at avoiding such peeking at the moment, but that we could do\n>>>> a bit better.\n>>>>\n>>>> Here's a series to avoid such `lk.tempfile.foo` in favor of\n>>>> `get_lock_file_foo(&lk)`.\n>>>>\n>>>> [1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/\n>>>\n>>> Thanks for being diligent and keeping the code clean.\n>>>\n>>> This series is good-to-go.\n>>>\n>>> Reviewed-by: Derrick Stolee \n>>\n>> Thanks, both.\n> \n> I liked what I saw. The code after these patches got certainly\n> clearer.\n> \n> But it was not quite clear what I was *NOT* seeing in these patches.\n> \n> IOW, how extensive is the coverage of these patches? If we renamed\n> the .tempfile field to, say, .tmpfile in \"struct lock_file\" in\n> \"lockfile.h\", for example, would \"lockfile.[ch]\" be the *only* files\n> that need to be adjusted to make the code compile again? The same\n> question for various fields in \"struct tempfile\".\n \nThere was a note in patch 5 about how do_write_index() takes a\ntempfile instead of a lockfile, because sometimes the index is\nwritten without a lock.\n\nI can't say that this is otherwise comprehensive. Definitely a\nstep in the right direction.\n\nI think the only way to enforce this is to make 'struct lock_file'\nanonymous in lockfile.h and actually defined in lockfile.c, assuming\nthat's possible. It seems like external callers would only be able\nto declare a pointer to one, but without access to sizeof(struct\nlock_file) these callers would be severely limited.\n\nThanks,\n-Stolee\n"},{"id":"413661","messageId":"CAN0heSqdphJWgG6gLM4a8mrDLh6qGUVq5w2FYAr7g8uePrcd2w@mail.gmail.com","threadId":"54941","inReplyTo":"8ae92e79-ef13-3faf-2fc2-d4b107e73c36@gmail.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-07T07:55:32Z","receivedAt":"2021-01-07T07:56:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 7 Jan 2021 at 03:08, Derrick Stolee wrote:\n>\n> On 1/6/2021 5:36 PM, Junio C Hamano wrote:\n> > Junio C Hamano writes:\n> > I liked what I saw. The code after these patches got certainly\n> > clearer.\n> >\n> > But it was not quite clear what I was *NOT* seeing in these patches.\n> >\n> > IOW, how extensive is the coverage of these patches? If we renamed\n> > the .tempfile field to, say, .tmpfile in \"struct lock_file\" in\n> > \"lockfile.h\", for example, would \"lockfile.[ch]\" be the *only* files\n> > that need to be adjusted to make the code compile again? The same\n> > question for various fields in \"struct tempfile\".\n\nTo be perfectly honest, I just grepped around. I just tried your\nsuggestion and it seems like I really did catch everyone who looks at\n`->tempfile` or `.tempfile`.\n\nI could add a remark in the commit message of the last patch along the\nlines of \"After this commit, renaming the `tempfile` field only triggers\ncompilation errors in lockfile.[ch] and this one instance that we're\nintentionally leaving here.\".\n\n> There was a note in patch 5 about how do_write_index() takes a\n> tempfile instead of a lockfile, because sometimes the index is\n> written without a lock.\n\nRight, so outside of lockfile.[ch], that's the one spot where I needed\nto do s/tempfile/tmpfile/ to be able to build again after trying out\nJunio's idea. (We could have `get_lock_file_tempfile()` but it feels\nfunny to me to have an API for leaking the implementation. Just having\none user who needs to access the internal tempfile also sort of argues\nfor punting on introducing such a function, to me at least.)\n\nI did on purpose avoid doing the same \"don't peek\" for tempfiles,\nbecause I sort of assumed it would be a much deeper rabbit hole. Except\nfor in the last patch: In read-cache.c, the use of tempfiles/lock files\nis intertwined/entangled enough that I felt silly modifying the spots\nwhere we use lock files without doing the exact same cleanups to\ntempfiles.\n\nI just tried quickly renaming `fd` of `struct tempfile`. There aren't\n*too* many hits, but from a quick glance it seems like some of them\nmight want a more informed rewrite of the logic to express intent even\nbetter. For example, rather than grabbing the `->fd` using the \"proper\"\nfunction and handling it sort of as a boolean, it might express intent\neven better to not grab it at all and instead explicitly ask \"do we\nhold the lock?\".\n\nI'm tempted to promise I'll try a similar round of cleanups for tempfile\nusers, rather than rerolling and turning ~5 patches in v1 into ~10 in\nv2, especially when those might be slightly more invasive. Hmm?\n\n> I think the only way to enforce this is to make 'struct lock_file'\n> anonymous in lockfile.h and actually defined in lockfile.c, assuming\n> that's possible. It seems like external callers would only be able\n> to declare a pointer to one, but without access to sizeof(struct\n> lock_file) these callers would be severely limited.\n\nI've never used coccinelle, but I'm guessing it might be a decent fit\nfor the problem. Either by encoding all 10(?) \"don't do this, do that\"\nor if it's somehow possible to more generally say \"don't look at\nlockfile{.,->}tempfile\". We'll obviously need some allowlisting as well.\n\nThanks both for your comments.\n\nMartin\n"},{"id":"413662","messageId":"xmqqft3dma54.fsf@gitster.c.googlers.com","threadId":"54941","inReplyTo":"CAN0heSqdphJWgG6gLM4a8mrDLh6qGUVq5w2FYAr7g8uePrcd2w@mail.gmail.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-07T08:19:51Z","receivedAt":"2021-01-07T08:20:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> To be perfectly honest, I just grepped around. I just tried your\n> suggestion and it seems like I really did catch everyone who looks at\n> `->tempfile` or `.tempfile`.\n\nThat's wonderful ;-)\n\n> I could add a remark in the commit message of the last patch along the\n> lines of \"After this commit, renaming the `tempfile` field only triggers\n> compilation errors in lockfile.[ch] and this one instance that we're\n> intentionally leaving here.\".\n\nIt would be a nice addition to the log message to help readers to\nfeel confident about the conversion. It is OK if you want to add\none, and certainly a good trick to have in your toolbox for your\nnext conversion, but it may not be worth rerolling only to update\nthe log message with such a remark.\n\nThanks.\n"},{"id":"413716","messageId":"CAN0heSoK21Qx_x-69KtmuScU6cN1SqB45ptgDwV8qpeh1iTC0Q@mail.gmail.com","threadId":"54941","inReplyTo":"xmqqft3dma54.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/5] avoid peeking into `struct lock_file`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-01-07T18:17:23Z","receivedAt":"2021-01-07T18:18:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 7 Jan 2021 at 09:19, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > I could add a remark in the commit message of the last patch along the\n> > lines of \"After this commit, renaming the `tempfile` field only triggers\n> > compilation errors in lockfile.[ch] and this one instance that we're\n> > intentionally leaving here.\".\n>\n> It would be a nice addition to the log message to help readers to\n> feel confident about the conversion. It is OK if you want to add\n> one, and certainly a good trick to have in your toolbox for your\n> next conversion, but it may not be worth rerolling only to update\n> the log message with such a remark.\n\nOk, got it. So I'm not planning to reroll this series. Unless something\nelse shows up, obviously.\n\nThank you.\n\nMartin\n"}]}