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

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

[Martin Ågren]

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/


Martin Ågren (5):
  builtin/gc: don't peek into `struct lock_file`
  commit-graph: don't peek into `struct lock_file`
  midx: don't peek into `struct lock_file`
  refs/files-backend: don't peek into `struct lock_file`
  read-cache: try not to peek into `struct {lock_,temp}file`

 builtin/gc.c         |  6 +++---
 commit-graph.c       |  6 +++---
 midx.c               |  2 +-
 read-cache.c         | 12 ++++++------
 refs/files-backend.c |  4 ++--
 5 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.30.0

[PATCH 1/5] builtin/gc: don't peek into `struct lock_file`

[Martin Ågren]

A `struct lock_file` is pretty much just a wrapper around a tempfile.
But it's easy enough to avoid relying on this. Use the wrappers that the
lock file API provides rather than peeking at the temp file or even into
*its* internals.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/gc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 4c24f41852..64f2b52d6e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -92,7 +92,7 @@ static void process_log_file(void)
 		 */
 		int saved_errno = errno;
 		fprintf(stderr, _("Failed to fstat %s: %s"),
-			get_tempfile_path(log_lock.tempfile),
+			get_lock_file_path(&log_lock),
 			strerror(saved_errno));
 		fflush(stderr);
 		commit_lock_file(&log_lock);
@@ -1518,7 +1518,7 @@ static int update_background_schedule(int run_maintenance)
 	strvec_split(&crontab_list.args, crontab_name);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
-	crontab_list.out = dup(lk.tempfile->fd);
+	crontab_list.out = dup(get_lock_file_fd(&lk));
 	crontab_list.git_cmd = 0;
 
 	if (start_command(&crontab_list)) {
@@ -1533,7 +1533,7 @@ static int update_background_schedule(int run_maintenance)
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
 	 */
-	cron_list = fdopen(lk.tempfile->fd, "r");
+	cron_list = fdopen(get_lock_file_fd(&lk), "r");
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, crontab_name);
-- 
2.30.0

[PATCH 2/5] commit-graph: don't peek into `struct lock_file`

[Martin Ågren]

Similar to the previous commit, avoid peeking into the `struct
lock_file`. Use the lock file API instead.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 commit-graph.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 06f8dc1d89..031641014f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1694,8 +1694,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
 					       LOCK_DIE_ON_ERROR, 0444);
-		fd = lk.tempfile->fd;
-		f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
+		fd = get_lock_file_fd(&lk);
+		f = hashfd(fd, get_lock_file_path(&lk));
 	}
 
 	chunks[0].id = GRAPH_CHUNKID_OIDFANOUT;
@@ -1833,7 +1833,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		result = rename(ctx->graph_name, final_graph_name);
 
 		for (i = 0; i < ctx->num_commit_graphs_after; i++)
-			fprintf(lk.tempfile->fp, "%s\n", ctx->commit_graph_hash_after[i]);
+			fprintf(get_lock_file_fp(&lk), "%s\n", ctx->commit_graph_hash_after[i]);
 
 		if (result) {
 			error(_("failed to rename temporary commit-graph file"));
-- 
2.30.0

[PATCH 3/5] midx: don't peek into `struct lock_file`

[Martin Ågren]

Similar to the previous commits, avoid peeking into the `struct
lock_file`. Use the lock file API instead.

The two functions we're calling here double-check that the tempfile is
indeed "active", which is arguably overkill considering how we took the
lock on the line immediately above. More importantly, this future-proofs
us against, e.g., other code appearing between these two lines or the
lock file and/or tempfile internals changing.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 79c282b070..9d41b9c271 100644
--- a/midx.c
+++ b/midx.c
@@ -918,7 +918,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
 					(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
 	hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
-	f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
+	f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	FREE_AND_NULL(midx_name);
 
 	if (packs.m)
-- 
2.30.0

[PATCH 4/5] refs/files-backend: don't peek into `struct lock_file`

[Martin Ågren]

Similar to the previous commits, avoid peeking into the `struct
lock_file`. Use the lock file API instead. Note how we obtain the path
to the lock file if `fdopen_lock_file()` failed and that this is not a
problem: as documented in lockfile.h, failure to "fdopen" does not roll
back the lock file and we're free to, e.g., query it for its path.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 04e85e7002..4fdc68810b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1824,12 +1824,12 @@ static int create_symref_locked(struct files_ref_store *refs,
 
 	if (!fdopen_lock_file(&lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
-			     lock->lk.tempfile->filename.buf, strerror(errno));
+			     get_lock_file_path(&lock->lk), strerror(errno));
 
 	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
-	fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
+	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
-- 
2.30.0

[PATCH 5/5] read-cache: try not to peek into `struct {lock_,temp}file`

[Martin Ågren]

Similar to the previous commits, try to avoid peeking into the `struct
lock_file`. We also have some `struct tempfile`s -- let's avoid looking
into those as well.

Note that `do_write_index()` takes a tempfile and that when we call it,
we either have a tempfile which we can easily hand down, or we have a
lock file, from which we need to somehow obtain the internal tempfile.
So we need to leave that one instance of peeking-into. Nevertheless,
this commit leaves us not relying on exactly how the path of the
tempfile / lock file is stored internally.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index ecf6f68994..29144cf879 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3014,10 +3014,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_flush(&c, newfd, istate->oid.hash))
 		return -1;
 	if (close_tempfile_gently(tempfile)) {
-		error(_("could not close '%s'"), tempfile->filename.buf);
+		error(_("could not close '%s'"), get_tempfile_path(tempfile));
 		return -1;
 	}
-	if (stat(tempfile->filename.buf, &st))
+	if (stat(get_tempfile_path(tempfile), &st))
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
@@ -3058,10 +3058,10 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 	 * that is associated with the given "istate".
 	 */
 	trace2_region_enter_printf("index", "do_write_index", the_repository,
-				   "%s", lock->tempfile->filename.buf);
+				   "%s", get_lock_file_path(lock));
 	ret = do_write_index(istate, lock->tempfile, 0);
 	trace2_region_leave_printf("index", "do_write_index", the_repository,
-				   "%s", lock->tempfile->filename.buf);
+				   "%s", get_lock_file_path(lock));
 
 	if (ret)
 		return ret;
@@ -3158,10 +3158,10 @@ static int write_shared_index(struct index_state *istate,
 	move_cache_to_base_index(istate);
 
 	trace2_region_enter_printf("index", "shared/do_write_index",
-				   the_repository, "%s", (*temp)->filename.buf);
+				   the_repository, "%s", get_tempfile_path(*temp));
 	ret = do_write_index(si->base, *temp, 1);
 	trace2_region_leave_printf("index", "shared/do_write_index",
-				   the_repository, "%s", (*temp)->filename.buf);
+				   the_repository, "%s", get_tempfile_path(*temp));
 
 	if (ret)
 		return ret;
-- 
2.30.0

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

[Derrick Stolee]

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>

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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".

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

[Derrick Stolee]

On 1/6/2021 5:36 PM, Junio C Hamano wrote:
> 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

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

[Martin Ågren]

On Thu, 7 Jan 2021 at 03:08, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/6/2021 5:36 PM, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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`.

That's wonderful ;-)

> 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.".

It would be a nice addition to the log message to help readers to
feel confident about the conversion.  It is OK if you want to add
one, and certainly a good trick to have in your toolbox for your
next conversion, but it may not be worth rerolling only to update
the log message with such a remark.

Thanks.

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

[Martin Ågren]

On Thu, 7 Jan 2021 at 09:19, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > 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.".
>
> It would be a nice addition to the log message to help readers to
> feel confident about the conversion.  It is OK if you want to add
> one, and certainly a good trick to have in your toolbox for your
> next conversion, but it may not be worth rerolling only to update
> the log message with such a remark.

Ok, got it. So I'm not planning to reroll this series. Unless something
else shows up, obviously.

Thank you.

Martin