v5, 3 partssend-email: make produced outputs more readable

 * send-email: make produced outputs more readable by separating
   the result statuses and prompts from the subsequent messages

This series makes the outputs of "git send-mail" more readable, by adding vertical whitespace to make discerning the messages produced for each patch easy, and by adding vertical whitespace to separate the emitted prompts from the other produced messages. Having the prompts and some of the produced messages bunched together made the navigation through the produced outputs unnecessarily hard.

Making the outputs of "git send-mail" more readable is quite important, because the Git users rather regularly complain about the workflows that require project patches to be sent to various mailing lists. Making the produced outputs more readable can only help there.

Changes in v5:
    - Split into a three-patch series, because changes introduced in
      some versions of this patch made the original assumptions about
      squashing the changes together no longer apply [1]
    - Updated and extended the patch descriptions, to hopefully describe
      the changes performed in each patch a bit better

Changes in v4:
    - Dropped the changes to the styling of the produced prompts, as
      reasonably requested by Junio, [2] because it extended the scope
      of the patch with no good reason, and may also create issues on
      some platforms, whose Perl packages may actually not support the
      "->ornaments()" feature of Term::ReadLine
    - Updated the patch description and the "what's cooking" summary
      to cover the changes

Changes in v3:
    - Removed a redundant comment, as suggested by Junio [3]
    - Did the right thing and made the vertical separators emitted as
      message separators, instead of having them emitted as message
      terminators, as suggested by Junio [3]
    - Additional vertical whitespace is now also emitted after the
      prompt for sending emails, to "de-bunch" it from the subsequent
      messages and make discerning the messages easier
    - The above-mentioned prompt no longer uses underlined text, to make
      it significantly easier on the eyes
    - Fixed one indentation by spaces to use tabs and removed one stray
      newline in the source code, as spotted
    - Updated and extended the patch description to cover the changes
    - Updated the "what's cooking" summary to cover the changes
    - Cleaned up the older notes a bit

Changes in v2:
    - Improved the way additional newline separators are introduced to
      the source code, as suggested by Junio, [4], to help a bit with
      the translation efforts
    - Improved the patch subject and description a bit, to provide some
      additional information, as suggested by Junio [4]
    - Added a Helped-by tag

Notes for v1:
    - This is a resubmission of the patch I submitted about a week and
      a half ago; [5]  the patch subject in the original submission was
      selected in a bit unfortunate way, which this submission corrects,
      and also improves the patch description a bit
    - There are no changes to the patch itself, vs. the original patch

Link to v1: https://lore.kernel.org/git/62553db377c28458883b66bcdc0c58cc0f32d15b.1712250366.git.dsimic@manjaro.org/T/#u Link to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u Link to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u Link to v4: https://lore.kernel.org/git/8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org/T/#u

[1] https://lore.kernel.org/git/713c28cfc9dff2d01159105ddd2fd0f5@manjaro.org/ [2] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/ [3] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/ [4] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/ [5] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/

Dragan Simic (3):
  send-email: move newline character out of a translatable string
  send-email: make it easy to discern the messages for each patch
  send-email: separate the confirmation prompts from the messages

 git-send-email.perl | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Move the already existing newline character out of a translatable string, to help a bit with the translation efforts.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 git-send-email.perl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

When sending multiple patches at once, without prompting the user to confirm the sending of each patch separately, the displayed result statuses for each patch become bunched together with the messages produced for the subsequent patch. This unnecessarily makes discerning each of the result statuses a bit difficult, as visible in the sample output excerpt below:

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Result: 250
    OK. Log says:
    ...

As visible in the excerpt above, bunching the "Result: <status-code>" lines together with the messages produced for the subsequent patch makes the output unreadable, which actually becomes worse as the number of patches sent at once increases. To make the produced outputs more readable, add vertical whitespace (more precisely, a newline) between the displayed result statuses and the subsequent messages, as visible in the sample output excerpt below, produced after the addition of vertical whitespace:

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Result: 250

    OK. Log says:
    ...

These changes don't emit additional vertical whitespace after the result status produced for the last processed patch, i.e. the vertical whitespace is treated as a separator between the groups of produced messages, not as their terminator. This follows the Git's general approach of not wasting the vertical screen space whenever reasonably possible.

While there, remove a couple of spotted stray newlines in the source code and convert one indentation from spaces to tabs, for consistency.

The associated test, t9001, requires no updates to cover these changes.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 git-send-email.perl | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Dragan Simic <dsimic@manjaro.org> writes:

The above feels a bit roundabout way to say "the logic is that we need to add a gap before showing the next message, if we did things that cause the smtp traces to be shown", but OK.

I do not see this paragraph is relevant to the target audience. It may be a good advice to give to a reader who attempts to solve the problem this patch solved themselves, botches the attempt and ends up with a code with the terminator semantics. But for other readers of "git log" and reviewers of the patch, "I did not make a silly mistake, and instead correctly chose to use the separator semantics" is not something worth boasting about.

These are worth recording.

If you do not add this "else" clause to the outer "are we doing confirmation?" if statement, and instead just set $needs_separator *after* it, it would make it even more obvious what is going on. The codeflow would become

	sub send_message {
		do bunch of things that do not yet send e-mail
	        and possibly return or die

		$needs_separator = 1;

		do things that cause the smtp exchange and trace
		to be emitted
	}

That makes it obvious that the purpose of $needs_separator is to record the fact that "this" message has already been sent and we need to add a "gap" before attempting to send the "next" message.

Other than the above points, very well done.

nice ;-)

On 2024-04-08 23:08, Junio C Hamano wrote:

Yeah, the wording I used isn't perfect, but I think it's still understandable. I'll see to possibly improve it in the v6.

Makes sense, will delete that paragraph in the v6.

Thanks.

Very good point, thanks! I've somehow managed to miss that while iterating through a few code variants and the associated testing. Will be improved in the v6.

Thank you! :)

It had to be fixed, IMHO. :)

Emit additional vertical whitespace after the "Send this email [y/n/...]?" confirmation prompts, more specifically after each confirmed email is sent, but before the subsequent messages are emitted, to make the produced output more readable. The subsequent produced messages were bunched together with the confirmation prompts, as visible in the sample output excerpt below, which made discerning the outputs unnecessarily harder.

    ...
    Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
    OK. Log says:
    ...

The introduced changes don't emit additional vertical whitespace after the confirmation prompt if the user selects to skip sending the email they were asked about, or if the user selects to quit the procedure entirely. This follows the Git's general approach of not wasting the vertical screen space whenever reasonably possible.

The associated test, t9001, requires no updates to cover these changes.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 git-send-email.perl | 3 +++
 1 file changed, 3 insertions(+)

Dragan Simic <dsimic@manjaro.org> writes:

What comes before "send this email" prompt needs to be shown to make the argument more convincing, but with "..." there is no cue to decide if the output is hard to read.

"prompt separator" sounds more like a separator that separates prompts, but that is not what is going on, no?

Do we even need that new varible in the first place? I am wondering if you can just do the print "\n" right where you assign to that variable.

When $confirm is set to 'never', you have both $needs_separtor and $prompt_separator set. Would it mean you'd have two extra blank lines for that message?

All these questions you should have been able to avoided with a bit more helpful explanation in the proposed log message, I hope.

Thanks.

Hello Junio,

On 2024-04-08 23:21, Junio C Hamano wrote:

Believe it or not, I also saw the provided sample excerpt as too short. However, when I tried to make it longer and more obvious, it turned out that simply too many lines needed to be included. I'll give it some more though, and possibly delete the sample entirely.

Yup, I already wanted to rename that variable, because its name isn't really great, but I didn't do that because it went as such in earlier versions of the patch. Will rename it in the v6.

The reason why the newline isn't emitted right away is because sending the message takes some time, and if we print it right away, there's an additional empty line staring at the user while they wait for the message to be sent. If we emit the newline a bit later, using that variable, it gets displayed together with the printed message, making the displaying of the output much smoother.

I already tried to describe that behavior in the patch description. I'll try to improve that description in the v6.

Actually, there are no unneeded blank lines in that case, which I've also tested before sending the patches. One of the blank lines (the one for $needs_separator) goes after the patch messages, and the other one (the one for $prompt_separator) goes after the prompt, which is displayed before the patch messages.

I already tried to do that, but it seems it needed more detailed explanations in the patch description. Will be improved in the v6.

On 2024-04-09 05:25, Dragan Simic wrote:

Actually, there's a much, _much_ simpler solution for everything, which also works as expected when running "git send-email --quiet" with sendmail.confirm set to "auto" or "never".

I need to test it a bit more, and I'll send the updated patches.

On 2024-04-10 05:53, Dragan Simic wrote:

This new, simplified approach works well, but I've spotted some more scenarios that also require addition of newlines. Though, based on previous lessons, :) I'll leave that to the follow-up patches.