Re: [PATCH] t1400: fix --no-create-reflog test and description

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 21, 2024, 12:08 UTC
Message-ID: <ZxZETN7WjbNiSRyF@pks.im>
In-Reply-To: <ab7d4c8d89c075de05bf04f1f9dc195145455964.1729439476.git.code@khaugsbakk.name>

On Sun, Oct 20, 2024 at 06:12:03PM +0200, kristofferhaugsbakk@fastmail.com wrote:

Show 11 quoted lines

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
> This test has had the wrong title since it was created.[1]  Use `always`
> like the description says and negate the expected outcome.
> 
> The test works since `core.logAllRefUpdates` set to `true` does not
> create a reflog for that ref namespace.  So the test is testing the
> config variable, not the option.
> 
> The test itself is fine and does not hide a bug: `--no-create-reflog` is
> not supposed to override `core.logAllRefUpdates`.

That's actually surprising to me, as command line options tend to override configuration.

Show 17 quoted lines

> † 1: 341fb286212 (refs: add option core.logAllRefUpdates = always,
>     2017-01-27)
> 
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> 
> Notes (series):
>     From the commit message:
>     
>       “ The test itself is fine and does not hide a bug:
>         `--no-create-reflog` is not supposed to override
>     
>     A source for that: roundabout through git-branch(1):
>     
>       “ The negated form --no-create-reflog only overrides an earlier
>         --create-reflog, but currently does not negate the setting of
>         core.logAllRefUpdates.

Hm. The "currently" reads as if this was a known shortcoming rather than by design.

Show 8 quoted lines

>     I *suppose* that the same applies to update-ref since (I suppose) they
>     use the same underlying machinery.
>     
>     See also git-tag(1) which says the same thing.
>     
>     update-ref should document the same thing, then.  I have that marked as
>     a todo item.  The changes there are a bit too involved to implicate in
>     this submission.

So I'm quite torn here. It's documented, even though the documentation doesn't exactly feel like this was designed, but rather like it was a side effect. The test also contradicts the documentation, even though it only worked by chance. And as mentioned above, everywhere else we typically have a design where the command line option overrides the config.

Overall I'm rather leaning into the direction of making this work properly. But that would of course be a backwards-incompatible change.

Patrick