Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

commit-graph: add --[no-]progress to write and verify #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions Documentation/git-commit-graph.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ SYNOPSIS
--------
Copy link

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" <gitgitgadget@gmail.com> 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.

Copy link

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

>  	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/

>  	test_i18ngrep ! warning err &&
>  	test_line_count = 3 $graphdir/commit-graph-chain
> -- 
> gitgitgadget

Copy link

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

Copy link

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

[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]


DESCRIPTION
Expand All @@ -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
shown if standard error is connected to a terminal.

COMMANDS
--------
Expand Down
21 changes: 15 additions & 6 deletions builtin/commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--object-dir <objdir>]"),
N_("git commit-graph read [--object-dir <objdir>]"),
N_("git commit-graph verify [--object-dir <objdir>] [--shallow]"),
N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] <split options>"),
N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
NULL
};

static const char * const builtin_commit_graph_verify_usage[] = {
N_("git commit-graph verify [--object-dir <objdir>] [--shallow]"),
N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
NULL
};

Expand All @@ -26,7 +26,7 @@ static const char * const builtin_commit_graph_read_usage[] = {
};

static const char * const builtin_commit_graph_write_usage[] = {
N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] <split options>"),
N_("git commit-graph write [--object-dir <objdir>] [--append|--split] [--reachable|--stdin-packs|--stdin-commits] [--[no-]progress] <split options>"),
NULL
};

Expand All @@ -38,6 +38,7 @@ static struct opts_commit_graph {
int append;
int split;
int shallow;
int progress;
} opts;

static int graph_verify(int argc, const char **argv)
Expand All @@ -55,9 +56,11 @@ static int graph_verify(int argc, const char **argv)
N_("The object directory to store the graph")),
OPT_BOOL(0, "shallow", &opts.shallow,
N_("if the commit-graph is split, only verify the tip file")),
OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
OPT_END(),
};

opts.progress = isatty(2);
argc = parse_options(argc, argv, NULL,
builtin_commit_graph_verify_options,
builtin_commit_graph_verify_usage, 0);
Expand All @@ -66,7 +69,9 @@ static int graph_verify(int argc, const char **argv)
opts.obj_dir = get_object_directory();
if (opts.shallow)
flags |= COMMIT_GRAPH_VERIFY_SHALLOW;

if (opts.progress)
flags |= COMMIT_GRAPH_WRITE_PROGRESS;

graph_name = get_commit_graph_filename(opts.obj_dir);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok && errno != ENOENT)
Expand Down Expand Up @@ -154,7 +159,7 @@ static int graph_write(int argc, const char **argv)
struct string_list *commit_hex = NULL;
struct string_list lines;
int result = 0;
enum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_PROGRESS;
enum commit_graph_write_flags flags = 0;

static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", &opts.obj_dir,
Expand All @@ -168,6 +173,7 @@ static int graph_write(int argc, const char **argv)
N_("start walk at commits listed by stdin")),
OPT_BOOL(0, "append", &opts.append,
N_("include all commits already in the commit-graph file")),
OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
OPT_BOOL(0, "split", &opts.split,
N_("allow writing an incremental commit-graph file")),
OPT_INTEGER(0, "max-commits", &split_opts.max_commits,
Expand All @@ -179,6 +185,7 @@ static int graph_write(int argc, const char **argv)
OPT_END(),
};

opts.progress = isatty(2);
split_opts.size_multiple = 2;
split_opts.max_commits = 0;
split_opts.expire_time = 0;
Expand All @@ -195,6 +202,8 @@ static int graph_write(int argc, const char **argv)
flags |= COMMIT_GRAPH_WRITE_APPEND;
if (opts.split)
flags |= COMMIT_GRAPH_WRITE_SPLIT;
if (opts.progress)
flags |= COMMIT_GRAPH_WRITE_PROGRESS;

read_replace_refs = 0;

Expand Down
6 changes: 4 additions & 2 deletions commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit, *odb_commit;
struct commit_list *graph_parents, *odb_parents;
Expand Down
36 changes: 36 additions & 0 deletions t/t5318-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,42 @@ test_expect_success 'Add more commits' '
git repack
'

test_expect_success 'commit-graph write progress off for redirected stderr' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph write 2>err &&
test_line_count = 0 err
'

test_expect_success 'commit-graph write force progress on for stderr' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph write --progress 2>err &&
test_file_not_empty err
'

test_expect_success 'commit-graph write with the --no-progress option' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph write --no-progress 2>err &&
test_line_count = 0 err
'

test_expect_success 'commit-graph verify progress off for redirected stderr' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph verify 2>err &&
test_line_count = 0 err
'

test_expect_success 'commit-graph verify force progress on for stderr' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph verify --progress 2>err &&
test_file_not_empty err
'

test_expect_success 'commit-graph verify with the --no-progress option' '
cd "$TRASH_DIRECTORY/full" &&
git commit-graph verify --no-progress 2>err &&
test_line_count = 0 err
'

# Current graph structure:
#
# __M3___
Expand Down
2 changes: 1 addition & 1 deletion t/t5324-split-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
test_line_count = 3 err &&
test_i18ngrep ! warning err &&
test_line_count = 3 $graphdir/commit-graph-chain
Expand Down