| Summary: | ASTERISK-26480: [patch] CLI: core set debug: Auto-completes File not Module | ||||
| Reporter: | Alexander Traud (traud) | Labels: | |||
| Date Opened: | 2016-10-18 03:22:01 | Date Closed: | 2016-10-27 22:23:22 | ||
| Priority: | Minor | Regression? | |||
| Status: | Closed/Complete | Components: | General | ||
| Versions: | 11.23.1 13.11.2 14.0.2 | Frequency of Occurrence | |||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) cli_debug_module.patch | |||
| Description: | Commit [ae6008e|https://github.com/asterisk/asterisk/commit/ae6008ef3acdb582aca60357f7a7870aabba78a1] (code review [574|https://reviewboard.asterisk.org/r/574/]) changed the command-line interface {{core set debug}} to work not per-file but per-module. That change was incomplete because when you tab-complete the command (CLI Generate), not modules (with the file extension .so) but files (.c) get listed. This is not only semantically a different list, but {{.c}} at the end of the name is not valid.
For example, you typed {noformat}core set debug 5 codec_al<TAB>{noformat} which auto-completes to {noformat}core set debug 5 codec_alaw.c{noformat} and registers {{codec_alaw.c}} as module to be debugged. That is accepted without error. However, that does not match any module because of that {{.c}}. The correct name would have been {{codec_alaw.so}} or {{codec_alaw}}. The attached patch fixes this. However, the patch is incomplete because main/cli.c:handle_debug should reject invalid module names. Furthermore, this patch removes the last use of {{ast_complete_source_filename}}. Consequently, that should be removed from the branch master. That was the last consumer of the list {{registered_files}}. Therefore, {{ast_(un)register_file}} should be removed. After that {{ASTERISK_REGISTER_FILE}} has no use anymore and should be removed all over Asterisk. Essentially, making the commit [4a58261|https://github.com/asterisk/asterisk/commit/4a58261694f1538a419dd869f87ea4590171a9f2] (code review [58|https://gerrit.asterisk.org/58]) superfluous as well. Normally, I would link to the causing and the related issue report. However for both commits, I was not able to find their issue report. This is a pity because I do not even know how to refer those commits correctly. *An issue tracker like this one here has several benefits:* If there were an issue report, I could link to this report here. That eases to understand an issue, especially because an report includes more: steps to reproduce, discussion, other related reports. Furthermore, it helps a Quality Manager to know how long a bug remains in the source code. Furthermore, it helps the original authors (and interested readers) to learn from mistakes. For example, when you fix a change but do not link to the issue report, the original author is not notified about that fix at all. Without notifying, the original author can not learn from others/mistakes. Therefore, linking issues is crucial as it aids in avoiding similar bugs in futures. Finally, the original author is a candidate for a code-review, speeding up the inclusion of that change. Therefore, I please the Asterisk team to create issue reports. A change without a related issue report, please, do not accept that anymore, do not commit a change without an entry in the issue tracker. -Because of this, I had to invite the authors of those commits directly-. was not able to do so either | ||||
| Comments: | By: Rusty Newton (rnewton) 2016-10-26 08:14:05.034-0500 [~traud] You can link to commits via code.asterisk.org https://code.asterisk.org/code/changelog/asterisk?cs=ae6008ef3acdb582aca60357f7a7870aabba78a1 By: Friendly Automation (friendly-automation) 2016-10-27 08:35:33.867-0500 Change 4205 had a related patch set uploaded by Corey Farrell: Remove ASTERISK_REGISTER_FILE. [https://gerrit.asterisk.org/4205|https://gerrit.asterisk.org/4205] By: Friendly Automation (friendly-automation) 2016-10-27 08:57:56.849-0500 Change 4205 had a related patch set uploaded by Corey Farrell: Remove ASTERISK_REGISTER_FILE. [https://gerrit.asterisk.org/4205|https://gerrit.asterisk.org/4205] By: Friendly Automation (friendly-automation) 2016-10-27 22:23:22.812-0500 Change 4205 merged by zuul: Remove ASTERISK_REGISTER_FILE. [https://gerrit.asterisk.org/4205|https://gerrit.asterisk.org/4205] By: Alexander Traud (traud) 2016-10-31 07:26:30.385-0500 [~rnewton], thank for that but did you understand what I was about? From your response, I do not know whether I was understood. In a bug tracker, related issues, causing issues and depending issues can be linked. When the E-mail notifications are configured correctly, each link allows to learn about mistakes made. Furthermore, those links show how an issue evolved. This eases many things, even backporting. Bypassing Jira and directly going for Gerrit kills all this. By-passing the issue tracker should be a no-go. This bug here is an excellent example why all this is important. By: Rusty Newton (rnewton) 2016-10-31 09:30:51.901-0500 [~traud] , I understand and I agree. However, this ticket isn't the best place to bring it up - if your objective is to rally and remind others to put more effort towards always creating a JIRA issue for every bug. I recommend posting about your concern and discussing it on the asterisk-dev mailing list as many more people will see it there. Thanks! By: Alexander Traud (traud) 2016-11-01 02:18:39.339-0500 Nope, this ticket is the best place to do so, because it is a role example. This is something regarding the Asterisk Team, and you are part of it. You read it. You bring that matter up internally. Then, you motivate that internally. It is not the job of an external contributor to educate the team about organizational behavior. Furthermore, there is nothing to discuss, just to learn. There is zero motivation to bring something up on a mailing list. This organizational problem is evident here in this issue report and should be your internal starting point. By: Joshua C. Colp (jcolp) 2016-11-01 05:53:11.200-0500 The Asterisk team is made up of the community, not Digium employees. Digium employees are part of the community but there are others (like [~coreyfarrell] - he doesn't work for Digium) and sometimes we drive a lot of the discussion and broach topics - but others do as well. There is no internal chat to be used, everything is public and driven by the community of which you are even a member. I'm not really sure either what you were wanting to strive for. If you think the policy should be changed such that issue IDs are required, it's something that could be done but it must be discussed on the mailing list. We don't institute such changes without discussion. If that is what you would like I can start the discussion and you can participate if you wish. | ||||