{"thread":{"id":"32242","subject":"[PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","startedAt":"2012-11-29T15:14:22Z","lastAt":"2012-12-12T22:34:29Z","messageCount":6,"participants":["Adam Tkac","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"204256","messageId":"20121129151418.GA19169@redhat.com","threadId":"32242","inReplyTo":null,"subject":"[PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Adam Tkac","fromEmail":"atkac@redhat.com","sentAt":"2012-11-29T15:14:22Z","receivedAt":"2012-11-29T15:14:22Z","isPatch":true,"sender":{"key":"atkac@redhat.com","avatar":null},"body":"Originally reported as https://bugzilla.redhat.com/show_bug.cgi?id=863780\n\nSigned-off-by: Adam Tkac \nSigned-off-by: Holger Arnold \n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 0960acc..79073c2 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -565,7 +565,7 @@ __git_complete_strategy ()\n __git_list_all_commands ()\n {\n \tlocal i IFS=\" \"$'\\n'\n-\tfor i in $(git help -a|egrep '^ [a-zA-Z0-9]')\n+\tfor i in $(git help -a| \\egrep '^ [a-zA-Z0-9]')\n \tdo\n \t\tcase $i in\n \t\t*--*) : helper pattern;;\n-- \n1.8.0\n\n\n-- \nAdam Tkac, Red Hat, Inc.\n"},{"id":"204267","messageId":"7vpq2wqr3v.fsf@alter.siamese.dyndns.org","threadId":"32242","inReplyTo":"20121129151418.GA19169@redhat.com","subject":"Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T17:03:00Z","receivedAt":"2012-11-29T17:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Tkac writes:\n\n> Subject: Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion\n\nThe code does not seem to do anything special if it is not aliased,\nthough, so \"If ...\" part does not sound correct; perhaps you meant\n\"just in case egrep is aliased to something totally wacky\" or\nsomething?\n\nThe script seems to use commands other than 'egrep' that too can be\naliased to do whatever unexpected things. How does this patch get\naway without backslashing them all, like\n\n\t\\echo ...\n \\sed ...\n \\test ...\n \\: comment ...\n\t\\git args ...\n\nand still fix problems for users? Can't the same solution you would\ngive to users who alias one of the above to do something undesirable\nbe applied to those who alias egrep?\n\nPuzzled...\n\n> Originally reported as https://bugzilla.redhat.com/show_bug.cgi?id=863780\n>\n> Signed-off-by: Adam Tkac \n> Signed-off-by: Holger Arnold \n> ---\n> contrib/completion/git-completion.bash | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 0960acc..79073c2 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -565,7 +565,7 @@ __git_complete_strategy ()\n> __git_list_all_commands ()\n> {\n> \tlocal i IFS=\" \"$'\\n'\n> -\tfor i in $(git help -a|egrep '^ [a-zA-Z0-9]')\n> +\tfor i in $(git help -a| \\egrep '^ [a-zA-Z0-9]')\n> \tdo\n> \t\tcase $i in\n> \t\t*--*) : helper pattern;;\n> -- \n> 1.8.0\n"},{"id":"204269","messageId":"7vk3t4qpoe.fsf@alter.siamese.dyndns.org","threadId":"32242","inReplyTo":"7vpq2wqr3v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T17:33:53Z","receivedAt":"2012-11-29T17:33:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Adam Tkac writes:\n>\n>> Subject: Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion\n>\n> The code does not seem to do anything special if it is not aliased,\n> though, so \"If ...\" part does not sound correct; perhaps you meant\n> \"just in case egrep is aliased to something totally wacky\" or\n> something?\n>\n> The script seems to use commands other than 'egrep' that too can be\n> aliased to do whatever unexpected things. How does this patch get\n> away without backslashing them all, like\n>\n> \t\\echo ...\n> \\sed ...\n> \\test ...\n> \\: comment ...\n> \t\\git args ...\n>\n> and still fix problems for users? Can't the same solution you would\n> give to users who alias one of the above to do something undesirable\n> be applied to those who alias egrep?\n>\n> Puzzled...\n\nSorry for having been more snarky than necessary (blame it to lack\nof caffeine). What I was trying to get at were:\n\n * I have this suspicion that this patch exists only because you saw\n somebody who aliases egrep to something unexpected by the use of\n it in this script, and egrep *happened* to be the only such\n \"unreasonable\" alias. The reporter may not have aliased echo or\n sed away, or the aliases to these command *happened* to produce\n \"acceptable\" output (even though it might have been slightly\n different from unaliased one, the difference *happened* not to\n matter for the purpose of this script).\n\n * To the person who observes the same aliasing breakage due to his\n aliasing sed to something else, you would solve his problem by\n telling him \"don't do that, then\". If that is the solution, why\n wouldn't it work for egrep?\n\n * The next person who aliased other commands this script uses in\n such a way that the behaviour of the alias differs sufficiently\n from the unaliased version, you will have to patch the file\n again, with the same backslashing. This patch is not a solution,\n but a band-aid that only works for a particular case you\n *happened* to have seen.\n\n * A complete solution that follows the direction this patch\n suggests would involve backslashing *all* commands that can\n potentially aliased away. Is that really the direction we would\n want to go in (answer: I doubt it)? Is that the only approach to\n solve this aliasing issue (answer: I don't know, but we should\n try to pursue it before applying a band-aid that is not a\n solution)?\n\nIs there a way to tell bash \"do not alias-expand from here up to\nthere\"? Perhaps \"shopt -u expand_aliases\" upon entry and restore\nits original value when we exit, or something?\n\nIOW, something along this line?\n\n contrib/completion/git-completion.bash | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash\nindex 0b77eb1..193f53c 100644\n--- i/contrib/completion/git-completion.bash\n+++ w/contrib/completion/git-completion.bash\n@@ -23,6 +23,14 @@\n # 3) Consider changing your PS1 to also show the current branch,\n # see git-prompt.sh for details.\n \n+if shopt -q expand_aliases\n+then\n+\t_git__aliases_were_enabled=yes\n+else\n+\t_git__aliases_were_enabled=\n+fi\n+shopt -u expand_aliases\n+\n case \"$COMP_WORDBREAKS\" in\n *:*) : great ;;\n *) COMP_WORDBREAKS=\"$COMP_WORDBREAKS:\"\n@@ -2504,3 +2512,8 @@ __git_complete gitk __gitk_main\n if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n __git_complete git.exe __git_main\n fi\n+\n+if test -n \"$_git__aliases_were_enabled\"\n+then\n+\tshopt -s expand_aliases\n+fi\n"},{"id":"204561","messageId":"20121206140541.GA4892@redhat.com","threadId":"32242","inReplyTo":"7vk3t4qpoe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Adam Tkac","fromEmail":"atkac@redhat.com","sentAt":"2012-12-06T14:05:42Z","receivedAt":"2012-12-06T14:05:42Z","isPatch":true,"sender":{"key":"atkac@redhat.com","avatar":null},"body":"On Thu, Nov 29, 2012 at 09:33:53AM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Adam Tkac writes:\n> >\n> >> Subject: Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion\n> >\n> > The code does not seem to do anything special if it is not aliased,\n> > though, so \"If ...\" part does not sound correct; perhaps you meant\n> > \"just in case egrep is aliased to something totally wacky\" or\n> > something?\n> >\n> > The script seems to use commands other than 'egrep' that too can be\n> > aliased to do whatever unexpected things. How does this patch get\n> > away without backslashing them all, like\n> >\n> > \t\\echo ...\n> > \\sed ...\n> > \\test ...\n> > \\: comment ...\n> > \t\\git args ...\n> >\n> > and still fix problems for users? Can't the same solution you would\n> > give to users who alias one of the above to do something undesirable\n> > be applied to those who alias egrep?\n> >\n> > Puzzled...\n> \n> Sorry for having been more snarky than necessary (blame it to lack\n> of caffeine). What I was trying to get at were:\n> \n> * I have this suspicion that this patch exists only because you saw\n> somebody who aliases egrep to something unexpected by the use of\n> it in this script, and egrep *happened* to be the only such\n> \"unreasonable\" alias. The reporter may not have aliased echo or\n> sed away, or the aliases to these command *happened* to produce\n> \"acceptable\" output (even though it might have been slightly\n> different from unaliased one, the difference *happened* not to\n> matter for the purpose of this script).\n> \n> * To the person who observes the same aliasing breakage due to his\n> aliasing sed to something else, you would solve his problem by\n> telling him \"don't do that, then\". If that is the solution, why\n> wouldn't it work for egrep?\n> \n> * The next person who aliased other commands this script uses in\n> such a way that the behaviour of the alias differs sufficiently\n> from the unaliased version, you will have to patch the file\n> again, with the same backslashing. This patch is not a solution,\n> but a band-aid that only works for a particular case you\n> *happened* to have seen.\n> \n> * A complete solution that follows the direction this patch\n> suggests would involve backslashing *all* commands that can\n> potentially aliased away. Is that really the direction we would\n> want to go in (answer: I doubt it)? Is that the only approach to\n> solve this aliasing issue (answer: I don't know, but we should\n> try to pursue it before applying a band-aid that is not a\n> solution)?\n> \n> Is there a way to tell bash \"do not alias-expand from here up to\n> there\"? Perhaps \"shopt -u expand_aliases\" upon entry and restore\n> its original value when we exit, or something?\n> \n> IOW, something along this line?\n\nThis won't work, unfortunately, because shopt settings aren't inherited by\nsubshell (and for example egrep is called in subshell).\n\nI discussed this issue with colleagues and we found basically two \"fixes\":\n\n1. Tell people \"do not use aliases which breaks completion script\"\n2. Prefix all commands with \"command\", i.e. `command egrep` etc.\n\nIn my opinion \"2.\" is better long time solution, what do you think?\n\nRegards, Adam\n\n> \n> contrib/completion/git-completion.bash | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n> \n> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash\n> index 0b77eb1..193f53c 100644\n> --- i/contrib/completion/git-completion.bash\n> +++ w/contrib/completion/git-completion.bash\n> @@ -23,6 +23,14 @@\n> # 3) Consider changing your PS1 to also show the current branch,\n> # see git-prompt.sh for details.\n> \n> +if shopt -q expand_aliases\n> +then\n> +\t_git__aliases_were_enabled=yes\n> +else\n> +\t_git__aliases_were_enabled=\n> +fi\n> +shopt -u expand_aliases\n> +\n> case \"$COMP_WORDBREAKS\" in\n> *:*) : great ;;\n> *) COMP_WORDBREAKS=\"$COMP_WORDBREAKS:\"\n> @@ -2504,3 +2512,8 @@ __git_complete gitk __gitk_main\n> if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n> __git_complete git.exe __git_main\n> fi\n> +\n> +if test -n \"$_git__aliases_were_enabled\"\n> +then\n> +\tshopt -s expand_aliases\n> +fi\n> \n> \n\n-- \nAdam Tkac, Red Hat, Inc.\n"},{"id":"204563","messageId":"7v4njzjbzo.fsf@alter.siamese.dyndns.org","threadId":"32242","inReplyTo":"20121206140541.GA4892@redhat.com","subject":"Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-06T18:01:47Z","receivedAt":"2012-12-06T18:01:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Tkac writes:\n\n> On Thu, Nov 29, 2012 at 09:33:53AM -0800, Junio C Hamano wrote:\n> ...\n>> IOW, something along this line?\n>\n> This won't work, unfortunately, because shopt settings aren't inherited by\n> subshell (and for example egrep is called in subshell).\n>\n> I discussed this issue with colleagues and we found basically two \"fixes\":\n>\n> 1. Tell people \"do not use aliases which breaks completion script\"\n> 2. Prefix all commands with \"command\", i.e. `command egrep` etc.\n>\n> In my opinion \"2.\" is better long time solution, what do you think?\n\nJudging from what is in /etc/bash_completion.d/ (I am on Debian), I\nthink that others are divided. Many but not all prefix \"command\" in\nfront of \"grep\", but nobody does the same for \"egrep\", \"cut\", \"tr\",\n\"sed\", etc.\n\nIf it were up to me, I would say we pick #1, but I cc'ed the people\nwho have been more involved in our bash-completion code because they\nare in a better position to argue between the two than I am.\n\nThoughts?\n"},{"id":"204790","messageId":"CAMP44s2QHrwv0wZ=r+_E2i19Y-zJChPHaX=UeHXaGAppNzqm6A@mail.gmail.com","threadId":"32242","inReplyTo":"7v4njzjbzo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] If `egrep` is aliased, temporary disable it in bash.completion","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-12-12T22:34:29Z","receivedAt":"2012-12-12T22:34:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Dec 6, 2012 at 12:01 PM, Junio C Hamano wrote:\n> Adam Tkac writes:\n>\n>> On Thu, Nov 29, 2012 at 09:33:53AM -0800, Junio C Hamano wrote:\n>> ...\n>>> IOW, something along this line?\n>>\n>> This won't work, unfortunately, because shopt settings aren't inherited by\n>> subshell (and for example egrep is called in subshell).\n>>\n>> I discussed this issue with colleagues and we found basically two \"fixes\":\n>>\n>> 1. Tell people \"do not use aliases which breaks completion script\"\n>> 2. Prefix all commands with \"command\", i.e. `command egrep` etc.\n>>\n>> In my opinion \"2.\" is better long time solution, what do you think?\n>\n> Judging from what is in /etc/bash_completion.d/ (I am on Debian), I\n> think that others are divided. Many but not all prefix \"command\" in\n> front of \"grep\", but nobody does the same for \"egrep\", \"cut\", \"tr\",\n> \"sed\", etc.\n>\n> If it were up to me, I would say we pick #1, but I cc'ed the people\n> who have been more involved in our bash-completion code because they\n> are in a better position to argue between the two than I am.\n\nWhy not both? I do prefer #1, but I don't see why we wouldn't prefix\nsome commonly problematic ones (\\egrep), prefixing all of them seems\noverkill for me.\n\n-- \nFelipe Contreras\n"}]}