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.

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

Housekeeping: Combined PR of all of my outstanding PRs. #405

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

Conversation

inkarkat
Copy link
Member

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:

  • Fork the repository and create your branch from master.
  • If you've added code that should be tested, add tests!
  • Ensure the test suite passes.
  • Format your code with ShellCheck.
  • Include a human-readable description of what the pull request is trying to accomplish.
  • Steps for the reviewer(s) on how they can manually QA the changes.
  • Have a fixes #XX reference to the issue that this pull request fixes.

inkarkat and others added 19 commits September 16, 2021 21:38
The action script creation in both test helper functions is pretty similar; extract a function for that so that the invocation is a single command.
Instead of potentially falling back to the built-in action that a custom action was intended to override, but (e.g. due to file system reorganizations) now results in a broken link. The extension functionality that is then skipped may result in undesired results, but this may not be immedately obvious to the user (if the extension is not particularly verbose), so some data corruption could occur if this remains undetected.
To avoid duplicating (or somehow extracting) all the built-in actions, simply detect _any_ broken symlink; i.e. offer a superset of the required functionality. So this would also complain about a broken symlink to a non-executable custom (auxiliary) file (rarely used) if that is mistakenly passed as a custom action (unlikely).

Fixes todotxt#359
There's no command-line option to reduce verbosity (just -v to increase it), so users who would like to remove the additional messages (cp. todotxt#364) have to configure this, but the variable is hard to find.
Include the default value in commented-out form and some documentation of the possible values.
And only coincidentally picked up as the global configuration (if CONFIG_DIR=/etc).

Fixes todotxt#377
I didn't know about printf's capability when I introduced quoting 10 years ago. The %q format will do the quoting, and "-v VAR" can be used to reassign to the variable.

Note: The shellquote() function has been exported for reuse by add-ons. I don't think anyone has ever used that (it was mostly intended for my own, extensive extensions, and I never used it), and is trivial to move away from, anyway.
This doesn't matter if (as currently recommended) the script is placed into a eagerly loaded location (like /etc/bash_completion.d/) - any name will do.
However, there's now lazy loading of completion scripts (in /usr/share/bash-completion/completions/), and that only works when the completion script is named exactly like the command the completion is for. As our command is todo.sh (ignoring aliases, which become more complex with lazy loading), the corresponding completion needs to be todo.sh (with the .sh extension) as well. Renaming does not do any harm for our recommended location, but makes it easier for users (and packagers who prepare a todo.sh package) that want to use lazy loading.

See todotxt#383 for the complete discussion.
So that any combination of priority / date entered in the replacement will replace the corresponding original ones, but if they are left out, the original ones will be kept.
In essence, omitted stuff will be kept, added stuff will override, only deletion of existing stuff is not possible (but this is replace, after all).

Fixes todotxt#386
If a custom action cannot be made non-executable, it needs to be removed as well (and the test skipped); otherwise its existence will break following tests that assume it's inactive.
We don't "shamelessly steal" code, we refactoring it ;-)
I've seen strange readline editing behavior when the editing doesn't start at the first column: I can actually backspace into the prepended message (with Del, Ctrl-W or Ctrl-U), and then the whole edit becomes messed up.

read can output a prompt on its own (hopefully in all versions of Bash that we aim to support - the tests will tell), and that doesn't have this problem, and it's also a bit cleaner and shorter.

The prompt is only displayed if input is coming from a terminal. For the tests (currently only deletion and move confirmations are covered), this means that the prompt itself cannot be covered, and an empty line instead has to be expected. (On the positive side, this removes the ugly trick with $SPACE.)
By convention, error output should be printed to standard error, not standard out. Same for the usage help that may accompany the error message.
To indicate that something went wrong (e.g. the task already was unprioritized).
Note: For actions that handle multiple ITEMs in a loop, we cannot use die() as that would abort processing of any following ITEM(s). Instead, use a status variable and set it to 1 on error, then exit at the end.
@chrysle
Copy link
Contributor

chrysle commented Jan 22, 2023

I'm all for a new release when this is merged!

@karbassi
Copy link
Member

@inkarkat Thank you so much for this. Much easier to review.

Do you know why the mac tests fail?

@inkarkat
Copy link
Member Author

@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: #369 (comment)

@chrysle
Copy link
Contributor

chrysle commented Jun 27, 2023

@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.

source

@inkarkat
Copy link
Member Author

@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.

@chrysle
Copy link
Contributor

chrysle commented Jun 28, 2023

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).

@inkarkat
Copy link
Member Author

@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 "$@"
}

@chrysle
Copy link
Contributor

chrysle commented Jun 29, 2023

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.

@inkarkat
Copy link
Member Author

inkarkat commented Jul 3, 2023

@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!

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.

None yet

4 participants