Re: [RFC PATCH] revert: Use assert to catch inherent program bugs

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> This situation is hence treated as an inherent bug in
> the program, rather than a runtime error.

Well, programming errors can be run-time errors, too. :)  But anyway,
it is not obvious at a glance to me that this assertion would always
hold, which makes it a bad candidate for an assert().

"Wait a second," you might say.  "You said in a previous message that
the check in get_encoding() could be an assert() or die("BUG: ...") to
avoid having to remember the commit id for use in the error message."

And that's still true:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -123,8 +123,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	int abbrev_len, subject_len;
>  	char *q;
>  
> -	if (!raw_message)
> -		return -1;
> +	assert(raw_message);
>  	encoding = get_encoding(raw_message);

get_encoding() only gets called after the check here, so in the
existing code, the raw_message parameter to get_encoding() can never
be NULL.

By contrast, get_message() gets called by do_pick_commit() and passed
a buffer that is populated long before then.  Can it ever be NULL?  I
don't know, so the check is certainly doing something useful for my
peace of mind.

> @@ -444,9 +441,7 @@ static int do_pick_commit(void)
>  		die(_("%s: cannot parse parent commit %s"),
>  		    me, sha1_to_hex(parent->object.sha1));
>  
> -	if (get_message(commit->buffer, &msg) != 0)
> -		die(_("Cannot get commit message for %s"),
> -				sha1_to_hex(commit->object.sha1));
> +	get_message(commit->buffer, &msg);

Forgetting what's mentioned above, this change would be a regression.
What if get_message() learns to return error() for some other reason?
It is only safe to call a function like this if deliberately ignoring
errors (in which case a comment might be helpful for explaining why to
ignore the errors) or if the function returns "void".