On Fri, May 1, 2015 at 6:35 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> It's safe to free the char pointer `p` unconditionally.
>
> The pointer is assigned just 2 lines earlier as a return from
> prefix_path, which allocates new memory for its return value.
>
> Then it is used in checkout_file, which passes the pointer on to
> cache_name_pos and write_tempfile_record, both of which do not store
> the pointer in any permanent record.
> So the condition on when to free the pointer is just "always".
In addition to Peff's comment about the Subject: being incorrect for
this updated patch, the commit message itself is specific to
checkout-index.c by mentioning only cache_name_pos() and
write_tempfile_record(), neither of which are applicable to
update-index.c.
In fact, the commit message, by talking about 'p' and this or that
called function, is probably too detailed. It should be more than
sufficient merely to observe that the string returned by prefix_path()
is always newly allocated (and, parenthetically, that that result is
never stored away for later use), therefore freeing it unconditionally
is the correct thing to do.
> Looking at the history this behavior must be fixed since at least
> (f5114a40c0d0, 2011-08-04, git-check-attr: Normalize paths), where
> the result of prefix_path is freed unconditionally.
This is a strange justification. How does the reader know that the
author of that change was disposing of the result of prefix_path()
properly? Rather than increasing the reader's confidence that your
change is correct, this sort of hand-wavy argument decreases
confidence.
The change which made prefix_path() always return a newly allocated
string was d089eba (setup: sanitize absolute and funny paths in
get_pathspec(), 2008-01-28), so why not cite that instead?
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..5325f92 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -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((char *)p);
> }
>
> if (read_from_stdin) {
> @@ -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((char *)p);
> }
> strbuf_release(&nbuf);
> strbuf_release(&buf);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 6271b54..584efa5 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -534,8 +534,7 @@ static int do_unresolve(int ac, const char **av,
> const char *arg = av[i];
> const char *p = prefix_path(prefix, prefix_length, arg);
> err |= unresolve_one(p);
> - if (p < arg || p > arg + strlen(arg))
> - free((char *)p);
> + free((char *)p);
> }
> return err;
> }
> --
> 2.4.0.rc3.16.g0ab00b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html