* Re: [PATCH] completion: complete paths for git send-email
From: Junio C Hamano @ 2026-07-21 17:09 UTC (permalink / raw)
To: D. Ben Knoble
Cc: Yury Norov (NVIDIA), git, Thiago Perrotta, Philippe Blain,
Rubén Justo, Yury Norov, linux-kernel, Codex
In-Reply-To: <CALnO6CAuitGp_xLYkXpkQYV9oiXsNNfsXZ_OqzkW7_6ND49=LA@mail.gmail.com>
"D. Ben Knoble" <ben.knoble@gmail.com> writes:
> On Sun, Jul 19, 2026 at 9:45 AM Yury Norov (NVIDIA)
> <yury.norov@gmail.com> wrote:
>>
>> From: Yury Norov <ynorov@nvidia.com>
>>
>> git send-email accepts either revisions or paths to patch files, but its
>> Bash completion only offers revisions. This prevents patch files from
>> being completed. It can also make a prefix such as "0" expand to an
>> unrelated hexadecimal ref even when matching 0001-*.patch files exist.
>>
>> In my Linux tree, an attempt to autocomplete the standard-named patch
>> brings a random hashtag:
>
> It is unusual to call this a "hashtag." Perhaps "hash" or "object
> name" (or id) based on the glossary and datamodel docs?
Very good point, but I am not sure if the author truly meant object
names here. The reproduction test uses a long hexadecimal string,
but that is not an object name; it is an unusual-looking tag name.
It is like naming a topic branch '012345' and complaining that:
$ git send-email 0<TAB>
completes the input to the branch name while ignoring the
0001-changes.patch file.
When you have a branch named '0-tolerance-policy' and:
$ git send-email 0<TAB>
completes to that branch name, you would not dream of complaining
about the completion. IOW, I think the complaint is somewhat unfair
to begin with.
Actually, I do not know if the completion script really expands an
abbreviated object name to a full one. I tried:
$ git rev-parse seen^2
179eccf0d01729c19a3238905b951b1880aa4ba1
$ git checkout master
$ . contrib/completion/git-completion.bash
$ git send-email 17<TAB>
and waited for some time, but it did not complete to anything.
In any case, when both a '0001-my-changes.patch' file and a
'0-tolerance-policy' branch exist in your repository and current
working directory, running:
$ git send-email 0<TAB>
should offer both as candidates, I thihk. Since I only ever pass
filenames to the command, I personally do not think it is a huge
loss if the completion script stops looking at refs and sticks to
filenames only, but others may have a use for that feature.
^ permalink raw reply
* Re: [PATCH] t0213: skip ancestry tests under user-mode emulation
From: Junio C Hamano @ 2026-07-21 17:17 UTC (permalink / raw)
To: Jamie Magee via GitGitGadget; +Cc: git, Jamie Magee
In-Reply-To: <xmqqa4s38rbe.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Jamie Magee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> -# Determine if cmd_ancestry is supported on this platform.
>> +# Enable these tests only when cmd_ancestry reports real process names.
>> +# The procinfo stub emits no event; under user-mode emulation (e.g.
>> +# qemu-user) /proc reports the emulator, not the guest. Spawn test-tool
>> +# from test-tool and require "test-tool" in the child's ancestry.
>
> T.r.i.c.k.y. ;-)
>
>> test_expect_success 'detect cmd_ancestry support' '
>> test_when_finished "rm -f trace.detect" &&
>> GIT_TRACE2_BRIEF=1 GIT_TRACE2="$(pwd)/trace.detect" \
>> - test-tool trace2 001return 0 &&
>> - if grep -q "^cmd_ancestry" trace.detect
>> + test-tool trace2 004child test-tool trace2 001return 0 &&
>> + if grep -q "^cmd_ancestry.*test-tool" trace.detect
>
> This will be happy even if "test-tool-trash" that happens to have
> "test-tool" as its prefix appears on a cmd_ancestry line (for that
> matter, things like "cmd_ancestry-not-quite" that has "cmd_ancestry"
> as its prefix would be accepted). I guess that is OK because we are
> testing this in a fairly tightly controlled environment (trace keys
> are taken from known vocabulary, not arbitrary strings, for example).
>
> Will queue. Thanks.
Just to clarify, 'Will queue' simply means that the patch will
appear in 'seen'. It does not imply that I have personally
reviewed the change and found it acceptable to merge into 'next'.
I would appreciate a review from someone familiar with the trace2
facility.
Thanks.
^ permalink raw reply
* Re: [PATCH] completion: complete paths for git send-email
From: Yury Norov @ 2026-07-21 18:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: D. Ben Knoble, Yury Norov (NVIDIA), git, Thiago Perrotta,
Philippe Blain, Rubén Justo, linux-kernel, Codex
In-Reply-To: <xmqqcxwgz2u3.fsf@gitster.g>
On Tue, Jul 21, 2026 at 10:09:56AM -0700, Junio C Hamano wrote:
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > On Sun, Jul 19, 2026 at 9:45 AM Yury Norov (NVIDIA)
> > <yury.norov@gmail.com> wrote:
> >>
> >> From: Yury Norov <ynorov@nvidia.com>
> >>
> >> git send-email accepts either revisions or paths to patch files, but its
> >> Bash completion only offers revisions. This prevents patch files from
> >> being completed. It can also make a prefix such as "0" expand to an
> >> unrelated hexadecimal ref even when matching 0001-*.patch files exist.
> >>
> >> In my Linux tree, an attempt to autocomplete the standard-named patch
> >> brings a random hashtag:
> >
> > It is unusual to call this a "hashtag." Perhaps "hash" or "object
>
> Very good point, but I am not sure if the author truly meant object
> names here. > name" (or id) based on the glossary and datamodel docs?
I said hashtag because for me it's a hash of the tag:
git send-email 0<TAB>
git send-email 05c69d298c96703741cac9a5cbbf6c53bd55a6e2
But also it's a name of the tag, and git warns about it:
$ git show 05c69d298c96703741cac9a5cbbf6c53bd55a6e2
warning: refname '05c69d298c96703741cac9a5cbbf6c53bd55a6e2' is ambiguous.
Git normally never creates a ref that ends with 40 hex characters
because it will be ignored when you just specify 40-hex. These refs
may be created by mistake. For example,
git switch -c $br $(git rev-parse ...)
where "$br" is somehow empty and a 40-hex ref is created. Please
examine these refs and maybe delete them. Turn this message off by
running "git config set advice.objectNameWarning false"
commit 05c69d298c96703741cac9a5cbbf6c53bd55a6e2 (tag: 05c69d298c96703741cac9a5cbbf6c53bd55a6e2)
Author: Tejun Heo <tj@kernel.org>
Date: Tue May 15 08:22:04 2012 +0200
...
I have no local branch or local file with that name, but the tag exists
for 14 years, and will not go away. And yes, it breaks autocompletion.
So, after rethinking, the problem looks like this: if autocompletion
logic finds a tag beginning with that pattern, it doesn't attempt to
search for the matching files, which is wrong
> The reproduction test uses a long hexadecimal string,
> but that is not an object name; it is an unusual-looking tag name.
> It is like naming a topic branch '012345' and complaining that:
>
> $ git send-email 0<TAB>
>
> completes the input to the branch name while ignoring the
> 0001-changes.patch file.
>
> When you have a branch named '0-tolerance-policy' and:
>
> $ git send-email 0<TAB>
>
> completes to that branch name, you would not dream of complaining
> about the completion. IOW, I think the complaint is somewhat unfair
> to begin with.
>
> Actually, I do not know if the completion script really expands an
> abbreviated object name to a full one. I tried:
>
> $ git rev-parse seen^2
> 179eccf0d01729c19a3238905b951b1880aa4ba1
> $ git checkout master
> $ . contrib/completion/git-completion.bash
> $ git send-email 17<TAB>
>
> and waited for some time, but it did not complete to anything.
>
> In any case, when both a '0001-my-changes.patch' file and a
> '0-tolerance-policy' branch exist in your repository and current
> working directory, running:
>
> $ git send-email 0<TAB>
>
> should offer both as candidates, I thihk. Since I only ever pass
> filenames to the command, I personally do not think it is a huge
> loss if the completion script stops looking at refs and sticks to
> filenames only, but others may have a use for that feature.
Agree. The test should create a file 0001.patch, then a tag
0-tag, then a branch 0-branch, maybe something else that is
relevant; and then make sure every option is correctly offered
by autocompletion.
Guys please let me know if everything else is needed before I send v2.
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH 0/1] rebase: add --[no-]edit to --continue
From: Junio C Hamano @ 2026-07-21 18:04 UTC (permalink / raw)
To: Hugo Sales; +Cc: git
In-Reply-To: <20260721140443.1809379-1-hugo@hsal.es>
Hugo Sales <hugo@hsal.es> writes:
> When a rebase stops for conflicts and the user runs `git rebase --continue`, the
> merge backend opens $EDITOR so the commit message can be revised. That is often
> useful, but not always: sometimes the user only wants to keep the message that
> is already there.
>
> This series adds:
>
> - `git rebase --continue --no-edit` to commit without opening an editor
Meh. "GIT_SEQUENCE_EDITOR=: git rebase --continue" is your friend ;-)
> - `rebase.noEdit` to make that the default on continue
> - `git rebase --continue --edit` to override `rebase.noEdit`
The new 'rebase.noEdit' configuration is especially concerning. It
encourages users to assume by default that their rebase sessions
will not produce notable changes worth recording in the commit logs.
A few immediate edge cases come to mind:
- What if 'rebase -i' stops to give control back to the user for
reasons other than a merge conflict? If the user chose 'edit',
their original intent was to modify both the commit message and
the content. With 'rebase.noEdit' enabled, would they now have
to remember to pass '--edit' when continuing? Does the answer
depend on whether the 'edit' step resulted in a merge conflict?
- What if the user chose 'reword', which is an explicit signal to
update the commit message, but 'rebase.noEdit' is enabled? If
the rebase does not stop with a conflict, it might open the
editor regardless of the configuration. But if a conflict does
occur and requires manual resolution, will the user still need to
remember to pass '--edit' when continuing?
The proposed tests only cover the code path where control returns to
the user due to a conflict. This is understandable since that
scenario was your primary motivation. However, they do not verify
what happens when there are no conflicts but the user explicitly
intended to edit the message from the start. You may want to expand
the test coverage to address these scenarios (and potentially
others, as this is not an exhaustive list).
Thanks.
^ permalink raw reply
* Re: [PATCH] t0213: skip ancestry tests under user-mode emulation
From: Weijie Yuan @ 2026-07-21 18:06 UTC (permalink / raw)
To: Jamie Magee via GitGitGadget; +Cc: git, Jamie Magee, Junio C Hamano
In-Reply-To: <pull.2168.git.1783359242130.gitgitgadget@gmail.com>
On Mon, Jul 06, 2026 at 05:34:01PM +0000, Jamie Magee via GitGitGadget wrote:
> From: Jamie Magee <jamie.magee@gmail.com>
>
> The tests added in 3c8c638df6 (t0213: add trace2 cmd_ancestry tests,
> 2026-02-13) expect the cmd_ancestry event to name "test-tool" and
> "git". On Linux those names come from the "comm" field of
> /proc/<pid>/stat. Under user-mode emulation (e.g. qemu-user) /proc
> reports the emulator ("qemu-riscv64") instead, so the event is still
> emitted, the TRACE2_ANCESTRY probe enables the tests, and tests 2-5
> fail even though they pass on native riscv64.
>
> Require the probe to see "test-tool" in the ancestry of a test-tool
> spawned from test-tool, so the tests skip when the names are unreliable.
>
> Cc: Matthew John Cheetham <mjcheetham@outlook.com>
> Signed-off-by: Jamie Magee <jamie.magee@gmail.com>
> ---
Very sorry to say something completely outside the patch.
But may I ask what's the point of writting the line started with "Cc:"?
I know that Linux kernel has something about writting Cc in the commit
message, while I don't see much from Git's documentation about trailers,
including MyFirstContribution and SubmittingPatches. Although I know
that "git send-email" and "git interpret-trailers" could use "Cc:", I
still wonder whether using this trailer is encouraged or not. If so,
adding some decriptions about it would be nice, I guess, since I was
confused when reading kernel docs about using Cc trailers in the commit
message sometime ago.
Sorry in advance if I missed any context.
Thanks very much.
^ permalink raw reply
* Re: [PATCH v9] show-branch: convert per-branch flags to commit-slab
From: Junio C Hamano @ 2026-07-21 18:51 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260717224435.74504-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> I took time to read through the code carefully before sending this.
Hmph. I hate to say this, but I am finding it difficult to trust
your "carefully" at this point.
$ make
$ ./git show-branch master next
Floating point exception (core dumped).
While I have not spent the time to exhaustively find all bugs in
this code [*], it is disturbing that the simplest use of the command
immediately crashes. The reason for this crash is trivial.
> static struct commit_name_slab name_slab;
>
> +define_commit_slab(commit_rev_flags, uint64_t);
> +static struct commit_rev_flags rev_flags_slab;
> +static int flags_stride; /* number of uint64_t words per commit */
> +
> static struct commit_name *commit_to_name(struct commit *commit)
> {
> return *commit_name_slab_at(&name_slab, commit);
> }
The code still uses name_slab to associate names with commits, and
commit_to_name() is called by functions like name_commits(), which
is used in cmd_show_branch(). Yet, the patch does this:
> @@ -713,8 +773,6 @@ int cmd_show_branch(int ac,
> const char **args_copy = NULL;
> int ret;
>
> - init_commit_name_slab(&name_slab);
> -
The code simply discards the initialization for that slab, leaving
name_slab BSS-initialized. Consequently, all members in the struct
are initialized to 0, including the '.slab_size' member.
This init_commit_name_slab() call is not moved elsewhere; it simply
disappeared without an explanation.
When commit_to_name() tries to look up a commit in the slab, it
first attempts to determine which slab should contain the data for
the commit, using this code from <commit-slab-impl.h>:
scope elemtype *slabname## _at_peek(struct slabname *s, \
const struct commit *c, \
int add_if_missing) \
{ \
unsigned int nth_slab, nth_slot; \
\
nth_slab = c->index / s->slab_size; \
nth_slot = c->index % s->slab_size; \
\
And we all know what would happen when you divide by zero.
[Footnote]
* ... and I will not.
^ permalink raw reply
* Re: [PATCH] completion: complete paths for git send-email
From: Junio C Hamano @ 2026-07-21 19:22 UTC (permalink / raw)
To: Yury Norov
Cc: D. Ben Knoble, Yury Norov (NVIDIA), git, Thiago Perrotta,
Philippe Blain, Rubén Justo, linux-kernel, Codex
In-Reply-To: <al-0ckPhoa-ZPhSi@yury>
Yury Norov <ynorov@nvidia.com> writes:
>> In any case, when both a '0001-my-changes.patch' file and a
>> '0-tolerance-policy' branch exist in your repository and current
>> working directory, running:
>>
>> $ git send-email 0<TAB>
>>
>> should offer both as candidates, I thihk. Since I only ever pass
>> filenames to the command, I personally do not think it is a huge
>> loss if the completion script stops looking at refs and sticks to
>> filenames only, but others may have a use for that feature.
>
> Agree. The test should create a file 0001.patch, then a tag
> 0-tag, then a branch 0-branch, maybe something else that is
> relevant; and then make sure every option is correctly offered
> by autocompletion.
>
> Guys please let me know if everything else is needed before I send v2.
So in short, we want the problem description updated to something
like:
When branches and tags whose names share the same prefix as a
file (or a directory???) that stores a patch exist, the attempt
to complete that shared prefix
$ git send-email that-shared-prefix<TAB>
should offer both branches, tags, and files (and directories???).
But the completion only offers branches and tags and fails to
offer files.
And the description of the solution would follow after that in the
proposed log message.
As to the tests, using 40-hex is misleading, and 0-branch as you
said would be sufficient to reproduce and demonstrate the issue, and
that your code change fixes it.
Ben, anything I missed?
Thanks.
^ permalink raw reply
* Re: [PATCH v2] userdiff: add support for Swift
From: Junio C Hamano @ 2026-07-21 19:33 UTC (permalink / raw)
To: Shlok Kulshreshtha
Cc: git, Johannes Sixt, D . Ben Knoble, René Scharfe,
Eric Sunshine, Scott L . Burson
In-Reply-To: <20260721065736.8747-1-diy2903@gmail.com>
Shlok Kulshreshtha <diy2903@gmail.com> writes:
> Add a built-in userdiff driver for the Swift programming language so that
> diff hunk headers and word diffs work out of the box for ".swift" files.
>
> The funcname pattern is built for Swift's own declaration grammar: an
> optional run of attributes ("@objc", "@available(iOS 13, *)", ...),
> followed by an optional run of lowercase modifiers ("public", "static",
> "final", ...), followed by a declaration keyword (func, class, struct,
> enum, protocol, extension, actor, init, deinit, subscript). The keyword
> is followed by a boundary that allows whitespace, "(" (init/subscript),
> "?" or "!" (failable init), or "<" (generics), while still acting as a
> word boundary so e.g. "initialize(" does not match.
>
> The word regex recognizes Swift identifiers, hexadecimal, octal, binary,
> integer and floating-point literals, and the language's operators.
>
> Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
> ---
> v2, addressing Johannes Sixt's review of v1
> (<2a3a73c5-5e90-44a3-bf6a-6e98ce5e5a59@kdbg.org>). Changes since v1:
>
> - t4018/swift-{init,failable-init,generic-subscript}: "RIGHT" now
> appears only once, on the declaration line, so the expected header is
> unambiguous.
> - word regex: dropped the redundant "?" after the single-character
> operator class. Single characters are already covered by the
> "|[^[:space:]]" fallback that the PATTERNS macro appends, so only the
> two-character forms need to be spelled out.
>
> (A couple of Hannes's other suggestions I kept as-is; I have explained
> the reasoning in a reply to his review.)
Thanks for an update.
Let's wait for a few days to see if we hear more comments and
otherwise mark the topic for 'next'.
^ permalink raw reply
* Re: [PATCH 2/2] stash: avoid sparse-index expansion for in-cone paths
From: Junio C Hamano @ 2026-07-21 19:34 UTC (permalink / raw)
To: Taylor Blau
Cc: tnyman, git, Derrick Stolee, Taylor Blau, Jeff King, Victoria Dye
In-Reply-To: <al61UTM0aK9j9eiP@com-79390>
Taylor Blau <ttaylorr@openai.com> writes:
> On Mon, Jul 20, 2026 at 03:31:21PM -0700, tnyman@openai.com wrote:
>> Signed-off-by: Ted Nyman <tnyman@openai.com>
>> ---
>> builtin/stash.c | 4 +-
>> t/perf/p2000-sparse-operations.sh | 1 +
>> t/t1092-sparse-checkout-compatibility.sh | 55 ++++++++++++++++++++++++
>> 3 files changed, 58 insertions(+), 2 deletions(-)
>
> All looks reasonable, and it's very nice indeed to see another one of
> these /* TODO */ comments go away ;-).
>
> Very pleasant read, this series is
>
> Reviewed-by: Taylor Blau <ttaylorr@openai.com>
>
> , and looks good to me.
>
> Thanks,
> Taylor
Thanks, both of you. Let me mark the topic for 'next'.
^ permalink raw reply
* Re: [PATCH v2 1/2] http: avoid concurrent appends to partial packs
From: Junio C Hamano @ 2026-07-21 19:56 UTC (permalink / raw)
To: Ted Nyman; +Cc: git, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <160a9b9fd0982dadfbf6f8fbb378d1a3e9173698.1784582665.git.tnyman@openai.com>
Ted Nyman <tnyman@openai.com> writes:
> Pack requests stage downloads in a predictable partial-pack file so an
> interrupted transfer can be resumed. Both packfile URI and ordinary dumb
> HTTP requests use this staging path. Opening it in append mode lets
> concurrent fetches interleave their writes, corrupting the pack or
> causing a later fetch to request a range at EOF.
>
> Open the partial pack read-write, seek to its current end, and retain a
> per-descriptor offset for incoming data. Reopen newly created partial
> packs without O_CREAT so Windows permits concurrent unlink, and keep the
> descriptor for index-pack when another downloader removes the staging
> path. Accept HTTP 416 when a partial pack is already complete.
>
> Exercise resumed transfers, EOF ranges, and overlapping 200 and 206
> responses. Clarify the staging-key documentation and correct the stale
> --index-pack-args spelling in the documentation and error messages; the
> repeatable --index-pack-arg option is already accepted.
Hmph. So the idea is to allow multiple processes to open the same
file and, because they all know where their respective chunks of
data fit in the final file, have them use pwrite(2) to deposit those
pieces at the exact target locations, and this prevents them from
stepping on each other's toes?
I cannot exactly explain why but it somehow makes me feel dirty.
It is also surprising that the workaround on MinGW works when
one of these multiple processes finishes writing and attempts to
finalize the temporary file while others still have open file
descriptors to the same file.
> - The hash is used to determine the name of the temporary file and is
> - arbitrary. The output of index-pack is printed to stdout. Requires
> - --index-pack-args.
> + The hash is used to determine the name of the temporary file. It need
> + not be the pack hash, but it must uniquely identify the pack contents
> + for resumption. The output of index-pack is printed to stdout. Requires
> + one or more --index-pack-arg options.
OK.
> ---index-pack-args=<args>::
> - For internal use only. The command to run on the contents of the
> - downloaded pack. Arguments are URL-encoded separated by spaces.
> +--index-pack-arg=<arg>::
> + For internal use only. An argument to the command run on the contents
> + of the downloaded pack. This option can be specified multiple times.
Was the 'internal use only' thing renamed in order to prevent the
new code from accidentally working with an older caller?
... goes and notices that the code uses singular form throughout ...
Ah, no, this is an unrelated typo fix that remains valid even if the
rest of this patch is dropped. Good catch.
It would be easier to review the actual changes if this cleanup were
isolated in a preliminary patch. Are there other cleanup changes in
this series that fall into the same category?
> diff --git a/http-fetch.c b/http-fetch.c
> index f9b6ecb061..05f68f306a 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -70,7 +70,8 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
>
> if (start_active_slot(preq->slot)) {
> run_active_slot(preq->slot);
> - if (results.curl_result != CURLE_OK) {
> + if (results.curl_result != CURLE_OK &&
> + results.http_code != 416) {
We do not seem to use symbolic constants for these '4xx' codes (or
'2xx', for that matter), so I will let that pass. Eventually, we
may want to give symbolic constants to them to improve readability,
but doing so is certainly outside the scope of this topic.
> @@ -155,7 +156,7 @@ int cmd_main(int argc, const char **argv)
>
> if (packfile) {
> if (!index_pack_args.nr)
> - die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-args");
> + die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-arg");
This and ...
> @@ -164,7 +165,7 @@ int cmd_main(int argc, const char **argv)
> }
>
> if (index_pack_args.nr)
> - die(_("the option '%s' requires '%s'"), "--index-pack-args", "--packfile");
> + die(_("the option '%s' requires '%s'"), "--index-pack-arg", "--packfile");
>
> if (commits_on_stdin) {
> commits = walker_targets_stdin(&commit_id, &write_ref);
... this is the same "index-pack-arg" fix and can be moved to a
separate preliminary clean-up patch.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] remote: renamed remote push tracking
From: Harald Nordgren @ 2026-07-21 20:10 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <CALnO6CAY2x-adAxSXW1f_+OHjV_tVhLmkN7D+wE39rj3wc8LEQ@mail.gmail.com>
It's very valid and I need to explain it better if you are confused.
Harald
^ permalink raw reply
* Re: [PATCH GSoC v20 00/13] cat-file: add remote-object-info to batch-command
From: Karthik Nayak @ 2026-07-21 20:12 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6595 bytes --]
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[snip]
> Changes in v20:
> - Moved the prep patch that fixes hash_algo's type to be before
> write_fetch_command_and_capabilities() being moved to 'connect.c'
> - Reverted git-cat-file.adoc documentation comments about CAVEATS
> - Fixed style for EXPAND_DATA_INIT
> - Added more context for comman line die()
>
[snip]
> Range-diff versus v19:
>
> 1: 6df5bcb3bc = 1: 31b02f10b5 transport-helper: fix memory leak of helper on disconnect
> 2: 141d85a76f = 2: 7d62e0586f cat-file: declare loop counter inside for()
> 3: 87457a1fe3 = 3: 082d593fb7 t1006: extract helper functions into new 'lib-cat-file.sh'
> 4: c26d378931 = 4: 8646b9d7cf fetch-pack: drop the static advertise_sid variable
> -: ---------- > 5: 5531a3c399 fetch-pack: use unsigned int for hash_algo variable
> 5: ee16e17228 ! 6: 92b4122642 fetch-pack: move write_fetch_command_and_capabilities() to connect.c
> @@ connect.c: int server_supports(const char *feature)
> + }
> +
> + if (server_feature_v2("object-format", &hash_name)) {
> -+ int hash_algo = hash_algo_by_name(hash_name);
> ++ const unsigned int hash_algo = hash_algo_by_name(hash_name);
> + if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
> + die(_("mismatched algorithms: client %s; server %s"),
> + the_hash_algo->name, hash_name);
> @@ fetch-pack.c: static int add_haves(struct fetch_negotiator *negotiator,
> - }
> -
> - if (server_feature_v2("object-format", &hash_name)) {
> -- int hash_algo = hash_algo_by_name(hash_name);
> +- const unsigned int hash_algo = hash_algo_by_name(hash_name);
> - if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
> - die(_("mismatched algorithms: client %s; server %s"),
> - the_hash_algo->name, hash_name);
> 6: bb915f57ad < -: ---------- connect: use unsigned int for hash_algo_by_name() calls
> 7: b3b06b0cae = 7: b954a5994a connect: make write_fetch_command_and_capabilities() more generic
> 8: 79a87c7011 = 8: 627242a7a1 fetch-pack: move fetch initialization
> 9: 37c3f93975 = 9: 0578594733 protocol-caps: check object existence regardless of the attributes requested
> 10: c5062ecaf2 = 10: 816bfa9162 serve: advertise object-info feature
> 11: 22d72168bc = 11: 2323f45cb2 transport: add client support for object-info
> 12: 2cf3b24a35 ! 12: a39975766b cat-file: add remote-object-info to batch-command
> @@ Documentation/git-cat-file.adoc: one per line, and print information based on th
> You can specify the information shown for each object by using a custom
> `<format>`. The `<format>` is copied literally to stdout for each
> @@ Documentation/git-cat-file.adoc: newline. The available atoms are:
> - reports).
> -
> - `objectsize:disk`::
> -- The size, in bytes, that the object takes up on disk. See the
> -- note about on-disk sizes in the `CAVEATS` section below.
> -+ The size, in bytes, that the object takes up on disk.
> -
> `deltabase`::
> If the object is stored as a delta on-disk, this expands to the
> full hex representation of the delta base object name.
> - Otherwise, expands to the null OID (all zeroes). See `CAVEATS`
> -- below.
> -+ Otherwise, expands to the null OID (all zeroes).
> ++ Otherwise, expands to the null OID (all zeroes). See `CAVEATS` section
> + below.
>
This chagne is still unnecessary, no?
> `rest`::
> - If this atom is used in the output string, input lines are split
> @@ Documentation/git-cat-file.adoc: newline. The available atoms are:
> after that first run of whitespace (i.e., the "rest" of the
> line) are output in place of the `%(rest)` atom.
> @@ builtin/cat-file.c: struct expand_data {
> + */
> + unsigned is_remote:1;
> +};
> ++
> +#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD }
> +
> +static const char *remote_object_info_atoms[] = {
> @@ builtin/cat-file.c: static void parse_cmd_mailmap(struct batch_options *opt UNUS
> + line_to_split = xstrdup(line);
> + count = split_cmdline(line_to_split, &argv);
> + if (count < 0)
> -+ die(_("remote-object-info: %s"), split_cmdline_strerror(count));
> ++ die(_("remote-object-info: failed to parse command line: %s"),
> ++ split_cmdline_strerror(count));
> + if (count - 1 > MAX_ALLOWED_OBJ_LIMIT)
> + die(_("remote-object-info supports at most %d objects"),
> + MAX_ALLOWED_OBJ_LIMIT);
> 13: ebdfc22fb7 ! 13: 70a11d2aea cat-file: make remote-object-info allow-list adapt to the server
> @@ builtin/cat-file.c: struct expand_data {
> */
> unsigned is_remote:1;
> -};
> +-
> -#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD }
>
> -static const char *remote_object_info_atoms[] = {
> - "objectname",
> - "objectsize",
> ++ /*
> ++ * List of atoms (i.e. "objectsize") that the server supports. Built
> ++ * from the server's object-info advertised capabilities.
> ++ */
> + struct string_list remote_allowed_atoms;
> };
> -+#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD, \
> -+ .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
>
> ++#define EXPAND_DATA_INIT { .mode = S_IFINVALID, \
> ++ .type = OBJ_BAD, \
> ++ .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
> ++
I guess this is output of running the style check script (which I did
recommend, but I should've mentioned that you should take it with a
pinch of salt, it doesn't always provide the best solutions.)
> static int is_atom(const char *atom, const char *s, int slen)
> {
> + int alen = strlen(atom);
> @@ builtin/cat-file.c: static int expand_atom(struct strbuf *sb, const char *atom, int len,
> struct expand_data *data)
> {
> @@ builtin/cat-file.c: static int expand_atom(struct strbuf *sb, const char *atom,
> - if (is_atom(remote_object_info_atoms[i], atom, len))
> + size_t i;
> + for (i = 0; i < data->remote_allowed_atoms.nr; i++)
> -+ if (is_atom(data->remote_allowed_atoms.items[i].string, atom, len))
> ++ if (is_atom(data->remote_allowed_atoms.items[i].string,
> ++ atom, len))
> break;
> -
> - /*
>
> ---
> base-commit: 44de1520f08d1dfebc3ab2d9f644208eaa5ac925
I reviewed v18 last, but the diff here looks good to me. Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v3 0/2] remote: url-based pushRemote with renamed remotes
From: Harald Nordgren via GitGitGadget @ 2026-07-21 20:14 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Harald Nordgren
In-Reply-To: <pull.2358.v2.git.git.1784624306.gitgitgadget@gmail.com>
Fix git status not showing the push branch after remotes are renamed, when
branch.<name>.pushRemote is a URL matching exactly one configured remote.
Changes in v4:
* Match against the actual push URL, including pushurl and pushInsteadOf.
* Clarify how rearranging remotes exposes the git status tracking problem.
* Simplify and correct the documentation for URL-valued pushRemote.
Changes in v3:
* Revamp commit messages to clarify motivation.
Changes in v2:
* Clarify that URL push destinations already work and that this change only
restores their tracking information.
* Document URL values for branch.<name>.pushRemote and their @{push}
behavior.
Harald Nordgren (2):
remote: pass repository to push tracking helper
remote: find tracking branches for URL push destinations
Documentation/config/branch.adoc | 1 +
Documentation/revisions.adoc | 3 +
remote.c | 43 +++++++++--
remote.h | 2 +
t/t5505-remote.sh | 124 +++++++++++++++++++++++++++++++
transport.c | 5 +-
6 files changed, 172 insertions(+), 6 deletions(-)
base-commit: 48bbf81c29ca9a4479ec7850fe206518682cdb2f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2358%2FHaraldNordgren%2Fremote-resolve-url-push-tracking-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2358/HaraldNordgren/remote-resolve-url-push-tracking-v3
Pull-Request: https://github.com/git/git/pull/2358
Range-diff vs v2:
1: b1ac49de87 = 1: b1ac49de87 remote: pass repository to push tracking helper
2: 6e924a7fec ! 2: a343af9d50 remote: find tracking branches for URL push destinations
@@ Metadata
## Commit message ##
remote: find tracking branches for URL push destinations
- Git already accepts a repository URL as branch.<name>.pushRemote and
- can push to it. When a configured remote has the same URL, however,
- "git status" cannot show that remote's push branch.
+ Git accepts a repository URL as branch.<name>.pushRemote and can push
+ to it. This branch setting takes precedence over remote.pushDefault.
- This can happen in fork workflows when the original remote is renamed
- to "upstream", the fork is added as "origin", and an existing
- pushRemote value still contains the fork URL. The URL still points to
- the right repository, so pushing works. However, @{push} is unavailable
- because Git does not connect the URL to "origin". As a result,
+ A branch can be configured with a URL-valued pushRemote before any push
+ occurs. If the remotes are later rearranged with "git remote rename" and
+ "git remote add", the newly added remote may use that URL. The URL value
+ is unaffected by the rename and continues to take precedence over
+ remote.pushDefault. The URL and the remote then point to the same
+ repository, but Git does not connect them for tracking. Pushing works,
+ but @{push} cannot identify the remote's tracking branch. As a result,
"git status" cannot show the push branch, and an up-to-date push can
- leave its local tracking information stale.
+ leave its tracking information stale.
- When exactly one configured remote has the URL as one of its
- remote.<name>.url values, use its fetch refspec to find and refresh the
- push branch. Keep the URL as the push destination so the configured
- remote's push settings do not change existing behavior. Keep the
- current behavior when no remote matches or multiple remotes match.
+ When exactly one configured remote uses the push destination URL, use
+ that remote for push tracking. Continue to push to the URL so the
+ configured remote's push settings do not change existing behavior. Keep
+ the current behavior when no remote matches or multiple remotes match.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
@@ Documentation/config/branch.adoc: This option defaults to `never`.
repository), you would want to set `remote.pushDefault` to
specify the remote to push to for all branches, and use this
option to override it for a specific branch.
-+ The value may be the name of a configured remote or a repository
-+ URL. A URL is used directly as the push destination.
++ The value may be the name of a configured remote or a repository URL.
`branch.<name>.merge`::
Defines, together with `branch.<name>.remote`, the upstream branch
@@ Documentation/revisions.adoc: some output processing may assume ref names in UTF
`git push` were run while `branchname` was checked out (or the current
`HEAD` if no branchname is specified). Like for '@\{upstream\}', we report
the remote-tracking branch that corresponds to that branch at the remote.
-+ If the push destination is a URL and exactly one configured remote has the
-+ same `remote.<name>.url`, '@\{push}' reports the remote-tracking branch for
-+ that remote.
++ If the push destination is a URL and exactly one configured remote has
++ that URL among its `remote.<name>.url` values, '@\{push}' reports that
++ remote's remote-tracking branch.
+
Here's an example to make it more clear:
+
@@ remote.c: const char *branch_get_upstream(struct branch *branch, struct strbuf *
+struct remote *repo_remote_for_push_tracking(struct repository *repo,
+ struct remote *remote)
+{
++ const struct strvec *push_urls;
+ struct remote *first_match = NULL;
+ struct remote_state *remote_state = repo->remote_state;
++ const char *check_url;
+
-+ if (remote->origin != REMOTE_UNCONFIGURED || remote->url.nr != 1)
++ if (remote->origin != REMOTE_UNCONFIGURED)
+ return remote;
+
++ push_urls = push_url_of_remote(remote);
++ if (push_urls->nr != 1)
++ return remote;
++ check_url = push_urls->v[0];
++
+ for (int i = 0; i < remote_state->remotes_nr; i++) {
+ struct remote *candidate = remote_state->remotes[i];
+
+ if (!candidate || candidate == remote ||
+ !remote_is_configured(candidate, 0) ||
-+ !remote_has_url(candidate, remote->url.v[0]))
++ !remote_has_url(candidate, check_url))
+ continue;
+ if (first_match)
+ return remote;
@@ t/t5505-remote.sh: setup_repository () {
+ rm -rf fork.git client &&
+ git clone --bare one fork.git &&
+ git clone one client &&
-+ fork_url="$TRASH_DIRECTORY/fork.git" &&
++ fork_url="file://$TRASH_DIRECTORY/fork.git" &&
+ (
+ cd client &&
+ git checkout -b topic --track origin/main &&
@@ t/t5505-remote.sh: test_expect_success 'rename a remote renames repo remote.push
+ EOF
+'
+
-+test_expect_success 'adding fork remote makes URL-valued pushRemote trackable' '
++test_expect_success 'adding matching remote makes URL-valued pushRemote trackable' '
+ setup_url_pushremote &&
+
+ (
@@ t/t5505-remote.sh: test_expect_success 'rename a remote renames repo remote.push
+ EOF
+'
+
++test_expect_success 'pushInsteadOf URL pushRemote is trackable' '
++ setup_url_pushremote &&
++ (
++ cd client &&
++ git remote rename origin upstream &&
++ git remote add -f origin "$fork_url" &&
++ git config "url.$fork_url.pushInsteadOf" fork: &&
++ git config branch.topic.pushRemote fork:
++ ) &&
++
++ check_status <<-EOF
++ On branch topic
++ Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
++
++ Your branch is up to date with ${SQ}origin/topic${SQ}.
++
++ nothing to commit, working tree clean
++ EOF
++'
++
+test_expect_success 'up-to-date URL push refreshes stale tracking branch' '
+ setup_url_pushremote &&
+ (
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 1/2] remote: pass repository to push tracking helper
From: Harald Nordgren via GitGitGadget @ 2026-07-21 20:14 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2358.v3.git.git.1784664859.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
The next commit needs tracking_for_push_dest() to inspect the
repository's configured remotes. Pass the repository through the
existing callers and mark the new parameter as unused.
No change in behavior.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
remote.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/remote.c b/remote.c
index b17648d6ef..0dc36956c3 100644
--- a/remote.c
+++ b/remote.c
@@ -1887,7 +1887,8 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
return branch->merge[0]->dst;
}
-static char *tracking_for_push_dest(struct remote *remote,
+static char *tracking_for_push_dest(struct repository *repo UNUSED,
+ struct remote *remote,
const char *refname,
struct strbuf *err)
{
@@ -1925,13 +1926,13 @@ static char *branch_get_push_1(struct repository *repo,
_("push refspecs for '%s' do not include '%s'"),
remote->name, branch->name);
- ret = tracking_for_push_dest(remote, dst, err);
+ ret = tracking_for_push_dest(repo, remote, dst, err);
free(dst);
return ret;
}
if (remote->mirror)
- return tracking_for_push_dest(remote, branch->refname, err);
+ return tracking_for_push_dest(repo, remote, branch->refname, err);
switch (push_default) {
case PUSH_DEFAULT_NOTHING:
@@ -1939,7 +1940,7 @@ static char *branch_get_push_1(struct repository *repo,
case PUSH_DEFAULT_MATCHING:
case PUSH_DEFAULT_CURRENT:
- return tracking_for_push_dest(remote, branch->refname, err);
+ return tracking_for_push_dest(repo, remote, branch->refname, err);
case PUSH_DEFAULT_UPSTREAM:
return xstrdup_or_null(branch_get_upstream(branch, err));
@@ -1953,7 +1954,7 @@ static char *branch_get_push_1(struct repository *repo,
up = branch_get_upstream(branch, err);
if (!up)
return NULL;
- cur = tracking_for_push_dest(remote, branch->refname, err);
+ cur = tracking_for_push_dest(repo, remote, branch->refname, err);
if (!cur)
return NULL;
if (strcmp(cur, up)) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/2] remote: find tracking branches for URL push destinations
From: Harald Nordgren via GitGitGadget @ 2026-07-21 20:14 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2358.v3.git.git.1784664859.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
Git accepts a repository URL as branch.<name>.pushRemote and can push
to it. This branch setting takes precedence over remote.pushDefault.
A branch can be configured with a URL-valued pushRemote before any push
occurs. If the remotes are later rearranged with "git remote rename" and
"git remote add", the newly added remote may use that URL. The URL value
is unaffected by the rename and continues to take precedence over
remote.pushDefault. The URL and the remote then point to the same
repository, but Git does not connect them for tracking. Pushing works,
but @{push} cannot identify the remote's tracking branch. As a result,
"git status" cannot show the push branch, and an up-to-date push can
leave its tracking information stale.
When exactly one configured remote uses the push destination URL, use
that remote for push tracking. Continue to push to the URL so the
configured remote's push settings do not change existing behavior. Keep
the current behavior when no remote matches or multiple remotes match.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Documentation/config/branch.adoc | 1 +
Documentation/revisions.adoc | 3 +
remote.c | 34 ++++++++-
remote.h | 2 +
t/t5505-remote.sh | 124 +++++++++++++++++++++++++++++++
transport.c | 5 +-
6 files changed, 167 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/branch.adoc b/Documentation/config/branch.adoc
index a4db9fa5c8..5a85fde8de 100644
--- a/Documentation/config/branch.adoc
+++ b/Documentation/config/branch.adoc
@@ -55,6 +55,7 @@ This option defaults to `never`.
repository), you would want to set `remote.pushDefault` to
specify the remote to push to for all branches, and use this
option to override it for a specific branch.
+ The value may be the name of a configured remote or a repository URL.
`branch.<name>.merge`::
Defines, together with `branch.<name>.remote`, the upstream branch
diff --git a/Documentation/revisions.adoc b/Documentation/revisions.adoc
index 6ea6c7cead..78f96fe8b0 100644
--- a/Documentation/revisions.adoc
+++ b/Documentation/revisions.adoc
@@ -127,6 +127,9 @@ some output processing may assume ref names in UTF-8.
`git push` were run while `branchname` was checked out (or the current
`HEAD` if no branchname is specified). Like for '@\{upstream\}', we report
the remote-tracking branch that corresponds to that branch at the remote.
+ If the push destination is a URL and exactly one configured remote has
+ that URL among its `remote.<name>.url` values, '@\{push}' reports that
+ remote's remote-tracking branch.
+
Here's an example to make it more clear:
+
diff --git a/remote.c b/remote.c
index 0dc36956c3..4a29669443 100644
--- a/remote.c
+++ b/remote.c
@@ -1887,13 +1887,45 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
return branch->merge[0]->dst;
}
-static char *tracking_for_push_dest(struct repository *repo UNUSED,
+struct remote *repo_remote_for_push_tracking(struct repository *repo,
+ struct remote *remote)
+{
+ const struct strvec *push_urls;
+ struct remote *first_match = NULL;
+ struct remote_state *remote_state = repo->remote_state;
+ const char *check_url;
+
+ if (remote->origin != REMOTE_UNCONFIGURED)
+ return remote;
+
+ push_urls = push_url_of_remote(remote);
+ if (push_urls->nr != 1)
+ return remote;
+ check_url = push_urls->v[0];
+
+ for (int i = 0; i < remote_state->remotes_nr; i++) {
+ struct remote *candidate = remote_state->remotes[i];
+
+ if (!candidate || candidate == remote ||
+ !remote_is_configured(candidate, 0) ||
+ !remote_has_url(candidate, check_url))
+ continue;
+ if (first_match)
+ return remote;
+ first_match = candidate;
+ }
+
+ return first_match ? first_match : remote;
+}
+
+static char *tracking_for_push_dest(struct repository *repo,
struct remote *remote,
const char *refname,
struct strbuf *err)
{
char *ret;
+ remote = repo_remote_for_push_tracking(repo, remote);
ret = apply_refspecs(&remote->fetch, refname);
if (!ret)
return error_buf(err,
diff --git a/remote.h b/remote.h
index 72a54d84ad..cca02033b9 100644
--- a/remote.h
+++ b/remote.h
@@ -345,6 +345,8 @@ char *remote_ref_for_branch(struct branch *branch, int for_push);
const char *repo_default_remote(struct repository *repo);
const char *repo_remote_from_url(struct repository *repo, const char *url);
+struct remote *repo_remote_for_push_tracking(struct repository *repo,
+ struct remote *remote);
/* returns true if the given branch has merge configuration given. */
int branch_has_merge_config(struct branch *branch);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 6f5e86dede..983aff6552 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -24,6 +24,28 @@ setup_repository () {
)
}
+setup_url_pushremote () {
+ rm -rf fork.git client &&
+ git clone --bare one fork.git &&
+ git clone one client &&
+ fork_url="file://$TRASH_DIRECTORY/fork.git" &&
+ (
+ cd client &&
+ git checkout -b topic --track origin/main &&
+ git commit --allow-empty -m topic-change &&
+ git config push.default current &&
+ git config status.compareBranches "@{upstream} @{push}" &&
+ git config branch.topic.pushRemote "$fork_url" &&
+ git push
+ )
+}
+
+check_status () {
+ git -C client status >actual &&
+ cat >expected &&
+ test_cmp expected actual
+}
+
tokens_match () {
echo "$1" | tr ' ' '\012' | sort | sed -e '/^$/d' >expect &&
echo "$2" | tr ' ' '\012' | sort | sed -e '/^$/d' >actual &&
@@ -1018,6 +1040,108 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but keeps g
)
'
+test_expect_success 'URL-valued pushRemote without matching remote is not trackable' '
+ setup_url_pushremote &&
+
+ check_status <<-EOF
+ On branch topic
+ Your branch is ahead of ${SQ}origin/main${SQ} by 1 commit.
+ (use "git push" to publish your local commits)
+
+ nothing to commit, working tree clean
+ EOF
+'
+
+test_expect_success 'adding matching remote makes URL-valued pushRemote trackable' '
+ setup_url_pushremote &&
+
+ (
+ cd client &&
+ git remote rename origin upstream &&
+ git remote add -f origin "$fork_url"
+ ) &&
+
+ check_status <<-EOF
+ On branch topic
+ Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
+
+ Your branch is up to date with ${SQ}origin/topic${SQ}.
+
+ nothing to commit, working tree clean
+ EOF
+'
+
+test_expect_success 'pushInsteadOf URL pushRemote is trackable' '
+ setup_url_pushremote &&
+ (
+ cd client &&
+ git remote rename origin upstream &&
+ git remote add -f origin "$fork_url" &&
+ git config "url.$fork_url.pushInsteadOf" fork: &&
+ git config branch.topic.pushRemote fork:
+ ) &&
+
+ check_status <<-EOF
+ On branch topic
+ Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
+
+ Your branch is up to date with ${SQ}origin/topic${SQ}.
+
+ nothing to commit, working tree clean
+ EOF
+'
+
+test_expect_success 'up-to-date URL push refreshes stale tracking branch' '
+ setup_url_pushremote &&
+ (
+ cd client &&
+ git remote rename origin upstream &&
+ git remote add -f origin "$fork_url" &&
+ git commit --allow-empty -m another-topic-change &&
+ git -C ../fork.git fetch ../client topic:topic
+ ) &&
+
+ check_status <<-EOF &&
+ On branch topic
+ Your branch is ahead of ${SQ}upstream/main${SQ} by 2 commits.
+
+ Your branch is ahead of ${SQ}origin/topic${SQ} by 1 commit.
+ (use "git push" to publish your local commits)
+
+ nothing to commit, working tree clean
+ EOF
+
+ git -C client push >actual 2>&1 &&
+ test_grep "Everything up-to-date" actual &&
+
+ check_status <<-EOF
+ On branch topic
+ Your branch is ahead of ${SQ}upstream/main${SQ} by 2 commits.
+
+ Your branch is up to date with ${SQ}origin/topic${SQ}.
+
+ nothing to commit, working tree clean
+ EOF
+'
+
+test_expect_success 'duplicate remote URL leaves URL-valued pushRemote ambiguous' '
+ setup_url_pushremote &&
+ (
+ cd client &&
+ git remote rename origin upstream &&
+ git remote add -f origin "$fork_url" &&
+ git remote add duplicate "$fork_url"
+ ) &&
+
+ check_status <<-EOF
+ On branch topic
+ Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
+ (use "git push" to publish your local commits)
+
+ nothing to commit, working tree clean
+ EOF
+'
+
test_expect_success 'rename handles remote without fetch refspec' '
git clone --bare one no-refspec.git &&
# confirm assumption that bare clone does not create refspec
diff --git a/transport.c b/transport.c
index fc144f0aed..30a4ab2cd5 100644
--- a/transport.c
+++ b/transport.c
@@ -1553,8 +1553,11 @@ int transport_push(struct repository *r,
if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
struct ref *ref;
+ struct remote *tracking_remote = repo_remote_for_push_tracking(
+ r, transport->remote);
+
for (ref = remote_refs; ref; ref = ref->next)
- transport_update_tracking_ref(transport->remote, ref, verbose);
+ transport_update_tracking_ref(tracking_remote, ref, verbose);
}
if (porcelain && !push_ret)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v10] show-branch: convert per-branch flags to commit-slab
From: Gatla Vishweshwar Reddy @ 2026-07-21 20:30 UTC (permalink / raw)
To: gitster; +Cc: git, Gatla Vishweshwar Reddy
In-Reply-To: <xmqqzezkw4zq.fsf@gitster.g>
show-branch uses commit->object.flags to store per-branch
reachability bits, one bit per branch starting at REV_SHIFT.
The flags word has only a fixed number of available bits, limiting
the number of branches that can be shown simultaneously.
Convert the per-branch bits to a dedicated commit-slab using uint64_t
as the element type, initialized with a stride via
init_commit_rev_flags_with_stride(). Keep the UNINTERESTING bit in
object.flags where it belongs, as it is used for revision walking and
does not need to be in the per-branch slab. With UNINTERESTING removed
from the slab, REV_SHIFT becomes 0 and all 64 bits of uint64_t are
available for branch tracking.
Add helper functions get_rev_flags_ptr(), peek_rev_flags_ptr(),
has_any_rev_flags(), or_rev_flag_bit(), test_rev_flag_bit(),
has_all_rev_flags(), has_only_rev_flag_bit(), and
has_subset_rev_flags() to encapsulate per-bit slab access cleanly.
Use has_only_rev_flag_bit() in show_independent() to preserve the
original semantics: a commit is independent only if reachable from
exactly one tip. Use has_subset_rev_flags() in join_revs() to skip
parents whose flags already include all of the current commit's flags,
avoiding redundant propagation and queue thrashing in dense histories.
Fix join_revs() to correctly propagate UNINTERESTING to parents using
a local commit_is_merge_base variable, without smudging the commit
itself, matching the original behavior.
Remove the hardcoded MAX_REVS limit entirely by dynamically growing
the ref_name[] array with ALLOC_GROW, and allocating rev[] and
reflog_msg[] dynamically after all refs are collected. Compute
flags_stride at runtime as (ref_name_cnt + 63) / 64 so the slab
supports an arbitrary number of branches.
Remove the hardcoded branch limit from documentation since there is
no longer a fixed maximum. Add tests to verify show-branch works
correctly with more than 27 branches. Include revision.h for the
shared UNINTERESTING definition.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
> Hmph. I hate to say this, but I am finding it difficult to trust
> your "carefully" at this point.
>
> $ make
> $ ./git show-branch master next
> Floating point exception (core dumped).
You are right to not trust it. I missed this completely. I ran the
full test suite but did not run the binary manually before sending.
That was the wrong approach. I have now run every mode manually
before sending this version.
> The code simply discards the initialization for that slab, leaving
> name_slab BSS-initialized. Consequently, all members in the struct
> are initialized to 0, including the '.slab_size' member.
>
> This init_commit_name_slab() call is not moved elsewhere; it simply
> disappeared without an explanation.
>
> When commit_to_name() tries to look up a commit in the slab, it
> first attempts to determine which slab should contain the data for
> the commit... And we all know what would happen when you divide by zero.
Fixed. init_commit_name_slab(&name_slab) is restored before
repo_config(). It was removed in v7 without being moved elsewhere —
a clear oversight that I should have caught by running the binary.
As said above I ran every mode manually this time:
$ ./git show-branch master
$ ./git show-branch -a
$ ./git show-branch --merge-base master origin/next
$ ./git show-branch --independent master origin/next
$ ./git show-branch --reflog master
$ ./git show-branch --list master origin/next
$ ./git show-branch --topics master origin/next
$ make -C t t3202-show-branch.sh
All tests pass. No crashes in any mode.
---
Changes in v10:
- Restore init_commit_name_slab(&name_slab) before repo_config()
that was accidentally dropped in v7. Without it, name_slab.slab_size
is 0 causing division by zero on first commit lookup.
Documentation/git-show-branch.adoc | 2 -
builtin/show-branch.c | 236 ++++++++++++++++++-----------
t/t3202-show-branch.sh | 32 ++++
3 files changed, 179 insertions(+), 91 deletions(-)
diff --git a/Documentation/git-show-branch.adoc b/Documentation/git-show-branch.adoc
index 7e86d54a24..3df107b7f4 100644
--- a/Documentation/git-show-branch.adoc
+++ b/Documentation/git-show-branch.adoc
@@ -22,8 +22,6 @@ Shows the commit ancestry graph starting from the commits named
with <rev>s or <glob>s (or all refs under refs/heads
and/or refs/tags) semi-visually.
-It cannot show more than 26 branches and commits at a time.
-
It uses `showbranch.default` multi-valued configuration items if
no <rev> or <glob> is given on the command line.
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f02831b085..4f02f0678e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -9,6 +9,7 @@
#include "hex.h"
#include "pretty.h"
#include "refs.h"
+#include "revision.h"
#include "color.h"
#include "strvec.h"
#include "object-name.h"
@@ -35,15 +36,11 @@ static enum git_colorbool showbranch_use_color = GIT_COLOR_UNKNOWN;
static struct strvec default_args = STRVEC_INIT;
/*
- * TODO: convert this use of commit->object.flags to commit-slab
- * instead to store a pointer to ref name directly. Then use the same
- * UNINTERESTING definition from revision.h here.
+ * TODO: store a pointer to ref name directly in the commit-slab
+ * instead, and use the UNINTERESTING definition from revision.h
+ * here once that is done.
*/
-#define UNINTERESTING 01
-
-#define REV_SHIFT 2
-#define MAX_REVS (FLAG_BITS - REV_SHIFT) /* should not exceed bits_per_int - REV_SHIFT */
-
+#define REV_SHIFT 0
#define DEFAULT_REFLOG 4
static const char *get_color_code(int idx)
@@ -79,11 +76,87 @@ struct commit_name {
define_commit_slab(commit_name_slab, struct commit_name *);
static struct commit_name_slab name_slab;
+define_commit_slab(commit_rev_flags, uint64_t);
+static struct commit_rev_flags rev_flags_slab;
+static int flags_stride; /* number of uint64_t words per commit */
+
static struct commit_name *commit_to_name(struct commit *commit)
{
return *commit_name_slab_at(&name_slab, commit);
}
+static uint64_t *get_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_at(&rev_flags_slab, commit);
+}
+
+static uint64_t *peek_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_peek(&rev_flags_slab, commit);
+}
+
+static int has_any_rev_flags(struct commit *commit)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++)
+ if (f[i])
+ return 1;
+ return 0;
+}
+
+static void or_rev_flag_bit(struct commit *commit, int branch)
+{
+ get_rev_flags_ptr(commit)[branch / 64] |= UINT64_C(1) << (branch % 64);
+}
+
+static int test_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ return f && !!(f[branch / 64] & (UINT64_C(1) << (branch % 64)));
+}
+
+static int has_all_rev_flags(struct commit *commit, int num_rev)
+{
+ int i;
+ for (i = 0; i < num_rev; i++)
+ if (!test_rev_flag_bit(commit, i))
+ return 0;
+ return 1;
+}
+
+static int has_only_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++) {
+ uint64_t expected = (i == branch / 64)
+ ? (UINT64_C(1) << (branch % 64))
+ : 0;
+ if (f[i] != expected)
+ return 0;
+ }
+ return 1;
+}
+
+static int has_subset_rev_flags(struct commit *src, struct commit *dst)
+{
+ uint64_t *s = peek_rev_flags_ptr(src);
+ uint64_t *d = peek_rev_flags_ptr(dst);
+ int i;
+ if (!s)
+ return 1; /* src has no flags, trivially a subset */
+ if (!d)
+ return 0; /* dst has no flags but src has some */
+ for (i = 0; i < flags_stride; i++)
+ if (s[i] & ~d[i])
+ return 0;
+ return 1;
+}
/* Name the commit as nth generation ancestor of head_name;
* we count only the first-parent relationship for naming purposes.
@@ -215,7 +288,7 @@ static void name_commits(struct commit_list *list,
static int mark_seen(struct commit *commit, struct commit_list **seen_p)
{
- if (!commit->object.flags) {
+ if (!has_any_rev_flags(commit)) {
commit_list_insert(commit, seen_p);
return 1;
}
@@ -226,39 +299,41 @@ static void join_revs(struct prio_queue *queue,
struct commit_list **seen_p,
int num_rev, int extra)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
while (queue->nr) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
struct commit *commit = prio_queue_peek(queue);
bool get_pending = true;
- int flags = commit->object.flags & all_mask;
if (!still_interesting && extra <= 0)
break;
mark_seen(commit, seen_p);
- if ((flags & all_revs) == all_revs)
- flags |= UNINTERESTING;
- parents = commit->parents;
-
- while (parents) {
- struct commit *p = parents->item;
- int this_flag = p->object.flags;
- parents = parents->next;
- if ((this_flag & flags) == flags)
- continue;
- repo_parse_commit(the_repository, p);
- if (mark_seen(p, seen_p) && !still_interesting)
- extra--;
- p->object.flags |= flags;
- if (get_pending)
- prio_queue_replace(queue, p);
- else
- prio_queue_put(queue, p);
- get_pending = false;
+ {
+ int commit_is_merge_base = has_all_rev_flags(commit, num_rev);
+ parents = commit->parents;
+
+ while (parents) {
+ struct commit *p = parents->item;
+ int _b;
+ parents = parents->next;
+ if (has_subset_rev_flags(commit, p) &&
+ (!commit_is_merge_base || (p->object.flags & UNINTERESTING)))
+ continue;
+ repo_parse_commit(the_repository, p);
+ if (mark_seen(p, seen_p) && !still_interesting)
+ extra--;
+ for (_b = 0; _b < num_rev; _b++)
+ if (test_rev_flag_bit(commit, _b))
+ or_rev_flag_bit(p, _b);
+ if (commit_is_merge_base)
+ p->object.flags |= UNINTERESTING;
+ if (get_pending)
+ prio_queue_replace(queue, p);
+ else
+ prio_queue_put(queue, p);
+ get_pending = false;
+ }
}
if (get_pending)
prio_queue_get(queue);
@@ -278,7 +353,7 @@ static void join_revs(struct prio_queue *queue,
struct commit *c = s->item;
struct commit_list *parents;
- if (((c->object.flags & all_revs) != all_revs) &&
+ if (!has_all_rev_flags(c, num_rev) &&
!(c->object.flags & UNINTERESTING))
continue;
@@ -335,8 +410,9 @@ static void show_one_commit(struct commit *commit, int no_name)
strbuf_release(&pretty);
}
-static char *ref_name[MAX_REVS + 1];
+static char **ref_name;
static int ref_name_cnt;
+static int ref_name_alloc;
static const char *find_digit_prefix(const char *s, int *v)
{
@@ -409,12 +485,7 @@ static int append_ref(const char *refname, const struct object_id *oid,
if (!strcmp(refname, ref_name[i]))
return 0;
}
- if (MAX_REVS <= ref_name_cnt) {
- warning(Q_("ignoring %s; cannot handle more than %d ref",
- "ignoring %s; cannot handle more than %d refs",
- MAX_REVS), refname, MAX_REVS);
- return 0;
- }
+ ALLOC_GROW(ref_name, ref_name_cnt + 2, ref_name_alloc);
ref_name[ref_name_cnt++] = xstrdup(refname);
ref_name[ref_name_cnt] = NULL;
return 0;
@@ -511,15 +582,12 @@ static int rev_is_head(const char *head, const char *name)
static int show_merge_base(const struct commit_list *seen, int num_rev)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
int exit_status = 1;
for (const struct commit_list *s = seen; s; s = s->next) {
struct commit *commit = s->item;
- int flags = commit->object.flags & all_mask;
- if (!(flags & UNINTERESTING) &&
- ((flags & all_revs) == all_revs)) {
+ if (!(commit->object.flags & UNINTERESTING) &&
+ has_all_rev_flags(commit, num_rev)) {
puts(oid_to_hex(&commit->object.oid));
exit_status = 0;
commit->object.flags |= UNINTERESTING;
@@ -528,17 +596,13 @@ static int show_merge_base(const struct commit_list *seen, int num_rev)
return exit_status;
}
-static int show_independent(struct commit **rev,
- int num_rev,
- unsigned int *rev_mask)
+static int show_independent(struct commit **rev, int num_rev)
{
int i;
for (i = 0; i < num_rev; i++) {
struct commit *commit = rev[i];
- unsigned int flag = rev_mask[i];
-
- if (commit->object.flags == flag)
+ if (has_only_rev_flag_bit(commit, i))
puts(oid_to_hex(&commit->object.oid));
commit->object.flags |= UNINTERESTING;
}
@@ -560,8 +624,7 @@ static void append_one_rev(const char *av)
match_ref_slash = count_slashes(av);
refs_for_each_ref(get_main_ref_store(the_repository),
append_matching_ref, NULL);
- if (saved_matches == ref_name_cnt &&
- ref_name_cnt < MAX_REVS)
+ if (saved_matches == ref_name_cnt)
error(_("no matching refs with %s"), av);
sort_ref_range(saved_matches, ref_name_cnt);
return;
@@ -603,13 +666,12 @@ static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
* Otherwise, if it is a merge that is reachable from only one
* tip, it is not that interesting.
*/
- int i, flag, count;
+ int i, count;
for (i = 0; i < n; i++)
if (rev[i] == commit)
return 0;
- flag = commit->object.flags;
for (i = count = 0; i < n; i++) {
- if (flag & (1u << (i + REV_SHIFT)))
+ if (test_rev_flag_bit(commit, i))
count++;
}
if (count == 1)
@@ -644,14 +706,12 @@ int cmd_show_branch(int ac,
const char *prefix,
struct repository *repo UNUSED)
{
- struct commit *rev[MAX_REVS], *commit;
- char *reflog_msg[MAX_REVS] = {0};
+ struct commit **rev = NULL, *commit;
+ char **reflog_msg = NULL;
struct commit_list *seen = NULL;
struct prio_queue queue = { compare_commits_by_commit_date };
- unsigned int rev_mask[MAX_REVS];
int num_rev, i, extra = 0;
int all_heads = 0, all_remotes = 0;
- int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
char *head;
struct object_id head_oid;
@@ -712,9 +772,7 @@ int cmd_show_branch(int ac,
};
const char **args_copy = NULL;
int ret;
-
init_commit_name_slab(&name_slab);
-
repo_config(the_repository, git_show_branch_config, NULL);
/* If nothing is specified, try the default first */
@@ -728,6 +786,8 @@ int cmd_show_branch(int ac,
show_branch_usage, PARSE_OPT_STOP_AT_NON_OPTION);
if (all_heads)
all_remotes = 1;
+ if (reflog)
+ CALLOC_ARRAY(reflog_msg, reflog + 1);
if (extra || reflog) {
/* "listing" mode is incompatible with
@@ -754,7 +814,6 @@ int cmd_show_branch(int ac,
/* If nothing is specified, show all branches by default */
if (ac <= topics && all_heads + all_remotes == 0)
all_heads = 1;
-
if (reflog) {
struct object_id oid;
char *ref;
@@ -777,11 +836,6 @@ int cmd_show_branch(int ac,
}
if (ac != 1)
die(_("--reflog option needs one branch name"));
-
- if (MAX_REVS < reflog)
- die(Q_("only %d entry can be shown at one time.",
- "only %d entries can be shown at one time.",
- MAX_REVS), MAX_REVS);
if (!repo_dwim_ref(the_repository, *av, strlen(*av), &oid,
&ref, 0))
die(_("no such ref %s"), *av);
@@ -868,14 +922,18 @@ int cmd_show_branch(int ac,
goto out;
}
+ flags_stride = (ref_name_cnt + 63) / 64;
+ if (!flags_stride)
+ flags_stride = 1;
+ init_commit_rev_flags_with_stride(&rev_flags_slab, flags_stride);
+ CALLOC_ARRAY(rev, ref_name_cnt);
+ if (!reflog_msg)
+ CALLOC_ARRAY(reflog_msg, ref_name_cnt);
+
for (num_rev = 0; ref_name[num_rev]; num_rev++) {
struct object_id revkey;
- unsigned int flag = 1u << (num_rev + REV_SHIFT);
+ int first_seen;
- if (MAX_REVS <= num_rev)
- die(Q_("cannot handle more than %d rev.",
- "cannot handle more than %d revs.",
- MAX_REVS), MAX_REVS);
if (repo_get_oid(the_repository, ref_name[num_rev], &revkey))
die(_("'%s' is not a valid ref."), ref_name[num_rev]);
commit = lookup_commit_reference(the_repository, &revkey);
@@ -885,17 +943,15 @@ int cmd_show_branch(int ac,
repo_parse_commit(the_repository, commit);
mark_seen(commit, &seen);
- /* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
- * and so on. REV_SHIFT bits from bit 0 are used for
- * internal bookkeeping.
+ /* rev#0 uses bit 0, rev#1 uses bit 1,
+ * and so on. All bits are available for branch tracking.
*/
- commit->object.flags |= flag;
- if (commit->object.flags == flag)
+ first_seen = !has_any_rev_flags(commit);
+ or_rev_flag_bit(commit, num_rev);
+ if (first_seen)
prio_queue_put(&queue, commit);
rev[num_rev] = commit;
}
- for (i = 0; i < num_rev; i++)
- rev_mask[i] = rev[i]->object.flags;
if (0 <= extra)
join_revs(&queue, &seen, num_rev, extra);
@@ -908,7 +964,7 @@ int cmd_show_branch(int ac,
}
if (independent) {
- ret = show_independent(rev, num_rev, rev_mask);
+ ret = show_independent(rev, num_rev);
goto out;
}
@@ -958,13 +1014,9 @@ int cmd_show_branch(int ac,
if (!sha1_name && !no_name)
name_commits(seen, rev, ref_name, num_rev);
- all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
for (struct commit_list *l = seen; l; l = l->next) {
struct commit *commit = l->item;
- int this_flag = commit->object.flags;
- int is_merge_point = ((this_flag & all_revs) == all_revs);
+ int is_merge_point = has_all_rev_flags(commit, num_rev);
shown_merge_point |= is_merge_point;
@@ -973,14 +1025,14 @@ int cmd_show_branch(int ac,
commit->parents->next);
if (topics &&
!is_merge_point &&
- (this_flag & (1u << REV_SHIFT)))
+ test_rev_flag_bit(commit, 0))
continue;
if (!sparse && is_merge &&
omit_in_dense(commit, rev, num_rev))
continue;
for (i = 0; i < num_rev; i++) {
int mark;
- if (!(this_flag & (1u << (i + REV_SHIFT))))
+ if (!test_rev_flag_bit(commit, i))
mark = ' ';
else if (is_merge)
mark = '-';
@@ -1006,10 +1058,16 @@ int cmd_show_branch(int ac,
ret = 0;
out:
- for (size_t i = 0; i < ARRAY_SIZE(reflog_msg); i++)
+ for (i = 0; i < ref_name_cnt; i++)
free(reflog_msg[i]);
+ free(reflog_msg);
+ free(rev);
+ for (i = 0; i < ref_name_cnt; i++)
+ free(ref_name[i]);
+ free(ref_name);
commit_list_free(seen);
clear_prio_queue(&queue);
+ clear_commit_rev_flags(&rev_flags_slab);
free(args_copy);
free(head);
return ret;
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index a1139f79e2..d04f642998 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -283,4 +283,36 @@ test_expect_success '--reflog handles missing reflog' '
test_must_be_empty actual
'
+test_expect_success 'show-branch with 30 branches succeeds' '
+ git checkout initial &&
+ for i in $(test_seq 11 30)
+ do
+ git checkout -b branch$i initial &&
+ test_commit --no-tag branch$i || return 1
+ done &&
+ git show-branch $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --independent with 30 branches' '
+ git show-branch --independent $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --merge-base with 30 branches' '
+ git rev-parse initial >expect &&
+ git show-branch --merge-base $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_cmp expect actual
+'
+
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH] fsmonitor: flush pending FSEvents before cookie wait
From: Tamir Duberstein @ 2026-07-21 21:04 UTC (permalink / raw)
To: git
Cc: Jeff Hostetler, Paul Tarjan, Patrick Steinhardt, Junio C Hamano,
Jeff King, Taylor Blau, Johannes Schindelin, Koji Nakamaru,
Tamir Duberstein
56cef9cb1a (fsmonitor: use pthread_cond_timedwait for cookie wait,
2026-04-15) limits the cookie wait to one second so that a filesystem
which never delivers events cannot hang fsmonitor clients. A client that
times out receives a trivial response and scans the entire index.
FSEvents can defer delivery while it batches notifications and does not
guarantee that its queue is drained in one latency interval. A loaded
macOS system can therefore time out even though the event stream is
working.
On an Apple M4 Max (16 cores, 128 GiB RAM) running macOS 26.5.2, two
worktrees with a 1,001,178-entry index timed out 484 of 545 and 297 of
365 fsmonitor requests. One status call performed 934,519 lstat() calls
during a 47-second preload and took 52 seconds overall.
Ask FSEvents to flush pending notifications after creating the cookie
and before starting the timed wait. Use the asynchronous form because
the client handler holds main_lock, which the listener callback also
acquires. Keep the timeout and the behavior of the other backends
unchanged.
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
builtin/fsmonitor--daemon.c | 3 +++
compat/fsmonitor/fsm-darwin-gcc.h | 1 +
compat/fsmonitor/fsm-listen-darwin.c | 5 +++++
compat/fsmonitor/fsm-listen-linux.c | 4 ++++
compat/fsmonitor/fsm-listen-win32.c | 4 ++++
compat/fsmonitor/fsm-listen.h | 6 ++++++
6 files changed, 23 insertions(+)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 4161dd8282..8e32b5ae5e 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -206,6 +206,9 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(
close(fd);
unlink(cookie_pathname.buf);
+ /* The listener callback takes main_lock, so this must not block. */
+ fsm_listen__flush_async(state);
+
/*
* Wait for the listener thread to observe the cookie file.
* Time out after a short interval so that the client
diff --git a/compat/fsmonitor/fsm-darwin-gcc.h b/compat/fsmonitor/fsm-darwin-gcc.h
index 3496e29b3a..c209dc2f68 100644
--- a/compat/fsmonitor/fsm-darwin-gcc.h
+++ b/compat/fsmonitor/fsm-darwin-gcc.h
@@ -82,6 +82,7 @@ CFRunLoopRef CFRunLoopGetCurrent(void);
extern CFStringRef kCFRunLoopDefaultMode;
void FSEventStreamSetDispatchQueue(FSEventStreamRef stream, dispatch_queue_t q);
unsigned char FSEventStreamStart(FSEventStreamRef stream);
+FSEventStreamEventId FSEventStreamFlushAsync(FSEventStreamRef stream);
void FSEventStreamStop(FSEventStreamRef stream);
void FSEventStreamInvalidate(FSEventStreamRef stream);
void FSEventStreamRelease(FSEventStreamRef stream);
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index 43c3a915a0..64bee248d2 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -496,6 +496,11 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
pthread_mutex_unlock(&data->dq_lock);
}
+void fsm_listen__flush_async(struct fsmonitor_daemon_state *state)
+{
+ FSEventStreamFlushAsync(state->listen_data->stream);
+}
+
void fsm_listen__loop(struct fsmonitor_daemon_state *state)
{
struct fsm_listen_data *data;
diff --git a/compat/fsmonitor/fsm-listen-linux.c b/compat/fsmonitor/fsm-listen-linux.c
index e3dca14b62..7aae29ea22 100644
--- a/compat/fsmonitor/fsm-listen-linux.c
+++ b/compat/fsmonitor/fsm-listen-linux.c
@@ -493,6 +493,10 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
state->listen_data->shutdown = SHUTDOWN_STOP;
}
+void fsm_listen__flush_async(struct fsmonitor_daemon_state *state UNUSED)
+{
+}
+
/*
* Process a single inotify event and queue for publication.
*/
diff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c
index 9a6efc9bea..039d797000 100644
--- a/compat/fsmonitor/fsm-listen-win32.c
+++ b/compat/fsmonitor/fsm-listen-win32.c
@@ -290,6 +290,10 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
SetEvent(state->listen_data->hListener[LISTENER_SHUTDOWN]);
}
+void fsm_listen__flush_async(struct fsmonitor_daemon_state *state UNUSED)
+{
+}
+
static struct one_watch *create_watch(const char *path)
{
struct one_watch *watch = NULL;
diff --git a/compat/fsmonitor/fsm-listen.h b/compat/fsmonitor/fsm-listen.h
index 41650bf897..cfeca1f4b6 100644
--- a/compat/fsmonitor/fsm-listen.h
+++ b/compat/fsmonitor/fsm-listen.h
@@ -38,6 +38,12 @@ void fsm_listen__dtor(struct fsmonitor_daemon_state *state);
*/
void fsm_listen__loop(struct fsmonitor_daemon_state *state);
+/*
+ * Prompt the listener to deliver queued filesystem events, if supported.
+ * This does not wait for the events to be processed.
+ */
+void fsm_listen__flush_async(struct fsmonitor_daemon_state *state);
+
/*
* Gently request that the fsmonitor listener thread shutdown.
* It does not wait for it to stop. The caller should do a JOIN
---
base-commit: 5d2e7709234afea1b6ddb25cd4f60d3d5fb3c200
change-id: 20260721-fsmonitor-darwin-cookie-flush-0f0d6e554a56
^ permalink raw reply related
* [PATCH] submodule: resolve insteadof-aliases when matching remote
From: Éric NICOLAS @ 2026-07-21 21:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller, Éric NICOLAS
When ca62f524c1 introduced a mechanism to identify which remote is to be
used by a submodule, we had it compare the URL stored in the .gitmodules
inventory to those of each available remote.
However, when using URL aliasing via url.<base>.insteadOf, we store
in .gitmodules the URL pre-resolution of the alias, whereas the
corresponding remote set up in the submodule reports using the
*resolved* URL. This mechanism therefore fails to find a match then,
and resorts to the fallback logic, which does use either the only
configured remote if there is only one, or attempts using "origin"
otherwise.
Resolve the alias in the URL inventoried in .gitmodules before comparing
it against those of the corresponding submodule's configured remotes.
Signed-off-by: Éric NICOLAS <ccjmne@gmail.com>
---
remote.c | 15 ++++++++++++---
t/t7406-submodule-update.sh | 21 +++++++++++++++++++++
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index b17648d6ef..ae187fb3d6 100644
--- a/remote.c
+++ b/remote.c
@@ -1821,17 +1821,26 @@ const char *repo_default_remote(struct repository *repo)
const char *repo_remote_from_url(struct repository *repo, const char *url)
{
+ char *rewritten_url;
+ const char *url_to_match;
+ const char *remote_name = NULL;
+
read_config(repo, 0);
+ rewritten_url = alias_url(url, &repo->remote_state->rewrites);
+ url_to_match = rewritten_url ? rewritten_url : url;
for (int i = 0; i < repo->remote_state->remotes_nr; i++) {
struct remote *remote = repo->remote_state->remotes[i];
if (!remote)
continue;
- if (remote_has_url(remote, url))
- return remote->name;
+ if (remote_has_url(remote, url_to_match)) {
+ remote_name = remote->name;
+ break;
+ }
}
- return NULL;
+ free(rewritten_url);
+ return remote_name;
}
int branch_has_merge_config(struct branch *branch)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9554720152..84e2cbbef9 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -256,6 +256,27 @@ test_expect_success 'submodule update --remote should fetch upstream changes' '
)
'
+test_expect_success 'submodule update --remote resolves URL rewrites' '
+ test_config_global "url.$(pwd)/.insteadOf" local: &&
+ mkdir aliased-super aliased-submodule &&
+ (
+ cd aliased-submodule &&
+ git init &&
+ echo line >file &&
+ git add file &&
+ git commit -m "Initial commit"
+ ) &&
+ (
+ cd aliased-super &&
+ git init &&
+ git submodule add local:aliased-submodule submodule &&
+ git submodule update --force submodule &&
+ git -C submodule remote rename origin upstream &&
+ git -C submodule remote add fork user@host &&
+ git submodule update --remote submodule
+ )
+'
+
test_expect_success 'submodule update --remote should fetch upstream changes with .' '
(
cd super &&
--
2.55.0
^ permalink raw reply related
* Re: [PATCH] t0213: skip ancestry tests under user-mode emulation
From: Junio C Hamano @ 2026-07-21 21:55 UTC (permalink / raw)
To: Weijie Yuan; +Cc: Jamie Magee via GitGitGadget, git, Jamie Magee
In-Reply-To: <al-1JkcH91aW6VWM@wyuan.org>
Weijie Yuan <wy@wyuan.org> writes:
>> ...
>> Cc: Matthew John Cheetham <mjcheetham@outlook.com>
>> Signed-off-by: Jamie Magee <jamie.magee@gmail.com>
>> ---
>
> Very sorry to say something completely outside the patch.
>
> But may I ask what's the point of writting the line started with "Cc:"?
> ...
> I know that Linux kernel has something about writting Cc in the commit
> message, while I don't see much from Git's documentation about trailers,
> including MyFirstContribution and SubmittingPatches.
If you ask me, 'Cc:' belongs in e-mail headers, not in commit
messages, though the Linux kernel community has a different
convention.
GitGitGadget collects 'Cc:' lines from the commit message and, when
sending e-mails on behalf of the author, copies the recipients
listed there, if I am not mistaken. Thus, it is not surprising that
contributors use the trailer for that purpose.
We do not use the 'Cc:' trailer to allow a commit author to say, "As
the commit object indicates, I CC'd this change to that expert. I
am no longer solely responsible for any bugs in this commit. That
expert should have caught my mistake!" ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH 5/5] use repo_hold_lock_file_for_update{,_mode,_timeout}() with custom repos
From: Patrick Steinhardt @ 2026-07-21 22:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
In-Reply-To: <xmqqmrvmn6a5.fsf@gitster.g>
On Sun, Jul 19, 2026 at 12:11:46PM -0700, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > On 7/15/26 11:52 AM, Patrick Steinhardt wrote:
> >> On Tue, Jul 14, 2026 at 07:59:56PM +0200, René Scharfe wrote:
> >>> Apply the config setting core.sharedRepository from the repository at
> >>> hand instead of from the_repository.
> >>
> >> We only do this for a subset of callsites, apparently. How did you
> >> select which subsystems to convert and which not to? To make this
> >> explicit: I don't mind a partial migration, but I think the commit
> >> message should briefly explain the reasoning behind it.
> >
> > All those that have a repository reference other than the_repository.
> >
> >> Also, as you don't get rid of the old functions that still implicitly
> >> depend on `the_repository`, I think we should have an additional commit
> >> on top that guards all functions that have this implicit dependency with
> >> `USE_THE_REPOSITORY_VARIABLE`. This ensures that we cannot accidentally
> >> call such functions from other subsystems that already got rid of the
> >> global dependency.
> >
> > Probably, but the lockfile conversions deserve their own patch series.
> > Patch 5 is only included here because it was easy to write. We can drop
> > it and leave the low-hanging fruit on the tree if that's preferable.
>
> I am personally indifferent as to what we do immediately in this
> series, as long as we all agree on the longer-term direction. It
> seems we are in agreement on providing additional safety in the
> medium term?
It would be an easy thing to guard existing interfaces that depend on
`the_repository` behind `USE_THE_REPOSITORY_VARIABLE`. But the patch
series is already a strict improvement over the status quo, so I don't
mind if we merge it as-is and defer that to a later point.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v3 2/2] remote: find tracking branches for URL push destinations
From: Junio C Hamano @ 2026-07-21 22:15 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, D. Ben Knoble, Harald Nordgren
In-Reply-To: <a343af9d500a598826c5fe9a3abbe9df2f5916e8.1784664859.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Git accepts a repository URL as branch.<name>.pushRemote and can push
> to it. This branch setting takes precedence over remote.pushDefault.
>
> A branch can be configured with a URL-valued pushRemote before any push
> occurs. If the remotes are later rearranged with "git remote rename" and
> "git remote add", the newly added remote may use that URL. The URL value
> is unaffected by the rename and continues to take precedence over
> remote.pushDefault. The URL and the remote then point to the same
> repository, but Git does not connect them for tracking. Pushing works,
> but @{push} cannot identify the remote's tracking branch. As a result,
> "git status" cannot show the push branch, and an up-to-date push can
> leave its tracking information stale.
>
> When exactly one configured remote uses the push destination URL, use
> that remote for push tracking. Continue to push to the URL so the
> configured remote's push settings do not change existing behavior. Keep
> the current behavior when no remote matches or multiple remotes match.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> Documentation/config/branch.adoc | 1 +
> Documentation/revisions.adoc | 3 +
> remote.c | 34 ++++++++-
> remote.h | 2 +
> t/t5505-remote.sh | 124 +++++++++++++++++++++++++++++++
> transport.c | 5 +-
> 6 files changed, 167 insertions(+), 2 deletions(-)
Hmph, the changes since the previous round look a bit incoherent.
> diff --git a/Documentation/config/branch.adoc b/Documentation/config/branch.adoc
> index a4db9fa5c8..5a85fde8de 100644
> --- a/Documentation/config/branch.adoc
> +++ b/Documentation/config/branch.adoc
> @@ -55,6 +55,7 @@ This option defaults to `never`.
> repository), you would want to set `remote.pushDefault` to
> specify the remote to push to for all branches, and use this
> option to override it for a specific branch.
> + The value may be the name of a configured remote or a repository URL.
>
> `branch.<name>.merge`::
> Defines, together with `branch.<name>.remote`, the upstream branch
> diff --git a/Documentation/revisions.adoc b/Documentation/revisions.adoc
> index 6ea6c7cead..78f96fe8b0 100644
> --- a/Documentation/revisions.adoc
> +++ b/Documentation/revisions.adoc
> @@ -127,6 +127,9 @@ some output processing may assume ref names in UTF-8.
> `git push` were run while `branchname` was checked out (or the current
> `HEAD` if no branchname is specified). Like for '@\{upstream\}', we report
> the remote-tracking branch that corresponds to that branch at the remote.
> + If the push destination is a URL and exactly one configured remote has
> + that URL among its `remote.<name>.url` values, '@\{push}' reports that
> + remote's remote-tracking branch.
We claim we use remote.<name>.url here.
> Here's an example to make it more clear:
> +
> diff --git a/remote.c b/remote.c
> index 0dc36956c3..4a29669443 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1887,13 +1887,45 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> return branch->merge[0]->dst;
> }
>
> -static char *tracking_for_push_dest(struct repository *repo UNUSED,
> +struct remote *repo_remote_for_push_tracking(struct repository *repo,
> + struct remote *remote)
> +{
> + const struct strvec *push_urls;
> + struct remote *first_match = NULL;
> + struct remote_state *remote_state = repo->remote_state;
> + const char *check_url;
> +
> + if (remote->origin != REMOTE_UNCONFIGURED)
> + return remote;
> +
> + push_urls = push_url_of_remote(remote);
> + if (push_urls->nr != 1)
> + return remote;
> + check_url = push_urls->v[0];
But we correctly pay attention to both .url and .pushurl, giving
precedence to the latter.
> + for (int i = 0; i < remote_state->remotes_nr; i++) {
> + struct remote *candidate = remote_state->remotes[i];
> +
> + if (!candidate || candidate == remote ||
> + !remote_is_configured(candidate, 0) ||
> + !remote_has_url(candidate, check_url))
> + continue;
> + if (first_match)
> + return remote;
> + first_match = candidate;
> + }
> +
> + return first_match ? first_match : remote;
> +}
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 6f5e86dede..983aff6552 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -24,6 +24,28 @@ setup_repository () {
> )
> }
>
> +setup_url_pushremote () {
> + rm -rf fork.git client &&
> + git clone --bare one fork.git &&
> + git clone one client &&
> + fork_url="file://$TRASH_DIRECTORY/fork.git" &&
> + (
> + cd client &&
> + git checkout -b topic --track origin/main &&
> + git commit --allow-empty -m topic-change &&
> + git config push.default current &&
> + git config status.compareBranches "@{upstream} @{push}" &&
> + git config branch.topic.pushRemote "$fork_url" &&
> + git push
> + )
> +}
> +
> +check_status () {
> + git -C client status >actual &&
> + cat >expected &&
> + test_cmp expected actual
> +}
> +
> tokens_match () {
> echo "$1" | tr ' ' '\012' | sort | sed -e '/^$/d' >expect &&
> echo "$2" | tr ' ' '\012' | sort | sed -e '/^$/d' >actual &&
> @@ -1018,6 +1040,108 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but keeps g
> )
> '
>
> +test_expect_success 'URL-valued pushRemote without matching remote is not trackable' '
> + setup_url_pushremote &&
> +
> + check_status <<-EOF
> + On branch topic
> + Your branch is ahead of ${SQ}origin/main${SQ} by 1 commit.
> + (use "git push" to publish your local commits)
> +
> + nothing to commit, working tree clean
> + EOF
> +'
> +
> +test_expect_success 'adding matching remote makes URL-valued pushRemote trackable' '
> + setup_url_pushremote &&
> +
> + (
> + cd client &&
> + git remote rename origin upstream &&
> + git remote add -f origin "$fork_url"
> + ) &&
> +
> + check_status <<-EOF
> + On branch topic
> + Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
> +
> + Your branch is up to date with ${SQ}origin/topic${SQ}.
> +
> + nothing to commit, working tree clean
> + EOF
> +'
But the test does not seem to exercise remote.<name>.pushURL
anywhere.
> +test_expect_success 'pushInsteadOf URL pushRemote is trackable' '
> + setup_url_pushremote &&
> + (
> + cd client &&
> + git remote rename origin upstream &&
> + git remote add -f origin "$fork_url" &&
> + git config "url.$fork_url.pushInsteadOf" fork: &&
> + git config branch.topic.pushRemote fork:
> + ) &&
Testing insteadof is a nice touch, though.
> +test_expect_success 'duplicate remote URL leaves URL-valued pushRemote ambiguous' '
> + setup_url_pushremote &&
> + (
> + cd client &&
> + git remote rename origin upstream &&
> + git remote add -f origin "$fork_url" &&
> + git remote add duplicate "$fork_url"
> + ) &&
> +
> + check_status <<-EOF
> + On branch topic
> + Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
> + (use "git push" to publish your local commits)
> +
> + nothing to commit, working tree clean
> + EOF
> +'
So is a test that checks non-unique case where the machinery should
not kick in.
Thanks.
^ permalink raw reply
* Re: [PATCH] completion: complete paths for git send-email
From: SZEDER Gábor @ 2026-07-21 22:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: D. Ben Knoble, Yury Norov (NVIDIA), git, Thiago Perrotta,
Philippe Blain, Rubén Justo, Yury Norov, linux-kernel, Codex
In-Reply-To: <xmqqcxwgz2u3.fsf@gitster.g>
On Tue, Jul 21, 2026 at 10:09:56AM -0700, Junio C Hamano wrote:
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > On Sun, Jul 19, 2026 at 9:45 AM Yury Norov (NVIDIA)
> > <yury.norov@gmail.com> wrote:
> >>
> >> From: Yury Norov <ynorov@nvidia.com>
> >>
> >> git send-email accepts either revisions or paths to patch files, but its
> >> Bash completion only offers revisions. This prevents patch files from
> >> being completed. It can also make a prefix such as "0" expand to an
> >> unrelated hexadecimal ref even when matching 0001-*.patch files exist.
> >>
> >> In my Linux tree, an attempt to autocomplete the standard-named patch
> >> brings a random hashtag:
> >
> > It is unusual to call this a "hashtag." Perhaps "hash" or "object
> > name" (or id) based on the glossary and datamodel docs?
>
> Very good point, but I am not sure if the author truly meant object
> names here. The reproduction test uses a long hexadecimal string,
> but that is not an object name; it is an unusual-looking tag name.
> It is like naming a topic branch '012345' and complaining that:
>
> $ git send-email 0<TAB>
>
> completes the input to the branch name while ignoring the
> 0001-changes.patch file.
>
> When you have a branch named '0-tolerance-policy' and:
>
> $ git send-email 0<TAB>
>
> completes to that branch name, you would not dream of complaining
> about the completion. IOW, I think the complaint is somewhat unfair
> to begin with.
>
> Actually, I do not know if the completion script really expands an
> abbreviated object name to a full one. I tried:
>
> $ git rev-parse seen^2
> 179eccf0d01729c19a3238905b951b1880aa4ba1
> $ git checkout master
> $ . contrib/completion/git-completion.bash
> $ git send-email 17<TAB>
>
> and waited for some time, but it did not complete to anything.
We definietely don't do that. I'm not sure what the use-case would be
for completing full object names, but considering how many objects a
repo might contain, I doubt it can be usable for anything.
> In any case, when both a '0001-my-changes.patch' file and a
> '0-tolerance-policy' branch exist in your repository and current
> working directory, running:
>
> $ git send-email 0<TAB>
>
> should offer both as candidates, I thihk. Since I only ever pass
> filenames to the command, I personally do not think it is a huge
> loss if the completion script stops looking at refs and sticks to
> filenames only, but others may have a use for that feature.
There are a couple of similar Git commands that accept both refs and
paths, "diff" and "log" being the obvious examples, and our completion
script doesn't list refs and paths for any of them, only refs [1].
I think that's intentional, because:
- It's easier to pick the ref you want from a list containing only
refs than from a list of refs and paths mixed together, because
the list to choose from is shorter, and the unique prefix is
likely shorter as well.
The same goes for picking the path you want from a list containing
only paths.
- Even when our completion script only lists refs for a particular
command, it's easy to trigger Bash's filename completion via one
of the following methods:
- git diff ./foo<TAB> # No ref can start with "./".
- git log foo<ALT-/> # Bash/readline's keybinding to trigger
# filename completion.
- git log -- foo<TAB> # No --options or refs after the
# disambiguating doubledash.
Although I'm not sure "git send-email" supports the disambiguating
doubledash; its completion function surely doesn't.
- There is no similarly easy way to trigger refs completion.
[1] There are a couple of (sub)commands, like "git worktree add" or
"git bungle create", where our completion script lists either
paths or refs (but never both) depending on what's already on the
command line. But both of these expect a single path followed by
a single ref or any revision arguments, so we can unambigously
figure out when to list paths and when to list refs. With "diff",
"log" and "send-email" this is not possible, because they accept
any revision arguments followed by paths.
^ permalink raw reply
* [PATCH v3 0/3] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-21 23:29 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1783982021.git.tnyman@openai.com>
Packfile URI and dumb HTTP downloads stage packs at
objects/pack/pack-<hash>.pack.temp so an interrupted transfer can
resume. Opening that file in append mode forces every write to its
current end. Two Git processes fetching the same pack into one object
database can therefore append duplicate data and corrupt the pack.
The first patch separates the unrelated --index-pack-arg documentation
and error-message correction requested during review.
The second patch keeps the predictable staging name but removes append
mode. Each downloader seeks once to the current end, requests the
corresponding Range, and writes using its own descriptor offset. Since
the staging key must identify immutable pack contents, overlapping
responses write identical bytes at identical offsets. There is no need
for pwrite(2) or cross-process coordination, and resumption continues to
work for both packfile URI and ordinary dumb HTTP downloads.
A downloader can also find that the partial pack has completed and
request a range starting at EOF. Servers may respond with HTTP 416 in
that case. Treat the response as a completed download and let
index-pack validate the pack.
On MinGW, the non-append O_RDWR open grants FILE_SHARE_DELETE only for an
existing file. Create a missing staging file exclusively, close it, and
reopen it without O_CREAT so every retained descriptor permits another
downloader to unlink the path. Keep the open descriptor for index-pack;
it installs its own pack, so the shared staging file is only unlinked,
never renamed.
The third patch handles the related .keep race. When another process has
already created the keep file, index-pack reports "pack<TAB><hash>"
instead of "keep<TAB><hash>". Accept both successful forms and remove
only keep files created by the current process. Read only the prefix and
hash so any following fsck output remains available to fetch-pack.
The tests cover resumption, a completed partial returning 416,
overlapping 200 and 206 responses, unlinking the staging path while
index-pack holds its descriptor, and a pre-existing .keep file. The
unlink test does not require FIFOs, so it can exercise MinGW's sharing
behavior even though the concurrent-download tests are skipped there.
Changes since v2:
* Split the --index-pack-arg documentation and error-message cleanup
into a preliminary patch, as requested by Junio.
* Clarify why per-descriptor offsets keep overlapping writes safe and
why MinGW permits the shared staging path to be unlinked.
* Add a non-FIFO unlink-while-indexing regression test that can run on
MinGW.
* Rebase onto the current master.
The v2 discussion is at:
https://lore.kernel.org/git/cover.1784582665.git.tnyman@openai.com/
Ted Nyman (3):
http-fetch: correct --index-pack-arg documentation
http: avoid concurrent appends to partial packs
fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 13 +-
fetch-pack.c | 33 ++--
http-fetch.c | 7 +-
http-push.c | 3 +-
http-walker.c | 3 +-
http.c | 56 ++++---
t/t5550-http-fetch-dumb.sh | 244 ++++++++++++++++++++++++++++++
t/t5702-protocol-v2.sh | 31 ++++
8 files changed, 344 insertions(+), 46 deletions(-)
Range-diff against v2:
-: ---------- > 1: a6a40b8046 http-fetch: correct --index-pack-arg documentation
1: 160a9b9fd0 ! 2: 6c91054afc http: avoid concurrent appends to partial packs
@@ Commit message
Pack requests stage downloads in a predictable partial-pack file so an
interrupted transfer can be resumed. Both packfile URI and ordinary dumb
- HTTP requests use this staging path. Opening it in append mode lets
- concurrent fetches interleave their writes, corrupting the pack or
- causing a later fetch to request a range at EOF.
+ HTTP requests use this staging path. Opening it in append mode forces
+ each write to the current end of the file, so concurrent responses can
+ append duplicate data and corrupt the pack.
- Open the partial pack read-write, seek to its current end, and retain a
- per-descriptor offset for incoming data. Reopen newly created partial
- packs without O_CREAT so Windows permits concurrent unlink, and keep the
- descriptor for index-pack when another downloader removes the staging
- path. Accept HTTP 416 when a partial pack is already complete.
+ Open the partial pack read-write without O_APPEND and seek once to its
+ current end. Each downloader then retains the offset matching the Range
+ it requested. Because the staging key must uniquely identify immutable
+ pack contents, overlapping responses write the same bytes at the same
+ offsets instead of extending the file with duplicate data.
- Exercise resumed transfers, EOF ranges, and overlapping 200 and 206
- responses. Clarify the staging-key documentation and correct the stale
- --index-pack-args spelling in the documentation and error messages; the
- repeatable --index-pack-arg option is already accepted.
+ MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
+ existing file. Create a missing partial pack exclusively, close it, and
+ reopen it without O_CREAT so every retained descriptor permits another
+ downloader to unlink the staging path. Duplicate that descriptor for
+ index-pack instead of reopening the path after closing the stream;
+ index-pack installs its own pack and the shared staging file is only
+ unlinked, never renamed. Accept HTTP 416 when a partial pack is already
+ complete and let index-pack validate its contents.
+
+ Exercise resumed transfers, EOF ranges, overlapping 200 and 206
+ responses, and unlinking the staging path while index-pack still holds
+ its descriptor. Clarify the staging-key documentation.
Signed-off-by: Ted Nyman <tnyman@openai.com>
@@ Documentation/git-http-fetch.adoc: commit-id::
URL and uses index-pack to generate corresponding .idx and .keep files.
- The hash is used to determine the name of the temporary file and is
- arbitrary. The output of index-pack is printed to stdout. Requires
-- --index-pack-args.
+ The hash is used to determine the name of the temporary file. It need
+ not be the pack hash, but it must uniquely identify the pack contents
+ for resumption. The output of index-pack is printed to stdout. Requires
-+ one or more --index-pack-arg options.
-
----index-pack-args=<args>::
-- For internal use only. The command to run on the contents of the
-- downloaded pack. Arguments are URL-encoded separated by spaces.
-+--index-pack-arg=<arg>::
-+ For internal use only. An argument to the command run on the contents
-+ of the downloaded pack. This option can be specified multiple times.
+ one or more --index-pack-arg options.
- --recover::
- Verify that everything reachable from target is fetched. Used after
+ --index-pack-arg=<arg>::
## http-fetch.c ##
@@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
@@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
struct url_info url;
char *nurl = url_normalize(preq->url, &url);
if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
-@@ http-fetch.c: int cmd_main(int argc, const char **argv)
-
- if (packfile) {
- if (!index_pack_args.nr)
-- die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-args");
-+ die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-arg");
-
- fetch_single_packfile(&packfile_hash, argv[arg],
- index_pack_args.v);
-@@ http-fetch.c: int cmd_main(int argc, const char **argv)
- }
-
- if (index_pack_args.nr)
-- die(_("the option '%s' requires '%s'"), "--index-pack-args", "--packfile");
-+ die(_("the option '%s' requires '%s'"), "--index-pack-arg", "--packfile");
-
- if (commits_on_stdin) {
- commits = walker_targets_stdin(&commit_id, &write_ref);
## http-push.c ##
@@ http-push.c: static void finish_request(struct transfer_request *request)
@@ http.c: struct http_pack_request *new_http_pack_request(
- if (!preq->packfile) {
- error("Unable to open local file %s for pack",
- preq->tmpfile.buf);
-+ /* Reopen without O_CREAT so MinGW permits another writer to unlink it. */
++ /*
++ * MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
++ * existing file; reopen a newly created file so others may unlink it.
++ */
+ for (;;) {
+ fd = open(preq->tmpfile.buf, O_RDWR);
+ if (fd >= 0 || errno != ENOENT)
@@ http.c: struct http_pack_request *new_http_pack_request(
+ if (fd < 0) {
+ error_errno("unable to open local file %s for pack",
+ preq->tmpfile.buf);
-+ goto abort;
-+ }
+ goto abort;
+ }
+ prev_posn = lseek(fd, 0, SEEK_END);
+ if (prev_posn < 0) {
+ error_errno("unable to seek local file %s for pack",
+ preq->tmpfile.buf);
+ close(fd);
- goto abort;
- }
++ goto abort;
++ }
+ preq->packfile = xfdopen(fd, "w");
preq->slot = get_active_slot();
@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile' '
+ git -C packfileclient-resume cat-file -e "$HASH"
+'
+
++test_expect_success 'http-fetch --packfile permits unlink while indexing' '
++ git init packfileclient-unlink &&
++ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
++ ls objects/pack/pack-*.pack) &&
++ tmpfile="packfileclient-unlink/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
++ write_script git-unlink-index-pack <<-\EOF &&
++ test -f "$GIT_TEST_PACK_TEMP" || exit 1
++ rm "$GIT_TEST_PACK_TEMP" || exit 1
++ exec git index-pack "$@"
++ EOF
++ test_when_finished "rm -f git-unlink-index-pack" &&
++ PATH="$TRASH_DIRECTORY:$PATH" \
++ GIT_TEST_PACK_TEMP="$TRASH_DIRECTORY/$tmpfile" \
++ git -C packfileclient-unlink http-fetch --packfile="$ARBITRARY" \
++ --index-pack-arg=unlink-index-pack \
++ --index-pack-arg=--stdin --index-pack-arg=--keep \
++ "$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
++ test_path_is_missing "$tmpfile" &&
++ git -C packfileclient-unlink cat-file -e "$HASH"
++'
++
+test_expect_success PIPE 'concurrent http-fetch --packfile accepts a complete partial' '
+ git init packfileclient-concurrent &&
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
2: 9b41d4ddb3 = 3: 1ee5d7e027 fetch-pack: accept "pack" output for packfile URIs
base-commit: 5d2e7709234afea1b6ddb25cd4f60d3d5fb3c200
--
2.55.0.openai.131.g83a728de1eb6
^ permalink raw reply
* [PATCH v3 1/3] http-fetch: correct --index-pack-arg documentation
From: Ted Nyman @ 2026-07-21 23:29 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1784676106.git.tnyman@openai.com>
The --packfile mode accepts one --index-pack-arg=<arg> option per
argument passed to index-pack, but its documentation and option
dependency errors still refer to the plural --index-pack-args form.
Correct the spelling and describe the repeatable per-argument form.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
Documentation/git-http-fetch.adoc | 8 ++++----
http-fetch.c | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 2200f073c4..09b5d675ee 100644
--- a/Documentation/git-http-fetch.adoc
+++ b/Documentation/git-http-fetch.adoc
@@ -50,11 +50,11 @@ commit-id::
URL and uses index-pack to generate corresponding .idx and .keep files.
The hash is used to determine the name of the temporary file and is
arbitrary. The output of index-pack is printed to stdout. Requires
- --index-pack-args.
+ one or more --index-pack-arg options.
---index-pack-args=<args>::
- For internal use only. The command to run on the contents of the
- downloaded pack. Arguments are URL-encoded separated by spaces.
+--index-pack-arg=<arg>::
+ For internal use only. An argument to the command run on the contents
+ of the downloaded pack. This option can be specified multiple times.
--recover::
Verify that everything reachable from target is fetched. Used after
diff --git a/http-fetch.c b/http-fetch.c
index f9b6ecb061..601a77c3c1 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -155,7 +155,7 @@ int cmd_main(int argc, const char **argv)
if (packfile) {
if (!index_pack_args.nr)
- die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-args");
+ die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-arg");
fetch_single_packfile(&packfile_hash, argv[arg],
index_pack_args.v);
@@ -164,7 +164,7 @@ int cmd_main(int argc, const char **argv)
}
if (index_pack_args.nr)
- die(_("the option '%s' requires '%s'"), "--index-pack-args", "--packfile");
+ die(_("the option '%s' requires '%s'"), "--index-pack-arg", "--packfile");
if (commits_on_stdin) {
commits = walker_targets_stdin(&commit_id, &write_ref);
--
2.55.0.openai.131.g83a728de1eb6
^ permalink raw reply related
* [PATCH v3 2/3] http: avoid concurrent appends to partial packs
From: Ted Nyman @ 2026-07-21 23:29 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1784676106.git.tnyman@openai.com>
Pack requests stage downloads in a predictable partial-pack file so an
interrupted transfer can be resumed. Both packfile URI and ordinary dumb
HTTP requests use this staging path. Opening it in append mode forces
each write to the current end of the file, so concurrent responses can
append duplicate data and corrupt the pack.
Open the partial pack read-write without O_APPEND and seek once to its
current end. Each downloader then retains the offset matching the Range
it requested. Because the staging key must uniquely identify immutable
pack contents, overlapping responses write the same bytes at the same
offsets instead of extending the file with duplicate data.
MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
existing file. Create a missing partial pack exclusively, close it, and
reopen it without O_CREAT so every retained descriptor permits another
downloader to unlink the staging path. Duplicate that descriptor for
index-pack instead of reopening the path after closing the stream;
index-pack installs its own pack and the shared staging file is only
unlinked, never renamed. Accept HTTP 416 when a partial pack is already
complete and let index-pack validate its contents.
Exercise resumed transfers, EOF ranges, overlapping 200 and 206
responses, and unlinking the staging path while index-pack still holds
its descriptor. Clarify the staging-key documentation.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
Documentation/git-http-fetch.adoc | 5 +-
http-fetch.c | 3 +-
http-push.c | 3 +-
http-walker.c | 3 +-
http.c | 56 ++++---
t/t5550-http-fetch-dumb.sh | 244 ++++++++++++++++++++++++++++++
6 files changed, 289 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 09b5d675ee..60ca91cf3a 100644
--- a/Documentation/git-http-fetch.adoc
+++ b/Documentation/git-http-fetch.adoc
@@ -48,8 +48,9 @@ commit-id::
line (which is not expected in
this case), 'git http-fetch' fetches the packfile directly at the given
URL and uses index-pack to generate corresponding .idx and .keep files.
- The hash is used to determine the name of the temporary file and is
- arbitrary. The output of index-pack is printed to stdout. Requires
+ The hash is used to determine the name of the temporary file. It need
+ not be the pack hash, but it must uniquely identify the pack contents
+ for resumption. The output of index-pack is printed to stdout. Requires
one or more --index-pack-arg options.
--index-pack-arg=<arg>::
diff --git a/http-fetch.c b/http-fetch.c
index 601a77c3c1..05f68f306a 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -70,7 +70,8 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
if (start_active_slot(preq->slot)) {
run_active_slot(preq->slot);
- if (results.curl_result != CURLE_OK) {
+ if (results.curl_result != CURLE_OK &&
+ results.http_code != 416) {
struct url_info url;
char *nurl = url_normalize(preq->url, &url);
if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
diff --git a/http-push.c b/http-push.c
index 60f6f8f054..ef8abe3908 100644
--- a/http-push.c
+++ b/http-push.c
@@ -595,7 +595,8 @@ static void finish_request(struct transfer_request *request)
} else if (request->state == RUN_FETCH_PACKED) {
int fail = 1;
- if (request->curl_result != CURLE_OK) {
+ if (request->curl_result != CURLE_OK &&
+ request->http_code != 416) {
fprintf(stderr, "Unable to get pack file %s\n%s",
request->url, curl_errorstr);
} else {
diff --git a/http-walker.c b/http-walker.c
index b58a3b2a92..abafca84d6 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -451,7 +451,8 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo,
if (start_active_slot(preq->slot)) {
run_active_slot(preq->slot);
- if (results.curl_result != CURLE_OK) {
+ if (results.curl_result != CURLE_OK &&
+ results.http_code != 416) {
error("Unable to get pack file %s\n%s", preq->url,
curl_errorstr);
goto abort;
diff --git a/http.c b/http.c
index caccf2108e..a0d399b274 100644
--- a/http.c
+++ b/http.c
@@ -2688,10 +2688,13 @@ int finish_http_pack_request(struct http_pack_request *preq)
int tmpfile_fd;
int ret = 0;
+ /* Another downloader may unlink the staging path while we index it. */
+ tmpfile_fd = xdup(fileno(preq->packfile));
fclose(preq->packfile);
preq->packfile = NULL;
-
- tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
+ if (lseek(tmpfile_fd, 0, SEEK_SET) < 0)
+ die_errno("unable to seek local file %s for pack",
+ preq->tmpfile.buf);
ip.git_cmd = 1;
ip.in = tmpfile_fd;
@@ -2704,13 +2707,8 @@ int finish_http_pack_request(struct http_pack_request *preq)
else
ip.no_stdout = 1;
- if (run_command(&ip)) {
+ if (run_command(&ip))
ret = -1;
- goto cleanup;
- }
-
-cleanup:
- close(tmpfile_fd);
unlink(preq->tmpfile.buf);
return ret;
}
@@ -2738,22 +2736,45 @@ struct http_pack_request *new_http_pack_request(
struct http_pack_request *new_direct_http_pack_request(
const unsigned char *packed_git_hash, char *url)
{
- off_t prev_posn = 0;
+ off_t prev_posn;
struct http_pack_request *preq;
+ int fd;
CALLOC_ARRAY(preq, 1);
strbuf_init(&preq->tmpfile, 0);
-
preq->url = url;
odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
strbuf_addstr(&preq->tmpfile, ".temp");
- preq->packfile = fopen(preq->tmpfile.buf, "a");
- if (!preq->packfile) {
- error("Unable to open local file %s for pack",
- preq->tmpfile.buf);
+ /*
+ * MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
+ * existing file; reopen a newly created file so others may unlink it.
+ */
+ for (;;) {
+ fd = open(preq->tmpfile.buf, O_RDWR);
+ if (fd >= 0 || errno != ENOENT)
+ break;
+ fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ if (fd >= 0) {
+ close(fd);
+ continue;
+ }
+ if (errno != EEXIST)
+ break;
+ }
+ if (fd < 0) {
+ error_errno("unable to open local file %s for pack",
+ preq->tmpfile.buf);
goto abort;
}
+ prev_posn = lseek(fd, 0, SEEK_END);
+ if (prev_posn < 0) {
+ error_errno("unable to seek local file %s for pack",
+ preq->tmpfile.buf);
+ close(fd);
+ goto abort;
+ }
+ preq->packfile = xfdopen(fd, "w");
preq->slot = get_active_slot();
preq->headers = object_request_headers();
@@ -2762,12 +2783,7 @@ struct http_pack_request *new_direct_http_pack_request(
curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->headers);
- /*
- * If there is data present from a previous transfer attempt,
- * resume where it left off
- */
- prev_posn = ftello(preq->packfile);
- if (prev_posn>0) {
+ if (prev_posn > 0) {
if (http_is_verbose)
fprintf(stderr,
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index f00eeae48f..65b42c4719 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -293,6 +293,250 @@ test_expect_success 'http-fetch --packfile' '
git -C packfileclient cat-file -e "$HASH"
'
+test_expect_success 'http-fetch --packfile resumes a partial download' '
+ git init packfileclient-resume &&
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ ls objects/pack/pack-*.pack) &&
+ tmpfile="packfileclient-resume/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
+ test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/resume.trace" \
+ git -C packfileclient-resume http-fetch --packfile="$ARBITRARY" \
+ --index-pack-arg=index-pack --index-pack-arg=--stdin \
+ --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
+ test_grep "Range: bytes=64-" resume.trace &&
+ test_path_is_missing "$tmpfile" &&
+ git -C packfileclient-resume cat-file -e "$HASH"
+'
+
+test_expect_success 'http-fetch --packfile permits unlink while indexing' '
+ git init packfileclient-unlink &&
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ ls objects/pack/pack-*.pack) &&
+ tmpfile="packfileclient-unlink/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
+ write_script git-unlink-index-pack <<-\EOF &&
+ test -f "$GIT_TEST_PACK_TEMP" || exit 1
+ rm "$GIT_TEST_PACK_TEMP" || exit 1
+ exec git index-pack "$@"
+ EOF
+ test_when_finished "rm -f git-unlink-index-pack" &&
+ PATH="$TRASH_DIRECTORY:$PATH" \
+ GIT_TEST_PACK_TEMP="$TRASH_DIRECTORY/$tmpfile" \
+ git -C packfileclient-unlink http-fetch --packfile="$ARBITRARY" \
+ --index-pack-arg=unlink-index-pack \
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
+ test_path_is_missing "$tmpfile" &&
+ git -C packfileclient-unlink cat-file -e "$HASH"
+'
+
+test_expect_success PIPE 'concurrent http-fetch --packfile accepts a complete partial' '
+ git init packfileclient-concurrent &&
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ ls objects/pack/pack-*.pack) &&
+ packhash=$(basename "$p" .pack) &&
+ packhash=${packhash#pack-} &&
+ tmpfile="packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
+ test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
+ mkfifo first-ready first-continue &&
+ exec 8<>first-ready &&
+ exec 9<>first-continue &&
+ write_script git-wait-index-pack <<-\EOF &&
+ echo ready >"$GIT_TEST_WAIT_READY" &&
+ read continue <"$GIT_TEST_WAIT_CONTINUE" &&
+ exec git index-pack "$@"
+ EOF
+ {
+ (
+ if ! PATH="$TRASH_DIRECTORY:$PATH" \
+ GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
+ GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/first.trace" \
+ git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
+ --index-pack-arg=wait-index-pack \
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
+ then
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
+ exit 1
+ fi
+ ) &
+ first_pid=$!
+ } &&
+ test_when_finished "
+ echo continue >&9
+ kill $first_pid 2>/dev/null || :
+ wait $first_pid 2>/dev/null || :
+ exec 8>&-
+ exec 9>&-
+ rm -f first-ready first-continue git-wait-index-pack
+ " &&
+ read ready <&8 &&
+ test "$ready" = ready &&
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/second.trace" \
+ git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
+ --index-pack-arg=index-pack \
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >second.out &&
+ echo continue >&9 &&
+ wait "$first_pid" &&
+ printf "pack\t%s\n" "$packhash" >expect &&
+ test_cmp expect first.out &&
+ printf "keep\t%s\n" "$packhash" >expect &&
+ test_cmp expect second.out &&
+ test_grep "Range: bytes=64-" first.trace &&
+ test_grep "Range: bytes=[0-9]*-" second.trace &&
+ test_grep "HTTP/[0-9.]* 416" second.trace &&
+ test_path_is_missing "$tmpfile" &&
+ git -C packfileclient-concurrent cat-file -e "$HASH"
+'
+
+test_expect_success PERL,PIPE 'concurrent http-fetch --packfile cannot corrupt an overlapping download' '
+ git init packfileclient-overlap &&
+ blob=$(test-tool genrandom pack-overlap 2m |
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
+ hash-object -w --stdin) &&
+ packhash=$(printf "%s\n" "$blob" |
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
+ pack-objects "$TRASH_DIRECTORY/overlap-pack") &&
+ pack="$TRASH_DIRECTORY/overlap-pack-$packhash.pack" &&
+ tmpfile="packfileclient-overlap/.git/objects/pack/pack-$packhash.pack.temp" &&
+ mkfifo server-ready first-ready &&
+ exec 7<>server-ready &&
+ exec 8<>first-ready &&
+ write_script slow-pack-server "$PERL_PATH" <<-\EOF &&
+ use strict;
+ use warnings;
+ use IO::Socket::INET;
+
+ my ($packfile, $server_ready, $first_ready) = @ARGV;
+ open(my $in, "<:raw", $packfile) or die "open $packfile: $!";
+ my $pack = do { local $/; <$in> };
+ close($in) or die "close $packfile: $!";
+ my $server = IO::Socket::INET->new(LocalAddr => "127.0.0.1",
+ LocalPort => 0, Proto => "tcp", Listen => 2, ReuseAddr => 1)
+ or die "listen: $!";
+
+ sub signal_ready {
+ my ($file, $value) = @_;
+ open(my $out, ">", $file) or die "open $file: $!";
+ print $out "$value\n" or die "write $file: $!";
+ close($out) or die "close $file: $!";
+ }
+
+ sub write_all {
+ my ($out, $data) = @_;
+ my $offset = 0;
+ while ($offset < length($data)) {
+ my $written = syswrite($out, $data,
+ length($data) - $offset, $offset);
+ defined($written) && $written or die "write response: $!";
+ $offset += $written;
+ }
+ }
+
+ sub start_response {
+ my $out = $server->accept() or die "accept: $!";
+ <$out> or die "read request: $!";
+ my $start = 0;
+ while (<$out>) {
+ last if /^\r?\n$/;
+ $start = $1 if /^Range: bytes=(\d+)-/i;
+ }
+ $start < length($pack) or die "invalid range $start";
+ my $length = length($pack) - $start;
+ my $middle = int($length / 2);
+ my $status = $start ? "206 Partial Content" : "200 OK";
+ my $headers = "HTTP/1.1 $status\r\n" .
+ "Content-Length: $length\r\n" .
+ ($start ? "Content-Range: bytes $start-" .
+ (length($pack) - 1) . "/" . length($pack) . "\r\n" : "") .
+ "Connection: close\r\n\r\n";
+ write_all($out, $headers);
+ write_all($out, substr($pack, $start, $middle));
+ return ($out, $start + $middle);
+ }
+
+ signal_ready($server_ready, $server->sockport());
+ my ($first, $first_pos) = start_response();
+ signal_ready($first_ready, "ready");
+ my ($second, $second_pos) = start_response();
+ write_all($first, substr($pack, $first_pos));
+ write_all($second, substr($pack, $second_pos));
+ close($first) or die "close first response: $!";
+ close($second) or die "close second response: $!";
+ EOF
+ {
+ (
+ if ! "$TRASH_DIRECTORY/slow-pack-server" "$pack" \
+ "$TRASH_DIRECTORY/server-ready" \
+ "$TRASH_DIRECTORY/first-ready"
+ then
+ echo failed >"$TRASH_DIRECTORY/server-ready" &&
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
+ exit 1
+ fi
+ ) >server.log 2>&1 &
+ server_pid=$!
+ } &&
+ test_when_finished "
+ kill $server_pid 2>/dev/null || :
+ wait $server_pid 2>/dev/null || :
+ exec 7>&-
+ exec 8>&-
+ rm -f server-ready first-ready slow-pack-server
+ " &&
+ read port <&7 &&
+ url="http://127.0.0.1:$port/pack" &&
+ {
+ (
+ if ! GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-first.trace" \
+ GIT_TRACE_CURL_NO_DATA=1 \
+ git -C packfileclient-overlap http-fetch --packfile="$packhash" \
+ --index-pack-arg=index-pack \
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$url" >first.out
+ then
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
+ exit 1
+ fi
+ ) &
+ first_pid=$!
+ } &&
+ test_when_finished "
+ kill $first_pid 2>/dev/null || :
+ wait $first_pid 2>/dev/null || :
+ " &&
+ read ready <&8 &&
+ test "$ready" = ready &&
+ test_path_is_file "$tmpfile" &&
+ test -s "$tmpfile" &&
+ {
+ GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-second.trace" \
+ GIT_TRACE_CURL_NO_DATA=1 \
+ git -C packfileclient-overlap http-fetch --packfile="$packhash" \
+ --index-pack-arg=index-pack \
+ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$url" >second.out &
+ second_pid=$!
+ } &&
+ test_when_finished "
+ kill $second_pid 2>/dev/null || :
+ wait $second_pid 2>/dev/null || :
+ " &&
+ wait "$server_pid" &&
+ wait "$first_pid" &&
+ wait "$second_pid" &&
+ test_grep "HTTP/[0-9.]* 200" overlap-first.trace &&
+ test_grep "Range: bytes=[1-9][0-9]*-" overlap-second.trace &&
+ test_grep "HTTP/[0-9.]* 206" overlap-second.trace &&
+ printf "keep\t%s\npack\t%s\n" "$packhash" "$packhash" | sort >expect &&
+ sort first.out second.out >actual &&
+ test_cmp expect actual &&
+ test_path_is_missing "$tmpfile" &&
+ git -C packfileclient-overlap cat-file -e "$blob"
+'
+
test_expect_success 'fetch notices corrupt pack' '
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
--
2.55.0.openai.131.g83a728de1eb6
^ 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