Skip to content

Commit 1a4abcb

Browse files
gitstergitster
committed
Merge branch 'jh/notes-fanout-fix' into maint
The code to automatically shrink the fan-out in the notes tree had an off-by-one bug, which has been killed. * jh/notes-fanout-fix: notes.c: fix off-by-one error when decreasing notes fanout t3305: check notes fanout more carefully and robustly
2 parents 7e84f46 + dbc2747 commit 1a4abcb

File tree

2 files changed

+94
-33
lines changed

2 files changed

+94
-33
lines changed

‎notes.c‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -576,16 +576,16 @@ static int for_each_note_helper(struct notes_tree *t, struct int_node *tree,
576576
* the note tree that have not yet been explored. There
577577
* is a direct relationship between subtree entries at
578578
* level 'n' in the tree, and the 'fanout' variable:
579-
* Subtree entries at level 'n <= 2 * fanout' should be
579+
* Subtree entries at level 'n < 2 * fanout' should be
580580
* preserved, since they correspond exactly to a fanout
581581
* directory in the on-disk structure. However, subtree
582-
* entries at level 'n > 2 * fanout' should NOT be
582+
* entries at level 'n >= 2 * fanout' should NOT be
583583
* preserved, but rather consolidated into the above
584584
* notes tree level. We achieve this by unconditionally
585585
* unpacking subtree entries that exist below the
586586
* threshold level at 'n = 2 * fanout'.
587587
*/
588-
if (n <= 2 * fanout &&
588+
if (n < 2 * fanout &&
589589
flags & FOR_EACH_NOTE_YIELD_SUBTREES) {
590590
/* invoke callback with subtree */
591591
unsigned int path_len =
@@ -602,7 +602,7 @@ static int for_each_note_helper(struct notes_tree *t, struct int_node *tree,
602602
path,
603603
cb_data);
604604
}
605-
if (n > fanout * 2 ||
605+
if (n >= 2 * fanout ||
606606
!(flags & FOR_EACH_NOTE_DONT_UNPACK_SUBTREES)) {
607607
/* unpack subtree and resume traversal */
608608
tree->a[i] = NULL;
@@ -723,13 +723,15 @@ static int write_each_note_helper(struct tree_write_stack *tws,
723723

724724
struct write_each_note_data {
725725
struct tree_write_stack *root;
726-
struct non_note *next_non_note;
726+
struct non_note **nn_list;
727+
struct non_note *nn_prev;
727728
};
728729

729730
static int write_each_non_note_until(const char *note_path,
730731
struct write_each_note_data *d)
731732
{
732-
struct non_note *n = d->next_non_note;
733+
struct non_note *p = d->nn_prev;
734+
struct non_note *n = p ? p->next : *d->nn_list;
733735
int cmp = 0, ret;
734736
while (n && (!note_path || (cmp = strcmp(n->path, note_path)) <= 0)) {
735737
if (note_path && cmp == 0)
@@ -740,9 +742,10 @@ static int write_each_non_note_until(const char *note_path,
740742
if (ret)
741743
return ret;
742744
}
745+
p = n;
743746
n = n->next;
744747
}
745-
d->next_non_note = n;
748+
d->nn_prev = p;
746749
return 0;
747750
}
748751

@@ -1177,7 +1180,8 @@ int write_notes_tree(struct notes_tree *t, struct object_id *result)
11771180
strbuf_init(&root.buf, 256 * (32 + the_hash_algo->hexsz)); /* assume 256 entries */
11781181
root.path[0] = root.path[1] = '\0';
11791182
cb_data.root = &root;
1180-
cb_data.next_non_note = t->first_non_note;
1183+
cb_data.nn_list = &(t->first_non_note);
1184+
cb_data.nn_prev = NULL;
11811185

11821186
/* Write tree objects representing current notes tree */
11831187
flags = FOR_EACH_NOTE_DONT_UNPACK_SUBTREES |

‎t/t3305-notes-fanout.sh‎

Lines changed: 82 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,38 @@ test_description='Test that adding/removing many notes triggers automatic fanout
44

55
. ./test-lib.sh
66

7+
path_has_fanout() {
8+
path=$1 &&
9+
fanout=$2 &&
10+
after_last_slash=$((40 - $fanout * 2)) &&
11+
echo $path | grep -q "^\([0-9a-f]\{2\}/\)\{$fanout\}[0-9a-f]\{$after_last_slash\}$"
12+
}
13+
14+
touched_one_note_with_fanout() {
15+
notes_commit=$1 &&
16+
modification=$2 && # 'A' for addition, 'D' for deletion
17+
fanout=$3 &&
18+
diff=$(git diff-tree --no-commit-id --name-status --root -r $notes_commit) &&
19+
path=$(echo $diff | sed -e "s/^$modification[\t ]//") &&
20+
path_has_fanout "$path" $fanout;
21+
}
22+
23+
all_notes_have_fanout() {
24+
notes_commit=$1 &&
25+
fanout=$2 &&
26+
git ls-tree -r --name-only $notes_commit 2>/dev/null |
27+
while read path
28+
do
29+
path_has_fanout $path $fanout || return 1
30+
done
31+
}
32+
33+
test_expect_success 'tweak test environment' '
34+
git checkout -b nondeterminism &&
35+
test_commit A &&
36+
git checkout --orphan with_notes;
37+
'
38+
739
test_expect_success 'creating many notes with git-notes' '
840
num_notes=300 &&
941
i=0 &&
@@ -20,7 +52,7 @@ test_expect_success 'creating many notes with git-notes' '
2052

2153
test_expect_success 'many notes created correctly with git-notes' '
2254
git log | grep "^ " > output &&
23-
i=300 &&
55+
i=$num_notes &&
2456
while test $i -gt 0
2557
do
2658
echo " commit #$i" &&
@@ -30,34 +62,46 @@ test_expect_success 'many notes created correctly with git-notes' '
3062
test_cmp expect output
3163
'
3264

33-
test_expect_success 'many notes created with git-notes triggers fanout' '
34-
# Expect entire notes tree to have a fanout == 1
35-
git ls-tree -r --name-only refs/notes/commits |
36-
while read path
65+
test_expect_success 'stable fanout 0 is followed by stable fanout 1' '
66+
i=$num_notes &&
67+
fanout=0 &&
68+
while test $i -gt 0
3769
do
38-
echo $path | grep "^../[0-9a-f]*$" || {
39-
echo "Invalid path \"$path\"" &&
40-
return 1;
41-
}
42-
done
70+
i=$(($i - 1)) &&
71+
if touched_one_note_with_fanout refs/notes/commits~$i A $fanout
72+
then
73+
continue
74+
elif test $fanout -eq 0
75+
then
76+
fanout=1 &&
77+
if all_notes_have_fanout refs/notes/commits~$i $fanout
78+
then
79+
echo "Fanout 0 -> 1 at refs/notes/commits~$i" &&
80+
continue
81+
fi
82+
fi &&
83+
echo "Failed fanout=$fanout check at refs/notes/commits~$i" &&
84+
git ls-tree -r --name-only refs/notes/commits~$i &&
85+
return 1
86+
done &&
87+
all_notes_have_fanout refs/notes/commits 1
4388
'
4489

4590
test_expect_success 'deleting most notes with git-notes' '
46-
num_notes=250 &&
91+
remove_notes=285 &&
4792
i=0 &&
4893
git rev-list HEAD |
49-
while test $i -lt $num_notes && read sha1
94+
while test $i -lt $remove_notes && read sha1
5095
do
5196
i=$(($i + 1)) &&
5297
test_tick &&
53-
git notes remove "$sha1" ||
54-
exit 1
98+
git notes remove "$sha1" 2>/dev/null || return 1
5599
done
56100
'
57101

58102
test_expect_success 'most notes deleted correctly with git-notes' '
59-
git log HEAD~250 | grep "^ " > output &&
60-
i=50 &&
103+
git log HEAD~$remove_notes | grep "^ " > output &&
104+
i=$(($num_notes - $remove_notes)) &&
61105
while test $i -gt 0
62106
do
63107
echo " commit #$i" &&
@@ -67,16 +111,29 @@ test_expect_success 'most notes deleted correctly with git-notes' '
67111
test_cmp expect output
68112
'
69113

70-
test_expect_success 'deleting most notes triggers fanout consolidation' '
71-
# Expect entire notes tree to have a fanout == 0
72-
git ls-tree -r --name-only refs/notes/commits |
73-
while read path
114+
test_expect_success 'stable fanout 1 is followed by stable fanout 0' '
115+
i=$remove_notes &&
116+
fanout=1 &&
117+
while test $i -gt 0
74118
do
75-
echo $path | grep -v "^../.*" || {
76-
echo "Invalid path \"$path\"" &&
77-
return 1;
78-
}
79-
done
119+
i=$(($i - 1)) &&
120+
if touched_one_note_with_fanout refs/notes/commits~$i D $fanout
121+
then
122+
continue
123+
elif test $fanout -eq 1
124+
then
125+
fanout=0 &&
126+
if all_notes_have_fanout refs/notes/commits~$i $fanout
127+
then
128+
echo "Fanout 1 -> 0 at refs/notes/commits~$i" &&
129+
continue
130+
fi
131+
fi &&
132+
echo "Failed fanout=$fanout check at refs/notes/commits~$i" &&
133+
git ls-tree -r --name-only refs/notes/commits~$i &&
134+
return 1
135+
done &&
136+
all_notes_have_fanout refs/notes/commits 0
80137
'
81138

82139
test_done

0 commit comments

Comments
 (0)