git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Garima Singh via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Garima Singh <garima.singh@microsoft.com>
Subject: Re: [PATCH v2 1/1] commit-graph: add --[no-]progress to write and verify.
Date: Tue, 17 Sep 2019 14:22:15 +0200	[thread overview]
Message-ID: <20190917122215.GA29845@szeder.dev> (raw)
In-Reply-To: <7a9581ea-dc90-5ce1-fc3b-578c6dbf6efc@gmail.com>

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

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@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,

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


  reply	other threads:[~2019-09-17 12:22 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-20 18:37 [PATCH 0/1] commit-graph: add --[no-]progress to write and verify Garima Singh via GitGitGadget
2019-08-20 18:37 ` [PATCH 1/1] " Garima Singh via GitGitGadget
2019-08-20 21:11   ` Junio C Hamano
2019-08-20 21:13   ` Eric Sunshine
2019-08-21 16:47     ` Junio C Hamano
2019-08-20 18:45 ` [PATCH 0/1] " Derrick Stolee
2019-08-26 16:29 ` [PATCH v2 " Garima Singh via GitGitGadget
2019-08-26 16:29   ` [PATCH v2 1/1] " Garima Singh via GitGitGadget
2019-09-12 20:40     ` Junio C Hamano
2019-09-16 22:36     ` SZEDER Gábor
2019-09-17 10:47       ` Derrick Stolee
2019-09-17 12:22         ` SZEDER Gábor [this message]
2019-09-10 14:00   ` [PATCH v2 0/1] " Garima Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190917122215.GA29845@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=garima.singh@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).