git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/3] pager: properly log pager exit code when signalled

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 1, 2021, 18:07 UTC
Message-ID
<xmqqft2fmzk6.fsf@gitster.c.googlers.com>
In-Reply-To
<20210201144921.8664-4-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 42 quoted lines
> When git invokes a pager that exits with non-zero the common case is
> that we'll already return the correct SIGPIPE failure from git itself,
> but the exit code logged in trace2 has always been incorrectly
> reported[1]. Fix that and log the correct exit code in the logs.
>
> Since this gives us something to test outside of our recently-added
> tests needing a !MINGW prerequisite, let's refactor the test to run on
> MINGW and actually check for SIGPIPE outside of MINGW.
>
> The wait_or_whine() is only called with a true "in_signal" from from
> finish_command_in_signal(), which in turn is only used in pager.c.
>
> I'm not quite sure about that BUG() case. Can we have a true in_signal
> and not have a true WIFEXITED(status)? I haven't been able to think of
> a test case for it.
>
> 1. The incorrect logging of the exit code in was seemingly copy/pasted
>    into finish_command_in_signal() in ee4512ed481 (trace2: create new
>    combined trace facility, 2019-02-22)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  run-command.c    |  8 +++++--
>  t/t7006-pager.sh | 61 +++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 58 insertions(+), 11 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index ea4d0fb4b15..10e1c96c2bd 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -551,8 +551,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>  
>  	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
>  		;	/* nothing */
> -	if (in_signal)
> -		return 0;
> +	if (in_signal && WIFEXITED(status))
> +		return WEXITSTATUS(status);
> +	if (in_signal) {
> +		BUG("was not expecting waitpid() status %d", status);
> +		return -1;
> +	}

Doesn't BUG die, never to return control back to us? How about "warning()" or "error()"?

Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 35 of 60 in “git fails with a broken pipe when one quits the pager”
  1. Vincent LefevreJan 15, 2021
  2. pager: exit without error on SIGPIPEDenton Liu, Jan 29, 2021
  3. Johannes SixtJan 30, 2021
  4. Johannes SixtJan 30, 2021
  5. Ævar Arnfjörð BjarmasonFeb 1, 2021
  6. Junio C HamanoFeb 1, 2021
  7. Ævar Arnfjörð BjarmasonFeb 1, 2021
  8. Junio C HamanoFeb 1, 2021
  9. Ævar Arnfjörð BjarmasonFeb 2, 2021
  10. Junio C HamanoFeb 2, 2021
  11. Junio C HamanoFeb 2, 2021
  12. Johannes SixtFeb 2, 2021
  13. Junio C HamanoFeb 2, 2021
  14. Johannes SixtFeb 2, 2021
  15. Junio C HamanoFeb 2, 2021
  16. Johannes SixtFeb 3, 2021
  17. Junio C HamanoFeb 3, 2021
  18. Vincent LefevreFeb 4, 2021
  19. Ævar Arnfjörð BjarmasonFeb 3, 2021
  20. Junio C HamanoFeb 3, 2021
  21. Ævar Arnfjörð BjarmasonFeb 3, 2021
  22. Johannes SixtFeb 3, 2021
  23. Ævar Arnfjörð BjarmasonJan 31, 2021
  24. Vincent LefevreJan 31, 2021
  25. Vincent LefevreJan 31, 2021
  26. Ævar Arnfjörð BjarmasonJan 31, 2021
  27. Vincent LefevreFeb 1, 2021
  28. Chris TorekFeb 1, 2021
  29. Vincent LefevreFeb 1, 2021
  30. Chris TorekFeb 1, 2021
  31. Vincent LefevreFeb 1, 2021
  32. Ævar Arnfjörð BjarmasonFeb 1, 2021
  33. Ævar Arnfjörð BjarmasonFeb 1, 2021
  34. 3/3 pager: properly log pager exit code when signalledÆvar Arnfjörð Bjarmason, Feb 1, 2021
  35. Junio C HamanoFeb 1, 2021
  36. Ævar Arnfjörð BjarmasonFeb 1, 2021
  37. Junio C HamanoFeb 1, 2021
  38. Ævar Arnfjörð BjarmasonFeb 1, 2021
  39. 2/3 pager: refactor wait_for_pager() functionÆvar Arnfjörð Bjarmason, Feb 1, 2021
  40. 1/3 pager: test for exit codeÆvar Arnfjörð Bjarmason, Feb 1, 2021
  41. 0/3 pager: test for exit behavior & trace2 bug fixÆvar Arnfjörð Bjarmason, Feb 1, 2021
  42. 1/5 pager: refactor wait_for_pager() functionÆvar Arnfjörð Bjarmason, Feb 2, 2021
  43. 0/5 pager: test for exit behavior & trace2 bug fixÆvar Arnfjörð Bjarmason, Feb 2, 2021
  44. 3/5 run-command: add braces for "if" block in wait_or_whine()Ævar Arnfjörð Bjarmason, Feb 2, 2021
  45. 2/5 pager: test for exit code with and without SIGPIPEÆvar Arnfjörð Bjarmason, Feb 2, 2021
  46. Denton LiuFeb 2, 2021
  47. Johannes SixtFeb 5, 2021
  48. 4/5 pager: properly log pager exit code when signalledÆvar Arnfjörð Bjarmason, Feb 2, 2021
  49. Johannes SixtFeb 5, 2021
  50. Junio C HamanoFeb 5, 2021
  51. 5/5 WIP pager: respect exit code of pager over SIGPIPEÆvar Arnfjörð Bjarmason, Feb 2, 2021
  52. Vincent LefevreFeb 1, 2021
  53. Ævar Arnfjörð BjarmasonFeb 1, 2021
  54. Johannes SixtFeb 1, 2021
  55. Ævar Arnfjörð BjarmasonFeb 3, 2021
  56. Johannes SixtFeb 3, 2021
  57. Vincent LefevreFeb 3, 2021
  58. Ævar Arnfjörð BjarmasonFeb 4, 2021
  59. Vincent LefevreFeb 4, 2021
  60. Johannes SixtFeb 1, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.