Re: [PATCH 5/5] completion: fix completion of certain aliases

Junio C Hamano wrote:

Gábor Szeder [off-list ref] writes:

quoted

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.

quoted

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,...

quoted

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