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

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2017, 04:16 UTC
Message-ID: <xmqq1srdfu5v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170524051537.29978-19-whydoubt@gmail.com>

Jeff Smith <whydoubt@gmail.com> writes:

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

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

Show 16 quoted lines

> Allow the interface user to decide how to handle a progress update.
>
> Signed-off-by: Jeff Smith <whydoubt@gmail.com>
> ---
>  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).

Show 9 quoted lines

> @@ -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.

Show 13 quoted lines

>  
> +	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.