Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve GNU sed fallback while allowing use of -i.bak #447

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Improve GNU sed fallback while allowing use of -i.bak #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dcd470a
fix: Improve fallback when using sed
hyperupcall Aug 26, 2023
d162645
Removed: Superfluous error handling
inkarkat Dec 26, 2024
8783186
ENH: Allow overriding sed command via TODOTXT_SED_COMMAND
inkarkat Dec 26, 2024
96818cf
ENH: Allow opt-in to sed emulation for add-ons via TODOTXT_SED_EXPORT…
inkarkat Dec 26, 2024
2268f21
Minor: Prefer $TMPDIR if set; use /tmp as fallback
inkarkat Dec 29, 2024
e84a78e
Refactoring: Minor: Use if...then, single quotes, other separator, en…
inkarkat Dec 29, 2024
3232901
ENH: Auto-detect sed in-place editing capability during "make install"
inkarkat Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,15 @@ clean: test-pre-clean VERSION-FILE ## remove dist directory and all release fi
install: build installdirs ## local package install
$(INSTALL_PROGRAM) $(DISTNAME)/todo.sh $(DEST_COMMAND)
$(INSTALL_DATA) $(DISTNAME)/todo_completion $(DEST_COMPLETION)
[ -e $(DEST_CONFIG) ] || \
sed "s/^\(export[ \t]*TODO_DIR=\).*/\1~\/.todo/" $(DISTNAME)/todo.cfg > $(DEST_CONFIG)
if [ ! -e $(DEST_CONFIG) ]; then \
sed 's@^\(export TODO_DIR=\).*@\1~/.todo@' $(DISTNAME)/todo.cfg > $(DEST_CONFIG); \
if sed -i.bak 's@^# \(export TODOTXT_SED_COMMAND=sed\)$$@\1@' $(DEST_CONFIG) 2>/dev/null; then \
rm -f $(DEST_CONFIG).bak; \
echo 'This sed supports in-place editing.'; \
else \
echo 'Need sed in-place emulation here.'; \
fi; \
fi

.PHONY: uninstall
uninstall: ## uninstall package
Expand Down
10 changes: 10 additions & 0 deletions todo.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,13 @@ export REPORT_FILE="$TODO_DIR/report.txt"
# Set a default action for calling todo.sh without arguments.
# Also allows for parameters for the action.
# export TODOTXT_DEFAULT_ACTION=''

## sed capabilities
# If you have GNU sed or another sed that supports in-place editing
# via -i[SUFFIX], uncomment this for a minuscule performance gain.
# export TODOTXT_SED_COMMAND=sed

# Uncomment this if you have add-ons that also do in-place editing
# via sed, pass the -i.bak as the first argument like todo.sh itself,
# and your system needs the in-place emulation.
# export TODOTXT_SED_EXPORT_FOR_ADDONS=1
19 changes: 19 additions & 0 deletions todo.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,25 @@ export TODO_SH TODO_FULL_SH

oneline_usage="$TODO_SH [-fhpantvV] [-d todo_config] action [task_number] [task_description]"

# Assumption: The in-place argument is the first
# Assumption: Only a single file is processed with sed
sed() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead of TODOTXT_SED_COMMAND, we introduce some sort of TODOTXT_ENABLE_SED_SHIM, which is disabled by default?

Then, we can do:

Suggested change
sed() {
[ "$TODOTXT_SED_ENABLE_SHIM" = 1 ] && sed() {

That way, we can have better performance by default. If we go this route, then the variable TODOTXT_SED_EXPORT_FOR_ADDONS could be renamed to a more consistent TODOTXT_SED_EXPORT_SHIM_FOR_ADDONS. (and we would also have to check for this new variable before the export -f sed below)

Copy link
Contributor

@hyperupcall hyperupcall Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, and maybe SHIM isn't the best word, because that could imply that it's some special file at bin/sed, which imply an worse performance impact than it actually is. But even with a better name, the idea is the same

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to inject a custom sed command covers scenarios where people have a capable sed installed, but not accessible through PATH, or with a non-standard name. (Additionally, it enables nifty dev use cases, like injecting a verboseSed command that logs the sed arguments for troubleshooting.) To me, that's more flexibility for zero cost (versus an alternative yes/no flag).

If we go back to disabling the workaround by default, BSD and Busybox users are faced with a strange error message, and somehow have to find the workaround, like before. It's just a bit more convenient because the function is now packaged inside todo.sh, not a snippet they have to copy-and-paste into their configuration. But I'm afraid many won't easily find the flag in the config (who reads documentation and comments?), maybe google it, and then either find the GitHub issues (or just the older discussion mentioning the snippet).

I found a (somewhat clever 😊) way to auto-detect the in-place capability in the Makefile and manipulate the default config (which is already done for the TODO_DIR, anyway). It's still quite a bit of code for a very minor optimization, but now that CI/CD performs make [un]install for every change, it feels acceptable to me. I have no idea how many users use which installation method, and how many are actually using BSD / Busybox with todo.sh; if someone else argues with simplicity and to scrap the whole workaround in this PR, I'd be fine with that, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to inject a custom sed command covers scenarios where people have a capable sed installed, but not accessible through PATH, or with a non-standard name. (Additionally, it enables nifty dev use cases, like injecting a verboseSed command that logs the sed arguments for troubleshooting.) To me, that's more flexibility for zero cost (versus an alternative yes/no flag).

👍

If we go back to disabling the workaround by default, BSD and Busybox users are faced with a strange error message, and somehow have to find the workaround, like before.

I really wouldn't want to force somebody to look up some random GitHub issue to get things to work if say they're on Busybox and get some sort of error. So yeah, better to enable the workaround by default

I have no idea how many users use which installation method, and how many are actually using BSD / Busybox with todo.sh; if someone else argues with simplicity and to scrap the whole workaround in this PR, I'd be fine with that, too.

With the code already written, I see no reason to remove it! I think the auto-detection optimization is a good improvement

if [ -n "$TODOTXT_SED_COMMAND" ]; then
command "$TODOTXT_SED_COMMAND" "$@"
elif command -v gsed &>/dev/null; then
gsed "$@"
elif [ "$1" = '-i.bak' ]; then
shift
filepath=${!#}
filepath_temp=${TMPDIR-/tmp}/todo.sh-sed.$RANDOM.$$
command sed "$@" > "$filepath_temp" && mv "$filepath_temp" "$filepath"
else
command sed "$@"
fi
}
[ "$TODOTXT_SED_EXPORT_FOR_ADDONS" = 1 ] \
&& export -f sed

usage()
{
cat <<-EndUsage
Expand Down