summaryrefslogtreecommitdiff
path: root/builtin/submodule--helper.c
AgeCommit message (Collapse)Author
6 daysMerge branch 'ab/fewer-the-index-macros'Junio C Hamano
Progress on removing 'the_index' convenience wrappers. * ab/fewer-the-index-macros: cocci: apply "pending" index-compatibility to some "builtin/*.c" cache.h & test-tool.h: add & use "USE_THE_INDEX_VARIABLE" {builtin/*,repository}.c: add & use "USE_THE_INDEX_VARIABLE" cocci: apply "pending" index-compatibility to "t/helper/*.c" cocci & cache.h: apply variable section of "pending" index-compatibility cocci & cache.h: apply a selection of "pending" index-compatibility cocci: add a index-compatibility.pending.cocci read-cache API & users: make discard_index() return void cocci & cache.h: remove rarely used "the_index" compat macros builtin/{grep,log}.: don't define "USE_THE_INDEX_COMPATIBILITY_MACROS" cache.h: remove unused "the_index" compat macros
13 dayscocci: apply "pending" index-compatibility to some "builtin/*.c"Ævar Arnfjörð Bjarmason
Apply "index-compatibility.pending.cocci" rule to "builtin/*", but exclude those where we conflict with in-flight changes. As a result some of them end up using only "the_index", so let's have them use the more narrow "USE_THE_INDEX_VARIABLE" rather than "USE_THE_INDEX_COMPATIBILITY_MACROS". Manual changes not made by coccinelle, that were squashed in: * Whitespace-wrap argument lists for repo_hold_locked_index(), repo_read_index_preload() and repo_refresh_and_write_index(), in cases where the line became too long after the transformation. * Change "refresh_cache()" to "refresh_index()" in a comment in "builtin/update-index.c". * For those whose call was followed by perror("<macro-name>"), change it to perror("<function-name>"), referring to the new function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
13 dayscocci & cache.h: apply variable section of "pending" index-compatibilityÆvar Arnfjörð Bjarmason
Mostly apply the part of "index-compatibility.pending.cocci" that renames the global variables like "active_nr", which are a shorthand to referencing (in that case) a struct member as "the_index.cache_nr". In doing so move more of "index-compatibility.pending.cocci" to "index-compatibility.cocci". In the case of "active_nr" we'd have a textual conflict with "ab/various-leak-fixes" in "next"[1]. Let's exclude that specific case while moving the rule over from "pending". 1. 407b94280f8 (commit: discard partial cache before (re-)reading it, 2022-11-08) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-11-08submodule--helper: use OPT_SUBCOMMAND() APIÆvar Arnfjörð Bjarmason
Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API introduced in fa83cc834da (parse-options: add support for parsing subcommands, 2022-08-19). This is only a marginal reduction in line count, but once we start unifying this with a yet-to-be-added "builtin/submodule.c" it'll be much easier to reason about those changes, as they'll both use OPT_SUBCOMMAND(). We don't need to worry about "argv[0]" being NULL in the die() because we'd have errored out in parse_options() as we're not using "PARSE_OPT_SUBCOMMAND_OPTIONAL". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-11-08submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"Ævar Arnfjörð Bjarmason
Since 29a5e9e1ffe (submodule--helper update-clone: learn --init, 2022-03-04) we've been passing "-C <prefix>" from "git-submodule.sh" whenever we pass "--prefix <prefix>", so the latter is redundant to the former. Let's drop the "--prefix" option. Suggested-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-11-08submodule--helper: remove --prefix from "absorbgitdirs"Ævar Arnfjörð Bjarmason
Let's pass the "-C <prefix>" option instead to "absorbgitdirs" from its only caller. When it was added in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12) there were other "submodule--helper" subcommands that were invoked with "-C <prefix>", so we could have done this all along. Suggested-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-11-08submodule API & "absorbgitdirs": remove "----recursive" optionÆvar Arnfjörð Bjarmason
Remove the "----recursive" option to "git submodule--helper absorbgitdirs" (yes, with 4 dashes, not 2). This option and all the "else" when "flags & ABSORB_GITDIR_RECURSE_SUBMODULES" is false has never been used since it was added in f6f85861400 (submodule: add absorb-git-dir function, 2016-12-12), which we'd have had to do as "----recursive", a "--recursive" would have errored out. It would be nice to follow-up with an optbug() assertion to parse-options.c for such funnily named options, I manually validated that this was the only long option whose name started with "-", but let's skip adding such an assertion for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-11-08submodule--helper: fix a memory leak in "status"Ævar Arnfjörð Bjarmason
The "status" sub-command was leaking the "struct strvec" it was setting up for the reasons explained in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that need free()-ing, 2022-08-02), so let's use the "free_removed_argv_elements" option to setup_revisions() to fix the leak. Even if we did that, clobbering the "diff_files_args.nr" with the return value of setup_revisions() would leave leaks in place, but we can just stop clobbering it. Ever since that code was added in a9f8a37584a (submodule: port submodule subcommand 'status' from shell to C, 2017-10-06) we've had no reason to modify the "nr" member ("argc" at the time): The next use of "diff_files_args" after this is the "strvec_clear()" at the end of the function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-11-08submodule--helper: move "config" to a test-toolÆvar Arnfjörð Bjarmason
As with other moves to "test-tool" in f322e9f51b5 (Merge branch 'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was only used by our own tests. It was last used by "git submodule" itself in code that went away with a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10). Let's move it over, and while doing so make it easier to reason about by splitting up the various uses for it into separate sub-commands, so that we don't need to count arguments to see what it does. This also has the advantage that we stop wasting future translator time on this command, currently the usage information for this internal-only tool has been translated into several languages. The use of the "_" function has also been removed from the "please make sure..." message. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2022-10-27Merge branch 'ab/run-hook-api-cleanup'Junio C Hamano
Move a global variable added as a hack during regression fixes to its proper place in the API. * ab/run-hook-api-cleanup: run-command.c: remove "max_processes", add "const" to signal() handler run-command.c: pass "opts" further down, and use "opts->processes" run-command.c: use "opts->processes", not "pp->max_processes" run-command.c: don't copy "data" to "struct parallel_processes" run-command.c: don't copy "ungroup" to "struct parallel_processes" run-command.c: don't copy *_fn to "struct parallel_processes" run-command.c: make "struct parallel_processes" const if possible run-command API: move *_tr2() users to "run_processes_parallel()" run-command API: have run_process_parallel() take an "opts" struct run-command.c: use designated init for pp_init(), add "const" run-command API: don't fall back on online_cpus() run-command API: make "n" parameter a "size_t" run-command tests: use "return", not "exit" run-command API: have "run_processes_parallel{,_tr2}()" return void run-command test helper: use "else if" pattern
2022-10-18submodule--helper: drop unused argc from module_list_compute()Jeff King
The module_list_compute() function takes an argc/argv pair, but never looks at argc. This is OK, as the NULL terminator in argv is sufficient for our purposes (we feed it to parse_pathspec(), which takes only the array, not a count). Note that one of the callers _looks_ like it would be buggy, but isn't: we pass 0/NULL for argc/argv from module_foreach(), so finding the terminating NULL in that argv naively would segfault. However, parse_pathspec() is smart enough to interpret a bare NULL as an empty argv. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-10-12run-command API: move *_tr2() users to "run_processes_parallel()"Ævar Arnfjörð Bjarmason
Have the users of the "run_processes_parallel_tr2()" function use "run_processes_parallel()" instead. In preceding commits the latter was refactored to take a "struct run_process_parallel_opts" argument, since the only reason for "run_processes_parallel_tr2()" to exist was to take arguments that are now a part of that struct we can do away with it. See ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) for the addition of the "*_tr2()" variant of the function, it was used by every caller except "t/helper/test-run-command.c".. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-19Merge branch 'jk/list-objects-filter-cleanup'Junio C Hamano
A couple of bugfixes with code clean-up. * jk/list-objects-filter-cleanup: list-objects-filter: convert filter_spec to a strbuf list-objects-filter: add and use initializers list-objects-filter: handle null default filter spec list-objects-filter: don't memset after releasing filter struct
2022-09-14Merge branch 'ab/submodule-helper-leakfix'Junio C Hamano
Plugging leaks in submodule--helper. * ab/submodule-helper-leakfix: submodule--helper: fix a configure_added_submodule() leak submodule--helper: free rest of "displaypath" in "struct update_data" submodule--helper: free some "displaypath" in "struct update_data" submodule--helper: fix a memory leak in print_status() submodule--helper: fix a leak in module_add() submodule--helper: fix obscure leak in module_add() submodule--helper: fix "reference" leak submodule--helper: fix a memory leak in get_default_remote_submodule() submodule--helper: fix a leak with repo_clear() submodule--helper: fix "sm_path" and other "module_cb_list" leaks submodule--helper: fix "errmsg_str" memory leak submodule--helper: add and use *_release() functions submodule--helper: don't leak {run,capture}_command() cp.dir argument submodule--helper: "struct pathspec" memory leak in module_update() submodule--helper: fix most "struct pathspec" memory leaks submodule--helper: fix trivial get_default_remote_submodule() leak submodule--helper: fix a leak in "clone_submodule"
2022-09-14Merge branch 'ab/unused-annotation'Junio C Hamano
Undoes 'jk/unused-annotation' topic and redoes it to work around Coccinelle rules misfiring false positives in unrelated codepaths. * ab/unused-annotation: git-compat-util.h: use "deprecated" for UNUSED variables git-compat-util.h: use "UNUSED", not "UNUSED(var)"
2022-09-14Merge branch 'jk/unused-annotation'Junio C Hamano
Annotate function parameters that are not used (but cannot be removed for structural reasons), to prepare us to later compile with -Wunused warning turned on. * jk/unused-annotation: is_path_owned_by_current_uid(): mark "report" parameter as unused run-command: mark unused async callback parameters mark unused read_tree_recursive() callback parameters hashmap: mark unused callback parameters config: mark unused callback parameters streaming: mark unused virtual method parameters transport: mark bundle transport_options as unused refs: mark unused virtual method parameters refs: mark unused reflog callback parameters refs: mark unused each_ref_fn parameters git-compat-util: add UNUSED macro
2022-09-13Merge branch 'ab/submodule-helper-prep'Junio C Hamano
Code clean-up of "git submodule--helper". * ab/submodule-helper-prep: (33 commits) submodule--helper: fix bad config API usage submodule--helper: libify even more "die" paths for module_update() submodule--helper: libify more "die" paths for module_update() submodule--helper: check repo{_submodule,}_init() return values submodule--helper: libify "must_die_on_failure" code paths (for die) submodule--helper update: don't override 'checkout' exit code submodule--helper: libify "must_die_on_failure" code paths submodule--helper: libify determine_submodule_update_strategy() submodule--helper: don't exit() on failure, return submodule--helper: use "code" in run_update_command() submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string() submodule--helper: don't call submodule_strategy_to_string() in BUG() submodule--helper: add missing braces to "else" arm submodule--helper: return "ret", not "1" from update_submodule() submodule--helper: rename "int res" to "int ret" submodule--helper: don't redundantly check "else if (res)" submodule--helper: refactor "errmsg_str" to be a "struct strbuf" submodule--helper: add "const" to passed "struct update_data" submodule--helper: add "const" to copy of "update_data" submodule--helper: add "const" to passed "module_clone_data" ...
2022-09-12list-objects-filter: add and use initializersJeff King
In 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec strings, 2022-09-08), we noted that the filter_spec string_list was inconsistent in how it handled memory ownership of strings stored in the list. The fix there was a bit of a band-aid to set the "strdup_strings" variable right before adding anything. That works OK, and it lets the users of the API continue to zero-initialize the struct. But it makes the code a bit hard to follow and accident-prone, as any other spots appending the filter_spec need to think about whether to set the strdup_strings value, too (there's one such spot in partial_clone_get_default_filter_spec(), which is probably a possible memory leak). So let's do that full cleanup now. We'll introduce a LIST_OBJECTS_FILTER_INIT macro and matching function, and use them as appropriate (though it is for the "_options" struct, this matches the corresponding list_objects_filter_release() function). This is harder than it seems! Many other structs, like git_transport_data, embed the filter struct. So they need to initialize it themselves even if the rest of the enclosing struct is OK with zero-initialization. I found all of the relevant spots by grepping manually for declarations of list_objects_filter_options. And then doing so recursively for structs which embed it, and ones which embed those, and so on. I'm pretty sure I got everything, but there's no change that would alert the compiler if any topics in flight added new declarations. To catch this case, we now double-check in the parsing function that things were initialized as expected and BUG() if appropriate. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a configure_added_submodule() leakÆvar Arnfjörð Bjarmason
Fix config API a memory leak added in a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06) by using the *_tmp() variant of git_config_get_string(). In this case we're only checking whether the (repo|git)_config_get_string() call is telling us that the "submodule.active" key exists. As with the preceding commit we'll find many other such patterns in the codebase if we go fishing. E.g. "git gc" leaks in the code added in 61f7a383d3b (maintenance: use 'incremental' strategy by default, 2020-10-15). Similar code in "git gc" added in b08ff1fee00 (maintenance: add --schedule option and config, 2020-09-11) doesn't leak, but we could avoid the malloc() & free() in that case. A coccinelle rule to find those would find and fix some leaks, and cases where we're doing needless malloc() + free()'s but only care about the key existence, or are copying the (repo|git)_config_get_string() return value right away. But as with the preceding commit let's punt on all of that for now, and just narrowly fix this specific case in submodule--helper. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: free rest of "displaypath" in "struct update_data"Ævar Arnfjörð Bjarmason
Fix a leak in code added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24), we clobber the "displaypath" member of the passed-in "struct update_data" both so that die() messages in this update_submodule() function itself can use it, and for the run_update_procedure() called within this function. Fix a leak in code added in 51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). We'd always clobber the old "displaypath" member of the previously passed-in "struct update_data". A better fix for this would be to remove the "displaypath" member from the "struct update_data" entirely. Along with "oid", "suboid", "just_cloned" and "sm_path" it's managing members that mainly need to be passed between 1-3 stack frames of functions adjacent to this code. But doing so would be a much larger change (I have it locally, and fully untangling that in an incremental way is a 10 patch journey). So let's go for this much more isolated fix suggested by Glen. We FREE_AND_NULL() the "update_data->displaypath", the "AND_NULL()" part of that is needed due to the later "free(ud->displaypath)" in "update_data_release()" introduced in the preceding commit Moving ensure_core_worktree() out of update_submodule() may not be strictly required, but in doing so we are left with the exact same ordering as before, making this a smaller functional change. Helped-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: free some "displaypath" in "struct update_data"Ævar Arnfjörð Bjarmason
Make the update_data_release() function free "displaypath" member when appropriate. The "displaypath" member is always ours, the "const" on the "char *" was wrong to begin with. This leaves a leak of "displaypath" in update_submodule(), which as we'll see in subsequent commits is harder to deal with than this trivial fix. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a memory leak in print_status()Ævar Arnfjörð Bjarmason
Fix a leak in print_status(), the compute_rev_name() function implemented in this file will return a strbuf_detach()'d value, or NULL. This leak has existed since this code was added in a9f8a37584a (submodule: port submodule subcommand 'status' from shell to C, 2017-10-06), but in 0b5e2ea7cf3 (submodule--helper: don't print null in 'submodule status', 2018-04-18) we added a "const" intermediate variable for the return value, that "const" should be removed. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a leak in module_add()Ævar Arnfjörð Bjarmason
Fix a leak in module_path(), since a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10), we've been freeing add_data.sm_path, but in this case we clobbered it, and didn't free the value we clobbered. This makes test 28 of "t/t7400-submodule-basic.sh" ("submodule add in subdirectory") pass when we're compiled with SANITIZE=leak.. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix obscure leak in module_add()Ævar Arnfjörð Bjarmason
Fix an obscure leak in module_add(), if the "git add" command we were piping to failed we'd fail to strbuf_release(&sb). This fixes a leak introduced in a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C, 2021-08-10). In fixing it move to a "goto cleanup" pattern, and since we need to introduce a "ret" variable to do that let's also get rid of the intermediate "exit_code" variable. The initialization to "-1" in a6226fd772b has always been redundant, we'd only use the "exit_code" value after assigning the return value of pipe_command() to it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix "reference" leakÆvar Arnfjörð Bjarmason
Fix leaks in the "reference" variable declared in add_submodule() and module_clone(). In preceding commits this variable was refactored out of the "struct module_clone_data", but the leak has been with us since 31224cbdc72 (clone: recursive and reference option triggers submodule alternates, 2016-08-17) and 8c8195e9c3e (submodule--helper: introduce add-clone subcommand, 2021-07-10). Those commits added an xstrdup()'d member of the STRING_LIST_INIT_NODUP'd "struct string_list". We need to free() those, but not the ones we get from argv, let's make use of the "util" member, if it has a pointer it's the pointer we'll need to free, otherwise it'll be NULL (i.e. from argv). Note that the free() of the "util" member is needed in both module_clone() and add_submodule(). The module_clone() function itself doesn't populate the "util" pointer as add_submodule() does, but module_clone() is upstream of the add_possible_reference_from_superproject() caller we're modifying here, which does do that. This does preclude the use of the "util" pointer for any other reasons for now, but that's OK. If we ever need to use it for something else we could turn it into a small "struct" with an optional "to_free" member, and switch to using string_list_clear_func(). Alternatively we could have another "struct string_list to_free" which would keep a copy of the strings we've dup'd to free(). But for now this is perfectly adequate. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a memory leak in get_default_remote_submodule()Ævar Arnfjörð Bjarmason
Fix a memory leak in the get_default_remote_submodule() function added in a77c3fcb5ec (submodule--helper: get remote names from any repository, 2022-03-04), we need to repo_clear() the submodule we initialize. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a leak with repo_clear()Ævar Arnfjörð Bjarmason
Call repo_clear() in ensure_core_worktree() to free the "struct repository". Fixes a leak that's been here since 74d4731da1f (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix "sm_path" and other "module_cb_list" leaksÆvar Arnfjörð Bjarmason
Fix leaks in "struct module_cb_list" and the "struct module_cb" which it contains, these fix leaks in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13). The "sm_path" should always have been a "char *", not a "const char *", we always create it with xstrdup(). We can't mark any tests passing passing with SANITIZE=leak using "TEST_PASSES_SANITIZE_LEAK=true" as a result of this change, but "t7401-submodule-summary.sh" gets closer to passing as a result of this change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix "errmsg_str" memory leakÆvar Arnfjörð Bjarmason
Fix a memory leak introduced in e83e3333b57 (submodule: port submodule subcommand 'summary' from shell to C, 2020-08-13), we sometimes append to the "errmsg", and need to free the "struct strbuf". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: add and use *_release() functionsÆvar Arnfjörð Bjarmason
Add release functions for "struct module_list", "struct submodule_update_clone" and "struct update_data". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: don't leak {run,capture}_command() cp.dir argumentÆvar Arnfjörð Bjarmason
Fix a memory leak in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) and 3c3558f0953 (submodule--helper: run update using child process struct, 2022-03-15) by not allocating memory in the first place. The "dir" member of "struct child_process" will not be modified by that API, and it's declared to be "const char *". So let's not needlessly duplicate these strings. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: "struct pathspec" memory leak in module_update()Ævar Arnfjörð Bjarmason
The module_update() function calls module_list_compute() twice, which in turn will reset the "struct pathspec" passed to it. Let's instead track two of them, and clear them both. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix most "struct pathspec" memory leaksÆvar Arnfjörð Bjarmason
Call clear_pathspec() at the end of various functions that work with and allocate a "struct pathspec". In some cases the zero-initialization here isn't strictly needed, but as we're moving to a "goto cleanup" pattern let's make sure that it's safe to call clear_pathspec(), we don't want the data to be uninitialized. E.g. for module_foreach() we can see from looking at module_list_compute() that if it returns non-zero that the "pathspec" will always have been initialized. But relying on that both assumes knowledge about parse_pathspec(), and would set up a fragile pattern going forward. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix trivial get_default_remote_submodule() leakÆvar Arnfjörð Bjarmason
Fix a leak in code added in 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote, 2022-03-04), we need to free() the xstrdup()'d string. This gets e.g. t/t7419-submodule-set-branch.sh closer to passing under SANITIZE=leak. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix a leak in "clone_submodule"Ævar Arnfjörð Bjarmason
Fix a memory leak of the "clone_data_path" variable that we copy or derive from the "struct module_clone_data" in clone_submodule(). This code was refactored in preceding commits, but the leak has been with us since f8eaa0ba98b (submodule--helper, module_clone: always operate on absolute paths, 2016-03-31). For the "else" case we don't need to xstrdup() the "clone_data->path", and we don't need to free our own "clone_data_path". We can therefore assign the "clone_data->path" to our own "clone_data_path" right away, and only override it (and remember to free it!) if we need to xstrfmt() a replacement. In the case of the module_clone() caller it's from "argv", and doesn't need to be free'd, and in the case of the add_submodule() caller we get a pointer to "sm_path", which doesn't need to be directly free'd either. Fixing this leak makes several tests pass, so let's mark them as passing with TEST_PASSES_SANITIZE_LEAK=true. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: fix bad config API usageÆvar Arnfjörð Bjarmason
Fix bad config API usage added in a452128a36c (submodule--helper: introduce add-config subcommand, 2021-08-06). After git_config_get_string() returns successfully we know the "char **dest" will be non-NULL. A coccinelle patch that transforms this turns up a couple of other such issues, one in fetch-pack.c, and another in upload-pack.c: @@ identifier F =~ "^(repo|git)_config_get_string(_tmp)?$"; identifier V; @@ !F(..., &V) - && (V) But let's focus narrowly on submodule--helper for now, we can fix those some other time. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: libify even more "die" paths for module_update()Ævar Arnfjörð Bjarmason
As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. We've addressed the former of those in a preceding commit, let's now address the latter. In addition to lib-ifying the function this fixes a potential (but obscure) segfault introduced by a logic error in 1012a5cbc3f (submodule--helper run-update-procedure: learn --remote, 2022-03-04): We were assuming that remote_submodule_branch() would always return non-NULL, but if the submodule_from_path() call in that function fails we'll return NULL. See its introduction in 92bbe7ccf1f (submodule--helper: add remote-branch helper, 2016-08-03). I.e. we'd previously have segfaulted in the xstrfmt() call in update_submodule() seen in the context. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: libify more "die" paths for module_update()Ævar Arnfjörð Bjarmason
As noted in a preceding commit the get_default_remote_submodule() and remote_submodule_branch() functions would invoke die(), and thus leave update_submodule() only partially lib-ified. Let's address the former of those cases. Change the functions to return an int exit code (non-zero on failure), while leaving the get_default_remote() function for the callers that still want the die() semantics. This change addresses 1/2 of the "die" issue in these two lines in update_submodule(): char *remote_name = get_default_remote_submodule(update_data->sm_path); const char *branch = remote_submodule_branch(update_data->sm_path); We can safely remove the "!default_remote" case from sync_submodule(), because our get_default_remote_submodule() function now returns a die_message() on failure, so we can have it and other callers check if the exit code should be non-zero instead. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: check repo{_submodule,}_init() return valuesÆvar Arnfjörð Bjarmason
Fix code added in ce125d431aa (submodule: extract path to submodule gitdir func, 2021-09-15) and a77c3fcb5ec (submodule--helper: get remote names from any repository, 2022-03-04) which failed to check the return values of repo_init() and repo_submodule_init(). If we failed to initialize the repository or submodule we could segfault when trying to access the invalid repository structs. Let's also check that these were the only such logic errors in the codebase by making use of the "warn_unused_result" attribute. This is valid as of GCC 3.4.0 (and clang will catch it via its faking of __GNUC__ ). As the comment being added to git-compat-util.h we're piggy-backing on the LAST_ARG_MUST_BE_NULL version check out of lazyness. See 9fe3edc47f1 (Add the LAST_ARG_MUST_BE_NULL macro, 2013-07-18) for its addition. The marginal benefit of covering gcc 3.4.0..4.0.0 is near-zero (or zero) at this point. It mostly matters that we catch this somewhere. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: libify "must_die_on_failure" code paths (for die)Ævar Arnfjörð Bjarmason
Continue the libification of codepaths that previously relied on "must_die_on_failure". In these cases we've always been early aborting by calling die(), but as we know that these codepaths will properly handle return codes of 128 to mean an early abort let's have them use die_message() instead. This still isn't a complete migration away from die() for these codepaths, in particular this code in update_submodule() will still call die() in some cases: char *remote_name = get_default_remote_submodule(update_data->sm_path); const char *branch = remote_submodule_branch(update_data->sm_path); But as that code is used by other callers than the "update" code let's leave converting it for a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper update: don't override 'checkout' exit codeÆvar Arnfjörð Bjarmason
When "git submodule update" runs it might call "checkout", "merge", "rebase", or a custom command. Ever since run_update_command() was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) we'd either exit immediately if the "submodule.<name>.update" method failed, or in the case of "checkout" continue trying to update other submodules. This code used to use the magical "2" return code, but in 55b3f12cb54 (submodule update: use die_message(), 2022-03-15) it was made to exit(128), which in preceding commits has been changed to return that 128 code to the top-level. Let's "libify" this code even more by not having it arbitrarily override the return code. In practice this doesn't change anything as the code "git checkout" would return on any normal failure is "1", but we'll now in principle properly abort the operation if "git checkout" were to exit with 128. It would make sense to follow-up this change with a change to allow the "submodule.<name>.update = !..." (SM_UPDATE_COMMAND) method the same liberties as "checkout", and perhaps to do the same with a failed "merge" or "rebase". But let's leave that for now. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: libify "must_die_on_failure" code pathsÆvar Arnfjörð Bjarmason
In preceding commits the codepaths around update_submodules() were changed from using exit() or die() to ferrying up a "must_die_on_failure" in the cases where we'd exit(), and in most cases where we'd die(). We needed to do this this to ensure that we'd early exit or otherwise abort the update_submodules() processing before it was completed. Now that those preceding changes have shown that we've converted those paths, we can remove the remaining "ret == 128" special-cases, leaving the only such special-case in update_submodules(). I.e. we now know after having gone through the various codepaths that we were only returning 128 if we meant to early abort. In update_submodules() we'll for now set any non-zero non-128 exit codes to "1", but will start ferrying up the exit code as-is in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: libify determine_submodule_update_strategy()Ævar Arnfjörð Bjarmason
Libify the determine_submodule_update_strategy() by having it invoke die_message() rather than die(), and returning the code die_message() returns on failure. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: don't exit() on failure, returnÆvar Arnfjörð Bjarmason
Change code downstream of module_update() to short-circuit and return to the top-level on failure, rather than calling exit(). To do so we need to diligently check whether we "must_die_on_failure", which is a pattern started in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24), but which hadn't been completed to the point where we could avoid calling exit() here. This introduces no functional changes, but makes it easier to both call these routines as a library in the future, and to eventually avoid leaking memory. This and similar control flow in submodule--helper.c could be made simpler by properly "libifying" it, i.e. to have it consistently return -1 on failures, and to early return on any non-success. But let's leave that larger project for now, and (mostly) emulate what were doing with the "exit(128)" before this change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: use "code" in run_update_command()Ævar Arnfjörð Bjarmason
Apply some DRY principles in run_update_command() and don't have two "switch" statements over "ud->update_strategy.type" determine the same thing. First we were setting "must_die_on_failure = 1" in all cases except "SM_UPDATE_CHECKOUT" (and we'd BUG(...) out on the rest). This code was added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24). Then we'd duplicate same "switch" logic when we were using the "must_die_on_failure" variable. Let's instead have the "case" branches in that inner "switch" determine whether or not the "update must continue" by picking an exit code. This also mostly avoids hardcoding the "128" exit code, instead we can make use of the return value of the die_message() function, which we've been calling here since 55b3f12cb54 (submodule update: use die_message(), 2022-03-15). We're still hardcoding it to determine if we "exit()", but subsequent commit(s) will address that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule API: don't handle SM_..{UNSPECIFIED,COMMAND} in to_string()Ævar Arnfjörð Bjarmason
Change the submodule_strategy_to_string() function added in 3604242f080 (submodule: port init from shell to C, 2016-04-15) to really return a "const char *". In the "SM_UPDATE_COMMAND" case it would return a strbuf_detach(). Furthermore, this function would return NULL on SM_UPDATE_UNSPECIFIED, so it wasn't safe to xstrdup() its return value in the general case, or to use it in a sprintf() format as the code removed in the preceding commit did. But its callers would never call it with either SM_UPDATE_UNSPECIFIED or SM_UPDATE_COMMAND. Let's have its behavior reflect how its only user expects it to behave, and BUG() out on the rest. By doing this we can also stop needlessly xstrdup()-ing and free()-ing the memory for the config we're setting. We can instead always use constant strings. We can also use the *_tmp() variant of git_config_get_string(). Let's also rename this submodule_strategy_to_string() function to submodule_update_type_to_string(). Now that it's only tasked with returning a string version of the "enum submodule_update_type type". Before it would look at the "command" field in "struct submodule_update_strategy". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: don't call submodule_strategy_to_string() in BUG()Ævar Arnfjörð Bjarmason
Don't call submodule_strategy_to_string() in a BUG() message. These calls added in c51f8f94e5b (submodule--helper: run update procedures from C, 2021-08-24) don't need the extra information submodule_strategy_to_string() gives us, as we'll never reach the SM_UPDATE_COMMAND case here. That case is the only one where we'd get any information beyond the straightforward number-to-string mapping. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: add missing braces to "else" armÆvar Arnfjörð Bjarmason
Add missing braces to an "else" arm in init_submodule(), this stylistic change makes this code conform to the CodingGuidelines, and makes a subsequent commit smaller. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: return "ret", not "1" from update_submodule()Ævar Arnfjörð Bjarmason
Amend the update_submodule() function to return the failing "ret" on error, instead of overriding it with "1". This code was added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15), and this change ends up not making a difference as this function is only called in update_submodules(). If we return non-zero here we'll always in turn return "1" in module_update(). But if we didn't do that and returned any other non-zero exit code in update_submodules() we'd fail the test that's being amended here. We're still testing the status quo here. This change makes subsequent refactoring of update_submodule() easier, as we'll no longer need to worry about clobbering the "ret" we get from the run_command(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2022-09-02submodule--helper: rename "int res" to "int ret"Ævar Arnfjörð Bjarmason
Rename the "res" variable added in b3c5f5cb048 (submodule: move core cmd_update() logic to C, 2022-03-15) to "ret", which is the convention in the rest of this file. Eventual follow-up commits will change the code in update_submodule() to a "goto cleanup" pattern, let's have the post image look consistent with the rest. For update_submodules() let's also use a "ret" for consistency, that use was also added in b3c5f5cb048. We'll be modifying that codepath in subsequent commits. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>