| Message ID | fc30365e1f13d47eb89339603f6a4744525b3967.1725008898.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | environment: guard reliance on `the_repository` | expand |
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 > > if (forcing) > msg = xstrfmt("branch: Reset to %s", start_name); > @@ -630,7 +631,7 @@ void create_branch(struct repository *r, > if (!transaction || > ref_transaction_update(transaction, ref.buf, > &oid, forcing ? NULL : null_oid(), > - NULL, NULL, 0, msg, &err) || > + NULL, NULL, flags, msg, &err) || > ref_transaction_commit(transaction, &err)) > die("%s", err.buf); > ref_transaction_free(transaction); > -- > 2.46.0.421.g159f2d50e7.dirty >
On Wed, Sep 11, 2024 at 12:14:55PM -0500, Justin Tobler wrote: > 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
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; if (forcing) msg = xstrfmt("branch: Reset to %s", start_name); @@ -630,7 +631,7 @@ void create_branch(struct repository *r, if (!transaction || ref_transaction_update(transaction, ref.buf, &oid, forcing ? NULL : null_oid(), - NULL, NULL, 0, msg, &err) || + NULL, NULL, flags, msg, &err) || ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction);
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(-)