Kill the useless progress meter in merge-recursive

PATCH In review Shawn O. Pearce 5 participants 5 messages Jun 15, 2016

Share & tools

Shawn O. Pearce

author

2016-06-15 22:43 UTC #0 lore raw

PATCH 1/1 Kill the useless progress meter in merge-recursive

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.

Since I don't have a great solution for how a progress meter should work here, I'm proposing we back it out entirely.

Signed-off-by Shawn O. Pearce <[email protected]>

---

In addition to Nico's nice progress meter cleanups. :-) And now hopefully I am done for the night.

 merge-recursive.c |   65 ++--------------------------------------------------
 1 files changed, 3 insertions(+), 62 deletions(-)

merge-recursive.c +3 ~~-62~~

index 595b022..bd05ee3 100644
@@ -95,11 +95,6 @@ static struct path_list current_directory_set = {NULL, 0, 0, 1};
 static int call_depth = 0;
 static int verbosity = 2;
 static int buffer_output = 1;
-static int do_progress = 1;
-static unsigned last_percent;
-static unsigned merged_cnt;
-static unsigned total_cnt;
-static volatile sig_atomic_t progress_update;
 static struct output_buffer *output_list, *output_end;
 
 static int show (int v)

@@ -174,39 +169,6 @@ static void output_commit_title(struct commit *commit)
 	}
 }
 
-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 void display_progress()
-{
-	unsigned percent = total_cnt ? merged_cnt * 100 / total_cnt : 0;
-	if (progress_update || percent != last_percent) {
-		fprintf(stderr, "%4u%% (%u/%u) done\r",
-			percent, merged_cnt, total_cnt);
-		progress_update = 0;
-		last_percent = percent;
-	}
-}
-
 static struct cache_entry *make_cache_entry(unsigned int mode,
 		const unsigned char *sha1, const char *path, int stage, int refresh)
 {

@@ -377,14 +339,11 @@ static struct path_list *get_unmerged(void)
 	int i;
 
 	unmerged->strdup_paths = 1;
-	total_cnt += active_nr;
 
-	for (i = 0; i < active_nr; i++, merged_cnt++) {
+	for (i = 0; i < active_nr; i++) {
 		struct path_list_item *item;
 		struct stage_data *e;
 		struct cache_entry *ce = active_cache[i];
-		if (do_progress)
-			display_progress();
 		if (!ce_stage(ce))
 			continue;

@@ -1214,15 +1173,12 @@ static int merge_trees(struct tree *head,
 		re_merge = get_renames(merge, common, head, merge, entries);
 		clean = process_renames(re_head, re_merge,
 				branch1, branch2);
-		total_cnt += entries->nr;
-		for (i = 0; i < entries->nr; i++, merged_cnt++) {
+		for (i = 0; i < entries->nr; i++) {
 			const char *path = entries->items[i].path;
 			struct stage_data *e = entries->items[i].util;
 			if (!e->processed
 				&& !process_entry(path, e, branch1, branch2))
 				clean = 0;
-			if (do_progress)
-				display_progress();
 		}
 
 		path_list_clear(re_merge, 0);

@@ -1330,15 +1286,6 @@ static int merge(struct commit *h1,
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
 	}
-	if (!call_depth && do_progress) {
-		/* Make sure we end at 100% */
-		if (!total_cnt)
-			total_cnt = 1;
-		merged_cnt = total_cnt;
-		progress_update = 1;
-		display_progress();
-		fputc('\n', stderr);
-	}
 	flush_output();
 	return clean;
 }

@@ -1415,12 +1362,8 @@ int main(int argc, char *argv[])
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
 		die("Not handling anything other than two heads merge.");
-	if (verbosity >= 5) {
+	if (verbosity >= 5)
 		buffer_output = 0;
-		do_progress = 0;
-	}
-	else
-		do_progress = isatty(1);
 
 	branch1 = argv[++i];
 	branch2 = argv[++i];

@@ -1431,8 +1374,6 @@ int main(int argc, char *argv[])
 	branch1 = better_branch_name(branch1);
 	branch2 = better_branch_name(branch2);
 
-	if (do_progress)
-		setup_progress_signal();
 	if (show(3))
 		printf("Merging %s with %s\n", branch1, branch2);

Signature · 1 line

1.5.1.1.135.gf948

Junio C Hamano

2016-06-15 22:43 UTC #1 lore raw

Quoted 8 lines · Shawn O. Pearce

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(-)

unpack-trees.c +0 ~~-62~~

index 5139481..1419653 100644
@@ -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)

Martin Waitz

2016-06-15 22:43 UTC #2 lore raw

hoi :)

Quoted 2 lines · Junio C Hamano

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.

Signature · 1 line

Martin Waitz

Daniel Barkalow

2016-06-15 22:43 UTC #3 lore raw

In reply to Junio C Hamano

Quoted 7 lines · Junio C Hamano

Quoted 5 lines · Shawn O. Pearce
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.

Quoted 6 lines

Quoted 3 lines
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*

Linus Torvalds

2016-06-15 22:43 UTC #4 lore raw

In reply to Junio C Hamano

Quoted 3 lines · Junio C Hamano

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