* [PATCH] Kill the useless progress meter in merge-recursive
@ 2007-04-20 6:37 Shawn O. Pearce
2007-04-20 8:21 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Shawn O. Pearce @ 2007-04-20 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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 <spearce@spearce.org>
---
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(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 595b022..bd05ee3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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);
--
1.5.1.1.135.gf948
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Kill the useless progress meter in merge-recursive
2007-04-20 6:37 [PATCH] Kill the useless progress meter in merge-recursive Shawn O. Pearce
@ 2007-04-20 8:21 ` Junio C Hamano
2007-04-20 9:00 ` Martin Waitz
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Junio C Hamano @ 2007-04-20 8:21 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
"Shawn O. Pearce" <spearce@spearce.org> 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)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Kill the useless progress meter in merge-recursive
2007-04-20 8:21 ` Junio C Hamano
@ 2007-04-20 9:00 ` Martin Waitz
2007-04-20 15:14 ` Linus Torvalds
2007-04-20 18:00 ` Daniel Barkalow
2 siblings, 0 replies; 5+ messages in thread
From: Martin Waitz @ 2007-04-20 9:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
[-- Attachment #1: Type: text/plain, Size: 283 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Kill the useless progress meter in merge-recursive
2007-04-20 8:21 ` Junio C Hamano
2007-04-20 9:00 ` Martin Waitz
@ 2007-04-20 15:14 ` Linus Torvalds
2007-04-20 18:00 ` Daniel Barkalow
2 siblings, 0 replies; 5+ messages in thread
From: Linus Torvalds @ 2007-04-20 15:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Kill the useless progress meter in merge-recursive
2007-04-20 8:21 ` Junio C Hamano
2007-04-20 9:00 ` Martin Waitz
2007-04-20 15:14 ` Linus Torvalds
@ 2007-04-20 18:00 ` Daniel Barkalow
2 siblings, 0 replies; 5+ messages in thread
From: Daniel Barkalow @ 2007-04-20 18:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
On Fri, 20 Apr 2007, Junio C Hamano wrote:
> "Shawn O. Pearce" <spearce@spearce.org> 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.
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*
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-04-20 18:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-04-20 6:37 [PATCH] Kill the useless progress meter in merge-recursive Shawn O. Pearce
2007-04-20 8:21 ` Junio C Hamano
2007-04-20 9:00 ` Martin Waitz
2007-04-20 15:14 ` Linus Torvalds
2007-04-20 18:00 ` Daniel Barkalow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).