Re: [PATCH v1] blame: add support for --[no-]progress option

From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Date: Nov 22, 2015, 05:27 UTC
Message-ID: <CAOc6etbfzrqT4-5dorCAq_=5Oh9wXV4LgRwK9hrEqjmSYHiBEg@mail.gmail.com>
In-Reply-To: <1448169116-32335-1-git-send-email-eantoranz@gmail.com>

Hey, guys!

Time for some more patch destruction.

On Sat, Nov 21, 2015 at 11:11 PM, Edmundo Carmona Antoranz <eantoranz@gmail.com> wrote: -static void assign_blame(struct scoreboard *sb, int opt) +static void assign_blame(struct scoreboard *sb, int opt, int show_progress)

Would it be better to include show_progress as a binary flag in opt instead of a separate variable?

> +       struct progress * progress = NULL;
> +       int blamed_lines = -1;

I'm wondering if it would be better to save the 'last number used in progress' inside struct progress so that we could skip blamed_lines. That would also allow progress itself (as in the API) to avoid printing duplicated progress values. In my case, I'm going through a number of cycles where _I think_ I might have the same number of blamed lines detected. To avoid asking to do a new progress print out with the same value, I'm checking the value I had used last time. If progress took care of that check up, this variable would go for good and I would just ask progress to print with the current value (no matter if it's duplicate or an increased value).

> +
> +       if (show_progress) {
> +               progress = start_progress(_("Blaming lines"), sb->num_lines);
> +       }

Already noticed I can get rid of the curly brackets.

Show 24 quoted lines

>
>         while (commit) {
>                 struct blame_entry *ent;
> @@ -1822,9 +1838,21 @@ static void assign_blame(struct scoreboard *sb, int opt)
>                 }
>                 origin_decref(suspect);
>
> +               if (progress) {
> +                       int current_blamed_lines = count_blamed_lines(sb);
> +                       if (current_blamed_lines > blamed_lines) {
> +                               blamed_lines = current_blamed_lines;
> +                               display_progress(progress, blamed_lines);
> +                       }
> +               }
> +
>                 if (DEBUG) /* sanity */
>                         sanity_check_refcnt(sb);
>         }
> +
> +       if (progress) {
> +               stop_progress(&progress);
> +       }
>  }
>

It first I tried to detect how many revisions where going to be checked so that I could report progress on them but I found extracting information from scoreboard a little tricky (to be read: I could not extract anything out of it). Then I though of counting lines and it works so.... (same thing with the curly brackets).

Show 7 quoted lines

> +       assign_blame(&sb, opt, show_progress);
> +
>         if (!incremental)
>                 setup_pager();
>
> -       assign_blame(&sb, opt);
> -

setup_pager() was breaking progress so I moved it _after_ assign_blame() and it seems to behave. Hope that's not a problem.

Looking forward to your feedback.