{"thread":{"id":"37583","subject":"[PATCH] help: Fix size passed to qsort","startedAt":"2014-09-17T12:14:39Z","lastAt":"2014-09-18T17:34:48Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249596","messageId":"1410956079-23513-1-git-send-email-stefanbeller@gmail.com","threadId":"37583","inReplyTo":null,"subject":"[PATCH] help: Fix size passed to qsort","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-09-17T12:14:39Z","receivedAt":"2014-09-17T12:14:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We actually want to have the size of one 'name' and not the size\nof the names array.\n\nSigned-off-by: Stefan Beller <stefanbeller@gmail.com>\n---\n help.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/help.c b/help.c\nindex 7af65e2..2072a87 100644\n--- a/help.c\n+++ b/help.c\n@@ -305,7 +305,7 @@ const char *help_unknown_cmd(const char *cmd)\n \tadd_cmd_list(&main_cmds, &aliases);\n \tadd_cmd_list(&main_cmds, &other_cmds);\n \tqsort(main_cmds.names, main_cmds.cnt,\n-\t      sizeof(main_cmds.names), cmdname_compare);\n+\t      sizeof(*main_cmds.names), cmdname_compare);\n \tuniq(&main_cmds);\n \n \t/* This abuses cmdname->len for levenshtein distance */\n-- \n2.1.0.238.gce1d3a9\n"},{"id":"249641","messageId":"xmqq8ulgq25h.fsf@gitster.dls.corp.google.com","threadId":"37583","inReplyTo":"1410956079-23513-1-git-send-email-stefanbeller@gmail.com","subject":"Re: [PATCH] help: Fix size passed to qsort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-18T17:17:14Z","receivedAt":"2014-09-18T17:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@gmail.com> writes:\n\n> We actually want to have the size of one 'name' and not the size\n> of the names array.\n\nI suspect that the latter is \"size of a pointer that points at a\ncmdname structure\", but the original code in help_unknown_cmd() is\nwrong.  The ones in load_command_list() do this correctly and\nanother qsort() invocation in this function does so as well.  I\nwonder why they didn't correctly cut&paste ;-)\n\n746c221a (git wrapper: also use aliases to correct mistyped\ncommands, 2008-09-10) seemed to have introduced the culprit.\n\nThe call to uniq() would fail to uniquify because main_cmds would\nhave the standard command all in front and then aliases and commands\nin the user's PATH later, but I do not quite see if there is any\nend-user observable breakages that can arise from this.  What is the\npractical implication of this breakage?\n\nNo, I am not saying we do not have to fix it; I am just being\ncurious why this patch does not show the existing breakage with a\nnew test.\n\nThanks.\n\n>\n> Signed-off-by: Stefan Beller <stefanbeller@gmail.com>\n> ---\n>  help.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/help.c b/help.c\n> index 7af65e2..2072a87 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -305,7 +305,7 @@ const char *help_unknown_cmd(const char *cmd)\n>  \tadd_cmd_list(&main_cmds, &aliases);\n>  \tadd_cmd_list(&main_cmds, &other_cmds);\n>  \tqsort(main_cmds.names, main_cmds.cnt,\n> -\t      sizeof(main_cmds.names), cmdname_compare);\n> +\t      sizeof(*main_cmds.names), cmdname_compare);\n>  \tuniq(&main_cmds);\n>  \n>  \t/* This abuses cmdname->len for levenshtein distance */\n"},{"id":"249643","messageId":"xmqqzjdwomrr.fsf@gitster.dls.corp.google.com","threadId":"37583","inReplyTo":"xmqq8ulgq25h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] help: Fix size passed to qsort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-18T17:34:48Z","receivedAt":"2014-09-18T17:34:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <stefanbeller@gmail.com> writes:\n>\n>> We actually want to have the size of one 'name' and not the size\n>> of the names array.\n> ...\n> I suspect that the latter is \"size of a pointer that points at a\n> cmdname structure\", but the original code in help_unknown_cmd() is\n> wrong.  The ones in load_command_list() do this correctly and\n> another qsort() invocation in this function does so as well.  I\n> wonder why they didn't correctly cut&paste ;-)\n>\n> 746c221a (git wrapper: also use aliases to correct mistyped\n> commands, 2008-09-10) seemed to have introduced the culprit.\n>\n> The call to uniq() would fail to uniquify because main_cmds would\n> have the standard command all in front and then aliases and commands\n> in the user's PATH later, but I do not quite see if there is any\n> end-user observable breakages that can arise from this.  What is the\n> practical implication of this breakage?\n\nHeh, I should have spent a bit more time before starting to type.\nThe answer probably is \"nothing\", as\n\n\tstruct cmdnames {\n        \t...\n                struct cmdname {\n                \t...\n\t\t} **names;\n\t} main_cmds;\n\nis what we are dealing here, so main_cmds.names is a pointer that\npoints at a slab of memory to hold many pointers, each of which\npoints at \"struct cmdname\".  And sizeof(struct cmdname **) is\nincorrectly passed where we should pass sizeof(struct cmdname *),\nwhich you fixed, but in practice they would be the same size anyway\n;-)\n\n>>  \tqsort(main_cmds.names, main_cmds.cnt,\n>> -\t      sizeof(main_cmds.names), cmdname_compare);\n>> +\t      sizeof(*main_cmds.names), cmdname_compare);\n>>  \tuniq(&main_cmds);\n>>  \n>>  \t/* This abuses cmdname->len for levenshtein distance */\n"}]}