Conversation
Signed-off-by: Sven Strickroth <email@cs-ware.de>
| @@ -3246,14 +3246,18 @@ int git_repository_set_workdir( | |||
| if (git_fs_path_prettify_dir(&path, workdir, NULL) < 0) | |||
| return -1; | |||
|
|
|||
| if (repo->workdir && strcmp(repo->workdir, path.ptr) == 0) | |||
There was a problem hiding this comment.
Just wanted to propose ad additional optimisation: I don't see that much sense in allocating a string and potentially to deallocate it. I think we should do it if and only if workdir != repo->workdir (namely we should call git_fs_path_prettify_dir after checking that we really have a new different working dir).
if (!repo->workdir || strcmp(repo->workdir, wordir) == 0)
return 0;
if (git_fs_path_prettify_dir(&path, workdir, NULL) < 0)
return -1;
There was a problem hiding this comment.
This is not logically equivalent, though. Currently, we're comparing the canonicalized input against the existing workdir (which is also already canonicalized). If we do the comparison before the canonicalization (prettify), then if repo->workdir = "/path/to/current/workdir/" and set_workdir was called with /path/to/current/workdir, then the strcmp would not be equivalent, and we'd go through the whole "change the current workdir" logic incorrectly.
There was a problem hiding this comment.
Oh, I see! Sorry again (I didn't check too much in detail what git_fs_path_prettify_dir was really doing).
There was a problem hiding this comment.
No worries at all - I appreciate the extra set of eyes!
| @@ -3246,14 +3246,18 @@ int git_repository_set_workdir( | |||
| if (git_fs_path_prettify_dir(&path, workdir, NULL) < 0) | |||
There was a problem hiding this comment.
I have one comment about this call as well. When checking its implementation there are 2 paths where we do not deallocate the buffer if the function fails, while there's one where we correctly deallocate path.
Here where we do not deallocate in case of error:
Line 389 in 99018a0
Line 403 in 99018a0
I apologise if I am wrong (not too familiar with libgit2 codebase), but I would assume a function does clean up in case of error for all paths, and because of this a fix like this:
if (git_fs_path_prettify_dir(&path, workdir, NULL) < 0) {
git_str_dispose(&path);
return -1;
}
won't work because we might hit a double-free problem.
There was a problem hiding this comment.
I think that this is safe as it stands — there's a couple of things worth point out:
-
The library's philosophy (as of today) is that we will accept a few memory leaks during an out-of-memory scenario, on the assumption that we're about to exit. This admittedly seems to assume a lot, and I think that we should revisit these assumptions, but today, you will see a lot of instances of things like (pseudo-code):
if ((ptr1 = malloc(1024)) == NULL || (ptr2 = malloc(1024)) == NULL) return NULL;This would obviously leak
ptr1's memory in the case that theptr2allocation failed, but this is the design as it exists today. The thinking is that if you can't allocate a few kilobytes, we're well and truly screwed. (Note, however, that we should not gratuitously leak memory on large allocation failures.) -
Here, though, we're using a
git_str, which should be self-contained. If any of thegit_str_...functions fail, thegit_stritself should probably not be mutated. For example, given:git_str_clear(&foo); git_str_puts(&foo, "bar"); git_str_puts(&foo, twenty_gigs_of_content);if that huge append at the end fails, then thegit_strshould still containbar. So I think that trying to do any cleanup infs_path_prettify_dirwould be incorrect. -
You can safely call
git_str_dispose()multiple times, agit_strwon't double-free.
|
Thanks for the fix! |
fixes #6746