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

Ævar Arnfjörð Bjarmason  [off-list ref] 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.

On Fri, Aug 27 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

+	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.

Will fix. Thanks!

quoted

+	}
+
+	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.

I agree, but I landed myself in a game of reviewer ping-pong. I had it
that way originally, then Derrick Stolee suggested changing it in this
way in [off-list ref], I'll just
change it back :)