Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion

[Adam Tkac <atkac@redhat.com>]

On Thu, Nov 29, 2012 at 09:33:53AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Adam Tkac <atkac@redhat.com> writes:
> >
> >> Subject: Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion
> >
> > The code does not seem to do anything special if it is not aliased,
> > though, so "If ..." part does not sound correct; perhaps you meant
> > "just in case egrep is aliased to something totally wacky" or
> > something?
> >
> > The script seems to use commands other than 'egrep' that too can be
> > aliased to do whatever unexpected things.  How does this patch get
> > away without backslashing them all, like
> >
> > 	\echo ...
> >         \sed ...
> >         \test ...
> >         \: comment ...
> > 	\git args ...
> >
> > and still fix problems for users?  Can't the same solution you would
> > give to users who alias one of the above to do something undesirable
> > be applied to those who alias egrep?
> >
> > Puzzled...
> 
> Sorry for having been more snarky than necessary (blame it to lack
> of caffeine).  What I was trying to get at were:
> 
>  * I have this suspicion that this patch exists only because you saw
>    somebody who aliases egrep to something unexpected by the use of
>    it in this script, and egrep *happened* to be the only such
>    "unreasonable" alias.  The reporter may not have aliased echo or
>    sed away, or the aliases to these command *happened* to produce
>    "acceptable" output (even though it might have been slightly
>    different from unaliased one, the difference *happened* not to
>    matter for the purpose of this script).
> 
>  * To the person who observes the same aliasing breakage due to his
>    aliasing sed to something else, you would solve his problem by
>    telling him "don't do that, then".  If that is the solution, why
>    wouldn't it work for egrep?
> 
>  * The next person who aliased other commands this script uses in
>    such a way that the behaviour of the alias differs sufficiently
>    from the unaliased version, you will have to patch the file
>    again, with the same backslashing.  This patch is not a solution,
>    but a band-aid that only works for a particular case you
>    *happened* to have seen.
> 
>  * A complete solution that follows the direction this patch
>    suggests would involve backslashing *all* commands that can
>    potentially aliased away.  Is that really the direction we would
>    want to go in (answer: I doubt it)?  Is that the only approach to
>    solve this aliasing issue (answer: I don't know, but we should
>    try to pursue it before applying a band-aid that is not a
>    solution)?
> 
> Is there a way to tell bash "do not alias-expand from here up to
> there"?  Perhaps "shopt -u expand_aliases" upon entry and restore
> its original value when we exit, or something?
> 
> IOW, something along this line?

This won't work, unfortunately, because shopt settings aren't inherited by
subshell (and for example egrep is called in subshell).

I discussed this issue with colleagues and we found basically two "fixes":

1. Tell people "do not use aliases which breaks completion script"
2. Prefix all commands with "command", i.e. `command egrep` etc.

In my opinion "2." is better long time solution, what do you think?

Regards, Adam

> 
>  contrib/completion/git-completion.bash | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
> index 0b77eb1..193f53c 100644
> --- i/contrib/completion/git-completion.bash
> +++ w/contrib/completion/git-completion.bash
> @@ -23,6 +23,14 @@
>  #    3) Consider changing your PS1 to also show the current branch,
>  #       see git-prompt.sh for details.
>  
> +if shopt -q expand_aliases
> +then
> +	_git__aliases_were_enabled=yes
> +else
> +	_git__aliases_were_enabled=
> +fi
> +shopt -u expand_aliases
> +
>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
>  *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
> @@ -2504,3 +2512,8 @@ __git_complete gitk __gitk_main
>  if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
>  __git_complete git.exe __git_main
>  fi
> +
> +if test -n "$_git__aliases_were_enabled"
> +then
> +	shopt -s expand_aliases
> +fi
> 
> 

-- 
Adam Tkac, Red Hat, Inc.