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

WIP: Add newline support #162

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

shelvacu
Copy link

@shelvacu shelvacu commented Jun 20, 2021

So I've been working on #50 and it sort of feels like things are sprawling out more and more, and I wanted to both contribute what I'd done so far and get feedback before making more changes.

As it stands right now, this PR does correctly record commands-with-newline(s) into the database.

The first issue I ran into was that the search interface would not show newlines correctly. I believe I've fixed it, but I haven't tested it rigourously.

While developing, I noticed that the search was running the wrong version of mcfly (installed version in .cargo/bin rather than the development version in target) despite using dev.bash, because the history addendum uses $MCFLY_PATH whereas the bind command just calls mcfly. I was unable to wrassle the multi-level quoting into working correctly in the bind command with a variable substitution, so I instead removed $MCFLY_PATH and relied on mcfly calling the right binary. This may not be desireable.

In changing the way the command is read in from history, I cut out some of the code that wrote to a file, filtered, and wrote back out history to then be re-read by history -cr. I think for bash the best solution would involve no itermediary files, just the database and history calls.

There seems to be some logic for removing history entries starting with #mcfly, but I don't see that in my history anyway so I don't understand why it's there. As it is in this PR, that functionality is never used I think.

Also worth noting that currently this is for bash only. I haven't looked into zsh or fish at all.

Let me know your thoughts.

@shelvacu
Copy link
Author

@cantino I'd like your thoughts on this before I go further, not sure if draft pull requests make a notification or not.

I now better understand the #mcfly: functionality; In bash >= 4 it is possible to get a full multiline search from $READLINE_LINE, but for earlier bash I assume it will just mess up a multi-command when you press C-r

@cantino
Copy link
Owner

cantino commented Oct 30, 2021

Sorry for the delay @shelvacu! You're correct about #mcfly: and bash versions <4.

I think it's entirely possible that the current logic to get history into both the shell history and the mcfly database is crufty / unnecessarily complex. It's evolved over time as I worked on this and I've never really revisited it.

@@ -21,8 +21,7 @@ MCFLY_SESSION_ID="$(command dd if=/dev/urandom bs=256 count=1 2> /dev/null | LC_
export MCFLY_SESSION_ID

# Find the binary
MCFLY_PATH=${MCFLY_PATH:-$(which mcfly)}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was definitely a reason for $MCFLY_PATH to exist, but I can't recall anymore what it was.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leave it out then? I can't find anything that reads it, just things setting it in the dev.* scripts.

# Run mcfly with the last history entry on stdin. It will:
# * Append the command to $HISTFILE, (~/.bash_history by default)
# * Parse out the command and and save it to the database.
HISTTIMEFORMAT="%s:" history 1 | mcfly add --exit $exit_code --command-from-stdin
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still work on bash <4?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the bash release notes, HISTTIMEFORMAT was added in Bash 3, released in 2004. I did a quick test to be sure, it behaves the same as bash 4. Should be good.

src/interface.rs Outdated Show resolved Hide resolved
@cannonpalms
Copy link

@shelvacu What's the status of this PR now that @cantino has left a few comments? From your description I can't tell if there is more to be done. You mention that as-is, the PR does correctly handle multi-line commands, but also state that you were sharing the PR as a draft before going further.

What is left? Perhaps I can help? This bug is a particular nuisance to my workflow as I tend to lean heavily on multi-line commands.

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