[Regression] make autocompletion skips install

[tsdgeos]

Describe the bug

To reproduce

0. rename the Makefile.txt to Makefile (github won't let me upload it with it's correct name) and put it in a folder
1. cd to 'directory_that_contains_attached_Makefile'
2. Type 'make inst' and press the TAB key
3. See that it autocompletes to make install/

Expected behavior

It should autocomplete to make install like it used to (given that there is an install target in the Makefile)

Versions (please complete the following information)

• Operating system name/distribution and version: ArchLinux
• bash version, echo "$BASH_VERSION": 5.2.26(1)-release
• bash-completion version, (IFS=.; echo "${BASH_COMPLETION_VERSINFO[*]}"): 2.14.0

Additional context

This used to work with bash completion 2.11

[akinomyoga]

0. rename the Makefile.txt

Minimal reproducer:

install:
.PHONY : install
install/fast:
.PHONY : install/fast
install/local:
.PHONY : install/local

[akinomyoga]

The current implementation of the make completion assumes install to be a directory name when other targets like install/fast and install/local exist. Then, it considers that the main targets are install/fast and install/local and removes the target install assuming it is a secondary target just to prepare a directory for the main targets.

Maybe we can preserve the phony targets as is, but in the current implementation, the stages of generating targets and filtering the paths are separated. The latter stage doesn't know which ones were generated as phony targets.

[tsdgeos]

The current implementation of the make completion assumes install to be a directory name when other targets like install/fast and install/local exist.

Please don't do that :)

[scop]

Please don't do that :)

PR's including test suite additions welcome :)

[tsdgeos]

Just revert the commit that broke it?

[scop]

The make completion used to be in a state that nobody was willing to touch it. It has been completely redone between 2.11 and now, and even though people might not be that eager to touch it these days either, it has changed for the better and we're not going back to what it was.

[tsdgeos]

@scop so you're asking a random person like me to fix the bug you folks introduced when someone like @akinomyoga basically said "this is hard" on their comment?

[akinomyoga]

It's not a bug. It's design. Or you might say it is a design bug, but it's not a bug that causes unintentional behavior. Also, you seem to request reverting the change, but the previous version had several explicit bugs (which cause unintentional random behaviors). In addition, the previous implementation was terrible. We've already put effort into improving the implementation and fixed the problems, yet the implementation is not as clean as ideal.

It should also be noted that OSS is maintained by random unpaid people, so it's normal to ask random people. The current maintainers are also random people. We are always looking for contributors as we don't have enough human resources. Then, it's natural to think about the possibility of a fix from the original reporter or other people watching this issue, who would likely have more energy to solve it than others.

I didn't say it's hard. It's actually not hard to implement the requested behavior, but it's hard to decide what would be the best implementation (and even whether it should be implemented at all). It's a tradeoff between the feature/design and the maintenance cost. The main question is whether it's worth complicating the current implementation. In particular, I'm reluctant to add complications just because cmake generates strange names for the phony targets. Why did they choose such a confusing naming that looks like file paths? We probably finally need to accept that, though.

The way I suggested complicates the implementation, but there might be another cleaner way without further complications of the implementation, in which case we can just implement it. Anyway, it requires energy to think about possibilities, and the priority of this issue is not so high among the many issues. Someone who is particularly interested in this issue needs to take a look at it.

[tsdgeos]

It should also be noted that OSS is maintained by random unpaid people

I know, i am one of those unpaid people https://invent.kde.org/aacid

I didn't say it's hard

It very much seemed as you saying it's hard, actually after reading all this wall of text you wrote, it still seems you're saying it's hard.

Honestly from the totally naive point of view, it seems like you can fix this relatively easily in 2 ways

way 1: Do not autocomplete the / if there is a target without the /, that is if you have 2 targets, one called install/fast and one called install/local it's perfectly fine to autocomplete to install/, but if there's also one called install, auto-completing the / seems wrong since you're autocompleting more than the base common string for all 3 targets

If way 1 is not acceptable

way 2: You say that you autocomplete the / because there is an assumption that if there's install install/foo and install/bar that install will be a folder, well, then check if install is actually a folder called install before adding the / ?

[akinomyoga]

I didn't say it's hard

It very much seemed as you saying it's hard, actually after reading all this wall of text you wrote, it still seems you're saying it's hard.

In my first reply, I just described the current implementation. In the second reply, technically yes, I said that it'd be hard in the sense that it would take time to carefully determine the internal implementation.

way 1:

The current behavior is designed to be consistent with the built-in completions of Bash for file paths, where a slash is suffixed at the end of a directory name.

As I already wrote, even if the directory name is not the primary target, Makefile will contain the directory names because it is necessary for preparing those directories where the primary targets will be placed. So we cannot conclude xxx (where there are targets xxx/yyy) would be a primary target that the user would try to specify. Rather, it is unlikely that the primary target would be just to create a directory.

All those logic are based on the assumption that the targets including a slash would be file paths. If that assumption breaks, one would need to check .PHONY to judge the type of the target, which I meant in my first reply.

way 2:

This doesn't seem to make sense. After checking whether the name is an actual folder, what should we do? Both a phony target and a directory name expect that the directory does not exist. If there is a target of the directory name, it means that the directory does not exist initially. If the directory already exists, one doesn't need to create the directory, so there is no reason to generate the directory name for the completion.