On Mon, Aug 04, 2025 at 09:38:29AM +0200, Patrick Steinhardt wrote:
> > Shouldn't we quietly drop the HEAD reflog update, rather than forcing
> > the whole transaction to fail? The user never asked us to update HEAD at
> > all. It was something we opportunistically decided to do, and now we
> > find out that it is not appropriate to do so.
>
> That's something I wasn't quite sure about, either. Honestly, the reason
> I shied away from it is that it needs a bit more munging for an edge
> case that is hard to test reliably. But I guess we can do something like
> the below patch to skip writing the reflog message instea.
Yeah, I wondered what it would look like to drop a single update from a
transaction, since that's not something we currently allow. And indeed,
this is a bit scary:
> @@ -2626,8 +2632,16 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re
> */
> if (!(update->type & REF_ISSYMREF) ||
> strcmp(update->parent_update->refname, referent.buf)) {
> - strbuf_addstr(err, "HEAD has been racily updated");
> - ret = REF_TRANSACTION_ERROR_GENERIC;
> + if (unlock_ref(lock))
> + strmap_remove(&backend_data->ref_locks,
> + update->refname, 0);
> +
> + memmove(transaction->updates + update_idx,
> + transaction->updates + update_idx + 1,
> + (transaction->nr - update_idx - 1) * sizeof(*transaction->updates));
> + transaction->nr--;
> +
> + ret = 1;
> goto out;
> }
Not because it's necessarily wrong, but it feels like a maintainability
problem, when the transaction code learns some new struct field similar
to "ref_locks", and we have to update it here, too. I dunno. Pulling it
out into a "transaction_drop_update()" helper would make that a bit more
obvious, but you're right that the fundamental issue is that we're not
going to be testing this very well.
Maybe erroring out, as your original patch did, is the least-bad thing,
then? I think that _might_ even be what happens in the current code as
an emergent behavior. We leave HAVE_OLD_OID set, so we'd expect HEAD to
resolve to the same thing it originally did. If you've pointed it
elsewhere, then it probably would fail to resolve to that same oid
(unless you pointed to a different branch with the same tip commit).
I really wish there was an easy way to test this. I guess something like
this:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 89ae4517a9..6aeec2e8e0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2393,6 +2393,7 @@ static enum ref_transaction_error split_head_update(struct ref_update *update,
const char *head_ref,
struct strbuf *err)
{
+ static int force_split = -1;
struct ref_update *new_update;
if ((update->flags & REF_LOG_ONLY) ||
@@ -2401,7 +2402,10 @@ static enum ref_transaction_error split_head_update(struct ref_update *update,
(update->flags & REF_UPDATE_VIA_HEAD))
return 0;
- if (strcmp(update->refname, head_ref))
+ if (force_split < 0)
+ force_split = git_env_bool("GIT_TEST_FORCE_SPLIT_HEAD_UPDATE", 0);
+
+ if (!force_split && strcmp(update->refname, head_ref))
return 0;
/*
along with:
git commit --allow-empty one
git commit --allow-empty two
GIT_TEST_FORCE_SPLIT_HEAD_UPDATE=1 git branch foo HEAD^
creates roughly the situation (HEAD was never pointed at "foo", but
we'll create the reflog update for it anyway). It does fail with:
fatal: cannot lock ref 'HEAD': reference already exists
even before your patch. And after, we get:
fatal: HEAD has been racily updated
So it probably is just not something that happens very often, as I don't
recall ever seeing any discussion of it.
I dunno. Looks like you posted a new version of the series that loosens
this, so I'll take a peek at that (I also wondered whether what you
posted above leaks entries in the update struct, so maybe you've dealt
with that).
-Peff
