* Re: Understanding why Git defaults to show author date and not committer date
From: Jeff King @ 2026-07-11 8:03 UTC (permalink / raw)
To: Omri Sarig; +Cc: git
In-Reply-To: <CAP9es6tyaGwfTguz5zgBmE5xN7MLDN3-rxRfo_JJBf79RCNzgg@mail.gmail.com>
On Fri, Jul 10, 2026 at 05:08:11PM +0200, Omri Sarig wrote:
> I understand the distinction between the 2 formats, and I can see the utility of
> both. I'm curious about the decision to show the author date and not the
> committer date as default one in Git commands.
> Are there some workflows where the author date is more relevant, or is that
> mostly a legacy decision?
>
> I'd be interested in hearing about workflows where the author date is the more
> useful one, as I use the committer date almost always.
In a workflow based on mailing patches, the committer date is usually
much less interesting. It is "when the maintainer happened to pick up
your patch", as opposed to when you wrote it. Likewise, we show the
author's name by default, not the committer's.
-Peff
^ permalink raw reply
* 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
* [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 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
* 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
* [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 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
* 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 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 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 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 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 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 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 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 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 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 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 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 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 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
* [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/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
* 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
* [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
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