Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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>
  • Loading branch information...
commit 183868578049f5a28f7166716a4b00768d148958 1 parent 9ff5e21
@jherland jherland authored gitster committed
Showing with 29 additions and 7 deletions.
  1. +25 −3 fast-import.c
  2. +4 −4 t/t9301-fast-import-notes.sh
View
28 fast-import.c
@@ -2173,6 +2173,11 @@ static uintmax_t do_change_note_fanout(
if (tmp_hex_sha1_len == 40 && !get_sha1_hex(hex_sha1, sha1)) {
/* This is a note entry */
+ if (fanout == 0xff) {
+ /* Counting mode, no rename */
+ num_notes++;
+ continue;
+ }
construct_path_with_fanout(hex_sha1, fanout, realpath);
if (!strcmp(fullpath, realpath)) {
/* Note entry is in correct location */
@@ -2379,7 +2384,7 @@ static void file_change_cr(struct branch *b, int rename)
leaf.tree);
}
-static void note_change_n(struct branch *b, unsigned char old_fanout)
+static void note_change_n(struct branch *b, unsigned char *old_fanout)
{
const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
@@ -2390,6 +2395,23 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
uint16_t inline_data = 0;
unsigned char new_fanout;
+ /*
+ * When loading a branch, we don't traverse its tree to count the real
+ * number of notes (too expensive to do this for all non-note refs).
+ * This means that recently loaded notes refs might incorrectly have
+ * b->num_notes == 0, and consequently, old_fanout might be wrong.
+ *
+ * Fix this by traversing the tree and counting the number of notes
+ * when b->num_notes == 0. If the notes tree is truly empty, the
+ * calculation should not take long.
+ */
+ if (b->num_notes == 0 && *old_fanout == 0) {
+ /* Invoke change_note_fanout() in "counting mode". */
+ b->num_notes = change_note_fanout(&b->branch_tree, 0xff);
+ *old_fanout = convert_num_notes_to_fanout(b->num_notes);
+ }
+
+ /* Now parse the notemodify command. */
/* <dataref> or 'inline' */
if (*p == ':') {
char *x;
@@ -2450,7 +2472,7 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)
typename(type), command_buf.buf);
}
- construct_path_with_fanout(sha1_to_hex(commit_sha1), old_fanout, path);
+ construct_path_with_fanout(sha1_to_hex(commit_sha1), *old_fanout, path);
if (tree_content_remove(&b->branch_tree, path, NULL))
b->num_notes--;
@@ -2637,7 +2659,7 @@ static void parse_new_commit(void)
else if (!prefixcmp(command_buf.buf, "C "))
file_change_cr(b, 0);
else if (!prefixcmp(command_buf.buf, "N "))
- note_change_n(b, prev_fanout);
+ note_change_n(b, &prev_fanout);
else if (!strcmp("deleteall", command_buf.buf))
file_change_deleteall(b);
else if (!prefixcmp(command_buf.buf, "ls "))
View
8 t/t9301-fast-import-notes.sh
@@ -536,7 +536,7 @@ EXPECT_END
j=$(($j + 1))
done
-test_expect_failure 'change a few existing notes' '
+test_expect_success 'change a few existing notes' '
git fast-import <input &&
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' '
'
-test_expect_failure 'verify that changing notes respect existing fanout' '
+test_expect_success 'verify that changing notes respect existing fanout' '
# None of the entries in the top-level notes tree should be a full SHA1
git ls-tree --name-only refs/notes/many_notes |
@@ -594,7 +594,7 @@ EXPECT_END
i=$(($i - 1))
done
-test_expect_failure 'remove lots of notes' '
+test_expect_success 'remove lots of notes' '
git fast-import <input &&
GIT_NOTES_REF=refs/notes/many_notes git log refs/heads/many_commits |
@@ -603,7 +603,7 @@ test_expect_failure 'remove lots of notes' '
'
-test_expect_failure 'verify that removing notes trigger fanout consolidation' '
+test_expect_success 'verify that removing notes trigger fanout consolidation' '
# All entries in the top-level notes tree should be a full SHA1
git ls-tree --name-only -r refs/notes/many_notes |
Please sign in to comment.
Something went wrong with that request. Please try again.