* [PATCH 0/2] Fix some constness errors in fetch-pack
@ 2012-05-02 10:40 mhagger
2012-05-02 10:40 ` [PATCH 1/2] cmd_fetch_pack(): declare dest to be const mhagger
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: mhagger @ 2012-05-02 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] cmd_fetch_pack(): declare dest to be const
2012-05-02 10:40 [PATCH 0/2] Fix some constness errors in fetch-pack mhagger
@ 2012-05-02 10:40 ` mhagger
2012-05-02 10:40 ` [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak mhagger
2012-05-19 14:05 ` [PATCH 0/2] Fix some constness errors in fetch-pack Michael Haggerty
2 siblings, 0 replies; 13+ messages in thread
From: mhagger @ 2012-05-02 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 10:40 [PATCH 0/2] Fix some constness errors in fetch-pack mhagger
2012-05-02 10:40 ` [PATCH 1/2] cmd_fetch_pack(): declare dest to be const mhagger
@ 2012-05-02 10:40 ` mhagger
2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
2012-05-21 1:47 ` Junio C Hamano
2012-05-19 14:05 ` [PATCH 0/2] Fix some constness errors in fetch-pack Michael Haggerty
2 siblings, 2 replies; 13+ messages in thread
From: mhagger @ 2012-05-02 10:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 10:40 ` [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak mhagger
@ 2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
2012-05-02 13:35 ` Michael Haggerty
2012-05-02 17:14 ` [PATCH 2/2] " Junio C Hamano
2012-05-21 1:47 ` Junio C Hamano
1 sibling, 2 replies; 13+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-05-02 11:14 UTC (permalink / raw)
To: mhagger; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
@ 2012-05-02 13:35 ` Michael Haggerty
2012-05-02 14:38 ` [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion Nguyễn Thái Ngọc Duy
2012-05-02 17:14 ` [PATCH 2/2] " Junio C Hamano
1 sibling, 1 reply; 13+ messages in thread
From: Michael Haggerty @ 2012-05-02 13:35 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
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/
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion
2012-05-02 13:35 ` Michael Haggerty
@ 2012-05-02 14:38 ` Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 1/3] cmd_fetch_pack(): declare dest to be const Nguyễn Thái Ngọc Duy
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-05-02 14:38 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty, Junio C Hamano,
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] cmd_fetch_pack(): declare dest to be const
2012-05-02 14:38 ` [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion Nguyễn Thái Ngọc Duy
@ 2012-05-02 14:38 ` Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 2/3] fetch-pack: use parse_options() Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak Nguyễn Thái Ngọc Duy
2 siblings, 0 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-05-02 14:38 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty, Junio C Hamano,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] fetch-pack: use parse_options()
2012-05-02 14:38 ` [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 1/3] cmd_fetch_pack(): declare dest to be const Nguyễn Thái Ngọc Duy
@ 2012-05-02 14:38 ` Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak Nguyễn Thái Ngọc Duy
2 siblings, 0 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-05-02 14:38 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty, Junio C Hamano,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 14:38 ` [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 1/3] cmd_fetch_pack(): declare dest to be const Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 2/3] fetch-pack: use parse_options() Nguyễn Thái Ngọc Duy
@ 2012-05-02 14:38 ` Nguyễn Thái Ngọc Duy
2 siblings, 0 replies; 13+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2012-05-02 14:38 UTC (permalink / raw)
To: git; +Cc: Michael Haggerty, Junio C Hamano,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
2012-05-02 13:35 ` Michael Haggerty
@ 2012-05-02 17:14 ` Junio C Hamano
1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2012-05-02 17:14 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: mhagger, git
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Fix some constness errors in fetch-pack
2012-05-02 10:40 [PATCH 0/2] Fix some constness errors in fetch-pack mhagger
2012-05-02 10:40 ` [PATCH 1/2] cmd_fetch_pack(): declare dest to be const mhagger
2012-05-02 10:40 ` [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak mhagger
@ 2012-05-19 14:05 ` Michael Haggerty
2 siblings, 0 replies; 13+ messages in thread
From: Michael Haggerty @ 2012-05-19 14:05 UTC (permalink / raw)
To: mhagger; +Cc: Junio C Hamano, git
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/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-02 10:40 ` [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak mhagger
2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
@ 2012-05-21 1:47 ` Junio C Hamano
2012-05-21 8:13 ` Michael Haggerty
1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2012-05-21 1:47 UTC (permalink / raw)
To: mhagger; +Cc: git
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".
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak
2012-05-21 1:47 ` Junio C Hamano
@ 2012-05-21 8:13 ` Michael Haggerty
0 siblings, 0 replies; 13+ messages in thread
From: Michael Haggerty @ 2012-05-21 8:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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/
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-05-21 8:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-02 10:40 [PATCH 0/2] Fix some constness errors in fetch-pack mhagger
2012-05-02 10:40 ` [PATCH 1/2] cmd_fetch_pack(): declare dest to be const mhagger
2012-05-02 10:40 ` [PATCH 2/2] cmd_fetch_pack(): fix constness problem and memory leak mhagger
2012-05-02 11:14 ` Nguyen Thai Ngoc Duy
2012-05-02 13:35 ` Michael Haggerty
2012-05-02 14:38 ` [PATCH 0/3] Fix some constness errors in fetch-pack and parseopt conversion Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 1/3] cmd_fetch_pack(): declare dest to be const Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 2/3] fetch-pack: use parse_options() Nguyễn Thái Ngọc Duy
2012-05-02 14:38 ` [PATCH 3/3] cmd_fetch_pack(): fix constness problem and memory leak Nguyễn Thái Ngọc Duy
2012-05-02 17:14 ` [PATCH 2/2] " Junio C Hamano
2012-05-21 1:47 ` Junio C Hamano
2012-05-21 8:13 ` Michael Haggerty
2012-05-19 14:05 ` [PATCH 0/2] Fix some constness errors in fetch-pack Michael Haggerty
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).