* [PATCH GSoC v17 06/13] fetch-pack: move write_fetch_command_and_capabilities() to connect.c
From: Pablo Sabater @ 2026-07-14 11:45 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
write_fetch_command_and_capabilities() is refactored in a subsequent
commit where it becomes a more general-purpose function, making it
more accessible to additional commands in the future.
Move write_fetch_command_and_capabilities() to 'connect.c', where
there are similar purpose functions.
Because string_list is only used as a pointer, use a forward
declaration [1].
[1]: https://lore.kernel.org/git/Z0RIqUAoEob8lGfM@pks.im/
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
connect.c | 34 ++++++++++++++++++++++++++++++++++
connect.h | 4 ++++
fetch-pack.c | 34 ----------------------------------
3 files changed, 38 insertions(+), 34 deletions(-)
diff --git a/connect.c b/connect.c
index 47e39d2a73..c09947cc56 100644
--- a/connect.c
+++ b/connect.c
@@ -700,6 +700,40 @@ int server_supports(const char *feature)
return !!server_feature_value(feature, NULL);
}
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options)
+{
+ const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
+
+ ensure_server_supports_v2("fetch");
+ packet_buf_write(req_buf, "command=fetch");
+ if (server_supports_v2("agent"))
+ packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
+ if (advertise_sid && server_supports_v2("session-id"))
+ packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
+ if (server_options && server_options->nr) {
+ ensure_server_supports_v2("server-option");
+ for (size_t i = 0; i < server_options->nr; i++)
+ packet_buf_write(req_buf, "server-option=%s",
+ server_options->items[i].string);
+ }
+
+ if (server_feature_v2("object-format", &hash_name)) {
+ const unsigned int hash_algo = hash_algo_by_name(hash_name);
+ if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
+ die(_("mismatched algorithms: client %s; server %s"),
+ the_hash_algo->name, hash_name);
+ packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
+ } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
+ die(_("the server does not support algorithm '%s'"),
+ the_hash_algo->name);
+ }
+ packet_buf_delim(req_buf);
+}
+
static const char *url_scheme_name(enum url_scheme scheme)
{
switch (scheme) {
diff --git a/connect.h b/connect.h
index aa482a37fb..c4f6ea4b0a 100644
--- a/connect.h
+++ b/connect.h
@@ -34,4 +34,8 @@ void check_stateless_delimiter(int stateless_rpc,
struct packet_reader *reader,
const char *error);
+struct string_list;
+void write_fetch_command_and_capabilities(struct strbuf *req_buf,
+ const struct string_list *server_options);
+
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index 8e04db8640..5e7c4f1d46 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1375,40 +1375,6 @@ static int add_haves(struct fetch_negotiator *negotiator,
return haves_added;
}
-static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
- const struct string_list *server_options)
-{
- const char *hash_name;
- int advertise_sid = 0;
-
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
-
- ensure_server_supports_v2("fetch");
- packet_buf_write(req_buf, "command=fetch");
- if (server_supports_v2("agent"))
- packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
- if (advertise_sid && server_supports_v2("session-id"))
- packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
- if (server_options && server_options->nr) {
- ensure_server_supports_v2("server-option");
- for (size_t i = 0; i < server_options->nr; i++)
- packet_buf_write(req_buf, "server-option=%s",
- server_options->items[i].string);
- }
-
- if (server_feature_v2("object-format", &hash_name)) {
- const unsigned int hash_algo = hash_algo_by_name(hash_name);
- if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
- die(_("mismatched algorithms: client %s; server %s"),
- the_hash_algo->name, hash_name);
- packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name);
- } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1_LEGACY) {
- die(_("the server does not support algorithm '%s'"),
- the_hash_algo->name);
- }
- packet_buf_delim(req_buf);
-}
-
static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
struct fetch_pack_args *args,
const struct ref *wants, struct oidset *common,
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 05/13] fetch-pack: drop static advertise_sid variable
From: Pablo Sabater @ 2026-07-14 11:45 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
write_fetch_command_and_capabilities() is moved to 'connect.c' in a
subsequent commit. To prepare for that, drop the static variable usage
of advertise_sid. Currently advertise_sid is used in two places:
1. In function do_fetch_pack():
if (!server_supports("session-id"))
advertise_sid = 0;
2. In function fetch_pack_config():
repo_config_get_bool("transfer.advertisesid", &advertise_sid);
About 1, it is only relevant for v0/v1 protocol, move it into
find_common().
About 2, call repo_config_get_bool() inside of
write_fetch_command_and_capabilities() and find_common() replacing the
static variable.
Because repo_config_get_bool() leaves advertise_sid as is if it is not
set, initialize it to 0 matching its default.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index eea72b2500..8e04db8640 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -49,7 +49,6 @@ static int fetch_fsck_objects = -1;
static int transfer_fsck_objects = -1;
static int agent_supported;
static int server_supports_filtering;
-static int advertise_sid;
static struct shallow_lock shallow_lock;
static const char *alternate_shallow_file;
static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -363,6 +362,9 @@ static int find_common(struct fetch_negotiator *negotiator,
size_t state_len = 0;
struct packet_reader reader;
struct oidset negotiation_include_oids = OIDSET_INIT;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (args->stateless_rpc && multi_ack == 1)
die(_("the option '%s' requires '%s'"), "--stateless-rpc", "multi_ack_detailed");
@@ -414,7 +416,7 @@ static int find_common(struct fetch_negotiator *negotiator,
if (deepen_not_ok) strbuf_addstr(&c, " deepen-not");
if (agent_supported) strbuf_addf(&c, " agent=%s",
git_user_agent_sanitized());
- if (advertise_sid)
+ if (advertise_sid && server_supports("session-id"))
strbuf_addf(&c, " session-id=%s", trace2_session_id());
if (args->filter_options.choice)
strbuf_addstr(&c, " filter");
@@ -1160,9 +1162,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
(int)agent_len, agent_feature);
}
- if (!server_supports("session-id"))
- advertise_sid = 0;
-
if (server_supports("shallow"))
print_verbose(args, _("Server supports %s"), "shallow");
else if (args->depth > 0 || is_repository_shallow(r))
@@ -1380,6 +1379,9 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
const struct string_list *server_options)
{
const char *hash_name;
+ int advertise_sid = 0;
+
+ repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
ensure_server_supports_v2("fetch");
packet_buf_write(req_buf, "command=fetch");
@@ -1998,7 +2000,6 @@ static void fetch_pack_config(void)
repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);
repo_config_get_bool(the_repository, "fetch.fsckobjects", &fetch_fsck_objects);
repo_config_get_bool(the_repository, "transfer.fsckobjects", &transfer_fsck_objects);
- repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);
if (!uri_protocols.nr) {
char *str;
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 04/13] fetch-pack: fix hash_algo variable type
From: Pablo Sabater @ 2026-07-14 11:45 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
hash_algo_by_name() returns "unsigned int", but the variable that it is
assigned to is "int".
Change hash_algo variable type to match hash_algo_by_name() type, also
make it const because it is never modified.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index f13951d154..eea72b2500 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1395,7 +1395,7 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
}
if (server_feature_v2("object-format", &hash_name)) {
- int hash_algo = hash_algo_by_name(hash_name);
+ const unsigned int hash_algo = hash_algo_by_name(hash_name);
if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
die(_("mismatched algorithms: client %s; server %s"),
the_hash_algo->name, hash_name);
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 03/13] t1006: split test utility functions into new 'lib-cat-file.sh'
From: Pablo Sabater @ 2026-07-14 11:44 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
This refactor extracts utility functions from the cat-file's test
script 't1006-cat-file.sh' into a new 'lib-cat-file.sh' dedicated
library file.
A subsequent commit will need this functions, the goal is to improve
code reuse and readability,enabling future tests to leverage these
utilities without duplicating code.
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
t/lib-cat-file.sh | 16 ++++++++++++++++
t/t1006-cat-file.sh | 13 +------------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/t/lib-cat-file.sh b/t/lib-cat-file.sh
new file mode 100644
index 0000000000..44af232d74
--- /dev/null
+++ b/t/lib-cat-file.sh
@@ -0,0 +1,16 @@
+# Library of git-cat-file related test functions.
+
+# Print a string without a trailing newline.
+echo_without_newline () {
+ printf '%s' "$*"
+}
+
+# Print a string without newlines and replace them with a NULL character (\0).
+echo_without_newline_nul () {
+ echo_without_newline "$@" | tr '\n' '\0'
+}
+
+# Calculate the length of a string.
+strlen () {
+ echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
+}
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 8e2c52652c..8360f3bbd9 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -4,6 +4,7 @@ test_description='git cat-file'
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-loose.sh"
+. "$TEST_DIRECTORY"/lib-cat-file.sh
test_cmdmode_usage () {
test_expect_code 129 "$@" 2>err &&
@@ -99,18 +100,6 @@ do
'
done
-echo_without_newline () {
- printf '%s' "$*"
-}
-
-echo_without_newline_nul () {
- echo_without_newline "$@" | tr '\n' '\0'
-}
-
-strlen () {
- echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
-}
-
run_tests () {
type=$1
object_name="$2"
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 02/13] cat-file: declare loop counter inside for()
From: Pablo Sabater @ 2026-07-14 11:44 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
From: Eric Ju <eric.peijian@gmail.com>
Some code used in this series declares variable i and only uses it
in a for loop, not in any other logic outside the loop.
Change the declaration of i to be inside the for loop for readability.
While at it, we also change its type from int to size_t where the
latter makes more sense.
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Eric Ju <eric.peijian@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
builtin/cat-file.c | 13 ++++---------
fetch-pack.c | 3 +--
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 60869b8b37..26ad07b62c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -721,14 +721,12 @@ static void dispatch_calls(struct batch_options *opt,
struct strbuf *output,
struct expand_data *data,
struct queued_cmd *cmd,
- int nr)
+ size_t nr)
{
- int i;
-
if (!opt->buffer_output)
die(_("flush is only for --buffer mode"));
- for (i = 0; i < nr; i++)
+ for (size_t i = 0; i < nr; i++)
cmd[i].fn(opt, cmd[i].line, output, data);
fflush(stdout);
@@ -736,9 +734,7 @@ static void dispatch_calls(struct batch_options *opt,
static void free_cmds(struct queued_cmd *cmd, size_t *nr)
{
- size_t i;
-
- for (i = 0; i < *nr; i++)
+ for (size_t i = 0; i < *nr; i++)
FREE_AND_NULL(cmd[i].line);
*nr = 0;
@@ -765,7 +761,6 @@ static void batch_objects_command(struct batch_options *opt,
size_t alloc = 0, nr = 0;
while (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {
- int i;
const struct parse_cmd *cmd = NULL;
const char *p = NULL, *cmd_end;
struct queued_cmd call = {0};
@@ -775,7 +770,7 @@ static void batch_objects_command(struct batch_options *opt,
if (isspace(*input.buf))
die(_("whitespace before command: '%s'"), input.buf);
- for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ for (size_t i = 0; i < ARRAY_SIZE(commands); i++) {
if (!skip_prefix(input.buf, commands[i].name, &cmd_end))
continue;
diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..f13951d154 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1388,9 +1388,8 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
if (advertise_sid && server_supports_v2("session-id"))
packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
if (server_options && server_options->nr) {
- int i;
ensure_server_supports_v2("server-option");
- for (i = 0; i < server_options->nr; i++)
+ for (size_t i = 0; i < server_options->nr; i++)
packet_buf_write(req_buf, "server-option=%s",
server_options->items[i].string);
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 01/13] transport-helper: fix memory leak of helper on disconnect
From: Pablo Sabater @ 2026-07-14 11:44 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260714-ps-eric-work-rebase-v17-0-afabfc83260e@gmail.com>
disconnect_helper() only frees data inside of the if(data->helper) block
[1]. When the transport is disconnected without the helper being fully
started, data->name allocated in transport_helper_init()
is never freed.
Move FREE_AND_NULL(data->name) outside the conditional block so it's
always freed on disconnect.
[1]: https://lore.kernel.org/git/05fbadbae2184479c87c37675dde7bd79b3e32ab.1716465556.git.ps@pks.im/
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
transport-helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index 80f90eb7ba..f195070788 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -266,9 +266,9 @@ static int disconnect_helper(struct transport *transport)
close(data->helper->out);
fclose(data->out);
res = finish_command(data->helper);
- FREE_AND_NULL(data->name);
FREE_AND_NULL(data->helper);
}
+ FREE_AND_NULL(data->name);
return res;
}
--
2.54.0
^ permalink raw reply related
* [PATCH GSoC v17 00/13] cat-file: add remote-object-info to batch-command
From: Pablo Sabater @ 2026-07-14 11:44 UTC (permalink / raw)
To: pabloosabaterr
Cc: chandrapratap3519, chriscool, eric.peijian, git, gitster,
jltobler, karthik.188, peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
This patch series is a continuation of Eric Ju's
(eric.peijian@gmail.com) and Calvin Wan's (calvinwan@google.com) patch
series [1] and [2] respectively.
Sometimes it is beneficial to retrieve information about an object
without having to download it completely. The server logic for
retrieving size has already been implemented and merged in a2ba162cda
(object-info: support for retrieving object info, 2021-04-20) [3].
This patch series implement the client option for it.
Eric's series adds the remote-object-info command to cat-file
--batch-command. This command allows the client to make an object-info
command request to a server that supports protocol v2.
If the server uses protocol v2 but does not support the object-info
capability, cat-file --batch-command will die.
If a user attempts to use remote-object-info with protocol v1, cat-file
--batch-command will die.
Currently, only the size (%(objectsize)) is supported end to end in this
implementation. The type (%(objecttype)) is known by the client's
allow-list and request path but is not supported on the server side
nor the response parsing. A follow up series will add full end-to-end
support for %(objecttype).
The default format for remote-object-info is set to "%(objectname)
%(objectsize)". Once %(objecttype) is supported, the default format will
be unified accordingly.
If the batch command format includes unsupported fields such as
%(objecttype), %(objectsize:disk), or %(deltabase), the command will
return empty strings for each unsupported field.
This series completes Eric's work mainly with the refactor of the
validation of the placeholder with an allow-list that filters what the
client asks with what the server is capable of provide following Jeff
King's idea [4].
GitHub CI: https://github.com/pabloosabaterr/git/actions/runs/28435046129
[1]: https://lore.kernel.org/git/20250221190451.12536-1-eric.peijian@gmail.com/
[2]: https://lore.kernel.org/git/20220728230210.2952731-1-calvinwan@google.com/#t
[3]: https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2ba162cda2acc171c3e36acbbc854792b093cb7
[4]: https://lore.kernel.org/git/20250313060250.GH94015@coredump.intra.peff.net/
Changes since v16:
- Droped a wrongly introduced include at transport.c
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Calvin Wan (3):
fetch-pack: move fetch initialization
serve: advertise object-info feature
transport: add client support for object-info
Eric Ju (3):
cat-file: declare loop counter inside for()
t1006: split test utility functions into new 'lib-cat-file.sh'
cat-file: add remote-object-info to batch-command
Pablo Sabater (7):
transport-helper: fix memory leak of helper on disconnect
fetch-pack: fix hash_algo variable type
fetch-pack: drop static advertise_sid variable
fetch-pack: move write_fetch_command_and_capabilities() to connect.c
connect: make write_fetch_command_and_capabilities() more generic
cat-file: validate remote atoms with an allow-list
cat-file: make remote-object-info allow-list dynamic
Documentation/git-cat-file.adoc | 29 +-
Documentation/gitprotocol-v2.adoc | 11 +-
Makefile | 1 +
builtin/cat-file.c | 221 ++++++++++-
connect.c | 34 ++
connect.h | 8 +
fetch-object-info.c | 129 ++++++
fetch-object-info.h | 22 ++
fetch-pack.c | 58 +--
fetch-pack.h | 1 +
meson.build | 1 +
object-file.c | 10 +
odb.h | 3 +
serve.c | 5 +-
t/lib-cat-file.sh | 16 +
t/meson.build | 1 +
t/t1006-cat-file.sh | 13 +-
t/t1017-cat-file-remote-object-info.sh | 699 +++++++++++++++++++++++++++++++++
transport-helper.c | 15 +-
transport-internal.h | 8 +
transport.c | 45 +++
transport.h | 10 +
22 files changed, 1254 insertions(+), 86 deletions(-)
---
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
^ permalink raw reply
* Re: [PATCH GSoC v16 10/13] transport: add client support for object-info
From: Pablo Sabater @ 2026-07-14 11:28 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: chandrapratap3519, chriscool, eric.peijian, gitster, jltobler,
karthik.188, peff, toon, Calvin Wan
In-Reply-To: <20260710-ps-eric-work-rebase-v16-10-66e07b58a8fe@gmail.com>
On Fri Jul 10, 2026 at 6:41 PM CEST, Pablo Sabater wrote:
[snip]
> diff --git a/transport.c b/transport.c
> index fc144f0aed..3e0a6558b7 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1,3 +1,4 @@
> +#include "compat/posix.h"
> #define USE_THE_REPOSITORY_VARIABLE
Hi while working on another series I realized that I had introduced this
include which is wrong. I didn't notice because strangely it didn't gave
any problems on the CI.
I'll drop it next reroll.
Regards,
Pablo.
^ permalink raw reply
* Re: [PATCH v11 7/7] graph: add --[no-]graph-indent and log.graphIndent
From: Chandra Pratap @ 2026-07-14 10:19 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, ayu.chandekar, christian.couder, gitster, jltobler,
karthik.188, krka, mroik, peff, phillip.wood, siddharthasthana31
In-Reply-To: <20260713-ps-pre-commit-indent-v11-7-dcb65bc4ba99@gmail.com>
On Mon, 13 Jul 2026 at 22:14, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>
> Some users may prefer to not have graph indentation.
>
> Add "log.graphIndent" config variable to graph_read_config() to read the
> default preference. By default is graph indentation is true.
>
> Add --graph-indent and --no-graph-indent options to overwrite the
> default preference.
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> Documentation/config/log.adoc | 4 +++
> Documentation/rev-list-options.adoc | 8 ++++++
> graph.c | 10 +++++--
> revision.c | 9 +++++++
> revision.h | 2 ++
> t/t4218-log-graph-indentation.sh | 52 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/log.adoc b/Documentation/config/log.adoc
> index 757a7be196..f7dfce69b5 100644
> --- a/Documentation/config/log.adoc
> +++ b/Documentation/config/log.adoc
> @@ -59,6 +59,10 @@ This is the same as the `--decorate` option of the `git log`.
> A list of colors, separated by commas, that can be used to draw
> history lines in `git log --graph`.
>
> +`log.graphIndent`::
> + If `true`, indent visual roots when rendering the graphs with `--graph`.
> + Set true by default. It can be overriden with `--[no-]graph-indent`.
> +
> `log.showRoot`::
> If true, the initial commit will be shown as a big creation event.
> This is equivalent to a diff against an empty tree.
> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index eaee6ee839..af74f10bb4 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -1269,6 +1269,14 @@ This implies the `--topo-order` option by default, but the
> By default it is set to 0 (no limit), zero and negative values
> are ignored and treated as no limit.
>
> +`--no-graph-indent`::
> +`--graph-indent`::
> + When used with `--graph`, indent visual roots (commits with no parents
> + or whose parents are not shown) to differentiate them from commits that
> + are vertically adjacent but unrelated. Enabled by default. Use
> + `--no-graph-indent` to disable or set `graph.indent` to set a deafault
s/deafault/default
Also, I think you meant log.graphIndent instead of graph.indent here.
[snip]
> +test_expect_success '--no-graph-indent disables indentation' '
> + lib_test_check_graph --no-graph-indent _58 _59 _60 _61 _62 _63 _64 _65 _66 _67 <<-\EOF
> + * 67_A
> + * 66_A
> + * 65_A
> + * 64_A
> + * 63_A
> + * 62_A
> + * 61_A
> + * 60_A
> + * 59_A
> + * 58_B
> + * 58_A
> + EOF
> +'
> +
> +test_expect_success 'log.graphIndent config disables indentation' '
> + test_config log.graphIndent false &&
> + lib_test_check_graph _58 _59 _60 _61 _62 _63 _64 _65 _66 _67 <<-\EOF
> + * 67_A
> + * 66_A
> + * 65_A
> + * 64_A
> + * 63_A
> + * 62_A
> + * 61_A
> + * 60_A
> + * 59_A
> + * 58_B
> + * 58_A
> + EOF
> +'
> +
> +test_expect_success '--graph-indent forces indentation when graph.indent is unset' '
> + test_config log.graphIndent false &&
> + lib_test_check_graph --graph-indent _58 _59 _60 _61 _62 _63 _64 _65 _66 _67 <<-\EOF
> + * 67_A
> + * 66_A
> + * 65_A
> + * 64_A
> + * 63_A
> + * 62_A
> + * 61_A
> + * 60_A
> + * 59_A
> + * 58_B
> + * 58_A
> + EOF
> +'
> +
> +# graph.indent true and no --option is the default state.
Same thing here.
^ permalink raw reply
* Re: [PATCH v8 0/5] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-07-14 9:04 UTC (permalink / raw)
To: Matt Hunter
Cc: Harald Nordgren via GitGitGadget, git, Phillip Wood,
D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <CAHwyqnVVYMqYTD=Hri1gYW6CvkjKgTMv8AGP59bkOOd+-huwbg@mail.gmail.com>
I made a fix for this and also took the opportunity to create test
helpers to clarify the tests.
Same pattern as the ones @Phillip Wood helped me with on my
'delete-merged' topic. I would like to push out a new version before
anyone needs to review the current tests -- the new version would be a
lot easier to look at. Should I?
Harald
^ permalink raw reply
* Re: [PATCH] fast-export: standardize usage string and SYNOPSIS
From: Patrick Steinhardt @ 2026-07-14 8:54 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Elijah Newren, Jeff King, brian m . carlson,
Johannes Schindelin, Justin Tobler, Christian Couder
In-Reply-To: <20260713124153.245268-1-christian.couder@gmail.com>
On Mon, Jul 13, 2026 at 02:41:53PM +0200, Christian Couder wrote:
> diff --git a/Documentation/git-fast-export.adoc b/Documentation/git-fast-export.adoc
> index 297b57bb2e..719aeca244 100644
> --- a/Documentation/git-fast-export.adoc
> +++ b/Documentation/git-fast-export.adoc
> @@ -9,7 +9,7 @@ git-fast-export - Git data exporter
> SYNOPSIS
> --------
> [verse]
> -'git fast-export' [<options>] | 'git fast-import'
> +'git fast-export' [<options>] [<revision-range>] [[--] <path>...]
>
> DESCRIPTION
> -----------
Makes sense, as it is more consistent with all the other commands that
we have. I don't recall any other commands that use "|".
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 0be43104dc..629d7c591a 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -33,7 +33,7 @@
> #include "gpg-interface.h"
>
> static const char *const fast_export_usage[] = {
> - N_("git fast-export [<rev-list-opts>]"),
> + N_("git fast-export [<options>] [<revision-range>] [[--] <path>...]"),
> NULL
> };
>
This is being adapted to match.
> diff --git a/t/t0450/adoc-help-mismatches b/t/t0450/adoc-help-mismatches
> index e8d6c13ccd..c4a55ff4e3 100644
> --- a/t/t0450/adoc-help-mismatches
> +++ b/t/t0450/adoc-help-mismatches
> @@ -12,7 +12,6 @@ column
> credential
> credential-cache
> credential-store
> -fast-export
> fast-import
> fetch-pack
> fmt-merge-msg
And as both match now we can also update t0450. Nice!
Patrick
^ permalink raw reply
* [PATCH] strbuf: avoid redundant reset in strbuf_getwholeline()
From: René Scharfe @ 2026-07-14 8:45 UTC (permalink / raw)
To: Git List
The HAVE_GETDELIM variant of strbuf_getwholeline() calls strbuf_reset()
on the strbuf before handing it over to getdelim(3). This is
unnecessary:
- getdelim(3) doesn't care whether the old buffer contents is
NUL-terminated and has no access to ->len,
- on success getdelim(3) NUL-terminates the buffer and we set ->len,
- on error we either call strbuf_init() or strbuf_reset().
Remove the superfluous preparatory call.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
strbuf.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index 764b629927..44955669e8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -646,8 +646,6 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
if (feof(fp))
return EOF;
- strbuf_reset(sb);
-
/* Translate slopbuf to NULL, as we cannot call realloc on it */
if (!sb->alloc)
sb->buf = NULL;
--
2.55.0
^ permalink raw reply related
* Re: [PATCH v8 0/5] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-07-14 8:38 UTC (permalink / raw)
To: Matt Hunter
Cc: Harald Nordgren via GitGitGadget, git, Phillip Wood,
D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <DJY0QSJYNG0J.210HZQH198Y1N@lfurio.us>
> This feature looks like it's coming together pretty well imo. I just have
> one observation I want to comment on:
>
> I noticed that 'git history squash <range>', when --reedit-message is
> omitted, will ignore any amend! message in the range that targets the
> first folded commit.
>
> On the surface, this makes sense. The feature is pretty explicit that
> it will faithfully stick with the first commit's message, unless
> modified by use of --reedit-message.
>
> However, this edge case is a little surprising, given that
> 'git history squash' seems to be aware of the semantics of fixup!, amend!,
> and squash! messages whether --reedit-message was given or not. For instance,
> the default command notices when the range contains a squash! commit whose
> target is elsewhere (a useful feature). It seems consistent then, that the
> default command would incorporate an amend! it is aware of when placing the
> "first commit's" message in the resulting squash. This seems useful to me
> as well.
>
> At the same time, I can understand why the current implementation does
> what it does. So I'm not entirely sure what the correct answer is here.
>
> I'll mention as well that I really like the decisions made for how this
> command handles squashing a bunch of related fixups. This "fixup
> consolidation" is a use-case that this command may steal away from rebase
> for me. And the way a final amend! is handled in this case is what got me
> thinking about it in the general case.
>
> Thanks for the work on this topic!
Thanks!
That's an interesting observation, I'll see what I can do about it.
Harald
^ permalink raw reply
* bug in `git log --cherry-mark`
From: Uwe Kleine-König @ 2026-07-14 7:56 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]
Hello,
in a linux tree I have a bunch of commits that I sent out for
application to the mainline. A part of that looks as follows:
$ git version # that's 2.55.0 + Phillip Wood's series addressing my previous bug report
git version 2.55.0.11.g153666a7d9bb
$ git log --pretty=oneline --abbrev-commit --decorate --boundary --graph --cherry-mark --right-only next/master...a54cadc575df
* a54cadc575df merge mod_devicetable.h cleanups
|\
| * fc69191474ff virtio-pci: Drop inclusion of <linux/mod_devicetable.h>
| * 80964227feed greybus: Drop #include of <linux/mod_devicetable.h>
| * ede7ce64f20e Documentation: Update after split of <linux/mod_devicetable.h>
| * c8efa35aeff7 HID: wacom: #include <linux/device-id/hid.h> instead of <linux/mod_devicetable.h>
| * baead64db0b9 checkpatch: Adapt comment to mod_devicetable.h split
| * ca270a534d0f net: phy: Drop #inclusion of <linux/mod_devicetable.h> from <linux/mdio.h>
* | 9874577217c5 WIP: Don't build XFS on m68k due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122438
* | 554e345d3223 s390: export memory encryption helper functions
* | 41d6dc9f88ec Add defconfigs for x86 and arm64 to yield stable builds
* | 9a6220166a5e drm/vmwgfx: Don't use UTS_RELEASE directly
o | 49362394dad7 (tag: next-20260713, next/master, next/HEAD) Add linux-next specific files for 20260713
/
o 8cdeaa50eae8 (tag: v7.2-rc2) Linux 7.2-rc2
So this suggests that all the commits are not yet in next. But if I look
at the right branch only, one actually is:
$ git log --pretty=oneline --abbrev-commit --decorate --boundary --graph --cherry-mark --right-only next/master...a54cadc575df^2
* fc69191474ff virtio-pci: Drop inclusion of <linux/mod_devicetable.h>
* 80964227feed greybus: Drop #include of <linux/mod_devicetable.h>
* ede7ce64f20e Documentation: Update after split of <linux/mod_devicetable.h>
* c8efa35aeff7 HID: wacom: #include <linux/device-id/hid.h> instead of <linux/mod_devicetable.h>
* baead64db0b9 checkpatch: Adapt comment to mod_devicetable.h split
= ca270a534d0f net: phy: Drop #inclusion of <linux/mod_devicetable.h> from <linux/mdio.h>
o 8cdeaa50eae8 (tag: v7.2-rc2) Linux 7.2-rc2
I would have expected that ca270a534d0f is marked with = already in the
upper dump. Is my expectation wrong here, or is that a bug?
If you want to look at it, I can provide the relevant commits in a
bundle via private mail.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] send-pack: introduce a `no-ref-delta` capability
From: Jeff King @ 2026-07-14 7:45 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <alQ7U8TOWjhasaWk@com-79390>
On Sun, Jul 12, 2026 at 06:11:47PM -0700, Taylor Blau wrote:
> Some 'receive-pack' implementations may wish to retain the incoming pack
> without first building an object ID index, in which case requiring delta
> bases to appear earlier in the same pack makes them easier to locate.
This explanation puzzles me. OK, I can see why you might want to take in
the incoming pack and then sit on it for a bit. But surely you are not
going to update refs without seeing what's in the pack, right? Otherwise
any pushing client can corrupt your repo.
And the only way to know what's in the pack is to index it. At which
point resolving REF_DELTAs is the least of your worries there.
So I have the feeling that there's some ulterior motive, or that this is
part of a larger system, but I don't quite understand what it is. And so
it's hard to say whether this is a sensible approach.
> Bitmap pack reuse is different, since it copies entries directly from
> an existing pack. Under `--no-ref-delta`, it must inspect candidate
> objects individually, omit `REF_DELTA` entries from direct pack reuse,
> and leave them to the normal object-writing path.
Hmm. We wouldn't normally expect verbatim pack-reuse to kick in, since
this is about the client sending to the server. But OK, we certainly
need to make sure that path remains correct.
> - The final patch advertises and consumes the new `no-ref-delta`
> capability.
What about thin packs? They'll result in REF_DELTAs on the server once
the pack is completed/indexed. I guess we have the "no-thin" capability,
but I don't think our receive-pack implementation support sending it. I
also wouldn't be terribly surprised if not every client implementation
supports it (it was added in 2013 I think to support libgit2). But I
guess that is also true of your new no-ref-delta; only updated clients
will respect it.
What will/should a server do when they get a ref delta anyway? That
again goes back to the question of: why don't we want ref deltas?
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] t1100: modernize test style
From: Patrick Steinhardt @ 2026-07-14 7:39 UTC (permalink / raw)
To: Shlok Kulshreshtha; +Cc: git, Junio C Hamano
In-Reply-To: <20260713140142.27898-2-diy2903@gmail.com>
On Mon, Jul 13, 2026 at 07:31:40PM +0530, Shlok Kulshreshtha wrote:
> diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
> index ae66ba5bab..fabe5a97cb 100755
> --- a/t/t1100-commit-tree-options.sh
> +++ b/t/t1100-commit-tree-options.sh
> @@ -22,28 +22,28 @@ committer Committer Name <committer@email> 1117150200 +0000
> comment text
> EOF
>
> -test_expect_success \
> - 'test preparation: write empty tree' \
> - 'git write-tree >treeid'
> -
> -test_expect_success \
> - 'construct commit' \
> - 'echo comment text |
> - GIT_AUTHOR_NAME="Author Name" \
> - GIT_AUTHOR_EMAIL="author@email" \
> - GIT_AUTHOR_DATE="2005-05-26 23:00" \
> - GIT_COMMITTER_NAME="Committer Name" \
> - GIT_COMMITTER_EMAIL="committer@email" \
> - GIT_COMMITTER_DATE="2005-05-26 23:30" \
> - TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null'
> -
> -test_expect_success \
> - 'read commit' \
> - 'git cat-file commit $(cat commitid) >commit'
> -
> -test_expect_success \
> - 'compare commit' \
> - 'test_cmp expected commit'
> +test_expect_success 'test preparation: write empty tree' '
> + git write-tree >treeid
> +'
> +
> +test_expect_success 'construct commit' '
> + echo comment text |
> + GIT_AUTHOR_NAME="Author Name" \
> + GIT_AUTHOR_EMAIL="author@email" \
> + GIT_AUTHOR_DATE="2005-05-26 23:00" \
> + GIT_COMMITTER_NAME="Committer Name" \
> + GIT_COMMITTER_EMAIL="committer@email" \
> + GIT_COMMITTER_DATE="2005-05-26 23:30" \
> + TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null
> +'
> +
> +test_expect_success 'read commit' '
> + git cat-file commit $(cat commitid) >commit
> +'
> +
> +test_expect_success 'compare commit' '
> + test_cmp expected commit
> +'
>
>
> test_expect_success 'flags and then non flags' '
Nit: let's remove the extraneous empty line while at it.
Patrick
^ permalink raw reply
* Re: [PATCH 0/2] packfile URIs: support concurrent downloads
From: Jeff King @ 2026-07-14 7:31 UTC (permalink / raw)
To: Taylor Blau
Cc: Ted Nyman, Junio C Hamano, git, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alWiEXbP5vOcCJ7F@com-79390>
On Mon, Jul 13, 2026 at 07:42:25PM -0700, Taylor Blau wrote:
> As some background, my workflow for sending patches to the mailing list
> is to use a script called 'git mail' that effectively runs format-patch
> to build an *.mbox and then opens Mutt in that directory. I then review
> the patches one last time before sending, and then run a macro I have
> bound to 'B', which (effectively) runs <resend-message>.
>
> For reasons that I cannot quite recall, I chose this workflow many years
> ago when it would likely have been more appropriate to use `mutt -H`,
> which does *not* rewrite Message-ID headers when resending.
You might have inherited the <resend-message> thing from me. I thought I
used it exactly because "-H" insisted on rewriting the message-id, but
it doesn't seem to now. So either I've completely forgotten the reason,
or perhaps the behavior used to be different.
I also like that <resend-message> lets me open the whole mbox and send
each message within a single session. But I never run into the issue
you're mentioning because I write my cover letter separately as a normal
email, and then generate the actual patches as in-reply-to (with some
script magic to pull the message-id from my sent folder).
So possibly my fault for leading you in the wrong direction many years
ago, or your fault for not following my sage advice to the letter. ;)
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`
From: Jeff King @ 2026-07-14 7:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano, Taylor Blau
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:
> Changes in v3:
> - Weave Peff's patch into the patch series.
> - Link to v2: https://patch.msgid.link/20260710-pks-odb-for-each-object-filter-v2-0-3710a9cc165a@pks.im
Yay, thank you. :)
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects
From: Jeff King @ 2026-07-14 7:17 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, git, Justin Tobler, Junio C Hamano
In-Reply-To: <alWz_gRs_D0Y0aOy@com-79390>
On Mon, Jul 13, 2026 at 08:58:54PM -0700, Taylor Blau wrote:
> > That's fair. But adapting `traverse_commit_list()` requires tons of
> > changes all over the tree, so I'm inclined to rather leave both
> > `traverse_bitmap_commit_list()` and `traverse_commit_list()` as-is.
> > Does that work for both of you?
>
> I think that it's fine to leave it as-is for the purpose of this series,
> though I would like to address it.
Me too.
> I don't think we need to adapt `traverse_commit_list()`, though. We can
> go in the other direction Peff suggested, which would be to split the
> callback type used by `for_each_bitmapped_object()` from
> `show_reachable_fn`, keep the former abortable, and make the latter
> return void.
>
> That keeps `traverse_bitmap_commit_list()` in sync with
> `traverse_commit_list()` without changing the non-bitmap traversal
> machinery. I have a small two-patch follow-up on top of v3 that does
> this, which I'll send separately.
That would be a nice cleanup if it's possible, but I wondered if you
would find that one or more of the callbacks actually rely on this abort
feature. Only one way to find out. :)
-Peff
^ permalink raw reply
* [PATCH v2 2/2] t1100: move creation of expected output into setup test
From: Shlok Kulshreshtha @ 2026-07-14 7:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shlok Kulshreshtha
In-Reply-To: <20260714071633.35446-1-diy2903@gmail.com>
The "expected" file is created at the top-level of the script, outside
of any test. Code that runs outside of a test is not protected by the
test harness: a failure there is not reported as a test failure and is
easy to miss.
Move the here-doc that creates "expected" into the existing setup test
("test preparation: write empty tree"), using a "<<-" here-doc so its
body can be indented along with the rest of the test.
Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
---
t/t1100-commit-tree-options.sh | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index fabe5a97cb..b434d1848e 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -14,15 +14,14 @@ Also make sure that command line parser understands the normal
. ./test-lib.sh
-cat >expected <<EOF
-tree $EMPTY_TREE
-author Author Name <author@email> 1117148400 +0000
-committer Committer Name <committer@email> 1117150200 +0000
-
-comment text
-EOF
-
test_expect_success 'test preparation: write empty tree' '
+ cat >expected <<-EOF &&
+ tree $EMPTY_TREE
+ author Author Name <author@email> 1117148400 +0000
+ committer Committer Name <committer@email> 1117150200 +0000
+
+ comment text
+ EOF
git write-tree >treeid
'
--
2.52.0
^ permalink raw reply related
* [PATCH v2 1/2] t1100: modernize test style
From: Shlok Kulshreshtha @ 2026-07-14 7:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shlok Kulshreshtha
In-Reply-To: <20260714071633.35446-1-diy2903@gmail.com>
The tests in this script use the old style in which the test title and
body are passed as separate backslash-continued arguments, with bodies
indented using spaces:
test_expect_success \
'title' \
'body'
Convert them to the modern style in which the body is a single-quoted
block on its own lines, indented with a tab:
test_expect_success 'title' '
body
'
This is a style-only change; no test logic is modified.
Signed-off-by: Shlok Kulshreshtha <diy2903@gmail.com>
---
t/t1100-commit-tree-options.sh | 44 +++++++++++++++++-----------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index ae66ba5bab..fabe5a97cb 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -22,28 +22,28 @@ committer Committer Name <committer@email> 1117150200 +0000
comment text
EOF
-test_expect_success \
- 'test preparation: write empty tree' \
- 'git write-tree >treeid'
-
-test_expect_success \
- 'construct commit' \
- 'echo comment text |
- GIT_AUTHOR_NAME="Author Name" \
- GIT_AUTHOR_EMAIL="author@email" \
- GIT_AUTHOR_DATE="2005-05-26 23:00" \
- GIT_COMMITTER_NAME="Committer Name" \
- GIT_COMMITTER_EMAIL="committer@email" \
- GIT_COMMITTER_DATE="2005-05-26 23:30" \
- TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null'
-
-test_expect_success \
- 'read commit' \
- 'git cat-file commit $(cat commitid) >commit'
-
-test_expect_success \
- 'compare commit' \
- 'test_cmp expected commit'
+test_expect_success 'test preparation: write empty tree' '
+ git write-tree >treeid
+'
+
+test_expect_success 'construct commit' '
+ echo comment text |
+ GIT_AUTHOR_NAME="Author Name" \
+ GIT_AUTHOR_EMAIL="author@email" \
+ GIT_AUTHOR_DATE="2005-05-26 23:00" \
+ GIT_COMMITTER_NAME="Committer Name" \
+ GIT_COMMITTER_EMAIL="committer@email" \
+ GIT_COMMITTER_DATE="2005-05-26 23:30" \
+ TZ=GMT git commit-tree $(cat treeid) >commitid 2>/dev/null
+'
+
+test_expect_success 'read commit' '
+ git cat-file commit $(cat commitid) >commit
+'
+
+test_expect_success 'compare commit' '
+ test_cmp expected commit
+'
test_expect_success 'flags and then non flags' '
--
2.52.0
^ permalink raw reply related
* [PATCH v2 0/2] t1100: modernize test script
From: Shlok Kulshreshtha @ 2026-07-14 7:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shlok Kulshreshtha
In-Reply-To: <20260713140142.27898-1-diy2903@gmail.com>
This is v2 of the microproject cleaning up
t/t1100-commit-tree-options.sh ("Modernize a test script").
Thanks to Junio for reviewing v1. The only change since v1 is in the
commit message of patch 2/2: it now uses the present tense ("is
created") to describe the current behavior of the script, as suggested.
Patch 1/2 is unchanged.
1/2 converts the tests from the old backslash-continued
test_expect_success style with space-indented bodies to the
modern quoted-body form indented with tabs.
2/2 moves the here-doc that creates "expected" out of the script's
top level and into the existing setup test, so it runs under the
protection of the test harness.
t1100 continues to pass all 5 tests.
Shlok Kulshreshtha (2):
t1100: modernize test style
t1100: move creation of expected output into setup test
t/t1100-commit-tree-options.sh | 59 +++++++++++++++++-----------------
1 file changed, 29 insertions(+), 30 deletions(-)
Range-diff against v1:
1: 45f590f110 = 1: 45f590f110 t1100: modernize test style
2: f74c71c104 ! 2: 36ea70be9d t1100: move creation of expected output into setup test
@@ Metadata
## Commit message ##
t1100: move creation of expected output into setup test
- The "expected" file was created at the top level of the script, outside
+ The "expected" file is created at the top-level of the script, outside
of any test. Code that runs outside of a test is not protected by the
test harness: a failure there is not reported as a test failure and is
easy to miss.
--
2.52.0
^ permalink raw reply
* Re: [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs
From: Jeff King @ 2026-07-14 7:13 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20260714071231.GD2516582@coredump.intra.peff.net>
On Tue, Jul 14, 2026 at 03:12:31AM -0400, Jeff King wrote:
> Would a more generic name like "cmd_output" or something make sense? I
> also think this would all be much nicer with a strbuf (which would let
> us get rid of the magic numbers), but that is a slightly larger
> refactor:
>
> diff --git a/fetch-pack.c b/fetch-pack.c
In case anybody does pursue this, it is obviously missing this bit:
diff --git a/fetch-pack.c b/fetch-pack.c
index 5f94f35c30..359740f231 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1935,6 +1935,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
xstrfmt("%s/pack/pack-%s.keep",
repo_get_object_directory(the_repository),
packhash));
+
+ strbuf_release(&cmd_output);
}
string_list_clear(&packfile_uris, 0);
strvec_clear(&index_pack_args);
to avoid a leak.
-Peff
^ permalink raw reply related
* Re: [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs
From: Jeff King @ 2026-07-14 7:12 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alVoA5-fDDPwKPZZ@com-76773>
On Mon, Jul 13, 2026 at 03:34:43PM -0700, Ted Nyman wrote:
> When "index-pack --keep" creates a .keep file, it reports
> "keep<TAB><hash>". If the file already exists, index-pack leaves it
> untouched and reports "pack<TAB><hash>" instead.
>
> Since dd4b732df7 (upload-pack: send part of packfile response as uri,
> 2020-06-10), fetch-pack has accepted only the "keep" form for packs
> downloaded through packfile URIs. A concurrent fetch can install the
> same pack and create its .keep file before another process reaches
> index-pack. The latter process then fails even though index-pack
> completed successfully.
>
> Accept both successful forms. Add a path to pack_lockfiles only for the
> "keep" form, so cleanup removes only a keep file created by the current
> process and preserves a pre-existing one.
OK, that all makes sense.
> for (i = 0; i < packfile_uris.nr; i++) {
> + int created_keep = 0;
> int j;
> struct child_process cmd = CHILD_PROCESS_INIT;
> - char packname[GIT_MAX_HEXSZ + 1];
> + char packname[GIT_MAX_HEXSZ + 6];
> + const char *packhash;
> + const int packname_len = the_hash_algo->hexsz + 6;
The "+ 6" here is gross, but not really any more than the bare "5" in
the original code.
Calling it "packhash" made me wonder about this line of code:
> - if (memcmp(packfile_uris.items[i].string, packname,
> + if (memcmp(packfile_uris.items[i].string, packhash,
Surely we need to change more than this if we now have the hash rather
than the whole packname? But no, the original code was really just
storing the hash in packname.
Which was rather misleading, but it is not much better after your patch.
Now packname still just has the packhash, along with the extra keep/pack
marker.
Would a more generic name like "cmd_output" or something make sense? I
also think this would all be much nicer with a strbuf (which would let
us get rid of the magic numbers), but that is a slightly larger
refactor:
diff --git a/fetch-pack.c b/fetch-pack.c
index 1e8461d07e..5f94f35c30 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1890,9 +1890,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
int created_keep = 0;
int j;
struct child_process cmd = CHILD_PROCESS_INIT;
- char packname[GIT_MAX_HEXSZ + 6];
+ struct strbuf cmd_output = STRBUF_INIT;
const char *packhash;
- const int packname_len = the_hash_algo->hexsz + 6;
const char *uri = packfile_uris.items[i].string +
the_hash_algo->hexsz + 1;
@@ -1910,14 +1909,11 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
if (start_command(&cmd))
die("fetch-pack: unable to spawn http-fetch");
- if (read_in_full(cmd.out, packname, packname_len) != packname_len ||
- packname[packname_len - 1] != '\n')
- die("fetch-pack: expected pack or keep, TAB, hash, "
- "then LF in http-fetch output");
- packname[packname_len - 1] = '\0';
- if (skip_prefix(packname, "keep\t", &packhash))
+ if (strbuf_read(&cmd_output, cmd.out, 0) < 0)
+ die("failed to read http-fetch output");
+ if (skip_prefix(cmd_output.buf, "keep\t", &packhash))
created_keep = 1;
- else if (!skip_prefix(packname, "pack\t", &packhash))
+ else if (!skip_prefix(cmd_output.buf, "pack\t", &packhash))
die("fetch-pack: expected pack or keep, TAB, hash, "
"then LF in http-fetch output");
BTW, two things that puzzled me while poking at your patch (but neither
I think are new or the fault of your patch):
1. git-http-fetch documents --index-pack-args, but the actual option
is the singular --index-pack-arg. The caller in fetch-pack
obviously uses the one that works.
2. The code you're touching insists on reading "keep" in the output,
but wouldn't that depend on feeding "--keep" via index_pack_args? I
didn't see immediately where we set it, but I certainly don't think
your patch could be making anything worse here (since the existing
code would have just died upon seeing a "pack" line). I think it
happens as a side effect in get_pack(), which is...subtle. But
again, not anything new.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Jeff King @ 2026-07-14 6:46 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alVn-QmK3K91_tkH@com-76773>
On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
Yuck. In theory they're writing the same thing, but I think the source
of the corruption is append mode. Two concurrent writers will keep
auto-seeking to the end of the file, rather than keeping their own file
pointers. There's no way to ask for O_APPEND without O_TRUNC via stdio,
but we can drop down a level like this:
diff --git a/http.c b/http.c
index b4e7b8d00b..d7362c99a2 100644
--- a/http.c
+++ b/http.c
@@ -2740,6 +2740,7 @@ struct http_pack_request *new_direct_http_pack_request(
{
off_t prev_posn = 0;
struct http_pack_request *preq;
+ int fd;
CALLOC_ARRAY(preq, 1);
strbuf_init(&preq->tmpfile, 0);
@@ -2748,12 +2749,13 @@ struct http_pack_request *new_direct_http_pack_request(
odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
strbuf_addstr(&preq->tmpfile, ".temp");
- preq->packfile = fopen(preq->tmpfile.buf, "a");
- if (!preq->packfile) {
+ fd = open(preq->tmpfile.buf, O_WRONLY|O_CREAT, 0666);
+ if (fd < 0) {
error("Unable to open local file %s for pack",
preq->tmpfile.buf);
goto abort;
}
+ preq->packfile = xfdopen(fd, "w");
preq->slot = get_active_slot();
preq->headers = object_request_headers();
That patch (with no other code changes) passes your test.
I suspect it could cause us to racily send an http range of "N-" to the
server, where N is the total number of bytes in the file (because we
don't know how many bytes there are supposed to be). I don't know if
that would cause an HTTP 416 or not. I think possibly not, and the 416
you saw (and that I see when running the test without any code changes)
might be from sending a range that starts _past_ N. We end up with a
too-long when both processes are appending.
I can't say I love the overall notion of "two processes are writing the
same data, it will probably be fine!". There might be portability
issues, and I'm not sure what would happen if we ever did get
conflicting data. If we're just feeding this to "index-pack --stdin"
we'd at least notice the problem (rather than quietly corrupting the
indexed file!).
So I'm offering this as a point for further discussion, and not
necessarily a counter-proposal. ;)
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.
Arguably losing the ability to retry is a regression. In general, I
think we should prefer correctness to efficiency. But I wonder if this
is a case where the user might want to make the choice to say "I am not
going to fetch two packfiles at once; please enable resumable fetches".
Especially because one of the selling points of packfile URIs is that
they are resumable.
One other thought on resumable transfers: if we are not going to resume
the transfer, then why spool the pack to disk at all? In other words,
why not just send it straight to "index-pack --stdin". That fixes your
concurrency issue (because it uses its own tempfiles behind the scene),
but has two other big advantages:
1. It halves the number of disk writes, and lowers the peak disk usage
(with the current code, there is a moment where both the tempfile
and the indexed pack are present on disk).
2. It pipelines the data processing. The current code bottlenecks on
the network while the CPU sits idle, and then bottlenecks on the
CPU once we have the whole file. We could be doing useful CPU work
during the network transfer, just like a regular pack code does.
So I'm not quite sold on losing the ability to resume entirely. And in
cases where we do lose it, I think it opens up other improvements.
But I'll reader over the rest of the patch with the notion that this is
the direction we want to go in.
> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
> line (which is not expected in
> this case), 'git http-fetch' fetches the packfile directly at the given
> URL and uses index-pack to generate corresponding .idx and .keep files.
> - The hash is used to determine the name of the temporary file and is
> - arbitrary. The output of index-pack is printed to stdout. Requires
> - --index-pack-args.
> + The hash is arbitrary. The output of index-pack is printed to stdout.
> + Requires --index-pack-args.
Do we even need to provide a hash anymore? After your patch I don't
think we even use it. It might be worth keeping around, though, as it
would be a unique key for de-duping or resuming, if we ever did
implement those on top.
> void release_http_pack_request(struct http_pack_request *preq)
> {
> - if (preq->packfile) {
> + if (preq->tempfile) {
> + delete_tempfile(&preq->tempfile);
> + preq->packfile = NULL;
> + } else if (preq->packfile) {
> fclose(preq->packfile);
> preq->packfile = NULL;
> }
OK. I think this is correct, though see my comments elsewhere in the
thread.
> @@ -2688,7 +2691,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
> int tmpfile_fd;
> int ret = 0;
>
> - fclose(preq->packfile);
> + if (preq->tempfile)
> + close_tempfile_gently(preq->tempfile);
> + else
> + fclose(preq->packfile);
> preq->packfile = NULL;
OK, and this is correct because preq->packfile is just an alias for
preq->tempfile.fp when the tempfile is valid. The NULL assignment is
important here so that the release() function doesn't double-free.
> -struct http_pack_request *new_http_pack_request(
> - const unsigned char *packed_git_hash, const char *base_url) {
> -
> - struct strbuf buf = STRBUF_INIT;
> -
> - end_url_with_slash(&buf, base_url);
> - strbuf_addf(&buf, "objects/pack/pack-%s.pack",
> - hash_to_hex(packed_git_hash));
> - return new_direct_http_pack_request(packed_git_hash,
> - strbuf_detach(&buf, NULL));
> -}
This hunk puzzled me at first, but it's because we used to just be a
wrapper for the "direct" variant, and now the two will share a single
static helper. That might have been a little more clear as a preparatory
patch, but OK.
> + if (resumable) {
> + odb_pack_name(the_repository, &preq->tmpfile,
> + packed_git_hash, "pack");
> + strbuf_addstr(&preq->tmpfile, ".temp");
> + preq->packfile = fopen(preq->tmpfile.buf, "a");
> + } else {
> + strbuf_addf(&preq->tmpfile, "%s/pack/tmp_pack_XXXXXX",
> + repo_get_object_directory(the_repository));
> + preq->tempfile = mks_tempfile_m(preq->tmpfile.buf, 0444);
> + if (preq->tempfile) {
> + strbuf_reset(&preq->tmpfile);
> + strbuf_addstr(&preq->tmpfile,
> + get_tempfile_path(preq->tempfile));
> + preq->packfile = fdopen_tempfile(preq->tempfile, "w");
> + }
> + }
> if (!preq->packfile) {
> error("Unable to open local file %s for pack",
> preq->tmpfile.buf);
OK, and this is the meat of the change. We usually use odb_mkstemp() for
tmp_pack_* files, but that annoyingly doesn't give you a tempfile
struct. So setting up your own filename and using mks_tempfile_m() makes
sense here.
The error path is a little funny, but we catch it in the context when
preq->packfile is NULL. Good.
> @@ -2766,8 +2776,9 @@ struct http_pack_request *new_direct_http_pack_request(
> * If there is data present from a previous transfer attempt,
> * resume where it left off
> */
> - prev_posn = ftello(preq->packfile);
> - if (prev_posn>0) {
> + if (resumable)
> + prev_posn = ftello(preq->packfile);
> + if (prev_posn > 0) {
I think this is not technically necessary, as ftello() would just return
"0" for our newly-created file. But it does make the intent clear.
> @@ -2779,12 +2790,28 @@ struct http_pack_request *new_direct_http_pack_request(
> return preq;
>
> abort:
> - strbuf_release(&preq->tmpfile);
> - free(preq->url);
> - free(preq);
> + release_http_pack_request(preq);
> return NULL;
> }
OK, now we have potentially more to free, so we rely on the release
function. That could cause problems if we jump to this abort label when
the struct isn't fully initialized. I think it is OK, though. We zero
the whole thing, so the extra fields that the release() function
considers will just be ignored.
> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>
> FILE *packfile;
> struct strbuf tmpfile;
> + struct tempfile *tempfile;
> struct active_request_slot *slot;
> struct curl_slist *headers;
Yuck, now we have "tempfile" and "tmpfile" with two different types and
totally different semantics (and even when "tempfile" is in use,
"tmpfile" is still meaningful!).
Can we even just call the second one non_resumable_tempfile or
something? It's a mouthful, but it makes it less likely to confuse the
two.
> + # Hold the first download before it is indexed, so that the second
> + # download installs the pack first.
> + {
> + (
> + if ! PATH="$TRASH_DIRECTORY:$PATH" \
> + GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
> + GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
> + git -C packfileclient-concurrent http-fetch \
> + --packfile="$packhash" \
> + --index-pack-arg=wait-index-pack \
> + --index-pack-arg=--stdin \
> + --index-pack-arg=--keep \
> + "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
> + then
> + echo failed >"$TRASH_DIRECTORY/first-ready" &&
> + exit 1
> + fi
> + ) &
> + first_pid=$!
> + } &&
OK. I wonder if it would be simpler and a more robust test if rather
than writing the correct bytes (and then waiting), the first process
just wrote total garbage. Then we'd be sure the other process is not
reading it, because it would definitely corrupt their input.
I dunno. This is a more realistic scenario, so in that sense maybe it is
more interesting.
> + test_when_finished "
> + echo continue >&9
> + wait $first_pid 2>/dev/null || :
> + exec 8>&-
> + exec 9>&-
> + rm -f first-ready first-continue git-wait-index-pack
> + " &&
> [...]
The rest of the fifo handling looks plausibly correct. This is a tricky
area and it's common to introduce funky races, but I didn't see anything
wrong, and it passed a few dozen rounds of --stress.
> @@ -313,7 +381,9 @@ test_expect_success 'http-fetch --packfile with corrupt pack' '
> git init packfileclient &&
> p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && ls objects/pack/pack-*.pack) &&
> test_must_fail git -C packfileclient http-fetch --packfile \
> - "$HTTPD_URL"/dumb/repo_bad1.git/$p
> + "$HTTPD_URL"/dumb/repo_bad1.git/$p &&
> + find packfileclient/.git/objects/pack -name "tmp_pack_*" -print >tmpfiles &&
> + test_must_be_empty tmpfiles
> '
OK, so here we just detect that we cleaned up after ourselves. Makes
sense.
-Peff
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox