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".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html