{"thread":{"id":"30392","subject":"[PATCH 0/2] Fix some constness errors in fetch-pack","startedAt":"2012-05-02T10:40:57Z","lastAt":"2012-05-21T08:13:48Z","messageCount":13,"participants":["mhagger@alum.mit.edu","Nguyen Thai Ngoc Duy","Michael Haggerty","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"190532","messageId":"1335955259-15309-1-git-send-email-mhagger@alum.mit.edu","threadId":"30392","inReplyTo":null,"subject":"[PATCH 0/2] Fix some constness errors in fetch-pack","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T10:40:57Z","receivedAt":"2012-05-02T10:40:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nFix some constness errors that I noticed while reading the code in\nbuiltin/fetch-pack.c.\n\nMichael Haggerty (2):\n cmd_fetch_pack(): declare dest to be const\n cmd_fetch_pack(): fix constness problem and memory leak\n\n builtin/fetch-pack.c | 152 +++++++++++++++++++++++++-------------------------\n 1 file changed, 77 insertions(+), 75 deletions(-)\n\n-- \n1.7.10\n"},{"id":"190533","messageId":"1335955259-15309-2-git-send-email-mhagger@alum.mit.edu","threadId":"30392","inReplyTo":"1335955259-15309-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] cmd_fetch_pack(): declare dest to be const","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T10:40:58Z","receivedAt":"2012-05-02T10:40:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThere is no need for it to be non-const, and this avoids the need\nfor casting away constness of argv elements.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fetch-pack.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 10db15b..7e9d62f 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, ret, nr_heads;\n \tstruct ref *ref = NULL;\n-\tchar *dest = NULL, **heads;\n+\tconst char *dest = NULL;\n+\tchar **heads;\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n@@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tusage(fetch_pack_usage);\n \t\t}\n-\t\tdest = (char *)arg;\n+\t\tdest = arg;\n \t\theads = (char **)(argv + i + 1);\n \t\tnr_heads = argc - i - 1;\n \t\tbreak;\n@@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n-\t\tconn = git_connect(fd, (char *)dest, args.uploadpack,\n+\t\tconn = git_connect(fd, dest, args.uploadpack,\n \t\t\t\t args.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-- \n1.7.10\n"},{"id":"190534","messageId":"1335955259-15309-3-git-send-email-mhagger@alum.mit.edu","threadId":"30392","inReplyTo":"1335955259-15309-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T10:40:59Z","receivedAt":"2012-05-02T10:40:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty \n\nThe old code cast away the constness of the strings passed to the\nfunction in argument argv[], which could result in their being\nmodified by filter_refs(). Moreover, if refs were passed via stdin,\nthen the memory allocated for them was never freed (though, of course,\nthis function is only called once so it is not a real problem).\n\nFix both errors by copying *all* reference names into our own array\nand always freeing the array at the end of the function.\n\nSigned-off-by: Michael Haggerty \n---\n\nI understand that it is not crucial to free memory allocated in a\ncmd_*() function but it is unclear to me whether it is *preferred* to\nlet the process clean up take care of things. If so, the last chunk\nof this patch can be omitted.\n\n builtin/fetch-pack.c | 149 +++++++++++++++++++++++++-------------------------\n 1 file changed, 75 insertions(+), 74 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 7e9d62f..5f769e9 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -899,10 +899,11 @@ static void fetch_pack_setup(void)\n \n int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, ret, nr_heads;\n+\tint i, ret;\n \tstruct ref *ref = NULL;\n \tconst char *dest = NULL;\n-\tchar **heads;\n+\tint alloc_heads = 0, nr_heads = 0;\n+\tchar **heads = NULL;\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n@@ -910,86 +911,82 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n-\tnr_heads = 0;\n-\theads = NULL;\n-\tfor (i = 1; i < argc; i++) {\n+\tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\n \t\tconst char *arg = argv[i];\n \n-\t\tif (*arg == '-') {\n-\t\t\tif (!prefixcmp(arg, \"--upload-pack=\")) {\n-\t\t\t\targs.uploadpack = arg + 14;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!prefixcmp(arg, \"--exec=\")) {\n-\t\t\t\targs.uploadpack = arg + 7;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--quiet\", arg) || !strcmp(\"-q\", arg)) {\n-\t\t\t\targs.quiet = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--keep\", arg) || !strcmp(\"-k\", arg)) {\n-\t\t\t\targs.lock_pack = args.keep_pack;\n-\t\t\t\targs.keep_pack = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--thin\", arg)) {\n-\t\t\t\targs.use_thin_pack = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--include-tag\", arg)) {\n-\t\t\t\targs.include_tag = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--all\", arg)) {\n-\t\t\t\targs.fetch_all = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--stdin\", arg)) {\n-\t\t\t\targs.stdin_refs = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"-v\", arg)) {\n-\t\t\t\targs.verbose = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!prefixcmp(arg, \"--depth=\")) {\n-\t\t\t\targs.depth = strtol(arg + 8, NULL, 0);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--no-progress\", arg)) {\n-\t\t\t\targs.no_progress = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--stateless-rpc\", arg)) {\n-\t\t\t\targs.stateless_rpc = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--lock-pack\", arg)) {\n-\t\t\t\targs.lock_pack = 1;\n-\t\t\t\tpack_lockfile_ptr = &pack_lockfile;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tusage(fetch_pack_usage);\n+\t\tif (!prefixcmp(arg, \"--upload-pack=\")) {\n+\t\t\targs.uploadpack = arg + 14;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!prefixcmp(arg, \"--exec=\")) {\n+\t\t\targs.uploadpack = arg + 7;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--quiet\", arg) || !strcmp(\"-q\", arg)) {\n+\t\t\targs.quiet = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--keep\", arg) || !strcmp(\"-k\", arg)) {\n+\t\t\targs.lock_pack = args.keep_pack;\n+\t\t\targs.keep_pack = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--thin\", arg)) {\n+\t\t\targs.use_thin_pack = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--include-tag\", arg)) {\n+\t\t\targs.include_tag = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--all\", arg)) {\n+\t\t\targs.fetch_all = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--stdin\", arg)) {\n+\t\t\targs.stdin_refs = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"-v\", arg)) {\n+\t\t\targs.verbose = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!prefixcmp(arg, \"--depth=\")) {\n+\t\t\targs.depth = strtol(arg + 8, NULL, 0);\n+\t\t\tcontinue;\n \t\t}\n-\t\tdest = arg;\n-\t\theads = (char **)(argv + i + 1);\n-\t\tnr_heads = argc - i - 1;\n-\t\tbreak;\n+\t\tif (!strcmp(\"--no-progress\", arg)) {\n+\t\t\targs.no_progress = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--stateless-rpc\", arg)) {\n+\t\t\targs.stateless_rpc = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(\"--lock-pack\", arg)) {\n+\t\t\targs.lock_pack = 1;\n+\t\t\tpack_lockfile_ptr = &pack_lockfile;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tusage(fetch_pack_usage);\n \t}\n-\tif (!dest)\n+\tif (i < argc)\n+\t\tdest = argv[i++];\n+\telse\n \t\tusage(fetch_pack_usage);\n \n+\t/*\n+\t * Copy refs from cmdline to new growable list, then append\n+\t * any refs from the standard input.\n+\t */\n+\tALLOC_GROW(heads, argc - i, alloc_heads);\n+\tfor (; i < argc; i++)\n+\t\theads[nr_heads++] = xstrdup(argv[i]);\n+\n \tif (args.stdin_refs) {\n-\t\t/*\n-\t\t * Copy refs from cmdline to new growable list, then\n-\t\t * append the refs from the standard input.\n-\t\t */\n-\t\tint alloc_heads = nr_heads;\n-\t\tint size = nr_heads * sizeof(*heads);\n-\t\theads = memcpy(xmalloc(size), heads, size);\n \t\tif (args.stateless_rpc) {\n-\t\t\t/* in stateless RPC mode we use pkt-line to read\n+\t\t\t/*\n+\t\t\t * in stateless RPC mode we use pkt-line to read\n \t\t\t * from stdin, until we get a flush packet\n \t\t\t */\n \t\t\tstatic char line[1000];\n@@ -1055,6 +1052,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tref = ref->next;\n \t}\n \n+\tfor (i = 0; i < nr_heads; ++i)\n+\t\tfree(heads[i]);\n+\tfree(heads);\n+\n \treturn ret;\n }\n \n-- \n1.7.10\n"},{"id":"190535","messageId":"CACsJy8AE7Y1Y5Drda39OfSKwTQN7oSSrb2Ai+jq_hi8XC4dtsw@mail.gmail.com","threadId":"30392","inReplyTo":"1335955259-15309-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-02T11:14:36Z","receivedAt":"2012-05-02T11:14:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 2, 2012 at 5:40 PM, wrote:\n>                const char *arg = argv[i];\n>\n> -               if (*arg == '-') {\n> -                       if (!prefixcmp(arg, \"--upload-pack=\")) {\n> -                               args.uploadpack = arg + 14;\n> -                               continue;\n> -                       }\n> -                       if (!prefixcmp(arg, \"--exec=\")) {\n> -                               args.uploadpack = arg + 7;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--quiet\", arg) || !strcmp(\"-q\", arg)) {\n> -                               args.quiet = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--keep\", arg) || !strcmp(\"-k\", arg)) {\n> -                               args.lock_pack = args.keep_pack;\n> -                               args.keep_pack = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--thin\", arg)) {\n> -                               args.use_thin_pack = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--include-tag\", arg)) {\n> -                               args.include_tag = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--all\", arg)) {\n> -                               args.fetch_all = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--stdin\", arg)) {\n> -                               args.stdin_refs = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"-v\", arg)) {\n> -                               args.verbose = 1;\n> -                               continue;\n> -                       }\n> -                       if (!prefixcmp(arg, \"--depth=\")) {\n> -                               args.depth = strtol(arg + 8, NULL, 0);\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--no-progress\", arg)) {\n> -                               args.no_progress = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--stateless-rpc\", arg)) {\n> -                               args.stateless_rpc = 1;\n> -                               continue;\n> -                       }\n> -                       if (!strcmp(\"--lock-pack\", arg)) {\n> -                               args.lock_pack = 1;\n> -                               pack_lockfile_ptr = &pack_lockfile;\n> -                               continue;\n> -                       }\n> -                       usage(fetch_pack_usage);\n> +               if (!prefixcmp(arg, \"--upload-pack=\")) {\n> +                       args.uploadpack = arg + 14;\n> +                       continue;\n> +               }\n> +               if (!prefixcmp(arg, \"--exec=\")) {\n> +                       args.uploadpack = arg + 7;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--quiet\", arg) || !strcmp(\"-q\", arg)) {\n> +                       args.quiet = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--keep\", arg) || !strcmp(\"-k\", arg)) {\n> +                       args.lock_pack = args.keep_pack;\n> +                       args.keep_pack = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--thin\", arg)) {\n> +                       args.use_thin_pack = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--include-tag\", arg)) {\n> +                       args.include_tag = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--all\", arg)) {\n> +                       args.fetch_all = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--stdin\", arg)) {\n> +                       args.stdin_refs = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"-v\", arg)) {\n> +                       args.verbose = 1;\n> +                       continue;\n> +               }\n> +               if (!prefixcmp(arg, \"--depth=\")) {\n> +                       args.depth = strtol(arg + 8, NULL, 0);\n> +                       continue;\n>                }\n> -               dest = arg;\n> -               heads = (char **)(argv + i + 1);\n> -               nr_heads = argc - i - 1;\n> -               break;\n> +               if (!strcmp(\"--no-progress\", arg)) {\n> +                       args.no_progress = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--stateless-rpc\", arg)) {\n> +                       args.stateless_rpc = 1;\n> +                       continue;\n> +               }\n> +               if (!strcmp(\"--lock-pack\", arg)) {\n> +                       args.lock_pack = 1;\n> +                       pack_lockfile_ptr = &pack_lockfile;\n> +                       continue;\n> +               }\n> +               usage(fetch_pack_usage);\n\nUgh, perhaps you can convert the above to parse_options() too while\nyou're making changes in this part? You can say no, I'll do it (my\nitch anyway).\n-- \nDuy\n"},{"id":"190549","messageId":"4FA13835.7080204@alum.mit.edu","threadId":"30392","inReplyTo":"CACsJy8AE7Y1Y5Drda39OfSKwTQN7oSSrb2Ai+jq_hi8XC4dtsw@mail.gmail.com","subject":"Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-02T13:35:49Z","receivedAt":"2012-05-02T13:35:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:\n> On Wed, May 2, 2012 at 5:40 PM, wrote:\n>> const char *arg = argv[i];\n>>\n>> - if (*arg == '-') {\n>> - if (!prefixcmp(arg, \"--upload-pack=\")) {\n>> - args.uploadpack = arg + 14;\n>> - continue;\n>> - }\n>> [...]\n> Ugh, perhaps you can convert the above to parse_options() too while\n> you're making changes in this part? You can say no, I'll do it (my\n> itch anyway).\n\nSorry, I have too many balls in the air already. But feel free to veto \nmy patch if my changes make it harder to switch to parse_options().\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"190565","messageId":"1335969490-9181-1-git-send-email-pclouds@gmail.com","threadId":"30392","inReplyTo":"4FA13835.7080204@alum.mit.edu","subject":"[PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-02T14:38:07Z","receivedAt":"2012-05-02T14:38:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 2, 2012 at 8:35 PM, Michael Haggerty wrote:\n> On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:\n>>\n>> On Wed, May 2, 2012 at 5:40 PM, wrote:\n>>>\n>>> const char *arg = argv[i];\n>>>\n>>> - if (*arg == '-') {\n>>> - if (!prefixcmp(arg, \"--upload-pack=\")) {\n>>> - args.uploadpack = arg + 14;\n>>> - continue;\n>>> - }\n>>> [...]\n>>\n>> Ugh, perhaps you can convert the above to parse_options() too while\n>> you're making changes in this part? You can say no, I'll do it (my\n>> itch anyway).\n>\n>\n> Sorry, I have too many balls in the air already. But feel free to veto my\n> patch if my changes make it harder to switch to parse_options().\n\nNo problem. My new patch may slow down your patches to master though.\n\nMichael Haggerty (2):\n cmd_fetch_pack(): declare dest to be const\n cmd_fetch_pack(): fix constness problem and memory leak\n\nNguyễn Thái Ngọc Duy (1):\n fetch-pack: use parse_options()\n\n builtin/fetch-pack.c | 138 ++++++++++++++++++++------------------------------\n fetch-pack.h | 20 ++++----\n 2 files changed, 65 insertions(+), 93 deletions(-)\n\n-- \n1.7.8.36.g69ee2\n"},{"id":"190563","messageId":"1335969490-9181-2-git-send-email-pclouds@gmail.com","threadId":"30392","inReplyTo":"1335969490-9181-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/3] cmd_fetch_pack(): declare dest to be const","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-02T14:38:08Z","receivedAt":"2012-05-02T14:38:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Michael Haggerty \n\nThere is no need for it to be non-const, and this avoids the need\nfor casting away constness of argv elements.\n\nSigned-off-by: Michael Haggerty \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/fetch-pack.c | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 10db15b..7e9d62f 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, ret, nr_heads;\n \tstruct ref *ref = NULL;\n-\tchar *dest = NULL, **heads;\n+\tconst char *dest = NULL;\n+\tchar **heads;\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n@@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tusage(fetch_pack_usage);\n \t\t}\n-\t\tdest = (char *)arg;\n+\t\tdest = arg;\n \t\theads = (char **)(argv + i + 1);\n \t\tnr_heads = argc - i - 1;\n \t\tbreak;\n@@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tfd[0] = 0;\n \t\tfd[1] = 1;\n \t} else {\n-\t\tconn = git_connect(fd, (char *)dest, args.uploadpack,\n+\t\tconn = git_connect(fd, dest, args.uploadpack,\n \t\t\t\t args.verbose ? CONNECT_VERBOSE : 0);\n \t}\n \n-- \n1.7.8.36.g69ee2\n"},{"id":"190566","messageId":"1335969490-9181-3-git-send-email-pclouds@gmail.com","threadId":"30392","inReplyTo":"1335969490-9181-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] fetch-pack: use parse_options()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-02T14:38:09Z","receivedAt":"2012-05-02T14:38:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I _think_ this also fixes a case when --keep is passed twice, then\n lock_pack is set to 1, but pack_lockfile_ptr is not set while it is\n set when --lock-pack is given.\n\n builtin/fetch-pack.c | 110 ++++++++++++++++++--------------------------------\n fetch-pack.h | 20 +++++-----\n 2 files changed, 49 insertions(+), 81 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 7e9d62f..65ac111 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -10,6 +10,7 @@\n #include \"remote.h\"\n #include \"run-command.h\"\n #include \"transport.h\"\n+#include \"parse-options.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -22,10 +23,12 @@ static struct fetch_pack_args args = {\n \t/* .uploadpack = */ \"git-upload-pack\",\n };\n \n-static const char fetch_pack_usage[] =\n-\"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] \"\n-\"[--include-tag] [--upload-pack=] [--depth=] \"\n-\"[--no-progress] [-v] [:] [...]\";\n+static const char* fetch_pack_usage[] = {\n+\t\"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] \"\n+\t\"[--include-tag] [--upload-pack=] [--depth=] \"\n+\t\"[--no-progress] [-v] [:] [...]\",\n+\tNULL\n+};\n \n #define COMPLETE\t(1U << 0)\n #define COMMON\t\t(1U << 1)\n@@ -906,79 +909,44 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n+\tint progress = 1;\n \tstruct child_process *conn;\n+\tstruct option opts[] = {\n+\t\tOPT_STRING(0, \"upload-pack\", &args.uploadpack, \"path\",\n+\t\t\t \"path to upload pack on remote end\"),\n+\t\tOPT_STRING( 0 , \"exec\", &args.uploadpack, \"upload-pack\",\n+\t\t\t \"path to upload pack on remote end\"),\n+\t\tOPT__QUIET(&args.quiet, \"do not print results to stdout\"),\n+\t\tOPT_COUNTUP('k', \"keep\", &args.keep_pack, \"keep downloaded pack\"),\n+\t\tOPT_BOOL(0, \"thin\", &args.use_thin_pack, \"create thin packs\"),\n+\t\tOPT_BOOL(0, \"include-tag\", &args.include_tag,\n+\t\t\t \"include tag objects that refer to objects to be packed\"),\n+\t\tOPT_BOOL(0, \"all\", &args.fetch_all, \"fetch from all remotes\" ),\n+\t\tOPT_BOOL(0, \"stdin\", &args.stdin_refs, \"read refs from stdin\" ),\n+\t\tOPT__VERBOSE(&args.verbose, \"be more verbose\"),\n+\t\tOPT_INTEGER(0, \"depth\", &args.depth,\n+\t\t\t \"deepen history of shallow clone\"),\n+\t\tOPT_BOOL(0, \"progress\", &progress, \"show progress\"),\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &args.stateless_rpc, \"use stateless RPC\"),\n+\t\tOPT_BOOL(0, \"lock-pack\", &args.lock_pack, \"lock the downloaded pack\"),\n+\t\tOPT_END()\n+\t};\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n \tnr_heads = 0;\n \theads = NULL;\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\n-\t\tif (*arg == '-') {\n-\t\t\tif (!prefixcmp(arg, \"--upload-pack=\")) {\n-\t\t\t\targs.uploadpack = arg + 14;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!prefixcmp(arg, \"--exec=\")) {\n-\t\t\t\targs.uploadpack = arg + 7;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--quiet\", arg) || !strcmp(\"-q\", arg)) {\n-\t\t\t\targs.quiet = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--keep\", arg) || !strcmp(\"-k\", arg)) {\n-\t\t\t\targs.lock_pack = args.keep_pack;\n-\t\t\t\targs.keep_pack = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--thin\", arg)) {\n-\t\t\t\targs.use_thin_pack = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--include-tag\", arg)) {\n-\t\t\t\targs.include_tag = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--all\", arg)) {\n-\t\t\t\targs.fetch_all = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--stdin\", arg)) {\n-\t\t\t\targs.stdin_refs = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"-v\", arg)) {\n-\t\t\t\targs.verbose = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!prefixcmp(arg, \"--depth=\")) {\n-\t\t\t\targs.depth = strtol(arg + 8, NULL, 0);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--no-progress\", arg)) {\n-\t\t\t\targs.no_progress = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--stateless-rpc\", arg)) {\n-\t\t\t\targs.stateless_rpc = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(\"--lock-pack\", arg)) {\n-\t\t\t\targs.lock_pack = 1;\n-\t\t\t\tpack_lockfile_ptr = &pack_lockfile;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tusage(fetch_pack_usage);\n-\t\t}\n-\t\tdest = arg;\n-\t\theads = (char **)(argv + i + 1);\n-\t\tnr_heads = argc - i - 1;\n-\t\tbreak;\n-\t}\n-\tif (!dest)\n-\t\tusage(fetch_pack_usage);\n+\targc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0);\n+\targs.no_progress = !progress;\n+\tif (args.keep_pack > 1)\n+\t\targs.lock_pack = 1;\n+\tif (args.lock_pack)\n+\t\tpack_lockfile_ptr = &pack_lockfile;\n+\tdest = argv[0];\n+\tif (!argc || !dest)\n+\t\tusage_with_options(fetch_pack_usage, opts);\n+\theads = (char **)(argv + 1);\n+\tnr_heads = argc - 1;\n \n \tif (args.stdin_refs) {\n \t\t/*\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 7c2069c..d440162 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -5,16 +5,16 @@ struct fetch_pack_args {\n \tconst char *uploadpack;\n \tint unpacklimit;\n \tint depth;\n-\tunsigned quiet:1,\n-\t\tkeep_pack:1,\n-\t\tlock_pack:1,\n-\t\tuse_thin_pack:1,\n-\t\tfetch_all:1,\n-\t\tstdin_refs:1,\n-\t\tverbose:1,\n-\t\tno_progress:1,\n-\t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\tint quiet;\n+\tint keep_pack;\n+\tint lock_pack;\n+\tint use_thin_pack;\n+\tint fetch_all;\n+\tint stdin_refs;\n+\tint verbose;\n+\tint no_progress;\n+\tint include_tag;\n+\tint stateless_rpc;\n };\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n1.7.8.36.g69ee2\n"},{"id":"190564","messageId":"1335969490-9181-4-git-send-email-pclouds@gmail.com","threadId":"30392","inReplyTo":"1335969490-9181-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-05-02T14:38:10Z","receivedAt":"2012-05-02T14:38:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Michael Haggerty \n\nThe old code cast away the constness of the strings passed to the\nfunction in argument argv[], which could result in their being\nmodified by filter_refs(). Moreover, if refs were passed via stdin,\nthen the memory allocated for them was never freed (though, of course,\nthis function is only called once so it is not a real problem).\n\nFix both errors by copying *all* reference names into our own array\nand always freeing the array at the end of the function.\n\nSigned-off-by: Michael Haggerty \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/fetch-pack.c | 31 +++++++++++++++++--------------\n 1 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 65ac111..beabdc2 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -902,10 +902,11 @@ static void fetch_pack_setup(void)\n \n int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, ret, nr_heads;\n+\tint i, ret;\n \tstruct ref *ref = NULL;\n \tconst char *dest = NULL;\n-\tchar **heads;\n+\tint alloc_heads = 0, nr_heads = 0;\n+\tchar **heads = NULL;\n \tint fd[2];\n \tchar *pack_lockfile = NULL;\n \tchar **pack_lockfile_ptr = NULL;\n@@ -934,8 +935,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n-\tnr_heads = 0;\n-\theads = NULL;\n \targc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0);\n \targs.no_progress = !progress;\n \tif (args.keep_pack > 1)\n@@ -945,19 +944,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tdest = argv[0];\n \tif (!argc || !dest)\n \t\tusage_with_options(fetch_pack_usage, opts);\n-\theads = (char **)(argv + 1);\n-\tnr_heads = argc - 1;\n+\n+\t/*\n+\t * Copy refs from cmdline to new growable list, then append\n+\t * any refs from the standard input.\n+\t */\n+\tALLOC_GROW(heads, argc - 1, alloc_heads);\n+\tfor (i = 1; i < argc; i++)\n+\t\theads[nr_heads++] = xstrdup(argv[i]);\n \n \tif (args.stdin_refs) {\n-\t\t/*\n-\t\t * Copy refs from cmdline to new growable list, then\n-\t\t * append the refs from the standard input.\n-\t\t */\n-\t\tint alloc_heads = nr_heads;\n-\t\tint size = nr_heads * sizeof(*heads);\n-\t\theads = memcpy(xmalloc(size), heads, size);\n \t\tif (args.stateless_rpc) {\n-\t\t\t/* in stateless RPC mode we use pkt-line to read\n+\t\t\t/*\n+\t\t\t * in stateless RPC mode we use pkt-line to read\n \t\t\t * from stdin, until we get a flush packet\n \t\t\t */\n \t\t\tstatic char line[1000];\n@@ -1023,6 +1022,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\tref = ref->next;\n \t}\n \n+\tfor (i = 0; i < nr_heads; ++i)\n+\t\tfree(heads[i]);\n+\tfree(heads);\n+\n \treturn ret;\n }\n \n-- \n1.7.8.36.g69ee2\n"},{"id":"190579","messageId":"7vhavyijyw.fsf@alter.siamese.dyndns.org","threadId":"30392","inReplyTo":"CACsJy8AE7Y1Y5Drda39OfSKwTQN7oSSrb2Ai+jq_hi8XC4dtsw@mail.gmail.com","subject":"Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-02T17:14:15Z","receivedAt":"2012-05-02T17:14:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> Ugh, perhaps you can convert the above to parse_options() too while\n> you're making changes in this part?\n\nPlease don't. A parse-opt'ification is a very low priority change for a\ncommand nobody runs from the command line, and is not worth the hassle of\nhaving to deal with unnecessary merge conflicts.\n\nDo so when the codebase around the area is otherwise quiet.\n"},{"id":"191736","messageId":"4FB7A893.8080109@alum.mit.edu","threadId":"30392","inReplyTo":"1335955259-15309-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Fix some constness errors in fetch-pack","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-19T14:05:07Z","receivedAt":"2012-05-19T14:05:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/02/2012 12:40 PM, mhagger@alum.mit.edu wrote:\n> From: Michael Haggerty\n>\n> Fix some constness errors that I noticed while reading the code in\n> builtin/fetch-pack.c.\n>\n> Michael Haggerty (2):\n> cmd_fetch_pack(): declare dest to be const\n> cmd_fetch_pack(): fix constness problem and memory leak\n>\n> builtin/fetch-pack.c | 152 +++++++++++++++++++++++++-------------------------\n> 1 file changed, 77 insertions(+), 75 deletions(-)\n\nI think that these patches got dropped because of an inconclusive \ndiscussion [1]: Duy suggested converting the code for fetch-pack to use \nparse_options(), which idea Junio didn't like. But I think that any \nobjections were raised against these two patches, which are just simple \ncleanups.\n\nMichael\n\n[1] http://comments.gmane.org/gmane.comp.version-control.git/196797\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"191777","messageId":"7vk4069ug8.fsf@alter.siamese.dyndns.org","threadId":"30392","inReplyTo":"1335955259-15309-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-21T01:47:03Z","receivedAt":"2012-05-21T01:47:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> I understand that it is not crucial to free memory allocated in a\n> cmd_*() function but it is unclear to me whether it is *preferred* to\n> let the process clean up take care of things.\n\nTraditionally, the cmd_foo() functions roughly correspond to main() in\nother programs, so from that point of view, \"it is not crucial to free\" is\nan understatement. It is not even worth wasting people's time to (1)\ndecide which way is *preferred* and to (2) churn the code only to match\nwhichever way.\n\nThese cmd_foo() functions, being roughly equivalent to main(), have logic\nto interpret the end-user's intentions (i.e. parse_options()), and carry\nout that intention. In the long run, some _other_ parts of the system may\nwant to do \"foo\" (whatever that \"foo\" may be) from inside the process\nwithout forking, and the first step to do so may be to split cmd_foo()\ninto two, one that does \"parse options\", and the other that does \"foo\".\nAt that point, it starts to matter that we make the part that does \"foo\"\nleak free, especially if such a caller (or callers) can ask to do \"foo\"\nnumber of times.\n\nIf you have a plan to split cmd_fetch_pack() and make other parts of the\nsystem call it, probably restructuring the current separation between\n\"figure out what refs will be fetched\" done in cmd_fetch_pack() and \"fetch\nthese refs in heads[] array\" in fetch_pack() into something else (like\n\"the new caller also wants to read the list of refs from the standard\ninput, instead of having parse them into head[] array itself\"), then\nfreeing the memory would matter a lot more, and the approach this patch\ntakes is a first step in the right direction, I would think.\n\nIt also seems that some cruft has snuck into this patch, e.g. like this\npart,\n\n> -\tint i, ret, nr_heads;\n> +\tint i, ret;\n\nthat do not have anything to do with \"fix constness\" nor \"memory leak\".\n"},{"id":"191794","messageId":"4FB9F93C.4010308@alum.mit.edu","threadId":"30392","inReplyTo":"7vk4069ug8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T08:13:48Z","receivedAt":"2012-05-21T08:13:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/21/2012 03:47 AM, Junio C Hamano wrote:\n> mhagger@alum.mit.edu writes:\n>\n>> I understand that it is not crucial to free memory allocated in a\n>> cmd_*() function but it is unclear to me whether it is *preferred* to\n>> let the process clean up take care of things.\n>\n> Traditionally, the cmd_foo() functions roughly correspond to main() in\n> other programs, so from that point of view, \"it is not crucial to free\" is\n> an understatement. It is not even worth wasting people's time to (1)\n> decide which way is *preferred* and to (2) churn the code only to match\n> whichever way.\n\nOK, thanks for the info. I will remove the \"freeing-memory\" part of the \npatch.\n\n> If you have a plan to split cmd_fetch_pack() and make other parts of the\n> system call it, [...]\n\nNo, I have no plans for cmd_fetch_pack() besides cleaning up the \nconstness errors that I found when randomly reading the code.\n\n> It also seems that some cruft has snuck into this patch, e.g. like this\n> part,\n>\n>> -\tint i, ret, nr_heads;\n>> +\tint i, ret;\n>\n> that do not have anything to do with \"fix constness\" nor \"memory leak\".\n\nThis particular hunk is part of moving alloc_heads, nr_heads, and heads \ntogether to make it more obvious that they are part of an ALLOC_GROW \ntriplet. Previously alloc_heads was a block-local variable used only in \nthe case of the --stdin option.\n\nBut I admit that the patch is harder than necessary to read because of \nthe indentation changes etc, so I will break it up into more digestible \nquanta.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}