git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johan Herland <johan@herland.net>
To: grubba@grubba.org
Cc: git@vger.kernel.org, jrnieder@gmail.com, johan@herland.net
Subject: [RFC/PATCH 1/3] t9301: Fix testcase covering up a bug in fast-import's notes fanout handling
Date: Fri, 25 Nov 2011 01:09:45 +0100	[thread overview]
Message-ID: <1322179787-4422-2-git-send-email-johan@herland.net> (raw)
In-Reply-To: <1322179787-4422-1-git-send-email-johan@herland.net>

There is a bug in fast-import where the fanout levels of an existing notes
tree being loaded into the fast-import machinery is disregarded. Instead, any
tree loaded is assumed to have a fanout level of 0. If the true fanout level
is deeper, any attempt to remove a note from that tree will silently fail
(as the note will not be found at fanout level 0).

However, this bug was covered up by the way in which the t9301 testcase was
written: When generating the fast-import commands to test mass removal of
notes, we appended these commands to an already existing 'input' file which
happened to already contain the fast-import commands used in the previous
subtest to generate the very same notes tree. This would normally be harmless
(but suboptimal) as the notes created were identical to the notes already
present in the notes tree. But the act of repeating all the notes additions
caused the internal fast-import data structures to recalculate the fanout,
instead of hanging on to the initial (incorrect) fanout (that causes the bug
described above). Thus, the subsequent removal of notes in the same 'input'
file would succeed, thereby covering up the bug described above.

This patch creates a new 'input' file instead of appending to the file from
the previous subtest. Thus, we end up properly testing removal of notes that
were added by a previous fast-import command. As a side effect, the notes
removal can no longer refer to commits using the marks set by the previous
fast-import run, instead the commits names must be referenced directly.

The underlying fast-import bug is still present after this patch, but now we
have at least uncovered it. Therefore, the affected subtests are labeled as
expected failures until the underlying bug is fixed.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t9301-fast-import-notes.sh |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 463254c..fd08161 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -507,7 +507,7 @@ test_expect_success 'verify that non-notes are untouched by a fanout change' '
 '
 remaining_notes=10
 test_tick
-cat >>input <<INPUT_END
+cat >input <<INPUT_END
 commit refs/notes/many_notes
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <<COMMIT
@@ -516,12 +516,11 @@ COMMIT
 from refs/notes/many_notes^0
 INPUT_END
 
-i=$remaining_notes
-while test $i -lt $num_commits
+i=$(($num_commits - $remaining_notes))
+for sha1 in $(git rev-list -n $i refs/heads/many_commits)
 do
-	i=$(($i + 1))
 	cat >>input <<INPUT_END
-N 0000000000000000000000000000000000000000 :$i
+N 0000000000000000000000000000000000000000 $sha1
 INPUT_END
 done
 
@@ -541,7 +540,7 @@ EXPECT_END
 	i=$(($i - 1))
 done
 
-test_expect_success 'remove lots of notes' '
+test_expect_failure 'remove lots of notes' '
 
 	git fast-import <input &&
 	GIT_NOTES_REF=refs/notes/many_notes git log refs/heads/many_commits |
@@ -550,7 +549,7 @@ test_expect_success 'remove lots of notes' '
 
 '
 
-test_expect_success 'verify that removing notes trigger fanout consolidation' '
+test_expect_failure '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 |
-- 
1.7.5.rc1.3.g4d7b

  reply	other threads:[~2011-11-25  1:10 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-11-23 12:09 Incremental use of fast-import may cause conflicting notes Henrik Grubbström
2011-11-23 12:10 ` Henrik Grubbström
2011-11-24 23:09 ` Jonathan Nieder
2011-11-25  0:09 ` [RFC/PATCH 0/3] fast-import: Fix incremental use of notes Johan Herland
2011-11-25  0:09   ` Johan Herland [this message]
2011-11-25  0:09   ` [RFC/PATCH 2/3] t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling Johan Herland
2011-11-25  0:09   ` [RFC/PATCH 3/3] fast-import: Fix incorrect fanout level when modifying existing notes refs Johan Herland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1322179787-4422-2-git-send-email-johan@herland.net \
    --to=johan@herland.net \
    --cc=git@vger.kernel.org \
    --cc=grubba@grubba.org \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).