Re: [PATCH 1/2] help: add help_unknown_ref

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 1, 2013, 20:32 UTC
Message-ID: <CALkWK0nJSoBoAXR=ViGan6keFzXjmDGkfKqqWkTHqF89mHDDOQ@mail.gmail.com>
In-Reply-To: <51817319.6060201@gmail.com>

Vikrant Varma wrote:

> I agree with Matthieu, the people who don't want to see this advice never
> will, because they won't make that mistake. Maybe advice is the wrong word,
> corrections might be more appropriate.

Makes sense.

Perhaps it would make sense to hook into help.autocorrect. I would definitely like that.

Show 7 quoted lines

>> I see that there are other structs in our codebase suffixing _cb, to
>> indicate "callback data".  I normally reserve _cb for callback
>> functions.
>
>
> I'm following the convention (builtin/merge.c: struct append_ref_cb). If
> there's a better way to name it, I'll use that.

Fine leaving it as it is.

Show 5 quoted lines

> ref_cb.similar_refs has already been defined. The compiler won't let me
> assign to it unless I cast first. However, I think compound literals are a
> C99/gcc feature. Is this better?
>
>         struct similar_ref_cb ref_cb = {ref, STRING_LIST_INIT_NODUP};

As Johannes pointed out, ref is a variable and that is problematic. Leave the cast on: I didn't notice the compiler warning in my head.

> Q_ is a pluralization helper that picks one of the two strings based on
> ref_cb.similar_refs.nr. It's used in help.c:help_unknown_cmd for the same
> reason.

Thanks.

> Again, because help_unknown_cmd exited with 1. I've tried to follow the
> convention as laid down there.

Ah, I didn't notice that.

> What's the significance of the error code for
> die()?

When something _really_ bad happens, exit() with 128, without bothering to go up the callstack. Return error() is a common idiom to print an error message immediately, and propagate the return status -1 up the callstack.

> When is it correct to use die(), and when to use error() followed by
> exit(128)?

exit(128) is a bit rare [grep for it yourself]. It's for when you want to die(), but exit() only after doing an important task in the caller.