Thanks, I appreciate the kind words 🙂 And yes,>[!TIP] is a handy feature for sure.There are a few others as well.

About Fuzzing-Fixtures

Overall, the extra-files are just about 2MB in total, and I wonder how stable they will be. If stable, I could imagine including them in this repo. If there is any concerns about this,gitpython-developers could fork the repo and make you an admin on it.

I think a dedicated repo under thegitpython-developers org would be the best option. No need to fork mine though, a new one with whatever license you prefer would be fine. I have no preference as far my own permissions though.

Ideally, a seed corpus would be added for each new fuzz target. Obviously that means more files but it doesn't necessarily mean "10 fuzz targets = 10mb". The size & number of interesting inputs will change over time and depending on the complexity/number of code paths in the functionality under test, and it's hard to know what that equates to in terms of per target corpus size at this point. The two zips in my repo right now actually demonstrate that pretty nicely: both of them basically top out their respective coverage quickly but one zip is ~450kb while the other is ~1.5mb.

I like how Bitcoin Core handles inhttps://github.com/bitcoin-core/qa-assets. From what I've seen, that seems like the cleanest and most flexible.

About archives for publishing on PyPy

I created a new archive from the branch of this PR withpython -m build --sdist --wheel and didn't see any trace offuzzing in the produced archive.

Regarding the OSS fuzz PR

I didn't review it for correctness, but approved it in case it helps getting it merge.

Regarding my review

I just glanced at the shell scripts, and basically ignored the google-provided fuzz targets.

Awesome thanks!

I think a dedicated repo under thegitpython-developers org would be the best option. No need to fork mine though, a new one with whatever license you prefer would be fine. I have no preference as far my own permissions though.

Great! I have created a fork of your repository under this organization and added you as maintainer. This should give you enough permissions, but if not, you could beadmin as well.

You see that I named it after the same repository of the bitcoin organization.

Once you adjusted the repository URL to the fork under this organisation's control, the PR would be ready for merge from my side.

Thanks again for all your work here!

Thanks@Byron! I've updated the URL to unblock this PR and I'll follow up with a PR to cleanup the new qa-assets repo following the pattern of the Bitcoin one.

Thanks for the thorough review@EliahKagan! It's very helpful, and I'm particularly appreciative of the thoughtful explanations and suggestions you shared.

I'll push some changes shortly to address your feedback, prioritizing the updates related to the license, so we can get this merged and unblock the downstream OSS-Fuzz PR.

As for anything else that you explicitly noted as non-blocking: I'll either address them in a follow-up, or reply to in the
relevant comments to discuss further. I do think they're all valuable suggestions and worth addressing ASAP.

@EliahKagan, commit945a767 addresses the blocking concerns regarding the licensing and documentation of Apache licensed files. Please see the diff of that commit for specifics.

Sorry about the force-push; I wanted to make sure the OSS-Fuzz commit at the time I copied the Apache license file from was captured in the commit message.¹

Regarding:

It seems to me that the most hassle-free way to achieve this is to copy the license file from the repository the Apache 2.0 licensed Python scripts originally came from (so long as there is nothing unusual about it) into thefuzzing/ directory, and name itLICENSE-APACHE.

I did exactly that. I also checked the diff of the contents of the file included here againsthttps://www.apache.org/licenses/LICENSE-2.0.txt just to be sure; it looks good.

--- fuzzing/LICENSE-APACHE+++ fuzzing/LICENSE-APACHE-from-website-src@@ -1,3 +1,4 @@+                              Apache License                        Version 2.0, January 2004                     http://www.apache.org/licenses/@@ -178,7 +179,7 @@APPENDIX: How to apply the Apache License to your work.       To apply the Apache License to your work, attach the following-      boilerplate notice, with the fields enclosed by brackets "{}"+      boilerplate notice, with the fields enclosed by brackets "[]"       replaced with your own identifying information. (Don't include       the brackets!)  The text should be enclosed in the appropriate       comment syntax for the file format. We also recommend that a@@ -186,7 +187,7 @@same "printed page" as the copyright notice for easieridentification within third-party archives.-   Copyright {yyyy} {name of copyright owner}+   Copyright [yyyy] [name of copyright owner]    Licensed under the Apache License, Version 2.0 (the "License");    you may not use this file except in compliance with the License.    @@ -198,4 +199,4 @@    distributed under the License is distributed on an "AS IS" BASIS,    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.    See the License for the specific language governing permissions and-   limitations under the License.+   limitations under the License.    \ No newline at end of file

CC@Byron

FYI, I'm hesitant to push more commits here, including via the suggestion comments, because I don't want to keep making the review approvals stale.

I'll reply to the remaining as necessary of course, but I'm going to start a new branch for them unless someone suggests otherwise. Lmk if either of you would prefer them on this PR directly.

FYI, I'm hesitant to push more commits here, including via the suggestion comments, because I don't want to keep making the review approvals stale.

I'll reply to the remaining as necessary of course, but I'm going to start a new branch for them unless someone suggests otherwise. Lmk if either of you would prefer them on this PR directly.

I agree. I think this PR is in good shape to be merged, and further refinements can be made in one or more subsequent PRs.

This is the case even for future adjustments, if any, that would affect how the licensing situation is described. Everything that was blocking has been fully resolved.

FWIW@EliahKagan I tackled most of the simpler changes and put up a Draft PR:#1903

Thanks both! I updated the branch for#1903 and marked it ready for review.

I agree the state offuzz_tree.py is not ideal and appreciate the feedback you each left about it. I've given it some thought as well but I hadn't considered some of the points raised.

I still plan to share in a reply on the unresolved thread here, I just haven't had a chance to yet. I should be able to in a few hours.

Regarding the hardcoded/tmp path that then expects a/tmp/.git, I agree that this under no circumstances should be running in an unisolated environment. Since the documentation clearly states that docker is needed, andif it's unlikely to impossible to run outside (or to get to that line), then I think it's OK to leave it.

To be clear,Docker is not needed, so this can be the real, unisolated/tmp, and the documentation presents that as a reasonable thing to do under some circumstances. It waswith awareness of this that I recommended this be merged even before that is fixed.

However, I realize I omitted a piece of my reasoning: Ithink it's easier to fix this after both this PR andgoogle/oss-fuzz#11803 are merged (the latter is not merged yet but looks like it will be merged soon), because that will make it easier to test the fix both outside and inside Docker.

(The Docker case is, I believe, more important, since that's how the tests are automatically run on the OSS-Fuzz infrastructure, and running individual tests outside of Docker is, if I understand correctly, mostly of interest when working on the fuzz tests themselves.)

For the same reason, I think it is justified for#1903 not to include a fix for this, since the changes in#1903 otherwise could be merged at any time and do not need to wait ongoogle/oss-fuzz#11803.

Ok, sorry for the delay. Below are my thoughts regardingfuzz_tree.py, but also, a little extra context on the OSS-Fuzz execution environment because it may be helpful for thinking about#1901 (comment).

About the OSS-Fuzz Environment

You both touched on it, but for the sake of shared understanding, the scripts and tests look like they do in part because of the OSS-Fuzz container environment implementation details.

The key points to understand are:

• There are two stages of an OSS-Fuzz run that influence why the shell scripts and fuzz tests look like they do: abuild stage and afuzzer execution stage.
• Each stage has its own distinct container environment.
• Each container environment's filesystem is mostly read-only.
• The containers are Linux based (Ubuntu 20.04 right now I believe.)

Build Stage Execution Environment

As per this page of the docs:

build.sh script environment
When your build.sh script is executed, the following locations are available within the image:

Location	Env Variable	Description
/out/	$OUT	Directory to store build artifacts (fuzz targets, dictionaries, options files, seed corpus archives).
/src/	$SRC	Directory to checkout source files.
/work/	$WORK	Directory to store intermediate files.

Everything else is read-only.

Fuzzer Execution Stage Environment

As per this page of the docs:

File system
Everything except/tmp is read-only, including the directory that your fuzz target executable lives in.
/dev is also unavailable.

Regarding the Fuzz Tests

When I initially started on this, my primary objective was really only about fixing the build. I did try to clean them up a bit in the process and apply some best practices, but those changes were mostly incidental and I intentionally avoided any significant refactors.

After we decided to bring them here, my focus shifted to lowering the barrier to entry for contributors because of the nature of the project. In particular, I tried to distill the useful information from various place that I haven't seen documented in one place yet. One such tip is the direct execution section, but in hindsight I overlooked the environment assumptions infuzz_tree.py.

Onfuzz_tree.py Specifically

This test is less than ideal for several reasons.

1. Writing to the/tmp dir

In addition to what has been mentioned,modifying global state andI/O operations (e.g., writing to the temp dir) are problematic for fuzzing in general.

Further,@EliahKagan's remark in#1901 (comment) about "implicating other things in/tmp" made me realize this may already be causing issues inside the OSS-Fuzz execution environment. A non-obvious implementation detail specific to Python projects in OSS-Fuzz thatthecompile_python_fuzzer function in thebuild.sh script usesPyinstaller to bundle the fuzz harness and all related dependenciesinto a single file archive that gets transferred to the fuzzer execution stage, where it is unpacked into a subdirectory of/tmp for execution. Which, I believe means, the test execution is actually using the test executable as a fixture 🥴

I have yet to confirm this, but I wouldn't be surprised if that is the cause of the issue I described as a possible " circular dependency " ingoogle/oss-fuzz#11763

2. It's very slow

With an average execs per second of ~4 after the seed corpus loads on a short local run,it is far, far, below what is hoped for. In contrast, I'm seeing about 1432 exec/s onfuzz_config.py in a similar local run.fuzz_config.py creates a config fake in-memory but never writes it to disk.

I haven't dug in yet, but I think this is partly related to point 1 above and partly related to some of the implementation details of the legacy code in GitPython.

This is less of a concern/priority than the/tmp dir issue though because even if it's slow, it's still at least testing the API.

Thoughts on Next Steps

I'm fully in agreement that the/tmp issue and documentation related to it must be fixed before other updates are considered. I think it can be broken down into smaller problems in the following prioritized order:

1. Address adverse potential that the local execution documentation exposes unaware contributors to.

Possible quick fix options:

• Remove that section all together for now (probably quickest and safest?)
• Adda Warning or Caution callout to that section aboutfuzz_tree.py (this feels not as great IMHO)

2. Address the/tmp writing implementation issues infuzz_tree.py

• TemporaryDirectory as suggested by@EliahKagan looks like a good solution but I would need to test it in OSS-Fuzz (which I can certainly do regardless of the downstream PR's status.)
• I've also considered proposing we add a Dockerfile to thefuzzing/ dir that would provide a "batteries included" way for folks to use the direct execution method quickly and easily inside a container environment prepared specifically to support it. Even before this discussion, I thought that would be the ideal way to document it because users wouldn't need to worry about any of the particulars beyond having Docker installed, which they'd already need for the alternative anyway.

I think the most robust solution would be some combination of both points above, but I don't know enough to comment on the potential issues that we may face goinf theTemporaryDirectory route inside OSS-Fuzz. The Dockerfile solution on the other hand is something I've already been toying with locally.

I also looked at options likeMemoryFS fromhttps://github.com/PyFilesystem/pyfilesystem2 because an in-memory solution would be ideal, but I think thegit executable invocation may cause issues with that approach.

What do you both think?

Thanks so much for the follow-up - I feel well-informed now!

As OSS-fuzz strongly relies on docker, maybe it would be easiest to 'go with the flow' and provide a dockerfile as part of thefuzzing directory. That way, the docs can refer exclusively to that and thanks to docker, everything, including dependencies, would work just as if they would run on the OSS-fuzz infrastructure.
Doing so would allow me to run these as well, as generally I am no fan of venv and all - docker is fine though and I have experience with it.

As second step, one could see if improving the tempdir usage would fix certain issues that you have been seeing, including to make using OSS fuzz safe for running it directly.gitoxide can do that without issues, by the way, and maybe being able to do that helps with the local development workflow.

With that said, I also think that… I'd really want you to do more withgitoxide 😁, and even though Rust OSS fuzzing is currently broken across the ecosystem, maybe there are some parsers that still don't have enough fuzzerage (fuzzing coverage). Maybe it would be more fun, too, as 'the system' won't be in your way anymore. But I digress 😅.

As OSS-fuzz strongly relies on docker, maybe it would be easiest to 'go with the flow' and provide a dockerfile as part of thefuzzing directory. That way, the docs can refer exclusively to that and thanks to docker, everything, including dependencies, would work just as if they would run on the OSS-fuzz infrastructure.
Doing so would allow me to run these as well, as generally I am no fan of venv and all - docker is fine though and I have experience with it.

I've added a simple Dockerfile and I instructions for using it and a warning about not using Docker in#1904. Let me know what you think!

As second step, one could see if improving the tempdir usage would fix certain issues that you have been seeing, including to make using OSS fuzz safe for running it directly.

I'm planning to take a deeper at this. Even without the/tmp concerns, there is something that is blockingFuzz Introspector from working properly, which would be nice to resolve.

With that said, I also think that… I'd really want you to do more withgitoxide 😁

I'm definitely interested in taking you up on that! I've been interested in learning more about Rust for a while and I've found working on fizzers a great way to get familiar with new projects and languages, so you can likely expect to see me pop up in that project sooner or later 😁

In other news, the downstream OSS-Fuzz PR was merged and the Gitpython builds are succeeding!

https://introspector.oss-fuzz.com/project-profile?project=gitpython

The Coverage build is still reporting failures, but that is expected until ClusterFuzz generates enough corpora to run the analysis which I expect will happen in the next day or two.

Exciting to see!