Subject: Re: [PATCH] commit-graph: fix "Collecting commits from input" progress line

From: SZEDER Gábor <[email-masked]>
Date: Wed, 15 Jul 2020 20:33:47 +0200

Junio,

it looks like this patch hasn't been picked up, but it fixes a minor
bug introduced in this release cycle.

On Fri, Jul 10, 2020 at 09:02:38PM +0200, SZEDER Gábor wrote:
> To display a progress line while reading commits from standard input
> and looking them up, 5b6653e523 (builtin/commit-graph.c: dereference
> tags in builtin, 2020-05-13) should have added a pair of
> start_delayed_progress() and stop_progress() calls around the loop
> reading stdin.  Alas, the stop_progress() call ended up at the wrong
> place, after write_commit_graph(), which does all the commit-graph
> computation and writing, and has several progress lines of its own.
> Consequintly, that new
> 
>   Collecting commits from input: 1234
> 
> progress line is overwritten by the first progress line shown by
> write_commit_graph(), and its final "done" line is shown last, after
> everything is finished:
> 
>   $ { sleep 3 ; git rev-list -3 HEAD ; sleep 1 ; } | ~/src/git/git commit-graph write --stdin-commits
>   Expanding reachable commits in commit graph: 873402, done.
>   Writing out commit graph in 4 passes: 100% (3493608/3493608), done.
>   Collecting commits from input: 3, done.
> 
> Furthermore, that stop_progress() call was added after the 'cleanup'
> label, where that loop reading stdin jumps in case of an error.  In
> case of invalid input this then results in the "done" line shown after
> the error message:
> 
>   $ { sleep 3 ; git rev-list -3 HEAD ; echo junk ; } | ~/src/git/git commit-graph write --stdin-commits
>   error: unexpected non-hex object ID: junk
>   Collecting commits from input: 3, done.
> 
> Move that stop_progress() call to the right place.
> 
> While at it, drop the unnecessary 'if (progress)' condition protecting
> the stop_progress() call, because that function is prepared to handle
> a NULL progress struct.
> 
> Signed-off-by: SZEDER Gábor <[email-masked]>
> ---
>  builtin/commit-graph.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 75455da138..796954da60 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -251,7 +251,7 @@ static int graph_write(int argc, const char **argv)
>  			}
>  		}
>  
> -
> +		stop_progress(&progress);
>  	}
>  
>  	if (write_commit_graph(odb,
> @@ -264,8 +264,6 @@ static int graph_write(int argc, const char **argv)
>  cleanup:
>  	string_list_clear(&pack_indexes, 0);
>  	strbuf_release(&buf);
> -	if (progress)
> -		stop_progress(&progress);
>  	return result;
>  }
>  
> -- 
> 2.27.0.547.g4ba2d26563
>