Delete temporary packfile in indexer#4060
Conversation
This change deletes the temporary packfile that the indexer creates to avoid littering the pack/ directory with garbage.
|
in |
|
Now with a test that actually fails if I remove the check in |
I forgot that Windows chokes while trying to delete open files.
|
now the tests actually pass -_-. PTAL. |
ethomson
left a comment
ethomson
left a comment
There was a problem hiding this comment.
Thanks for fixing this - I have some stylistic comments, but on the whole it looks good! Thanks!
| if (fd != -1) | ||
| p_close(fd); | ||
|
|
||
| if (git_buf_is_allocated(&tmp_path)) |
There was a problem hiding this comment.
git_buf_len(&tmp_path) is correct here.
is_allocated literally looks at whether we have malloced the structure, not whether it has contents. These are orthogonal concepts: the buffer may point to static memory and thus be nonempty but not allocated, similarly, we could allocate a buffer, set contents into it, then clear the contents, but it would still be allocated.
| p_close(fd); | ||
|
|
||
| if (git_buf_is_allocated(&tmp_path)) | ||
| (void)p_unlink(git_buf_cstr(&tmp_path)); |
There was a problem hiding this comment.
We do not cast unused return values to void. There are a few other places in here that should be removed also.
There was a problem hiding this comment.
Done (saw this was used inconsistently in a few other places).
| 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)) { |
There was a problem hiding this comment.
I wouldn't cl_skip here. Do you expect temp files might exist? Then please don't skip the test, fix it. If not, assert that they don't so that we fail.
There was a problem hiding this comment.
i expect they shouldn't exist (and currently don't), but cannot make that guarantee currently (I opened #4066 for that).
If we are cool with this test failing when other tests leak files, i can change the skip to a warning to make it clear that it wasn't this test that caused the failure, but another one.
There was a problem hiding this comment.
After a second thought, if this fails, it is obvious from the comment in L154 that it's not this test's fault, so changed it to simple assert with no log after all.
| cl_warning(git_buf_cstr(&first_tmp_file)); | ||
| git_buf_free(&first_tmp_file); | ||
| cl_fail("Found a temporary file"); | ||
| } |
There was a problem hiding this comment.
There's no need to free things on failures, we don't bother testing for memory leaks if it doesn't even work. This 5 lines can be more easily rewritten as:
cl_assert(git_buf_len(&first_tmp_file) > 0)
git_buf_free(&first_tmp_file);
(Which, I think, is probably what the test above should look like.)
3 participants
This change deletes the temporary packfile that the indexer creates to
avoid littering the pack/ directory with garbage.