* Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
From: Junio C Hamano @ 2026-06-26 21:39 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <e3d2e46443d0b32ce29215563dde04ebcf850679.1782500507.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> +/*
> + * Flag bit set on commits that belong to an included pack during
> + * '--stdin-packs=follow-reachable'. Used by the pre-walk to
> + * identify which reachable commits should be tips for the main
> + * object traversal.
> + */
> +#define IN_INCLUDED_PACK (1u<<11)
> +
> +static int mark_included_pack_tip(const struct object_id *oid,
> + struct packed_git *p,
> + uint32_t pos,
> + void *data)
> +{
> + struct rev_info *main_revs = data;
> + off_t ofs = nth_packed_object_offset(p, pos);
> + enum object_type type;
> + struct object_info oi = OBJECT_INFO_INIT;
> + struct object *obj;
> +
> + oi.typep = &type;
> + if (packed_object_info(p, ofs, &oi) < 0)
> + return 0;
> + if (type != OBJ_COMMIT && type != OBJ_TAG)
> + return 0;
We do not care about non commits, non tags.
> + obj = parse_object(the_repository, oid);
> + if (!obj)
> + return 0;
> +
> + obj->flags |= IN_INCLUDED_PACK;
> +
> + if (type == OBJ_TAG && main_revs)
> + add_pending_object(main_revs, obj, "");
Any tag object is added to the pending list of the second phase
traversal here. Doesn't this retain unreachable tags and
(unreachable) objects that are only reachable from these unreachable
tags found in the packfile? Don't we want to limit this code to add
only tags that actually are reachable from refs, or something?
> + return 0;
> +}
The other function ...
> +static int mark_loose_object_tip(const struct object_id *oid,
> + struct object_info *oi UNUSED,
> + void *data)
... is structured in a very similar way, and gives the same
puzzlement to me.
^ permalink raw reply
* Re: [PATCH v2 1/2] dir: encapsulate excludes_file lazy-load
From: Junio C Hamano @ 2026-06-26 21:45 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Tian Yuchen, git, cirnovskyv, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <aj7rtj9NsejqN357@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> On Fri, Jun 26, 2026 at 03:50:36PM +0800, Tian Yuchen wrote:
>> diff --git a/environment.c b/environment.c
>> index ba2c60103f..8efcaeafa6 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -134,6 +134,13 @@ int is_bare_repository(void)
>> return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
>> }
>>
>> +const char *repo_excludes_file(struct repository *repo)
>> +{
>> + if (!excludes_file)
>> + excludes_file = xdg_config_home("ignore");
>> + return excludes_file;
>> +}
>
> This function has a 'repo' parameter, which is not used in the
> function at all. This causes build failure when trying to build this
> commit using DEVELOPER=1:
>
> environment.c: In function ‘repo_excludes_file’:
> environment.c:137:51: error: unused parameter ‘repo’ [-Werror=unused-parameter]
> 137 | const char *repo_excludes_file(struct repository *repo)
> | ~~~~~~~~~~~~~~~~~~~^~~~
> cc1: all warnings being treated as errors
> make: *** [Makefile:2922: environment.o] Error 1
>
> Please make sure that all commits can be built with 'make
> DEVELOPER=1'.
Good point. In this case, we can start with UNUSED in step 1/2 and
then drop the UNUSED in the second step. I wonder how harder to read
it would become if these two patches are squashed together...
Thanks.
^ permalink raw reply
* Re: [PATCH v3 5/8] commit-reach: introduce struct paint_state with per-side counters
From: Kristofer Karlsson @ 2026-06-26 21:57 UTC (permalink / raw)
To: René Scharfe
Cc: Kristofer Karlsson via GitGitGadget, git, Derrick Stolee,
Elijah Newren
In-Reply-To: <bd37b80d-9eff-496d-8f1f-436594968678@web.de>
On Fri, 26 Jun 2026 at 23:13, René Scharfe <l.s.r@web.de> wrote:
>
> > +struct paint_state {
> > + struct prio_queue queue;
> > + int p1_count;
> > + int p2_count;
> > + int pending_merge_bases;
> > +};
> Can they become negative? Wouldn't size_t be a more natural fit,
> matching nr from struct prio_queue?
Negative would be a clear indication of a bug though that's
not checked right now anyway. And since it's not checked
we might as well use size_t instead - and it would technically
be more correct though I struggle to imagine a case where
the number of active elements in the frontier exceeds 2^31
or whatever a signed int would give.
I am happy to change to size_t.
> And some bikeshedding:
>
> Why abbreviate? parent1_count and parent2_count would be slightly
> easier to read and associate with PARENT1 and PARENT2.
>
> And pending_merge_bases is a counter as well. Why not call it
> like that, pending_merge_base_count? Well, that's pretty long.
> both_count? That's quite generic and nondescript. Call the other
> counters parents1 and parents2? Nah. Or parent1s and parent2s?
> Not sure why this inconsistency bothers me to begin with.
Fair point, I was thinking that the surrounding context is so small
that the naming almost doesn't matter - the terms don't
escape paint_down_to_common.
I am happy to change to something like:
parent1_count, parent2_count, mb_candidate_count
to make it more consistent.
It seems the mb_ prefix is already used for
merge bases in some files - best example is perhaps builtin/diff.c
I see in the codebase that we are using multiple styles,
perhaps depending on specific context.
- nr_ prefix: nr_objects, nr_paths_watching
- num_ prefix: num_commits, num_hashes, num_workers
- _count suffix: entry_count, max_count, skip_count
so I think _count suffix is a good choice at least - it matches
other usages where we typically just increment or decrement.
Thanks,
Kristofer
^ permalink raw reply
* Re: [L10N] Kickoff for Git 2.55.0 translations
From: Junio C Hamano @ 2026-06-26 22:05 UTC (permalink / raw)
To: Jiang Xin
Cc: Git List, Alexander Shopov, Mikel Forcada, Ralf Thielow,
Jean-Noël Avila, Bagas Sanjaya, Dimitriy Ryazantcev,
Peter Krefting, Emir SARI, Arkadii Yakovets,
Vũ Tiến Hưng, Teng Long, Yi-Jyun Pan
In-Reply-To: <20260613061658.1767987-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Git v2.55.0-rc0 has been released, and we are starting a new round of
> localization for Git 2.55.0. Since the last release, 125 catalog entries need
> to be translated. Please open a pull request against the l10n coordinator
> repository (URL below) before the update window closes on Sat, 27 Jun 2026.
> ...
> **Reminder: the update window closes on Sat, 27 Jun 2026.**
Thanks.
The cut-off date will be tomorrow. Hopefully I'll hear from you on
28th in time for the final on 29th.
^ permalink raw reply
* Re: [PATCH GSoC v14 09/13] serve: advertise object-info feature
From: Karthik Nayak @ 2026-06-26 22:23 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <20260625-ps-eric-work-rebase-v14-9-09f7ffe21a53@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> From: Calvin Wan <calvinwan@google.com>
>
> In order for a client to know what object-info components a server can
> provide, advertise supported object-info features. This will allow a
> client to decide whether to query the server for object-info or fetch
> as a fallback.
>
> 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>
> ---
> serve.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/serve.c b/serve.c
> index 49a6e39b1d..2b07d922b3 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -89,7 +89,7 @@ static void session_id_receive(struct repository *r UNUSED,
> trace2_data_string("transfer", NULL, "client-sid", client_sid);
> }
>
> -static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)
> +static int object_info_advertise(struct repository *r, struct strbuf *value)
> {
> if (advertise_object_info == -1 &&
> repo_config_get_bool(r, "transfer.advertiseobjectinfo",
> @@ -97,6 +97,9 @@ static int object_info_advertise(struct repository *r, struct strbuf *value UNUS
> /* disabled by default */
> advertise_object_info = 0;
> }
> + /* Currently only size is supported */
> + if (value && advertise_object_info)
> + strbuf_addstr(value, "size");
So is the plan that further options will be added here to value? If so,
whats the format we will follow?
> return advertise_object_info;
> }
>
>
> --
> 2.54.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [RFH] Why do osx CI jobs so unreliable?
From: Michael Montalbo @ 2026-06-26 23:26 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git, Junio C Hamano
In-Reply-To: <aj5ZaZK7xylfs4Xw@pks.im>
On Fri, Jun 26, 2026 at 3:50 AM Patrick Steinhardt <ps@pks.im> wrote:
> The bug manifests both with HTTP/1.1 and HTTP/2 though, so this wouldn't
> fully fix the flakes we see, right?
Yes you are right. The linked fix would just prevent the hanging after timeout
for HTTP/2 tests, but still leaves HTTP/1.1 fakes.
> I was also wondering whether we can maybe work around the issue by
> increasing the Apache timeout value. That sounds like an easy potential
> solution to try, and from all we've discovered so far it doesn't feel
> like this is something we can address on the Git side.
I think Peff and Patrick's suggestion to just increase the Apache timeout
makes sense. I ran some experiments using a really long timeout with an
artificially slowed down CI runner and all the jobs made progress
(if slowly) without stalling, and eventually completed successfully:
https://github.com/mmontalbo/git/actions/runs/28267019651
I haven't spent a lot of time trying to figure out what the right timeout
value should be. An hour definitely seems like overkill, with something
on the order of 5-10 minutes seeming more reasonable, but I don't
have a principled number.
^ permalink raw reply
* Re: [RFH] Why do osx CI jobs so unreliable?
From: Jeff King @ 2026-06-26 23:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <aj5ZaZK7xylfs4Xw@pks.im>
On Fri, Jun 26, 2026 at 12:50:17PM +0200, Patrick Steinhardt wrote:
> > Thanks both of you for digging into this. I'm not familiar enough with
> > Apache's code to pass confident judgement, but your findings certainly
> > convinced me that this is just an apache bug.
>
> The bug manifests both with HTTP/1.1 and HTTP/2 though, so this wouldn't
> fully fix the flakes we see, right?
If I understand the situation correctly, there are really two problems.
First, the EXPENSIVE tests in t5551 sometimes trigger a timeout with
Apache's stock settings. This presumably became a problem recently due
to 7a094d68a2 (ci: run expensive tests on push builds to integration
branches, 2026-05-08). The same problem exists in t5559, which just
wraps t5551 but tells us to use http2.
This timeout will cause test failures in t5551, because we aren't able
to complete a request we expected to. Obviously bad and annoying.
The second problem is that when Apache hits the timeout in HTTP/2 mode,
it hangs forever. And then the CI job hangs for 6 hours until it's
killed, which is an even more annoying failure.
So the root cause is the same (a timeout), but the effect depends on
HTTP/1.1 vs HTTP/2. I was able to reproduce both cases on my local
Debian unstable system by dropping the timeout as you suggested. Running
t5551 with GIT_TEST_LONG yields a failure, whereas running t5559 yields
a hang.
We can mitigate both cases by bumping the timeout value, since it's
addressing the root cause.
There's an open question of whether this is just papering over a problem
that real users might experience, and whether Git should be doing more
to keep the connection alive. I think it's probably OK to ignore this in
practice. This is an intentionally large request being served by a very
underpowered platform. The default apache timeout is 60s. If a
real-world server is seeing ls-refs requests take that long then they
probably need to reconsider some other decisions, from ref packing to
better hardware to dropping some users. ;) I don't think trying to
insert keepalives at the Git layer here is worth the trouble.
To give a sense of the time options, here are a few timings from my
local machine, timing "git upload-pack . </dev/null >/dev/null" in
t5551's big repo.git (that's a v0 advertisement, but it should be
roughly the same work as the v2 ls-refs).
cold-cache, refs not packed:
real 0m9.973s
user 0m0.354s
sys 0m1.364s
warm cache, refs not packed:
real 0m0.410s
user 0m0.153s
sys 0m0.257s
cold-cache, refs packed:
real 0m0.149s
user 0m0.086s
sys 0m0.035s
warm cache, refs packed:
real 0m0.069s
user 0m0.054s
sys 0m0.016s
So 10s is pretty abysmal (and on an SSD, no less). I would expect the
cache to be warm (we just wrote these refs!) but I could also believe
that CI systems are under heavy I/O and memory pressure, so we sometimes
end up crossing the 60s mark.
So bumping Apache's timeout to 600s or something would probably be a
fine mitigation. That's still not _solving_ the problem, but presumably
an order of magnitude is enough for it to never come up in practice.
Michael suggested packing the refs as a mitigation. I was lukewarm on
that in my previous email, because it wasn't clear to me how close we
were on the timeout budget, and if it would just make the race less
frequent (rather than never happen). But seeing those cold-cache numbers
makes me think it might be worth doing just on principle to make the
tests more efficient, and any timeout mitigation is a bonus.
Of course the pack-refs process (and the initial ref writes) will still
have to touch all of those loose files, so those will still be slow. But
they're not on a timeout, and I suspect we read the result many more
times than we write/pack (the test failures we are seeing are not in the
expensive tests, but just "normal" tests that are stuck with the
gigantic ref state).
-Peff
^ permalink raw reply
* Re: [PATCH GSoC v14 10/13] transport: add client support for object-info
From: Karthik Nayak @ 2026-06-27 0:22 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <20260625-ps-eric-work-rebase-v14-10-09f7ffe21a53@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 14897 bytes --]
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> From: Calvin Wan <calvinwan@google.com>
>
> Sometimes, it is beneficial to retrieve information about an object
> without downloading it entirely. The server-side logic for this
> functionality was implemented in commit "a2ba162cda (object-info:
> support for retrieving object info, 2021-04-20)." And the wire
> format is documented at
> https://git-scm.com/docs/protocol-v2#_object_info.
>
> This commit introduces client functions to interact with the server.
>
> Currently, the client supports requesting a list of object IDs with
> the 'size' feature from a v2 server. If the server does not advertise
> this feature (i.e., transfer.advertiseobjectinfo is set to false),
> the client will return an error and exit.
>
> Notice that the entire request is written into req_buf before being
> sent to the remote. This approach follows the pattern used in the
> `send_fetch_request()` logic within fetch-pack.c.
> Streaming the request is not addressed in this patch.
>
> 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>
> ---
> Makefile | 1 +
> fetch-object-info.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> fetch-object-info.h | 22 +++++++++++++
> fetch-pack.c | 3 ++
> fetch-pack.h | 2 ++
> meson.build | 1 +
> transport-helper.c | 11 +++++--
> transport.c | 28 ++++++++++++++++-
> transport.h | 11 +++++++
> 9 files changed, 166 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 1cec251f43..ec4df39a6b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1159,6 +1159,7 @@ LIB_OBJS += ewah/ewah_rlw.o
> LIB_OBJS += exec-cmd.o
> LIB_OBJS += fetch-negotiator.o
> LIB_OBJS += fetch-pack.o
> +LIB_OBJS += fetch-object-info.o
> LIB_OBJS += fmt-merge-msg.o
> LIB_OBJS += fsck.o
> LIB_OBJS += fsmonitor.o
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> new file mode 100644
> index 0000000000..9c4ae9bd11
> --- /dev/null
> +++ b/fetch-object-info.c
> @@ -0,0 +1,90 @@
> +#include "git-compat-util.h"
> +#include "gettext.h"
> +#include "hex.h"
> +#include "pkt-line.h"
> +#include "connect.h"
> +#include "oid-array.h"
> +#include "odb.h"
> +#include "fetch-object-info.h"
> +#include "string-list.h"
> +
> +/* Sends git-cat-file object-info command and its arguments into the request buffer. */
This file doesn't know about git-cat-file(1), nor should it care about
it, right? Since git-cat-file(1) is simply a user of this function.
Would it make more sense to structure the document around what this
function is supposed to do?
Also the comment for `fetch_object_info` seems to be similar, perhaps
worthwhile changing both without referring to git-cat-file(1).
Theoretically there could be other users in the future too.
> +static void send_object_info_request(const int fd_out, struct object_info_args *args)
> +{
> + struct strbuf req_buf = STRBUF_INIT;
> +
> + write_command_and_capabilities(&req_buf, "object-info", args->server_options);
> +
> + if (unsorted_string_list_has_string(args->object_info_options, "size"))
> + packet_buf_write(&req_buf, "size");
> +
Okay so if the user requests 'size', we forward that to the server. But
what about the 'else' condition? Should we BUG() out?
> + if (args->oids)
> + for (size_t i = 0; i < args->oids->nr; i++)
> + packet_buf_write(&req_buf, "oid %s", oid_to_hex(&args->oids->oid[i]));
> +
> + packet_buf_flush(&req_buf);
> + if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)
> + die_errno(_("unable to write request to remote"));
> +
We write out all the oids, flush and write to the fd. Okay.
> + strbuf_release(&req_buf);
> +}
> +
> +int fetch_object_info(const enum protocol_version version, struct object_info_args *args,
> + struct packet_reader *reader, struct object_info *object_info_data,
> + const int stateless_rpc, const int fd_out)
> +{
> + int size_index = -1;
> +
> + switch (version) {
> + case protocol_v2:
> + if (!server_supports_v2("object-info"))
> + die(_("object-info capability is not enabled on the server"));
> + send_object_info_request(fd_out, args);
So if the server does support 'object-info', we call
`send_object_info_request()`. Makes sense.
> + break;
> + case protocol_v1:
> + case protocol_v0:
> + die(_("unsupported protocol version. expected v2"));
> + case protocol_unknown_version:
> + BUG("unknown protocol version");
> + }
> +
Now that we've sent the request, we should start parsing the response.
> + for (size_t i = 0; i < args->object_info_options->nr; i++) {
> + if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
> + check_stateless_delimiter(stateless_rpc, reader,
> + "stateless delimiter expected");
> + return -1;
> + }
> +
> + if (!string_list_has_string(args->object_info_options, reader->line))
> + return -1;
> +
> + if (!strcmp(reader->line, "size")) {
> + size_index = i;
> + for (size_t j = 0; j < args->oids->nr; j++)
> + object_info_data[j].sizep = xcalloc(1, sizeof(*object_info_data[j].sizep));
> + }
> + }
> +
So this function seems to iterate over the list of options to only find
and store the indexes. If the server does support size we also allocate
the pointers to store the size.
Shouldn't we similarly BUG() if there is anything apart from 'size' here?
> + for (size_t i = 0; packet_reader_read(reader) == PACKET_READ_NORMAL && i < args->oids->nr; i++) {
> + struct string_list object_info_values = STRING_LIST_INIT_DUP;
> +
> + string_list_split(&object_info_values, reader->line, " ", -1);
> + if (0 <= size_index) {
> + if (!strcmp(object_info_values.items[1 + size_index].string, "")) {
> + FREE_AND_NULL(object_info_data[i].sizep);
> + string_list_clear(&object_info_values, 0);
> + continue;
> + }
> + if (strtoul_szt(object_info_values.items[1 + size_index].string,
> + 10, object_info_data[i].sizep))
This is now no longer correctly aligned.
> + die("object-info: ref %s has invalid size %s",
> + object_info_values.items[0].string,
> + object_info_values.items[1 + size_index].string);
> + }
> +
> + string_list_clear(&object_info_values, 0);
> + }
> + check_stateless_delimiter(stateless_rpc, reader, "stateless delimiter expected");
> +
We parse each line and obtain the size and parse it into
`object_info_data[i].sizep`. If the value is missing, we simply continue
the iteration.
I think we had this discussion off the list about how this means that
oids which do not have a size will not error out but rather display a
missing info value.
The argument for not error'ing out was better user experience where the
command would complete without exiting. I still think we should error
out, because:
1. Without error'ing out, we'd have to display to the user a missing
value token. There is contention around what this token should be,
as such a token shouldn't be a valid value for the info type being
displayed. Future options must be considered here.
2. What will the error code of such a situation be? Do we consider it a
success or a failure? Is there a situation where an object can have a
missing size?
> + return 0;
> +}
> diff --git a/fetch-object-info.h b/fetch-object-info.h
> new file mode 100644
> index 0000000000..d35284bd6b
> --- /dev/null
> +++ b/fetch-object-info.h
> @@ -0,0 +1,22 @@
> +#ifndef FETCH_OBJECT_INFO_H
> +#define FETCH_OBJECT_INFO_H
> +
> +#include "pkt-line.h"
> +#include "protocol.h"
> +#include "odb.h"
> +
> +struct object_info_args {
> + struct string_list *object_info_options;
> + const struct string_list *server_options;
> + struct oid_array *oids;
> +};
> +
> +/*
> + * Sends git-cat-file object-info command into the request buf and read the
> + * results from packets.
> + */
> +int fetch_object_info(enum protocol_version version, struct object_info_args *args,
> + struct packet_reader *reader, struct object_info *object_info_data,
> + int stateless_rpc, int fd_out);
> +
> +#endif /* FETCH_OBJECT_INFO_H */
> diff --git a/fetch-pack.c b/fetch-pack.c
> index cdebd3476f..a86c93fc52 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1742,6 +1742,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> if (args->depth > 0 || args->deepen_since || args->deepen_not)
> args->deepen = 1;
>
> + if (args->object_info)
> + state = FETCH_SEND_REQUEST;
> +
> while (state != FETCH_DONE) {
> switch (state) {
> case FETCH_CHECK_LOCAL:
> diff --git a/fetch-pack.h b/fetch-pack.h
> index 6d0dec7f41..5a428f11ed 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -16,6 +16,7 @@ struct fetch_pack_args {
> const struct string_list *deepen_not;
> struct list_objects_filter_options filter_options;
> const struct string_list *server_options;
> + struct object_info *object_info_data;
>
> /*
> * If not NULL, during packfile negotiation, fetch-pack will send "have"
> @@ -43,6 +44,7 @@ struct fetch_pack_args {
> unsigned reject_shallow_remote:1;
> unsigned deepen:1;
> unsigned refetch:1;
> + unsigned object_info:1;
>
> /*
> * Indicate that the remote of this request is a promisor remote. The
> diff --git a/meson.build b/meson.build
> index 3247697f74..145c6882eb 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -347,6 +347,7 @@ libgit_sources = [
> 'exec-cmd.c',
> 'fetch-negotiator.c',
> 'fetch-pack.c',
> + 'fetch-object-info.c',
> 'fmt-merge-msg.c',
> 'fsck.c',
> 'fsmonitor.c',
> diff --git a/transport-helper.c b/transport-helper.c
> index f195070788..c77599f6fb 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -727,8 +727,8 @@ static int fetch_refs(struct transport *transport,
>
> /*
> * If we reach here, then the server, the client, and/or the transport
> - * helper does not support protocol v2. --negotiate-only requires
> - * protocol v2.
> + * helper does not support protocol v2. --negotiate-only and cat-file
> + * remote-object-info require protocol v2.
This is not really true as of this commit. This only comes into effect
in the next commit. So shouldn't this be added there? That said, I would
modify this to only talk about object-info requiring v2 and drop the
reference to cat-file.
> */
> if (data->transport_options.acked_commits) {
> warning(_("--negotiate-only requires protocol v2"));
> @@ -744,6 +744,13 @@ static int fetch_refs(struct transport *transport,
> free_refs(dummy);
> }
>
> + /* fail the command explicitly to avoid further commands input. */
> + if (transport->smart_options->object_info)
> + die(_("remote-object-info requires protocol v2"));
> +
> + if (!data->get_refs_list_called)
> + get_refs_list_using_list(transport, 0);
> +
Don't we already do this right above? Why is this needed again?
> count = 0;
> for (i = 0; i < nr_heads; i++)
> if (!(to_fetch[i]->status & REF_STATUS_UPTODATE))
> diff --git a/transport.c b/transport.c
> index 0f5ec30247..7d3246e12b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -9,6 +9,7 @@
> #include "hook.h"
> #include "pkt-line.h"
> #include "fetch-pack.h"
> +#include "fetch-object-info.h"
> #include "remote.h"
> #include "connect.h"
> #include "send-pack.h"
> @@ -467,8 +468,33 @@ static int fetch_refs_via_pack(struct transport *transport,
> args.negotiation_restrict_tips = data->options.negotiation_restrict_tips;
> args.negotiation_include_tips = data->options.negotiation_include_tips;
> args.reject_shallow_remote = transport->smart_options->reject_shallow;
> + args.object_info = transport->smart_options->object_info;
> +
Hmm, so we piggy-back on top of the `fetch_refs_via_pack()` function...
> + if (transport->smart_options->object_info
> + && transport->smart_options->object_info_oids->nr > 0) {
> + struct packet_reader reader;
> + struct object_info_args obj_info_args = { 0 };
> +
> + obj_info_args.server_options = transport->server_options;
> + obj_info_args.oids = transport->smart_options->object_info_oids;
> + obj_info_args.object_info_options = transport->smart_options->object_info_options;
> + string_list_sort(obj_info_args.object_info_options);
> +
> + connect_setup(transport, 0);
> + packet_reader_init(&reader, data->fd[0], NULL, 0,
> + PACKET_READ_CHOMP_NEWLINE |
> + PACKET_READ_GENTLE_ON_EOF |
> + PACKET_READ_DIE_ON_ERR_PACKET);
> +
> + data->version = discover_version(&reader);
> + transport->hash_algo = reader.hash_algo;
> +
> + ret = fetch_object_info(data->version, &obj_info_args, &reader,
> + data->options.object_info_data, transport->stateless_rpc,
> + data->fd[1]);
> + goto cleanup;
>
... and we jump to exit when we only want object info information. This
skips the call to fetch_pack(). I'm a bit uneasy with this. Ideally we
should be adding a new function to the vtable to only fetch object info.
While this works, this doesn't fit the contract of what this function is
supposed to do. See the comment around `fetch_refs` in `struct
transport_vtable`. Shouldn't we update that documentation at the very
least?
> - if (!data->finished_handshake) {
> + } else if (!data->finished_handshake) {
> int i;
> int must_list_refs = 0;
> for (i = 0; i < nr_heads; i++) {
> diff --git a/transport.h b/transport.h
> index 7e5867cffa..bd60b10af4 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -6,6 +6,7 @@
> #include "list-objects-filter-options.h"
> #include "string-list.h"
> #include "connect.h"
> +#include "odb.h"
>
> struct git_transport_options {
> unsigned thin : 1;
> @@ -31,6 +32,12 @@ struct git_transport_options {
> */
> unsigned connectivity_checked:1;
>
> + /*
> + * Transport will attempt to retrieve only object-info.
> + * If object-info is not supported, the operation will error and exit.
> + */
> + unsigned object_info : 1;
> +
According to our style, this should be `unsigned object_info:1`.
> int depth;
> const char *deepen_since;
> const struct string_list *deepen_not;
> @@ -55,6 +62,10 @@ struct git_transport_options {
> * common commits to this oidset instead of fetching any packfiles.
> */
> struct oidset *acked_commits;
> +
> + struct oid_array *object_info_oids;
> + struct object_info *object_info_data;
> + struct string_list *object_info_options;
> };
>
> enum transport_family {
>
> --
> 2.54.0
Nit: I do think the commit message doesn't sufficiently capture the
entirety of this patch. We do not talk about:
1. How we piggy-back on top of `fetch_refs_via_pack()` and don't
introduce our own pointer in the vtable.
2. The changes made to 'transport-helper.c' and why that is required.
3. How we don't error out when there is no size value provided for an
OID and what the implication of this is.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
From: Taylor Blau @ 2026-06-27 0:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqqpl1d56dd.fsf@gitster.g>
On Fri, Jun 26, 2026 at 02:39:10PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +/*
> > + * Flag bit set on commits that belong to an included pack during
> > + * '--stdin-packs=follow-reachable'. Used by the pre-walk to
> > + * identify which reachable commits should be tips for the main
> > + * object traversal.
> > + */
> > +#define IN_INCLUDED_PACK (1u<<11)
> > +
> > +static int mark_included_pack_tip(const struct object_id *oid,
> > + struct packed_git *p,
> > + uint32_t pos,
> > + void *data)
> > +{
> > + struct rev_info *main_revs = data;
> > + off_t ofs = nth_packed_object_offset(p, pos);
> > + enum object_type type;
> > + struct object_info oi = OBJECT_INFO_INIT;
> > + struct object *obj;
> > +
> > + oi.typep = &type;
> > + if (packed_object_info(p, ofs, &oi) < 0)
> > + return 0;
> > + if (type != OBJ_COMMIT && type != OBJ_TAG)
> > + return 0;
>
> We do not care about non commits, non tags.
This should not be an &&, but rather an ||. We only want to handle
objects which are either commits *or* tags via this function.
> > + obj = parse_object(the_repository, oid);
> > + if (!obj)
> > + return 0;
> > +
> > + obj->flags |= IN_INCLUDED_PACK;
> > +
> > + if (type == OBJ_TAG && main_revs)
> > + add_pending_object(main_revs, obj, "");
>
> Any tag object is added to the pending list of the second phase
> traversal here. Doesn't this retain unreachable tags and
> (unreachable) objects that are only reachable from these unreachable
> tags found in the packfile? Don't we want to limit this code to add
> only tags that actually are reachable from refs, or something?
Hmm. You're right, if we have an unreachable annotated tag object in one
of the STDIN_PACK_INCLUDE packs, then we'll pick it up and place it in
the non-cruft pack.
I'd have to think more about how to handle this properly to ensure that
*only* reachable objects are included. We probably want to re-order this
code so that we start a traversal from the reference tips, then mark
reachable objects as starting points based on whether or not they appear
in an included pack (and correspondingly do not appear in an excluded
one).
That adjusts how the traversal would work, I think, since we should be
able to pick up the resulting set of objects in a single pass. But I'd
have to think more about it...
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 04/10] repack: teach MIDX retention about geometric rollups
From: Taylor Blau @ 2026-06-27 0:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqqwlvl56vh.fsf@gitster.g>
On Fri, Jun 26, 2026 at 02:28:18PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +static int pack_geometry_contains_pack(struct packed_git **packs,
> > + uint32_t packs_nr,
> > + const char *base)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + uint32_t i;
> > +
> > + for (i = 0; i < packs_nr; i++) {
> > + strbuf_reset(&buf);
> > + strbuf_addstr(&buf, pack_basename(packs[i]));
> > + strbuf_strip_suffix(&buf, ".pack");
> > +
> > + if (!strcmp(buf.buf, base)) {
> > + strbuf_release(&buf);
> > + return 1;
> > + }
> > + }
> > +
> > + strbuf_release(&buf);
> > + return 0;
> > +}
>
> It feels slightly inefficient to repeatedly strbuf_reset(),
> strbuf_addstr(), and strbuf_strip_suffix() in the loop. I do not
> know if my understanding of what existing_packs_retain_midx_packs()
> passes down in buf.buf as base is correct or not, but if so,
> wouldn't it equivalent to
>
> for (uint32_t i = 0; i < packs_nr; i++) {
> const char *pack_name = pack_basename(packs[i]);
> const char *suffix;
>
> if (skip_prefix(pack_name, base, &suffix) &&
> !strcmp(suffix, ".pack"))
> return 1;
> }
>
> perhaps?
>
> Starting from "/path/to/objects/pack/pack-deadbeef.pack", you take
> the basename of it to have "pack-deadbeef.pack" in buf, strip out
> the ".pack" suffix to get "pack-deadbeef" in buf and then compare it
> with the base.
I think that this would work nicely. I think that the skip_prefix()
variant is easy enough to read, and is clearly more efficient.
> Instead, pack_name in the rewitten one becomes the basename of the
> packfile path, i.e., "pack-deadbeef.pack", then we see if it begins
> with base and take the remainder in suffix, and finally we check if
> that remaining suffix is ".pack".
>
> Which should be equivalent.
>
> > + * freshly-written pack supersedes them. When doing a geometric repack,
> > + * packs below the split are rewritten into the new MIDX tip and should
> > + * remain eligible for deletion.
> > */
> > -void existing_packs_retain_midx_packs(struct existing_packs *existing)
> > +void existing_packs_retain_midx_packs(struct existing_packs *existing,
> > + const struct pack_geometry *geometry)
> > {
> > struct string_list_item *item;
> > struct strbuf buf = STRBUF_INIT;
> > @@ -315,6 +351,9 @@ void existing_packs_retain_midx_packs(struct existing_packs *existing)
> > strbuf_strip_suffix(&buf, ".pack");
> > strbuf_strip_suffix(&buf, ".idx");
>
> Not a fault of this patch, but it makes the hairs on the back of my
> head tingle to see that a bogus input like "pack-foobar.idx.pack"
> happily is taken, while "pack-foobar.pack.idx", an equally bogus
> input, is not.
Yeah, this is gross (and my fault). Presumably I was swapping out one
variant for another and didn't stage the removal of one of the
strip_suffix() calls.
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
From: Junio C Hamano @ 2026-06-27 2:09 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <aj8cOhH6hGVZIFft@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
>> > + if (packed_object_info(p, ofs, &oi) < 0)
>> > + return 0;
>> > + if (type != OBJ_COMMIT && type != OBJ_TAG)
>> > + return 0;
>>
>> We do not care about non commits, non tags.
>
> This should not be an &&, but rather an ||. We only want to handle
> objects which are either commits *or* tags via this function.
My comment above did not mean to say anything is wrong in the code;
I was just thinking aloud. We return for blob or tree because they
are not commit and they are not tag. If we say "||" then we return
for everybody, because anything that is a commit (failing the LHS of
the "||") cannot be a tag (satisfying the RHS of the "||").
^ permalink raw reply
* Re: [RFC PATCH 08/10] pack-objects: introduce '--stdin-packs=follow-reachable'
From: Taylor Blau @ 2026-06-27 2:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Elijah Newren, Patrick Steinhardt
In-Reply-To: <xmqq8q8068f7.fsf@gitster.g>
On Fri, Jun 26, 2026 at 07:09:32PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> > + if (packed_object_info(p, ofs, &oi) < 0)
> >> > + return 0;
> >> > + if (type != OBJ_COMMIT && type != OBJ_TAG)
> >> > + return 0;
> >>
> >> We do not care about non commits, non tags.
> >
> > This should not be an &&, but rather an ||. We only want to handle
> > objects which are either commits *or* tags via this function.
>
> My comment above did not mean to say anything is wrong in the code;
> I was just thinking aloud. We return for blob or tree because they
> are not commit and they are not tag. [...]
Absolutely. Sorry about that, I have no idea how I tricked myself into
misreading the patch here. The second sentence of what I wrote above is
correct, but not the first.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] Move libgit.a sources into separate "lib/" directory
From: SZEDER Gábor @ 2026-06-27 6:40 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, brian m. carlson, Junio C Hamano, Elijah Newren,
Derrick Stolee, Phillip Wood
In-Reply-To: <20260622-pks-libgit-in-subdir-v2-2-cb946c51ee7b@pks.im>
On Mon, Jun 22, 2026 at 12:38:22PM +0200, Patrick Steinhardt wrote:
> The Git project is not exactly the easiest project to get started in:
> it's written in C and POSIX shell, with bits of Perl, Rust and other
> languages sprinkled into it. On top of that, the project has grown
> somewhat organically over time, making the codebase hard to navigate.
>
> These are problems that we're aware of, and there have been and still
> are efforts to clean up some of the technical debt that is natural to
> exist an a project that is more than 20 years old. Furthermore, we
> provide resources to newcomers that help them out like our coding
> guidelines, code of conduct or "MyFirstContribution.adoc".
>
> But there is a rather practical problem: finding your way around in our
> project's tree is not easy. Doing a directory listing in the top-level
> directory will present you with more than 550 files, which makes it
> extremely hard for a newcomer to figure out what files they are even
> supposed to look at. This makes the onboarding experience somewhat
> harder than it really needs to be. This isn't only a problem for
> newcomers though, as I myself struggle to find the files I am looking
> for because of the sheer number of files.
>
> Besides the problem of discoverability it also creates a problem of
> structure. It is not obvious at all which files are part of "libgit.a"
> and which files are only linked into our final executables. So while we
> have this split in our build systems, that split is not evident at all
> in our tree.
>
> Introduce a new "lib/" directory and move all of our sources for
> "libgit.a" into it to fix these issues. It makes the split we have
> evident and reduces the number of files in our top-level tree from 550
> files to ~80 files.
>
> This is still a lot of files, but it's significantly easier to navigate
> already. Furthermore, we can further iterate after this step and think
> about introducing a better structure for remaining files, as well.
Please also discuss the drawbacks of this proposal, and try to argue
convincingly that the benefits outweigh the drawbacks.
I, for one, see myself being rather annoyed by regular 'git log
lib/foo.c' stopping at the rename barrier, and by the limitations of
'--follow'.
In my opinion this patch just trades one annoyance to an other, and,
therefore, it's not worth the churn.
^ permalink raw reply
* Re: [PATCH] t3420-rebase-autostash: don't try to grep non-existing files
From: SZEDER Gábor @ 2026-06-27 6:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Montalbo, Denton Liu
In-Reply-To: <20211010172809.1472914-1-szeder.dev@gmail.com>
On Sun, Oct 10, 2021 at 07:28:09PM +0200, SZEDER Gábor wrote:
> Several tests in 't3420-rebase-autostash.sh' start various rebase
> processes that are expected to fail because of merge conflicts. The
> tests [1] checking that 'git rebase --quit' and autostash work
> together as expected after such a failure then run '! grep ...' to
> ensure that the dirty contents of the file is gone. However, due to
> the test repo's history and the choice of upstream branch that file
> shouldn't exist in the conflicted state at all, and thus it shouldn't
> exist after the subsequent 'git rebase --quit' either. Consequently,
> this 'grep' doesn't fail as expected, i.e. because it can't find the
> dirty content, but instead it fails, because it can't open the file.
>
> Thighten this check by using 'test_path_is_missing' instead, thereby
> avoiding unexpected errors from 'grep' as well.
>
> Previously 2745817028 (t3420-rebase-autostash: don't try to grep
> non-existing files, 2018-08-22) fixed a couple of similar issues; this
> one was added later in 9b2df3e8d0 (rebase: save autostash entry into
> stash reflog on --quit, 2020-04-28).
>
> [1] This patch modifies only a single test, but that test is run
> several times with different strategies ('--apply', '--merge', and
> '--interactive'), hence the plural "tests".
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> t/t3420-rebase-autostash.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index 43fcb68f27..bbe82d2c0c 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -200,7 +200,7 @@ testrebase () {
> git rebase --quit &&
> test_when_finished git stash drop &&
> test_path_is_missing $dotest/autostash &&
> - ! grep dirty file3 &&
> + test_path_is_missing file3 &&
> git stash show -p >actual &&
> test_cmp expect actual &&
> git reset --hard &&
> --
> 2.33.0.1279.g1a260bf8c2
It appears that this patch might have fallen quite deep through the
cracks... ;)
But the issue this patch is addressing is still there, and the patch
still applies cleanly after almost 5 years.
^ permalink raw reply
* Re: [PATCH v2 5/6] t: convert grep assertions to test_grep
From: SZEDER Gábor @ 2026-06-27 7:08 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget
Cc: git, D. Ben Knoble, Eric Sunshine, Michael Montalbo
In-Reply-To: <3a589ef7386303075413f388e61c203c4e325d44.1781323575.git.gitgitgadget@gmail.com>
On Sat, Jun 13, 2026 at 04:06:14AM +0000, Michael Montalbo via GitGitGadget wrote:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> Replace bare grep with test_grep in test assertions across the
> suite, including sourced test helpers (lib-*.sh, *-tests.sh).
> test_grep prints the contents of the file being searched on
> failure, making debugging easier than a bare grep which fails
> silently.
>
> Only assertion-style greps are converted: grep used as a filter
> in pipelines, command substitutions, conditionals, or with
> redirected I/O is left as-is with a "# lint-ok" annotation.
> Existing '! test_grep' calls are rewritten to 'test_grep !' so
> that the diagnostic output is preserved on failure.
Thanks for taking the effort for cleaning up all those negated '!
grep' and '! test_grep' callsites and turning them into 'test_grep !'.
> The conversion was generated using a grep-assertion linter
> (greplint.pl, added in the following commit) to identify bare
> grep calls at command position. To reproduce:
>
> # Step 1: mark bare greps that should not be converted
> sed -i '/! grep "$m" \.git\/packed-refs/s/$/ # lint-ok: file may not exist (reftable)/' \
> t/t1400-update-ref.sh
> sed -i '/! grep dirty file3 &&/{/lint-ok/!s/$/ # lint-ok: file may not exist after --quit/}' \
> t/t3420-rebase-autostash.sh
I think in this case checking the file3's contents is wrong, because
at this point file3 should not exist in the first place. I've sent a
patch to fix this long ago, but apparently didn't manage to follow
through back then.
https://lore.kernel.org/git/20211010172809.1472914-1-szeder.dev@gmail.com/
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index f0bbc476ff..f6cb3dd72e 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -141,8 +141,8 @@ testrebase () {
> git checkout -b rebased-feature-branch feature-branch &&
> echo dirty >>file3 &&
> git rebase$type unrelated-onto-branch >actual 2>&1 &&
> - grep unrelated file4 &&
> - grep dirty file3 &&
> + test_grep unrelated file4 &&
> + test_grep dirty file3 &&
> git checkout feature-branch
> '
>
> @@ -165,8 +165,8 @@ testrebase () {
> echo dirty >>file3 &&
> git add file3 &&
> git rebase$type unrelated-onto-branch &&
> - grep unrelated file4 &&
> - grep dirty file3 &&
> + test_grep unrelated file4 &&
> + test_grep dirty file3 &&
> git checkout feature-branch
> '
>
> @@ -197,7 +197,7 @@ testrebase () {
> git add file2 &&
> git rebase --continue &&
> test_path_is_missing $dotest/autostash &&
> - grep dirty file3 &&
> + test_grep dirty file3 &&
> git checkout feature-branch
> '
>
> @@ -212,7 +212,7 @@ testrebase () {
> test_path_is_missing file3 &&
> git rebase --skip &&
> test_path_is_missing $dotest/autostash &&
> - grep dirty file3 &&
> + test_grep dirty file3 &&
> git checkout feature-branch
> '
>
> @@ -227,7 +227,7 @@ testrebase () {
> test_path_is_missing file3 &&
> git rebase --abort &&
> test_path_is_missing $dotest/autostash &&
> - grep dirty file3 &&
> + test_grep dirty file3 &&
> git checkout feature-branch
> '
>
> @@ -244,7 +244,7 @@ testrebase () {
> git rebase --quit &&
> test_when_finished git stash drop &&
> test_path_is_missing $dotest/autostash &&
> - ! grep dirty file3 &&
> + ! grep dirty file3 && # lint-ok: file may not exist after --quit
> git stash show -p >actual &&
> test_cmp expect actual &&
> git reset --hard &&
> @@ -260,11 +260,11 @@ testrebase () {
> git rebase$type unrelated-onto-branch >actual 2>&1 &&
> test_path_is_missing $dotest &&
> git reset --hard &&
> - grep unrelated file4 &&
> - ! grep dirty file4 &&
> + test_grep unrelated file4 &&
> + test_grep ! dirty file4 &&
> git checkout feature-branch &&
> git stash pop &&
> - grep dirty file4
> + test_grep dirty file4
> '
>
> test_expect_success "rebase$type: check output with conflicting stash" '
> @@ -286,7 +286,7 @@ test_expect_success "rebase: fast-forward rebase" '
> test_when_finished git branch -D behind-feature-branch &&
> echo dirty >>file1 &&
> git rebase feature-branch &&
> - grep dirty file1 &&
> + test_grep dirty file1 &&
> git checkout feature-branch
> '
>
> @@ -297,7 +297,7 @@ test_expect_success "rebase: noop rebase" '
> test_when_finished git branch -D same-feature-branch &&
> echo dirty >>file1 &&
> git rebase feature-branch &&
> - grep dirty file1 &&
> + test_grep dirty file1 &&
> git checkout feature-branch
> '
>
^ permalink raw reply
* Re: [PATCH] t3420-rebase-autostash: don't try to grep non-existing files
From: Phillip Wood @ 2026-06-27 9:03 UTC (permalink / raw)
To: SZEDER Gábor, git; +Cc: Junio C Hamano, Michael Montalbo, Denton Liu
In-Reply-To: <aj90x3DsER5HASUS@szeder.dev>
On 27/06/2026 07:59, SZEDER Gábor wrote:
> On Sun, Oct 10, 2021 at 07:28:09PM +0200, SZEDER Gábor wrote:
>> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
>> index 43fcb68f27..bbe82d2c0c 100755
>> --- a/t/t3420-rebase-autostash.sh
>> +++ b/t/t3420-rebase-autostash.sh
>> @@ -200,7 +200,7 @@ testrebase () {
With an extra context line we see
test_path_is_missing file3 &&>> git rebase --quit &&
>> test_when_finished git stash drop &&
>> test_path_is_missing $dotest/autostash &&
>> - ! grep dirty file3 &&
>> + test_path_is_missing file3 &&
and so it is quite clear that this change is correct
Thanks
Phillip
>> git stash show -p >actual &&
>> test_cmp expect actual &&
>> git reset --hard &&
>> --
>> 2.33.0.1279.g1a260bf8c2
>
> It appears that this patch might have fallen quite deep through the
> cracks... ;)
>
> But the issue this patch is addressing is still there, and the patch
> still applies cleanly after almost 5 years.
>
^ permalink raw reply
* Re: [PATCH v3 0/2] Silence po catalog output under "make -s"
From: Johannes Sixt @ 2026-06-27 9:34 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <CAHwyqnX78ePVhiL+_T3FzCSA5oGaU_RPvQj6YP=s1WyULg=tdg@mail.gmail.com>
Am 26.06.26 um 21:27 schrieb Harald Nordgren:
> What should I expect here, will it be merged to master now?
These patches are cooking in my respective j6t-testing branches in my
repositories[1][2]. I'll ask for inclusion in the Git repository in the
coming weeks (but certainly not for v2.55).
-- Hannes
[1] https://github.com/j6t/git-gui/commits/j6t-testing/
[2] https://github.com/j6t/gitk/commits/j6t-testing/
^ permalink raw reply
* Re: [PATCH GSoC v14 11/13] cat-file: add remote-object-info to batch-command
From: Karthik Nayak @ 2026-06-27 13:14 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <20260625-ps-eric-work-rebase-v14-11-09f7ffe21a53@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7007 bytes --]
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[snip]
> diff --git a/Documentation/git-cat-file.adoc b/Documentation/git-cat-file.adoc
> index 86b9181599..aba20eb770 100644
> --- a/Documentation/git-cat-file.adoc
> +++ b/Documentation/git-cat-file.adoc
> @@ -169,6 +169,13 @@ info <object>::
> Print object info for object reference `<object>`. This corresponds to the
> output of `--batch-check`.
>
> +remote-object-info <remote> <object>...::
> + Print object info for object references `<object>` at specified
> + `<remote>` without downloading objects from the remote.
> + Raise an error when the `object-info` capability is not supported by the remote.
> + Raise an error when no object references are provided.
> + This command may be combined with `--buffer`.
> +
> flush::
> Used with `--buffer` to execute all preceding commands that were issued
> since the beginning or since the last flush was issued. When `--buffer`
> @@ -312,7 +319,8 @@ newline. The available atoms are:
> The full hex representation of the object name.
>
> `objecttype`::
> - The type of the object (the same as `cat-file -t` reports).
> + The type of the object (the same as `cat-file -t` reports). See
> + `CAVEATS` below. Not supported by `remote-object-info`.
>
Do we have to keep adding 'Not supported by `remote-object-info`' to
each type? Can't we do the inverse and only add 'Supported by
`remote-object-info`' to `objectsize`. This avoid having to add this
line to every new type.
> If no format is specified, the default format is `%(objectname)
> -%(objecttype) %(objectsize)`.
> +%(objecttype) %(objectsize)`, except for `remote-object-info` commands which use
> +`%(objectname) %(objectsize)` for now because "%(objecttype)" is not supported yet.
Nit: I would drop the 'for now' here, since we don't know when the changes
for 'objecttype' will land.
[snip]
> enum batch_mode {
> BATCH_MODE_CONTENTS,
> @@ -633,6 +649,81 @@ static void batch_one_object(const char *obj_name,
> object_context_release(&ctx);
> }
>
> +static int get_remote_info(struct batch_options *opt,
> + int argc,
> + const char **argv,
> + struct object_info **remote_object_info,
> + struct oid_array *object_info_oids)
> +{
> + int retval = 0;
> + struct remote *remote = NULL;
> + struct object_id oid;
> + struct string_list object_info_options = STRING_LIST_INIT_NODUP;
> + struct transport *gtransport;
> +
> + /*
> + * Change the format to "%(objectname) %(objectsize)" when
Nit: perhaps prepend a "TODO"
> + * remote-object-info command is used. Once we start supporting objecttype
> + * the default format should change to DEFAULT_FORMAT.
> + */
> + if (!opt->format)
> + opt->format = "%(objectname) %(objectsize)";
> +
> + remote = remote_get(argv[0]);
> + if (!remote)
> + die(_("must supply valid remote when using remote-object-info"));
> +
> + oid_array_clear(object_info_oids);
> + for (size_t i = 1; i < argc; i++) {
> + if (get_oid_hex(argv[i], &oid)) {
> + size_t len = strlen(argv[i]);
> +
> + if (len < the_hash_algo->hexsz && len >= 4) {
> + size_t j;
> + for (j = 0; j < len; j++)
> + if (!isxdigit(argv[i][j]))
> + break;
> + if (j == len)
> + die(_("remote-object-info does not support "
> + "short oids, %d characters required"),
> + (int)the_hash_algo->hexsz);
> + }
> + die(_("not a valid object name '%s'"), argv[i]);
> + }
> + oid_array_append(object_info_oids, &oid);
> + }
> +
> + if (!object_info_oids->nr)
> + die(_("remote-object-info requires objects"));
> +
> + gtransport = transport_get(remote, NULL);
> +
> + if (!gtransport->smart_options) {
> + retval = -1;
> + goto cleanup;
> + }
> +
> + CALLOC_ARRAY(*remote_object_info, object_info_oids->nr);
> + gtransport->smart_options->object_info = 1;
> + gtransport->smart_options->object_info_oids = object_info_oids;
> +
> + /* 'objectsize' is the only option currently supported */
> + if (!strstr(opt->format, "%(objectsize)"))
> + die(_("%s is currently not supported with remote-object-info"), opt->format);
> +
Aren't we setting the opt->format ourselves in this function? Why do we
need to check it?
> + string_list_append(&object_info_options, "size");
> +
> + if (object_info_options.nr > 0) {
> + gtransport->smart_options->object_info_options = &object_info_options;
> + gtransport->smart_options->object_info_data = *remote_object_info;
> + retval = transport_fetch_refs(gtransport, NULL);
> + }
> +cleanup:
> + string_list_clear(&object_info_options, 0);
> + transport_disconnect(gtransport);
> + return retval;
> +}
> +
> struct object_cb_data {
> struct batch_options *opt;
> struct expand_data *expand;
> @@ -714,6 +805,57 @@ static void parse_cmd_mailmap(struct batch_options *opt UNUSED,
> load_mailmap();
> }
>
> +static void parse_cmd_remote_object_info(struct batch_options *opt,
> + const char *line, struct strbuf *output,
> + struct expand_data *data)
> +{
> + int count;
> + const char **argv;
> + char *line_to_split;
> + struct object_info *remote_object_info = NULL;
> + struct oid_array object_info_oids = OID_ARRAY_INIT;
> +
> + if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
> + die(_("remote-object-info command too long"));
> +
> + line_to_split = xstrdup(line);
> + count = split_cmdline(line_to_split, &argv);
> + if (count < 0)
> + die(_("split remote-object-info command"));
We should be using `split_cmdline_strerror()` here
> + if (count - 1 > MAX_ALLOWED_OBJ_LIMIT)
> + die(_("remote-object-info supports at most %d objects"),
> + MAX_ALLOWED_OBJ_LIMIT);
> +
> + if (get_remote_info(opt, count, argv, &remote_object_info,
> + &object_info_oids))
> + goto cleanup;
> +
> + data->skip_object_info = 1;
> + for (size_t i = 0; i < object_info_oids.nr; i++) {
> + data->oid = object_info_oids.oid[i];
> + if (remote_object_info[i].sizep) {
> + /*
> + * When reaching here, it means remote-object-info can retrieve
> + * information from server without downloading them.
> + */
> + data->size = *remote_object_info[i].sizep;
> + opt->batch_mode = BATCH_MODE_INFO;
> + batch_object_write(argv[i + 1], output, opt, data, NULL, 0);
> + } else {
> + report_object_status(opt, oid_to_hex(&data->oid), &data->oid, "missing");
> + }
> + }
> + data->skip_object_info = 0;
> +
> +cleanup:
> + for (size_t i = 0; i < object_info_oids.nr; i++)
> + free_object_info_contents(&remote_object_info[i]);
> + free(line_to_split);
> + free(argv);
> + free(remote_object_info);
> + oid_array_clear(&object_info_oids);
> +}
> +
[snip]
> diff --git a/t/meson.build b/t/meson.build
> index 3219264fe7..54d21111a3 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -170,6 +170,7 @@ integration_tests = [
> 't1014-read-tree-confusing.sh',
> 't1015-read-index-unmerged.sh',
> 't1016-compatObjectFormat.sh',
> + 't1017-cat-file-remote-object-info.sh',
> 't1020-subdirectory.sh',
> 't1022-read-tree-partial-clone.sh',
> 't1050-large.sh',
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/3] replay: offer an option to linearize the commit topology
From: Phillip Wood @ 2026-06-27 13:44 UTC (permalink / raw)
To: Junio C Hamano, Toon Claes; +Cc: git, Elijah Newren, Johannes Schindelin
In-Reply-To: <xmqq5x358byf.fsf@gitster.g>
On 26/06/2026 18:10, Junio C Hamano wrote:
> Toon Claes <toon@iotcl.com> writes:
>
>> Documentation/git-replay.adoc | 8 ++++-
>> builtin/replay.c | 6 +++-
>> replay.c | 50 ++++++++++++++++----------
>> replay.h | 5 +++
>> t/t3650-replay-basics.sh | 84 ++++++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 132 insertions(+), 21 deletions(-)
>
> "replay --linearize" behaves differently from the flattening rebase
> in a case where X and Y that forked from A are merged at Z, and we
> ask to flatten the history leading to Z, doesn't it?
That's a good point. rebase takes the list of commits given by "git
rev-list --reverse --no-merges" and cherry-picks each on on top of the
previous one. In contrast replay cherry-picks each commit on top of its
rewritten parent so it does not flatten the topology.
Thanks
Phillip
> A----X
> \ \
> Y----Z (tip)
>
> A typical flattening rebase would rewrite X to X', Y to Y', while
> dropping Z, and would leave us a flattened history, like
>
> A---X'---Y' (updated tip, the order of X' and Y' may be swapped)
>
> I may be misreading the logic, but doesn't "replay --linearize"
> instead produce
>
> A----X' (dangling)
> \
> Y' (tip -- Z is dropped and gets mapped)
>
> and leave X' dangling (or Y'; the point is that only one of them
> will survive), never incorporating it in the resulting history?
>
>> + if (commit->parents && commit->parents->next) {
>> + if (!opts->linearize)
>> + die(_("replaying merge commits is not supported yet!"));
>> + /*
>> + * Drop the merge commit: do not pick it, leave
>> + * `last_commit` unchanged, and fall through to the
>> + * rest of the loop. As a result:
>> + * - the merge commit is mapped to `last_commit` in
>> + * `replayed_commits`, this will become the parent for
>> + * the child commits.
>> + * - refs previously pointing to the merge commit are
>> + * rewritten to point to the previous non-merge commit.
>> + */
>> + } else {
>> + /*
>> + * pick_regular_commit() looks up the parent of `commit` in
>> + * `replayed_commits` to determine the ancestor to replay onto.
>> + * The `default_base` parameter is used when no ancestor is found,
>> + * which happens for the first commit in the revision range.
>> + * When reverting, commits are replayed in reverse order, so the
>> + * lookup never succeeds, and we need to pass `last_commit`.
>> + */
>> + struct commit *base = onto;
>> + if (mode == REPLAY_MODE_REVERT)
>> + base = last_commit;
>> +
>> + last_commit = pick_regular_commit(revs->repo, commit, base,
>> + replayed_commits,
>> + &merge_opt, &result,
>> + mode, opts->empty);
>> + }
>> +
>> if (!last_commit)
>> break;
>
> Immediately after this hunk beyond the post-context are these lines.
>
> /* Record commit -> last_commit mapping */
> put_mapped_commit(replayed_commits, commit, last_commit);
>
> Let's imagine X gets processed first. X (and other commits on its
> branch) gets replayed, last_commit is set to X' (which is the
> rewritten X). replayed_commits mapping holds X->X' mapping.
>
> Then let's imagine the history leading to Y is replayed next.
> last_commit becomes Y', and Y->Y' mapping is stored in
> replayed_commits.
>
> Finally, we see Z. We are going to _drop_ it. last_commit is left
> unchanged, pointing at Y'. Then last_commit (i.e., Y') is used as
> the merge commit Z maps to (i.e., correctly dropping Z).
>
> Any descendants of Z, if any, will be grafted as descendants of Y'.
> If X did not have any descendants other than Z in the rewritten part
> of the history, then X' (and commits leading to it) would be lost,
> no?
>
> This "loss of the other branch" may be an inherent characteristic of
> this feature (i.e., I do not think it is necessarily a bug, and it
> may even be that the "bug" is in the way I am reading the patch),
> but then I wonder if the user may want to have control over which
> side branch should survive, perhaps? It would probably need to be
> documented, and a test or two to cast this behaviour in stone.
>
>> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
>> index 3353bc4a4d..34c038eab9 100755
>> --- a/t/t3650-replay-basics.sh
>> +++ b/t/t3650-replay-basics.sh
>> @@ -52,8 +52,12 @@ test_expect_success 'setup' '
>
> The pre-context here has
>
> git switch --detach topic4 &&
> test_commit N &&
> test_commit O &&
> git switch -c topic-with-merge topic4 &&
>
>> test_merge P O --no-ff &&
>> git switch main &&
>
> The above does prepare topic-with-merge branch, but ...
>
>> +test_expect_success 'replay to rebase merge commit with --linearize' '
>> + git replay --ref-action=print --linearize \
>> + --onto main I..topic-with-merge >result &&
>
> ... this does not really exersize linearizing replay in a typical
> mergy history. P merges O with --no-ff because otherwise there
> won't be a merge, since O is a descendant of the commit "test_merge
> P O" runs on (i.e., topic4 == topic-with-merge).
>
> topic4 --- N --- O
> \ \
> .-----------P
>
> So, as long as O is replayed later than the parent of N (which is
> true), O' will be the surviving tip (corresponds to Y' that the
> dropped Z was mapped to in the earlier example), and nothing gets
> orphaned, I think.
>
> Perhaps a test to try a real merge may look something like this.
>
> diff --git c/t/t3650-replay-basics.sh w/t/t3650-replay-basics.sh
> index 34c038eab9..bb737f729a 100755
> --- c/t/t3650-replay-basics.sh
> +++ w/t/t3650-replay-basics.sh
> @@ -647,4 +647,37 @@ test_expect_success 'replay with --linearize to rebase multiple divergent branch
> test_cmp expect actual
> '
>
> +test_expect_success 'replay with --linearize of a divergent merge drops one branch' '
> + git switch -c topic-divergent-base main &&
> + test_commit base &&
> + # Fork 1: base -> X
> + git switch -c topic-divergent-x &&
> + test_commit X &&
> + # Fork 2: base -> Y
> + git switch topic-divergent-base &&
> + git switch -c topic-divergent-y &&
> + test_commit Y &&
> + # Merge them at Z
> + git switch topic-divergent-x &&
> + test_merge Z topic-divergent-y --no-ff &&
> +
> + # History is now:
> + #
> + # X - Z (topic-divergent-x)
> + # / /
> + # base - Y
> + #
> +
> + git replay --ref-action=print --linearize \
> + --onto main topic-divergent-base..topic-divergent-x >result &&
> + test_line_count = 1 result &&
> + tip=$(cut -f 3 -d " " result) &&
> + # Get the commits replayed onto main
> + git log --format=%s main..$tip >actual &&
> + # We expect exactly one commit to be replayed (either X or Y)
> + # because the other one is left dangling due to the merge being dropped.
> + test_line_count = 1 actual &&
> + test_grep "^[XY]$" actual
> +'
> +
> test_done
>
^ permalink raw reply
* Re: [PATCH v2 0/2] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-27 13:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, cirnovskyv, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <xmqqh5mp9uke.fsf@gitster.g>
On 6/26/26 23:42, Junio C Hamano wrote:
Sorry, there was a problem with my Thunderbird client... so the my
emails didn't send without me noticing...
> Tian Yuchen <cat@malon.dev> writes:
>
>> This series continues the libification effort by migrating the global
>> string variable 'excludes_file' into 'struct repo_config_values'. Since
>> this is a dynamically allocated variable, the migration requires proper
>> heap memory management.
>
> This appears here:
>
> https://lore.kernel.org/git/20260626075037.532164-1-cat@malon.dev/
>
> and as you can see, there is no linkage back to the previous round.
>
> The lack of In-Reply-To and References headers unfortunately delays my
> automation in marking topics with newer iterations available to be
> reviewed when I come back to the keyboard, which happens overnight.
Regarding this, I forgot to bring --in-reply-to. I'll remember next time.
Thanks, yuchen
^ permalink raw reply
* Re: [PATCH v2 2/2] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-27 14:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, cirnovskyv, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <xmqqjyrl6rpt.fsf@gitster.g>
On 6/27/26 03:12, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
>
>> Continue the libification effort by moving the 'excludes_file' global
>> variable into 'struct repo_config_values'.
>>
>> Since 'excludes_file' is a dynamically allocated string (char *), it
>> requires proper memory management. Introduce repo_config_values_clear()
>> to safely free the heap memory when repository instance is destroyed.
>>
>> Note:
>>
>> - 'if (repo != the_repository)' fallback logic is temporarily added
>> in both the getter and the clear function. This prevents calling
>> repo_config_values() on uninitialized submodules, which triggers BUG().
>
> Would it be possible for the function to be called on the_repository
> before it gets initialized?
>
>> +void repo_config_values_clear(struct repository *repo)
>> +{
>> + struct repo_config_values *cfg;
>
> What I am wondering is if this check
>
>> + if (repo != the_repository)
>> + return;
>
> wants to be more like
>
> if (!repo->initialized)
> return;
>
> or even
>
> if (!repo->initialized) {
> BUG("clearing uninitialised repo config");
> return;
> }
>
> Or perhaps not doing anything special there.
>
> For that matter,
>
>> +
>> + cfg = repo_config_values(repo);
>> + if (!cfg)
>> + return;
>
> Wouldn't it be a bug to see NULL returned from the above function?
> Why is it healthy to pretend as if nothing bad happened?
>
>> + FREE_AND_NULL(cfg->attributes_file);
>> + FREE_AND_NULL(cfg->excludes_file);
>> +}
>
>
> What do we want to happen when somebody does want to access (not
> _clear(), but repo_config_values() itself) repo_config_values() in
> today's code? Don't we want to catch such a code as buggy? Isn't
> it the reason why repository.c:repo_config_values() check these
> conditions and calls BUG() in the first place? And if that is the
> case, I find that a caller that "works around" by pretending nothing
> bad happened and not calling repo_config_values(), like the above
> code does, highly questionable, as it smells like sweeping problems
> that you designed other parts of the code to detect with BUG() under
> the rug.
>
> In the longer run, we would want to have separate settings, which
> used to be global variables but now are stored in per repository
> config, available and usable in the context of each repository that
> they are configured within. If a caller wants to clear per repo
> config for a repository instance by calling this function, this
> function is in no place to tweak the intention of the caller by
> short-circuiting the request and pretending it did what it was asked
> to do. In other words, the rest of the code not quite prepared to
> deal with these global variables that turned into per repository
> configuration values is *not* a problem this function should
> address. Let it be noticed by repo_config_values() function to
> catch offending callers for now, and once the codebase becomes ready
> to use one repo_config_values per repository, this function does not
> have to change.
Yes, indeed. I initially thought these were two solutions leading to the
same goal, but now that you mention it, I think your point is more valid.
First, 'repo_config_values_clear()' shouldn't know whether "only
'the_repository' is currently supported" (even if this logic is
necessary, this function is clearly not the optimal place), so we
shouldn't use 'repo != the_repository' to determine this.
Second, what's wrong with '!cfg' is that what it's supposed to avoid
deviates from what it tries to avoid. 'repo_config_values(repo)'
shouldn't validly return NULL at all. I admit this code is nonsensical.
Using 'repo->initialized' to determine this is consistent with our
previous approach. I'll try it. Thanks for the review!
Regards, yuchen
^ permalink raw reply
* Re: [PATCH v2 1/2] dir: encapsulate excludes_file lazy-load
From: Tian Yuchen @ 2026-06-27 14:13 UTC (permalink / raw)
To: Junio C Hamano, SZEDER Gábor
Cc: git, cirnovskyv, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <xmqqldc1563l.fsf@gitster.g>
On 6/27/26 05:45, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> On Fri, Jun 26, 2026 at 03:50:36PM +0800, Tian Yuchen wrote:
>>> diff --git a/environment.c b/environment.c
>>> index ba2c60103f..8efcaeafa6 100644
>>> --- a/environment.c
>>> +++ b/environment.c
>>> @@ -134,6 +134,13 @@ int is_bare_repository(void)
>>> return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
>>> }
>>>
>>> +const char *repo_excludes_file(struct repository *repo)
>>> +{
>>> + if (!excludes_file)
>>> + excludes_file = xdg_config_home("ignore");
>>> + return excludes_file;
>>> +}
>>
>> This function has a 'repo' parameter, which is not used in the
>> function at all. This causes build failure when trying to build this
>> commit using DEVELOPER=1:
>>
>> environment.c: In function ‘repo_excludes_file’:
>> environment.c:137:51: error: unused parameter ‘repo’ [-Werror=unused-parameter]
>> 137 | const char *repo_excludes_file(struct repository *repo)
>> | ~~~~~~~~~~~~~~~~~~~^~~~
>> cc1: all warnings being treated as errors
>> make: *** [Makefile:2922: environment.o] Error 1
>>
>> Please make sure that all commits can be built with 'make
>> DEVELOPER=1'.
>
> Good point. In this case, we can start with UNUSED in step 1/2 and
> then drop the UNUSED in the second step. I wonder how harder to read
> it would become if these two patches are squashed together...
That makes sense. I think these two commits can be squadshed together...
I hope so.
>
> Thanks.
Regards, yuchen
^ permalink raw reply
* Re: [PATCH] t3420-rebase-autostash: don't try to grep non-existing files
From: Todd Zullinger @ 2026-06-27 14:27 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Michael Montalbo, Denton Liu
In-Reply-To: <aj90x3DsER5HASUS@szeder.dev>
SZEDER Gábor wrote:
> On Sun, Oct 10, 2021 at 07:28:09PM +0200, SZEDER Gábor wrote:
>> Several tests in 't3420-rebase-autostash.sh' start various rebase
>> processes that are expected to fail because of merge conflicts. The
>> tests [1] checking that 'git rebase --quit' and autostash work
>> together as expected after such a failure then run '! grep ...' to
>> ensure that the dirty contents of the file is gone. However, due to
>> the test repo's history and the choice of upstream branch that file
>> shouldn't exist in the conflicted state at all, and thus it shouldn't
>> exist after the subsequent 'git rebase --quit' either. Consequently,
>> this 'grep' doesn't fail as expected, i.e. because it can't find the
>> dirty content, but instead it fails, because it can't open the file.
>>
>> Thighten this check by using 'test_path_is_missing' instead, thereby
>> avoiding unexpected errors from 'grep' as well.
Thighten -> Tighten
--
Todd
^ permalink raw reply
* Re: [PATCH v2 5/6] t: convert grep assertions to test_grep
From: Junio C Hamano @ 2026-06-27 14:36 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Michael Montalbo via GitGitGadget, git, D. Ben Knoble,
Eric Sunshine, Michael Montalbo
In-Reply-To: <aj93BE8MYatQAjoy@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> The conversion was generated using a grep-assertion linter
>> (greplint.pl, added in the following commit) to identify bare
>> grep calls at command position. To reproduce:
>>
>> # Step 1: mark bare greps that should not be converted
>> sed -i '/! grep "$m" \.git\/packed-refs/s/$/ # lint-ok: file may not exist (reftable)/' \
>> t/t1400-update-ref.sh
>> sed -i '/! grep dirty file3 &&/{/lint-ok/!s/$/ # lint-ok: file may not exist after --quit/}' \
>> t/t3420-rebase-autostash.sh
>
> I think in this case checking the file3's contents is wrong, because
> at this point file3 should not exist in the first place. I've sent a
> patch to fix this long ago, but apparently didn't manage to follow
> through back then.
>
> https://lore.kernel.org/git/20211010172809.1472914-1-szeder.dev@gmail.com/
Thanks. I guess the test_grep can be extended to catch this case,
where
test_grep ! -e pattern1 -e pattern2 file
does not find any hits, but only because 'file' is missing, as an
error, just like "test_must_fail git foo" that segfaults is flagged
as "yes, it fails but that is not the kind of failure we expect".
^ permalink raw reply
* [PATCH v3 0/1] move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-27 15:45 UTC (permalink / raw)
To: git
Cc: cirnovskyv, szeder . dev @ gmail . com--in-reply-to=, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
This patch continues the libification effort by migrating the global
string variable 'excludes_file' into 'struct repo_config_values'. Since
this is a dynamically allocated variable, the migration requires proper
heap memory management.
This patch mainly does three things:
- Abstract the XDG fallback lazy-loading logic out of dir.c into a proper
getter.
- Move the variables into the struct repo_config_values.
- Introduce the memory destructor 'repo_config_values_clear()'.
Changes since V2:
- Squash together the previous two commits into one.
- The 'repo->initialized' check is used in both the getter and destructor.
This eliminates redundant checks and follows the fail-fast principle. This
is consistent with the previous global variable removal patches [1][2][3].
Thanks!
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>
[1] https://lore.kernel.org/git/20260610093635.139719-1-cat@malon.dev/T/#m856253610936d052a798259bfc06d598561e53c4
[2] https://lore.kernel.org/git/20260606143412.15443-1-cat@malon.dev/
[3] https://lore.kernel.org/git/20260617154929.564498-2-cat@malon.dev/T/#m8843984a6175a1a4c7e00877085c77b0c72f5803
Tian Yuchen (1):
environment: move excludes_file into repo_config_values
dir.c | 4 ++--
environment.c | 30 +++++++++++++++++++++++++++---
environment.h | 13 ++++++++++++-
repository.c | 1 +
4 files changed, 42 insertions(+), 6 deletions(-)
--
2.43.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox