Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 17, 2019, 10:47 UTC
Message-ID: <7a9581ea-dc90-5ce1-fc3b-578c6dbf6efc@gmail.com>
In-Reply-To: <20190916223607.GE6190@szeder.dev>

On 9/16/2019 6:36 PM, SZEDER Gábor wrote:

Show 22 quoted lines

> On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
>> From: Garima Singh <garima.singh@microsoft.com>
>>
>> Add --[no-]progress to git commit-graph write and verify.
>> The progress feature was introduced in 7b0f229
>> ("commit-graph write: add progress output", 2018-09-17) but
>> the ability to opt-out was overlooked.
> 
>> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
>> index 99f4ef4c19..4fc3fda9d6 100755
>> --- a/t/t5324-split-commit-graph.sh
>> +++ b/t/t5324-split-commit-graph.sh
>> @@ -319,7 +319,7 @@ test_expect_success 'add octopus merge' '
>>  	git merge commits/3 commits/4 &&
>>  	git branch merge/octopus &&
>>  	git commit-graph write --reachable --split &&
>> -	git commit-graph verify 2>err &&
>> +	git commit-graph verify --progress 2>err &&
> 
> Why is it necessary to use '--progress' here?  It should not be
> necessary, because the commit message doesn't mention that it changed
> the default behavior of 'git commit-graph verify'...

It does change the default when stderr is not a terminal window. If we
were not redirecting to a file, this change would not be necessary.

Show 6 quoted lines

>>  	test_line_count = 3 err &&
> 
> Having said that, this test should not check the number of progress
> lines in the first place; see the recent discussion:
> 
> https://public-inbox.org/git/ec14865f-98cb-5e1a-b580-8b6fddaa6217@gmail.com/

True, this is an old issue. I think it never got corrected because your reply sounded like the issue doesn't exist in the normal test suite, only in a private branch where you changed the behavior of GIT_TEST_GETTEXT_POISON.

If we still think that should be fixed, it should not be a part of this series, but should be a separate one that focuses on just those changes.

Thanks, -Stolee