[PATCH] notes: do not drop notes written by a concurrent process

[PATCH] notes: do not drop notes written by a concurrent process

[Farid Zakaria]

"git notes add" and the other commands that modify notes read the
notes ref, update the notes tree in memory, and then commit it on top
of whatever the ref points to by then. If another process updated the
ref in the meantime, its notes are silently lost.

The fix is to remember the commit the notes were read from, and use
it as both the parent of the new notes commit and the expected old
value of the ref update. A concurrent update now makes the command
fail with "cannot lock ref" instead.

Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
---
I found this with Rewind, a deterministic VM I am building that replays
a program under many thread schedules until one fails. Two "git notes
add" commands run at once on different commits lost one of the two
notes in 15 of 32 schedules, while both exited 0. It is not specific to
Rewind: on a plain Linux machine 25 of 30 runs lost a note. How Rewind
narrowed it down to commit_notes() is written up at:

  https://fzakaria.com/2026/10/28/reading-the-code-is-the-hard-part

With this patch no notes are lost; when two commands race, one of them
fails with "cannot lock ref" and can be retried.
---
 notes-utils.c    | 15 +++++++++++++--
 notes.c          |  8 ++++++--
 notes.h          |  3 ++-
 t/t3301-notes.sh | 18 ++++++++++++++++++
 4 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index 5c1c75d5b8..0ce430304f 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -46,6 +46,7 @@ void create_notes_commit(struct repository *r,
 void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)
 {
 	struct strbuf buf = STRBUF_INIT;
+	struct commit_list *parents = NULL;
 	struct object_id commit_oid;
 
 	if (!t)
@@ -59,12 +60,22 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)
 	strbuf_addstr(&buf, msg);
 	strbuf_complete_line(&buf);
 
-	create_notes_commit(r, t, NULL, buf.buf, buf.len, &commit_oid);
+	/* Build on the commit the notes were loaded from */
+	if (!is_null_oid(&t->base_oid)) {
+		struct commit *parent = lookup_commit(r, &t->base_oid);
+		if (repo_parse_commit(r, parent))
+			die("Failed to find/parse commit %s", t->ref);
+		commit_list_insert(parent, &parents);
+	}
+
+	create_notes_commit(r, t, parents, buf.buf, buf.len, &commit_oid);
 	strbuf_insertstr(&buf, 0, "notes: ");
 	refs_update_ref(get_main_ref_store(the_repository), buf.buf,
-			t->update_ref, &commit_oid, NULL, 0,
+			t->update_ref, &commit_oid, &t->base_oid, 0,
 			UPDATE_REFS_DIE_ON_ERR);
+	oidcpy(&t->base_oid, &commit_oid);
 
+	commit_list_free(parents);
 	strbuf_release(&buf);
 }
 
diff --git a/notes.c b/notes.c
index ec9c2cb150..d71b528f37 100644
--- a/notes.c
+++ b/notes.c
@@ -1039,12 +1039,16 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	t->combine_notes = combine_notes;
 	t->initialized = 1;
 	t->dirty = 0;
+	oidclr(&t->base_oid, the_repository->hash_algo);
 
 	if (flags & NOTES_INIT_EMPTY ||
 	    repo_get_oid_treeish(the_repository, notes_ref, &object_oid))
 		goto out;
-	if (flags & NOTES_INIT_WRITABLE && refs_read_ref(get_main_ref_store(the_repository), notes_ref, &object_oid))
-		die("Cannot use notes ref %s", notes_ref);
+	if (flags & NOTES_INIT_WRITABLE) {
+		if (refs_read_ref(get_main_ref_store(the_repository), notes_ref, &object_oid))
+			die("Cannot use notes ref %s", notes_ref);
+		oidcpy(&t->base_oid, &object_oid);
+	}
 	if (get_tree_entry(the_repository, &object_oid, "", &oid, &mode))
 		die("Failed to read notes tree referenced by %s (%s)",
 		    notes_ref, oid_to_hex(&object_oid));
diff --git a/notes.h b/notes.h
index 6dc6d7b265..30f4ece5e9 100644
--- a/notes.h
+++ b/notes.h
@@ -1,9 +1,9 @@
 #ifndef NOTES_H
 #define NOTES_H
 
+#include "hash.h"
 #include "string-list.h"
 
-struct object_id;
 struct repository;
 struct strbuf;
 
@@ -54,6 +54,7 @@ extern struct notes_tree {
 	struct non_note *first_non_note, *prev_non_note;
 	char *ref;
 	char *update_ref;
+	struct object_id base_oid;
 	combine_notes_fn combine_notes;
 	int initialized;
 	int dirty;
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 18a6000171..561f9ccc18 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1629,4 +1629,22 @@ test_expect_success 'git notes append aborts when editor fails with -e' '
 	test_must_fail git notes show
 '
 
+test_expect_success 'git notes add does not drop notes added while it runs' '
+	test_commit 23rd &&
+	test_commit 24th &&
+
+	# The editor adds a note to another commit while the outer
+	# "git notes add" is waiting for it, as a concurrent process would.
+	write_script racing_editor <<-\EOF &&
+	git notes add -m "added meanwhile" 23rd &&
+	echo "edited" >"$1"
+	EOF
+	test_must_fail env GIT_EDITOR=./racing_editor git notes add 24th &&
+
+	echo "added meanwhile" >expect &&
+	git notes show 23rd >actual &&
+	test_cmp expect actual &&
+	test_must_fail git notes show 24th
+'
+
 test_done

---
base-commit: 8d1b215c01512b9658d4525c54ff9b9245f9a7e8
change-id: 20261028-fz-notes-concurrent-write-4ee590954ece

Re: [PATCH] notes: do not drop notes written by a concurrent process

[Kristoffer Haugsbakk]

On Thu, Oct 29, 2026, at 04:55, Farid Zakaria wrote:
> "git notes add" and the other commands that modify notes read the
> notes ref, update the notes tree in memory, and then commit it on top
> of whatever the ref points to by then. If another process updated the
> ref in the meantime, its notes are silently lost.

This looks like a really nice find.

>
> The fix is to remember the commit the notes were read from, and use
> it as both the parent of the new notes commit and the expected old
> value of the ref update. A concurrent update now makes the command
> fail with "cannot lock ref" instead.

This makes sense and is how I would have assumed `git notes add` and
similar to work. Because you have:

    git update-ref <ref> <new-oid> [<old-oid>

Exactly so that you can try to update a ref with the assumption that
nothing else intervenes, because it will fail if <old-oid> does not
become the previous OID.

So I thought that the code that git-notes(1) uses would work
the same way.

>
> Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
> ---
>[snip]

So if this change is:

• Before it silently dropped the change
• Now it loudly fails

Then that is a pure improvement.

However, thinking beyond this patch. Let’s say one program wants to
add a note on `master`. But another one wants to add a note on
`feature1`. One will fail. But should that happen in a perfect world?
Let’s say that `master` wins. `feature1` is not trying to “clobber”
anything relative to the `master` program; it wants to add a note to a
different commit. And does the `feature1` program care if the commit
corresponding to the note is a child of the commit when it first started
the process? I don’t think so. It does matter for two programs that want
to update a branch, but here the commit structure is a kind of
implementation detail, how Notes are implemented so that you get
snapshot of the notes as well as the whole edit history. As an example:

    commit A, commit B, commit C, commit D...

It could just be shuffled.

Instead, maybe this would happen in an ideal world:

1. `master` and `feature1` race to commit a note
2. `master` wins
3. `feature1` notices that there is a new note commit (from `master`)
4. It checks whether that commit changed the commit that *it*
   (`feature1`) wanted to update
   • If so: fail
   • If not: continue
5. `feature1` retries (go to (1))

Maybe this is too much logic for a “porcelain” command (interactive
use), but there is no plumbing Notes command.

Re: [PATCH] notes: do not drop notes written by a concurrent process

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> Instead, maybe this would happen in an ideal world:
>
> 1. `master` and `feature1` race to commit a note
> 2. `master` wins
> 3. `feature1` notices that there is a new note commit (from `master`)
> 4. It checks whether that commit changed the commit that *it*
>    (`feature1`) wanted to update
>    • If so: fail
>    • If not: continue
> 5. `feature1` retries (go to (1))
>
> Maybe this is too much logic for a “porcelain” command (interactive
> use), but there is no plumbing Notes command.

Or possibly

 1. two races. Both remembers what commit was at the tip of the
    notes tree (call it commit #1).

 2. each creates a new note commit on top of #1.  Call them commit
    #2master and #2feature1.

 3. 'master' one wins the race.  As #2master is a fast-forward from
    #1, the notes tree gets #2master at its tip.

 4. 'feature1' loses the race.  It merges #2feature1 into, or
    rebases #2feature1 on top of, #2master and updates the notes
    tree.

;-)

Re: [PATCH] notes: do not drop notes written by a concurrent process

[Farid Zakaria]

On Thu Oct 29, 2026 at 11:25 AM PDT, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
>
>> Instead, maybe this would happen in an ideal world:
>>
>> 1. `master` and `feature1` race to commit a note
>> 2. `master` wins
>> 3. `feature1` notices that there is a new note commit (from `master`)
>> 4. It checks whether that commit changed the commit that *it*
>>    (`feature1`) wanted to update
>>    • If so: fail
>>    • If not: continue
>> 5. `feature1` retries (go to (1))
>>
>> Maybe this is too much logic for a “porcelain” command (interactive
>> use), but there is no plumbing Notes command.
>
> Or possibly
>
>  1. two races. Both remembers what commit was at the tip of the
>     notes tree (call it commit #1).
>
>  2. each creates a new note commit on top of #1.  Call them commit
>     #2master and #2feature1.
>
>  3. 'master' one wins the race.  As #2master is a fast-forward from
>     #1, the notes tree gets #2master at its tip.
>
>  4. 'feature1' loses the race.  It merges #2feature1 into, or
>     rebases #2feature1 on top of, #2master and updates the notes
>     tree.
>
> ;-)

Your proposals for retries when possible make sense but I may leave that
to you to implement.

I felt confident reviewing the current patch at it's current state
(AI-assisted) and I do concur that it's a net-positive change in the
interim.

(I guess I could try to implement the retry logic - AI
assisted if you like)

Would it be OK if I also suggested the other two bugs (+small fixes)
I found on the mailing list (with the same tool)?

They are:
* Similar race-type bug for credential-cache leaving a stale daemon+credential
* fsmonitor daemon keeps running after its worktree is deleted or moved

Both patches are of similar size and complexity to this one, with a test
case.

Re: [PATCH] notes: do not drop notes written by a concurrent process

[Junio C Hamano]

"Farid Zakaria" <farid.m.zakaria@gmail.com> writes:

> I felt confident reviewing the current patch at it's current state
> (AI-assisted) and I do concur that it's a net-positive change in the
> interim.

Sorry, but I find the above very confusing, and I cannot figure out
who is reviewing whose patch, and who is concuring whose statement
that said what.

Re: [PATCH] notes: do not drop notes written by a concurrent process

[Farid Zakaria]

On Thu Oct 29, 2026 at 2:42 PM PDT, Junio C Hamano wrote:
> "Farid Zakaria" <farid.m.zakaria@gmail.com> writes:
>
>> I felt confident reviewing the current patch at it's current state
>> (AI-assisted) and I do concur that it's a net-positive change in the
>> interim.
>
> Sorry, but I find the above very confusing, and I cannot figure out
> who is reviewing whose patch, and who is concuring whose statement
> that said what.

It's because I bottom-posted; I thought it might be easier to read that
way -- I guess not.

I was just trying to say that I felt confident in my comprehension of
this current patch, and agreed with Kristoffer that it's a positive
change as it stands, although the algorithm could be improved to handle
cases with different branches as mentioned.

(hopefully that's a little more concise and clear)