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

use alias for command #157678

Closed
wants to merge 5 commits into from
Closed

use alias for command #157678

wants to merge 5 commits into from

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Aug 9, 2022

fix #156385

@meganrogge meganrogge requested a review from Tyriar August 9, 2022 17:18
@meganrogge meganrogge self-assigned this Aug 9, 2022
@meganrogge meganrogge added this to the August 2022 milestone Aug 9, 2022
@meganrogge
Copy link
Contributor Author

meganrogge commented Aug 9, 2022

According to my research, BASH_COMMAND is replaced with its definition when it gets executed. This is what i've come up with as a workaround

one issue w this approach is if someone runs the full path, it'll be changed to the alias if re-run. I don't think that's a big problem though

EDIT:
we will need to somehow keep the arguments as well if we use this approach so things aren't broken like echo "hi"

@meganrogge meganrogge requested a review from Tyriar August 9, 2022 19:49
This reverts commit 452f6d7.
This reverts commit 7df0c89.
@meganrogge meganrogge changed the title save and use raw command for rerun action and run recent command label use alias for command Aug 9, 2022
@meganrogge meganrogge marked this pull request as draft August 9, 2022 20:52
@meganrogge
Copy link
Contributor Author

new idea: save the aliases in an array and cycle through them - I don't like this idea though because ppl could have many and that could be slow

Comment on lines +109 to +110
__vsc_command= "$(which $BASH_COMMAND)"
__vsc_command= basename __vsc_command
Copy link
Member

Choose a reason for hiding this comment

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

BASH_COMMAND should have the full command line so we wouldn't be able to run basename on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah currently working on a different idea
"The first word of each simple command, if unquoted, is checked to see if it has an alias. If so, that word is replaced by the text of the alias. " so maybe we can quote it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already tried unsetting the expand alias option at the top of the file and setting it at the bottom, but that didn't work - maybe permissions? does work when I manually unset that in the terminal though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but also "Aliases are expanded when a command is read, not when it is executed." so idk if that's even possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is doable atm without storing all aliases and checking each (slow) since the moment the command is read, it's expanded

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2022

According to my research, BASH_COMMAND is replaced with its definition when it gets executed. This is what i've come up with as a workaround

one issue w this approach is if someone runs the full path, it'll be changed to the alias if re-run. I don't think that's a big problem though

We might be confusing absolute path with alias? The main place the issue shows up for me is because I have git pointing at hub:

Screen Shot 2022-08-09 at 2 57 38 pm

So if you run:

alias myalias=echo
myalias "hello world"

You end up seeing this:

Screen Shot 2022-08-09 at 2 58 39 pm

@meganrogge
Copy link
Contributor Author

@Tyriar this is how i'm reproducing it #156385 (comment)

is that correct?

@Tyriar
Copy link
Member

Tyriar commented Aug 9, 2022

I got kind of close by turning off alias resolution, resolving it myself but that fell apart when trying it actually run it and get $? working correctly. It also required not unsetting setting shopt -u expand_aliases but also setting shopt -s extdebug which probably has performance problems and other unforeseen consequences. This was my prototype:

__vsc_preexec() {
	# echo "  __vsc_preexec BASH_COMMAND=$BASH_COMMAND, $BASH_ARGC, ${BASH_CMDS[@]}"
	# echo "  inside"

	__vsc_initialized=1
	if [[ ! "$BASH_COMMAND" =~ ^__vsc_prompt* ]]; then
		__vsc_current_command=$BASH_COMMAND
	else
		__vsc_current_command=""
	fi

	# The unresolved alias is stored, resolve the alias ourselves since expand_aliases is disabled
	#local resolvedAlias=$(alias $BASH_COMMAND 2> /dev/null)
	local lhs=$(echo $BASH_COMMAND | cut -d ' ' -f1)
	local rhs=$(echo $BASH_COMMAND | sed 's/.* //')
	echo "lhs=$lhs"
	local resolvedAlias=${BASH_ALIASES[$lhs]}
	if [ "$resolvedAlias" = "" ]; then
		resolvedAlias=$lhs
	fi
	echo "resolved: $resolvedAlias $rhs"
	__vsc_resolved_command="$resolvedAlias $rhs"
	__vsc_command_output_start
}

shopt -u expand_aliases
shopt -s extdebug
		__vsc_preexec_all() {
			__vsc_resolved_command="$BASH_COMMAND"
			if [ "$__vsc_in_command_execution" = "0" ]; then
				__vsc_in_command_execution="1"
				builtin eval ${__vsc_dbg_trap}
				__vsc_preexec
			fi

			eval "$__vsc_resolved_command"
			# exitCode=$(eval "$__vsc_resolved_command")
			# (exit ${exitCode})
			return 1 # cancels the original command
		}

Because of all the downsides in attempting to do something clever like that, I suggest we step back and try solve the problem in a less perfect but much simpler way, such as:

  1. Reading the buffer to check for the alias
  2. Adding both the aliased and non-aliased version whenever a command is run

I think 1 should be easy enough and end up with pretty good results, especially considering we can typically rely on bash being non-Windows and that it's not the end of the world if it fails anyway. If we do this, let's only do it when IsWindows is not set.

One idea that is probably overkill to prevent extra work and only do this check on aliases is to run bash -ic "alias" or bash -ilc "alias" once per session in a separate shell when bash shell integration is activated. Let's keep that in our back pocket for now.

@meganrogge meganrogge closed this Aug 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2022
@meganrogge meganrogge deleted the merogge/alias branch December 7, 2022 02:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run recent command shows resolved aliases - ls appends --color=auto to executed command
2 participants