Skip to content

Fuzzer Migration Follow-ups - #1903

Merged
Byron merged 10 commits into
gitpython-developers:mainfrom
DaveLak:fuzzing-integration-follow-ups
Apr 18, 2024
Merged

Byron merged 10 commits into
gitpython-developers:mainfrom
DaveLak:fuzzing-integration-follow-ups

Conversation

@DaveLak

@DaveLak DaveLak commented Apr 16, 2024

Copy link
Copy Markdown
Contributor

This PR addresses most unresolved review comments from #1901. It also updates the README

Addressed in This PR

  • Remove shebangs from fuzz harnesses (Comment 1 & Comment 2)
  • Replace shebang in build.sh with ShellCheck directive (Comment)
  • Set executable bit on fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh to mark it executable in Git (Comment)
  • Make the link text in fuzzing/README.md for the OSS-Fuzz test status URL more descriptive (Comment)
  • Fix capitalization of GitPython repository name in fuzzing/README.md (Comment)
  • Simplify read delimiter to use empty string in build.sh's build fuzz harness loop (Comment)
  • Remove unnecessary semicolons in fuzzing/oss-fuzz-scripts/container-environment-bootstrap.sh for consistent script formatting (Comment)
  • Fix various misspellings of "corpora" (Comment)

Misc

TODO / Pending Further Discussion

I felt that these items are better addressed in separate PRs.

DaveLak added 7 commits April 16, 2024 14:41
Prefer executing these files using the OSS-Fuzz or `python` command
methods outlined in the `fuzzing/README`.

Based on feedback and discussion on:
gitpython-developers#1901
This script is meant to be sourced by the OSS-Fuzz file of the same
name, rather than executed directly. The shebang may lead to the
incorrect assumption that the script is meant for direct execution.
Replacing it with this directive instructs ShellCheck to treat
the script as a Bash script, regardless of how it is executed.

Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
This script is executed directly, not sourced as is the case with
`build.sh`, so it should have an executable bit set to avoid ambiguity.

Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
- Make the link text for the OSS-Fuzz test status URL more descriptive
- Fix capitalization of GitPython repository name

Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
Replaces the null character delimiter `-d $'\0'` with the simpler
empty string `-d ''` in the fuzzing harness build loop.

This changes leverages the Bash `read` builtin behavior to avoid
unnecessary complexity and improving script readability.

Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
A misspelling in the https://github.com/gitpython-developers/qa-assets
repository is still present here. It will need to be fixed in that
repository first.

"corpora" is a difficult word to spell consistently I guess. This made
for a good opportunity to improve the phrasing of two other comments at
at least.

Based @EliahKagan's suggestion and feedback on:
gitpython-developers#1901
@DaveLak
DaveLak marked this pull request as ready for review April 17, 2024 16:06