| Message ID | pull.315.v2.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | commit-graph: add --[no-]progress to write and verify | expand |
Ping :) Any more comments or concerns about this? On 8/26/2019 12:29 PM, Garima Singh via GitGitGadget wrote: > 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 7b0f229222 > ("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: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com > > Garima Singh (1): > commit-graph: add --[no-]progress to write and verify. > > Documentation/git-commit-graph.txt | 7 ++++-- > builtin/commit-graph.c | 21 ++++++++++++----- > commit-graph.c | 6 +++-- > t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++ > t/t5324-split-commit-graph.sh | 2 +- > 5 files changed, 61 insertions(+), 11 deletions(-) > > > base-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/315 > > Range-diff vs v1: > > 1: da89f7dadb ! 1: 47cc99bd15 commit-graph: add --[no-]progress to write and verify. > @@ -17,16 +17,19 @@ > [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>] > ++'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress] > > > + DESCRIPTION > @@ > commit-graph file is expected to be in the `<dir>/info` directory and > the packfiles are expected to be in `<dir>/pack`. > > +--[no-]progress:: > -+ Toggle whether to show progress or not. > ++ Turn progress on/off explicitly. If neither is specified, progress is > ++ shown if standard error is connected to a terminal. > > COMMANDS > -------- > @@ -35,11 +38,6 @@ > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ > - #include "repository.h" > - #include "commit-graph.h" > - #include "object-store.h" > -+#include "unistd.h" > - > static char const * const builtin_commit_graph_usage[] = { > N_("git commit-graph [--object-dir <objdir>]"), > N_("git commit-graph read [--object-dir <objdir>]"), > @@ -74,15 +72,6 @@ > > static int graph_verify(int argc, const char **argv) > @@ > - int fd; > - struct stat st; > - int flags = 0; > -- > -+ int defaultProgressState = isatty(2); > -+ > - static struct option builtin_commit_graph_verify_options[] = { > - OPT_STRING(0, "object-dir", &opts.obj_dir, > - N_("dir"), > 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")), > @@ -90,8 +79,7 @@ > OPT_END(), > }; > > -+ opts.progress = defaultProgressState; > -+ > ++ opts.progress = isatty(2); > argc = parse_options(argc, argv, NULL, > builtin_commit_graph_verify_options, > builtin_commit_graph_verify_usage, 0); > @@ -101,7 +89,7 @@ > flags |= COMMIT_GRAPH_VERIFY_SHALLOW; > - > + if (opts.progress) > -+ flags |= COMMIT_GRAPH_PROGRESS; > ++ flags |= COMMIT_GRAPH_WRITE_PROGRESS; > + > graph_name = get_commit_graph_filename(opts.obj_dir); > open_ok = open_commit_graph(graph_name, &fd, &st); > @@ -110,14 +98,11 @@ > struct string_list *commit_hex = NULL; > struct string_list lines; > int result = 0; > -- unsigned int flags = COMMIT_GRAPH_PROGRESS; > -- > -+ unsigned int flags = 0; > -+ int defaultProgressState = isatty(2); > -+ > +- 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, > - N_("dir"), > @@ > N_("start walk at commits listed by stdin")), > OPT_BOOL(0, "append", &opts.append, > @@ -130,16 +115,16 @@ > OPT_END(), > }; > > -+ opts.progress = defaultProgressState; > ++ opts.progress = isatty(2); > split_opts.size_multiple = 2; > split_opts.max_commits = 0; > split_opts.expire_time = 0; > @@ > - flags |= COMMIT_GRAPH_APPEND; > + flags |= COMMIT_GRAPH_WRITE_APPEND; > if (opts.split) > - flags |= COMMIT_GRAPH_SPLIT; > + flags |= COMMIT_GRAPH_WRITE_SPLIT; > + if (opts.progress) > -+ flags |= COMMIT_GRAPH_PROGRESS; > ++ flags |= COMMIT_GRAPH_WRITE_PROGRESS; > > read_replace_refs = 0; > > @@ -153,20 +138,13 @@ > > - progress = start_progress(_("Verifying commits in commit graph"), > - g->num_commits); > -+ if (flags & COMMIT_GRAPH_PROGRESS) > ++ 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; > - uint32_t max_generation = 0; > - > - display_progress(progress, i + 1); > -+ > - hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i); > - > - graph_commit = lookup_commit(r, &cur_oid); > > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > --- a/t/t5318-commit-graph.sh > @@ -175,7 +153,7 @@ > git repack > ' > > -+test_expect_success 'commit-graph write progress off by default for stderr' ' > ++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 > @@ -193,7 +171,7 @@ > + test_line_count = 0 err > +' > + > -+test_expect_success 'commit-graph verify progress off by default for stderr' ' > ++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 >
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 7b0f229222 ("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: stolee@gmail.com, avarab@gmail.com, garimasigit@gmail.com Garima Singh (1): commit-graph: add --[no-]progress to write and verify. Documentation/git-commit-graph.txt | 7 ++++-- builtin/commit-graph.c | 21 ++++++++++++----- commit-graph.c | 6 +++-- t/t5318-commit-graph.sh | 36 ++++++++++++++++++++++++++++++ t/t5324-split-commit-graph.sh | 2 +- 5 files changed, 61 insertions(+), 11 deletions(-) base-commit: 745f6812895b31c02b29bdfe4ae8e5498f776c26 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-315%2Fgarimasi514%2FcoreGit-commit-graph-progress-toggle-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-315/garimasi514/coreGit-commit-graph-progress-toggle-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/315 Range-diff vs v1: 1: da89f7dadb ! 1: 47cc99bd15 commit-graph: add --[no-]progress to write and verify. @@ -17,16 +17,19 @@ [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>] ++'git commit-graph write' <options> [--object-dir <dir>] [--[no-]progress] + DESCRIPTION @@ commit-graph file is expected to be in the `<dir>/info` directory and the packfiles are expected to be in `<dir>/pack`. +--[no-]progress:: -+ Toggle whether to show progress or not. ++ Turn progress on/off explicitly. If neither is specified, progress is ++ shown if standard error is connected to a terminal. COMMANDS -------- @@ -35,11 +38,6 @@ --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ - #include "repository.h" - #include "commit-graph.h" - #include "object-store.h" -+#include "unistd.h" - static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph [--object-dir <objdir>]"), N_("git commit-graph read [--object-dir <objdir>]"), @@ -74,15 +72,6 @@ static int graph_verify(int argc, const char **argv) @@ - int fd; - struct stat st; - int flags = 0; -- -+ int defaultProgressState = isatty(2); -+ - static struct option builtin_commit_graph_verify_options[] = { - OPT_STRING(0, "object-dir", &opts.obj_dir, - N_("dir"), 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")), @@ -90,8 +79,7 @@ OPT_END(), }; -+ opts.progress = defaultProgressState; -+ ++ opts.progress = isatty(2); argc = parse_options(argc, argv, NULL, builtin_commit_graph_verify_options, builtin_commit_graph_verify_usage, 0); @@ -101,7 +89,7 @@ flags |= COMMIT_GRAPH_VERIFY_SHALLOW; - + if (opts.progress) -+ flags |= COMMIT_GRAPH_PROGRESS; ++ flags |= COMMIT_GRAPH_WRITE_PROGRESS; + graph_name = get_commit_graph_filename(opts.obj_dir); open_ok = open_commit_graph(graph_name, &fd, &st); @@ -110,14 +98,11 @@ struct string_list *commit_hex = NULL; struct string_list lines; int result = 0; -- unsigned int flags = COMMIT_GRAPH_PROGRESS; -- -+ unsigned int flags = 0; -+ int defaultProgressState = isatty(2); -+ +- 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, - N_("dir"), @@ N_("start walk at commits listed by stdin")), OPT_BOOL(0, "append", &opts.append, @@ -130,16 +115,16 @@ OPT_END(), }; -+ opts.progress = defaultProgressState; ++ opts.progress = isatty(2); split_opts.size_multiple = 2; split_opts.max_commits = 0; split_opts.expire_time = 0; @@ - flags |= COMMIT_GRAPH_APPEND; + flags |= COMMIT_GRAPH_WRITE_APPEND; if (opts.split) - flags |= COMMIT_GRAPH_SPLIT; + flags |= COMMIT_GRAPH_WRITE_SPLIT; + if (opts.progress) -+ flags |= COMMIT_GRAPH_PROGRESS; ++ flags |= COMMIT_GRAPH_WRITE_PROGRESS; read_replace_refs = 0; @@ -153,20 +138,13 @@ - progress = start_progress(_("Verifying commits in commit graph"), - g->num_commits); -+ if (flags & COMMIT_GRAPH_PROGRESS) ++ 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; - uint32_t max_generation = 0; - - display_progress(progress, i + 1); -+ - hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i); - - graph_commit = lookup_commit(r, &cur_oid); diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh --- a/t/t5318-commit-graph.sh @@ -175,7 +153,7 @@ git repack ' -+test_expect_success 'commit-graph write progress off by default for stderr' ' ++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 @@ -193,7 +171,7 @@ + test_line_count = 0 err +' + -+test_expect_success 'commit-graph verify progress off by default for stderr' ' ++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