[v2,2/2] notes.c: fix off-by-one error when decreasing notes fanout
diff mbox series

Message ID 20200208204404.5531-3-johan@herland.net
State New
Headers show
  • Minor fixes to git notes fanout code
Related show

Commit Message

Johan Herland Feb. 8, 2020, 8:44 p.m. UTC
As noted in the previous commit, the nature of the fanout heuristic
in the notes code causes the exact point at which we increase or
decrease the notes fanout to vary with the objects being annotated.
Since the object ids generated by the test environment are
deterministic (by design), the notes generated and tested by t3305
are always the same, and we therefore happen to see the same fanout
behavior from one run to the next.

Coincidentally, if we were to change the test environment slightly
(say by making a test commit on an unrelated branch before we start
the t3305 test proper), we not only see the fanout switch happen at
different points, we also manage to trigger a _bug_ in the notes
code where the fanout 1 -> 0 switch is not applied uniformly across
the notes tree, but instead yields a notes tree like this:


The bug occurs when we are writing out a notes tree with a newly
decreased fanout, and the notes tree contains unexpanded subtrees
that should be consolidated into the parent tree as a consequence of
the decreased fanout):

Subtrees that happen to sit at an _even_ level in the internal notes
16-tree structure (in other words: subtrees whose path - "d3" in the
example above - is unique in the first nibble - i.e. there are no
other note paths that start with "d") are _not_ unpacked as part of
the tree writeout. This error will repeat itself in subsequent note
trees until the subtree is forced to be unpacked. In t3305 this only
happens when the d38ec8f8 note is itself removed from the tree.

The error is not severe (no information is lost, and the notes code
is able to read/decode this tree and manipulate it correctly), but
this is nonetheless a bug in the current implementation that should
be fixed.

That said, fixing the off-by-one error is not without complications:
We must take into account that the load_subtree() call from
for_each_note_helper() (that is now done to correctly unpack the
subtree while we're writing out the notes tree) may end up inserting
unpacked non-notes into the linked list of non_note entries held by
the struct notes_tree. Since we are in the process of writing out the
notes tree, this linked list is currently in the process of being
traversed by write_each_non_note_until(). The unpacked non-notes are
necessarily inserted between the last non-note we wrote out, and the
next non-note to be written. Hence, we cannot simply hold the
next_non_note to write in struct write_each_note_data (as we would
then silently skip these newly inserted notes), but must instead
always follow the ->next pointer from the last non-note we wrote.
(This part was caught by an existing test in t3304.)

Signed-off-by: Johan Herland <johan@herland.net>
 notes.c                 | 20 ++++++++++++--------
 t/t3305-notes-fanout.sh |  6 ++++++
 2 files changed, 18 insertions(+), 8 deletions(-)

diff mbox series

diff --git a/notes.c b/notes.c
index 0c79964c26..2de7f4bcfb 100644
--- a/notes.c
+++ b/notes.c
@@ -576,16 +576,16 @@  static int for_each_note_helper(struct notes_tree *t, struct int_node *tree,
 			 * the note tree that have not yet been explored. There
 			 * is a direct relationship between subtree entries at
 			 * level 'n' in the tree, and the 'fanout' variable:
-			 * Subtree entries at level 'n <= 2 * fanout' should be
+			 * Subtree entries at level 'n < 2 * fanout' should be
 			 * preserved, since they correspond exactly to a fanout
 			 * directory in the on-disk structure. However, subtree
-			 * entries at level 'n > 2 * fanout' should NOT be
+			 * entries at level 'n >= 2 * fanout' should NOT be
 			 * preserved, but rather consolidated into the above
 			 * notes tree level. We achieve this by unconditionally
 			 * unpacking subtree entries that exist below the
 			 * threshold level at 'n = 2 * fanout'.
-			if (n <= 2 * fanout &&
+			if (n < 2 * fanout &&
 				/* invoke callback with subtree */
 				unsigned int path_len =
@@ -602,7 +602,7 @@  static int for_each_note_helper(struct notes_tree *t, struct int_node *tree,
-			if (n > fanout * 2 ||
+			if (n >= 2 * fanout ||
 				/* unpack subtree and resume traversal */
 				tree->a[i] = NULL;
@@ -723,13 +723,15 @@  static int write_each_note_helper(struct tree_write_stack *tws,
 struct write_each_note_data {
 	struct tree_write_stack *root;
-	struct non_note *next_non_note;
+	struct non_note **nn_list;
+	struct non_note *nn_prev;
 static int write_each_non_note_until(const char *note_path,
 		struct write_each_note_data *d)
-	struct non_note *n = d->next_non_note;
+	struct non_note *p = d->nn_prev;
+	struct non_note *n = p ? p->next : *d->nn_list;
 	int cmp = 0, ret;
 	while (n && (!note_path || (cmp = strcmp(n->path, note_path)) <= 0)) {
 		if (note_path && cmp == 0)
@@ -740,9 +742,10 @@  static int write_each_non_note_until(const char *note_path,
 			if (ret)
 				return ret;
+		p = n;
 		n = n->next;
-	d->next_non_note = n;
+	d->nn_prev = p;
 	return 0;
@@ -1177,7 +1180,8 @@  int write_notes_tree(struct notes_tree *t, struct object_id *result)
 	strbuf_init(&root.buf, 256 * (32 + the_hash_algo->hexsz)); /* assume 256 entries */
 	root.path[0] = root.path[1] = '\0';
 	cb_data.root = &root;
-	cb_data.next_non_note = t->first_non_note;
+	cb_data.nn_list = &(t->first_non_note);
+	cb_data.nn_prev = NULL;
 	/* Write tree objects representing current notes tree */
diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh
index 402057c83a..3b4753e1b4 100755
--- a/t/t3305-notes-fanout.sh
+++ b/t/t3305-notes-fanout.sh
@@ -30,6 +30,12 @@  all_notes_have_fanout() {
+test_expect_success 'tweak test environment' '
+	git checkout -b nondeterminism &&
+	test_commit A &&
+	git checkout --orphan with_notes;
 test_expect_success 'creating many notes with git-notes' '
 	num_notes=300 &&
 	i=0 &&