From 07950e019a6c4330cfe7136404b74efe49cf054f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2024 04:27:49 -0400 Subject: [PATCH 1/2] sparse-checkout: use fdopen_lock_file() instead of xfdopen() When updating sparse patterns, we open a lock_file to write out the new data. The lock_file struct holds the file descriptor, but we call fdopen() to get a stdio handle to do the actual write. After we finish writing, we fflush() so that all of the data is on disk, and then call commit_lock_file() which closes the descriptor. But we never fclose() the stdio handle, leaking it. The obvious solution seems like it would be to just call fclose(). But when? If we do it before commit_lock_file(), then the lock_file code is left thinking it owns the now-closed file descriptor, and will do an extra close() on the descriptor. But if we do it before, we have the opposite problem: the lock_file code will close the descriptor, and fclose() will do the extra close(). We can handle this correctly by using fdopen_lock_file(). That leaves ownership of the stdio handle with the lock_file, which knows not to double-close it. We do have to adjust the code a bit: - we have to handle errors ourselves; we can just die(), since that's what xfdopen() would have done (and we can even provide a more specific error message). - we no longer need to call fflush(); committing the lock-file auto-closes it, which will now do the flush for us. As a bonus, this will actually check that the flush was successful before renaming the file into place. Let's likewise report when committing the lock fails (rather than quietly returning success from the command). - we can get rid of the local "fd" variable, since we never look at it ourselves now Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 2604ab04df5aee..f1bd31b2f78869 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl) { char *sparse_filename; FILE *fp; - int fd; struct lock_file lk = LOCK_INIT; int result; @@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl) if (safe_create_leading_directories(sparse_filename)) die(_("failed to create directory for sparse-checkout file")); - fd = hold_lock_file_for_update(&lk, sparse_filename, - LOCK_DIE_ON_ERROR); + hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR); free(sparse_filename); result = update_working_directory(pl); @@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl) return result; } - fp = xfdopen(fd, "w"); + fp = fdopen_lock_file(&lk, "w"); + if (!fp) + die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk)); if (core_sparse_checkout_cone) write_cone_to_file(fp, pl); else write_patterns_to_file(fp, pl); - fflush(fp); - commit_lock_file(&lk); + if (commit_lock_file(&lk)) + die_errno(_("unable to write %s"), get_locked_file_path(&lk)); clear_pattern_list(pl); From da1cc2ad26e2db2cacec72f58a010e7fd1a46343 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 5 Sep 2024 14:19:17 -0700 Subject: [PATCH 2/2] SQUASH??? --- builtin/sparse-checkout.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index f1bd31b2f78869..27d181a6129102 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -328,7 +328,7 @@ static int write_patterns_and_update(struct pattern_list *pl) char *sparse_filename; FILE *fp; struct lock_file lk = LOCK_INIT; - int result; + int result = 0; sparse_filename = get_sparse_checkout_filename(); @@ -336,19 +336,18 @@ static int write_patterns_and_update(struct pattern_list *pl) die(_("failed to create directory for sparse-checkout file")); hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR); - free(sparse_filename); result = update_working_directory(pl); if (result) { rollback_lock_file(&lk); clear_pattern_list(pl); update_working_directory(NULL); - return result; + goto out; } fp = fdopen_lock_file(&lk, "w"); if (!fp) - die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk)); + die_errno(_("unable to fdopen %s"), sparse_filename); if (core_sparse_checkout_cone) write_cone_to_file(fp, pl); @@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl) write_patterns_to_file(fp, pl); if (commit_lock_file(&lk)) - die_errno(_("unable to write %s"), get_locked_file_path(&lk)); + die_errno(_("unable to write %s"), sparse_filename); clear_pattern_list(pl); - return 0; +out: + free(sparse_filename); + return result; } enum sparse_checkout_mode {