On Thu, Feb 20, 2020 at 2:37 PM Emily Shaffer <emilyshaffer@xxxxxxxxxx> wrote:
>
> On Wed, Feb 19, 2020 at 08:34:00PM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@xxxxxxxxx>
> >
> > +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
