Subject: Re: [PATCH 3/4] remote-curl: optionally show progress for HTTP get

From: Junio C Hamano <[email-masked]>
Date: Wed, 08 May 2024 15:29:20 -0700

Toon Claes <[email-masked]> writes:

> git-remote-curl supports the `option progress` basically since it's

"it's" -> "its".

> inception. But this option had no effect for regular HTTP(S) downloads.
>
> Add progress indicator when downloading files through curl HTTP GET.
>
> Signed-off-by: Toon Claes <[email-masked]>
> ---
>  remote-curl.c       |  8 +++++++-
>  t/t5557-http-get.sh | 15 +++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 0b6d7815fd..9fc7c3580c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1293,6 +1293,7 @@ static void parse_get(const char *arg)
>  {
>  	struct strbuf url = STRBUF_INIT;
>  	struct strbuf path = STRBUF_INIT;
> +	struct http_get_options http_options = {0};
>  	const char *space;
>  
>  	space = strchr(arg, ' ');
> @@ -1303,7 +1304,12 @@ static void parse_get(const char *arg)
>  	strbuf_add(&url, arg, space - arg);
>  	strbuf_addstr(&path, space + 1);
>  
> -	if (http_get_file(url.buf, path.buf, NULL))
> +	http_options.initial_request = 1;
> +
> +	if (options.progress)
> +		http_options.progress = 1;
> +
> +	if (http_get_file(url.buf, path.buf, &http_options))
>  		die(_("failed to download file at URL '%s'"), url.buf);
>  
>  	strbuf_release(&url);
> diff --git a/t/t5557-http-get.sh b/t/t5557-http-get.sh
> index 76a4bbd16a..92a138caaf 100755
> --- a/t/t5557-http-get.sh
> +++ b/t/t5557-http-get.sh
> @@ -36,4 +36,19 @@ test_expect_success 'get by URL: 200' '
>  	test_cmp "$HTTPD_DOCUMENT_ROOT_PATH/exists.txt" file2
>  '
>  
> +test_expect_success 'get by URL with progress' '
> +	echo hello >"$HTTPD_DOCUMENT_ROOT_PATH/hello.txt" &&
> +
> +	url="$HTTPD_URL/hello.txt" &&
> +	cat >input <<-EOF &&
> +	capabilities
> +	option progress true
> +	get $url file3
> +
> +	EOF
> +
> +	git remote-http $url <input 2>err &&
> +        test_grep "^Downloading via HTTP: 100%" err
> +'

Are we sure that by the time we finish the transfer we know the
total?  Does this rely on the fact that somehow we are in
control of the server and can force it to use content-length:
(instead of chunked encoding without any explicit indication of the
total length)?

Also, I am curious if we know how often the progress indicator is
updated by cURL library with the API used in [2/4] (and if we can
control the frequency if we wanted to).  Note that this is me just
being curious, not a request to make it tweakable.  The findings may
help polish the [2/4], though.

Thanks.

> +
>  test_done