| Message ID | pull.907.v2.git.1618008249632.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fetch: show progress for packfile uri downloads | expand |
"Albert Cui via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Albert Cui <albertqcui@gmail.com> > > 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. ... meaning, hopefully later we'd hook into transport->progress and implement the byte-level progress display down there? And when that happens, we'd remove this file-level progress as it would be too confusing to have both at the same time? Is this start_progress() call a way to unconditionally enable the progress display? How does it interact with transport->progress that is driven by transport_set_verbosity(), which in turn is called by builtin/fetch.c and friends? If it doesn't, shouldn't this codepath pay attention to the transport->progress and enable the progress meter only when it is enabled (i.e. the stderr going to a terminal, or --progress explicitly being asked)? > @@ -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; > 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); > + > for (i = 0; i < packfile_uris.nr; i++) { > int j; > struct child_process cmd = CHILD_PROCESS_INIT; > @@ -1696,6 +1700,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > const char *uri = packfile_uris.items[i].string + > the_hash_algo->hexsz + 1; > > + display_progress(packfile_uri_progress, i + 1); > strvec_push(&cmd.args, "http-fetch"); > strvec_pushf(&cmd.args, "--packfile=%.*s", > (int) the_hash_algo->hexsz, > @@ -1739,6 +1744,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > get_object_directory(), > packname)); > } > + > + stop_progress(&packfile_uri_progress); > + > string_list_clear(&packfile_uris, 0); > strvec_clear(&index_pack_args); > > diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh > index 2e1243ca40b0..0476b3f50455 100755 > --- 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 \ > + GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \ > git -c protocol.version=2 \ > -c fetch.uriprotocols=http,https \ > - clone "$HTTPD_URL/smart/http_parent" http_child && > + clone "$HTTPD_URL/smart/http_parent" http_child \ > + --progress 2>progress && > > # Ensure that my-blob and other-blob are in separate packfiles. > for idx in http_child/.git/objects/pack/*.idx > @@ -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 && > + > # Ensure that there are exactly 3 packfiles with associated .idx > ls http_child/.git/objects/pack/*.pack \ > http_child/.git/objects/pack/*.idx >filelist && > > base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f
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 <albertqcui@gmail.com> > --- > 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. > --- 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. > 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 = ...; [...] > --- 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] [...] > @@ -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 <jrnieder@gmail.com> Thanks.
diff --git a/fetch-pack.c b/fetch-pack.c index 6a61a464283e..7ca85f40cf23 100644 --- 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; 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); + for (i = 0; i < packfile_uris.nr; i++) { int j; struct child_process cmd = CHILD_PROCESS_INIT; @@ -1696,6 +1700,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, const char *uri = packfile_uris.items[i].string + the_hash_algo->hexsz + 1; + display_progress(packfile_uri_progress, i + 1); strvec_push(&cmd.args, "http-fetch"); strvec_pushf(&cmd.args, "--packfile=%.*s", (int) the_hash_algo->hexsz, @@ -1739,6 +1744,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, get_object_directory(), packname)); } + + stop_progress(&packfile_uri_progress); + string_list_clear(&packfile_uris, 0); strvec_clear(&index_pack_args); diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 2e1243ca40b0..0476b3f50455 100755 --- 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 \ + GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \ git -c protocol.version=2 \ -c fetch.uriprotocols=http,https \ - clone "$HTTPD_URL/smart/http_parent" http_child && + clone "$HTTPD_URL/smart/http_parent" http_child \ + --progress 2>progress && # Ensure that my-blob and other-blob are in separate packfiles. for idx in http_child/.git/objects/pack/*.idx @@ -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 && + # Ensure that there are exactly 3 packfiles with associated .idx ls http_child/.git/objects/pack/*.pack \ http_child/.git/objects/pack/*.idx >filelist &&