* cygwin v2.55.0 test failures
From: Ramsay Jones @ 2026-07-10 18:32 UTC (permalink / raw)
To: GIT Mailing-list
Cc: Johannes Schindelin, Patrick Steinhardt, Junio C Hamano,
Johannes Sixt, Adam Dinwoodie, Torsten Bögershausen
Sorry for being a bit tardy, but I was somewhat unwell and, as a result,
AFK for some weeks during the last part of the v2.55.0 cycle. So, I'm
still catching up. As part of that, I found that the cygwin test-suite
fails for v2.55.0, like so:
$ tail -13 test-out-2-55-rel
Test Summary Report
-------------------
unit-tests/bin/unit-tests.exe (Wstat: 256 (exited 1) Tests: 249 Failed: 1)
Failed test: 242
Non-zero exit status: 1
t9904-url-parse.sh (Wstat: 256 (exited 1) Tests: 53 Failed: 4)
Failed tests: 39, 42-43, 47
Non-zero exit status: 1
Files=1047, Tests=32848, 4584 wallclock secs (29.05 usr 85.59 sys + 9679.08 cusr 14883.94 csys = 24677.67 CPU)
Result: FAIL
make[1]: *** [Makefile:78: prove] Error 1
make[1]: Leaving directory '/home/ramsay/git/t'
make: *** [Makefile:3381: test] Error 2
$
Unsurprisingly, the -rc0, -rc1 and -rc2 builds show the same failures.
After a quick squint, the reason for the failure looked familiar ... ;)
Indeed, I have been aware of the reason for this failure since v2.23.1
at the end of 2019. For those who don't instantly recognize it, this was
one of a series of security releases, which mainly affected GfW. From
the release Notes:
Git v2.23.1 Release Notes
=========================
This release merges up the fixes that appear in v2.14.6, v2.15.4,
v2.17.3, v2.20.2 and in v2.21.1, addressing the security issues
CVE-2019-1348, CVE-2019-1349, CVE-2019-1350, CVE-2019-1351,
CVE-2019-1352, CVE-2019-1353, CVE-2019-1354, CVE-2019-1387, and
CVE-2019-19604; see the release notes for those versions for details.
For example, the test-suite for v2.25.0-rc0 looks like:
$ tail -17 test-out/test-out-2-25-rc0
Test Summary Report
-------------------
t5500-fetch-pack.sh (Wstat: 256 Tests: 369 Failed: 12)
Failed tests: 142, 145, 161-162, 239, 242, 258-259, 336
339, 355-356
Non-zero exit status: 1
t5580-clone-push-unc.sh (Wstat: 256 Tests: 6 Failed: 1)
Failed test: 4
Non-zero exit status: 1
t5601-clone.sh (Wstat: 256 Tests: 104 Failed: 4)
Failed tests: 62-64, 66
Non-zero exit status: 1
Files=890, Tests=21098, 16304 wallclock secs ( 0.48 usr 0.73 sys + 2374.33 cusr 7438.69 csys = 9814.24 CPU)
Result: FAIL
make[1]: *** [Makefile:52: prove] Error 1
make[1]: Leaving directory '/home/ramsay/git/t'
make: *** [Makefile:2754: test] Error 2
$
[t5580-clone-push-unc.sh was later renamed to t5580-unc-paths.sh]
From about that time, I had hacked up a 'fix' for cygwin, but hadn't decided
how to proceed with an actual patch. :)
So, I cherry-picked the 'fix' onto 'master' (then on v2.55.0 release). I was
a little surprised it went without problem, given how much 'git-compat-util.h'
has changed, but it just required a 'slide' from line 203 back to line 153.
This patch is equivalent to the 'git-compat-util.h' only part of the patch
given below.
This fixed up the current test failures (unit-tests and t9904-url-parse.sh).
Note that Patrick wanted to have a clean test-suite run on cygwin, so in
commit 5f8af25ff9 ("t5500, t5601: skip tests which exercise paths with '[::1]'
on Cygwin", 2024-10-16), he suppressed the test failures in t5500 and t5601.
(that was about the time of the v2.48.0 release).
The changes to tests t5500 and t5601, in the patch given below, essentially
reverts Patrick's commit 5f8af25ff9. This fixes all of the tests in t5601 and
ten of the twelve failures in t5500. (I don't recall what happened to t5580,
the single failure - the push test - was fixed somewhere between v2.43.0 and
v2.44.0-rc0).
As luck would have it, I left a note to myself about the remaining two
failure cases. This leads to the remaining hunk, to connect.c, in the patch
below; ie. the removal of a conditional (which should only fire for GfW and
cygwin). The '#ifdef DUMMY/#endif' should probably be replaced with an
'#ifdef GIT_WINDOWS_NATIVE/#endif' so that GfW is not affected. (Having said
that, I suspect that even GfW should drop it ['somebody was smoking something
exotic'], but I have no way to test it, so ...).
With this final hunk, this patch results in a clean test-suite run. :)
So, what does this mean? Well, I should probably not leave it another five
years, or thereabouts, before actually sending a real patch (series) to fix
this up properly! ;)
But what is properly? The 'fix' works, but the problem was principally caused
by cygwin being a bit schizophrenic about the 'pathname utilities' and its
support for POSIX only paths, WIN32 only paths or both.
For example, f82a97eb91 ("mingw: handle `subst`-ed "DOS drives"", 2019-09-06)
notes in the commit message:
Note: `[::1]:repo` is a valid URL, but not a valid path on Windows.
As `[` is now considered a valid drive letter, we need to be very
careful to avoid misinterpreting such a string as valid local path in
`url_is_local_not_ssh()`. To do that, we use the just-introduced
function `is_valid_path()` (which will label the string as invalid file
name because of the colon characters).
This fixes CVE-2019-1351.
Note that part of the fix involves a cygwin specific 'is_valid_path()' which
would otherwise be defined, like Linux, as simply '1' in git-compat-util.h.
I was a bit surprised that an IPv6 address was not parsed at a higher level
and take priority over an (possible) '[' drive letter, rather than being a
side-effect of calling an 'is_valid_path()' helper function. ;)
The 'is_valid_path()' was also part of the security fixes to check for some
'illegal' win32 paths which included those ending in spaces or periods or
for names such as AUX, COM1, LPTn, NUL, PRN, CONIN$, etc,. I know that some
of these paths are valid in cygwin (eg. those ending in spaces or periods)
so the win32 version of that function cannot be used without change. (this
could lead to one part of git allowing you to use said paths and other
parts not)! Note that some of the CVEs may actually still apply to cygwin.
I didn't check.
Also, note that I replaced the win32 version of the 'offset_1st_component()'
function. I can't quite remember why I did that, but I think the fact that
the win32 version doesn't ensure that the 'path' consists of both the <server>
and <share> component before returning the offset. (ie make sure that the
'path' consists of "//<server>/<share>/" at the very least and return one,
for '/', otherwise).
Personally, I would be quite happy to rip out all win32 path handling and
only support POSIX paths (I have been using cygwin since about 1996 and
have only ever used win32 paths when testing git ... that is the whole
point of cygwin! :) ), but I already know that that is a no-go. (there is
always somebody that complains when you suggest it).
So, for now anyway, it seems that I need to tidy up the patch and move in
the opposite direction to e.g. commit 1cadad6f65 ("git clone <url>
C:\cygwin\home\USER\repo' is working (again)", 2018-12-15).
Part of the reason for vacillating on the correct way forward with this
patch, was because I have often thought that I should use the cygwin API
to cater to both POSIX and win32 paths. For example, we could possibly use
the 'cygwin_conv_path()' function to do the path conversion (somewhat
similar to the macos pre-composed-utf8 stuff, minus the directory reading).
However, I think that would open a different can of worms, including some
potential memory leaks. So, not exactly a slam dunk.
[I also had a note-to-self about 'mixed / and \ urls' in the config file
which is exposed by these same tests. So, another patch may be needed?]
Anyway, something to think about. Hmm, I suspect it would be best to just
tidy up this patch first. ;)
Just FYI. Thanks!
ATB,
Ramsay Jones
----- >8 -----
Subject: [PATCH] cygwin: fix up IPv6 scp urls
---
connect.c | 2 ++
git-compat-util.h | 39 +++++++++++++++++++++++++++++++++++++++
t/t5500-fetch-pack.sh | 14 ++++----------
t/t5601-clone.sh | 11 ++---------
4 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/connect.c b/connect.c
index 47e39d2a73..6f5715e938 100644
--- a/connect.c
+++ b/connect.c
@@ -1088,10 +1088,12 @@ static enum url_scheme parse_connect_url(const char *url_orig, char **ret_host,
if (scheme == URL_SCHEME_LOCAL)
path = end;
+#ifdef DUMMY
else if (scheme == URL_SCHEME_FILE && *host != '/' &&
!has_dos_drive_prefix(host) &&
offset_1st_component(host - 2) > 1)
path = host - 2; /* include the leading "//" */
+#endif
else if (scheme == URL_SCHEME_FILE && has_dos_drive_prefix(end))
path = end; /* "file://$(pwd)" may be "file://C:/projects/repo" */
else
diff --git a/git-compat-util.h b/git-compat-util.h
index 8809776407..645ff96048 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -153,6 +153,45 @@ static inline int is_xplatform_dir_sep(int c)
#if defined(__CYGWIN__)
#include "compat/win32/path-utils.h"
+
+static inline int is_valid_cygwin_path(const char *path)
+{
+ if (path && strlen(path) > 2 &&
+ path[0] == '[' && path[1] == ':' &&
+ strchr(&path[2], ':'))
+ return 0;
+ return 1;
+}
+
+#define is_valid_path(path) is_valid_cygwin_path(path)
+
+static inline int cygwin_offset_1st_component(const char *path)
+{
+ int ret = 0;
+
+ /* C: prefix */
+ if ((ret = has_dos_drive_prefix(path)))
+ return ret;
+
+ /* //server/share/ prefix */
+ if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
+ char *pos = strpbrk(path + 2, "\\/");
+ if (pos && *(pos + 1) && !is_dir_sep(*(pos + 1))) {
+ pos = strpbrk(pos + 1, "\\/");
+ if (pos && *(pos + 1))
+ return pos + 1 - path;
+ }
+ }
+
+ /* / prefix */
+ if (is_dir_sep(path[0]))
+ return 1;
+
+ return 0;
+}
+
+#undef offset_1st_component
+#define offset_1st_component cygwin_offset_1st_component
#endif
#if defined(__MINGW32__)
/* pull in Windows compatibility stuff */
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 649a615ec9..2c6c0a04be 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -774,7 +774,7 @@ do
# file with scheme
for p in file
do
- test_expect_success !WINDOWS "fetch-pack --diag-url $p://$h/$r" '
+ test_expect_success !MINGW "fetch-pack --diag-url $p://$h/$r" '
check_prot_path $p://$h/$r $p "/$r"
'
test_expect_success MINGW "fetch-pack --diag-url $p://$h/$r" '
@@ -784,7 +784,7 @@ do
check_prot_path $p:///$r $p "/$r"
'
# No "/~" -> "~" conversion for file
- test_expect_success !WINDOWS "fetch-pack --diag-url $p://$h/~$r" '
+ test_expect_success !MINGW "fetch-pack --diag-url $p://$h/~$r" '
check_prot_path $p://$h/~$r $p "/~$r"
'
test_expect_success MINGW "fetch-pack --diag-url $p://$h/~$r" '
@@ -806,17 +806,11 @@ do
p=ssh
for h in host [::1]
do
- expectation="success"
- if test_have_prereq CYGWIN && test "$h" = "[::1]"
- then
- expectation="failure"
- fi
-
- test_expect_$expectation "fetch-pack --diag-url $h:$r" '
+ test_expect_success "fetch-pack --diag-url $h:$r" '
check_prot_host_port_path $h:$r $p "$h" NONE "$r"
'
# Do "/~" -> "~" conversion
- test_expect_$expectation "fetch-pack --diag-url $h:/~$r" '
+ test_expect_success "fetch-pack --diag-url $h:/~$r" '
check_prot_host_port_path $h:/~$r $p "$h" NONE "~$r"
'
done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 3dd229c186..35e9f3eb0d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -530,17 +530,10 @@ do
'
done
-# Parsing of paths that look like IPv6 addresses is broken on Cygwin.
-expectation_for_ipv6_tests=success
-if test_have_prereq CYGWIN
-then
- expectation_for_ipv6_tests=failure
-fi
-
#ipv6
for repo in rep rep/home/project 123
do
- test_expect_$expectation_for_ipv6_tests "clone [::1]:$repo" '
+ test_expect_success "clone [::1]:$repo" '
test_clone_url [::1]:$repo ::1 "$repo"
'
done
@@ -553,7 +546,7 @@ test_expect_success !SANITIZE_LEAK "clone host:/~repo" '
test_clone_url host:/~repo host "~repo"
'
-test_expect_$expectation_for_ipv6_tests !SANITIZE_LEAK "clone [::1]:/~repo" '
+test_expect_success !SANITIZE_LEAK "clone [::1]:/~repo" '
test_clone_url [::1]:/~repo ::1 "~repo"
'
--
2.55.0
^ permalink raw reply related
* [PATCH] Makefile: fix up lib directory move
From: Ramsay Jones @ 2026-07-10 18:38 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: GIT Mailing-list, Junio C Hamano
Commit 9759608622 ("Move libgit.a sources into separate "lib/" directory",
2026-06-22) moved some files into a lib directory, but forgot to update
a sparse dependency in the Makefile, resulting in a sparse error:
SP lib/pack-revindex.c
lib/pack-revindex.c:78:17: error: memset with byte count of 262144
make: *** [Makefile:3446: lib/pack-revindex.sp] Error 1
Add the missing 'lib/' prefix to the pack-revindex.sp path.
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Patrick,
If you need to re-roll your 'ps/libgit-in-subdir' branch, could you please squash
this into the relevant patch. (This patch was created directly on top of the 'seen'
branch, rather than on top of your branch).
Thanks
ATB,
Ramsay Jones
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 703772ba4f..a36d2c1942 100644
--- a/Makefile
+++ b/Makefile
@@ -2974,7 +2974,7 @@ lib/gettext.sp lib/gettext.s lib/gettext.o: EXTRA_CPPFLAGS = \
http-push.sp lib/http.sp lib/http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \
-DCURL_DISABLE_TYPECHECK
-pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
+lib/pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
ifdef NO_EXPAT
lib/http-walker.sp lib/http-walker.s lib/http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
--
2.55.0
^ permalink raw reply related
* Re: [PATCH v2] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Junio C Hamano @ 2026-07-10 18:51 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260710074105.50737-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> @@ -187,7 +186,6 @@ static int edit_patch(struct repository *repo,
> const char *prefix)
> {
> char *file = repo_git_path(repo, "ADD_EDIT.patch");
> - struct child_process child = CHILD_PROCESS_INIT;
> struct rev_info rev;
> int out;
> struct stat st;
> @@ -217,11 +215,17 @@ static int edit_patch(struct repository *repo,
> if (!st.st_size)
> die(_("empty patch. aborted"));
>
> - child.git_cmd = 1;
> - strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> - NULL);
> - if (run_command(&child))
> + struct apply_state state;
> + const char *apply_argv[] = { file, NULL };
These are -Wdeclaration-after-statement violations; we should move
them to the beginning of the function alongside the other variable
declarations.
> +
> + if (init_apply_state(&state, repo, prefix))
> + die(_("could not initialize apply state"));
> + state.cached = 1;
> + if (check_apply_state(&state, 0))
> + die(_("could not check apply state"));
> + if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
> die(_("could not apply '%s'"), file);
> + clear_apply_state(&state);
Does it work properly when run in a subdirectory, such as "cd t &&
git add -e")? The apply_all_patches() function adjusts the path to
patch files by calling prefix_filename() to prepend state->prefix,
which represents our current directory.
This is not a rhetorical question, as I am unsure what "file"
actually holds at this point after calling repo_git_path(). I don't
know if it is ADD_EDIT.patch relative to a specific directory, an
absolute path to the file, or something else entirely. It would be
highly beneficial to include a test or two verifying the behavour of
'add -e' from within a subdirectory.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH
From: brian m. carlson @ 2026-07-10 19:50 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Karthik Nayak, Jeff King,
Elijah Newren
In-Reply-To: <20260710085137.4171240-1-christian.couder@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]
On 2026-07-10 at 08:51:34, Christian Couder wrote:
> Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> 2024-04-16), lazy fetching has been controlled by the
> `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> nothing" boolean that is set to 'true' by default when calling `git
> upload-pack` for security reasons.
>
> Recently the "promisor-remote" capability was added to protocol v2,
> allowing servers and clients to agree on the promisor remotes they
> can safely use.
>
> This series leverages that capability to implement a pragmatic middle
> ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> fetching is allowed only when fetching from promisor remotes that are
> both advertised by the server and accepted by the client.
>
> Note that using an environment variable for this is probably not the
> best from a usability perspective. An `upload-pack.allowLazyFetch`
> configuration variable would likely be better.
>
> Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> things currently work. It would be a much bigger and more invasive
> change to implement `upload-pack.allowLazyFetch` in a way that is
> compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> backward compatibility. Therefore, transitioning to a configuration
> variable is left for future work.
I don't think this is a good idea. We get a lot of reports on the
security list involving various tooling that isn't within the scope of
our threat model. This substantially increases the amount of code which
is now subject to that threat model and therefore our security
guarantees and I don't think we should do that as it stands, very
especially while so much of our network-facing code is written in C.
The fetch code by default reads lots of configuration information from
the repository, including remote settings and information and we really
want absolutely none of that code running in the context of an untrusted
repository.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* [PATCH v3] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Gatla Vishweshwar Reddy @ 2026-07-10 19:58 UTC (permalink / raw)
To: git; +Cc: Gatla Vishweshwar Reddy
In-Reply-To: <xmqqechad6g9.fsf@gitster.g>
When the user runs "git add -e", the diff of the working tree changes
is written to a temporary file, opened in an editor, and then applied
back to the index. The application step is done by spawning a child
process running "git apply --recount --cached <file>", which is an
unnecessary subprocess since the apply machinery is available as a
native C API.
Replace the run_command() call with a direct call to apply_all_patches()
using an initialized apply_state with the cached and recount options set
appropriately. This avoids the overhead of forking a subprocess, keeps
the operation within the same process, and makes the intent of the code
clearer to the reader.
Remove the now-unused includes of "run-command.h" and "strvec.h" since
no other code in this file requires them after this change.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
Changes in v3:
- Moved struct apply_state and apply_argv declarations to the top of
the function to fix -Wdeclaration-after-statement violations
In response to review:
- repo_git_path() returns an absolute path built from gitdir.
prefix_filename() in apply_all_patches() explicitly skips absolute
paths (see abspath.c lines 271-272 where is_absolute_path(arg)
causes the prefix to be skipped). Running "git add -e" from a
subdirectory is therefore safe.
- A dedicated test for "git add -e" from a subdirectory would be
valuable. I looked but found no existing "add -e" tests in the test
suite to use as a reference. I would appreciate guidance on the
preferred approach, or I can attempt to write one if you can point
me to a similar test pattern.
builtin/add.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..1858adf289 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -13,7 +13,6 @@
#include "dir.h"
#include "gettext.h"
#include "pathspec.h"
-#include "run-command.h"
#include "object-file.h"
#include "odb.h"
#include "odb/transaction.h"
@@ -23,9 +22,9 @@
#include "diff.h"
#include "read-cache.h"
#include "revision.h"
-#include "strvec.h"
#include "submodule.h"
#include "add-interactive.h"
+#include "apply.h"
static const char * const builtin_add_usage[] = {
N_("git add [<options>] [--] <pathspec>..."),
@@ -187,7 +186,8 @@ static int edit_patch(struct repository *repo,
const char *prefix)
{
char *file = repo_git_path(repo, "ADD_EDIT.patch");
- struct child_process child = CHILD_PROCESS_INIT;
+ struct apply_state state;
+ const char *apply_argv[2];
struct rev_info rev;
int out;
struct stat st;
@@ -217,11 +217,16 @@ static int edit_patch(struct repository *repo,
if (!st.st_size)
die(_("empty patch. aborted"));
- child.git_cmd = 1;
- strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
- NULL);
- if (run_command(&child))
+ apply_argv[0] = file;
+ apply_argv[1] = NULL;
+ if (init_apply_state(&state, repo, prefix))
+ die(_("could not initialize apply state"));
+ state.cached = 1;
+ if (check_apply_state(&state, 0))
+ die(_("could not check apply state"));
+ if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
die(_("could not apply '%s'"), file);
+ clear_apply_state(&state);
unlink(file);
free(file);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 0/7] refs: remove use of `the_repository`
From: Junio C Hamano @ 2026-07-10 20:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <xmqqo6gedbq2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> It is more than probable that it was what happened. Will retry the
> merge during the integration run I'll make later today.
>
> Thanks.
And indeed, I had a mismerge.
Thanks, the topic is back in.
^ permalink raw reply
* Re: [PATCH v8 4/4] graph: indent visual root in graph
From: Pablo Sabater @ 2026-07-10 20:29 UTC (permalink / raw)
To: Mirko Faina
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31
In-Reply-To: <alEroo_DhFaWm3DH@exploit>
El vie, 10 jul 2026 a las 20:07, Mirko Faina (<mroik@delayed.space>) escribió:
>
> On Fri, Jul 10, 2026 at 12:37:07PM +0200, Pablo Sabater wrote:
> > When rendering a graph, if the history contains multiple "visual roots",
> > actual roots or commits that look like roots (i.e. have their parents
> > filtered out) can end up being vertically adjacent to unrelated commits,
> > falsely appearing to be related.
> >
> > A fix for this issue was already attempted [1] a while ago.
> >
> > This happens because the commits fill the space from left to right and
> > when a visual root ends, its column becomes free for the following
> > commit even if they are not related. Once this happens the unrelated
> > commit is rendered below the visual root. Because there is no special
> > character or way to identify when a visual root is rendered making the
> > graph confusing.
> >
> > By indenting the visual roots when there are still commits to show the
> > vertical adjacency can be avoided.
> >
> > Add is_visual_root flag to git_graph making it visible in all graph states,
> > give graph_update() a new function, graph_is_visual_root() to know if the
> > current commit is a visual root and set is_visual_root.
> > The different handled cases are:
> >
> > - If a visual root has children: similar to GRAPH_PRE_COMMIT state when
> > octopus merges need space, an edge row needs to be printed to connect
> > the child with the indented visual root. A new state GRAPH_PRE_ROOT is
> > needed to connect the child with the visual root:
> >
> > * child of the visual root
> > \ GRAPH_PRE_ROOT
> > * visual root indented
> >
> > - If a visual root is child-less we can skip GRAPH_PRE_ROOT state and
> > render the indented commit directly.
> >
> > * visual root indented
> > * unrelated commit
> >
> > - If two or more visual roots are adjacent: by having a lookahead to the
> > next commit that will be rendered, if the next commit is also a visual
> > root and we are on a visual root, meaning two visual root adjacent in
> > the history, the top one can omit the indent, making the one below to
> > indent only once, if there are more adjacent visual commits, the
> > indentation will increase for each adjacent one, cascading.
> >
> > * visual root
> > * visual root
> > * visual root
> > * last commit
> >
> > Even if the last commit is a root, because there is nothing that will be
> > rendered below we can omit the indentation on purpose.
> >
> > [1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
> >
> > Helped-by: Kristofer Karlsson <krka@spotify.com>
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> > ---
> > graph.c | 235 +++++++++++++++++++
> > t/meson.build | 1 +
> > t/t4218-log-graph-indentation.sh | 473 +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 709 insertions(+)
>
> This doesn't seem to work for every visual root e.g.
>
> git log --graph --oneline --author="Mirko Faina"
>
> The visual roots are not indented.
>
> > +/*
> > + * A commit can be a visual root when:
> > + *
> > + * - It has no parents.
> > + *
> > + * - It has parents but they are all filtered out and
> > + * commit->parents arrives NULL.
> > + *
> > + * - It is not a boundary commit. Boundary commits also have no visible
> > + * parents, but they are not selected as visual roots because they cannot
> > + * cause the ambiguity of being vertically adjacent because:
> > + *
> > + * 1. A boundary only appears because an included commit is its child.
> > + * Children are always above, and the renderer draws an edge down to
> > + * the boundary from that child. Rather than starting a column like a
> > + * visual root would do, it inherits its child column.
> > + *
> > + * 2. Included commits cannot appear below a boundary. Boundaries are
> > + * ancestors of the exclusion point; if an included commit were an
> > + * ancestor of the boundary it would be excluded and not rendered.
> > + * Boundaries therefore always sink to the bottom.
> > + */
> > +static int graph_is_visual_root_candidate(struct commit *c)
> > +{
> > + return c->parents == NULL && !(c->object.flags & BOUNDARY);
> > +}
>
> I suspect this behaviour is due to these assumptions being too strict.
>
> When we use the --author option the parents are not filtered out, so it
> doesn't return NULL desipte being a visual root. We realize it is a
> visual root only on the next commit, but once we are on the next commit
> we can't indent as we have already printed this commit.
>
> We realize only on the next commit after hitting simplify_commit(), it
> calls get_commit_action() and checks if should keep the commit based on
> the regex we provided. If the regex is not matched the commit is just
> ignored (we do not filter parents based on regex when we expand a topo
> walk).
>
> At least that's what I gather, if anyone can confirm this...
Hi!
Yes, I just tried with the same:
git log --graph --oneline --author="Mirko Faina"
And no indentation sadly, if when we use --author the parents are not
excluded then the c->parents is not enough.
I think that it should be fine if we iterate each parent and call
graph_is_interesting() that also calls get_commit_action() as a
fallback.
Something like:
graph_is_visual_root_candidate():
/* We keep ignoring boundary commits */
if (c->object.flags & BOUNDARY)
return 0;
/* Check the parents if they are not excluded because of options like
--author */
for (p = c->parents; p; p = p->next)
if(graph_is_interesting(graph, p->item))
return 0;
return 1;
I haven't tried yet though, but if --author has this problem, probably
other options like --grep would likely fail too because of the same
reason.
Thanks for the feedback,
Pablo
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Taylor Blau @ 2026-07-10 22:09 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: Taylor Blau, Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <CAL71e4PuD9D8LRbP3mfxxeMrM+1q--3sCp6oJs=hezdasZUPMw@mail.gmail.com>
On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:
> > Instead of writing "# BUG ..." and then an incorrect assertion, I
> > would suggest that you write the assertion you expect:
> >
> > test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
> >
> > , but mark the test as "test_expect_failure".
>
> I started with this actually and then changed my mind in order
> to demonstrate exactly how the counter changed, not just that it
> changed from failure to success. But I'd be happy to change this
> too if needed - it would effectively reduce the second commit to
> just the bugfix line and switching from test_expect_failure
> to test_expect_success.
Yeah, I think this would be ideal.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers
From: Taylor Blau @ 2026-07-10 22:14 UTC (permalink / raw)
To: Kristofer Karlsson, '
Cc: Taylor Blau, Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <CAL71e4OuU1+KHd0TrcxDX2dyoWEJXmi86m8u+E7vtxhcSF6M1Q@mail.gmail.com>
On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:
> (b) Move topo_levels to struct object_database. Since
> fill_commit_graph_info() can already reach the odb via
> g->odb_source->odb, no signature changes are needed.
> The write side becomes a single assignment:
>
> ctx.r->objects->topo_levels = &topo_levels;
>
> and cleanup becomes:
>
> ctx.r->objects->topo_levels = NULL;
>
> No chain walk needed and the diff is fairly small.
> I am not sure about the semantics of it though -- should the odb
> have a reference to topo_levels?
This seems to be the most promising approach, though I'd be curious what
Patrick's thoughts are. The commit-slab API is really a property of the
object database, but we treat these as a global as I do not recall them
yet being touched by the ODB refactoring effort.
> [...]
>
> I have a prototype of (b) that compiles and passes the test suite.
>
> For now though, I think the minimal bugfix is the right thing to do.
Agreed.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 0/2] commit-graph: fix topo_levels slab propagation regression
From: Taylor Blau @ 2026-07-10 22:15 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Taylor Blau, Kristofer Karlsson, Patrick Steinhardt
In-Reply-To: <pull.2170.v2.git.1783609382.gitgitgadget@gmail.com>
On Thu, Jul 09, 2026 at 03:02:59PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> Changes since v1:
>
> * Fixed wrong commit title and date in the reference (Junio, Taylor).
> * use test_expect_failure with the correct assertion instead of a # BUG
> comment (Taylor).
> * Simplified commit messages.
>
> Kristofer Karlsson (2):
> commit-graph: add trace2 instrumentation for generation DFS
> commit-graph: propagate topo_levels slab to all chain layers
>
> commit-graph.c | 7 ++++++-
> t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
> 2 files changed, 30 insertions(+), 1 deletion(-)
Thanks, this version looks good to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Junio C Hamano @ 2026-07-10 22:21 UTC (permalink / raw)
To: Paulius Zaleckas
Cc: git, Glen Choo, Ævar Arnfjörð Bjarmason,
Patrick Steinhardt
In-Reply-To: <20260710122655.3066377-3-paulius.zaleckas@gmail.com>
Paulius Zaleckas <paulius.zaleckas@gmail.com> writes:
> When fetching with --recurse-submodules, a submodule commit that is not
> yet reachable from any of the submodule's remote refs causes the entire
> fetch to fail. This is overly strict when the missing commit belongs to
> an upstream branch that is still being prepared (e.g. an in-progress
> merge topic): the local branch does not need that commit, so there is no
> reason to treat its absence as fatal.
>
> Add a new config key fetch.submoduleErrors (values: fail/warn) and a
> corresponding --submodule-errors=(fail|warn) command-line option that
> control this behaviour. The default remains fail (existing behaviour);
> setting the value to warn causes submodule fetch failures to be reported
> on stderr without affecting the overall exit status of git fetch / git
> pull.
>
> Forward the option to child fetches in add_options_to_argv() so that it
> also takes effect for `git fetch --all` / `--multiple` (where per-remote
> child processes handle the submodule recursion themselves) and for
> nested submodule recursion.
>
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
> ---
> Documentation/config/fetch.adoc | 14 ++++++
> Documentation/fetch-options.adoc | 8 ++++
> builtin/fetch.c | 41 ++++++++++++++++-
> submodule.c | 8 +++-
> submodule.h | 7 ++-
> t/t5526-fetch-submodules.sh | 76 ++++++++++++++++++++++++++++++++
> 6 files changed, 150 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
> index 04ac90912d..5c9c942a70 100644
> --- a/Documentation/config/fetch.adoc
> +++ b/Documentation/config/fetch.adoc
> @@ -10,6 +10,20 @@
> reference.
> Defaults to `on-demand`, or to the value of `submodule.recurse` if set.
>
> +`fetch.submoduleErrors`::
> + Controls how errors from submodule fetches are handled when
> + `--recurse-submodules` is in effect. When set to `fail` (the default),
> + any submodule fetch error causes the overall `git fetch` or `git pull`
> + to exit with a non-zero status. When set to `warn`, submodule fetch
> + errors are reported to standard error but do not affect the exit
> + status of the command. This is useful when working in repositories
> + where some branches reference submodule commits that are not yet
> + available on the submodule remote, but those commits are not needed
> + for the currently checked-out branch.
> ++
> +The value of this option can be overridden by the `--submodule-errors`
> +option of linkgit:git-fetch[1].
> +
> `fetch.fsckObjects`::
> If it is set to true, git-fetch-pack will check all fetched
> objects. See `transfer.fsckObjects` for what's
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 035f780e58..78525f6848 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -294,6 +294,14 @@ ifndef::git-pull[]
> `--no-recurse-submodules`::
> Disable recursive fetching of submodules (this has the same effect as
> using the `--recurse-submodules=no` option).
> +
> +`--submodule-errors=(fail|warn)`::
> + Control how errors from submodule fetches are handled when
> + `--recurse-submodules` is in effect. When set to `fail` (the default),
> + any submodule fetch error causes the overall `git fetch` to exit with a
> + non-zero status. When set to `warn`, submodule fetch errors are reported
> + to standard error but do not affect the exit status of the command. Can
> + also be configured via `fetch.submoduleErrors`. See linkgit:git-config[1].
> endif::git-pull[]
>
> `--set-upstream`::
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c1d7c672f4..40daaf5cc7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -110,6 +110,7 @@ struct fetch_config {
> int recurse_submodules;
> int parallel;
> int submodule_fetch_jobs;
> + int submodule_errors;
> };
>
> static int git_fetch_config(const char *k, const char *v,
> @@ -152,6 +153,19 @@ static int git_fetch_config(const char *k, const char *v,
> return 0;
> }
>
> + if (!strcmp(k, "fetch.submoduleerrors")) {
> + if (!v)
> + return config_error_nonbool(k);
> + else if (!strcasecmp(v, "fail"))
> + fetch_config->submodule_errors = SUBMODULE_ERRORS_FAIL;
> + else if (!strcasecmp(v, "warn"))
> + fetch_config->submodule_errors = SUBMODULE_ERRORS_WARN;
> + else
> + die(_("invalid value for '%s': '%s'"),
> + "fetch.submoduleErrors", v);
> + return 0;
> + }
> +
> if (!strcmp(k, "fetch.parallel")) {
> fetch_config->parallel = git_config_int(k, v, ctx->kvi);
> if (fetch_config->parallel < 0)
> @@ -2205,6 +2219,8 @@ static void add_options_to_argv(struct strvec *argv,
> strvec_push(argv, "--no-recurse-submodules");
> else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
> strvec_push(argv, "--recurse-submodules=on-demand");
> + if (config->submodule_errors == SUBMODULE_ERRORS_WARN)
> + strvec_push(argv, "--submodule-errors=warn");
> if (tags == TAGS_SET)
> strvec_push(argv, "--tags");
> else if (tags == TAGS_UNSET)
If (config->submodule_errors != SUBMODULE_ERRORS_WARN), then the argv[]
would not see any --submodule-errors=<anything> to propagate down.
Specifically, this function is called when recurse-submodules is not
disabled, and prepares argv[] used to call fetch_submodules().
> int cmd_fetch(int argc,
> const char **argv,
> const char *prefix,
> @@ -2477,6 +2506,7 @@ int cmd_fetch(int argc,
> .recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
> .parallel = 1,
> .submodule_fetch_jobs = -1,
> + .submodule_errors = SUBMODULE_ERRORS_FAIL,
> };
Here, .submodule_errors member is initialized to
SUBMODULE_ERRORS_FAIL (i.e. 0).
> @@ -2491,6 +2521,7 @@ int cmd_fetch(int argc,
> int max_jobs = -1;
> int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
> int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
> + int submodule_errors_cli = -1; /* -1: not set on command line */
> int fetch_write_commit_graph = -1;
> int stdin_refspecs = 0;
> int negotiate_only = 0;
> @@ -2527,6 +2558,10 @@ int cmd_fetch(int argc,
> OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
> N_("control recursive fetching of submodules"),
> PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
> + OPT_CALLBACK_F(0, "submodule-errors", &submodule_errors_cli,
> + N_("(fail|warn)"),
> + N_("control how submodule fetch errors are handled"),
> + 0, option_parse_submodule_errors),
And command line option "--submodule-errors={warn,fail}" may update
the local variable submodule_errors_cli (initialied to -1) to one of
SUBMODULE_ERRORS_{WARN,FAIL}. These are different from -1, so we
can reliably tell if we saw a command line override, which is good.
> OPT_BOOL(0, "dry-run", &dry_run,
> N_("dry run")),
> OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
> @@ -2616,6 +2651,9 @@ int cmd_fetch(int argc,
> if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
> config.recurse_submodules = recurse_submodules_cli;
>
> + if (submodule_errors_cli != -1)
> + config.submodule_errors = submodule_errors_cli;
And we override what we read from the configuration if we got a
command line override.
And the value in config.submodule_errors is used much later, in a
call to add_options_to_argv() we saw earlier, but this patch does
not touch the caller so we do not see the calling site.
I do not do submodules, so my expectation here may be a bit skewed,
but what happens when we configure fetch.submoduleErrors to warn,
but override it from the command line to fail? .submodule_errors is
set to SUBMODULE_ERRORS_FAIL here? As we saw, add_options_to_argv()
stuff --submodule-error=<setting> only when config.submodule_errors
is set to SUBMODULE_ERRORS_WARN, so we do not pass command line
override. Is this desirable? Don't we want to pass down not just
--submodule-error=warn but --submodule-error=fail if that is what
was given from the command line? Or does it not matter because fail
is the default?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects
From: Taylor Blau @ 2026-07-10 22:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-1-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> Fix the issue by using `packed_object_info()` directly.
What you wrote here makes sense to me insofar as I understand the
pluggable ODB code.
However, I am confused by the way this function is written in general.
We use `bsearch_one_midx()` to locate the first possible MIDX position
in which an object matching the given prefix may exist, which is
sensible. However, we go from that position up to "num", where "num" is
the total number of objects in the MIDX!
Functionally this is not incorrect as we will happily discard objects
that do not match the prefix. But it causes us to waste CPU cycles
repeatedly calling `match_hash()` (at least for the first byte of the
prefix) for objects that we know will match.
How often do we call this function with a prefix longer than a
single byte? I have no idea, but I would suspect that it makes up the
majority of calls. If we read the OID fanout chunk, we could narrow the
range that we enumerate through, and only compare the second byte
onwards of the given prefix, if one exists. In the single-byte prefix
case, this means that we shouldn't have to do any memory comparisons at
all.
> While at it, rename the `store` variable to `source`.
Unrelated, but please keep these to a minimum, as they make the patch
more difficult to read than is necessary.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 2/8] pack-bitmap: mark object filter as `const`
From: Taylor Blau @ 2026-07-10 22:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-2-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:54AM +0200, Patrick Steinhardt wrote:
> The function `for_each_bitmapped_object()` accepts an optional object
> filter. This filter is never modified by the function, but is not
> declared as `const`. Fix this.
Makes sense. "Fix" this seems to imply that the existing behavior was
broken or otherwise incorrect, but I think this is fine.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> pack-bitmap.c | 6 +++---
> pack-bitmap.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Junio C Hamano @ 2026-07-10 22:28 UTC (permalink / raw)
To: Taylor Blau
Cc: Kristofer Karlsson, Taylor Blau,
Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <alFthqGQjsowvpEz@com-79390>
Taylor Blau <ttaylorr@openai.com> writes:
> On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:
>> > Instead of writing "# BUG ..." and then an incorrect assertion, I
>> > would suggest that you write the assertion you expect:
>> >
>> > test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
>> >
>> > , but mark the test as "test_expect_failure".
>>
>> I started with this actually and then changed my mind in order
>> to demonstrate exactly how the counter changed, not just that it
>> changed from failure to success. But I'd be happy to change this
>> too if needed - it would effectively reduce the second commit to
>> just the bugfix line and switching from test_expect_failure
>> to test_expect_success.
>
> Yeah, I think this would be ideal.
If the test involved is longer than 3 lines, I would recommend
against it, as "git show" of such a patch will show the full code
change to implement a different behaviour plus "_failure" changing
to "_success" in the test, with the body of the test hidden outside
the context, which makes it hard to guess what the behaviour change
is really about.
^ permalink raw reply
* Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects
From: Taylor Blau @ 2026-07-10 22:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-3-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:55AM +0200, Patrick Steinhardt wrote:
> In a subsequent commit we'll lift iteration of bitmapped objects into
> the "packed" backend and make it accessible via `odb_for_each_object()`.
> The calling convention for that function is that the callback may return
> a non-zero exit code, and if so we'll abort iteration. This is currently
> impossible to realize though, as `for_each_bitmapped_object()` will
> ignore any return value and just churn through all objects completely.
>
> This doesn't matter to the callers of `for_each_bitmapped_object()`, as
> there's only one of them in git-cat-file(1), and the callbacks we pass
> always return zero. But once we move the logic into the generic
> infrastructure it becomes a latent bug waiting to happen.
Makes sense.
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index ea5eab4cf8..8ff92c5272 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
> return 0;
>
> create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
> - return 1;
> + return 0;
> }
I was initially rather surprised to read this diff. I suspected that
this was a "we used to return non-zero to indicate success but now
return zero to match the project conventions", but was stumped by the
unchanged "return 0" in the context above.
But I suppose that is demonstrating the thing that you're trying to fix
here, which is that the caller doesn't actually care what is returned
from the callback, so the change here (and analogous ones below) make
sense to me.
> -static void show_objects_for_type(
> +static int show_objects_for_type(
> struct bitmap_index *bitmap_git,
> struct bitmap *objects,
> enum object_type object_type,
> @@ -1704,6 +1704,7 @@ static void show_objects_for_type(
> {
> size_t i = 0;
> uint32_t offset;
> + int ret;
This has a broader scope than is strictly necessary, but I think that is
OK.
> static int in_bitmapped_pack(struct bitmap_index *bitmap_git,
> @@ -2062,6 +2069,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
> show_reachable_fn show_reach,
> void *payload)
> {
> + const enum object_type types[] = {
> + OBJ_COMMIT,
> + OBJ_TREE,
> + OBJ_BLOB,
> + OBJ_TAG,
> + };
> struct bitmap *filtered_bitmap = NULL;
> uint32_t objects_nr;
> size_t full_word_count;
> @@ -2086,14 +2099,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
> goto out;
> }
>
> - show_objects_for_type(bitmap_git, filtered_bitmap,
> - OBJ_COMMIT, show_reach, payload);
> - show_objects_for_type(bitmap_git, filtered_bitmap,
> - OBJ_TREE, show_reach, payload);
> - show_objects_for_type(bitmap_git, filtered_bitmap,
> - OBJ_BLOB, show_reach, payload);
> - show_objects_for_type(bitmap_git, filtered_bitmap,
> - OBJ_TAG, show_reach, payload);
> + for (size_t i = 0; i < ARRAY_SIZE(types); i++) {
> + ret = show_objects_for_type(bitmap_git, filtered_bitmap,
> + types[i], show_reach, payload);
> + if (ret)
> + goto out;
> + }
OK. So now we call this function in a loop instead of the unrolled
version, presumably because we want to propagate a failure from any one
of these before falling through to the remaining object types.
That makes sense, and I think the clean-up is well justified here.
However, the remaining `show_objects_for_type()` callers from within
`traverse_bitmap_commit_list()` do *not* bother to inspect the return
value, despite taking in an arbitrary 'show_reachable_fn', which itself
may return a non-zero value.
I guess this must be effectively OK in practice with respect to the
existing code for the same reason you indicate in the commit message
above, but we should change this function to *also* propagate non-zero
return values to eliminate the foot-gun completely.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 4/8] pack-bitmap: iterate object sources when opening bitmaps
From: Taylor Blau @ 2026-07-10 22:40 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-4-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:56AM +0200, Patrick Steinhardt wrote:
> When opening a bitmap for a repository we perform two steps:
>
> - We first look for a multi-pack index bitmap in any of the object
> sources connected to the repository.
>
> - We then look for a packfile bitmap in any of the packfiles of any of
> the object sources.
>
> Both of these steps thus iterate through object sources themselves, one
> via `odb_prepare_alternates()` and one via `repo_for_each_pack()`. This
> layout makes it hard to introduce a way to open the bitmap of one
> specific object source, which is functionality that we'll require in a
> subsequent commit.
>
> Reverse the loop so that we instead loop through all sources in the
> outer loop, and then for each source we try to load its bitmap via
> either the multi-pack index or via a packfile.
This makes sense. An individual object store should be considered to
have a bitmap in the abstract sense if it provides either a multi-pack
bitmap (or an incremental multi-pack bitmap ), or a single-pack
bitmap.
> Note that this changes the precedence of bitmaps in one specific edge
> case: when an earlier object source only has a packfile bitmap, but a
> later source has a multi-pack index bitmap, we now pick the packfile
> bitmap of the earlier source. Previously, a multi-pack index bitmap from
> any source would have taken precedence over all packfile bitmaps. Given
> that object sources are ordered such that the local source comes first,
> this arguably is an improvement, as we now prefer local bitmaps over
> bitmaps in alternates. Furthermore, we already warn about repositories
> that have multiple bitmaps, so this setup is broken and thus arguably
> not worth worrying about too much.
Yeah, I think the existing behavior should be considered broken, so I
think that this behavior change is a positive one.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 5/8] pack-bitmap: drop `_1` suffix from functions that open bitmaps
From: Taylor Blau @ 2026-07-10 22:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-5-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:57AM +0200, Patrick Steinhardt wrote:
> In the preceding commit we've refactored how we open bitmaps. As part of
> the refactoring we have consolidated `open_pack_bitmap()` as well as
> `open_midx_bitmap()` into `open_bitmap_for_source()`. Consequently, we
> only have their `open_pack_bitmap_1()` and `open_midx_bitmap_1()`
> variants left over, where the `_1` suffix doesn't really make much sense
> anymore.
>
> Drop the suffix.
Makes sense. Thanks for keeping this in a separate commit.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 7/8] odb: introduce object filters to `odb_for_each_object()`
From: Taylor Blau @ 2026-07-10 22:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-7-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:59AM +0200, Patrick Steinhardt wrote:
> ---
> odb.h | 12 +++++++++++
> odb/source-packed.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> pack-bitmap.c | 3 +--
> pack-bitmap.h | 3 +++
> 4 files changed, 78 insertions(+), 2 deletions(-)
This all looks about as expected to me. As mentioned earlier in this
thread, I am not as familiar with the pluggable ODB code as I'd like to
be, but the patch looks plausibly correct to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Taylor Blau @ 2026-07-10 22:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kristofer Karlsson, Taylor Blau,
Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <xmqqik6mbhtw.fsf@gitster.g>
On Fri, Jul 10, 2026 at 03:28:11PM -0700, Junio C Hamano wrote:
> Taylor Blau <ttaylorr@openai.com> writes:
>
> > On Tue, Jul 07, 2026 at 04:08:36PM +0200, Kristofer Karlsson wrote:
> >> > Instead of writing "# BUG ..." and then an incorrect assertion, I
> >> > would suggest that you write the assertion you expect:
> >> >
> >> > test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
> >> >
> >> > , but mark the test as "test_expect_failure".
> >>
> >> I started with this actually and then changed my mind in order
> >> to demonstrate exactly how the counter changed, not just that it
> >> changed from failure to success. But I'd be happy to change this
> >> too if needed - it would effectively reduce the second commit to
> >> just the bugfix line and switching from test_expect_failure
> >> to test_expect_success.
> >
> > Yeah, I think this would be ideal.
>
> If the test involved is longer than 3 lines, I would recommend
> against it, as "git show" of such a patch will show the full code
> change to implement a different behaviour plus "_failure" changing
> to "_success" in the test, with the body of the test hidden outside
> the context, which makes it hard to guess what the behaviour change
> is really about.
Hmm, I am not sure that I agree. Or, at the very least, that is now how
I have written series in the past where I want to demonstrate and then
subsequently fix an existing bug.
When either the test setup or the bugfix is trivial, I think having it
in the same commit is just fine. But I think there are two good reasons
for splitting it out if the test or bug is complex:
- If the test is complex, but the complexity is not directly related to
the bugfix, having to explain both in the same commit message can be
awkward, and makes it harder for a reviewer to reason about either
component of the patch.
- If the bugfix is complex, having the failing test in a separate
commit demonstrates that the bug existed before, but is definitively
fixed in the following commit, as both would be expected to 'make
test' cleanly.
I am happy to change my style if you feel strongly. It would be nice to
document this in CodingGuidelines (or SubmittingPatches?) if it is not
already.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v3] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Junio C Hamano @ 2026-07-11 4:51 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260710195949.54928-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> In response to review:
> - repo_git_path() returns an absolute path built from gitdir.
> prefix_filename() in apply_all_patches() explicitly skips absolute
> paths (see abspath.c lines 271-272 where is_absolute_path(arg)
> causes the prefix to be skipped). Running "git add -e" from a
> subdirectory is therefore safe.
I agree that we are safe when it is absolute (no room for prefix to
take part); my question was more about repo_git_path() that derives
its value from repo->gitdir which may or may not be absolute.
Does it always give you absolute, or sometimes it is relative and
sometimes it is absolute?
> - A dedicated test for "git add -e" from a subdirectory would be
> valuable. I looked but found no existing "add -e" tests in the test
> suite to use as a reference.
"git grep -e 'add -e' t/" finds t3702.
^ permalink raw reply
* [PATCH v4] builtin/add.c: replace run_command() with direct apply_all_patches() call
From: Gatla Vishweshwar Reddy @ 2026-07-11 6:06 UTC (permalink / raw)
To: git; +Cc: Gatla Vishweshwar Reddy
In-Reply-To: <xmqqechab03t.fsf@gitster.g>
When the user runs "git add -e", the diff of the working tree changes
is written to a temporary file, opened in an editor, and then applied
back to the index. The application step is done by spawning a child
process running "git apply --recount --cached <file>", which is an
unnecessary subprocess since the apply machinery is available as a
native C API.
Replace the run_command() call with a direct call to apply_all_patches()
using an initialized apply_state with the cached and recount options set
appropriately. This avoids the overhead of forking a subprocess, keeps
the operation within the same process, and makes the intent of the code
clearer to the reader.
Remove the now-unused includes of "run-command.h" and "strvec.h" since
no other code in this file requires them after this change.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
---
Changes in v4:
- Pass NULL instead of prefix to init_apply_state() since the file
path from repo_git_path() is a git-internal path that should not
be prefixed. This is safe regardless of whether repo->gitdir is
absolute or relative, as prefix_filename(NULL, arg) returns the
path unchanged (abspath.c line 269).
- Add a test in t3702-add-edit.sh verifying that "git add -e" works
correctly when run from a subdirectory.
- Tested with t3702-add-edit.sh: all 4 tests pass.
In response to review:
- You are right that repo->gitdir may not always be absolute
(setup.c line 1109). Passing NULL as prefix to init_apply_state()
avoids the issue entirely — prefix_filename(NULL, arg) sets
pfx_len=0 and returns the path unchanged regardless of whether
it is absolute or relative.
- t3702-add-edit.sh was found via "git grep -e 'add -e' t/" as
suggested. A new test using GIT_EDITOR=cat verifies that
"git add -e" works correctly from a subdirectory.
builtin/add.c | 19 ++++++++++++-------
t/t3702-add-edit.sh | 10 ++++++++++
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..20a86a1611 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -13,7 +13,6 @@
#include "dir.h"
#include "gettext.h"
#include "pathspec.h"
-#include "run-command.h"
#include "object-file.h"
#include "odb.h"
#include "odb/transaction.h"
@@ -23,9 +22,9 @@
#include "diff.h"
#include "read-cache.h"
#include "revision.h"
-#include "strvec.h"
#include "submodule.h"
#include "add-interactive.h"
+#include "apply.h"
static const char * const builtin_add_usage[] = {
N_("git add [<options>] [--] <pathspec>..."),
@@ -187,7 +186,8 @@ static int edit_patch(struct repository *repo,
const char *prefix)
{
char *file = repo_git_path(repo, "ADD_EDIT.patch");
- struct child_process child = CHILD_PROCESS_INIT;
+ struct apply_state state;
+ const char *apply_argv[2];
struct rev_info rev;
int out;
struct stat st;
@@ -217,11 +217,16 @@ static int edit_patch(struct repository *repo,
if (!st.st_size)
die(_("empty patch. aborted"));
- child.git_cmd = 1;
- strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
- NULL);
- if (run_command(&child))
+ apply_argv[0] = file;
+ apply_argv[1] = NULL;
+ if (init_apply_state(&state, repo, NULL))
+ die(_("could not initialize apply state"));
+ state.cached = 1;
+ if (check_apply_state(&state, 0))
+ die(_("could not check apply state"));
+ if (apply_all_patches(&state, 1, apply_argv, APPLY_OPT_RECOUNT))
die(_("could not apply '%s'"), file);
+ clear_apply_state(&state);
unlink(file);
free(file);
diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh
index 8bacacbac6..f628564005 100755
--- a/t/t3702-add-edit.sh
+++ b/t/t3702-add-edit.sh
@@ -124,5 +124,15 @@ test_expect_success 'add -e notices editor failure' '
test_must_fail env GIT_EDITOR=false git add -e &&
test_expect_code 1 git diff --exit-code
'
+test_expect_success 'add -e works from a subdirectory' '
+ git reset --hard &&
+ echo change >>file &&
+ mkdir -p subdir &&
+ (
+ cd subdir &&
+ GIT_EDITOR=cat git add -e ../file
+ ) &&
+ git diff --cached | grep -q "^+change"
+'
test_done
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] object-file: fix closing object stream twice
From: Jeff King @ 2026-07-11 7:33 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, xuqing yang, Toon Claes
In-Reply-To: <20260710-pks-odb-stream-double-close-v1-1-d5fa233a37c7@pks.im>
On Fri, Jul 10, 2026 at 04:54:16PM +0200, Patrick Steinhardt wrote:
> And while the mentioned commit did drop one call that closed the stream,
> there's a second such call that was missed when reading from the stream
> fails. The consequence of this can be a double free of the stream.
>
> Fix the bug by dropping that leftover call to `odb_read_stream_close()`.
Thanks, both the patch and the new test look good to me.
> Note that it was originally discussed whether this should be treated as
> a security vulnerability. But there are only two callers: once via
> `parse_object_with_flags()`, and once via `verify_packfile()`. Neither
> of these callers plays any role on the transport layer, so this issue is
> only relevant for objects that are already available via the local
> object database. Furthermore, a packfile that is corrupted in this way
> would be detected when receiving the packfile, so it's not easy for an
> adversary to plant such a packfile, either. Consequently, we decided
> that this is not covered as part of our threat model.
I think this case probably would violate our "it is OK to clone from the
local untrusted .git repo" goal (since you could perhaps get to this
code path via upload-pack/pack-objects, though I didn't try it myself).
But the text in git(1)'s SECURITY section is pretty clear that it is
more goal than promise, and that this scenario carries extra risk
exactly because of the increased attack surface. And that you can
mitigate by serving from an untrusted user.
-Peff
^ permalink raw reply
* Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects
From: Jeff King @ 2026-07-11 7:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Justin Tobler, git
In-Reply-To: <alCafO91ZtFdikPg@pks.im>
On Fri, Jul 10, 2026 at 09:08:44AM +0200, Patrick Steinhardt wrote:
> On Thu, Jul 09, 2026 at 03:19:52PM -0500, Justin Tobler wrote:
> > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > > index ea5eab4cf8..8ff92c5272 100644
> > > --- a/builtin/pack-objects.c
> > > +++ b/builtin/pack-objects.c
> > > @@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
> > > return 0;
> > >
> > > create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
> > > - return 1;
> > > + return 0;
> >
> > I wonder why this was even returning 1 to begin with? As you mentioned,
> > the return value appears to be ignored anyways. I'm assuming it was
> > signal that an object entry was created?
>
> The function is only called from a single location, and the return value
> was completely ignored until this commit. It has always been this way
> since the function was originally introduced in 6b8fda2db1
> (pack-objects: use bitmaps when packing objects, 2013-12-21), so it
> never seemed to have any purpose. The commit message doesn't mention
> anything either.
I think it was copying the semantics of its non-bitmap counterpart,
add_object_entry(). Of course nobody looks at that return value either!
Long ago there were callers that cared about whether we actually created
an entry, but I think the last one went away in 5379a5c5ee (Thin pack
generation: optimization., 2006-04-05), which was quite some time ago.
So I think we could probably drop the return value from
add_object_entry() entirely (but of course we can't do the same for the
bitmap variant, because of its use as a callback).
I mention this mostly as answering Justin's "I wonder why...", but it
might be worth cleaning up add_object_entry() here, as its return value
semantics have diverged from add_object_entry_from_bitmap().
-Peff
^ permalink raw reply
* [PATCH v2 9/8?] pack-objects: drop unused return value from add_object_entry()
From: Jeff King @ 2026-07-11 7:58 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im>
On Fri, Jul 10, 2026 at 10:48:52AM +0200, Patrick Steinhardt wrote:
> The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
> 2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
> info fields, 2026-07-02) merged into it.
Here's a patch doing the cleanup I proposed upthread.
-- >8 --
Subject: pack-objects: drop unused return value from add_object_entry()
This function returns 0/1 to its caller to tell them whether we actually
added a new entry (or if we considered it redundant). But nobody has
relied on that behavior since 5379a5c5ee (Thin pack generation:
optimization., 2006-04-05).
The extra return does not hurt much, but it recently became a bit more
confusing. We have a sister function, add_object_entry_from_bitmap(),
which had the same return value semantics. That function recently
changed to always return 0 (not void, because it must conform to a
callback function interface). So now we have two related functions which
both return an "int" but with different semantics.
Let's drop the unused "int" return from add_object_entry() entirely,
which makes it more clear that the two functions have diverged.
Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't reference the commit by its id, since Junio has not yet
picked up the v2 sent a few hours ago. ;)
builtin/pack-objects.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8ff92c5272..3673b14b89 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1867,16 +1867,16 @@ static const char no_closure_warning[] = N_(
"disabling bitmap writing, as some objects are not being packed"
);
-static int add_object_entry(const struct object_id *oid, enum object_type type,
- const char *name, int exclude)
+static void add_object_entry(const struct object_id *oid, enum object_type type,
+ const char *name, int exclude)
{
struct packed_git *found_pack = NULL;
off_t found_offset = 0;
display_progress(progress_state, ++nr_seen);
if (have_duplicate_entry(oid, exclude))
- return 0;
+ return;
if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
/* The pack is missing an object, so it will not have closure */
@@ -1885,13 +1885,12 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
warning(_(no_closure_warning));
write_bitmap_index = 0;
}
- return 0;
+ return;
}
create_object_entry(oid, type, pack_name_hash_fn(name),
exclude, name && no_try_delta(name),
found_pack, found_offset);
- return 1;
}
static int add_object_entry_from_bitmap(const struct object_id *oid,
--
2.55.0.580.gbbcb530e9e
^ permalink raw reply related
* Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects
From: Jeff King @ 2026-07-11 8:01 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, git, Justin Tobler, Junio C Hamano
In-Reply-To: <alFzja98avOoKjQE@com-79390>
On Fri, Jul 10, 2026 at 03:34:53PM -0700, Taylor Blau wrote:
> However, the remaining `show_objects_for_type()` callers from within
> `traverse_bitmap_commit_list()` do *not* bother to inspect the return
> value, despite taking in an arbitrary 'show_reachable_fn', which itself
> may return a non-zero value.
>
> I guess this must be effectively OK in practice with respect to the
> existing code for the same reason you indicate in the commit message
> above, but we should change this function to *also* propagate non-zero
> return values to eliminate the foot-gun completely.
The matching non-bitmap traverse_commit_list() does not allow aborting
based on callback returns, either. In fact, its callbacks return void!
Whichever direction we go, those two should probably stay in sync (so
either both should allow aborting early with a non-zero return, or both
should return void).
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox