@matheusbsbs / matheus-bagliano / commits / 1d7297513d

Fork 0

notes: break set_display_notes() into smaller functions

In 8164c961e1 (format-patch: use --notes behavior for format.notes, 2019-12-09), we introduced set_display_notes() which was a monolithic function with three mutually exclusive branches. Break the function up into three small and simple functions that each are only responsible for one task. This family of functions accepts an `int *show_notes` instead of returning a value suitable for assignment to `show_notes`. This is for two reasons. First of all, this guarantees that the external `show_notes` variable changes in lockstep with the `struct display_notes_opt`. Second, this prompts future developers to be careful about doing something meaningful with this value. In fact, a NULL check is intentionally omitted because causing a segfault here would tell the future developer that they are meant to use the value for something meaningful. One alternative was making the family of functions accept a `struct rev_info *` instead of the `struct display_notes_opt *`, since the former contains the `show_notes` field as well. This does not work because we have to call git_config() before repo_init_revisions(). However, if we had a `struct rev_info`, we'd need to initialize it before it gets assigned values from git_config(). As a result, we break the circular dependency by having standalone `int show_notes` and `struct display_notes_opt notes_opt` variables which temporarily hold values from git_config() until the values are copied over to `rev`. To implement this change, we need to get a pointer to `rev_info::show_notes`. Unfortunately, this is not possible with bitfields and only direct-assignment is possible. Change `rev_info::show_notes` to a non-bitfield int so that we can get its address. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Denton Liu committed Dec 11, 2019 at 16:49 UTC 1d7297513df66873e68af4b254804151b8ba5359

5 files changed +45 -32

builtin/log.c

+6 -1

@@ -868,7 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb)

868 }

869 if (!strcmp(var, "format.notes")) {

870 int b = git_parse_maybe_bool(value);

871 - show_notes = set_display_notes(&notes_opt, b, b < 0 ? value : NULL);

871 + if (b < 0)

872 + enable_ref_display_notes(&notes_opt, &show_notes, value);

873 + else if (b)

874 + enable_default_display_notes(&notes_opt, &show_notes);

875 + else

876 + disable_display_notes(&notes_opt, &show_notes);

877 return 0;

878 }

879

notes.c

+23 -20

@@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt)

1045 opt->use_default_notes = -1;

1046 }

1047

1048 -int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)

1048 +void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes)

1049 {

1050 - if (show_notes) {

1051 - if (opt_ref) {

1052 - struct strbuf buf = STRBUF_INIT;

1053 - strbuf_addstr(&buf, opt_ref);

1054 - expand_notes_ref(&buf);

1055 - string_list_append(&opt->extra_notes_refs,

1056 - strbuf_detach(&buf, NULL));

1057 - } else {

1058 - opt->use_default_notes = 1;

1059 - }

1060 - } else {

1061 - opt->use_default_notes = -1;

1062 - /* we have been strdup'ing ourselves, so trick

1063 - * string_list into free()ing strings */

1064 - opt->extra_notes_refs.strdup_strings = 1;

1065 - string_list_clear(&opt->extra_notes_refs, 0);

1066 - opt->extra_notes_refs.strdup_strings = 0;

1067 - }

1050 + opt->use_default_notes = 1;

1051 + *show_notes = 1;

1052 +}

1053

1069 - return !!show_notes;

1054 +void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes,

1055 + const char *ref) {

1056 + struct strbuf buf = STRBUF_INIT;

1057 + strbuf_addstr(&buf, ref);

1058 + expand_notes_ref(&buf);

1059 + string_list_append(&opt->extra_notes_refs,

1060 + strbuf_detach(&buf, NULL));

1061 + *show_notes = 1;

1062 +}

1063 +

1064 +void disable_display_notes(struct display_notes_opt *opt, int *show_notes)

1065 +{

1066 + opt->use_default_notes = -1;

1067 + /* we have been strdup'ing ourselves, so trick

1068 + * string_list into free()ing strings */

1069 + opt->extra_notes_refs.strdup_strings = 1;

1070 + string_list_clear(&opt->extra_notes_refs, 0);

1071 + opt->extra_notes_refs.strdup_strings = 0;

1072 + *show_notes = 0;

1073 }

1074

1075 void load_display_notes(struct display_notes_opt *opt)

notes.h

+12 -7

@@ -266,14 +266,19 @@ struct display_notes_opt {

266 void init_display_notes(struct display_notes_opt *opt);

267

268 /*

269 - * Set a display_notes_opt to a given state. 'show_notes' is a boolean

270 - * representing whether or not to show notes. 'opt_ref' points to a

271 - * string for the notes ref, or is NULL if the default notes should be

272 - * used.

273 - *

274 - * Return 'show_notes' normalized to 1 or 0.

269 + * This family of functions enables or disables the display of notes. In

270 + * particular, 'enable_default_display_notes' will display the default notes,

271 + * 'enable_default_display_notes' will display the notes ref 'ref' and

272 + * 'disable_display_notes' will disable notes, including those added by previous

273 + * invocations of the 'enable_*_display_notes' functions.

274 + *

275 + * 'show_notes' is a points to a boolean which will be set to 1 if notes are

276 + * displayed, else 0. It must not be NULL.

277 */

276 -int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);

278 +void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes);

279 +void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes,

280 + const char *ref);

281 +void disable_display_notes(struct display_notes_opt *opt, int *show_notes);

282

283 /*

284 * Load the notes machinery for displaying several notes trees.

revision.c

+3 -3

@@ -2172,7 +2172,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg

2172 die("'%s': not a non-negative integer", arg);

2173 revs->expand_tabs_in_log = val;

2174 } else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {

2175 - revs->show_notes = set_display_notes(&revs->notes_opt, 1, NULL);

2175 + enable_default_display_notes(&revs->notes_opt, &revs->show_notes);

2176 revs->show_notes_given = 1;

2177 } else if (!strcmp(arg, "--show-signature")) {

2178 revs->show_signature = 1;

@@ -2191,10 +2191,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg

2191 if (starts_with(arg, "--show-notes=") &&

2192 revs->notes_opt.use_default_notes < 0)

2193 revs->notes_opt.use_default_notes = 1;

2194 - revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);

2194 + enable_ref_display_notes(&revs->notes_opt, &revs->show_notes, optarg);

2195 revs->show_notes_given = 1;

2196 } else if (!strcmp(arg, "--no-notes")) {

2197 - revs->show_notes = set_display_notes(&revs->notes_opt, 0, NULL);

2197 + disable_display_notes(&revs->notes_opt, &revs->show_notes);

2198 revs->show_notes_given = 1;

2199 } else if (!strcmp(arg, "--standard-notes")) {

2200 revs->show_notes_given = 1;

revision.h

+1 -1

@@ -177,10 +177,10 @@ struct rev_info {

177 always_show_header:1;

178

179 /* Format info */

180 + int show_notes;

181 unsigned int shown_one:1,

182 shown_dashes:1,

183 show_merge:1,

183 - show_notes:1,

184 show_notes_given:1,

185 show_signature:1,

186 pretty_given:1,