Re: [PATCH v3 1/2] advice: revamp advise API

[Heba Waly <heba.waly@gmail.com>]

On Thu, Feb 20, 2020 at 2:37 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Wed, Feb 19, 2020 at 08:34:00PM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > +static int get_config_value(enum advice_type type)
> > +{
> > +     int value = 1;
> > +     char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
>
> Same comment as your other call for xstrfmt. I think you need to manage
> the output.

Got it.

> > +void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > +{
> > +     struct strbuf buf = STRBUF_INIT;
> > +     char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
>
> Hmm, doesn't this leak?
>
> On the surface it looks like xstrfmt can save you a strbuf allocation,
> but if you check in strbuf.c, it actually allocates and detaches a
> strbuf for you anyways. I'd argue that it's easier to tell whether
> you're leaking a strbuf than the result of this call, so you might as
> well do it that way.
>

You're right.

> > +     SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28,
> > +};
>
> Hmm. I wanted to say, "Our codebase uses ALL_CAPS or snake_case in enums
> so you could use lowers if you wanted" - but based on 'git grep -A4
> "^enum"' it's actually pretty unusual to see enums with lower-case
> members. Dang :)
>

Yeah I thought the same at first too but reached the same result.

> > +
> > +             advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), tag, object_ref);
>
> Hm, if it was me, I would have put this bit in its own commit. But I see
> that it's a pretty tiny change...
>

That'd have been better of course, don't know why I didn't think of that :)

> > +     //use any advice type for testing
> I think this might be misleading - your t0018 does quite a few checks
> explicitly for the NESTED_TAG advice. Maybe it's better to say something
> like "Make sure this agrees with t0018"?

I see your point, I'll need to think more about this one.

> Also nit, I've been told off a
> few times for using //c++ style comments.

Oh ok.

Thanks a lot, Emily

Heba