editing existing aliases leads to unexpected results

[rrotter]

homebrew-aliases seems to support editing alias files (there is the brew alias --edit command). But, some edits that result in otherwise working scripts can confuse the alias command leading to nonsensical output, useless files written to disk, and with the addition of the user deleting the symlink at $HOMEBREW_PREFIX/bin/brew-*, causes brew alias to overwrite user created alias scripts with corrupted data.

The following is shell output demonstrating all of the issues mentioned above. For the sake of brevity I didn't include the alias scripts, but they should be reproducible bit-for-bit using the brew alias and sed commands below.

I'm not saying that these exact scripts are useful, they aren't, but rather these are the minimal steps to reproduce issues I had with actual scripts I wrote.

~ $ # get a clean working state
~ $ rm -rf .brew-aliases/
~ $ ls /opt/homebrew/bin/brew-*
ls: /opt/homebrew/bin/brew-*: No such file or directory
~ $ # create aliases
~ $ brew alias hello='!echo hello world'
~ $ brew alias hello2='!echo hello again'
~ $ # edit aliases
# this edits the `#  alias:` comment at the top
~ $ sed -i '' 's/alias:.*$/better not edit this/' ~/.brew-aliases/hello
# this wraps the `echo` line in an `if` statement
~ $ sed -i '' 's/\(^echo.*$\)/if true; then\n  \1\nfi/' ~/.brew-aliases/hello2
~ $ # get checksums for reference
~ $ md5 .brew-aliases/*
MD5 (.brew-aliases/hello) = 5cf77855dae72c39996d8519b562cd39
MD5 (.brew-aliases/hello2) = afc0805389b434f0526959e94937370e
~ $ brew alias
brew alias # better not edit this='!echo hello world'
brew alias hello2='!if true; then'
~ $ # that output is odd, but the md5 is okay
~ $ # but there is another file?
~ $ md5 .brew-aliases/*
MD5 (.brew-aliases/__better_not_edit_this) = 17194e87812aac6f4e5be6cb8ec95d6c
MD5 (.brew-aliases/hello) = 5cf77855dae72c39996d8519b562cd39
MD5 (.brew-aliases/hello2) = afc0805389b434f0526959e94937370e
~ $ ls -1 /opt/homebrew/bin/brew-*
/opt/homebrew/bin/brew-# better not edit this
/opt/homebrew/bin/brew-hello
/opt/homebrew/bin/brew-hello2
~ $ # remove the odd file and all the symlinks
~ $ rm /opt/homebrew/bin/brew-*; rm ~/.brew-aliases/__better_not_edit_this
~ $ # run alias command again to regen symlinks
~ $ brew alias
brew alias # better not edit this='!echo hello world'
brew alias hello2='!if true; then'
~ $ md5 .brew-aliases/*
MD5 (.brew-aliases/__better_not_edit_this) = 17194e87812aac6f4e5be6cb8ec95d6c
MD5 (.brew-aliases/hello) = 5cf77855dae72c39996d8519b562cd39
MD5 (.brew-aliases/hello2) = 05e54836d8513b4d67ddc757b6734504
~ $ # ^ recreated the bad file, and now hello2 is corrupted

It's pretty reasonable if these types of edits aren't supported, but brew alias should fail a little more gracefully if it can't parse a script. I found all of this by accident, and in one case was lucky that my dotfiles are in a git repo.

[rrotter]

I'd propose the following:

1. Parsing of files is fine for brew alias guessing at what the "alias" does and printing that out, but the symlink creation should depend only on the filename and ignore the script parse.
2. brew alias foo=whatever and brew alias --edit foo should write/edit the scripts. brew alias needs to write to the bin dir to fix missing links but should not be writing anything to ~/.brew-aliases/.

[MikeMcQuaid]

@rrotter Yes, this sounds reasonable. Could you open a PR? What, if anything, would this change for users using brew alias in terms of behaviour/flow changes? Feel free to put this in the PR instead of here.

[rrotter]

The only possibly desirable behavior this would change is that the description line (last comment line in the script) would not be "corrected" by brew alias when the symlink is recreated (this only happens if you rm /opt/homebrew/bin/brew-myalias). That's arguably a bug anyway, and it's hard to imagine anyone was depending upon this.

[rrotter]

I will submit a PR for this. One question since you wrote it, @MikeMcQuaid. This bug seems to be caused by the misuse of existing_alias.write override: true here https://github.com/Homebrew/homebrew-aliases/blob/70e51cc954558b91cebb6f1cf925af3e5a744e98/lib/aliases.rb#L62 and I can't figure out why :override or the opts hash even exist here: https://github.com/Homebrew/homebrew-aliases/blob/70e51cc954558b91cebb6f1cf925af3e5a744e98/lib/alias.rb#L43-L47

Was there previously a brew alias --override flag that was perhaps broken in the move to AbstractCommand? I checked out fcf9dfc1, but brew has changed enough that it's no longer possible to run such an old version of homebrew-aliases.

[MikeMcQuaid]

Was there previously a brew alias --override flag that was perhaps broken in the move to AbstractCommand?

Yes, potentially. You could check the git log -p output in the repository.

I don't use homebrew-aliases so honestly I defer to you here.