| Message ID | patch-6.8-933ac853bca-20211106T210711Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: simplify & delete code by changing obscure cfg variable behavior | expand |
On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote: > The grep_init() function used the odd pattern of initializing the > passed-in "struct grep_opt" with a statically defined "grep_defaults" > struct, which would be modified in-place when we invoked > grep_config(). > > So we effectively (b) initialized config, (a) then defaults, (c) > followed by user options. Usually those are ordered as "a", "b" and > "c" instead. Do we risk changing any user-visible behavior here? Based on my reading of grep.c before and after this patch, I think the answer is "no", but I wasn't sure if you had done a similar analysis. In any case, I think the "bring your own structure" instead of getting one copied around is much easier to reason about. Even if we weren't accidentally stomping on ownership of the struct before, not having to reason about it is a nice benefit. > As the comments being removed here show the previous behavior needed > to be carefully explained as we'd potentially share the populated > configuration among different instances of grep_init(). In practice we > didn't do that, but now that it can't be a concern anymore let's > remove those comments. Makes sense, I agree. > diff --git a/builtin/grep.c b/builtin/grep.c > index 960c7aac123..7f95f44e948 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -288,7 +288,7 @@ static int wait_all(void) > static int grep_cmd_config(const char *var, const char *value, void *cb) > { > int st = grep_config(var, value, cb); > - if (git_color_default_config(var, value, cb) < 0) > + if (git_color_default_config(var, value, NULL) < 0) This doesn't appear strictly related to the rest of your changes, but only serves to prevent the caller-provided data from being sent down to git_color_default_config(). It didn't matter before because (a) the caller doesn't specify any data to begin with, and git_color_default_config() (or the functions that it calls) don't do anything with the extra pointer. Now cmd_grep() is going to start passing around a pointer to a struct grep_opt. But git_color_default_config() still doesn't do anything with the pointer it receives, and passing that pointer around is standard practice among config.c code. So I don't think that this hunk is strictly necessary, and it's somewhat different than the pattern established within config.c. I wouldn't be sad to see this hunk dropped (and in fact have a slight preference leaning this way), but I don't mind keeping it around, either. > st = -1; > > if (!strcmp(var, "grep.threads")) { > @@ -969,8 +969,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) > OPT_END() > }; > > - git_config(grep_cmd_config, NULL); > grep_init(&opt, the_repository); > + git_config(grep_cmd_config, &opt); > opt.caller_priv = &opt_cmd; > > /* > diff --git a/builtin/log.c b/builtin/log.c > index f75d87e8d7f..bfddacdfa6c 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -505,8 +505,6 @@ static int git_log_config(const char *var, const char *value, void *cb) > return 0; > } > > - if (grep_config(var, value, cb) < 0) > - return -1; > if (git_gpg_config(var, value, cb) < 0) > return -1; > return git_diff_ui_config(var, value, cb); > @@ -521,6 +519,8 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix) > git_config(git_log_config, NULL); > > repo_init_revisions(the_repository, &rev, prefix); > + git_config(grep_config, &rev.grep_filter); > + > rev.diff = 1; > rev.simplify_history = 0; > memset(&opt, 0, sizeof(opt)); > @@ -635,6 +635,8 @@ int cmd_show(int argc, const char **argv, const char *prefix) > > memset(&match_all, 0, sizeof(match_all)); > repo_init_revisions(the_repository, &rev, prefix); > + git_config(grep_config, &rev.grep_filter); > + > rev.diff = 1; > rev.always_show_header = 1; > rev.no_walk = 1; > @@ -718,6 +720,8 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix) > > repo_init_revisions(the_repository, &rev, prefix); > init_reflog_walk(&rev.reflog_info); > + git_config(grep_config, &rev.grep_filter); > + > rev.verbose_header = 1; > memset(&opt, 0, sizeof(opt)); > opt.def = "HEAD"; > @@ -751,6 +755,8 @@ int cmd_log(int argc, const char **argv, const char *prefix) > git_config(git_log_config, NULL); > > repo_init_revisions(the_repository, &rev, prefix); > + git_config(grep_config, &rev.grep_filter); > + > rev.always_show_header = 1; > memset(&opt, 0, sizeof(opt)); > opt.def = "HEAD"; > @@ -1833,10 +1839,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > extra_hdr.strdup_strings = 1; > extra_to.strdup_strings = 1; > extra_cc.strdup_strings = 1; > + > init_log_defaults(); > init_display_notes(¬es_opt); > git_config(git_format_config, NULL); > repo_init_revisions(the_repository, &rev, prefix); > + git_config(grep_config, &rev.grep_filter); > + > rev.show_notes = show_notes; > memcpy(&rev.notes_opt, ¬es_opt, sizeof(notes_opt)); > rev.commit_format = CMIT_FMT_EMAIL; > diff --git a/grep.c b/grep.c > index c9065254aeb..fb3f63c63ef 100644 > --- a/grep.c > +++ b/grep.c > @@ -19,27 +19,6 @@ 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, > -}; > - > static const char *color_grep_slots[] = { > [GREP_COLOR_CONTEXT] = "context", > [GREP_COLOR_FILENAME] = "filename", > @@ -75,20 +54,12 @@ define_list_config_array_extra(color_grep_slots, {"match"}); > */ > int grep_config(const char *var, const char *value, void *cb) > { > - struct grep_opt *opt = &grep_defaults; > + struct grep_opt *opt = cb; > const char *slot; > > 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; > @@ -134,14 +105,10 @@ int grep_config(const char *var, const char *value, void *cb) > return 0; > } > > -/* > - * Initialize one instance of grep_opt and copy the > - * default values from the template we read the configuration > - * information in an earlier call to git_config(grep_config). > - */ > void grep_init(struct grep_opt *opt, struct repository *repo) > { > - *opt = grep_defaults; > + struct grep_opt blank = GREP_OPT_INIT; > + memcpy(opt, &blank, sizeof(*opt)); I'm nit-picking, but creating a throwaway struct for the convenience of using designated initialization (at the cost of having to memcpy an entire struct around) seems like overkill. Especially since we're just going to write into the other fields of the the target struct anyway, I'd probably rather have seen everything written out explicitly without the throwaway or memcpy. Thanks, Taylor
On Mon, Nov 08 2021, Taylor Blau wrote: > On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote: >> The grep_init() function used the odd pattern of initializing the >> passed-in "struct grep_opt" with a statically defined "grep_defaults" >> struct, which would be modified in-place when we invoked >> grep_config(). >> >> So we effectively (b) initialized config, (a) then defaults, (c) >> followed by user options. Usually those are ordered as "a", "b" and >> "c" instead. > > Do we risk changing any user-visible behavior here? Based on my reading > of grep.c before and after this patch, I think the answer is "no", but I > wasn't sure if you had done a similar analysis. > > In any case, I think the "bring your own structure" instead of getting > one copied around is much easier to reason about. Even if we weren't > accidentally stomping on ownership of the struct before, not having to > reason about it is a nice benefit. I don't think we're changing any behavior except the one noted in this series. We only set a few config variables, so I thought that was fairly easy to trace... > [...] >> diff --git a/builtin/grep.c b/builtin/grep.c >> index 960c7aac123..7f95f44e948 100644 >> --- a/builtin/grep.c >> +++ b/builtin/grep.c >> @@ -288,7 +288,7 @@ static int wait_all(void) >> static int grep_cmd_config(const char *var, const char *value, void *cb) >> { >> int st = grep_config(var, value, cb); >> - if (git_color_default_config(var, value, cb) < 0) >> + if (git_color_default_config(var, value, NULL) < 0) > > This doesn't appear strictly related to the rest of your changes, but > only serves to prevent the caller-provided data from being sent down to > git_color_default_config(). > > It didn't matter before because (a) the caller doesn't specify any data > to begin with, and git_color_default_config() (or the functions that it > calls) don't do anything with the extra pointer. Now cmd_grep() is going > to start passing around a pointer to a struct grep_opt. > > But git_color_default_config() still doesn't do anything with the > pointer it receives, and passing that pointer around is standard > practice among config.c code. So I don't think that this hunk is > strictly necessary, and it's somewhat different than the pattern > established within config.c. > > I wouldn't be sad to see this hunk dropped (and in fact have a slight > preference leaning this way), but I don't mind keeping it around, > either. Will either split it up or drop it. > [...] >> -/* >> - * Initialize one instance of grep_opt and copy the >> - * default values from the template we read the configuration >> - * information in an earlier call to git_config(grep_config). >> - */ >> void grep_init(struct grep_opt *opt, struct repository *repo) >> { >> - *opt = grep_defaults; >> + struct grep_opt blank = GREP_OPT_INIT; >> + memcpy(opt, &blank, sizeof(*opt)); > > I'm nit-picking, but creating a throwaway struct for the convenience of > using designated initialization (at the cost of having to memcpy an > entire struct around) seems like overkill. > > Especially since we're just going to write into the other fields of the > the target struct anyway, I'd probably rather have seen everything > written out explicitly without the throwaway or memcpy. It's a widely used pattern in the codebase at this point, see 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01) (mine, but I stole it from Jeff King). As his linked-to compiler test shows the memcpy() is optimized away, so modern compilers will treat these idioms the same way. There was a suggestions somewhere that we should prorably move to that "*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the board, it would be less verbose. But I haven't tested if it's as widely supported, so I've just been sticking with that blank/memcpy() pattern for "do init in terms of macro".
On Tue, Nov 09, 2021 at 03:06:22AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> -/* > >> - * Initialize one instance of grep_opt and copy the > >> - * default values from the template we read the configuration > >> - * information in an earlier call to git_config(grep_config). > >> - */ > >> void grep_init(struct grep_opt *opt, struct repository *repo) > >> { > >> - *opt = grep_defaults; > >> + struct grep_opt blank = GREP_OPT_INIT; > >> + memcpy(opt, &blank, sizeof(*opt)); > > > > I'm nit-picking, but creating a throwaway struct for the convenience of > > using designated initialization (at the cost of having to memcpy an > > entire struct around) seems like overkill. > > > > Especially since we're just going to write into the other fields of the > > the target struct anyway, I'd probably rather have seen everything > > written out explicitly without the throwaway or memcpy. > > It's a widely used pattern in the codebase at this point, see > 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT > macro, 2021-07-01) (mine, but I stole it from Jeff King). > > As his linked-to compiler test shows the memcpy() is optimized away, so > modern compilers will treat these idioms the same way. > > There was a suggestions somewhere that we should prorably move to that > "*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the > board, it would be less verbose. But I haven't tested if it's as widely > supported, so I've just been sticking with that blank/memcpy() pattern > for "do init in terms of macro". I do at least prefer memcpy() over *<x> = <y> when x and y are structures. But I wasn't aware that this was common in our codebase. Anyway, my suggestion was only along the lines of "you're already writing individual fields below, so why not just do that throughout instead of memcpy()-ing some of them via a macro which expands to a designated initializer?" But this is a cosmetic point, so whatever you feel fits in most with the surrounding style (so long as the pattern we're propagating isn't terrible, which is the case here) then I'm OK with it. Thanks, Taylor
diff --git a/builtin/grep.c b/builtin/grep.c index 960c7aac123..7f95f44e948 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -288,7 +288,7 @@ static int wait_all(void) static int grep_cmd_config(const char *var, const char *value, void *cb) { int st = grep_config(var, value, cb); - if (git_color_default_config(var, value, cb) < 0) + if (git_color_default_config(var, value, NULL) < 0) st = -1; if (!strcmp(var, "grep.threads")) { @@ -969,8 +969,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_END() }; - git_config(grep_cmd_config, NULL); grep_init(&opt, the_repository); + git_config(grep_cmd_config, &opt); opt.caller_priv = &opt_cmd; /* diff --git a/builtin/log.c b/builtin/log.c index f75d87e8d7f..bfddacdfa6c 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -505,8 +505,6 @@ static int git_log_config(const char *var, const char *value, void *cb) return 0; } - if (grep_config(var, value, cb) < 0) - return -1; if (git_gpg_config(var, value, cb) < 0) return -1; return git_diff_ui_config(var, value, cb); @@ -521,6 +519,8 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); repo_init_revisions(the_repository, &rev, prefix); + git_config(grep_config, &rev.grep_filter); + rev.diff = 1; rev.simplify_history = 0; memset(&opt, 0, sizeof(opt)); @@ -635,6 +635,8 @@ int cmd_show(int argc, const char **argv, const char *prefix) memset(&match_all, 0, sizeof(match_all)); repo_init_revisions(the_repository, &rev, prefix); + git_config(grep_config, &rev.grep_filter); + rev.diff = 1; rev.always_show_header = 1; rev.no_walk = 1; @@ -718,6 +720,8 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix) repo_init_revisions(the_repository, &rev, prefix); init_reflog_walk(&rev.reflog_info); + git_config(grep_config, &rev.grep_filter); + rev.verbose_header = 1; memset(&opt, 0, sizeof(opt)); opt.def = "HEAD"; @@ -751,6 +755,8 @@ int cmd_log(int argc, const char **argv, const char *prefix) git_config(git_log_config, NULL); repo_init_revisions(the_repository, &rev, prefix); + git_config(grep_config, &rev.grep_filter); + rev.always_show_header = 1; memset(&opt, 0, sizeof(opt)); opt.def = "HEAD"; @@ -1833,10 +1839,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) extra_hdr.strdup_strings = 1; extra_to.strdup_strings = 1; extra_cc.strdup_strings = 1; + init_log_defaults(); init_display_notes(¬es_opt); git_config(git_format_config, NULL); repo_init_revisions(the_repository, &rev, prefix); + git_config(grep_config, &rev.grep_filter); + rev.show_notes = show_notes; memcpy(&rev.notes_opt, ¬es_opt, sizeof(notes_opt)); rev.commit_format = CMIT_FMT_EMAIL; diff --git a/grep.c b/grep.c index c9065254aeb..fb3f63c63ef 100644 --- a/grep.c +++ b/grep.c @@ -19,27 +19,6 @@ 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, -}; - static const char *color_grep_slots[] = { [GREP_COLOR_CONTEXT] = "context", [GREP_COLOR_FILENAME] = "filename", @@ -75,20 +54,12 @@ define_list_config_array_extra(color_grep_slots, {"match"}); */ int grep_config(const char *var, const char *value, void *cb) { - struct grep_opt *opt = &grep_defaults; + struct grep_opt *opt = cb; const char *slot; 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; @@ -134,14 +105,10 @@ int grep_config(const char *var, const char *value, void *cb) return 0; } -/* - * Initialize one instance of grep_opt and copy the - * default values from the template we read the configuration - * information in an earlier call to git_config(grep_config). - */ void grep_init(struct grep_opt *opt, struct repository *repo) { - *opt = grep_defaults; + struct grep_opt blank = GREP_OPT_INIT; + memcpy(opt, &blank, sizeof(*opt)); opt->repo = repo; opt->pattern_tail = &opt->pattern_list; diff --git a/grep.h b/grep.h index 6b923d8599c..30a7dfd3294 100644 --- a/grep.h +++ b/grep.h @@ -178,6 +178,27 @@ struct grep_opt { void *output_priv; }; +#define GREP_OPT_INIT { \ + .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, \ +} + int grep_config(const char *var, const char *value, void *); void grep_init(struct grep_opt *, struct repository *repo); void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config(). So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead. As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments. See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/grep.c | 4 ++-- builtin/log.c | 13 +++++++++++-- grep.c | 39 +++------------------------------------ grep.h | 21 +++++++++++++++++++++ 4 files changed, 37 insertions(+), 40 deletions(-)