git/bundle.c, branch v2.40.2

Merge branch 'ab/various-leak-fixes'

2023-02-22T22:55:45Z

Leak fixes. * ab/various-leak-fixes: push: free_refs() the "local_refs" in set_refspecs() push: refactor refspec_append_mapped() for subsequent leak-fix receive-pack: release the linked "struct command *" list grep API: plug memory leaks by freeing "header_list" grep.c: refactor free_grep_patterns() builtin/merge.c: free "&buf" on "Your local changes..." error builtin/merge.c: use fixed strings, not "strbuf", fix leak show-branch: free() allocated "head" before return commit-graph: fix a parse_options_concat() leak http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}() http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main() worktree: fix a trivial leak in prune_worktrees() repack: fix leaks on error with "goto cleanup" name-rev: don't xstrdup() an already dup'd string various: add missing clear_pathspec(), fix leaks clone: use free() instead of UNLEAK() commit-graph: use free_commit_graph() instead of UNLEAK() bundle.c: don't leak the "args" in the "struct child_process" tests: mark tests as passing with SANITIZE=leak

bundle.c: don't leak the "args" in the "struct child_process"

2023-02-06T23:34:36Z

Fix a leak that's been here since 7366096de9d (bundle API: change "flags" to be "extra_index_pack_args", 2021-09-05). If we can't verify the bundle, we didn't call child_process_clear() to clear the "args". But rather than adding an additional child_process_clear() call, let's verify the bundle before we start preparing the process we're going to spawn. If we fail to verify, we don't need to push anything to the child_process "args". Helped-by: Elijah Newren Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

bundle: verify using check_connected()

2023-01-31T16:57:47Z

When Git verifies a bundle to see if it is safe for unbundling, it first looks to see if the prerequisite commits are in the object store. This is an easy way to "fail fast" but it is not a sufficient check for updating refs that guarantee closure under reachability. There could still be issues if those commits are not reachable from the repository's references. The repository only has guarantees that its object store is closed under reachability for the objects that are reachable from references. Thus, the code in verify_bundle() has previously had the additional check that all prerequisite commits are reachable from repository references. This is done via a revision walk from all references, stopping only if all prerequisite commits are discovered or all commits are walked. This uses a custom walk to verify_bundle(). This check is more strict than what Git applies to fetched pack-files. In the fetch case, Git guarantees that the new references are closed under reachability by walking from the new references until walking commits that are reachable from repository refs. This is done through the well-used check_connected() method. To better align with the restrictions required by 'git fetch', reimplement this check in verify_bundle() to use check_connected(). This also simplifies the code significantly. The previous change added a test that verified the behavior of 'git bundle verify' and 'git bundle unbundle' in this case, and the error messages looked like this: error: Could not read fatal: Failed to traverse parents of commit However, by changing the revision walk slightly within check_connected() and using its quiet mode, we can omit those messages. Instead, we get only this message, tailored to describing the current state of the repository: error: some prerequisite commits exist in the object store, but are not connected to the repository's history (Line break added here for the commit message formatting, only.) While this message does not include any object IDs, there is no guarantee that those object IDs would help the user diagnose what is going on, as they could be separated from the prerequisite commits by some distance. At minimum, this situation describes the situation in a more informative way than the previous error messages. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

bundle-uri: quiet failed unbundlings

2022-10-12T16:13:25Z

When downloading a list of bundles in "all" mode, Git has no understanding of the dependencies between the bundles. Git attempts to unbundle the bundles in some order, but some may not pass the verify_bundle() step because of missing prerequisites. This is passed as error messages to the user, even when they eventually succeed in later attempts after their dependent bundles are unbundled. Add a new VERIFY_BUNDLE_QUIET flag to verify_bundle() that avoids the error messages from the missing prerequisite commits. The method still returns the number of missing prerequisit commits, allowing callers to unbundle() to notice that the bundle failed to apply. Use this flag in bundle-uri.c and test that the messages go away for 'git clone --bundle-uri' commands. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

bundle: add flags to verify_bundle()

2022-10-12T16:13:25Z

The verify_bundle() method has a 'verbose' option, but we will want to extend this method to have more granular control over its output. First, replace this 'verbose' option with a new 'flags' option with a single possible value: VERIFY_BUNDLE_VERBOSE. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

bundle: properly clear all revision flags

2022-10-12T16:13:25Z

The verify_bundle() method checks two things for a bundle's prerequisites: 1. Are these objects in the object store? 2. Are these objects reachable from our references? In this second question, multiple uses of verify_bundle() in the same process can report an invalid bundle even though it is correct. The reason is due to not clearing all of the commit marks on the commits previously walked. The revision walk machinery was first introduced in-process by fb9a54150d3 (git-bundle: avoid fork() in verify_bundle(), 2007-02-22). This implementation used "-1" as the set of flags to clear. The next meaningful change came in 2b064697a5b (revision traversal: retire BOUNDARY_SHOW, 2007-03-05), which introduced the PREREQ_MARK flag instead of a flag normally controlled by the revision-walk machinery. In 86a0a408b90 (commit: factor out clear_commit_marks_for_object_array, 2011-10-01), the loop over the array of commits was replaced with a new clear_commit_marks_for_object_array(), but simultaneously the "-1" value was replaced with "ALL_REV_FLAGS", which stopped un-setting the PREREQ_MARK flag. This means that if multiple commits were marked by the PREREQ_MARK in a previous run of verify_bundle(), then this loop could terminate early due to 'i' going to zero: while (i && (commit = get_revision(&revs))) if (commit->object.flags & PREREQ_MARK) i--; The flag clearing work was changed again in 63647391e6c (bundle: avoid using the rev_info flag leak_pending, 2017-12-25), but that was only cosmetic and did not change the behavior. It may seem that it would be sufficient to add the PREREQ_MARK flag to the clear_commit_marks() call in its current location. However, we actually need to do it in the "cleanup:" step, since the first loop checking "Are these objects in the object store?" might add the PREREQ_MARK flag to some objects and then terminate without performing a walk due to one missing object. By clearing the flags in all cases, we avoid this issue when running verify_bundle() multiple times in the same process. Moving this loop to the cleanup step alone would cause a segfault when running 'git bundle verify' outside of a repository, but this is because of that error condition using "goto cleanup" when returning is perfectly safe. Nothing has been initialized at that point, so we can return immediately without causing any leaks. This behavior is verified carefully by a test that will be added soon when Git learns to download bundle lists in a 'git clone --bundle-uri' command. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

Merge branch 'ab/plug-leak-in-revisions'

2022-06-07T21:10:56Z

Plug the memory leaks from the trickiest API of all, the revision walker. * ab/plug-leak-in-revisions: (27 commits) revisions API: add a TODO for diff_free(&revs->diffopt) revisions API: have release_revisions() release "topo_walk_info" revisions API: have release_revisions() release "date_mode" revisions API: call diff_free(&revs->pruning) in revisions_release() revisions API: release "reflog_info" in release revisions() revisions API: clear "boundary_commits" in release_revisions() revisions API: have release_revisions() release "prune_data" revisions API: have release_revisions() release "grep_filter" revisions API: have release_revisions() release "filter" revisions API: have release_revisions() release "cmdline" revisions API: have release_revisions() release "mailmap" revisions API: have release_revisions() release "commits" revisions API users: use release_revisions() for "prune_data" users revisions API users: use release_revisions() with UNLEAK() revisions API users: use release_revisions() in builtin/log.c revisions API users: use release_revisions() in http-push.c revisions API users: add "goto cleanup" for release_revisions() stash: always have the owner of "stash_info" free it revisions API users: use release_revisions() needing REV_INFO_INIT revision.[ch]: document and move code declared around "init" ...

bundle.h: make "fd" version of read_bundle_header() public

2022-05-16T22:02:10Z

Change the parse_bundle_header() function to be non-static, and rename it to parse_bundle_header_fd(). The parse_bundle_header() function is already public, and it's a thin wrapper around this function. This will be used by code that wants to pass a fd to the bundle API. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano

revisions API users: use release_revisions() needing REV_INFO_INIT

2022-04-14T06:56:08Z

Use release_revisions() to various users of "struct rev_list" which need to have their "struct rev_info" zero-initialized before we can start using it. For the bundle.c code see the early exit case added in 3bbbe467f29 (bundle verify: error out if called without an object database, 2019-05-27). For the relevant bisect.c code see 45b6370812c (bisect: libify `check_good_are_ancestors_of_bad` and its dependents, 2020-02-17). For the submodule.c code see the "goto" on "(!left || !right || !sub)" added in 8e6df65015f (submodule: refactor show_submodule_summary with helper function, 2016-08-31). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano

bundle: output hash information in 'verify'

2022-03-23T20:14:09Z

The previous change moved the 'filter' capability to the end of the 'git bundle verify' output. Now, add the 'object-format' capability to the output, when it exists. This change makes 'git bundle verify' output the hash used in all cases, even if the capability is not in the bundle. This means that v2 bundles will always output that they use "sha1". This might look noisy to some users, but it does simplify the implementation and the test strategy for this feature. Since 'verify' ends early when a prerequisite commit is missing, we need to insert this hash message carefully into our expected test output throughout t6020. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano