[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2] fetch: show progress for packfile uri downloads
From:       Jonathan Nieder <jrnieder () gmail ! com>
Date:       2021-06-30 23:09:04
Message-ID: YNz5kEs4ivfJdhP3 () google ! com
[Download RAW message or body]

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.
[prev in list] [next in list] [prev in thread] [next in thread]