Improve GNU sed fallback while allowing use of -i.bak #447
Conversation
There was a problem hiding this comment.
Adding to my original comment, these changes are a good improvement! I have suggested an idea that could address the concern that enabling the sed shim/function by default could be suboptimal for performance (it also feels a bit messy)
| @@ -26,6 +26,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() { | |||
There was a problem hiding this comment.
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:
| 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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
The shift is guaranteed to work, as the branch is only taken when $1 is set.
On most Linux distributions (e.g. Ubuntu), there's no gsed command; the default "sed" is GNU sed with in-place editing capabilities. For those systems, we don't want the redirect + file move workaround applied. However, detecting the sed capabilities is error-prone and just as inefficient as the separate "mv" it would avoid. Instead, add a configuration that users can uncomment. Ideally, package maintainers for Linux distributions that have GNU sed ship with this already commented out in the default configuration.
…_FOR_ADDONS flag Most add-ons likely copied the "sed -i.bak ..." from todo.sh, but we can't be sure. Therefore, just document the issue in the configuration and offer a simple switch to enable it. As most users are either on Linux or Windows/Cygwin where this isn't an issue, the rather technical language only applies to users of BSD or MacOS users who don't want to install GNU sed.
Co-authored-by: Edwin Kofler <[email protected]>
…force space between export and var
We're already preprocessing the default configuration; it's easy to add another sed invocation to uncomment the TODOTXT_SED_COMMAND definition. And since we're using in-place editing here, the sed command will only succeed if it has that capability. In other words, it's both the capability detection and the corresponding config adaptation in one command! The config.bak file is cleaned up on success (a backup-less -i could be used, but as todo.sh uses the backup file variant and there might be sed commands that support one but not the other, it's safer this way). Both outcomes (emulation required or not) are logged as well.
inkarkat
force-pushed
the
fix-inplace-sed
branch
from
92124cb to
3232901
Compare
Before submitting a pull request, please make sure the following is done:
master.fixes #XXreference to the issue that this pull request fixes.This builds upon #421, but adds a new
TODOTXT_SED_COMMANDconfig variable to allow (Linux and Cygwin) users who have GNU sed to skip the less efficient emulation.It also adds a
TODOTXT_SED_EXPORT_FOR_ADDONSflag that exports thesed()wrapper for use of (compatible) add-ons.