[PATCH 0/2] Fix some constness errors in fetch-pack

[PATCH 0/2] Fix some constness errors in fetch-pack

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Fix some constness errors that I noticed while reading the code in
builtin/fetch-pack.c.

Michael Haggerty (2):
  cmd_fetch_pack(): declare dest to be const
  cmd_fetch_pack(): fix constness problem and memory leak

 builtin/fetch-pack.c |  152 +++++++++++++++++++++++++-------------------------
 1 file changed, 77 insertions(+), 75 deletions(-)

-- 
1.7.10

[PATCH 1/2] cmd_fetch_pack(): declare dest to be const

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

There is no need for it to be non-const, and this avoids the need
for casting away constness of argv elements.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 10db15b..7e9d62f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, ret, nr_heads;
 	struct ref *ref = NULL;
-	char *dest = NULL, **heads;
+	const char *dest = NULL;
+	char **heads;
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
@@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			}
 			usage(fetch_pack_usage);
 		}
-		dest = (char *)arg;
+		dest = arg;
 		heads = (char **)(argv + i + 1);
 		nr_heads = argc - i - 1;
 		break;
@@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		fd[0] = 0;
 		fd[1] = 1;
 	} else {
-		conn = git_connect(fd, (char *)dest, args.uploadpack,
+		conn = git_connect(fd, dest, args.uploadpack,
 				   args.verbose ? CONNECT_VERBOSE : 0);
 	}
 
-- 
1.7.10

[PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The old code cast away the constness of the strings passed to the
function in argument argv[], which could result in their being
modified by filter_refs().  Moreover, if refs were passed via stdin,
then the memory allocated for them was never freed (though, of course,
this function is only called once so it is not a real problem).

Fix both errors by copying *all* reference names into our own array
and always freeing the array at the end of the function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I understand that it is not crucial to free memory allocated in a
cmd_*() function but it is unclear to me whether it is *preferred* to
let the process clean up take care of things.  If so, the last chunk
of this patch can be omitted.

 builtin/fetch-pack.c |  149 +++++++++++++++++++++++++-------------------------
 1 file changed, 75 insertions(+), 74 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 7e9d62f..5f769e9 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -899,10 +899,11 @@ static void fetch_pack_setup(void)
 
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
-	int i, ret, nr_heads;
+	int i, ret;
 	struct ref *ref = NULL;
 	const char *dest = NULL;
-	char **heads;
+	int alloc_heads = 0, nr_heads = 0;
+	char **heads = NULL;
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
@@ -910,86 +911,82 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("fetch-pack");
 
-	nr_heads = 0;
-	heads = NULL;
-	for (i = 1; i < argc; i++) {
+	for (i = 1; i < argc && *argv[i] == '-'; i++) {
 		const char *arg = argv[i];
 
-		if (*arg == '-') {
-			if (!prefixcmp(arg, "--upload-pack=")) {
-				args.uploadpack = arg + 14;
-				continue;
-			}
-			if (!prefixcmp(arg, "--exec=")) {
-				args.uploadpack = arg + 7;
-				continue;
-			}
-			if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
-				args.quiet = 1;
-				continue;
-			}
-			if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
-				args.lock_pack = args.keep_pack;
-				args.keep_pack = 1;
-				continue;
-			}
-			if (!strcmp("--thin", arg)) {
-				args.use_thin_pack = 1;
-				continue;
-			}
-			if (!strcmp("--include-tag", arg)) {
-				args.include_tag = 1;
-				continue;
-			}
-			if (!strcmp("--all", arg)) {
-				args.fetch_all = 1;
-				continue;
-			}
-			if (!strcmp("--stdin", arg)) {
-				args.stdin_refs = 1;
-				continue;
-			}
-			if (!strcmp("-v", arg)) {
-				args.verbose = 1;
-				continue;
-			}
-			if (!prefixcmp(arg, "--depth=")) {
-				args.depth = strtol(arg + 8, NULL, 0);
-				continue;
-			}
-			if (!strcmp("--no-progress", arg)) {
-				args.no_progress = 1;
-				continue;
-			}
-			if (!strcmp("--stateless-rpc", arg)) {
-				args.stateless_rpc = 1;
-				continue;
-			}
-			if (!strcmp("--lock-pack", arg)) {
-				args.lock_pack = 1;
-				pack_lockfile_ptr = &pack_lockfile;
-				continue;
-			}
-			usage(fetch_pack_usage);
+		if (!prefixcmp(arg, "--upload-pack=")) {
+			args.uploadpack = arg + 14;
+			continue;
+		}
+		if (!prefixcmp(arg, "--exec=")) {
+			args.uploadpack = arg + 7;
+			continue;
+		}
+		if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
+			args.quiet = 1;
+			continue;
+		}
+		if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
+			args.lock_pack = args.keep_pack;
+			args.keep_pack = 1;
+			continue;
+		}
+		if (!strcmp("--thin", arg)) {
+			args.use_thin_pack = 1;
+			continue;
+		}
+		if (!strcmp("--include-tag", arg)) {
+			args.include_tag = 1;
+			continue;
+		}
+		if (!strcmp("--all", arg)) {
+			args.fetch_all = 1;
+			continue;
+		}
+		if (!strcmp("--stdin", arg)) {
+			args.stdin_refs = 1;
+			continue;
+		}
+		if (!strcmp("-v", arg)) {
+			args.verbose = 1;
+			continue;
+		}
+		if (!prefixcmp(arg, "--depth=")) {
+			args.depth = strtol(arg + 8, NULL, 0);
+			continue;
 		}
-		dest = arg;
-		heads = (char **)(argv + i + 1);
-		nr_heads = argc - i - 1;
-		break;
+		if (!strcmp("--no-progress", arg)) {
+			args.no_progress = 1;
+			continue;
+		}
+		if (!strcmp("--stateless-rpc", arg)) {
+			args.stateless_rpc = 1;
+			continue;
+		}
+		if (!strcmp("--lock-pack", arg)) {
+			args.lock_pack = 1;
+			pack_lockfile_ptr = &pack_lockfile;
+			continue;
+		}
+		usage(fetch_pack_usage);
 	}
-	if (!dest)
+	if (i < argc)
+		dest = argv[i++];
+	else
 		usage(fetch_pack_usage);
 
+	/*
+	 * Copy refs from cmdline to new growable list, then append
+	 * any refs from the standard input.
+	 */
+	ALLOC_GROW(heads, argc - i, alloc_heads);
+	for (; i < argc; i++)
+		heads[nr_heads++] = xstrdup(argv[i]);
+
 	if (args.stdin_refs) {
-		/*
-		 * Copy refs from cmdline to new growable list, then
-		 * append the refs from the standard input.
-		 */
-		int alloc_heads = nr_heads;
-		int size = nr_heads * sizeof(*heads);
-		heads = memcpy(xmalloc(size), heads, size);
 		if (args.stateless_rpc) {
-			/* in stateless RPC mode we use pkt-line to read
+			/*
+			 * in stateless RPC mode we use pkt-line to read
 			 * from stdin, until we get a flush packet
 			 */
 			static char line[1000];
@@ -1055,6 +1052,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		ref = ref->next;
 	}
 
+	for (i = 0; i < nr_heads; ++i)
+		free(heads[i]);
+	free(heads);
+
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[Nguyen Thai Ngoc Duy]

On Wed, May 2, 2012 at 5:40 PM,  <mhagger@alum.mit.edu> wrote:
>                const char *arg = argv[i];
>
> -               if (*arg == '-') {
> -                       if (!prefixcmp(arg, "--upload-pack=")) {
> -                               args.uploadpack = arg + 14;
> -                               continue;
> -                       }
> -                       if (!prefixcmp(arg, "--exec=")) {
> -                               args.uploadpack = arg + 7;
> -                               continue;
> -                       }
> -                       if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> -                               args.quiet = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
> -                               args.lock_pack = args.keep_pack;
> -                               args.keep_pack = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--thin", arg)) {
> -                               args.use_thin_pack = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--include-tag", arg)) {
> -                               args.include_tag = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--all", arg)) {
> -                               args.fetch_all = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--stdin", arg)) {
> -                               args.stdin_refs = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("-v", arg)) {
> -                               args.verbose = 1;
> -                               continue;
> -                       }
> -                       if (!prefixcmp(arg, "--depth=")) {
> -                               args.depth = strtol(arg + 8, NULL, 0);
> -                               continue;
> -                       }
> -                       if (!strcmp("--no-progress", arg)) {
> -                               args.no_progress = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--stateless-rpc", arg)) {
> -                               args.stateless_rpc = 1;
> -                               continue;
> -                       }
> -                       if (!strcmp("--lock-pack", arg)) {
> -                               args.lock_pack = 1;
> -                               pack_lockfile_ptr = &pack_lockfile;
> -                               continue;
> -                       }
> -                       usage(fetch_pack_usage);
> +               if (!prefixcmp(arg, "--upload-pack=")) {
> +                       args.uploadpack = arg + 14;
> +                       continue;
> +               }
> +               if (!prefixcmp(arg, "--exec=")) {
> +                       args.uploadpack = arg + 7;
> +                       continue;
> +               }
> +               if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
> +                       args.quiet = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
> +                       args.lock_pack = args.keep_pack;
> +                       args.keep_pack = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--thin", arg)) {
> +                       args.use_thin_pack = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--include-tag", arg)) {
> +                       args.include_tag = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--all", arg)) {
> +                       args.fetch_all = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--stdin", arg)) {
> +                       args.stdin_refs = 1;
> +                       continue;
> +               }
> +               if (!strcmp("-v", arg)) {
> +                       args.verbose = 1;
> +                       continue;
> +               }
> +               if (!prefixcmp(arg, "--depth=")) {
> +                       args.depth = strtol(arg + 8, NULL, 0);
> +                       continue;
>                }
> -               dest = arg;
> -               heads = (char **)(argv + i + 1);
> -               nr_heads = argc - i - 1;
> -               break;
> +               if (!strcmp("--no-progress", arg)) {
> +                       args.no_progress = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--stateless-rpc", arg)) {
> +                       args.stateless_rpc = 1;
> +                       continue;
> +               }
> +               if (!strcmp("--lock-pack", arg)) {
> +                       args.lock_pack = 1;
> +                       pack_lockfile_ptr = &pack_lockfile;
> +                       continue;
> +               }
> +               usage(fetch_pack_usage);

Ugh, perhaps you can convert the above to parse_options() too while
you're making changes in this part? You can say no, I'll do it (my
itch anyway).
-- 
Duy

Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[Michael Haggerty]

On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:
> On Wed, May 2, 2012 at 5:40 PM,<mhagger@alum.mit.edu>  wrote:
>>                 const char *arg = argv[i];
>>
>> -               if (*arg == '-') {
>> -                       if (!prefixcmp(arg, "--upload-pack=")) {
>> -                               args.uploadpack = arg + 14;
>> -                               continue;
>> -                       }
>> [...]
> Ugh, perhaps you can convert the above to parse_options() too while
> you're making changes in this part? You can say no, I'll do it (my
> itch anyway).

Sorry, I have too many balls in the air already.  But feel free to veto 
my patch if my changes make it harder to switch to parse_options().

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion

[Nguyễn Thái Ngọc Duy]

On Wed, May 2, 2012 at 8:35 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/02/2012 01:14 PM, Nguyen Thai Ngoc Duy wrote:
>>
>> On Wed, May 2, 2012 at 5:40 PM,<mhagger@alum.mit.edu>  wrote:
>>>
>>>                const char *arg = argv[i];
>>>
>>> -               if (*arg == '-') {
>>> -                       if (!prefixcmp(arg, "--upload-pack=")) {
>>> -                               args.uploadpack = arg + 14;
>>> -                               continue;
>>> -                       }
>>> [...]
>>
>> Ugh, perhaps you can convert the above to parse_options() too while
>> you're making changes in this part? You can say no, I'll do it (my
>> itch anyway).
>
>
> Sorry, I have too many balls in the air already.  But feel free to veto my
> patch if my changes make it harder to switch to parse_options().

No problem. My new patch may slow down your patches to master though.

Michael Haggerty (2):
  cmd_fetch_pack(): declare dest to be const
  cmd_fetch_pack(): fix constness problem and memory leak

Nguyễn Thái Ngọc Duy (1):
  fetch-pack: use parse_options()

 builtin/fetch-pack.c |  138 ++++++++++++++++++++------------------------------
 fetch-pack.h         |   20 ++++----
 2 files changed, 65 insertions(+), 93 deletions(-)

-- 
1.7.8.36.g69ee2

[PATCH 1/3] cmd_fetch_pack(): declare dest to be const

[Nguyễn Thái Ngọc Duy]

From: Michael Haggerty <mhagger@alum.mit.edu>

There is no need for it to be non-const, and this avoids the need
for casting away constness of argv elements.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 10db15b..7e9d62f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -901,7 +901,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, ret, nr_heads;
 	struct ref *ref = NULL;
-	char *dest = NULL, **heads;
+	const char *dest = NULL;
+	char **heads;
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
@@ -971,7 +972,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			}
 			usage(fetch_pack_usage);
 		}
-		dest = (char *)arg;
+		dest = arg;
 		heads = (char **)(argv + i + 1);
 		nr_heads = argc - i - 1;
 		break;
@@ -1018,7 +1019,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		fd[0] = 0;
 		fd[1] = 1;
 	} else {
-		conn = git_connect(fd, (char *)dest, args.uploadpack,
+		conn = git_connect(fd, dest, args.uploadpack,
 				   args.verbose ? CONNECT_VERBOSE : 0);
 	}
 
-- 
1.7.8.36.g69ee2

[PATCH 2/3] fetch-pack: use parse_options()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I _think_ this also fixes a case when --keep is passed twice, then
 lock_pack is set to 1, but pack_lockfile_ptr is not set while it is
 set when --lock-pack is given.

 builtin/fetch-pack.c |  110 ++++++++++++++++++--------------------------------
 fetch-pack.h         |   20 +++++-----
 2 files changed, 49 insertions(+), 81 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 7e9d62f..65ac111 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -10,6 +10,7 @@
 #include "remote.h"
 #include "run-command.h"
 #include "transport.h"
+#include "parse-options.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -22,10 +23,12 @@ static struct fetch_pack_args args = {
 	/* .uploadpack = */ "git-upload-pack",
 };
 
-static const char fetch_pack_usage[] =
-"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] "
-"[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] "
-"[--no-progress] [-v] [<host>:]<directory> [<refs>...]";
+static const char* fetch_pack_usage[] = {
+	"git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] "
+	"[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] "
+	"[--no-progress] [-v] [<host>:]<directory> [<refs>...]",
+	NULL
+};
 
 #define COMPLETE	(1U << 0)
 #define COMMON		(1U << 1)
@@ -906,79 +909,44 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
+	int progress = 1;
 	struct child_process *conn;
+	struct option opts[] = {
+		OPT_STRING(0, "upload-pack", &args.uploadpack, "path",
+			   "path to upload pack on remote end"),
+		OPT_STRING( 0 , "exec", &args.uploadpack, "upload-pack",
+			    "path to upload pack on remote end"),
+		OPT__QUIET(&args.quiet, "do not print results to stdout"),
+		OPT_COUNTUP('k', "keep", &args.keep_pack, "keep downloaded pack"),
+		OPT_BOOL(0, "thin", &args.use_thin_pack, "create thin packs"),
+		OPT_BOOL(0, "include-tag", &args.include_tag,
+			 "include tag objects that refer to objects to be packed"),
+		OPT_BOOL(0, "all", &args.fetch_all, "fetch from all remotes" ),
+		OPT_BOOL(0, "stdin", &args.stdin_refs, "read refs from stdin" ),
+		OPT__VERBOSE(&args.verbose, "be more verbose"),
+		OPT_INTEGER(0, "depth", &args.depth,
+			    "deepen history of shallow clone"),
+		OPT_BOOL(0, "progress", &progress, "show progress"),
+		OPT_BOOL(0, "stateless-rpc", &args.stateless_rpc, "use stateless RPC"),
+		OPT_BOOL(0, "lock-pack", &args.lock_pack, "lock the downloaded pack"),
+		OPT_END()
+	};
 
 	packet_trace_identity("fetch-pack");
 
 	nr_heads = 0;
 	heads = NULL;
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (*arg == '-') {
-			if (!prefixcmp(arg, "--upload-pack=")) {
-				args.uploadpack = arg + 14;
-				continue;
-			}
-			if (!prefixcmp(arg, "--exec=")) {
-				args.uploadpack = arg + 7;
-				continue;
-			}
-			if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
-				args.quiet = 1;
-				continue;
-			}
-			if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
-				args.lock_pack = args.keep_pack;
-				args.keep_pack = 1;
-				continue;
-			}
-			if (!strcmp("--thin", arg)) {
-				args.use_thin_pack = 1;
-				continue;
-			}
-			if (!strcmp("--include-tag", arg)) {
-				args.include_tag = 1;
-				continue;
-			}
-			if (!strcmp("--all", arg)) {
-				args.fetch_all = 1;
-				continue;
-			}
-			if (!strcmp("--stdin", arg)) {
-				args.stdin_refs = 1;
-				continue;
-			}
-			if (!strcmp("-v", arg)) {
-				args.verbose = 1;
-				continue;
-			}
-			if (!prefixcmp(arg, "--depth=")) {
-				args.depth = strtol(arg + 8, NULL, 0);
-				continue;
-			}
-			if (!strcmp("--no-progress", arg)) {
-				args.no_progress = 1;
-				continue;
-			}
-			if (!strcmp("--stateless-rpc", arg)) {
-				args.stateless_rpc = 1;
-				continue;
-			}
-			if (!strcmp("--lock-pack", arg)) {
-				args.lock_pack = 1;
-				pack_lockfile_ptr = &pack_lockfile;
-				continue;
-			}
-			usage(fetch_pack_usage);
-		}
-		dest = arg;
-		heads = (char **)(argv + i + 1);
-		nr_heads = argc - i - 1;
-		break;
-	}
-	if (!dest)
-		usage(fetch_pack_usage);
+	argc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0);
+	args.no_progress = !progress;
+	if (args.keep_pack > 1)
+		args.lock_pack = 1;
+	if (args.lock_pack)
+		pack_lockfile_ptr = &pack_lockfile;
+	dest = argv[0];
+	if (!argc || !dest)
+		usage_with_options(fetch_pack_usage, opts);
+	heads = (char **)(argv + 1);
+	nr_heads = argc - 1;
 
 	if (args.stdin_refs) {
 		/*
diff --git a/fetch-pack.h b/fetch-pack.h
index 7c2069c..d440162 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -5,16 +5,16 @@ struct fetch_pack_args {
 	const char *uploadpack;
 	int unpacklimit;
 	int depth;
-	unsigned quiet:1,
-		keep_pack:1,
-		lock_pack:1,
-		use_thin_pack:1,
-		fetch_all:1,
-		stdin_refs:1,
-		verbose:1,
-		no_progress:1,
-		include_tag:1,
-		stateless_rpc:1;
+	int quiet;
+	int keep_pack;
+	int lock_pack;
+	int use_thin_pack;
+	int fetch_all;
+	int stdin_refs;
+	int verbose;
+	int no_progress;
+	int include_tag;
+	int stateless_rpc;
 };
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
-- 
1.7.8.36.g69ee2

[PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak

[Nguyễn Thái Ngọc Duy]

From: Michael Haggerty <mhagger@alum.mit.edu>

The old code cast away the constness of the strings passed to the
function in argument argv[], which could result in their being
modified by filter_refs().  Moreover, if refs were passed via stdin,
then the memory allocated for them was never freed (though, of course,
this function is only called once so it is not a real problem).

Fix both errors by copying *all* reference names into our own array
and always freeing the array at the end of the function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch-pack.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 65ac111..beabdc2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -902,10 +902,11 @@ static void fetch_pack_setup(void)
 
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
-	int i, ret, nr_heads;
+	int i, ret;
 	struct ref *ref = NULL;
 	const char *dest = NULL;
-	char **heads;
+	int alloc_heads = 0, nr_heads = 0;
+	char **heads = NULL;
 	int fd[2];
 	char *pack_lockfile = NULL;
 	char **pack_lockfile_ptr = NULL;
@@ -934,8 +935,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("fetch-pack");
 
-	nr_heads = 0;
-	heads = NULL;
 	argc = parse_options(argc, argv, prefix, opts, fetch_pack_usage, 0);
 	args.no_progress = !progress;
 	if (args.keep_pack > 1)
@@ -945,19 +944,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	dest = argv[0];
 	if (!argc || !dest)
 		usage_with_options(fetch_pack_usage, opts);
-	heads = (char **)(argv + 1);
-	nr_heads = argc - 1;
+
+	/*
+	 * Copy refs from cmdline to new growable list, then append
+	 * any refs from the standard input.
+	 */
+	ALLOC_GROW(heads, argc - 1, alloc_heads);
+	for (i = 1; i < argc; i++)
+		heads[nr_heads++] = xstrdup(argv[i]);
 
 	if (args.stdin_refs) {
-		/*
-		 * Copy refs from cmdline to new growable list, then
-		 * append the refs from the standard input.
-		 */
-		int alloc_heads = nr_heads;
-		int size = nr_heads * sizeof(*heads);
-		heads = memcpy(xmalloc(size), heads, size);
 		if (args.stateless_rpc) {
-			/* in stateless RPC mode we use pkt-line to read
+			/*
+			 * in stateless RPC mode we use pkt-line to read
 			 * from stdin, until we get a flush packet
 			 */
 			static char line[1000];
@@ -1023,6 +1022,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		ref = ref->next;
 	}
 
+	for (i = 0; i < nr_heads; ++i)
+		free(heads[i]);
+	free(heads);
+
 	return ret;
 }
 
-- 
1.7.8.36.g69ee2

Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Ugh, perhaps you can convert the above to parse_options() too while
> you're making changes in this part?

Please don't.  A parse-opt'ification is a very low priority change for a
command nobody runs from the command line, and is not worth the hassle of
having to deal with unnecessary merge conflicts.

Do so when the codebase around the area is otherwise quiet.

Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> I understand that it is not crucial to free memory allocated in a
> cmd_*() function but it is unclear to me whether it is *preferred* to
> let the process clean up take care of things.

Traditionally, the cmd_foo() functions roughly correspond to main() in
other programs, so from that point of view, "it is not crucial to free" is
an understatement. It is not even worth wasting people's time to (1)
decide which way is *preferred* and to (2) churn the code only to match
whichever way.

These cmd_foo() functions, being roughly equivalent to main(), have logic
to interpret the end-user's intentions (i.e. parse_options()), and carry
out that intention.  In the long run, some _other_ parts of the system may
want to do "foo" (whatever that "foo" may be) from inside the process
without forking, and the first step to do so may be to split cmd_foo()
into two, one that does "parse options", and the other that does "foo".
At that point, it starts to matter that we make the part that does "foo"
leak free, especially if such a caller (or callers) can ask to do "foo"
number of times.

If you have a plan to split cmd_fetch_pack() and make other parts of the
system call it, probably restructuring the current separation between
"figure out what refs will be fetched" done in cmd_fetch_pack() and "fetch
these refs in heads[] array" in fetch_pack() into something else (like
"the new caller also wants to read the list of refs from the standard
input, instead of having parse them into head[] array itself"), then
freeing the memory would matter a lot more, and the approach this patch
takes is a first step in the right direction, I would think.

It also seems that some cruft has snuck into this patch, e.g. like this
part,

> -	int i, ret, nr_heads;
> +	int i, ret;

that do not have anything to do with "fix constness" nor "memory leak".

Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak

[Michael Haggerty]

On 05/21/2012 03:47 AM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
>
>> I understand that it is not crucial to free memory allocated in a
>> cmd_*() function but it is unclear to me whether it is *preferred* to
>> let the process clean up take care of things.
>
> Traditionally, the cmd_foo() functions roughly correspond to main() in
> other programs, so from that point of view, "it is not crucial to free" is
> an understatement. It is not even worth wasting people's time to (1)
> decide which way is *preferred* and to (2) churn the code only to match
> whichever way.

OK, thanks for the info.  I will remove the "freeing-memory" part of the 
patch.

> If you have a plan to split cmd_fetch_pack() and make other parts of the
> system call it, [...]

No, I have no plans for cmd_fetch_pack() besides cleaning up the 
constness errors that I found when randomly reading the code.

> It also seems that some cruft has snuck into this patch, e.g. like this
> part,
>
>> -	int i, ret, nr_heads;
>> +	int i, ret;
>
> that do not have anything to do with "fix constness" nor "memory leak".

This particular hunk is part of moving alloc_heads, nr_heads, and heads 
together to make it more obvious that they are part of an ALLOC_GROW 
triplet.  Previously alloc_heads was a block-local variable used only in 
the case of the --stdin option.

But I admit that the patch is harder than necessary to read because of 
the indentation changes etc, so I will break it up into more digestible 
quanta.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/2] Fix some constness errors in fetch-pack

[Michael Haggerty]

On 05/02/2012 12:40 PM, mhagger@alum.mit.edu wrote:
> From: Michael Haggerty<mhagger@alum.mit.edu>
>
> Fix some constness errors that I noticed while reading the code in
> builtin/fetch-pack.c.
>
> Michael Haggerty (2):
>    cmd_fetch_pack(): declare dest to be const
>    cmd_fetch_pack(): fix constness problem and memory leak
>
>   builtin/fetch-pack.c |  152 +++++++++++++++++++++++++-------------------------
>   1 file changed, 77 insertions(+), 75 deletions(-)

I think that these patches got dropped because of an inconclusive 
discussion [1]: Duy suggested converting the code for fetch-pack to use 
parse_options(), which idea Junio didn't like.  But I think that any 
objections were raised against these two patches, which are just simple 
cleanups.

Michael

[1] http://comments.gmane.org/gmane.comp.version-control.git/196797

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/