fast-import: Fix incorrect fanout level when modifying existing notes refs
This fixes the bug uncovered by the tests added in the previous two patches. When an existing notes ref was loaded into the fast-import machinery, the num_notes counter associated with that ref remained == 0, even though the true number of notes in the loaded ref was higher. This caused a fanout level of 0 to be used, although the actual fanout of the tree could be > 0. Manipulating the notes tree at an incorrect fanout level causes removals to silently fail, and modifications of existing notes to instead produce an additional note (leaving the old object in place at a different fanout level). This patch fixes the bug by explicitly counting the number of notes in the notes tree whenever it looks like the num_notes counter could be wrong (when num_notes == 0). There may be false positives (i.e. triggering the counting when the notes tree is truly empty), but in those cases, the counting should not take long. Signed-off-by: Johan Herland <johan@herland.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johan Herland committed
Nov 25, 2011 at 01:09 UTC
183868578049f5a28f7166716a4b00768d148958
2 files changed
+29
-7
fast-import.c
+25
-3
@@ -2173,6 +2173,11 @@ static uintmax_t do_change_note_fanout(
2173
2174
if (tmp_hex_sha1_len == 40 && !get_sha1_hex(hex_sha1, sha1)) {
2175
/* This is a note entry */
2176
+ if (fanout == 0xff) {
2177
+ /* Counting mode, no rename */
2178
+ num_notes++;
2179
+ continue;
2180
+ }
2181
construct_path_with_fanout(hex_sha1, fanout, realpath);
2182
if (!strcmp(fullpath, realpath)) {
2183
/* Note entry is in correct location */
@@ -2379,7 +2384,7 @@ static void file_change_cr(struct branch *b, int rename)
2384
leaf.tree);
2385
}
2386
2382
-static void note_change_n(struct branch *b, unsigned char old_fanout)
2387
+static void note_change_n(struct branch *b, unsigned char *old_fanout)
2388
{
2389
const char *p = command_buf.buf + 2;
2390
static struct strbuf uq = STRBUF_INIT;
@@ -2390,6 +2395,23 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
2395
uint16_t inline_data = 0;
2396
unsigned char new_fanout;
2397
2398
+ /*
2399
+ * When loading a branch, we don't traverse its tree to count the real
2400
+ * number of notes (too expensive to do this for all non-note refs).
2401
+ * This means that recently loaded notes refs might incorrectly have
2402
+ * b->num_notes == 0, and consequently, old_fanout might be wrong.
2403
+ *
2404
+ * Fix this by traversing the tree and counting the number of notes
2405
+ * when b->num_notes == 0. If the notes tree is truly empty, the
2406
+ * calculation should not take long.
2407
+ */
2408
+ if (b->num_notes == 0 && *old_fanout == 0) {
2409
+ /* Invoke change_note_fanout() in "counting mode". */
2410
+ b->num_notes = change_note_fanout(&b->branch_tree, 0xff);
2411
+ *old_fanout = convert_num_notes_to_fanout(b->num_notes);
2412
+ }
2413
+
2414
+ /* Now parse the notemodify command. */
2415
/* <dataref> or 'inline' */
2416
if (*p == ':') {
2417
char *x;
@@ -2450,7 +2472,7 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
2472
typename(type), command_buf.buf);
2473
}
2474
2453
- construct_path_with_fanout(sha1_to_hex(commit_sha1), old_fanout, path);
2475
+ construct_path_with_fanout(sha1_to_hex(commit_sha1), *old_fanout, path);
2476
if (tree_content_remove(&b->branch_tree, path, NULL))
2477
b->num_notes--;
2478
@@ -2637,7 +2659,7 @@ static void parse_new_commit(void)
2659
else if (!prefixcmp(command_buf.buf, "C "))
2660
file_change_cr(b, 0);
2661
else if (!prefixcmp(command_buf.buf, "N "))
2640
- note_change_n(b, prev_fanout);
2662
+ note_change_n(b, &prev_fanout);
2663
else if (!strcmp("deleteall", command_buf.buf))
2664
file_change_deleteall(b);
2665
else if (!prefixcmp(command_buf.buf, "ls "))
t/t9301-fast-import-notes.sh
+4
-4
@@ -536,7 +536,7 @@ EXPECT_END
536
j=$(($j + 1))
537
done
538
539
-test_expect_failure 'change a few existing notes' '
539
+test_expect_success 'change a few existing notes' '
540
541
git fast-import <input &&
542
GIT_NOTES_REF=refs/notes/many_notes git log -n3 refs/heads/many_commits |
@@ -545,7 +545,7 @@ test_expect_failure 'change a few existing notes' '
545
546
'
547
548
-test_expect_failure 'verify that changing notes respect existing fanout' '
548
+test_expect_success 'verify that changing notes respect existing fanout' '
549
550
# None of the entries in the top-level notes tree should be a full SHA1
551
git ls-tree --name-only refs/notes/many_notes |
@@ -594,7 +594,7 @@ EXPECT_END
594
i=$(($i - 1))
595
done
596
597
-test_expect_failure 'remove lots of notes' '
597
+test_expect_success 'remove lots of notes' '
598
599
git fast-import <input &&
600
GIT_NOTES_REF=refs/notes/many_notes git log refs/heads/many_commits |
@@ -603,7 +603,7 @@ test_expect_failure 'remove lots of notes' '
603
604
'
605
606
-test_expect_failure 'verify that removing notes trigger fanout consolidation' '
606
+test_expect_success 'verify that removing notes trigger fanout consolidation' '
607
608
# All entries in the top-level notes tree should be a full SHA1
609
git ls-tree --name-only -r refs/notes/many_notes |