[Home]

Summary:ASTERISK-13120: [patch] Need tab completion of "core set debug X filename.c"
Reporter:John Todd (jtodd)Labels:
Date Opened:2008-11-25 13:00:07.000-0600Date Closed:2008-12-10 11:09:22.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20081204__bug13969.diff.txt
( 1) 20081205__bug13969.diff.txt
( 2) 2008120500_bug13969.diff.txt
Description:There is a method by which debugging can be activated for particular C files by typing something like this from the CLI:

core set debug 2 chan_sip.c

There is no indication that this is possible in the tab completion of the command line.  After typing "2" in the example above, hitting "tab" gives a beep instead of something like "<filename.c>" or a list of available files.  For that matter, the idea that a number can be put in as a debug level is not clear, either.
Comments:By: Leif Madsen (lmadsen) 2008-12-02 16:27:13.000-0600

Hey Michel,

I've assigned this to you since you've have so much experience with the Asterisk CLI lately that you may be able to close out this bug fairly quickly.

Let me know if that is not the case. Thanks!
By: Michiel van Baak (mvanbaak) 2008-12-04 10:10:30.000-0600

Did this work in older versions ?
If you cant remember I'll checkout a version from before the devcon because there we touched this CLI command IIRC.
By: John Todd (jtodd) 2008-12-04 10:34:09.000-0600

I never tried with older versions, I was noting only what I saw with the most recent version.  Sorry!
By: Michiel van Baak (mvanbaak) 2008-12-04 10:43:01.000-0600

No problem.
I'll look into it
By: Eliel Sardanons (eliel) 2008-12-04 12:30:10.000-0600

If I am not wrong there isn't such a filename.c list. We could use loaded modules and replace .so with .c.
Also this CLI command let you put what you want as a file, it just set that "filename" to a list, you are able to put 'whatever.c'.
By: John Todd (jtodd) 2008-12-04 12:42:02.000-0600

I don't think we need to list all the possible .c files, but we should at least indicate that it is possible to have additional parameters in the tab-completion list.  This goes for numeric values as well - we typically do not give a hint that there is a numeric option when one hits "tab" at the appropriate time, which is kind of confusing for someone who doesn't know that there is a numeric value that can be filled in.
By: Tilghman Lesher (tilghman) 2008-12-04 12:56:59.000-0600

Just using the module list is hardly enough.  Some of the most useful debug information is available from the core, which exists within the asterisk binary.

Secondarily, I'm not sure how useful this is to the general population.  Debug messages are meant to be useful to programmers, not to the general population.  If we asked someone to provide debugging information, it is likely that we would give them the exact command to type, anyway.
By: John Todd (jtodd) 2008-12-04 13:07:29.000-0600

Tilghman: I'm uncertain if you're advocating not showing this undocumented feature in the tab help.  Is that the case?  I suspect most developers don't even know about this option.
By: Tilghman Lesher (tilghman) 2008-12-04 13:58:51.000-0600

jtodd: that would seem to be a reason for putting this command in the developer documentation (coding guidelines?), not a reason for adding tab completion.
By: John Todd (jtodd) 2008-12-04 14:11:01.000-0600

I'm unclear on the course of thought that leads to that conclusion.  I think that a reasonable person would expect that any command-line options that are possible would be hinted at via tab completion, and not merely discoverable in additional documentation.  Otherwise, it would be an arbitrary decision of someone to determine what should or should not be hinted at in tab completions, and it would discourage users from believing what tab completions provided for hinting.  This leads to the perception of inconsistent behaviors which I think we're trying to avoid when new or even experienced users attempt to learn about less commonly-used features.
By: Tilghman Lesher (tilghman) 2008-12-04 14:21:51.000-0600

Yes, but you advocated previously that we should only hint at the available modules, which does not include a substantial set of debug functionality.  That would correctly be deemed a bug, whereas right now, it could, at most, be deemed a missing feature.  I am advocating that we either do it right, the first time, or not at all.
By: John Todd (jtodd) 2008-12-04 14:57:50.000-0600

I don't consider missing portions of tab-completion documentation to be "missing functionality", since most (but perhaps not all) commands are expected and do behave in a way that describes hints.  I think this is splitting semantic hairs over a specific hint feature, though if other debug commands are similarly missing tab hinting perhaps this is worthy of a janitor request for other elements (indeed, that may already be on the list, and I am just picking one out that became obvious to me.)  

If it is a difficult proposition to create an exact list of all possible modules that can be displayed, and this is a rarely-used element, then it is possible that the additional effort to create a full list is not worth the development time.

However, if it is not overly difficult to list all of the modules (taking eliel's suggestion of replacing ".so" with ".c") then that perhaps is also a possible solution.  The full list is optimal, but I suspect if it requires significant effort then perhaps only the hint of "<modulename.c>" would be sufficient for the reasonable person to determine what is required.

When I type "show ip route ?" on a Cisco, it doesn't show me all possible routes in the table.  (granted, the "?" replaces what we use [tab] for)  It shows me a list of possible options, including options which represent a table or unspecified list of other items, such as IP addresses.

core1.sjc1>sh ip route ?
 Hostname or A.B.C.D  Network to display information about or hostname
 bgp                  Border Gateway Protocol (BGP)
 connected            Connected
 dhcp                 Show routes added by DHCP Server or Relay
 eigrp                Enhanced Interior Gateway Routing Protocol (EIGRP)
 isis                 ISO IS-IS
 list                 IP Access list
 mobile               Mobile routes
 odr                  On Demand stub Routes
 ospf                 Open Shortest Path First (OSPF)
 profile              IP routing table profile
 rip                  Routing Information Protocol (RIP)
 static               Static routes
 summary              Summary of all routes
 supernets-only       Show supernet entries only
 vrf                  Display routes from a VPN Routing/Forwarding instance
 |                    Output modifiers
 <cr>

core1.sjc1>sh ip route


While I am not advocating that we reproduce the Cisco method in it's entirety, I am using the case to describe what one might argue is the most well-known appliance vendor's take on the methods to describe options in a command line.  Listing tables full of options is not always required or expected, though it is useful to do so when it is convenient for the developer or makes a significant ease-of-use improvement for the user.  It is sufficient in many cases to simply indicate that further options are available, even if they are not explicitly outlined in the hint.

In the case of debugging individual .c files, I think that typically the user would know what file they wish to obtain debugging information from, therefore listing the entire possible array of .c files available may not be required (though I wouldn't discourage anyone from doing it.)  I would not say at all that a lack of a list in this instance isn't "do[ing] it right" and that the opposite ("not at all") would suggest a method of development under which that many components of Asterisk would have never been implemented at all.
By: Tilghman Lesher (tilghman) 2008-12-04 15:24:29.000-0600

The issue is that if you implement this for just a portion of the files, we WILL be forced down the road to implement for ALL the files.  A reasonable person would expect that if tab-completion is available, ALL options will be completeable via the tab-completion.  That is why I am opposed to completing this half-assed.  Either we need to do it completely or not at all.
By: Tilghman Lesher (tilghman) 2008-12-04 15:49:57.000-0600

[15:26:03] <Corydon76-dig> russellb: would you agree that tab-completing a subset of available options is worse than not tab-completing at all?
[15:26:18] <russellb> good question
[15:26:53] <russellb> I suppose so
[15:27:01] <Corydon76-dig> M13969
[15:27:03] <russellb> I'm just concerned that people use tab to see what's available, and if it shows something, but not everything ...
[15:27:03] <MuffinMan> [assigned] [Asterisk] General 0013969: Need tab completion of "core set debug X filename.c" reported by jtodd (Karma: +26.75) http://bugs.digium.com/view.php?id=13969
[15:27:12] <Corydon76-dig> I've been trying to convince jtodd of that
[15:27:32] <russellb> what can't be tab completed in this case?
[15:27:37] <russellb> filenames?
[15:27:40] <eliel> numbers
[15:27:48] <Corydon76-dig> All filenames available for 'core set debug'
[15:27:50] <eliel> verbose levels
[15:27:51] <eliel> and filenames
[15:28:17] <Corydon76-dig> because some of the filenames are not directly available in the binary, but only by searching the source directory
[15:28:19] <eliel> the problem is also when the next parameter is a number
[15:28:40] <Corydon76-dig> Right, because the number can be anywhere from 0 to 2.1 billion
[15:28:51] <Corydon76-dig> (all of which are reasonable)
[15:28:59] <russellb> ok, so clearly the number isn't going to fly
[15:29:04] <russellb> filenames we might be able to do
[15:29:12] <russellb> since we already have the ASTERISK_REGISTER_FILE_VERSION macro
[15:29:17] <russellb> it's registering filenames into a list
[15:29:26] <russellb> so you can pull from that
[15:29:33] <Corydon76-dig> That's a good hint.
[15:29:37] <russellb> :)
[15:29:55] <eliel> wow... i miss that one
[15:30:12] <russellb> check out "core show file version"
[15:30:38] <eliel> exactly what we need
[15:30:42] <russellb> yay
[15:31:18] <russellb> it just means we have to make sure every file has that macro in it ...
[15:31:27] <russellb> which I'm sure a number don't
[15:32:13] <Corydon76-dig> but some of those files don't have ast_debug() in them, so it probably doesn't matter
[15:32:20] <russellb> true
[15:32:24] <russellb> (or ast_verb())
[15:32:53] <Corydon76-dig> You can set verbose only for a single file, too?
[15:33:03] <russellb> in trunk/1.6, yeah
By: John Todd (jtodd) 2008-12-04 16:09:46.000-0600

Sounds like the best of the two options has a potential (the list of filenames) and that would be great!
By: Tilghman Lesher (tilghman) 2008-12-04 19:25:39.000-0600

Let's try something like this, which does tab completion for both the numbers, as well as the filenames.
By: Mark Michelson (mmichelson) 2008-12-04 19:45:28.000-0600

While I appreciate the approach of being able to tab-complete individual numbers, I have to question whether it's actually necessary. Wouldn't it be enough to just put something like "<number>" for the tab-completion? That would give the appropriate hint that a number is expected.
By: Mark Michelson (mmichelson) 2008-12-04 19:59:52.000-0600

I had my idea slapped out of my hand on IRC to a degree. If a number is the only possible something that can be tab-completed, then literally putting "<number>" on the command line is a bad idea. My idea was to "suggest" a number instead of actually completing the word <number> on the command line, similar to the behavior exhibited by the tab key if there are ambiguous choices.
By: Michiel van Baak (mvanbaak) 2008-12-05 02:46:50.000-0600

How about this?
It's the same patch as Corydon's one, but does not print the 0... only the single 0 because I dont think we need 'core set debug 000000000000 <filename>'
By: Eliel Sardanons (eliel) 2008-12-05 08:55:16.000-0600

Both patches work fine, except for a a little reason:

When doing: "core set debug a<TAB>" it still autocompletes the numbers and should only finish "atleast" autocompletion.
By: Tilghman Lesher (tilghman) 2008-12-05 10:22:41.000-0600

New patch uploaded that should fix this.
By: Eliel Sardanons (eliel) 2008-12-05 11:55:10.000-0600

Now it is working as expected!
By: Digium Subversion (svnbot) 2008-12-10 11:09:21.000-0600

Repository: asterisk
Revision: 162687

U   trunk/include/asterisk.h
U   trunk/main/asterisk.c
U   trunk/main/cli.c

------------------------------------------------------------------------
r162687 | mvanbaak | 2008-12-10 11:09:20 -0600 (Wed, 10 Dec 2008) | 8 lines

add tab completion for 'core set debug X filename.c'

(closes issue ASTERISK-13120)
Reported by: jtodd
Patches:
     20081205__bug13969.diff.txt uploaded by Corydon76 (license 14)
Tested by: mvanbaak, eliel

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=162687