Re: [PATCH] Kill the useless progress meter in merge-recursive

"Shawn O. Pearce" [off-list ref] writes:

The mess known as the progress meter in merge-recursive was my own
fault; I put it in thinking that we might be spending a lot of time
resolving unmerged entries in the index that were not handled by
the simple 3-way index merge code.

Turns out we don't really spend that much time there, so the progress
meter was pretty much always jumping to "(n/n) 100%" as soon as
the program started.  That isn't a very good indication of progress.

I would propose removing the progress meter for "Checking out
files" in unpack-trees, for the same reason.

---
 unpack-trees.c |   62 --------------------------------------------------------
 1 files changed, 0 insertions(+), 62 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 5139481..1419653 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -288,73 +288,15 @@ static void unlink_entry(char *name)
 	}
 }
 
-static volatile sig_atomic_t progress_update;
-
-static void progress_interval(int signum)
-{
-	progress_update = 1;
-}
-
-static void setup_progress_signal(void)
-{
-	struct sigaction sa;
-	struct itimerval v;
-
-	memset(&sa, 0, sizeof(sa));
-	sa.sa_handler = progress_interval;
-	sigemptyset(&sa.sa_mask);
-	sa.sa_flags = SA_RESTART;
-	sigaction(SIGALRM, &sa, NULL);
-
-	v.it_interval.tv_sec = 1;
-	v.it_interval.tv_usec = 0;
-	v.it_value = v.it_interval;
-	setitimer(ITIMER_REAL, &v, NULL);
-}
-
 static struct checkout state;
 static void check_updates(struct cache_entry **src, int nr,
 		struct unpack_trees_options *o)
 {
 	unsigned short mask = htons(CE_UPDATE);
-	unsigned last_percent = 200, cnt = 0, total = 0;
-
-	if (o->update && o->verbose_update) {
-		for (total = cnt = 0; cnt < nr; cnt++) {
-			struct cache_entry *ce = src[cnt];
-			if (!ce->ce_mode || ce->ce_flags & mask)
-				total++;
-		}
-
-		/* Don't bother doing this for very small updates */
-		if (total < 250)
-			total = 0;
-
-		if (total) {
-			fprintf(stderr, "Checking files out...\n");
-			setup_progress_signal();
-			progress_update = 1;
-		}
-		cnt = 0;
-	}
 
 	while (nr--) {
 		struct cache_entry *ce = *src++;
 
-		if (total) {
-			if (!ce->ce_mode || ce->ce_flags & mask) {
-				unsigned percent;
-				cnt++;
-				percent = (cnt * 100) / total;
-				if (percent != last_percent ||
-				    progress_update) {
-					fprintf(stderr, "%4u%% (%u/%u) done\r",
-						percent, cnt, total);
-					last_percent = percent;
-					progress_update = 0;
-				}
-			}
-		}
 		if (!ce->ce_mode) {
 			if (o->update)
 				unlink_entry(ce->name);

@@ -366,10 +308,6 @@ static void check_updates(struct cache_entry **src, int nr,
 				checkout_entry(ce, &state, NULL);
 		}
 	}
-	if (total) {
-		signal(SIGALRM, SIG_IGN);
-		fputc('\n', stderr);
-	}
 }
 
 int unpack_trees(struct object_list *trees, struct unpack_trees_options *o)

hoi :)

On Fri, Apr 20, 2007 at 01:21:59AM -0700, Junio C Hamano wrote:

I would propose removing the progress meter for "Checking out
files" in unpack-trees, for the same reason.

well, for large trees it is indeed useful, so I'd like to leave it in.

-- 
Martin Waitz

On Fri, 20 Apr 2007, Junio C Hamano wrote:

I would propose removing the progress meter for "Checking out
files" in unpack-trees, for the same reason.

Have you tried this with something like the kernel on a 128MB machine, or 
over NFS? Or, indeed, if you just do

	echo 5 > /proc/sys/vm/dirty_background_ratio
	echo 2 > /proc/sys/vm/dirty_ratio

or similar, to tell the kernel to not allow a lot of dirty files.

No, I've not tried it either, but you may think that checking files out is 
fast just because you're actually just writing to memory, and the 
background writeout will do the real work. That isn't always true. 
Checking files out can be very expensive indeed.

		Linus

On Fri, 20 Apr 2007, Junio C Hamano wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

The mess known as the progress meter in merge-recursive was my own
fault; I put it in thinking that we might be spending a lot of time
resolving unmerged entries in the index that were not handled by
the simple 3-way index merge code.

Turns out we don't really spend that much time there, so the progress
meter was pretty much always jumping to "(n/n) 100%" as soon as
the program started.  That isn't a very good indication of progress.

I would propose removing the progress meter for "Checking out
files" in unpack-trees, for the same reason.

Maybe it should instead be arranged to only show the progress meter when 
the timer expires the first time (or, maybe, if it expires a few times). 
Rather than deciding whether it's going to be slow based on the total 
number, decide based on whether it's taken long enough for the user to 
wonder what's going on yet.

	-Daniel
*This .sig left intentionally blank*