Re: Bug + patch: _expand() fails to insert unambiguous prefix

[Oliver Kiddle <opk@xxxxxxx>]

Lawrence Velázquez wrote:
> Sorry, that was more to elicit a review from a dev, but you're welcome to expand the patch of course.

_expand takes a deliberate approach of setting
  compstate[insert]=menu

That forces menu completion and was a conscious design decision. This
tends to be applicable for completions that aren't taking an incomplete
prefix and adding characters but are transforming what was entered.

It is reasonable that you might prefer different behaviour here so I'm
not averse to enabling it via a style. A simple pattern like a plain *
makes for a rather contrived example. With a more realistic pattern,
the case is less clear. I've always used just the all-expansions tag
with _expand. I don't see why you'd want completion to turn a carefully
written pattern into two characters of common prefix.

The trouble is that this patch has needed to change _main_complete, so
it results in a change in the behaviour of any other completion function
that sets compstate[insert]. In quite a few cases, _pids for instance, I
don't think the change is an improvement.

The relevant part of _main_complete checks whether compstate[insert]
has been changed by the function and applies the values from styles and
defaults if not. The patch continues regardless of the compstate[insert]
change. In general, it is good if the user can override defaults but
this is changing the default behaviour.

The menu style does get looked up with the context
:completion::expand:::expansions but _expand setting compstate[insert]
overrides it. Ideally, the precedence order for preferences should be:

1 User style configured for a specific context (:completion:*:expand:*:expansions)
2 Specific default set explicitly by a completion function, _expand in this case
3 User style configured for the general context (menu style default tag in this case)
4 A general default

Your patch is throwing away (2) in a limited manner. The old code
gave (2) precedence over (1). Getting it right is not simple, though
- consider also the fact that the menu style is not really meant
to control the unambiguous aspect of compstate[insert]. But I think
we can do better while preserving existing behaviour.

For what it's worth, I'm also not keen on the patch leaving a block with
the wrong indentation level (apart from a final fi and preceding line).
It kept the patch short but makes it harder to follow the logic.

Oliver