| Message ID | cover.1717073346.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Patrick Steinhardt <ps@pks.im> writes: > @@ reftable/readwrite_test.c: static void test_write_key_order(void) > + struct strbuf buf = STRBUF_INIT; > + struct reftable_writer *w = > reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); > ++ char a[] = "a", b[] = "b", target[] = "target"; So you decided to go in the complete opposite direction, hmph... I was hoping that we do not add more "writable" pieces of memory like target[] only to please the constness-strict compilers.
On Fri, May 31, 2024 at 02:13:27AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > @@ reftable/readwrite_test.c: static void test_write_key_order(void) > > + struct strbuf buf = STRBUF_INIT; > > + struct reftable_writer *w = > > reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); > > ++ char a[] = "a", b[] = "b", target[] = "target"; > > So you decided to go in the complete opposite direction, hmph... > > I was hoping that we do not add more "writable" pieces of memory > like target[] only to please the constness-strict compilers. I guess I misunderstood what you were saying. I'll revise this to go into the other direction. Patrick
Hi, this is the second version of my patch series that prepares our code base to compile with `-Wwrite-strings`. This warning will convert the type of string constants from `char []` to `const char[]` so that it becomes harder to for example write to or free such constants by accident. Changes compared to v2: - Merged the reftable-specific test into the second patch. Instead of adding casts, we now allocate the required strings on the stack. - Apply a micro-optimization to "remote-curl.c" that was suggested by Junio. - Restore the `nongit_ok` variable in "imap-send.c". It's somewhat concerning that we do not have test coverage for git-imap-send(1) at all. But it might be a bit more involved as we do not have any IMAP test infra, to the best of my knowledge. - Rework the patch to "builtin/rebase.c". It is now split into two patches. The first patch reworks initialization of the rebase options so that the defaults are still self-contained in a single place. And the second patch refactors how we set up the merge strategy. Thanks! Patrick Patrick Steinhardt (19): global: improve const correctness when assigning string constants global: assign non-const strings as required global: convert intentionally-leaking config strings to consts compat/win32: fix const-correctness with string constants refspec: remove global tag refspec structure http: do not assign string constant to non-const field line-log: always allocate the output prefix object-file: make `buf` parameter of `index_mem()` a constant parse-options: cast long name for OPTION_ALIAS send-pack: always allocate receive status remote-curl: avoid assigning string constant to non-const variable revision: always store allocated strings in output encoding mailmap: always store allocated strings in mailmap blob imap-send: drop global `imap_server_conf` variable imap-send: fix leaking memory in `imap_server_conf` builtin/rebase: do not assign default backend to non-constant field builtin/rebase: always store allocated string in `options.strategy` builtin/merge: always store allocated strings in `pull_twohead` config.mak.dev: enable `-Wwrite-strings` warning builtin/bisect.c | 3 +- builtin/blame.c | 2 +- builtin/bugreport.c | 2 +- builtin/check-ignore.c | 4 +- builtin/clone.c | 14 ++-- builtin/commit.c | 6 +- builtin/diagnose.c | 2 +- builtin/fetch.c | 11 ++- builtin/log.c | 2 +- builtin/mailsplit.c | 4 +- builtin/merge.c | 18 +++-- builtin/pull.c | 52 +++++++------- builtin/rebase.c | 81 ++++++++++++---------- builtin/receive-pack.c | 4 +- builtin/remote.c | 3 +- builtin/revert.c | 2 +- builtin/send-pack.c | 2 + compat/basename.c | 15 +++- compat/mingw.c | 25 +++---- compat/regex/regcomp.c | 2 +- compat/winansi.c | 2 +- config.mak.dev | 1 + diff.c | 7 +- diffcore-rename.c | 6 +- entry.c | 7 +- fmt-merge-msg.c | 2 +- fsck.c | 2 +- fsck.h | 2 +- gpg-interface.c | 6 +- http-backend.c | 2 +- http.c | 5 +- ident.c | 9 ++- imap-send.c | 130 ++++++++++++++++++++--------------- line-log.c | 21 +++--- mailmap.c | 2 +- merge-ll.c | 11 ++- object-file.c | 17 ++--- parse-options.h | 2 +- pretty.c | 7 +- refs.c | 2 +- refs.h | 2 +- refs/reftable-backend.c | 5 +- refspec.c | 13 ---- refspec.h | 1 - reftable/basics_test.c | 5 +- reftable/block_test.c | 4 +- reftable/merged_test.c | 52 +++++++------- reftable/readwrite_test.c | 60 +++++++++------- reftable/record.c | 6 +- reftable/stack_test.c | 87 ++++++++++++----------- remote-curl.c | 53 +++++++------- revision.c | 3 +- run-command.c | 2 +- send-pack.c | 2 +- t/helper/test-hashmap.c | 3 +- t/helper/test-json-writer.c | 10 +-- t/helper/test-regex.c | 4 +- t/helper/test-rot13-filter.c | 5 +- t/t3900-i18n-commit.sh | 1 + t/t3901-i18n-patch.sh | 1 + t/unit-tests/t-strbuf.c | 10 +-- trailer.c | 2 +- userdiff.c | 10 +-- userdiff.h | 12 ++-- wt-status.c | 2 +- 65 files changed, 479 insertions(+), 373 deletions(-) Range-diff against v1: 1: 25c31e550f = 1: 25c31e550f global: improve const correctness when assigning string constants 5: dc5d85257e ! 2: 3430bcc09b reftable: improve const correctness when assigning string constants @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - reftable: improve const correctness when assigning string constants + global: assign non-const strings as required - There are many cases in the reftable tests where we assign string - constants to non-const fields. This is harmless because we know that - those fields are only used for reading access, but will break once we - enable `-Wwrite-strings`. Add explicit casts to prepare for this. + There are several cases where we initialize non-const fields with string + constants. This is invalid and will cause warnings once we enable the + `-Wwrite-strings` warning. Adapt those cases to instead use string + arrays. Signed-off-by: Patrick Steinhardt <ps@pks.im> + ## builtin/remote.c ## +@@ builtin/remote.c: static int get_head_names(const struct ref *remote_refs, struct ref_states *stat + struct ref *ref, *matches; + struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map; + struct refspec_item refspec; ++ char refspec_str[] = "refs/heads/*"; + + memset(&refspec, 0, sizeof(refspec)); + refspec.force = 0; + refspec.pattern = 1; +- refspec.src = refspec.dst = "refs/heads/*"; ++ refspec.src = refspec.dst = refspec_str; + get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0); + matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"), + fetch_map, 1); + + ## diff.c ## +@@ diff.c: size_t fill_textconv(struct repository *r, + struct diff_filespec *df, + char **outbuf) + { ++ static char empty_str[] = ""; + size_t size; + + if (!driver) { + if (!DIFF_FILE_VALID(df)) { +- *outbuf = ""; ++ *outbuf = empty_str; + return 0; + } + if (diff_populate_filespec(r, df, NULL)) + + ## entry.c ## +@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) + struct string_list_item *filter, *path; + struct progress *progress = NULL; + struct delayed_checkout *dco = state->delayed_checkout; ++ char empty_str[] = ""; + + if (!state->delayed_checkout) + return errs; +@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) + if (!async_query_available_blobs(filter->string, &available_paths)) { + /* Filter reported an error */ + errs = 1; +- filter->string = ""; ++ filter->string = empty_str; + continue; + } + if (available_paths.nr <= 0) { +@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) + * filter from the list (see + * "string_list_remove_empty_items" call below). + */ +- filter->string = ""; ++ filter->string = empty_str; + continue; + } + +@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) + * Do not ask the filter for available blobs, + * again, as the filter is likely buggy. + */ +- filter->string = ""; ++ filter->string = empty_str; + continue; + } + ce = index_file_exists(state->istate, path->string, + + ## ident.c ## +@@ ident.c: static struct passwd *xgetpwuid_self(int *is_bogus) + pw = getpwuid(getuid()); + if (!pw) { + static struct passwd fallback; +- fallback.pw_name = "unknown"; ++ static char fallback_name[] = "unknown"; + #ifndef NO_GECOS_IN_PWENT +- fallback.pw_gecos = "Unknown"; ++ static char fallback_gcos[] = "Unknown"; ++#endif ++ ++ fallback.pw_name = fallback_name; ++#ifndef NO_GECOS_IN_PWENT ++ fallback.pw_gecos = fallback_gcos; + #endif + pw = &fallback; + if (is_bogus) + + ## line-log.c ## +@@ line-log.c: static int process_diff_filepair(struct rev_info *rev, + struct range_set tmp; + struct diff_ranges diff; + mmfile_t file_parent, file_target; ++ char empty_str[] = ""; + + assert(pair->two->path); + while (rg) { +@@ line-log.c: static int process_diff_filepair(struct rev_info *rev, + file_parent.ptr = pair->one->data; + file_parent.size = pair->one->size; + } else { +- file_parent.ptr = ""; ++ file_parent.ptr = empty_str; + file_parent.size = 0; + } + + + ## object-file.c ## +@@ object-file.c: static struct cached_object { + } *cached_objects; + static int cached_object_nr, cached_object_alloc; + ++static char empty_tree_buf[] = ""; + static struct cached_object empty_tree = { + .oid = { + .hash = EMPTY_TREE_SHA1_BIN_LITERAL, + }, + .type = OBJ_TREE, +- .buf = "", ++ .buf = empty_tree_buf, + }; + + static struct cached_object *find_cached_object(const struct object_id *oid) + + ## pretty.c ## +@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ + return 1; + case 'D': + { ++ char empty_str[] = ""; + const struct decoration_options opts = { +- .prefix = "", +- .suffix = "" ++ .prefix = empty_str, ++ .suffix = empty_str, + }; + + format_decorations(sb, commit, c->auto_color, &opts); + + ## refs/reftable-backend.c ## +@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) + struct strbuf errbuf = STRBUF_INIT; + size_t logs_nr = 0, logs_alloc = 0, i; + const char *committer_info; ++ char head[] = "HEAD"; + int ret; + + committer_info = git_committer_info(0); +@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) + if (append_head_reflog) { + ALLOC_GROW(logs, logs_nr + 1, logs_alloc); + logs[logs_nr] = logs[logs_nr - 1]; +- logs[logs_nr].refname = "HEAD"; ++ logs[logs_nr].refname = head; + logs_nr++; + } + } +@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) + string_list_clear(&skip, 0); + strbuf_release(&errbuf); + for (i = 0; i < logs_nr; i++) { +- if (!strcmp(logs[i].refname, "HEAD")) ++ if (logs[i].refname == head) + continue; + logs[i].refname = NULL; + reftable_log_record_release(&logs[i]); + ## reftable/basics_test.c ## @@ reftable/basics_test.c: static void test_binsearch(void) @@ reftable/basics_test.c: static void test_binsearch(void) { - char *a[] = { "a", "b", NULL }; - EXPECT(names_length(a) == 2); -+ char *names[] = { (char *)"a", (char *)"b", NULL }; ++ char a[] = "a", b[] = "b"; ++ char *names[] = { a, b, NULL }; + EXPECT(names_length(names) == 2); } static void test_parse_names_normal(void) ## reftable/block_test.c ## +@@ reftable/block_test.c: license that can be found in the LICENSE file or at + static void test_block_read_write(void) + { + const int header_off = 21; /* random */ +- char *names[30]; ++ char *names[30], empty_str[] = ""; + const int N = ARRAY_SIZE(names); + const int block_size = 1024; + struct reftable_block block = { NULL }; @@ reftable/block_test.c: static void test_block_read_write(void) block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size, header_off, hash_size(GIT_SHA1_FORMAT_ID)); - rec.u.ref.refname = ""; -+ rec.u.ref.refname = (char *)""; ++ rec.u.ref.refname = empty_str; rec.u.ref.value_type = REFTABLE_REF_DELETION; n = block_writer_add(&bw, &rec); EXPECT(n == REFTABLE_API_ERROR); ## reftable/merged_test.c ## @@ reftable/merged_test.c: static void readers_destroy(struct reftable_reader **readers, size_t n) + static void test_merged_between(void) { ++ char a[] = "a", b[] = "b"; struct reftable_ref_record r1[] = { { - .refname = "b", -+ .refname = (char *)"b", ++ .refname = b, .update_index = 1, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 1, 2, 3, 0 }, } }; struct reftable_ref_record r2[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 2, .value_type = REFTABLE_REF_DELETION, } }; -@@ reftable/merged_test.c: static void test_merged(void) +@@ reftable/merged_test.c: static void test_merged_between(void) + + static void test_merged(void) { ++ char a[] = "a", b[] = "b", c[] = "c", d[] = "d"; struct reftable_ref_record r1[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 1, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 1 }, }, { - .refname = "b", -+ .refname = (char *)"b", ++ .refname = b, .update_index = 1, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 1 }, }, { - .refname = "c", -+ .refname = (char *)"c", ++ .refname = c, .update_index = 1, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 1 }, @@ reftable/merged_test.c: static void test_merged(void) }; struct reftable_ref_record r2[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 2, .value_type = REFTABLE_REF_DELETION, } }; struct reftable_ref_record r3[] = { { - .refname = "c", -+ .refname = (char *)"c", ++ .refname = c, .update_index = 3, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 2 }, }, { - .refname = "d", -+ .refname = (char *)"d", ++ .refname = d, .update_index = 3, .value_type = REFTABLE_REF_VAL1, .value.val1 = { 1 }, -@@ reftable/merged_test.c: static void test_merged_logs(void) +@@ reftable/merged_test.c: merged_table_from_log_records(struct reftable_log_record **logs, + + static void test_merged_logs(void) { ++ char a[] = "a"; ++ char name[] = "jane doe", email[] = "jane@invalid"; ++ char message1[] = "message1", message2[] = "message2"; ++ char message3[] = "message3"; struct reftable_log_record r1[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 2, .value_type = REFTABLE_LOG_UPDATE, .value.update = { @@ reftable/merged_test.c: static void test_merged_logs(void) - .name = "jane doe", - .email = "jane@invalid", - .message = "message2", -+ .name = (char *)"jane doe", -+ .email = (char *)"jane@invalid", -+ .message = (char *)"message2", ++ .name = name, ++ .email = email, ++ .message = message2, } }, { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 1, .value_type = REFTABLE_LOG_UPDATE, .value.update = { @@ reftable/merged_test.c: static void test_merged_logs(void) - .name = "jane doe", - .email = "jane@invalid", - .message = "message1", -+ .name = (char *)"jane doe", -+ .email = (char *)"jane@invalid", -+ .message = (char *)"message1", ++ .name = name, ++ .email = email, ++ .message = message1, } }, }; struct reftable_log_record r2[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 3, .value_type = REFTABLE_LOG_UPDATE, .value.update = { @@ reftable/merged_test.c: static void test_merged_logs(void) - .name = "jane doe", - .email = "jane@invalid", - .message = "message3", -+ .name = (char *)"jane doe", -+ .email = (char *)"jane@invalid", -+ .message = (char *)"message3", ++ .name = name, ++ .email = email, ++ .message = message3, } }, }; struct reftable_log_record r3[] = { { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 2, .value_type = REFTABLE_LOG_DELETION, }, -@@ reftable/merged_test.c: static void test_default_write_opts(void) +@@ reftable/merged_test.c: static void test_merged_logs(void) + + static void test_default_write_opts(void) + { ++ char master[] = "master"; + struct reftable_write_options opts = { 0 }; struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); - struct reftable_ref_record rec = { - .refname = "master", -+ .refname = (char *)"master", ++ .refname = master, .update_index = 1, }; int err; ## reftable/readwrite_test.c ## +@@ reftable/readwrite_test.c: static void write_table(char ***names, struct strbuf *buf, int N, + int i = 0, n; + struct reftable_log_record log = { NULL }; + const struct reftable_stats *stats = NULL; ++ char message[] = "message"; + + REFTABLE_CALLOC_ARRAY(*names, N + 1); + @@ reftable/readwrite_test.c: static void write_table(char ***names, struct strbuf *buf, int N, log.update_index = update_index; log.value_type = REFTABLE_LOG_UPDATE; set_test_hash(log.value.update.new_hash, i); - log.value.update.message = "message"; -+ log.value.update.message = (char *)"message"; ++ log.value.update.message = message; n = reftable_writer_add_log(w, &log); EXPECT(n == 0); -@@ reftable/readwrite_test.c: static void test_log_buffer_size(void) +@@ reftable/readwrite_test.c: static void write_table(char ***names, struct strbuf *buf, int N, + + static void test_log_buffer_size(void) + { ++ char refname[] = "refs/heads/master"; ++ char name[] = "Han-Wen Hienhuys"; ++ char email[] = "hanwen@google.com"; ++ char message[] = "commit: 9\n"; + struct strbuf buf = STRBUF_INIT; + struct reftable_write_options opts = { + .block_size = 4096, }; int err; int i; @@ reftable/readwrite_test.c: static void test_log_buffer_size(void) - .message = "commit: 9\n", - } } }; + struct reftable_log_record log = { -+ .refname = (char *)"refs/heads/master", ++ .refname = refname, + .update_index = 0xa, + .value_type = REFTABLE_LOG_UPDATE, + .value.update = { -+ .name = (char *)"Han-Wen Nienhuys", -+ .email = (char *)"hanwen@google.com", ++ .name = name, ++ .email = email, + .tz_offset = 100, + .time = 0x5e430672, -+ .message = (char *)"commit: 9\n", ++ .message = message, + }, + }; struct reftable_writer *w = reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); +@@ reftable/readwrite_test.c: static void test_log_buffer_size(void) + + static void test_log_overflow(void) + { ++ char refname[] = "refs/heads/master"; ++ char name[] = "Han-Wen Hienhuys"; ++ char email[] = "hanwen@google.com"; + struct strbuf buf = STRBUF_INIT; + char msg[256] = { 0 }; + struct reftable_write_options opts = { @@ reftable/readwrite_test.c: static void test_log_overflow(void) }; int err; struct reftable_log_record log = { - .refname = "refs/heads/master", -+ .refname = (char *)"refs/heads/master", ++ .refname = refname, .update_index = 0xa, .value_type = REFTABLE_LOG_UPDATE, .value = { @@ reftable/readwrite_test.c: static void test_log_overflow(void) .new_hash = { 2 }, - .name = "Han-Wen Nienhuys", - .email = "hanwen@google.com", -+ .name = (char *)"Han-Wen Nienhuys", -+ .email = (char *)"hanwen@google.com", ++ .name = name, ++ .email = email, .tz_offset = 100, .time = 0x5e430672, .message = msg, @@ reftable/readwrite_test.c: static void test_log_zlib_corruption(void) + struct reftable_writer *w = + reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); + const struct reftable_stats *stats = NULL; ++ char refname[] = "refname"; ++ char name[] = "My Name"; ++ char email[] = "myname@invalid"; char message[100] = { 0 }; int err, i, n; struct reftable_log_record log = { - .refname = "refname", -+ .refname = (char *)"refname", ++ .refname = refname, .value_type = REFTABLE_LOG_UPDATE, .value = { .update = { @@ reftable/readwrite_test.c: static void test_log_zlib_corruption(void) .old_hash = { 2 }, - .name = "My Name", - .email = "myname@invalid", -+ .name = (char *)"My Name", -+ .email = (char *)"myname@invalid", ++ .name = name, ++ .email = email, .message = message, }, }, @@ reftable/readwrite_test.c: static void test_write_empty_key(void) + struct strbuf buf = STRBUF_INIT; struct reftable_writer *w = reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); ++ char refname[] = ""; struct reftable_ref_record ref = { - .refname = "", -+ .refname = (char *)"", ++ .refname = refname, .update_index = 1, .value_type = REFTABLE_REF_DELETION, }; @@ reftable/readwrite_test.c: static void test_write_key_order(void) + struct strbuf buf = STRBUF_INIT; + struct reftable_writer *w = reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts); ++ char a[] = "a", b[] = "b", target[] = "target"; struct reftable_ref_record refs[2] = { { - .refname = "b", -+ .refname = (char *)"b", ++ .refname = b, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, .value = { - .symref = "target", -+ .symref = (char *)"target", ++ .symref = target, }, }, { - .refname = "a", -+ .refname = (char *)"a", ++ .refname = a, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, .value = { - .symref = "target", -+ .symref = (char *)"target", ++ .symref = target, }, } }; @@ reftable/stack_test.c: static void test_parse_names(void) static int write_test_ref(struct reftable_writer *wr, void *arg) @@ reftable/stack_test.c: static void test_reftable_stack_add_one(void) + }; struct reftable_stack *st = NULL; int err; ++ char head[] = "HEAD", master[] = "master"; struct reftable_ref_record ref = { - .refname = "HEAD", -+ .refname = (char *)"HEAD", ++ .refname = head, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; struct reftable_ref_record dest = { NULL }; struct stat stat_result = { 0 }; @@ reftable/stack_test.c: static void test_reftable_stack_uptodate(void) + char *dir = get_tmp_dir(__LINE__); int err; ++ char head[] = "HEAD", branch2[] = "branch2", master[] = "master"; struct reftable_ref_record ref1 = { - .refname = "HEAD", -+ .refname = (char *)"HEAD", ++ .refname = head, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; struct reftable_ref_record ref2 = { - .refname = "branch2", -+ .refname = (char *)"branch2", ++ .refname = branch2, .update_index = 2, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; @@ reftable/stack_test.c: static void test_reftable_stack_transaction_api(void) + struct reftable_stack *st = NULL; + int err; struct reftable_addition *add = NULL; - +- ++ char head[] = "HEAD", master[] = "master"; struct reftable_ref_record ref = { - .refname = "HEAD", -+ .refname = (char *)"HEAD", ++ .refname = head, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; struct reftable_ref_record dest = { NULL }; @@ reftable/stack_test.c: static void test_reftable_stack_transaction_api_performs_auto_compaction(void) + EXPECT_ERR(err); + + for (i = 0; i <= n; i++) { ++ char master[] = "master"; struct reftable_ref_record ref = { .update_index = reftable_stack_next_update_index(st), .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; char name[100]; @@ reftable/stack_test.c: static void test_reftable_stack_transaction_api_performs_auto_compaction(void) + static void test_reftable_stack_auto_compaction_fails_gracefully(void) { ++ char master[] = "refs/meads/master"; struct reftable_ref_record ref = { - .refname = "refs/heads/master", -+ .refname = (char *)"refs/heads/master", ++ .refname = master, .update_index = 1, .value_type = REFTABLE_REF_VAL1, .value.val1 = {0x01}, -@@ reftable/stack_test.c: static void test_reftable_stack_update_index_check(void) +@@ reftable/stack_test.c: static int write_error(struct reftable_writer *wr, void *arg) + static void test_reftable_stack_update_index_check(void) + { + char *dir = get_tmp_dir(__LINE__); +- + struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; ++ char name1[] = "name1", name2[] = "name2", master[] = "master"; struct reftable_ref_record ref1 = { - .refname = "name1", -+ .refname = (char *)"name1", ++ .refname = name1, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; struct reftable_ref_record ref2 = { - .refname = "name2", -+ .refname = (char *)"name2", ++ .refname = name2, .update_index = 1, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; err = reftable_new_stack(&st, dir, cfg); @@ reftable/stack_test.c: static void test_reftable_stack_log_normalize(void) + }; struct reftable_stack *st = NULL; char *dir = get_tmp_dir(__LINE__); ++ char branch[] = "branch"; ++ char onetwomessage[] = "one\ntwo"; ++ char onemessage[] = "one"; ++ char twomessage[] = "two\n"; struct reftable_log_record input = { - .refname = "branch", -+ .refname = (char *)"branch", ++ .refname = branch, .update_index = 1, .value_type = REFTABLE_LOG_UPDATE, .value = { @@ reftable/stack_test.c: static void test_reftable_stack_log_normalize(void) EXPECT_ERR(err); - input.value.update.message = "one\ntwo"; -+ input.value.update.message = (char *)"one\ntwo"; ++ input.value.update.message = onetwomessage; err = reftable_stack_add(st, &write_test_log, &arg); EXPECT(err == REFTABLE_API_ERROR); - input.value.update.message = "one"; -+ input.value.update.message = (char *)"one"; ++ input.value.update.message = onemessage; err = reftable_stack_add(st, &write_test_log, &arg); EXPECT_ERR(err); @@ reftable/stack_test.c: static void test_reftable_stack_log_normalize(void) EXPECT(0 == strcmp(dest.value.update.message, "one\n")); - input.value.update.message = "two\n"; -+ input.value.update.message = (char *)"two\n"; ++ input.value.update.message = twomessage; arg.update_index = 2; err = reftable_stack_add(st, &write_test_log, &arg); EXPECT_ERR(err); -@@ reftable/stack_test.c: static void test_reftable_stack_hash_id(void) +@@ reftable/stack_test.c: static void test_reftable_stack_tombstone(void) + static void test_reftable_stack_hash_id(void) + { + char *dir = get_tmp_dir(__LINE__); +- + struct reftable_write_options cfg = { 0 }; + struct reftable_stack *st = NULL; int err; - +- ++ char master[] = "master", target[] = "target"; struct reftable_ref_record ref = { - .refname = "master", -+ .refname = (char *)"master", ++ .refname = master, .value_type = REFTABLE_REF_SYMREF, - .value.symref = "target", -+ .value.symref = (char *)"target", ++ .value.symref = target, .update_index = 1, }; struct reftable_write_options cfg32 = { .hash_id = GIT_SHA256_FORMAT_ID }; @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void) + EXPECT_ERR(err); + + for (i = 0; i < N; i++) { +- char name[100]; ++ char name[100], master[] = "master"; + struct reftable_ref_record ref = { .refname = name, .update_index = reftable_stack_next_update_index(st), .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; snprintf(name, sizeof(name), "branch%04d", i); @@ reftable/stack_test.c: static void test_reftable_stack_add_performs_auto_compaction(void) + EXPECT_ERR(err); + + for (i = 0; i <= n; i++) { ++ char master[] = "master"; struct reftable_ref_record ref = { .update_index = reftable_stack_next_update_index(st), .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; /* @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent(void) + EXPECT_ERR(err); + + for (i = 0; i < N; i++) { +- char name[100]; ++ char name[100], master[] = "master"; + struct reftable_ref_record ref = { .refname = name, .update_index = reftable_stack_next_update_index(st1), .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; snprintf(name, sizeof(name), "branch%04d", i); @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_clean(void) + EXPECT_ERR(err); + + for (i = 0; i < N; i++) { +- char name[100]; ++ char name[100], master[] = "master"; + struct reftable_ref_record ref = { .refname = name, .update_index = reftable_stack_next_update_index(st1), .value_type = REFTABLE_REF_SYMREF, - .value.symref = "master", -+ .value.symref = (char *)"master", ++ .value.symref = master, }; snprintf(name, sizeof(name), "branch%04d", i); 3: 8f3decbb76 ! 3: 8b71dfa208 global: convert intentionally-leaking config strings to consts @@ Commit message configured via `diff.<driver>.*` to add additional drivers. Again, these have a global lifetime and are never free'd. - All of these are intentionally kept alive and never free'd. Let's mark - the respective fields as `const char *` and cast away the constness when - assigning those values. + All of these are intentionally kept alive and never free'd. Furthermore, + all of these are being assigned both string constants in some places, + and allocated strings in other places. This will cause warnings once we + enable `-Wwrite-strings`, so let's mark the respective fields as `const + char *` and cast away the constness when assigning those values. Signed-off-by: Patrick Steinhardt <ps@pks.im> 4: ba50e49f86 = 4: 961b3357d5 compat/win32: fix const-correctness with string constants 6: 0eaa73c109 = 5: b73a45133b refspec: remove global tag refspec structure 7: 03b13c449b = 6: 6da87a0905 http: do not assign string constant to non-const field 8: 699eeae92c = 7: 3da7df97a5 line-log: always allocate the output prefix 9: 6cbb8444a6 = 8: e5d14a5173 object-file: make `buf` parameter of `index_mem()` a constant 10: c07b27bbb4 = 9: dd40c7464d parse-options: cast long name for OPTION_ALIAS 11: 3cd28ae38c = 10: 462502127d send-pack: always allocate receive status 12: 00b4a7dbbc ! 11: 884fbe1da5 remote-curl: avoid assigning string constant to non-const variable @@ remote-curl.c: int cmd_main(int argc, const char **argv) } else if (skip_prefix(buf.buf, "option ", &arg)) { - char *value = strchr(arg, ' '); -+ const char *value = strchr(arg, ' '); -+ size_t arglen; ++ const char *value = strchrnul(arg, ' '); ++ size_t arglen = value - arg; int result; - if (value) - *value++ = '\0'; -- else -+ if (value) { -+ arglen = value - arg; -+ value++; -+ } else { -+ arglen = strlen(arg); ++ if (*value) ++ value++; /* skip over SP */ + else value = "true"; -+ } - result = set_option(arg, value); + result = set_option(arg, arglen, value); 13: 68a7d24e4a = 12: 502380c2ca revision: always store allocated strings in output encoding 14: 0e393fa6a7 = 13: ffacdc3779 mailmap: always store allocated strings in mailmap blob 15: 18ba9f7b3b = 14: c0fce9b87e imap-send: drop global `imap_server_conf` variable 16: 357d69fa8b ! 15: e0a5b83f0e imap-send: fix leaking memory in `imap_server_conf` @@ Commit message `struct imap_server_conf`. Fix this by creating a common exit path where we can free resources. - While at it, drop the unused variables `imap_server_conf::name` and - `nongit_ok`. + While at it, drop the unused member `imap_server_conf::name`. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ imap-send.c: static int git_imap_config(const char *var, const char *val, return 0; } @@ imap-send.c: int cmd_main(int argc, const char **argv) - }; struct strbuf all_msgs = STRBUF_INIT; int total; -- int nongit_ok; + int nongit_ok; + int ret; -- setup_git_directory_gently(&nongit_ok); -+ setup_git_directory_gently(NULL); + setup_git_directory_gently(&nongit_ok); git_config(git_imap_config, &server); - - argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0); @@ imap-send.c: int cmd_main(int argc, const char **argv) if (!server.folder) { 2: 51ee5660a1 ! 16: 36a7b0a4b0 global: assign non-const strings as required @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - global: assign non-const strings as required + builtin/rebase: do not assign default backend to non-constant field - There are several cases where we initialize non-const fields with string - constants. This is invalid and will cause warnings once we enable the - `-Wwrite-strings` warning. Adapt those cases to instead use string - arrays. + The `struct rebase_options::default_backend` field is a non-constant + string, but is being assigned a constant via `REBASE_OPTIONS_INIT`. + Refactor the code to initialize and release options via two functions + `rebase_options_init()` and `rebase_options_release()`. Like this, we + can easily adapt the former funnction to use `xstrdup()` on the default + value without hiding it away in a macro. Signed-off-by: Patrick Steinhardt <ps@pks.im> - ## builtin/remote.c ## -@@ builtin/remote.c: static int get_head_names(const struct ref *remote_refs, struct ref_states *stat - struct ref *ref, *matches; - struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map; - struct refspec_item refspec; -+ char refspec_str[] = "refs/heads/*"; - - memset(&refspec, 0, sizeof(refspec)); - refspec.force = 0; - refspec.pattern = 1; -- refspec.src = refspec.dst = "refs/heads/*"; -+ refspec.src = refspec.dst = refspec_str; - get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0); - matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"), - fetch_map, 1); - - ## diff.c ## -@@ diff.c: size_t fill_textconv(struct repository *r, - struct diff_filespec *df, - char **outbuf) - { -+ static char empty_str[] = ""; - size_t size; - - if (!driver) { - if (!DIFF_FILE_VALID(df)) { -- *outbuf = ""; -+ *outbuf = empty_str; - return 0; - } - if (diff_populate_filespec(r, df, NULL)) - - ## entry.c ## -@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) - struct string_list_item *filter, *path; - struct progress *progress = NULL; - struct delayed_checkout *dco = state->delayed_checkout; -+ char empty_str[] = ""; - - if (!state->delayed_checkout) - return errs; -@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) - if (!async_query_available_blobs(filter->string, &available_paths)) { - /* Filter reported an error */ - errs = 1; -- filter->string = ""; -+ filter->string = empty_str; - continue; - } - if (available_paths.nr <= 0) { -@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) - * filter from the list (see - * "string_list_remove_empty_items" call below). - */ -- filter->string = ""; -+ filter->string = empty_str; - continue; - } + ## builtin/rebase.c ## +@@ builtin/rebase.c: struct rebase_options { + int config_update_refs; + }; -@@ entry.c: int finish_delayed_checkout(struct checkout *state, int show_progress) - * Do not ask the filter for available blobs, - * again, as the filter is likely buggy. - */ -- filter->string = ""; -+ filter->string = empty_str; - continue; - } - ce = index_file_exists(state->istate, path->string, - - ## ident.c ## -@@ ident.c: static struct passwd *xgetpwuid_self(int *is_bogus) - pw = getpwuid(getuid()); - if (!pw) { - static struct passwd fallback; -- fallback.pw_name = "unknown"; -+ static char fallback_name[] = "unknown"; - #ifndef NO_GECOS_IN_PWENT -- fallback.pw_gecos = "Unknown"; -+ static char fallback_gcos[] = "Unknown"; -+#endif +-#define REBASE_OPTIONS_INIT { \ +- .type = REBASE_UNSPECIFIED, \ +- .empty = EMPTY_UNSPECIFIED, \ +- .keep_empty = 1, \ +- .default_backend = "merge", \ +- .flags = REBASE_NO_QUIET, \ +- .git_am_opts = STRVEC_INIT, \ +- .exec = STRING_LIST_INIT_NODUP, \ +- .git_format_patch_opt = STRBUF_INIT, \ +- .fork_point = -1, \ +- .reapply_cherry_picks = -1, \ +- .allow_empty_message = 1, \ +- .autosquash = -1, \ +- .rebase_merges = -1, \ +- .config_rebase_merges = -1, \ +- .update_refs = -1, \ +- .config_update_refs = -1, \ +- .strategy_opts = STRING_LIST_INIT_NODUP,\ +- } ++static void rebase_options_init(struct rebase_options *opts) ++{ ++ memset(opts, 0, sizeof(*opts)); ++ opts->type = REBASE_UNSPECIFIED; ++ opts->empty = EMPTY_UNSPECIFIED; ++ opts->default_backend = xstrdup("merge"); ++ opts->keep_empty = 1; ++ opts->flags = REBASE_NO_QUIET; ++ strvec_init(&opts->git_am_opts); ++ string_list_init_nodup(&opts->exec); ++ strbuf_init(&opts->git_format_patch_opt, 0); ++ opts->fork_point = -1; ++ opts->reapply_cherry_picks = -1; ++ opts->allow_empty_message = 1; ++ opts->autosquash = -1; ++ opts->rebase_merges = -1; ++ opts->config_rebase_merges = -1; ++ opts->update_refs = -1; ++ opts->config_update_refs = -1; ++ string_list_init_nodup(&opts->strategy_opts); ++} + -+ fallback.pw_name = fallback_name; -+#ifndef NO_GECOS_IN_PWENT -+ fallback.pw_gecos = fallback_gcos; - #endif - pw = &fallback; - if (is_bogus) - - ## line-log.c ## -@@ line-log.c: static int process_diff_filepair(struct rev_info *rev, - struct range_set tmp; - struct diff_ranges diff; - mmfile_t file_parent, file_target; -+ char empty_str[] = ""; ++static void rebase_options_release(struct rebase_options *opts) ++{ ++ free(opts->default_backend); ++ free(opts->reflog_action); ++ free(opts->head_name); ++ strvec_clear(&opts->git_am_opts); ++ free(opts->gpg_sign_opt); ++ string_list_clear(&opts->exec, 0); ++ free(opts->strategy); ++ string_list_clear(&opts->strategy_opts, 0); ++ strbuf_release(&opts->git_format_patch_opt); ++} - assert(pair->two->path); - while (rg) { -@@ line-log.c: static int process_diff_filepair(struct rev_info *rev, - file_parent.ptr = pair->one->data; - file_parent.size = pair->one->size; - } else { -- file_parent.ptr = ""; -+ file_parent.ptr = empty_str; - file_parent.size = 0; + static struct replay_opts get_replay_opts(const struct rebase_options *opts) + { +@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, } - - ## object-file.c ## -@@ object-file.c: static struct cached_object { - } *cached_objects; - static int cached_object_nr, cached_object_alloc; - -+static char empty_tree_buf[] = ""; - static struct cached_object empty_tree = { - .oid = { - .hash = EMPTY_TREE_SHA1_BIN_LITERAL, - }, - .type = OBJ_TREE, -- .buf = "", -+ .buf = empty_tree_buf, - }; + if (!strcmp(var, "rebase.backend")) { ++ FREE_AND_NULL(opts->default_backend); + return git_config_string(&opts->default_backend, var, value); + } - static struct cached_object *find_cached_object(const struct object_id *oid) - - ## pretty.c ## -@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ - return 1; - case 'D': - { -+ char empty_str[] = ""; - const struct decoration_options opts = { -- .prefix = "", -- .suffix = "" -+ .prefix = empty_str, -+ .suffix = empty_str, - }; +@@ builtin/rebase.c: static int check_exec_cmd(const char *cmd) - format_decorations(sb, commit, c->auto_color, &opts); - - ## refs/reftable-backend.c ## -@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) - struct strbuf errbuf = STRBUF_INIT; - size_t logs_nr = 0, logs_alloc = 0, i; - const char *committer_info; -+ char head[] = "HEAD"; - int ret; + int cmd_rebase(int argc, const char **argv, const char *prefix) + { +- struct rebase_options options = REBASE_OPTIONS_INIT; ++ struct rebase_options options; + const char *branch_name; + int ret, flags, total_argc, in_progress = 0; + int keep_base = 0; +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + }; + int i; - committer_info = git_committer_info(0); -@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) - if (append_head_reflog) { - ALLOC_GROW(logs, logs_nr + 1, logs_alloc); - logs[logs_nr] = logs[logs_nr - 1]; -- logs[logs_nr].refname = "HEAD"; -+ logs[logs_nr].refname = head; - logs_nr++; - } - } -@@ refs/reftable-backend.c: static int write_copy_table(struct reftable_writer *writer, void *cb_data) - string_list_clear(&skip, 0); - strbuf_release(&errbuf); - for (i = 0; i < logs_nr; i++) { -- if (!strcmp(logs[i].refname, "HEAD")) -+ if (logs[i].refname == head) - continue; - logs[i].refname = NULL; - reftable_log_record_release(&logs[i]); ++ rebase_options_init(&options); ++ + if (argc == 2 && !strcmp(argv[1], "-h")) + usage_with_options(builtin_rebase_usage, + builtin_rebase_options); +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + cleanup: + strbuf_release(&buf); + strbuf_release(&revisions); +- free(options.reflog_action); +- free(options.head_name); +- strvec_clear(&options.git_am_opts); +- free(options.gpg_sign_opt); +- string_list_clear(&options.exec, 0); +- free(options.strategy); +- string_list_clear(&options.strategy_opts, 0); +- strbuf_release(&options.git_format_patch_opt); ++ rebase_options_release(&options); + free(squash_onto_name); + free(keep_base_onto_name); + return !!ret; 17: 16d3d28243 ! 17: 3552ab9748 builtin/rebase: adapt code to not assign string constants to non-const @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - builtin/rebase: adapt code to not assign string constants to non-const + builtin/rebase: always store allocated string in `options.strategy` - When computing the rebase strategy we temporarily assign a string - constant to `options.strategy` before we call `xstrdup()` on it. - Furthermore, the default backend is being assigned a string constant via - `REBASE_OPTIONS_INIT`. Both of these will cause warnings once we enable - `-Wwrite-strings`. + The `struct rebase_options::strategy` field is a `char *`, but we do end + up assigning string constants to it in two cases: - Adapt the code such that we only store allocated strings in those - variables. + - When being passed a `--strategy=` option via the command line. + + - When being passed a strategy option via `--strategy-option=`, but + not a strategy. + + This will cause warnings once we enable `-Wwrite-strings`. + + Ideally, we'd just convert the field to be a `const char *`. But we also + assign to this field via the GIT_TEST_MERGE_ALGORITHM envvar, which we + have to strdup(3P) into it. + + Instead, refactor the code to make sure that we only ever assign + allocated strings to this field. Signed-off-by: Patrick Steinhardt <ps@pks.im> ## builtin/rebase.c ## -@@ builtin/rebase.c: struct rebase_options { - .type = REBASE_UNSPECIFIED, \ - .empty = EMPTY_UNSPECIFIED, \ - .keep_empty = 1, \ -- .default_backend = "merge", \ - .flags = REBASE_NO_QUIET, \ - .git_am_opts = STRVEC_INIT, \ - .exec = STRING_LIST_INIT_NODUP, \ -@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, - } - - if (!strcmp(var, "rebase.backend")) { -+ FREE_AND_NULL(opts->default_backend); - return git_config_string(&opts->default_backend, var, value); - } - @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + { + struct rebase_options options; + const char *branch_name; ++ const char *strategy_opt = NULL; + int ret, flags, total_argc, in_progress = 0; + int keep_base = 0; + int ok_to_skip_pre_rebase = 0; +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + PARSE_OPT_OPTARG, parse_opt_rebase_merges), + OPT_BOOL(0, "fork-point", &options.fork_point, + N_("use 'merge-base --fork-point' to refine upstream")), +- OPT_STRING('s', "strategy", &options.strategy, ++ OPT_STRING('s', "strategy", &strategy_opt, + N_("strategy"), N_("use the given merge strategy")), + OPT_STRING_LIST('X', "strategy-option", &options.strategy_opts, + N_("option"), +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) + } } - if (options.strategy_opts.nr && !options.strategy) +- if (options.strategy_opts.nr && !options.strategy) - options.strategy = "ort"; - - if (options.strategy) { - options.strategy = xstrdup(options.strategy); ++ if (strategy_opt) ++ options.strategy = xstrdup(strategy_opt); ++ else if (options.strategy_opts.nr && !options.strategy) + options.strategy = xstrdup("ort"); -+ else -+ options.strategy = xstrdup_or_null(options.strategy); + if (options.strategy) imply_merge(&options, "--strategy"); - } if (options.root && !options.onto_name) imply_merge(&options, "--root without --onto"); -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) - } - - if (options.type == REBASE_UNSPECIFIED) { -- if (!strcmp(options.default_backend, "merge")) -+ if (!options.default_backend) -+ options.type = REBASE_MERGE; -+ else if (!strcmp(options.default_backend, "merge")) - options.type = REBASE_MERGE; - else if (!strcmp(options.default_backend, "apply")) - options.type = REBASE_APPLY; -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) - cleanup: - strbuf_release(&buf); - strbuf_release(&revisions); -+ free(options.default_backend); - free(options.reflog_action); - free(options.head_name); - strvec_clear(&options.git_am_opts); 18: 129482dbaa = 18: bf854b3979 builtin/merge: always store allocated strings in `pull_twohead` 19: 37e7aaed97 = 19: 9b9d57ae84 config.mak.dev: enable `-Wwrite-strings` warning