Message ID | da89f7dadb0be2d4ada22dd3e2d1f5524c73f70d.1566326275.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | commit-graph: add --[no-]progress to write and verify | expand |
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. Nicely described. > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > index 38027b83d9..71796910fc 100644 > --- a/builtin/commit-graph.c > +++ b/builtin/commit-graph.c > @@ -6,17 +6,18 @@ > #include "repository.h" > #include "commit-graph.h" > #include "object-store.h" > +#include "unistd.h" Please do not contaminate *.c files with #include of system headers. Often, various platforms require system include files in specific order, and the project convention is to include them in git-compat-util.h in the right order (with #ifdef and friends as necessary). *.c files are required to include git-compat-util.h (or one of the well known headers that include git-compat-util.h as the first one) as the first file. In fact, "builtin.h" includes "git-compat-util.h" as the first thing, and "git-compat-util.h" in turn includes unistd reasonably early. Do you really need to include it again here? > @@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv) > int fd; > struct stat st; > int flags = 0; > - > + int defaultProgressState = isatty(2); As you can see from the naming of other variables, we do not do camelCase variable names. In fact you do not need this variable, do you? > 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")), > + OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), > OPT_END(), > }; > > + opts.progress = defaultProgressState; ... as you can assign isatty(2) to opts.progress here directly. > @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv) > 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); Likewise. > diff --git a/commit-graph.c b/commit-graph.c > index fe954ab5f8..b10d47f99a 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -1986,14 +1986,17 @@ 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_PROGRESS) > + progress = start_progress(_("Verifying commits in commit graph"), > + g->num_commits); Makes sense. > 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); Drop this change---I do not see a reason for the extra blank line here.
On Tue, Aug 20, 2019 at 2:38 PM Garima Singh via GitGitGadget <gitgitgadget@gmail.com> wrote: > 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 <garima.singh@microsoft.com> > --- > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt > @@ -10,7 +10,7 @@ SYNOPSIS > 'git commit-graph read' [--object-dir <dir>] > -'git commit-graph verify' [--object-dir <dir>] [--shallow] > +'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress] > 'git commit-graph write' <options> [--object-dir <dir>] This synopsis shows only 'verify' accepting --[no-]progress, however, the "usage" message in commit-graph.c itself shows 'verify' and 'write' accepting the option. > @@ -29,6 +29,8 @@ OPTIONS > +--[no-]progress:: > + Toggle whether to show progress or not. This is misleading. The --progress option does not _toggle_ the setting. The positive form explicitly enables it, and the negated form explicitly disables it. Try to take inspiration for the wording of this description by consulting other existing documentation. For instance, from Documentation/merge-options.txt: Turn progress on/off explicitly. If neither is specified, progress is shown if standard error is connected to a terminal. > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c > @@ -6,17 +6,18 @@ > static char const * const builtin_commit_graph_usage[] = { > - 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>"), This disagrees with the synopsis in the documentation, as mentioned above. > static int graph_verify(int argc, const char **argv) > @@ -48,16 +50,20 @@ static int graph_verify(int argc, const char **argv) > int fd; > struct stat st; > int flags = 0; > - > + int defaultProgressState = isatty(2); > + You have stray whitespace at the end of the "blank" line following the new variable declaration, hence the odd-looking diff. > @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv) > - unsigned int flags = COMMIT_GRAPH_PROGRESS; > - > + unsigned int flags = 0; > + int defaultProgressState = isatty(2); > + Ditto. > diff --git a/commit-graph.c b/commit-graph.c > @@ -1986,14 +1986,17 @@ 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_PROGRESS) > + progress = start_progress(_("Verifying commits in commit graph"), > + g->num_commits); The progress reporting functions can safely handle a NULL pointer for 'progress'. In the original code 'progress' was assigned explicitly, thus could not be NULL, However, in the revised code, it's not clear from this snippet what the value of 'progress' is if COMMIT_GRAPH_PROGRESS is not in 'flags'. If 'progress' is uninitialized, then the behavior would be undefined. Looking at the variable declaration, I see that it is indeed initialized to NULL, so this code is safe. Okay. > 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); No need to make changes (such as inserting an unnecessary blank line) unrelated to the stated purposed of the patch. > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh > @@ -116,6 +116,42 @@ test_expect_success 'Add more commits' ' > +test_expect_success 'commit-graph write progress off by default for stderr' ' > + cd "$TRASH_DIRECTORY/full" && > + git commit-graph write 2>err && > + test_line_count = 0 err > +' Changing the working directory ('cd') outside of a subshell is heavily discouraged in this test suite, however, since this particular script is riddled with the 'cd "$TRASH_DIRECTORY/full"' idiom, this can probably pass, however, it's not good to get into the habit of doing it this way.
Eric Sunshine <sunshine@sunshineco.com> writes: > This synopsis shows only 'verify' accepting --[no-]progress, > however, ... > This is misleading. The --progress option does not _toggle_ the > setting. ... Thanks for a careful review.
diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt index eb5e7865f0..1256a80a81 100644 --- a/Documentation/git-commit-graph.txt +++ b/Documentation/git-commit-graph.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git commit-graph read' [--object-dir <dir>] -'git commit-graph verify' [--object-dir <dir>] [--shallow] +'git commit-graph verify' [--object-dir <dir>] [--shallow] [--[no-]progress] 'git commit-graph write' <options> [--object-dir <dir>] @@ -29,6 +29,8 @@ 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:: + Toggle whether to show progress or not. COMMANDS -------- diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index 38027b83d9..71796910fc 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -6,17 +6,18 @@ #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>]"), - 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 }; @@ -26,7 +27,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 }; @@ -38,6 +39,7 @@ static struct opts_commit_graph { int append; int split; int shallow; + int progress; } opts; static int graph_verify(int argc, const char **argv) @@ -48,16 +50,20 @@ 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")), + OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_END(), }; + opts.progress = defaultProgressState; + argc = parse_options(argc, argv, NULL, builtin_commit_graph_verify_options, builtin_commit_graph_verify_usage, 0); @@ -66,7 +72,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_PROGRESS; + graph_name = get_commit_graph_filename(opts.obj_dir); open_ok = open_commit_graph(graph_name, &fd, &st); if (!open_ok && errno != ENOENT) @@ -154,8 +162,9 @@ static int graph_write(int argc, const char **argv) 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); + static struct option builtin_commit_graph_write_options[] = { OPT_STRING(0, "object-dir", &opts.obj_dir, N_("dir"), @@ -168,6 +177,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, @@ -179,6 +189,7 @@ static int graph_write(int argc, const char **argv) OPT_END(), }; + opts.progress = defaultProgressState; split_opts.size_multiple = 2; split_opts.max_commits = 0; split_opts.expire_time = 0; @@ -195,6 +206,8 @@ static int graph_write(int argc, const char **argv) flags |= COMMIT_GRAPH_APPEND; if (opts.split) flags |= COMMIT_GRAPH_SPLIT; + if (opts.progress) + flags |= COMMIT_GRAPH_PROGRESS; read_replace_refs = 0; diff --git a/commit-graph.c b/commit-graph.c index fe954ab5f8..b10d47f99a 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1986,14 +1986,17 @@ 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_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 index 22cb9d6643..a98d9f88f4 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -116,6 +116,42 @@ test_expect_success 'Add more commits' ' git repack ' +test_expect_success 'commit-graph write progress off by default for 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 by default for 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___ 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 && test_line_count = 3 err && test_i18ngrep ! warning err && test_line_count = 3 $graphdir/commit-graph-chain