Re: [PATCH 2/2] progress: remove redundant null-checking

[Taylor Blau <me@ttaylorr.com>]

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