* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 0:13 ` Taylor Blau
@ 2021-05-25 0:39 ` Derrick Stolee
2021-05-25 6:32 ` Junio C Hamano
2021-05-25 2:54 ` Junio C Hamano
2021-05-25 7:39 ` Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 18+ messages in thread
From: Derrick Stolee @ 2021-05-25 0:39 UTC (permalink / raw)
To: Taylor Blau, Ævar Arnfjörð Bjarmason
Cc: Jonathan Nieder, Derrick Stolee via GitGitGadget, git, gitster,
Derrick Stolee, Derrick Stolee
On 5/24/2021 8:13 PM, Taylor Blau wrote:
> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, May 24 2021, Taylor Blau wrote:
>>
>>> But I don't think this isolated instance should push in the direction of
>>> adding support for either of the above, regardless of how easy it might
>>> be.
>>
>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1
>> or something for "inf".
>
> Ironically, I think that this already works, since we parse the value of
> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the
> input is negative (since we eventually call git_parse_unsigned(), which
> doesn't have any extra checks other than for overflow).
>
> So we silently convert -1 to 2^64-1, and call it a day.
That works for me. I'll send a v2 with that tomorrow unless someone
presents a better option.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 0:39 ` Derrick Stolee
@ 2021-05-25 6:32 ` Junio C Hamano
2021-05-25 10:54 ` Derrick Stolee
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2021-05-25 6:32 UTC (permalink / raw)
To: Derrick Stolee
Cc: Taylor Blau, Ævar Arnfjörð Bjarmason,
Jonathan Nieder, Derrick Stolee via GitGitGadget, git,
Derrick Stolee, Derrick Stolee
Derrick Stolee <stolee@gmail.com> writes:
>> So we silently convert -1 to 2^64-1, and call it a day.
>
> That works for me. I'll send a v2 with that tomorrow unless someone
> presents a better option.
I'll queue with this tweak for tonight's integration run.
Thanks.
1: d327f7d3b9 ! 1: e2b05746e1 t1092: use GIT_PROGRESS_DELAY for consistent results
@@ Commit message
values may be different as those indexes have a different number of
entries.
- Instead, use GIT_PROGRESS_DELAY=100000 to ensure that any reasonable
- machine running these tests would never display delayed progress
- indicators.
+ Instead, use GIT_PROGRESS_DELAY=-1 (which will turn into UINT_MAX)
+ to ensure that any reasonable machine running these tests would
+ never display delayed progress indicators.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {
(
cd sparse-checkout &&
- "$@" >../sparse-checkout-out 2>../sparse-checkout-err
-+ GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
++ GIT_PROGRESS_DELAY=-1 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
) &&
(
cd sparse-index &&
- "$@" >../sparse-index-out 2>../sparse-index-err
-+ GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
++ GIT_PROGRESS_DELAY=-1 "$@" >../sparse-index-out 2>../sparse-index-err
)
}
@@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {
(
cd full-checkout &&
- "$@" >../full-checkout-out 2>../full-checkout-err
-+ GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
++ GIT_PROGRESS_DELAY=-1 "$@" >../full-checkout-out 2>../full-checkout-err
) &&
run_on_sparse "$@"
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 6:32 ` Junio C Hamano
@ 2021-05-25 10:54 ` Derrick Stolee
2021-05-25 20:46 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Derrick Stolee @ 2021-05-25 10:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, Ævar Arnfjörð Bjarmason,
Jonathan Nieder, Derrick Stolee via GitGitGadget, git,
Derrick Stolee, Derrick Stolee
On 5/25/2021 2:32 AM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>>> So we silently convert -1 to 2^64-1, and call it a day.
>>
>> That works for me. I'll send a v2 with that tomorrow unless someone
>> presents a better option.
>
> I'll queue with this tweak for tonight's integration run.
>
> Thanks.
>
> 1: d327f7d3b9 ! 1: e2b05746e1 t1092: use GIT_PROGRESS_DELAY for consistent results
> @@ Commit message
> values may be different as those indexes have a different number of
> entries.
>
> - Instead, use GIT_PROGRESS_DELAY=100000 to ensure that any reasonable
> - machine running these tests would never display delayed progress
> - indicators.
> + Instead, use GIT_PROGRESS_DELAY=-1 (which will turn into UINT_MAX)
> + to ensure that any reasonable machine running these tests would
> + never display delayed progress indicators.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {
> (
> cd sparse-checkout &&
> - "$@" >../sparse-checkout-out 2>../sparse-checkout-err
> -+ GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
> ++ GIT_PROGRESS_DELAY=-1 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
> ) &&
> (
> cd sparse-index &&
> - "$@" >../sparse-index-out 2>../sparse-index-err
> -+ GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
> ++ GIT_PROGRESS_DELAY=-1 "$@" >../sparse-index-out 2>../sparse-index-err
> )
> }
>
> @@ t/t1092-sparse-checkout-compatibility.sh: init_repos () {
> (
> cd full-checkout &&
> - "$@" >../full-checkout-out 2>../full-checkout-err
> -+ GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
> ++ GIT_PROGRESS_DELAY=-1 "$@" >../full-checkout-out 2>../full-checkout-err
> ) &&
> run_on_sparse "$@"
> }
Thank you for proactively modifying the patch. This works
for me. I didn't realize that this was affecting other
contributors [1] until I woke up this morning.
[1] https://lore.kernel.org/git/036b01d750ed$642b75c0$2c826140$@nexbridge.com/
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 10:54 ` Derrick Stolee
@ 2021-05-25 20:46 ` Junio C Hamano
2021-05-25 21:14 ` Junio C Hamano
2021-05-25 21:49 ` Taylor Blau
0 siblings, 2 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-05-25 20:46 UTC (permalink / raw)
To: Derrick Stolee
Cc: Taylor Blau, Ævar Arnfjörð Bjarmason,
Jonathan Nieder, Derrick Stolee via GitGitGadget, git,
Derrick Stolee, Derrick Stolee
Derrick Stolee <stolee@gmail.com> writes:
> On 5/25/2021 2:32 AM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>>> So we silently convert -1 to 2^64-1, and call it a day.
>>>
>>> That works for me. I'll send a v2 with that tomorrow unless someone
>>> presents a better option.
>>
>> I'll queue with this tweak for tonight's integration run.
>>
>> ...
> Thank you for proactively modifying the patch. This works
> for me. I didn't realize that this was affecting other
> contributors [1] until I woke up this morning.
>
> [1] https://lore.kernel.org/git/036b01d750ed$642b75c0$2c826140$@nexbridge.com/
Well, not so well X-<. It seems that some builds are not happy with
this change. See https://github.com/git/git/actions/runs/876229761
specifically these two:
https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549
https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988
I suspect that it has something to do with 32-bit platforms?
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 20:46 ` Junio C Hamano
@ 2021-05-25 21:14 ` Junio C Hamano
2021-05-25 21:49 ` Taylor Blau
1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-05-25 21:14 UTC (permalink / raw)
To: Derrick Stolee
Cc: Taylor Blau, Ævar Arnfjörð Bjarmason,
Jonathan Nieder, Derrick Stolee via GitGitGadget, git,
Derrick Stolee, Derrick Stolee
Junio C Hamano <gitster@pobox.com> writes:
> Well, not so well X-<. It seems that some builds are not happy with
> this change. See https://github.com/git/git/actions/runs/876229761
> specifically these two:
>
> https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549
> https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988
>
> I suspect that it has something to do with 32-bit platforms?
Reverting the change that was squashed in seems to do the job.
https://github.com/git/git/actions/runs/876338380 is labelled as
'seen' but it is 'next' that failed in the message I am responding
to plus the revert of s/=100000/=-1/g.
Let's queue the 100000 version for now and get the =inf patch
applied after the release.
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 20:46 ` Junio C Hamano
2021-05-25 21:14 ` Junio C Hamano
@ 2021-05-25 21:49 ` Taylor Blau
1 sibling, 0 replies; 18+ messages in thread
From: Taylor Blau @ 2021-05-25 21:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee, Taylor Blau,
Ævar Arnfjörð Bjarmason, Jonathan Nieder,
Derrick Stolee via GitGitGadget, git, Derrick Stolee,
Derrick Stolee
On Wed, May 26, 2021 at 05:46:16AM +0900, Junio C Hamano wrote:
> Well, not so well X-<. It seems that some builds are not happy with
> this change. See https://github.com/git/git/actions/runs/876229761
> specifically these two:
>
> https://github.com/git/git/runs/2669177395?check_suite_focus=true#step:7:3549
> https://github.com/git/git/runs/2669080101?check_suite_focus=true#step:6:988
>
> I suspect that it has something to do with 32-bit platforms?
Thanks. Of course, redirecting stderr into a file and halting after we
get a non-zero exit code makes this pretty hard to debug from that
output alone, but this is pretty easily reproducible on a 32-bit Docker
image:
root@99cfe0d56673:/git-master# getconf LONG_BIT
32
root@99cfe0d56673:/git-master# GIT_PROGRESS_DELAY=-1 ./bin-wrappers/git status
fatal: failed to parse GIT_PROGRESS_DELAY
Looking more closely in a debugger shows that we're failing because of
this check in 'config.c:git_parse_unsigned()':
if (unsigned_mult_overflows(factor, val) ||
factor * val > max) {
errno = ERANGE;
return 0;
}
unsigned_mult_overflows() doesn't trigger regardless of architecture,
since even though val is large, factor is 1, so factor * val == val. But
val is much larger than max, so we fail there. 'max' is just
'maximum_unsigned_value_of_type(long)', or 2^32-1, while val is 2^64-1.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 0:13 ` Taylor Blau
2021-05-25 0:39 ` Derrick Stolee
@ 2021-05-25 2:54 ` Junio C Hamano
2021-05-25 15:10 ` Taylor Blau
2021-05-25 7:39 ` Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2021-05-25 2:54 UTC (permalink / raw)
To: Taylor Blau
Cc: Ævar Arnfjörð Bjarmason, Derrick Stolee,
Jonathan Nieder, Derrick Stolee via GitGitGadget, git,
Derrick Stolee, Derrick Stolee
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, May 24 2021, Taylor Blau wrote:
>>
>> > But I don't think this isolated instance should push in the direction of
>> > adding support for either of the above, regardless of how easy it might
>> > be.
>>
>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1
>> or something for "inf".
>
> Ironically, I think that this already works, since we parse the value of
> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the
> input is negative (since we eventually call git_parse_unsigned(), which
> doesn't have any extra checks other than for overflow).
>
> So we silently convert -1 to 2^64-1, and call it a day.
Stepping back a bit, this is an unattended test---why do we even see
progress meters? Are we forcing the output to tty somehow in our
tests, or do some codepaths forget to ask isatty() when the command
line does not say --progress or --no-progress?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 2:54 ` Junio C Hamano
@ 2021-05-25 15:10 ` Taylor Blau
0 siblings, 0 replies; 18+ messages in thread
From: Taylor Blau @ 2021-05-25 15:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, Ævar Arnfjörð Bjarmason,
Derrick Stolee, Jonathan Nieder, Derrick Stolee via GitGitGadget,
git, Derrick Stolee, Derrick Stolee
On Tue, May 25, 2021 at 11:54:56AM +0900, Junio C Hamano wrote:
> Stepping back a bit, this is an unattended test---why do we even see
> progress meters? Are we forcing the output to tty somehow in our
> tests, or do some codepaths forget to ask isatty() when the command
> line does not say --progress or --no-progress?
I found this while responding to Randall (who is observing the same
problem in another thread):
https://lore.kernel.org/git/YK0TKVZidW%2FG8XBr@nand.local/
Thanks,
Taylor
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 0:13 ` Taylor Blau
2021-05-25 0:39 ` Derrick Stolee
2021-05-25 2:54 ` Junio C Hamano
@ 2021-05-25 7:39 ` Ævar Arnfjörð Bjarmason
2021-05-25 8:06 ` Junio C Hamano
2 siblings, 1 reply; 18+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-25 7:39 UTC (permalink / raw)
To: Taylor Blau
Cc: Derrick Stolee, Jonathan Nieder, Derrick Stolee via GitGitGadget,
git, gitster, Derrick Stolee, Derrick Stolee
On Mon, May 24 2021, Taylor Blau wrote:
> On Tue, May 25, 2021 at 12:57:52AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, May 24 2021, Taylor Blau wrote:
>>
>> > But I don't think this isolated instance should push in the direction of
>> > adding support for either of the above, regardless of how easy it might
>> > be.
>>
>> I don't see why we wouldn't just tweak GIT_PROGRESS_DELAY to support -1
>> or something for "inf".
>
> Ironically, I think that this already works, since we parse the value of
> GIT_PROGRESS_DELAY as unsigned, and don't bother checking for if the
> input is negative (since we eventually call git_parse_unsigned(), which
> doesn't have any extra checks other than for overflow).
>
> So we silently convert -1 to 2^64-1, and call it a day.
Well yes, it works in the sense that instead of arbitrary big value for
delay we have the biggerest and largerest value we can manage :)
I mean why do just that when we can also do this:
diff --git a/progress.c b/progress.c
index 680c6a8bf93..191c62cbbfb 100644
--- a/progress.c
+++ b/progress.c
@@ -252,7 +252,13 @@ void display_progress(struct progress *progress, uint64_t n)
static struct progress *start_progress_delay(const char *title, uint64_t total,
unsigned delay, unsigned sparse)
{
- struct progress *progress = xmalloc(sizeof(*progress));
+ struct progress *progress;
+
+ /* GIT_PROGRESS_DELAY=-1 */
+ if (delay == (unsigned)-1)
+ return NULL;
+
+ progress = xmalloc(sizeof(*progress));
progress->title = title;
progress->total = total;
progress->last_value = -1;
Which will cause the progress code to abort early in that case, and IMO
is less magical if we're going to have this GIT_PROGRESS_DELAY=-1 in the
codebase and relying on the wrap-around of -1.
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] t1092: use GIT_PROGRESS_DELAY for consistent results
2021-05-25 7:39 ` Ævar Arnfjörð Bjarmason
@ 2021-05-25 8:06 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2021-05-25 8:06 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Taylor Blau, Derrick Stolee, Jonathan Nieder,
Derrick Stolee via GitGitGadget, git, Derrick Stolee,
Derrick Stolee
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Well yes, it works in the sense that instead of arbitrary big value for
> delay we have the biggerest and largerest value we can manage :)
>
> I mean why do just that when we can also do this:
Don't make unnecessary changes before any release. If a breakage
can be fixed without risking to introduce any new breakages with
code change, postpone such a change and save bandwidth to finding
and fixing _other_ regressions.
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread