Add support to develop Git in Visual Studio Code #2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@telezhnaya you asked how I got started with this... so let's see what I can remember, as it has been quite a while that I first looked into using VS Code for Git development. I got interested in this project when I read that there was a C/C++ extension for VS Code. So basically I tried all the things that were documented on that page, including the use of Intellisense. It was quite a rough start because I had no prior experience with VS Code, so I learned on the go how to operate it, what its ideas are, and what the useful keyboard shortcuts are. My favorite shortcuts:
I did pick up some Typescript development in VS Code in the meantime, and VS Code is simply awesome for Typescript development. So even if some of the C/C++ support is not quite where I want to see it, I know at least where it is heading. Already very early on, I ran into substantial problems with "problems". VS Code would point out some problems with certain header files, but when I opened said header files, the problems would miraculously go away. I reported this internally, and was delighted when some VS Code developer got back to me with the news that it was fixed. After that, I tried to figure out automatic spell correction (looking for an extension, finding the cSpell thingie, configuring it in the documented ways), and how to format code automatically in the way the core Git reviewers seem to prefer it. From years of experience on the Git mailing list, I knew what to look for: by far the most common reviewer comments point out lines that are too long: they need to be <=80 columns/line in code, and <=76 columns in commit messages. Telling contributors about whitespace is also a fun occupation of the core Git reviewers, so that was next. I did not find out how to enforce the alignment of parameters (maybe you can find something?), i.e. if (some_function_call(...) ||
some_other_call(...)) {
...
}Note how the second line is aligned to start just after the opening parentheses on the first line? That is exactly how the core Git reviewers like their code. It can get more complicated, too: static int this_is_my_static_function(int some_argument,
int next_argument)
{
...
}And of course, when you have nested parentheses, the indentation should align with the appropriate opening parenthesis. |
Member
Author
|
@telezhnaya any feedback? 😃 |
telezhnaya
reviewed
Jun 14, 2018
telezhnaya
left a comment
There was a problem hiding this comment.
Oh, I am sorry for such long waiting.
I have read the code - found nothing suspicious.
I also tested the solution on Linux, and there was a problem that it was not possible to debug the solution at the first time - there wasn't ssl.h file. I solved the issue by building Git for Windows in Linux terminal, and everything works fine after it, but I guess there must be a way not to use native Linux terminal at all.
Other problem was about stdout/stderr. I can't find it anywhere, I searched through all outputs. Maybe it's just my problem, but I think it's a good idea to add information about how to find the output to the Guidance.
|
And - thanks a lot for your story! I am sure I'm not the only person interested in such kind of information. |
Member
Author
Could you describe this problem and the |
Member
Author
@telezhnaya I think we can actually address both issues in the script. There are settings to force internal/external terminals (which might address your stdout/stderr issue, not sure until I know more about the issue), and we can make sure that we auto-detect Ubuntu and suggest to install the packages that are suggested in https://github.com/git-for-windows/git/blob/master/CONTRIBUTING.md (unless they are already installed). |
3e71fcf to
60ce1e4
Compare
Member
Author
|
/submit |
|
Submitted as [email protected] |
VS Code is a lightweight but powerful source code editor which runs on your desktop and is available for Windows, macOS and Linux. Among other languages, it has support for C/C++ via an extension, which offers to not only build and debug the code, but also Intellisense, i.e. code-aware completion and similar niceties. This patch adds a script that helps set up the environment to work effectively with VS Code: simply run the Unix shell script contrib/vscode/init.sh, which creates the relevant files, and open the top level folder of Git's source code in VS Code. Signed-off-by: Johannes Schindelin <[email protected]>
Sadly, we do not get all of the definitions via ALL_CFLAGS. Some defines are passed to GCC *only* when compiling specific files, such as git.o. Let's just hard-code them into the script for the time being. Signed-off-by: Johannes Schindelin <[email protected]>
While it is technically possible, it is confusing. Not only the user, but also VS Code's intellisense. Signed-off-by: Johannes Schindelin <[email protected]>
This helps VS Code's intellisense to figure out that we want to include windows.h, and that we want to define the minimum target Windows version as Windows Vista/2008R2. Signed-off-by: Johannes Schindelin <[email protected]>
The C/C++ settings are special, as they are the only generated VS Code configurations that *will* change over the course of Git's development, e.g. when a new constant is defined. Therefore, let's only update the C/C++ settings, also to prevent user modifications from being overwritten. Ideally, we would keep user modifications in the C/C++ settings, but that would require parsing JSON, a task for which a Unix shell script is distinctly unsuited. So we write out .new files instead, and warn the user if they may want to reconcile their changes. Signed-off-by: Johannes Schindelin <[email protected]>
When configuring VS Code as core.editor (via `code --wait`), we really want to adhere to the Git conventions of wrapping commit messages. Signed-off-by: Johannes Schindelin <[email protected]>
This adds a couple settings for the .c/.h files so that it is easier to conform to Git's conventions while editing the source code. Signed-off-by: Johannes Schindelin <[email protected]>
The quite useful cSpell extension allows VS Code to have "squiggly" lines under spelling mistakes. By default, this would add too much clutter, though, because so much of Git's source code uses words that would trigger cSpell. Let's add a few words to make the spell checking more useful by reducing the number of false positives. Signed-off-by: Johannes Schindelin <[email protected]>
By default, the cSpell extension ignores all files under .git/. That includes, unfortunately, COMMIT_EDITMSG, i.e. commit messages. However, spell checking is *quite* useful when writing commit messages... And since the user hardly ever opens any file inside .git (apart from commit messages, the config, and sometimes interactive rebase's todo lists), there is really not much harm in *not* ignoring .git/. The default also ignores `node_modules/`, but that does not apply to Git, so let's skip ignoring that, too. Signed-off-by: Johannes Schindelin <[email protected]>
Member
Author
|
/submit |
|
Submitted as [email protected] |
dscho
pushed a commit
that referenced
this pull request
Nov 4, 2019
Signed-off-by: Alessandro Menti <[email protected]>
dscho
pushed a commit
that referenced
this pull request
Nov 4, 2019
* 'update-italian-translation' of github.com:AlessandroMenti/git-po: l10n: it.po: update the Italian translation for Git 2.24.0 round #2
dscho
pushed a commit
that referenced
this pull request
Nov 4, 2019
l10n-2.24.0-rnd2 * tag 'l10n-2.24.0-rnd2' of https://github.com/git-l10n/git-po: l10n: zh_CN: for git v2.24.0 l10n round 1~2 l10n: de.po: Update German translation l10n: sv.po: Update Swedish translation (4695t0f0u) l10n: bg.po: Updated Bulgarian translation (4694) l10n: vi(4694t): Updated translation for v2.24.0 l10n: es: 2.24.0 round 2 l10n: it.po: update the Italian translation for Git 2.24.0 round #2 l10n: fr v2.24.0 rnd2 l10n: git.pot: v2.24.0 round 2 (1 new) l10n: it.po: update the Italian translation for Git 2.24.0 l10n: fr 2.24.0 rnd 1 l10n: git.pot: v2.24.0 round 1 (35 new, 16 removed) l10n: bg.po: Updated Bulgarian translation (4693) l10n: sv.po: Update Swedish translation (4674t0f0u) l10n: Update Catalan translation
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jan 29, 2025
When trying to create a Unix socket in a path that exceeds the maximum
socket name length we try to first change the directory into the parent
folder before creating the socket to reduce the length of the name. When
this fails we error out of `unix_sockaddr_init()` with an error code,
which indicates to the caller that the context has not been initialized.
Consequently, they don't release that context.
This leads to a memory leak: when we have already populated the context
with the original directory that we need to chdir(3p) back into, but
then the chdir(3p) into the socket's parent directory fails, then we
won't release the original directory's path. The leak is exposed by
t0301, but only via Meson with `meson setup -Dsanitize=leak`:
Direct leak of 129 byte(s) in 1 object(s) allocated from:
#0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o
#1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8
#2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2
#3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3
#4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7
#5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6
#6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11
#7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6
#8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3
#9 0x555555700547 in run_builtin ../git.c:480:11
#10 0x5555556ff0e0 in handle_builtin ../git.c:740:9
#11 0x5555556ffee8 in run_argv ../git.c:807:4
#12 0x5555556fee6b in cmd_main ../git.c:947:19
#13 0x55555593f689 in main ../common-main.c:64:11
#14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#16 0x5555555ad1d4 in _start (git+0x591d4)
DEDUP_TOKEN: ___interceptor_realloc.part.0--xrealloc--strbuf_grow--strbuf_getcwd--unix_sockaddr_init--unix_stream_connect--send_request--do_cache--cmd_credential_cache--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).
Fix this leak.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jan 29, 2025
We don't free the result of `remote_default_branch()`, leading to a
memory leak. This leak is exposed by t9211, but only when run with Meson
via `meson setup -Dsanitize=leak`:
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 0x5555555cfb93 in malloc (scalar+0x7bb93)
#1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8
#2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8
#3 0x5555556b0656 in xmallocz ../wrapper.c:97:9
#4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16
#5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9
#6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14
#7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28
#8 0x5555555d196b in cmd_main ../scalar.c:992:14
#9 0x5555557c4059 in main ../common-main.c:64:11
#10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#12 0x555555592054 in _start (scalar+0x3e054)
DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--remote_default_branch--cmd_clone--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).
As the `branch` variable may contain a string constant obtained from
parsing command line arguments we cannot free the leaking variable
directly. Instead, introduce a new `branch_to_free` variable that only
ever gets assigned the allocated string and free that one to plug the
leak.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jan 30, 2025
When trying to create a Unix socket in a path that exceeds the maximum
socket name length we try to first change the directory into the parent
folder before creating the socket to reduce the length of the name. When
this fails we error out of `unix_sockaddr_init()` with an error code,
which indicates to the caller that the context has not been initialized.
Consequently, they don't release that context.
This leads to a memory leak: when we have already populated the context
with the original directory that we need to chdir(3p) back into, but
then the chdir(3p) into the socket's parent directory fails, then we
won't release the original directory's path. The leak is exposed by
t0301, but only when running tests in a directory hierarchy whose path
is long enough to make the socket name length exceed the maximum socket
name length:
Direct leak of 129 byte(s) in 1 object(s) allocated from:
#0 0x5555555e85c6 in realloc.part.0 lsan_interceptors.cpp.o
#1 0x55555590e3d6 in xrealloc ../wrapper.c:140:8
#2 0x5555558c8fc6 in strbuf_grow ../strbuf.c:114:2
#3 0x5555558cacab in strbuf_getcwd ../strbuf.c:605:3
#4 0x555555923ff6 in unix_sockaddr_init ../unix-socket.c:65:7
#5 0x555555923e42 in unix_stream_connect ../unix-socket.c:84:6
#6 0x55555562a984 in send_request ../builtin/credential-cache.c:46:11
#7 0x55555562a89e in do_cache ../builtin/credential-cache.c:108:6
#8 0x55555562a655 in cmd_credential_cache ../builtin/credential-cache.c:178:3
#9 0x555555700547 in run_builtin ../git.c:480:11
#10 0x5555556ff0e0 in handle_builtin ../git.c:740:9
#11 0x5555556ffee8 in run_argv ../git.c:807:4
#12 0x5555556fee6b in cmd_main ../git.c:947:19
#13 0x55555593f689 in main ../common-main.c:64:11
#14 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#15 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#16 0x5555555ad1d4 in _start (git+0x591d4)
DEDUP_TOKEN: ___interceptor_realloc.part.0--xrealloc--strbuf_grow--strbuf_getcwd--unix_sockaddr_init--unix_stream_connect--send_request--do_cache--cmd_credential_cache--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
SUMMARY: LeakSanitizer: 129 byte(s) leaked in 1 allocation(s).
Fix this leak.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jan 30, 2025
We don't free the result of `remote_default_branch()`, leading to a
memory leak. This leak is exposed by t9211, but only when run with Meson
with the `-Db_sanitize=leak` option:
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 0x5555555cfb93 in malloc (scalar+0x7bb93)
#1 0x5555556b05c2 in do_xmalloc ../wrapper.c:55:8
#2 0x5555556b06c4 in do_xmallocz ../wrapper.c:89:8
#3 0x5555556b0656 in xmallocz ../wrapper.c:97:9
#4 0x5555556b0728 in xmemdupz ../wrapper.c:113:16
#5 0x5555556b07a7 in xstrndup ../wrapper.c:119:9
#6 0x5555555d3a4b in remote_default_branch ../scalar.c:338:14
#7 0x5555555d20e6 in cmd_clone ../scalar.c:493:28
#8 0x5555555d196b in cmd_main ../scalar.c:992:14
#9 0x5555557c4059 in main ../common-main.c:64:11
#10 0x7ffff7a2a1fb in __libc_start_call_main (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a1fb) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#11 0x7ffff7a2a2b8 in __libc_start_main@GLIBC_2.2.5 (/nix/store/h7zcxabfxa7v5xdna45y2hplj31ncf8a-glibc-2.40-36/lib/libc.so.6+0x2a2b8) (BuildId: 0a855678aa0cb573cecbb2bcc73ab8239ec472d0)
#12 0x555555592054 in _start (scalar+0x3e054)
DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--remote_default_branch--cmd_clone--cmd_main--main--__libc_start_call_main--__libc_start_main@GLIBC_2.2.5--_start
SUMMARY: LeakSanitizer: 5 byte(s) leaked in 1 allocation(s).
As the `branch` variable may contain a string constant obtained from
parsing command line arguments we cannot free the leaking variable
directly. Instead, introduce a new `branch_to_free` variable that only
ever gets assigned the allocated string and free that one to plug the
leak.
It is unclear why the leak isn't flagged when running the test via our
Makefile.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Feb 26, 2025
Submodule merges are, in general, similar to other merges based on oid three-way-merge. When a conflict happens, however, Git has two special cases (introduced in 68d03e4) on handling the conflict before yielding it to the user. From the merge-ort and merge-recursive sources: - "Case #1: a is contained in b or vice versa": both strategies try to perform a fast-forward in the submodules if the commit referred by the conflicted submodule is descendant of another; - "Case #2: There are one or more merges that contain a and b in the submodule. If there is only one, then present it as a suggestion to the user, but leave it marked unmerged so the user needs to confirm the resolution." Add a small paragraph on merge-strategies.adoc describing this behavior. Helped-by: Junio C Hamano <[email protected]> Helped-by: Elijah Newren <[email protected]> Signed-off-by: Lucas Seiki Oshiro <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Apr 23, 2025
The GitHub's CI workflow uses 'actions/checkout@v4' to checkout the repository. This action defaults to using the GitHub REST API to obtain the repository if the `git` executable isn't available. The step to build Git in the GitHub workflow can be summarized as: ... - uses: actions/checkout@v4 #1 - run: ci/install-dependencies.sh #2 ... - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh #3 ... Step #1, clones the repository, since the `git` executable isn't present at this step, it uses GitHub's REST API to obtain a tar of the repository. Step #2, installs all dependencies, which includes the `git` executable. Step #3, sets up the build, which includes setting up meson in the meson job. At this point the `git` executable is present. This means while the `git` executable is present, the repository doesn't contain the '.git' folder. To keep both the CI's (GitLab and GitHub) behavior consistent and to ensure that the build is performed on a real-world scenario, install `git` before the repository is checked out. This ensures that 'actions/checkout@v4' will clone the repository instead of using a tarball. We also update the package cache while installing `git`, this is because some distros will fail to locate the package without updating the cache. Helped-by: Phillip Wood <[email protected]> Signed-off-by: Karthik Nayak <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jul 22, 2025
find_cfg_ent() allocates a struct reflog_expire_entry_option via
FLEX_ALLOC_MEM and inserts it into a linked list in the
reflog_expire_options structure. The entries in this list are never
freed, resulting in a leak in cmd_reflog_expire and the gc reflog expire
maintenance task:
Direct leak of 39 byte(s) in 1 object(s) allocated from:
#0 0x7ff975ee6883 in calloc (/lib64/libasan.so.8+0xe6883)
#1 0x0000010edada in xcalloc ../wrapper.c:154
#2 0x000000df0898 in find_cfg_ent ../reflog.c:28
#3 0x000000df0898 in reflog_expire_config ../reflog.c:70
#4 0x00000095c451 in configset_iter ../config.c:2116
#5 0x0000006d29e7 in git_config ../config.h:724
#6 0x0000006d29e7 in cmd_reflog_expire ../builtin/reflog.c:205
#7 0x0000006d504c in cmd_reflog ../builtin/reflog.c:419
#8 0x0000007e4054 in run_builtin ../git.c:480
#9 0x0000007e4054 in handle_builtin ../git.c:746
#10 0x0000007e8a35 in run_argv ../git.c:813
#11 0x0000007e8a35 in cmd_main ../git.c:953
#12 0x000000441e8f in main ../common-main.c:9
#13 0x7ff9754115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4)
#14 0x7ff9754116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7)
#15 0x000000444184 in _start (/home/jekeller/libexec/git-core/git+0x444184)
Close this leak by adding a reflog_clear_expire_config() function which
iterates the linked list and frees its elements. Call it upon exit of
cmd_reflog_expire() and reflog_expire_condition().
Signed-off-by: Jacob Keller <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Jul 23, 2025
find_cfg_ent() allocates a struct reflog_expire_entry_option via
FLEX_ALLOC_MEM and inserts it into a linked list in the
reflog_expire_options structure. The entries in this list are never
freed, resulting in a leak in cmd_reflog_expire and the gc reflog expire
maintenance task:
Direct leak of 39 byte(s) in 1 object(s) allocated from:
#0 0x7ff975ee6883 in calloc (/lib64/libasan.so.8+0xe6883)
#1 0x0000010edada in xcalloc ../wrapper.c:154
#2 0x000000df0898 in find_cfg_ent ../reflog.c:28
#3 0x000000df0898 in reflog_expire_config ../reflog.c:70
#4 0x00000095c451 in configset_iter ../config.c:2116
#5 0x0000006d29e7 in git_config ../config.h:724
#6 0x0000006d29e7 in cmd_reflog_expire ../builtin/reflog.c:205
#7 0x0000006d504c in cmd_reflog ../builtin/reflog.c:419
#8 0x0000007e4054 in run_builtin ../git.c:480
#9 0x0000007e4054 in handle_builtin ../git.c:746
#10 0x0000007e8a35 in run_argv ../git.c:813
#11 0x0000007e8a35 in cmd_main ../git.c:953
#12 0x000000441e8f in main ../common-main.c:9
#13 0x7ff9754115f4 in __libc_start_call_main (/lib64/libc.so.6+0x35f4)
#14 0x7ff9754116a7 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x36a7)
#15 0x000000444184 in _start (/home/jekeller/libexec/git-core/git+0x444184)
Close this leak by adding a reflog_clear_expire_config() function which
iterates the linked list and frees its elements. Call it upon exit of
cmd_reflog_expire() and reflog_expire_condition().
Add a basic test which covers this leak. While at it, cover the
functionality from commit commit 3cb22b8 (Per-ref reflog expiry
configuration, 2008-06-15). We've had this support for years, but lacked
any tests.
Co-developed-by: Jeff King <[email protected]>
Signed-off-by: Jacob Keller <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Aug 28, 2025
The fill_packs_from_midx() method was refactored in fcb2205 (midx: implement support for writing incremental MIDX chains, 2024-08-06) to allow for preferred packfiles and incremental multi-pack-indexes. However, this led to some conditions that can cause improperly initialized memory in the context's list of packfiles. The conditions caring about the preferred pack name or the incremental flag are currently necessary to load a packfile. But the context is still being populated with pack_info structs based on the packfile array for the existing multi-pack-index even if prepare_midx_pack() isn't called. Add a new test that breaks under --stress when compiled with SANITIZE=address. The chosen number of 100 packfiles was selected to get the --stress output to fail about 50% of the time, while 50 packfiles could not get a failure in most --stress runs. This test has a very minor check at the end confirming only one packfile remaining. The failing nature of this test actually relies on auto-GC cleaning up some packfiles during the creation of the commits, as tests setting gc.auto to zero make the packfile count match the number of added commits but also avoids hitting the memory issue. The test case is marked as EXPENSIVE not only because of the number of packfiles it creates, but because some CI environments were reporting errors during the test that I could not reproduce, specifically around being unable to open the packfiles or their pack-indexes. When it fails under SANITIZE=address, it provides the following error: AddressSanitizer:DEADLYSIGNAL ================================================================= ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 ==3263517==The signal is caused by a READ memory access. ==3263517==Hint: address points to the zero page. #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 #1 0x562d5d82d3ab in close_pack packfile.c:354 #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 ... This failure stack trace is disconnected from the real fix because it the bad pointers are accessed later when closing the packfiles from the context. There are a few different aspects to this fix that are worth noting: 1. We return to the previous behavior of fill_packs_from_midx to not rely on the incremental flag or existence of a preferred pack. 2. The behavior to scan all layers of an incremental midx is kept, so this is not a full revert of the change. 3. We skip allocating more room in the pack_info array if the pack fails prepare_midx_pack(). 4. The method has always returned 0 for success and 1 for failure, but the condition checking for error added a check for a negative result for failure, so that is now updated. 5. The call to open_pack_index() is removed, but this is needed later in the case of a preferred pack. That call is moved to immediately before its result is needed (checking for the object count). Signed-off-by: Derrick Stolee <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Aug 30, 2025
The fill_packs_from_midx() method was refactored in fcb2205 (midx: implement support for writing incremental MIDX chains, 2024-08-06) to allow for preferred packfiles and incremental multi-pack-indexes. However, this led to some conditions that can cause improperly initialized memory in the context's list of packfiles. The conditions caring about the preferred pack name or the incremental flag are currently necessary to load a packfile. But the context is still being populated with pack_info structs based on the packfile array for the existing multi-pack-index even if prepare_midx_pack() isn't called. Add a new test that breaks under --stress when compiled with SANITIZE=address. The chosen number of 100 packfiles was selected to get the --stress output to fail about 50% of the time, while 50 packfiles could not get a failure in most --stress runs. The test case is marked as EXPENSIVE not only because of the number of packfiles it creates, but because some CI environments were reporting errors during the test that I could not reproduce, specifically around being unable to open the packfiles or their pack-indexes. When it fails under SANITIZE=address, it provides the following error: AddressSanitizer:DEADLYSIGNAL ================================================================= ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 ==3263517==The signal is caused by a READ memory access. ==3263517==Hint: address points to the zero page. #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 #1 0x562d5d82d3ab in close_pack packfile.c:354 #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 ... This failure stack trace is disconnected from the real fix because the bad pointers are accessed later when closing the packfiles from the context. There are a few different aspects to this fix that are worth noting: 1. We return to the previous behavior of fill_packs_from_midx to not rely on the incremental flag or existence of a preferred pack. 2. The behavior to scan all layers of an incremental midx is kept, so this is not a full revert of the change. 3. We skip allocating more room in the pack_info array if the pack fails prepare_midx_pack(). 4. The method has always returned 0 for success and 1 for failure, but the condition checking for error added a check for a negative result for failure, so that is now updated. 5. The call to open_pack_index() is removed, but this is needed later in the case of a preferred pack. That call is moved to immediately before its result is needed (checking for the object count). Signed-off-by: Derrick Stolee <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Sep 2, 2025
The fill_packs_from_midx() method was refactored in fcb2205 (midx: implement support for writing incremental MIDX chains, 2024-08-06) to allow for preferred packfiles and incremental multi-pack-indexes. However, this led to some conditions that can cause improperly initialized memory in the context's list of packfiles. The conditions caring about the preferred pack name or the incremental flag are currently necessary to load a packfile. But the context is still being populated with pack_info structs based on the packfile array for the existing multi-pack-index even if prepare_midx_pack() isn't called. Add a new test that breaks under --stress when compiled with SANITIZE=address. The chosen number of 100 packfiles was selected to get the --stress output to fail about 50% of the time, while 50 packfiles could not get a failure in most --stress runs. The test case is marked as EXPENSIVE not only because of the number of packfiles it creates, but because some CI environments were reporting errors during the test that I could not reproduce, specifically around being unable to open the packfiles or their pack-indexes. When it fails under SANITIZE=address, it provides the following error: AddressSanitizer:DEADLYSIGNAL ================================================================= ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 ==3263517==The signal is caused by a READ memory access. ==3263517==Hint: address points to the zero page. #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 #1 0x562d5d82d3ab in close_pack packfile.c:354 #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 ... This failure stack trace is disconnected from the real fix because the bad pointers are accessed later when closing the packfiles from the context. There are a few different aspects to this fix that are worth noting: 1. We return to the previous behavior of fill_packs_from_midx to not rely on the incremental flag or existence of a preferred pack. 2. The behavior to scan all layers of an incremental midx is kept, so this is not a full revert of the change. 3. We skip allocating more room in the pack_info array if the pack fails prepare_midx_pack(). 4. The method has always returned 0 for success and 1 for failure, but the condition checking for error added a check for a negative result for failure, so that is now updated. 5. The call to open_pack_index() is removed, but this is needed later in the case of a preferred pack. That call is moved to immediately before its result is needed (checking for the object count). Signed-off-by: Derrick Stolee <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Sep 5, 2025
The fill_packs_from_midx() method was refactored in fcb2205 (midx: implement support for writing incremental MIDX chains, 2024-08-06) to allow for preferred packfiles and incremental multi-pack-indexes. However, this led to some conditions that can cause improperly initialized memory in the context's list of packfiles. The conditions caring about the preferred pack name or the incremental flag are currently necessary to load a packfile. But the context is still being populated with pack_info structs based on the packfile array for the existing multi-pack-index even if prepare_midx_pack() isn't called. Add a new test that breaks under --stress when compiled with SANITIZE=address. The chosen number of 100 packfiles was selected to get the --stress output to fail about 50% of the time, while 50 packfiles could not get a failure in most --stress runs. The test case is marked as EXPENSIVE not only because of the number of packfiles it creates, but because some CI environments were reporting errors during the test that I could not reproduce, specifically around being unable to open the packfiles or their pack-indexes. When it fails under SANITIZE=address, it provides the following error: AddressSanitizer:DEADLYSIGNAL ================================================================= ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 ==3263517==The signal is caused by a READ memory access. ==3263517==Hint: address points to the zero page. #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 #1 0x562d5d82d3ab in close_pack packfile.c:354 #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 ... This failure stack trace is disconnected from the real fix because the bad pointers are accessed later when closing the packfiles from the context. There are a few different aspects to this fix that are worth noting: 1. We return to the previous behavior of fill_packs_from_midx to not rely on the incremental flag or existence of a preferred pack. 2. The behavior to scan all layers of an incremental midx is kept, so this is not a full revert of the change. 3. We skip allocating more room in the pack_info array if the pack fails prepare_midx_pack(). 4. The method has always returned 0 for success and 1 for failure, but the condition checking for error added a check for a negative result for failure, so that is now updated. 5. The call to open_pack_index() is removed, but this is needed later in the case of a preferred pack. That call is moved to immediately before its result is needed (checking for the object count). Signed-off-by: Derrick Stolee <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 7, 2025
When performing auto-maintenance we check whether commit graphs need to
be generated by counting the number of commits that are reachable by any
reference, but not covered by a commit graph. This search is performed
by iterating through all references and then doing a depth-first search
until we have found enough commits that are not present in the commit
graph.
This logic has a memory leak though:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x55555562e433 in malloc (git+0xda433)
#1 0x555555964322 in do_xmalloc ../wrapper.c:55:8
#2 0x5555559642e6 in xmalloc ../wrapper.c:76:9
#3 0x55555579bf29 in commit_list_append ../commit.c:1872:35
#4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4
#5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12
#6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9
#7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9
#8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11
#9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8
#10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9
#11 0x55555575166a in run_builtin ../git.c:506:11
#12 0x5555557502f0 in handle_builtin ../git.c:779:9
#13 0x555555751127 in run_argv ../git.c:862:4
#14 0x55555575007b in cmd_main ../git.c:984:19
#15 0x5555557523aa in main ../common-main.c:9:11
#16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#18 0x5555555f0934 in _start (git+0x9c934)
The root cause of this memory leak is our use of `commit_list_append()`.
This function expects as parameters the item to append and the _tail_ of
the list to append. This tail will then be overwritten with the new tail
of the list so that it can be used in subsequent calls. But we call it
with `commit_list_append(parent->item, &stack)`, so we end up losing
everything but the new item.
This issue only surfaces when counting merge commits. Next to being a
memory leak, it also shows that we're in fact miscounting as we only
respect children of the last parent. All previous parents are discarded,
so their children will be disregarded unless they are hit via another
reference.
While crafting a test case for the issue I was puzzled that I couldn't
establish the proper border at which the auto-condition would be
fulfilled. As it turns out, there's another bug: if an object is at the
tip of any reference we don't mark it as seen. Consequently, if it is
reachable via any other reference, we'd count that object twice.
Fix both of these bugs so that we properly count objects without leaking
any memory.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 7, 2025
In the next commit we are about to move the packfile store into the ODB
source so that we have one store per source. This will lead to a memory
leak in the following commit when reading data from a submodule via
git-grep(1):
Direct leak of 192 byte(s) in 1 object(s) allocated from:
#0 0x55555562e726 in calloc (git+0xda726)
#1 0x555555964734 in xcalloc ../wrapper.c:154:8
#2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2
#3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6
#4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17
#5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3
#6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2
#7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2
#8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4
#9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8
#10 0x5555558540bd in odb_read_object ../odb.c:920:6
#11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12
#12 0x55555580a13a in grep_source_load ../grep.c:1986:10
#13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7
#14 0x555555807574 in grep_source_1 ../grep.c:1625:8
#15 0x555555807322 in grep_source ../grep.c:1837:10
#16 0x5555556a5c58 in run ../builtin/grep.c:208:10
#17 0x55555562bb42 in void* ThreadStartFunc<false>(void*) lsan_interceptors.cpp.o
#18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
The root caues of this leak is the way we set up and release the
submodule:
1. We use `repo_submodule_init()` to initialize a new repository. This
repository is stored in `repos_to_free`.
2. We now read data from the submodule repository.
3. We then call `repo_clear()` on the submodule repositories.
4. `repo_clear()` calls `odb_free()`.
5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`.
The issue here is the 5th step: we call `odb_free_sources()` _before_ we
call `odb_close()`. But `odb_free_sources()` already frees all sources,
so the logic that closes them in `odb_close()` now becomes a no-op. As a
consequence, we never explicitly close sources at all.
Fix the leak by closing the store before we free the sources.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 7, 2025
The MIDX file format currently requires that pack files be identified by
the lexicographic ordering of their names (that is, a pack having a
checksum beginning with "abc" would have a numeric pack_int_id which is
smaller than the same value for a pack beginning with "bcd").
As a result, it is impossible to combine adjacent MIDX layers together
without permuting bits from bitmaps that are in more recent layer(s).
To see why, consider the following example:
| packs | preferred pack
--------+-------------+---------------
MIDX #0 | { X, Y, Z } | Y
MIDX #1 | { A, B, C } | B
MIDX #2 | { D, E, F } | D
, where MIDX #2's base MIDX is MIDX #1, and so on. Suppose that we want
to combine MIDX layers #0 and #1, to create a new layer #0' containing
the packs from both layers. With the original three MIDX layers, objects
are laid out in the bitmap in the order they appear in their source
pack, and the packs themselves are arranged according to the pseudo-pack
order. In this case, that ordering is Y, X, Z, B, A, C.
But recall that the pseudo-pack ordering is defined by the order that
packs appear in the MIDX, with the exception of the preferred pack,
which sorts ahead of all other packs regardless of its position within
the MIDX. In the above example, that means that pack 'Y' could be placed
anywhere (so long as it is designated as preferred), however, all other
packs must be placed in the location listed above.
Because that ordering isn't sorted lexicographically, it is impossible
to compact MIDX layers in the above configuration without permuting the
object-to-bit-position mapping. Changing this mapping would affect all
bitmaps belonging to newer layers, rendering the bitmaps associated with
MIDX #2 unreadable.
One of the goals of MIDX compaction is that we are able to shrink the
length of the MIDX chain *without* invalidating bitmaps that belong to
newer layers, and the lexicographic ordering constraint is at odds with
this goal.
However, packs do not *need* to be lexicographically ordered within the
MIDX. As far as I can gather, the only reason they are sorted lexically
is to make it possible to perform a binary search over the pack names in
a MIDX, necessary to make `midx_contains_pack()`'s performance
logarithmic in the number of packs rather than linear.
Relax this constraint by allowing MIDX writes to proceed with packs that
are not arranged in lexicographic order. `midx_contains_pack()` will
lazily instantiate a `pack_names_sorted` array on the MIDX, which will
be used to implement the binary search over pack names.
Note that this produces MIDXs which may be incompatible with earlier
versions of Git that have stricter requirements on the layout of packs
within a MIDX. This patch does *not* modify the version number of the
MIDX format, since existing versions of Git already know to gracefully
ignore a MIDX with packs that appear out-of-order.
Signed-off-by: Taylor Blau <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 7, 2025
When pushing to a set of remotes using a nickname for the group, the
client initializes the connection to each remote, talks to the
remote and reads and parses capabilities line, and holds the
capabilities in a file-scope static variable server_capabilities_v1.
There are a few other such file-scope static variables, and these
connections cannot be parallelized until they are refactored to a
structure that keeps track of active connections.
Which is *not* the theme of this patch ;-)
For a single connection, the server_capabilities_v1 variable is
initialized to NULL (at the program initialization), populated when
we talk to the other side, used to look up capabilities of the other
sdie possible multiple times, and the memory is held by the variable
until program exit, without leaking. When talking to multiple remotes,
however, the server capabilities from the second connection overwrites
without freeing the one from the first connection, which leaks.
==1080970==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 421 byte(s) in 2 object(s) allocated from:
#0 0x5615305f849e in strdup (/home/gitster/g/git-jch/bin/bin/git+0x2b349e) (BuildId: 54d149994c9e85374831958f694bd0aa3b8b1e26)
#1 0x561530e76cc4 in xstrdup /home/gitster/w/build/wrapper.c:43:14
#2 0x5615309cd7fa in process_capabilities /home/gitster/w/build/connect.c:243:27
#3 0x5615309cd502 in get_remote_heads /home/gitster/w/build/connect.c:366:4
#4 0x561530e2cb0b in handshake /home/gitster/w/build/transport.c:372:3
#5 0x561530e29ed7 in get_refs_via_connect /home/gitster/w/build/transport.c:398:9
#6 0x561530e26464 in transport_push /home/gitster/w/build/transport.c:1421:16
#7 0x561530800bec in push_with_options /home/gitster/w/build/builtin/push.c:387:8
#8 0x5615307ffb99 in do_push /home/gitster/w/build/builtin/push.c:442:7
#9 0x5615307fe926 in cmd_push /home/gitster/w/build/builtin/push.c:664:7
#10 0x56153065673f in run_builtin /home/gitster/w/build/git.c:506:11
#11 0x56153065342f in handle_builtin /home/gitster/w/build/git.c:779:9
#12 0x561530655b89 in run_argv /home/gitster/w/build/git.c:862:4
#13 0x561530652cba in cmd_main /home/gitster/w/build/git.c:984:19
#14 0x5615308dda0a in main /home/gitster/w/build/common-main.c:9:11
#15 0x7f051651bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
SUMMARY: AddressSanitizer: 421 byte(s) leaked in 2 allocation(s).
Free the capablities data for the previous server before overwriting
it with the next server to plug this leak.
The added test fails without the freeing with SANITIZE=leak; I
somehow couldn't get it fail reliably with SANITIZE=leak,address
though.
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 9, 2025
When pushing to a set of remotes using a nickname for the group, the
client initializes the connection to each remote, talks to the
remote and reads and parses capabilities line, and holds the
capabilities in a file-scope static variable server_capabilities_v1.
There are a few other such file-scope static variables, and these
connections cannot be parallelized until they are refactored to a
structure that keeps track of active connections.
Which is *not* the theme of this patch ;-)
For a single connection, the server_capabilities_v1 variable is
initialized to NULL (at the program initialization), populated when
we talk to the other side, used to look up capabilities of the other
side possibly multiple times, and the memory is held by the variable
until program exit, without leaking. When talking to multiple remotes,
however, the server capabilities from the second connection overwrites
without freeing the one from the first connection, which leaks.
==1080970==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 421 byte(s) in 2 object(s) allocated from:
#0 0x5615305f849e in strdup (/home/gitster/g/git-jch/bin/bin/git+0x2b349e) (BuildId: 54d149994c9e85374831958f694bd0aa3b8b1e26)
#1 0x561530e76cc4 in xstrdup /home/gitster/w/build/wrapper.c:43:14
#2 0x5615309cd7fa in process_capabilities /home/gitster/w/build/connect.c:243:27
#3 0x5615309cd502 in get_remote_heads /home/gitster/w/build/connect.c:366:4
#4 0x561530e2cb0b in handshake /home/gitster/w/build/transport.c:372:3
#5 0x561530e29ed7 in get_refs_via_connect /home/gitster/w/build/transport.c:398:9
#6 0x561530e26464 in transport_push /home/gitster/w/build/transport.c:1421:16
#7 0x561530800bec in push_with_options /home/gitster/w/build/builtin/push.c:387:8
#8 0x5615307ffb99 in do_push /home/gitster/w/build/builtin/push.c:442:7
#9 0x5615307fe926 in cmd_push /home/gitster/w/build/builtin/push.c:664:7
#10 0x56153065673f in run_builtin /home/gitster/w/build/git.c:506:11
#11 0x56153065342f in handle_builtin /home/gitster/w/build/git.c:779:9
#12 0x561530655b89 in run_argv /home/gitster/w/build/git.c:862:4
#13 0x561530652cba in cmd_main /home/gitster/w/build/git.c:984:19
#14 0x5615308dda0a in main /home/gitster/w/build/common-main.c:9:11
#15 0x7f051651bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
SUMMARY: AddressSanitizer: 421 byte(s) leaked in 2 allocation(s).
Free the capablities data for the previous server before overwriting
it with the next server to plug this leak.
The added test fails without the freeing with SANITIZE=leak; I
somehow couldn't get it fail reliably with SANITIZE=leak,address
though.
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 11, 2025
When performing auto-maintenance we check whether commit graphs need to
be generated by counting the number of commits that are reachable by any
reference, but not covered by a commit graph. This search is performed
by iterating through all references and then doing a depth-first search
until we have found enough commits that are not present in the commit
graph.
This logic has a memory leak though:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x55555562e433 in malloc (git+0xda433)
#1 0x555555964322 in do_xmalloc ../wrapper.c:55:8
#2 0x5555559642e6 in xmalloc ../wrapper.c:76:9
#3 0x55555579bf29 in commit_list_append ../commit.c:1872:35
#4 0x55555569f160 in dfs_on_ref ../builtin/gc.c:1165:4
#5 0x5555558c33fd in do_for_each_ref_iterator ../refs/iterator.c:431:12
#6 0x5555558af520 in do_for_each_ref ../refs.c:1828:9
#7 0x5555558ac317 in refs_for_each_ref ../refs.c:1833:9
#8 0x55555569e207 in should_write_commit_graph ../builtin/gc.c:1188:11
#9 0x55555569c915 in maintenance_is_needed ../builtin/gc.c:3492:8
#10 0x55555569b76a in cmd_maintenance ../builtin/gc.c:3542:9
#11 0x55555575166a in run_builtin ../git.c:506:11
#12 0x5555557502f0 in handle_builtin ../git.c:779:9
#13 0x555555751127 in run_argv ../git.c:862:4
#14 0x55555575007b in cmd_main ../git.c:984:19
#15 0x5555557523aa in main ../common-main.c:9:11
#16 0x7ffff7a2a4d7 in __libc_start_call_main (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a4d7) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#17 0x7ffff7a2a59a in __libc_start_main@GLIBC_2.2.5 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x2a59a) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#18 0x5555555f0934 in _start (git+0x9c934)
The root cause of this memory leak is our use of `commit_list_append()`.
This function expects as parameters the item to append and the _tail_ of
the list to append. This tail will then be overwritten with the new tail
of the list so that it can be used in subsequent calls. But we call it
with `commit_list_append(parent->item, &stack)`, so we end up losing
everything but the new item.
This issue only surfaces when counting merge commits. Next to being a
memory leak, it also shows that we're in fact miscounting as we only
respect children of the last parent. All previous parents are discarded,
so their children will be disregarded unless they are hit via another
reference.
While crafting a test case for the issue I was puzzled that I couldn't
establish the proper border at which the auto-condition would be
fulfilled. As it turns out, there's another bug: if an object is at the
tip of any reference we don't mark it as seen. Consequently, if it is
reachable via any other reference, we'd count that object twice.
Fix both of these bugs so that we properly count objects without leaking
any memory.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
gitgitgadget bot
pushed a commit
that referenced
this pull request
Dec 11, 2025
It is possible to hit a memory leak when reading data from a submodule
via git-grep(1):
Direct leak of 192 byte(s) in 1 object(s) allocated from:
#0 0x55555562e726 in calloc (git+0xda726)
#1 0x555555964734 in xcalloc ../wrapper.c:154:8
#2 0x555555835136 in load_multi_pack_index_one ../midx.c:135:2
#3 0x555555834fd6 in load_multi_pack_index ../midx.c:382:6
#4 0x5555558365b6 in prepare_multi_pack_index_one ../midx.c:716:17
#5 0x55555586c605 in packfile_store_prepare ../packfile.c:1103:3
#6 0x55555586c90c in packfile_store_reprepare ../packfile.c:1118:2
#7 0x5555558546b3 in odb_reprepare ../odb.c:1106:2
#8 0x5555558539e4 in do_oid_object_info_extended ../odb.c:715:4
#9 0x5555558533d1 in odb_read_object_info_extended ../odb.c:862:8
#10 0x5555558540bd in odb_read_object ../odb.c:920:6
#11 0x55555580a330 in grep_source_load_oid ../grep.c:1934:12
#12 0x55555580a13a in grep_source_load ../grep.c:1986:10
#13 0x555555809103 in grep_source_is_binary ../grep.c:2014:7
#14 0x555555807574 in grep_source_1 ../grep.c:1625:8
#15 0x555555807322 in grep_source ../grep.c:1837:10
#16 0x5555556a5c58 in run ../builtin/grep.c:208:10
#17 0x55555562bb42 in void* ThreadStartFunc<false>(void*) lsan_interceptors.cpp.o
#18 0x7ffff7a9a979 in start_thread (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x9a979) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
#19 0x7ffff7b22d2b in __GI___clone3 (/nix/store/xx7cm72qy2c0643cm1ipngd87aqwkcdp-glibc-2.40-66/lib/libc.so.6+0x122d2b) (BuildId: cddea92d6cba8333be952b5a02fd47d61054c5ab)
The root caues of this leak is the way we set up and release the
submodule:
1. We use `repo_submodule_init()` to initialize a new repository. This
repository is stored in `repos_to_free`.
2. We now read data from the submodule repository.
3. We then call `repo_clear()` on the submodule repositories.
4. `repo_clear()` calls `odb_free()`.
5. `odb_free()` calls `odb_free_sources()` followed by `odb_close()`.
The issue here is the 5th step: we call `odb_free_sources()` _before_ we
call `odb_close()`. But `odb_free_sources()` already frees all sources,
so the logic that closes them in `odb_close()` now becomes a no-op. As a
consequence, we never explicitly close sources at all.
Fix the leak by closing the store before we free the sources.
Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Visual Studio Code (nickname "VS Code") is a light-weight, cross-platform, Open Source development environment, with an increasingly powerful extension to support C/C++ development. In particular the intellisense support as well as all the other niceties developers might have come to expect from Integrated Development Environments will help accelerate development.
Due to the way Git's source code and build process is structured, it can be quite challenging to use VS Code effectively for developing Git. Which is a shame, as developing with vim and the command-line causes unnecessary churn, and it is quite understandable that most Git developers simply do not want to fight with a modern development environment just to try whether they like developing Git with it.
This topic branch makes it easy to get started using VS Code to develop Git.
Simply run the script
./contrib/vscode/init.sh. This will initialize the.vscode/directory and some files in that directory. After that, just open Git's top-level directory as "folder" in VS Code.The files have to be generated because of the curious way Git determines what flags to pass to the C compiler, in particular which constants are defined, because they change the compile flow in rather dramatic ways (determining, e.g. which SHA-1 backend to use).
Changes since v1: