From db535d0a7dfa3e5c2ded4a408234623b0241de00 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sun, 1 Jan 2017 12:45:02 -0800 Subject: [PATCH 1/6] Delete temporary packfile in indexer This change deletes the temporary packfile that the indexer creates to avoid littering the pack/ directory with garbage. --- src/indexer.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/indexer.c b/src/indexer.c index a3a866989..27f49473a 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -151,6 +151,12 @@ cleanup: if (fd != -1) p_close(fd); + if (git_buf_is_allocated(&tmp_path)) + (void)p_unlink(git_buf_cstr(&tmp_path)); + + if (idx->pack != NULL) + (void)p_unlink(idx->pack->pack_name); + git_buf_free(&path); git_buf_free(&tmp_path); git__free(idx); @@ -1084,6 +1090,9 @@ void git_indexer_free(git_indexer *idx) git_vector_free_deep(&idx->deltas); + /* Try to delete the temporary file in case it was not committed. */ + (void)p_unlink(idx->pack->pack_name); + if (!git_mutex_lock(&git__mwindow_mutex)) { git_packfile_free(idx->pack); git_mutex_unlock(&git__mwindow_mutex); From def644e48a3cd4afd0cee975d3214eeeb3671c99 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sun, 1 Jan 2017 17:35:29 -0800 Subject: [PATCH 2/6] Add a test --- tests/pack/indexer.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c index 49a106d98..15a9017f1 100644 --- a/tests/pack/indexer.c +++ b/tests/pack/indexer.c @@ -125,3 +125,43 @@ void test_pack_indexer__fix_thin(void) git_indexer_free(idx); } } + +static int find_tmp_file_recurs(void *opaque, git_buf *path) +{ + int error = 0; + git_buf *first_tmp_file = opaque; + struct stat st; + + if ((error = p_lstat_posixly(path->ptr, &st)) < 0) + return error; + + if (S_ISDIR(st.st_mode)) + return git_path_direach(path, 0, find_tmp_file_recurs, opaque); + + /* This is the template that's used in git_futils_mktmp. */ + if (strstr(git_buf_cstr(path), "_git2_") != NULL) + return git_buf_sets(first_tmp_file, git_buf_cstr(path)); + + return 0; +} + +void test_pack_indexer__no_tmp_files(void) +{ + git_indexer *idx = NULL; + git_buf path = GIT_BUF_INIT; + git_buf first_tmp_file = GIT_BUF_INIT; + + /* Precondition: there are no temporary files. */ + cl_git_pass(git_buf_sets(&path, clar_sandbox_path())); + cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); + if (git_buf_is_allocated(&first_tmp_file)) { + cl_warning("Found a temporary file before running the test"); + cl_skip(); + } + + cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); + git_indexer_free(idx); + + cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); + cl_check(!git_buf_is_allocated(&first_tmp_file)); +} From a7ff6e5e5e2715ecccdb217f2018dc2567e1eebd Mon Sep 17 00:00:00 2001 From: lhchavez Date: Tue, 3 Jan 2017 18:24:51 -0800 Subject: [PATCH 3/6] Fix the memory leak --- tests/pack/indexer.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c index 15a9017f1..2c9724605 100644 --- a/tests/pack/indexer.c +++ b/tests/pack/indexer.c @@ -154,7 +154,9 @@ void test_pack_indexer__no_tmp_files(void) /* Precondition: there are no temporary files. */ cl_git_pass(git_buf_sets(&path, clar_sandbox_path())); cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); + git_buf_free(&path); if (git_buf_is_allocated(&first_tmp_file)) { + git_buf_free(&first_tmp_file); cl_warning("Found a temporary file before running the test"); cl_skip(); } @@ -162,6 +164,12 @@ void test_pack_indexer__no_tmp_files(void) cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); git_indexer_free(idx); + cl_git_pass(git_buf_sets(&path, clar_sandbox_path())); cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); - cl_check(!git_buf_is_allocated(&first_tmp_file)); + git_buf_free(&path); + if (git_buf_is_allocated(&first_tmp_file)) { + cl_warning(git_buf_cstr(&first_tmp_file)); + git_buf_free(&first_tmp_file); + cl_fail("Found a temporary file"); + } } From 96df833b63f37c21240094aa740f05600ef9c2b9 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Tue, 3 Jan 2017 19:15:09 -0800 Subject: [PATCH 4/6] Close the file before unlinking I forgot that Windows chokes while trying to delete open files. --- src/indexer.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/indexer.c b/src/indexer.c index 27f49473a..93153621a 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -1091,6 +1091,12 @@ void git_indexer_free(git_indexer *idx) git_vector_free_deep(&idx->deltas); /* Try to delete the temporary file in case it was not committed. */ + git_mwindow_free_all(&idx->pack->mwf); + /* We need to close the descriptor here so Windows doesn't choke on unlink */ + if (idx->pack->mwf.fd != -1) { + (void)p_close(idx->pack->mwf.fd); + idx->pack->mwf.fd = -1; + } (void)p_unlink(idx->pack->pack_name); if (!git_mutex_lock(&git__mwindow_mutex)) { From f5586f5c73ac162393df10feec0117f59bbd1409 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Sat, 14 Jan 2017 16:37:00 +0000 Subject: [PATCH 5/6] Addressed review feedback --- src/indexer.c | 10 +++++----- tests/pack/indexer.c | 13 +++---------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/indexer.c b/src/indexer.c index 93153621a..3d1c81c86 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -151,11 +151,11 @@ cleanup: if (fd != -1) p_close(fd); - if (git_buf_is_allocated(&tmp_path)) - (void)p_unlink(git_buf_cstr(&tmp_path)); + if (git_buf_len(&tmp_path) > 0) + p_unlink(git_buf_cstr(&tmp_path)); if (idx->pack != NULL) - (void)p_unlink(idx->pack->pack_name); + p_unlink(idx->pack->pack_name); git_buf_free(&path); git_buf_free(&tmp_path); @@ -1094,10 +1094,10 @@ void git_indexer_free(git_indexer *idx) git_mwindow_free_all(&idx->pack->mwf); /* We need to close the descriptor here so Windows doesn't choke on unlink */ if (idx->pack->mwf.fd != -1) { - (void)p_close(idx->pack->mwf.fd); + p_close(idx->pack->mwf.fd); idx->pack->mwf.fd = -1; } - (void)p_unlink(idx->pack->pack_name); + p_unlink(idx->pack->pack_name); if (!git_mutex_lock(&git__mwindow_mutex)) { git_packfile_free(idx->pack); diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c index 2c9724605..1e514b2d6 100644 --- a/tests/pack/indexer.c +++ b/tests/pack/indexer.c @@ -155,11 +155,7 @@ void test_pack_indexer__no_tmp_files(void) cl_git_pass(git_buf_sets(&path, clar_sandbox_path())); cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); git_buf_free(&path); - if (git_buf_is_allocated(&first_tmp_file)) { - git_buf_free(&first_tmp_file); - cl_warning("Found a temporary file before running the test"); - cl_skip(); - } + cl_assert(git_buf_len(&first_tmp_file) == 0); cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); git_indexer_free(idx); @@ -167,9 +163,6 @@ void test_pack_indexer__no_tmp_files(void) cl_git_pass(git_buf_sets(&path, clar_sandbox_path())); cl_git_pass(find_tmp_file_recurs(&first_tmp_file, &path)); git_buf_free(&path); - if (git_buf_is_allocated(&first_tmp_file)) { - cl_warning(git_buf_cstr(&first_tmp_file)); - git_buf_free(&first_tmp_file); - cl_fail("Found a temporary file"); - } + cl_assert(git_buf_len(&first_tmp_file) == 0); + git_buf_free(&first_tmp_file); } From d030bba9fa64c4363368f9e3e0ed3f115dc06dc2 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 21 Jan 2017 17:15:33 +0000 Subject: [PATCH 6/6] indexer: only delete temp file if it was unused Only try to `unlink` our temp file when we know that we didn't copy it into its permanent location. --- src/indexer.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/indexer.c b/src/indexer.c index 3d1c81c86..aa5646023 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -33,7 +33,7 @@ struct entry { struct git_indexer { unsigned int parsed_header :1, - opened_pack :1, + pack_committed :1, have_stream :1, have_delta :1; struct git_pack_header hdr; @@ -1060,6 +1060,7 @@ int git_indexer_commit(git_indexer *idx, git_transfer_progress *stats) /* And don't forget to rename the packfile to its new place. */ p_rename(idx->pack->pack_name, git_buf_cstr(&filename)); + idx->pack_committed = 1; git_buf_free(&filename); git_hash_ctx_cleanup(&ctx); @@ -1092,12 +1093,13 @@ void git_indexer_free(git_indexer *idx) /* Try to delete the temporary file in case it was not committed. */ git_mwindow_free_all(&idx->pack->mwf); + /* We need to close the descriptor here so Windows doesn't choke on unlink */ - if (idx->pack->mwf.fd != -1) { + if (idx->pack->mwf.fd != -1) p_close(idx->pack->mwf.fd); - idx->pack->mwf.fd = -1; - } - p_unlink(idx->pack->pack_name); + + if (!idx->pack_committed) + p_unlink(idx->pack->pack_name); if (!git_mutex_lock(&git__mwindow_mutex)) { git_packfile_free(idx->pack);