scop / bash-completion

Programmable completion functions for bash
GNU General Public License v2.0
2.95k stars 381 forks source link

Feature request: generalized implementation of `_comp_command_offset` #1234

Closed marcxjo closed 4 months ago

marcxjo commented 4 months ago

Describe the feature/solution

This is more a statement of intent/request for comments than a solicitation of developer effort. I am happy to work towards completing this addition if there's interest to merge it; just putting out feelers to determine whether that would be the case.

Full disclosure: while I propose and would intend upon re-using and extending the implementation of _comp_command_offset to achieve this add, I have not yet reviewed it closely enough to be 100% sure that its code could be adapted for this use case. Hence, the cheeky remark about near-zero maintenance burden offset below may not be totally accurate.

I have several scripts that wrap existing commands while providing additional positional args or options before forwarding remaining args to the "original" wrapped command.

Here's an example command line utilizing my git-profile-extension:

git profile work clone https://path.to/user/repo.git

The positional argv profile work simply sets the environment variable GIT_CONFIG_GLOBAL such that the executed command is equivalent to

GIT_CONFIG_GLOBAL=/path/to/profiles/**work**/config git clone https://path.to/user/repo.git

As far as I'm aware, the standard approach to enable completion for this kind of command wrapper (or at least the one that I implement) when handling a wrapper command with n wrapper-specific words, involves locally overwriting the command-like tracking environment variables according to an algorithm like the following when:

Using this algorithm, the command line is reconstructed such that the entire deleted subarray of words, with all of its flanking spaces, is replaced with a single space, and the word and cursor index positions are recalculated accordingly.

These locally overwritten values are then forwarded on to the original, wrapped command's completion routine, essentially tricking the function into thinking the user has entered a command line containing no words specific to the command wrapper.

I believe this logic can be easily encapsulated in a function which generalizes the logic of _comp_command_offset. I am suggesting a function with the following contract (with names here used for illustrative purposes, subject to adaptation to codebase standards):

_comp_command_spliced() {
  local -ri _start_idx=$1
  local -ri _offset=$2

  # Magic happens here
}

where $_start_idx is the index of the first word to be deleted from the forwarded compline, and $_offset is the total number of successive words to be deleted.

If accepted, this addition also comes with the advantage that _comp_command_offset can be refactored to something as simple (or at least very nearly as simple) as

_comp_command_offset() {
   local -ri _offset=$1

  _comp_command_spliced 0 $_offset
}

bringing the net offset (pun intended 😉) of maintenance burden to near-zero.

Maintenance (please complete the following information)

This is not a request for a new command completion, but rather an extension of bash-completion's capabilities for use across completion routines.

I am more than happy to put forth the effort to merge this work if interest to merge is sufficient, and to maintain this work insofar as possible. I hesitate to offer an ETA to completion but currently have the bandwidth to work on this in earnest. I am able to ensure full support for Linux systems but will likely require assistance for additional operating systems if the logic of this work turns out to contain any system-specific considerations (assuming this addition does not turn out to amount to a simple refactor of _comp_command_offset in practice).

I am also happy to accept all feedback on the approach described above and suspect it is imperfect as described.

Finally, I am certainly happy to be corrected on my observation that the need as described is not already solved-for; I see no such function available in the current state of the library (and I've looked many times 😉), but this could absolutely be an oversight on my part.

akinomyoga commented 4 months ago

I'm not sure if I understand the request correctly, but wouldn't that be achievable just by calling _comp_command_offset 0 after modifying COMP_* as you like?

marcxjo commented 4 months ago

The problem is that _comp_command_offset handles the case of a command where ${@:n} represents the wrapped command to be completed. My case is something more like (${0} ${@:n}), with ${@:1:n-1} removed.

The intent of _comp_command_spliced would be specifically to preclude having to modify the COMP_* vars inline in completions where this more nuanced need applies, with the flexibility of configurable start and offset indexes, allowing the user to easily assemble the forwarded command line from arbitrary partitions (e.g., (${0:3} ${@:7})).

marcxjo commented 4 months ago

I can see an argument that a fully configurable start index may not prove valuable, and to that end I'd be more than happy with a compromise to the effect of _comp_command_wrapped, which could work similarly to _comp_command_offset without removing $0 from the modified command line.

This would be sufficient to enable easy extension of established commands like git. I use git as the main example here because its completion capabilities are specifically designed to accommodate such a use case based on the extension command's filename, but it does not provide convenience functions to facilitate re-assembling the forwarded command line like I'm suggesting here.

akinomyoga commented 4 months ago

So, it's achievable by modifying COMP_* and calling _comp_command_offset 0, but you just wanted to preclude the code from the completion of your command but include the code in bash-completion?

The suggested _comp_command_spliced seems too specific to the completion of your command. The original behavior of _comp_command_offset can be used for many commands. In fact, many completions for commands provided by bash-completion use _comp_command_offset. Compared to that, the suggested behavior doesn't seem a common pattern. If bash-completion starts to cover such minor cases, there are actually many different possibilities for modifying COMP_*. For example, when one wants to skip the second and fourth words (e.g. cmd 1 2 3 4 5 -> cmd 2 4 5), _comp_command_sliced doesn't work. When one wants to add an option after the command name (e.g. cmd a b -> cmd -i a b), the suggested _comp_command_sliced doesn't work. Letting the caller modify COMP_* seems a general and clear solution. In the present case, I think the related code of modifying COMP_* should still be maintained in the completion setting for the command that uses the feature.


How would the second suggestion _comp_command_wrapped be different from _comp_command_offset? It seems to me that calling _comp_command_offset without arguments would already behave in the way you expect by _comp_command_wrapped. It doesn't remove the first word ${COMP_WORDS[0]}.

marcxjo commented 4 months ago

It doesn't remove the first word ${COMP_WORDS[0]}.

It does not remove $0 from the interpreted command, but locally and for completion purposes, removing ${COMP_WORDS[0]} appears to be the specific mechanism in use.

In retrospect, I haven't picked my terminology well, because I'm failing to make a distinction between command wrapping and command extension.

My understanding of _comp_command_offset is that it specifically addresses cases in which $0 is a command that wraps a subsequent command $n for some n; examples include env and sudo, both of which utilize _comp_command_offset to achieve completion of the "inner" command. In the example cases of sudo pacman -S <package> and env HOME=/nowhere firefox -no-remote -P <profile>, completion for the respective pacman and firefox commands on these two command lines, is handled by invoking _comp_command_offset, effectively shifting away the words to the left of pacman and firefox for completion purposes.

The use case that I'm attempting to solve for, is for invocations such that the command line contains no "subsequent command $n," but rather one where functionality of $0 can be extended by custom code and commands picked up by its completion handling. In the case of my Git extensions, the command line is rendered such that "native" Git command-line completion should be available after a subarray ${@:1:n} for some n. In the example I provide in my first post

git profile work clone https://path.to/user/repo.git

The words profile and work are specifically handled by my extension git-profile, but all elements of the command line git clone https://path.to/user/repo.git should itself be handled by Git completion.

I can perhaps utilize a combination of _comp_command_offset and re-inserting the word git into COMP_* to accomplish this. This is a less general and more error-prone approach from my point of view - my hope was to have a utility function available at the library level to reliably and repeatably facilitate this use case.

As I type this out, though, I realize that even this more nuanced use case is potentially still too specialized to be generally useful. Admittedly, the only other project I'm aware of that allows for extension-driven completion in a fashion similar to Git, is pass, and while I have also developed similar extensions for pass, I'm beginning to understand that my use case seems to simply work around limitations of the completion API in both settings. Neither seems innately designed to accommodate extensions which pass back into existing subcommands.

I'll leave this open in case there is interest to host functionality of this kind, but I acknowledge the specificity of the need and I'll consider implementing it as a separate project if closed.

marcxjo commented 4 months ago

For example, when one wants to skip the second and fourth words (e.g. cmd 1 2 3 4 5 -> cmd 2 4 5), _comp_command_sliced doesn't work. When one wants to add an option after the command name (e.g. cmd a b -> cmd -i a b), the suggested _comp_commandsliced doesn't work. Letting the caller modify COMP* seems a general and clear solution.

Fwiw I agree with this, and I tried to toe the line between flexibility and excessive granularity with my proposal. I guess an ideal least-common-denominator, then (from my point of view), would be a simple function that either could be called multiple times in succession or, better yet, easily extended to handle cases of multiple words.

To that end, I'd be extremely happy to see a _comp_word_remove to delete a single compword and adjust COMP_* accordingly. Not sure if there are edge cases I'm neglecting, but in my head the effect would be to replace the word and all flanking whitespace with a single whitespace, with all COMP_* vars modulated accordingly.

This I would imagine to be more widely useful, but this is certainly simple enough to implement and maintain separately, and to build off of for my own needs.

akinomyoga commented 4 months ago

It doesn't remove the first word ${COMP_WORDS[0]}.

It does not remove $0 from the interpreted command, but locally and for completion purposes, removing ${COMP_WORDS[0]} appears to be the specific mechanism in use.

No, _comp_command_offset doesn't remove ${COMP_WORDS[0]} when you specify no argument or specify an argument 0.

In retrospect, [...]

[...]

The words profile and work are specifically handled by my extension git-profile, but all elements of the command line git clone https://path.to/user/repo.git should itself be handled by Git completion.

These seem just a repetition of the use case you already explained twice. I don't seem to be able to find a new argument providing the reason for not using _comp_command_offset.

I can perhaps utilize a combination of _comp_command_offset and re-inserting the word git into COMP_* to accomplish this.

You don't need to re-insert the word git into COMP_*. _comp_command_offset 0 doesn't remove ${COMP_WORDS[0]}. When 0 is passed, it doesn't modify any COMP_*.

Did you actually check the behavior of _comp_command_offset 0, which I'm suggesting from the beginning?

To that end, I'd be extremely happy to see a _comp_word_remove to delete a single compword and adjust COMP_* accordingly.

I don't think it deserves a utility function maintained in bash-completion.

First of all, bash-completion doesn't directly use COMP_WORDS, but it re-assembles the command line into local variables, words, cword, etc., and then work on them. Even if a completion under the bash-completion convention would try to manipulate the command line in the future, it wouldn't do that based on the positions in COMP_WORDS. So the suggested function based on COMP_WORDS won't be used by the completions under the bash-completion convention.

marcxjo commented 4 months ago

First of all, bash-completion doesn't directly use COMP_WORDS, but it re-assembles the command line into local variables, words, cword, etc., and then work on them. Even if a completion under the bash-completion convention would try to manipulate the command line in the future, it wouldn't do that based on the positions in COMP_WORDS. So the suggested function COMP_WORDS won't be used by the completions under the bash-completion convention.

Not globally, I can see that, but using the same names locally, correct? Again, my fault for lack of precision.

The code here appears to be masking global variables by creating local variables with the same COMP_* names, no?

Fwiw I'd have expected to leverage a similar pattern for the additions I'd proposed - not necessarily modifying the COMP_* globally - but I take the point that my description has not previously captured that intent.

These seem just a repetition of the use case you already explained twice. I don't seem to be able to find a new argument providing the reason for not using _comp_command_offset.

You asked

How would the second suggestion _comp_command_wrapped be different from _comp_command_offset?

My intent was to provide a clarifying explanation in answer to that question, not a new defense, but I realize I failed to provide a direct answer (which in practice amounts to "_comp_command_wrapped would encapsulate both modifications to COMP_* and subsequently calling _comp_command_offset 0").

I did hope that adjusting the proposal to support a narrower but more common use case, would make a more compelling case for lifting the burden of COMP_* modification from the user, but I accept and understand why it did not. (The "narrower use case" relative to _comp_command_spliced would have been to support removing ${1:n} for some n rather than ${m:n} for some start index m. After thinking through my own [occasionally abusive] utilization of this use case, I understand why it would still be too niche to support in bash-completion. Just recounting the thought process at the time for further clarification.)

Did you actually check the behavior of _comp_command_offset 0, which I'm suggesting from the beginning?

I admit that initially I did not, as I misunderstood and was fixated on the COMP_* portion of the request, so I didn't understand the utility. I did proceed to rework a few of my scripts to use it and now understand that it resolves my pass-through need in general (insofar as I can call _comp_command_offset 0 in place of e.g., re-inserting ${COMP_WORDS[0]) without lifting responsibility for modifications to COMP_*.

I don't think it deserves a utility function maintained in bash-completion.

I accept that and will work towards implementing a solution for my needs (and/or quickly learn the hazards of attempting to support such a function for the general case :) ). Thanks for the insight and patience here.

akinomyoga commented 4 months ago

I'm really confused. I'm unsure about whether we are talking about the same thing.

Not globally, I can see that, but using the same names locally, correct? Again, my fault for lack of precision.

Neither globally nor locally. It keeps the global/previous-scope COMP_* so that it doesn't affect the caller. So it doesn't change the global COMP_WORDS as you know. In addition, when the argument 0 is passed, it keeps also the local COMP_*. It doesn't change the local COMP_WORDS either.

akinomyoga commented 4 months ago

I didn't understand the utility. I did proceed to rework a few of my scripts to use it and now understand that it resolves my pass-through need in general (insofar as I can call _comp_command_offset 0 in place of e.g., re-inserting ${COMP_WORDS[0]) without lifting responsibility for modifications to COMP_*.

Ah, OK. Now the confusion seems to have been resolved. Thanks.

marcxjo commented 4 months ago

Yes, we're on the same page, I just didn't choose my words well in a few passages of my response. Apologies for the obtuseness and any distraction.

Again, I appreciate the willingness to talk through this, my takeaway is that is that it is likely much wiser to keep any COMP_* handling inline (and in any case I should rework my own scripts, as they currently do manipulate the global COMP_* directly, which I don't advocate for in the first place). I'll likely continue to experiment with abstracting it out, but I'm prepared to accept that it may prove non-optimal to do so.

akinomyoga commented 4 months ago

I'll likely continue to experiment with abstracting it out, but I'm prepared to accept that it may prove non-optimal to do so.

Abstraction/factorization is fine, but it's just not a function that should be maintained within bash-completion. If you like, you can maintain the utility function along with the completion of your command. Since no commands in bash-completion follow the suggested use pattern, there is a maintenance problem if the function is included in bash-completion.