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

Remove unused variable and fix command prompt display issue #168

Closed
wants to merge 2 commits into from

Conversation

millaker
Copy link

@millaker millaker commented Mar 6, 2024

Commit ("Remove unused variable") removes an unused variable in console.c. prompt_flag in console.c is specified as static and the only usage is the if statement at line 583 and the only assignment to it is at line 588 which assigns 1 to prompt_flag. Since no other code can modify this variable, it can be removed.

Commit ("Fix command prompt showing when using file input") fixes command prompt display issue when using file as input. Commit b13335b ("Introduce eventmux callback function for linenoise") removes set_echo(0) when infd is not the standard input, which will show command prompt while executing command from input files. Adding it back fixes the issue.

millaker added 2 commits March 7, 2024 01:19
`prompt_flag` in `console.c` is specified as static and the only usage
is the if statement at line 583 and the only assignment to it is at line
588 which assigns 1 to `prompt_flag`. Since no other code can modify
this variable, it can be removed.
Commit dd6a761 removes `set_echo(0)` when `infd` is not the standard
input, which will show command prompt while executing command from input
files. Adding it back fixes the issue.
@visitorckw
Copy link
Contributor

When mentioning a commit, besides referencing the commit SHA-1 ID, including the commit's subject title will make the description clearer, such as b13335b ("Introduce eventmux callback function for linenoise").

@jserv
Copy link
Contributor

jserv commented Mar 7, 2024

Refine the subject of this issue, making it informative.

@millaker millaker changed the title Minor fixes to console.c Remove unused variable and fix command prompt display issue Mar 7, 2024
@visitorckw
Copy link
Contributor

When describing changes, it is not advisable to mention commit IDs that have not yet been merged into the upstream, as they are highly likely to be altered, rendering the description invalid. However, when referring to a commit already merged into the upstream, it should be mentioned using its SHA-1 ID along with a one-line summary of the commit.

@jserv
Copy link
Contributor

jserv commented Mar 21, 2024

Split the proposed changes into two pull requests.

@millaker millaker closed this Mar 25, 2024
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.

3 participants