runpty.c: Replace fprintf()+exit() pairs with new die().

Detect function-pointer `done' being set twice in a row.

Make `done' a function that uses a file-static function pointer, and add
set_done() to alter it.  That wants either the old or new value to be
exit(3).  If it's not, it still alters the stored pointer, to maintain
the existing behaviour, but emits a warning on stderr.  mhbuild(1)
triggers that warning, making this hunch worthwhile.

man: Replace csh `%' prompt in examples with sh's `$'.

Youngsters these days probably wouldn't recognise csh.  None of the
commands needed changing due to syntax differences.

mh-draft.man: Use ASCII double-quote for itself in example output.

Not the TeX doubling of backquote and single-quote.

man: Add backslash before backquote so it appears as sh backtick.

man: Fix backquote used in error for single quote.

A repeated bit of prose had the typo `foo` instead of `foo'.

mhlsbr.c: Delete file-static function pointer mhl_action.

Never set, so always NULL due to BSS.  It used to be set by c808c5d5.

mhlsbr.c: Delete `jmp_buf mhlenv', never setjmp(3)'d.

Only ever conditionally longjmp(3)'d.  The setjmp() was removed by
c808c5d5.

Strip parenthesis from `return (state = OK);' and similar.

There are sufficient cases without the parenthesis that various
compilers must be happy without.

pop_init(): After proxy's execve(2) failure, exit(1), not exit(10).

The value 10 seems undocumented, and I can't find anything that tests
for that specific value.

sendfiles: Always die with exit status of 1 rather than -1.

The sh function die() took the exit status as an optional parameter,
with a default of -1, but this was never used so remove the option and
always use an exit status of 1.

Remove preprocessor conditionals on `lint'.

Treat all tests for `lint' being defined as false.  Don't know when
anyone had a lint(1) that could be run on the source, but none of the
areas had been touched in git's history, and at least one would have
been a compile error had it been defined.

Replace `_exit(-1)' with `_exit(1)'.

In line with other changes to limit a process's exit value to small
non-negative integers distinguishable with sh's `$?'.

whom.c: Don't call rename(2) with source that's uninitialised.

The intent is that two rename(2)s take place: to move a file to a
backup and then return it.  The first test controlled the generation of
the destination in the char array `backup' and calling the first
rename().  The second test used that same array for the restoring
rename(), but used a different test and so could run when the first
rename hadn't, and with an uninitialised array for the source filename.

The initialisation of `backup' and the first rename needed both
environment variables `mhdist' and `mhaltmsg' to be non-empty, and the
former to have a non-zero atoi(3) value.  The second rename ignored
`mhaltmsg'.  Bug pre-dates git.

whom.c: Re-work fork/exec logic to clarify interaction with distsw.

If fork(2) fails then die;  don't fall through to execve(2).

It's already been agreed that failure to fork() is so rare these days
that multiple attempts aren't required.  Nor should the code persevere
by calling execve() as if the fork had succeeded, else why bother to
fork?  Instead, die.

whom.c: Don't increment atoi(3)'s return value as it's already true.

atoi(3) has returned non-zero and distsw holds that value.  Only the
truthness of distsw matters.  Don't increment distsw, as if to show it's
been set, because that would turn the true -1 to false 0.  Bug pre-dates
git.

whom.c: Don't store atoi(getenv("mhdist")) when value only tested.

It's the non-zero value of atoi(3) that's important in the if-condition;
the value is never used again.

mhlsbr.c: Don't strchr(3) non-string NUL-less buffer.

Up to 8 KiB was read into a buffer and then strchr() used to test for a
linefeed.  There was no guarantee the linefeed would be present, else
why test for it, nor that the buffer would contain a NUL to terminate
the search, either from the read bytes, or the bytes not trampled by
read().  Replace the two similar lumps of code with a new
linefeed_typed().  Bug pre-dates git.

mhlsbr.c: Don't read(2) from fileno(3) of stdout.

Remove the assumption that file descriptor 1 is readable.  It can be if
it, or where it was dup(2)'d from, was opened read/write, e.g. on
/dev/tty.  But it can easily be arranged with shell re-direction that it
isn't, and then the read(2) fails.  Pass 0, standard input's file
descriptor, instead.  Don't use fileno(stdin) as I can't think of a need
to support that being non-zero, but its use makes the reader think it
might.  Bug pre-dates git.

Invert ternary condition to remove one negative.

Replaces `i != NOTOK ? OK : NOTOK' with `i == NOTOK ? NOTOK : OK'.

mime_type.c: Fix regexp in comment that describes following code.

Fixes e35fb433.

addrsbr.c: Alter ismymbox() to return bool, not int.

It was returning 0 or 1.

smtp.c: Have function returning char pointer return NULL, not 0.

ext_hook(): Flip if-statement condition so then-block simply returns.

No need for rest of function to be in an else-block.

new.c: Order two return statements to match comment.

No change in behaviour.

Edit out a few more tautological `the foo(5) man page'.

nmh_completion(): Simplify, removing local variable.

Remove casts of NULL to a data pointer.

With a function prototype in place stating a parameter is a foo pointer,
where foo is data, not a function, then `NULL' suffices so the cast to
foo pointer is redundant.  If the function parameter is retrieved with
va_arg(3) then the pointer passed must be the retrieved type, e.g.
concat()'s arguments are fetched as char pointer and so it should be
called as `concat("foo", (char *)0)'.  Using NULL is incorrect, though
NULL could be used instead of 0 but still needs casting.  However, the
source doesn't bother getting this right and just passes NULL in most
cases so make the few match.  Most of those changed were passing NULL
cast to a void pointer.

Remove more outer wrapping parenthesis from return statements.

For example, `return(foo(a, b));' becomes `return foo(a, b);'.

dmktime(): Break assignments so `*=' and `+=' can be used.

Add comments tracking the units of `result' from days to seconds.

Replace `return (0)' and similar with `return 0'.

Just outer parenthesis removed.

Hoick FENDNULL(key) out of the search loop.

`key' is a loop-invariant so can be tested for NULL once before the
loop.  In one case, this also avoid passing NULL to printf(3) for "%s",
and means getcpy()s can be replaced with mh_xstrdup()s.

mh_oauth_cred_fn()'s return value no longer needs duplicating.

It now returns a fresh allocation so that can be used directly rather
than obtaining another copy.

mh_oauth_cred_fn(): Always freshly allocate result.

If a `credential-file' context was found, and started with a `/', then
the m_defs's n_field was returned, otherwise a newly allocated string.
The caller couldn't tell whether to free the result or not so memory was
leaked.  Alter it to always freshly allocate the result.  Fixes
803f25412.

exmaildir(): Don't leak memory allocated by path().

Bug pre-dates git.

mime_type(): Don't leak memory allocated by get_file_info().

Fixes b4f2851d4.

lock_file.c: close(2) file descriptor on failure, avoiding leak.

Fixes d6e398f9c.

mhfixmsg.c: Fix memory leak of temporary filename if write fails.

mhfixmsg.c: Fix memory leaks of boundary string on early returns.

Delete mh_xfree(), changing callers to call free(3) instead.

A free(3) that didn't handle NULL is now long past.

rmf.c: free(3) return value from m_mailpath() calls.

And add comment to m_mailpath() that its return value needs freeing.

path.c: Add comments declaring return values must be free(3)'d.

Saves working it out afresh each visit.

path.c: Remove path()'s test for expath() returning NULL.

All paths out of expath() have it calling mh_xstrdup().

path.c: Re-format path() to make it more clear what's happening.

slocal.c: Alter trim() to return static array, not malloc(3).

The callers are immediately passing the return value to printf(3) for
"%s".  There's only one call per printf().  None of the callers are
bothering to free(3) the existing return value.  Return the address of
trim()'s char array, now static, instead.  Leave trim() returning NULL
when passed NULL, even though that gives NULL to print(3) for "%s".  It,
and slocal's other bugs, remain.

Remove test for `\0' in `*s && isfoo(*s)' for ctype.h calls.

find_cache(): Remove test that's always false.

The if-statement's condition is a conjunctive that tests `!writing' and
then, only if that's true, calls find_cache_aux() and tests `writing' in
determining the first argument;  that must be false.  Bug pre-dates git
history.

readline(3) wrapper: Move free(3) of line to where it's reachable.

Every line returned by readline() has to be freed.  The call to free()
was unreachable because all paths heading towards it veered off,
resulting in a memory leak.  Move the free() to just after where the
line has been used for the last time.  Fixes 3a85e0bc9.

fmt_compile.c: Alter FINDCOMP(): the caller must supply the semicolon.

The macro expands to a for-loop, including the semicolon body.  The
caller typically supplies their own semicolon.  This results in two on
one line that looks odd.  Alter the macro to have the for-loop inside a
do-while loop that doesn't have a semicolon, ensuring the caller must
supply it.

argsplit(): Incorrect type used in sizeof calculation for array.

Switched to referring to the variable being assigned the resulting
memory rather than duplicating its type information.  Fixes 78211e93.

get_file_info(): Flip logic throughout, reducing nesting.

Now a series of small paragraphs, each of which might return on error.
Also, the Cygwin oddity described in the comment needs no special
handling given the recent bug fixes so it can be deleted.

get_file_info(): Don't return filename from quote onwards on error.

If `file_name' contained a quote then `cp' points to it, and remains
pointing to it if any of concat(), popen(3), or fgets(3) fail to give
the expected result.  On returning, cp is not NULL and so strdup(cp) is
returned.  Also, if fgets() fails on Cygwin in the way the comment
describes then cp is wrongly free(3)'d.  (One of the problems with
generic variable names is their overloaded semantics cause faulty
re-use.)  Fixes b4f2851d and 0c50c669.

get_file_info(): Don't dereference out of scope char array.

A pointer to `char buf[...]' is taken, the array being declared in a
block scope inside the function.  The pointer is passed to strdup(3) as
the function returned, but the array is by then out of scope.  Move the
array to the function's scope.  Fixes 0c50c669.

Replace parallel MIME-preference arrays with struct array.

New array of struct has `type' and `subtype' members, replacing the old
preferred_types[] and preferred_subtypes[] arrays.

pidstatus(): Rewrite to print stopped and continued statuses too.

Delete own versions of WTERMSIG and WCOREDUMP macros now we're assuming
POSIX.

pidstatus(): Delete commented-out code testing for exit(255).

Pre-dating git, a test of wait(2)'s status having its top eight bits all
set was commented out an "I've no idea what this does" explanation.  The
top eight bits hold either exit(3)'s parameter or the signal that caused
the child to stop.  Either way, there are macros for pulling apart that
value and they're being used.  Perhaps they were broken on a much older
system.  Delete the comment, including its code.

Replace printf("%s\n", foo) and similar with puts(foo).

datetime.c: Flip if-conditions so returns move from else block.

Then there's no need for else-blocks after return.

icalendar.l: Refer to base64.h relative to root of source.

Ken reported a missing-include-file problem when building so the product
ends up outside the source tree, e.g. running configure from elsewhere.
It must also depend on his environment, e.g. OS or compiler, because the
problem isn't reproducible with Arch Linux and gcc, and be specific to C
source built from other source files, e.g. lex(1).  However, given that
the existing sibling icalparse.h is referred to as sbr/..., duplicate
that so it builds for Ken too.

test/getcanon.c: exit(3) with 0 or 1, not -1 or symbolic error code.

gethostname(3) returns -1 on failure and that was being returned from
main;  use 1 instead.  getaddrinfo(3) returns EAI_ADDRFAMILY et al on
failure, but here they're defined as small negative integers, unsuitable
for main()'s return value;  again, use 1 instead.  Along the way, flip
the logic so the end is nigh when possible, avoiding the cognitive
overhead of tracking state when continuing to read through the source.

test/getfullname.c: Move return case first in if-else chain.

Refer to #include files from the root of nmh's source.

One of the -I options given to the C compiler is the root of the nmh
source.  This means uip/foo.c's #include of the relative ../sbr/bar.h
can also be written more tidily as sbr/bar.h.

icalparse.y: Remove else-block that returns by merging if-conditions.

The code tested two values to see if they were present, and if they
were, and were equal, then it returned this node.  If either were
missing then this node was still returned because it matched the more
limited criteria that had already been tested.  So the test for
returning this node can more simply be if either value to compare is
missing, or they're (both present and) equal.

seq_setprev.c: Flip if-condition so return moves from else block.

Then no need for else block after return.

folder_pack.c: Flip if-condition so return moves from else block.

Then no need for else block after return.

seq_setunseen.c: Flip if-condition so return moves from else block.

Then no need for else block after return.

mhstoresbr.c: Flip if-condition so return moves from else block.

Then no need for else block after return.

read_switch_multiword.c: Flip if-condition so return moves from else block.

Then no need for else block after return.

datetime.c: Don't atoi("0") if zone is NULL.

datetime.c: Fix parse-failure's error-message's daylight indication.

`in_standard' was being used to decide between `standard' and `daylight'
in the error message, but it would always be false by that point.
`in_daylight' is what's passed to the parsing function so use that to
determine the string instead.  Bug present since the initial
implementation.

datetime.c: Replace some int with bool.

Only call fork(2) once, not up to five times on failure.

The existing loop that slept five seconds between each attempt was
copy-pasted widely.  Those days are gone.  Just call it once.

content_error(): Add missing va_end(3).

A va_start(3) without a matching va_end() is undefined behaviour.

Send a QUIT instead of RSET at session end when doing 'whom'.

When running 'whom -check' (which really invokes post(8)), at the
end of the SMTP session we would send a RSET instead of a QUIT.  This was
technically a RFC violation (RFC 5321 says a QUIT has to be the last
thing you send), and this would cause some SMTP servers to complain.
So make sure if we're being invoked by whom to send a QUIT at the end
of the session.  Reported by Ralph Corderoy.

Add a -credentials argument when we call post.

If we are using -check, post(8) will need to talk to a remote SMTP server,
and it might need to perform authentication when doing so.  So include a
-credentials option if there is the appropriate line in the user's profile.

popsbr.c: Remove impossible case from switch statement.

Actually, just convert the overkill switch statement into a couple of
if-statements as it's a lot less lines.

mhbuildsbr.c: Attempt fork(2) multiple times, as intended.

The loop attempting fork() intends to try five times but the comparison
operator was wrong so would only try once.  Bug present since pre-git.

base64.c: Don't out-of-bounds printf("%s", &encoded[-1]).

`cp' is walking through encoded[] when an error occurs and is stepped
back up to 20 elements to provide some lead-in context for the error
message.  If might be stepped back to encoded-1, but it attempts to cope
with that by `cp ? cp : encoded'.  cp is always non-NULL so true and cp
is printed.  Presumably, `cp > encoded' was meant.  But it's all a bit
of a rigmarole so just use min() instead to ensure cp stays within
encoded and print cp.  Fixes bfc6b93af.

content_error(): Don't strlen(invo_name) that might be NULL.

Earlier in content_error(), it checks invo_name isn't NULL before using
it.  Later, it passes it to strlen(3) without the check.  Either the
former is redundant or the latter wrong.  Rather than work out which,
delete the strlen(invo_name) because it was only used for a
variable-width indent on a second diagnostic line.  Instead, indent by a
fixed four spaces, which will look better when invo_name is stupidly
long anyway.  Bug present since pre-git.

replsbr.c: In parent, free child's argv[] built before fork().

Fixes ccafa1944.

getpass.c: Don't fileno(NULL) when fopen("/dev/tty") fails.

If stdin is a TTY, but opening it for writing fails, then stdin and
stderr are used as defaults and the TTY doesn't have echo disabled.  The
later test to restore echo just checks if stdin is a TTY and not that it
was opened successfully causing a NULL FILE pointer to be fileno(3)'d.
Fixes da6af9633.  This function's logic remains a bit contorted;  I went
for the minimal fix.

mhbuildsbr.c: Don't free(3) string then print it; do reverse.

Fixes c87ca5cca.

smtp.c: Use read-end of pipe, not random integer.

Instead of `pdi[0]' the array of two elements was being indexed by `i'.
That is typically zero so everything works, but can be up to five
depending how many times fork(2) failed before succeeding.  Fixes
e65127948.

Revised expected output with -prefer in test/mhlist/test-mhlist.

Update to commit 5dce1b38ace6cf61398df3ee4ca5aee0e1dedb5d.

man: Fix some of the font-changing macros' parameters.

Various recurring problems on two themes.  `.BR foo,' lacks the space in
`.BR foo ,'.  `.IR foo' doesn't need to alternate and can be just `.I
foo'.  Other similar changes made, e.g. `.B foo' becoming `foo'.

Add -noprefer switch to mhshow, mhstore, and mhlist

Allow profile-specified -prefer switches to be cancelled from the
command line.

Change the priority ordering for multiple -prefer switches

Interpreting multiple -prefer switches in ascending priority
order allows the command line to override the user's profile.

man: Use `.IR' for `foo(1)', not the few `.BR'.

man: Change the tautological `the foo(1) man page' to `foo(1)'.

oauth: Replace `if...return false; return true' with `return ...'.

In functions returning bool, these are equivalent.

    Before:                 After:
        if (!foo())             return foo();
            return false;
        return true;

oauth.c: Remove unused assignments of function return value.

`if ((status = foo()) != OK) return false;' doesn't need to set local
variable status.

Replace FALSE and TRUE with C99's false and true.

Remove the duplicate definitions of FALSE and TRUE.  Change a few
variables from int to bool at the same time.  The semantics of bool are
different to int because `bool b = 42' maps 42 to 1, but if the code was
relying on that then it needs shaking out anyway.

print_intro(): Change parameter brief from int to bool.

flist.c: Use bool instead of int for some of the flags.

Replace boolean with bool everywhere.

boolean's comment said it existed to ensure storage was a char and not
an int so it could be packed in a struct, but the only struct using it
doesn't care about the space taken.

mh.h: Always define NMH_UNUSED as a cast to void.

The not-gcc-or-earlier-than-gcc-2 version was the identity function
seems unnecessary as what C99 compiler would complain about a cast to
void?

mh.h: Move the gcc-4.3-conditional macro definitions.

Having them outside of the gcc-2 ones makes the logic easier to follow
as there's no nesting.

dropsbr.c: Remove unused ntohl() macro, and its NTOHLSWAP test.

Not used since e6c91771 removed support for ./.foo.map index for ./foo
mailbox.

fakehttp.c: Remove unused LINESIZE macro; never used.