From 1d3878799f8260968ea9f6a75a92c4daca1da133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 21 Nov 2020 19:31:07 +0100 Subject: [PATCH 1/4] grep: don't set up a "default" repo for grep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `init_grep_defaults()` fills a `static struct grep_opt grep_defaults`. This struct is then used by `grep_init()` as a blueprint for other such structs. Notably, `grep_init()` takes a `struct repo *` and assigns it into the target struct. As a result, it is unnecessary for us to take a `struct repo *` in `init_grep_defaults()` as well. We assign it into the default struct and never look at it again. And in light of how we return early if we have already set up the default struct, it's not just unnecessary, but is also a bit confusing: If we are called twice and with different repos, is it a bug or a feature that we ignore the second repo? Drop the repo parameter for `init_grep_defaults()`. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- Documentation/MyFirstObjectWalk.txt | 2 +- builtin/grep.c | 2 +- builtin/log.c | 2 +- grep.c | 3 +-- grep.h | 2 +- revision.c | 2 +- 6 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt index c3f2d1a831..85434d1938 100644 --- a/Documentation/MyFirstObjectWalk.txt +++ b/Documentation/MyFirstObjectWalk.txt @@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add ---- static void init_walken_defaults(void) { - init_grep_defaults(the_repository); + init_grep_defaults(); } ... diff --git a/builtin/grep.c b/builtin/grep.c index e58e57504c..2b96efa8c2 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_END() }; - init_grep_defaults(the_repository); + init_grep_defaults(); git_config(grep_cmd_config, NULL); grep_init(&opt, the_repository, prefix); diff --git a/builtin/log.c b/builtin/log.c index 49eb8f6431..eee4beca4d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg, static void init_log_defaults(void) { - init_grep_defaults(the_repository); + init_grep_defaults(); init_diff_ui_defaults(); decoration_style = auto_decoration_style(); diff --git a/grep.c b/grep.c index 54af9f813e..b351449f7f 100644 --- a/grep.c +++ b/grep.c @@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes) * We could let the compiler do this, but without C99 initializers * the code gets unwieldy and unreadable, so... */ -void init_grep_defaults(struct repository *repo) +void init_grep_defaults(void) { struct grep_opt *opt = &grep_defaults; static int run_once; @@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo) run_once++; memset(opt, 0, sizeof(*opt)); - opt->repo = repo; opt->relative = 1; opt->pathname = 1; opt->max_depth = -1; diff --git a/grep.h b/grep.h index 9115db8515..1c5478f381 100644 --- a/grep.h +++ b/grep.h @@ -170,7 +170,7 @@ struct grep_opt { void *output_priv; }; -void init_grep_defaults(struct repository *); +void init_grep_defaults(void); int grep_config(const char *var, const char *value, void *); void grep_init(struct grep_opt *, struct repository *repo, const char *prefix); void grep_destroy(void); diff --git a/revision.c b/revision.c index aa62212040..f35ea1db11 100644 --- a/revision.c +++ b/revision.c @@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r, revs->commit_format = CMIT_FMT_DEFAULT; revs->expand_tabs_in_log_default = 8; - init_grep_defaults(revs->repo); + init_grep_defaults(); grep_init(&revs->grep_filter, revs->repo, prefix); revs->grep_filter.status_only = 1; From 96313423a75fa8d88b6ecd5a15c21a7fbaf9e9be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 21 Nov 2020 19:31:08 +0100 Subject: [PATCH 2/4] grep: use designated initializers for `grep_defaults` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In 15fabd1bbd ("builtin/grep.c: make configuration callback more reusable", 2012-10-09), we learned to fill a `static struct grep_opt grep_defaults` which we can use as a blueprint for other such structs. At the time, we didn't consider designated initializers to be widely useable, but these days, we do. (See, e.g., cbc0f81d96 ("strbuf: use designated initializers in STRBUF_INIT", 2017-07-10).) Use designated initializers to let the compiler set up the struct and so that we don't need to remember to call `init_grep_defaults()`. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- Documentation/MyFirstObjectWalk.txt | 10 +---- builtin/grep.c | 1 - builtin/log.c | 1 - grep.c | 64 +++++++++++------------------ grep.h | 1 - revision.c | 1 - 6 files changed, 26 insertions(+), 52 deletions(-) diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt index 85434d1938..7f4bffc4dd 100644 --- a/Documentation/MyFirstObjectWalk.txt +++ b/Documentation/MyFirstObjectWalk.txt @@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is equivalent to running `git log --author=`. We can add a filter by modifying `rev_info.grep_filter`, which is a `struct grep_opt`. -First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add -`grep_config()` to `git_walken_config()`: +First some setup. Add `grep_config()` to `git_walken_config()`: ---- -static void init_walken_defaults(void) -{ - init_grep_defaults(); -} - -... - static int git_walken_config(const char *var, const char *value, void *cb) { grep_config(var, value, cb); diff --git a/builtin/grep.c b/builtin/grep.c index 2b96efa8c2..ca259af441 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_END() }; - init_grep_defaults(); git_config(grep_cmd_config, NULL); grep_init(&opt, the_repository, prefix); diff --git a/builtin/log.c b/builtin/log.c index eee4beca4d..cf41714fb0 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg, static void init_log_defaults(void) { - init_grep_defaults(); init_diff_ui_defaults(); decoration_style = auto_decoration_style(); diff --git a/grep.c b/grep.c index b351449f7f..8f2009ec9f 100644 --- a/grep.c +++ b/grep.c @@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs); static int grep_source_is_binary(struct grep_source *gs, struct index_state *istate); -static struct grep_opt grep_defaults; +static void std_output(struct grep_opt *opt, const void *buf, size_t size) +{ + fwrite(buf, size, 1, stdout); +} + +static struct grep_opt grep_defaults = { + .relative = 1, + .pathname = 1, + .max_depth = -1, + .pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, + .colors = { + [GREP_COLOR_CONTEXT] = "", + [GREP_COLOR_FILENAME] = "", + [GREP_COLOR_FUNCTION] = "", + [GREP_COLOR_LINENO] = "", + [GREP_COLOR_COLUMNNO] = "", + [GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED, + [GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED, + [GREP_COLOR_SELECTED] = "", + [GREP_COLOR_SEP] = GIT_COLOR_CYAN, + }, + .only_matching = 0, + .color = -1, + .output = std_output, +}; #ifdef USE_LIBPCRE2 static pcre2_general_context *pcre2_global_context; @@ -42,49 +66,11 @@ static const char *color_grep_slots[] = { [GREP_COLOR_SEP] = "separator", }; -static void std_output(struct grep_opt *opt, const void *buf, size_t size) -{ - fwrite(buf, size, 1, stdout); -} - static void color_set(char *dst, const char *color_bytes) { xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes); } -/* - * Initialize the grep_defaults template with hardcoded defaults. - * We could let the compiler do this, but without C99 initializers - * the code gets unwieldy and unreadable, so... - */ -void init_grep_defaults(void) -{ - struct grep_opt *opt = &grep_defaults; - static int run_once; - - if (run_once) - return; - run_once++; - - memset(opt, 0, sizeof(*opt)); - opt->relative = 1; - opt->pathname = 1; - opt->max_depth = -1; - opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED; - color_set(opt->colors[GREP_COLOR_CONTEXT], ""); - color_set(opt->colors[GREP_COLOR_FILENAME], ""); - color_set(opt->colors[GREP_COLOR_FUNCTION], ""); - color_set(opt->colors[GREP_COLOR_LINENO], ""); - color_set(opt->colors[GREP_COLOR_COLUMNNO], ""); - color_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED); - color_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED); - color_set(opt->colors[GREP_COLOR_SELECTED], ""); - color_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN); - opt->only_matching = 0; - opt->color = -1; - opt->output = std_output; -} - static int parse_pattern_type_arg(const char *opt, const char *arg) { if (!strcmp(arg, "default")) diff --git a/grep.h b/grep.h index 1c5478f381..b5c4e223a8 100644 --- a/grep.h +++ b/grep.h @@ -170,7 +170,6 @@ struct grep_opt { void *output_priv; }; -void init_grep_defaults(void); int grep_config(const char *var, const char *value, void *); void grep_init(struct grep_opt *, struct repository *repo, const char *prefix); void grep_destroy(void); diff --git a/revision.c b/revision.c index f35ea1db11..963868f699 100644 --- a/revision.c +++ b/revision.c @@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r, revs->commit_format = CMIT_FMT_DEFAULT; revs->expand_tabs_in_log_default = 8; - init_grep_defaults(); grep_init(&revs->grep_filter, revs->repo, prefix); revs->grep_filter.status_only = 1; From 6ba9bb76e0279bce9f614cb7f4ee28d8a601e79e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 29 Nov 2020 20:52:21 +0100 Subject: [PATCH 3/4] grep: copy struct in one fell swoop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a `struct grep_opt` with our defaults which we then copy into the caller's struct. Rather than zeroing the target struct and copying each element one by one, just copy everything at once. This leaves the code simpler and more maintainable. We don't have any ownership issues with what we're copying now and can just greedily copy the whole thing. If and when we do need to handle such elements (`char *`?), we must and can handle it appropriately. Make sure to leave a comment to our future selves. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- grep.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/grep.c b/grep.c index 8f2009ec9f..efeb6dc58d 100644 --- a/grep.c +++ b/grep.c @@ -66,11 +66,6 @@ static const char *color_grep_slots[] = { [GREP_COLOR_SEP] = "separator", }; -static void color_set(char *dst, const char *color_bytes) -{ - xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes); -} - static int parse_pattern_type_arg(const char *opt, const char *arg) { if (!strcmp(arg, "default")) @@ -100,6 +95,14 @@ int grep_config(const char *var, const char *value, void *cb) if (userdiff_config(var, value) < 0) return -1; + /* + * The instance of grep_opt that we set up here is copied by + * grep_init() to be used by each individual invocation. + * When populating a new field of this structure here, be + * sure to think about ownership -- e.g., you might need to + * override the shallow copy in grep_init() with a deep copy. + */ + if (!strcmp(var, "grep.extendedregexp")) { opt->extended_regexp_option = git_config_bool(var, value); return 0; @@ -157,9 +160,6 @@ int grep_config(const char *var, const char *value, void *cb) */ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix) { - struct grep_opt *def = &grep_defaults; - int i; - #if defined(USE_LIBPCRE2) if (!pcre2_global_context) pcre2_global_context = pcre2_general_context_create( @@ -171,26 +171,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix pcre_free = free; #endif - memset(opt, 0, sizeof(*opt)); + *opt = grep_defaults; + opt->repo = repo; opt->prefix = prefix; opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0; opt->pattern_tail = &opt->pattern_list; opt->header_tail = &opt->header_list; - - opt->only_matching = def->only_matching; - opt->color = def->color; - opt->extended_regexp_option = def->extended_regexp_option; - opt->pattern_type_option = def->pattern_type_option; - opt->linenum = def->linenum; - opt->columnnum = def->columnnum; - opt->max_depth = def->max_depth; - opt->pathname = def->pathname; - opt->relative = def->relative; - opt->output = def->output; - - for (i = 0; i < NR_GREP_COLORS; i++) - color_set(opt->colors[i], def->colors[i]); } void grep_destroy(void) From 3bf97e12703d27ac1928f2551cc09bbf13597149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 29 Nov 2020 20:52:22 +0100 Subject: [PATCH 4/4] MyFirstObjectWalk: drop `init_walken_defaults()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a recent commit, we stopped calling `init_grep_defaults()` from this function. Thus, by the end of the tutorial, we still haven't added any contents to this function. Let's remove it for simplicity. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- Documentation/MyFirstObjectWalk.txt | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt index 7f4bffc4dd..2d10eea7a9 100644 --- a/Documentation/MyFirstObjectWalk.txt +++ b/Documentation/MyFirstObjectWalk.txt @@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks `grep` and `diff` to initialize themselves by calling each of their initialization functions. -For our first example within `git walken`, we don't intend to use any other -components within Git, and we don't have any configuration to do. However, we -may want to add some later, so for now, we can add an empty placeholder. Create -a new function in `builtin/walken.c`: - ----- -static void init_walken_defaults(void) -{ - /* - * We don't actually need the same components `git log` does; leave this - * empty for now. - */ -} ----- - -Make sure to add a line invoking it inside of `cmd_walken()`. - ----- -int cmd_walken(int argc, const char **argv, const char *prefix) -{ - init_walken_defaults(); -} ----- - ==== Configuring From `.gitconfig` Next, we should have a look at any relevant configuration settings (i.e.,