From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f178.google.com (mail-qk1-f178.google.com [209.85.222.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B0B216435 for ; Sat, 13 Jan 2024 22:06:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ttaylorr-com.20230601.gappssmtp.com header.i=@ttaylorr-com.20230601.gappssmtp.com header.b="Wt8q55x1" Received: by mail-qk1-f178.google.com with SMTP id af79cd13be357-78313f4d149so777453285a.1 for ; Sat, 13 Jan 2024 14:06:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ttaylorr-com.20230601.gappssmtp.com; s=20230601; t=1705183573; x=1705788373; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=9ToeVjoVOIJjwQna2bPQU5LeaifHdEi/2q+kvkNzXKs=; b=Wt8q55x1zgdrXw7b/jdpYFqhZJaH0RGSPX8z4d1kPGfvrUSGN1QF4I2e6FABXNyzYM EjXKnSwqaW4sDl/vHljLPgIFH90ds2QeZZS1/Ai0CNXFp/7GeV2Q6tCYSO7o3UzXL1/p f/Hlz/y4gzPRSVO8q0uQla1v+ZwMJpD3yA6y5fgzlXqam4+gVw7ZXEIc/qoTxDVYswZj EGMkSZyrXTPznSgFLEeT8KGUFx3Cg90bMAi6K9pjL4NCQkY0IFXiwpWbkT0VCAmSlqUP O6HmmdBakOWq0R+7j+xR/646vI0ybsttXchi+xYOXCb7ZKxZSfJadU8TGS/C1lVEPZKI Zr4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705183573; x=1705788373; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=9ToeVjoVOIJjwQna2bPQU5LeaifHdEi/2q+kvkNzXKs=; b=m1qXjgBvDCz1Of0X0AjBdVrOM0ky8Dpl18uLxJoMpJJR/l8u7FzhAFnYJYql7RomU6 4/DF8gC9Xzoon5XEsn5k9z2M5QPIWg9zFHdW0B9bY0lHH2OV7x3qC83cC84tMn6itesT 68qyrvyHvDg8u94FNnrN4tWbZgUMtFcuM0WxL1BLHoeSvhRoG/NhdPi51mpfKsqn++Xj gJ8w0mNx/ZJ4FxKvXvZxGCSmU/DDLeofaD4MNMjBKXrF9cPPLPH/OcZLjBJ/gPglTVo0 W3d5v3AYy7U1LWPii44aTWHV+0QrlhISRCuHwKhXPiqWJ8VLwugiKK5Zd8tGdekDSHZj YjJw== X-Gm-Message-State: AOJu0Yz62XwAjZ8BhALEIGqROVErSkr3fivwMCjM2VdgTvM2r9Tagw8j K5TkC9FHpI0y5ryH9/frDDYjNFr3aPGPEQ== X-Google-Smtp-Source: AGHT+IHOMCnzMaUZ4wjSgXGzNDlPJgBASMUnxy/la1ot1mITPbOzA820bTeUcevvCso0jkjt5Q4HEw== X-Received: by 2002:a05:620a:22da:b0:778:ba89:2fbd with SMTP id o26-20020a05620a22da00b00778ba892fbdmr3761535qki.36.1705183573019; Sat, 13 Jan 2024 14:06:13 -0800 (PST) Received: from localhost (104-178-186-189.lightspeed.milwwi.sbcglobal.net. [104.178.186.189]) by smtp.gmail.com with ESMTPSA id pc9-20020a05620a840900b007831f65d030sm1961416qkn.36.2024.01.13.14.06.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 13 Jan 2024 14:06:12 -0800 (PST) Date: Sat, 13 Jan 2024 17:06:11 -0500 From: Taylor Blau To: SZEDER =?utf-8?B?R8OhYm9y?= Cc: Junio C Hamano , git@vger.kernel.org Subject: Re: What's cooking in git.git (Jan 2024, #01; Tue, 2) Message-ID: References: <20240113183544.GA3000857@szeder.dev> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240113183544.GA3000857@szeder.dev> Hi Gábor, Thanks very much for your patience while I figure all of this out. I'm confident that with the fixup! below that your test passes in the next round of this series. On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote: > On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote: > > Taylor Blau writes: > > > > >> * tb/path-filter-fix (2023-10-18) 17 commits > > >> - bloom: introduce `deinit_bloom_filters()` > > >> ... > > >> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()` > > >> > > >> The Bloom filter used for path limited history traversal was broken > > >> on systems whose "char" is unsigned; update the implementation and > > >> bump the format version to 2. > > >> > > >> Expecting a reroll. > > >> cf. <20231023202212.GA5470@szeder.dev> > > >> source: > > > > > > I was confused by this one, since I couldn't figure out which tests > > > Gábor was referring to here. I responded in [1], but haven't heard back > > > since the end of October. > > > ... > > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/ > > I keep referring to the test in: > > https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/ > > which, rather disappointingly, is still the only test out there > exercising the interaction between split commit graphs and different > modified path Bloom filter versions. Note that in that message I > mentioned that merging layers with differenet Bloom filter versions > seemed to work, but that, alas, is no longer the case, because it's > now broken in Taylor's recent iterations of the patch series. Thanks for clarifying. With all of the different versions of this series and the couple of tests that you and I were talking about, I think that I got confused and thought you were referring to a different test. Indeed, the current version of this series (v4) does not pass the test in <20230830200218.GA5147@szeder.dev>, but the fix in order for it to pass is relatively straightforward. I have this on top of my local branch as a fixup! and I'll squash it down on Tuesday[^1] and send a reroll after double-checking that the fix is correct and doesn't conflict with any other parts of the series. Since it's been so long since I've looked at this, I'll re-read the series as a whole with fresh eyes to make sure that everything still makes sense. In any case, here's the patch on top (with a lightly modified version of the test you wrote in <20230830200218.GA5147@szeder.dev>: Subject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with consistent settings Signed-off-by: Taylor Blau --- commit-graph.c | 3 ++- t/t4216-log-bloom.sh | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index 60fa64d956..82a4632c4e 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -507,7 +507,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g) } if (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry || - g->bloom_filter_settings->num_hashes != settings->num_hashes) { + g->bloom_filter_settings->num_hashes != settings->num_hashes || + g->bloom_filter_settings->hash_version != settings->hash_version) { g->chunk_bloom_indexes = NULL; g->chunk_bloom_data = NULL; FREE_AND_NULL(g->bloom_filter_settings); diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index 569f2b6f8b..d8c42e2e27 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -438,7 +438,7 @@ test_expect_success 'setup for mixed Bloom setting tests' ' done ' -test_expect_success 'ensure incompatible Bloom filters are ignored' ' +test_expect_success 'ensure Bloom filters with incompatible settings are ignored' ' # Compute Bloom filters with "unusual" settings. git -C $repo rev-parse one >in && GIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \ @@ -488,6 +488,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' ' test_must_be_empty err ' +test_expect_success 'ensure Bloom filter with incompatible versions are ignored' ' + rm "$repo/$graph" && + + git -C $repo log --oneline --no-decorate -- $CENT >expect && + + # Compute v1 Bloom filters for commits at the bottom. + git -C $repo rev-parse HEAD^ >in && + git -C $repo commit-graph write --stdin-commits --changed-paths \ + --split in && + git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \ + --stdin-commits --changed-paths --split=no-merge actual 2>err && + test_cmp expect actual && + + layer="$(head -n 1 $repo/$chain)" && + cat >expect.err <<-EOF && + warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings + EOF + test_cmp expect.err err +' + get_first_changed_path_filter () { test-tool read-graph bloom-filters >filters.dat && head -n 1 filters.dat -- 2.38.0.16.g393fd4c6db (Excuse the old version of Git here, I'm in the middle of a long-running process which checks out older versions of the tree to count the number of unused-parameters over time.) Thanks, Taylor [^1]: Monday is a US holiday, so I likely won't be at my computer.