On Tue, Sep 17, 2019 at 06:47:38AM -0400, Derrick Stolee wrote:
>
> On 9/16/2019 6:36 PM, SZEDER Gábor wrote:
> > On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
> >> From: Garima Singh <garima.singh@xxxxxxxxxxxxx>
> >>
> >> 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.
OK, yesterday I overlooked that the patch added this line:
+ opts.progress = isatty(2);
So, the first question is whether that behavior change is desired; I
don't really have an opinion. But if it is desired, then it should be
changed in a separate patch, explaining why it is desired, I would
think.
> >> 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@xxxxxxxxx/
>
> 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,
Well, the way I see it the root issue is that the test checks things
that it shouldn't.
> 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.
Yeah, it should rather go on top of 'ds/commit-graph-octopus-fix'.
