Re: [PATCH v1] convert: display progress for filtered objects that have been delayed

From
Junio C Hamano <hidden>
Date
2017-08-24 19:41:01
Lars Schneider [off-list ref] writes:
In 2841e8f ("convert: add "status=delayed" to filter process protocol",
2017-06-30) we taught the filter process protocol to delayed responses.
These responses are processed after the "Checking out files" phase.
If the processing takes noticeable time, then the user might think Git
is stuck.

Display the progress of the delayed responses to let the user know that
Git is still processing objects. This works very well for objects that
can be filtered quickly. If filtering of an individual object takes
noticeable time, then the user might still think that Git is stuck.
However, in that case the user would at least know what Git is doing.

It would be technical more correct to display "Checking out files whose
content filtering has been delayed". For brevity we only print
"Filtering content".

The finish_delayed_checkout() call was moved below the stop_progress()
call in unpack-trees.c to ensure that the "Checking out files" progress
is properly stopped before the "Filtering content" progress starts in
finish_delayed_checkout().

Signed-off-by: Lars Schneider <redacted>
Suggested-by: Taylor Blau <redacted>
---
Makes sense.  The only thing that made me wonder was if we want the
change in unpack-trees.c in this patch.  After all, the procedure to
finish up the delayed checkout _is_ a part of the work need to be
done to populate the working tree files, so stopping the progress
before feels somewhat wrong at the phylosophical level.

I think our output cannot express nested progress bars, and I think
that is the reason why this patch tweaks unpack-trees.c; so I am
fine with the end result (and that is why I said "made me wonder
was", not "makes me wonder", the latter would imply "this we might
want fix before applying", but I do not think we want to change
anything this patch does to unpack-trees.c in this case).

The delayed progress API is being simplified so I'll probably do a
bit of evil merge while merging this to 'pu'.

Thanks.
quoted hunk
 entry.c        | 16 +++++++++++++++-
 unpack-trees.c |  2 +-
 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/entry.c b/entry.c
index 65458f07a4..1d1a09f47e 100644
--- a/entry.c
+++ b/entry.c
@@ -3,6 +3,7 @@
 #include "dir.h"
 #include "streaming.h"
 #include "submodule.h"
+#include "progress.h"
 
 static void create_directories(const char *path, int path_len,
 			       const struct checkout *state)
@@ -161,16 +162,23 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data)
 int finish_delayed_checkout(struct checkout *state)
 {
 	int errs = 0;
+	unsigned delayed_object_count;
+	off_t filtered_bytes = 0;
 	struct string_list_item *filter, *path;
+	struct progress *progress;
 	struct delayed_checkout *dco = state->delayed_checkout;
 
 	if (!state->delayed_checkout)
 		return errs;
 
 	dco->state = CE_RETRY;
+	delayed_object_count = dco->paths.nr;
+	progress = start_progress_delay(
+		_("Filtering content"), delayed_object_count, 50, 1);
 	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 */
@@ -216,11 +224,17 @@ int finish_delayed_checkout(struct checkout *state)
 				}
 				ce = index_file_exists(state->istate, path->string,
 						       strlen(path->string), 0);
-				errs |= (ce ? checkout_entry(ce, state, NULL) : 1);
+				if (ce) {
+					errs |= checkout_entry(ce, state, NULL);
+					filtered_bytes += ce->ce_stat_data.sd_size;
+					display_throughput(progress, filtered_bytes);
+				} else
+					errs = 1;
 			}
 		}
 		string_list_remove_empty_items(&dco->filters, 0);
 	}
+	stop_progress(&progress);
 	string_list_clear(&dco->filters, 0);
 
 	/* At this point we should not have any delayed paths anymore. */
diff --git a/unpack-trees.c b/unpack-trees.c
index 862cfce661..90fb270d77 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -395,8 +395,8 @@ static int check_updates(struct unpack_trees_options *o)
 			}
 		}
 	}
-	errs |= finish_delayed_checkout(&state);
 	stop_progress(&progress);
+	errs |= finish_delayed_checkout(&state);
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
 	return errs != 0;
base-commit: b3622a4ee94e4916cd05e6d96e41eeb36b941182
Thread (3 messages) 3 messages, 2 authors, last reply 2017-10-04
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help