Re: [PATCH v3 5/5] bundle: show progress on "unbundle"

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2021, 01:54 UTC
Message-ID: <xmqqmtp28hbv.fsf@gitster.g>
In-Reply-To: <patch-v3-5.5-cd38b0f0fed-20210826T140414Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	if (progress) {
> +		strvec_push(&extra_args, "-v");
> +		strvec_push(&extra_args, "--progress-title");
> +		strvec_push(&extra_args, _("Unbundling objects"));

Nice.  I would have expected to see pushl() though.

> +	}
> +
> +	ret = !!unbundle(the_repository, &header, bundle_fd, progress ?
> +			 &extra_args : NULL) ||

Again, I wouldn't make the &extra_args conditional to progress here. Future code change may decide to pass more args to underlying index-pack and the criteria for doing so may be different from progress.

If this code cares about readability, it should uncondtionally pass &extra_args.

If this code cares about readability *and* micro-optimization, then the condition should be on !!extra_args.nr, not on whatever set of conditions happen to be used in today's code to throw items into extra_args array.

Other than that, this was a pleasant read.

Thanks.