Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

From: Jeff King <peff@peff.net>
Date: Jan 30, 2017, 23:37 UTC
Message-ID: <20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net>
In-Reply-To: <xmqq37g0us5p.fsf@gitster.mtv.corp.google.com>

On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> >     When writing the test for git-tag, I realized that the option
> >     --no-create-reflog to git-tag does not take precedence over
> >     logAllRefUpdate=always. IOW the setting cannot be overridden on the command
> >     line. Do you think this is a defect or would it not be desirable to have this
> >     feature anyway?
> 
> "--no-create-reflog" should override the configuration set to "true"
> or "always".  Also "--create-reflog" should override the
> configuration set to "false".
> 
> If the problem was inherited from the original code before your
> change (e.g. you set logAllRefUpdates to true and then did
> "update-ref --no-create-reflog refs/heads/foo".  Does the code
> before your change ignore the command lne option and create a reflog
> for the branch?), then it would be ideal to fix the bug before this
> series as a preparatory fix.  If the problem was introduced by this
> patch set, then we would need a fix not to introduce it ;-)

I hadn't thought about that. I think "git branch --no-create-reflog" has the same problem in the existing code.

I suspect nobody cares much in practice. Even if you say "don't create a reflog now", if you have core.logAllRefUpdates turned on, then it's likely that some _other_ operation will create the reflog later accidentally (e.g., as soon as you "git checkout foo && git commit", you'll get a reflog). I think you're fighting an uphill battle to turn logAllRefUpdates on and then try to disable some reflogs selectively.

So I agree the current behavior is quietly broken, which is not good. But I wonder if "--no-create-reflog" is really sane in the first place, and whether we might be better off to simply disallow it.

-Peff