Conversation
|
You committed a pretty big Makefile under |
|
Damn, CMake keeps overwriting that every time I need to update the file list. Think of the 20 times I didn't commit it, instead of the one I did :) |
|
The implementation looks correct, but it races. The locking behavior of Git takes into account both the loose ref file and the reflog, but I don't think this can be done atomically unless we unify reflog and refdb access. I guess there's an interdependence between those two PRS... Hm. |
|
It does race (and so does reading and appending). The only way out of that would be to make the backend know about the combined operations (we could probably simply add a |
|
For that matter, we should probably also add |
|
This is getting there. Writing a ref now forces you to write a reflog entry so we match git. That appending happens as a proper append, but that still needs to be exposed to the outside. Stash is still racy, as it uses |
|
Turns out it's not getting that close. This would currently forbit It also turns out that not all refs get a reflog, even if you ask nicecly ( |
This is as yet unused.
|
So, I think this is ready. Stash is still racy, but fixing that requires transactions and that's a whole different thing which will needs its own PR. Something I still have doubts about is what we do when there is no default signature configured. git-core will do its magical guessing, but we don't do that (nor do we want to), so we go for "unknown ". This situation is bound to be rather rare, but maybe we want to figure out something else to do? |
Whenever a reference is created or updated, we need to write to the reflog regardless of whether the user gave us a message, so we shouldn't leave that to the ref frontend, but integrate it into the backend. This also eliminates the race between ref update and writing to the reflog, as we protect the reflog with the ref lock. As an additional benefit, this reflog append on the backend happens by appending to the file instead of parsing and rewriting it.
This was a convenience method for the refs front-end to do the reflog writing. This is now done in the backend and it has no more reason for being.
|
Is there anything left to fix in the scope of this PR? Once this is merge we can more easily work on conditional updates and transactions. |
include/git2/refs.h
Outdated
There was a problem hiding this comment.
@return? From the name I'm kind of expecting a true/false type, but the doc-string makes me think this has side effects, so it could be an error code?
There was a problem hiding this comment.
Then maybe "ensure" isn't a good name. What this is meant to do is to force the backend to write a log for the given reference. The return is the usual "0 or error code". I'll add that. Do you have a better name for this maybe?
There was a problem hiding this comment.
Oh, whoops. My eyes wandered up to git_reference_has_log, then I read this doc-comment, and I associated the two.
So the only nitpick I have is the missing @return. Everything else is fine. 😊
|
Super nitpicky things. I 😄 the new APIs, and I 😍 having test coverage. |
git-core only writes to the reflogs of HEAD, refs/heads/ and, refs/notes/ or if there is already a reflog in place. Adjust our code to follow these semantics.
Sometimes (e.g. stash) we want to make sure that a log will be written, even if it's not in one of the standard locations. Let's make that easier.
The frontend used to look at the file directly, but that's obviously not the right thing to do. Expose it on the backend and use that function instead.
|
Nits un-picked (or something). The test coverage is obviously @nulltoken 's doing. |
|
From @nulltoken in #1577 (posting here to keep conversation in one place):
What's the rationale for that? The plain variants still write to the reflog—just without any useful information—so a couple of optional parameters would make more sense to me. As a maintainer of a bindings library, breaking changes (like adding parameters) here would actually be really helpful, because I was having trouble figuring out why the reflog was getting populated with junk. |
|
At the Failing to compile would certainly provide a strong message that something has changed, but the basic call should look like the one we were already providing, and every reference update needs to have a corresponding reflog update which is performed under the reference lock, so this is fixing a bug you were working around in a racy manner. We should figure out a way to broadcast these type of changes to users of the library. |
Unless something's changed since I looked at the code, it still creates a reflog entry, just without a message. That's unnecessarily confusing to me—if an entry will always be written, I'd say there should always be a |
|
That's right, it will create a reflog entry with no message unless you provide one. This is how A well-behaved application should provide a message, but Git doesn't require it. It does want to have an entry for every change. |
|
I'm inclined to agree with @jspahrsummers here. Given that the only difference between these APIs is that one takes a couple of optional extra parameters, why do we even need separate APIs? It's easy enough to just pass NULLs for the optional values if you want to use the default / empty values and it would reduce the number of APIs. |
|
We can change it and make it more awkward in C not to pass the reflog data. It would diverge from what we've been doing, though it might not necessarily be a bad thing. I have cursed the amount of functions when doing other work on the refs. So if there's some consensus that we should remove the |
I'll +1 this. |
Reference operations with log
This is basically @nulltoken 's #1577 but on top of the new reflog API which @vmg wanted to see. We don't need to load the reflog ourselves before writing it back out (though it still happens in the background)