inc/test-eom-align: Fix MMDF-mbox `B' size calculation.

Also, have the email body end in what appears to be the MMDF magic
string for the first few bytes;  otherwise, the test mbox is just a
boring single email compared to the `A', From_, version.  Fixes
8699f1cc.

inc/test-eom-align: Fix MMDF-mbox `A' size calculation.

The `...LINESZ' variables are number of bytes, not number of lines, so
MMDF's is now 5 rather than 1.  And the MMDF magic string tops and tails
each email so needs to be subtracted twice, not once like From_'s.
Fixes 8699f1cc.

test/common.sh.in: Only rm(1) if the cd(1) succeeds.

The rm specified the full path anyway, but if the cd fails for some odd
reason then it's best not to continue.

annosbr.c: free(3) takes void pointer; don't cast non-const pointer.

(const pointers are another whole can of worms, and their const
poisoning causes much noise for little benefit.)

void-pointer parameters don't need casting to char pointer.

E.g. `memcmp((char *)digest...' doesn't need the cast if it's already a
pointer.

Replace `sizeof (char)' with 1, then remove if a multiplier.

Don't cast void pointer return value of malloc() et al.

Stop failed m_mktemp2() that give directory reporting get_temp_dir().

The first parameter of m_mktemp2(), if not NULL, is a path and the
basename is used as the directory to hold the new temporary file, but
the error message on failing to create that used get_temp_dir() and
that's often somewhere else, e.g. `mhbuild /dev/null' attempts /dev/...
Don't attempt to state the failed path in the error message as at least
that's not misleading.  Perhaps the function needs some other indication
of success so it can pass back the attempted filename that failed.

mhbuild.c: Don't use set_done() to unlink temporary files.

Because they're created with m_mktemp2(), the temporary files are
already in a list to be unlink(2)'d at exit(3), thanks to atexit(3), so
don't set_done() to attempt that twice.  Updates 6bc64765f.

mhshow/test-charset: Test iconv(1)'s result, not just its exit status.

Problems with generating the input to iconv(1) were hidden as long as it
could be converted from EBCDIC-US to UTF-8 because only the exit status
was tested.  Check the output is as expected too.

mhshow/test-charset: printf(1)'s octal doesn't need a leading zero.

The `\0344' was being interpreted as `\034' and `4'.  This can still be
converted from EBCDIC-US to UTF-8 by iconv(1) so the test worked, but it
was misleading.  Remove the leading zero as printf's octal escape is
`\nnn' where nnn is one to three octal digits.  It's echo(1) that
insists on a leading zero.  Fixes 6fd1d18f.

inc/test-eom-align: Use octal, not hex, in printf(1) format string.

POSIX's printf(1), and dash(1)'s, does not support hex.  Fixes 8699f1cc.

mhshow/test-charset: Use octal, not hex, in printf(1) format string.

POSIX's printf(1), and dash(1)'s, does not support hex.  Fixes 792d7e1e.

Added tests of MMDF mailbox format to test-eom-align.

runpty.c: Move child slave code into new run_command().

runpty.c: Handle fork(2) failure first, rather than child.

Gets the error handling out of the way so it can be forgotten.

runpty.c: Factor duplicate code into new open_master_pty().

Only called twice, but it shows the same actions are done both times.

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.