On 4 June 2018 at 23:55, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
