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 ifworkdir != repo->workdir (namely we should callgit_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;

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 ifrepo->workdir = "/path/to/current/workdir/" andset_workdir was called with/path/to/current/workdir, then thestrcmp would not be equivalent, and we'd go through the whole "change the current workdir" logic incorrectly.

Oh, I see! Sorry again (I didn't check too much in detail whatgit_fs_path_prettify_dir was really doing).

No worries at all - I appreciate the extra set of eyes!

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 deallocatepath.

Here where we do not deallocate in case of error:

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.

I think that this is safe as it stands — there's a couple of things worth point out:

1. 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 leakptr1's memory in the case that theptr2 allocation 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 shouldnot gratuitously leak memory on large allocation failures.)

2. Here, though, we're using agit_str, which should be self-contained. If any of thegit_str_... functions fail, thegit_str itself 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_str should still containbar. So I think that trying to do any cleanup infs_path_prettify_dir would be incorrect.

3. You can safely callgit_str_dispose() multiple times, agit_str won't double-free.