On 05/04/15 at 02:47pm, Allan McRae wrote:

On 23/04/15 06:27, David Macek wrote:

...

Hello everyone. I see a problem with --noprogressbar output, but it seems to be dependent on server/network. Can anyone confirm with the following settings?

# append to pacman.conf [mingw64] Server = ftp://ftp.heanet.ie/mirrors/download.sourceforge.net/pub/sourceforge/m/ms/msys2/REPOS/MINGW/x86_64

Don't let Dave see you using ftp!

...

$ pacman -Syy --noprogressbar

My output contains multiple "downloading" lines for this DB:

downloading mingw64.db... downloading mingw64.db... downloading mingw64.db... downloading mingw64.db... downloading mingw64.db...

allan@arya ~ $ pacman -Syy --noprogressbar :: Synchronizing package databases... downloading staging.db... downloading testing.db... downloading core.db... downloading extra.db... downloading extra.db... downloading extra.db... downloading extra.db... downloading extra.db... downloading community-testing.db... downloading community.db... downloading multilib-staging.db... downloading multilib-testing.db... downloading multilib.db...

I can replicate using the Arch dev server (which is really slow from here).

...

It seems that the cause is libcurl calling the progress callback multiple times before the first bytes get downloaded, which then in turns `cb_dl_progress` multiple times with `file_xfered` = 0, which then prints the message each time.

I don't want to just complain, so I whipped up a quick patch. I can submit it properly if it gets an upvote.

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 695e38d..3bc43b7 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -628,6 +628,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) static double rate_last; static off_t xfered_last; static int64_t initial_time = 0; + static char *filename_last = NULL; int infolen; int filenamelen; char *fname, *p; @@ -646,7 +647,11 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) const unsigned short cols = getcols();

if(config->noprogressbar || cols == 0 || file_total == -1) { - if(file_xfered == 0) { + if(filename_last != NULL || strcmp(filename_last, filename) != 0) { + if(filename_last != NULL) { + free(filename_last); + } + filename_last = strdup(filename); printf(_("downloading %s...\n"), filename); fflush(stdout); }

This seems a reasonable solution.

I don't think this is the right solution. The comments in dload_progress_cb suggest that the callback is not intended to be called until we have actually downloaded something. So, file_xfered should always equal 0 exactly once per downloaded file. I think it makes more sense to fix dload_progress_cb. We might also consider using the event callback rather than the download callback when noprogressbar is set. apg

On 05/06/15 at 12:32am, David Macek wrote:

On 4. 5. 2015 14:55, Andrew Gregory wrote:

...

I don't think this is the right solution. The comments in dload_progress_cb suggest that the callback is not intended to be called until we have actually downloaded something. So, file_xfered should always equal 0 exactly once per downloaded file. I think it makes more sense to fix dload_progress_cb. We might also consider using the event callback rather than the download callback when noprogressbar is set.

Okay. I was thinking and came up with another idea, along the lines of this incomplete patch:

--- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -127,15 +127,17 @@ static int dload_progress_cb(void *file, double dltotal, double dlnow,

/* initialize the progress bar here to avoid displaying it when * a repo is up to date and nothing gets downloaded */ - if(payload->prevprogress == 0) { + if(payload->prevprogress == -1) { payload->handle->dlcb(payload->remote_name, 0, (off_t)dltotal); + payload->prevprogress = payload->initial_size; }

/* do NOT include initial_size since it wasn't part of the package's * download_size (nor included in the total download size callback) */ - payload->handle->dlcb(payload->remote_name, (off_t)dlnow, (off_t)dltotal); - - payload->prevprogress = current_size; + if(payload->prevprogress != current_size) { + payload->handle->dlcb(payload->remote_name, (off_t)dlnow, (off_t)dltotal); + payload->prevprogress = current_size; + }

return 0; }

If putting -1 into prevprogress is undesirable, can we introduce a new field into the dload_payload type? (If introducing a new field is a big deal for libalpm clients, maybe we can future-proof the struct by adding a `struct dload_payload_internal *_alpm_internal` field instead and keeping `struct dload_payload_internal` unexported.)

There's an easier solution: just check for dlnow > 0 at the same time we check for the existence of dlcb. apg