* Re: [PATCH v2 6/7] commit-reach: remove unused nonstale_queue dedup wrappers
From: Derrick Stolee @ 2026-06-24 13:55 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git
Cc: Elijah Newren, Kristofer Karlsson
In-Reply-To: <8c72f01083237c00397dd074beda8f854e882cbe.1782303254.git.gitgitgadget@gmail.com>
On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> nonstale_queue_put_dedup() and nonstale_queue_get_dedup() became
> unused after the previous commit. The core nonstale_queue functions
> remain in use by ahead_behind().
This is a nice cleanup that makes the previous diff easier to
read. Thanks!
^ permalink raw reply
* Re: [PATCH v2 7/7] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Derrick Stolee @ 2026-06-24 14:02 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git
Cc: Elijah Newren, Kristofer Karlsson
In-Reply-To: <d84b932e5b078edc8255b6944ecb67fc1aa086b0.1782303254.git.gitgitgadget@gmail.com>
On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add an early termination check to paint_down_to_common() using the
> per-side counters introduced earlier. Once the walk enters the
> finite-generation region, terminate early when one side's exclusive
> count drops to zero -- no new merge-base can form without both paint
> sides meeting.
Having this as the last patch is truly a nice climax moment for the
patch series!
> @@ -94,6 +94,9 @@ ends when one of the following conditions holds:
>
> 1. The queue is empty.
> 2. The queue contains only stale entries.
> + 3. Side exhaustion: no pure PARENT1 or pure PARENT2 commits
> + remain in the queue, no pending merge-base candidates exist,
> + and the walk has entered the finite-generation region.
...> +Side-exhaustion condition
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +A new merge-base requires commits from both sides to meet. When one
> +side's exclusive counter reaches zero and there are no pending
> +merge-base candidates, no future traversal step can produce a new
> +candidate.
> +
> +This optimization only activates in the finite-generation region
> +where topological ordering holds. In that region, children are
> +always visited before parents, so paint flags are final at visit
> +time and an exhausted side cannot reappear. In the INFINITY region,
> +commit-date ordering can violate this guarantee, so the check is
> +skipped.
> +
And these doc updates inline make me happy.
> Related documentation
> ---------------------
>
> diff --git a/commit-reach.c b/commit-reach.c
> index e0d9874f99..f79d0b64d6 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -133,17 +133,30 @@ static void paint_queue_put(struct paint_state *state,
>
> static struct commit *paint_queue_get(struct paint_state *state)
> {
> - struct commit *commit;
> + struct commit *commit = prio_queue_get(&state->queue);
>
> - if (!state->p1_count && !state->p2_count &&
> - !state->pending_merge_bases)
> + if (!commit)
> return NULL;
I see how the previous implementation has a termination condition
before calling prio_queue_get(), which is technically more
efficient. It does make this initial diff a bit more complicated
because we are moving the prio_queue_get() line.
If the introduction of the method in patch 5/7 looked like this:
+static struct commit *paint_queue_get(struct paint_state *state)
+{
+ struct commit *commit = prio_queue_get(&state->queue);
+
+ if (!commit)
+ return NULL;
+
+ if (!state->p1_count && !state->p2_count &&
+ !state->pending_merge_bases)
+ return NULL;
+
+ commit->object.flags &= ~ENQUEUED;
+ paint_count_update(state, commit->object.flags, -1);
+ return commit;
+}
Then this diff would look cleaner.
(This is the nittiest of nitpicks so feel free to ignore if this
doesn't bother you at all.)
> - commit = prio_queue_get(&state->queue);
> - if (commit) {
> - commit->object.flags &= ~ENQUEUED;
> - paint_count_update(state, commit->object.flags, -1);
> + commit->object.flags &= ~ENQUEUED;
> +
> + if (!state->pending_merge_bases) {
> + if (!state->p1_count && !state->p2_count)
> + return NULL;
> + /*
> + * Side exhaustion: a new merge-base can only form
> + * when both PARENT1-only and PARENT2-only commits
> + * remain in the queue. In the finite-generation
> + * region the queue is ordered topologically, so
> + * no future step can add paint to visited commits
> + * and an exhausted side cannot reappear.
> + */
> + if ((!state->p1_count || !state->p2_count) &&
> + commit_graph_generation(commit) < GENERATION_NUMBER_INFINITY)
> + return NULL;
> }
> +
> + paint_count_update(state, commit->object.flags, -1);
> return commit;
> }
I like how the crux of this implementation is entirely within
paint_queue_get() now.
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index c1109fb42f..03175befb3 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -332,12 +332,12 @@ test_expect_success 'merge-base --all commit-walk steps' '
> cp commit-graph-full .git/objects/info/commit-graph &&
> GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \
> git merge-base --all commit-9-9 commit-9-1 >actual &&
> - test_trace2_data paint_down_to_common steps 80 <trace-full.txt &&
> + test_trace2_data paint_down_to_common steps 9 <trace-full.txt &&
>
> cp commit-graph-half .git/objects/info/commit-graph &&
> GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \
> git merge-base --all commit-9-9 commit-9-1 >actual &&
> - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
> '
I love to see these steps change. If you take my suggestion to
update more tests with these checks, then this diff will get bigger
(but in a deserved way).
Also, when I suggested that 'test_all_modes' creates the trace
files on our behalf, I forgot to mention that this specific test
that you added in patch 4/7 simplifies by running the merge-base
check under 'test_all_modes' and then checking the trace2 data
on the three well-known files afterwards.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 0/7] commit-reach: terminate merge-base walk when one side is exhausted
From: Derrick Stolee @ 2026-06-24 14:09 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget, git
Cc: Elijah Newren, Kristofer Karlsson
In-Reply-To: <pull.2149.v2.git.1782303254.gitgitgadget@gmail.com>
On 6/24/2026 8:14 AM, Kristofer Karlsson via GitGitGadget wrote:
> commit-reach: terminate merge-base walk when one side is exhausted
>
> Optimize paint_down_to_common() for merge-base queries that hit large
> one-sided histories.
I completed my review of this version. All of my comments are around
either making the commit history a little cleaner or expanding the
tests that use the trace2 data.
I believe that this code is _correct_ and could be shipped as-is. My
comments are focused on making it the best that it could be, with an
eye towards a cleaner final result or a more robust test setup.
The most actionable things are:
1. You can add tracing before the new tests, allowing the new tests
to also check the step counts in their first versions and then
get updated in the final patch to demonstrate how they change
with that behavior change.
2. The t6600 helper 'test_all_modes' could set GIT_TRACE2_EVENT for
each mode into a different trace file that can be scanned later.
This will simplify your current tracing tests but also unlock
easier tracing like this in the future.
3. The termination condition depending on min_generation could be
refactored into paint_queue_get() to help make things even more
obvious as to when we terminate. This should help with your
concerns that you mentioned in response to patch 2/6 of the
previous version:
> I am not 100% happy with the halt-condition placement yet --
> the existing loop in master already has several exit paths
> (while condition, min_generation break, FIND_ALL break) and I
> think there is an opportunity to consolidate them. But that is
> a separate discussion and I do not want to derail this series.
> I can propose some alternatives in a follow-up after this
> lands.
I then have some super minor comments around making the diffs
even easier to read, but they could be ignored as they are very
nit-picky. It's the kind of detail that I would try to resolve
if I was the author, but I'm _not_. You are. Your time is
valuable so make your own conclusions as to whether you want to
go down that road. You've already entertained my ideas around
updating the docs as the implementation changes.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 0/7] commit-reach: terminate merge-base walk when one side is exhausted
From: Kristofer Karlsson @ 2026-06-24 14:25 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <67c00a9f-2aa2-4e83-9c0a-317ca589b232@gmail.com>
On Wed, 24 Jun 2026 at 15:34, Derrick Stolee <stolee@gmail.com> wrote:
>
> I like seeing these updates including the deterministic steps. Is there
> a reason you don't include the step data for 'merge-tree (across import)'
> in your monorepo case? The wall-clock is substantial, so it's not like the
> last two examples in git.git where there may not be any difference.
I will have to attribute to laziness I suppose :)
I ran the initial benchmarks before adding the trace, and I didn't
update all of them,
just enough to show the improvement and value of the trace data.
I will ensure that I include all of it in the next version though
(maybe 1-2 days from now?) or maybe drop some of the benchmarks to
not overload with partly redundant information.
(merge-tree benchmarks doesn't perhaps add much significance on top
of merge-base in practice).
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v2 4/7] commit-reach: add trace2 instrumentation to paint_down_to_common()
From: Kristofer Karlsson @ 2026-06-24 14:31 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <560c91df-3c07-4c8f-9924-ef0cc7646e08@gmail.com>
On Wed, 24 Jun 2026 at 15:41, Derrick Stolee <stolee@gmail.com> wrote:
>
> (highlighting this chunk)
>
> > + rm -rf .git/objects/info/commit-graph \
> > + .git/objects/info/commit-graphs &&
> > +
> > + GIT_TRACE2_EVENT="$(pwd)/trace-none.txt" \
> > + git merge-base --all commit-9-9 commit-9-1 >actual &&
> > + test_trace2_data paint_down_to_common steps 81 <trace-none.txt &&
>
> I'd rather see the whitespace line before the `rm` to make it
> more obvious that it's setting up the "none" case.
Ah yes, good point, will fix.
> > + cp commit-graph-full .git/objects/info/commit-graph &&
> > + GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \
> > + git merge-base --all commit-9-9 commit-9-1 >actual &&
> > + test_trace2_data paint_down_to_common steps 80 <trace-full.txt &&
> > +
> > + cp commit-graph-half .git/objects/info/commit-graph &&
> > + GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \
> > + git merge-base --all commit-9-9 commit-9-1 >actual &&
> > + test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> > +'
> > +
>
> This test is a great example. I look forward to seeing that it
> updates in the future.
>
> One thing I was hoping to see was that your side-exhaustion tests
> (from patch v2 2/7) would also include these checks so they are
> more obviously updating when the implementation updates later.
I was internally contemplating how much I should introduce the steps
validation to existing tests. My worry was that it might make tests
fragile - for example I repeatedly got some off-by-one changes
after refactoring the halt condition slightly (differs depending on
adding the halts solely within paint_queue_get or having it at the end
of the loop) and I think potentially other future work could affect it.
But I'm happy to attach the steps checks for more relevant tests,
it's not much work to change.
> One way to accomplish that is to reorder this patch before adding
> those tests so their first version includes these checks and then
> the values update when changing the implementation.
I was thinking I could keep the same order, but the patch to introduce
the trace could also modify the tests at the same time - that would
perhaps make it even more clear. Also this means I could avoid
making changes to Elijah's commit that I already _partly_ butchered
(extracted the test change as-is, but dropped the other file changes)
and I don't want to make that one more unclean.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v2 2/7] t6600: add test cases for side-exhaustion edge cases
From: Kristofer Karlsson @ 2026-06-24 14:33 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, git, Elijah Newren
In-Reply-To: <b4b33635-1279-46c0-819a-d29cc13921f5@gmail.com>
On Wed, 24 Jun 2026 at 15:43, Derrick Stolee <stolee@gmail.com> wrote:
>
> One way to make these tests have potential to check exact stats
> without too much extra work would be to update 'test_all_modes'
> to run each command with GIT_TRACE2_EVENT set to a known trace
> file (reset each time) that can then be checked after verifying
> that the results of each command is the same.
>
> Then, these tests could have lines such as
>
> test_trace2_data paint_down_to_common steps 20 <trace-full.txt &&
> test_trace2_data paint_down_to_common steps 30 <trace-half.txt &&
> test_trace2_data paint_down_to_common steps 40 <trace-none.txt
>
> after the test_all_modes line.
That does indeed look quite clean, I will try to massage the tests to
utilize that, though I haven't fully worked it through in my head yet so
I am not sure if/where I would get stuck. :)
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v2 5/7] commit-reach: introduce struct paint_state with per-side counters
From: Kristofer Karlsson @ 2026-06-24 14:38 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <19639ad3-2d16-4f3b-be79-138e00144ea3@gmail.com>
On Wed, 24 Jun 2026 at 15:54, Derrick Stolee <stolee@gmail.com> wrote:
>
> I'm grateful to see these changes happening to the doc in real-
> time. I know it was extra work, but I'm grateful right now.
>
> Hopefully future historians will also benefit from this effort.
It was honestly not bad at all, and I agree it felt quite nice to
see how the doc naturally changed along with the implementation.
> > +static struct commit *paint_queue_get(struct paint_state *state)
> > +{
>
> Since we are going to make this a more complete termination
> condition, we may want to make that very explicit with a doc-
> comment. Something along the lines of "dequeue a commit when
> possible, but also signal termination of the walk when we
> conclude that no more merge bases will be discovered due to
> internal state."
Yes, I'll make sure to clean that part up more, maybe also
rename the function to be more descriptive.
> You mentioned in your cover letter how the min_generation value
> can add extra termination conditions. It may be a good idea to
> insert min_generation into the paint_queue struct and make it a
> termination condition for paint_queue_get(). If you consider this
> direction, then I'd make it a separate patch on top of this one
> _before_ adding the one-sided change. The extra tests that cover
> the exact number of walked commits can help to guarantee the same
> behavior, assuming that some of those tests check a non-zero
> min_generation input. (It may be good to add such trace tests in
> an earlier patch to help confidence in this case.)
I think I might wait with this - the patch series already feels
quite big, and I think it has a natural progression and finish now.
But I can definitely commit to following up later -- it would be a
smaller series that is easier to reason about, likely a single commit.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v2 7/7] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Kristofer Karlsson @ 2026-06-24 14:47 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <6b0d81e7-7617-4fb4-9e39-cdf8bc778837@gmail.com>
On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote:
>
> I see how the previous implementation has a termination condition
> before calling prio_queue_get(), which is technically more
> efficient. It does make this initial diff a bit more complicated
> because we are moving the prio_queue_get() line.
I was thinking the efficiency here does not matter in practice -
prio_queue_get() only returns NULL once, and all other times
where we keep looping we do need the value.
I agree it does get a bit complex though.
> If the introduction of the method in patch 5/7 looked like this:
>
> +static struct commit *paint_queue_get(struct paint_state *state)
> +{
> + struct commit *commit = prio_queue_get(&state->queue);
> +
> + if (!commit)
> + return NULL;
> +
> + if (!state->p1_count && !state->p2_count &&
> + !state->pending_merge_bases)
> + return NULL;
> +
> + commit->object.flags &= ~ENQUEUED;
> + paint_count_update(state, commit->object.flags, -1);
> + return commit;
> +}
>
> Then this diff would look cleaner.
>
> (This is the nittiest of nitpicks so feel free to ignore if this
> doesn't bother you at all.)
That's a good point. It doesn't technically bother me,
but it would be cleaner. The refactor commit would effectively
be looking into the future and prepare for it. I can change it for
the next version - my only thinking was that the current refactor
patch matched my original idea for how to best handle
the halt condition, but that did indeed change after this discussion.
> > - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> > + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
> > '
> I love to see these steps change. If you take my suggestion to
> update more tests with these checks, then this diff will get bigger
> (but in a deserved way).
I will try to add them to some (but not all) tests since it's more
closely related to performance than correctness and I want to
avoid making too many tests overly fragile.
> Also, when I suggested that 'test_all_modes' creates the trace
> files on our behalf, I forgot to mention that this specific test
> that you added in patch 4/7 simplifies by running the merge-base
> check under 'test_all_modes' and then checking the trace2 data
> on the three well-known files afterwards.
That's a nice bonus, I will try to see if I can manage to utilize it.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v2 7/7] commit-reach: terminate merge-base walk when one paint side is exhausted
From: Derrick Stolee @ 2026-06-24 15:07 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Kristofer Karlsson via GitGitGadget, git, Elijah Newren
In-Reply-To: <CAL71e4N1zMz=v9umGdGPTvLP1nF-tNLVQc+vAEBnekt2L0b6zQ@mail.gmail.com>
On 6/24/2026 10:47 AM, Kristofer Karlsson wrote:
> On Wed, 24 Jun 2026 at 16:02, Derrick Stolee <stolee@gmail.com> wrote:
>>> - test_trace2_data paint_down_to_common steps 81 <trace-half.txt
>>> + test_trace2_data paint_down_to_common steps 57 <trace-half.txt
>>> '
>> I love to see these steps change. If you take my suggestion to
>> update more tests with these checks, then this diff will get bigger
>> (but in a deserved way).
>
> I will try to add them to some (but not all) tests since it's more
> closely related to performance than correctness and I want to
> avoid making too many tests overly fragile.
In this case, I think it's more about protecting all of our special-
cased termination conditions. The rigidity means that it is hard to
accidentally change the behavior. It does have the downside that
more tests need to change if there is an intentional change, but it
also gives the same _evidence_ that the change has the intended
impact.
We are definitely leaning into personal preferences, though. There
is no hard rule one way or another.
Thanks,
-Stolee
^ permalink raw reply
* Re: Fetching missing submodule refs unnecessarily fatal
From: Ben Knoble @ 2026-06-24 16:15 UTC (permalink / raw)
To: Mike Crowe; +Cc: Git maillinglist
In-Reply-To: <ajvouniXVAPH8nyZ@mcrowe.com>
[re adding list, woops!]
> Le 24 juin 2026 à 10:24, Mike Crowe <mac@mcrowe.com> a écrit :
>
> On Wednesday 24 June 2026 at 08:39:39 -0400, Ben Knoble wrote:
>>
>>>> Le 23 juin 2026 à 11:04, Mike Crowe <mac@mcrowe.com> a écrit :
>>>
>>> When Git fetches in a superproject with --recurse-submodules, it appears to
>>> try to fetch the corresponding submodule repository commits for every new
>>> or updated superproject branch. Presumably this is so that everthing is
>>> ready to switch to one of those branches without further fetching.
>>>
>>> Developers may create commits that contain submodules that reference
>>> commits in the submodule repository, but those commits may not be pushed to
>>> the submodule's remote repository. When the superproject commits are pushed
>>> to a personal remote branch anyone else's Git fetch cannot find the
>>> corresponding submodule commit and fails.
>>
>> This is the part that confuses me: if a (public) commit of history refers
>> to a submodule at a particular commit, and that commit is not available
>> anywhere, then we won’t be able to properly update submodules when using
>> that commit. That creates a problem!
>
> It does. But only for that user's personal branch. Even though it is
> public, a personal branch is mostly only for the use of that user and it
> doesn't matter to anyone but them. (The user is probably working
> simultaneously on both the superproject and the submodule.)
>
>> Why not instead make sure the submodule commit is also available for fetching?
>
> This relies on the user realising what they've done. They might even think
> that they've made the right submodule commit available but forgot that they
> rebased it before pushing or something changing the commit hash.
Yeah, the recurse-submodules option for pushing makes this easier to notice/act on, too.
> From a resilience point of view it shouldn't be possible for someone who
> can push changes to their own personal branch to perform a "denial of
> service" on anyone else fetching from the repository by making that fetch
> fail.
>
> I hope this makes it clearer.
That does make some sense: I wouldn’t want to get interrupted by a colleague or collaborator’s bad push of an unrelated branch.
> Thanks.
>
> Mike.
>
> (Was there a reason that you didn't reply to the list?)
Nope, mis-click.
^ permalink raw reply
* Re: [PATCH v2 4/4] connected: search promisor objects generically
From: Junio C Hamano @ 2026-06-24 16:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-4-132d73ee47b9@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> When performing connectivity checks we have to figure out whether any of
> the new objects are promisor objects, as we cannot assume full
> connectivity if so.
>
> This check is performed by iterating through all packfiles in the
> repository and searching each of them for the given object. Of course,
> this mechanism is quite specific to implementation details of the object
> database, as we assume that it uses packfiles in the first place.
>
> Refactor the logic so that we instead use `odb_for_each_object_ext()`
> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
> flag. This will yield all objects that have the exact object name and
> that are part of a promisor pack in a generic way.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> connected.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index d2b334173f..b557ff5db9 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -11,6 +11,13 @@
> #include "packfile.h"
> #include "promisor-remote.h"
>
> +static int promised_object_cb(const struct object_id *oid UNUSED,
> + struct object_info *oi UNUSED,
> + void *payload UNUSED)
> +{
> + return 1;
> +}
> +
> /*
> * For partial clones, we don't want to have to do a regular connectivity check
> * because we have to enumerate and exclude all promisor objects (slow), and
> @@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,
> void *cb_data,
> const struct object_id **oid)
> {
> + struct odb_for_each_object_options opts = {
> + .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
> + .prefix_hex_len = the_repository->hash_algo->hexsz,
> + };
> + int err;
> +
> odb_reprepare(the_repository->objects);
> do {
> - struct packed_git *p;
> + opts.prefix = *oid;
>
> - repo_for_each_pack(the_repository, p) {
> - if (!p->pack_promisor)
> - continue;
> - if (find_pack_entry_one(*oid, p))
> - goto promisor_pack_found;
> - }
> + err = odb_for_each_object_ext(the_repository->objects,
> + NULL, promised_object_cb,
> + NULL, &opts);
promised_object_cb() returns 1 without any computation since we are
only interested in learning ODB_FOR_EACH_OBJECT_PROMISOR_ONLY finds
any such object.
odb_for_each_object_ext() returns 0 (if it iterates all the sources
to the end), but if its call to odb_source_for_each_object() yields
non-zero value, the returned value comes back as "err" here,
terminating the for-each iteration immediately.
odb_source_for_each_object() is implemented differently per the
source backend, but taking an example of "packfile" backend,
packfile_loose_for_each_object() ends up calling cb (wrapped in
packfile_store_for_each_object_wrapper_data) via
for_each_object_in_pack(), which stops immediately when cb returns
non-zero and the value returned from there is the value given by cb,
i.e., 1. So we will have err==1 when we find any object.
> + if (err < 0)
> + return err;
And err presumably is 1 in such a case, so this does not trigger.
> /*
> * We have found an object that is not part of a promisor pack,
> * and thus we cannot skip the full connectivity check.
> */
> - return 0;
> -
> -promisor_pack_found:
> - ;
> + if (err > 0)
> + return 0;
And this does.
I may be misreading the patch, but as we return 0 from here, do we
cause the caller to fall back to full connectivity check? The
caller, check_connected(), sees a zero returned from here.
> } while ((*oid = fn(cb_data)) != NULL);
>
> return 1;
^ permalink raw reply
* Re: [PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF
From: Junio C Hamano @ 2026-06-24 16:58 UTC (permalink / raw)
To: Antonio De Stefani; +Cc: git
In-Reply-To: <20260624093618.17456-1-antonio.destefani08@gmail.com>
Antonio De Stefani <antonio.destefani08@gmail.com> writes:
> c4adea82 (Convert CR/LF to LF in tag signatures, 2008-07-11)
> introduced CR stripping for GPG output on Windows, but intentionally
> stripped all CR characters unconditionally to "keep the code simpler",
> even though only \r\n sequences (Windows line endings) needed to be
> normalized. 2f47eae2 (Split GPG interface into its own helper library,
> 2011-09-07) moved the code into gpg-interface.c, and 29b31577 (ssh
> signing: add ssh key format and signing code, 2021-09-10) extracted
> it into the remove_cr_after() helper when adding SSH signing support.
>
> The original laziness was safe at the time because lone CR characters
> are not expected in GPG signature output. However, the NEEDSWORK
> comment left by a previous reader correctly identified that only
> \r\n pairs should be stripped, not lone \r characters.
>
> Fix the loop to skip \r only when immediately followed by \n, keeping
> lone trailing CR characters intact. Rename the function to
> strip_cr_before_lf to reflect its corrected behavior, and update
> both call sites and their comments accordingly.
>
> Signed-off-by: Antonio De Stefani <antonio.destefani08@gmail.com>
> ---
> gpg-interface.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
Looking good. Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix
From: Christian Couder @ 2026-06-24 17:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Christian Couder
In-Reply-To: <20260624-pks-connected-generic-promisor-checks-v2-2-132d73ee47b9@pks.im>
On Wed, Jun 24, 2026 at 12:37 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> Callers of `odb_for_each_object()` can specify an optional object name
> prefix so that we only yield objects that match it. This is incompatible
> though with passing flags at the same time, as we don't yet know to
> handle them.
>
> Loosen this restriction by calling `should_exclude_pack()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> odb/source-packed.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 3afc4bf01f..6f31f0ff94 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(
> const struct odb_for_each_object_options *opts,
> struct odb_source_packed_for_each_object_wrapper_data *data)
> {
> + bool pack_errors = false;
> int ret;
>
> for (; m; m = m->base_midx) {
> @@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(
> const struct object_id *current = NULL;
> struct object_id oid;
>
> + if (opts->flags) {
> + uint32_t pack_id = nth_midxed_pack_int_id(m, i);
> + struct packed_git *pack;
> +
> + if (prepare_midx_pack(m, pack_id)) {
> + pack_errors = true;
> + continue;
> + }
> +
> + pack = nth_midxed_pack(m, pack_id);
> + if (should_exclude_pack(pack, opts->flags))
> + continue;
> + }
> +
> current = nth_midxed_object_oid(&oid, m, i);
>
> if (!match_hash(len, opts->prefix->hash, current->hash))
It looks like this is:
if (!match_hash(len, opts->prefix->hash, current->hash))
break;
and I wonder if the `if (opts->flags) { ... }` block would be better
after that prefix check rather than before it.
Putting it after the prefix check would make sure we don't continue
when the prefix doesn't match.
^ permalink raw reply
* Re: [PATCH v2 1/7] Documentation/technical: add paint-down-to-common doc
From: Junio C Hamano @ 2026-06-24 17:09 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Elijah Newren, Kristofer Karlsson
In-Reply-To: <19ed743bd10be5341eee040eb8070876b984773d.1782303254.git.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add a technical document describing the paint_down_to_common()
> algorithm used for merge-base computation, covering the paint
> walk, generation number regions, and termination conditions.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> Documentation/Makefile | 1 +
> Documentation/technical/meson.build | 1 +
> .../technical/paint-down-to-common.adoc | 114 ++++++++++++++++++
> commit-reach.c | 6 +-
> 4 files changed, 121 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/technical/paint-down-to-common.adoc
Great write-up that very clearly and concisely explains what goes on
inside the merge-base computation. Thanks for a pleasant read.
^ permalink raw reply
* Re: [PATCH 0/6] odb: refactor source-specific information in object info
From: Junio C Hamano @ 2026-06-24 17:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260624-b4-pks-odb-drop-whence-v1-0-8d1877b790ac@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> this patch series refactors `struct object_info` to not contain the
> `whence` field anymore.
>
> This field only gave the caller information about the type of source
> this was read from, but it didn't allow them to figure out which source
> specifically yielded the object. So instead, we replace this information
> with a new `struct object_info_source` field that both contains info
> about the source, and any backend-specific data.
Great.
^ permalink raw reply
* Re: [GSoC Patch v8 1/3] path: extract format_path() and use in rev-parse
From: Junio C Hamano @ 2026-06-24 18:15 UTC (permalink / raw)
To: K Jayatheerth
Cc: a3205153416, git, jltobler, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <20260624033748.108281-2-jayatheerthkulkarni2005@gmail.com>
K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> +void format_path(struct strbuf *dest, const char *path,
> + const char *prefix, enum path_format format)
> +{
> + strbuf_reset(dest);
> +
> + switch (format) {
> + case PATH_FORMAT_UNMODIFIED:
> + strbuf_addstr(dest, path);
> + break;
> +
> + case PATH_FORMAT_RELATIVE: {
> ...
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> +
> + strbuf_release(&relative_buf);
> + strbuf_release(&real_path);
> + strbuf_release(&real_prefix);
> + free(cwd);
> + break;
> + }
> +
> + case PATH_FORMAT_RELATIVE_IF_SHARED: {
> ...
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> + strbuf_release(&relative_buf);
> + break;
> + }
> +
> + case PATH_FORMAT_CANONICAL:
> + /*
> + * strbuf_realpath_forgiving inherently resets the destination
> + * buffer, safely aligning with our replace semantics.
> + */
> + strbuf_realpath_forgiving(dest, path, 1);
> + break;
> +
> + default:
> + BUG("unknown path_format value %d", format);
> + }
> +}
Hmph.
I was hoping that we could lose even more strbuf, but since
relative_path() does not always leave its result in the strbuf that
is passed to it as its third parameter, we do need addstr() into
dest, which is a bit unsatisfying but not a fault of this patch at
all. At least, we lost extra copy in the canonical codepath ;-)
Looking good. Thanks.
^ permalink raw reply
* Re: [PATCH 1/6] object-file: rename files transaction prepare function
From: Junio C Hamano @ 2026-06-24 18:26 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, ps
In-Reply-To: <20260624041920.2601961-2-jltobler@gmail.com>
Justin Tobler <jltobler@gmail.com> writes:
> The "files" ODB transaction backend lazily creates a temporary object
> directory when the first loose object is written to the transaction via
> `prepare_loose_object_transaction()`. In a subsequent commit, the
> temporary directory is used to also write packfiles to.
>
> Rename the function to `odb_transaction_files_prepare()` accordingly.
Taken by itself this renaming does make sense, but there are many
other function that follow the historical naming convention, like
{fsync,flush}_loose_object_transaction(). Should we rename them for
consistency with the new naming scheme, not necessarily as part of
this series but with a todo comment to do so once the dust settles,
or something?
^ permalink raw reply
* Re: [PATCH 2/6] object-file: propagate files transaction errors
From: Junio C Hamano @ 2026-06-24 18:35 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, ps
In-Reply-To: <20260624041920.2601961-3-jltobler@gmail.com>
Justin Tobler <jltobler@gmail.com> writes:
> The "files" transaction backend may encounter errors related to managing
> the temporary directory used to stage objects, but silently ignores
> these errors. Instead return errors encountered in the
> `odb_transaction_files_{prepare,begin,commit}()` interfaces to allow
> callers to handle as needed.
"handle them as needed", perhaps.
> -static void odb_transaction_files_prepare(struct odb_transaction *base)
> +static int odb_transaction_files_prepare(struct odb_transaction *base)
> {
> struct odb_transaction_files *transaction =
> container_of_or_null(base, struct odb_transaction_files, base);
> @@ -511,11 +511,15 @@ static void odb_transaction_files_prepare(struct odb_transaction *base)
> * added at the time they call odb_transaction_files_begin.
> */
> if (!transaction || transaction->objdir)
> - return;
> + return 0;
>
> transaction->objdir = tmp_objdir_create(base->source->odb->repo, "bulk-fsync");
If this fails, NULL is returned, and ...
> - if (transaction->objdir)
> - tmp_objdir_replace_primary_odb(transaction->objdir, 0);
> + if (!transaction->objdir)
> + return -1;
... we return -1 from here to signal an error now.
But callers of this function in write_loose_object(), and
odb_source_loose_write_stream() are not prepared to react to such an
error.
I guess this is nothing new. The callers ignored such an error from here
in the original and proceeded writing the primary ODB anyway, and we
continue to do so after this step.
> @@ -542,13 +546,13 @@ static void fsync_loose_object_transaction(struct odb_transaction *base,
> /*
> * Cleanup after batch-mode fsync_object_files.
> */
> -static void flush_loose_object_transaction(struct odb_transaction_files *transaction)
> +static int flush_loose_object_transaction(struct odb_transaction_files *transaction)
> {
> struct strbuf temp_path = STRBUF_INIT;
> struct tempfile *temp;
>
> if (!transaction->objdir)
> - return;
> + return 0;
>
> /*
> * Issue a full hardware flush against a temporary file to ensure
> @@ -570,8 +574,12 @@ static void flush_loose_object_transaction(struct odb_transaction_files *transac
> * Make the object files visible in the primary ODB after their data is
> * fully durable.
> */
> - tmp_objdir_migrate(transaction->objdir);
> + if (tmp_objdir_migrate(transaction->objdir))
> + return -1;
> +
> transaction->objdir = NULL;
> +
> + return 0;
> }
The caller of this function does react to a failure of it, ...
> @@ -1670,27 +1678,34 @@ int read_loose_object(struct repository *repo,
> return ret;
> }
>
> -static void odb_transaction_files_commit(struct odb_transaction *base)
> +static int odb_transaction_files_commit(struct odb_transaction *base)
> {
> struct odb_transaction_files *transaction =
> container_of(base, struct odb_transaction_files, base);
>
> - flush_loose_object_transaction(transaction);
> + if (flush_loose_object_transaction(transaction))
> + return -1;
> flush_packfile_transaction(transaction);
> +
> + return 0;
> }
... like this, which is good. Do we need an explicit "abort-transaction",
or is that implicit?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/3] replay: refactor enum replay_mode into a bool
From: Toon Claes @ 2026-06-24 19:15 UTC (permalink / raw)
To: Junio C Hamano, Patrick Steinhardt; +Cc: git, Elijah Newren
In-Reply-To: <xmqq7bnq37jm.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> That's fair, and the result is easier to write. But is it really easier
>> to read?
You're bringing up a very valid point there, and not directly in what
you're saying, but how it makes me reconsider.
So we're comparing:
pick_regular_commit(revs->repo, commit, replayed_commits,
mode == REPLAY_MODE_REVERT ? last_commit : onto,
&merge_opt, &result, mode, opts->empty);
with:
pick_regular_commit(revs->repo, commit, replayed_commits,
reverse ? last_commit : onto,
&merge_opt, &result, reverse, opts->empty);
You can argue which of both is easier to read, but the problem isn't
really whether it's a bool or an enum, but the ternary operator in this
lengthy function call is. That is the problem I was trying to solve, and
converting enum to bool isn't really the solution.
>> And what if we ever have to create a third mode going forward?
Personally I find this weak argument. As far as I know we most of the
time do not write code in a way so "it will be ready to add X in the
future". In my personal experience, I'm always wrong in predicting what
might be added in the future. Although I must say this case is
different, because we're not adding something new, no this commit was
dumbing down something existing. So I'll revisit this commit in the next
iteration.
>> I'm generally no fan of booleans as parameters as they basically give
>> you no information at all at the callsite, except if you're lucky and
>> you already have an aptly-named variable available that you can pass.
>> Which seems to be the case here, but I'm still not sure whether this
>> change really improves the code.
That's also a very valid argument, which I didn't take in mind.
> I tend to agree with you on both counts. The "what happens when
> somebody else wants a third choice?" is a quesiton I would ask the
> first thing as the maintainer of a project.
>
> Even if the boolean parameter is so obviously named, the callsite
> can only say "true" or "false", unlike some other popular languages
> that lets you say
>
> my_function(use_revert_mode=true, verbose=false);
>
> and you cannot tell what effect the author wanted out of that "true"
> if all you can write were
>
> my_function(true, false);
>
> Of course, we could go ultra verbose, like
>
> my_function(true, /* use_revert_mode */
> false, /* verbose */);
>
> but then we are often better off writing:
>
> my_function(REPLAY_MODE_REVERT, REPLAY_QUIET);
Thanks for bringing in this illustrative example. Point made, I'll
revisit.
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH 0/6] receive-pack: use ODB transactions to stage object writes
From: Junio C Hamano @ 2026-06-24 20:09 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, ps
In-Reply-To: <20260624041920.2601961-1-jltobler@gmail.com>
Justin Tobler <jltobler@gmail.com> writes:
> Greetings,
>
> This patch series replaces direct usage of the `tmp_objdir` interfaces
> in git-receive-pack(1) to instead use the `odb_transaction` interfaces
> to create/manage a staging area to write objects to. The purpose of this
> change is to get git-receive-pack(1) one step closer to being ODB
> backend agnostic. For now, the object writes themselves are still
> "files" backend specific due to being handled by the git-index-pack(1)
> and git-unpack-objects(1) child processes. This will be tackled in a
> separate series though.
>
> Thanks,
> -Justin
The integration cycle this morning was somehow more painful than
other cycles.
FYI, this topic had some interactions with ps/odb-source-packed and
ps/refs-avoid-chdir-notify-reparent and needed the following
evil-merge fix to make it build.
commit 721c15c1d5ef8f8aab8b9f50463e979e890b1ab6
Author: Junio C Hamano <gitster@pobox.com>
Date: Wed Jun 24 12:45:35 2026 -0700
merge-fix/jt/receive-pack-use-odb-transactions
conflict with ps/odb-source-packed and ps/refs-avoid-chdir-notify-reparent
diff --git a/odb/source-packed.c b/odb/source-packed.c
index c3c26fb53e..b9231a8da0 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -545,7 +545,8 @@ static int odb_source_packed_write_object_stream(struct odb_source *source UNUSE
}
static int odb_source_packed_begin_transaction(struct odb_source *source UNUSED,
- struct odb_transaction **out UNUSED)
+ struct odb_transaction **out UNUSED,
+ enum odb_transaction_flags flags UNUSED)
{
return error("packed backend cannot begin transactions");
}
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index a35bbc8004..dd5db06534 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -235,7 +235,8 @@ void test_reftable_table__seek_invalid_log_offset(void)
uint8_t *footer;
cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs),
- logs, ARRAY_SIZE(logs), NULL);
+ logs, ARRAY_SIZE(logs),
+ REFTABLE_HASH_SHA1, NULL);
/*
* Corrupt the log section offset stored in the footer so that it points
@@ -278,7 +279,8 @@ void test_reftable_table__new_with_truncated_table(void)
struct reftable_table *table;
struct reftable_buf buf = REFTABLE_BUF_INIT;
- cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0, NULL);
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0,
+ REFTABLE_HASH_SHA1, NULL);
/*
* Truncate the table so that it is large enough to read the header, but
^ permalink raw reply related
* Re: [PATCH GSoC RFC v13 06/12] connect: refactor packet writing
From: Junio C Hamano @ 2026-06-24 20:19 UTC (permalink / raw)
To: Karthik Nayak
Cc: Pablo Sabater, peff, eric.peijian, chriscool, git, jltobler, toon,
chandrapratap3519, Jonathan Tan, Calvin Wan
In-Reply-To: <CAOLa=ZSvxXuf_bSzKMvViNQ5MuDAqxnQdo4asF9vfMhJaDQcVw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> +/*
>> + * Writes a command along with the requested server capabilities/features into a
>> + * request buffer.
>> + */
>> struct string_list;
>
> The comment should be above the function and not the forward
> declaration.
>
> While we're here, why not `#include "string-list.h"` and remove the
> forward declaration, is there a circular dependency?
Isn't it to avoid unnecessary include? When the header itself only
needs to know about the presence of the type, and not the concrete
shape of the type (e.g., because it only uses a pointer to that
type), it may be overkill to include the entire header file.
^ permalink raw reply
* Re: [PATCH v5 07/11] refs: move parsing of "core.logAllRefUpdates" back into ref stores
From: Justin Tobler @ 2026-06-24 21:22 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Jeff King
In-Reply-To: <20260622-b4-pks-refs-avoid-chdir-notify-reparent-v5-7-018475013dbc@pks.im>
On 26/06/22 10:28AM, Patrick Steinhardt wrote:
> In cc42c88945 (refs: extract out reflog config to generic layer,
> 2026-05-04) we have refactored how we parse "core.logAllRefUpdates" so
> that it happens in the generic layer. Unfortunately, this has worsened a
> preexisting issue where we may recurse when creating the reference store
> because of a chicken-and-egg problem between parsing the configuration
> and evaluating "onbranch" conditions.
Ok so IIUC, parsing "core.logAllRefUpdates" in the generic layer forces
us to read the config earlier. This is problematic though since the
refstore has not been initialized yet which we need to evaluate
"onbranch" conditions.
> Prepare for a fix by essentially reverting that change so that we handle
> this setting in the respective backends again. The backends are already
> parsing other configuration anyway, so by moving the logic back in there
> we can ensure that all backend configuration is parsed the same way.
Makes sense.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/checkout.c | 7 +++++--
> refs.c | 10 +++++++++-
> refs.h | 9 +++++++++
> refs/files-backend.c | 20 +++++++++++++++++---
> refs/refs-internal.h | 6 ------
> refs/reftable-backend.c | 20 +++++++++++---------
> repo-settings.c | 16 ----------------
> repo-settings.h | 9 ---------
> setup.c | 7 ++++++-
> 9 files changed, 57 insertions(+), 47 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b78b3a1d16..aee84ca897 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -952,10 +952,13 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
> const char *old_desc, *reflog_msg;
> if (opts->new_branch) {
> if (opts->new_orphan_branch) {
> - enum log_refs_config log_all_ref_updates =
> - repo_settings_get_log_all_ref_updates(the_repository);
> + enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
> + const char *value;
> char *refname;
>
> + if (!repo_config_get_string_tmp(the_repository, "core.logallrefupdates", &value))
> + log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
> +
> refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
> if (opts->new_branch_log &&
> !should_autocreate_reflog(log_all_ref_updates, refname)) {
> diff --git a/refs.c b/refs.c
> index d3caa9a633..5b773b1c15 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1053,6 +1053,15 @@ static char *normalize_reflog_message(const char *msg)
> return strbuf_detach(&sb, NULL);
> }
>
> +enum log_refs_config refs_parse_log_all_ref_updates_config(const char *value)
> +{
> + if (value && !strcasecmp(value, "always"))
> + return LOG_REFS_ALWAYS;
> + else if (git_config_bool("core.logallrefupdates", value))
> + return LOG_REFS_NORMAL;
> + return LOG_REFS_NONE;
> +}
This function replaces `repo_settings_get_log_all_ref_updates()`. I
assume we just wanted a slightly more simple function where the only
concern was parsing the `core.logallrefupdates` value.
> +
> int should_autocreate_reflog(enum log_refs_config log_all_ref_updates,
> const char *refname)
> {
> @@ -2327,7 +2336,6 @@ static struct ref_store *ref_store_init(struct repository *repo,
> struct ref_store *refs;
> struct ref_store_init_options opts = {
> .access_flags = flags,
> - .log_all_ref_updates = repo_settings_get_log_all_ref_updates(repo),
This config is no longer handled in the generic layer.
[snip]
> diff --git a/setup.c b/setup.c
> index 79125db565..0c6efb0560 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2584,10 +2584,15 @@ static int create_default_files(struct repository *repo,
> if (is_bare_repository())
> repo_config_set(repo, "core.bare", "true");
> else {
> + const char *value;
> +
> repo_config_set(repo, "core.bare", "false");
> +
> /* allow template config file to override the default */
> - if (repo_settings_get_log_all_ref_updates(repo) == LOG_REFS_UNSET)
> + if (repo_config_get_string_tmp(repo, "core.logallrefupdates", &value) ||
> + refs_parse_log_all_ref_updates_config(value) == LOG_REFS_UNSET)
Huh, can `refs_parse_log_all_ref_updates_config()` even return
LOG_REFS_UNSET?
-Justin
^ permalink raw reply
* Re: [PATCH v5 08/11] refs/files: lazy-load configuration to fix chicken-and-egg
From: Justin Tobler @ 2026-06-24 21:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Jeff King
In-Reply-To: <20260622-b4-pks-refs-avoid-chdir-notify-reparent-v5-8-018475013dbc@pks.im>
On 26/06/22 10:28AM, Patrick Steinhardt wrote:
> When initializing the "files" reference backend we read the repository's
> config to parse "core.preferSymlinkRefs" and "core.logAllRefUpdates".
> This results in a chicken-and-egg problem though, because parsing the
> configuration may require us to have access to the reference store
> already when an "onbranch" condition exists.
Ok so both of these configuration options are currently parsed at ref
store initialization time. This is problematic because we need the ref
store to properly handle "onbranch" conditions in the config.
> Luckily, all the configuration that we honor only relates to writing
> references. Consequently, we don't strictly need that configuration to
> be readily available at initialization time, and we can easiliy defer
> parsing it to a later point in time.
That's nice. So we don't actually need this configuration during
initialization and can instead lazily load it when writing the first
references. Makes sense.
> Implement this fix and add tests that verify that we can indeed properly
> parse these config knobs via an "onbranch" condition.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs/files-backend.c | 37 ++++++++++++++++++++++++++-----------
> t/t0600-reffiles-backend.sh | 21 +++++++++++++++++++++
> 2 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 79fb6735e1..d0f379dcd6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -84,12 +84,14 @@ struct files_ref_store {
> unsigned int store_flags;
>
> char *gitcommondir;
> - enum log_refs_config log_all_ref_updates;
> - int prefer_symlink_refs;
> -
> struct ref_cache *loose;
> -
> struct ref_store *packed_ref_store;
> +
> + struct files_ref_store_write_options {
> + enum log_refs_config log_all_ref_updates;
> + int prefer_symlink_refs;
> + bool initialized;
> + } write_opts_lazy_loaded;
It might be nice to leave some sort of breadcrumb comment to future
readers to explain why we lazy load this configuration.
> };
>
> static void clear_loose_ref_cache(struct files_ref_store *refs)
> @@ -121,17 +123,31 @@ static int files_ref_store_config(const char *var, const char *value,
> const struct config_context *ctx UNUSED,
> void *payload)
> {
> - struct files_ref_store *refs = payload;
> + struct files_ref_store_write_options *opts = payload;
>
> if (!strcmp(var, "core.prefersymlinkrefs")) {
> - refs->prefer_symlink_refs = git_config_bool(var, value);
> + opts->prefer_symlink_refs = git_config_bool(var, value);
> } else if (!strcmp(var, "core.logallrefupdates")) {
> - refs->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
> + opts->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
> }
>
> return 0;
> }
>
> +static const struct files_ref_store_write_options *files_ref_store_write_options(struct files_ref_store *refs)
> +{
> + struct files_ref_store_write_options *opts = &refs->write_opts_lazy_loaded;
> +
> + if (opts->initialized)
> + return opts;
> +
> + opts->log_all_ref_updates = LOG_REFS_UNSET;
> + repo_config(refs->base.repo, files_ref_store_config, opts);
> +
> + opts->initialized = true;
> + return opts;
> +}
> +
> /*
> * Create a new submodule ref cache and add it to the internal
> * set of caches.
> @@ -156,9 +172,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
> refs->packed_ref_store =
> packed_ref_store_init(repo, NULL, refs->gitcommondir, opts);
> refs->store_flags = opts->access_flags;
> - refs->log_all_ref_updates = LOG_REFS_UNSET;
>
> - repo_config(repo, files_ref_store_config, refs);
Configs are no longer read eagerly during initialization.
The rest of this patch looks good to me.
-Justin
^ permalink raw reply
* [PATCH v15 0/2] checkout: --track=fetch
From: Harald Nordgren via GitGitGadget @ 2026-06-24 21:54 UTC (permalink / raw)
To: git
Cc: Ramsay Jones, D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud,
Phillip Wood, Harald Nordgren
In-Reply-To: <pull.2281.v14.git.git.1781786652.gitgitgadget@gmail.com>
Extend checkout --track with a fetch mode to refresh start-point.
Changes in v15:
* Reword commit message to lead with motivation.
* Drop RESOLVE_REF_NO_RECURSE so <ns>/HEAD lookup matches what git checkout
does for the same ref. Drop redundant check_refname_format on a ref we
just read. Replace memset with brace initializer. Use refs_ref_exists and
pass NULL for the OID out-params.
* Split the "set HEAD" error onto its own line via die_message and advise,
matching the suggested format.
* Remove 6 redundant tests, replace test $a = $b with test_cmp_rev, and
rename test titles to avoid "namespace".
Changes in v14:
* Handle .h files in a better way.
Changes in v13:
* Create a preparatory commit that exposes find_tracking_remote_for_ref()
and advise_ambiguous_fetch_refspec() from branch.c, so checkout can reuse
the same lookup git branch --track uses.
* Use advise_ambiguous_fetch_refspec() for the "multiple remotes match"
case, so the wording matches git branch --track.
Harald Nordgren (2):
branch: expose helpers for finding the remote owning a tracking ref
checkout: extend --track with a "fetch" mode to refresh start-point
Documentation/git-checkout.adoc | 17 ++-
Documentation/git-switch.adoc | 5 +-
branch.c | 96 +++++++-------
branch.h | 16 +++
builtin/checkout.c | 138 +++++++++++++++++++-
t/t7201-co.sh | 222 ++++++++++++++++++++++++++++++++
6 files changed, 443 insertions(+), 51 deletions(-)
base-commit: ab776a62a78576513ee121424adb19597fbb7613
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2281%2FHaraldNordgren%2Fcheckout-fetch-start-point-v15
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2281/HaraldNordgren/checkout-fetch-start-point-v15
Pull-Request: https://github.com/git/git/pull/2281
Range-diff vs v14:
1: f79689c23d = 1: 8139490c36 branch: expose helpers for finding the remote owning a tracking ref
2: 8518f090b1 ! 2: b7e387f123 checkout: extend --track with a "fetch" mode to refresh start-point
@@ Metadata
## Commit message ##
checkout: extend --track with a "fetch" mode to refresh start-point
- Add a "fetch" mode to the "--track" option of "git checkout" / "git
- switch" that refreshes <start-point> before checking it out:
+ Forking from an existing remote branch without refreshing first often
+ has consequences: you start work that has already been done, or you
+ build on an old version of the code which causes big conflicts later
+ when you pull. The workaround is two commands ("git fetch <remote>
+ <branch> && git checkout -b <topic> <remote>/<branch>"), and when
+ the fetch is skipped the checkout silently starts from a stale tip.
- git checkout -b new_branch --track=fetch origin/some-branch
+ Users may already expect "<remote>/<branch>" to refer to the latest
+ tip on the remote. While this blurs the line between fetch and
+ checkout, git already does this in places where it pays off: "git
+ clone" fetches and checks out, and "git pull" fetches and merges.
- is shorthand for
+ Add a "fetch" mode to "--track" that refreshes <start-point> before
+ checking it out:
- git fetch origin some-branch
- git checkout -b new_branch --track origin/some-branch
+ git checkout -b new_branch --track=fetch origin/some-branch
- Identify the remote whose configured fetch refspec maps to
- <start-point> using find_tracking_remote_for_ref() (the same lookup
- "--track" uses to pick which remote to record in
- branch.<name>.remote), then run "git fetch <remote> <src-ref>" for
- just that ref so other remote-tracking branches are left untouched.
- When <start-point> is a bare <remote> (e.g. "origin"), follow
- refs/remotes/<remote>/HEAD to learn which branch to refresh. If
- "git fetch" fails but the remote-tracking ref already exists locally,
- warn and proceed from the existing tip; otherwise abort.
+ Only the requested branch is fetched so other remote-tracking
+ branches are left untouched. When <start-point> is a bare <remote>
+ (e.g. "origin"), follow refs/remotes/<remote>/HEAD to learn which
+ branch to refresh. If "git fetch" fails but the remote-tracking ref
+ already exists locally, warn and proceed from the existing tip,
+ otherwise abort.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
@@ builtin/checkout.c: struct branch_info {
+static void fetch_remote_for_start_point(const char *arg, int quiet)
+{
+ struct strbuf dst = STRBUF_INIT;
-+ struct tracking tracking;
++ struct tracking tracking = { 0 };
+ struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
+ struct string_list ambiguous_remotes = STRING_LIST_INIT_DUP;
+ struct child_process cmd = CHILD_PROCESS_INIT;
-+ struct object_id oid;
+ struct remote *named_remote;
+ int bare_ns;
+
@@ builtin/checkout.c: struct branch_info {
+ const char *head_target =
+ refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+ head_path,
-+ RESOLVE_REF_READING |
-+ RESOLVE_REF_NO_RECURSE,
-+ &oid, NULL);
++ RESOLVE_REF_READING,
++ NULL, NULL);
+ if (head_target &&
+ starts_with(head_target, dst.buf) &&
-+ head_target[dst.len] == '/' &&
-+ !check_refname_format(head_target, 0)) {
++ head_target[dst.len] == '/') {
+ strbuf_reset(&dst);
+ strbuf_addstr(&dst, head_target);
+ bare_ns = 0;
@@ builtin/checkout.c: struct branch_info {
+ free(head_path);
+ }
+
-+ memset(&tracking, 0, sizeof(tracking));
+ tracking.spec.dst = dst.buf;
+ tracking.srcs = &tracking_srcs;
+ find_tracking_remote_for_ref(&tracking, &ambiguous_remotes);
@@ builtin/checkout.c: struct branch_info {
+
+ if (!tracking.matches) {
+ if (bare_ns && named_remote &&
-+ remote_is_configured(named_remote, 1))
-+ die(_("cannot fetch start-point '%s': "
-+ "'refs/remotes/%s/HEAD' is not set; run "
-+ "'git remote set-head %s --auto' to set it"),
-+ arg, arg, arg);
++ remote_is_configured(named_remote, 1)) {
++ int status = die_message(_("cannot fetch start-point '%s' "
++ "because 'refs/remotes/%s/HEAD' "
++ "does not exist."), arg, arg);
++ advise(_("To create it run\n"
++ "\n"
++ " git remote set-head %s --auto\n"), arg);
++ exit(status);
++ }
+ die(_("cannot fetch start-point '%s': no configured remote's "
+ "fetch refspec matches it"), arg);
+ }
@@ builtin/checkout.c: struct branch_info {
+ tracking_srcs.items[0].string, NULL);
+ cmd.git_cmd = 1;
+ if (run_command(&cmd)) {
-+ if (!refs_read_ref(get_main_ref_store(the_repository),
-+ dst.buf, &oid))
++ if (refs_ref_exists(get_main_ref_store(the_repository), dst.buf))
+ warning(_("failed to fetch start-point '%s'; "
+ "using existing '%s'"), arg, dst.buf);
+ else
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ git -C fetch_upstream checkout -b fetch_other &&
+ test_commit -C fetch_upstream u_other_pre &&
+ git fetch fetch_upstream &&
-+ other_before=$(git rev-parse refs/remotes/fetch_upstream/fetch_other) &&
++ git update-ref refs/heads/snapshot_other refs/remotes/fetch_upstream/fetch_other &&
+ git -C fetch_upstream checkout fetch_target &&
+ test_commit -C fetch_upstream u_target_post &&
+ git -C fetch_upstream checkout fetch_other &&
+ test_commit -C fetch_upstream u_other_post &&
+ git checkout --track=fetch -b local_target fetch_upstream/fetch_target &&
+ test_cmp_rev refs/remotes/fetch_upstream/fetch_target HEAD &&
-+ test "$(git rev-parse refs/remotes/fetch_upstream/fetch_other)" = "$other_before"
++ test_cmp_rev refs/remotes/fetch_upstream/fetch_other snapshot_other
+'
+
+test_expect_success 'checkout --track=fetch with bare remote name fetches only <remote>/HEAD target' '
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ git -C fetch_upstream checkout -b fetch_unrelated &&
+ test_commit -C fetch_upstream u_unrelated_pre &&
+ git fetch fetch_upstream fetch_unrelated &&
-+ unrelated_before=$(git rev-parse refs/remotes/fetch_upstream/fetch_unrelated) &&
++ git update-ref refs/heads/snapshot_unrelated \
++ refs/remotes/fetch_upstream/fetch_unrelated &&
+ git -C fetch_upstream checkout main &&
+ test_commit -C fetch_upstream u_main_post &&
+ git -C fetch_upstream checkout fetch_unrelated &&
+ test_commit -C fetch_upstream u_unrelated_post &&
+ git checkout --track=fetch -b local_from_remote fetch_upstream &&
+ test_cmp_rev refs/remotes/fetch_upstream/main HEAD &&
-+ test "$(git rev-parse refs/remotes/fetch_upstream/fetch_unrelated)" = "$unrelated_before"
++ test_cmp_rev refs/remotes/fetch_upstream/fetch_unrelated snapshot_unrelated
+'
+
+test_expect_success 'checkout --track=fetch aborts and does not create branch when no existing ref' '
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_cmp_rev refs/remotes/custom-ns/fetch_refspec HEAD
+'
+
-+test_expect_success 'checkout --track=fetch on namespace bare name follows <ns>/HEAD' '
++test_expect_success 'checkout --track=fetch on bare remote-tracking prefix follows <prefix>/HEAD' '
+ git checkout main &&
+ git remote add fetch_ns ./fetch_upstream &&
+ test_when_finished "git remote remove fetch_ns" &&
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_cmp_config refs/heads/main branch.local_ns.merge
+'
+
-+test_expect_success '--track=fetch on bare hierarchical remote name follows <ns>/HEAD' '
-+ git checkout main &&
-+ git remote add nested/bare ./fetch_upstream &&
-+ test_when_finished "git remote remove nested/bare" &&
-+ test_when_finished "git update-ref -d refs/remotes/nested/bare/HEAD" &&
-+ git fetch nested/bare &&
-+ git symbolic-ref refs/remotes/nested/bare/HEAD \
-+ refs/remotes/nested/bare/main &&
-+ git -C fetch_upstream checkout main &&
-+ test_commit -C fetch_upstream u_nested_bare_post &&
-+ git checkout --track=fetch -b local_nested_bare nested/bare &&
-+ test_cmp_rev refs/remotes/nested/bare/main HEAD
-+'
-+
-+test_expect_success 'checkout --track=fetch handles hierarchical remote name' '
-+ git checkout main &&
-+ git remote add nested/remote ./fetch_upstream &&
-+ test_when_finished "git remote remove nested/remote" &&
-+ git -C fetch_upstream checkout -b fetch_hier &&
-+ test_commit -C fetch_upstream u_hier &&
-+ test_must_fail git rev-parse --verify refs/remotes/nested/remote/fetch_hier &&
-+ git checkout --track=fetch -b local_hier nested/remote/fetch_hier &&
-+ test_cmp_rev refs/remotes/nested/remote/fetch_hier HEAD
-+'
-+
+test_expect_success 'checkout --track=fetch dies on bare remote name with no <ns>/HEAD' '
+ git checkout main &&
+ git remote add fetch_nohead ./fetch_upstream &&
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_must_fail git rev-parse --verify refs/heads/local_unknown
+'
+
-+test_expect_success 'checkout --track=fetch rejects <ns>/HEAD pointing outside namespace' '
++test_expect_success 'checkout --track=fetch rejects <ns>/HEAD pointing outside the tracking prefix' '
+ git checkout main &&
+ git remote add fetch_crossns ./fetch_upstream &&
+ test_when_finished "git remote remove fetch_crossns" &&
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_must_fail git rev-parse --verify refs/heads/local_invalid
+'
+
-+test_expect_success 'checkout --track=fetch,inherit rejects invalid refname components' '
-+ git checkout main &&
-+ test_must_fail git checkout --track=fetch,inherit -b local_invalid \
-+ "foo..bar" 2>err &&
-+ test_grep "valid" err &&
-+ test_must_fail git rev-parse --verify refs/heads/local_invalid
-+'
-+
+test_expect_success 'checkout --track=inherit,direct is rejected' '
+ test_must_fail git checkout --track=inherit,direct -b bad fetch_upstream/fetch_new 2>err &&
+ test_grep "cannot combine" err
+'
+
-+test_expect_success 'checkout --track=direct,inherit is rejected' '
-+ test_must_fail git checkout --track=direct,inherit -b bad fetch_upstream/fetch_new 2>err &&
-+ test_grep "cannot combine" err
-+'
-+
+test_expect_success 'checkout --track=fetch then --track=direct drops fetch (last-one-wins)' '
+ git checkout main &&
+ git -C fetch_upstream checkout -b fetch_lastwin &&
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_lastwin
+'
+
-+test_expect_success 'checkout --track=fetch then --no-track drops fetch' '
-+ git checkout main &&
-+ git -C fetch_upstream checkout -b fetch_notrack &&
-+ test_commit -C fetch_upstream u_notrack &&
-+ test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_notrack &&
-+ test_must_fail git checkout --track=fetch --no-track \
-+ -b local_notrack fetch_upstream/fetch_notrack &&
-+ test_must_fail git rev-parse --verify refs/remotes/fetch_upstream/fetch_notrack
-+'
-+
+test_expect_success 'checkout --track=fetch,inherit fetches remote-tracking start-point' '
+ git checkout main &&
+ git -C fetch_upstream checkout -b fetch_inherit &&
@@ t/t7201-co.sh: test_expect_success 'tracking info copied with autoSetupMerge=inh
+ test_cmp_rev refs/remotes/fetch_upstream/fetch_inherit HEAD
+'
+
-+test_expect_success 'checkout --track=fetch,inherit errors when start-point does not map to a remote' '
-+ git checkout main &&
-+ test_must_fail git checkout --track=fetch,inherit -b bad main 2>err &&
-+ test_grep "no configured remote" err &&
-+ test_must_fail git rev-parse --verify refs/heads/bad
-+'
-+
+test_expect_success 'checkout --track=fetch on local start-point errors' '
+ git checkout main &&
+ test_must_fail git checkout --track=fetch -b bad main 2>err &&
--
gitgitgadget
^ permalink raw reply
* [PATCH v15 1/2] branch: expose helpers for finding the remote owning a tracking ref
From: Harald Nordgren via GitGitGadget @ 2026-06-24 21:54 UTC (permalink / raw)
To: git
Cc: Ramsay Jones, D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud,
Phillip Wood, Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2281.v15.git.git.1782338098.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
The remote-lookup that setup_tracking() does is useful outside
branch.c too; for example, deciding which remote to "git fetch"
from given a remote-tracking ref.
Move 'struct tracking' to branch.h and add two helpers backed by the
existing for_each_remote walk: find_tracking_remote_for_ref() and
advise_ambiguous_fetch_refspec(). setup_tracking() uses both. No
behavior change.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
branch.c | 96 ++++++++++++++++++++++++++++++--------------------------
branch.h | 16 ++++++++++
2 files changed, 68 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index 243db7d0fc..46ae7f0035 100644
--- a/branch.c
+++ b/branch.c
@@ -20,16 +20,9 @@
#include "run-command.h"
#include "strmap.h"
-struct tracking {
- struct refspec_item spec;
- struct string_list *srcs;
- const char *remote;
- int matches;
-};
-
struct find_tracked_branch_cb {
struct tracking *tracking;
- struct string_list ambiguous_remotes;
+ struct string_list *ambiguous_remotes;
};
static int find_tracked_branch(struct remote *remote, void *priv)
@@ -45,10 +38,10 @@ static int find_tracked_branch(struct remote *remote, void *priv)
break;
case 2:
/* there are at least two remotes; backfill the first one */
- string_list_append(&ftb->ambiguous_remotes, tracking->remote);
+ string_list_append(ftb->ambiguous_remotes, tracking->remote);
/* fall through */
default:
- string_list_append(&ftb->ambiguous_remotes, remote->name);
+ string_list_append(ftb->ambiguous_remotes, remote->name);
free(tracking->spec.src);
string_list_clear(tracking->srcs, 0);
break;
@@ -59,6 +52,51 @@ static int find_tracked_branch(struct remote *remote, void *priv)
return 0;
}
+void find_tracking_remote_for_ref(struct tracking *tracking,
+ struct string_list *ambiguous_remotes)
+{
+ struct find_tracked_branch_cb ftb_cb = {
+ .tracking = tracking,
+ .ambiguous_remotes = ambiguous_remotes,
+ };
+
+ for_each_remote(find_tracked_branch, &ftb_cb);
+}
+
+void advise_ambiguous_fetch_refspec(const char *dst,
+ const struct string_list *ambiguous_remotes)
+{
+ struct strbuf remotes_advice = STRBUF_INIT;
+ struct string_list_item *item;
+
+ if (!advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))
+ return;
+
+ for_each_string_list_item(item, ambiguous_remotes)
+ /*
+ * TRANSLATORS: This is a line listing a remote with duplicate
+ * refspecs in the advice message below. For RTL languages you'll
+ * probably want to swap the "%s" and leading " " space around.
+ */
+ strbuf_addf(&remotes_advice, _(" %s\n"), item->string);
+
+ /*
+ * TRANSLATORS: The second argument is a \n-delimited list of
+ * duplicate refspecs, composed above.
+ */
+ advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
+ "tracking ref '%s':\n"
+ "%s"
+ "\n"
+ "This is typically a configuration error.\n"
+ "\n"
+ "To support setting up tracking branches, ensure that\n"
+ "different remotes' fetch refspecs map into different\n"
+ "tracking namespaces."), dst,
+ remotes_advice.buf);
+ strbuf_release(&remotes_advice);
+}
+
static int should_setup_rebase(const char *origin)
{
switch (autorebase) {
@@ -254,11 +292,8 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
{
struct tracking tracking;
struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
+ struct string_list ambiguous_remotes = STRING_LIST_INIT_DUP;
int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
- struct find_tracked_branch_cb ftb_cb = {
- .tracking = &tracking,
- .ambiguous_remotes = STRING_LIST_INIT_DUP,
- };
if (!track)
BUG("asked to set up tracking, but tracking is disallowed");
@@ -267,7 +302,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
tracking.spec.dst = (char *)orig_ref;
tracking.srcs = &tracking_srcs;
if (track != BRANCH_TRACK_INHERIT)
- for_each_remote(find_tracked_branch, &ftb_cb);
+ find_tracking_remote_for_ref(&tracking, &ambiguous_remotes);
else if (inherit_tracking(&tracking, orig_ref))
goto cleanup;
@@ -293,34 +328,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
if (tracking.matches > 1) {
int status = die_message(_("not tracking: ambiguous information for ref '%s'"),
orig_ref);
- if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {
- struct strbuf remotes_advice = STRBUF_INIT;
- struct string_list_item *item;
-
- for_each_string_list_item(item, &ftb_cb.ambiguous_remotes)
- /*
- * TRANSLATORS: This is a line listing a remote with duplicate
- * refspecs in the advice message below. For RTL languages you'll
- * probably want to swap the "%s" and leading " " space around.
- */
- strbuf_addf(&remotes_advice, _(" %s\n"), item->string);
-
- /*
- * TRANSLATORS: The second argument is a \n-delimited list of
- * duplicate refspecs, composed above.
- */
- advise(_("There are multiple remotes whose fetch refspecs map to the remote\n"
- "tracking ref '%s':\n"
- "%s"
- "\n"
- "This is typically a configuration error.\n"
- "\n"
- "To support setting up tracking branches, ensure that\n"
- "different remotes' fetch refspecs map into different\n"
- "tracking namespaces."), orig_ref,
- remotes_advice.buf);
- strbuf_release(&remotes_advice);
- }
+ advise_ambiguous_fetch_refspec(orig_ref, &ambiguous_remotes);
exit(status);
}
@@ -347,7 +355,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
cleanup:
string_list_clear(&tracking_srcs, 0);
- string_list_clear(&ftb_cb.ambiguous_remotes, 0);
+ string_list_clear(&ambiguous_remotes, 0);
}
int read_branch_desc(struct strbuf *buf, const char *branch_name)
diff --git a/branch.h b/branch.h
index 3dc6e2a0ff..c2e6725491 100644
--- a/branch.h
+++ b/branch.h
@@ -1,9 +1,25 @@
#ifndef BRANCH_H
#define BRANCH_H
+#include "refspec.h"
+
+struct string_list;
struct repository;
struct strbuf;
+struct tracking {
+ struct refspec_item spec;
+ struct string_list *srcs;
+ const char *remote;
+ int matches;
+};
+
+void find_tracking_remote_for_ref(struct tracking *tracking,
+ struct string_list *ambiguous_remotes);
+
+void advise_ambiguous_fetch_refspec(const char *dst,
+ const struct string_list *ambiguous_remotes);
+
enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,
BRANCH_TRACK_NEVER = 0,
--
gitgitgadget
^ 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