Fixes #6344: git_branch_move now renames the reflog instead of deleting. #6345
Conversation
|
Please don't merge yet, may need a tweak in error recovery. |
|
Ok, ready to merge is CI and reviewers are happy. :) |
src/libgit2/refdb_fs.c
Outdated
| @@ -1769,26 +1769,35 @@ static int refdb_fs_backend__rename( | |||
| (error = refdb_fs_backend__lookup(&old, _backend, old_name)) < 0) | |||
| return error; | |||
|
|
|||
| /* Try to rename the refog; it's ok if the old doesn't exist */ | |||
| /* Must be done before calling refdb_fs_backend__delete, otherwise */ | |||
There was a problem hiding this comment.
Instead of trying to do all the recovery during failures - I wonder if we refactor this a bit to stop calling refdb_fs_backend__delete. It almost looks like this was factored in such a way that we could do that. Would the following work:
- lock the old
- lock the new
- create the new
- rename the reflog
- delete the old (via
delete_tailinstead ofdelete)
does this make sense? is this the right ordering if something goes wrong? 🤔
There was a problem hiding this comment.
Give me a few days to look into this. I didn't like much how it ended up, especially because if an error happens during recovering, it will mess up the error class and message that are stored in the thread storage.
I don't know if there's a way to always guarantee that everything ends up in a consistent state, since we have no transactions here. On the bright side, it's likely many of these operations usually either all fail (because the repo has no write permission, for example) or all succeed.
Regarding the refactoring: yeah, it could probably be done like that. When I was writing the patch, I hesitated between the changes in the PR or moving the reflog deletion to git_refdb_delete. I'm not entirely sure which are the assumptions behind this code, specifically if the file-system implementation of references (refdb_fs.c) should know it has to handle reflog when it does other things like deleting/moving references, or if it should only do what it is told to (read/write reference files), and the knowledge that reflog files also need to have operations performed on them as part of deletion/moving/etc should be on the layer above in refdb.c (where git_refdb_delete lives).
I am tempted to pick the latter, because a reflog would exist no matter what, regardless of the backing storage of the reference, but I need to read some more code to know if this makes sense. That, however, would require moving the reflog management to refdb.c for all functions, to keep things consistent. There are some reflog functions there already (git_refdb_reflog_read and git_refdb_should_write_reflog) which is also an indication that managing the reflog when moving/deleting branches might be the work of refdb.c and not refdb_fs.c, since it means refdb.c is aware of the existence of reflogs, even if it delegates to the implementation how to actually read or write them.
|
cross-ref: issue #6344 |
|
Sorry for the delay, "a few days" ended up being more than I expected. I couldn't make the order of the operations exactly like the one suggested by @ethomson since that order causes Per the previous comment, I don't think there's a way to always guarantee everything is rolled back (especially because the rollback itself could fail), so I went with a best effort. Let me know if this is OK to be merged. |
|
Sigh. There was something wrong. Now I think it's OK. Sorry! |
It should be the `new` pointer, not the `old` one.
|
Just as it maybe-useful to you, #4316 was an attempt at tackling (most? I don't recall 😞) the reflog-losing issues, so there might be something to salvage from it (the testcases, mostly). But it's dated and likely to require a rewrite. |
|
Thanks for the fix! |
No description provided.