Skip to content

Update gh-md-toc, setting the -e flag - #163

Open
mmattel wants to merge 1 commit into
ekalinin:masterfrom
mmattel:patch-1
Open

mmattel wants to merge 1 commit into
ekalinin:masterfrom
mmattel:patch-1

Conversation

@mmattel

@mmattel mmattel commented Nov 5, 2025

Copy link
Copy Markdown

Setting set -e to stop on first error. This is good bash practice...

BTW, I like this script a lot !!

Setting `set -e` to stop on first error. This is good bash practice...
@ekalinin

Copy link
Copy Markdown
Owner

Hi @mmattel, thanks for the PR and for the kind words!

I tested set -e against current master. It makes things worse in a few places, and it still doesn't catch the errors it's meant to catch:

  1. Stdin input can silently lose the TOC. gh_toc_md2html runs on the left side of a pipeline, and that subshell inherits set -e. For example, say there is an unreadable token.txt next to the script and GH_TOC_TOKEN is not set. Before, printf '# Hello\n## World\n' | ./gh-md-toc - fell back to an unauthenticated request and printed the TOC. With set -e it prints an empty line and exits 0.
  2. The network error message is lost in POSIX mode. When bash runs in POSIX mode (for example sh gh-md-toc README.md on macOS), command substitutions inherit set -e. A failing curl then exits before the network error check added in fix(md2html): check curl exit status for network errors #172 runs. Instead of "Parsing local markdown file requires access to github API" and exit 1, you get no message and exit 7. With --skip-header it also leaves a README.md~~ temp file behind.
  3. --indent and --depth without a value now exit 1 with no message, because shift 2 fails.
  4. Common failures still exit 0. There is no pipefail, and curl -s runs without --fail. So a network error on stdin input, a rate limit, or a 401 from a bad token still gives an empty TOC with exit code 0.

In --insert mode, set -e can also stop the script halfway. If writing the new TOC fails after sed -i has already cleared the old one, the script exits without telling the user where the backup is.

So I'd rather not add a global set -e. I think a targeted fix works better:

  • check the XXNetworkErrorXX / XXRateLimitXX markers in the stdin path, the same way it's already done for local files
  • possibly use curl --fail
  • check that --indent and --depth were given a value
  • add bats tests for these error paths

If you'd like to rework the PR in that direction, I'm happy to review it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants