{"thread":{"id":"55870","subject":"[PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","startedAt":"2021-06-07T14:43:33Z","lastAt":"2021-06-20T12:54:37Z","messageCount":24,"participants":["Ævar Arnfjörð Bjarmason","Derrick Stolee","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"426610","messageId":"patch-2.2-042f598826-20210607T144206Z-avarab@gmail.com","threadId":"55870","inReplyTo":"cover-0.2-0000000000-20210607T144206Z-avarab@gmail.com","subject":"[PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T14:43:23Z","receivedAt":"2021-06-07T14:43:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a potential incorrect display of the number of items (off by one)\nand stalling of the progress bar in refresh_index().\n\nThe off-by-one error is minor, we should say we're processing the 1st\nitem, not the 0th. This along with the next change also allows us to\nremove the last display_progress() call outside the loop, as we'll\nalways have reached 100% now.\n\nLet's also move the display_progress() call to the very start of the\nloop refresh_index() loop. In the loop we first check whether e.g. we\nignore submodules and the entry we're processing is a submodule,\nwhether we ignore certain paths etc.. Thus we could have a\npathological case where we have a huge index consisting of such\nignored entries, and we'd stall on the progress bar.\n\nSee ae9af12287 (status: show progress bar if refreshing the index\ntakes too long, 2018-09-15) for the initial addition of this progress\nbar to refresh_index().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n read-cache.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 470f800855..8b0073a839 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1594,6 +1594,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \t\tint t2_did_lstat = 0;\n \t\tint t2_did_scan = 0;\n \n+\t\tdisplay_progress(progress, i + 1);\n+\n \t\tce = istate->cache[i];\n \t\tif (ignore_submodules && S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n@@ -1627,7 +1629,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \t\tt2_sum_scan += t2_did_scan;\n \t\tif (new_entry == ce)\n \t\t\tcontinue;\n-\t\tdisplay_progress(progress, i);\n \t\tif (!new_entry) {\n \t\t\tconst char *fmt;\n \n@@ -1662,7 +1663,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \ttrace2_data_intmax(\"index\", NULL, \"refresh/sum_lstat\", t2_sum_lstat);\n \ttrace2_data_intmax(\"index\", NULL, \"refresh/sum_scan\", t2_sum_scan);\n \ttrace2_region_leave(\"index\", \"refresh\", NULL);\n-\tdisplay_progress(progress, istate->cache_nr);\n \tstop_progress(&progress);\n \ttrace_performance_leave(\"refresh index\");\n \treturn has_errors;\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426611","messageId":"cover-0.2-0000000000-20210607T144206Z-avarab@gmail.com","threadId":"55870","inReplyTo":null,"subject":"[PATCH 0/2] trivial progress.c API usage fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T14:43:21Z","receivedAt":"2021-06-07T14:43:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A couple of small fixes to the usage of the progress.c API. I have\nsome subsequent changes queued up locally, but thought it would be\nbetter to submit this more incrementally, and to focus on smaller\ncleanups in the immediate post-release period.\n\nÆvar Arnfjörð Bjarmason (2):\n read-cache.c: don't guard calls to progress.c API\n read-cache: fix incorrect count and progress bar stalling\n\n read-cache.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426612","messageId":"patch-1.2-1680109655-20210607T144206Z-avarab@gmail.com","threadId":"55870","inReplyTo":"cover-0.2-0000000000-20210607T144206Z-avarab@gmail.com","subject":"[PATCH 1/2] read-cache.c: don't guard calls to progress.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T14:43:22Z","receivedAt":"2021-06-07T14:44:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Don't guard the calls to the progress.c API with \"if (progress)\". The\nAPI itself will check this. This doesn't change any behavior, but\nmakes this code consistent with the rest of the codebase.\n\nSee ae9af12287b (status: show progress bar if refreshing the index\ntakes too long, 2018-09-15) for the commit that added the pattern\nwe're changing here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n read-cache.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 1b3c2eb408..470f800855 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1627,8 +1627,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \t\tt2_sum_scan += t2_did_scan;\n \t\tif (new_entry == ce)\n \t\t\tcontinue;\n-\t\tif (progress)\n-\t\t\tdisplay_progress(progress, i);\n+\t\tdisplay_progress(progress, i);\n \t\tif (!new_entry) {\n \t\t\tconst char *fmt;\n \n@@ -1663,10 +1662,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \ttrace2_data_intmax(\"index\", NULL, \"refresh/sum_lstat\", t2_sum_lstat);\n \ttrace2_data_intmax(\"index\", NULL, \"refresh/sum_scan\", t2_sum_scan);\n \ttrace2_region_leave(\"index\", \"refresh\", NULL);\n-\tif (progress) {\n-\t\tdisplay_progress(progress, istate->cache_nr);\n-\t\tstop_progress(&progress);\n-\t}\n+\tdisplay_progress(progress, istate->cache_nr);\n+\tstop_progress(&progress);\n \ttrace_performance_leave(\"refresh index\");\n \treturn has_errors;\n }\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426615","messageId":"51e00403-dcab-c57f-b77a-de01a61a60eb@gmail.com","threadId":"55870","inReplyTo":"patch-1.2-1680109655-20210607T144206Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-07T15:28:03Z","receivedAt":"2021-06-07T15:28:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n> Don't guard the calls to the progress.c API with \"if (progress)\". The\n> API itself will check this. This doesn't change any behavior, but\n> makes this code consistent with the rest of the codebase.\n\nSince stop_progress() closes a trace2 region, this actually\ndoes make a change in behavior, I think. In a good way.\n\nThanks,\n-Stolee\n"},{"id":"426617","messageId":"8f336b1b-6cb7-8277-79d5-0f331159b00c@gmail.com","threadId":"55870","inReplyTo":"patch-2.2-042f598826-20210607T144206Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-07T15:31:00Z","receivedAt":"2021-06-07T15:31:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n> Fix a potential incorrect display of the number of items (off by one)\n> and stalling of the progress bar in refresh_index().\n> \n> The off-by-one error is minor, we should say we're processing the 1st\n> item, not the 0th. This along with the next change also allows us to\n> remove the last display_progress() call outside the loop, as we'll\n> always have reached 100% now.\n\nThis \"pre-announce the progress\" seems correct and is unlikely\nto have a user sitting at \"100%\" while the loop is actually doing\nwork on that last cache entry.\n\nThanks,\n-Stolee\n"},{"id":"426621","messageId":"87mts14qgz.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"51e00403-dcab-c57f-b77a-de01a61a60eb@gmail.com","subject":"Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T15:52:13Z","receivedAt":"2021-06-07T15:59:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 07 2021, Derrick Stolee wrote:\n\n> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n>> Don't guard the calls to the progress.c API with \"if (progress)\". The\n>> API itself will check this. This doesn't change any behavior, but\n>> makes this code consistent with the rest of the codebase.\n>\n> Since stop_progress() closes a trace2 region, this actually\n> does make a change in behavior, I think. In a good way.\n\nI don't see the behavior change.\n\nYes start_delayed_progress() will call start_progress_delay() which\nmallocs and enters the trace2 region, and then if you don't call\nstop_progress() at all you won't leave it.\n\nBut in read-cache.c both before & after my change we only malloc & only\nenter the region if we're actually displaying the progress, there's an\nisatty() guard on it.\n\nOnce we start that progress bar we will leave the trace2 region, via\nstop_progress(), but note that stop_progress() will exit early if the\npointer you dereference is NULL and not do that, and since we'll have\n\"*progress = NULL\" in the case of not wanting the progress bar we won't\nleave the region we didn't enter in the first place.\n\nThe change here is just to remove the needless nano-optimization of\nguarding the calls with a NULL check of \"progress\", which the API itself\ndoes, no?\n"},{"id":"426623","messageId":"87k0n54qb6.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"8f336b1b-6cb7-8277-79d5-0f331159b00c@gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T15:58:23Z","receivedAt":"2021-06-07T16:02:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 07 2021, Derrick Stolee wrote:\n\n> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n>> Fix a potential incorrect display of the number of items (off by one)\n>> and stalling of the progress bar in refresh_index().\n>> \n>> The off-by-one error is minor, we should say we're processing the 1st\n>> item, not the 0th. This along with the next change also allows us to\n>> remove the last display_progress() call outside the loop, as we'll\n>> always have reached 100% now.\n>\n> This \"pre-announce the progress\" seems correct and is unlikely\n> to have a user sitting at \"100%\" while the loop is actually doing\n> work on that last cache entry.\n\nI guess pre-announce v.s. post-announce is a matter of some philosophy,\nfor O(n) when can we be said to be doing work on n[0]? We entered the\nfor-loop and are doing work on that istate->cache[i] item, so I'd like\nto think of it more as post-announce :)\n\nIn any case, I'm changing this to the established pattern we use in most\nother places in the codebase, this one was an odd one out.\n\nThanks for the review of this.\n"},{"id":"426624","messageId":"6cb306a5-4c01-5241-4430-0de21a7953f9@gmail.com","threadId":"55870","inReplyTo":"87mts14qgz.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] read-cache.c: don't guard calls to progress.c API","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-06-07T16:11:12Z","receivedAt":"2021-06-07T16:11:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/7/2021 11:52 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jun 07 2021, Derrick Stolee wrote:\n> \n>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> Don't guard the calls to the progress.c API with \"if (progress)\". The\n>>> API itself will check this. This doesn't change any behavior, but\n>>> makes this code consistent with the rest of the codebase.\n>>\n>> Since stop_progress() closes a trace2 region, this actually\n>> does make a change in behavior, I think. In a good way.\n> \n> I don't see the behavior change.\n> \n> Yes start_delayed_progress() will call start_progress_delay() which\n> mallocs and enters the trace2 region, and then if you don't call\n> stop_progress() at all you won't leave it.\n> \n> But in read-cache.c both before & after my change we only malloc & only\n> enter the region if we're actually displaying the progress, there's an\n> isatty() guard on it.\n\nThat's right. I misremembered where this trace2 stuff was.\n\nThe idea we didn't pursue was to create the progress struct\nunconditionally, and just leave it as \"quiet\" based on an\ninput parameter. That would keep the trace2 regions consistent.\n\nBut I'm wrong and there is no behavior change here.\n\nThanks,\n-Stolee\n"},{"id":"426654","messageId":"eaf2b6b0-4202-d5ea-87a2-b828fdbc60a1@web.de","threadId":"55870","inReplyTo":"87k0n54qb6.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-07T19:20:50Z","receivedAt":"2021-06-07T19:20:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Jun 07 2021, Derrick Stolee wrote:\n>\n>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> Fix a potential incorrect display of the number of items (off by one)\n>>> and stalling of the progress bar in refresh_index().\n>>>\n>>> The off-by-one error is minor, we should say we're processing the 1st\n>>> item, not the 0th. This along with the next change also allows us to\n>>> remove the last display_progress() call outside the loop, as we'll\n>>> always have reached 100% now.\n>>\n>> This \"pre-announce the progress\" seems correct and is unlikely\n>> to have a user sitting at \"100%\" while the loop is actually doing\n>> work on that last cache entry.\n>\n> I guess pre-announce v.s. post-announce is a matter of some philosophy,\n> for O(n) when can we be said to be doing work on n[0]? We entered the\n> for-loop and are doing work on that istate->cache[i] item, so I'd like\n> to think of it more as post-announce :)\n\nSay you have a single item to process and it takes a minute. The\noriginal code shows 0% for a minute, then 100% at the end. With your\nchange you'd get 100% for a minute. Both would be annoying, but the\nlatter would have me raging. \"If you're done\", I'd yell at the uncaring\nmachine, \"what are you still doing!?\".\n\nShowing only the completed items makes sense. That the next one is\nbeing processed is self-understood. Once all of them are done, 100% is\nshown and the progress line is finished.\n\nSo I think this pattern works:\n\n\tfor (i = 0; i < nr; i++) {\n\t\tdisplay_progress(p, i);\n\t\t/* work work work */\n\t}\n\tdisplay_progress(p, nr);\n\nAlternatively, if the work part doesn't contain continue statements:\n\n\tfor (i = 0; i < nr; i++) {\n\t\t/* work work work */\n\t\tdisplay_progress(p, i + 1);\n\t}\n\n> In any case, I'm changing this to the established pattern we use in most\n> other places in the codebase, this one was an odd one out.\n\nConsistency is a good thing, but perhaps some of these other places\nshould be changed. It doesn't matter much because most items git deals\nwith are processed quickly, so an off-by-one error should barely be\nnoticeable, but still it would be nice to get it right. It's hard to\ntest, though.\n\nRené\n"},{"id":"426657","messageId":"8735tt4fhx.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"eaf2b6b0-4202-d5ea-87a2-b828fdbc60a1@web.de","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-07T19:49:32Z","receivedAt":"2021-06-07T19:54:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 07 2021, René Scharfe wrote:\n\n> Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Mon, Jun 07 2021, Derrick Stolee wrote:\n>>\n>>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:\n>>>> Fix a potential incorrect display of the number of items (off by one)\n>>>> and stalling of the progress bar in refresh_index().\n>>>>\n>>>> The off-by-one error is minor, we should say we're processing the 1st\n>>>> item, not the 0th. This along with the next change also allows us to\n>>>> remove the last display_progress() call outside the loop, as we'll\n>>>> always have reached 100% now.\n>>>\n>>> This \"pre-announce the progress\" seems correct and is unlikely\n>>> to have a user sitting at \"100%\" while the loop is actually doing\n>>> work on that last cache entry.\n>>\n>> I guess pre-announce v.s. post-announce is a matter of some philosophy,\n>> for O(n) when can we be said to be doing work on n[0]? We entered the\n>> for-loop and are doing work on that istate->cache[i] item, so I'd like\n>> to think of it more as post-announce :)\n>\n> Say you have a single item to process and it takes a minute. The\n> original code shows 0% for a minute, then 100% at the end. With your\n> change you'd get 100% for a minute. Both would be annoying, but the\n> latter would have me raging. \"If you're done\", I'd yell at the uncaring\n> machine, \"what are you still doing!?\".\n\nPerhaps if we said \"100% and Reticulating splines[...]\" :)\n\n> Showing only the completed items makes sense. That the next one is\n> being processed is self-understood. Once all of them are done, 100% is\n> shown and the progress line is finished.\n>\n> So I think this pattern works:\n>\n> \tfor (i = 0; i < nr; i++) {\n> \t\tdisplay_progress(p, i);\n> \t\t/* work work work */\n> \t}\n> \tdisplay_progress(p, nr);\n>\n> Alternatively, if the work part doesn't contain continue statements:\n>\n> \tfor (i = 0; i < nr; i++) {\n> \t\t/* work work work */\n> \t\tdisplay_progress(p, i + 1);\n> \t}\n\nBut yes, I agree with the issue in theory, but I think in practice we\ndon't need to worry about these 100% cases.\n\nWe usually only display this anyway with a really big O(n), or (if we\ncorrectly use the API) one where each item isn't that expensive, we just\ndo a lot of work in the aggregate.\n\nSo having a display_progress() at the top of the for-loop with \"i + 1\"\navoids needing two of them, or worrying about \"continue\" statements etc,\nor (as in this case) where the data we're processing can be 10k items\nwith the first 8k being items we skip, so we'd be seen to hang, or\n\"jump\" from 10% to 50%, then smoothly update 50%..60%, and jump again\netc.\n"},{"id":"426674","messageId":"xmqqczsxtf8g.fsf@gitster.g","threadId":"55870","inReplyTo":"8735tt4fhx.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-07T23:41:19Z","receivedAt":"2021-06-07T23:41:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> So I think this pattern works:\n>>\n>> \tfor (i = 0; i < nr; i++) {\n>> \t\tdisplay_progress(p, i);\n>> \t\t/* work work work */\n>> \t}\n>> \tdisplay_progress(p, nr);\n>>\n>> Alternatively, if the work part doesn't contain continue statements:\n>>\n>> \tfor (i = 0; i < nr; i++) {\n>> \t\t/* work work work */\n>> \t\tdisplay_progress(p, i + 1);\n>> \t}\n>\n> But yes, I agree with the issue in theory, but I think in practice we\n> don't need to worry about these 100% cases.\n\nHmph, but in practice we do need to worry, don't we? Otherwise you\nwouldn't have started this thread and René wouldn't have responded.\n\nI agree with the issue and I think we should count what we have\nfinished.\n\n"},{"id":"426756","messageId":"87wnr4394y.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"xmqqczsxtf8g.fsf@gitster.g","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T10:58:42Z","receivedAt":"2021-06-08T11:10:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 08 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> So I think this pattern works:\n>>>\n>>> \tfor (i = 0; i < nr; i++) {\n>>> \t\tdisplay_progress(p, i);\n>>> \t\t/* work work work */\n>>> \t}\n>>> \tdisplay_progress(p, nr);\n>>>\n>>> Alternatively, if the work part doesn't contain continue statements:\n>>>\n>>> \tfor (i = 0; i < nr; i++) {\n>>> \t\t/* work work work */\n>>> \t\tdisplay_progress(p, i + 1);\n>>> \t}\n>>\n>> But yes, I agree with the issue in theory, but I think in practice we\n>> don't need to worry about these 100% cases.\n>\n> Hmph, but in practice we do need to worry, don't we? Otherwise you\n> wouldn't have started this thread and René wouldn't have responded.\n\nI started this thread because of:\n\n\tfor (i = 0; i < large_number; i++) {\n\t\tif (maybe_branch_here())\n\t\t\tcontinue;\n\t\t/* work work work */\n\t\tdisplay_progress(p, i);\n\t}\n\tdisplay_progress(p, large_number);\n\nMainly because it's a special snowflake in how the process.c API is\nused, with most other callsites doing:\n\n\tfor (i = 0; i < large_number; i++) {\n\t\tdisplay_progress(p, i + 1);\n\t\t/* work work work */\n\t}\n\nWhich yes, as René points out *could* hang on 100%, but I think in\npractice isn't an issue here, and changing the code per my patch here\nsolves the practical issue with us always taking the maybe_branch_here()\n(or enough that the progress bar hangs).\n\n> I agree with the issue and I think we should count what we have\n> finished.\n\nFair enough, but in the meantime can we take this patch? I think fixing\nthat (IMO in practice hypothetical issue) is much easier when we\nconsistently use that \"i + 1\" pattern above (which we mostly do\nalready). We can just search-replace \"++i\" to \"i++\" and \"i + 1\" to \"i\"\nand have stop_progress() be what bumps it to 100%.\n\nI have some unsent patches queued on top of this which has some general\nfixes to edge cases in the progress.c API making that & more easier...q\n"},{"id":"426789","messageId":"74183ce6-e17f-1b11-1ceb-7a8d873bc1c7@web.de","threadId":"55870","inReplyTo":"87wnr4394y.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-08T16:14:42Z","receivedAt":"2021-06-08T16:14:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Jun 08 2021, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>>> So I think this pattern works:\n>>>>\n>>>> \tfor (i = 0; i < nr; i++) {\n>>>> \t\tdisplay_progress(p, i);\n>>>> \t\t/* work work work */\n>>>> \t}\n>>>> \tdisplay_progress(p, nr);\n>>>>\n>>>> Alternatively, if the work part doesn't contain continue statements:\n>>>>\n>>>> \tfor (i = 0; i < nr; i++) {\n>>>> \t\t/* work work work */\n>>>> \t\tdisplay_progress(p, i + 1);\n>>>> \t}\n>>>\n>>> But yes, I agree with the issue in theory, but I think in practice we\n>>> don't need to worry about these 100% cases.\n>>\n>> Hmph, but in practice we do need to worry, don't we? Otherwise you\n>> wouldn't have started this thread and René wouldn't have responded.\n>\n> I started this thread because of:\n>\n> \tfor (i = 0; i < large_number; i++) {\n> \t\tif (maybe_branch_here())\n> \t\t\tcontinue;\n> \t\t/* work work work */\n> \t\tdisplay_progress(p, i);\n> \t}\n> \tdisplay_progress(p, large_number);\n>\n> Mainly because it's a special snowflake in how the process.c API is\n> used, with most other callsites doing:\n>\n> \tfor (i = 0; i < large_number; i++) {\n> \t\tdisplay_progress(p, i + 1);\n> \t\t/* work work work */\n> \t}\n\nMoving the first call to the top of the loop makes sense. It ensures\nall kind of progress -- skipping and actual work -- is reported without\nundue delay.\n\nAdding one would introduce an off-by-one error. Removing the call after\nthe loop would leave the progress report at one short of 100%. I don't\nsee any benefits of these additional changes, only downsides.\n\nIf other callsites have an off-by-one error and we care enough then we\nshould fix them. Copying their style and spreading the error doesn't\nmake sense -- correctness trumps consistency.\n\n> Fair enough, but in the meantime can we take this patch? I think fixing\n> that (IMO in practice hypothetical issue) is much easier when we\n> consistently use that \"i + 1\" pattern above (which we mostly do\n> already). We can just search-replace \"++i\" to \"i++\" and \"i + 1\" to \"i\"\n> and have stop_progress() be what bumps it to 100%.\n\nThis assumes the off-by-one error is consistent. Even if that is the\ncase you could apply your mechanical fix and leave out read-cache.\nThis would happen automatically because when keeping i there is no ++i\nto be found.\n\nstop_progress() doesn't set the progress to 100%:\n\n $ (echo progress 0; echo update) |\n ./t/helper/test-tool progress --total 1 test\n test: 0% (0/1), done.\n\nI wonder (only in a semi-curious way, though) if we can detect\noff-by-one errors by adding an assertion to display_progress() that\nrequires the first update to have the value 0, and in stop_progress()\none that requires the previous display_progress() call to have a value\nequal to the total number of work items. Not sure it'd be worth the\nhassle..\n\nRené\n"},{"id":"426822","messageId":"87lf7k2bem.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"74183ce6-e17f-1b11-1ceb-7a8d873bc1c7@web.de","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T22:12:58Z","receivedAt":"2021-06-08T23:18:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 08 2021, René Scharfe wrote:\n\n> Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Tue, Jun 08 2021, Junio C Hamano wrote:\n>>\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>>>> So I think this pattern works:\n>>>>>\n>>>>> \tfor (i = 0; i < nr; i++) {\n>>>>> \t\tdisplay_progress(p, i);\n>>>>> \t\t/* work work work */\n>>>>> \t}\n>>>>> \tdisplay_progress(p, nr);\n>>>>>\n>>>>> Alternatively, if the work part doesn't contain continue statements:\n>>>>>\n>>>>> \tfor (i = 0; i < nr; i++) {\n>>>>> \t\t/* work work work */\n>>>>> \t\tdisplay_progress(p, i + 1);\n>>>>> \t}\n>>>>\n>>>> But yes, I agree with the issue in theory, but I think in practice we\n>>>> don't need to worry about these 100% cases.\n>>>\n>>> Hmph, but in practice we do need to worry, don't we? Otherwise you\n>>> wouldn't have started this thread and René wouldn't have responded.\n>>\n>> I started this thread because of:\n>>\n>> \tfor (i = 0; i < large_number; i++) {\n>> \t\tif (maybe_branch_here())\n>> \t\t\tcontinue;\n>> \t\t/* work work work */\n>> \t\tdisplay_progress(p, i);\n>> \t}\n>> \tdisplay_progress(p, large_number);\n>>\n>> Mainly because it's a special snowflake in how the process.c API is\n>> used, with most other callsites doing:\n>>\n>> \tfor (i = 0; i < large_number; i++) {\n>> \t\tdisplay_progress(p, i + 1);\n>> \t\t/* work work work */\n>> \t}\n>\n> Moving the first call to the top of the loop makes sense. It ensures\n> all kind of progress -- skipping and actual work -- is reported without\n> undue delay.\n>\n> Adding one would introduce an off-by-one error. Removing the call after\n> the loop would leave the progress report at one short of 100%. I don't\n> see any benefits of these additional changes, only downsides.\n>\n> If other callsites have an off-by-one error and we care enough then we\n> should fix them. Copying their style and spreading the error doesn't\n> make sense -- correctness trumps consistency.\n>\n>> Fair enough, but in the meantime can we take this patch? I think fixing\n>> that (IMO in practice hypothetical issue) is much easier when we\n>> consistently use that \"i + 1\" pattern above (which we mostly do\n>> already). We can just search-replace \"++i\" to \"i++\" and \"i + 1\" to \"i\"\n>> and have stop_progress() be what bumps it to 100%.\n>\n> This assumes the off-by-one error is consistent. Even if that is the\n> case you could apply your mechanical fix and leave out read-cache.\n> This would happen automatically because when keeping i there is no ++i\n> to be found.\n>\n> stop_progress() doesn't set the progress to 100%:\n>\n> $ (echo progress 0; echo update) |\n> ./t/helper/test-tool progress --total 1 test\n> test: 0% (0/1), done.\n>\n\nI was too quick with that \"But yes, I agree with the issue in theory\".\n\nHaving thought about it some more I think you're wrong, it doesn't make\nsense to use the API in the way you're suggesting.\n\nThere's an off-by-one error, but it's in the pattern you're\nsuggesting. Calling \"i + 1\" on the \"first item\" doesn't just make for\nless verbose code, it's also more correct.\n\nWhy? Because:\n\n /* 1. Setup progress */\n large_number = 3;\n progress = start_progress(title, total);\n\n /* 2. Before we \"actually\" do anything */\n for (i = 0; i < 3; i++) {\n /* 3. Now doing O(large_number) work */\n display_progress(progress, i + 1);\n }\n\n /* 4. Done */\n stop_progress(&progress);\n\nAs you'll see if you insert a sleep or whatever at \"2\" we'll signal and\ndisplay the progress bar even if we haven't called display_progress()\nyet.\n\nThus calling display_progress with n=0 makes \"we are doing the first\nitem\" indistinguishable from \"we haven't gotten to the first item\nyet\". When you're in the loop the first item should be n=1.\n\nThis isn't just more correct with this API. I think it also makes sense\nnot to leak the zero indexed C abstraction to human output. As in:\n\n I sat down at the table with my three apples (stages 1..2 above),\n and now I'm eating my first apple (stage 3), I'm not starting to eat\n my zeroeth apple. At some point I'm done eating all 3 apples (stage\n 4).\n\nI think this gets to the crux of the issue for you, per your upthread:\n\n [...]With your change you'd get 100% for a minute. Both would be\n annoying, but the latter would have me raging. \"If you're done\",\n I'd yell at the uncaring machine, \"what are you still doing!?\".\n\nIf we said we're done and we're not then yes, that would be a bug.\n\nBut that's not what we said. Distinguishing \"I'm on 3/3\" from \"I'm done\"\nis exactly what the progress.c output does:\n\n $ perl -wE 'for (0..3) { say \"update\"; say \"progress $_\" }' |\n ./helper/test-tool progress --total=3 Apples 2>&1 |\n cat -v | perl -pe 's/\\^M\\K/\\n/g'\n Apples: 0% (0/3)^M\n Apples: 33% (1/3)^M\n Apples: 66% (2/3)^M\n Apples: 100% (3/3)^M\n Apples: 100% (3/3), done.\n\nIt's not immediately obvious from that output, but the last line ending\nin \", done\" isn't added by any display_progress() call, but by calling\nstop_progress(). That's when we're done.\n\nArguably this is too subtle and we should say \", but not done yet!\" or\nsomething on that penultimate line. But that's bikeshedding a display\nissue; The API use in my patch is correct.\n\nAs I noted upthread that's a \"matter of some philosophy\". I guess you\nmight report your progress as being at 2/3 apples, even though you're\none bite away from finishing the third apple. Personally I'd say I'm on\nthe third apple, I'm just not done with it.\n\nBut the philosophizing of whether your \"progress\" is the zeroeth apple\nof the day as you're chewing on it aside, I think it's clear that in the\ncontext of the progress.c API using n=0 for the first apple would be a\nbug.\n\nYou'll conflate \"setup\" with \"work\" per the above, and you'll say you're\non the second apple even if you're a bite away from finishing it. We\ndon't conflate that \"100%\" state with being \"done\", so I don't see why\nwe'd do that.\n\n> I wonder (only in a semi-curious way, though) if we can detect\n> off-by-one errors by adding an assertion to display_progress() that\n> requires the first update to have the value 0, and in stop_progress()\n> one that requires the previous display_progress() call to have a value\n> equal to the total number of work items. Not sure it'd be worth the\n> hassle..\n\nThat's intentional. We started eating 3 apples, got to one, but now our\nhouse is on fire and we're eating no more apples today, even if we\nplanned to eat 3 when we sat down.\n\nThe progress bar reflects this unexpected but recoverable state:\n \n $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n ./helper/test-tool progress --total=3 Apples 2>&1 |\n cat -v | perl -pe 's/\\^M\\K/\\n/g'\n Apples: 0% (0/3)^M\n Apples: 33% (1/3)^M\n Apples: 33% (1/3), done.\n\nWe're at 1/3, but we're done. No more apples.\n\nThis isn't just some hypothetical, e.g. consider neeing to unlink() or\nremove files/directories one at a time in a directory and getting the\nestimated number from st_nlink (yeah yeah, unportable, but it was the\nfirst thing I thought of).\n\nWe might think we're processing 10 entries, but another other processes\nmight make our progress bar end at more or less than the 100% we\nexpected. That's OK, not something we should invoke BUG() about.\n\nSimilarly, the n=0 being distinguishable from the first\ndisplay_progress() is actually useful in practice. It's something I've\nseen git.git emit (not recently, I patched the relevant code to emit\nmore granular progress).\n\nIt's useful to know that we're stalling on the setup code before the\nfor-loop, not on the first item.\n"},{"id":"426946","messageId":"xmqq4ke6ffqv.fsf@gitster.g","threadId":"55870","inReplyTo":"87lf7k2bem.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-10T05:30:48Z","receivedAt":"2021-06-10T05:30:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I was too quick with that \"But yes, I agree with the issue in theory\".\n>\n> Having thought about it some more I think you're wrong, it doesn't make\n> sense to use the API in the way you're suggesting.\n\nSorry, I've read the message I am responding to a few times, but I\ncouldn't tell what you are arguing against in the suggestion given\nearlier by René, which offered two possibile ways to consistently\ncall display() with the number of things that we have DONE\nprocessing (not the number of things that we are about to touch) [*1*].\n\n> Why? Because:\n>\n> /* 1. Setup progress */\n> large_number = 3;\n> progress = start_progress(title, total);\n>\n> /* 2. Before we \"actually\" do anything */\n> for (i = 0; i < 3; i++) {\n> /* 3. Now doing O(large_number) work */\n> display_progress(progress, i + 1);\n> }\n>\n> /* 4. Done */\n> stop_progress(&progress);\n>\n> As you'll see if you insert a sleep or whatever at \"2\" we'll signal and\n> display the progress bar even if we haven't called display_progress()\n> yet.\n\nThat is, the signal start_progress_delay() kicking in? But\nprogress_test_force_update() is a curiosity that appears only in\ntest-tool and in real life programs, you'd have to call display\nyourself, so a long delay at \"3\" will appear as a long silence,\nI would think.\n\nIn any case, if we somehow managed to cause display_progress() to\nhappen somewhere near \"2\", we haven't finished even a single item\nyet at that point, so \"we are giving progress bar, and we have\nfinished 0%\" that is an output from such a call would make sense.\nAs we finish one item, we'd increment it to 1 (that happens after we\nspent time at \"3\" during the first iteration, and display_progress()\nis called with \"i+1\").\n\n> Thus calling display_progress with n=0 makes \"we are doing the first\n> item\" indistinguishable from \"we haven't gotten to the first item\n> yet\".\n\nI am guessing that you are talking about \"what value should we call\ndisplay() if '2' takes a long time and we want to give progress\nearly even before we enter the loop?\"\n\nI do not view such a call as \"we haven't gotten to\" or \"we are\ndoing\"; an extra call we would make with display(n=0) around \"2\" is\n\"how much have we finished? we have completed none\".\n\n> When you're in the loop the first item should be n=1.\n\nDoesn't that depend on where in the loop you do the work?\n\nIf you spend cycles and finish processing the first item _before_\ncalling display_progress(), you are reporting how many you have\nfinished, so at the end of the first iteration of the loop, you'd\ncall with n=1.\n\nIf on the other hand you somehow report at the beginning (perhaps\nbecause otherwise you'd have tough time structuring the code when\nthe loop body has a continue etc.) before finishing the processing\nfor the first item, you'd call with n=0 (and make sure before\ncalling stop_progress(), you'd call another display() after exiting\nthe loop).\n\nAnd I think that is consistent with what Réne suggested earlier, so\nI am not sure where your \"I am right you are wrong\" is coming from.\nTo me, you two seem to be agreeing.\n\n\n[Footnote]\n\n*1* \n\n> Showing only the completed items makes sense. That the next one is\n> being processed is self-understood. Once all of them are done, 100% is\n> shown and the progress line is finished.\n> \n> So I think this pattern works:\n> \n> \tfor (i = 0; i < nr; i++) {\n> \t\tdisplay_progress(p, i);\n> \t\t/* work work work */\n> \t}\n> \tdisplay_progress(p, nr);\n> \n> Alternatively, if the work part doesn't contain continue statements:\n> \n> \tfor (i = 0; i < nr; i++) {\n> \t\t/* work work work */\n> \t\tdisplay_progress(p, i + 1);\n> \t}\n\n\n"},{"id":"427024","messageId":"f7a54b2d-42c6-1308-f1b0-2f67b2dcadc7@web.de","threadId":"55870","inReplyTo":"xmqq4ke6ffqv.fsf@gitster.g","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-10T15:14:50Z","receivedAt":"2021-06-10T15:15:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.06.21 um 07:30 schrieb Junio C Hamano:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I was too quick with that \"But yes, I agree with the issue in theory\".\n>>\n>> Having thought about it some more I think you're wrong, it doesn't make\n>> sense to use the API in the way you're suggesting.\n>\n> Sorry, I've read the message I am responding to a few times, but I\n> couldn't tell what you are arguing against in the suggestion given\n> earlier by René, which offered two possibile ways to consistently\n> call display() with the number of things that we have DONE\n> processing (not the number of things that we are about to touch) [*1*].\n\nSame here.\n\nPerhaps a demo helps. The patch at the bottom adds an echo command to\nthe test helper. This way we can intersperse the progress lines with\nindications when items are processed.\n\nSo here's the pattern for calling display_progress at the top of the\nloop and again with the number of items after the loop:\n\n $ (\n for i in 0 1 2\n do\n echo progress $i\n echo update\n echo echo WORK\n done\n echo progress 3\n ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\\r' '\\n'\n\n test: 0% (0/3)\n WORK\n test: 33% (1/3)\n WORK\n test: 66% (2/3)\n WORK\n test: 100% (3/3)\n test: 100% (3/3), done.\n\nThe progress lines reflect the number of finished items at all times.\n\nHere's the pattern for display_progress at the bottom of the loop:\n\n\n $ (\n for i in 0 1 2\n do\n echo echo WORK\n echo progress $(( $i + 1 ))\n echo update\n done\n ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\\r' '\\n'\n\n WORK\n test: 33% (1/3)\n WORK\n test: 66% (2/3)\n WORK\n test: 100% (3/3)\n test: 100% (3/3), done.\n\nSame here, the progress line shows the correct number of finished items.\n\nHere's the pattern suggested in your patch:\n\n $ (\n for i in 0 1 2\n do\n echo progress $(( $i + 1 ))\n echo update\n echo echo WORK\n done\n ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\\r' '\\n'\n\n test: 33% (1/3)\n WORK\n test: 66% (2/3)\n WORK\n test: 100% (3/3)\n WORK\n test: 100% (3/3), done.\n\nIt reports one item too many in the intermediate progress lines and\nis correct only at the very end.\n\nRené\n\n\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 5d05cbe789..b6589f3878 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -65,6 +65,8 @@ int cmd__progress(int argc, const char **argv)\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 if (skip_prefix(line.buf, \"echo \", (const char **) &end))\n+\t\t\tfprintf(stderr, \"%s\\n\", end);\n \t\telse\n \t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n \t}\n"},{"id":"427025","messageId":"f5e181fc-af94-bccf-051f-a58a04fe1ffc@web.de","threadId":"55870","inReplyTo":"87lf7k2bem.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-10T15:14:52Z","receivedAt":"2021-06-10T15:15:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Jun 08 2021, René Scharfe wrote:\n>\n>> I wonder (only in a semi-curious way, though) if we can detect\n>> off-by-one errors by adding an assertion to display_progress() that\n>> requires the first update to have the value 0, and in stop_progress()\n>> one that requires the previous display_progress() call to have a value\n>> equal to the total number of work items. Not sure it'd be worth the\n>> hassle..\n>\n> That's intentional. We started eating 3 apples, got to one, but now our\n> house is on fire and we're eating no more apples today, even if we\n> planned to eat 3 when we sat down.\n>\n> The progress bar reflects this unexpected but recoverable state:\n>\n> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n> ./helper/test-tool progress --total=3 Apples 2>&1 |\n> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n> Apples: 0% (0/3)^M\n> Apples: 33% (1/3)^M\n> Apples: 33% (1/3), done.\n>\n> We're at 1/3, but we're done. No more apples.\n>\n> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n> remove files/directories one at a time in a directory and getting the\n> estimated number from st_nlink (yeah yeah, unportable, but it was the\n> first thing I thought of).\n>\n> We might think we're processing 10 entries, but another other processes\n> might make our progress bar end at more or less than the 100% we\n> expected. That's OK, not something we should invoke BUG() about.\n\nIt doesn't have to be a BUG; a warning would suffice. And I hope not\nfinishing the expected number of items due to a catastrophic event is\nrare enough that an additional warning wouldn't cause too much pain.\n\nLoops that *regularly* end early are not a good fit for progress\npercentages, I think.\n\n> Similarly, the n=0 being distinguishable from the first\n> display_progress() is actually useful in practice. It's something I've\n> seen git.git emit (not recently, I patched the relevant code to emit\n> more granular progress).\n>\n> It's useful to know that we're stalling on the setup code before the\n> for-loop, not on the first item.\n\nHmm, preparations that take a noticeable time might deserve their own\nprogress line.\n\nAnyway, if no guard rails can be built then we have to rely on our math\nskills alone. Off-by-one errors may look silly, but are no joke -- they\nare surprisingly easy to make.\n\nRené\n"},{"id":"427326","messageId":"87zgvszo8i.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"f5e181fc-af94-bccf-051f-a58a04fe1ffc@web.de","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-14T11:07:50Z","receivedAt":"2021-06-14T11:24:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 10 2021, René Scharfe wrote:\n\n> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Tue, Jun 08 2021, René Scharfe wrote:\n>>\n>>> I wonder (only in a semi-curious way, though) if we can detect\n>>> off-by-one errors by adding an assertion to display_progress() that\n>>> requires the first update to have the value 0, and in stop_progress()\n>>> one that requires the previous display_progress() call to have a value\n>>> equal to the total number of work items. Not sure it'd be worth the\n>>> hassle..\n>>\n>> That's intentional. We started eating 3 apples, got to one, but now our\n>> house is on fire and we're eating no more apples today, even if we\n>> planned to eat 3 when we sat down.\n>>\n>> The progress bar reflects this unexpected but recoverable state:\n>>\n>> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n>> ./helper/test-tool progress --total=3 Apples 2>&1 |\n>> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n>> Apples: 0% (0/3)^M\n>> Apples: 33% (1/3)^M\n>> Apples: 33% (1/3), done.\n>>\n>> We're at 1/3, but we're done. No more apples.\n>>\n>> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n>> remove files/directories one at a time in a directory and getting the\n>> estimated number from st_nlink (yeah yeah, unportable, but it was the\n>> first thing I thought of).\n>>\n>> We might think we're processing 10 entries, but another other processes\n>> might make our progress bar end at more or less than the 100% we\n>> expected. That's OK, not something we should invoke BUG() about.\n>\n> It doesn't have to be a BUG; a warning would suffice. And I hope not\n> finishing the expected number of items due to a catastrophic event is\n> rare enough that an additional warning wouldn't cause too much pain.\n\nIt's not a catastrophic event, just a run of the mill race condition\nwe'll expect if we're dealing with the real world.\n\nE.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked\nalready, or the command is asked to recursively unlink all files in a\ndirectory tree, and new ones have showed up.\n\nIn those cases we should just just shrug and move on, no need for a\nwarning. We just don't always have perfect information about future\nstate at the start of the loop.\n\n> Loops that *regularly* end early are not a good fit for progress\n> percentages, I think.\n\nArguably yes, but in these fuzzy cases not providing a \"total\" means\nshowing no progress at all, just a counter. Perhaps we should have some\nother \"provide total, and it may be fuzzy\" flag. Not providing it might\nrun into your proposed BUG(), my point was that the current API\nproviding this flexibility is intentional.\n\n>> Similarly, the n=0 being distinguishable from the first\n>> display_progress() is actually useful in practice. It's something I've\n>> seen git.git emit (not recently, I patched the relevant code to emit\n>> more granular progress).\n>>\n>> It's useful to know that we're stalling on the setup code before the\n>> for-loop, not on the first item.\n>\n> Hmm, preparations that take a noticeable time might deserve their own\n> progress line.\n\nSure, and I've split some of those up in the past, but this seems like\nducking/not addressing the point that the API use we disagree on has\nyour preferred use conflating these conditions, but mine does not...\n\n> Anyway, if no guard rails can be built then we have to rely on our math\n> skills alone. Off-by-one errors may look silly, but are no joke -- they\n> are surprisingly easy to make.\n\n...which, regardless of whether one views a progress of \"1/5 items\" has\n\"finished 1/5\" or \"working on 1/5\", which I think *in general* is an\narbitrary choice, I think the progress.c API we have in git.git clearly\nfits the usage I'm describing better.\n"},{"id":"427365","messageId":"4f251a35-8b5e-30f0-c742-960cb7c30b57@web.de","threadId":"55870","inReplyTo":"87zgvszo8i.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-14T17:18:05Z","receivedAt":"2021-06-14T17:18:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Thu, Jun 10 2021, René Scharfe wrote:\n>\n>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Tue, Jun 08 2021, René Scharfe wrote:\n>>>\n>>>> I wonder (only in a semi-curious way, though) if we can detect\n>>>> off-by-one errors by adding an assertion to display_progress() that\n>>>> requires the first update to have the value 0, and in stop_progress()\n>>>> one that requires the previous display_progress() call to have a value\n>>>> equal to the total number of work items. Not sure it'd be worth the\n>>>> hassle..\n>>>\n>>> That's intentional. We started eating 3 apples, got to one, but now our\n>>> house is on fire and we're eating no more apples today, even if we\n>>> planned to eat 3 when we sat down.\n>>>\n>>> The progress bar reflects this unexpected but recoverable state:\n>>>\n>>> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n>>> ./helper/test-tool progress --total=3 Apples 2>&1 |\n>>> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n>>> Apples: 0% (0/3)^M\n>>> Apples: 33% (1/3)^M\n>>> Apples: 33% (1/3), done.\n>>>\n>>> We're at 1/3, but we're done. No more apples.\n>>>\n>>> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n>>> remove files/directories one at a time in a directory and getting the\n>>> estimated number from st_nlink (yeah yeah, unportable, but it was the\n>>> first thing I thought of).\n>>>\n>>> We might think we're processing 10 entries, but another other processes\n>>> might make our progress bar end at more or less than the 100% we\n>>> expected. That's OK, not something we should invoke BUG() about.\n>>\n>> It doesn't have to be a BUG; a warning would suffice. And I hope not\n>> finishing the expected number of items due to a catastrophic event is\n>> rare enough that an additional warning wouldn't cause too much pain.\n>\n> It's not a catastrophic event, just a run of the mill race condition\n> we'll expect if we're dealing with the real world.\n>\n> E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked\n> already, or the command is asked to recursively unlink all files in a\n> directory tree, and new ones have showed up.\n>\n> In those cases we should just just shrug and move on, no need for a\n> warning. We just don't always have perfect information about future\n> state at the start of the loop.\n\nIf a planned work item is cancelled then it can still be counted as\ndone. Or the total could be adjusted, but that might look awkward.\n\n>> Loops that *regularly* end early are not a good fit for progress\n>> percentages, I think.\n>\n> Arguably yes, but in these fuzzy cases not providing a \"total\" means\n> showing no progress at all, just a counter. Perhaps we should have some\n> other \"provide total, and it may be fuzzy\" flag. Not providing it might\n> run into your proposed BUG(), my point was that the current API\n> providing this flexibility is intentional.\n\nYour patch turns a loop that doesn't immediately report skipped items\ninto one with contiguous progress updates. That's a good way to deal\nwith the imagined restrictions for error detection. Another would be\nto make the warnings optional.\n\n>>> Similarly, the n=0 being distinguishable from the first\n>>> display_progress() is actually useful in practice. It's something I've\n>>> seen git.git emit (not recently, I patched the relevant code to emit\n>>> more granular progress).\n>>>\n>>> It's useful to know that we're stalling on the setup code before the\n>>> for-loop, not on the first item.\n>>\n>> Hmm, preparations that take a noticeable time might deserve their own\n>> progress line.\n>\n> Sure, and I've split some of those up in the past, but this seems like\n> ducking/not addressing the point that the API use we disagree on has\n> your preferred use conflating these conditions, but mine does not...\n\nSubtle. If preparation takes a long time and each item less than that\nthen the progress display is likely to jump from \"0/n\" to \"i/n\", where\ni > 1, and the meaning of \"1/n\" becomes moot.\n\nThe progress display could show just the title before the first\ndisplay_progress() call to make the distinction clear. Would it really\nbe useful, though? Perhaps a trace2 region started by the first\ndisplay_progress() call and ended by the last one (n == total) would\nbe better.\n\n>> Anyway, if no guard rails can be built then we have to rely on our math\n>> skills alone. Off-by-one errors may look silly, but are no joke -- they\n>> are surprisingly easy to make.\n>\n> ...which, regardless of whether one views a progress of \"1/5 items\" has\n> \"finished 1/5\" or \"working on 1/5\", which I think *in general* is an\n> arbitrary choice, I think the progress.c API we have in git.git clearly\n> fits the usage I'm describing better.\n\nHow so? start_progress() specifies a title and the total number of\nitems, display_progress() reports some other number that is shown in\nrelation to the total, and stop_progress() finishes the progress line.\nThis API is not documented and thus its meaning is (strictly speaking)\nleft unspecified.\n\nIt can be used to show a classic \"percent-done progress indicator\", as\nhttps://dl.acm.org/doi/10.1145/1165385.317459 calls it. That's how I\nread a growing percentage shown by a program, and \"done\" I understand\nas \"has been done\" (completed), not as \"is being done\".\n\nWikipedia sent me to\nhttps://chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf,\nwhich has some fun ideas on how to warp the perception of time for\nusers staring at a progress bar, but also states typical ones grow\nwith the amount of completed work.\n\nRené\n"},{"id":"427379","messageId":"87o8c8z105.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"4f251a35-8b5e-30f0-c742-960cb7c30b57@web.de","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-14T19:08:11Z","receivedAt":"2021-06-14T19:42:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 14 2021, René Scharfe wrote:\n\n> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Thu, Jun 10 2021, René Scharfe wrote:\n>>\n>>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>>>>\n>>>> On Tue, Jun 08 2021, René Scharfe wrote:\n>>>>\n>>>>> I wonder (only in a semi-curious way, though) if we can detect\n>>>>> off-by-one errors by adding an assertion to display_progress() that\n>>>>> requires the first update to have the value 0, and in stop_progress()\n>>>>> one that requires the previous display_progress() call to have a value\n>>>>> equal to the total number of work items. Not sure it'd be worth the\n>>>>> hassle..\n>>>>\n>>>> That's intentional. We started eating 3 apples, got to one, but now our\n>>>> house is on fire and we're eating no more apples today, even if we\n>>>> planned to eat 3 when we sat down.\n>>>>\n>>>> The progress bar reflects this unexpected but recoverable state:\n>>>>\n>>>> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n>>>> ./helper/test-tool progress --total=3 Apples 2>&1 |\n>>>> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n>>>> Apples: 0% (0/3)^M\n>>>> Apples: 33% (1/3)^M\n>>>> Apples: 33% (1/3), done.\n>>>>\n>>>> We're at 1/3, but we're done. No more apples.\n>>>>\n>>>> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n>>>> remove files/directories one at a time in a directory and getting the\n>>>> estimated number from st_nlink (yeah yeah, unportable, but it was the\n>>>> first thing I thought of).\n>>>>\n>>>> We might think we're processing 10 entries, but another other processes\n>>>> might make our progress bar end at more or less than the 100% we\n>>>> expected. That's OK, not something we should invoke BUG() about.\n>>>\n>>> It doesn't have to be a BUG; a warning would suffice. And I hope not\n>>> finishing the expected number of items due to a catastrophic event is\n>>> rare enough that an additional warning wouldn't cause too much pain.\n>>\n>> It's not a catastrophic event, just a run of the mill race condition\n>> we'll expect if we're dealing with the real world.\n>>\n>> E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked\n>> already, or the command is asked to recursively unlink all files in a\n>> directory tree, and new ones have showed up.\n>>\n>> In those cases we should just just shrug and move on, no need for a\n>> warning. We just don't always have perfect information about future\n>> state at the start of the loop.\n>\n> If a planned work item is cancelled then it can still be counted as\n> done. Or the total could be adjusted, but that might look awkward.\n>\n>>> Loops that *regularly* end early are not a good fit for progress\n>>> percentages, I think.\n>>\n>> Arguably yes, but in these fuzzy cases not providing a \"total\" means\n>> showing no progress at all, just a counter. Perhaps we should have some\n>> other \"provide total, and it may be fuzzy\" flag. Not providing it might\n>> run into your proposed BUG(), my point was that the current API\n>> providing this flexibility is intentional.\n>\n> Your patch turns a loop that doesn't immediately report skipped items\n> into one with contiguous progress updates. That's a good way to deal\n> with the imagined restrictions for error detection. Another would be\n> to make the warnings optional.\n\nI don't see how there's anything wrong with the API use, how it needs a\nwarning etc.\n\n>>>> Similarly, the n=0 being distinguishable from the first\n>>>> display_progress() is actually useful in practice. It's something I've\n>>>> seen git.git emit (not recently, I patched the relevant code to emit\n>>>> more granular progress).\n>>>>\n>>>> It's useful to know that we're stalling on the setup code before the\n>>>> for-loop, not on the first item.\n>>>\n>>> Hmm, preparations that take a noticeable time might deserve their own\n>>> progress line.\n>>\n>> Sure, and I've split some of those up in the past, but this seems like\n>> ducking/not addressing the point that the API use we disagree on has\n>> your preferred use conflating these conditions, but mine does not...\n>\n> Subtle. If preparation takes a long time and each item less than that\n> then the progress display is likely to jump from \"0/n\" to \"i/n\", where\n> i > 1, and the meaning of \"1/n\" becomes moot.\n\nIn practice we're making humongous jumps all over the place, we don't\nwrite to the terminal for every item processed, and if we did it would\nbe too fast to be perceptable to the user.\n\nSo I don't think this is an issue in the first place, as noted upthread\nin <8735tt4fhx.fsf@evledraar.gmail.com>. Regardless of what we think of\nthe supposed off-by-one issue you seemed to think that it was enough of\nan issue to justify complexity at the API use level (needing to think\nabout \"continue\" statements in loops, etc.), but now you think it's\nmoot?\n\n> The progress display could show just the title before the first\n> display_progress() call to make the distinction clear. Would it really\n> be useful, though? Perhaps a trace2 region started by the first\n> display_progress() call and ended by the last one (n == total) would\n> be better.\n\nYes, it should display 0/N if it's stalled. I have some WIP patches to\ndo that. I misrecalled that we were updating the progress from the\nsignal handler, but no, we wait for the first display(), but if we just\ncall display() from the handler with a \"stalled\" message we'll clearly\nshow these cases where we're stalling in the preparation.\n\n>>> Anyway, if no guard rails can be built then we have to rely on our math\n>>> skills alone. Off-by-one errors may look silly, but are no joke -- they\n>>> are surprisingly easy to make.\n>>\n>> ...which, regardless of whether one views a progress of \"1/5 items\" has\n>> \"finished 1/5\" or \"working on 1/5\", which I think *in general* is an\n>> arbitrary choice, I think the progress.c API we have in git.git clearly\n>> fits the usage I'm describing better.\n>\n> How so? start_progress() specifies a title and the total number of\n> items, display_progress() reports some other number that is shown in\n> relation to the total, and stop_progress() finishes the progress line.\n> This API is not documented and thus its meaning is (strictly speaking)\n> left unspecified.\n\nI don't mean that it's clearly documented, I'm making the case that the\nAPI is most sanely used in the way I've laid out. It seems we're\nrespectfully disagreeing on that, but ...\n\n> It can be used to show a classic \"percent-done progress indicator\", as\n> https://dl.acm.org/doi/10.1145/1165385.317459 calls it. That's how I\n> read a growing percentage shown by a program, and \"done\" I understand\n> as \"has been done\" (completed), not as \"is being done\".\n\n...I think that's pretty moot in the first place, but I also think\nthinking about progress.c's idea of the world as \"has been done\" doesn't\nmake sense when you look at its API holistically.\n\n> Wikipedia sent me to\n> https://chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf,\n> which has some fun ideas on how to warp the perception of time for\n> users staring at a progress bar, but also states typical ones grow\n> with the amount of completed work.\n\nHow does the idea that we show \"has been done\" make sense when you\ncombine the progress.c API with the display_throughput(). I.e. output\nlike:\n\t\n\t+Working hard: 50% (1/2)\n\t+Working hard: 50% (1/2), 1.91 MiB | 195.00 KiB/s\n\t+Working hard: 50% (1/2), 2.86 MiB | 146.00 KiB/s\n\t+Working hard: 50% (1/2), 3.81 MiB | 130.00 KiB/s\n\t+Working hard: 50% (1/2), 3.81 MiB | 97.00 KiB/s, done.\n\nGiven something like (I've locally modified this a bit):\n\n\tcat >in <<-\\EOF &&\n\tstart 2\n\tthroughput 1 10000\n\tupdate\n\tprogress 1\n\tupdate\n\tthroughput 100000 10000\n\tupdate\n\tthroughput 2000000 20000\n\tupdate\n\tthroughput 3000000 30000\n\tupdate\n\tthroughput 4000000 40000\n\tupdate\n\tstop\n\tEOF\n\ttest-tool progress stderr &&\n\nThat's another reason I'm maintaining that reporting progress as \"is\nbeing done\" is the only sane way to look at it, because if you think it's\n\"has been done\" you preclude the API from being used for cases where you\ne.g. want to download 2 files, each file takes 1 minute, and you want to\nshow progress on the item itself.\n\nOur API mostly just stops before the \"progress on the item itself\", but\nif you think that layer should be \"has been done\" doesn't that mean that\ndisplay_throughput() would have to be entirely redesigned?\n\nAfter all there's no way to feed a N for item number into it, it\noptionally *updates* the progress on an existing item, such a thing\ncan't exist if we only call display_progress() to report items as being\ndone.\n\nI do think the actual output we show is pretty crappy for that use-case,\nbut that output can be changed, but not really if we insist on the\ninvariant you're maintaining that display_progress() must only be called\nif the item is done.\n\nJust like showing a \"stalled\" message if we get a signal before we show\nthe 1st item doing that is actually pretty easy, i.e. we could showh 1/2\ndownloaded files, and then as we get signals show a spinner...\n"},{"id":"427394","messageId":"xmqqy2bbyi0a.fsf@gitster.g","threadId":"55870","inReplyTo":"87o8c8z105.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-15T02:32:53Z","receivedAt":"2021-06-15T02:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> How does the idea that we show \"has been done\" make sense when you\n> combine the progress.c API with the display_throughput(). I.e. output\n> like:\n> \t\n> \t+Working hard: 50% (1/2)\n> \t+Working hard: 50% (1/2), 1.91 MiB | 195.00 KiB/s\n> \t+Working hard: 50% (1/2), 2.86 MiB | 146.00 KiB/s\n> \t+Working hard: 50% (1/2), 3.81 MiB | 130.00 KiB/s\n> \t+Working hard: 50% (1/2), 3.81 MiB | 97.00 KiB/s, done.\n> ...\n> That's another reason I'm maintaining that reporting progress as \"is\n> being done\" is the only sane way to look at it, because if you think it's\n> \"has been done\" you preclude the API from being used for cases where you\n> e.g. want to download 2 files, each file takes 1 minute, and you want to\n> show progress on the item itself.\n\nSorry, but I do not understand your argument here at all.\n\nIf you show \"has been completed\", when such a thing starts, I'd see\nfor a minute this:\n\n Downloading (0/2) ... X MiB | Y kiB/s\n\nand then it will switch to\n\n Downloading (1/2) ... progress ...\n\nand after staring at it for another minute, I'd see\n\n Downloading (2/2) ... done.\n\nAnd such an output makes sense to me. It is obvious, at least to\nme, that the output during the first minute is telling me that it\nhasn't finished but working hard to turn that \"0\" into \"1\" at the\npace the throughput thing is showing.\n"},{"id":"427486","messageId":"34d5febf-508c-52b8-a04b-98298d75bd8d@web.de","threadId":"55870","inReplyTo":"87o8c8z105.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-15T15:14:07Z","receivedAt":"2021-06-15T15:14:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Jun 14 2021, René Scharfe wrote:\n>\n>> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Thu, Jun 10 2021, René Scharfe wrote:\n>>>\n>>>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>>>>>\n>>>>> On Tue, Jun 08 2021, René Scharfe wrote:\n>>>>>\n>>>>>> I wonder (only in a semi-curious way, though) if we can detect\n>>>>>> off-by-one errors by adding an assertion to display_progress() that\n>>>>>> requires the first update to have the value 0, and in stop_progress()\n>>>>>> one that requires the previous display_progress() call to have a value\n>>>>>> equal to the total number of work items. Not sure it'd be worth the\n>>>>>> hassle..\n>>>>>\n>>>>> That's intentional. We started eating 3 apples, got to one, but now our\n>>>>> house is on fire and we're eating no more apples today, even if we\n>>>>> planned to eat 3 when we sat down.\n>>>>>\n>>>>> The progress bar reflects this unexpected but recoverable state:\n>>>>>\n>>>>> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n>>>>> ./helper/test-tool progress --total=3 Apples 2>&1 |\n>>>>> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n>>>>> Apples: 0% (0/3)^M\n>>>>> Apples: 33% (1/3)^M\n>>>>> Apples: 33% (1/3), done.\n>>>>>\n>>>>> We're at 1/3, but we're done. No more apples.\n>>>>>\n>>>>> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n>>>>> remove files/directories one at a time in a directory and getting the\n>>>>> estimated number from st_nlink (yeah yeah, unportable, but it was the\n>>>>> first thing I thought of).\n>>>>>\n>>>>> We might think we're processing 10 entries, but another other processes\n>>>>> might make our progress bar end at more or less than the 100% we\n>>>>> expected. That's OK, not something we should invoke BUG() about.\n>>>>\n>>>> It doesn't have to be a BUG; a warning would suffice. And I hope not\n>>>> finishing the expected number of items due to a catastrophic event is\n>>>> rare enough that an additional warning wouldn't cause too much pain.\n>>>\n>>> It's not a catastrophic event, just a run of the mill race condition\n>>> we'll expect if we're dealing with the real world.\n>>>\n>>> E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked\n>>> already, or the command is asked to recursively unlink all files in a\n>>> directory tree, and new ones have showed up.\n>>>\n>>> In those cases we should just just shrug and move on, no need for a\n>>> warning. We just don't always have perfect information about future\n>>> state at the start of the loop.\n>>\n>> If a planned work item is cancelled then it can still be counted as\n>> done. Or the total could be adjusted, but that might look awkward.\n>>\n>>>> Loops that *regularly* end early are not a good fit for progress\n>>>> percentages, I think.\n>>>\n>>> Arguably yes, but in these fuzzy cases not providing a \"total\" means\n>>> showing no progress at all, just a counter. Perhaps we should have some\n>>> other \"provide total, and it may be fuzzy\" flag. Not providing it might\n>>> run into your proposed BUG(), my point was that the current API\n>>> providing this flexibility is intentional.\n>>\n>> Your patch turns a loop that doesn't immediately report skipped items\n>> into one with contiguous progress updates. That's a good way to deal\n>> with the imagined restrictions for error detection. Another would be\n>> to make the warnings optional.\n>\n> I don't see how there's anything wrong with the API use, how it needs a\n> warning etc.\n\nYou pointed out that many callsites do:\n\n\tfor (i = 0; i < large_number; i++) {\n\t\tdisplay_progress(p, i + 1);\n\t\t/* work work work */\n\t}\n\nThis is an off-by-one error because a finished item is reported before\nwork on it starts. Adding a warning can help find these cases reliably.\n\n>>>>> Similarly, the n=0 being distinguishable from the first\n>>>>> display_progress() is actually useful in practice. It's something I've\n>>>>> seen git.git emit (not recently, I patched the relevant code to emit\n>>>>> more granular progress).\n>>>>>\n>>>>> It's useful to know that we're stalling on the setup code before the\n>>>>> for-loop, not on the first item.\n>>>>\n>>>> Hmm, preparations that take a noticeable time might deserve their own\n>>>> progress line.\n>>>\n>>> Sure, and I've split some of those up in the past, but this seems like\n>>> ducking/not addressing the point that the API use we disagree on has\n>>> your preferred use conflating these conditions, but mine does not...\n>>\n>> Subtle. If preparation takes a long time and each item less than that\n>> then the progress display is likely to jump from \"0/n\" to \"i/n\", where\n>> i > 1, and the meaning of \"1/n\" becomes moot.\n>\n> In practice we're making humongous jumps all over the place, we don't\n> write to the terminal for every item processed, and if we did it would\n> be too fast to be perceptable to the user.\n>\n> So I don't think this is an issue in the first place, as noted upthread\n> in <8735tt4fhx.fsf@evledraar.gmail.com>. Regardless of what we think of\n> the supposed off-by-one issue you seemed to think that it was enough of\n> an issue to justify complexity at the API use level (needing to think\n> about \"continue\" statements in loops, etc.), but now you think it's\n> moot?\n\nI don't understand your question. Let me rephrase what I find moot:\n\nYou wrote that the first display_progress() call being made with n>0\nwould be useful to you to see long-running preparations. If items are\nprocessed quicker than one per second, then whatever number the first\ndisplay_progress() call writes to the screen will be overwritten within\na second. So the value of the first update shouldn't actually matter\nmuch for your use case -- unless items takes a long time to process.\n\nRené\n"},{"id":"427502","messageId":"877divystm.fsf@evledraar.gmail.com","threadId":"55870","inReplyTo":"34d5febf-508c-52b8-a04b-98298d75bd8d@web.de","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-15T16:46:17Z","receivedAt":"2021-06-15T16:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 15 2021, René Scharfe wrote:\n\n> Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Mon, Jun 14 2021, René Scharfe wrote:\n>>\n>>> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:\n>>>>\n>>>> On Thu, Jun 10 2021, René Scharfe wrote:\n>>>>\n>>>>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:\n>>>>>>\n>>>>>> On Tue, Jun 08 2021, René Scharfe wrote:\n>>>>>>\n>>>>>>> I wonder (only in a semi-curious way, though) if we can detect\n>>>>>>> off-by-one errors by adding an assertion to display_progress() that\n>>>>>>> requires the first update to have the value 0, and in stop_progress()\n>>>>>>> one that requires the previous display_progress() call to have a value\n>>>>>>> equal to the total number of work items. Not sure it'd be worth the\n>>>>>>> hassle..\n>>>>>>\n>>>>>> That's intentional. We started eating 3 apples, got to one, but now our\n>>>>>> house is on fire and we're eating no more apples today, even if we\n>>>>>> planned to eat 3 when we sat down.\n>>>>>>\n>>>>>> The progress bar reflects this unexpected but recoverable state:\n>>>>>>\n>>>>>> $ perl -wE 'for (0..1) { say \"update\"; say \"progress $_\" }' |\n>>>>>> ./helper/test-tool progress --total=3 Apples 2>&1 |\n>>>>>> cat -v | perl -pe 's/\\^M\\K/\\n/g'\n>>>>>> Apples: 0% (0/3)^M\n>>>>>> Apples: 33% (1/3)^M\n>>>>>> Apples: 33% (1/3), done.\n>>>>>>\n>>>>>> We're at 1/3, but we're done. No more apples.\n>>>>>>\n>>>>>> This isn't just some hypothetical, e.g. consider neeing to unlink() or\n>>>>>> remove files/directories one at a time in a directory and getting the\n>>>>>> estimated number from st_nlink (yeah yeah, unportable, but it was the\n>>>>>> first thing I thought of).\n>>>>>>\n>>>>>> We might think we're processing 10 entries, but another other processes\n>>>>>> might make our progress bar end at more or less than the 100% we\n>>>>>> expected. That's OK, not something we should invoke BUG() about.\n>>>>>\n>>>>> It doesn't have to be a BUG; a warning would suffice. And I hope not\n>>>>> finishing the expected number of items due to a catastrophic event is\n>>>>> rare enough that an additional warning wouldn't cause too much pain.\n>>>>\n>>>> It's not a catastrophic event, just a run of the mill race condition\n>>>> we'll expect if we're dealing with the real world.\n>>>>\n>>>> E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked\n>>>> already, or the command is asked to recursively unlink all files in a\n>>>> directory tree, and new ones have showed up.\n>>>>\n>>>> In those cases we should just just shrug and move on, no need for a\n>>>> warning. We just don't always have perfect information about future\n>>>> state at the start of the loop.\n>>>\n>>> If a planned work item is cancelled then it can still be counted as\n>>> done. Or the total could be adjusted, but that might look awkward.\n>>>\n>>>>> Loops that *regularly* end early are not a good fit for progress\n>>>>> percentages, I think.\n>>>>\n>>>> Arguably yes, but in these fuzzy cases not providing a \"total\" means\n>>>> showing no progress at all, just a counter. Perhaps we should have some\n>>>> other \"provide total, and it may be fuzzy\" flag. Not providing it might\n>>>> run into your proposed BUG(), my point was that the current API\n>>>> providing this flexibility is intentional.\n>>>\n>>> Your patch turns a loop that doesn't immediately report skipped items\n>>> into one with contiguous progress updates. That's a good way to deal\n>>> with the imagined restrictions for error detection. Another would be\n>>> to make the warnings optional.\n>>\n>> I don't see how there's anything wrong with the API use, how it needs a\n>> warning etc.\n>\n> You pointed out that many callsites do:\n>\n> \tfor (i = 0; i < large_number; i++) {\n> \t\tdisplay_progress(p, i + 1);\n> \t\t/* work work work */\n> \t}\n>\n> This is an off-by-one error because a finished item is reported before\n> work on it starts. Adding a warning can help find these cases reliably.\n\nI understand that we're respectfully disagreeing and that's what you\nthink, but I really don't think it helps anyone if we just repeat our\nrespective points.\n\nI don't think it's off-by-one, but you do.\n\nYes, I understand that you think that progress bars should absolutely\nnever ever show 1/5 if the first item is not finished. I disagree and\nthink that's not intuitive, per my \"eating an Apple\" example\nupthread.\n\nWe disagree, and I for one think I understand what you mean, perhaps you\ndon't understand what I mean, but let's try to move on.\n\n>>>>>> Similarly, the n=0 being distinguishable from the first\n>>>>>> display_progress() is actually useful in practice. It's something I've\n>>>>>> seen git.git emit (not recently, I patched the relevant code to emit\n>>>>>> more granular progress).\n>>>>>>\n>>>>>> It's useful to know that we're stalling on the setup code before the\n>>>>>> for-loop, not on the first item.\n>>>>>\n>>>>> Hmm, preparations that take a noticeable time might deserve their own\n>>>>> progress line.\n>>>>\n>>>> Sure, and I've split some of those up in the past, but this seems like\n>>>> ducking/not addressing the point that the API use we disagree on has\n>>>> your preferred use conflating these conditions, but mine does not...\n>>>\n>>> Subtle. If preparation takes a long time and each item less than that\n>>> then the progress display is likely to jump from \"0/n\" to \"i/n\", where\n>>> i > 1, and the meaning of \"1/n\" becomes moot.\n>>\n>> In practice we're making humongous jumps all over the place, we don't\n>> write to the terminal for every item processed, and if we did it would\n>> be too fast to be perceptable to the user.\n>>\n>> So I don't think this is an issue in the first place, as noted upthread\n>> in <8735tt4fhx.fsf@evledraar.gmail.com>. Regardless of what we think of\n>> the supposed off-by-one issue you seemed to think that it was enough of\n>> an issue to justify complexity at the API use level (needing to think\n>> about \"continue\" statements in loops, etc.), but now you think it's\n>> moot?\n>\n> I don't understand your question. Let me rephrase what I find moot:\n>\n> You wrote that the first display_progress() call being made with n>0\n> would be useful to you to see long-running preparations. If items are\n> processed quicker than one per second, then whatever number the first\n> display_progress() call writes to the screen will be overwritten within\n> a second. So the value of the first update shouldn't actually matter\n> much for your use case -- unless items takes a long time to process.\n\nI think it would be better if you replied specifically to the comments I\nhad later about throughput progress bars, i.e.:\n\n How does the idea that we show \"has been done\" make sense when you\n combine the progress.c API with the display_throughput(). I.e. output\n like[...]\n\nAnyway, in this case I understood you to mean that you thought the\noff-by-one wasn't a big deal in practice most of the time, I don't think\nso either for e.g. counting objects in pack files.\n\nI do think it's useful to be consistent though, and for e.g. cases of\ndownloading 5 files it makes sense to show 1/5 if we are currently in\nthe process of downloading files 1 out of 5, not 0/5 or whatever.\n"},{"id":"427973","messageId":"0a30d7f3-1718-f2c5-39ab-75acfd754d9a@web.de","threadId":"55870","inReplyTo":"877divystm.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-20T12:53:05Z","receivedAt":"2021-06-20T12:54:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.06.21 um 18:46 schrieb Ævar Arnfjörð Bjarmason:\n> We disagree, and I for one think I understand what you mean, perhaps you\n> don't understand what I mean, but let's try to move on.\n\nPerhaps. You seem to think of progress as being represented by a real\nnumber. We show integers, though, and you want to round up.\n\nThe progress display's function is to inform the user that work is being\ndone and how much there is still to do. It allows them to decide\nwhether to keep the program running.\n\nA progress of \"100%\" being shown for an extended duration would lead me\nto the conclusion that the program hangs and I'd cancel it. Rounding\ndown (truncating) prevents that.\n\nShowing an estimated time of completion as well would be even nicer,\nbut is only practical if the time taken for each work item is roughly\nthe same.\n\nBut let's move on indeed. The part of your patch that moves the\ndisplay_progress() call to the top of the loop to avoid stalling is a\ngood idea and worth splitting out into its own patch (keeping the \"i\").\n\nIn general it seems that changes described with \"Let's also ...\" or\n\"While at it ...\" almost always deserve their own patch. I need to\nfollow that insight more myself..\n\n> I think it would be better if you replied specifically to the comments I\n> had later about throughput progress bars, i.e.:\n>\n> How does the idea that we show \"has been done\" make sense when you\n> combine the progress.c API with the display_throughput(). I.e. output\n> like[...]\n\nJunio already replied to that, but since you ask, here are my thoughts:\n\nProgress and throughput are separate metrics. Adding one doesn't change\nthe other. The throughput value is not specific to the currently\nprocessed item.\n\nSay we download a number of files of different sizes and want to show\nour progress. Then from time to time we display the number of processed\nfiles and how many bytes we got since the last update, divided by the\ntime passed since then. The reported bytes could belong to multiple\nfiles. Or we could process lots of zero-sized files, which would keep\nthroughput low.\n\n> Anyway, in this case I understood you to mean that you thought the\n> off-by-one wasn't a big deal in practice most of the time, I don't think\n> so either for e.g. counting objects in pack files.\n\nNot exactly. While I think a difference of one isn't a big deal most\nof the time, also think there is a correct way, i.e. to show the number\nof completed items. You have found ways to use an off-by-one error, and\nmy point was that this usage is not reliable. Perhaps that's a weak\nand convoluted argument.\n\n> I do think it's useful to be consistent though, and for e.g. cases of\n> downloading 5 files it makes sense to show 1/5 if we are currently in\n> the process of downloading files 1 out of 5, not 0/5 or whatever.\n\nI agree that we should be consistent. If we have downloaded 70% of the\nfirst of five files then we have 0.7 files, which is not yet 1 file, so\nwe have to say 0/5.\n\nBut let's move on, for real.\n\nRené\n"}]}