{"thread":{"id":"30434","subject":"[PATCH v3] completion: add new _GIT_complete helper","startedAt":"2012-05-05T15:23:20Z","lastAt":"2012-05-14T09:11:32Z","messageCount":24,"participants":["Felipe Contreras","Jonathan Nieder","Tay Ray Chuan","SZEDER Gábor"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"190846","messageId":"1336231400-6878-1-git-send-email-felipe.contreras@gmail.com","threadId":"30434","inReplyTo":null,"subject":"[PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-05T15:23:20Z","receivedAt":"2012-05-05T15:23:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This simplifies the completions, and makes it easier to define aliases:\n\n _GIT_complete gf git_fetch\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n\nSince v3:\n\n * Rename to _GIT_complete to follow bash completion \"guidelines\"\n * Get rid of foo_wrap name\n\nSince v2:\n\n * Remove stuff related to aliases fixes; should work on top of master\n\n contrib/completion/git-completion.bash |   67 ++++++++++++++------------------\n t/t9902-completion.sh                  |    2 +-\n 2 files changed, 31 insertions(+), 38 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9f56ec7..f300b87 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2603,21 +2603,6 @@ _git ()\n {\n \tlocal i c=1 command __git_dir\n \n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n-\n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n@@ -2667,22 +2652,6 @@ _git ()\n \n _gitk ()\n {\n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n-\n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n-\n \t__git_has_doubledash && return\n \n \tlocal g=\"$(__gitdir)\"\n@@ -2703,16 +2672,40 @@ _gitk ()\n \t__git_complete_revlist\n }\n \n-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git\n-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _gitk gitk\n+__git_func_wrap ()\n+{\n+\tif [[ -n ${ZSH_VERSION-} ]]; then\n+\t\temulate -L bash\n+\t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bug that leaves 'words' as a special\n+\t\t# variable in versions < 4.3.12\n+\t\ttypeset -h words\n+\n+\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n+\t\t# array if IFS doesn't contain spaces.\n+\t\ttypeset -h IFS\n+\tfi\n+\tlocal cur words cword prev\n+\t_get_comp_words_by_ref -n =: cur words cword prev\n+\t__git_func \"$@\"\n+}\n+\n+_GIT_complete ()\n+{\n+\tlocal name=\"${2-$1}\"\n+\teval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n+\tcomplete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n+\t\t|| complete -o default -o nospace -F _${name}_wrap $1\n+}\n+\n+_GIT_complete git\n+_GIT_complete gitk\n \n # The following are necessary only for Cygwin, and only are needed\n # when the user has tab-completed the executable name and consequently\n # included the '.exe' suffix.\n #\n if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git.exe\n+_GIT_complete git.exe git\n fi\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 5bda6b6..331a5b9 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -63,7 +63,7 @@ run_completion ()\n \tlocal _cword\n \t_words=( $1 )\n \t(( _cword = ${#_words[@]} - 1 ))\n-\t_git && print_comp\n+\t_git_wrap && print_comp\n }\n \n test_completion ()\n-- \n1.7.10\n"},{"id":"190847","messageId":"20120505155423.GA14684@burratino","threadId":"30434","inReplyTo":"1336231400-6878-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T15:54:23Z","receivedAt":"2012-05-05T15:54:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> Since v3:\n>\n>  * Rename to _GIT_complete to follow bash completion \"guidelines\"\n>  * Get rid of foo_wrap name\n\nThanks.  Gábor, does the \"all caps _GIT_ prefix for public API\nfunctions\" convention look like one we should adopt?  If I understand\ncorrectly, previously contrib/completion/git-completion.bash used\nleading double underscores for everything except completion functions,\nso this is a change.\n\nFollowing a convention similar to the bash-completion project's\nproposed future convention doesn't really help compatibility.  If we\nwant to be able to include this function in that project without\nchange some day, we'd have to call it _BC_git_complete. :)\n\nI personally would be happier with a git_complete function provided\nby another script, like this:\n\n\tcontrib/completion/git-completion.bash:\n\n\t\t__git_complete () {\n\t\t\t...\n\t\t}\n\n\tcontrib/completion/bash-helpers.bash:\n\n\t\tgit_complete () {\n\t\t\t__git_complete \"$@\"\n\t\t}\n\nOne might object that if the user includes bash-helpers.bash (name is\njust a strawman) in .bashrc for interactive shells because he is\ndefining some custom completion functions,\n\n\tgit<TAB>\n\nwould show the git_complete function.  I think that's fine.  Maybe\nthe user would enjoy the reminder.  git<TAB> also shows any\ndashed-form git commands that happen to be on the $PATH.\n\nPlease don't take this as a strong objection.  Maybe the\nuser-unfriendly version would be called _GIT_complete and someone\ninterested can provide the friendly git_complete and git_ps1 wrappers\nseparately, for example.  I just think it is worth thinking carefully\nand being consistent about.\n\nHope that helps,\nJonathan\n"},{"id":"190850","messageId":"CAMP44s1dhAjKt3mxVmg2+0qp-QTyjYb1knhxf+m177Cg2ZBC1Q@mail.gmail.com","threadId":"30434","inReplyTo":"20120505155423.GA14684@burratino","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-05T16:38:02Z","receivedAt":"2012-05-05T16:38:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>\n>> Since v3:\n>>\n>>  * Rename to _GIT_complete to follow bash completion \"guidelines\"\n>>  * Get rid of foo_wrap name\n>\n> Thanks.  Gábor, does the \"all caps _GIT_ prefix for public API\n> functions\" convention look like one we should adopt?  If I understand\n> correctly, previously contrib/completion/git-completion.bash used\n> leading double underscores for everything except completion functions,\n> so this is a change.\n>\n> Following a convention similar to the bash-completion project's\n> proposed future convention doesn't really help compatibility.  If we\n> want to be able to include this function in that project without\n> change some day, we'd have to call it _BC_git_complete. :)\n\nNo, that's for bash-completion's functions, this is a git bash\ncompletion function.\n\nAnd in any case, if they want something different they can change it\nthemselves, and they could tell us.\n\nBut wasn't you the one that suggested we follow the bash-completion's\nguidelines, or that was only when the guidelines happened to match\nyour preference?\n\nThere are basically four arguments that have been brought forward.\n\n1) Namespace\n\nYou said there were two namespaces:\n\n> _git_*  (completion functions)\n>__git_* (everything else, including public interfaces like __git_ps1)\n\nBut that's not actually true, we have these:\n\n  __gitfoo (__gitdir, __gitcomp_1, __gitcomp, __gitcomp_nl)\n  __git_foo\n  _git_foo\n  _git, _gitk\n\nAnd what is used for what is not exactly clear.\n\nCurrently the only function meant to be public is __git_ps1, but\nthere's other __git_foo functions that are not meant to be public, so\nclearly there's no namespace for public functions. Everything is\nad-hoc.\n\nIt could be assumed that anything that doesn't start with a _ is\nreserved for the user. Of course, there's no such guideline anywhere,\nso this would be a self-imposed limitation.\n\n2) Guideline\n\nYou brought this argument forward, but it turns out the\nbash-completion guys have no actual guideline; they are still trying\nto decide what would be the naming for public functions.\n\nIf there's anything close to a guideline for bash-completion\nfunctions, it would be a _BC_ prefix. Our script thus would be using\n_GIT_ for its public functions. This would mean __git_ps1 should be\nrenamed to _GIT_ps1.\n\nBut now it seems you want to separate from this guideline.\n\n3) Conflicts\n\nAnother problem is that a user might already have a function named\nlike that, which means 'git_complete' has higher chances of collision.\nBut I wrote checks that would ensure this doesn't happen, still,\nnobody was interested in those checks.\n\nIt seems people are not interested in *real* conflicts, but rather\ntheoretical namespace collisions.\n\n4) Convenience\n\ngit_complete is nicer than _GIT_complete.\n\n\nIt seems to me the push-back away from 'git_complete' is mostly due to\nimaginary reasons, and now apparently specious reasons as well. So I\nguess there's no point in discussing; no amount of evidence is going\nto convince anybody to anything.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"190852","messageId":"20120505164736.GC14684@burratino","threadId":"30434","inReplyTo":"CAMP44s1dhAjKt3mxVmg2+0qp-QTyjYb1knhxf+m177Cg2ZBC1Q@mail.gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T16:47:36Z","receivedAt":"2012-05-05T16:47:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> Following a convention similar to the bash-completion project's\n>> proposed future convention doesn't really help compatibility.  If we\n>> want to be able to include this function in that project without\n>> change some day, we'd have to call it _BC_git_complete. :)\n>\n> No, that's for bash-completion's functions, this is a git bash\n> completion function.\n\nPlease read again.  \"If we want to be able to include this ...\".  I\nassume we do not, but that would be the reason to follow their\nconvention to the letter.\n\n[...]\n> But wasn't you the one that suggested we follow the bash-completion's\n> guidelines, or that was only when the guidelines happened to match\n> your preference?\n\nSorry for the lack of clarity before.  I like to hope that \"Because\nJonathan said so\" is _never_ the only justification for putting up\nwith a technical change you disagree with.  In this instance, my\npersonal justification was \"Because our completion scripts are already\nusing this convention, which happens to come from bash-completion's\nguidelines and here are the reasons behind those\".\n\nAlso, I think you have misunderstood me.  I was asking Gábor for input\nbecause you were proposing changing a convention and I thought Gábor\nhad been maintaining the completion scripts.  I was not trying to say\n\"Please don't do this\".\n\nI was not inviting you to argue with me.\n\nKind regards,\nJonathan\n"},{"id":"190854","messageId":"20120505165259.GD14684@burratino","threadId":"30434","inReplyTo":"20120505164736.GC14684@burratino","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T16:52:59Z","receivedAt":"2012-05-05T16:52:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n>> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>>> Following a convention similar to the bash-completion project's\n>>> proposed future convention doesn't really help compatibility.  If we\n>>> want to be able to include this function in that project without\n>>> change some day, we'd have to call it _BC_git_complete. :)\n[...]\n> Please read again.  \"If we want to be able to include this ...\".  I\n> assume we do not, but that would be the reason to follow their\n> convention to the letter.\n\nQuick clarification: I actually think it would be nice to make it easy\nto pass maintenance of the git completion script to that project if\nautomatic option discovery means changes to the script settle down or\nif we have less time to work on it some day.  Unfortunately, the\nproposed namespace rules[1] (I didn't think the change had happened or\nbeen set in stone yet?) would not make that easy.  Oh well.\n\n[1] http://thread.gmane.org/gmane.comp.shells.bash.completion.scm/2013/focus=3158\n"},{"id":"190855","messageId":"CAMP44s2NRMxuvQXk4RyqFYR=oQw_vrJhmbdwtXsrg=eHhGBS2w@mail.gmail.com","threadId":"30434","inReplyTo":"20120505164736.GC14684@burratino","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-05T17:20:38Z","receivedAt":"2012-05-05T17:20:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, May 5, 2012 at 6:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> On Sat, May 5, 2012 at 5:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>>> Following a convention similar to the bash-completion project's\n>>> proposed future convention doesn't really help compatibility.  If we\n>>> want to be able to include this function in that project without\n>>> change some day, we'd have to call it _BC_git_complete. :)\n>>\n>> No, that's for bash-completion's functions, this is a git bash\n>> completion function.\n>\n> Please read again.  \"If we want to be able to include this ...\".  I\n> assume we do not, but that would be the reason to follow their\n> convention to the letter.\n\nWe don't know what is their convention, so we can't possibly follow it\nto the letter. From the looks of it, they want _BC_* for *their*\npublic APIs, we shouldn't be using that:\n\nhttp://article.gmane.org/gmane.comp.shells.bash.completion.devel/3158\n\n> [...]\n>> But wasn't you the one that suggested we follow the bash-completion's\n>> guidelines, or that was only when the guidelines happened to match\n>> your preference?\n>\n> Sorry for the lack of clarity before.  I like to hope that \"Because\n> Jonathan said so\" is _never_ the only justification for putting up\n> with a technical change you disagree with.\n\nAnd I'd like to think that when you filibuster a discussion there's a\ngood reason for it.\n\n> In this instance, my\n> personal justification was \"Because our completion scripts are already\n> using this convention, which happens to come from bash-completion's\n> guidelines and here are the reasons behind those\".\n\nI see, so the bash-completion guidelines was not the main point, that\nwas just extra sauce?\n\nThen it would follow that if the bash-completion guidelines were\ndifferent, that wouldn't change your main argument, because the\nreasons would be the same. But when I argued that there were no such\nbash-completion guidelines you didn't just drop this side-argument,\nyou pressed on it and even included the bash-completion mailing list.\nSo all the discussion about the existence of these guidelines was\npointless?\n\nI am going to refrain from expressing what I think of this.\n\n> Also, I think you have misunderstood me.  I was asking Gábor for input\n> because you were proposing changing a convention and I thought Gábor\n> had been maintaining the completion scripts.  I was not trying to say\n> \"Please don't do this\".\n>\n> I was not inviting you to argue with me.\n\nThen I'm going to ignore your arguments about bash-completion guidelines.\n\nThis is what I propose:\n\n1) We name the new function __git_complete; this would be a temporary\nname, the function would not be meant to be public\n2) We discuss later what would be the namespace for public functions,\nand rename, or add wrappers for them (e.g. _GIT_ps1, _GIT_complete)\n3) We standardize the odd functions: __gitdir -> __git_dir\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"190857","messageId":"20120505173327.GE14684@burratino","threadId":"30434","inReplyTo":"CAMP44s2NRMxuvQXk4RyqFYR=oQw_vrJhmbdwtXsrg=eHhGBS2w@mail.gmail.com","subject":"Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T17:33:27Z","receivedAt":"2012-05-05T17:33:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> And I'd like to think that when you filibuster a discussion there's a\n> good reason for it.\n\nKind other people on the list, please enlighten me.  What did I say to\ntrigger this crap?\n\nJonathan\n"},{"id":"190858","messageId":"20120505174410.GA17937@burratino","threadId":"30434","inReplyTo":"CAMP44s2NRMxuvQXk4RyqFYR=oQw_vrJhmbdwtXsrg=eHhGBS2w@mail.gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T17:44:11Z","receivedAt":"2012-05-05T17:44:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> This is what I propose:\n>\n> 1) We name the new function __git_complete; this would be a temporary\n> name, the function would not be meant to be public\n> 2) We discuss later what would be the namespace for public functions,\n> and rename, or add wrappers for them (e.g. _GIT_ps1, _GIT_complete)\n> 3) We standardize the odd functions: __gitdir -> __git_dir\n\nSounds excellent to me.\n\nThanks,\nJonathan\n"},{"id":"190860","messageId":"CAMP44s1SLb6CXMKxRDFUbxkg56dU3=ObZPZh7XxQC3vovNnvdw@mail.gmail.com","threadId":"30434","inReplyTo":"20120505173327.GE14684@burratino","subject":"Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-05T18:23:33Z","receivedAt":"2012-05-05T18:23:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, May 5, 2012 at 7:33 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>\n>> And I'd like to think that when you filibuster a discussion there's a\n>> good reason for it.\n>\n> Kind other people on the list, please enlighten me.  What did I say to\n> trigger this crap?\n\nYou said \"Because our completion scripts are already using this\nconvention, which happens to come from bash-completion's guidelines\nand here are the reasons behind those\", so their guidelines are not\nessential, only the reasons behind the guidelines, but:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/195685\nhttp://article.gmane.org/gmane.comp.version-control.git/195689\nhttp://article.gmane.org/gmane.comp.version-control.git/195691\nhttp://article.gmane.org/gmane.comp.shells.bash.completion.devel/3877\n\nYou could have skipped this, apparently it was not relevant for the\ndiscussion, it's not feedback for the patch, and the abrasiveness\nunnecessary.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/195719\nhttp://article.gmane.org/gmane.comp.version-control.git/195723\nhttp://article.gmane.org/gmane.comp.version-control.git/195737\nhttp://article.gmane.org/gmane.comp.version-control.git/195742\n\nAnd more irrelevant bash-completion stuff, and then you even get angry\nwhen I suggest those guidelines were not there, but isn't supposed to\nbe irrelevant?\n\nhttp://article.gmane.org/gmane.comp.version-control.git/195744\n\nAnd then you finally assume that because I say the guidelines were not\nthere, I must not like namespace conventions. I don't see how that\nhelps in any way.\n\nSo the discussion about whether bash-completion actually had public\nAPI guidelines or not took basically the whole thread, and barely\nanything else got discussed. And now you say whether or not they had\nthis guidelines is not relevant.\n\nIf you had said \"You know, I think they have this guideline, but it's\nnot really relevant, what is relevant is X\" right when the topic of\nbash-completion guidelines popped up, this thread would have looked\nmuch different.\n\nIn addition to that you are saying that I shouldn't have took all\nthose mails as some kind of impediment from you, just feedback, even\nthough you say: \"you refuse to put two and two together\", or \"OK, you\nwin\", or \"it isn't my responsibility to waste time arguing with you\"\nbecause I counter-argue your feedback.\n\nI honestly don't know what to think. I guess I will think three times\nbefore replying to your feedback... hopefully you won't take offense\nin my silence as well =/\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"190861","messageId":"20120505183954.GA18844@burratino","threadId":"30434","inReplyTo":"CAMP44s1SLb6CXMKxRDFUbxkg56dU3=ObZPZh7XxQC3vovNnvdw@mail.gmail.com","subject":"Re: Jonathan gives feedback --> flamewars inevitable?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T18:39:54Z","receivedAt":"2012-05-05T18:39:54Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> If you had said \"You know, I think they have this guideline, but it's\n> not really relevant, what is relevant is X\" right when the topic of\n> bash-completion guidelines popped up, this thread would have looked\n> much different.\n\nAh, apparently I was still unclear.\n\nThe leading underscore is a convention.  They do use it.  They are\nthinking of moving to another convention and will probably do so.\n\nI believe that convention is relevant to the git completion script.\nNot because we should blindly follow everything the bash-completion\nproject does, but because there are reasons behind that convention,\nfollowing common practices means our behavior is less surprising, and\nin fact I do want it to be possible to incorporate git's completion\nscript in the bash-completion project if that's convenient.\n\nA little changed because their proposed future namespace does not make\nit possible for outside contributors to make unofficial extensions and\nthen include them as something official later without change.  Maybe\nthat's a flaw.\n\nWhen I stated what I thought was (and still think is) a fact, and you\ndecided that the best use of your time was to argue with me about that\ninstead of talking about the consequences, yes, I was annoyed.  It was\na rabbit hole I shouldn't have followed; you're right.  But I still\ndon't know what I should have said instead, or how to stop it from\ncoming up again and again.\n\nJonathan\n"},{"id":"190862","messageId":"20120505184255.GB18844@burratino","threadId":"30434","inReplyTo":"CAMP44s1SLb6CXMKxRDFUbxkg56dU3=ObZPZh7XxQC3vovNnvdw@mail.gmail.com","subject":"Re: Jonathan gives feedback --> flamewars inevitable?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-05T18:42:55Z","receivedAt":"2012-05-05T18:42:55Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Felipe Contreras wrote:\n\n> http://article.gmane.org/gmane.comp.version-control.git/195685\n> http://article.gmane.org/gmane.comp.version-control.git/195689\n> http://article.gmane.org/gmane.comp.version-control.git/195691\n> http://article.gmane.org/gmane.comp.shells.bash.completion.devel/3877\n>\n> You could have skipped this\n\nI'm also really confused about this.  Are you saying that trying\nto figure out the bash-completion conventions was a waste of time?\nIt even resulted in a patch to bash-completion being applied that\nimproved consistency, which I thought was an attribute you valued.\n\nJonathan\n"},{"id":"190876","messageId":"CALUzUxp-4ogm-qmtoyHLicE7eD6Ut8m_hQjCMgXvxzbi7YPhQQ@mail.gmail.com","threadId":"30434","inReplyTo":"20120505173327.GE14684@burratino","subject":"Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T05:23:56Z","receivedAt":"2012-05-06T05:23:56Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, May 6, 2012 at 1:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Felipe Contreras wrote:\n>\n>> And I'd like to think that when you filibuster a discussion there's a\n>> good reason for it.\n>\n> Kind other people on the list, please enlighten me.  What did I say to\n> trigger this crap?\n\nFelipe, Jonathan is and continues to be one of the kindest, diligent\nreviewers on this list. The accusation is uncalled for.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"190891","messageId":"20120506103006.GI2164@goldbirke","threadId":"30434","inReplyTo":"20120505155423.GA14684@burratino","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T10:30:06Z","receivedAt":"2012-05-06T10:30:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, May 05, 2012 at 10:54:23AM -0500, Jonathan Nieder wrote:\n> Felipe Contreras wrote:\n> \n> > Since v3:\n> >\n> >  * Rename to _GIT_complete to follow bash completion \"guidelines\"\n> >  * Get rid of foo_wrap name\n> \n> Thanks.  Gábor, does the \"all caps _GIT_ prefix for public API\n> functions\" convention look like one we should adopt?  If I understand\n> correctly, previously contrib/completion/git-completion.bash used\n> leading double underscores for everything except completion functions,\n> so this is a change.\n\nDunno.  I have only three concerns:\n\n- It doesn't contaminate \"my\" namespace, where my installed programs,\n  aliases, and shell functions are, i.e. it begins with an underscore.\n- Its name conveys that it's git-specific.\n- It's not called _git_complete, so the completion script (in\n  particular at the end of _git()) won't misrecognize it as a\n  completion function for the 'git complete' command, just in case\n  somebody ever happens to have such a command or alias.\n\nI'm not sure about the capital letters, but it fulfills all three.\n\n\n> I personally would be happier with a git_complete function provided\n> by another script, like this:\n> \n> \tcontrib/completion/git-completion.bash:\n> \n> \t\t__git_complete () {\n> \t\t\t...\n> \t\t}\n> \n> \tcontrib/completion/bash-helpers.bash:\n> \n> \t\tgit_complete () {\n> \t\t\t__git_complete \"$@\"\n> \t\t}\n> \n> One might object that if the user includes bash-helpers.bash (name is\n> just a strawman) in .bashrc for interactive shells because he is\n> defining some custom completion functions,\n> \n> \tgit<TAB>\n> \n> would show the git_complete function.  I think that's fine.\n\nIt depends on what else will go into that bash-helpers.bash file.  If\nI have to source it to use git completion or the git-specific bash\nprompt, then I won't be very happy about it.\n\n> Maybe\n> the user would enjoy the reminder.\n\nA reminder for what?\n\nIt's a configuration thing, so it will be used in .bashrc; I think\nit's quite unlikely that it will be used interactively.\n\n\nBest,\nGábor\n"},{"id":"190895","messageId":"20120506111425.GJ2164@goldbirke","threadId":"30434","inReplyTo":"1336231400-6878-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T11:14:25Z","receivedAt":"2012-05-06T11:14:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n\nOn Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n> This simplifies the completions, and makes it easier to define aliases:\n> \n>  _GIT_complete gf git_fetch\n\nSo, 'gf' is an alias for 'git fetch', for which the user would like to\nuse the completion for 'git fetch', right?  But that completion\nfunction is caled _git_fetch(), so the underscore prefix is missing\nhere.\n\nBesides, this example won't work, because the completion for 'git\nfetch' uses __git_complete_remote_or_refspec(), which in turn relies\non finding out the name of the git command from the word on the\ncommand line, and it won't be able to do that from 'gf'.\n\nI remember we discussed this in an earlier round, and you even\nsuggested a possible fix (passing the command name as argument to\n__git_complete_remote_or_refspec()).  I think that's the right thing\nto do here.\n\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n> \n> Since v3:\n\n_This_ is v3 ;)\n\n>  * Rename to _GIT_complete to follow bash completion \"guidelines\"\n>  * Get rid of foo_wrap name\n> \n> Since v2:\n> \n>  * Remove stuff related to aliases fixes; should work on top of master\n> \n>  contrib/completion/git-completion.bash |   67 ++++++++++++++------------------\n>  t/t9902-completion.sh                  |    2 +-\n>  2 files changed, 31 insertions(+), 38 deletions(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 9f56ec7..f300b87 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n\n\n> +__git_func_wrap ()\n\nGood.\n\n> +{\n> +\tif [[ -n ${ZSH_VERSION-} ]]; then\n> +\t\temulate -L bash\n> +\t\tsetopt KSH_TYPESET\n> +\n> +\t\t# workaround zsh's bug that leaves 'words' as a special\n> +\t\t# variable in versions < 4.3.12\n> +\t\ttypeset -h words\n> +\n> +\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n> +\t\t# array if IFS doesn't contain spaces.\n> +\t\ttypeset -h IFS\n> +\tfi\n> +\tlocal cur words cword prev\n> +\t_get_comp_words_by_ref -n =: cur words cword prev\n> +\t__git_func \"$@\"\n\nWhat is this \"$@\" for and why?  None of the _git_<cmd>() functions\ntake any arguments, nor does _git() and _gitk(), and AFAICT Bash won't\npass any either.\n\n> +}\n> +\n> +_GIT_complete ()\n> +{\n> +\tlocal name=\"${2-$1}\"\n> +\teval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n\nStill don't like the subshell and sed here ...\n\n> +\tcomplete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n> +\t\t|| complete -o default -o nospace -F _${name}_wrap $1\n> +}\n> +\n> +_GIT_complete git\n> +_GIT_complete gitk\n\n... because it adds delay when the completion script is loaded.  But I\nstill don't have ideas how to avoid them.\n"},{"id":"190896","messageId":"20120506113021.GK2164@goldbirke","threadId":"30434","inReplyTo":"20120506111425.GJ2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T11:30:21Z","receivedAt":"2012-05-06T11:30:21Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:\n> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n> > This simplifies the completions, and makes it easier to define aliases:\n> > \n> >  _GIT_complete gf git_fetch\n> \n> So, 'gf' is an alias for 'git fetch', for which the user would like to\n> use the completion for 'git fetch', right?  But that completion\n> function is caled _git_fetch(), so the underscore prefix is missing\n> here.\n> \n> Besides, this example won't work, because the completion for 'git\n> fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n> on finding out the name of the git command from the word on the\n> command line, and it won't be able to do that from 'gf'.\n\nI scanned the completion script for places where we iterate over the\nwords on the command line, i.e. for the pattern 'while.*\\$cword'.\n\nIt seems that with the exception of __git_complete_remote_or_refspec()\nall those places seem to be OK to be used with aliases.  They all\nstart the iteration at the first word on the command line ('git' or\n'gf' being the nullth) so they will iterate over all relevant words in\ncase of aliases, too.  Perhaps this is a heritage of the dashed\ncommands; back then the completion script had to deal with 'git cmd'\nand 'git-cmd', too.  __git_complete_remote_or_refspec() starts at the\nsecond word, so that must be changed.\n\n\nBest,\nGábor\n"},{"id":"190897","messageId":"20120506113622.GL2164@goldbirke","threadId":"30434","inReplyTo":"20120506113021.GK2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T11:36:22Z","receivedAt":"2012-05-06T11:36:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, May 06, 2012 at 01:30:21PM +0200, SZEDER Gábor wrote:\n> On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:\n> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n> > > This simplifies the completions, and makes it easier to define aliases:\n> > > \n> > >  _GIT_complete gf git_fetch\n> > \n> > So, 'gf' is an alias for 'git fetch', for which the user would like to\n> > use the completion for 'git fetch', right?  But that completion\n> > function is caled _git_fetch(), so the underscore prefix is missing\n> > here.\n> > \n> > Besides, this example won't work, because the completion for 'git\n> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n> > on finding out the name of the git command from the word on the\n> > command line, and it won't be able to do that from 'gf'.\n> \n> I scanned the completion script for places where we iterate over the\n> words on the command line, i.e. for the pattern 'while.*\\$cword'.\n> \n> It seems that with the exception of __git_complete_remote_or_refspec()\n> all those places seem to be OK to be used with aliases.  They all\n> start the iteration at the first word on the command line ('git' or\n> 'gf' being the nullth) so they will iterate over all relevant words in\n> case of aliases, too.  Perhaps this is a heritage of the dashed\n> commands; back then the completion script had to deal with 'git cmd'\n> and 'git-cmd', too.  __git_complete_remote_or_refspec() starts at the\n> second word, so that must be changed.\n\nThere is one more odd case, though: __git_config_get_set_variables()\niterates over the words on the command line backwards, i.e. starting\nat the index $cword until the index of the word is greater than 1.\nThis means that the iteration will stop at the second word, so this\nmust be adjusted, too, just in case someone wants an alias for 'git\nconfig'.\n"},{"id":"190898","messageId":"20120506121204.GA5799@goldbirke","threadId":"30434","inReplyTo":"20120506111425.GJ2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T12:12:04Z","receivedAt":"2012-05-06T12:12:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n\nOn Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:\n> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n> > +__git_func_wrap ()\n> > +{\n> > +\tif [[ -n ${ZSH_VERSION-} ]]; then\n> > +\t\temulate -L bash\n> > +\t\tsetopt KSH_TYPESET\n> > +\n> > +\t\t# workaround zsh's bug that leaves 'words' as a special\n> > +\t\t# variable in versions < 4.3.12\n> > +\t\ttypeset -h words\n> > +\n> > +\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n> > +\t\t# array if IFS doesn't contain spaces.\n> > +\t\ttypeset -h IFS\n> > +\tfi\n> > +\tlocal cur words cword prev\n> > +\t_get_comp_words_by_ref -n =: cur words cword prev\n> > +\t__git_func \"$@\"\n> > +}\n> > +\n> > +_GIT_complete ()\n> > +{\n> > +\tlocal name=\"${2-$1}\"\n> > +\teval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n> \n> Still don't like the subshell and sed here ...\n> \n> > +\tcomplete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n> > +\t\t|| complete -o default -o nospace -F _${name}_wrap $1\n> > +}\n> > +\n> > +_GIT_complete git\n> > +_GIT_complete gitk\n> \n> ... because it adds delay when the completion script is loaded.  But I\n> still don't have ideas how to avoid them.\n\nOk, I think I got it.  How about this on top of Felipe's patch?\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex f300b87d..8c18db92 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2688,19 +2688,19 @@ __git_func_wrap ()\n \tfi\n \tlocal cur words cword prev\n \t_get_comp_words_by_ref -n =: cur words cword prev\n-\t__git_func \"$@\"\n+\t$1\n }\n \n _GIT_complete ()\n {\n-\tlocal name=\"${2-$1}\"\n-\teval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n-\tcomplete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n-\t\t|| complete -o default -o nospace -F _${name}_wrap $1\n+\tlocal wrapper=\"__git_wrap_$1\"\n+\teval \"$wrapper () { __git_func_wrap $2 ; }\"\n+\tcomplete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n+\t\t|| complete -o default -o nospace -F $wrapper $1\n }\n \n-_GIT_complete git\n-_GIT_complete gitk\n+_GIT_complete git _git\n+_GIT_complete gitk _gitk\n \n # The following are necessary only for Cygwin, and only are needed\n # when the user has tab-completed the executable name and consequently\n\n\nThe point is that __git_func_wrap() is not a template function\nprocessed by _GIT_complete() (or whatever we'll end up calling it)\nanymore, but simply a wrapper function around existing completion\nfunctions.  The name of the completion function to be invoked should\nbe given as argument.  _GIT_complete() then uses 'eval' to create\nanother wrapper function to invoke __git_func_wrap() with the name of\nthe desired completion function.  The name of this dynamically created\nwrapper function is derived from the name of the command or alias,\ni.e. for 'gf' it will be __git_wrap_gf().\n\nThe overhead of the additional function call is not even measureable,\nwhile it would spare the overhead of fork()ing a subshell and\nfork()+exec()ing 'sed' twice when loading the completion script and\nthen for each subsequent alias.\n\n\nBest,\nGábor\n"},{"id":"190936","messageId":"CAMP44s3xwgsfjZA6r+bydu-5r1nEO5cGc=wcDSr+WdwunTtpwg@mail.gmail.com","threadId":"30434","inReplyTo":"20120506111425.GJ2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-06T20:37:06Z","receivedAt":"2012-05-06T20:37:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nOn Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n>> This simplifies the completions, and makes it easier to define aliases:\n>>\n>>  _GIT_complete gf git_fetch\n>\n> So, 'gf' is an alias for 'git fetch', for which the user would like to\n> use the completion for 'git fetch', right?  But that completion\n> function is caled _git_fetch(), so the underscore prefix is missing\n> here.\n\nNo, it's not missing:\n\n       local name=\"${2-$1}\"\n       eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n       complete -o bashdefault -o default -o nospace -F _${name}_wrap\n$1 2>/dev/null \\\n               || complete -o default -o nospace -F _${name}_wrap $1\n\nSee how we add '_' before $name? There's not point in burdening the\nuser with adding a prefix that will _always_ be there anyway.\n\n> Besides, this example won't work, because the completion for 'git\n> fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n> on finding out the name of the git command from the word on the\n> command line, and it won't be able to do that from 'gf'.\n\nThat's irrelevant, it's an example; replace with another command that\ndoesn't use 'words', and it would work.\n\n> I remember we discussed this in an earlier round, and you even\n> suggested a possible fix (passing the command name as argument to\n> __git_complete_remote_or_refspec()).  I think that's the right thing\n> to do here.\n\nYeah, but I suggested that in order to avoid the eval and the typeset\nthat I require for future future patches, but it turns out it's still\nneeded anyway, so my suggestion is to have a 'cmd' variable that\nstores the command; __git_func_wrap would take the responsibility of\ndoing that.\n\n>> +     __git_func \"$@\"\n>\n> What is this \"$@\" for and why?  None of the _git_<cmd>() functions\n> take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't\n> pass any either.\n\nbash's complete passes 3 arguments. They might not be used, but it\ndoesn't hurt to pass them either.\n\n>> +}\n>> +\n>> +_GIT_complete ()\n>> +{\n>> +     local name=\"${2-$1}\"\n>> +     eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n>\n> Still don't like the subshell and sed here ...\n\nI haven't found any other way.\n\nAs for the fix to 'git fetch' and others, I've attached a crude diff\nof all the fixes I have queued together, and there it works perfectly\nfine, but there's a lot of modifications required to properly traverse\nthe words array. I've meant to create a special function to traverse\nthis array, but I haven't had time time for that; I haven't even\nlanded this patch due to apparently irrelevant discussions.\n\nCheers.\n\n-- \nFelipe Contreras\n\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9f56ec7..ec2c3cc 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -721,10 +721,18 @@ __git_complete_revlist ()\n \t__git_complete_revlist_file\n }\n \n+__git_get_pos ()\n+{\n+\techo $(( t = cmd_pos - 2 + $1 ))\n+}\n+\n __git_complete_remote_or_refspec ()\n {\n-\tlocal cur_=\"$cur\" cmd=\"${words[1]}\"\n-\tlocal i c=2 remote=\"\" pfx=\"\" lhs=1 no_complete_refspec=0\n+\tlocal cur_=\"$cur\"\n+\tlocal i c remote=\"\" pfx=\"\" lhs=1 no_complete_refspec=0\n+\n+\tc=$(__git_get_pos 2)\n+\n \tif [ \"$cmd\" = \"remote\" ]; then\n \t\t((c++))\n \tfi\n@@ -976,7 +984,8 @@ __git_aliased_command ()\n # __git_find_on_cmdline requires 1 argument\n __git_find_on_cmdline ()\n {\n-\tlocal word subcommand c=1\n+\tlocal word subcommand c\n+\tc=$(__git_get_pos 1)\n \twhile [ $c -lt $cword ]; do\n \t\tword=\"${words[c]}\"\n \t\tfor subcommand in $1; do\n@@ -991,7 +1000,8 @@ __git_find_on_cmdline ()\n \n __git_has_doubledash ()\n {\n-\tlocal c=1\n+\tlocal c\n+\tc=$(__git_get_pos 1)\n \twhile [ $c -lt $cword ]; do\n \t\tif [ \"--\" = \"${words[c]}\" ]; then\n \t\t\treturn 0\n@@ -1111,8 +1121,8 @@ _git_bisect ()\n \n _git_branch ()\n {\n-\tlocal i c=1 only_local_ref=\"n\" has_r=\"n\"\n-\n+\tlocal i c only_local_ref=\"n\" has_r=\"n\"\n+\tc=$(__git_get_pos 1)\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n@@ -1142,16 +1152,19 @@ _git_branch ()\n \n _git_bundle ()\n {\n-\tlocal cmd=\"${words[2]}\"\n-\tcase \"$cword\" in\n+\tlocal subcommands='create list-heads verify unbundle'\n+\tlocal subcommand=\"$(__git_find_on_cmdline \"$subcommands\")\"\n+\tlocal r\n+\t(( r = cword - cmd_pos + 2 ))\n+\tcase \"$r\" in\n \t2)\n-\t\t__gitcomp \"create list-heads verify unbundle\"\n+\t\t__gitcomp \"$subcommands\"\n \t\t;;\n \t3)\n \t\t# looking for a file\n \t\t;;\n \t*)\n-\t\tcase \"$cmd\" in\n+\t\tcase \"$subcommand\" in\n \t\t\tcreate)\n \t\t\t\t__git_complete_revlist\n \t\t\t;;\n@@ -1427,6 +1440,7 @@ __git_match_ctag() {\n _git_grep ()\n {\n \t__git_has_doubledash && return\n+\tlocal p=$(__git_get_pos 2)\n \n \tcase \"$cur\" in\n \t--*)\n@@ -1447,7 +1461,7 @@ _git_grep ()\n \tesac\n \n \tcase \"$cword,$prev\" in\n-\t2,*|*,-*)\n+\t$p,*|*,-*)\n \t\tif test -r tags; then\n \t\t\t__gitcomp_nl \"$(__git_match_ctag \"$cur\" tags)\"\n \t\t\treturn\n@@ -2562,7 +2576,8 @@ _git_svn ()\n \n _git_tag ()\n {\n-\tlocal i c=1 f=0\n+\tlocal i c f=0\n+\tc=$(__git_get_pos 1)\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n@@ -2599,39 +2614,24 @@ _git_whatchanged ()\n \t_git_log\n }\n \n-_git ()\n+_main_git ()\n {\n-\tlocal i c=1 command __git_dir\n-\n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n+\tlocal i c=1 cmd cmd_pos __git_dir\n \n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n \twhile [ $c -lt $cword ]; do\n \t\ti=\"${words[c]}\"\n \t\tcase \"$i\" in\n \t\t--git-dir=*) __git_dir=\"${i#--git-dir=}\" ;;\n \t\t--bare)      __git_dir=\".\" ;;\n-\t\t--help) command=\"help\"; break ;;\n+\t\t--help) cmd=\"help\"; break ;;\n \t\t-c) c=$((++c)) ;;\n \t\t-*) ;;\n-\t\t*) command=\"$i\"; break ;;\n+\t\t*) cmd=\"$i\"; break ;;\n \t\tesac\n \t\t((c++))\n \tdone\n \n-\tif [ -z \"$command\" ]; then\n+\tif [ -z \"$cmd\" ]; then\n \t\tcase \"$cur\" in\n \t\t--*)   __gitcomp \"\n \t\t\t--paginate\n@@ -2655,34 +2655,20 @@ _git ()\n \t\treturn\n \tfi\n \n-\tlocal completion_func=\"_git_${command//-/_}\"\n+\t(( cmd_pos = c + 1 ))\n+\n+\tlocal completion_func=\"_git_${cmd//-/_}\"\n \tdeclare -f $completion_func >/dev/null && $completion_func && return\n \n-\tlocal expansion=$(__git_aliased_command \"$command\")\n+\tlocal expansion=$(__git_aliased_command \"$cmd\")\n \tif [ -n \"$expansion\" ]; then\n \t\tcompletion_func=\"_git_${expansion//-/_}\"\n \t\tdeclare -f $completion_func >/dev/null && $completion_func\n \tfi\n }\n \n-_gitk ()\n+_main_gitk ()\n {\n-\tif [[ -n ${ZSH_VERSION-} ]]; then\n-\t\temulate -L bash\n-\t\tsetopt KSH_TYPESET\n-\n-\t\t# workaround zsh's bug that leaves 'words' as a special\n-\t\t# variable in versions < 4.3.12\n-\t\ttypeset -h words\n-\n-\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n-\t\t# array if IFS doesn't contain spaces.\n-\t\ttypeset -h IFS\n-\tfi\n-\n-\tlocal cur words cword prev\n-\t_get_comp_words_by_ref -n =: cur words cword prev\n-\n \t__git_has_doubledash && return\n \n \tlocal g=\"$(__gitdir)\"\n@@ -2703,16 +2689,42 @@ _gitk ()\n \t__git_complete_revlist\n }\n \n-complete -o bashdefault -o default -o nospace -F _git git 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git\n-complete -o bashdefault -o default -o nospace -F _gitk gitk 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _gitk gitk\n+foo_wrap ()\n+{\n+\tlocal cmd=foo_cmd cmd_pos=1\n+\tif [[ -n ${ZSH_VERSION-} ]]; then\n+\t\temulate -L bash\n+\t\tsetopt KSH_TYPESET\n+\n+\t\t# workaround zsh's bug that leaves 'words' as a special\n+\t\t# variable in versions < 4.3.12\n+\t\ttypeset -h words\n+\n+\t\t# workaround zsh's bug that quotes spaces in the COMPREPLY\n+\t\t# array if IFS doesn't contain spaces.\n+\t\ttypeset -h IFS\n+\tfi\n+\tlocal cur words cword prev\n+\t_get_comp_words_by_ref -n =: cur words cword prev\n+\tfoo \"$@\"\n+}\n+\n+git_complete ()\n+{\n+\tlocal name=\"${2-main_$1}\"\n+\tlocal cmd=\"${name#git_}\"\n+\teval \"$(typeset -f foo_wrap | sed -e \"s/foo/_$name/\" -e \"s/foo_cmd/$cmd/\")\"\n+\tcomplete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n+\t\t|| complete -o default -o nospace -F _${name}_wrap $1\n+}\n+\n+git_complete git\n+git_complete gitk\n \n # The following are necessary only for Cygwin, and only are needed\n # when the user has tab-completed the executable name and consequently\n # included the '.exe' suffix.\n #\n if [ Cygwin = \"$(uname -o 2>/dev/null)\" ]; then\n-complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \\\n-\t|| complete -o default -o nospace -F _git git.exe\n+git_complete git.exe main_git\n fi\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex cc12732..2966a18 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -63,7 +63,7 @@ run_completion ()\n \tlocal _cword\n \t_words=( $1 )\n \t(( _cword = ${#_words[@]} - 1 ))\n-\t_git && print_comp\n+\t${comp_wrap-_main_git_wrap} && print_comp\n }\n \n test_completion ()\n@@ -73,6 +73,22 @@ test_completion ()\n \ttest_cmp expected out\n }\n \n+setup_repository ()\n+{\n+\tmkdir \"$1\" && (\n+\tcd \"$1\" &&\n+\tgit init &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"Initial\"\n+\t)\n+}\n+\n+test_expect_success 'prepare' '\n+\tsetup_repository one &&\n+\tgit clone one test &&\n+\tcd test\n+'\n+\n test_expect_success 'basic' '\n \trun_completion \"git \\\"\\\"\" &&\n \t# built-in\n@@ -155,4 +171,71 @@ test_expect_success 'general options plus command' '\n \ttest_completion \"git --no-replace-objects check\" \"checkout \"\n '\n \n+test_expect_success 'remote or refspec' '\n+\ttest_completion \"git fetch o\" \"origin \" &&\n+\ttest_completion \"git fetch origin m\" \"master:master \" &&\n+\ttest_completion \"git pull o\" \"origin \" &&\n+\ttest_completion \"git pull origin m\" \"master \" &&\n+\ttest_completion \"git push o\" \"origin \" &&\n+\ttest_completion \"git push origin m\" \"master \"\n+'\n+\n+test_expect_success 'subcomands' '\n+\ttest_completion \"git bisect st\" \"start \"\n+'\n+\n+test_expect_success 'has double dash' '\n+\ttest_completion \"git add -- foo\" \"\"\n+'\n+\n+test_expect_success 'config' '\n+\tgit config --file=foo color.ui auto &&\n+\ttest_completion \"git config --file=foo --get c\" \"color.ui \"\n+'\n+\n+test_expect_success 'other' '\n+\tcat >expected <<-\\EOF &&\n+\torigin/HEAD \n+\torigin/master \n+\tEOF\n+\ttest_completion \"git branch -r o\" &&\n+\ttest_completion \"git bundle cr\" \"create \" &&\n+\n+\techo foobar > tags &&\n+\ttest_completion \"git grep f\" \"foobar \" &&\n+\n+\ttest_completion \"git notes --ref m\" \"master \" &&\n+\n+\tgit tag v0.1 &&\n+\ttest_completion \"git tag -d v\" \"v0.1 \"\n+'\n+\n+test_expect_success 'global options extra checks' '\n+\ttest_completion \"git --no-pager fetch o\" \"origin \" &&\n+\ttest_completion \"git --no-pager fetch origin m\" \"master:master \" &&\n+\ttest_completion \"git --no-pager pull o\" \"origin \" &&\n+\ttest_completion \"git --no-pager pull origin m\" \"master \" &&\n+\ttest_completion \"git --no-pager push o\" \"origin \" &&\n+\ttest_completion \"git --no-pager push origin m\" \"master \" &&\n+\ttest_completion \"git --no-pager bisect st\" \"start \" &&\n+\ttest_completion \"git --no-pager add -- foo\" \"\" &&\n+\ttest_completion \"git --no-pager config --file=foo --get c\" \"color.ui \" &&\n+\tcat >expected <<-\\EOF &&\n+\torigin/HEAD \n+\torigin/master \n+\tEOF\n+\ttest_completion \"git --no-pager branch -r o\" &&\n+\ttest_completion \"git --no-pager bundle cr\" \"create \" &&\n+\ttest_completion \"git --no-pager grep f\" \"foobar \" &&\n+\ttest_completion \"git --no-pager notes --ref m\" \"master \" &&\n+\ttest_completion \"git --no-pager tag -d v\" \"v0.1 \"\n+'\n+\n+test_expect_success 'aliases' '\n+\tlocal comp_wrap=_git_fetch_wrap &&\n+\tgit_complete gf git_fetch &&\n+\ttest_completion \"gf o\" \"origin \" &&\n+\ttest_completion \"gf origin m\" \"master:master \"\n+'\n+\n test_done\n"},{"id":"190937","messageId":"20120506204721.GD3063@burratino","threadId":"30434","inReplyTo":"20120506103006.GI2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-06T20:47:21Z","receivedAt":"2012-05-06T20:47:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n\n> It's a configuration thing, so it will be used in .bashrc; I think\n> it's quite unlikely that it will be used interactively.\n\nYeah, makes sense.  So naming the public functions _GIT_complete\nand _GIT_ps1 sounds reasonable.\n\nSorry, usually this kind of discussion would be easy but there is an\nelement of the human interaction I haven't learned to handle well yet.\nThanks for your help.\n\nJonathan\n"},{"id":"190939","messageId":"CAMP44s1bvez99p=hBreB4xDy9hRmwomYx8keae5Cj-mywdxjDQ@mail.gmail.com","threadId":"30434","inReplyTo":"20120506121204.GA5799@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-06T20:53:24Z","receivedAt":"2012-05-06T20:53:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, May 6, 2012 at 2:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> Hi,\n>\n>\n> On Sun, May 06, 2012 at 01:14:25PM +0200, SZEDER Gábor wrote:\n>> On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n>> > +__git_func_wrap ()\n>> > +{\n>> > +   if [[ -n ${ZSH_VERSION-} ]]; then\n>> > +           emulate -L bash\n>> > +           setopt KSH_TYPESET\n>> > +\n>> > +           # workaround zsh's bug that leaves 'words' as a special\n>> > +           # variable in versions < 4.3.12\n>> > +           typeset -h words\n>> > +\n>> > +           # workaround zsh's bug that quotes spaces in the COMPREPLY\n>> > +           # array if IFS doesn't contain spaces.\n>> > +           typeset -h IFS\n>> > +   fi\n>> > +   local cur words cword prev\n>> > +   _get_comp_words_by_ref -n =: cur words cword prev\n>> > +   __git_func \"$@\"\n>> > +}\n>> > +\n>> > +_GIT_complete ()\n>> > +{\n>> > +   local name=\"${2-$1}\"\n>> > +   eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n>>\n>> Still don't like the subshell and sed here ...\n>>\n>> > +   complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n>> > +           || complete -o default -o nospace -F _${name}_wrap $1\n>> > +}\n>> > +\n>> > +_GIT_complete git\n>> > +_GIT_complete gitk\n>>\n>> ... because it adds delay when the completion script is loaded.  But I\n>> still don't have ideas how to avoid them.\n>\n> Ok, I think I got it.  How about this on top of Felipe's patch?\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index f300b87d..8c18db92 100755\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2688,19 +2688,19 @@ __git_func_wrap ()\n>        fi\n>        local cur words cword prev\n>        _get_comp_words_by_ref -n =: cur words cword prev\n> -       __git_func \"$@\"\n> +       $1\n>  }\n>\n>  _GIT_complete ()\n>  {\n> -       local name=\"${2-$1}\"\n> -       eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n> -       complete -o bashdefault -o default -o nospace -F _${name}_wrap $1 2>/dev/null \\\n> -               || complete -o default -o nospace -F _${name}_wrap $1\n> +       local wrapper=\"__git_wrap_$1\"\n> +       eval \"$wrapper () { __git_func_wrap $2 ; }\"\n> +       complete -o bashdefault -o default -o nospace -F $wrapper $1 2>/dev/null \\\n> +               || complete -o default -o nospace -F $wrapper $1\n>  }\n\nThis has unnecessary changes, and can be simplified this way:\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex f300b87..dd1ff33 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2688,13 +2688,13 @@ __git_func_wrap ()\n        fi\n        local cur words cword prev\n        _get_comp_words_by_ref -n =: cur words cword prev\n-       __git_func \"$@\"\n+       _$1\n }\n\n _GIT_complete ()\n {\n        local name=\"${2-$1}\"\n-       eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n+       eval \"_${name}_wrap () { __git_func_wrap $name ; }\"\n        complete -o bashdefault -o default -o nospace -F _${name}_wrap\n$1 2>/dev/null \\\n                || complete -o default -o nospace -F _${name}_wrap $1\n }\n\nOf course, it might make sense to use the $wrapper variable, but that\nincreases the diff, so I avoided it for reviewing purposes. And I\nstill see no point forcing users to specify the full name of the\nfunction; they should not be bothered with such details.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"190948","messageId":"20120506233235.GN2164@goldbirke","threadId":"30434","inReplyTo":"CAMP44s3xwgsfjZA6r+bydu-5r1nEO5cGc=wcDSr+WdwunTtpwg@mail.gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-06T23:32:35Z","receivedAt":"2012-05-06T23:32:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n\nOn Sun, May 06, 2012 at 10:37:06PM +0200, Felipe Contreras wrote:\n> On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n> >> This simplifies the completions, and makes it easier to define aliases:\n> >>\n> >>  _GIT_complete gf git_fetch\n> >\n> > So, 'gf' is an alias for 'git fetch', for which the user would like to\n> > use the completion for 'git fetch', right?  But that completion\n> > function is caled _git_fetch(), so the underscore prefix is missing\n> > here.\n> \n> No, it's not missing:\n> \n>        local name=\"${2-$1}\"\n>        eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n>        complete -o bashdefault -o default -o nospace -F _${name}_wrap\n> $1 2>/dev/null \\\n>                || complete -o default -o nospace -F _${name}_wrap $1\n> \n> See how we add '_' before $name?\n\nIndeed, the '_' is added before $name no less than three times.  How\ncould I have missed that?! ;)  It would be better to do it once and be\ndone with it.\n\n> There's not point in burdening the\n> user with adding a prefix that will _always_ be there anyway.\n\nI don't think it's that much of a burden.  The function is called\n_git_fetch, use that as second argument\n\n> > Besides, this example won't work, because the completion for 'git\n> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n> > on finding out the name of the git command from the word on the\n> > command line, and it won't be able to do that from 'gf'.\n> \n> That's irrelevant, it's an example; \n\nIt's relevant; if you give an example, then at least that example\nshould work properly, don't you think?\n\n> replace with another command that\n> doesn't use 'words', and it would work.\n\nThat it doesn't work has nothing to do with $words.  The problem is that\n__git_complete_remote_or_refspec() expects to find the git command in\n${words[1]}, but in case of an alias it can't.\n\n> > I remember we discussed this in an earlier round, and you even\n> > suggested a possible fix (passing the command name as argument to\n> > __git_complete_remote_or_refspec()).  I think that's the right thing\n> > to do here.\n> \n> Yeah, but I suggested that in order to avoid the eval and the typeset\n> that I require for future future patches, but it turns out it's still\n> needed anyway, so my suggestion is to have a 'cmd' variable that\n> stores the command; __git_func_wrap would take the responsibility of\n> doing that.\n\nWell, now I suggest to do that to fix\n__git_complete_remote_or_refspec(), because that seems to be the\neasiest, cleanest, and fastest solution.\n\n> >> +     __git_func \"$@\"\n> >\n> > What is this \"$@\" for and why?  None of the _git_<cmd>() functions\n> > take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't\n> > pass any either.\n> \n> bash's complete passes 3 arguments.\n\nOh, indeed; the first argument is the command name, the second is the\ncurrent word, and the third is the previous word.  All these are\navailable in our completion functions as ${words[0]}, $cur, and $prev,\nrespectively.\n\n> They might not be used, but it\n> doesn't hurt to pass them either.\n\nThey _are_ not used, so passing them has no benefit either.  I would\nrather stick to using $cur and $prev than $2 and $3.\n\n\nBest,\nGábor\n"},{"id":"190952","messageId":"CAMP44s3q-X7W5qeu6LZzu13C6SfhEZy9i4AfJ9Sszcou52MsGw@mail.gmail.com","threadId":"30434","inReplyTo":"20120506233235.GN2164@goldbirke","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-07T00:20:54Z","receivedAt":"2012-05-07T00:20:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nOn Mon, May 7, 2012 at 1:32 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> On Sun, May 06, 2012 at 10:37:06PM +0200, Felipe Contreras wrote:\n>> On Sun, May 6, 2012 at 1:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>> > On Sat, May 05, 2012 at 05:23:20PM +0200, Felipe Contreras wrote:\n>> >> This simplifies the completions, and makes it easier to define aliases:\n>> >>\n>> >>  _GIT_complete gf git_fetch\n>> >\n>> > So, 'gf' is an alias for 'git fetch', for which the user would like to\n>> > use the completion for 'git fetch', right?  But that completion\n>> > function is caled _git_fetch(), so the underscore prefix is missing\n>> > here.\n>>\n>> No, it's not missing:\n>>\n>>        local name=\"${2-$1}\"\n>>        eval \"$(typeset -f __git_func_wrap | sed -e \"s/__git_func/_$name/\")\"\n>>        complete -o bashdefault -o default -o nospace -F _${name}_wrap\n>> $1 2>/dev/null \\\n>>                || complete -o default -o nospace -F _${name}_wrap $1\n>>\n>> See how we add '_' before $name?\n>\n> Indeed, the '_' is added before $name no less than three times.  How\n> could I have missed that?! ;)  It would be better to do it once and be\n> done with it.\n\nSure.\n\n>> There's not point in burdening the\n>> user with adding a prefix that will _always_ be there anyway.\n>\n> I don't think it's that much of a burden.  The function is called\n> _git_fetch, use that as second argument\n\nPerhaps not, but why are you arguing for making users' life more\ndifficult? Even if it's just a little.\n\nIn fact, it could be even simpler:\n\n_GIT_complete gf fetch\n\n>> > Besides, this example won't work, because the completion for 'git\n>> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n>> > on finding out the name of the git command from the word on the\n>> > command line, and it won't be able to do that from 'gf'.\n>>\n>> That's irrelevant, it's an example;\n>\n> It's relevant; if you give an example, then at least that example\n> should work properly, don't you think?\n\nIt does work... on my branch.\n\nIf you have another example that works, feel free to suggest it, but\nI'm going to remove that message, along with _GIT_complete (and\nreplace it with __git_complete and a note that it's not public API),\nbecause I'm tired of trying to make users' life easier; I just want\nthe patch in.\n\n>> replace with another command that\n>> doesn't use 'words', and it would work.\n>\n> That it doesn't work has nothing to do with $words.  The problem is that\n> __git_complete_remote_or_refspec() expects to find the git command in\n> ${words[1]}, but in case of an alias it can't.\n\n${words[1]} is part of $words. And BTW, git bundle also fails similarly.\n\nAnd then, even if you fetch the command properly,\n__git_complete_remote_or_refspec will still fail because it would\nassume the remote is 'git'.\n\nAlso BTW, git fetch is already broken anyway:\n\n git --no-pager fetch <TAB>\n\nSo don't blame my patch :)\n\n>> > I remember we discussed this in an earlier round, and you even\n>> > suggested a possible fix (passing the command name as argument to\n>> > __git_complete_remote_or_refspec()).  I think that's the right thing\n>> > to do here.\n>>\n>> Yeah, but I suggested that in order to avoid the eval and the typeset\n>> that I require for future future patches, but it turns out it's still\n>> needed anyway, so my suggestion is to have a 'cmd' variable that\n>> stores the command; __git_func_wrap would take the responsibility of\n>> doing that.\n>\n> Well, now I suggest to do that to fix\n> __git_complete_remote_or_refspec(), because that seems to be the\n> easiest, cleanest, and fastest solution.\n\nThat's not enough to make 'git fetch' work, try it.\n\n>> >> +     __git_func \"$@\"\n>> >\n>> > What is this \"$@\" for and why?  None of the _git_<cmd>() functions\n>> > take any arguments, nor does _git() and _gitk(), and AFAICT Bash won't\n>> > pass any either.\n>>\n>> bash's complete passes 3 arguments.\n>\n> Oh, indeed; the first argument is the command name, the second is the\n> current word, and the third is the previous word.  All these are\n> available in our completion functions as ${words[0]}, $cur, and $prev,\n> respectively.\n\nYeah, but that's still what bash does, and I saw no reason to change it.\n\n>> They might not be used, but it\n>> doesn't hurt to pass them either.\n>\n> They _are_ not used, so passing them has no benefit either.  I would\n> rather stick to using $cur and $prev than $2 and $3.\n\nSure, but it was just 4 more characters that didn't hurt anybody. Your\nversion makes passing those arguments more difficult, so I see no need\nto try to implement that.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"190973","messageId":"20120507092213.GO2164@goldbirke","threadId":"30434","inReplyTo":"CAMP44s3q-X7W5qeu6LZzu13C6SfhEZy9i4AfJ9Sszcou52MsGw@mail.gmail.com","subject":"Re: [PATCH v3] completion: add new _GIT_complete helper","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-05-07T09:22:13Z","receivedAt":"2012-05-07T09:22:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n\nOn Mon, May 07, 2012 at 02:20:54AM +0200, Felipe Contreras wrote:\n> In fact, it could be even simpler:\n> \n> _GIT_complete gf fetch\n\nThat would be even better; just need to take care of 'cherry-pick' and\nthe like.\n\n> >> > Besides, this example won't work, because the completion for 'git\n> >> > fetch' uses __git_complete_remote_or_refspec(), which in turn relies\n> >> > on finding out the name of the git command from the word on the\n> >> > command line, and it won't be able to do that from 'gf'.\n\n> It does work... on my branch.\n\nI though the patch was supposed to be applied on git.git's master.  I\ndidn't know what else you had on your branch, obviously.\n\n> If you have another example that works, feel free to suggest it\n\nI played around with 'gb' as 'git branch' and 'gc' as 'git checkout',\nthey seemed to work properly, and they both use $words directly or\nindirectly.  I suspect most of them Just Works, except those using\n__git_complete_remote_or_refspec(), and _git_bundle() as you mention\nbelow, and perhaps a few surprises we hadn't spotted yet.\n\n> >> replace with another command that\n> >> doesn't use 'words', and it would work.\n> >\n> > That it doesn't work has nothing to do with $words.  The problem is that\n> > __git_complete_remote_or_refspec() expects to find the git command in\n> > ${words[1]}, but in case of an alias it can't.\n> \n> ${words[1]} is part of $words.\n\nThe problem is not with $words per se, but with the '1'.\n\n> And BTW, git bundle also fails similarly.\n\nIndeed; using __git_find_on_cmdline() and checking which subcommand it\nfound, if any, would allow us to get rid of all numbers from that\nfunction.\n\n> Also BTW, git fetch is already broken anyway:\n> \n>  git --no-pager fetch <TAB>\n> \n> So don't blame my patch :)\n\nYeah, I know, but that's broken since \"forever\".  However, I think\nthat would be a separate, though related topic, because it's not\nrequired to make completion for alias work.\n\n> >> > I remember we discussed this in an earlier round, and you even\n> >> > suggested a possible fix (passing the command name as argument to\n> >> > __git_complete_remote_or_refspec()).  I think that's the right thing\n> >> > to do here.\n\n> That's not enough to make 'git fetch' work, try it.\n\nAs already mentioned it earlier in this thread, the iteration over\n$words in __git_complete_remote_or_refspec() have to be fixed, too.\n\n> >> >> +     __git_func \"$@\"\n\n> Sure, but it was just 4 more characters that didn't hurt anybody. Your\n> version makes passing those arguments more difficult, so I see no need\n> to try to implement that.\n\nOK.  I doesn't hurt, but I think it's more important to avoid\nfork()+exec() overheads than to pass arguments whose values are\nreadily available in other variables anyway.\n\n\nGábor\n"},{"id":"191485","messageId":"CAMP44s2m07PQJnW-c+LPrm80H4SyQV1G=pmGstc5+UG1g8W0xw@mail.gmail.com","threadId":"30434","inReplyTo":"CALUzUxp-4ogm-qmtoyHLicE7eD6Ut8m_hQjCMgXvxzbi7YPhQQ@mail.gmail.com","subject":"Re: Jonathan gives feedback --> flamewars inevitable? (Re: [PATCH v3] completion: add new _GIT_complete helper)","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-05-14T09:11:32Z","receivedAt":"2012-05-14T09:11:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, May 6, 2012 at 7:23 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:\n> On Sun, May 6, 2012 at 1:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Felipe Contreras wrote:\n>>\n>>> And I'd like to think that when you filibuster a discussion there's a\n>>> good reason for it.\n>>\n>> Kind other people on the list, please enlighten me.  What did I say to\n>> trigger this crap?\n>\n> Felipe, Jonathan is and continues to be one of the kindest, diligent\n> reviewers on this list. The accusation is uncalled for.\n\nIt's not an accusation; Jonathan's comments did prevent the patches to\nmove forward, which is a good thing if there's a good reason for that.\nThat's the whole point of review.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}