[PATCH 1/9] csum-file: drop discard_hashfile()

[Jeff King <peff@peff.net>]

Commit c3d034df16 (csum-file: introduce discard_hashfile(), 2024-07-25)
added a cleanup function that no longer has any callers. In that commit
we adjusted do_write_index() to use the new function. But a similar fix
occurred on a parallel branch, making free_hashfile() public, and the
merge resolution in 1b6b2bfae5 (Merge branch 'ps/leakfixes-part-4',
2024-08-23) took the free_hashfile() version.

So now we have two functions, discard_hashfile() and free_hashfile(),
and we only need one. Which one do we want to keep?

The only difference between them is that the discard variant also closes
the descriptors held in the struct. Let's look at the three callers:

  1. In finalize_hashfile() we've either already closed the descriptors
     (if the CSUM_CLOSE flag is passed) or the caller didn't want them
     closed (if it didn't pass that flag). So we want the more limited
     free_hashfile().

  2. In object-file.c:flush_packfile_transaction() we close the
     descriptor ourselves. So discard_hashfile() could save us a line of
     code.

  3. In do_write_index() we don't close the descriptor. This was the spot
     for which c3d034df16 added the discard function in the first place,
     but I'm skeptical that closing the descriptor here is the right
     thing. It is true that we are done with the descriptor at this
     point and closing it would be ideal. But we don't really own it!

     The descriptor comes from a tempfile struct (as part of a lock) and
     that tempfile will hold on to the descriptor and try to close it
     when it is deleted. This might happen at the end of the program, in
     which case the double-close is mostly harmless (we might
     accidentally close some other open descriptor, but at that point
     we're just closing and unlinking everything we can).

     But in theory it could also cause subtle bugs. If do_write_index()
     fails, we return the error up the stack and would eventually end up
     in write_locked_index(). There we roll back the lock file on error,
     which will close the descriptor. So now we get our double close,
     and we might actually close something else that was opened in the
     interim.

     This is probably unlikely in practice (as soon as we see the error
     we'd mostly be unwinding the stack, not opening new files). But it
     highlights a potential problem with the discard_hashfile()
     interface: the hashfile doesn't necessarily own that descriptor.

Note that I said "descriptors" plural above. Those callers all care
about the "fd" member of the struct. But discard_hashfile() also closes
check_fd. That is only used if the struct is initialized with
hashfd_check(), and neither of its two callers call either discard or
free (they always "finalize" instead). So closing it is irrelevant for
the current callers.

I think we're better off sticking with the simpler free_hashfile()
interface, and the handful of callers can decide how to handle the
descriptors themselves.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a semi-related cleanup that is in this series because we'll be
touching the free function in a bit. And at first I thought we'd
want the discard() variant, but after poking around a bit I'm pretty
sure we don't.

I do like the name discard() better, as it makes it more clear that it
is an alternative to finalize(). Since they have the same signature,
swapping the names/implementations _could_ confuse long-running branches
or topics in flight, but I kind of doubt there are any, given the
history.

 csum-file.c | 9 ---------
 csum-file.h | 1 -
 2 files changed, 10 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index d7a682c2b6..8ca9246a80 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -101,15 +101,6 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,
 	return fd;
 }
 
-void discard_hashfile(struct hashfile *f)
-{
-	if (0 <= f->check_fd)
-		close(f->check_fd);
-	if (0 <= f->fd)
-		close(f->fd);
-	free_hashfile(f);
-}
-
 void hashwrite(struct hashfile *f, const void *buf, uint32_t count)
 {
 	while (count) {
diff --git a/csum-file.h b/csum-file.h
index a270738a7a..d1a0ff29cd 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -74,7 +74,6 @@ void free_hashfile(struct hashfile *f);
  * Finalize the hashfile by flushing data to disk and free'ing it.
  */
 int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);
-void discard_hashfile(struct hashfile *);
 void hashwrite(struct hashfile *, const void *, uint32_t);
 void hashflush(struct hashfile *f);
 void crc32_begin(struct hashfile *);
-- 
2.55.0.418.g37da59dd42