Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Jun 5, 2018, 05:10 UTC
- Message-ID
- <CAN0heSprfMRf97J0zeJ4+e7vdM-hk5Z3M1mX2hniBVrRByM4KQ@mail.gmail.com>
- In-Reply-To
- <87tvqiz06t.fsf@evledraar.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:
[...]
Show 7 quoted lines
> -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. ;-)
Show 15 quoted lines
> -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