tq: standardized progress meter formatting #2811
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
technoweenie
approved these changes
Jan 17, 2018
Contributor
technoweenie
left a comment
There was a problem hiding this comment.
Looks great. 1 super minor nit.
tq/meter.go
Outdated
| // TransferBytes increments the number of bytes transferred | ||
| func (m *Meter) TransferBytes(direction, name string, read, total int64, current int) { | ||
| now := time.Now() | ||
|
|
Contributor
There was a problem hiding this comment.
I'd put this under the if m == nil check, since nothing happens with now before that return.
Contributor
Author
There was a problem hiding this comment.
Good call. I was wavering on this when writing the patch, and left it at the top because I had convinced myself that waiting until after the if would introduce imprecision in the averaging. While probably true to a degree, I'm sure that the effect is negligible, and the code would be more readable if the "first thing" that is supposed to happen came after the nullity check. Changed in cf8d827.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Nov 17, 2020
Since commit df2007d in PR git-lfs#646 it appears that the push_all_setup() helper function has accidentally short-circuited all but the first test calling it by exiting early with an exit code of 0 when the test repo exists. The calling tests thus appear to pass but are instead skipping all of their actual test logic. The original intent appears to have been to reuse a single test repo for the set of related tests of the --all push option. However, these tests do not leave the repo state in the same condition created by push_all_setup(), e.g., they replace the "origin" remote, and most importantly they each remove an LFS object file. Thus simply replacing "exit 0" with "return 0" in push_all_setup() is insufficient. To simplify matters, we no longer try to reuse the test repos between tests. Rather, we let push_all_setup() create a unique test repo for each caller by appending the distinct suffix argument provided by the caller to the name of the test repo. We then have to account for the fact that these tests have not fully run in some time, and therefore missed changes like commit db4e3b2 in PR git-lfs#2811 which updated the Git LFS progress meter output format. Several calls to "git push" also need adjustment to avoid errors such as "--all can't be combined with refspecs" and "tag shorthand without <tag>", and one short-circuited test had an incorrect count of "push ... => file1.dat" lines which we revise.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 10, 2024
In commit db4e3b2 of PR git-lfs#2811 in 2018 we updated the progress meter text of Git LFS from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of were changed at this time to check for the revised text strings in the output logs the capture from Git and Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it does not actually run to completion, due to a long-standing bug in the conditional used to check the TRAVIS variable. As described in git-lfs#5658, the test instead always exits early and declares success. We expect to address this problem in a subsequent commit in this PR, so we first update the test to check for the same progress meter text as all our other tests, which will help ensure it succeeds once it is fully enabled. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test also does not run to completion at the moment, for the same reason as the "cloneSSL" test.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 15, 2024
In commit db4e3b2 of PR git-lfs#2811 in 2018 we updated the progress meter text of Git LFS from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of were changed at this time to check for the revised text strings in the output logs the capture from Git and Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it does not actually run to completion, due to a long-standing bug in the conditional used to check the TRAVIS variable. As described in git-lfs#5658, the test instead always exits early and declares success. We expect to address this problem in a subsequent commit in this PR, so we first update the test to check for the same progress meter text as all our other tests, which will help ensure it succeeds once it is fully enabled. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test also does not run to completion at the moment, for the same reason as the "cloneSSL" test.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 7, 2024
In commit db4e3b2 of PR git-lfs#2811 in 2018 we updated the progress meter text of Git LFS from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of were changed at this time to check for the revised text strings in the output logs the capture from Git and Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it does not actually run to completion, due to a long-standing bug in the conditional used to check the TRAVIS variable. As described in git-lfs#5658, the test instead always exits early and declares success. We expect to address this problem in a subsequent commit in this PR, so we first update the test to check for the same progress meter text as all our other tests, which will help ensure it succeeds once it is fully enabled. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test also does not run to completion at the moment, for the same reason as the "cloneSSL" test.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 9, 2024
In commit db4e3b2 of PR git-lfs#2811 we updated the progress meter text of the Git LFS client from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of tests were changed in the same commit because they check the progress meter output in logs they capture from Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it has not actually run to completion for a long time, as described in git-lfs#5658, so it did not fail when the progress meter text changed. Due to a bug in the conditional used to check the TRAVIS variable, the test instead always exits early and declares success. We will address this problem in a subsequent commit in this PR, at which point the test will fail unless we adjust it to check for the same progress meter text as all our other tests, so we do that now. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests in the t/t-clone.sh script into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test does not run to completion at the moment either, for the same reason as the "cloneSSL" test.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 9, 2024
In commit db4e3b2 of PR git-lfs#2811 we updated the progress meter text of the Git LFS client from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of tests were changed in the same commit because they check the progress meter output in logs they capture from Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it has not actually run to completion for a long time, as described in git-lfs#5658, so it did not fail when the progress meter text changed. Due to a bug in the conditional used to check the TRAVIS variable, the test instead always exits early and declares success. We will address this problem in a subsequent commit in this PR, at which point the test will fail unless we adjust it to check for the same progress meter text as all our other tests, so we do that now. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests in the t/t-clone.sh script into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test does not run to completion at the moment either, for the same reason as the "cloneSSL" test.)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 29, 2024
In commit db4e3b2 of PR git-lfs#2811 we updated the progress meter text of the Git LFS client from "Git LFS" to the more explicit "Uploading LFS objects" or "Downloading LFS objects". A large number of tests were changed in the same commit because they check the progress meter output in logs they capture from Git LFS operations. However, the "cloneSSL" test in our t/t-clone.sh test script was not updated because it has not actually run to completion for a long time, as described in git-lfs#5658, so it did not fail when the progress meter text changed. Due to a bug in the conditional used to check the TRAVIS variable, the test instead always exits early and declares success. We will address this problem in a subsequent commit in this PR, at which point the test will fail unless we adjust it to check for the same progress meter text as all our other tests, so we do that now. We also take the opportunity to add a similar check of the same progress meter message to the "clone ClientCert" test. This brings our tests in the t/t-clone.sh script into slightly closer alignment with each other, which should help ensure they stay consistent in the future. (Note that this test does not run to completion at the moment either, for the same reason as the "cloneSSL" test.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adopts the recommendation for a new style of progress meter proposed by @bitinn and @technoweenie in #2802, and #2802 (comment), respectively:
The style that is implemented in this pull request is exactly as @technoweenie's recommendation above:
Here's a large overview of the changes:
Remove the notion of 'skipped' files. This term has been used to indicate files that are unnecessary to re-transfer, for they are already present on either the client (in the case of a fetch) or the server (in the case of a push). This term has unfortunately also been the source of some confusion by users. To remove this source of confusion, let's instead treat "skipped" files as already having been transferred, and instead of incrementing a "skipped" count, simply increment the number of files and bytes as already having been transferred, without affecting the true transfer speed.
Introduce a transfer rate indicator. To match the style of progress meter used by Git to indicate push/pull operations, introduce to Git LFS's progress meter the average rate at which the transfer is occurring.1 To keep track of this, each time we receive a read update, we first check if it has been sufficiently long enough since the last time we calculated an average (to avoid a false sense of precision) and, if so, recalculate the average rate of transfer by using a Cumulative Moving Average (C.M.A) function, so as to weight more heavily recent transfer speeds, so that they dominate the average.
The average is calculated as:
Where:
avgis the average rate of transfer (given as bytes/second)nis the number of samples takensis the number of bytes transferred since the last sample was takendis the duration of time that has passed since the last sampleI am hoping that this new formatting will clarify what is going on when LFS uploads and downloads objects. The majority of this pull request is changing outdated assertions in the integration suite to match the new formatting.
Closes: #2802.
1: This is actually something that I've been wanting to add to LFS for some time now, but haven't found the right time to do it. I have published this Gist around a year ago at the time of writing this pull request. That code was originally intended for something exactly like this.
/cc @git-lfs/core #2802