[PATCH 0/5] more progress display stuff

[PATCH 0/5] more progress display stuff

[Nicolas Pitre]

This is the result of some changes and cleanup I did to nicely support
progress with throughput display, and avoid those random crashes the
previous patches introduced.  All tests are OK now after every patch.

This is meant to replace my latest 2 patches currently in pu.


Nicolas

[PATCH 1/5] prune-packed: don't call display_progress() for every file

[Nicolas Pitre]

The progress count is per fanout directory, so it is useless to call
it for every file as the count doesn't change that often.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-prune-packed.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index 015c8bb..907e368 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -15,6 +15,9 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 	struct dirent *de;
 	char hex[40];
 
+	if (opts == VERBOSE)
+		display_progress(&progress, i + 1);
+
 	sprintf(hex, "%02x", i);
 	while ((de = readdir(dir)) != NULL) {
 		unsigned char sha1[20];
@@ -26,8 +29,6 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 		if (!has_sha1_pack(sha1, NULL))
 			continue;
 		memcpy(pathname + len, de->d_name, 38);
-		if (opts == VERBOSE)
-			display_progress(&progress, i + 1);
 		if (opts & DRY_RUN)
 			printf("rm -f %s\n", pathname);
 		else if (unlink(pathname) < 0)
-- 
1.5.3.4.1463.gf79ad2

[PATCH 2/5] make struct progress an opaque type

[Nicolas Pitre]

This allows for better management of progress "object" existence,
as well as making the progress display implementation more independent
from its callers.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c   |   16 ++++++++--------
 builtin-prune-packed.c   |    7 +++----
 builtin-unpack-objects.c |    6 +++---
 index-pack.c             |   12 ++++++------
 progress.c               |   33 +++++++++++++++++++++++++++------
 progress.h               |   18 +++++-------------
 unpack-trees.c           |   10 +++++-----
 7 files changed, 57 insertions(+), 45 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 21ba977..3ca5cc7 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -73,7 +73,7 @@ static int depth = 50;
 static int delta_search_threads = 1;
 static int pack_to_stdout;
 static int num_preferred_base;
-static struct progress progress_state;
+static struct progress *progress_state;
 static int pack_compression_level = Z_DEFAULT_COMPRESSION;
 static int pack_compression_seen;
 
@@ -598,7 +598,7 @@ static void write_pack_file(void)
 	uint32_t nr_remaining = nr_result;
 
 	if (do_progress)
-		start_progress(&progress_state, "Writing objects", nr_result);
+		progress_state = start_progress("Writing objects", nr_result);
 	written_list = xmalloc(nr_objects * sizeof(struct object_entry *));
 
 	do {
@@ -630,7 +630,7 @@ static void write_pack_file(void)
 				break;
 			offset = offset_one;
 			if (do_progress)
-				display_progress(&progress_state, written);
+				display_progress(progress_state, written);
 		}
 
 		/*
@@ -854,7 +854,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 		object_ix[-1 - ix] = nr_objects;
 
 	if (progress)
-		display_progress(&progress_state, nr_objects);
+		display_progress(progress_state, nr_objects);
 
 	if (name && no_try_delta(name))
 		entry->no_try_delta = 1;
@@ -1518,7 +1518,7 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
 		progress_lock();
 		(*processed)++;
 		if (progress)
-			display_progress(&progress_state, *processed);
+			display_progress(progress_state, *processed);
 		progress_unlock();
 
 		/*
@@ -1718,8 +1718,8 @@ static void prepare_pack(int window, int depth)
 	if (nr_deltas && n > 1) {
 		unsigned nr_done = 0;
 		if (progress)
-			start_progress(&progress_state, "Compressing objects",
-					nr_deltas);
+			progress_state = start_progress("Compressing objects",
+							nr_deltas);
 		qsort(delta_list, n, sizeof(*delta_list), type_size_sort);
 		ll_find_deltas(delta_list, n, window+1, depth, &nr_done);
 		if (progress)
@@ -2135,7 +2135,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	prepare_packed_git();
 
 	if (progress)
-		start_progress(&progress_state, "Counting objects", 0);
+		progress_state = start_progress("Counting objects", 0);
 	if (!use_internal_rev_list)
 		read_object_list_from_stdin();
 	else {
diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index 907e368..c66fb03 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -8,7 +8,7 @@ static const char prune_packed_usage[] =
 #define DRY_RUN 01
 #define VERBOSE 02
 
-static struct progress progress;
+static struct progress *progress;
 
 static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 {
@@ -16,7 +16,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 	char hex[40];
 
 	if (opts == VERBOSE)
-		display_progress(&progress, i + 1);
+		display_progress(progress, i + 1);
 
 	sprintf(hex, "%02x", i);
 	while ((de = readdir(dir)) != NULL) {
@@ -46,8 +46,7 @@ void prune_packed_objects(int opts)
 	int len = strlen(dir);
 
 	if (opts == VERBOSE)
-		start_progress_delay(&progress,
-			"Removing duplicate objects",
+		progress = start_progress_delay("Removing duplicate objects",
 			256, 95, 2);
 
 	if (len > PATH_MAX - 42)
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 2317b8f..e0ecbc5 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -311,7 +311,7 @@ static void unpack_one(unsigned nr)
 static void unpack_all(void)
 {
 	int i;
-	struct progress progress;
+	struct progress *progress = NULL;
 	struct pack_header *hdr = fill(sizeof(struct pack_header));
 	unsigned nr_objects = ntohl(hdr->hdr_entries);
 
@@ -322,12 +322,12 @@ static void unpack_all(void)
 	use(sizeof(struct pack_header));
 
 	if (!quiet)
-		start_progress(&progress, "Unpacking objects", nr_objects);
+		progress = start_progress("Unpacking objects", nr_objects);
 	obj_list = xmalloc(nr_objects * sizeof(*obj_list));
 	for (i = 0; i < nr_objects; i++) {
 		unpack_one(i);
 		if (!quiet)
-			display_progress(&progress, i + 1);
+			display_progress(progress, i + 1);
 	}
 	if (!quiet)
 		stop_progress(&progress);
diff --git a/index-pack.c b/index-pack.c
index 2f149a4..b4543a4 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -46,7 +46,7 @@ static int nr_resolved_deltas;
 static int from_stdin;
 static int verbose;
 
-static struct progress progress;
+static struct progress *progress;
 
 /* We always read in 4kB chunks. */
 static unsigned char input_buffer[4096];
@@ -406,7 +406,7 @@ static void parse_pack_objects(unsigned char *sha1)
 	 * - remember base (SHA1 or offset) for all deltas.
 	 */
 	if (verbose)
-		start_progress(&progress, "Indexing objects", nr_objects);
+		progress = start_progress("Indexing objects", nr_objects);
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
 		data = unpack_raw_entry(obj, &delta->base);
@@ -419,7 +419,7 @@ static void parse_pack_objects(unsigned char *sha1)
 			sha1_object(data, obj->size, obj->type, obj->idx.sha1);
 		free(data);
 		if (verbose)
-			display_progress(&progress, i+1);
+			display_progress(progress, i+1);
 	}
 	objects[i].idx.offset = consumed_bytes;
 	if (verbose)
@@ -455,7 +455,7 @@ static void parse_pack_objects(unsigned char *sha1)
 	 *   for some more deltas.
 	 */
 	if (verbose)
-		start_progress(&progress, "Resolving deltas", nr_deltas);
+		progress = start_progress("Resolving deltas", nr_deltas);
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
 		union delta_base base;
@@ -487,7 +487,7 @@ static void parse_pack_objects(unsigned char *sha1)
 			}
 		free(data);
 		if (verbose)
-			display_progress(&progress, nr_resolved_deltas);
+			display_progress(progress, nr_resolved_deltas);
 	}
 }
 
@@ -595,7 +595,7 @@ static void fix_unresolved_deltas(int nr_unresolved)
 		append_obj_to_pack(d->base.sha1, data, size, type);
 		free(data);
 		if (verbose)
-			display_progress(&progress, nr_resolved_deltas);
+			display_progress(progress, nr_resolved_deltas);
 	}
 	free(sorted_by_pos);
 }
diff --git a/progress.c b/progress.c
index 7629e05..7f9b00e 100644
--- a/progress.c
+++ b/progress.c
@@ -1,6 +1,15 @@
 #include "git-compat-util.h"
 #include "progress.h"
 
+struct progress {
+	const char *title;
+	int last_value;
+	unsigned total;
+	unsigned last_percent;
+	unsigned delay;
+	unsigned delayed_percent_treshold;
+};
+
 static volatile sig_atomic_t progress_update;
 
 static void progress_interval(int signum)
@@ -76,12 +85,18 @@ static int display(struct progress *progress, unsigned n, int done)
 
 int display_progress(struct progress *progress, unsigned n)
 {
-	return display(progress, n, 0);
+	return progress ? display(progress, n, 0) : 0;
 }
 
-void start_progress_delay(struct progress *progress, const char *title,
-			  unsigned total, unsigned percent_treshold, unsigned delay)
+struct progress * start_progress_delay(const char *title, unsigned total,
+				       unsigned percent_treshold, unsigned delay)
 {
+	struct progress *progress = malloc(sizeof(*progress));
+	if (!progress) {
+		/* unlikely, but here's a good fallback */
+		fprintf(stderr, "%s...\n", title);
+		return NULL;
+	}
 	progress->title = title;
 	progress->total = total;
 	progress->last_value = -1;
@@ -89,19 +104,25 @@ void start_progress_delay(struct progress *progress, const char *title,
 	progress->delayed_percent_treshold = percent_treshold;
 	progress->delay = delay;
 	set_progress_signal();
+	return progress;
 }
 
-void start_progress(struct progress *progress, const char *title, unsigned total)
+struct progress * start_progress(const char *title, unsigned total)
 {
-	start_progress_delay(progress, title, total, 0, 0);
+	return start_progress_delay(title, total, 0, 0);
 }
 
-void stop_progress(struct progress *progress)
+void stop_progress(struct progress **p_progress)
 {
+	struct progress *progress = *p_progress;
+	if (!progress)
+		return;
+	*p_progress = NULL;
 	if (progress->last_value != -1) {
 		/* Force the last update */
 		progress_update = 1;
 		display(progress, progress->last_value, 1);
 	}
 	clear_progress_signal();
+	free(progress);
 }
diff --git a/progress.h b/progress.h
index 07b56bd..29780ce 100644
--- a/progress.h
+++ b/progress.h
@@ -1,20 +1,12 @@
 #ifndef PROGRESS_H
 #define PROGRESS_H
 
-struct progress {
-	const char *title;
-	int last_value;
-	unsigned total;
-	unsigned last_percent;
-	unsigned delay;
-	unsigned delayed_percent_treshold;
-};
+struct progress;
 
 int display_progress(struct progress *progress, unsigned n);
-void start_progress(struct progress *progress, const char *title,
-		    unsigned total);
-void start_progress_delay(struct progress *progress, const char *title,
-			  unsigned total, unsigned percent_treshold, unsigned delay);
-void stop_progress(struct progress *progress);
+struct progress * start_progress(const char *title, unsigned total);
+struct progress * start_progress_delay(const char *title, unsigned total,
+				       unsigned percent_treshold, unsigned delay);
+void stop_progress(struct progress **progress);
 
 #endif
diff --git a/unpack-trees.c b/unpack-trees.c
index 3225101..6776c29 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -297,7 +297,7 @@ static void check_updates(struct cache_entry **src, int nr,
 {
 	unsigned short mask = htons(CE_UPDATE);
 	unsigned cnt = 0, total = 0;
-	struct progress progress;
+	struct progress *progress = NULL;
 	char last_symlink[PATH_MAX];
 
 	if (o->update && o->verbose_update) {
@@ -307,8 +307,8 @@ static void check_updates(struct cache_entry **src, int nr,
 				total++;
 		}
 
-		start_progress_delay(&progress, "Checking out files",
-				     total, 50, 2);
+		progress = start_progress_delay("Checking out files",
+						total, 50, 2);
 		cnt = 0;
 	}
 
@@ -318,7 +318,7 @@ static void check_updates(struct cache_entry **src, int nr,
 
 		if (total)
 			if (!ce->ce_mode || ce->ce_flags & mask)
-				display_progress(&progress, ++cnt);
+				display_progress(progress, ++cnt);
 		if (!ce->ce_mode) {
 			if (o->update)
 				unlink_entry(ce->name, last_symlink);
@@ -333,7 +333,7 @@ static void check_updates(struct cache_entry **src, int nr,
 		}
 	}
 	if (total)
-		stop_progress(&progress);;
+		stop_progress(&progress);
 }
 
 int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
-- 
1.5.3.4.1463.gf79ad2

[PATCH 3/5] relax usage of the progress API

[Nicolas Pitre]

Since it is now OK to pass a null pointer to display_progress() and
stop_progress() resulting in a no-op, then we can simplify the code
and remove a bunch of lines by not making those calls conditional all
the time.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c   |   18 ++++++------------
 builtin-prune-packed.c   |    6 ++----
 builtin-unpack-objects.c |    6 ++----
 index-pack.c             |   20 +++++++-------------
 unpack-trees.c           |    8 +++-----
 5 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 3ca5cc7..52a26a2 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -629,8 +629,7 @@ static void write_pack_file(void)
 			if (!offset_one)
 				break;
 			offset = offset_one;
-			if (do_progress)
-				display_progress(progress_state, written);
+			display_progress(progress_state, written);
 		}
 
 		/*
@@ -684,8 +683,7 @@ static void write_pack_file(void)
 	} while (nr_remaining && i < nr_objects);
 
 	free(written_list);
-	if (do_progress)
-		stop_progress(&progress_state);
+	stop_progress(&progress_state);
 	if (written != nr_result)
 		die("wrote %u objects while expecting %u", written, nr_result);
 	/*
@@ -853,8 +851,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
 	else
 		object_ix[-1 - ix] = nr_objects;
 
-	if (progress)
-		display_progress(progress_state, nr_objects);
+	display_progress(progress_state, nr_objects);
 
 	if (name && no_try_delta(name))
 		entry->no_try_delta = 1;
@@ -1517,8 +1514,7 @@ static void find_deltas(struct object_entry **list, unsigned list_size,
 
 		progress_lock();
 		(*processed)++;
-		if (progress)
-			display_progress(progress_state, *processed);
+		display_progress(progress_state, *processed);
 		progress_unlock();
 
 		/*
@@ -1722,8 +1718,7 @@ static void prepare_pack(int window, int depth)
 							nr_deltas);
 		qsort(delta_list, n, sizeof(*delta_list), type_size_sort);
 		ll_find_deltas(delta_list, n, window+1, depth, &nr_done);
-		if (progress)
-			stop_progress(&progress_state);
+		stop_progress(&progress_state);
 		if (nr_done != nr_deltas)
 			die("inconsistency with delta count");
 	}
@@ -2142,8 +2137,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		rp_av[rp_ac] = NULL;
 		get_object_list(rp_ac, rp_av);
 	}
-	if (progress)
-		stop_progress(&progress_state);
+	stop_progress(&progress_state);
 
 	if (non_empty && !nr_result)
 		return 0;
diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index c66fb03..f4287da 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -15,8 +15,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 	struct dirent *de;
 	char hex[40];
 
-	if (opts == VERBOSE)
-		display_progress(progress, i + 1);
+	display_progress(progress, i + 1);
 
 	sprintf(hex, "%02x", i);
 	while ((de = readdir(dir)) != NULL) {
@@ -64,8 +63,7 @@ void prune_packed_objects(int opts)
 		prune_dir(i, d, pathname, len + 3, opts);
 		closedir(d);
 	}
-	if (opts == VERBOSE)
-		stop_progress(&progress);
+	stop_progress(&progress);
 }
 
 int cmd_prune_packed(int argc, const char **argv, const char *prefix)
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index e0ecbc5..1e51865 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -326,11 +326,9 @@ static void unpack_all(void)
 	obj_list = xmalloc(nr_objects * sizeof(*obj_list));
 	for (i = 0; i < nr_objects; i++) {
 		unpack_one(i);
-		if (!quiet)
-			display_progress(progress, i + 1);
+		display_progress(progress, i + 1);
 	}
-	if (!quiet)
-		stop_progress(&progress);
+	stop_progress(&progress);
 
 	if (delta_list)
 		die("unresolved deltas left after unpacking");
diff --git a/index-pack.c b/index-pack.c
index b4543a4..879ea15 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -418,12 +418,10 @@ static void parse_pack_objects(unsigned char *sha1)
 		} else
 			sha1_object(data, obj->size, obj->type, obj->idx.sha1);
 		free(data);
-		if (verbose)
-			display_progress(progress, i+1);
+		display_progress(progress, i+1);
 	}
 	objects[i].idx.offset = consumed_bytes;
-	if (verbose)
-		stop_progress(&progress);
+	stop_progress(&progress);
 
 	/* Check pack integrity */
 	flush();
@@ -486,8 +484,7 @@ static void parse_pack_objects(unsigned char *sha1)
 						      obj->size, obj->type);
 			}
 		free(data);
-		if (verbose)
-			display_progress(progress, nr_resolved_deltas);
+		display_progress(progress, nr_resolved_deltas);
 	}
 }
 
@@ -594,8 +591,7 @@ static void fix_unresolved_deltas(int nr_unresolved)
 			die("local object %s is corrupt", sha1_to_hex(d->base.sha1));
 		append_obj_to_pack(d->base.sha1, data, size, type);
 		free(data);
-		if (verbose)
-			display_progress(progress, nr_resolved_deltas);
+		display_progress(progress, nr_resolved_deltas);
 	}
 	free(sorted_by_pos);
 }
@@ -774,8 +770,7 @@ int main(int argc, char **argv)
 	deltas = xmalloc(nr_objects * sizeof(struct delta_entry));
 	parse_pack_objects(sha1);
 	if (nr_deltas == nr_resolved_deltas) {
-		if (verbose)
-			stop_progress(&progress);
+		stop_progress(&progress);
 		/* Flush remaining pack final 20-byte SHA1. */
 		flush();
 	} else {
@@ -788,11 +783,10 @@ int main(int argc, char **argv)
 					   (nr_objects + nr_unresolved + 1)
 					   * sizeof(*objects));
 			fix_unresolved_deltas(nr_unresolved);
-			if (verbose) {
-				stop_progress(&progress);
+			stop_progress(&progress);
+			if (verbose)
 				fprintf(stderr, "%d objects were added to complete this thin pack.\n",
 					nr_objects - nr_objects_initial);
-			}
 			fixup_pack_header_footer(output_fd, sha1,
 				curr_pack, nr_objects);
 		}
diff --git a/unpack-trees.c b/unpack-trees.c
index 6776c29..c527d7d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -316,9 +316,8 @@ static void check_updates(struct cache_entry **src, int nr,
 	while (nr--) {
 		struct cache_entry *ce = *src++;
 
-		if (total)
-			if (!ce->ce_mode || ce->ce_flags & mask)
-				display_progress(progress, ++cnt);
+		if (!ce->ce_mode || ce->ce_flags & mask)
+			display_progress(progress, ++cnt);
 		if (!ce->ce_mode) {
 			if (o->update)
 				unlink_entry(ce->name, last_symlink);
@@ -332,8 +331,7 @@ static void check_updates(struct cache_entry **src, int nr,
 			}
 		}
 	}
-	if (total)
-		stop_progress(&progress);
+	stop_progress(&progress);
 }
 
 int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
-- 
1.5.3.4.1463.gf79ad2

[PATCH 4/5] add throughput to progress display

[Nicolas Pitre]

This adds the ability for the progress code to also display transfer
throughput when that makes sense.

The math was inspired by commit c548cf4ee0737a321ffe94f6a97c65baf87281be
from Linus.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 progress.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 progress.h |    1 +
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/progress.c b/progress.c
index 7f9b00e..23ee9f3 100644
--- a/progress.c
+++ b/progress.c
@@ -1,6 +1,19 @@
 #include "git-compat-util.h"
 #include "progress.h"
 
+#define TP_IDX_MAX      8
+
+struct throughput {
+	struct timeval prev_tv;
+	unsigned long count;
+	unsigned long avg_bytes;
+	unsigned long last_bytes[TP_IDX_MAX];
+	unsigned int avg_misecs;
+	unsigned int last_misecs[TP_IDX_MAX];
+	unsigned int idx;
+	char display[20];
+};
+
 struct progress {
 	const char *title;
 	int last_value;
@@ -8,6 +21,7 @@ struct progress {
 	unsigned last_percent;
 	unsigned delay;
 	unsigned delayed_percent_treshold;
+	struct throughput *throughput;
 };
 
 static volatile sig_atomic_t progress_update;
@@ -46,7 +60,7 @@ static void clear_progress_signal(void)
 
 static int display(struct progress *progress, unsigned n, int done)
 {
-	char *eol;
+	char *eol, *tp;
 
 	if (progress->delay) {
 		if (!progress_update || --progress->delay)
@@ -64,18 +78,20 @@ static int display(struct progress *progress, unsigned n, int done)
 	}
 
 	progress->last_value = n;
+	tp = (progress->throughput) ? progress->throughput->display : "";
 	eol = done ? ", done.   \n" : "   \r";
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%u/%u)%s", progress->title,
-				percent, n, progress->total, eol);
+			fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
+				progress->title, percent, n,
+				progress->total, tp, eol);
 			progress_update = 0;
 			return 1;
 		}
 	} else if (progress_update) {
-		fprintf(stderr, "%s: %u%s", progress->title, n, eol);
+		fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol);
 		progress_update = 0;
 		return 1;
 	}
@@ -83,6 +99,60 @@ static int display(struct progress *progress, unsigned n, int done)
 	return 0;
 }
 
+void display_throughput(struct progress *progress, unsigned long n)
+{
+	struct throughput *tp;
+	struct timeval tv;
+	unsigned int misecs;
+
+	if (!progress)
+		return;
+	tp = progress->throughput;
+
+	gettimeofday(&tv, NULL);
+
+	if (!tp) {
+		progress->throughput = tp = calloc(1, sizeof(*tp));
+		if (tp)
+			tp->prev_tv = tv;
+		return;
+	}
+
+	tp->count += n;
+
+	/*
+	 * We have x = bytes and y = microsecs.  We want z = KiB/s:
+	 *
+	 *	z = (x / 1024) / (y / 1000000)
+	 *	z = x / y * 1000000 / 1024
+	 *	z = x / (y * 1024 / 1000000)
+	 *	z = x / y'
+	 *
+	 * To simplify things we'll keep track of misecs, or 1024th of a sec
+	 * obtained with:
+	 *
+	 *	y' = y * 1024 / 1000000
+	 *	y' = y / (1000000 / 1024)
+	 *	y' = y / 977
+	 */
+	misecs = (tv.tv_sec - tp->prev_tv.tv_sec) * 1024;
+	misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
+
+	if (misecs > 512) {
+		tp->prev_tv = tv;
+		tp->avg_bytes += tp->count;
+		tp->avg_misecs += misecs;
+		snprintf(tp->display, sizeof(tp->display),
+			 ", %lu KiB/s", tp->avg_bytes / tp->avg_misecs);
+		tp->avg_bytes -= tp->last_bytes[tp->idx];
+		tp->avg_misecs -= tp->last_misecs[tp->idx];
+		tp->last_bytes[tp->idx] = tp->count;
+		tp->last_misecs[tp->idx] = misecs;
+		tp->idx = (tp->idx + 1) % TP_IDX_MAX;
+		tp->count = 0;
+	}
+}
+
 int display_progress(struct progress *progress, unsigned n)
 {
 	return progress ? display(progress, n, 0) : 0;
@@ -103,6 +173,7 @@ struct progress * start_progress_delay(const char *title, unsigned total,
 	progress->last_percent = -1;
 	progress->delayed_percent_treshold = percent_treshold;
 	progress->delay = delay;
+	progress->throughput = NULL;
 	set_progress_signal();
 	return progress;
 }
@@ -124,5 +195,6 @@ void stop_progress(struct progress **p_progress)
 		display(progress, progress->last_value, 1);
 	}
 	clear_progress_signal();
+	free(progress->throughput);
 	free(progress);
 }
diff --git a/progress.h b/progress.h
index 29780ce..7902ca5 100644
--- a/progress.h
+++ b/progress.h
@@ -3,6 +3,7 @@
 
 struct progress;
 
+void display_throughput(struct progress *progress, unsigned long n);
 int display_progress(struct progress *progress, unsigned n);
 struct progress * start_progress(const char *title, unsigned total);
 struct progress * start_progress_delay(const char *title, unsigned total,
-- 
1.5.3.4.1463.gf79ad2

[PATCH 5/5] add throughput display to index-pack

[Nicolas Pitre]

... and call it "Receiving objects" when over stdin to look clearer
to end users.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 index-pack.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/index-pack.c b/index-pack.c
index 879ea15..61ea762 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -87,6 +87,8 @@ static void *fill(int min)
 				die("early EOF");
 			die("read error on input: %s", strerror(errno));
 		}
+		if (from_stdin)
+			display_throughput(progress, ret);
 		input_len += ret;
 	} while (input_len < min);
 	return input_buffer;
@@ -406,7 +408,9 @@ static void parse_pack_objects(unsigned char *sha1)
 	 * - remember base (SHA1 or offset) for all deltas.
 	 */
 	if (verbose)
-		progress = start_progress("Indexing objects", nr_objects);
+		progress = start_progress(
+				from_stdin ? "Receiving objects" : "Indexing objects",
+				nr_objects);
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
 		data = unpack_raw_entry(obj, &delta->base);
-- 
1.5.3.4.1463.gf79ad2

[PATCH 6/5] add some copyright notice to the progress display code

[Nicolas Pitre]

Some self patting on the back to keep my ego alive.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
diff --git a/progress.c b/progress.c
index 23ee9f3..2c4057a 100644
--- a/progress.c
+++ b/progress.c
@@ -1,3 +1,13 @@
+/*
+ * Simple text-based progress display module for GIT
+ *
+ * Copyright (c) 2007 by Nicolas Pitre <nico@cam.org>
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
 #include "git-compat-util.h"
 #include "progress.h"
 

[PATCH 7/5] add throughput display to git-push

[Nicolas Pitre]

This one triggers only when git-pack-objects is called with 
--all-progress and --stdout which is the combination used by
git-push.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c |    2 +-
 csum-file.c            |    8 ++++++++
 csum-file.h            |    4 ++++
 3 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 52a26a2..25ec65d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -606,7 +606,7 @@ static void write_pack_file(void)
 		char *pack_tmp_name = NULL;
 
 		if (pack_to_stdout) {
-			f = sha1fd(1, "<stdout>");
+			f = sha1fd_throughput(1, "<stdout>", progress_state);
 		} else {
 			char tmpname[PATH_MAX];
 			int fd;
diff --git a/csum-file.c b/csum-file.c
index 9929991..3729e73 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -8,6 +8,7 @@
  * able to verify hasn't been messed with afterwards.
  */
 #include "cache.h"
+#include "progress.h"
 #include "csum-file.h"
 
 static void sha1flush(struct sha1file *f, unsigned int count)
@@ -17,6 +18,7 @@ static void sha1flush(struct sha1file *f, unsigned int count)
 	for (;;) {
 		int ret = xwrite(f->fd, buf, count);
 		if (ret > 0) {
+			display_throughput(f->tp, ret);
 			buf = (char *) buf + ret;
 			count -= ret;
 			if (count)
@@ -80,6 +82,11 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)
 
 struct sha1file *sha1fd(int fd, const char *name)
 {
+	return sha1fd_throughput(fd, name, NULL);
+}
+
+struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp)
+{
 	struct sha1file *f;
 	unsigned len;
 
@@ -94,6 +101,7 @@ struct sha1file *sha1fd(int fd, const char *name)
 	f->fd = fd;
 	f->error = 0;
 	f->offset = 0;
+	f->tp = tp;
 	f->do_crc = 0;
 	SHA1_Init(&f->ctx);
 	return f;
diff --git a/csum-file.h b/csum-file.h
index c3c792f..4d1b231 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -1,11 +1,14 @@
 #ifndef CSUM_FILE_H
 #define CSUM_FILE_H
 
+struct progress;
+
 /* A SHA1-protected file */
 struct sha1file {
 	int fd, error;
 	unsigned int offset, namelen;
 	SHA_CTX ctx;
+	struct progress *tp;
 	char name[PATH_MAX];
 	int do_crc;
 	uint32_t crc32;
@@ -13,6 +16,7 @@ struct sha1file {
 };
 
 extern struct sha1file *sha1fd(int fd, const char *name);
+extern struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp);
 extern int sha1close(struct sha1file *, unsigned char *, int);
 extern int sha1write(struct sha1file *, void *, unsigned int);
 extern void crc32_begin(struct sha1file *);

Re: [PATCH 2/5] make struct progress an opaque type

[Junio C Hamano]

The ../linux-2.6/scripts/checkpatch.pl (run with --no-tree)
script found a few instances of:

ERROR: "foo * bar" should be "foo *bar"
#239: FILE: progress.c:91:
+struct progress * start_progress_delay(const char *title, unsigned total,

I'll munge them away before applying.

Re: [PATCH 1/5] prune-packed: don't call display_progress() for every file

[Shawn O. Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> The progress count is per fanout directory, so it is useless to call
> it for every file as the count doesn't change that often.

If you go back into the history and look at the commit message for
when I introduced this per-object display_progress() call we find
the following:

 commit b5d72f0a4cd3cce945ca0d37e4fa0ebbfcdcdb52
 Author: Shawn O. Pearce <spearce@spearce.org>
 Date:   Fri Oct 19 00:08:37 2007 -0400

[...snip...]
    We perform the display_progress() call from within the very innermost
    loop in case we spend more than 1 second within any single object
    directory.  This ensures that a progress_update event from the
    timer will still trigger in a timely fashion and allow the user to
    see the progress meter.

During my testing with a 40,000 loose object case (yea, I fully
unpacked a git.git clone I had laying around) my system stalled
hard in the first object directory.  A *lot* longer than 1 second.
So I got no progress meter for a long time, and then a progress
meter appeared on the second directory.

The display_progress() call already does a reasonably cheap
comparsion to see if the timer has tripped or if the percent complete
has changed.  So I figured it was more useful to get feedback to
the user that we were working, but were going to take a while,
than it was to optimize a few machine instructions out of that
inner-most per-object loop.

So I'm a little against this patch.  But I think I understand why
you think its worth doing.  I just consider the progress feedback
more important than the few machine cycles avoiding it saves.
 
-- 
Shawn.

Re: [PATCH 1/5] prune-packed: don't call display_progress() for every file

[Nicolas Pitre]

On Wed, 31 Oct 2007, Shawn O. Pearce wrote:

> Nicolas Pitre <nico@cam.org> wrote:
> > The progress count is per fanout directory, so it is useless to call
> > it for every file as the count doesn't change that often.
> 
> If you go back into the history and look at the commit message for
> when I introduced this per-object display_progress() call we find
> the following:
> 
>  commit b5d72f0a4cd3cce945ca0d37e4fa0ebbfcdcdb52
>  Author: Shawn O. Pearce <spearce@spearce.org>
>  Date:   Fri Oct 19 00:08:37 2007 -0400
> 
> [...snip...]
>     We perform the display_progress() call from within the very innermost
>     loop in case we spend more than 1 second within any single object
>     directory.  This ensures that a progress_update event from the
>     timer will still trigger in a timely fashion and allow the user to
>     see the progress meter.

Hmmmm OK.  I overlooked that.

> During my testing with a 40,000 loose object case (yea, I fully
> unpacked a git.git clone I had laying around) my system stalled
> hard in the first object directory.  A *lot* longer than 1 second.
> So I got no progress meter for a long time, and then a progress
> meter appeared on the second directory.

But then don't you get a "0% (1/256)" on the screen for a while, like if 
it was stalled?  Might be better than nothing at all I suppose...

> So I'm a little against this patch.  But I think I understand why
> you think its worth doing.  I just consider the progress feedback
> more important than the few machine cycles avoiding it saves.

OK, I don't mind reverting the patch, or actually providing another one 
to move the call back inside the loop since other changes will prevent a 
clean revert.


Nicolas