fix(make): fix stripped directory names with `bind 'set show-all-if-ambiguous on'`

[akinomyoga]

Resolve #544

• Commit 53060e3b: support @pytest.mark.bashcomp(required_cmd=True) to skip all the tests in the class.
• Commit eb12832c: add failing test cases to illustrate the problems
• Commit 0ddae4c1: do not strip directory names when COMP_TYPE=37. This happens when the user presses TAB under the readline setting bind 'set show-all-if-ambiguous on' is set.
• Commit d5181c55: complete subdirectories as far as the path is unique

Test suite problem on COMP_TYPE

The test test_github_issue_544_4 (test_make.py) currently fails because of the design of the test suite. The test suite extracts the generated completions by changing the readline settings as

@@ -22,20 +22,20 @@
 set menu-complete-display-prefix off
 set meta-flag on
 set output-meta on
-set page-completions on
+set page-completions off
 set prefer-visible-bell on
 set print-completions-horizontally off
 set revert-all-at-newline off
-set show-all-if-ambiguous off
+set show-all-if-ambiguous on
 set show-all-if-unmodified off
 set show-mode-in-prompt off
 set skip-completed-text off
 set visible-stats off
-set bell-style audible
+set bell-style none
 set comment-begin #
-set completion-display-width -1
+set completion-display-width 0
 set completion-prefix-display-length 0
-set completion-query-items 100
+set completion-query-items 0
 set editing-mode emacs
 set emacs-mode-string @
 set history-size 100000

Under these settings, the completion functions are called with COMP_TYPE=33 (!, show list), which is different from the usual COMP_TYPE=9 (TAB) or COMP_TYPE=37 (%, menu-complete). The completion for make changes the behavior depending on COMP_TYPE so that it doesn't produce the expected results in the test suite. Note that make completion is the only one that changes its behavior depending on the value of COMP_TYPE among the completions in bash-completion.

@scop Do you have any thoughts on how we should solve this problem?

• Option 1: Use another approach to get the generated completions if any? I'm not sure if there is any.
• Option 2: Make the completion results independent of COMP_TYPE?
• Option 3: Add a special code in completions/make that detects the test suite and changes the behavior only in the test suite?
• Option X: any other idea?

Possibility of using awk

sed has been used to extract the target names by make. It is difficult to use sed to test whether the filenames in subdirectories are unique and to generate the full path only when it is unambiguous. In commit d5181c5, sed scripts has been changed so as to always produce the full paths, and instead, the generated results are afterward filtered using Bash code. But, maybe another approach is to rewrite everything using awk. It should be more efficient than processing in Bash.

[scop]

Added a few rather superficial comments. The make completion continues to terrify me -- the code is quite complex, and my make-fu isn't really up to speed to anywhere near always tell how the completion should behave. So I'm not sure how much I can be of assistance here. Let's just keep adding test cases for all changes we make, like you've done here.

* Option 2: Make the completion results independent of `COMP_TYPE`?

This sounds good to me in principle, but I can't tell offhand what it would mean to user experience.

But, maybe another approach is to rewrite everything using awk. It should be more efficient than processing in Bash.

Do we think that Makefiles can get sufficiently large to make efficiency here a issue we need to care much about? To me, code understandability and portability are more important unless there is an actual performance issue we do want to tackle, and in general bash wins over sed or awk on the understandabilty/portability front. But things can get hairy and overly verbose with bash, too, so this is something to look into case by case.

[akinomyoga]

* Option 2: Make the completion results independent of `COMP_TYPE`?

This sounds good to me in principle, but I can't tell offhand what it would mean to user experience.

Removing the dependence on COMP_TYPE means that we effectively revert the feature introduced by 39f00f9. This feature strips the directory name in the list of completions displayed in the terminal. For example, when we try to complete subdir/x and there are matching make targets subdir/x1, subdir/x2, and subdir/x3, the displayed list becomes x1 x2 x3 with this feature instead of subdir/x1 subdir/x2 subdir/x3.

But, maybe another approach is to rewrite everything using awk. It should be more efficient than processing in Bash.

Do we think that Makefiles can get sufficiently large to make efficiency here a issue we need to care much about?

I think basically a handwritten Makefile shouldn't be so large (unless the user uses fancy GNU make macros to automatically generate a bunch of rules). However, the result of Makefile generators such as autotools and cmake can be extremely large.

To me, code understandability and portability are more important unless there is an actual performance issue we do want to tackle, and in general bash wins over sed or awk on the understandabilty/portability front.

For understandability, I believe awk is better than the current sed or a Bash implementation. For the portability, awk may be harder than sed but is still manageable if we carefully write the awk script. I think the most challenging implementations for awk/sed among contemporary systems are Solaris awk and Solaris sed. At least, Solaris awk lacks many important features of POSIX awk. I actually have no experience with Solaris sed so am not even sure whether the POSIX sed scripts work as expected.

But things can get hairy and overly verbose with bash, too, so this is something to look into case by case.

Maybe I should make a sample implementation by awk and we compare it with the current implementation?

[scop]

Maybe I should make a sample implementation by awk and we compare it with the current implementation?

Sure, if you're interested.

While at it, no matter the language, I think it would be cool if we could extract the whole implementation to a separately invoked static script in helpers/ instead of generating it from bash code on the fly. That'd help with editor syntax highlighting and formatting, avoid some potentially hairy escaping issues, and likely make it easier to test.

[akinomyoga]

I'm going to update this PR after another PR #548, which the new commits for this PR would depend on, is processed.

• Option 2: Make the completion results independent of COMP_TYPE? (comment by @akinomyoga)

This sounds good to me in principle, but I can't tell offhand what it would mean to user experience. (comment by @scop)

Removing the dependence on COMP_TYPE means that we effectively revert the feature introduced by 39f00f9. (comment by @akinomyoga)

If you are fine with removing the COMP_TYPE-dependent feature 39f00f9, I can remove it. What do you think?

Maybe I should make a sample implementation by awk and we compare it with the current implementation?

Sure, if you're interested.

I would do it in another PR if I have time after this PR is settled.

[akinomyoga]

Under these settings, the completion functions are called with COMP_TYPE=33 (!, show list), which is different from the usual COMP_TYPE=9 (TAB) or COMP_TYPE=37 (%, menu-complete). The completion for make changes the behavior depending on COMP_TYPE so that it doesn't produce the expected results in the test suite. Note that make completion is the only one that changes its behavior depending on the value of COMP_TYPE among the completions in bash-completion.

@scop Do you have any thoughts on how we should solve this problem?

• Option 1: Use another approach to get the generated completions if any? I'm not sure if there is any.
• Option 2: Make the completion results independent of COMP_TYPE?
• Option 3: Add a special code in completions/make that detects the test suite and changes the behavior only in the test suite?
• Option X: any other idea?

* Option 2: Make the completion results independent of `COMP_TYPE`?

This sounds good to me in principle, but I can't tell offhand what it would mean to user experience.

I still hesitate to choose the solution for this issue. In the current test framework, the completions are extracted with COMP_TYPE=33 (display candidates) which is different from the usual COMP_TYPE=9 (tab-completion) and COMP_TYPE=37 (menu-completion). Considering the reason that Bash provides the variable COMP_TYPE, I feel it is a too strong restriction to require not using COMP_TYPE in completion functions.

Isn't there any way to update the test framework so that it can handle different cases of COMP_TYPE?

[Edit: Sorry there were many typos of COMP_TYPE being wrongly COMP_REPLY. I fixed them. ]

[scop]

This is an area in which I have little experience, so unsure how much I can be of assistance. A couple of (likely stupid) comments/questions though:

In the current test framework, the completions are extracted with COMP_TYPE=33

Not sure I follow -- the only git grep hit for that I find is the one in the current make completion that test it's not equal to 9...?

Isn't there any way to update the test framework so that it can handle different cases of COMP_REPLY?

Doesn't strike me as impossible off the bat, but nothing concrete to offer right now either.

[akinomyoga]

We might also need to exclude the case COMP_TYPE == 42 (cf https://github.com/spf13/cobra/pull/1509)

[akinomyoga]

The issue is reported again in #858. I think there is no reason to block the fixes just for the test suite problem. Here, I have rebased it on top of the latest master and, in commit d21ebba8, deactivated the display-only mode (39f00f92e52b783e7e9e43aac4b4274cc9dee152) causing the test-suite issue. If we would like to still use the "display-only mode", we can discuss how to make it work with the test framework in a separate issue, but I'm currently not sure if the "display-only mode" is worth re-enabling.

[ncfavier]

Can confirm this works fine. Not sure why we need all that code though, bash should be able to do the right thing with paths (complete -o filenames?)

[akinomyoga]

Can confirm this works fine.

Thank you for checking!

Not sure why we need all that code though, bash should be able to do the right thing with paths (complete -o filenames?)

I guess you mean complete -f (because -o filenames specifies that the completions generated by other means should be post-processed for quoting, suffixing with /, etc.)?

The reason for not using Bash's filename generation is that we would like to generate the target names defined in Makefile (or GNUmakefile, etc.). The target names are not necessarily filenames, and filenames are not necessarily targets, and Bash doesn't know which are the make targets and which are not. This is the reason that the generation is implemented in the script.

[ncfavier]

I am talking about processing, not generation. I guess what we're doing isn't even specific to paths. complete -W 'test/abc test/xyz' foo already seems to do the right thing, why do we need to reinvent anything?

[akinomyoga]

OK, I see. Thank you for your clarification!

What we are doing here is not equivalent to compgen -W 'test/abc test/xyz' -o filenames. We reduce the list test/abc test/xyz to test/, which is not available by -o filenames. This is the behavior in the current master, though I'm not sure for the background of this behavior. This PR partially suppresses this reduction only in the unambiguous cases as requested in #544. If there is anything built in Bash that can be used to implement this conditional reduction in a simple way, I'm not reluctant to switch to it. Do you have any idea?

P.S.

though I'm not sure for the background of this behavior.

It seems the reduction is introduced in commit b28d7108d3677c61bd01c51ccee8bb1cf9e3bfba. This seems to predate the time we moved to GitHub, so I don't know where to find the corresponding discussion.

[ncfavier]

If there is anything built in Bash that can be used to implement this conditional reduction in a simple way, I'm not reluctant to switch to it.

I mean, leaving aside -o filenames, if I just run complete -W 'test/abc test/xyz' foo then foo <Tab> completes to foo test/, and if I remove test/abc then it completes to test/xyz. Why do we need to do anything?

In other words this "reduction" behaviour you're talking about is just... how completion works, why would we need to reimplement it ourselves for make?

[akinomyoga]

Ah, I see what you mean now. I have been considering the case where there are multiple directories and multiple files therein and we do not want to list the entire file tree by show-all-if-ambiguous or by two consecutive TABs. For the normal completion with complete -f, we just obtain the list of filenames in the current directory or in the subdirectory already input, which the current implementation in the master and also this PR mimics.

But maybe the behavior you suggest makes sense.

[ncfavier]

Ah, I see what you mean, we're reducing the list of options shown. That makes sense.

[akinomyoga]

It's hard to decide for me. This is related to the background that introduced this line ten years ago. This is just my guess, but people might have wanted to mimic the behavior of Bash for normal paths.

[scop]

As noted, the make completion is already too complex for my taste and this makes it even more complex. Then again, we have the test cases here which appear to make sense as well as good amount of commentary, and no alternative implementations for taking care of the issue, so leaving it up to @akinomyoga to merge if he's comfortable with what we have here.

[akinomyoga]

@ncfavier @scop Thank you for your comments. After another look, I decided to separate the corresponding code in a new function (1ea3326e is the updated patch). I think I will merge this version after waiting for a week.