From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 02 Sep 2021 22:47:05 GMT
Subject: Re: [PATCH v3 5/5] bundle: show progress on "unbundle"
Message-ID: <87zgsu8ui3.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqmtp28hbv.fsf@gitster.g>


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

> Æ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!

>> +	}
>> +
>> +	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 :)