Subject: Re: [PATCH 18/29] blame: move progess updates to a scoreboard callback

From: Junio C Hamano <[email-masked]>
Date: Thu, 25 May 2017 13:16:12 +0900

Jeff Smith <[email-masked]> writes:

> Subject: Re: [PATCH 18/29] blame: move progess updates to a scoreboard callback

s/progess/progress/ (I can do this on my end).

> Allow the interface user to decide how to handle a progress update.
>
> Signed-off-by: Jeff Smith <[email-masked]>
> ---
>  builtin/blame.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> -static void found_guilty_entry(struct blame_entry *ent,
> -			   struct progress_info *pi)
> +static void found_guilty_entry(struct blame_entry *ent, void *data)
>  {
> +	struct progress_info *pi = (struct progress_info *)data;
> +
>  	if (incremental) {
>  		struct blame_origin *suspect = ent->suspect;
>  

This hunk is interesting.  The function not only does the "progress"
eye candy, but it actually handles the "--incremental" option.
Anybody who wants to do something similar using the libified blame
needs to implement it in their found_guilty callback like this one
does, which is probably a good division of labor between the blame
lib and the front-end (which builtin/blame.c is one instance of).

> @@ -1754,11 +1758,6 @@ static void assign_blame(struct blame_scoreboard *sb, int opt)
>  {
>  	struct rev_info *revs = sb->revs;
>  	struct commit *commit = prio_queue_get(&sb->commits);
> -	struct progress_info pi = { NULL, 0 };
> -
> -	if (show_progress)
> -		pi.progress = start_progress_delay(_("Blaming lines"),
> -						   sb->num_lines, 50, 1);

And this piece (and matching "stop progress" at the end of the
function) is now the responsibility of the caller, which makes
sense.

>  
> +	sb.found_guilty_entry = &found_guilty_entry;
> +	sb.found_guilty_entry_data = &pi;
> +	if (show_progress)
> +		pi.progress = start_progress_delay(_("Blaming lines"),
> +						   sb.num_lines, 50, 1);
> +
>  	assign_blame(&sb, opt);
>  
> +	stop_progress(&pi.progress);
> +
>  	if (!incremental)
>  		setup_pager();

Very nicely done so far.