* Re: [PATCH v3 02/11] doc: interpret-trailers: replace “lines” with “metadata”
From: Matt Hunter @ 2026-06-16 21:39 UTC (permalink / raw)
To: Kristoffer Haugsbakk, git
Cc: Christian Couder, jackmanb, Linus Arver, D. Ben Knoble
In-Reply-To: <26c2fcb2-2618-4bb9-a6e4-a6135934556d@app.fastmail.com>
On Tue Jun 16, 2026 at 4:32 PM EDT, Kristoffer Haugsbakk wrote:
> On Thu, Jun 11, 2026, at 05:10, Matt Hunter wrote:
>> On Wed Jun 10, 2026 at 5:21 PM EDT, kristofferhaugsbakk wrote:
>>>[snip]
>>> DESCRIPTION
>>> -----------
>>> -Add or parse _trailer_ lines at the end of the otherwise
>>> +Add or parse trailers metadata at the end of the otherwise
>>
>> fwiw, I think "trailer metadata" reads more naturally.
>
> You’re right that your version reads more naturally. I went back and
> forth on this.
After reading your points, I started debating the grammatical
correctness of it to myself too. Though I think I stand by my original
knee-jerk response.
>
> 1. We’re introducing the jargon, and the format is often discussed as
> plural “trailers”, with its constituent parts being singular
> “trailer”
> 2. What this replaces uses “trailer”, but it rescues the plural mood
> with “lines”
> 3. This is very soon going to go into the constituent parts, including
> each trailer, so we’re contrasting the concept name (trailers) with
> its parts
>
> But I think your version is overall better. It reads better and there is
> no way to confuse “trailer metadata” (trailers as a collection) with
> just a single “trailer”.
Yes, "metadata" reads as a hint to me that interpret-trailers works with
the overall trailer block too.
Another thought I had after seeing this again is that the text could
just say "Add or parse trailers at the end ...", but "metadata" is a lot
more useful, since it's an introduction to what trailers _are_. So the
patch is an improvement over the original context imo.
^ permalink raw reply
* Re: [PATCH v2 05/17] odb/source-packed: start converting to a proper `struct odb_source`
From: Justin Tobler @ 2026-06-16 21:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <20260609-pks-odb-source-packed-v2-5-839089132c8b@pks.im>
On 26/06/09 10:50AM, Patrick Steinhardt wrote:
> Start converting `struct odb_source_packed` into a proper pluggable
> `struct odb_source` by embedding the base struct and assigning it the
> new `ODB_SOURCE_PACKED` type. Furthermore, wire up lifecycle management
> of this source by implementing the `free` callback and taking ownership
> of the chdir notifications.
>
> Note that the packed source is not yet functional as a standalone `struct
> odb_source`, as it's missing all of the callback implementations. These
> will be wired up in subsequent commits.
Ok.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
> {
> - struct odb_source_packed *store;
> - CALLOC_ARRAY(store, 1);
> - store->files = parent;
> - strmap_init(&store->packs_by_path);
> - return store;
> + struct odb_source_packed *packed;
> +
> + CALLOC_ARRAY(packed, 1);
> + odb_source_init(&packed->base, parent->base.odb, ODB_SOURCE_PACKED,
> + parent->base.path, parent->base.local);
> + packed->files = parent;
> + strmap_init(&packed->packs_by_path);
> +
> + packed->base.free = odb_source_packed_free;
> +
> + if (!is_absolute_path(parent->base.path))
> + chdir_notify_register(NULL, odb_source_packed_reparent, packed);
Out of curiousity, did the packfile store previously not have to worry
about changing directories?
> +
> + return packed;
> }
> diff --git a/odb/source-packed.h b/odb/source-packed.h
> index 3c2d229a17..68e64cabab 100644
> --- a/odb/source-packed.h
> +++ b/odb/source-packed.h
> @@ -9,6 +9,7 @@
> * A store that manages packfiles for a given object database.
> */
> struct odb_source_packed {
> + struct odb_source base;
> struct odb_source_files *files;
At first I got confused as to why we were adding back a `struct
odb_source` pointer, but this is for the "base" not the parent ODB
source. We need to embed a `struct odb_source` here to make this a
proper ODB source.
Looking good.
-Justin
^ permalink raw reply
* Re: SHA-1/SHA-256 interoperability work is functional
From: brian m. carlson @ 2026-06-16 21:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqldce2r1a.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 6352 bytes --]
On 2026-06-16 at 20:01:37, Junio C Hamano wrote:
> Not that I specifically care about packfile URI, but this one is
> curious. How would regular "fetch" and "push" traffic work under
> the new world order? Presumably we will keep one characteristic of
> the protocol, that the packdata stream is the only thing that is
> given to the other side and no object names are given, because the
> receiving end would not want to blindly trust the object name the
> sending end _claims_ to have sent and instead recomputes the object
> name out of the packed objects in the data stream ("if we rehash
> and recompute the object names from the datastream, the other side
> cannot lie to us" IIRC was a security measure).
>
> For a regular "fetch" and "push" to work, we would need to recompute
> the native object names and also somehow compute the compatibility
> object names if we are in interoperability mode, no?
>
> If we download *.pack files from a packfile URI, wouldn't it be the
> same story?
Let me explain how conversion works. Say you have an empty local
repository with SHA-256 as the main algorithm and SHA-1 as the
compatibility algorithm, plus a SHA-1 remote. When you do `git fetch`,
you get a SHA-1 pack. You cannot write this into the repository because
your repository doesn't use SHA-1 as the main algorithm, so `git
index-pack` takes the pack and maps any objects. If the objects are in
the new pack, they get rewritten based on the dependencies; otherwise,
Git uses the existing maps in the repository to rewrite the objects.
`git index-pack` then writes a completely new SHA-256 pack with an index
containing the SHA-1 mapping, using the corresponding deltas[0].
However, `git index-pack` can only index and map objects for one pack at
a time. We therefore need any pack that we get to be connected to our
existing history so that we can rely on our existing maps to remap
objects that are not in the pack. For instance, if we get a commit
without its parents, then we'll simply die because those objects cannot
be mapped and we can't write the mapping in the index.
The problem is that that packfile URIs result in multiple packs (one of
which is the dynamically generated protocol pack) that, _in total_,
provide a complete history with what we have, but are not necessarily
individually connected to our existing history. Moreover, the
dynamically generated protocol pack is sent _first_, so if we have
packfile URIs, that pack is almost certainly guaranteed _not_ to connect
to our existing history. We would therefore have to pause index-pack,
download all the packfile URIs, index those packfiles (which would have
to be connected to our history), and then unpause index-pack to rewrite
the history. This is not impossible, but it's tricky, and it has yet to
be implemented. Someone may decide that this is a valuable feature and
implement it, but it's not on my to-do list.
This doesn't pose a problem with single-algorithm repositories because
if you have unreferenced and unconnected objects, no big deal. They
just don't get used and will eventually get GC'd. But since we can't
map those objects in a multi-algorithm world, that's fatal and those
packs can't be indexed.
> > * Large object promisors cannot be used if the server does not actually have
> > the entire history, since the server must have a complete history in order to
> > provide object mappings.
>
> Again, this one worries me a bit, but perhaps I am not reading it
> correctly. Does this mean that the server side says "this is the
> data for object whose name is X in the SHA-1 world, which translates
> to X256 in the SHA-256 world", the receiving end blindly trusts
> without having a way to verify?
The server provides algorithm mappings for for submodules, shallow
clones, and partial clones. For shallow clones and partial clones, you
have to trust the server anyway because you're already getting a
truncated history. If you complete the history by fetching the missing
objects and run `git fsck`, then it will detect if the mapping is
invalid because the server was dishonest and complain. You will have a
corrupt set of mappings to the compatibility algorithm, but those could
theoretically be repaired.
However, in order for the server to produce those mappings, it has to
know the entire history. If there are objects that are outside the
repository in a secondary location, the server will not have mappings
for those objects and so it will abort the protocol.
The server does not normally provide mappings for non-submodules if
you're doing a regular fetch or clone, since the client has a
self-contained history and does not need those objects to compute the
mapping. That means that regular clones and fetches work just fine
against existing servers as long as no submodules are involved[1].
The tricky part is submodules. Because the data is in a separate
repository, we cannot be certain of the mapping. The documentation says
this:
There is a potential security problem with providing mappings of
submodules over the protocol. Namely, there is no way to guarantee
that the SHA-1 object ID and the SHA-256 object ID correspond to the
same commit. This means that, for example, a malicious server could
provide a SHA-256 object ID for a submodule that was up to date with
all security fixes, but map that to a SHA-1 object ID for an older
commit with security problems.
We therefore reject submodule mappings if fsck verification for
transferred objects is enabled unless the user has explicitly enabled
submodule mappings.
[0] If A deltas against B in SHA-1, then when those are rewritten into
SHA-256, we delta the SHA-256 A against the SHA-256 B. This does not
guarantee the best possible delta, but it is much cheaper than
redeltifying and because we expect remapped objects to have the same
shape, it should delta well enough in most cases.
[1] For instance, if you build my branch, you can do
`git clone --object-format=sha256:sha1 https://github.com/bk2204/lawn.git`
and it just works since there are no submodules. My dotfiles, on the
other hand, have submodules and will not work without protocol support.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 09/12] transport: add client support for object-info
From: Junio C Hamano @ 2026-06-16 21:31 UTC (permalink / raw)
To: Pablo Sabater
Cc: eric.peijian, chriscool, git, jltobler, karthik.188, toon,
chandrapratap3519
In-Reply-To: <xmqq1pe62pgo.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> [jc: removed recipients from Cc: list whose addresses bounce]
>
>> 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:
>> ...
>> diff --git a/fetch-object-info.c b/fetch-object-info.c
>> ...
>> +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)
>> +{
>> ...
>> + 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, ""))
>> + die("object-info: server does not recognize object %s",
>> + object_info_values.items[0].string);
>> +
>> + if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))
>
>
> Overly long lines need to be fixed, by using a shorter and crisper
> variable name in such a short scope, and line wrapping if needed.
>
> More importantly, on this line (wrapped):
>
> if (strtoul_ul(object_info_values.items[1 + size_index].string,
> 10, object_info_data[i].sizep))
>
> we notice object_info_data[i] is of type "struct object_info", which
> is
>
> struct object_info {
> /* Request */
> enum object_type *typep;
> size_t *sizep;
> off_t *disk_sizep;
> ...
>
> but the last parameter strtoul_ul() takes is unsurprisingly a
> pointer to "unsigned long", not a pointer to "size_t".
>
> Which will break on 32-bit boxes where size_t is "unsigned int"
> that is 32-bit and different from "unsigned long".
>
> Perhaps something along this line?
Not quite. This "size_t *sizep" has been very recently introduced
by Dscho in a topic that is in-flight.
The ps/cat-file-remote-object-info topic alone does not have this
type-mismatch problem. Below needs to be addressed as an evil merge
at the integration side, so you have nothing to do. I'll have to
tweak the merges.
Sorry for a false alarm.
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> index 425929a269..5210e7d954 100644
> --- a/fetch-object-info.c
> +++ b/fetch-object-info.c
> @@ -75,14 +75,17 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>
> string_list_split(&object_info_values, reader->line, " ", -1);
> if (0 <= size_index) {
> + unsigned long sz;
> if (!strcmp(object_info_values.items[1 + size_index].string, ""))
> die("object-info: server does not recognize object %s",
> object_info_values.items[0].string);
>
> - if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))
> + if (strtoul_ul(object_info_values.items[1 + size_index].string,
> + 10, &sz))
> die("object-info: ref %s has invalid size %s",
> object_info_values.items[0].string,
> object_info_values.items[1 + size_index].string);
> + *object_info_data[i].sizep = sz;
> }
>
> string_list_clear(&object_info_values, 0);
^ permalink raw reply
* Re: [PATCH v2 04/17] odb/source-packed: store pointer to "files" instead of generic source
From: Justin Tobler @ 2026-06-16 21:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <ajG69JZHx_u2mt7q@denethor>
On 26/06/16 04:14PM, Justin Tobler wrote:
> On 26/06/09 10:50AM, Patrick Steinhardt wrote:
> > The `struct odb_source_packed` holds a pointer to its owning parent
> > source. The way that Git is currently structured, this parent is always
> > the "files" source. In subsequent commits we're going to detangle that
> > so that the "packed" source doesn't have any owning parent source at
> > all, which makes it usable as a completely standalone source.
>
> Out of curiousity, `struct odb_source_loose` also stores a similar to
> pointer to its owning parent. Is the plan to also eventually do the same
> there?
Ok, I think I got myself mixed up. IIUC the `struct odb_source` pointer
currently stored in `struct odb_source_loose` is not to the parent
source, but to its "base". And the eventual goal here is to move towards
that same direction which makes sense.
-Justin
^ permalink raw reply
* Re: [PATCH v2 2/7] patch-delta: use size_t for sizes
From: Junio C Hamano @ 2026-06-16 21:26 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Kristofer Karlsson, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <66a642c39e7755755fe388af7612ac8c9bf41a5a.1781524349.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Widen `patch_delta()`'s three size parameters to `size_t` and switch
> its internal use of `get_delta_hdr_size()` to the `_sz` variant.
> Then propagate the wider type through the callers.
Makes sense.
^ permalink raw reply
* Re: [PATCH v2 04/17] odb/source-packed: store pointer to "files" instead of generic source
From: Justin Tobler @ 2026-06-16 21:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <20260609-pks-odb-source-packed-v2-4-839089132c8b@pks.im>
On 26/06/09 10:50AM, Patrick Steinhardt wrote:
> The `struct odb_source_packed` holds a pointer to its owning parent
> source. The way that Git is currently structured, this parent is always
> the "files" source. In subsequent commits we're going to detangle that
> so that the "packed" source doesn't have any owning parent source at
> all, which makes it usable as a completely standalone source.
Out of curiousity, `struct odb_source_loose` also stores a similar to
pointer to its owning parent. Is the plan to also eventually do the same
there?
> Detangling this mess is somewhat intricate though, and is made even more
> intricate because it's not always clear which kind of source one is
> holding at a specific point in time -- either the parent "files" source,
> or the child "packed" source.
>
> Make this relationship more explicit by storing a pointer to the "files"
> source instead of storing a pointer to a generic `struct odb_source`.
> This will help make subsequent steps a bit clearer.
>
> Note that this is a temporary step, only. At the end of this series
> we will have dropped the parent pointer completely.
Ok, so IIUC the eventual goal is to get rid of the pointer entirely, but
for now we are just making its concrete type explicit without having to
downcast. It's not immediately obvious to me how this step gets us
closer to that goal, but that may become more obvious in the next
patches. :)
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> odb/source-files.c | 2 +-
> odb/source-packed.c | 4 ++--
> odb/source-packed.h | 4 ++--
> packfile.c | 12 ++++++------
> 4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 191562f316..e04525fb08 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -269,7 +269,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
> CALLOC_ARRAY(files, 1);
> odb_source_init(&files->base, odb, ODB_SOURCE_FILES, path, local);
> files->loose = odb_source_loose_new(odb, path, local);
> - files->packed = odb_source_packed_new(&files->base);
> + files->packed = odb_source_packed_new(files);
>
> files->base.free = odb_source_files_free;
> files->base.close = odb_source_files_close;
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 1e94b47ea0..12e785be48 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -1,11 +1,11 @@
> #include "git-compat-util.h"
> #include "odb/source-packed.h"
>
> -struct odb_source_packed *odb_source_packed_new(struct odb_source *source)
> +struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
> {
> struct odb_source_packed *store;
> CALLOC_ARRAY(store, 1);
> - store->source = source;
> + store->files = parent;
> strmap_init(&store->packs_by_path);
> return store;
> }
> diff --git a/odb/source-packed.h b/odb/source-packed.h
> index 327be4ad65..3c2d229a17 100644
> --- a/odb/source-packed.h
> +++ b/odb/source-packed.h
> @@ -9,7 +9,7 @@
> * A store that manages packfiles for a given object database.
> */
> struct odb_source_packed {
> - struct odb_source *source;
> + struct odb_source_files *files;
The rest of this patch updates the callsites accordingly and looks
correct.
-Justin
^ permalink raw reply
* Re: [PATCH v2 01/17] packfile: rename `struct packfile_store` to `odb_source_packed`
From: Justin Tobler @ 2026-06-16 21:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <20260609-pks-odb-source-packed-v2-1-839089132c8b@pks.im>
On 26/06/09 10:50AM, Patrick Steinhardt wrote:
> Not too long ago, we have introduced the packfile store in b7983adb51
> (packfile: introduce a new `struct packfile_store`, 2025-09-23). This
> struct is responsible for managing all of our access to packfiles and is
> used as one of the two sources of objects for the "files" source.
>
> Back when I introduced this structure I didn't have the clear vision yet
> that it will eventually also turn into a proper object database source,
> and how exactly that infrastructure will look like. Now though it's
> becoming increasingly clear that it does make sense to treat it just the
> same as any of our other ODB sources.
We already have `struct odb_source_loose` which is used in `struct
odb_source_files` which is a proper ODB source. Since it seems accessing
packed objects is moving in the same direction, the renaming here makes
sense to me.
> The consequence is that the naming is now a bit out-of-date: it's just
> another source and will be turned into a proper `struct odb_source` over
> the next couple of commits, but it's not named accordingly.
>
> Rename the structure to `odb_source_packed` to align it with this goal
> and to bring it in line with the other sources we already have.
The rest of this patch is just trivial renames and looks good to me.
-Justin
^ permalink raw reply
* Re: [PATCH GSoC RFC v12 09/12] transport: add client support for object-info
From: Junio C Hamano @ 2026-06-16 20:35 UTC (permalink / raw)
To: Pablo Sabater
Cc: eric.peijian, chriscool, git, jltobler, karthik.188, toon,
chandrapratap3519
In-Reply-To: <20260608-ps-eric-work-rebase-v12-9-5338b766e658@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
[jc: removed recipients from Cc: list whose addresses bounce]
> 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:
> ...
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> ...
> +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)
> +{
> ...
> + 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, ""))
> + die("object-info: server does not recognize object %s",
> + object_info_values.items[0].string);
> +
> + if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))
Overly long lines need to be fixed, by using a shorter and crisper
variable name in such a short scope, and line wrapping if needed.
More importantly, on this line (wrapped):
if (strtoul_ul(object_info_values.items[1 + size_index].string,
10, object_info_data[i].sizep))
we notice object_info_data[i] is of type "struct object_info", which
is
struct object_info {
/* Request */
enum object_type *typep;
size_t *sizep;
off_t *disk_sizep;
...
but the last parameter strtoul_ul() takes is unsurprisingly a
pointer to "unsigned long", not a pointer to "size_t".
Which will break on 32-bit boxes where size_t is "unsigned int"
that is 32-bit and different from "unsigned long".
Perhaps something along this line?
diff --git a/fetch-object-info.c b/fetch-object-info.c
index 425929a269..5210e7d954 100644
--- a/fetch-object-info.c
+++ b/fetch-object-info.c
@@ -75,14 +75,17 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
string_list_split(&object_info_values, reader->line, " ", -1);
if (0 <= size_index) {
+ unsigned long sz;
if (!strcmp(object_info_values.items[1 + size_index].string, ""))
die("object-info: server does not recognize object %s",
object_info_values.items[0].string);
- if (strtoul_ul(object_info_values.items[1 + size_index].string, 10, object_info_data[i].sizep))
+ if (strtoul_ul(object_info_values.items[1 + size_index].string,
+ 10, &sz))
die("object-info: ref %s has invalid size %s",
object_info_values.items[0].string,
object_info_values.items[1 + size_index].string);
+ *object_info_data[i].sizep = sz;
}
string_list_clear(&object_info_values, 0);
^ permalink raw reply related
* Re: [PATCH v3 02/11] doc: interpret-trailers: replace “lines” with “metadata”
From: Kristoffer Haugsbakk @ 2026-06-16 20:32 UTC (permalink / raw)
To: Matt Hunter, git; +Cc: Christian Couder, jackmanb, Linus Arver, D. Ben Knoble
In-Reply-To: <DJ5W2I8UYXAA.3O4JQUHFMKP5X@lfurio.us>
On Thu, Jun 11, 2026, at 05:10, Matt Hunter wrote:
> On Wed Jun 10, 2026 at 5:21 PM EDT, kristofferhaugsbakk wrote:
>>[snip]
>> DESCRIPTION
>> -----------
>> -Add or parse _trailer_ lines at the end of the otherwise
>> +Add or parse trailers metadata at the end of the otherwise
>
> fwiw, I think "trailer metadata" reads more naturally.
You’re right that your version reads more naturally. I went back and
forth on this.
1. We’re introducing the jargon, and the format is often discussed as
plural “trailers”, with its constituent parts being singular
“trailer”
2. What this replaces uses “trailer”, but it rescues the plural mood
with “lines”
3. This is very soon going to go into the constituent parts, including
each trailer, so we’re contrasting the concept name (trailers) with
its parts
But I think your version is overall better. It reads better and there is
no way to confuse “trailer metadata” (trailers as a collection) with
just a single “trailer”.
^ permalink raw reply
* Re: [PATCH 6/6] SubmittingPatches: note that trailer order matters
From: Kristoffer Haugsbakk @ 2026-06-16 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8q8mt4eo.fsf@gitster.g>
On Thu, Jun 11, 2026, at 00:30, Junio C Hamano wrote:
>>[snip]
>> Only capitalize the very first letter of the trailer, i.e. favor
>> `Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
>>
>> +Note that these trailers should come before your `Signed-off-by:`
>> +trailer. You are signing off to the patch as well as the message. This
>> +also makes it clear who added trailers when multiple people have signed
>> +off on a patch.
>
> Perhaps first mention the underlying rule that they are added in the
> order that helps us to understand the chronological order of events.
> That would avoid giving a wrong impression that the nature of each
> trailer keys determine the order of these lines.
You’re right. It’s best to lead with the time-based order. That
naturally leads into the implication that we can easily read what
trailers that any given person added.
I’ve seen discussions in the past about whether trailers ought to be
sorted by *kind*, and maybe if first e.g. you should first have
Tested-by, then Reviewed-by, then Signed-off-by... best to not give such
an impression by accident.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/6] SubmittingPatches: encourage trailer use for substantial help
From: Kristoffer Haugsbakk @ 2026-06-16 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1pedowl2.fsf@gitster.g>
On Thu, Jun 11, 2026, at 18:44, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> Let’s replace “If you like” with outright encouragment in this section
>
> "encouragement"?
Yep.
>
>> At the same, it is important to temper this recommendation to a sign-
>> ificant enough contribution; in my experience beginners can be eager
>
> "At the same time"?
Yep.
>
> It is a bit unusual to see a long word split at the end of a line
> to line-wrap in our documentation and commit log messages.
A bit unusual is an understatement. I cannot find any other commit log
message writers that have split a word on a syllable. All linebreaks
that I’ve found are on existing hyphens. Like
... multi-pack-
indexes
I’ll avoid this in the future.
>
>> ---
>> Documentation/SubmittingPatches | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> The patch text itself looks great. Thanks.
Thanks for going over these.
^ permalink raw reply
* Re: [PATCH 4/6] SubmittingPatches: document Based-on-patch-by trailer
From: Kristoffer Haugsbakk @ 2026-06-16 20:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqse6tnho1.fsf@gitster.g>
On Thu, Jun 11, 2026, at 18:52, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> +. `Based-on-patch-by:` can be used when someone else authored parts of
>> + the patch that you are submitting. This might be relevant if someone
>> + sent a patch to the mailing list without a commit message or a
>> + `Signed-off-by:` and you have picked it up.
>
> Hmph, this seems to encourage pick up material that come outside of
> the usual DCO process, which should not be the intention of this
> document.
Oh, I have misread the room on this subject. It would be better to drop
the mention of signoff here.
>
> Unless the changes are trivial enough to not be copyrightable, it
> may be better to say "... if someone submitted a preliminary patch or
> a detailed code snippet with their sign-off", plus encourage asking
> the original author to sign-off if it initially came without, or
> something like that?
Okay, since they provided something concrete to copy (cf. Helped-by
where they did not provide precise changes in patch form, according to
the below context), it’s best to mention that signoff is relevant here.
>
>> . `Helped-by:` is used to credit someone who suggested ideas for
>> changes without providing the precise changes in patch form.
>> . `Mentored-by:` is used to credit someone with helping develop a
^ permalink raw reply
* Re: [PATCH 2/6] SubmittingPatches: discuss non-ident trailers
From: Kristoffer Haugsbakk @ 2026-06-16 20:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aivvE6gVMGWhRbCB@pks.im>
On Fri, Jun 12, 2026, at 13:35, Patrick Steinhardt wrote:
> On Thu, Jun 11, 2026 at 12:22:45AM +0200,
> kristofferhaugsbakk@fastmail.com wrote:
>> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
>> index 0b12badf86d..51c308a89a8 100644
>> --- a/Documentation/SubmittingPatches
>> +++ b/Documentation/SubmittingPatches
>> @@ -474,7 +474,10 @@ These are the common trailers in use:
>>
>> While you can also create your own trailer if the situation warrants it, we
>> encourage you to instead use one of the common trailers in this project
>> -highlighted above.
>> +highlighted above. A trailer that credits someone might be more likely
>> +to be accepted since these are the most common ones. But another kind of
>> +trailer might be relevant, for example to link to an issue tracker
>> +belonging to a downstream project that is affected by a bug in Git.
>
> Hm, I wonder whether this is a bit too vague to really be helpful for a
> newcomer. Instead of alluding to such trailers, wouldn't it be
> preferable if we added those as actual examples to the list of known
> trailers and then tell folks that they can invent their own ones if
> there is a good reason to do so?
Honestly there are so few non-ident trailers that I don’t think they can
be listed as common trailers:
1. The Git project doesn’t need them (e.g. no bug tracker)
2. They seem mostly for use by other projects (bug trackers again)
With this list:
git log --format='%(trailers:only,keyonly)' | sort | uniq
If you filter out the ident-looking ones:
grep -v --extended-regexp -- '-[Bb]y$'
There are few left. And some can be discarded:
• Change-Id
• Message-ID
• Fixes (pointing to a commit)
So to address your point:
1. Maybe this is so niche that it is not worth mentioning; or
2. Maybe give a concrete example like `Closes: <bug link>`?
^ permalink raw reply
* Re: SHA-1/SHA-256 interoperability work is functional
From: Junio C Hamano @ 2026-06-16 20:01 UTC (permalink / raw)
To: brian m. carlson; +Cc: git
In-Reply-To: <ajCWBG9RHBrm8jMZ@fruit.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I'm pleased to announce that I have Git fully passing the testsuite and
> CI in interoperability mode, both with SHA-256 and SHA-1 as the main
> algorithm. While this is very exciting, the work is not ready to send
> to the list and is effectively a draft, since there is still cleanup
> and efficiency work to be done.
Great to hear about a great milestone.
> Features which are currently unsupported (and which may or may not be
> supported in the future):
>
> * Filtered bundles are unsupported because there is currently no way to provide
> a mapping.
> * Multi-pack index cannot be used as the sole pack index format because it does
> not yet provide mappings.
> * Pack index v1 and v2 cannot be used because they do not provide object
> mappings. Git automatically uses pack index v3 instead when necessary, which
> does handle mappings.
> * Packfile URIs are not supported because the protocol-provided packfile is not
> complete and its objects cannot be mapped.
Not that I specifically care about packfile URI, but this one is
curious. How would regular "fetch" and "push" traffic work under
the new world order? Presumably we will keep one characteristic of
the protocol, that the packdata stream is the only thing that is
given to the other side and no object names are given, because the
receiving end would not want to blindly trust the object name the
sending end _claims_ to have sent and instead recomputes the object
name out of the packed objects in the data stream ("if we rehash
and recompute the object names from the datastream, the other side
cannot lie to us" IIRC was a security measure).
For a regular "fetch" and "push" to work, we would need to recompute
the native object names and also somehow compute the compatibility
object names if we are in interoperability mode, no?
If we download *.pack files from a packfile URI, wouldn't it be the
same story?
> * Large object promisors cannot be used if the server does not actually have
> the entire history, since the server must have a complete history in order to
> provide object mappings.
Again, this one worries me a bit, but perhaps I am not reading it
correctly. Does this mean that the server side says "this is the
data for object whose name is X in the SHA-1 world, which translates
to X256 in the SHA-256 world", the receiving end blindly trusts
without having a way to verify?
> There is new documentation in `Documentation/gitformat-hash.adoc` that
> outlines the requirements for using the protocol. The protocol
> restrictions described there are hard technical limitations that cannot
> be avoided; I've intentionally made things as featureful as they can be.
> This imposes real restrictions on using protocol interoperability with many
> projects, including Git and Linux[0]. Interested parties may wish to look
> at t1017 to see what's tested vis-à-vis the protocol and
> interoperability.
> ...
> If you're interested in testing or perusing the work, you may get it
> from the `sha256-interop` branch of https://github.com/bk2204/git.git.
> Please note that it may be rebased, rewound, or otherwise folded,
> spindled, or mutilated at any time.
Sounds exciting.
Thanks.
^ permalink raw reply
* Re: Assisted-by tag
From: Kristoffer Haugsbakk @ 2026-06-16 19:53 UTC (permalink / raw)
To: Marius Spix, git
In-Reply-To: <20260616212553.31ddea83@rockhopper>
On Tue, Jun 16, 2026, at 21:25, Marius Spix wrote:
> as the Linux kernel requires the new Assisted-by tag for AI-assisted
> commits, I was researching how git handles such tags. Thereby I
> observed the following behaviour:
>
> git commit --signoff
> * adds an empty line before the Signed-off-by tag
> * ignores the Signed-off-by tag by checking for an empty commit message
That a bare message which is just `Signed-off-by` is considered an
“empty” commit message seems like a historical quirk. It checks
specifically for that tag/trailer.
>
> git commit --trailer "\nAssisted-by: OpenAI"
> * does not add an empty line (the "\n" is not converted to a newline)
> * does not ignore the tag by checking for empty commit message
This is just a regular trailer. I don’t know why you have a `\n`.
git commit --trailer "Assisted-by: OpenAI"
Any number of these will populate the trailer block.
> Since there will be more and more AI-assisted commits in projects like
> the Linux kernel in the future, this should be taken in account.
>
> When merging or squashing commits, that tag should also be
> automatically applied to the new commit message to make it clear that
> the commit is tainted by AI.
That `Signed-off-by` has dedicated options and logic is historical
baggage at this point.
A 2013 [patch] to add `git commit --fixes` because the Linux Kernel uses
`Fixes` was rejected because the Git project considers tags/trailers
project-specific. Instead git-interpret-trailers(1) was born which
eventually provided the code base for `git commit --trailer` and similar
options.
[patch]: https://lore.kernel.org/all/20131027013402.GA7146@leaf/
Handling how trailers are added is also deemed project-specific. There
are only the configurations and options that git-interpret-trailers(1)
provides. And handling that commits are rewritten (combined and so on)
in such a way that trailer-taint sticks is beyond any discussion I’ve
seen on the subject.
^ permalink raw reply
* Assisted-by tag
From: Marius Spix @ 2026-06-16 19:25 UTC (permalink / raw)
To: git
Hi there,
as the Linux kernel requires the new Assisted-by tag for AI-assisted
commits, I was researching how git handles such tags. Thereby I
observed the following behaviour:
git commit --signoff
* adds an empty line before the Signed-off-by tag
* ignores the Signed-off-by tag by checking for an empty commit message
git commit --trailer "\nAssisted-by: OpenAI"
* does not add an empty line (the "\n" is not converted to a newline)
* does not ignore the tag by checking for empty commit message
Since there will be more and more AI-assisted commits in projects like
the Linux kernel in the future, this should be taken in account.
When merging or squashing commits, that tag should also be
automatically applied to the new commit message to make it clear that
the commit is tainted by AI.
Your opinion?
Best regards
Marius
^ permalink raw reply
* Re: [PATCH v14 4/6] branch: add --prune-merged <branch>
From: Harald Nordgren @ 2026-06-16 19:15 UTC (permalink / raw)
To: Phillip Wood
Cc: Harald Nordgren via GitGitGadget, git, Kristoffer Haugsbakk,
Johannes Sixt
In-Reply-To: <78b6dfdd-df61-4c44-96eb-b527cb26243c@gmail.com>
> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> > index 4e7deddc04..27ea1319bb 100755
> > --- a/t/t3200-branch.sh
> > +++ b/t/t3200-branch.sh
> > @@ -1809,4 +1809,205 @@ test_expect_success '--forked requires a value' '
> > test_grep "requires a value" err
> > '
> >
> > +test_expect_success '--prune-merged: setup' '
> > + test_create_repo pm-upstream &&
>
> The rest of this test would be easier to read if we did
>
> (
> cd pm-upstream &&
> ...
> )
>
> rather than prefixing every command with "-C pm-upstream"
I feel like the discussion to nest or not to nest has come up many
times in other topics as well. I don't feel strongly about either way,
but I just want to flag that if I change it now, another reviewer
might ask me to change it back later.
Should the rules be to nest inside of setup functions (and helpers?)
but not inside the actual tests?
> > + test_commit -C pm-upstream base &&
> > + git -C pm-upstream checkout -b next &&
> > + test_commit -C pm-upstream one-commit &&
> > + test_commit -C pm-upstream two-commit &&
> > + git -C pm-upstream branch one HEAD~ &&
> > + git -C pm-upstream branch two HEAD &&
> > + git -C pm-upstream branch wip main &&
> > + git -C pm-upstream checkout main &&
> > + test_create_repo pm-fork
> > +'
> > +
> > +test_expect_success '--prune-merged deletes branches integrated into upstream' '
> > + test_when_finished "rm -rf pm-merged" &&
> > + git clone pm-upstream pm-merged &&
> > + git -C pm-merged remote add fork ../pm-fork &&
> > + test_config -C pm-merged remote.pushDefault fork &&
> > + test_config -C pm-merged push.default current &&
>
> So we clone upstream and add fork as the default push remote. I find the
> pm- prefixes rather distracting. It would be clearer to me if we just
> called the repositories "upstream", "fork" and "repo"
Good point.
> > + test_must_fail git -C pm-local rev-parse --verify refs/heads/one
> > +'
> > +
> > +test_expect_success '--prune-merged warns instead of erroring on un-integrated commits' '
> > + test_when_finished "rm -rf pm-unmerged" &&
> > + git clone pm-upstream pm-unmerged &&
> > + git -C pm-unmerged remote add fork ../pm-fork &&
> > + test_config -C pm-unmerged remote.pushDefault fork &&
> > + test_config -C pm-unmerged push.default current &&
> > + git -C pm-unmerged checkout -b wip origin/wip &&
> > + git -C pm-unmerged branch --set-upstream-to=origin/next wip &&
> > + test_commit -C pm-unmerged local-only &&
> > + git -C pm-unmerged checkout - &&
> > +
> > + git -C pm-unmerged branch --prune-merged "origin/*" 2>err &&
> > + test_grep "not fully merged" err &&
> > + test_grep ! "If you are sure you want to delete it" err &&
>
> I'm always suspicious of test_grep when we know what the output should
> look like - it might be better to use test_cmp. This test does not check
> that we also delete branches that are merged when we see one that isn't.
>
> I'm going to stop here - the tests I've read seem to me to be too much
> like unit tests checking one aspect of the implementation in isolation
> rather than checking that the whole feature works as expected.
I'll respond to the rest here: Excellent points regarding the testing
aboce, I will take a look at doing this.
Harald
^ permalink raw reply
* Re: [PATCH v14 6/6] branch: add --dry-run for --prune-merged
From: Harald Nordgren @ 2026-06-16 18:28 UTC (permalink / raw)
To: Phillip Wood
Cc: Harald Nordgren via GitGitGadget, git, Kristoffer Haugsbakk,
Johannes Sixt
In-Reply-To: <7b43a0f1-32a0-40f0-8c82-d2ee78809cc2@gmail.com>
> > With --dry-run, --prune-merged prints the local branches it would
> > delete, one "Would delete branch <name>" line each, and exits
> > without touching any ref. The same filtering applies, so the output
> > is exactly the set that the real run would delete.
>
> I can see this being very useful.
Great to hear and thanks for taking the time to review this! Much appreciated!
> > static int prune_merged_branches(int argc, const char **argv,
> > - int quiet)
> > + int quiet, int dry_run)
>
> Let's not start adding multiple boolean augments - use a flags argument
> like we do for delete_branches() - if you get feedback on one patch you
> should think about whether it applies later in the series as well. The
> rest of the implementation looks good.
I'm trying to generalize all feedback, but sometimes I miss things.
Thanks for pointing it out!
Harald
^ permalink raw reply
* Re: [GSoC Patch v5 2/4] rev-parse: use append_formatted_path() for path formatting
From: Phillip Wood @ 2026-06-16 18:26 UTC (permalink / raw)
To: K Jayatheerth
Cc: git, jltobler, lucasseikioshiro, gitster, phillip.wood, sandals,
kumarayushjha123, a3205153416
In-Reply-To: <CA+rGoLfhhRNrSReeJ1grhy+2K3BSrikTCNgGpCaGqc4fFp3Lfg@mail.gmail.com>
On 16/06/2026 18:04, K Jayatheerth wrote:
> Hi Phillip,
> Thanks for taking a look!
>
>> On 16/06/2026 05:49, K Jayatheerth wrote:
>>> The path-formatting logic in builtin/rev-parse.c is tightly coupled
>>> to that command and writes directly to stdout, making it impossible
>>> for other builtins to reuse.
>>>
>>> Extract the core algorithm into append_formatted_path() in path.c
>>> and expose a path_format enum in path.h so that any builtin can
>>> format paths consistently without duplicating logic.
>>
>> Sorry I haven't had time to look at this series recently, it is looking
>> much nicer now that we have a single enum. It would be helpful to
>> explain why we need PATH_FORMAT_DEFAULT that acts exactly like
>> PATH_FORMAT_UNMODIFIED. Looking at the next patch it seems this is still
>> a wart in the api due to rev-parse wanting needing to distinguish the
>> unmodified case from the default case.
> t);
>>> +
>>> # ifdef USE_THE_REPOSITORY_VARIABLE
>>> # include "strbuf.h"
>>> # include "repository.h"
>>
>
>
>>> int cmd_rev_parse(int argc,
>>> @@ -717,7 +661,7 @@ int cmd_rev_parse(int argc,
>>> const char *name = NULL;
>>> struct strbuf buf = STRBUF_INIT;
>>> int seen_end_of_options = 0;
>>> - enum format_type format = FORMAT_DEFAULT;
>>> + enum path_format arg_path_format = PATH_FORMAT_DEFAULT;
>>
>> This is the source of the api wart I referred to in the previous patch.
>> Could we keep the existing enums and convert them into the appropriate
>> PATH_FORMAT_* flag in print_path() above? I think we already have the
>> logic to do that in the existing code. That would mean that other users
>> of append_formatted_path() don't have to worry about the extra flag.
>>
>
> That is a much more elegant solution than the current one.
>
> For v6, I will clean this up by keeping the fallback logic
> localized within builtin/rev-parse.c and removing
> PATH_FORMAT_DEFAULT entirely from enum path_format in path.h.
>
> Instead, I'll re-introduce a small local enum (e.g., enum
> rev_parse_format) inside rev-parse.c to handle the
> command-line parsing state (tracking whether the user
> explicitly provided a flag or if we are still in a
> neutral/default state).
I think it is probably simplest to keep the existing enums and modify
print_path() to convert them to the appropriate PATH_FORMAT_*. That way
we can keep the option parsing code as is.
Thanks
Phillip
> As you said, most of the logic is already present. In
> print_path(), we will check that local tracking enum. If it’s
> set to the local default, we can map it directly to the
> path-specific def_format before invoking append_formatted_path().
> This ensures other users of the function don't have to worry
> about the extra flag.
>
> I will send out the v6 series with these fixes shortly.
>
> Regards,
> - K Jayatheerth
^ permalink raw reply
* [PATCH] sequencer: Skip copying notes for commits that disappear during rebase
From: Uwe Kleine-König @ 2026-06-16 17:40 UTC (permalink / raw)
To: git; +Cc: Phillip Wood
When a commit disappears during rebase because the patch content is
already there (but not by the same patch in which case the commit would
be skipped) the notes of that disappearing commit should not be copied
to the unrelated commit that happens to be HEAD.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,
after also my 2nd bug report[1] didn't motivate anyone to come up with a
fix, I invested the time to work out one according to Phillip Wood's
suggestion.
IMHO it's not pretty, but it works for me.
Note that Phillip also suggested to integrete the test into
t3400-rebase.sh . IMHO it doesn't matter much if this is considered a
rebase test or a notes test. I kept it where I have it because I'm lazy
and failed to understand the git history created in that test.
Best regards
Uwe
[1] https://lore.kernel.org/git/20260612143952.3281115-2-u.kleine-koenig@baylibre.com
sequencer.c | 20 ++++++++++----------
t/meson.build | 1 +
t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 10 deletions(-)
create mode 100755 t/t3322-notes-rebase.sh
diff --git a/sequencer.c b/sequencer.c
index 57855b0066ac..da2185a37c5d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2263,7 +2263,7 @@ static const char *reflog_message(struct replay_opts *opts,
static int do_pick_commit(struct repository *r,
struct todo_item *item,
struct replay_opts *opts,
- int final_fixup, int *check_todo)
+ int final_fixup, int *check_todo, int *dropped_commit)
{
struct replay_ctx *ctx = opts->ctx;
unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2273,7 +2273,7 @@ static int do_pick_commit(struct repository *r,
const char *base_label, *next_label, *reflog_action;
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
- int res, unborn = 0, reword = 0, allow, drop_commit;
+ int res, unborn = 0, reword = 0, allow;
enum todo_command command = item->command;
struct commit *commit = item->commit;
@@ -2492,7 +2492,7 @@ static int do_pick_commit(struct repository *r,
goto leave;
}
- drop_commit = 0;
+ *dropped_commit = 0;
allow = allow_empty(r, opts, commit);
if (allow < 0) {
res = allow;
@@ -2500,7 +2500,7 @@ static int do_pick_commit(struct repository *r,
} else if (allow == 1) {
flags |= ALLOW_EMPTY;
} else if (allow == 2) {
- drop_commit = 1;
+ *dropped_commit = 1;
refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
NULL, REF_NO_DEREF);
unlink(git_path_merge_msg(r));
@@ -2510,7 +2510,7 @@ static int do_pick_commit(struct repository *r,
_("dropping %s %s -- patch contents already upstream\n"),
oid_to_hex(&commit->object.oid), msg.subject);
} /* else allow == 0 and there's nothing special to do */
- if (!opts->no_commit && !drop_commit) {
+ if (!opts->no_commit && !*dropped_commit) {
if (author || command == TODO_REVERT || (flags & AMEND_MSG))
res = do_commit(r, msg_file, author, reflog_action,
opts, flags,
@@ -4943,12 +4943,12 @@ static int pick_one_commit(struct repository *r,
struct replay_opts *opts,
int *check_todo, int* reschedule)
{
- int res;
+ int res, dropped_commit;
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
- check_todo);
+ check_todo, &dropped_commit);
if (is_rebase_i(opts) && res < 0) {
/* Reschedule */
*reschedule = 1;
@@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
}
- if (is_rebase_i(opts) && !res)
+ if (is_rebase_i(opts) && !res && !dropped_commit)
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
if (res && is_fixup(item->command)) {
@@ -5523,14 +5523,14 @@ static int single_pick(struct repository *r,
struct commit *cmit,
struct replay_opts *opts)
{
- int check_todo;
+ int check_todo, dummy;
struct todo_item item;
item.command = opts->action == REPLAY_PICK ?
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- return do_pick_commit(r, &item, opts, 0, &check_todo);
+ return do_pick_commit(r, &item, opts, 0, &check_todo, &dummy);
}
int sequencer_pick_revisions(struct repository *r,
diff --git a/t/meson.build b/t/meson.build
index c5832fee0535..6927bd9c794f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -358,6 +358,7 @@ integration_tests = [
't3311-notes-merge-fanout.sh',
't3320-notes-merge-worktrees.sh',
't3321-notes-stripspace.sh',
+ 't3322-notes-rebase.sh',
't3400-rebase.sh',
't3401-rebase-and-am-rename.sh',
't3402-rebase-merge.sh',
diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh
new file mode 100755
index 000000000000..0eddde7f9961
--- /dev/null
+++ b/t/t3322-notes-rebase.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Test notes on rebase'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ git init &&
+ git config notes.rewriteRef refs/notes/commits &&
+ git version > version &&
+ echo A > A &&
+ git add A &&
+ git commit -m A &&
+ git branch branch &&
+ echo B > B &&
+ git add B &&
+ git commit -m B &&
+ git notes add -m "This is B" @ &&
+ echo C > C &&
+ git add C &&
+ git commit -m C &&
+ git checkout branch &&
+ echo B > B &&
+ echo D > D &&
+ git add B D &&
+ git commit -m BD
+'
+
+test_expect_success 'rebase B + C on top of BD' '
+ git rebase @ master
+'
+
+test_expect_success 'assert there is no note on BD' '
+ if git notes list branch >/tmp/lalaa; then return 1; fi
+'
+
+test_done
base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
--
2.47.3
^ permalink raw reply related
* Re: [PATCH] rebase: mention --abort alongside --continue
From: Junio C Hamano @ 2026-06-16 17:33 UTC (permalink / raw)
To: Phillip Wood; +Cc: Harald Nordgren via GitGitGadget, git, Harald Nordgren
In-Reply-To: <89d72342-5aa1-4dcf-951b-d0c791f91738@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Harald
>
> On 15/06/2026 20:19, Harald Nordgren via GitGitGadget wrote:
>> From: Harald Nordgren <haraldnordgren@gmail.com>
>>
>> The warning shown when an "exec" step fails and the "git status"
>> advice while splitting or editing a commit pointed users at "git
>> rebase --continue" but not "--abort". Mention it in both, matching
>> the conflict case.
>
> I'm not sure that the "failed exec" and "conflicts" cases are equivalent
> though. If you have some nasty conflict that you don't want to resolve
> then aborting and trying another approach such is incrementally rebasing
> is the only option. If an exec command fails then it likely means that a
> test has failed or some something similar which is minor inconvenience
> which needs fixing before continuing - it seems very unlikely that the
> user would want to abort the rebase.
It is very true that users who know what they are doing and got into
such conflicts are opted to go into such a situation tnat it is
unlikely that they would appreciate a choice to abort.
But given that for any system, everybody starts as a newbie, it may
be assuring to always give "here is a way out" option when they get
in a nasty confusing situation. Discouraging the way to use the
tool that can lead to confusing situation by guiding them with BCP
workflows would help, but they always get into pitfall.
The patch adds new message into the existing message to suggest how
to move forward, but as a training wheel option, it may not be a bad
thing to offer "--abort" as an extra hint, separate from the
existing warning() message.
^ permalink raw reply
* Re: [GSoC Patch v5 2/4] rev-parse: use append_formatted_path() for path formatting
From: K Jayatheerth @ 2026-06-16 17:04 UTC (permalink / raw)
To: Phillip Wood
Cc: git, jltobler, lucasseikioshiro, gitster, phillip.wood, sandals,
kumarayushjha123, a3205153416
In-Reply-To: <0077b1ae-3c85-4b34-a0ac-766395157c4f@gmail.com>
Hi Phillip,
Thanks for taking a look!
> On 16/06/2026 05:49, K Jayatheerth wrote:
> > The path-formatting logic in builtin/rev-parse.c is tightly coupled
> > to that command and writes directly to stdout, making it impossible
> > for other builtins to reuse.
> >
> > Extract the core algorithm into append_formatted_path() in path.c
> > and expose a path_format enum in path.h so that any builtin can
> > format paths consistently without duplicating logic.
>
> Sorry I haven't had time to look at this series recently, it is looking
> much nicer now that we have a single enum. It would be helpful to
> explain why we need PATH_FORMAT_DEFAULT that acts exactly like
> PATH_FORMAT_UNMODIFIED. Looking at the next patch it seems this is still
> a wart in the api due to rev-parse wanting needing to distinguish the
> unmodified case from the default case.
t);
> > +
> > # ifdef USE_THE_REPOSITORY_VARIABLE
> > # include "strbuf.h"
> > # include "repository.h"
>
> > int cmd_rev_parse(int argc,
> > @@ -717,7 +661,7 @@ int cmd_rev_parse(int argc,
> > const char *name = NULL;
> > struct strbuf buf = STRBUF_INIT;
> > int seen_end_of_options = 0;
> > - enum format_type format = FORMAT_DEFAULT;
> > + enum path_format arg_path_format = PATH_FORMAT_DEFAULT;
>
> This is the source of the api wart I referred to in the previous patch.
> Could we keep the existing enums and convert them into the appropriate
> PATH_FORMAT_* flag in print_path() above? I think we already have the
> logic to do that in the existing code. That would mean that other users
> of append_formatted_path() don't have to worry about the extra flag.
>
That is a much more elegant solution than the current one.
For v6, I will clean this up by keeping the fallback logic
localized within builtin/rev-parse.c and removing
PATH_FORMAT_DEFAULT entirely from enum path_format in path.h.
Instead, I'll re-introduce a small local enum (e.g., enum
rev_parse_format) inside rev-parse.c to handle the
command-line parsing state (tracking whether the user
explicitly provided a flag or if we are still in a
neutral/default state).
As you said, most of the logic is already present. In
print_path(), we will check that local tracking enum. If it’s
set to the local default, we can map it directly to the
path-specific def_format before invoking append_formatted_path().
This ensures other users of the function don't have to worry
about the extra flag.
I will send out the v6 series with these fixes shortly.
Regards,
- K Jayatheerth
^ permalink raw reply
* Re: [PATCH v5 2/2] graph: indent visual root in graph
From: Junio C Hamano @ 2026-06-16 16:36 UTC (permalink / raw)
To: Pablo Sabater
Cc: Taylor Blau, git, ayu.chandekar, chandrapratap3519,
christian.couder, jltobler, karthik.188, peff, phillip.wood,
siddharthasthana31
In-Reply-To: <CAN5EUNR-o_sLzeWuy7M9UMFHBKxSuytNd=4p2svtFuv40E8vZg@mail.gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> Back to the test:
>
> * 41_octopus
> | * 43_B
> | \
> | * 43_A
> | * 42_B
> | * 42_A
> * 41_B
> * 41_A
>
> 43_A is rendered on the second column (first column is active by the
> 41_* branch) and gets indented to the third one. With commit-graph it
> would be on the first and get indented to the second, making it the
> same as more general tests above in "t4218", it is an edge case but
> shows that indentation works correctly independently where the visual
> root is.
Sounds good.
^ permalink raw reply
* Re: [PATCH v2 0/6] Support hashing objects larger than 4GB on Windows
From: Junio C Hamano @ 2026-06-16 16:09 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <pull.2138.v2.git.1781621398.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Changes since v1:
>
> * Rebased to current master to resolve the conflicts with
> ps/odb-source-loose
Very much appreciated.
> * Dropped the !LONG_IS_64BIT prereq from the added/touched tests, as it is
> now no longer needed
Good thing to do and see that the code works as well as it could,
whether a long is 32-bit or 64-bit ;-).
> Philip Oakley (6):
> hash-object: demonstrate a >4GB/LLP64 problem
> object-file.c: use size_t for header lengths
> hash algorithms: use size_t for section lengths
> hash-object --stdin: verify that it works with >4GB/LLP64
> hash-object: add another >4GB/LLP64 test case
> hash-object: add a >4GB/LLP64 test case using filtered input
>
> object-file.c | 14 +++++++-------
> object-file.h | 6 +++---
> odb/source-files.c | 2 +-
> odb/source-inmemory.c | 2 +-
> odb/source-loose.c | 4 ++--
> odb/source.h | 2 +-
> sha1dc_git.c | 3 +--
> sha1dc_git.h | 2 +-
> t/t1007-hash-object.sh | 39 +++++++++++++++++++++++++++++++++++++++
> 9 files changed, 56 insertions(+), 18 deletions(-)
Will queue. Thanks.
>
>
> base-commit: 700432b2ba22603a0bcb71475c9c333d17c9b0d1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2138%2Fdscho%2FPhilipOakley%2Fhashliteral_t-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2138/dscho/PhilipOakley/hashliteral_t-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/2138
>
> Range-diff vs v1:
>
> 1: 84e1cd0aa0 = 1: 9c01bac407 hash-object: demonstrate a >4GB/LLP64 problem
> 2: 809d83e46f ! 2: aa5859c14f object-file.c: use size_t for header lengths
> @@ Commit message
By the way, how is range-diff driven via GGG? After applying these
patches on the same base commit, my "git range-diff v1...v2" invocation
punts on matching step 2 and I do not get a comparison like this
unless I give --creation-factor=<large number> from the command line.
^ 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