From: Junio C Hamano <hidden> Date: 2021-11-05 18:52:20
Jeff King [off-list ref] writes:
If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being
that the repository does not have bitmaps at all), then we'll segfault
accessing bitmap_git->hashes:
$ t/helper/test-tool bitmap dump-hashes
Segmentation fault
We should treat this the same as a repository with bitmaps but no
name-hashes, and quietly produce an empty output. The later call to
free_bitmap_index() in the cleanup label is OK, as it treats a NULL
pointer as a noop.
This isn't a big deal in practice, as this function is intended for and
used only by test-tool. It's probably worth fixing to avoid confusion,
but not worth adding coverage for this to the test suite.
Signed-off-by: Jeff King <redacted>
---
This is new in the v2.34.0 cycle, but it's so low impact it doesn't
matter much if we ship with the bug. OTOH, it's pretty low-risk since it
is only run by the test suite.
;-)
I wonder how you found it. Diagnosing a repository that did not
seem healthy? What I am getting at is if we want a new option to
make a plumbing command, other than the test-tool, that calls this
function, as the latter is usually not deployed in the field.
Will queue. Thanks.
From: Taylor Blau <hidden> Date: 2021-11-05 19:11:16
On Fri, Nov 05, 2021 at 11:52:16AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being
that the repository does not have bitmaps at all), then we'll segfault
accessing bitmap_git->hashes:
$ t/helper/test-tool bitmap dump-hashes
Segmentation fault
We should treat this the same as a repository with bitmaps but no
name-hashes, and quietly produce an empty output. The later call to
free_bitmap_index() in the cleanup label is OK, as it treats a NULL
pointer as a noop.
This isn't a big deal in practice, as this function is intended for and
used only by test-tool. It's probably worth fixing to avoid confusion,
but not worth adding coverage for this to the test suite.
Signed-off-by: Jeff King <redacted>
---
This is new in the v2.34.0 cycle, but it's so low impact it doesn't
matter much if we ship with the bug. OTOH, it's pretty low-risk since it
is only run by the test suite.
;-)
Yes, this looks obviously correct to me. Thanks for spotting and fixing
this, Peff.
I'd be happy to see it in the 2.34 cycle, too, but I agree that it would
be OK if it didn't make the cut (and certainly if it makes it easier for
Junio to handle the rest of the release cycle, then I'm in favor of
leaving it out).
I wonder how you found it. Diagnosing a repository that did not
seem healthy? What I am getting at is if we want a new option to
make a plumbing command, other than the test-tool, that calls this
function, as the latter is usually not deployed in the field.
I would not be surprised if this was discovered via Coverity, or by
manual inspection. Peff and I have been merging a slew of releases from
your tree into GitHub's fork and so have been reading code in the more
recently changed areas.
On the test-tool vs. plumbing thing: I think there are some compelling
reasons in either direction. There's no *good* home for these in our
current set of plumbing tools. E.g., the closest example we have is `git
rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed
these new inspection tools for some of the newer bitmap-related tests,
adding them via the test-helper suite was a conscious choice to not
build on the ugliness of `--test-bitmap`.
But on occasion these test-tool things are useful to have "in the
field", as you say. It's rare enough that I usually just clone a copy of
our fork as needed and build it when I do find myself reaching for
test-helpers.
Thanks,
Taylor
From: Jeff King <hidden> Date: 2021-11-05 23:29:51
On Fri, Nov 05, 2021 at 03:11:11PM -0400, Taylor Blau wrote:
quoted
I wonder how you found it. Diagnosing a repository that did not
seem healthy? What I am getting at is if we want a new option to
make a plumbing command, other than the test-tool, that calls this
function, as the latter is usually not deployed in the field.
I would not be surprised if this was discovered via Coverity, or by
manual inspection. Peff and I have been merging a slew of releases from
your tree into GitHub's fork and so have been reading code in the more
recently changed areas.
It was Coverity in this case. I haven't actually used the name-hash
dumper for any real-world debugging.
On the test-tool vs. plumbing thing: I think there are some compelling
reasons in either direction. There's no *good* home for these in our
current set of plumbing tools. E.g., the closest example we have is `git
rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed
these new inspection tools for some of the newer bitmap-related tests,
adding them via the test-helper suite was a conscious choice to not
build on the ugliness of `--test-bitmap`.
But on occasion these test-tool things are useful to have "in the
field", as you say. It's rare enough that I usually just clone a copy of
our fork as needed and build it when I do find myself reaching for
test-helpers.
Yeah, I could see arguments both ways on such tools (not just bitmaps,
but other "debug this binary format" tools like read-midx and
read-graph). I'm content to leave it as-is until I come across more
in-the-field cases where those tools would be useful. Half the time I
end up in a debugger anyway. ;)
-Peff
On Fri, Nov 05, 2021 at 11:52:16AM -0700, Junio C Hamano wrote:
quoted
Jeff King [off-list ref] writes:
quoted
If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being
that the repository does not have bitmaps at all), then we'll segfault
accessing bitmap_git->hashes:
$ t/helper/test-tool bitmap dump-hashes
Segmentation fault
We should treat this the same as a repository with bitmaps but no
name-hashes, and quietly produce an empty output. The later call to
free_bitmap_index() in the cleanup label is OK, as it treats a NULL
pointer as a noop.
This isn't a big deal in practice, as this function is intended for and
used only by test-tool. It's probably worth fixing to avoid confusion,
but not worth adding coverage for this to the test suite.
Signed-off-by: Jeff King <redacted>
---
This is new in the v2.34.0 cycle, but it's so low impact it doesn't
matter much if we ship with the bug. OTOH, it's pretty low-risk since it
is only run by the test suite.
;-)
Yes, this looks obviously correct to me. Thanks for spotting and fixing
this, Peff.
I'd be happy to see it in the 2.34 cycle, too, but I agree that it would
be OK if it didn't make the cut (and certainly if it makes it easier for
Junio to handle the rest of the release cycle, then I'm in favor of
leaving it out).
quoted
I wonder how you found it. Diagnosing a repository that did not
seem healthy? What I am getting at is if we want a new option to
make a plumbing command, other than the test-tool, that calls this
function, as the latter is usually not deployed in the field.
I would not be surprised if this was discovered via Coverity, or by
manual inspection. Peff and I have been merging a slew of releases from
your tree into GitHub's fork and so have been reading code in the more
recently changed areas.
On the test-tool vs. plumbing thing: I think there are some compelling
reasons in either direction. There's no *good* home for these in our
current set of plumbing tools. E.g., the closest example we have is `git
rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed
these new inspection tools for some of the newer bitmap-related tests,
adding them via the test-helper suite was a conscious choice to not
build on the ugliness of `--test-bitmap`.
But on occasion these test-tool things are useful to have "in the
field", as you say. It's rare enough that I usually just clone a copy of
our fork as needed and build it when I do find myself reaching for
test-helpers.
As part of the proposed integration for "scalar" I added a category to
the command-list.txt called "optionalcontrib", which we'll list on its
own in "man git" as (paraphrasing) super-duper-experimental.
I really don't see why we shouldn't do so very lightly with some of
these remotely-useful test-tool tools.
It's pretty much the same amount of work to create a new built-in as a
new test-tool, and as long as we make it clear in our documentation that
these aren't in the same "plumbing" category I don't see why we
shouldn't add those quite freely.
We can even make installing them be optional, as some distributors might
prefer that, which the scalar patch at [1] also does. So all the
boilerplate is there for someone who'd like to run with this idea.
It seems to me that we've ended up with the current status quo of not
adding "new plumbing" because we'll need to support it forever out of
some self-imposed constraint that we couldn't add new categories to the
"git" manual page.
But if we just prominently list them as being unstable helpers aimed at
git experts, and note the same thing prominently in their manual page
(trivially done via an include) everyone should be on the same page
about their stability, and we'll be able to use stuff like "test-tool
pkt-line" "in the field".
1. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/
From: Taylor Blau <hidden> Date: 2021-11-07 17:06:18
On Sat, Nov 06, 2021 at 05:08:25AM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
On the test-tool vs. plumbing thing: I think there are some compelling
reasons in either direction. There's no *good* home for these in our
current set of plumbing tools. E.g., the closest example we have is `git
rev-list --test-bitmap <rev>`, which is kind of ugly. When we needed
these new inspection tools for some of the newer bitmap-related tests,
adding them via the test-helper suite was a conscious choice to not
build on the ugliness of `--test-bitmap`.
But on occasion these test-tool things are useful to have "in the
field", as you say. It's rare enough that I usually just clone a copy of
our fork as needed and build it when I do find myself reaching for
test-helpers.
As part of the proposed integration for "scalar" I added a category to
the command-list.txt called "optionalcontrib", which we'll list on its
own in "man git" as (paraphrasing) super-duper-experimental.
I really don't see why we shouldn't do so very lightly with some of
these remotely-useful test-tool tools.
It's pretty much the same amount of work to create a new built-in as a
new test-tool, and as long as we make it clear in our documentation that
these aren't in the same "plumbing" category I don't see why we
shouldn't add those quite freely.
I'm not sure that I agree completely.
Just because it's easy to create a new builtin (or as easy as it is to
make a new test helper) does not alone make it a good reason to do so.
I think that your idea to make a distinction between these and normal
plumbing tools is a good one.
But...
It seems to me that we've ended up with the current status quo of not
adding "new plumbing" because we'll need to support it forever out of
some self-imposed constraint that we couldn't add new categories to the
"git" manual page.
But if we just prominently list them as being unstable helpers aimed at
git experts, and note the same thing prominently in their manual page
(trivially done via an include) everyone should be on the same page
about their stability, and we'll be able to use stuff like "test-tool
pkt-line" "in the field".
...just because we say that these tools are unstable does not mean that
users will listen to us (or even read the relevant documentation saying
so).
In my experience I *rarely* rely on test-helpers when debugging wedged
repositories, and much more often end up either in gdb, or in an
anonymized copy of the repository on a different server. I would imagine
that others have similar experiences.
So unless we had a much more compelling reason to have the test helpers
more readily available, I do not think that the risk our users will
begin to depend on these unstable tools is worth taking.
Thanks,
Taylor