{"thread":{"id":"55765","subject":"[PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","startedAt":"2021-05-24T19:55:16Z","lastAt":"2021-05-25T21:49:37Z","messageCount":18,"participants":["Derrick Stolee via GitGitGadget","Jonathan Nieder","Derrick Stolee","Taylor Blau","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425453","messageId":"pull.960.git.1621886108515.gitgitgadget@gmail.com","threadId":"55765","inReplyTo":null,"subject":"[PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-24T19:55:07Z","receivedAt":"2021-05-24T19:55:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe t1092-sparse-checkout-compatibility.sh tests compare the stdout and\nstderr for several Git commands across both full checkouts, sparse\ncheckouts with a full index, and sparse checkouts with a sparse index.\nSince these are direct comparisons, sometimes a progress indicator can\nflush at unpredictable points, especially on slower machines. This\ncauses the tests to be flaky.\n\nOne standard way to avoid this is to add GIT_PROGRESS_DELAY=0 to the Git\ncommands that are run, as this will force every progress indicator\ncreated with start_progress_delay() to be created immediately. However,\nthere are some progress indicators that are created in the case of a\nfull index that are not created with a sparse index. Moreover, their\nvalues may be different as those indexes have a different number of\nentries.\n\nInstead, use GIT_PROGRESS_DELAY=100000 to ensure that any reasonable\nmachine running these tests would never display delayed progress\nindicators.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    t1092: use GIT_PROGRESS_DELAY for consistent results\n    \n    We found this while running PR builds in microsoft/git.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-960%2Fderrickstolee%2Fsparse-index%2Fprogress-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-960/derrickstolee/sparse-index/progress-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/960\n\n t/t1092-sparse-checkout-compatibility.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 12e6c453024f..e9a815ca7aaa 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -106,18 +106,18 @@ init_repos () {\n run_on_sparse () {\n \t(\n \t\tcd sparse-checkout &&\n-\t\t\"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n \t) &&\n \t(\n \t\tcd sparse-index &&\n-\t\t\"$@\" >../sparse-index-out 2>../sparse-index-err\n+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-index-out 2>../sparse-index-err\n \t)\n }\n \n run_on_all () {\n \t(\n \t\tcd full-checkout &&\n-\t\t\"$@\" >../full-checkout-out 2>../full-checkout-err\n+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n \t) &&\n \trun_on_sparse \"$@\"\n }\n\nbase-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a\n-- \ngitgitgadget\n"},{"id":"425457","messageId":"YKwMU13DOtTIgaeP@google.com","threadId":"55765","inReplyTo":"pull.960.git.1621886108515.gitgitgadget@gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-24T20:28:03Z","receivedAt":"2021-05-24T20:28:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n\n> The t1092-sparse-checkout-compatibility.sh tests compare the stdout and\n> stderr for several Git commands across both full checkouts, sparse\n> checkouts with a full index, and sparse checkouts with a sparse index.\n> Since these are direct comparisons, sometimes a progress indicator can\n> flush at unpredictable points, especially on slower machines. This\n> causes the tests to be flaky.\n\nHm, I think this test strategy is going to be fundamentally flaky\nregardless: Git doesn't intend to guarantee any kind of stability in\nthe exact stderr output it writes.\n\nCould the tests be made to check more semantically meaningful\ninformation such as \"git ls-files -s\" output instead?\n\nThanks,\nJonathan\n"},{"id":"425458","messageId":"e35b5217-362f-fc4f-7396-08218a9abf63@gmail.com","threadId":"55765","inReplyTo":"YKwMU13DOtTIgaeP@google.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-24T20:38:18Z","receivedAt":"2021-05-24T20:38:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/21 4:28 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Derrick Stolee wrote:\n> \n>> The t1092-sparse-checkout-compatibility.sh tests compare the stdout and\n>> stderr for several Git commands across both full checkouts, sparse\n>> checkouts with a full index, and sparse checkouts with a sparse index.\n>> Since these are direct comparisons, sometimes a progress indicator can\n>> flush at unpredictable points, especially on slower machines. This\n>> causes the tests to be flaky.\n> \n> Hm, I think this test strategy is going to be fundamentally flaky\n> regardless: Git doesn't intend to guarantee any kind of stability in\n> the exact stderr output it writes.\n> \n> Could the tests be made to check more semantically meaningful\n> information such as \"git ls-files -s\" output instead?\n\nThe test is comparing the same exact Git command just with\ndifferent configurations. Any change to what Git writes to\nstderr should be consistent across these, unless there is\nan explicit reason why it would behave differently across\nthese options (for example, saying \"You are in a sparse\ncheckout\" in 'git status').\n\nThere are no expectations that stderr is stable across\nversions of Git. These tests don't add friction to developers\nmaking new features or changing the error messages that appear\nover stderr. It's just that these tests should catch any\nunintended inconsistency across these modes.\n\nThanks,\n-Stolee \n"},{"id":"425461","messageId":"YKwd2e5VxVmU6zqj@nand.local","threadId":"55765","inReplyTo":"e35b5217-362f-fc4f-7396-08218a9abf63@gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-24T21:42:49Z","receivedAt":"2021-05-24T21:42:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 24, 2021 at 04:38:18PM -0400, Derrick Stolee wrote:\n> On 5/24/21 4:28 PM, Jonathan Nieder wrote:\n> > Hm, I think this test strategy is going to be fundamentally flaky\n> > regardless: Git doesn't intend to guarantee any kind of stability in\n> > the exact stderr output it writes.\n>\n> There are no expectations that stderr is stable across\n> versions of Git. These tests don't add friction to developers\n> making new features or changing the error messages that appear\n> over stderr. It's just that these tests should catch any\n> unintended inconsistency across these modes.\n\nI agree with Stolee that these tests are valuable for asserting that\noutput is the consistent whether or not you are using the sparse index.\n\nI find setting GIT_PROGRESS_DELAY to a large number to a be a little\nugly, but there isn't an apparent better way to accomplish the same\nthing. Of course, it would be nice to have an environment variable to\nspecify where progress meters are written to, or a global option to\ndisable progress meters altogether.\n\nBut I don't think this isolated instance should push in the direction of\nadding support for either of the above, regardless of how easy it might\nbe.\n\nWhat would perhaps make more sense is to silence the progress meters\nfrom the commands themselves. AFAICT the only command called by\nrun_on_sparse() which generates a progress meter is 'git checkout',\n'git merge', and 'git submodule', all of which support '--no-progress'.\nMight it be worth passing that option instead of setting\nGIT_PROGRESS_DELAY to a large value?\n\n(For what it's worth, I have no strong opinion either way, so I would be\nhappy to attach my Reviewed-by to even the current version of this patch).\n\nThanks,\nTaylor\n"},{"id":"425465","messageId":"87fsybohy5.fsf@evledraar.gmail.com","threadId":"55765","inReplyTo":"YKwd2e5VxVmU6zqj@nand.local","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-24T22:57:52Z","receivedAt":"2021-05-24T23:00:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 24 2021, Taylor Blau wrote:\n\n> On Mon, May 24, 2021 at 04:38:18PM -0400, Derrick Stolee wrote:\n>> On 5/24/21 4:28 PM, Jonathan Nieder wrote:\n>> > Hm, I think this test strategy is going to be fundamentally flaky\n>> > regardless: Git doesn't intend to guarantee any kind of stability in\n>> > the exact stderr output it writes.\n>>\n>> There are no expectations that stderr is stable across\n>> versions of Git. These tests don't add friction to developers\n>> making new features or changing the error messages that appear\n>> over stderr. It's just that these tests should catch any\n>> unintended inconsistency across these modes.\n>\n> I agree with Stolee that these tests are valuable for asserting that\n> output is the consistent whether or not you are using the sparse index.\n>\n> I find setting GIT_PROGRESS_DELAY to a large number to a be a little\n> ugly, but there isn't an apparent better way to accomplish the same\n> thing. Of course, it would be nice to have an environment variable to\n> specify where progress meters are written to, or a global option to\n> disable progress meters altogether.\n>\n> But I don't think this isolated instance should push in the direction of\n> adding support for either of the above, regardless of how easy it might\n> be.\n\nI don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1\nor something for \"inf\".\n\nIt was added as a one-off (it seems for testing, but made public, so not\nin the GIT_TEST_* namespace) in 44a4693bfce (progress: create\nGIT_PROGRESS_DELAY, 2019-11-25).\n\nThe progress.c API will already nicely deal with this case if something\nin start_progress_delay() is made to return NULL if we pass a flag down\nto it.\n\n> What would perhaps make more sense is to silence the progress meters\n> from the commands themselves. AFAICT the only command called by\n> run_on_sparse() which generates a progress meter is 'git checkout',\n> 'git merge', and 'git submodule', all of which support '--no-progress'.\n> Might it be worth passing that option instead of setting\n> GIT_PROGRESS_DELAY to a large value?\n>\n> (For what it's worth, I have no strong opinion either way, so I would be\n> happy to attach my Reviewed-by to even the current version of this patch).\n>\n> Thanks,\n> Taylor\n\n"},{"id":"425472","messageId":"YKxBIP4lkoPeCYvb@nand.local","threadId":"55765","inReplyTo":"87fsybohy5.fsf@evledraar.gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-25T00:13:20Z","receivedAt":"2021-05-25T00:13:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> On Mon, May 24 2021, Taylor Blau wrote:\n>\n> > But I don't think this isolated instance should push in the direction of\n> > adding support for either of the above, regardless of how easy it might\n> > be.\n>\n> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1\n> or something for \"inf\".\n\nIronically, I think that this already works, since we parse the value of\nGIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the\ninput is negative (since we eventually call git_parse_unsigned(), which\ndoesn't have any extra checks other than for overflow).\n\nSo we silently convert -1 to 2^64-1, and call it a day.\n\nThanks,\nTaylor\n"},{"id":"425474","messageId":"68a2bf8f-07e8-b117-a3f4-862654c781b4@gmail.com","threadId":"55765","inReplyTo":"YKxBIP4lkoPeCYvb@nand.local","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-25T00:39:55Z","receivedAt":"2021-05-25T00:39:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2021 8:13 PM, Taylor Blau wrote:\n> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, May 24 2021, Taylor Blau wrote:\n>>\n>>> But I don't think this isolated instance should push in the direction of\n>>> adding support for either of the above, regardless of how easy it might\n>>> be.\n>>\n>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1\n>> or something for \"inf\".\n> \n> Ironically, I think that this already works, since we parse the value of\n> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the\n> input is negative (since we eventually call git_parse_unsigned(), which\n> doesn't have any extra checks other than for overflow).\n> \n> So we silently convert -1 to 2^64-1, and call it a day.\n\nThat works for me. I'll send a v2 with that tomorrow unless someone\npresents a better option.\n\nThanks,\n-Stolee\n"},{"id":"425477","messageId":"xmqqa6ojilhb.fsf@gitster.g","threadId":"55765","inReplyTo":"e35b5217-362f-fc4f-7396-08218a9abf63@gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T02:41:04Z","receivedAt":"2021-05-25T02:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> The test is comparing the same exact Git command just with\n> different configurations. Any change to what Git writes to\n> stderr should be consistent across these, unless there is\n> an explicit reason why it would behave differently across\n> these options (for example, saying \"You are in a sparse\n> checkout\" in 'git status').\n>\n> There are no expectations that stderr is stable across\n> versions of Git. These tests don't add friction to developers\n> making new features or changing the error messages that appear\n> over stderr. It's just that these tests should catch any\n> unintended inconsistency across these modes.\n\nIf it just happens that an auto-gc gets triggered, and millions of\nother similar reasons in the future, will break that expectation,\nwithout running two different vintages of Git.\n\nI agree with Jonathan that it fundamentally is flakey to expect two\ninvocations of Git will behave exactly the same.  Even repacking a\nrepository starting from exactly the same state into a single pack\nmay not produce byte-for-byte identical result due to thread\nscheduling.\n"},{"id":"425478","messageId":"xmqq5yz7il39.fsf@gitster.g","threadId":"55765","inReplyTo":"YKwd2e5VxVmU6zqj@nand.local","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T02:49:30Z","receivedAt":"2021-05-25T02:49:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> What would perhaps make more sense is to silence the progress meters\n> from the commands themselves. AFAICT the only command called by\n> run_on_sparse() which generates a progress meter is 'git checkout',\n> 'git merge', and 'git submodule', all of which support '--no-progress'.\n>\n> Might it be worth passing that option instead of setting\n> GIT_PROGRESS_DELAY to a large value?\n\nYup.  The progress meters are obvious source of variation, but there\nis no guarantee that is the only source of variation.  The test\nstrategy to run the same command twice and judge only by observing\ntheir stdout and stderr is, eh, suboptimal.  If we devise a sequence\nof commands that are tested immediately followed by a sequence of\ncommands to check the outcome of these commands, and the output from\nthe latter for two runs are the same, then that is a more sensible\napproach, as the latter \"visualize the outcome of the commands that\nare just tested\" can be controlled more tightly to ignore meaningless\nvariations (like progress meters or base-delta paring in a resulting\npackfile of a parallel packing) and focus on the aspects of the\noutcome that matter.\n\n\n\n\n\n\n"},{"id":"425479","messageId":"xmqq1r9viku7.fsf@gitster.g","threadId":"55765","inReplyTo":"YKxBIP4lkoPeCYvb@nand.local","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T02:54:56Z","receivedAt":"2021-05-25T02:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, May 24 2021, Taylor Blau wrote:\n>>\n>> > But I don't think this isolated instance should push in the direction of\n>> > adding support for either of the above, regardless of how easy it might\n>> > be.\n>>\n>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1\n>> or something for \"inf\".\n>\n> Ironically, I think that this already works, since we parse the value of\n> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the\n> input is negative (since we eventually call git_parse_unsigned(), which\n> doesn't have any extra checks other than for overflow).\n>\n> So we silently convert -1 to 2^64-1, and call it a day.\n\nStepping back a bit, this is an unattended test---why do we even see\nprogress meters?  Are we forcing the output to tty somehow in our\ntests, or do some codepaths forget to ask isatty() when the command\nline does not say --progress or --no-progress?\n\n"},{"id":"425491","messageId":"xmqqy2c3fhmy.fsf@gitster.g","threadId":"55765","inReplyTo":"68a2bf8f-07e8-b117-a3f4-862654c781b4@gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T06:32:21Z","receivedAt":"2021-05-25T06:32:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> So we silently convert -1 to 2^64-1, and call it a day.\n>\n> That works for me. I'll send a v2 with that tomorrow unless someone\n> presents a better option.\n\nI'll queue with this tweak for tonight's integration run.\n\nThanks.\n\n1:  d327f7d3b9 ! 1:  e2b05746e1 t1092: use GIT_PROGRESS_DELAY for consistent results\n    @@ Commit message\n         values may be different as those indexes have a different number of\n         entries.\n     \n    -    Instead, use GIT_PROGRESS_DELAY=100000 to ensure that any reasonable\n    -    machine running these tests would never display delayed progress\n    -    indicators.\n    +    Instead, use GIT_PROGRESS_DELAY=-1 (which will turn into UINT_MAX)\n    +    to ensure that any reasonable machine running these tests would\n    +    never display delayed progress indicators.\n     \n         Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n         Signed-off-by: Junio C Hamano <gitster@pobox.com>\n    @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {\n      \t(\n      \t\tcd sparse-checkout &&\n     -\t\t\"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n    -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n    ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n      \t) &&\n      \t(\n      \t\tcd sparse-index &&\n     -\t\t\"$@\" >../sparse-index-out 2>../sparse-index-err\n    -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-index-out 2>../sparse-index-err\n    ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../sparse-index-out 2>../sparse-index-err\n      \t)\n      }\n      \n    @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {\n      \t(\n      \t\tcd full-checkout &&\n     -\t\t\"$@\" >../full-checkout-out 2>../full-checkout-err\n    -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n    ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../full-checkout-out 2>../full-checkout-err\n      \t) &&\n      \trun_on_sparse \"$@\"\n      }\n"},{"id":"425499","messageId":"87y2c3mezw.fsf@evledraar.gmail.com","threadId":"55765","inReplyTo":"YKxBIP4lkoPeCYvb@nand.local","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-25T07:39:02Z","receivedAt":"2021-05-25T07:47:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 24 2021, Taylor Blau wrote:\n\n> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, May 24 2021, Taylor Blau wrote:\n>>\n>> > But I don't think this isolated instance should push in the direction of\n>> > adding support for either of the above, regardless of how easy it might\n>> > be.\n>>\n>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1\n>> or something for \"inf\".\n>\n> Ironically, I think that this already works, since we parse the value of\n> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the\n> input is negative (since we eventually call git_parse_unsigned(), which\n> doesn't have any extra checks other than for overflow).\n>\n> So we silently convert -1 to 2^64-1, and call it a day.\n\nWell yes, it works in the sense that instead of arbitrary big value for\ndelay we have the biggerest and largerest value we can manage :)\n\nI mean why do just that when we can also do this:\n\ndiff --git a/progress.c b/progress.c\nindex 680c6a8bf93..191c62cbbfb 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -252,7 +252,13 @@ void display_progress(struct progress *progress, uint64_t n)\n static struct progress *start_progress_delay(const char *title, uint64_t total,\n \t\t\t\t\t     unsigned delay, unsigned sparse)\n {\n-\tstruct progress *progress = xmalloc(sizeof(*progress));\n+\tstruct progress *progress;\n+\n+\t/* GIT_PROGRESS_DELAY=-1 */\n+\tif (delay == (unsigned)-1)\n+\t\treturn NULL;\n+\n+\tprogress = xmalloc(sizeof(*progress));\n \tprogress->title = title;\n \tprogress->total = total;\n \tprogress->last_value = -1;\n\nWhich will cause the progress code to abort early in that case, and IMO\nis less magical if we're going to have this GIT_PROGRESS_DELAY=-1 in the\ncodebase and relying on the wrap-around of -1.\n"},{"id":"425500","messageId":"xmqq35ubfd9y.fsf@gitster.g","threadId":"55765","inReplyTo":"87y2c3mezw.fsf@evledraar.gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T08:06:33Z","receivedAt":"2021-05-25T08:08:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Well yes, it works in the sense that instead of arbitrary big value for\n> delay we have the biggerest and largerest value we can manage :)\n>\n> I mean why do just that when we can also do this:\n\nDon't make unnecessary changes before any release.  If a breakage\ncan be fixed without risking to introduce any new breakages with\ncode change, postpone such a change and save bandwidth to finding\nand fixing _other_ regressions.\n\nThanks.\n\n"},{"id":"425520","messageId":"9d6568ee-c9bf-b349-d1a6-0bb2fcf06dab@gmail.com","threadId":"55765","inReplyTo":"xmqqy2c3fhmy.fsf@gitster.g","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-25T10:54:02Z","receivedAt":"2021-05-25T10:54:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/25/2021 2:32 AM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>>> So we silently convert -1 to 2^64-1, and call it a day.\n>>\n>> That works for me. I'll send a v2 with that tomorrow unless someone\n>> presents a better option.\n> \n> I'll queue with this tweak for tonight's integration run.\n> \n> Thanks.\n> \n> 1:  d327f7d3b9 ! 1:  e2b05746e1 t1092: use GIT_PROGRESS_DELAY for consistent results\n>     @@ Commit message\n>          values may be different as those indexes have a different number of\n>          entries.\n>      \n>     -    Instead, use GIT_PROGRESS_DELAY=100000 to ensure that any reasonable\n>     -    machine running these tests would never display delayed progress\n>     -    indicators.\n>     +    Instead, use GIT_PROGRESS_DELAY=-1 (which will turn into UINT_MAX)\n>     +    to ensure that any reasonable machine running these tests would\n>     +    never display delayed progress indicators.\n>      \n>          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>          Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>     @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {\n>       \t(\n>       \t\tcd sparse-checkout &&\n>      -\t\t\"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n>     -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n>     ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n>       \t) &&\n>       \t(\n>       \t\tcd sparse-index &&\n>      -\t\t\"$@\" >../sparse-index-out 2>../sparse-index-err\n>     -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-index-out 2>../sparse-index-err\n>     ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../sparse-index-out 2>../sparse-index-err\n>       \t)\n>       }\n>       \n>     @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {\n>       \t(\n>       \t\tcd full-checkout &&\n>      -\t\t\"$@\" >../full-checkout-out 2>../full-checkout-err\n>     -+\t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n>     ++\t\tGIT_PROGRESS_DELAY=-1 \"$@\" >../full-checkout-out 2>../full-checkout-err\n>       \t) &&\n>       \trun_on_sparse \"$@\"\n>       }\n\nThank you for proactively modifying the patch. This works\nfor me. I didn't realize that this was affecting other\ncontributors [1] until I woke up this morning.\n\n[1] https://lore.kernel.org/git/036b01d750ed$642b75c0$2c826140$@nexbridge.com/\n\nThanks,\n-Stolee\n"},{"id":"425526","messageId":"YK0TgfKFnqOSFKD8@nand.local","threadId":"55765","inReplyTo":"xmqq1r9viku7.fsf@gitster.g","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-25T15:10:57Z","receivedAt":"2021-05-25T15:11:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 25, 2021 at 11:54:56AM +0900, Junio C Hamano wrote:\n> Stepping back a bit, this is an unattended test---why do we even see\n> progress meters?  Are we forcing the output to tty somehow in our\n> tests, or do some codepaths forget to ask isatty() when the command\n> line does not say --progress or --no-progress?\n\nI found this while responding to Randall (who is observing the same\nproblem in another thread):\n\n  https://lore.kernel.org/git/YK0TKVZidW%2FG8XBr@nand.local/\n\nThanks,\nTaylor\n"},{"id":"425535","messageId":"xmqq1r9uee3r.fsf@gitster.g","threadId":"55765","inReplyTo":"9d6568ee-c9bf-b349-d1a6-0bb2fcf06dab@gmail.com","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T20:46:16Z","receivedAt":"2021-05-25T20:46:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 5/25/2021 2:32 AM, Junio C Hamano wrote:\n>> Derrick Stolee <stolee@gmail.com> writes:\n>> \n>>>> So we silently convert -1 to 2^64-1, and call it a day.\n>>>\n>>> That works for me. I'll send a v2 with that tomorrow unless someone\n>>> presents a better option.\n>> \n>> I'll queue with this tweak for tonight's integration run.\n>> \n>> ...\n> Thank you for proactively modifying the patch. This works\n> for me. I didn't realize that this was affecting other\n> contributors [1] until I woke up this morning.\n>\n> [1] https://lore.kernel.org/git/036b01d750ed$642b75c0$2c826140$@nexbridge.com/\n\nWell, not so well X-<.  It seems that some builds are not happy with\nthis change.  See https://github.com/git/git/actions/runs/876229761\nspecifically these two:\n\n    https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549\n    https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988\n\nI suspect that it has something to do with 32-bit platforms?\n\nThanks.\n"},{"id":"425536","messageId":"xmqqwnrmcy7l.fsf@gitster.g","threadId":"55765","inReplyTo":"xmqq1r9uee3r.fsf@gitster.g","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-25T21:14:54Z","receivedAt":"2021-05-25T21:15:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Well, not so well X-<.  It seems that some builds are not happy with\n> this change.  See https://github.com/git/git/actions/runs/876229761\n> specifically these two:\n>\n>     https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549\n>     https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988\n>\n> I suspect that it has something to do with 32-bit platforms?\n\nReverting the change that was squashed in seems to do the job.\nhttps://github.com/git/git/actions/runs/876338380 is labelled as\n'seen' but it is 'next' that failed in the message I am responding\nto plus the revert of s/=100000/=-1/g.\n\nLet's queue the 100000 version for now and get the =inf patch\napplied after the release.\n\nThanks.\n"},{"id":"425538","messageId":"YK1w7TPnH9CB0haj@nand.local","threadId":"55765","inReplyTo":"xmqq1r9uee3r.fsf@gitster.g","subject":"Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-25T21:49:33Z","receivedAt":"2021-05-25T21:49:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 26, 2021 at 05:46:16AM +0900, Junio C Hamano wrote:\n> Well, not so well X-<.  It seems that some builds are not happy with\n> this change.  See https://github.com/git/git/actions/runs/876229761\n> specifically these two:\n>\n>     https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549\n>     https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988\n>\n> I suspect that it has something to do with 32-bit platforms?\n\nThanks. Of course, redirecting stderr into a file and halting after we\nget a non-zero exit code makes this pretty hard to debug from that\noutput alone, but this is pretty easily reproducible on a 32-bit Docker\nimage:\n\n    root@99cfe0d56673:/git-master# getconf LONG_BIT\n    32\n    root@99cfe0d56673:/git-master# GIT_PROGRESS_DELAY=-1 ./bin-wrappers/git status\n    fatal: failed to parse GIT_PROGRESS_DELAY\n\nLooking more closely in a debugger shows that we're failing because of\nthis check in 'config.c:git_parse_unsigned()':\n\n    if (unsigned_mult_overflows(factor, val) ||\n        factor * val > max) {\n          errno = ERANGE;\n          return 0;\n    }\n\nunsigned_mult_overflows() doesn't trigger regardless of architecture,\nsince even though val is large, factor is 1, so factor * val == val. But\nval is much larger than max, so we fail there. 'max' is just\n'maximum_unsigned_value_of_type(long)', or 2^32-1, while val is 2^64-1.\n\nThanks,\nTaylor\n"}]}