todo_rollover: atomic write, honest output, real argument handling
Hardening pass over the script. Every fix below was reproduced before being
made; two findings that looked serious were tested and dropped.
THE ONE THAT MATTERED. The write was not atomic and could destroy a day's
todos. It did:
cp -p "$src" "$dst" # yesterday's UNPRUNED note lands at the target
cat "$tmp" > "$dst" # then gets overwritten with the real content
Interrupted between those, today's note silently becomes yesterday's,
unpruned. Interrupted during the redirect, it is truncated. Content is now
built in a temp inside the notes directory and renamed over the target, so
the target is either the old file or the new one and never a half-written
mixture.
⚠ `cp -p` was NOT redundant, and a review pass that recommended removing it
was wrong. A bare `cat > "$dst"` preserves the mode only when the target
already exists; on the normal path the target is new and takes 0644 from the
umask. Measured: source 600 -> new file via redirect 644, pre-existing file
600. The mode is now set explicitly with `chmod --reference` before the
rename, which is correct in both cases.
OUTPUT THAT LIED. The success line always said "(pruned)", including when
the prune script was missing and the plain-copy fallback ran. That is the
same defect as the error message that advertised a --force flag before one
existed: a user-facing string asserting work the code did not do. The
message now reports which path was taken, and a prune script that FAILS is
reported instead of aborting on set -e with no explanation.
ARGUMENT HANDLING. Anything that was not --force became a date, so
`todo_rollover --dry-run` failed with "source note .../--dry-run.md not
found" -- a missing-file error for what was really a bad flag. Unknown
options are now rejected, `--` and `-h/--help` work, arity is checked, and
dates are validated for shape AND realness (2026-13-45 is caught up front).
ALSO FIXED
- `[[ -x $PRUNE ]]` was the wrong test: PRUNE is run as `python3 "$PRUNE"`
and needs no execute bit, so a working prune script was silently skipped.
Now -f.
- src == dst aborted on a raw `cp: ... are the same file`. Checked explicitly
up front with a clear message. (It did fail safe -- a review pass claiming
the source was destroyed was tested and is not reproducible.)
- The heading regex rejected "# 2026-09-21 " with a trailing space and
prepended a duplicate heading. Now tolerates surrounding whitespace.
- `${tmp}.new` escaped the EXIT trap and leaked on interrupt.
- Dead `mkdir -p "$NOTES"` after the source check had already proven the
directory exists, and a `mode=` variable that was assigned and never used.
Verified: unknown flag/bad date/missing source exit 1, existing target exits
2, --help exits 0, mode 600 survives the rollover, a non-executable prune
script is used, a failing one leaves the target untouched, the trailing-space
heading is retitled rather than duplicated, and no temp files are left behind.
This commit is contained in:
connor committed
1 parent
ec5b9b3c0d
commit
fb62fdfc8e
1 file changed
+87
-25
+87
-25
@@ -1,34 +1,84 @@
|
||||
#!/usr/bin/env bash
|
||||
# On-demand rollover of a todo note.
|
||||
# By default copies yesterday's note to today, pruning finished work and retitling.
|
||||
# Usage: todo_rollover [--force] [source_date] [target_date]
|
||||
#
|
||||
# Usage: todo_rollover [--force] [--] [source_date] [target_date]
|
||||
# source_date defaults to yesterday (YYYY-MM-DD)
|
||||
# target_date defaults to today
|
||||
# --force overwrites an existing target
|
||||
# -h, --help show this message
|
||||
# Set TODO_NOTES to override notes directory.
|
||||
# Set TODO_PRUNE to override prune script.
|
||||
#
|
||||
# Exit codes: 1 bad input / missing source, 2 target exists (without --force).
|
||||
#
|
||||
# ⚠ The write is ATOMIC BY DESIGN: content is built in a temp file inside the
|
||||
# notes directory, then renamed over the target. This file holds live todos,
|
||||
# and the earlier version did `cp -p "$src" "$dst"` followed by
|
||||
# `cat "$tmp" > "$dst"` -- which put YESTERDAY'S UNPRUNED note at the target
|
||||
# path first, then overwrote it. Interrupted between the two, today's note
|
||||
# silently became yesterday's; interrupted during the redirect, it was left
|
||||
# truncated. Keep the rename. Do not "simplify" it back to a redirect.
|
||||
#
|
||||
# ⚠ `cp -p` was NOT redundant and removing it is a regression. A plain
|
||||
# `cat > "$dst"` preserves the mode only when the target ALREADY EXISTS;
|
||||
# on the normal path the target is new and gets 0644 from the umask instead
|
||||
# of the source's mode. Measured 2026-09-22: source 600 -> new file via
|
||||
# redirect 644, pre-existing file 600. Mode is now set explicitly on the temp
|
||||
# before the rename, which covers both cases.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
NOTES=${TODO_NOTES:-$HOME/docs/notes/todo}
|
||||
PRUNE=${TODO_PRUNE:-$HOME/.local/lib/todo/prune.py}
|
||||
|
||||
usage() { sed -n '2,13p' "$0" | sed 's/^# \{0,1\}//'; }
|
||||
|
||||
force=0
|
||||
args=()
|
||||
for a in "$@"; do
|
||||
if [[ $a == --force ]]; then
|
||||
force=1
|
||||
else
|
||||
args+=("$a")
|
||||
fi
|
||||
while (( $# )); do
|
||||
case $1 in
|
||||
--force) force=1 ;;
|
||||
-h|--help) usage; exit 0 ;;
|
||||
--) shift; args+=("$@"); break ;;
|
||||
# ⚠ Reject unknown flags rather than treating them as a date. Previously
|
||||
# `todo_rollover --dry-run` set source_date to "--dry-run" and failed with
|
||||
# "source note .../--dry-run.md not found", which reads as a missing file
|
||||
# rather than a bad flag.
|
||||
-*) echo "todo_rollover: unknown option '$1' (try --help)" >&2; exit 1 ;;
|
||||
*) args+=("$1") ;;
|
||||
esac
|
||||
shift
|
||||
done
|
||||
|
||||
if (( ${#args[@]} > 2 )); then
|
||||
echo "todo_rollover: expected at most 2 dates, got ${#args[@]} (try --help)" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
src_date=${args[0]:-$(date -d yesterday +%Y-%m-%d)}
|
||||
target_date=${args[1]:-$(date +%Y-%m-%d)}
|
||||
|
||||
# Validate shape AND that the date is real, so 2026-13-45 fails here rather
|
||||
# than as a confusing "source note not found".
|
||||
for d in "$src_date" "$target_date"; do
|
||||
if [[ ! $d =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]] || ! date -d "$d" >/dev/null 2>&1; then
|
||||
echo "todo_rollover: '$d' is not a valid YYYY-MM-DD date" >&2
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
|
||||
src="$NOTES/$src_date.md"
|
||||
dst="$NOTES/$target_date.md"
|
||||
|
||||
# ⚠ Explicit, BEFORE anything is written. `cp` happens to refuse a same-file
|
||||
# copy, so the old code failed safe -- but by aborting mid-script on a raw
|
||||
# `cp: ... are the same file` error. Say it plainly instead.
|
||||
if [[ $src -ef $dst || $src == "$dst" ]]; then
|
||||
echo "todo_rollover: source and target are the same file ($src)" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if [[ ! -f $src ]]; then
|
||||
echo "todo_rollover: source note $src not found" >&2
|
||||
exit 1
|
||||
@@ -36,35 +86,47 @@ fi
|
||||
|
||||
if [[ -f $dst && $force -eq 0 ]]; then
|
||||
echo "todo_rollover: target $dst already exists, skipping (use --force to overwrite)" >&2
|
||||
echo " Existing target was created from a previous rollover. If you really want to replace it, remove it first." >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
mkdir -p "$NOTES"
|
||||
tmp=$(mktemp)
|
||||
trap 'rm -f "$tmp"' EXIT
|
||||
# Temp lives in the notes directory so the final rename is same-filesystem,
|
||||
# and therefore atomic.
|
||||
tmp=$(mktemp "$NOTES/.todo_rollover.XXXXXX")
|
||||
trap 'rm -f "$tmp" "$tmp.new"' EXIT
|
||||
|
||||
if [[ -x $PRUNE ]]; then
|
||||
python3 "$PRUNE" "$src" > "$tmp"
|
||||
# ⚠ Test -f, not -x. PRUNE is invoked as `python3 "$PRUNE"`, which does not
|
||||
# need the execute bit; -x sent a perfectly good prune script down the
|
||||
# no-prune fallback silently.
|
||||
pruned=0
|
||||
if [[ -f $PRUNE ]]; then
|
||||
if ! python3 "$PRUNE" "$src" > "$tmp"; then
|
||||
echo "todo_rollover: prune script failed: python3 $PRUNE $src" >&2
|
||||
exit 1
|
||||
fi
|
||||
pruned=1
|
||||
else
|
||||
# fallback: just copy
|
||||
cp "$src" "$tmp"
|
||||
fi
|
||||
|
||||
# Retitle first line to target date, matching rollover.py behavior
|
||||
# Only replace line 1 if it looks like a date heading
|
||||
# Retitle line 1 to the target date. Tolerates surrounding whitespace -- the
|
||||
# stricter pattern treated "# 2026-09-21 " as not-a-heading and prepended a
|
||||
# second one.
|
||||
first_line=$(head -n1 "$tmp")
|
||||
if [[ $first_line =~ ^#\ [0-9]{4}-[0-9]{2}-[0-9]{2}$ ]]; then
|
||||
if [[ $first_line =~ ^[[:space:]]*#[[:space:]]+[0-9]{4}-[0-9]{2}-[0-9]{2}[[:space:]]*$ ]]; then
|
||||
sed -i "1s/.*/# $target_date/" "$tmp"
|
||||
else
|
||||
# prepend heading
|
||||
printf "# %s\n\n" "$target_date" | cat - "$tmp" > "${tmp}.new"
|
||||
mv "${tmp}.new" "$tmp"
|
||||
printf "# %s\n\n" "$target_date" | cat - "$tmp" > "$tmp.new"
|
||||
mv "$tmp.new" "$tmp"
|
||||
fi
|
||||
|
||||
# Preserve mode of source
|
||||
mode=$(stat -c %a "$src")
|
||||
cp -p "$src" "$dst" # to get mode
|
||||
cat "$tmp" > "$dst"
|
||||
chmod --reference="$src" "$tmp"
|
||||
mv -f "$tmp" "$dst"
|
||||
|
||||
echo "todo_rollover: created $dst from $src_date.md (pruned)"
|
||||
# ⚠ Only claim "(pruned)" when pruning actually ran. The previous version said
|
||||
# it unconditionally, including on the no-prune fallback -- the same defect as
|
||||
# an error message advertising a --force flag that was never implemented.
|
||||
if (( pruned )); then
|
||||
echo "todo_rollover: created $dst from $src_date.md (pruned)"
|
||||
else
|
||||
echo "todo_rollover: created $dst from $src_date.md (NOT pruned: $PRUNE not found)"
|
||||
fi
|
||||
Reference in new issue
Block a user