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

[Taylor Blau <me@ttaylorr.com>]

Hi Szeder,

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

s/Consequintly/Consequently

>
>   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 <szeder.dev@gmail.com>
> ---
>  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);
>  	}

Thanks for spotting this, and sorry for the couple of progress-related
bugs that I seem to have generated. I appreciate you looking into it and
fixing some of these.

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


All looks good here. With the exception of the small typo that I
noticed, this is:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor