Re: [PATCH v2 16/21] branch: stop modifying `log_all_ref_updates` variable

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 12, 2024, 11:17 UTC
Message-ID: <ZuLNwFIW2CQuHTI4@pks.im>
In-Reply-To: <6hoykgw3gm5hchmtfaci6gwwha6iouidtqvupx55vsjfoedquo@nfgaumnmu4po>

On Wed, Sep 11, 2024 at 12:14:55PM -0500, Justin Tobler wrote:

Show 46 quoted lines

> On 24/08/30 11:09AM, Patrick Steinhardt wrote:
> > In "branch.c" we modify the global `log_all_ref_updates` variable to
> > force creation of a reflog entry. Modifying global state like this is
> > discouraged, as it may have all kinds of consequences in other places of
> > our codebase.
> > 
> > Stop modifying the variable and pass the `REF_FORCE_CREATE_REFLOG` flag,
> > which has the same effect.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  branch.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/branch.c b/branch.c
> > index c887ea21514..08fa4094d2b 100644
> > --- a/branch.c
> > +++ b/branch.c
> > @@ -601,6 +601,7 @@ void create_branch(struct repository *r,
> >  	int forcing = 0;
> >  	struct ref_transaction *transaction;
> >  	struct strbuf err = STRBUF_INIT;
> > +	int flags = 0;
> >  	char *msg;
> >  
> >  	if (track == BRANCH_TRACK_OVERRIDE)
> > @@ -619,7 +620,7 @@ void create_branch(struct repository *r,
> >  		goto cleanup;
> >  
> >  	if (reflog)
> > -		log_all_ref_updates = LOG_REFS_NORMAL;
> > +		flags |= REF_FORCE_CREATE_REFLOG;
> 
> I'm trying to understand how setting the `REF_FORCE_CREATE_REFLOG` flag
> is ultimately equivalent to setting `log_all_ref_updates =
> LOG_REFS_NORMAL`.
> 
> From my understanding, when `log_all_ref_updates` is set to
> `LOG_REFS_NORMAL`, this is the equivalent to
> `core.logAllRefUpdates=true`. This means if a missing reflog file is
> created for a subset of references[1]. The `REF_FORCE_CREATE_REFLOG`
> flag seems more analogous to `always` value for `core.logAllRefUpdates`
> and would create reflogs for all references. Is this not the case? Or
> does it not really matter?
> 
> [1] https://git-scm.com/docs/git-config#Documentation/git-config.txt-corelogAllRefUpdates

It's not equivalent in general, but in this case it is. Note that we are only talking about branches here, not about arbitrary references. Quoting "core.logAllRefUpdates":

    If this configuration variable is set to true, missing
    "$GIT_DIR/logs/<ref>" file is automatically created for branch heads
    (i.e. under refs/heads/), remote refs (i.e. under refs/remotes/),
    note refs (i.e. under refs/notes/), and the symbolic ref HEAD. If it
    is set to always, then a missing reflog is automatically created for
    any ref under refs/.

So for branches, "true" and "always" have the same effect on the technical level.

And on the conceptual level, `reflog` is set whenever "--create-reflog" is passed to git-branch(1), and asks the command to cretae the reflog regardless of "core.logAllRefUpdates".

I'll add this information to the commit message.

Thanks for your review!

Patrick