Re: [PATCH 1/1] commit-graph: add --[no-]progress to write and verify.
From: Junio C Hamano <hidden>
Date: 2019-08-20 21:11:48
"Garima Singh via GitGitGadget" [off-list ref] writes:
From: Garima Singh <redacted>
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.
quoted hunk
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?
quoted hunk
@@ -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.
quoted hunk
@@ -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.
quoted hunk
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.