From: Jeff King <peff@peff.net>
Date: Thu, 26 Jan 2017 14:46:11 GMT
Subject: Re: [PATCH] refs: add option core.logAllRefUpdates = always
Message-ID: <20170126144610.7tosfix4v3tah7p2@sigill.intra.peff.net>
In-Reply-To: <4faf836a-40b6-da9a-877a-3b2ce7c863df@tngtech.com>

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