Re: [PATCH v2 2/2] imap-send: improve error messages with configuration hints

[Junio C Hamano <gitster@pobox.com>]

Joerg Thalheim <joerg@thalheim.io> writes:

> From: Jörg Thalheim <joerg@thalheim.io>
>
> Replace basic error messages with more helpful ones that guide users
> on how to resolve configuration issues. When imap.host or imap.folder
> are missing, provide the exact git config commands needed to fix the
> problem, along with examples of typical values.
>
> This uses the advise() API to display hints that can be disabled
> by users who don't want them,

Not quite.  We do not encourage or condone wholesale disabling of
all advise messages by end users.  The advice_if_enabled() helper
allows selective disabling of individual messages that the user has
seen often enough and find no more need for help, but I do not think
it is advisable (no pun intended) to use it in these particular code
paths, simply because once the relevant configuration variables are
set, the users will not see the errors anymore.

The reason advise() was suggested was primarily because of its
better handling of multi-line messages.  It can be fed a multi-line
message (i.e., with embedded LF in it), and give "hint:" prefix to
each line, which is lacking from the error(), warning(), die() family
of messaging functions.

> +			error(_("no IMAP host specified"));
> +			advise(_("set the IMAP host with 'git config imap.host <host>'"));
> +			advise(_("(e.g., 'git config imap.host imaps://imap.example.com')"));

Give a single multi-line message to a single advise() call, i.e.,

	advice(_("set the IMAP host with ... <host>'.\n"
        	 "(e.g., 'git config ....com')"));

instead of two back-to-back calls to advise.  It allows the
translators to find points to break lines better than the programmer
who wrote these advise() call(s).

> @@ -1831,7 +1834,9 @@ int cmd_main(int argc, const char **argv)
>  	}
>  
>  	if (!server.folder) {
> -		fprintf(stderr, "no IMAP folder specified\n");
> +		error(_("no IMAP folder specified"));
> +		advise(_("set the target folder with 'git config imap.folder <folder>'"));
> +		advise(_("(e.g., 'git config imap.folder Drafts')"));

Ditto.

As a #leftoverbit we might want to inspect many fprintf(stderr)
still remain in this program after this patch and turn them into
calls to appropriate error() or warning() or whatever, but that
certainly would be outside the scope of this patch.

Thanks.