Re: [PATCH] help: always suggest common-cmds if prefix of cmd
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Nov 24, 2010, 20:20 UTC
- Message-ID
- <AANLkTi=nxcODCvQ6hmaQe=q38e=bF7cRHWrRaFr+zen6@mail.gmail.com>
- In-Reply-To
- <7v1v6atsbd.fsf@alter.siamese.dyndns.org>
On Wed, Nov 24, 2010 at 8:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> @@ -320,9 +321,16 @@ const char *help_unknown_cmd(const char *cmd)
>> uniq(&main_cmds);
>>
>> /* This reuses cmdname->len for similarity index */
>> + for (i = 0; i < main_cmds.cnt; ++i) {
>> + main_cmds.names[i]->len = 1 +
>> levenshtein(cmd, main_cmds.names[i]->name, 0, 2, 1, 4);
>> + for (n = 0; n < ARRAY_SIZE(common_cmds); ++n) {
>> + if (!strcmp(main_cmds.names[i]->name,
>> + common_cmds[n].name) &&
>> + !prefixcmp(main_cmds.names[i]->name, cmd))
>> + main_cmds.names[i]->len = 0;
>> + }
>> + }
>
> So main_cmds.names[]->len (which is not "len" anymore at this point but is
> just a "score") gets levenshtein distance (i.e. a smaller number indicates
> cmd is more likely to be a typo of it), and in addition ->len == 0 is "it
> is prefix". Overall, the smaller the score, the likelier the match.
>Correct. This was already the case, though. I just reserved len = 0 as "this is a prefix of the entered command".
Show 19 quoted lines
>> @@ -330,9 +338,12 @@ const char *help_unknown_cmd(const char *cmd)
>> if (!main_cmds.cnt)
>> die ("Uh oh. Your system reports no Git commands at all.");
>>
>> - best_similarity = main_cmds.names[0]->len;
>> - n = 1;
>> - while (n < main_cmds.cnt && best_similarity == main_cmds.names[n]->len)
>> + n = 0;
>> + do {
>> + best_similarity = main_cmds.names[n++]->len;
>> + } while (!best_similarity);
>
> At this point, main_cmds.names[] is sorted by the above score (smaller to
> larger), and first you skip all the "prefix" ones that score 0.
>
> This relies on the fact that there is at least one entry with non-zero
> score, which in practice is true, but without even a comment? I feel
> dirty.
>Ah, yes. This needs clearer code badly. I'll see what I can cook up.
Show 9 quoted lines
> The score of the first non-prefix entry is in best_similarity and that > entry is at main_cmds.names[n-1] at this point. You haven't checked > main_cmds.names[n] yet... > >> + n++; > > ... but you increment n to skip that entry without even looking, and then > go on to ... >
This is a bug, thanks for spotting it. It was intended to be the same as "i = 1" in the old version, but I didn't think about it being increased in the previous loop as well, silly me.
Show 10 quoted lines
>> + while (n < main_cmds.cnt && best_similarity >= main_cmds.names[n]->len) >> ++n; > > You skip the entries with the same similarity as the closest typo, > presumably to point n to the first entry that is irrelevant (i.e. 0 thru n > but not including n are candidates). > > Your rewrite of the loop makes it very hard to read and spot bugs, I > think. >
Indeed. What about this intra-diff? Hopefully it's a bit clearer, as it's closer to the original, just reusing the same logic for the new similar loop... Also makes the final diff smaller, which is nice.
diff --git a/help.c b/help.c index dc76a62..d02a019 100644 --- a/help.c +++ b/help.c @@ -339,11 +339,10 @@ const char *help_unknown_cmd(const char *cmd) die ("Uh oh. Your system reports no Git commands at all."); n = 0; - do { - best_similarity = main_cmds.names[n++]->len; - } while (!best_similarity); - n++; - while (n < main_cmds.cnt && best_similarity >= main_cmds.names[n]->len) + while (n < main_cmds.cnt && !main_cmds.names[n]->len) + ++n; + best_similarity = main_cmds.names[n++]->len; + while (n < main_cmds.cnt && best_similarity == main_cmds.names[n]->len) ++n; if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { const char *assumed = main_cmds.names[0]->name;