Re: [PATCH 04/18] revert: Don't check lone argument in get_encoding

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> The get_encoding function has only one callsite, and its caller makes
> sure that a NULL argument isn't passed.  Remove a string marked for
> translation that will never be shown by not unnecessarily
> double-checking the argument.

The above just doesn't parse for me.  Isn't the truth something like:

"The only place get_encoding uses the 'commit' global is when writing
 an error message explaining that its text was NULL.  Since the only
 caller to get_encoding already ensures that the text is not NULL, we
 can remove this check, with two beneficial consequences:

 1) the function doesn't use the "commit" global any more, bringing
    us closer to removing that global altogether;
 2) translators no longer need to localize this error message that
    would never be shown.

 In a sense, after this patch there is an implied assert() replacing
 the "if ... die()" from before, since the function dereferences the
 pointer to commit object text and would segfault immediately if some
 future caller starts passing in NULL."

Please forgive the unclear phrasing; it is getting close to the time
I should sleep.  Hopefully the intent is clear (i.e., please, imagine
that the reader does not already know what is going on and help him).

Sorry to nitpick so.  The patch proper is obviously good.