{"thread":{"id":"51708","subject":"v2.22.1 and later regression wrt display of progress indicators","startedAt":"2019-08-22T14:20:15Z","lastAt":"2019-10-02T15:47:37Z","messageCount":11,"participants":["Charles Diza","SZEDER Gábor","Jeff King","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380934","messageId":"20190822141928.GA3163@323642-phi-gmno-clahs","threadId":"51708","inReplyTo":null,"subject":"v2.22.1 and later regression wrt display of progress indicators","fromName":"Charles Diza","fromEmail":"chdiza@gmail.com","sentAt":"2019-08-22T14:20:08Z","receivedAt":"2019-08-22T14:20:15Z","isPatch":false,"sender":{"key":"chdiza@gmail.com","avatar":null},"body":"By 2.22.1 at the latest (and continuing into 2.23.0) there is a problem with the display of progress indication during `git pull` (and possibly other commands, I don't know).\n\nI'm on macOS, and this happens in both Terminal.app and iTerm2.app, on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide terminal window, cd into a git repo and do `git pull`. The chances are high (though not 100%) that one will see this:\n\nremote: remote: Enumerating objects: 82, done.\nremote: remote: remote: remote: remote: \n[snip a large number of duplicate lines]\nremote: remote: \nremote: Counting objects: 100% (82/82), done.\nremote: remote: remote: remote: remote: remote: remote: Compressing objects: 100% (5/5), done.\nremote: Total 111 (delta 78), reused 78 (delta 77), pack-reused 29\n\nIt used to be that there were *no* lines that appear to consist just of \"remote:\". I say \"appear\" because most occurrences of \"remote:\" are followed by a bunch of spaces rather than linebreaks.\n\nCharles\n\n"},{"id":"380943","messageId":"20190822160702.GD20404@szeder.dev","threadId":"51708","inReplyTo":"20190822141928.GA3163@323642-phi-gmno-clahs","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-22T16:07:02Z","receivedAt":"2019-08-22T16:07:08Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 22, 2019 at 10:20:08AM -0400, Charles Diza wrote:\n> By 2.22.1 at the latest (and continuing into 2.23.0) there is a\n> problem with the display of progress indication during `git pull`\n> (and possibly other commands, I don't know).\n> \n> I'm on macOS, and this happens in both Terminal.app and iTerm2.app,\n> on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide\n> terminal window, cd into a git repo and do `git pull`. The chances\n> are high (though not 100%) that one will see this:\n\nI noticed this today when pushing to GitHub (I suppose they have very\nrecently upgraded?) from Linux, so this is neither specific to 'git\npull' nor to macOS.\n\nI'm sure the culprits are commits cd1096b282 (pager: add a helper\nfunction to clear the last line in the terminal, 2019-06-24) and\n5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\nadded complication of communicating with a remote.\n\nIf the standard error of 'git pull/push' is connected to a terminal,\nthen it will tell the transport helpers and in turn to the 'git\nupload-pack/receive-pack' processes on the remote to produce progress\noutput. However, since those processes run on the other side of the\ninternet, they don't have any idea about the local terminal (smart or\ndumb? how wide?) their progress will end up on, and, consequently,\nthey assume the worst, i.e. standard-width dumb terminal, and use 80\nspaces to cover up the previously displayed progress line.\n\nI'm not sure how to handle the situation. A few ideas to consider:\n\n 1. Update 'git upload-pack/receive-pack' to use some kind of magic\n character or char sequence instead of a \"real\" line clearing\n sequence, and update 'git pull/push' to replace that magic with\n the line clearing sequence appropriate for the terminal.\n\n 2. Variant of the above: leave 'git upload-pack/receive-pack' as they\n are now, and declare that those 80 spaces indicate when to clear\n progress lines. Update 'git push/pull' to catch those 80 spaces,\n and replace them with the line clearing sequence appropriate for\n the terminal.\n\n 3. Update 'git pull/push' to explicitly tell the remote what line\n clearing sequence to use.\n\n 4. Revert, and go back to calculating how many spaces we need to\n append to clear the previously displayed progress line, and hope\n that we don't mess it up (or even if we do, it still won't be as\n noticable as this).\n\nI suppose this issue affects other git clients as well, so (1), (2),\nand (3) might not even be an option.\n\n"},{"id":"380950","messageId":"20190822162907.GA17013@sigill.intra.peff.net","threadId":"51708","inReplyTo":"20190822160702.GD20404@szeder.dev","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-22T16:29:08Z","receivedAt":"2019-08-22T16:29:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 22, 2019 at 06:07:02PM +0200, SZEDER Gábor wrote:\n\n> On Thu, Aug 22, 2019 at 10:20:08AM -0400, Charles Diza wrote:\n> > By 2.22.1 at the latest (and continuing into 2.23.0) there is a\n> > problem with the display of progress indication during `git pull`\n> > (and possibly other commands, I don't know).\n> > \n> > I'm on macOS, and this happens in both Terminal.app and iTerm2.app,\n> > on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide\n> > terminal window, cd into a git repo and do `git pull`. The chances\n> > are high (though not 100%) that one will see this:\n> \n> I noticed this today when pushing to GitHub (I suppose they have very\n> recently upgraded?) from Linux, so this is neither specific to 'git\n> pull' nor to macOS.\n> \n> I'm sure the culprits are commits cd1096b282 (pager: add a helper\n> function to clear the last line in the terminal, 2019-06-24) and\n> 5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\n> added complication of communicating with a remote.\n\nYes, we moved to v2.22.1 last night. I'll revert those commits on our\nservers until we come up with a more permanent solution upstream.\n\n> I'm not sure how to handle the situation. A few ideas to consider:\n> \n> 1. Update 'git upload-pack/receive-pack' to use some kind of magic\n> character or char sequence instead of a \"real\" line clearing\n> sequence, and update 'git pull/push' to replace that magic with\n> the line clearing sequence appropriate for the terminal.\n> \n> 2. Variant of the above: leave 'git upload-pack/receive-pack' as they\n> are now, and declare that those 80 spaces indicate when to clear\n> progress lines. Update 'git push/pull' to catch those 80 spaces,\n> and replace them with the line clearing sequence appropriate for\n> the terminal.\n> \n> 3. Update 'git pull/push' to explicitly tell the remote what line\n> clearing sequence to use.\n> \n> 4. Revert, and go back to calculating how many spaces we need to\n> append to clear the previously displayed progress line, and hope\n> that we don't mess it up (or even if we do, it still won't be as\n> noticable as this).\n> \n> I suppose this issue affects other git clients as well, so (1), (2),\n> and (3) might not even be an option.\n\nYes on that final bit. We could always fall back to (4) if the terminal\ninformation is not available, but given that the benefit is mostly in\nsimplifying the code, I don't know if it's worth carrying around _two_\nsolutions.\n\nOne interesting bit: we have traditionally used \\033[K on the _client_\nside of the sideband demuxer. So I think in the \"remote:\" case we were\nalready handling this correctly, even before your patch.\n\n-Peff\n"},{"id":"380951","messageId":"20190822164032.GF20404@szeder.dev","threadId":"51708","inReplyTo":"20190822162907.GA17013@sigill.intra.peff.net","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-22T16:40:32Z","receivedAt":"2019-08-22T16:40:39Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 22, 2019 at 12:29:08PM -0400, Jeff King wrote:\n> On Thu, Aug 22, 2019 at 06:07:02PM +0200, SZEDER Gábor wrote:\n> > I noticed this today when pushing to GitHub (I suppose they have very\n> > recently upgraded?) from Linux, so this is neither specific to 'git\n> > pull' nor to macOS.\n> > \n> > I'm sure the culprits are commits cd1096b282 (pager: add a helper\n> > function to clear the last line in the terminal, 2019-06-24) and\n> > 5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\n> > added complication of communicating with a remote.\n> \n> Yes, we moved to v2.22.1 last night. I'll revert those commits on our\n> servers until we come up with a more permanent solution upstream.\n\nI think it's sufficient to revert only 5b12e3123b (progress: use\nterm_clear_line(), 2019-06-24). I only mentioned cd1096b282 here\nbecause it better explains the reasons for having term_clear_line(),\nand an other patch depends on that function as well.\n\nOr you can simply revert the whole series, of course :)\n\n> One interesting bit: we have traditionally used \\033[K on the _client_\n> side of the sideband demuxer. So I think in the \"remote:\" case we were\n> already handling this correctly, even before your patch.\n\nGah, I feared that the term \"sideband multiplexer\" will soon come up\nin this discussion...\n\n"},{"id":"380954","messageId":"20190822170435.GA26163@sigill.intra.peff.net","threadId":"51708","inReplyTo":"20190822164032.GF20404@szeder.dev","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-22T17:04:35Z","receivedAt":"2019-08-22T17:04:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 22, 2019 at 06:40:32PM +0200, SZEDER Gábor wrote:\n\n> > Yes, we moved to v2.22.1 last night. I'll revert those commits on our\n> > servers until we come up with a more permanent solution upstream.\n> \n> I think it's sufficient to revert only 5b12e3123b (progress: use\n> term_clear_line(), 2019-06-24). I only mentioned cd1096b282 here\n> because it better explains the reasons for having term_clear_line(),\n> and an other patch depends on that function as well.\n\nYes, that's what we did. :) It's out now, so everybody should see the\nfix.\n\n> > One interesting bit: we have traditionally used \\033[K on the _client_\n> > side of the sideband demuxer. So I think in the \"remote:\" case we were\n> > already handling this correctly, even before your patch.\n> \n> Gah, I feared that the term \"sideband multiplexer\" will soon come up\n> in this discussion...\n\nIt may open up some new options for us, though.\n\nIf we know on the server side that we are serving a remote request\n(which is easy, and does not require client cooperation), can we assume\nthat the other side will show any sideband lines with its own\nend-of-line clearing?\n\nI'm not sure. Git does and always has (and even on a dumb terminal makes\na token effort at throwing some spaces in ;) ). But I'm not sure if\nother clients do (and they of course might not even be displaying to a\nterminal at all, if they're pulling sideband data into a GUI dialog or\nsimilar).\n\n-Peff\n"},{"id":"380956","messageId":"20190822171609.GA56720@syl.lan","threadId":"51708","inReplyTo":"20190822160702.GD20404@szeder.dev","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-08-22T17:16:09Z","receivedAt":"2019-08-22T17:16:13Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nOn Thu, Aug 22, 2019 at 06:07:02PM +0200, SZEDER Gábor wrote:\n> On Thu, Aug 22, 2019 at 10:20:08AM -0400, Charles Diza wrote:\n> > By 2.22.1 at the latest (and continuing into 2.23.0) there is a\n> > problem with the display of progress indication during `git pull`\n> > (and possibly other commands, I don't know).\n> >\n> > I'm on macOS, and this happens in both Terminal.app and iTerm2.app,\n> > on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide\n> > terminal window, cd into a git repo and do `git pull`. The chances\n> > are high (though not 100%) that one will see this:\n>\n> I noticed this today when pushing to GitHub (I suppose they have very\n> recently upgraded?) from Linux, so this is neither specific to 'git\n> pull' nor to macOS.\n\nThere's already lots of discussion later on in the thread (which I'm\nreading now), but I just wanted to reply to this hunk to say 'thanks'\nfor pointing out the issue in the first place.\n\n> I'm sure the culprits are commits cd1096b282 (pager: add a helper\n> function to clear the last line in the terminal, 2019-06-24) and\n> 5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\n> added complication of communicating with a remote.\n\nThanks also for pointing out the culprits here and saving me a\nbisection.\n\nIf you're curious, I only reverted 5b12e3123b (progress: use\nterm_clear_line(), 2019-06-24), not the former.\n\n> [snip]\n\n\nThanks,\nTaylor\n"},{"id":"380957","messageId":"20190822171910.GA56780@syl.lan","threadId":"51708","inReplyTo":"20190822162907.GA17013@sigill.intra.peff.net","subject":"Re: v2.22.1 and later regression wrt display of progress indicators","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-08-22T17:19:10Z","receivedAt":"2019-08-22T17:19:14Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Aug 22, 2019 at 12:29:08PM -0400, Jeff King wrote:\n> On Thu, Aug 22, 2019 at 06:07:02PM +0200, SZEDER Gábor wrote:\n>\n> > On Thu, Aug 22, 2019 at 10:20:08AM -0400, Charles Diza wrote:\n> > > By 2.22.1 at the latest (and continuing into 2.23.0) there is a\n> > > problem with the display of progress indication during `git pull`\n> > > (and possibly other commands, I don't know).\n> > >\n> > > I'm on macOS, and this happens in both Terminal.app and iTerm2.app,\n> > > on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide\n> > > terminal window, cd into a git repo and do `git pull`. The chances\n> > > are high (though not 100%) that one will see this:\n> >\n> > I noticed this today when pushing to GitHub (I suppose they have very\n> > recently upgraded?) from Linux, so this is neither specific to 'git\n> > pull' nor to macOS.\n> >\n> > I'm sure the culprits are commits cd1096b282 (pager: add a helper\n> > function to clear the last line in the terminal, 2019-06-24) and\n> > 5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\n> > added complication of communicating with a remote.\n>\n> Yes, we moved to v2.22.1 last night. I'll revert those commits on our\n> servers until we come up with a more permanent solution upstream.\n>\n> > I'm not sure how to handle the situation. A few ideas to consider:\n> >\n> > 1. Update 'git upload-pack/receive-pack' to use some kind of magic\n> > character or char sequence instead of a \"real\" line clearing\n> > sequence, and update 'git pull/push' to replace that magic with\n> > the line clearing sequence appropriate for the terminal.\n> >\n> > 2. Variant of the above: leave 'git upload-pack/receive-pack' as they\n> > are now, and declare that those 80 spaces indicate when to clear\n> > progress lines. Update 'git push/pull' to catch those 80 spaces,\n> > and replace them with the line clearing sequence appropriate for\n> > the terminal.\n> >\n> > 3. Update 'git pull/push' to explicitly tell the remote what line\n> > clearing sequence to use.\n> >\n> > 4. Revert, and go back to calculating how many spaces we need to\n> > append to clear the previously displayed progress line, and hope\n> > that we don't mess it up (or even if we do, it still won't be as\n> > noticable as this).\n> >\n> > I suppose this issue affects other git clients as well, so (1), (2),\n> > and (3) might not even be an option.\n>\n> Yes on that final bit. We could always fall back to (4) if the terminal\n> information is not available, but given that the benefit is mostly in\n> simplifying the code, I don't know if it's worth carrying around _two_\n> solutions.\n\nMy thoughts exactly. I think that I prefer the solutions in the order\n{2, 1, 3, 4}, but (4) seems to be the most feasible by far.\n\nShould we revert the series?\n\n> One interesting bit: we have traditionally used \\033[K on the _client_\n> side of the sideband demuxer. So I think in the \"remote:\" case we were\n> already handling this correctly, even before your patch.\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"382458","messageId":"20190916205412.8602-1-szeder.dev@gmail.com","threadId":"51708","inReplyTo":"20190822162907.GA17013@sigill.intra.peff.net","subject":"[PATCH 0/2] Revert \"progress: use term_clear_line()\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-16T20:54:10Z","receivedAt":"2019-09-16T20:54:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 22, 2019 at 12:29:08PM -0400, Jeff King wrote:\n> On Thu, Aug 22, 2019 at 06:07:02PM +0200, SZEDER Gábor wrote:\n> \n> > On Thu, Aug 22, 2019 at 10:20:08AM -0400, Charles Diza wrote:\n> > > By 2.22.1 at the latest (and continuing into 2.23.0) there is a\n> > > problem with the display of progress indication during `git pull`\n> > > (and possibly other commands, I don't know).\n> > > \n> > > I'm on macOS, and this happens in both Terminal.app and iTerm2.app,\n> > > on both macOS 10.13.6 and 10.14.6: In a standard 80-column wide\n> > > terminal window, cd into a git repo and do `git pull`. The chances\n> > > are high (though not 100%) that one will see this:\n> > \n> > I noticed this today when pushing to GitHub (I suppose they have very\n> > recently upgraded?) from Linux, so this is neither specific to 'git\n> > pull' nor to macOS.\n> > \n> > I'm sure the culprits are commits cd1096b282 (pager: add a helper\n> > function to clear the last line in the terminal, 2019-06-24) and\n> > 5b12e3123b (progress: use term_clear_line(), 2019-06-24) with the\n> > added complication of communicating with a remote.\n\n> > I'm not sure how to handle the situation. A few ideas to consider:\n\n> > 4. Revert, and go back to calculating how many spaces we need to\n> > append to clear the previously displayed progress line, and hope\n> > that we don't mess it up (or even if we do, it still won't be as\n> > noticable as this).\n> > \n> > I suppose this issue affects other git clients as well, so (1), (2),\n> > and (3) might not even be an option.\n> \n> Yes on that final bit. We could always fall back to (4) if the terminal\n> information is not available, but given that the benefit is mostly in\n> simplifying the code, I don't know if it's worth carrying around _two_\n> solutions.\n\nOk, so here is a patch to revert 5b12e3123b (progress: use\nterm_clear_line(), 2019-06-24) with proper explanation.\n\nAs a bonus there is a new test script exercising the progress display\nas well, in particular how it covers up the previous progress line, so\nwe may have a bit more confidence in it.\n\n\nSZEDER Gábor (2):\n Revert \"progress: use term_clear_line()\"\n Test the progress display\n\n Makefile | 1 +\n progress.c | 61 ++++++--\n t/helper/test-progress.c | 81 ++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0500-progress-display.sh | 286 ++++++++++++++++++++++++++++++++++++\n t/t5541-http-push-smart.sh | 6 +-\n 7 files changed, 421 insertions(+), 16 deletions(-)\n create mode 100644 t/helper/test-progress.c\n create mode 100755 t/t0500-progress-display.sh\n\n-- \n2.23.0.331.g4e51dcdf11\n\n"},{"id":"382459","messageId":"20190916205412.8602-2-szeder.dev@gmail.com","threadId":"51708","inReplyTo":"20190916205412.8602-1-szeder.dev@gmail.com","subject":"[PATCH 1/2] Revert \"progress: use term_clear_line()\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-16T20:54:11Z","receivedAt":"2019-09-16T20:54:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This reverts commit 5b12e3123b (progress: use term_clear_line(),\n2019-06-24), because covering up the entire last line while refreshing\nthe progress line caused unexpected problems during 'git\nclone/fetch/push':\n\n $ git clone ssh://localhost/home/szeder/src/tmp/linux.git/\n Cloning into 'linux'...\n remote:\n remote:\n remote:\n remote: Enumerating objects: 999295\n\nThe length of the progress bar line can shorten when it includes\nthroughput and the unit changes, or when its length exceeds the width\nof the terminal and is broken into two lines. In these cases the\npreviously displayed longer progress line should be covered up,\nbecause otherwise the leftover characters from the previous progress\nline make the output look weird [1]. term_clear_line() makes this\nquite simple, as it covers up the entire last line either by using an\nANSI control sequence or by printing a terminal width worth of space\ncharacters, depending on whether the terminal is smart or dumb.\n\nUnfortunately, when accessing a remote repository via any non-local\nprotocol the remote 'git receive-pack/upload-pack' processes can't\npossibly have any idea about the local terminal (smart of dumb? how\nwide?) their progress will end up on. Consequently, they assume the\nworst, i.e. standard-width dumb terminal, and print 80 spaces to cover\nup the previously displayed progress line. The local 'git\nclone/fetch/push' processes then display the remote's progress,\nincluding these coverup spaces, with the 'remote: ' prefix, resulting\nin a total line length of 88 characters. If the local terminal is\nnarrower than that, then the coverup gets line-wrapped, and after that\nthe CR at the end doesn't return to the beginning of the progress\nline, but to the first column of its last line, resulting in those\nrepeated 'remote: ' lines.\n\nBy reverting 5b12e3123b (progress: use term_clear_line(),\n2019-06-24) we won't cover up the entire last line, but go back to\ncomparing the length of the current progress bar line with the\nprevious one, and cover up as many characters as needed.\n\n[1] See commits 545dc345eb (progress: break too long progress bar\n lines, 2019-04-12) and 9f1fd84e15 (progress: clear previous\n progress update dynamically, 2019-04-12).\n\nSigned-off-by: SZEDER Gábor \n---\n progress.c | 29 ++++++++++++++++++-----------\n t/t5541-http-push-smart.sh | 6 +++---\n 2 files changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 277db8afa2..0eddf1804d 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -88,6 +88,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tint last_count_len = counters_sb->len;\n \n \tif (progress->delay && (!progress_update || --progress->delay))\n \t\treturn;\n@@ -115,21 +116,27 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tif (show_update) {\n \t\tif (is_foreground_fd(fileno(stderr)) || done) {\n \t\t\tconst char *eol = done ? done : \"\\r\";\n+\t\t\tsize_t clear_len = counters_sb->len < last_count_len ?\n+\t\t\t\t\tlast_count_len - counters_sb->len + 1 :\n+\t\t\t\t\t0;\n+\t\t\t/* The \"+ 2\" accounts for the \": \". */\n+\t\t\tsize_t progress_line_len = progress->title_len +\n+\t\t\t\t\t\tcounters_sb->len + 2;\n+\t\t\tint cols = term_columns();\n \n-\t\t\tterm_clear_line();\n \t\t\tif (progress->split) {\n-\t\t\t\tfprintf(stderr, \" %s%s\", counters_sb->buf,\n-\t\t\t\t\teol);\n-\t\t\t} else if (!done &&\n-\t\t\t\t /* The \"+ 2\" accounts for the \": \". */\n-\t\t\t\t term_columns() < progress->title_len +\n-\t\t\t\t\t\t counters_sb->len + 2) {\n-\t\t\t\tfprintf(stderr, \"%s:\\n %s%s\",\n-\t\t\t\t\tprogress->title, counters_sb->buf, eol);\n+\t\t\t\tfprintf(stderr, \" %s%*s\", counters_sb->buf,\n+\t\t\t\t\t(int) clear_len, eol);\n+\t\t\t} else if (!done && cols < progress_line_len) {\n+\t\t\t\tclear_len = progress->title_len + 1 < cols ?\n+\t\t\t\t\t cols - progress->title_len - 1 : 0;\n+\t\t\t\tfprintf(stderr, \"%s:%*s\\n %s%s\",\n+\t\t\t\t\tprogress->title, (int) clear_len, \"\",\n+\t\t\t\t\tcounters_sb->buf, eol);\n \t\t\t\tprogress->split = 1;\n \t\t\t} else {\n-\t\t\t\tfprintf(stderr, \"%s: %s%s\", progress->title,\n-\t\t\t\t\tcounters_sb->buf, eol);\n+\t\t\t\tfprintf(stderr, \"%s: %s%*s\", progress->title,\n+\t\t\t\t\tcounters_sb->buf, (int) clear_len, eol);\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex b86ddb60f2..92bac43257 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -262,7 +262,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \ttest_commit noisy &&\n \ttest_terminal git push >output 2>&1 &&\n-\ttest_i18ngrep \"Writing objects\" output\n+\ttest_i18ngrep \"^Writing objects\" output\n '\n \n test_expect_success TTY 'push --quiet silences status and progress' '\n@@ -277,7 +277,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '\n \ttest_commit no-progress &&\n \ttest_terminal git push --no-progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep ! \"Writing objects\" output\n+\ttest_i18ngrep ! \"^Writing objects\" output\n '\n \n test_expect_success 'push --progress shows progress to non-tty' '\n@@ -285,7 +285,7 @@ test_expect_success 'push --progress shows progress to non-tty' '\n \ttest_commit progress &&\n \tgit push --progress >output 2>&1 &&\n \ttest_i18ngrep \"^To http\" output &&\n-\ttest_i18ngrep \"Writing objects\" output\n+\ttest_i18ngrep \"^Writing objects\" output\n '\n \n test_expect_success 'http push gives sane defaults to reflog' '\n-- \n2.23.0.331.g4e51dcdf11\n\n"},{"id":"382460","messageId":"20190916205412.8602-3-szeder.dev@gmail.com","threadId":"51708","inReplyTo":"20190916205412.8602-1-szeder.dev@gmail.com","subject":"[PATCH 2/2] Test the progress display","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-16T20:54:12Z","receivedAt":"2019-09-16T20:54:55Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'progress.c' has seen a few fixes recently [1], and, unfortunately,\nsome of those fixes required further fixes [2]. It seems it's time to\nhave a few tests focusing on the subtleties of the progress display.\n\nAdd the 'test-tool progress' subcommand to help testing the progress\ndisplay, reading instructions from standard input and turning them\ninto calls to the display_progress() and display_throughput()\nfunctions with the given parameters.\n\nThe progress display is, however, critically dependent on timing,\nbecause it's only updated once every second or, if the toal is known\nin advance, every 1%, and there is the throughput rate as well. These\nmake the progress display far too undeterministic for testing as-is.\nTo address this, add a few testing-specific variables and functions to\n'progress.c', allowing the the new test helper to:\n\n - Disable the triggered-every-second SIGALRM and set the\n 'progress_update' flag explicitly based in the input instructions.\n This way the progress line will be updated deterministically when\n the test wants it to be updated.\n\n - Specify the time elapsed since start_progress() to make the\n throughput rate calculations deterministic.\n\nAdd the new test script 't0500-progress-display.sh' to check a few\nsimple cases with and without throughput, and that a shorter progress\nline properly covers up the previously displayed line in different\nsituations.\n\n[1] See commits 545dc345eb (progress: break too long progress bar\n lines, 2019-04-12) and 9f1fd84e15 (progress: clear previous\n progress update dynamically, 2019-04-12).\n[2] 1aed1a5f25 (progress: avoid empty line when breaking the progress\n line, 2019-05-19)\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 1 +\n progress.c | 32 +++-\n t/helper/test-progress.c | 81 ++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0500-progress-display.sh | 286 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 400 insertions(+), 2 deletions(-)\n create mode 100644 t/helper/test-progress.c\n create mode 100755 t/t0500-progress-display.sh\n\ndiff --git a/Makefile b/Makefile\nindex ad71ae1219..3ca48ec4b0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -728,6 +728,7 @@ TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-path-utils.o\n TEST_BUILTINS_OBJS += test-pkt-line.o\n TEST_BUILTINS_OBJS += test-prio-queue.o\n+TEST_BUILTINS_OBJS += test-progress.o\n TEST_BUILTINS_OBJS += test-reach.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-read-midx.o\ndiff --git a/progress.c b/progress.c\nindex 0eddf1804d..0063559aab 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -45,6 +45,19 @@ struct progress {\n \n static volatile sig_atomic_t progress_update;\n \n+/*\n+ * These are only intended for testing the progress output, i.e. exclusively\n+ * for 'test-tool progress'.\n+ */\n+int progress_testing;\n+uint64_t progress_test_ns = 0;\n+void progress_test_force_update(void); /* To silence -Wmissing-prototypes */\n+void progress_test_force_update(void)\n+{\n+\tprogress_update = 1;\n+}\n+\n+\n static void progress_interval(int signum)\n {\n \tprogress_update = 1;\n@@ -55,6 +68,9 @@ static void set_progress_signal(void)\n \tstruct sigaction sa;\n \tstruct itimerval v;\n \n+\tif (progress_testing)\n+\t\treturn;\n+\n \tprogress_update = 0;\n \n \tmemset(&sa, 0, sizeof(sa));\n@@ -72,6 +88,10 @@ static void set_progress_signal(void)\n static void clear_progress_signal(void)\n {\n \tstruct itimerval v = {{0,},};\n+\n+\tif (progress_testing)\n+\t\treturn;\n+\n \tsetitimer(ITIMER_REAL, &v, NULL);\n \tsignal(SIGALRM, SIG_IGN);\n \tprogress_update = 0;\n@@ -154,6 +174,14 @@ static void throughput_string(struct strbuf *buf, uint64_t total,\n \tstrbuf_humanise_rate(buf, rate * 1024);\n }\n \n+static uint64_t progress_getnanotime(struct progress *progress)\n+{\n+\tif (progress_testing)\n+\t\treturn progress->start_ns + progress_test_ns;\n+\telse\n+\t\treturn getnanotime();\n+}\n+\n void display_throughput(struct progress *progress, uint64_t total)\n {\n \tstruct throughput *tp;\n@@ -164,7 +192,7 @@ void display_throughput(struct progress *progress, uint64_t total)\n \t\treturn;\n \ttp = progress->throughput;\n \n-\tnow_ns = getnanotime();\n+\tnow_ns = progress_getnanotime(progress);\n \n \tif (!tp) {\n \t\tprogress->throughput = tp = xcalloc(1, sizeof(*tp));\n@@ -296,7 +324,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tstruct throughput *tp = progress->throughput;\n \n \t\tif (tp) {\n-\t\t\tuint64_t now_ns = getnanotime();\n+\t\t\tuint64_t now_ns = progress_getnanotime(progress);\n \t\t\tunsigned int misecs, rate;\n \t\t\tmisecs = ((now_ns - progress->start_ns) * 4398) >> 32;\n \t\t\trate = tp->curr_total / (misecs ? misecs : 1);\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nnew file mode 100644\nindex 0000000000..4e9f7fafdf\n--- /dev/null\n+++ b/t/helper/test-progress.c\n@@ -0,0 +1,81 @@\n+/*\n+ * A test helper to exercise the progress display.\n+ *\n+ * Reads instructions from standard input, one instruction per line:\n+ *\n+ * \"progress \" - Call display_progress() with the given item count\n+ * as parameter.\n+ * \"throughput - Call display_throughput() with the given\n+ * byte count as parameter. The 'millis'\n+ * specify the time elapsed since the\n+ * start_progress() call.\n+ * \"update\" - Set the 'progress_update' flag.\n+ *\n+ * See 't0500-progress-display.sh' for examples.\n+ */\n+#include \"test-tool.h\"\n+#include \"gettext.h\"\n+#include \"parse-options.h\"\n+#include \"progress.h\"\n+#include \"strbuf.h\"\n+\n+/*\n+ * These are defined in 'progress.c', but are not exposed in 'progress.h',\n+ * because they are exclusively for testing.\n+ */\n+extern int progress_testing;\n+extern uint64_t progress_test_ns;\n+void progress_test_force_update(void);\n+\n+int cmd__progress(int argc, const char **argv)\n+{\n+\tuint64_t total = 0;\n+\tconst char *title;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct progress *progress;\n+\n+\tconst char *usage[] = {\n+\t\t\"test-tool progress [--total=] \",\n+\t\tNULL\n+\t};\n+\tstruct option options[] = {\n+\t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n+\t\tOPT_END(),\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, usage, 0);\n+\tif (argc != 1)\n+\t\tdie(\"need a title for the progress output\");\n+\ttitle = argv[0];\n+\n+\tprogress_testing = 1;\n+\tprogress = start_progress(title, total);\n+\twhile (strbuf_getline(&line, stdin) != EOF) {\n+\t\tchar *end;\n+\n+\t\tif (skip_prefix(line.buf, \"progress \", (const char **) &end)) {\n+\t\t\tuint64_t item_count = strtoull(end, &end, 10);\n+\t\t\tif (*end != '\\0')\n+\t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t\t\tdisplay_progress(progress, item_count);\n+\t\t} else if (skip_prefix(line.buf, \"throughput \",\n+\t\t\t\t (const char **) &end)) {\n+\t\t\tuint64_t byte_count, test_ms;\n+\n+\t\t\tbyte_count = strtoull(end, &end, 10);\n+\t\t\tif (*end != ' ')\n+\t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t\t\ttest_ms = strtoull(end + 1, &end, 10);\n+\t\t\tif (*end != '\\0')\n+\t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t\t\tprogress_test_ns = test_ms * 1000 * 1000;\n+\t\t\tdisplay_throughput(progress, byte_count);\n+\t\t} else if (!strcmp(line.buf, \"update\"))\n+\t\t\tprogress_test_force_update();\n+\t\telse\n+\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t}\n+\tstop_progress(&progress);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex ce7e89028c..19ee26d931 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -42,6 +42,7 @@ static struct test_cmd cmds[] = {\n \t{ \"path-utils\", cmd__path_utils },\n \t{ \"pkt-line\", cmd__pkt_line },\n \t{ \"prio-queue\", cmd__prio_queue },\n+\t{ \"progress\", cmd__progress },\n \t{ \"reach\", cmd__reach },\n \t{ \"read-cache\", cmd__read_cache },\n \t{ \"read-midx\", cmd__read_midx },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f805bb39ae..c2aa56ef50 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -32,6 +32,7 @@ int cmd__parse_options(int argc, const char **argv);\n int cmd__path_utils(int argc, const char **argv);\n int cmd__pkt_line(int argc, const char **argv);\n int cmd__prio_queue(int argc, const char **argv);\n+int cmd__progress(int argc, const char **argv);\n int cmd__reach(int argc, const char **argv);\n int cmd__read_cache(int argc, const char **argv);\n int cmd__read_midx(int argc, const char **argv);\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nnew file mode 100755\nindex 0000000000..24ccbd8d3b\n--- /dev/null\n+++ b/t/t0500-progress-display.sh\n@@ -0,0 +1,286 @@\n+#!/bin/sh\n+\n+test_description='progress display'\n+\n+. ./test-lib.sh\n+\n+show_cr () {\n+\ttr '\\015' Q | sed -e \"s/Q/\\\\$LF/g\"\n+}\n+\n+test_expect_success 'simple progress display' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 1\n+\tWorking hard: 2\n+\tWorking hard: 5\n+\tWorking hard: 5, done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tupdate\n+\tprogress 1\n+\tupdate\n+\tprogress 2\n+\tprogress 3\n+\tprogress 4\n+\tupdate\n+\tprogress 5\n+\tEOF\n+\ttest-tool progress \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display with total' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 33% (1/3)\n+\tWorking hard: 66% (2/3)\n+\tWorking hard: 100% (3/3)\n+\tWorking hard: 100% (3/3), done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tprogress 1\n+\tprogress 2\n+\tprogress 3\n+\tEOF\n+\ttest-tool progress --total=3 \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display breaks long lines #1' '\n+\tsed -e \"s/Z$//\" >expect <<\\EOF &&\n+Working hard.......2.........3.........4.........5.........6: 0% (100/100000)\n+Working hard.......2.........3.........4.........5.........6: 1% (1000/100000)\n+Working hard.......2.........3.........4.........5.........6: Z\n+ 10% (10000/100000)\n+ 100% (100000/100000)\n+ 100% (100000/100000), done.\n+EOF\n+\n+\tcat >in <<-\\EOF &&\n+\tprogress 100\n+\tprogress 1000\n+\tprogress 10000\n+\tprogress 100000\n+\tEOF\n+\ttest-tool progress --total=100000 \\\n+\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n+\t\tstderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display breaks long lines #2' '\n+\t# Note: we dont need that many spaces after the title to cover up\n+\t# the last line before breaking the progress line.\n+\tsed -e \"s/Z$//\" >expect <<\\EOF &&\n+Working hard.......2.........3.........4.........5.........6: 0% (1/100000)\n+Working hard.......2.........3.........4.........5.........6: 0% (2/100000)\n+Working hard.......2.........3.........4.........5.........6: Z\n+ 10% (10000/100000)\n+ 100% (100000/100000)\n+ 100% (100000/100000), done.\n+EOF\n+\n+\tcat >in <<-\\EOF &&\n+\tupdate\n+\tprogress 1\n+\tupdate\n+\tprogress 2\n+\tprogress 10000\n+\tprogress 100000\n+\tEOF\n+\ttest-tool progress --total=100000 \\\n+\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n+\t\tstderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display breaks long lines #3 - even the first is too long' '\n+\t# Note: we dont actually need any spaces at the end of the title\n+\t# line, because there is no previous progress line to cover up.\n+\tsed -e \"s/Z$//\" >expect <<\\EOF &&\n+Working hard.......2.........3.........4.........5.........6: Z\n+ 25% (25000/100000)\n+ 50% (50000/100000)\n+ 75% (75000/100000)\n+ 100% (100000/100000)\n+ 100% (100000/100000), done.\n+EOF\n+\n+\tcat >in <<-\\EOF &&\n+\tprogress 25000\n+\tprogress 50000\n+\tprogress 75000\n+\tprogress 100000\n+\tEOF\n+\ttest-tool progress --total=100000 \\\n+\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n+\t\tstderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display breaks long lines #4 - title line matches terminal width' '\n+\tcat >expect <<\\EOF &&\n+Working hard.......2.........3.........4.........5.........6.........7.........:\n+ 25% (25000/100000)\n+ 50% (50000/100000)\n+ 75% (75000/100000)\n+ 100% (100000/100000)\n+ 100% (100000/100000), done.\n+EOF\n+\n+\tcat >in <<-\\EOF &&\n+\tprogress 25000\n+\tprogress 50000\n+\tprogress 75000\n+\tprogress 100000\n+\tEOF\n+\ttest-tool progress --total=100000 \\\n+\t\t\"Working hard.......2.........3.........4.........5.........6.........7.........\" \\\n+\t\tstderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+# Progress counter goes backwards, this should not happen in practice.\n+test_expect_success 'progress shortens - crazy caller' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 10% (100/1000)\n+\tWorking hard: 20% (200/1000)\n+\tWorking hard: 0% (1/1000) \n+\tWorking hard: 100% (1000/1000)\n+\tWorking hard: 100% (1000/1000), done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tprogress 100\n+\tprogress 200\n+\tprogress 1\n+\tprogress 1000\n+\tEOF\n+\ttest-tool progress --total=1000 \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display with throughput' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 10\n+\tWorking hard: 20, 200.00 KiB | 100.00 KiB/s\n+\tWorking hard: 30, 300.00 KiB | 100.00 KiB/s\n+\tWorking hard: 40, 400.00 KiB | 100.00 KiB/s\n+\tWorking hard: 40, 400.00 KiB | 100.00 KiB/s, done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tthroughput 102400 1000\n+\tupdate\n+\tprogress 10\n+\tthroughput 204800 2000\n+\tupdate\n+\tprogress 20\n+\tthroughput 307200 3000\n+\tupdate\n+\tprogress 30\n+\tthroughput 409600 4000\n+\tupdate\n+\tprogress 40\n+\tEOF\n+\ttest-tool progress \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'progress display with throughput and total' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 25% (10/40)\n+\tWorking hard: 50% (20/40), 200.00 KiB | 100.00 KiB/s\n+\tWorking hard: 75% (30/40), 300.00 KiB | 100.00 KiB/s\n+\tWorking hard: 100% (40/40), 400.00 KiB | 100.00 KiB/s\n+\tWorking hard: 100% (40/40), 400.00 KiB | 100.00 KiB/s, done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tthroughput 102400 1000\n+\tprogress 10\n+\tthroughput 204800 2000\n+\tprogress 20\n+\tthroughput 307200 3000\n+\tprogress 30\n+\tthroughput 409600 4000\n+\tprogress 40\n+\tEOF\n+\ttest-tool progress --total=40 \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'cover up after throughput shortens' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 1\n+\tWorking hard: 2, 800.00 KiB | 400.00 KiB/s\n+\tWorking hard: 3, 1.17 MiB | 400.00 KiB/s \n+\tWorking hard: 4, 1.56 MiB | 400.00 KiB/s\n+\tWorking hard: 4, 1.56 MiB | 400.00 KiB/s, done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tthroughput 409600 1000\n+\tupdate\n+\tprogress 1\n+\tthroughput 819200 2000\n+\tupdate\n+\tprogress 2\n+\tthroughput 1228800 3000\n+\tupdate\n+\tprogress 3\n+\tthroughput 1638400 4000\n+\tupdate\n+\tprogress 4\n+\tEOF\n+\ttest-tool progress \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_expect_success 'cover up after throughput shortens a lot' '\n+\tcat >expect <<-\\EOF &&\n+\tWorking hard: 1\n+\tWorking hard: 2, 1000.00 KiB | 1000.00 KiB/s\n+\tWorking hard: 3, 3.00 MiB | 1.50 MiB/s \n+\tWorking hard: 3, 3.00 MiB | 1024.00 KiB/s, done.\n+\tEOF\n+\n+\tcat >in <<-\\EOF &&\n+\tthroughput 1 1000\n+\tupdate\n+\tprogress 1\n+\tthroughput 1024000 2000\n+\tupdate\n+\tprogress 2\n+\tthroughput 3145728 3000\n+\tupdate\n+\tprogress 3\n+\tEOF\n+\ttest-tool progress \"Working hard\" stderr &&\n+\n+\tshow_cr out &&\n+\ttest_i18ncmp expect out\n+'\n+\n+test_done\n-- \n2.23.0.331.g4e51dcdf11\n\n"},{"id":"383279","messageId":"20191002154734.GC6116@sigill.intra.peff.net","threadId":"51708","inReplyTo":"20190916205412.8602-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/2] Revert \"progress: use term_clear_line()\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-02T15:47:35Z","receivedAt":"2019-10-02T15:47:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 16, 2019 at 10:54:10PM +0200, SZEDER Gábor wrote:\n\n> > Yes on that final bit. We could always fall back to (4) if the terminal\n> > information is not available, but given that the benefit is mostly in\n> > simplifying the code, I don't know if it's worth carrying around _two_\n> > solutions.\n> \n> Ok, so here is a patch to revert 5b12e3123b (progress: use\n> term_clear_line(), 2019-06-24) with proper explanation.\n> \n> As a bonus there is a new test script exercising the progress display\n> as well, in particular how it covers up the previous progress line, so\n> we may have a bit more confidence in it.\n\nThanks for doing this. It's especially nice to get test coverage for the\nprogress meters, which have traditionally been neglected.\n\nI'm a little late on my review, as it looks like this has already hit\nnext, but it looks pretty good to me.\n\nMy only complaint is that I think putting the new \"private\" bits of the\nprogress API into the header (with a comment) is a lesser evil than\nre-declaring them in test-progress.c (if only because the compiler could\ntell us if the two get out of sync). But I can live with it either way.\n\n-Peff\n"}]}