{"thread":{"id":"7751","subject":"[PATCH] Kill the useless progress meter in merge-recursive","startedAt":"2007-04-20T06:37:18Z","lastAt":"2007-04-20T18:00:00Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano","Martin Waitz","Linus Torvalds","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"39944","messageId":"20070420063718.GA8424@spearce.org","threadId":"7751","inReplyTo":null,"subject":"[PATCH] Kill the useless progress meter in merge-recursive","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-20T06:37:18Z","receivedAt":"2007-04-20T06:37:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The mess known as the progress meter in merge-recursive was my own\nfault; I put it in thinking that we might be spending a lot of time\nresolving unmerged entries in the index that were not handled by\nthe simple 3-way index merge code.\n\nTurns out we don't really spend that much time there, so the progress\nmeter was pretty much always jumping to \"(n/n) 100%\" as soon as\nthe program started.  That isn't a very good indication of progress.\n\nSince I don't have a great solution for how a progress meter should\nwork here, I'm proposing we back it out entirely.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n In addition to Nico's nice progress meter cleanups.  :-)\n And now hopefully I am done for the night.\n\n merge-recursive.c |   65 ++--------------------------------------------------\n 1 files changed, 3 insertions(+), 62 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 595b022..bd05ee3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -95,11 +95,6 @@ static struct path_list current_directory_set = {NULL, 0, 0, 1};\n static int call_depth = 0;\n static int verbosity = 2;\n static int buffer_output = 1;\n-static int do_progress = 1;\n-static unsigned last_percent;\n-static unsigned merged_cnt;\n-static unsigned total_cnt;\n-static volatile sig_atomic_t progress_update;\n static struct output_buffer *output_list, *output_end;\n \n static int show (int v)\n@@ -174,39 +169,6 @@ static void output_commit_title(struct commit *commit)\n \t}\n }\n \n-static void progress_interval(int signum)\n-{\n-\tprogress_update = 1;\n-}\n-\n-static void setup_progress_signal(void)\n-{\n-\tstruct sigaction sa;\n-\tstruct itimerval v;\n-\n-\tmemset(&sa, 0, sizeof(sa));\n-\tsa.sa_handler = progress_interval;\n-\tsigemptyset(&sa.sa_mask);\n-\tsa.sa_flags = SA_RESTART;\n-\tsigaction(SIGALRM, &sa, NULL);\n-\n-\tv.it_interval.tv_sec = 1;\n-\tv.it_interval.tv_usec = 0;\n-\tv.it_value = v.it_interval;\n-\tsetitimer(ITIMER_REAL, &v, NULL);\n-}\n-\n-static void display_progress()\n-{\n-\tunsigned percent = total_cnt ? merged_cnt * 100 / total_cnt : 0;\n-\tif (progress_update || percent != last_percent) {\n-\t\tfprintf(stderr, \"%4u%% (%u/%u) done\\r\",\n-\t\t\tpercent, merged_cnt, total_cnt);\n-\t\tprogress_update = 0;\n-\t\tlast_percent = percent;\n-\t}\n-}\n-\n static struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tconst unsigned char *sha1, const char *path, int stage, int refresh)\n {\n@@ -377,14 +339,11 @@ static struct path_list *get_unmerged(void)\n \tint i;\n \n \tunmerged->strdup_paths = 1;\n-\ttotal_cnt += active_nr;\n \n-\tfor (i = 0; i < active_nr; i++, merged_cnt++) {\n+\tfor (i = 0; i < active_nr; i++) {\n \t\tstruct path_list_item *item;\n \t\tstruct stage_data *e;\n \t\tstruct cache_entry *ce = active_cache[i];\n-\t\tif (do_progress)\n-\t\t\tdisplay_progress();\n \t\tif (!ce_stage(ce))\n \t\t\tcontinue;\n \n@@ -1214,15 +1173,12 @@ static int merge_trees(struct tree *head,\n \t\tre_merge = get_renames(merge, common, head, merge, entries);\n \t\tclean = process_renames(re_head, re_merge,\n \t\t\t\tbranch1, branch2);\n-\t\ttotal_cnt += entries->nr;\n-\t\tfor (i = 0; i < entries->nr; i++, merged_cnt++) {\n+\t\tfor (i = 0; i < entries->nr; i++) {\n \t\t\tconst char *path = entries->items[i].path;\n \t\t\tstruct stage_data *e = entries->items[i].util;\n \t\t\tif (!e->processed\n \t\t\t\t&& !process_entry(path, e, branch1, branch2))\n \t\t\t\tclean = 0;\n-\t\t\tif (do_progress)\n-\t\t\t\tdisplay_progress();\n \t\t}\n \n \t\tpath_list_clear(re_merge, 0);\n@@ -1330,15 +1286,6 @@ static int merge(struct commit *h1,\n \t\tcommit_list_insert(h1, &(*result)->parents);\n \t\tcommit_list_insert(h2, &(*result)->parents->next);\n \t}\n-\tif (!call_depth && do_progress) {\n-\t\t/* Make sure we end at 100% */\n-\t\tif (!total_cnt)\n-\t\t\ttotal_cnt = 1;\n-\t\tmerged_cnt = total_cnt;\n-\t\tprogress_update = 1;\n-\t\tdisplay_progress();\n-\t\tfputc('\\n', stderr);\n-\t}\n \tflush_output();\n \treturn clean;\n }\n@@ -1415,12 +1362,8 @@ int main(int argc, char *argv[])\n \t}\n \tif (argc - i != 3) /* \"--\" \"<head>\" \"<remote>\" */\n \t\tdie(\"Not handling anything other than two heads merge.\");\n-\tif (verbosity >= 5) {\n+\tif (verbosity >= 5)\n \t\tbuffer_output = 0;\n-\t\tdo_progress = 0;\n-\t}\n-\telse\n-\t\tdo_progress = isatty(1);\n \n \tbranch1 = argv[++i];\n \tbranch2 = argv[++i];\n@@ -1431,8 +1374,6 @@ int main(int argc, char *argv[])\n \tbranch1 = better_branch_name(branch1);\n \tbranch2 = better_branch_name(branch2);\n \n-\tif (do_progress)\n-\t\tsetup_progress_signal();\n \tif (show(3))\n \t\tprintf(\"Merging %s with %s\\n\", branch1, branch2);\n \n-- \n1.5.1.1.135.gf948\n"},{"id":"39949","messageId":"7vd51z317c.fsf@assigned-by-dhcp.cox.net","threadId":"7751","inReplyTo":"20070420063718.GA8424@spearce.org","subject":"Re: [PATCH] Kill the useless progress meter in merge-recursive","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-20T08:21:59Z","receivedAt":"2007-04-20T08:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> The mess known as the progress meter in merge-recursive was my own\n> fault; I put it in thinking that we might be spending a lot of time\n> resolving unmerged entries in the index that were not handled by\n> the simple 3-way index merge code.\n>\n> Turns out we don't really spend that much time there, so the progress\n> meter was pretty much always jumping to \"(n/n) 100%\" as soon as\n> the program started.  That isn't a very good indication of progress.\n\nI would propose removing the progress meter for \"Checking out\nfiles\" in unpack-trees, for the same reason.\n\n---\n unpack-trees.c |   62 --------------------------------------------------------\n 1 files changed, 0 insertions(+), 62 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 5139481..1419653 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -288,73 +288,15 @@ static void unlink_entry(char *name)\n \t}\n }\n \n-static volatile sig_atomic_t progress_update;\n-\n-static void progress_interval(int signum)\n-{\n-\tprogress_update = 1;\n-}\n-\n-static void setup_progress_signal(void)\n-{\n-\tstruct sigaction sa;\n-\tstruct itimerval v;\n-\n-\tmemset(&sa, 0, sizeof(sa));\n-\tsa.sa_handler = progress_interval;\n-\tsigemptyset(&sa.sa_mask);\n-\tsa.sa_flags = SA_RESTART;\n-\tsigaction(SIGALRM, &sa, NULL);\n-\n-\tv.it_interval.tv_sec = 1;\n-\tv.it_interval.tv_usec = 0;\n-\tv.it_value = v.it_interval;\n-\tsetitimer(ITIMER_REAL, &v, NULL);\n-}\n-\n static struct checkout state;\n static void check_updates(struct cache_entry **src, int nr,\n \t\tstruct unpack_trees_options *o)\n {\n \tunsigned short mask = htons(CE_UPDATE);\n-\tunsigned last_percent = 200, cnt = 0, total = 0;\n-\n-\tif (o->update && o->verbose_update) {\n-\t\tfor (total = cnt = 0; cnt < nr; cnt++) {\n-\t\t\tstruct cache_entry *ce = src[cnt];\n-\t\t\tif (!ce->ce_mode || ce->ce_flags & mask)\n-\t\t\t\ttotal++;\n-\t\t}\n-\n-\t\t/* Don't bother doing this for very small updates */\n-\t\tif (total < 250)\n-\t\t\ttotal = 0;\n-\n-\t\tif (total) {\n-\t\t\tfprintf(stderr, \"Checking files out...\\n\");\n-\t\t\tsetup_progress_signal();\n-\t\t\tprogress_update = 1;\n-\t\t}\n-\t\tcnt = 0;\n-\t}\n \n \twhile (nr--) {\n \t\tstruct cache_entry *ce = *src++;\n \n-\t\tif (total) {\n-\t\t\tif (!ce->ce_mode || ce->ce_flags & mask) {\n-\t\t\t\tunsigned percent;\n-\t\t\t\tcnt++;\n-\t\t\t\tpercent = (cnt * 100) / total;\n-\t\t\t\tif (percent != last_percent ||\n-\t\t\t\t    progress_update) {\n-\t\t\t\t\tfprintf(stderr, \"%4u%% (%u/%u) done\\r\",\n-\t\t\t\t\t\tpercent, cnt, total);\n-\t\t\t\t\tlast_percent = percent;\n-\t\t\t\t\tprogress_update = 0;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n \t\tif (!ce->ce_mode) {\n \t\t\tif (o->update)\n \t\t\t\tunlink_entry(ce->name);\n@@ -366,10 +308,6 @@ static void check_updates(struct cache_entry **src, int nr,\n \t\t\t\tcheckout_entry(ce, &state, NULL);\n \t\t}\n \t}\n-\tif (total) {\n-\t\tsignal(SIGALRM, SIG_IGN);\n-\t\tfputc('\\n', stderr);\n-\t}\n }\n \n int unpack_trees(struct object_list *trees, struct unpack_trees_options *o)\n"},{"id":"39953","messageId":"20070420090010.GD27208@admingilde.org","threadId":"7751","inReplyTo":"7vd51z317c.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Kill the useless progress meter in merge-recursive","fromName":"Martin Waitz","fromEmail":"tali@admingilde.org","sentAt":"2007-04-20T09:00:10Z","receivedAt":"2007-04-20T09:00:10Z","isPatch":true,"sender":{"key":"tali@admingilde.org","avatar":"https://gravatar.com/avatar/3f89b03eee362187effabe257898735b475673a12265c398ea9161259ae91553?d=mp&s=160"},"body":"hoi :)\n\nOn Fri, Apr 20, 2007 at 01:21:59AM -0700, Junio C Hamano wrote:\n> I would propose removing the progress meter for \"Checking out\n> files\" in unpack-trees, for the same reason.\n\nwell, for large trees it is indeed useful, so I'd like to leave it in.\n\n-- \nMartin Waitz\n"},{"id":"39988","messageId":"alpine.LFD.0.98.0704200811470.9964@woody.linux-foundation.org","threadId":"7751","inReplyTo":"7vd51z317c.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Kill the useless progress meter in merge-recursive","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-20T15:14:20Z","receivedAt":"2007-04-20T15:14:20Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 20 Apr 2007, Junio C Hamano wrote:\n> \n> I would propose removing the progress meter for \"Checking out\n> files\" in unpack-trees, for the same reason.\n\nHave you tried this with something like the kernel on a 128MB machine, or \nover NFS? Or, indeed, if you just do\n\n\techo 5 > /proc/sys/vm/dirty_background_ratio\n\techo 2 > /proc/sys/vm/dirty_ratio\n\nor similar, to tell the kernel to not allow a lot of dirty files.\n\nNo, I've not tried it either, but you may think that checking files out is \nfast just because you're actually just writing to memory, and the \nbackground writeout will do the real work. That isn't always true. \nChecking files out can be very expensive indeed.\n\n\t\tLinus\n"},{"id":"39996","messageId":"Pine.LNX.4.64.0704201339160.27922@iabervon.org","threadId":"7751","inReplyTo":"7vd51z317c.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Kill the useless progress meter in merge-recursive","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-04-20T18:00:00Z","receivedAt":"2007-04-20T18:00:00Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 20 Apr 2007, Junio C Hamano wrote:\n\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > The mess known as the progress meter in merge-recursive was my own\n> > fault; I put it in thinking that we might be spending a lot of time\n> > resolving unmerged entries in the index that were not handled by\n> > the simple 3-way index merge code.\n> >\n> > Turns out we don't really spend that much time there, so the progress\n> > meter was pretty much always jumping to \"(n/n) 100%\" as soon as\n> > the program started.  That isn't a very good indication of progress.\n> \n> I would propose removing the progress meter for \"Checking out\n> files\" in unpack-trees, for the same reason.\n\nMaybe it should instead be arranged to only show the progress meter when \nthe timer expires the first time (or, maybe, if it expires a few times). \nRather than deciding whether it's going to be slow based on the total \nnumber, decide based on whether it's taken long enough for the user to \nwonder what's going on yet.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}