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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 20, 2011, 18:35 UTC
Message-ID: <20110520183513.GE17177@elie>
In-Reply-To: <1305880223-7542-2-git-send-email-artagnon@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:

Show 10 quoted lines

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

Show 8 quoted lines

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