| Message ID | 20200710014242.1088216-3-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable progress traces even when quiet | expand |
On 7/9/2020 9:42 PM, Emily Shaffer wrote: > display_progress() and stop_progress() are tolerant to NULL inputs. > Remove some places where redundant checks are performed before these > NULL-tolerant functions are called. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > builtin/commit-graph.c | 3 +-- > commit-graph.c | 3 +-- > read-cache.c | 9 +++------ > 3 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index ae4dc2dfcd..7964de3126 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv) > cleanup: > string_list_clear(&pack_indexes, 0); > strbuf_release(&buf); > - if (progress) > - stop_progress(&progress); > + stop_progress(&progress); > return result; > } > > diff --git a/commit-graph.c b/commit-graph.c > index b9a784fece..30d9a75932 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb, > flags, split_opts); > > oidset_clear(&commits); > - if (data.progress) > - stop_progress(&data.progress); > + stop_progress(&data.progress); > return result; > } > > diff --git a/read-cache.c b/read-cache.c > index 2ddc422dbd..feb7abe37a 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, > new_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed); > if (new_entry == ce) > continue; > - if (progress) > - display_progress(progress, i); > + display_progress(progress, i); > if (!new_entry) { > const char *fmt; > > @@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, > > replace_index_entry(istate, i, new_entry); > } > - if (progress) { > - display_progress(progress, istate->cache_nr); > - stop_progress(&progress); > - } > + display_progress(progress, istate->cache_nr); > + stop_progress(&progress); > trace_performance_leave("refresh index"); > return has_errors; Looks obviously correct to me. Thanks, -Stolee
On Thu, Jul 09, 2020 at 10:01:08PM -0400, Derrick Stolee wrote: > On 7/9/2020 9:42 PM, Emily Shaffer wrote: > > display_progress() and stop_progress() are tolerant to NULL inputs. > > Remove some places where redundant checks are performed before these > > NULL-tolerant functions are called. > > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > builtin/commit-graph.c | 3 +-- > > commit-graph.c | 3 +-- > > read-cache.c | 9 +++------ > > 3 files changed, 5 insertions(+), 10 deletions(-) > > > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > > index ae4dc2dfcd..7964de3126 100644 > > --- a/builtin/commit-graph.c > > +++ b/builtin/commit-graph.c > > @@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv) > > cleanup: > > string_list_clear(&pack_indexes, 0); > > strbuf_release(&buf); > > - if (progress) > > - stop_progress(&progress); > > + stop_progress(&progress); > > return result; > > } > > > > diff --git a/commit-graph.c b/commit-graph.c > > index b9a784fece..30d9a75932 100644 > > --- a/commit-graph.c > > +++ b/commit-graph.c > > @@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb, > > flags, split_opts); > > > > oidset_clear(&commits); > > - if (data.progress) > > - stop_progress(&data.progress); > > + stop_progress(&data.progress); > > return result; > > } > > > > diff --git a/read-cache.c b/read-cache.c > > index 2ddc422dbd..feb7abe37a 100644 > > --- a/read-cache.c > > +++ b/read-cache.c > > @@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, > > new_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed); > > if (new_entry == ce) > > continue; > > - if (progress) > > - display_progress(progress, i); > > + display_progress(progress, i); > > if (!new_entry) { > > const char *fmt; > > > > @@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, > > > > replace_index_entry(istate, i, new_entry); > > } > > - if (progress) { > > - display_progress(progress, istate->cache_nr); > > - stop_progress(&progress); > > - } > > + display_progress(progress, istate->cache_nr); > > + stop_progress(&progress); > > trace_performance_leave("refresh index"); > > return has_errors; > > Looks obviously correct to me. To me too, although note that this will generate merge conflicts with Szeder's patch from earlier today[1]. Unfortunately, the conflicts are a little deeper than "we both removed an unnecessary 'if' statement", since Szeder's patch moves the 'stop_progress()' call earlier to avoid a bug that I introduced. This is just something for Junio to look out for while queuing, otherwise I think just removing the hunk in builtin/commit-graph.c would be fine, too. > Thanks, > -Stolee Thanks, Taylor [1]: https://lore.kernel.org/git/xmqq5zawy4x5.fsf@gitster.c.googlers.com/T/#mbd2d5236a7fecf1287be75ffa396f3776fc9b891
Taylor Blau <me@ttaylorr.com> writes: > To me too, although note that this will generate merge conflicts with > Szeder's patch from earlier today[1]. > > Unfortunately, the conflicts are a little deeper than "we both removed > an unnecessary 'if' statement", since Szeder's patch moves the > 'stop_progress()' call earlier to avoid a bug that I introduced. > > This is just something for Junio to look out for while queuing, Actually that is something contributors learn to play better with each other ;-) Thanks for a heads-up.
On Fri, Jul 10, 2020 at 11:50:08AM -0700, Junio C Hamano wrote: > > Taylor Blau <me@ttaylorr.com> writes: > > > To me too, although note that this will generate merge conflicts with > > Szeder's patch from earlier today[1]. > > > > Unfortunately, the conflicts are a little deeper than "we both removed > > an unnecessary 'if' statement", since Szeder's patch moves the > > 'stop_progress()' call earlier to avoid a bug that I introduced. > > > > This is just something for Junio to look out for while queuing, > > Actually that is something contributors learn to play better with > each other ;-) Yeah, I was worried about the nature of this series in general. Sorry to have missed Szeder's change. > > Thanks for a heads-up. Will it make your life easier if I base this series on the other topic? Or would you rather I leave it to you? - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > Will it make your life easier if I base this series on the other topic? > Or would you rather I leave it to you? Either is fine. Just don't expect any quick turnaround time for a new and involved change like this while we are entering the pre-release freeze. We are supposed to be stablizing, not churning ;-).
On Fri, Jul 10, 2020 at 12:58:54PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > Will it make your life easier if I base this series on the other topic? > > Or would you rather I leave it to you? > > Either is fine. > > Just don't expect any quick turnaround time for a new and involved > change like this while we are entering the pre-release freeze. We > are supposed to be stablizing, not churning ;-). Understood. It's got a nit or two from Stolee's review so I need to reroll it anyway; I'll send it based on the other topic. - Emily
On Fri, Jul 10, 2020 at 01:29:55PM -0700, Emily Shaffer wrote: > > On Fri, Jul 10, 2020 at 12:58:54PM -0700, Junio C Hamano wrote: > > > > Emily Shaffer <emilyshaffer@google.com> writes: > > > > > Will it make your life easier if I base this series on the other topic? > > > Or would you rather I leave it to you? > > > > Either is fine. > > > > Just don't expect any quick turnaround time for a new and involved > > change like this while we are entering the pre-release freeze. We > > are supposed to be stablizing, not churning ;-). > > Understood. > > It's got a nit or two from Stolee's review so I need to reroll it > anyway; I'll send it based on the other topic. Hum. It seems that, as of `git fetch github.com/gitster/git` in the past hour, sg/commit-graph-progress-fix doesn't include es/trace-log-progress, so I don't think it is actually a good idea for me to submit this series based on sg/commit-graph-progress-fix instead. I'll send the next version based on master, after all. > > - Emily
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index ae4dc2dfcd..7964de3126 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -263,8 +263,7 @@ static int graph_write(int argc, const char **argv) cleanup: string_list_clear(&pack_indexes, 0); strbuf_release(&buf); - if (progress) - stop_progress(&progress); + stop_progress(&progress); return result; } diff --git a/commit-graph.c b/commit-graph.c index b9a784fece..30d9a75932 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1424,8 +1424,7 @@ int write_commit_graph_reachable(struct object_directory *odb, flags, split_opts); oidset_clear(&commits); - if (data.progress) - stop_progress(&data.progress); + stop_progress(&data.progress); return result; } diff --git a/read-cache.c b/read-cache.c index 2ddc422dbd..feb7abe37a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1581,8 +1581,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, new_entry = refresh_cache_ent(istate, ce, options, &cache_errno, &changed); if (new_entry == ce) continue; - if (progress) - display_progress(progress, i); + display_progress(progress, i); if (!new_entry) { const char *fmt; @@ -1614,10 +1613,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, replace_index_entry(istate, i, new_entry); } - if (progress) { - display_progress(progress, istate->cache_nr); - stop_progress(&progress); - } + display_progress(progress, istate->cache_nr); + stop_progress(&progress); trace_performance_leave("refresh index"); return has_errors; }
display_progress() and stop_progress() are tolerant to NULL inputs. Remove some places where redundant checks are performed before these NULL-tolerant functions are called. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/commit-graph.c | 3 +-- commit-graph.c | 3 +-- read-cache.c | 9 +++------ 3 files changed, 5 insertions(+), 10 deletions(-)