Junio C Hamano wrote: > Gábor Szeder <szeder@xxxxxxxxxx> writes: > > > words[] is just fine, we never modify it after it is filled by > > _get_comp_words_by_ref() at the very beginning. > > Hmph. I would have understood if the latter were "we never look at > it (to decide what to do)". "we never modify it" does not sound > like an enough justification behind "words[] is just fine"---maybe I > am not reading you correctly. > > > The root of the problem is that the expected position of the name > > of the git command in __git_complete_remote_or_refspec() is > > hardcoded as words[1], but that is not the case when: > > > > 1) it's an alias, as in Felipe's example: git p ori<TAB>, > > because while the index is ok, the content is not. > > > > 2) in presence of options of the main git command: git -c > > foo=bar push ori<TAB>, because the index is off. > > > > 3) the command is a shell alias for which the user explicitly > > set the completion function with __git_complete() (at his own > > risk): alias gp="git push"; __git_complete gp _git_push; gp > > ori<TAB> Neither the index nor the content are ok. > > > > Fixing the hard-coded indexing would only solve 2) but not 1) and > > 3), as it obviously couldn't turn the git or shell alias into a > > git command on its own. > > > > Felipe's patch only deals with 1), as it only kicks in in case of > > a git alias. Which is the far more common use-case, and the problem I've seen people report multiple times in multiple media. > Yeah, do completions for commands (not just for the ones that use > remote-or-refspec Felipe's patch addresses) have trouble with the > latter two in general? If that is the case,... > > > Communicating the name of the git command to > > __git_complete_remote_or_refspec() by its callers via a new > > variable as suggested by Junio, or perhaps by an additional > > parameter to the function is IMHO the right thing to do, because, > > unless I'm missing something, it would make all three cases work. > > ... while the above analysis may be correct, taking Felipe's patch > to address only (1) and leaving a solution to the more general > words[1] problem for other patches on top might not be too bad an > approach. > > Unless > > (A) remote-or-refspec thing is the primary offender, and other > commands do not suffer from the words[1] problem, in which case > I tend to agree that an additional parameter would be the way > to go (there are only a few callers of the function); or Since I already fixed the problem (all 3) years ago[1], you can take a look at the patch and see which commands which use a hard-coded index value might have real issues. My guess is that it's more than just remote-or-refspec, but I don't have the incentive to look deeper into this (the issue is solved for me). > (B) even if words[1] problem is more widespread, such a more > general solution to all three issues can be coded cleanly and > quickly, without having to have Felipe's patch as a stop-gap > measure. I already sent two patches, one that solves all the problems, and one that solves the most important one. I would gladly revisit my old patch and see which of those changes are really necessary and solve a real issue, *if* I knew the resulting patch wouldn't get the same road-blockers as the old one did; namely being held to higher standards than the current code. I say it's more important to fix the real issues real people have than hold on to arbitrary standards which might force the bug to remain present for years just because some variable was not documented in the original patch (just like all other variables in the script)... But that's just me. [1] http://thread.gmane.org/gmane.comp.version-control.git/197226 -- Felipe Contreras-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html