On Thu, Jan 26, 2017 at 03:06:40PM +0100, Cornelius Weig wrote: > > But it works quite by accident. I wonder if we should this > > "is_bare_repository" check into a function that can be called instead of > > accessing log_all_ref_updates() directly. > > Are you saying that we should move the `!log_all_ref_updates` check into > its own function where we should also check `is_bare_repository`? I > don't see that this would win much, because as you said: checkouts in a > bare repo are forbidden anyway.
Yes, I'm suggesting making something like the should_autocreate_reflog() function public. I agree it is working correctly now. It's just that it's rather subtle that it treats LOG_REFS_UNSET implicitly as LOG_REFS_NONE. It would also possibly break if more values are added to the enum (depending on what those values are). > However, I realized that I have not written tests about ref updates in a > bare repository. Do you think it's worthwile? There should already be a test for logAllRefUpdates=true in a bare repository (if there isn't, we should probably add one). Testing the "always" case individually does not add much over testing it in a non-bare repository. IMHO. -Peff
