On Fri, Jul 23, 2021 at 03:01:48PM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
> > From: SZEDER Gábor <szeder.dev@xxxxxxxxx>
> >
> > The "Filtering content" progress in entry.c:finish_delayed_checkout()
> > is unusual because of how it calculates the progress count and because
> > it shows the progress of a nested loop. It works basically like this:
> >
> > start_delayed_progress(p, nr_of_paths_to_filter)
> > for_each_filter {
> > display_progress(p, nr_of_paths_to_filter - nr_of_paths_still_left_to_filter)
> > for_each_path_handled_by_the_current_filter {
> > checkout_entry()
> > }
> > }
> > stop_progress(p)
> >
> > There are two issues with this approach:
> >
> > - The work done by the last filter (or the only filter if there is
> > only one) is never counted, so if the last filter still has some
> > paths to process, then the counter shown in the "done" progress
> > line will not match the expected total.
> >
> > This would cause a BUG() in an upcoming change that adds an
> > assertion checking if the "total" at the end matches the last
> > progress bar update..
>
> So the other series will semantically depend on this 3-patch series?
> Just making sure that is the intended topic structure.
>
> > diff --git a/entry.c b/entry.c
> > index 125fabdbd5..d92dd020b3 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -162,7 +162,7 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)
> > int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)
> > {
> > int errs = 0;
> > - unsigned delayed_object_count;
> > + unsigned processed_paths = 0;
> > off_t filtered_bytes = 0;
> > struct string_list_item *filter, *path;
> > struct progress *progress;
> > @@ -172,12 +172,10 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)
> > return errs;
> >
> > dco->state = CE_RETRY;
> > - delayed_object_count = dco->paths.nr;
> > - progress = start_delayed_progress(_("Filtering content"), delayed_object_count);
> > + progress = start_delayed_progress(_("Filtering content"), dco->paths.nr);
> > while (dco->filters.nr > 0) {
> > for_each_string_list_item(filter, &dco->filters) {
> > struct string_list available_paths = STRING_LIST_INIT_NODUP;
> > - display_progress(progress, delayed_object_count - dco->paths.nr);
> >
> > if (!async_query_available_blobs(filter->string, &available_paths)) {
> > /* Filter reported an error */
> > @@ -224,6 +222,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts)
> > ce = index_file_exists(state->istate, path->string,
> > strlen(path->string), 0);
> > if (ce) {
> > + display_progress(progress, ++processed_paths);
> > errs |= checkout_entry(ce, state, NULL, nr_checkouts);
> > filtered_bytes += ce->ce_stat_data.sd_size;
> > display_throughput(progress, filtered_bytes);
>
> Hmph. A missing cache entries will not increment processed; would
> that cause stop_progress() to see at the end the counter that is
> smaller than dco->paths.nr we saw at the beginning?
Yes, but this 'if (ce)' condition has an 'else errs = 1;' branch as
well, i.e. a missing cache entry is considered an error. This patch
fixes issues with this progress bar in the case when all went well,
i.e. when there were no errors. In my original submission it is
followed up by another patch that attempts to fix this progress line
when there are errors, arguing that it's wrong to show "done" at the
end when not all work was done because of said errors... although
"fix" is not quite the right word for the approach taken in that
patch, it's more like "papering over" ;)
https://public-inbox.org/git/20210620200303.2328957-7-szeder.dev@xxxxxxxxx/
