Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

["Martin Ågren" <martin.agren@gmail.com>]

On 4 June 2018 at 23:55, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> I think this makes more sense instead of this fix:
[...]
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int fetch)
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch)
>  {
>         memset(item, 0, sizeof(*item));
> +       int ret = parse_refspec(item, refspec, fetch);
> +       return ret;
> +}

Nit: Declaration after statement. Intriguingly, you do use a `ret`
variable, so I suspect you sort of thought about it but left the final
cleaning up for later. ;-)

> -void refspec_item_init(struct refspec_item *item, const char *refspec, int fetch);
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch);
> +void refspec_item_init_or_die(struct refspec_item *item, const char *refspec, int fetch);
>  void refspec_item_clear(struct refspec_item *item);
>  void refspec_init(struct refspec *rs, int fetch);
>  void refspec_append(struct refspec *rs, const char *refspec);
>
> I.e. let's fix the bug, but with this admittedly more verbose fix we're
> left with exactly two memset() in refspec.c, one for each type of struct
> that's initialized by the API.
>
> The reason this is difficult now is because the current API conflates
> the init function with an init_or_die, which is what most callers want,
> so let's just split those concerns up. Then we're left with one init
> function that does the memset.

I didn't test this, but it looks sane in general IMHO, and should fix
the issue I saw. Should we be worried that someone might already depend
on `refspec_item_init()` to die, and we'll silently break that
assumption?

Martin