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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2015, 02:08 UTC
Message-ID: <xmqq610to4bl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1448208133-29430-1-git-send-email-eantoranz@gmail.com>

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> Will also affect annotate

Is that a good thing? In any case, make it understandable without the title line (i.e. make it a full sentence, ending with a full stop).

Show 6 quoted lines

> +				if (progress) {
> +					for (next = suspect->suspects; next != NULL;
> +					     next = next->next)
> +						blamed_lines += next->num_lines;
> +					display_progress(progress, blamed_lines);
> +				}

Is this math and the placement of the code correct? It would probably be more obvious if this hunk is in found_guilty_entry(), which is already the dedicated function in which we report about a group of lines whose ultimate origin has become clear.

Show 14 quoted lines

> @@ -2830,11 +2851,11 @@ parse_done:
>  
>  	read_mailmap(&mailmap, NULL);
>  
> +	assign_blame(&sb, opt);
> +
>  	if (!incremental)
>  		setup_pager();
>  
> -	assign_blame(&sb, opt);
> -
>  	free(final_commit_name);
>  
>  	if (incremental)

Two comments.

 * How does this interact with incremental or porcelain blame?
   Shouldn't progress be turned off when these modes are in use?

 * Shouldn't progress be turned off if the result comes very
   quickly, using start_progress_delay()?