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

[Ramkumar Ramachandra <artagnon@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.

>> 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.

> 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.