Re: [PATCH v2 05/36] refspec: convert valid_fetch_refspec to use parse_refspec

From: Martin Ågren <martin.agren@gmail.com>
Date: Jun 3, 2018, 17:13 UTC
Message-ID: <CAN0heSr6dPa+--Mut0+3Zgy-mWF0d2762vKU=d08CKhR8BnKmA@mail.gmail.com>
In-Reply-To: <20180516225823.235426-6-bmwill@google.com>

Hi Brandon,

On 17 May 2018 at 00:57, Brandon Williams <bmwill@google.com> wrote:

Show 41 quoted lines

> Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
> function to only parse a single refspec and eliminate an allocation.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  refspec.c | 17 ++++++++---------
>  refspec.h |  3 ++-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index af9d0d4b3..ab37b5ba1 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -146,15 +146,6 @@ static struct refspec_item *parse_refspec_internal(int nr_refspec, const char **
>         die("Invalid refspec '%s'", refspec[i]);
>  }
>
> -int valid_fetch_refspec(const char *fetch_refspec_str)
> -{
> -       struct refspec_item *refspec;
> -
> -       refspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);
> -       free_refspec(1, refspec);
> -       return !!refspec;
> -}
> -
>  struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec)
>  {
>         return parse_refspec_internal(nr_refspec, refspec, 1, 0);
> @@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
>
>         rs->fetch = 0;
>  }
> +
> +int valid_fetch_refspec(const char *fetch_refspec_str)
> +{
> +       struct refspec_item refspec;
> +       int ret = parse_refspec(&refspec, fetch_refspec_str, REFSPEC_FETCH);
> +       refspec_item_clear(&refspec);
> +       return ret;
> +}

My compiler warned about this function. The `dst` and `src` pointers will equal some random data on the stack, then they may or may not be assigned to, then we will call `free()` on them.

At least I *think* that we "may or may not" assign to them. I don't have much or any time to really dig into this right now unfortunately.

I suppose this could use a REFSPEC_ITEM_INIT, or a memset inside `parse_refspec()`, but I am very unfamiliar with this code.

Martin