Re: [PATCH] commit-graph: fix "Collecting commits from input" progress line
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 10, 2020, 19:17 UTC
- Message-ID
- <20200710191704.GA65960@syl.lan>
- In-Reply-To
- <20200710190238.16376-1-szeder.dev@gmail.com>
Hi Szeder,
On Fri, Jul 10, 2020 at 09:02:38PM +0200, SZEDER Gábor wrote:
Show 8 quoted lines
> 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
Show 43 quoted lines
>
> 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.
Show 13 quoted lines
> > 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