Handling control sequences in commands

General
1

For context: #1576

I’m doing what I should have done before starting and using the forum to discuss a change before doing it but on the basis of better late than never, here we are. Before I go any further, was this a problem that needed fixing? If so and if I had asked here before opening the first PR, would there have been a better approach that I should have taken?

My original problem

At some point a couple of years ago I was messing around with CSI/SGR sequences and, amongst fg/bg/underline changes, I must have printed a ^[[8m sequence which hides text. When I was trying out atuin again recently, I ran atuin history list and the entire terminal scrollback buffer turned purple with no text visible.

The first PR

#1576 made it so that all control characters were replaced with their hex code equivalent, eg a literal^[ would be replaced with \x1b. This solved my problem but as was pointed out in the comments of the PR, it didn’t work in every case, for instance I’d only really tested in zsh and bash doesn’t handle hex codes the same way.

It also broke multi line commands by converting line breaks into \x0a sequences.

Fixing my fix

As far as I can see the things wrong with my patch are

  • Control characters should be replaced with caret notation instead of hex codes. This was brought up by @akinomyoga on the PR and makes sense. A change to do that is in #1586.
  • Multiline commands stopped working as the newline was replaced with \x0a which doesn’t work. I had been focussing too much on the ‘echo SGR sequence’ case that caused the original problem I didn’t think through the side effects well enough. There is a (potential) fix in #1585
  • I missed the command preview in the interactive tui. I have this on a branch but stopped opening PRs at this point to come here and get feedback first
  • Running atuin history list > history_file.txt now has the escaped characters rather than the literal control characters. This seems wrong and maybe the history printing should have the same istty logic as search (in #1586). Due to the way that’s handled though it might need a bigger refactoring though so, again, keen to hear ideas before I change anything.
1 Like
2

Hey! Welcome to the forum :pray:

Thank you for raising the issue! It does need fixing, but I wasn’t aware of the issue until you raised it. I’m certain people must have come across it before and just not raised it though.

It would be good to get the preview sorted here too!

For sure. I think this would be expected.

Thanks for jumping on this, please don’t hestitate to open any more PRs around the issue. I do very much appreciate you reaching out to discuss it though!

One consideration to bear in mind with any possible refactoring or restructuring; syntax highlighting. There are two kinds that we probably want Atuin to have in the near future

  1. Bash syntax highlighting - issue
  2. Search result highlighting - issue

Only mentioning in case they influence any decisions you may make

3

Right, I’ve had a look at this and it’s a bit awkward because the escaping needs to be done in the fmt method where you don’t have access to whether you’re writing to a terminal or not. One approach which I’m not sure about in terms of rust conventions, is using the alternate formatting flag which can then be accessed on the formatter. You could then have something like

if out.is_terminal() {
    write!(out, "{h:#}");
} else {
    write!(out, "{h}");
}

and the formatting would escape the control characters as required. Is this a done thing in rust? The docs are a bit vague and only really talk about it in terms of the auto derived Debug impls

# - This flag indicates that the “alternate” form of printing should be used.

Sounds like it could be classed as ‘not a hack’ if you squint a bit from a distance.

4

Or now I’ve written it out it might be better to convert FmtHistory to be an enum?

enum FmtHistory {
    Literal(History),
    Escaped(History),
}

or maybe

struct FmtHistory(History, FmtKind);

enum FmtKind {
    Literal,
    Escaped,
}

which would then be used via

let fh = if out.is_terminal() {
    FmtHistory::Escaped(h)
} else {
    FmtHistory::Literal(h)
};
// And the same write logic it uses at the moment

It might be possible for one of these to then be expanded to include a Highlighted(Theme) variant for the syntax highlighting issues you mentioned although they might only have been targeting the interactive search.

5

I like the Enum, it’s very clear what’s going on. Also nice because when we do highlighting, we won’t want that in the literal output either