From d39cc7185e0c1529917c0407036c9b09a94dd5ee Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2024 23:47:08 -0400 Subject: sparse-checkout: consolidate cleanup when writing patterns In write_patterns_and_update(), we always need to free the pattern list before exiting the function. Rather than handling it manually when we return early, we can jump to an "out" label where cleanup happens. This let us drop one line, but also establishes a pattern we can use for other cleanup. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 2604ab04df..dfefe609a1 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -343,9 +343,8 @@ static int write_patterns_and_update(struct pattern_list *pl) result = update_working_directory(pl); if (result) { rollback_lock_file(&lk); - clear_pattern_list(pl); update_working_directory(NULL); - return result; + goto out; } fp = xfdopen(fd, "w"); @@ -358,9 +357,9 @@ static int write_patterns_and_update(struct pattern_list *pl) fflush(fp); commit_lock_file(&lk); +out: clear_pattern_list(pl); - - return 0; + return result; } enum sparse_checkout_mode { -- cgit v1.3-5-g9baa From 19ace71de05465c690d4eb297bd5d503f8e312b1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2024 23:47:38 -0400 Subject: sparse-checkout: check commit_lock_file when writing patterns When writing a new "sparse-checkout" file, we do the usual strategy of writing to a lockfile and committing it into place. But we don't check the outcome of commit_lock_file(). Failing there would prevent us from writing a bogus file (good), but we would ignore the error and return a successful exit code (bad). Fix this by calling die(). Note that we need to keep the sparse_filename variable valid for longer, since the filename stored in the lock_file struct will be dropped when we run commit_lock_file(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index dfefe609a1..b5e220cc44 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -338,7 +338,6 @@ static int write_patterns_and_update(struct pattern_list *pl) fd = hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR); - free(sparse_filename); result = update_working_directory(pl); if (result) { @@ -355,10 +354,12 @@ static int write_patterns_and_update(struct pattern_list *pl) write_patterns_to_file(fp, pl); fflush(fp); - commit_lock_file(&lk); + if (commit_lock_file(&lk)) + die_errno(_("unable to write %s"), sparse_filename); out: clear_pattern_list(pl); + free(sparse_filename); return result; } -- cgit v1.3-5-g9baa From a71c47825d2650cfe53217d860107d9457cc375c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2024 23:48:41 -0400 Subject: 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. - 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 | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index b5e220cc44..5ccf696862 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); result = update_working_directory(pl); if (result) { @@ -346,14 +344,15 @@ static int write_patterns_and_update(struct pattern_list *pl) goto out; } - 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); if (commit_lock_file(&lk)) die_errno(_("unable to write %s"), sparse_filename); -- cgit v1.3-5-g9baa