Re: [PATCH v3 7/9] refs/files: detect race when generating reflog entry for HEAD
- From
Jeff King <peff@peff.net>
- Date
- Aug 4, 2025, 14:47 UTC
- Message-ID
- <20250804144725.GA109984@coredump.intra.peff.net>
- In-Reply-To
- <aJBjdWv9qUeBL25-@pks.im>
On Mon, Aug 04, 2025 at 09:38:29AM +0200, Patrick Steinhardt wrote:
Show 9 quoted lines
> > 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:
Show 18 quoted lines
> @@ -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