Open inkarkat opened 1 year ago
I'm all for a new release when this is merged!
@inkarkat Thank you so much for this. Much easier to review.
Do you know why the mac
tests fail?
@karbassi The Mac tests have been failing for a very long time. I'm not sure I remember this correctly, but there was an update to a newer MacOS version, and since then that newline handling (that's what add to file without EOL is about) was broken. Ah, it was discovered here: https://github.com/todotxt/todo.txt-cli/pull/369#issuecomment-973100872
@inkarkat Trying to fix the tests. I'm unsure wether I'm on the right track though, could this be related?
The cause for this is how the line break is actually created. The Mac, by default, uses a single carriage return (
<CR>
), represented as \r. Unix, on the other hand, uses a single linefeed (<LF>
), \n. Windows goes one step further and uses both, creating a (<CRLF>
) combination, \r\n.
@chrysle Yes, this is related to the CR vs. LF problem. The test failure (e.g. here) shows that the missing newline isn't added:
todo.sh add "a second task"
--- expect 2023-04-04 15:19:01.000000000 +0000
+++ output 2023-04-04 15:19:01.000000000 +0000
@@ -1,2 +1,2 @@
-2 a second task
-TODO: 2 added.
+1 a second task
+TODO: 1 added.
Instead, the second task is appended to the first one:
-2 a second task
-1 this is a first task without newline
+1 this is a first task without newlinea second task
In the code, the fixMissingEndOfLine()
function should add that missing newline, via this sed call: sed -i.bak -e '$a\' "${1:-$TODO_FILE}"
. Apparently, that only works for MacOS 10.15, but not in 11 any longer. We need to find out why, and then either come up with a modified command that works on all platforms or maybe updated dependency requirements (using GNU sed instead of the outdated crap that Apple installs might help), or worse need a special case for MacOS. I don't have any Apple hardware at hand, unfortunately.
Thank you for the reply!
Apparently, that only works for MacOS 10.15, but not in 11 any longer. We need to find out why,
It looks like the MacOS FreeBSD sed
since MacOS 11 just doesn't automatically add newlines to matches, if not present. See this comment for example. The proposed solution in the underlying post doesn't work on MacOS 11, I've tested that and other workarounds.
and then either come up with a modified command that works on all platforms or maybe updated dependency requirements (using GNU sed instead of the outdated crap that Apple installs might help), or worse need a special case for MacOS.
Since I'm too frustrated with BSD sed
by now, any suggestions on how to declare gnu-sed
(via Homebrew) a dependency? I don't think from the Makefile, nor the script itself?
We'd also need to tweak the PATH
to run gsed
as sed
(see this post).
@chrysle
I've tested that and other workarounds.
Have you tried [ -n "$(tail -c1 file)" ] && echo >> file
; that's apparently POSIX-compatible.
Since I'm too frustrated with BSD sed by now, any suggestions on how to declare gnu-sed (via Homebrew) a dependency? I don't think from the Makefile, nor the script itself?
I meant documenting (in the Readme) that the script requires GNU sed, and that / how it can be installed via Homebrew. Ideally only users who care about the missing newline would need to install GNU sed, and the rest could just use the script as-is.
That doesn't help with the failing GitHub action, though. But if we go with an optional dependency, I would rather keep the MacOS 11 image with its BSD sed, and just skip that failing test there.
We'd also need to tweak the PATH to run gsed as sed (see this post).
Or alternatively define a wrapper function:
sed()
{
command gsed "$@"
}
Have you tried
[ -n "$(tail -c1 file)" ] && echo >> file
; that's apparently POSIX-compatible.
I have, also with a similar command. But there is a problem.
@karbassi The tests on MacOS got fixed, and @chrysle is keen on taking this further by adding updated platforms (in #416). Please approve this PR as soon as possible to get the project going again; it has been languishing for far too long!
This is a combined merge of the PRs that I've submitted over the last couple of months; I've been using it all the time myself. @karbassi if you take this, you'll save us the hassle of figuring out any merge conflicts, and it's less work than going over each of my PRs individually.
Closes #360 Robustness: Check for broken symlinks to custom actions and complain Closes #365 Add TODOTXT_VERBOSE to the configuration Closes #379 Readme: Clarify CONFIG_DIR and recommend copy of config template Closes #380 Refactoring: Replace shellquote() with printf %q Closes #384 Renaming: Add .sh extension to completion script Closes #387 replace: Completely merge given priority / date with existing Closes #388 TESTSFIX: t8010-listaddons breaks on Cygwin Closes #389 Refactoring: Use read -p MSG instead of doing echo -n MSG separately Closes #404 FIX: Use standard error for die(), dieWithHelp(), and error messages
Before submitting a pull request, please make sure the following is done:
master
.fixes #XX
reference to the issue that this pull request fixes.