-
Notifications
You must be signed in to change notification settings - Fork 140
commit-graph: add --[no-]progress to write and verify #315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commit-graph: add --[no-]progress to write and verify #315
Conversation
Welcome to GitGitGadgetHi @garimasi514, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
6b6767b
to
d5b5e90
Compare
/allow |
User garimasi514 is now allowed to use GitGitGadget. |
d5b5e90
to
ca4d6c1
Compare
@garimasi514 it looks like there is a typo in your commit message:
The date of that commit should be |
Good catch, William! That typo is my fault, as I went to look for the format to write commit pointers, then sent it to Garima. |
01bc0fe
to
9bdc6bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more comment, it also looks like the date in the commit message still needs to be updated
9bdc6bc
to
16ba04e
Compare
16ba04e
to
da89f7d
Compare
/submit |
Submitted as [email protected] |
On the Git mailing list, Derrick Stolee wrote (reply to this):
|
e100ef3
to
0722c70
Compare
a4afdea
to
167cbb3
Compare
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. Signed-off-by: Garima Singh <[email protected]>
167cbb3
to
47cc99b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commit looks good to me
/submit |
Submitted as [email protected] |
On the Git mailing list, Garima Singh wrote (reply to this):
|
@@ -10,8 +10,8 @@ SYNOPSIS | |||
-------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Garima Singh via GitGitGadget" <[email protected]> writes:
> diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> index eb5e7865f0..ca0b1a683f 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,8 +10,8 @@ SYNOPSIS
> --------
> [verse]
> 'git commit-graph read' [--object-dir <dir>]
> -'git commit-graph verify' [--object-dir <dir>] [--shallow]
> -'git commit-graph write' <options> [--object-dir <dir>]
> +'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress]
> +'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress]
This is not a problem with this patch, but it is disturbing to see
<options> and other concrete "--option" listed explicitly. It could
be that "--object-dir <dir>" is so important an option that deserves
to be singled out while other random options can be left to individual
option's description, but in that case, would "--progress" be equally
important (if anything, as an option that is purely about appearance,
I would expect it to be with a lot lower importance)?
I guess with a preparatory clean-up patch to deal with the <options>
part, the result of applying this patch would not look so bad.
Perhaps renaming <options> to <write-specific-options> and moving it
to the end of the line might be sufficient. I dunno. At least we'd
need to make sure that it is clear to readers what options are
allowed where we wrote <options> above.
> @@ -29,6 +29,9 @@ OPTIONS
> commit-graph file is expected to be in the `<dir>/info` directory and
> the packfiles are expected to be in `<dir>/pack`.
>
> +--[no-]progress::
> + Turn progress on/off explicitly. If neither is specified, progress is
Trailing whitespace.
> + shown if standard error is connected to a terminal.
> ...
> + if (opts.progress)
> + flags |= COMMIT_GRAPH_WRITE_PROGRESS;
> +
Trailing whitespace.
> diff --git a/commit-graph.c b/commit-graph.c
> index f2888c203b..2802f2ade6 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1992,8 +1992,10 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)
> if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)
> return verify_commit_graph_error;
>
> - progress = start_progress(_("Verifying commits in commit graph"),
> - g->num_commits);
> + if (flags & COMMIT_GRAPH_WRITE_PROGRESS)
> + progress = start_progress(_("Verifying commits in commit graph"),
> + g->num_commits);
> +
This is correct, but it feels funny that it is sufficient to
castrate start_progress() and we do not have to muck with existing
calls to show and stop progress output. We rely on progress being
NULL for that to work, and existing code initializes the variable
to NULL, so we are OK.
This branch is now known as |
This patch series was integrated into pu via git@21d19b2. |
This patch series was integrated into pu via git@888472f. |
@@ -10,8 +10,8 @@ SYNOPSIS | |||
-------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Mon, Aug 26, 2019 at 09:29:58AM -0700, Garima Singh via GitGitGadget wrote:
> From: Garima Singh <[email protected]>
>
> 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'...
> 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/[email protected]/
> test_i18ngrep ! warning err &&
> test_line_count = 3 $graphdir/commit-graph-chain
> --
> gitgitgadget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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 <[email protected]>
>>
>> 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.
>> 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/[email protected]/
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
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 <[email protected]>
> >>
> >> 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/[email protected]/
>
> 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'.
This patch series was integrated into pu via git@b636c0e. |
This patch series was integrated into pu via git@0124200. |
This patch series was integrated into pu via git@caf150c. |
This patch series was integrated into next via git@caf150c. |
This patch series was integrated into master via git@caf150c. |
Closed via caf150c. |
Hey Git contributors!
My name is Garima Singh and I work at Microsoft. I recently started working
closely with the Microsoft team contributing to the git client ecosystem.
I am very glad to have the opportunity to work with this community. I am new
to the world of git client development but I did work on the Git service offering
of Azure Developer Services for a few years. I am sure I will get to learn a lot
from all of you.
Dr. Derrick Stolee helped me pick out my first task (Thanks Stolee!) He mentioned
an issue in the commit-graph builtin where git did not support opting in and out
of the progress output. This was bloating up the stderr logs in VFS for Git.
The progress feature was introduced in 7b0f229 ("commit-graph write: add
progress output", 2018-09-17) but the ability to opt-out was overlooked. This
patch adds the --no-progress option so that callers can control the amount of
logging they receive.
Looking forward to your review.
Cheers!
Garima Singh
CC: [email protected], [email protected], [email protected]