I honenstly find this function a bit strange. It makes it harder than necessary to apply multiple diffs to a tree in succession, as you first would need to derive a tree from the index again and write the resulting object into the ODB. Wouldn't it make more sense to have it return a tree, as well, or is there a specific reason why you provide it like this?

It's a bit hard to read due to indentation. The second (error = git_blob_lookup(... line should probably be dedented by 4 spaces.

Fixed.

No need to clear if you use git_buf_set

Fixed.

Instead of having all these noop foo_reader_free function we could just pass a NULL pointer and have git_reader_free only call the function if it's set.

It turns out that none of the readers provide a free implementation, so I'll remove it entirely.

Removed.

Shouldn't we increment the refcount of the tree and decrement it in tree_reader_free again? We already had lifecycle problems in other places using diff where we didn't increment the refcount to keep it alive

I don't think that's necessary. These readers really just provide a handy abstraction over querying them, and their lifecycle is never longer than the apply functions -- meaning that a caller couldn't try to free a tree or index in the middle of application (at least not on a single thread).

I'd prefer going for safety here IMHO, just to make sure the first person to use git_reader in a "non-atomic" fashion doesn't get to debug lifecycle problems. Though a comment stating the implementation doesn't support that would be fine as well 😉.

While not necessary now, I agree with @tiennou -- future proofing the API here might be sensible, as one will forget about this limitation and eventually be bitten by the limitation. The only thing keeping you from that is the additional required free function pointer, right?

This does not have an implementation. Probably a leftover?

Removed.

I'd prefer if we used a switch here. Like that, the compiler would throw a warning in case we ever add another enum

Changed this to a switch.

No need to handle BOTH here, I guess, because of the way we initialized the reader? I'd personally vote for a switch, as well

I'm not excited about using a switch here, but I also don't care very much, so I changed it.

I find this nested if confusing (especially given the implicit != 0 check).

if (error > 0) {
    /* current code */
} else if (error < 0) {
    goto done;
}

maybe ?

Also, GIT_EUSER comes to mind 😉.

Why does GIT_EUSER come to mind?

I might have mistook that for GIT_PASSTHROUGH, sorry.

Same complaint as above.

I wrote up your suggestion, but I found it less readable than what's here now, tbqh. @pks-t can you break a tie?

A:

if (error > 0) {
    error = 0;
    goto done;
} else if (error < 0) {
    goto done;
}

B:

if (error) {
    if (error > 0)
        error = 0;

    goto done;
}

I think I prefer style B, which matches what you have right now. Style A is a bit more subtle, as the reader would have to spot that neither of the conditionals catches the case error == 0, which is not that easy to see. Style B is a lot more explicit, as one cannot miss the fact that positive error codes are being reset.

I'd prefer going for safety here IMHO, just to make sure the first person to use git_reader in a "non-atomic" fashion doesn't get to debug lifecycle problems. Though a comment stating the implementation doesn't support that would be fine as well 😉.

Would git_apply_to_tree be a better name? Right now it indicates that we try to apply a tree, while we in fact apply a diff to a tree.

Yes, I agree. Fixed.

I think I prefer style B, which matches what you have right now. Style A is a bit more subtle, as the reader would have to spot that neither of the conditionals catches the case error == 0, which is not that easy to see. Style B is a lot more explicit, as one cannot miss the fact that positive error codes are being reset.

I think it'd make sense to also have an error message here. You overwrite GIT_ENOTFOUND, so the caller has no way of knowing that we failed due to the file not existing. So we should at least set an error message:

if (error == GIT_ENOTFOUND)
    error = apply_err("%s: file not found", delta->old_file.path);

I do overwrite GIT_ENOTFOUND, but I don't overwrite the error message itself, which would have been set by the reader. This means that it's actually more explicit, since the existing error message distinguishes between the index and the working directory. eg, if it's not found in the index, it will say index does not contain 'foo'.

Instead of skipping the rest of this function, you could make this code path similar to the previous one by using if (delta_status != GIT_DELTA_DELETED), moving remaining code into that condition's scope.

Indeed, I do like that better, it matches nicely the condition and formatting above it.

Nit: misformated comment

Wouldn't it be more sensible to pass an explicit index instead of an implicit one? First, this would match the previous git_reader_for_tree and second it would potentially open up further usecases

Jeez, I'm hungry already

cl_git_pass

If I didn't just skip over them, then renames are missing. Might be interesting to have different rename cases, too, like two-to-one, one-to-two and one-one (file1 -> file2, file2 -> file3).

Another test case would be the previous thing I mentioned, with having two patches for the same file in one application.

I'll add some additional tests. When you say two-to-one, you mean a mangled patch that has two sources being renamed to the same target? And by one-to-two, another mangled patch, this one with a single source being renamed to two different targets? These seem like reasonable tests.