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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 30, 2021, 23:09 UTC
Message-ID: <YNz5kEs4ivfJdhP3@google.com>
In-Reply-To: <pull.907.v2.git.1618008249632.gitgitgadget@gmail.com>

Hi,

Albert Cui wrote:

Show 12 quoted lines

> 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.

Show 15 quoted lines

> --- 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.

Show 8 quoted lines

>  	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 = ...;

[...]

Show 8 quoted lines

> --- 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]

[...]

Show 5 quoted lines

> @@ -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.