Re: [PATCH v3 5/5] bundle: show progress on "unbundle"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 2, 2021, 22:47 UTC
- Message-ID
- <87zgsu8ui3.fsf@evledraar.gmail.com>
- In-Reply-To
- <xmqqmtp28hbv.fsf@gitster.g>
On Fri, Aug 27 2021, Junio C Hamano wrote:
Show 8 quoted lines
> Æ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.Will fix. Thanks!
Show 12 quoted lines
>> + } >> + >> + 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 <30620e13-4509-1905-7644-9962b6adf9c5@gmail.com>, I'll just change it back :)