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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
Thanks,
Taylor
