{"thread":{"id":"30572","subject":"[PATCH v2 2/4] cmd_fetch_pack(): handle non-option arguments outside of the loop","startedAt":"2012-05-21T07:59:55Z","lastAt":"2012-05-21T07:59:59Z","messageCount":5,"participants":["mhagger@alum.mit.edu"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"191790","messageId":"1337587199-21099-1-git-send-email-mhagger@alum.mit.edu","threadId":"30572","inReplyTo":null,"subject":"[PATCH v2 0/4] Fix some constness errors in fetch-pack","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T07:59:55Z","receivedAt":"2012-05-21T07:59:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nFix some constness errors that I noticed while reading the code in\nbuiltin/fetch-pack.c.\n\nThis version differs from v1 as follows:\n\n* cmd_fetch_pack() is not made to free the memory that it allocates.\n\n* The second patch is broken into separate pieces to make each piece\n  more readable.\n\nMichael Haggerty (4):\n  cmd_fetch_pack(): declare dest to be const\n  cmd_fetch_pack(): handle non-option arguments outside of the loop\n  cmd_fetch_pack(): combine the loop termination conditions\n  cmd_fetch_pack(): respect constness of argv parameter\n\n builtin/fetch-pack.c |  145 ++++++++++++++++++++++++--------------------------\n 1 file changed, 71 insertions(+), 74 deletions(-)\n\n-- \n1.7.10\n"},{"id":"191791","messageId":"1337587199-21099-2-git-send-email-mhagger@alum.mit.edu","threadId":"30572","inReplyTo":"1337587199-21099-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/4] cmd_fetch_pack(): declare dest to be const","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T07:59:56Z","receivedAt":"2012-05-21T07:59:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThere is no need for it to be non-const, and this avoids the need\nfor casting away the constness of an argv element.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\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":"191788","messageId":"1337587199-21099-3-git-send-email-mhagger@alum.mit.edu","threadId":"30572","inReplyTo":"1337587199-21099-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/4] cmd_fetch_pack(): handle non-option arguments outside of the loop","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T07:59:57Z","receivedAt":"2012-05-21T07:59:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThis makes it more obvious that the code is always executed unless\nthere is an error, and that the first initialization of nr_heads is\nunnecessary.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c |   12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 7e9d62f..dabf5e9 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -910,7 +910,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 \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -972,14 +971,17 @@ 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 = arg;\n-\t\theads = (char **)(argv + i + 1);\n-\t\tnr_heads = argc - i - 1;\n \t\tbreak;\n \t}\n-\tif (!dest)\n+\n+\tif (i < argc)\n+\t\tdest = argv[i++];\n+\telse\n \t\tusage(fetch_pack_usage);\n \n+\theads = (char **)(argv + i);\n+\tnr_heads = argc - i;\n+\n \tif (args.stdin_refs) {\n \t\t/*\n \t\t * Copy refs from cmdline to new growable list, then\n-- \n1.7.10\n"},{"id":"191792","messageId":"1337587199-21099-4-git-send-email-mhagger@alum.mit.edu","threadId":"30572","inReplyTo":"1337587199-21099-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/4] cmd_fetch_pack(): combine the loop termination conditions","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T07:59:58Z","receivedAt":"2012-05-21T07:59:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nIf an argument that does not start with '-' is found, the loop is\nterminated.  So move that check into the for-loop condition.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch-pack.c |  113 ++++++++++++++++++++++++--------------------------\n 1 file changed, 55 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex dabf5e9..96849a4 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -911,67 +911,64 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"fetch-pack\");\n \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\tbreak;\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\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 \n \tif (i < argc)\n-- \n1.7.10\n"},{"id":"191793","messageId":"1337587199-21099-5-git-send-email-mhagger@alum.mit.edu","threadId":"30572","inReplyTo":"1337587199-21099-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 4/4] cmd_fetch_pack(): respect constness of argv parameter","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-05-21T07:59:59Z","receivedAt":"2012-05-21T07:59:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\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().  Fix by copying reference names from argv\nand putting them into our own array (similarly to how refnames passed\nto stdin were already handled).\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nThis change results in heads being left set to NULL if nr_heads==0,\nbut all of the code paths are OK with this.\n\n builtin/fetch-pack.c |   23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 96849a4..7ad9e54 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,7 +911,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch-pack\");\n \n-\theads = NULL;\n \tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\n \t\tconst char *arg = argv[i];\n \n@@ -976,17 +976,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \telse\n \t\tusage(fetch_pack_usage);\n \n-\theads = (char **)(argv + i);\n-\tnr_heads = argc - i;\n-\n+\t/*\n+\t * Copy refs from cmdline to growable list, then append any\n+\t * 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 \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 * from stdin, until we get a flush packet\n-- \n1.7.10\n"}]}