Re: [PATCH v2] fetch: show progress for packfile uri downloads
From: Jonathan Nieder <hidden>
Date: 2021-06-30 23:09:10
Hi, Albert Cui wrote:
Git appears to hang when downloading packfiles as this part of the fetch is silent, causing user confusion. This change implements progress for the number of packfiles downloaded; a progress display for bytes would involve deeper changes at the http-fetch layer instead of fetch-pack, the caller, so we do not do that in this patch. Signed-off-by: Albert Cui <redacted> --- fetch-pack.c | 8 ++++++++ t/t5702-protocol-v2.sh | 8 ++++++-- 2 files changed, 14 insertions(+), 2 deletions(-)
This is something that came up at the last in-person Git Contributor Summit; I'm glad to see it being taken care of.
quoted hunk ↗ jump to hunk
--- a/fetch-pack.c +++ b/fetch-pack.c@@ -23,6 +23,7 @@ #include "fetch-negotiator.h" #include "fsck.h" #include "shallow.h" +#include "progress.h" static int transfer_unpack_limit = -1; static int fetch_unpack_limit = -1;@@ -1585,6 +1586,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, struct fetch_negotiator *negotiator; int seen_ack = 0; struct string_list packfile_uris = STRING_LIST_INIT_DUP; + struct progress *packfile_uri_progress;
It seems to be more idiomatic to initialize this to NULL.
quoted hunk ↗ jump to hunk
int i; struct strvec index_pack_args = STRVEC_INIT; struct oidset gitmodules_oids = OIDSET_INIT;@@ -1689,6 +1691,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, } } + packfile_uri_progress = start_progress(_("Downloading packs"), packfile_uris.nr);
That way, we can respect a --quiet option by making this remain NULL when progress is not enabled: if (!args->quiet && !args->no_progress) packfile_uri_progress = ...; [...]
quoted hunk ↗ jump to hunk
--- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh@@ -848,10 +848,12 @@ test_expect_success 'part of packfile response provided as URI' ' configure_exclusion "$P" my-blob >h && configure_exclusion "$P" other-blob >h2 && - GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \ + GIT_PROGRESS_DELAY=0 GIT_TRACE=1 GIT_TRACE2_EVENT=1 \
This puts the trace in stderr mixed with other output. Would it make sense to put it in a separate file, like this? GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" \ GIT_PROGRESS_DELAY=0 GIT_TRACE2_EVENT="$(pwd)/trace2" \ GIT_TEST_SIDEBAND_ALL=1 \ git -c [etc] [...]
quoted hunk ↗ jump to hunk
@@ -875,6 +877,8 @@ test_expect_success 'part of packfile response provided as URI' ' test -f hfound && test -f h2found && + test_i18ngrep "Downloading packs" progress &&
That way, this "grep" could check the trace2 file which would contain output intended for machines, and we wouldn't have to worry e.g. about ANSII control codes potentially affecting the output around the space some day in the progress output intended for a terminal. With whatever subset of the changes described above make sense, this is Reviewed-by: Jonathan Nieder <redacted> Thanks.