* [PATCH] prefix_path(): Unconditionally free result of prefix_path
@ 2015-05-04 19:11 Stefan Beller
2015-05-04 20:19 ` Eric Sunshine
2015-05-05 3:21 ` Jeff King
0 siblings, 2 replies; 8+ messages in thread
From: Stefan Beller @ 2015-05-04 19:11 UTC (permalink / raw)
To: peff, git, gitster, sunshine; +Cc: Stefan Beller
prefix_path() always returns a newly allocated string since
d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
2008-01-28)
Additionally the const is dropped from the pointers, so the call to
free doesn't need a cast.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
Thanks for all the suggestions!
They are incorporated into this version of the patch.
builtin/checkout-index.c | 10 ++++------
builtin/update-index.c | 5 ++---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..8028c37 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
/* Check out named files first */
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
- const char *p;
+ char *p;
if (all)
die("git checkout-index: don't mix '--all' and explicit filenames");
@@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
die("git checkout-index: don't mix '--stdin' and explicit filenames");
p = prefix_path(prefix, prefix_length, arg);
checkout_file(p, prefix);
- if (p < arg || p > arg + strlen(arg))
- free((char *)p);
+ free(p);
}
if (read_from_stdin) {
@@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
die("git checkout-index: don't mix '--all' and '--stdin'");
while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
- const char *p;
+ char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset(&nbuf);
if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
}
p = prefix_path(prefix, prefix_length, buf.buf);
checkout_file(p, prefix);
- if (p < buf.buf || p > buf.buf + buf.len)
- free((char *)p);
+ free(p);
}
strbuf_release(&nbuf);
strbuf_release(&buf);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..a92eed2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
for (i = 1; i < ac; i++) {
const char *arg = av[i];
- const char *p = prefix_path(prefix, prefix_length, arg);
+ char *p = prefix_path(prefix, prefix_length, arg);
err |= unresolve_one(p);
- if (p < arg || p > arg + strlen(arg))
- free((char *)p);
+ free(p);
}
return err;
}
--
2.4.0.rc3.16.g0ab00b9
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-04 19:11 [PATCH] prefix_path(): Unconditionally free result of prefix_path Stefan Beller
@ 2015-05-04 20:19 ` Eric Sunshine
2015-05-04 22:29 ` Junio C Hamano
2015-05-05 3:21 ` Jeff King
1 sibling, 1 reply; 8+ messages in thread
From: Eric Sunshine @ 2015-05-04 20:19 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jeff King, Git List, Junio C Hamano
On Mon, May 4, 2015 at 3:11 PM, Stefan Beller <sbeller@google.com> wrote:
> prefix_path(): Unconditionally free result of prefix_path
Slightly redundant mention of "prefix_path". Also, prevailing custom
is to drop capitalization.
> prefix_path() always returns a newly allocated string since
> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
> 2008-01-28)
I'd probably turn this sentence fragment into a proper sentence:
As of d089eba (...), prefix_path() always returns a newly
allocated string, so free its result unconditionally.
> Additionally the const is dropped from the pointers, so the call to
> free doesn't need a cast.
Imperative mood:
Additionally, drop the const from variables to which the
prefix_path() result is assigned so they can be free()'d
without having to cast-away constness.
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
> Thanks for all the suggestions!
> They are incorporated into this version of the patch.
Thanks, this version looks much better.
FWIW, with or without addressing the very minor nits above:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..8028c37 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> /* Check out named files first */
> for (i = 0; i < argc; i++) {
> const char *arg = argv[i];
> - const char *p;
> + char *p;
>
> if (all)
> die("git checkout-index: don't mix '--all' and explicit filenames");
> @@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> die("git checkout-index: don't mix '--stdin' and explicit filenames");
> p = prefix_path(prefix, prefix_length, arg);
> checkout_file(p, prefix);
> - if (p < arg || p > arg + strlen(arg))
> - free((char *)p);
> + free(p);
> }
>
> if (read_from_stdin) {
> @@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> die("git checkout-index: don't mix '--all' and '--stdin'");
>
> while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
> - const char *p;
> + char *p;
> if (line_termination && buf.buf[0] == '"') {
> strbuf_reset(&nbuf);
> if (unquote_c_style(&nbuf, buf.buf, NULL))
> @@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
> }
> p = prefix_path(prefix, prefix_length, buf.buf);
> checkout_file(p, prefix);
> - if (p < buf.buf || p > buf.buf + buf.len)
> - free((char *)p);
> + free(p);
> }
> strbuf_release(&nbuf);
> strbuf_release(&buf);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 6271b54..a92eed2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
>
> for (i = 1; i < ac; i++) {
> const char *arg = av[i];
> - const char *p = prefix_path(prefix, prefix_length, arg);
> + char *p = prefix_path(prefix, prefix_length, arg);
> err |= unresolve_one(p);
> - if (p < arg || p > arg + strlen(arg))
> - free((char *)p);
> + free(p);
> }
> return err;
> }
> --
> 2.4.0.rc3.16.g0ab00b9
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-04 20:19 ` Eric Sunshine
@ 2015-05-04 22:29 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-05-04 22:29 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Stefan Beller, Jeff King, Git List
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Mon, May 4, 2015 at 3:11 PM, Stefan Beller <sbeller@google.com> wrote:
>> prefix_path(): Unconditionally free result of prefix_path
>
> Slightly redundant mention of "prefix_path". Also, prevailing custom
> is to drop capitalization.
>
>> prefix_path() always returns a newly allocated string since
>> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
>> 2008-01-28)
>
> I'd probably turn this sentence fragment into a proper sentence:
>
> As of d089eba (...), prefix_path() always returns a newly
> allocated string, so free its result unconditionally.
>
>> Additionally the const is dropped from the pointers, so the call to
>> free doesn't need a cast.
>
> Imperative mood:
>
> Additionally, drop the const from variables to which the
> prefix_path() result is assigned so they can be free()'d
> without having to cast-away constness.
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>> Thanks for all the suggestions!
>> They are incorporated into this version of the patch.
>
> Thanks, this version looks much better.
>
> FWIW, with or without addressing the very minor nits above:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Thanks, both.
To save a round-trip, I'll munge the log message myself stealing
Eric's suggestions.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-04 19:11 [PATCH] prefix_path(): Unconditionally free result of prefix_path Stefan Beller
2015-05-04 20:19 ` Eric Sunshine
@ 2015-05-05 3:21 ` Jeff King
2015-05-05 16:28 ` Stefan Beller
1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2015-05-05 3:21 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, gitster, sunshine
On Mon, May 04, 2015 at 12:11:54PM -0700, Stefan Beller wrote:
> prefix_path() always returns a newly allocated string since
> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
> 2008-01-28)
>
> Additionally the const is dropped from the pointers, so the call to
> free doesn't need a cast.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
> Thanks for all the suggestions!
> They are incorporated into this version of the patch.
>
> builtin/checkout-index.c | 10 ++++------
> builtin/update-index.c | 5 ++---
> 2 files changed, 6 insertions(+), 9 deletions(-)
Should we also squash in these sites? I think they are adequately
covered under the proposed log message.
Found by grepping for prefix_path calls. The only remainders are:
1. in blame, we assign the result to a const char that may also point
straight into to argv, but we never actually free either way
2. test-path-utils does not free at all, but we probably don't care
either way
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a92eed2..0665b31 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -870,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
case PARSE_OPT_DONE:
{
const char *path = ctx.argv[0];
- const char *p;
+ char *p;
setup_work_tree();
p = prefix_path(prefix, prefix_length, path);
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
- free((char *)p);
+ free(p);
ctx.argc--;
ctx.argv++;
break;
@@ -908,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
setup_work_tree();
while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
- const char *p;
+ char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset(&nbuf);
if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -919,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
- free((char *)p);
+ free(p);
}
strbuf_release(&nbuf);
strbuf_release(&buf);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-05 3:21 ` Jeff King
@ 2015-05-05 16:28 ` Stefan Beller
2015-05-05 17:36 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Beller @ 2015-05-05 16:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Eric Sunshine
On Mon, May 4, 2015 at 8:21 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 04, 2015 at 12:11:54PM -0700, Stefan Beller wrote:
>
>> prefix_path() always returns a newly allocated string since
>> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
>> 2008-01-28)
>>
>> Additionally the const is dropped from the pointers, so the call to
>> free doesn't need a cast.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>> Thanks for all the suggestions!
>> They are incorporated into this version of the patch.
>>
>> builtin/checkout-index.c | 10 ++++------
>> builtin/update-index.c | 5 ++---
>> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> Should we also squash in these sites? I think they are adequately
> covered under the proposed log message.
That sounds good to me.
>
> Found by grepping for prefix_path calls. The only remainders are:
>
> 1. in blame, we assign the result to a const char that may also point
> straight into to argv, but we never actually free either way
>
> 2. test-path-utils does not free at all, but we probably don't care
> either way
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a92eed2..0665b31 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -870,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> case PARSE_OPT_DONE:
> {
> const char *path = ctx.argv[0];
> - const char *p;
> + char *p;
>
> setup_work_tree();
> p = prefix_path(prefix, prefix_length, path);
> update_one(p);
> if (set_executable_bit)
> chmod_path(set_executable_bit, p);
> - free((char *)p);
> + free(p);
> ctx.argc--;
> ctx.argv++;
> break;
> @@ -908,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
> setup_work_tree();
> while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
> - const char *p;
> + char *p;
> if (line_termination && buf.buf[0] == '"') {
> strbuf_reset(&nbuf);
> if (unquote_c_style(&nbuf, buf.buf, NULL))
> @@ -919,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> update_one(p);
> if (set_executable_bit)
> chmod_path(set_executable_bit, p);
> - free((char *)p);
> + free(p);
> }
> strbuf_release(&nbuf);
> strbuf_release(&buf);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-05 16:28 ` Stefan Beller
@ 2015-05-05 17:36 ` Junio C Hamano
2015-05-05 17:56 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2015-05-05 17:36 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git, Eric Sunshine
Stefan Beller <sbeller@google.com> writes:
>> Should we also squash in these sites? I think they are adequately
>> covered under the proposed log message.
>
> That sounds good to me.
>>
>> Found by grepping for prefix_path calls. The only remainders are:
>>
>> 1. in blame, we assign the result to a const char that may also point
>> straight into to argv, but we never actually free either way
The return value from add_prefix() that is what prefix_path()
returned eventually becomes scoreboard.path that needs to be kept
during the lifetime of the process, and I think there isn't much
point doing the "free() immediately before exiting".
>> 2. test-path-utils does not free at all, but we probably don't care
>> either way
Anyway, here is what I'd queue for now.
-- >8 --
From: Stefan Beller <sbeller@google.com>
Date: Mon, 4 May 2015 12:11:54 -0700
Subject: [PATCH] prefix_path(): unconditionally free results in the callers
As of d089ebaa (setup: sanitize absolute and funny paths in
get_pathspec(), 2008-01-28), prefix_path() always returns a
newly allocated string, so callers should free its result.
Additionally, drop the const from variables to which the result of
the prefix_path() is assigned, so they can be free()'d without
having to cast-away the constness.
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/checkout-index.c | 10 ++++------
builtin/update-index.c | 13 ++++++-------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..8028c37 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
/* Check out named files first */
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
- const char *p;
+ char *p;
if (all)
die("git checkout-index: don't mix '--all' and explicit filenames");
@@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
die("git checkout-index: don't mix '--stdin' and explicit filenames");
p = prefix_path(prefix, prefix_length, arg);
checkout_file(p, prefix);
- if (p < arg || p > arg + strlen(arg))
- free((char *)p);
+ free(p);
}
if (read_from_stdin) {
@@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
die("git checkout-index: don't mix '--all' and '--stdin'");
while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
- const char *p;
+ char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset(&nbuf);
if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
}
p = prefix_path(prefix, prefix_length, buf.buf);
checkout_file(p, prefix);
- if (p < buf.buf || p > buf.buf + buf.len)
- free((char *)p);
+ free(p);
}
strbuf_release(&nbuf);
strbuf_release(&buf);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..0665b31 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
for (i = 1; i < ac; i++) {
const char *arg = av[i];
- const char *p = prefix_path(prefix, prefix_length, arg);
+ char *p = prefix_path(prefix, prefix_length, arg);
err |= unresolve_one(p);
- if (p < arg || p > arg + strlen(arg))
- free((char *)p);
+ free(p);
}
return err;
}
@@ -871,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
case PARSE_OPT_DONE:
{
const char *path = ctx.argv[0];
- const char *p;
+ char *p;
setup_work_tree();
p = prefix_path(prefix, prefix_length, path);
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
- free((char *)p);
+ free(p);
ctx.argc--;
ctx.argv++;
break;
@@ -909,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
setup_work_tree();
while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
- const char *p;
+ char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset(&nbuf);
if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -920,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
update_one(p);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
- free((char *)p);
+ free(p);
}
strbuf_release(&nbuf);
strbuf_release(&buf);
--
2.4.0-311-gf1d9b8d
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-05 17:36 ` Junio C Hamano
@ 2015-05-05 17:56 ` Jeff King
2015-05-05 18:09 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2015-05-05 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git, Eric Sunshine
On Tue, May 05, 2015 at 10:36:38AM -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> >> Should we also squash in these sites? I think they are adequately
> >> covered under the proposed log message.
> >
> > That sounds good to me.
> >>
> >> Found by grepping for prefix_path calls. The only remainders are:
> >>
> >> 1. in blame, we assign the result to a const char that may also point
> >> straight into to argv, but we never actually free either way
>
> The return value from add_prefix() that is what prefix_path()
> returned eventually becomes scoreboard.path that needs to be kept
> during the lifetime of the process, and I think there isn't much
> point doing the "free() immediately before exiting".
Yeah, sorry, I meant to say that more explicitly, but clearly didn't. I
think it is fine as-is.
> Anyway, here is what I'd queue for now.
Looks good, thanks.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path
2015-05-05 17:56 ` Jeff King
@ 2015-05-05 18:09 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-05-05 18:09 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git, Eric Sunshine
Jeff King <peff@peff.net> writes:
>> >> 1. in blame, we assign the result to a const char that may also point
>> >> straight into to argv, but we never actually free either way
>>
>> The return value from add_prefix() that is what prefix_path()
>> returned eventually becomes scoreboard.path that needs to be kept
>> during the lifetime of the process, and I think there isn't much
>> point doing the "free() immediately before exiting".
>
> Yeah, sorry, I meant to say that more explicitly, but clearly didn't. I
> think it is fine as-is.
Yeah, sorry, I didn't mean "You are wrong and here is why". I was
merely agreeing with you.
Thanks for running grep over the codebase to check possible
remaining problems.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-05 18:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-04 19:11 [PATCH] prefix_path(): Unconditionally free result of prefix_path Stefan Beller
2015-05-04 20:19 ` Eric Sunshine
2015-05-04 22:29 ` Junio C Hamano
2015-05-05 3:21 ` Jeff King
2015-05-05 16:28 ` Stefan Beller
2015-05-05 17:36 ` Junio C Hamano
2015-05-05 17:56 ` Jeff King
2015-05-05 18:09 ` Junio C Hamano
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 NNTP newsgroup(s).