Re: [PATCH] checkout-index.c: Unconditionally free memory

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 1, 2015, 23:41 UTC
Message-ID: <CAPig+cRCKDD--Px-FvrV1h8Os7WjYKQXN+5kh1WzVDoud9NJmg@mail.gmail.com>
In-Reply-To: <1430519737-6224-1-git-send-email-sbeller@google.com>

On Fri, May 1, 2015 at 6:35 PM, Stefan Beller <sbeller@google.com> wrote:

Show 9 quoted lines

> 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?

Show 42 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 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