| Message ID | 20200710190238.16376-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: fix "Collecting commits from input" progress line | expand |
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
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 <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); > } > > 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 >
On 7/15/2020 2:33 PM, SZEDER Gábor wrote: > Junio, > > it looks like this patch hasn't been picked up, but it fixes a minor > bug introduced in this release cycle. Thanks for the ping. I forgot to chime in with my :+1: on this patch. Outside of the small typo in the commit message, this is ready to ship. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> > 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); >> } >> >> 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 >>
Derrick Stolee <stolee@gmail.com> writes: > Thanks for the ping. I forgot to chime in with my :+1: on > this patch. Outside of the small typo in the commit message, > this is ready to ship. > > Reviewed-by: Derrick Stolee <dstolee@microsoft.com> Thanks, both. Will queue.
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; }
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 <szeder.dev@gmail.com> --- builtin/commit-graph.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)