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

Use readline for reading git commit messages #8

Merged
merged 3 commits into from Aug 16, 2014

Conversation

Projects
None yet
2 participants
@nilbus
Contributor

nilbus commented Aug 15, 2014

This way, you can use the arrow keys, etc. to edit input, and they work, rather than outputting control sequences.

I believe this is the only instance of reading user input where readline is useful.

Use readline for reading git commit messages
This way, you can use the arrow keys, etc. to edit input, and they work,
rather than outputting control sequences.
@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Aug 15, 2014

Contributor

It seems this breaks the build. Investigating.

Contributor

nilbus commented Aug 15, 2014

It seems this breaks the build. Investigating.

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Aug 15, 2014

Contributor

That gets close, and I like how you use vared for zsh. The tricky thing though is that read -e in bash acts as if input starts at the beginning of the line, which is why read needs to provide the prompt: read -e -p 'Git comment: '. zsh's vared does the same thing but actually clears the line, so the echo statement actually has no effect.

I will put something together along the lines of what you suggested that uses the read/vared prompt argument.

Contributor

nilbus commented Aug 15, 2014

That gets close, and I like how you use vared for zsh. The tricky thing though is that read -e in bash acts as if input starts at the beginning of the line, which is why read needs to provide the prompt: read -e -p 'Git comment: '. zsh's vared does the same thing but actually clears the line, so the echo statement actually has no effect.

I will put something together along the lines of what you suggested that uses the read/vared prompt argument.

@erichs

This comment has been minimized.

Show comment
Hide comment
@erichs

erichs Aug 15, 2014

Owner

excellent, thanks!

Owner

erichs commented Aug 15, 2014

excellent, thanks!

Only attempt using readline for input in bash shells
ksh does not support readline with read.
zsh supports readline with varedit, but its input cannot be fed with a
pipe, which causes the test suite to hang, waiting on the tty's stdin.
Show outdated Hide outdated composure.sh
@@ -71,6 +71,17 @@ _temp_filename_for ()
echo $file
}
_prompt_cmd ()
{
typeset prompt="$1"

This comment has been minimized.

@nilbus

nilbus Aug 15, 2014

Contributor

On line 115 below (originally 105),

typeset comment="${4:-}"

I noticed you provide an empty default here. Is this not equivalent?

typeset comment="$4"

If there is some nuance I am unaware of, I should also change this line to match that style.

@nilbus

nilbus Aug 15, 2014

Contributor

On line 115 below (originally 105),

typeset comment="${4:-}"

I noticed you provide an empty default here. Is this not equivalent?

typeset comment="$4"

If there is some nuance I am unaware of, I should also change this line to match that style.

This comment has been minimized.

@erichs

erichs Aug 15, 2014

Owner

usually, the empty defaults are for explicit non-zero checks later in the code. I don't see that here, so it is either old code, or I changed my mind about the calling semantics and didn't change the variable declaration.

should be safe just to say "$1"

@erichs

erichs Aug 15, 2014

Owner

usually, the empty defaults are for explicit non-zero checks later in the code. I don't see that here, so it is either old code, or I changed my mind about the calling semantics and didn't change the variable declaration.

should be safe just to say "$1"

@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Aug 15, 2014

Contributor

The test suite hangs here in the zsh test run, which implies varedit does not accept input from stdin via the pipe. The manpage (man zshzle) does not suggest a way to allow this with varedit. Can you think of a different workaround to allow interactive editing for zsh? For now, I am letting zsh fall back to the previous read behavior.

Also, I could not get any combination of quoting and escaping that would pass 'Git commit: ' as a single argument without using eval as I did. Let me know if you are able to make this work a better way.

Contributor

nilbus commented Aug 15, 2014

The test suite hangs here in the zsh test run, which implies varedit does not accept input from stdin via the pipe. The manpage (man zshzle) does not suggest a way to allow this with varedit. Can you think of a different workaround to allow interactive editing for zsh? For now, I am letting zsh fall back to the previous read behavior.

Also, I could not get any combination of quoting and escaping that would pass 'Git commit: ' as a single argument without using eval as I did. Let me know if you are able to make this work a better way.

@erichs

This comment has been minimized.

Show comment
Hide comment
@erichs

erichs Aug 15, 2014

Owner

sure, let me take a look.

Owner

erichs commented Aug 15, 2014

sure, let me take a look.

@erichs

This comment has been minimized.

Show comment
Hide comment
@erichs

erichs Aug 15, 2014

Owner

Hmm.. looks like there are local ksh test suite failures that are unrelated to this change. I'm going to fix those and commit to master.

perhaps, instead of a _prompt_cmd function, there should be a function whose goal is to do the appropriate thing and return a comment string. Maybe something like:

_add_composure_file(){
...

if [ -z "$comment" ]; then
  comment="$(_prompt_for_git_comment)"
fi

git commit -m "... $comment"
}
Owner

erichs commented Aug 15, 2014

Hmm.. looks like there are local ksh test suite failures that are unrelated to this change. I'm going to fix those and commit to master.

perhaps, instead of a _prompt_cmd function, there should be a function whose goal is to do the appropriate thing and return a comment string. Maybe something like:

_add_composure_file(){
...

if [ -z "$comment" ]; then
  comment="$(_prompt_for_git_comment)"
fi

git commit -m "... $comment"
}
@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Aug 15, 2014

Contributor

I like that. I'll make the change.

Contributor

nilbus commented Aug 15, 2014

I like that. I'll make the change.

Refactor prompt_cmd() into prompt() to avoid eval
The prompt must be displayed on stderr, or else it will become part of the result,
since stdout is used to pass the result back to the caller of _prompt().
@nilbus

This comment has been minimized.

Show comment
Hide comment
@nilbus

nilbus Aug 16, 2014

Contributor

How does this look?

Also, what ksh failures were you referring to?

Contributor

nilbus commented Aug 16, 2014

How does this look?

Also, what ksh failures were you referring to?

@erichs

This comment has been minimized.

Show comment
Hide comment
@erichs

erichs Aug 16, 2014

Owner

Looks good.

Ksh: I was seeing some transient test errors in ksh with the second _add_composure_file test. Not sure what was going on, and I haven't been able to reproduce the problem in a couple of days.

Onward and upward!

Owner

erichs commented Aug 16, 2014

Looks good.

Ksh: I was seeing some transient test errors in ksh with the second _add_composure_file test. Not sure what was going on, and I haven't been able to reproduce the problem in a couple of days.

Onward and upward!

erichs added a commit that referenced this pull request Aug 16, 2014

Merge pull request #8 from nilbus/readline
Use readline for reading git commit messages

@erichs erichs merged commit 5c8e9af into erichs:master Aug 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment