remove-echo #612

ghost · 2024-09-10T19:51:30Z

This pull request replaces a number of occurrences of echo with printf/_omb_util_print/_omb_util_put as appropriate. Note that not all occurrences of echo were replaced. Some occurred in comments and some occurrences were not appropriate due to the context of use (e.g., the file was intended to be sourced in a shell other than bash).

Fixes #606

… as appropriate.

akinomyoga

In many places, echo -e and echo -ne are replaced by _omb_util_print and _omb_util_put, but these are not interchangeable. The latter two don't process the backslash sequences such as \033. I initially thought that you've carefully thought about the impact and judged that the behavior can be changed, but after finishing the review, I suspect that you might misunderstand the behavior of _omb_util_print and _omb_util_put. Could you check them again?

akinomyoga · 2024-09-10T21:24:36Z

.github/workflows/test.sh

@@ -15,8 +15,8 @@ source "$HOME/.bashrc"
 set | grep -aE "^OSH"

 if [[ "$OSH_THEME" == "font" ]]; then
-  echo "Installation succeeded"
+  printf -- "Installation succeeded"


Suggested change

printf -- "Installation succeeded"

printf '%s\n' "Installation succeeded"

Newline is missing

akinomyoga · 2024-09-10T21:24:53Z

.github/workflows/test.sh

 else
-  echo "Installation failed, \$OSH_THEME is not set to 'font'"
+  printf -- "Installation failed, \$OSH_THEME is not set to 'font'"


Suggested change

printf -- "Installation failed, \$OSH_THEME is not set to 'font'"

printf '%s\n' "Installation failed, \$OSH_THEME is not set to 'font'"

Newline is missing.

akinomyoga · 2024-09-10T21:26:33Z

aliases/general.aliases.sh

@@ -66,9 +66,9 @@ alias ll='ls -lAFh'                         # Preferred 'ls' implementation
 alias less='less -FSRXc'                    # Preferred 'less' implementation
 alias wget='wget -c'                        # Preferred 'wget' implementation (resume download)
 alias c='clear'                             # c:            Clear terminal display
-alias path='echo -e ${PATH//:/\\n}'         # path:         Echo all executable Paths
+alias path='printf -- ${PATH//:/\\n}'       # path:         Echo all executable Paths


This doesn't cover the case where PATH contains % and other \.

akinomyoga · 2024-09-10T21:28:17Z

completions/fallback/docker-compose.bash

The files under completions/fallback are taken from the upstream. These should be kept the same as the upstream as much as possible, so we wouldn't change them on our side.

akinomyoga · 2024-09-10T21:29:03Z

completions/fallback/docker.bash

This file is also taken from the upstream.

akinomyoga · 2024-09-10T21:31:59Z

lib/mo.sh

This file is also taken from the upstream.

akinomyoga · 2024-09-10T21:44:25Z

lib/omb-prompt-base.sh

@@ -154,7 +154,7 @@ function scm_prompt_info {

 function scm_prompt_char_info {
  scm_prompt_char
-  echo -ne "${SCM_THEME_CHAR_PREFIX}${SCM_CHAR}${SCM_THEME_CHAR_SUFFIX}"
+  _omb_util_put "${SCM_THEME_CHAR_PREFIX}${SCM_CHAR}${SCM_THEME_CHAR_SUFFIX}"


This changes the behavior of this function. Users may specify \033... in SCM_THEME_CHAR_PREFIX, etc.

However, this behavioral change could be harmless if all the outputs of this function are specified to PS1 as e.g. PS1="$(scm_prompt_char_info)". They become PS1='...\033...' after the command substitution and are finally processed within the PS1 evaluation. If any calls are performed by PS1='$(scm_prompt_char_info)', these \033, etc. will not be expanded and are finally included in the prompt directly. The command substitution will be expanded in the PS1 evaluation phase, but the result will not be further evaluated.

I haven't checked whether there are places affected by this behavioral change. There doesn't seem to be any mention of this behavioral change. There doesn't seem to be an explanation about whether you have paid attention to this possible breakage. Were you aware of this possible problem? Have you confirmed all the calls of this function are not affected by this behavioral change?

akinomyoga · 2024-09-10T21:45:45Z

lib/omb-prompt-base.sh

@@ -212,7 +212,7 @@ function git_prompt_minimal_info {
    # Output the git prompt
    SCM_PREFIX=${SCM_THEME_PROMPT_PREFIX}
    SCM_SUFFIX=${SCM_THEME_PROMPT_SUFFIX}
-    echo -e "${SCM_PREFIX}${SCM_BRANCH}${SCM_STATE}${SCM_SUFFIX}"
+    _omb_util_print "${SCM_PREFIX}${SCM_BRANCH}${SCM_STATE}${SCM_SUFFIX}"


All the similar changes have the same subtlety. One needs to check the impact on all the calls of these functions.

akinomyoga · 2024-09-10T21:46:23Z

themes/agnoster/agnoster.theme.sh

@@ -146,7 +146,7 @@ OMB_PROMPT_SHOW_PYTHON_VENV=${OMB_PROMPT_SHOW_PYTHON_VENV:=true}
 DEBUG=0
 function debug {
  if [[ ${DEBUG} -ne 0 ]]; then
-    >&2 echo -e "$@"
+    >&2 _omb_util_print "$@"


Suggested change

>&2 _omb_util_print "$@"

>&2 _omb_util_print "$@"

This also changes the behavior.

akinomyoga · 2024-09-10T21:48:07Z

themes/dulcie/dulcie.theme.sh

@@ -74,7 +74,7 @@ function _omb_theme_PROMPT_COMMAND {
  fi

  # Change terminal title
-  printf "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"
+  _omb_util_put "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"


Suggested change

_omb_util_put "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"

_omb_util_put "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"

This clearly breaks the intended behavior. This directly prints \033]0;...\007 to stdout with the backslash sequences unprocessed.

ghost · 2024-09-11T00:07:12Z

Thanks for your comments... When you said you would rush to merge your branch containing omb_util_print/omb_util_put, I assumed you did the lifting of making sure they were drop-in replacements for echo/echo -n. That's my bad. No excuses... I can certainly address the issues you raise...quite expediently...and I will do that. However, not being schooled in the ways of GitHub, I am unsure of the way forward from a logistical point of view. I would appreciate some advice... - Did you just reject the PR so that I can withdraw the PR, nuke my fork and start over with a new, sync'ed fork? - Your comments suggested that you accepted some changes but not others. Do I just sync my fork (and my local repos based on that fork) so that the changes you accepted are merged and move forward with what you suggested? Inquiring minds want to know... Thanks in advance. Message ID: ***@***.***>

…

akinomyoga · 2024-09-11T00:53:32Z

I assumed you did the lifting of making sure they were drop-in replacements for echo/echo -n. That's my bad.

They are the drop-in replacements for echo/echo -n as you correctly assumed, but they are not the drop-in replacements for echo -e/echo -ne. The -e flag to the echo builtin has an effect that _omb_util_print/_omb_util_put doesn't have.

No excuses... I can certainly address the issues you raise...quite expediently...and I will do that. However, not being schooled in the ways of GitHub, I am unsure of the way forward from a logistical point of view. I would appreciate some advice...

You are supposed to update the local branch master by creating new commits to fix the issue, and then you can simply push them to the forked repository ([email protected]:sw030453/oh-my-bash.git).

$ cd <your local clone>
$ <update the codes>
$ git add <modified files>
$ git commit
$ git push origin master

Then, this page (i.e. the GitHub interface for the pull request) will automatically reflect the updated branch.

If you want to cancel or amend the existing commits, you can perform git reset --hard or git rebase or whatever you want. Then, you can forcibly push the new commits to the branch (git push -f origin master in this case).

Did you just reject the PR so that I can withdraw the PR, nuke my fork and start over with a new, sync'ed fork?

No. What I did are the requests for changes, meaning that I'm waiting for you to update the branch. A pull request on GitHub tracks the change of the branch that was used to create the pull request.

Your comments suggested that you accepted some changes but not others.

No, nothing has been merged. Currently, it is still in the discussion stage. I will accept some of the changes as is, but it's still in my mind. It will not be reflected in the master branch of this main repository until this pull request is merged. After all the discussions are settled, the pull request will be merged at once.

Do I just sync my fork (and my local repos based on that fork) so that the changes you accepted are merged and move forward with what you suggested? Inquiring minds want to know...

No need to sync because there are no changes that were actually applied. Please just update your local branch master and push it again to origin master.

akinomyoga · 2024-09-11T02:51:44Z

What I'm thinking now is to prepare functions that can replace echo -e and echo -ne. Something like the following:

# lib/omb-util.sh

function _omb_util_put_unescape {
  local IFS=$' \t\n'
  local s="$*"
  s=${s/'%'/'%%'}
  printf "$s"
}

function _omb_util_print_unescape {
  local IFS=$' \t\n'
  _omb_util_put_unescape "$*"$'\n'
}

One can pass the string to as a format string to printf, but one needs to suppress printf's conversion of %....

Also, some of the cases just passes a fixed string such as '\033c'. These cases can be converted to $'\033c' so that the called command doesn't need to handle the conversion of the backslash escape sequences.

ghost · 2024-09-11T05:05:39Z

I was kinda thinking the same thing… I was looking at ‘printf “%q” …”. Maybe that’s too simplistic…

…

On Tue, Sep 10, 2024 at 7:52 PM Koichi Murase ***@***.***> wrote: What I'm thinking now is to prepare functions that can replace echo -e and echo -ne. Something like the following: # lib/omb-util.sh function _omb_util_put_unescape { local IFS=$' \t\n' local s="$*" s=${s/'%'/'%%'} printf "$s" } function _omb_util_print_unescape { local IFS=$' \t\n' _omb_util_put_unescape "$*"$'\n' } One can pass the string to as a format string to printf, but one needs to suppress printf's conversion of %.... Also, some of the cases just passes a fixed string such as '\033c'. These cases can be converted to $'\033c' so that the called command doesn't need to handle the conversion of the backslash escape sequences. — Reply to this email directly, view it on GitHub <#612 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BGRNYQDVJX4KST4K4BJ77IDZV6V5NAVCNFSM6AAAAABN7PDHG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSGUYDSMBZGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

akinomyoga · 2024-09-11T05:12:07Z

I was kinda thinking the same thing… I was looking at ‘printf “%q” …”. Maybe that’s too simplistic…

printf %q doesn't do what you want. It's the reverse. We expect echo -e '\033' to print the real ESC control character, but printf %q '\033' prints the literal string \\033.

Steve Witten added 3 commits September 9, 2024 10:59

Changed occurrences of 'echo' to printf/_omb_util_print/_omb_util_put…

a940c69

… as appropriate.

Changed 'echo' to printf/_omb_util_put/_omb_util_print as appropriate

13038f5

Backout '-S' option for /usr/bin/env on '#!' line

e2ca265

akinomyoga requested changes Sep 10, 2024

View reviewed changes

Repository owner closed this by deleting the head repository Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove-echo #612

remove-echo #612

ghost commented Sep 10, 2024 •

edited by akinomyoga

Loading

akinomyoga left a comment •

edited

Loading

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

akinomyoga Sep 10, 2024

ghost commented Sep 11, 2024 via email •

edited by ghost

Loading

akinomyoga commented Sep 11, 2024 •

edited

Loading

akinomyoga commented Sep 11, 2024

ghost commented Sep 11, 2024 via email

akinomyoga commented Sep 11, 2024

	printf -- "Installation succeeded"
	printf '%s\n' "Installation succeeded"

	printf -- "Installation failed, \$OSH_THEME is not set to 'font'"
	printf '%s\n' "Installation failed, \$OSH_THEME is not set to 'font'"

	_omb_util_put "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"
	_omb_util_put "\033]0;%s@%s:%s\007" "${USER}" "${HOSTNAME%%.*}" "${PWD/#$HOME/\~}"

remove-echo #612

remove-echo #612

Conversation

ghost commented Sep 10, 2024 • edited by akinomyoga Loading

akinomyoga left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 11, 2024 via email • edited by ghost Loading

akinomyoga commented Sep 11, 2024 • edited Loading

akinomyoga commented Sep 11, 2024

ghost commented Sep 11, 2024 via email

akinomyoga commented Sep 11, 2024

ghost commented Sep 10, 2024 •

edited by akinomyoga

Loading

akinomyoga left a comment •

edited

Loading

ghost commented Sep 11, 2024 via email •

edited by ghost

Loading

akinomyoga commented Sep 11, 2024 •

edited

Loading