* Re: git-history(1) fixup broken with worktrees?
From: Phillip Wood @ 2026-07-18 9:31 UTC (permalink / raw)
To: Toon Claes, Patrick Steinhardt, git
In-Reply-To: <87jyqt1m6g.fsf@emacs.iotcl.com>
Hi Toon
On 17/07/2026 19:54, Toon Claes wrote:
>
> Imagine this repoducer:
>
> $ git init
> $ echo Hello > README
> $ git add .
> $ git commit -m'initial commit'
> $ git worktree add ../feature
> $ echo world >> README
> $ git add .
> $ git history fixup HEAD
> $ cd ../feature
>
> Now running git-status(1) in that other worktree gives me:
>
> $ git status
>
> On branch feature
> Changes to be committed:
> (use "git restore --staged <file>..." to unstage)
> modified: README
>
> And:
>
> $ git diff --staged
>
> diff --git a/README b/README
> index 65a56c3..e965047 100644
> --- a/README
> +++ b/README
> @@ -1,2 +1 @@
> Hello
> -world
>
>
> So suddenly my other worktree is dirty? With staged changes?
> And I didn't even touch it.
I think what's happening is that the branch "feature" is updated because
the commit it points to is rewritten, but the index and working copy in
the work tree "feature" are not. Rebase's --update-refs option refuses
to update branches that are checked out in other workers by default to
avoid exactly this problem[1]. As you can see in that thread there was
some discussion about updating the index and working copy when the work
tree is clean instead. I think that is a friendlier approach as it
preserves the relationships between branches and avoids materializing
changes in other worktrees.
On a related note, rebase refuses to rewrite a branch that is being
rewritten by another rebase running in a different work tree. That's an
important safety measure that I think the history command is missing.
Thanks
Phillip
[1]
https://lore.kernel.org/git/9354d1d3-c1b7-3baf-215f-30659ad48b22@github.com/
> Now the commit history is correct:
>
> $ git log --graph --oneline --all
>
> * 16ef548 (HEAD -> feature, main) initial commit
>
>
>
^ permalink raw reply
* Re: [PATCH v9 5/5] history: re-edit a squash with every message
From: Harald Nordgren @ 2026-07-18 9:36 UTC (permalink / raw)
To: Matt Hunter
Cc: Harald Nordgren via GitGitGadget, git, Phillip Wood,
D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <DK1KIH6CXW0X.1U2V3GU8L6HB7@lfurio.us>
Good points!
Harald
^ permalink raw reply
* Re: [RFC PATCH 1/7] builtin/repack.c: add --drop-filtered and --dry-run options
From: Christian Couder @ 2026-07-18 12:30 UTC (permalink / raw)
To: Siddharth Shrimali
Cc: git, gitster, siddharthasthana31, me, ps, johannes.schindelin,
l.s.r
In-Reply-To: <20260716132848.95982-2-r.siddharth.shrimali@gmail.com>
On Thu, Jul 16, 2026 at 3:29 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Add two new command-line options to 'git-repack':
>
> --drop-filtered: intended to eventually delete objects that match
> the filter specification. Requires --filter and -a,
> and is incompatible with --filter-to.
> --dry-run: show which objects would be dropped without making any
> changes. Only meaningful with --drop-filtered.
An alternative would be `--drop-filtered[=dry-run]`, which might be
extended with other `--drop-filtered` specific options later.
I think separating `--dry-run` from `--drop-filtered` like this patch
does makes sense though if we think that `--dry-run` could be useful
later without `--drop-filtered`. The fact that a number of other
commands already have a `--dry-run` option might be a good sign.
Anyway it would be nice if the commit message explained a bit the
choice to have a separate `--dry-run` option.
^ permalink raw reply
* Re: [PATCH] trace2: tolerate failed timestamp formatting
From: Derrick Stolee @ 2026-07-18 15:01 UTC (permalink / raw)
To: Taylor Blau, Derrick Stolee via GitGitGadget; +Cc: git, gitster
In-Reply-To: <alpXW5U6sndZtgqV@com-79390>
On 7/17/2026 12:24 PM, Taylor Blau wrote:
> On Wed, Jul 15, 2026 at 04:12:11PM +0000, Derrick Stolee via GitGitGadget wrote:
>> This change removes all uses of xsnprintf() from the trace2/ directory.
>> There are two uses of xstrdup() that could be considered for removal,
>> but they only die() on out-of-memory errors instead of formatting
>> issues. I chose to leave those in place for now.
>
> I may be missing some Git for Windows context, but I dug into this a
> little and I'm not sure 'gettimeofday()' is the culprit...
>
> In my understanding Git for Windows's 'gettext.h' appears[1] to redirect
> the 'vsnprintf()' inside 'xsnprintf()' to 'libintl_vsnprintf()'. In this
> case, we have seven '%' placeholders. Gettext can store only six plus
> its end marker inline, so parsing the seventh causes an allocation
> before any timestamp values are read.
>
> A failure there would produce the observed -1, after which 'xsnprintf()'
> dies and trace2 can recurse.
With this perspective, the issue is that gettext is doing dynamic
allocation and getting a failure there, which explains the transient
nature. This is an interesting idea, and a more likely "application
side" error. I'm still curious why this is creeping up for the first
time in this burst, since nothing has changed in the application, to
my knowledge.
> I think that also explains why calling 'snprintf()' directly helps.
> tr2_tbuf.c doesn't include gettext.h, so I think it bypasses libintl. If
> I'm reading compat/mingw.c correctly, 'gettimeofday()' fills tv and
> always returns zero [2], making the zero-initialization unrelated.
>
> Would it make more sense to fix the xsnprintf()/libintl boundary and
> treat Trace2 reentrancy separately? I still can't explain why the
> allocation failed, so there may be another GfW-specific piece I’m
> missing.
I think that your suggested change has merits and should be pursued.
I'll explore it a bit to confirm.
The other justification I'd like to make in my patch is that the
xsnprintf() calls die() and the trace2 machinery should be die()-free
whenever possible. Solving both possible causes is likely the right
long-term approach.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 3/3] bisect: add --auto-reset to leave when done
From: Johannes Sixt @ 2026-07-18 16:18 UTC (permalink / raw)
To: Harald Nordgren; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <5b3704fbd4129e6bf742fe9b38998d5c952c6f21.1784312854.git.gitgitgadget@gmail.com>
Am 17.07.26 um 20:27 schrieb Harald Nordgren via GitGitGadget:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When a bisection finishes, "git bisect" reports the first bad commit
> but leaves the session active until "git bisect reset" is run by hand.
>
> Add an "--auto-reset[=<where>]" option, accepted by both "git bisect
> start" and "git bisect run", that resets as soon as the first bad commit
> is found. The "original" value returns to the commit checked out before
> "git bisect start", while "found" leaves the first bad commit checked
> out; omitting the value defaults to "original".
IMHO, --auto-reset is too generic and doesn't give a clue what is
automatic about it. How about --reset-when-found?
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 1/3] bisect: read run output from the open descriptor
From: Johannes Sixt @ 2026-07-18 16:24 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <0de8b12f65530497320b6a4bca395dfd0556c959.1784312854.git.gitgitgadget@gmail.com>
Am 17.07.26 um 20:27 schrieb Harald Nordgren via GitGitGadget:
> "git bisect run" redirects each step's output into BISECT_RUN, then
> prints it back by reopening the file by name. Read it from the already
> open descriptor instead; this behaves the same and no longer needs the
> file to be reachable by name.
This rings alarm bells. Why would an open file not be reachable by name
anymore? Are we already progressed sufficiently in our Windows port that
it is possible to remove open files? Because it is (was?) the case that
on Windows, open files cannot be remove, so this shouldn't happen
anywhere in the code in general.
-- Hannes
^ permalink raw reply
* Re: [PATCH] branch: report kind of checkout when rejecting delete
From: Junio C Hamano @ 2026-07-18 17:34 UTC (permalink / raw)
To: René Scharfe, Phillip Wood, Toon Claes, Patrick Steinhardt
Cc: Git List, stsp
In-Reply-To: <9865fc6b-e3fe-4614-9ffe-71af776e1796@web.de>
René Scharfe <l.s.r@web.de> writes:
> git branch refuses to delete branches that are currently checked out
> with a message like this: "error: cannot delete branch 'foo' used by
> worktree at '/path/of/worktree'". This can be confusing with internal
> checkouts, e.g. if one tries to delete a branch associated with an
> active bisect run.
>
> Mention the kind of internal checkout, if any, to spare the user from
> remembering that they might have forgotten a bisect or rebase. To do
> that, register the checkout reason in a strintmap alongside the existing
> strmap that stores the worktree path.
>
> Suggested-by: stsp <stsp2@yandex.ru>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Original message:
> https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/
This reminds me of another recent discussion on rewriting a branch
that is checked out elsewhere, where the "git history" command
forgot to apply the same safety check:
https://lore.kernel.org/git/e7dbcede-4486-459c-aa64-e44690e01fe0@gmail.com/
We definitely need an easy-to-use API to determine consistently
which branches are in use, and to teach all commands that repoint
branch tips to use it to offer the same safety to users. The
framework that this patch introduces might be a good starting point
for that effort.
> diff --git a/branch.h b/branch.h
> index 3dc6e2a0ff..d1073fe1cd 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -15,6 +15,14 @@ enum branch_track {
> BRANCH_TRACK_SIMPLE,
> };
>
> +enum branch_checkout_kind {
> + BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
> + BRANCH_CHECKOUT_KIND_CHECKOUT,
> + BRANCH_CHECKOUT_KIND_REBASE,
> + BRANCH_CHECKOUT_KIND_BISECT,
> + BRANCH_CHECKOUT_KIND_UPDATE_REF,
> +};
> +
> ...
> +/*
> + * If the branch at 'refname' is currently checked out in a worktree,
> + * then return the kind of checkout, i.e. whether it was done by an
> + * actual checkout or a rebase etc.
> + */
> +enum branch_checkout_kind branch_checkout_kind(const char *refname);
OK.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index dede60d27b..3223347129 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> if (kinds == FILTER_REFS_BRANCHES) {
> const char *path;
> if ((path = branch_checked_out(name))) {
> - error(_("cannot delete branch '%s' "
> - "used by worktree at '%s'"),
> - bname.buf, path);
> + int kind = branch_checkout_kind(name);
Not "enum branch_checkout_kind" but "int"?
> + switch (kind) {
> + case BRANCH_CHECKOUT_KIND_CHECKOUT:
> + error(_("cannot delete branch '%s' "
> + "used by worktree at '%s'"),
> + bname.buf, path);
> + break;
We may want to be more explicit and say "cannot delete
branch 'frotz' checked out in worktree at '/tmp/nitfol'"
instead. Unless this is a catch-all entry for states that
are neither 'rebase', 'bisect', nor 'rebase-merges' but are
somehow otherwise in use, that is.
> + case BRANCH_CHECKOUT_KIND_UPDATE_REF:
> + error(_("cannot delete branch '%s' "
> + "used by worktree at '%s' "
> + "for update-ref"),
> + bname.buf, path);
> + break;
I was quite lost when searching for cases where this 'update-ref'
state might be encountered, and I still lack confidence. Can
we make the diagnostic message a bit friendlier to our users?
For instance, something like: 'You are rebasing a history with
merges in that other worktree, and the tip of this branch will
be updated when that process completes, so you cannot delete
it from here.' (Naturally, I may have misidentified the exact
nature of the error, but this illustrates the level of detail and
user-facing clarity I hope to see.)
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index e2682a83a0..e5df493b66 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
> git worktree add -b my7 my7 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> + test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
> rm -r my7 &&
> git worktree prune
> '
> @@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
> git -C my7 bisect start HEAD HEAD~2 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> + test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
> rm -r my7 &&
> git worktree prune
> '
We distinguish four kinds in the code but we test only two of them?
Thanks. I very much like the direction this is taking us.
^ permalink raw reply
* Re: [PATCH] userdiff: add support for Swift
From: Johannes Sixt @ 2026-07-18 18:11 UTC (permalink / raw)
To: Shlok Kulshreshtha
Cc: D. Ben Knoble, Junio C Hamano, René Scharfe, Eric Sunshine,
Scott L. Burson, git
In-Reply-To: <20260717140232.6722-1-diy2903@gmail.com>
Am 17.07.26 um 16:02 schrieb Shlok Kulshreshtha:
> 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>
> ---
> This addresses the "add a userdiff driver for a language" microproject.
I am mildly surprised that userdiff drivers can count as microproject.
At a minimum, they are on the challenging side of the spectrum.
> Swift is not covered by a built-in driver yet, and I did not find an
> in-flight patch adding one on the list; please let me know if one exists.
>
> Motivation: without a Swift driver, ".swift" files use the generic
> funcname heuristic, so "git diff" hunk headers name the enclosing type
> instead of the changed function.
>
> The pattern is built directly from Swift's declaration grammar rather
> than adapted from another language's driver, so it covers a few things
> that a straight port would miss:
>
> - attributes, with or without arguments, whether on their own line
> ("@objc" above a "func") or inline with the declaration
> ("@objc func foo()", "@available(iOS 13, *) public func bar()");
AFAIC, the regular expression does not match attributes on their own
line. What relevance does this statement have?
> - modifiers ("public", "static", "override", "mutating", ...) in any
> combination, before or after attributes;
> - failable initializers, "init?" and "init!";
> - generics, "init<T>" and "subscript<T>";
> - the keyword boundary still acts as a word boundary, so e.g.
> "initialize(" is not mistaken for "init".
>
> I verified all of the above against a built binary, including generic
> functions with "where" clauses, multi-line signatures, and operator
> functions such as "static func ==". I did not find a real case this
> pattern misses; the ones I intentionally left out are one-line
> declarations ("typealias", "associatedtype") and rarer forms
> ("operator", "precedencegroup", "macro"), which would not make useful
> section headers anyway.
>
> The word regex covers Swift identifiers; hexadecimal, octal, binary,
> integer and floating-point literals; and operators including "<<=" /
> ">>=", "??" and the range operators. All of t4018 passes, including the
> sorted builtin_drivers check and the new swift-* fixtures.
>
> Documentation/gitattributes.adoc | 2 ++
> t/t4018/swift-actor | 5 +++++
> t/t4018/swift-attribute-with-args | 7 +++++++
> t/t4018/swift-class | 5 +++++
> t/t4018/swift-enum | 5 +++++
> t/t4018/swift-extension | 5 +++++
> t/t4018/swift-failable-init | 7 +++++++
> t/t4018/swift-func | 5 +++++
> t/t4018/swift-generic-subscript | 7 +++++++
> t/t4018/swift-init | 7 +++++++
> t/t4018/swift-inline-attribute | 7 +++++++
> t/t4018/swift-modifiers | 4 ++++
> t/t4018/swift-protocol | 5 +++++
> t/t4018/swift-struct | 5 +++++
> userdiff.c | 10 ++++++++++
> 15 files changed, 86 insertions(+)
> create mode 100644 t/t4018/swift-actor
> create mode 100644 t/t4018/swift-attribute-with-args
> create mode 100644 t/t4018/swift-class
> create mode 100644 t/t4018/swift-enum
> create mode 100644 t/t4018/swift-extension
> create mode 100644 t/t4018/swift-failable-init
> create mode 100644 t/t4018/swift-func
> create mode 100644 t/t4018/swift-generic-subscript
> create mode 100644 t/t4018/swift-init
> create mode 100644 t/t4018/swift-inline-attribute
> create mode 100644 t/t4018/swift-modifiers
> create mode 100644 t/t4018/swift-protocol
> create mode 100644 t/t4018/swift-struct
>
> diff --git a/Documentation/gitattributes.adoc b/Documentation/gitattributes.adoc
> index bd76167a45..9fea75f96f 100644
> --- a/Documentation/gitattributes.adoc
> +++ b/Documentation/gitattributes.adoc
> @@ -914,6 +914,8 @@ patterns are available:
> - `scheme` suitable for source code in most Lisp dialects,
> including Scheme, Emacs Lisp, Common Lisp, and Clojure.
>
> +- `swift` suitable for source code in the Swift language.
> +
> - `tex` suitable for source code for LaTeX documents.
>
>
> diff --git a/t/t4018/swift-actor b/t/t4018/swift-actor
> new file mode 100644
> index 0000000000..e4852f40a7
> --- /dev/null
> +++ b/t/t4018/swift-actor
> @@ -0,0 +1,5 @@
> +actor RIGHT {
> + let a = 1
> + // a comment
> + let b = ChangeMe
> +}
> diff --git a/t/t4018/swift-attribute-with-args b/t/t4018/swift-attribute-with-args
> new file mode 100644
> index 0000000000..22b1ee32f1
> --- /dev/null
> +++ b/t/t4018/swift-attribute-with-args
> @@ -0,0 +1,7 @@
> +struct View {
> + @available(iOS 13, *) public func RIGHT() {
> + let a = 1
> + // a comment
> + print(ChangeMe)
> + }
> +}
So, this doesn't pick up the "struct View {" line, but the line below.
Good test.
> diff --git a/t/t4018/swift-failable-init b/t/t4018/swift-failable-init
> new file mode 100644
> index 0000000000..5e4091d97c
> --- /dev/null
> +++ b/t/t4018/swift-failable-init
> @@ -0,0 +1,7 @@
> +class Bar {
> + init?(RIGHT: Int) {
> + let value = RIGHT
> + // a comment
> + print(ChangeMe)
> + }
> +}
This test contains "RIGHT" twice. This is not good, because we do not
know which one is picked.
> diff --git a/t/t4018/swift-generic-subscript b/t/t4018/swift-generic-subscript
> new file mode 100644
> index 0000000000..565f93cd6c
> --- /dev/null
> +++ b/t/t4018/swift-generic-subscript
> @@ -0,0 +1,7 @@
> +struct Container {
> + subscript<RIGHT>(index: RIGHT) -> Int {
> + let a = 0
> + // a comment
> + return ChangeMe
> + }
> +}
It is strange to have "RIGHT" twice on the same line, but it does no
harm. Still, there should be only one for consistency.
> diff --git a/t/t4018/swift-init b/t/t4018/swift-init
> new file mode 100644
> index 0000000000..f683e74794
> --- /dev/null
> +++ b/t/t4018/swift-init
> @@ -0,0 +1,7 @@
> +class Foo {
> + init(RIGHT: Int) {
> + let value = RIGHT
> + // a comment
> + print(ChangeMe)
> + }
> +}
Again "RIGHT" twice in a harmful way.
All other test cases look good.
> diff --git a/userdiff.c b/userdiff.c
> index b5412e6bc3..df37dd78a6 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -362,6 +362,16 @@ PATTERNS("scheme",
> "\\|([^|\\\\]|\\\\.)*\\|"
> /* All other words should be delimited by spaces or parentheses. */
> "|([^][)(}{ \t])+"),
> +PATTERNS("swift",
> + "^[ \t]*((@[A-Za-z_][A-Za-z0-9_]*(\\([^()]*\\))?[ \t]+)*([a-z]+[ \t]+)*(func|init|deinit|subscript|class|struct|enum|protocol|extension|actor)[ \t(?!<].*)$",
This looks good.
Notice, however, how the regular expression matcher has to backtrack on
even simple lines such as
class foo {
On the first attempt, [a-z]+ matches "class", but then "foo" does not
match. On the next attempt, the clause with [a-z]+ matches zero times
and the next clause matches "class" and, in total, successfully.
It may be worth considering to enumerate all keywords and permit any run
of them:
(public|final|etc.|func|init|...|actor)[ \t(?!<]+)+
It does not matter that this would match any assemblement of keywords;
they wouldn't occur in correct Swift code anyway. (Or would they?)
> + /* -- */
> + "[a-zA-Z_][a-zA-Z0-9_]*"
> + /* hexadecimal, octal, and binary literals */
> + "|0[xX][0-9a-fA-F_]+|0[oO][0-7_]+|0[bB][01_]+"
You could just throw all of them into a single pattern like this:
0[xXoObB][0-9a-fA-F_]+
except when, for example,
0b1_abc
can occur in correct Swift code (perhaps the token 0b1 followd by the
token _abc).
> + /* integers and floating-point numbers */
> + "|[0-9][0-9_]*([.][0-9_]+)?([eE][-+]?[0-9]+)?"
Ok. Is ".5" a correct floating-point number? If so, it would not be
caught by this regular expression? But it wouldn't be particularly
harmful, either, to leave it as is. The ".5" would just be parsed as two
tokens, "." and "5".
> + /* unary and binary operators */
> + "|[-+*/%<>=!&|^~?]=?|&&|\\|\\||<<=?|>>=?|\\?\\?|\\.\\.[.<]|->"),
You do not have to account for single-character operators; they are
automatic. Drop the "?" from the first "=?".
These are my comments on the technical side. Since I do not speak Swift,
I cannot comment on how reasonable your choice which lines to pick out is.
-- Hannes
^ permalink raw reply
* Re: [PATCH GSoC v19 05/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
From: SZEDER Gábor @ 2026-07-18 18:34 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260717-ps-eric-work-rebase-v19-5-d4faee35764b@gmail.com>
On Fri, Jul 17, 2026 at 05:05:15PM +0200, Pablo Sabater wrote:
> In a subsequent commit write_fetch_command_and_capabilities() will be
> refactored to a more general-purpose function, making it more accessible
> to additional commands in the future.
>
> Move write_fetch_command_and_capabilities() to 'connect.c', where
> there are similar purpose functions.
>
> Because string_list is only used as a pointer, use a forward
> declaration [1].
>
> [1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Helped-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> connect.c | 34 ++++++++++++++++++++++++++++++++++
> connect.h | 4 ++++
> fetch-pack.c | 34 ----------------------------------
> 3 files changed, 38 insertions(+), 34 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 47e39d2a73..31e5ab958b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -700,6 +700,40 @@ int server_supports(const char *feature)
> return !!server_feature_value(feature, NULL);
> }
>
> +void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> + const struct string_list *server_options)
> +{
> + const char *hash_name;
> + int advertise_sid = 0;
> +
> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
> +
> + ensure_server_supports_v2("fetch");
> + packet_buf_write(req_buf, "command=fetch");
> + if (server_supports_v2("agent"))
> + packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
> + if (advertise_sid && server_supports_v2("session-id"))
> + packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
> + if (server_options && server_options->nr) {
> + ensure_server_supports_v2("server-option");
> + for (size_t i = 0; i < server_options->nr; i++)
> + packet_buf_write(req_buf, "server-option=%s",
> + server_options->items[i].string);
> + }
> +
> + if (server_feature_v2("object-format", &hash_name)) {
> + int hash_algo = hash_algo_by_name(hash_name);
> + if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
hash_algo_by_ptr() returns an unsigned integer, and comparing that to
a signed integer results in:
connect.c: In function ‘write_fetch_command_and_capabilities’:
connect.c:726:53: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
726 | if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
| ^~
cc1: all warnings being treated as errors
make: *** [Makefile:2924: connect.o] Error 1
This was not an issue while the function lived in "fetch-pack.c",
because that file uses #define DISABLE_SIGN_COMPARE_WARNINGS, but
"connect.c" doesn't.
I think as a preparatory step the type of the 'hash_algo' variable
should be changed to unsigned int while the function is still in
"fetch-pack.c" to avoid this; you'll convert it in the next
patch anyway.
> + die(_("mismatched algorithms: client %s; server %s"),
> + the_hash_algo->name, hash_name);
> + packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
> + } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
> + die(_("the server does not support algorithm '%s'"),
> + the_hash_algo->name);
> + }
> + packet_buf_delim(req_buf);
> +}
> +
> static const char *url_scheme_name(enum url_scheme scheme)
> {
> switch (scheme) {
> diff --git a/connect.h b/connect.h
> index aa482a37fb..c4f6ea4b0a 100644
> --- a/connect.h
> +++ b/connect.h
> @@ -34,4 +34,8 @@ void check_stateless_delimiter(int stateless_rpc,
> struct packet_reader *reader,
> const char *error);
>
> +struct string_list;
> +void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> + const struct string_list *server_options);
> +
> #endif
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 65ebfec09f..f7789e8456 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1375,40 +1375,6 @@ static int add_haves(struct fetch_negotiator *negotiator,
> return haves_added;
> }
>
> -static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
> - const struct string_list *server_options)
> -{
> - const char *hash_name;
> - int advertise_sid = 0;
> -
> - repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
> -
> - ensure_server_supports_v2("fetch");
> - packet_buf_write(req_buf, "command=fetch");
> - if (server_supports_v2("agent"))
> - packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
> - if (advertise_sid && server_supports_v2("session-id"))
> - packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
> - if (server_options && server_options->nr) {
> - ensure_server_supports_v2("server-option");
> - for (size_t i = 0; i < server_options->nr; i++)
> - packet_buf_write(req_buf, "server-option=%s",
> - server_options->items[i].string);
> - }
> -
> - if (server_feature_v2("object-format", &hash_name)) {
> - 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);
> - packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
> - } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
> - die(_("the server does not support algorithm '%s'"),
> - the_hash_algo->name);
> - }
> - packet_buf_delim(req_buf);
> -}
> -
> static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
> struct fetch_pack_args *args,
> const struct ref *wants, struct oidset *common,
>
> --
> 2.54.0
^ permalink raw reply
* Re: [PATCH GSoC v19 05/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
From: Pablo Sabater @ 2026-07-18 19:00 UTC (permalink / raw)
To: SZEDER Gábor, Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <alvHUX68McwlEau2@szeder.dev>
On Sat Jul 18, 2026 at 8:34 PM CEST, SZEDER Gábor wrote:
> On Fri, Jul 17, 2026 at 05:05:15PM +0200, Pablo Sabater wrote:
>> In a subsequent commit write_fetch_command_and_capabilities() will be
>> refactored to a more general-purpose function, making it more accessible
>> to additional commands in the future.
>>
>> Move write_fetch_command_and_capabilities() to 'connect.c', where
>> there are similar purpose functions.
>>
>> Because string_list is only used as a pointer, use a forward
>> declaration [1].
>>
>> [1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
>>
>> Helped-by: Jonathan Tan <jonathantanmy@google.com>
>> Helped-by: Christian Couder <chriscool@tuxfamily.org>
>> Signed-off-by: Calvin Wan <calvinwan@google.com>
>> Signed-off-by: Eric Ju <eric.peijian@gmail.com>
>> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>> ---
>> connect.c | 34 ++++++++++++++++++++++++++++++++++
>> connect.h | 4 ++++
>> fetch-pack.c | 34 ----------------------------------
>> 3 files changed, 38 insertions(+), 34 deletions(-)
>>
>> diff --git a/connect.c b/connect.c
>> index 47e39d2a73..31e5ab958b 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -700,6 +700,40 @@ int server_supports(const char *feature)
>> return !!server_feature_value(feature, NULL);
>> }
>>
>> +void write_fetch_command_and_capabilities(struct strbuf *req_buf,
>> + const struct string_list *server_options)
>> +{
>> + const char *hash_name;
>> + int advertise_sid = 0;
>> +
>> + repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
>> +
>> + ensure_server_supports_v2("fetch");
>> + packet_buf_write(req_buf, "command=fetch");
>> + if (server_supports_v2("agent"))
>> + packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
>> + if (advertise_sid && server_supports_v2("session-id"))
>> + packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
>> + if (server_options && server_options->nr) {
>> + ensure_server_supports_v2("server-option");
>> + for (size_t i = 0; i < server_options->nr; i++)
>> + packet_buf_write(req_buf, "server-option=%s",
>> + server_options->items[i].string);
>> + }
>> +
>> + if (server_feature_v2("object-format", &hash_name)) {
>> + int hash_algo = hash_algo_by_name(hash_name);
>> + if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
>
> hash_algo_by_ptr() returns an unsigned integer, and comparing that to
> a signed integer results in:
>
> connect.c: In function ‘write_fetch_command_and_capabilities’:
> connect.c:726:53: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
> 726 | if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
> | ^~
> cc1: all warnings being treated as errors
> make: *** [Makefile:2924: connect.o] Error 1
>
> This was not an issue while the function lived in "fetch-pack.c",
> because that file uses #define DISABLE_SIGN_COMPARE_WARNINGS, but
> "connect.c" doesn't.
>
> I think as a preparatory step the type of the 'hash_algo' variable
> should be changed to unsigned int while the function is still in
> "fetch-pack.c" to avoid this; you'll convert it in the next
> patch anyway.
Thanks for noticing it.
Yes, then the prep patch needs to be done in fetch-pack.c before moving
it to connect.c.
I had it after because 2 hunks of that patch fixes calls for connect.c
so this way there were all for connect.c instead of mixing.
[snip]
Thanks,
Pablo
^ permalink raw reply
* Re: [PATCH v7 2/4] read-cache: pass 'repo' to 'ce_mode_from_stat()'
From: SZEDER Gábor @ 2026-07-18 19:02 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, ps, Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260717063559.1633567-3-cat@malon.dev>
On Fri, Jul 17, 2026 at 02:35:57PM +0800, Tian Yuchen wrote:
> The ce_mode_from_stat() function is a performance-critical static
> inline helper in 'read-cache.h'. As we migrate configuration
> variables into the repository struct, this helper needs access
> to the repository context.
>
> Update the signature of ce_mode_from_stat() to take a 'struct
> repository *' parameter, and update all callers to pass the
> appropriate repository instance.
>
> To prepare for the overhead of replacing cheap global variable
> accesses with getter functions, the boolean expressions are
> reordered to evaluate 'S_ISREG(mode)' first.
>
> While at it, add a comment for ce_mode_from_stat().
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
> diff --git a/read-cache.h b/read-cache.h
> index 043da1f1aa..94b8d3e547 100644
> --- a/read-cache.h
> +++ b/read-cache.h
> @@ -4,15 +4,24 @@
> #include "read-cache-ll.h"
> #include "object.h"
> #include "pathspec.h"
> +#include "environment.h"
>
> -static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
> +/*
> + * Determine the appropriate index mode for a file based on its stat()
> + * information and the existing cache entry (if any).
> + *
> + * This function handles degradation for filesystems that lack
> + * symlink support or reliable executable bits.
> + */
> +static inline unsigned int ce_mode_from_stat(struct repository *repo,
This new parameter is not yet used in this function, which causes
compilation errors in all source files which include "read-cache.h"
when trying to build this commit using DEVELOPER=1, e.g.:
CC pathspec.o
In file included from pathspec.c:11:
read-cache.h: In function ‘ce_mode_from_stat’:
read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
| ~~~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors
make: *** [Makefile:2921: pathspec.o] Error 1
CC preload-index.o
In file included from preload-index.c:16:
read-cache.h: In function ‘ce_mode_from_stat’:
read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
| ~~~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors
make: *** [Makefile:2921: preload-index.o] Error 1
CC read-cache.o
In file included from read-cache.c:34:
read-cache.h: In function ‘ce_mode_from_stat’:
read-cache.h:16:65: error: unused parameter ‘repo’ [-Werror=unused-parameter]
16 | static inline unsigned int ce_mode_from_stat(struct repository *repo,
| ~~~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors
make: *** [Makefile:2921: read-cache.o] Error 1
I think the new parameter should be marked as UNUSED in this patch,
and then the UNUSED should be dropped in the next, where you start
using the parameter.
> + const struct cache_entry *ce,
> unsigned int mode)
> {
> extern int trust_executable_bit, has_symlinks;
> - if (!has_symlinks && S_ISREG(mode) &&
> + if (S_ISREG(mode) && !has_symlinks &&
> ce && S_ISLNK(ce->ce_mode))
> return ce->ce_mode;
> - if (!trust_executable_bit && S_ISREG(mode)) {
> + if (S_ISREG(mode) && !trust_executable_bit) {
> if (ce && S_ISREG(ce->ce_mode))
> return ce->ce_mode;
> return create_ce_mode(0666);
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH] branch: report kind of checkout when rejecting delete
From: René Scharfe @ 2026-07-18 19:07 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood, Toon Claes, Patrick Steinhardt
Cc: Git List, stsp
In-Reply-To: <xmqqjyqsqk1a.fsf@gitster.g>
On 7/18/26 7:34 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> git branch refuses to delete branches that are currently checked out
>> with a message like this: "error: cannot delete branch 'foo' used by
>> worktree at '/path/of/worktree'". This can be confusing with internal
>> checkouts, e.g. if one tries to delete a branch associated with an
>> active bisect run.
>>
>> Mention the kind of internal checkout, if any, to spare the user from
>> remembering that they might have forgotten a bisect or rebase. To do
>> that, register the checkout reason in a strintmap alongside the existing
>> strmap that stores the worktree path.
>>
>> Suggested-by: stsp <stsp2@yandex.ru>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> Original message:
>> https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/
>
> This reminds me of another recent discussion on rewriting a branch
> that is checked out elsewhere, where the "git history" command
> forgot to apply the same safety check:
>
> https://lore.kernel.org/git/e7dbcede-4486-459c-aa64-e44690e01fe0@gmail.com/
>
> We definitely need an easy-to-use API to determine consistently
> which branches are in use, and to teach all commands that repoint
> branch tips to use it to offer the same safety to users. The
> framework that this patch introduces might be a good starting point
> for that effort.
branch_checked_out() already allows to check whether a branch is in use,
but I guess git history needs to respond differently depending on the
kind of use, e.g. leave active bisects and rebases untouched and update
checked out branches. branch_checkout_kind() would allow that, but I
now wonder if it suffices for cases where branches appear in multiple
worktrees. So perhaps the query we need to enable are "is this branch
used by a rebase/bisect" and not the more limited "tell me one use of
this branch"?
>> diff --git a/branch.h b/branch.h
>> index 3dc6e2a0ff..d1073fe1cd 100644
>> --- a/branch.h
>> +++ b/branch.h
>> @@ -15,6 +15,14 @@ enum branch_track {
>> BRANCH_TRACK_SIMPLE,
>> };
>>
>> +enum branch_checkout_kind {
>> + BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
>> + BRANCH_CHECKOUT_KIND_CHECKOUT,
>> + BRANCH_CHECKOUT_KIND_REBASE,
>> + BRANCH_CHECKOUT_KIND_BISECT,
>> + BRANCH_CHECKOUT_KIND_UPDATE_REF,
>> +};
>> +
>> ...
>> +/*
>> + * If the branch at 'refname' is currently checked out in a worktree,
>> + * then return the kind of checkout, i.e. whether it was done by an
>> + * actual checkout or a rebase etc.
>> + */
>> +enum branch_checkout_kind branch_checkout_kind(const char *refname);
>
> OK.
>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index dede60d27b..3223347129 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>> if (kinds == FILTER_REFS_BRANCHES) {
>> const char *path;
>> if ((path = branch_checked_out(name))) {
>> - error(_("cannot delete branch '%s' "
>> - "used by worktree at '%s'"),
>> - bname.buf, path);
>> + int kind = branch_checkout_kind(name);
>
> Not "enum branch_checkout_kind" but "int"?
Yes, it doesn't matter for the switch and is easier to print.
>
>> + switch (kind) {
>> + case BRANCH_CHECKOUT_KIND_CHECKOUT:
>> + error(_("cannot delete branch '%s' "
>> + "used by worktree at '%s'"),
>> + bname.buf, path);
>> + break;
>
> We may want to be more explicit and say "cannot delete
> branch 'frotz' checked out in worktree at '/tmp/nitfol'"
> instead. Unless this is a catch-all entry for states that
> are neither 'rebase', 'bisect', nor 'rebase-merges' but are
> somehow otherwise in use, that is.
>
>> + case BRANCH_CHECKOUT_KIND_UPDATE_REF:
>> + error(_("cannot delete branch '%s' "
>> + "used by worktree at '%s' "
>> + "for update-ref"),
>> + bname.buf, path);
>> + break;
>
> I was quite lost when searching for cases where this 'update-ref'
> state might be encountered, and I still lack confidence. Can
> we make the diagnostic message a bit friendlier to our users?
>
> For instance, something like: 'You are rebasing a history with
> merges in that other worktree, and the tip of this branch will
> be updated when that process completes, so you cannot delete
> it from here.' (Naturally, I may have misidentified the exact
> nature of the error, but this illustrates the level of detail and
> user-facing clarity I hope to see.)
That's quite long. Would it make sense to throw that update-ref
case into the rebase bin, i.e. only distinguish between checkout,
bisect and rebase?
>
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index e2682a83a0..e5df493b66 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
>> git worktree add -b my7 my7 &&
>> test_must_fail git -C my7 branch -d my7 &&
>> test_must_fail git branch -d my7 2>actual &&
>> - grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>> + test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
>> rm -r my7 &&
>> git worktree prune
>> '
>> @@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
>> git -C my7 bisect start HEAD HEAD~2 &&
>> test_must_fail git -C my7 branch -d my7 &&
>> test_must_fail git branch -d my7 2>actual &&
>> - grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>> + test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
>> rm -r my7 &&
>> git worktree prune
>> '
>
> We distinguish four kinds in the code but we test only two of them?
Laziness, and it was enough for the user requirement..
René
^ permalink raw reply
* Re: [PATCH 0/9] object-file: move writing of loose objects into "loose" source
From: SZEDER Gábor @ 2026-07-18 19:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Justin Tobler
In-Reply-To: <20260717-pks-odb-move-loose-object-writing-v1-0-46446a3cb5b7@pks.im>
Hi Junio,
On Fri, Jul 17, 2026 at 11:32:08AM +0200, Patrick Steinhardt wrote:
> Hi,
>
> this patch series refactors "object-file.c" so that we can move the
> logic that writes loose objects into "odb/source-loose.c". This ensures
> that the logic to read and write loose objects is mostly self-contained
> within that source's implementation now.
>
> To achieve that, this series first refactors `force_object_loose()` so
> that it can work on top of the generic `odb_source_write_object()`
> instead of having to call into `write_loose_object()`. This is the bulk
> of the patch series.
>
> This patch series is built on top of 44de1520f0 (Merge branch 'master'
> of https://github.com/j6t/git-gui, 2026-07-16) with the following two
> topics merged into it:
>
> - jt/receive-pack-use-odb-transaction at bdee7b3013
> (builtin/receive-pack: stage incoming objects via ODB transactions,
> 2026-07-10).
>
> - jk/git-hash-cleanups at 9e396aa553 (hash: check ctx->active flag in
> all wrapper functions, 2026-07-07).
>
> Note that jt/receive-pack-use-odb-transaction requires an evil merge:
>
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 06b31dd743..cbb06da038 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -545,7 +545,8 @@ static int odb_source_packed_write_object_stream(struct odb_source *source UNUSE
> }
>
> static int odb_source_packed_begin_transaction(struct odb_source *source UNUSED,
> - struct odb_transaction **out UNUSED)
> + struct odb_transaction **out UNUSED,
> + enum odb_transaction_flags flags UNUSED)
> {
> return error("packed backend cannot begin transactions");
> }
It seems that you performed this evil merge when merging the topic
jt/receive-pack-use-odb-transaction into jch as 9727bd8447 (Merge
branch 'jt/receive-pack-use-odb-transactions' into jch, 2026-07-17),
but forgot to do so when creating the base for this patch series as
1d64e64326 (Merge branch 'jt/receive-pack-use-odb-transactions' into
ps/odb-move-loose-object-writing, 2026-07-17). Consequently, neither
1d64e64326 nor any of the the commits of this patch series can be
built because of the mismatching function signature:
GIT_VERSION=2.55.0.256.g1d64e64326
odb/source-packed.c: In function ‘odb_source_packed_new’:
odb/source-packed.c:778:40: error: assignment to ‘int (*)(struct odb_source *, struct odb_transaction **, enum odb_transaction_flags)’ from incompatible pointer type ‘int (*)(struct odb_source *, struct odb_transaction **)’ [-Werror=incompatible-pointer-types]
778 | packed->base.begin_transaction = odb_source_packed_begin_transaction;
| ^
cc1: all warnings being treated as errors
make: *** [Makefile:2924: odb/source-packed.o] Error 1
^ permalink raw reply
* Re: [PATCH 0/4] send-pack: introduce a `no-ref-delta` capability
From: Taylor Blau @ 2026-07-18 21:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20260718090527.GA30873@coredump.intra.peff.net>
On Sat, Jul 18, 2026 at 05:05:27AM -0400, Jeff King wrote:
> On Tue, Jul 14, 2026 at 02:58:36PM -0700, Taylor Blau wrote:
>
> > > So I have the feeling that there's some ulterior motive, or that this is
> > > part of a larger system, but I don't quite understand what it is. And so
> > > it's hard to say whether this is a sensible approach.
> >
> > The implementation motivating this is write-through in the sense that it
> > first parses and spools the incoming pack, then replays those exact
> > bytes together with the same ref commands to an upstream receive-pack.
>
> OK, that kind-of makes sense. But is that intermediate layer not
> indexing at all? As in, it does not know which OIDs are present in the
> incoming pack?
It does. The receive path computes object IDs while ingesting and
validating the incoming pack. The index I was referring to is the
durable index for the stored pack, not what we would compute in memory
during receipt of the incoming push. That durable index is built
asynchronously after the push is accepted.
> It sounds like you _do_ index it based on this:
>
> > In retrospect, I don't think the cover letter distinguishes this well.
> > The pack that we receive over the wire is stored byte-for-byte as an
> > immutable artifact, and the per-object physical index is derived
> > asynchronously. That indexer is designed to operate in a single pass
> > forward over the pack.
>
> So I can see how REF_DELTA makes a single streaming pass harder. But can
> you actually do a single pass even with OFS_DELTA? You'll need to look
> at earlier bytes in order to resolve each new object. And possibly
> recursively. So you must maintain random access to the new pack data,
> and you must maintain some kind of in-memory structure, at least for
> each delta family (especially if you want to avoid re-generating deltas
> over and over as you satisfy a long chain).
"Single pass" here means one forward read of the pack entries, with
random access to a disk-backed store of already-resolved objects. But...
> I.e., I am having trouble understanding what makes REF_DELTA more
> expensive than OFS_DELTA, either in terms of random-access to the pack
> or in terms of indexing memory. It's more _complicated_ for sure,
> though.
...by restricting objects to only be encoded as OFS_DELTA, we can always
load the base object knowing that it has already been processed. That
trivially permits you to avoid holding the entire pack in resident
memory, since you are able to look-up the right chunk(s) as needed.
If you have REF_DELTAs, the story is much more complicated. In that
case, you must be prepared to keep unresolved deltas around until the
base appears, which may be very far ahead in the pack. Alternatively,
you could build a map of pack positions by OID, but even in that case
you have to keep a pending work queue to be prepared for deltas whose
base object we have not yet seen.
> That doesn't seem all _that_ compelling to me. I think the more
> interesting thing is that in the worst case, a single REF_DELTA can kill
> your pipeline entirely (assuming you are trying to do as much CPU-heavy
> resolving work as possible while the pack is transferring). Imagine a
> pack like this:
>
> 0: REF_DELTA(abcd)
> 1: OFS_DELTA(offsetof(0))
> 2: OFS_DELTA(offsetof(1))
> 3: OFS_DELTA(offsetof(2))
> ...
> N: base object with hash "abcd"
>
> You can't resolve a single delta until you hit object N, after which you
> must then resolve each of 0..N-1 sequentially because they all depend on
> each other.
>
> The problem there is not REF_DELTA itself, but the fact that REF_DELTA
> allows you to place a base after the delta which depends on it.
Exactly.
> If _that_ is your main concern, would it be worth a tighter capability
> advertisement that insists that bases come before their deltas (if they
> are in the pack at all)? We already generate packs that way by default,
> and it would really just give the server a license to reject these
> non-standard packs.
That would address the ordering problem, but is weaker than the format
restriction this receiver wants. Even a backward REF_DELTA requires an
OID-to-entry lookup, whereas the retained pack's reconstruction metadata
is addressed by offset alone. Supporting that is possible, of course,
but adds another way to locate a base.
> But now I'm about 3 levels deep in guessing at your real issues, so I'll
> stop for now and see how close I got. ;)
I think you got pretty close ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/4] last-modified: check pathspec against Bloom filter first
From: Taylor Blau @ 2026-07-18 21:22 UTC (permalink / raw)
To: Jeff King; +Cc: Toon Claes, git, Gusted
In-Reply-To: <20260718083757.GD22588@coredump.intra.peff.net>
On Sat, Jul 18, 2026 at 04:37:57AM -0400, Jeff King wrote:
> > I don't think this is safe with '--show-trees'. The original pathspec
> > does not cover every entry in 'lm->paths', since the function
> > 'populate_paths_from_revs()' also adds ancestor tree entries.
>
> Hmm, interesting. I am surprised to learn that "-t" includes "d" when
> the pathspec asked for "d/a". I thought it was mostly about showing
> "d/a" when we recurse to find "d/a/b". But I guess it does not make a
> distinction between the two (probably because it is just telling the
> diff code to show trees, and it does not further apply the pathspec to
> the output).
>
> Does this mean there is also a bug in "git log"? I guess not, because it
> is purely pruning based on the pathspec, and only shows "d/" for those
> commits.
Right.
> > I think that the conditional is otherwise correct, if guarded when we
> > know that 'lm->show_trees' is false, like so:
> >
> > if (!lm->show_trees &&
> > !revs_maybe_changed_in_bloom(&lm->rev, filter))
> > return false;
>
> Hmph. That makes this optimization all but useless, because the intended
> use case of last-modified is almost always going to use "-t" to be able
> to mark the interior trees. And most callers are not going to care about
> seeing "d" here; their purpose was to find out about the things _inside_
> "d".
>
> Would we consider removing "d" from the output for this case? Presumably
> by double-checking the pathspecs again in add_path_from_diff(). That
> gives less surprising output (to me, anyway) and would enable this
> optimization. And the command is still marked as experimental, and I
> think this is exactly the kind of corner case that is meant to cover.
I think that we could feasibly get rid of "d" in the output in this
particular case within last-modified. As you note, the command is marked
EXPERIMENTAL for a reason, after all ;-).
If we wanted to do that, it should be straightforward to do. I think the
following (untested) patch would be sufficient:
--- 8< ---
diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index adc7cd8c74..0f0c1d1d17 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -103,7 +103,7 @@ struct last_modified_callback_data {
};
static void add_path_from_diff(struct diff_queue_struct *q,
- struct diff_options *opt UNUSED, void *data)
+ struct diff_options *opt, void *data)
{
struct last_modified *lm = data;
@@ -112,6 +112,11 @@ static void add_path_from_diff(struct diff_queue_struct *q,
struct last_modified_entry *ent;
const char *path = p->two->path;
+ if (!match_pathspec(opt->repo->index, &opt->pathspec, path,
+ strlen(path), 0, NULL,
+ S_ISDIR(p->two->mode)))
+ continue;
+
FLEX_ALLOC_STR(ent, path, path);
oidcpy(&ent->oid, &p->two->oid);
if (lm->rev.bloom_filter_settings)
--- >8 ---
If, on the other hand, we wanted to retain "d" in the output (which I am
inclined to suggest is a bad idea), we could keep a list of paths which
are not covered by the given pathspec.
If you had such a list, you could check only active entries within that
list, removing them as they are resolved. That makes a Bloom query miss
O(U*H) (where U is the uncovered subset of all paths, and H is the
number of hash functions in our Bloom key, which in our case is 7) as
opposed to O(P*H), where P is the number of active paths.
Of course, as U approaches P, the advantage disappears and so too do
the benefits of Toon's optimization.
If you wanted to go that route, you could do something like the
following (lightly tested):
--- 8< ---
diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index adc7cd8c74..e69c7a44b6 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -11,6 +11,7 @@
#include "ewah/ewok.h"
#include "hashmap.h"
#include "hex.h"
+#include "list.h"
#include "object-name.h"
#include "object.h"
#include "parse-options.h"
@@ -25,9 +26,11 @@
struct last_modified_entry {
struct hashmap_entry hashent;
+ struct list_head uncovered;
struct object_id oid;
struct bloom_key key;
size_t diff_idx;
+ bool covered_by_pathspec;
const char path[FLEX_ARRAY];
};
@@ -52,6 +55,7 @@ define_commit_slab(active_paths_for_commit, struct bitmap *);
struct last_modified {
struct hashmap paths;
+ struct list_head uncovered_paths;
struct rev_info rev;
bool show_trees;
bool nul_termination;
@@ -103,7 +107,7 @@ struct last_modified_callback_data {
};
static void add_path_from_diff(struct diff_queue_struct *q,
- struct diff_options *opt UNUSED, void *data)
+ struct diff_options *opt, void *data)
{
struct last_modified *lm = data;
@@ -114,6 +118,16 @@ static void add_path_from_diff(struct diff_queue_struct *q,
FLEX_ALLOC_STR(ent, path, path);
oidcpy(&ent->oid, &p->two->oid);
+
+ if (match_pathspec(opt->repo->index, &opt->pathspec, path,
+ strlen(path), 0, NULL,
+ S_ISDIR(p->two->mode))) {
+ ent->covered_by_pathspec = true;
+ } else {
+ list_add_tail(&ent->uncovered, &lm->uncovered_paths);
+ ent->covered_by_pathspec = false;
+ }
+
if (lm->rev.bloom_filter_settings)
bloom_key_fill(&ent->key, path, strlen(path),
lm->rev.bloom_filter_settings);
@@ -202,6 +216,8 @@ static void mark_path(const char *path, const struct object_id *oid,
last_modified_emit(data->lm, path, data->commit);
hashmap_remove(&data->lm->paths, &ent->hashent, path);
+ if (!ent->covered_by_pathspec)
+ list_del(&ent->uncovered);
bloom_key_clear(&ent->key);
free(ent);
}
@@ -272,8 +288,22 @@ static bool maybe_changed_path(struct last_modified *lm,
if (!filter)
return true;
- if (revs_maybe_changed_in_bloom(&lm->rev, filter) == 0)
+ if (revs_maybe_changed_in_bloom(&lm->rev, filter) == 0) {
+ struct list_head *pos;
+
+ list_for_each(pos, &lm->uncovered_paths) {
+ ent = list_entry(pos, struct last_modified_entry,
+ uncovered);
+ if (active && !bitmap_get(active, ent->diff_idx))
+ continue;
+
+ if (bloom_filter_contains(filter, &ent->key,
+ lm->rev.bloom_filter_settings))
+ return true;
+ }
+
return false;
+ }
hashmap_for_each_entry(&lm->paths, &iter, ent, hashent) {
if (active && !bitmap_get(active, ent->diff_idx))
@@ -490,6 +520,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
struct last_modified_entry *ent;
hashmap_init(&lm->paths, last_modified_entry_hashcmp, NULL, 0);
+ INIT_LIST_HEAD(&lm->uncovered_paths);
repo_init_revisions(r, &lm->rev, prefix);
lm->rev.def = "HEAD";
--- >8 ---
On my machine, in a synthetic repository containing 10,000 commits with
5,001 covered paths and 1 uncovered path, Toon's original patch runs in
~450ms. With the above patch, the timing drops to ~227ms, whereas it
drops further to ~190ms when omitting the uncovered path entirely.
So I'm inclined to suggest that we take advantage of the command's
EXPERIMENTAL nature and avoid printing the uncovered path entirely.
Thanks,
Taylor
^ permalink raw reply related
* [PATCH GSoC v20 00/13] cat-file: add remote-object-info to batch-command
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260717-ps-eric-work-rebase-v19-0-d4faee35764b@gmail.com>
This patch series is a continuation of Eric Ju's
(eric.peijian@gmail.com) and Calvin Wan's (calvinwan@google.com) patch
series [1] and [2] respectively.
Sometimes it is beneficial to retrieve information about an object
without having to download it completely. The server logic for
retrieving size has already been implemented and merged in a2ba162cda
(object-info: support for retrieving object info, 2021-04-20) [3].
This patch series implements the client option for it.
Eric's series adds the remote-object-info command to cat-file
--batch-command. This command allows the client to make an object-info
command request to a server that supports protocol v2.
If the server uses protocol v2 but does not support the object-info
capability, cat-file --batch-command will die.
If a user attempts to use remote-object-info with protocol v1, cat-file
--batch-command will die.
Currently, only the size (%(objectsize)) is supported end to end in this
implementation. The type (%(objecttype)) is known by the client's
allow-list and request path but is not supported on the server side
nor the response parsing. A follow up series will add full end-to-end
support for %(objecttype).
The default format for remote-object-info is set to "%(objectname)
%(objectsize)". Once %(objecttype) is supported, the default format will
be unified accordingly.
If the batch command format includes unsupported fields such as
%(objecttype), %(objectsize:disk), or %(deltabase), the command will
return empty strings for each unsupported field.
This series completes Eric's work mainly with the refactor of the
validation of the placeholders with an allow-list that filters what the
client asks with what the server is capable of providing, following Jeff
King's idea [4].
GitHub CI: https://github.com/pabloosabaterr/git/actions/runs/29660677279
[1]: https://lore.kernel.org/git/20250221190451.12536-1-eric.peijian@gmail.com/
[2]: https://lore.kernel.org/git/20220728230210.2952731-1-calvinwan@google.com/#t
[3]: https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2ba162cda2acc171c3e36acbbc854792b093cb7
[4]: https://lore.kernel.org/git/20250313060250.GH94015@coredump.intra.peff.net/
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()
---
Calvin Wan (3):
fetch-pack: move fetch initialization
serve: advertise object-info feature
transport: add client support for object-info
Eric Ju (3):
cat-file: declare loop counter inside for()
t1006: extract helper functions into new 'lib-cat-file.sh'
cat-file: add remote-object-info to batch-command
Pablo Sabater (7):
transport-helper: fix memory leak of helper on disconnect
fetch-pack: drop the static advertise_sid variable
fetch-pack: use unsigned int for hash_algo variable
fetch-pack: move write_fetch_command_and_capabilities() to connect.c
connect: make write_fetch_command_and_capabilities() more generic
protocol-caps: check object existence regardless of the attributes requested
cat-file: make remote-object-info allow-list adapt to the server
Documentation/git-cat-file.adoc | 24 +-
Documentation/gitprotocol-v2.adoc | 21 +-
Makefile | 1 +
builtin/cat-file.c | 222 +++++++++-
connect.c | 34 ++
connect.h | 8 +
fetch-object-info.c | 156 +++++++
fetch-object-info.h | 25 ++
fetch-pack.c | 58 +--
meson.build | 1 +
object-file.c | 10 +
odb.h | 9 +
protocol-caps.c | 45 +-
serve.c | 5 +-
t/lib-cat-file.sh | 16 +
t/meson.build | 1 +
t/t1006-cat-file.sh | 15 +-
t/t1017-cat-file-remote-object-info.sh | 747 +++++++++++++++++++++++++++++++++
t/t5701-git-serve.sh | 63 +++
transport-helper.c | 12 +-
transport-internal.h | 8 +
transport.c | 44 ++
transport.h | 9 +
23 files changed, 1442 insertions(+), 92 deletions(-)
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.
`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 }
++
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
^ permalink raw reply
* [PATCH GSoC v20 01/13] transport-helper: fix memory leak of helper on disconnect
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
disconnect_helper() only frees data inside of the if(data->helper) block
[1]. When the transport is disconnected without the helper being fully
started, data->name allocated in transport_helper_init()
is never freed.
Move FREE_AND_NULL(data->name) outside the conditional block so it's
always freed on disconnect.
[1]: https://lore.kernel.org/git/05fbadbae2184479c87c37675dde7bd79b3e32ab.1716465556.git.ps@pks.im/
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index 80f90eb7ba..f195070788 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -266,9 +266,9 @@ static int disconnect_helper(struct transport *transport)
close(data->helper->out);
fclose(data->out);
res = finish_command(data->helper);
- FREE_AND_NULL(data->name);
FREE_AND_NULL(data->helper);
}
+ FREE_AND_NULL(data->name);
return res;
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 02/13] cat-file: declare loop counter inside for()
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
Declare loop counters in the for statement when they are only used
within the loop body, limiting their scope and improving readability.
While updating the loop counters, use size_t instead of int for counters
that iterate over object counts.
Update the 'nr' parameter of dispatch_calls() to size_t as all callers
already pass a value of that type.
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
builtin/cat-file.c | 13 ++++---------
fetch-pack.c | 3 +--
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b4b99a73da..03afc44c5e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -721,14 +721,12 @@ static void dispatch_calls(struct batch_options *opt,
struct strbuf *output,
struct expand_data *data,
struct queued_cmd *cmd,
- int nr)
+ size_t nr)
{
- int i;
-
if (!opt->buffer_output)
die(_("flush is only for --buffer mode"));
- for (i = 0; i < nr; i++)
+ for (size_t i = 0; i < nr; i++)
cmd[i].fn(opt, cmd[i].line, output, data);
fflush(stdout);
@@ -736,9 +734,7 @@ static void dispatch_calls(struct batch_options *opt,
static void free_cmds(struct queued_cmd *cmd, size_t *nr)
{
- size_t i;
-
- for (i = 0; i < *nr; i++)
+ for (size_t i = 0; i < *nr; i++)
FREE_AND_NULL(cmd[i].line);
*nr = 0;
@@ -765,7 +761,6 @@ static void batch_objects_command(struct batch_options *opt,
size_t alloc = 0, nr = 0;
while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {
- int i;
const struct parse_cmd *cmd = NULL;
const char *p = NULL, *cmd_end;
struct queued_cmd call = {0};
@@ -775,7 +770,7 @@ static void batch_objects_command(struct batch_options *opt,
if (isspace(*input.buf))
die(_("whitespace before command: '%s'"), input.buf);
- for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
if (!skip_prefix(input.buf, commands[i].name, &cmd_end))
continue;
diff --git a/fetch-pack.c b/fetch-pack.c
index 29c41132ee..9eb8fc5399 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
if (advertise_sid && server_supports_v2("session-id"))
packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
if (server_options && server_options->nr) {
- int i;
ensure_server_supports_v2("server-option");
- for (i = 0; i < server_options->nr; i++)
+ for (size_t i = 0; i < server_options->nr; i++)
packet_buf_write(req_buf, "server-option=%s",
server_options->items[i].string);
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 03/13] t1006: extract helper functions into new 'lib-cat-file.sh'
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
Extract utility functions from the cat-file's test script
't1006-cat-file.sh' into a new 'lib-cat-file.sh' dedicated library file.
A subsequent commit will need these functions. This improves the code
reuse and readability, enabling future cat-file tests to share these
helpers without duplicating code.
While at it update the style of this line to follow coding
guidelines:
. "$TEST_DIRECTORY/lib-loose.sh"
to
. "$TEST_DIRECTORY"/lib-loose.sh
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
t/lib-cat-file.sh | 16 ++++++++++++++++
t/t1006-cat-file.sh | 15 ++-------------
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/t/lib-cat-file.sh b/t/lib-cat-file.sh
new file mode 100644
index 0000000000..7c2e877016
--- /dev/null
+++ b/t/lib-cat-file.sh
@@ -0,0 +1,16 @@
+# Library of git-cat-file related test functions.
+
+# Print a string without a trailing newline.
+echo_without_newline () {
+ printf '%s' "$*"
+}
+
+# Print a string without newlines and replace them with a NUL character (\0).
+echo_without_newline_nul () {
+ echo_without_newline "$@" | tr '\n' '\0'
+}
+
+# Calculate the length of a string.
+strlen () {
+ echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
+}
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8e2c52652c..cf65bfc88f 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -3,7 +3,8 @@
test_description='git cat-file'
. ./test-lib.sh
-. "$TEST_DIRECTORY/lib-loose.sh"
+. "$TEST_DIRECTORY"/lib-loose.sh
+. "$TEST_DIRECTORY"/lib-cat-file.sh
test_cmdmode_usage () {
test_expect_code 129 "$@" 2>err &&
@@ -99,18 +100,6 @@ do
'
done
-echo_without_newline () {
- printf '%s' "$*"
-}
-
-echo_without_newline_nul () {
- echo_without_newline "$@" | tr '\n' '\0'
-}
-
-strlen () {
- echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
-}
-
run_tests () {
type=$1
object_name="$2"
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 04/13] fetch-pack: drop the static advertise_sid variable
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
write_fetch_command_and_capabilities() is moved to 'connect.c' in a
subsequent commit. To prepare for that, drop the static variable usage
of advertise_sid.
Currently advertise_sid is set in fetch_pack_config() by reading
"transfer.advertisesid". It is used in three places:
1. In do_fetch_pack(), to clear it when the server lacks support:
if (!server_supports("session-id"))
advertise_sid = 0;
2. In find_common(), to advertise the session id over protocol v0/v1:
if (advertise_sid)
strbuf_addf(&c, " session-id=%s", trace2_session_id());
3. In write_fetch_command_and_capabilities(), to advertise it over
protocol v2:
if (advertise_sid && server_supports_v2("session-id"))
packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
About 1, the check only guards the v0/v1 path, and the v2 path
already checks server support inline in its condition. Follow the
same pattern and fold the check into the condition in find_common().
About 2 and 3, replace the static variable with a local read via
repo_config_get_bool() in each function.
Because repo_config_get_bool() leaves advertise_sid as is if it is not
set, initialize it to 0, matching its default.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 9eb8fc5399..65ebfec09f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
static int transfer_fsck_objects = -1;
static int agent_supported;
static int server_supports_filtering;
-static int advertise_sid;
static struct shallow_lock shallow_lock;
static const char *alternate_shallow_file;
static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
size_t state_len = 0;
struct packet_reader reader;
struct oidset negotiation_include_oids = OIDSET_INIT;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (args->stateless_rpc && multi_ack == 1)
die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
if (deepen_not_ok) strbuf_addstr(&c, " deepen-not");
if (agent_supported) strbuf_addf(&c, " agent=%s",
git_user_agent_sanitized());
- if (advertise_sid)
+ if (advertise_sid && server_supports("session-id"))
strbuf_addf(&c, " session-id=%s", trace2_session_id());
if (args->filter_options.choice)
strbuf_addstr(&c, " filter");
@@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
(int)agent_len, agent_feature);
}
- if (!server_supports("session-id"))
- advertise_sid = 0;
-
if (server_supports("shallow"))
print_verbose(args, _("Server supports %s"), "shallow");
else if (args->depth > 0 || is_repository_shallow(r))
@@ -1380,6 +1379,9 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
const struct string_list *server_options)
{
const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
ensure_server_supports_v2("fetch");
packet_buf_write(req_buf, "command=fetch");
@@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (!uri_protocols.nr) {
char *str;
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 05/13] fetch-pack: use unsigned int for hash_algo variable
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
hash_algo_by_name() returns unsigned int, but it is stored in
hash_algo variable as int. This goes unnoticed because of:
DISABLE_SIGN_COMPARE_WARNINGS
On 'fetch-pack.c'
On a subsequent commit this function will be moved to 'connect.c' that
would notice this.
Change hash_algo variable type to match its return type, also make it
const because they are never modified.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 65ebfec09f..f1e64160fc 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1397,7 +1397,7 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
}
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);
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 06/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
In a subsequent commit write_fetch_command_and_capabilities() will be
refactored to a more general-purpose function, making it more accessible
to additional commands in the future.
Move write_fetch_command_and_capabilities() to 'connect.c', where
there are similar purpose functions.
Because string_list is only used as a pointer, use a forward
declaration [1].
[1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
connect.c | 34 ++++++++++++++++++++++++++++++++++
connect.h | 4 ++++
fetch-pack.c | 34 ----------------------------------
3 files changed, 38 insertions(+), 34 deletions(-)
diff --git a/connect.c b/connect.c
index 47e39d2a73..c09947cc56 100644
--- a/connect.c
+++ b/connect.c
@@ -700,6 +700,40 @@ int server_supports(const char *feature)
return !!server_feature_value(feature, NULL);
}
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options)
+{
+ const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
+
+ ensure_server_supports_v2("fetch");
+ packet_buf_write(req_buf, "command=fetch");
+ if (server_supports_v2("agent"))
+ packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
+ if (advertise_sid && server_supports_v2("session-id"))
+ packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
+ if (server_options && server_options->nr) {
+ ensure_server_supports_v2("server-option");
+ for (size_t i = 0; i < server_options->nr; i++)
+ packet_buf_write(req_buf, "server-option=%s",
+ server_options->items[i].string);
+ }
+
+ if (server_feature_v2("object-format", &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);
+ packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
+ } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
+ die(_("the server does not support algorithm '%s'"),
+ the_hash_algo->name);
+ }
+ packet_buf_delim(req_buf);
+}
+
static const char *url_scheme_name(enum url_scheme scheme)
{
switch (scheme) {
diff --git a/connect.h b/connect.h
index aa482a37fb..c4f6ea4b0a 100644
--- a/connect.h
+++ b/connect.h
@@ -34,4 +34,8 @@ void check_stateless_delimiter(int stateless_rpc,
struct packet_reader *reader,
const char *error);
+struct string_list;
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options);
+
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index f1e64160fc..f7789e8456 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1375,40 +1375,6 @@ static int add_haves(struct fetch_negotiator *negotiator,
return haves_added;
}
-static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
- const struct string_list *server_options)
-{
- const char *hash_name;
- int advertise_sid = 0;
-
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
-
- ensure_server_supports_v2("fetch");
- packet_buf_write(req_buf, "command=fetch");
- if (server_supports_v2("agent"))
- packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
- if (advertise_sid && server_supports_v2("session-id"))
- packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
- if (server_options && server_options->nr) {
- ensure_server_supports_v2("server-option");
- for (size_t i = 0; i < server_options->nr; i++)
- packet_buf_write(req_buf, "server-option=%s",
- server_options->items[i].string);
- }
-
- if (server_feature_v2("object-format", &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);
- packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
- } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
- die(_("the server does not support algorithm '%s'"),
- the_hash_algo->name);
- }
- packet_buf_delim(req_buf);
-}
-
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 07/13] connect: make write_fetch_command_and_capabilities() more generic
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
Refactor write_fetch_command_and_capabilities(), enabling it to serve
both fetch and additional commands.
In this context, "command" refers to the "operations" supported by
Git's wire protocol Documentation/gitprotocol-v2.adoc, such as a Git
subcommand (e.g., git-fetch(1)) or a server-side operation like
"object-info" as implemented in commit a2ba162cda
(object-info: support for retrieving object info, 2021-04-20).
Refactor the function signature to accept a command instead of the
hardcoded "fetch".
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
connect.c | 8 ++++----
connect.h | 8 ++++++--
fetch-pack.c | 4 ++--
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/connect.c b/connect.c
index c09947cc56..127ed4a2e6 100644
--- a/connect.c
+++ b/connect.c
@@ -700,16 +700,16 @@ int server_supports(const char *feature)
return !!server_feature_value(feature, NULL);
}
-void write_fetch_command_and_capabilities(struct strbuf *req_buf,
- const struct string_list *server_options)
+void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
+ const struct string_list *server_options)
{
const char *hash_name;
int advertise_sid = 0;
repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
- ensure_server_supports_v2("fetch");
- packet_buf_write(req_buf, "command=fetch");
+ ensure_server_supports_v2(command);
+ packet_buf_write(req_buf, "command=%s", command);
if (server_supports_v2("agent"))
packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
if (advertise_sid && server_supports_v2("session-id"))
diff --git a/connect.h b/connect.h
index c4f6ea4b0a..957e5fe2b9 100644
--- a/connect.h
+++ b/connect.h
@@ -35,7 +35,11 @@ void check_stateless_delimiter(int stateless_rpc,
const char *error);
struct string_list;
-void write_fetch_command_and_capabilities(struct strbuf *req_buf,
- const struct string_list *server_options);
+/*
+ * Write a protocol v2 command request, along with the capability
+ * advertisements, into req_buf.
+ */
+void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
+ const struct string_list *server_options);
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index f7789e8456..3695059cd5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1386,7 +1386,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
int done_sent = 0;
struct strbuf req_buf = STRBUF_INIT;
- write_fetch_command_and_capabilities(&req_buf, args->server_options);
+ write_command_and_capabilities(&req_buf, "fetch", args->server_options);
if (args->use_thin_pack)
packet_buf_write(&req_buf, "thin-pack");
@@ -2253,7 +2253,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_restrict_tips,
the_repository, "%d",
negotiation_round);
strbuf_reset(&req_buf);
- write_fetch_command_and_capabilities(&req_buf, server_options);
+ write_command_and_capabilities(&req_buf, "fetch", server_options);
packet_buf_write(&req_buf, "wait-for-done");
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 08/13] fetch-pack: move fetch initialization
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
From: Calvin Wan <calvinwan@google.com>
There are some variables initialized at the start of the
do_fetch_pack_v2() state machine. Currently, they are initialized in
FETCH_CHECK_LOCAL, which is the initial state set at the beginning
of the function.
However, a subsequent patch will allow for another initial state,
while still requiring these initialized variables.
Move the initialization to be before the state machine,
so that they are set regardless of the initial state.
Note that there is no change in behavior, because we're moving code
from the beginning of the first state to just before the execution of
the state machine.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 3695059cd5..922a9b2581 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1735,18 +1735,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
reader.me = "fetch-pack";
}
+ /* v2 supports these by default */
+ allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
+ use_sideband = 2;
+ if (args->depth > 0 || args->deepen_since || args->deepen_not)
+ args->deepen = 1;
+
while (state != FETCH_DONE) {
switch (state) {
case FETCH_CHECK_LOCAL:
sort_ref_list(&ref, ref_compare_name);
QSORT(sought, nr_sought, cmp_ref_by_name);
- /* v2 supports these by default */
- allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
- use_sideband = 2;
- if (args->depth > 0 || args->deepen_since || args->deepen_not)
- args->deepen = 1;
-
/* Filter 'ref' by 'sought' and those that aren't local */
mark_complete_and_common_ref(negotiator, args, &ref);
filter_refs(args, &ref, sought, nr_sought);
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v20 09/13] protocol-caps: check object existence regardless of the attributes requested
From: Pablo Sabater @ 2026-07-18 21:49 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, chandrapratap3519, chriscool, eric.peijian,
gitster, jltobler, karthik.188, peff, toon, szeder.dev
In-Reply-To: <20260718-ps-eric-work-rebase-v20-0-0c13962ac532@gmail.com>
Currently, send_info() only checks for existence when the attribute
'size' is also requested. Requesting a bare OID, without attributes only
echoes back the OID.
Extract the existence check to be done regardless of the number of
attributes requested.
While at it, introduce a wrapper called get_object_info() similar to
odb_read_object_info() that returns OBJ_BAD on fail and adds
OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK flags.
OBJECT_INFO_SKIP_FETCH_OBJECT is so a server with a partial clone
doesn't trigger fetching objects when it gets an object-info request
with an OID that is not available locally. A server should only report
what it has locally.
Tighten the condition used to determine whether an object is
recognized. get_object_info() returns OBJ_BAD for unknown objects,
but OBJ_NONE (0) can also mean "not found". Change the check from '< 0'
to '<= OBJ_NONE' to cover both as unrecognized.
With this patch, a bare OID has two possible responses:
1. Recognized OID: the server answers with "<OID>"
2. Unrecognized OID: the server answers with "<OID> SP"
Update the object-info section in 'gitprotocol-v2.adoc':
- Require full obj-oid explicitly.
- Fix parentheses.
- Define obj-size explicitly.
- Make obj-size optional in obj-info and document the behavior
for unrecognized object IDs.
- Describe the attr header as zero or more pkt-lines, one per attribute,
matching what the server implements. A request with no attributes gets
no header.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Documentation/gitprotocol-v2.adoc | 21 ++++++++-----
protocol-caps.c | 45 ++++++++++++++++++++++++----
t/t5701-git-serve.sh | 63 +++++++++++++++++++++++++++++++++++++++
3 files changed, 115 insertions(+), 14 deletions(-)
diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
index 2beb70595f..7bf62014c3 100644
--- a/Documentation/gitprotocol-v2.adoc
+++ b/Documentation/gitprotocol-v2.adoc
@@ -568,21 +568,26 @@ An `object-info` request takes the following arguments:
oid <oid>
Indicates to the server an object which the client wants to obtain
- information for.
+ information for. They must be full OIDs.
-The response of `object-info` is a list of the requested object ids
-and associated requested information, each separated by a single space.
+The response of `object-info` consists of one pkt-line per requested attribute,
+echoing the attributes the server will report, followed by one pkt-line per
+requested object id with its information, each field separated by a single
+space.
output = info flush-pkt
- info = PKT-LINE(attrs) LF)
- *PKT-LINE(obj-info LF)
-
- attrs = attr | attrs SP attrs
+ info = *PKT-LINE(attr LF)
+ *PKT-LINE(obj-info LF)
attr = "size"
- obj-info = obj-id SP obj-size
+ obj-size = 1*DIGIT
+
+ obj-info = obj-id [SP [obj-size]]
+
+If the server does not recognize the OID, the response will be `<oid> SP`
+regardless of the number of attributes requested.
bundle-uri
~~~~~~~~~~
diff --git a/protocol-caps.c b/protocol-caps.c
index 8858ea4489..02261be14d 100644
--- a/protocol-caps.c
+++ b/protocol-caps.c
@@ -30,6 +30,32 @@ static int parse_oid(const char *line, struct string_list *oid_str_list)
return 1;
}
+/*
+ * odb_read_object_info_extended() wrapper. Similar to odb_read_object_info()
+ * but uses the flags:
+ *
+ * - OBJECT_INFO_SKIP_FETCH_OBJECT so a server won't fetch an object when a
+ * object-info request asks for an OID that it doesn't have.
+ *
+ * - OBJECT_INFO_QUICK to avoid re-scanning packs when the object is not found.
+ */
+static enum object_type get_object_info(struct object_database *odb,
+ const struct object_id *oid,
+ size_t *sizep)
+{
+ enum object_type type;
+ struct object_info oi = OBJECT_INFO_INIT;
+
+ oi.typep = &type;
+ oi.sizep = sizep;
+ if (odb_read_object_info_extended(odb, oid, &oi,
+ OBJECT_INFO_LOOKUP_REPLACE |
+ OBJECT_INFO_SKIP_FETCH_OBJECT |
+ OBJECT_INFO_QUICK) < 0)
+ return OBJ_BAD;
+ return type;
+}
+
/*
* Validates and send requested info back to the client. Any errors detected
* are returned as they are detected.
@@ -62,15 +88,22 @@ static void send_info(struct repository *r, struct packet_writer *writer,
strbuf_addstr(&send_buffer, oid_str);
+ /*
+ * Check the existence of the object first.
+ * If an object is not recognized by the server append SP to
+ * the response.
+ */
+ if (get_object_info(r->objects, &oid, &object_size) <= OBJ_NONE) {
+ strbuf_addstr(&send_buffer, " ");
+ goto write;
+ }
+
if (info->size) {
- if (odb_read_object_info(r->objects, &oid, &object_size) < 0) {
- strbuf_addstr(&send_buffer, " ");
- } else {
- strbuf_addf(&send_buffer, " %"PRIuMAX,
- (uintmax_t)object_size);
- }
+ strbuf_addf(&send_buffer, " %"PRIuMAX,
+ (uintmax_t)object_size);
}
+write:
packet_writer_write(writer, "%s", send_buffer.buf);
strbuf_reset(&send_buffer);
}
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index d4c28bae39..cacff4456c 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+unknown_oid=$(printf "test" | git hash-object --stdin)
+
test_expect_success 'setup to generate files with expected content' '
printf "agent=git/%s" "$(git version | cut -d" " -f3)" >agent_capability &&
@@ -364,6 +366,67 @@ test_expect_success 'basics of object-info' '
test_cmp expect actual
'
+test_expect_success 'bare OID request' '
+ test_config transfer.advertiseObjectInfo true &&
+
+ test-tool pkt-line pack >in <<-EOF &&
+ command=object-info
+ object-format=$(test_oid algo)
+ 0001
+ oid $(git rev-parse two:two.t)
+ 0000
+ EOF
+
+ cat >expect <<-EOF &&
+ $(git rev-parse two:two.t)
+ 0000
+ EOF
+
+ test-tool serve-v2 --stateless-rpc <in >out &&
+ test-tool pkt-line unpack <out >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'object-info with bare unrecognized OID' '
+ test_config transfer.advertiseObjectInfo true &&
+
+ test-tool pkt-line pack >in <<-EOF &&
+ command=object-info
+ object-format=$(test_oid algo)
+ 0001
+ oid $unknown_oid
+ 0000
+ EOF
+
+ printf "%s \n" "$unknown_oid" >expect &&
+ printf "0000\n" >>expect &&
+
+ test-tool serve-v2 --stateless-rpc <in >out &&
+ test-tool pkt-line unpack <out >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'object-info with size for unrecognized OID' '
+ test_config transfer.advertiseObjectInfo true &&
+
+ test-tool pkt-line pack >in <<-EOF &&
+ command=object-info
+ object-format=$(test_oid algo)
+ 0001
+ size
+ oid $unknown_oid
+ 0000
+ EOF
+
+ printf "size\n" >expect &&
+ printf "%s \n" "$unknown_oid" >>expect &&
+ printf "0000\n" >>expect &&
+
+ test-tool serve-v2 --stateless-rpc <in >out &&
+ test-tool pkt-line unpack <out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'test capability advertisement with uploadpack.advertiseBundleURIs' '
test_config uploadpack.advertiseBundleURIs true &&
--
2.54.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox