{"thread":{"id":"56850","subject":"[PATCH] test_bitmap_hashes(): handle repository without bitmaps","startedAt":"2021-11-05T09:01:34Z","lastAt":"2021-11-08T22:06:56Z","messageCount":9,"participants":["Jeff King","Junio C Hamano","Taylor Blau","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"440514","messageId":"YYTy6+DG5guzJIO7@coredump.intra.peff.net","threadId":"56850","inReplyTo":null,"subject":"[PATCH] test_bitmap_hashes(): handle repository without bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T09:01:31Z","receivedAt":"2021-11-05T09:01:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being\nthat the repository does not have bitmaps at all), then we'll segfault\naccessing bitmap_git->hashes:\n\n  $ t/helper/test-tool bitmap dump-hashes\n  Segmentation fault\n\nWe should treat this the same as a repository with bitmaps but no\nname-hashes, and quietly produce an empty output. The later call to\nfree_bitmap_index() in the cleanup label is OK, as it treats a NULL\npointer as a noop.\n\nThis isn't a big deal in practice, as this function is intended for and\nused only by test-tool. It's probably worth fixing to avoid confusion,\nbut not worth adding coverage for this to the test suite.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is new in the v2.34.0 cycle, but it's so low impact it doesn't\nmatter much if we ship with the bug. OTOH, it's pretty low-risk since it\nis only run by the test suite.\n\n pack-bitmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex a56ceb9441..f772d3cb7f 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1759,7 +1759,7 @@ int test_bitmap_hashes(struct repository *r)\n \tstruct object_id oid;\n \tuint32_t i, index_pos;\n \n-\tif (!bitmap_git->hashes)\n+\tif (!bitmap_git || !bitmap_git->hashes)\n \t\tgoto cleanup;\n \n \tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n-- \n2.34.0.rc1.634.g85d556ea55\n"},{"id":"440543","messageId":"xmqq35oaxwnz.fsf@gitster.g","threadId":"56850","inReplyTo":"YYTy6+DG5guzJIO7@coredump.intra.peff.net","subject":"Re: [PATCH] test_bitmap_hashes(): handle repository without bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-05T18:52:16Z","receivedAt":"2021-11-05T18:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being\n> that the repository does not have bitmaps at all), then we'll segfault\n> accessing bitmap_git->hashes:\n>\n>   $ t/helper/test-tool bitmap dump-hashes\n>   Segmentation fault\n>\n> We should treat this the same as a repository with bitmaps but no\n> name-hashes, and quietly produce an empty output. The later call to\n> free_bitmap_index() in the cleanup label is OK, as it treats a NULL\n> pointer as a noop.\n>\n> This isn't a big deal in practice, as this function is intended for and\n> used only by test-tool. It's probably worth fixing to avoid confusion,\n> but not worth adding coverage for this to the test suite.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This is new in the v2.34.0 cycle, but it's so low impact it doesn't\n> matter much if we ship with the bug. OTOH, it's pretty low-risk since it\n> is only run by the test suite.\n\n;-)\n\nI wonder how you found it.  Diagnosing a repository that did not\nseem healthy?  What I am getting at is if we want a new option to\nmake a plumbing command, other than the test-tool, that calls this\nfunction, as the latter is usually not deployed in the field.\n\nWill queue.  Thanks.\n\n\n>  pack-bitmap.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index a56ceb9441..f772d3cb7f 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1759,7 +1759,7 @@ int test_bitmap_hashes(struct repository *r)\n>  \tstruct object_id oid;\n>  \tuint32_t i, index_pos;\n>  \n> -\tif (!bitmap_git->hashes)\n> +\tif (!bitmap_git || !bitmap_git->hashes)\n>  \t\tgoto cleanup;\n>  \n>  \tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n"},{"id":"440544","messageId":"YYWBz6rjF+I+JkO3@nand.local","threadId":"56850","inReplyTo":"xmqq35oaxwnz.fsf@gitster.g","subject":"Re: [PATCH] test_bitmap_hashes(): handle repository without bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-05T19:11:11Z","receivedAt":"2021-11-05T19:11:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 05, 2021 at 11:52:16AM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n>\n> > If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being\n> > that the repository does not have bitmaps at all), then we'll segfault\n> > accessing bitmap_git->hashes:\n> >\n> >   $ t/helper/test-tool bitmap dump-hashes\n> >   Segmentation fault\n> >\n> > We should treat this the same as a repository with bitmaps but no\n> > name-hashes, and quietly produce an empty output. The later call to\n> > free_bitmap_index() in the cleanup label is OK, as it treats a NULL\n> > pointer as a noop.\n> >\n> > This isn't a big deal in practice, as this function is intended for and\n> > used only by test-tool. It's probably worth fixing to avoid confusion,\n> > but not worth adding coverage for this to the test suite.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > This is new in the v2.34.0 cycle, but it's so low impact it doesn't\n> > matter much if we ship with the bug. OTOH, it's pretty low-risk since it\n> > is only run by the test suite.\n>\n> ;-)\n\nYes, this looks obviously correct to me. Thanks for spotting and fixing\nthis, Peff.\n\nI'd be happy to see it in the 2.34 cycle, too, but I agree that it would\nbe OK if it didn't make the cut (and certainly if it makes it easier for\nJunio to handle the rest of the release cycle, then I'm in favor of\nleaving it out).\n\n> I wonder how you found it.  Diagnosing a repository that did not\n> seem healthy?  What I am getting at is if we want a new option to\n> make a plumbing command, other than the test-tool, that calls this\n> function, as the latter is usually not deployed in the field.\n\nI would not be surprised if this was discovered via Coverity, or by\nmanual inspection. Peff and I have been merging a slew of releases from\nyour tree into GitHub's fork and so have been reading code in the more\nrecently changed areas.\n\nOn the test-tool vs. plumbing thing: I think there are some compelling\nreasons in either direction. There's no *good* home for these in our\ncurrent set of plumbing tools. E.g., the closest example we have is `git\nrev-list --test-bitmap <rev>`, which is kind of ugly. When we needed\nthese new inspection tools for some of the newer bitmap-related tests,\nadding them via the test-helper suite was a conscious choice to not\nbuild on the ugliness of `--test-bitmap`.\n\nBut on occasion these test-tool things are useful to have \"in the\nfield\", as you say. It's rare enough that I usually just clone a copy of\nour fork as needed and build it when I do find myself reaching for\ntest-helpers.\n\nThanks,\nTaylor\n"},{"id":"440560","messageId":"YYW+baYx+M7ZhaJU@coredump.intra.peff.net","threadId":"56850","inReplyTo":"YYWBz6rjF+I+JkO3@nand.local","subject":"Re: [PATCH] test_bitmap_hashes(): handle repository without bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-05T23:29:49Z","receivedAt":"2021-11-05T23:29:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 05, 2021 at 03:11:11PM -0400, Taylor Blau wrote:\n\n> > I wonder how you found it.  Diagnosing a repository that did not\n> > seem healthy?  What I am getting at is if we want a new option to\n> > make a plumbing command, other than the test-tool, that calls this\n> > function, as the latter is usually not deployed in the field.\n> \n> I would not be surprised if this was discovered via Coverity, or by\n> manual inspection. Peff and I have been merging a slew of releases from\n> your tree into GitHub's fork and so have been reading code in the more\n> recently changed areas.\n\nIt was Coverity in this case. I haven't actually used the name-hash\ndumper for any real-world debugging.\n\n> On the test-tool vs. plumbing thing: I think there are some compelling\n> reasons in either direction. There's no *good* home for these in our\n> current set of plumbing tools. E.g., the closest example we have is `git\n> rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed\n> these new inspection tools for some of the newer bitmap-related tests,\n> adding them via the test-helper suite was a conscious choice to not\n> build on the ugliness of `--test-bitmap`.\n> \n> But on occasion these test-tool things are useful to have \"in the\n> field\", as you say. It's rare enough that I usually just clone a copy of\n> our fork as needed and build it when I do find myself reaching for\n> test-helpers.\n\nYeah, I could see arguments both ways on such tools (not just bitmaps,\nbut other \"debug this binary format\" tools like read-midx and\nread-graph). I'm content to leave it as-is until I come across more\nin-the-field cases where those tools would be useful. Half the time I\nend up in a debugger anyway. ;)\n\n-Peff\n"},{"id":"440565","messageId":"211106.86fss9hq3f.gmgdl@evledraar.gmail.com","threadId":"56850","inReplyTo":"YYWBz6rjF+I+JkO3@nand.local","subject":"move some test-tools to 'unstable plumbing' built-ins (was: [PATCH] test_bitmap_hashes(): handle repository without bitmaps)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-06T04:08:25Z","receivedAt":"2021-11-06T04:20:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 05 2021, Taylor Blau wrote:\n\n> On Fri, Nov 05, 2021 at 11:52:16AM -0700, Junio C Hamano wrote:\n>> Jeff King <peff@peff.net> writes:\n>>\n>> > If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being\n>> > that the repository does not have bitmaps at all), then we'll segfault\n>> > accessing bitmap_git->hashes:\n>> >\n>> >   $ t/helper/test-tool bitmap dump-hashes\n>> >   Segmentation fault\n>> >\n>> > We should treat this the same as a repository with bitmaps but no\n>> > name-hashes, and quietly produce an empty output. The later call to\n>> > free_bitmap_index() in the cleanup label is OK, as it treats a NULL\n>> > pointer as a noop.\n>> >\n>> > This isn't a big deal in practice, as this function is intended for and\n>> > used only by test-tool. It's probably worth fixing to avoid confusion,\n>> > but not worth adding coverage for this to the test suite.\n>> >\n>> > Signed-off-by: Jeff King <peff@peff.net>\n>> > ---\n>> > This is new in the v2.34.0 cycle, but it's so low impact it doesn't\n>> > matter much if we ship with the bug. OTOH, it's pretty low-risk since it\n>> > is only run by the test suite.\n>>\n>> ;-)\n>\n> Yes, this looks obviously correct to me. Thanks for spotting and fixing\n> this, Peff.\n>\n> I'd be happy to see it in the 2.34 cycle, too, but I agree that it would\n> be OK if it didn't make the cut (and certainly if it makes it easier for\n> Junio to handle the rest of the release cycle, then I'm in favor of\n> leaving it out).\n>\n>> I wonder how you found it.  Diagnosing a repository that did not\n>> seem healthy?  What I am getting at is if we want a new option to\n>> make a plumbing command, other than the test-tool, that calls this\n>> function, as the latter is usually not deployed in the field.\n>\n> I would not be surprised if this was discovered via Coverity, or by\n> manual inspection. Peff and I have been merging a slew of releases from\n> your tree into GitHub's fork and so have been reading code in the more\n> recently changed areas.\n>\n> On the test-tool vs. plumbing thing: I think there are some compelling\n> reasons in either direction. There's no *good* home for these in our\n> current set of plumbing tools. E.g., the closest example we have is `git\n> rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed\n> these new inspection tools for some of the newer bitmap-related tests,\n> adding them via the test-helper suite was a conscious choice to not\n> build on the ugliness of `--test-bitmap`.\n>\n> But on occasion these test-tool things are useful to have \"in the\n> field\", as you say. It's rare enough that I usually just clone a copy of\n> our fork as needed and build it when I do find myself reaching for\n> test-helpers.\n\nAs part of the proposed integration for \"scalar\" I added a category to\nthe command-list.txt called \"optionalcontrib\", which we'll list on its\nown in \"man git\" as (paraphrasing) super-duper-experimental.\n\nI really don't see why we shouldn't do so very lightly with some of\nthese remotely-useful test-tool tools.\n\nIt's pretty much the same amount of work to create a new built-in as a\nnew test-tool, and as long as we make it clear in our documentation that\nthese aren't in the same \"plumbing\" category I don't see why we\nshouldn't add those quite freely.\n\nWe can even make installing them be optional, as some distributors might\nprefer that, which the scalar patch at [1] also does. So all the\nboilerplate is there for someone who'd like to run with this idea.\n\nIt seems to me that we've ended up with the current status quo of not\nadding \"new plumbing\" because we'll need to support it forever out of\nsome self-imposed constraint that we couldn't add new categories to the\n\"git\" manual page.\n\nBut if we just prominently list them as being unstable helpers aimed at\ngit experts, and note the same thing prominently in their manual page\n(trivially done via an include) everyone should be on the same page\nabout their stability, and we'll be able to use stuff like \"test-tool\npkt-line\" \"in the field\".\n\n1. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/\n"},{"id":"440642","messageId":"YYgHhaOAsv7pVAMi@nand.local","threadId":"56850","inReplyTo":"211106.86fss9hq3f.gmgdl@evledraar.gmail.com","subject":"Re: move some test-tools to 'unstable plumbing' built-ins (was: [PATCH] test_bitmap_hashes(): handle repository without bitmaps)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-07T17:06:13Z","receivedAt":"2021-11-07T17:06:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Nov 06, 2021 at 05:08:25AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > On the test-tool vs. plumbing thing: I think there are some compelling\n> > reasons in either direction. There's no *good* home for these in our\n> > current set of plumbing tools. E.g., the closest example we have is `git\n> > rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed\n> > these new inspection tools for some of the newer bitmap-related tests,\n> > adding them via the test-helper suite was a conscious choice to not\n> > build on the ugliness of `--test-bitmap`.\n> >\n> > But on occasion these test-tool things are useful to have \"in the\n> > field\", as you say. It's rare enough that I usually just clone a copy of\n> > our fork as needed and build it when I do find myself reaching for\n> > test-helpers.\n>\n> As part of the proposed integration for \"scalar\" I added a category to\n> the command-list.txt called \"optionalcontrib\", which we'll list on its\n> own in \"man git\" as (paraphrasing) super-duper-experimental.\n>\n> I really don't see why we shouldn't do so very lightly with some of\n> these remotely-useful test-tool tools.\n>\n> It's pretty much the same amount of work to create a new built-in as a\n> new test-tool, and as long as we make it clear in our documentation that\n> these aren't in the same \"plumbing\" category I don't see why we\n> shouldn't add those quite freely.\n\nI'm not sure that I agree completely.\n\nJust because it's easy to create a new builtin (or as easy as it is to\nmake a new test helper) does not alone make it a good reason to do so.\nI think that your idea to make a distinction between these and normal\nplumbing tools is a good one.\n\nBut...\n\n> It seems to me that we've ended up with the current status quo of not\n> adding \"new plumbing\" because we'll need to support it forever out of\n> some self-imposed constraint that we couldn't add new categories to the\n> \"git\" manual page.\n>\n> But if we just prominently list them as being unstable helpers aimed at\n> git experts, and note the same thing prominently in their manual page\n> (trivially done via an include) everyone should be on the same page\n> about their stability, and we'll be able to use stuff like \"test-tool\n> pkt-line\" \"in the field\".\n\n...just because we say that these tools are unstable does not mean that\nusers will listen to us (or even read the relevant documentation saying\nso).\n\nIn my experience I *rarely* rely on test-helpers when debugging wedged\nrepositories, and much more often end up either in gdb, or in an\nanonymized copy of the repository on a different server. I would imagine\nthat others have similar experiences.\n\nSo unless we had a much more compelling reason to have the test helpers\nmore readily available, I do not think that the risk our users will\nbegin to depend on these unstable tools is worth taking.\n\nThanks,\nTaylor\n"},{"id":"440674","messageId":"xmqq7ddiv4oi.fsf@gitster.g","threadId":"56850","inReplyTo":"YYgHhaOAsv7pVAMi@nand.local","subject":"Re: move some test-tools to 'unstable plumbing' built-ins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T19:16:29Z","receivedAt":"2021-11-08T19:16:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> In my experience I *rarely* rely on test-helpers when debugging wedged\n> repositories, and much more often end up either in gdb, or in an\n> anonymized copy of the repository on a different server. I would imagine\n> that others have similar experiences.\n>\n> So unless we had a much more compelling reason to have the test helpers\n> more readily available, I do not think that the risk our users will\n> begin to depend on these unstable tools is worth taking.\n\nOK.  It sounds like a sensible argument against such a change.\n\nThanks.\n"},{"id":"440685","messageId":"211108.86zgqee6rm.gmgdl@evledraar.gmail.com","threadId":"56850","inReplyTo":"xmqq7ddiv4oi.fsf@gitster.g","subject":"Re: move some test-tools to 'unstable plumbing' built-ins","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-08T20:19:10Z","receivedAt":"2021-11-08T20:23:30Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 08 2021, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n>> In my experience I *rarely* rely on test-helpers when debugging wedged\n>> repositories, and much more often end up either in gdb, or in an\n>> anonymized copy of the repository on a different server. I would imagine\n>> that others have similar experiences.\n>>\n>> So unless we had a much more compelling reason to have the test helpers\n>> more readily available, I do not think that the risk our users will\n>> begin to depend on these unstable tools is worth taking.\n>\n> OK.  It sounds like a sensible argument against such a change.\n\nIt's an argument against not flipping \"make installing them be optional\"\nflag on by default, but we could otherwise move some of t/helper to\nbuiltin/, which would help by encouraging us to write at least\nboilerplate docs for them.\n\nGit developers & similar parties could then set them to be installed for\nad-hoc debugging.\n\nWhatever anyone things on that, just on Taylor's \"begin to depend on\"...\n\nI really don't buy the argument that there's no amount of warnings in\nour documentation that we can include which would give us future license\nto willy-nilly change certain things.\n\nIf that is being argued then that seems to categorically exclude certain\nother things, e.g. including \"scalar\" in-tree at all, because if we\ncan't trust users to read the warnings about it being \"contrib-y\"...\n"},{"id":"440692","messageId":"YYmfdze1N7tL+2FE@nand.local","threadId":"56850","inReplyTo":"211108.86zgqee6rm.gmgdl@evledraar.gmail.com","subject":"Re: move some test-tools to 'unstable plumbing' built-ins","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-08T22:06:47Z","receivedAt":"2021-11-08T22:06:56Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 08, 2021 at 09:19:10PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Mon, Nov 08 2021, Junio C Hamano wrote:\n>\n> > Taylor Blau <me@ttaylorr.com> writes:\n> >\n> >> In my experience I *rarely* rely on test-helpers when debugging wedged\n> >> repositories, and much more often end up either in gdb, or in an\n> >> anonymized copy of the repository on a different server. I would imagine\n> >> that others have similar experiences.\n> >>\n> >> So unless we had a much more compelling reason to have the test helpers\n> >> more readily available, I do not think that the risk our users will\n> >> begin to depend on these unstable tools is worth taking.\n> >\n> > OK.  It sounds like a sensible argument against such a change.\n>\n> It's an argument against not flipping \"make installing them be optional\"\n> flag on by default, but we could otherwise move some of t/helper to\n> builtin/, which would help by encouraging us to write at least\n> boilerplate docs for them.\n>\n> Git developers & similar parties could then set them to be installed for\n> ad-hoc debugging.\n\nI was talking about users not heeding our warning, but I'm still not\nreally that compelled by making the test-helpers an optional component\nin our build.\n\nI am pretty sure I have only reached for the test-helpers less than half\na dozen times over the years, and *much* more often find myself in a\ndebugger. If I'm in the minority (and there really are a lot of\nadministrators who find it useful to have the test-tools on hand), then\nthat is a different story, but my guiding assumption is that that isn't\nthe case.\n\n> I really don't buy the argument that there's no amount of warnings in\n> our documentation that we can include which would give us future license\n> to willy-nilly change certain things.\n\nMy point was only that we cannot guarantee that users read or care about\nour documentation.\n\nThanks,\nTaylor\n"}]}