Re: [PATCH v2] fetch: show progress for packfile uri downloads

[Junio C Hamano <gitster@pobox.com>]

"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