* [PATCH 6/9] upload-pack: disallow object-info capability by default
From: Jeff King @ 2024-02-28 22:38 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
From: Taylor Blau <me@ttaylorr.com>
We added an "object-info" capability to the v2 upload-pack protocol in
a2ba162cda (object-info: support for retrieving object info,
2021-04-20). In the almost 3 years since, we have not added any
client-side support, and it does not appear to exist in other
implementations either (JGit understands the verb on the server side,
but not on the client side).
Since this largely unused code is accessible over the network by
default, it increases the attack surface of upload-pack. I don't know of
any particularly severe problem, but one issue is that because of the
request/response nature of the v2 protocol, it will happily read an
unbounded number of packets, adding each one to a string list (without
regard to whether they are objects we know about, duplicates, etc).
This may be something we want to improve in the long run, but in the
short term it makes sense to disable the feature entirely. We'll add a
config option as an escape hatch for anybody who wants to develop the
feature further.
A more gentle option would be to add the config option to let people
disable it manually, but leave it enabled by default. But given that
there's no client side support, that seems like the wrong balance with
security.
Disabling by default will slow adoption a bit once client-side support
does become available (there were some patches[1] in 2022, but nothing
got merged and there's been nothing since). But clients have to deal
with older servers that do not understand the option anyway (and the
capability system handles that), so it will just be a matter of servers
flipping their config at that point (and hopefully once any unbounded
allocations have been addressed).
[jk: this is a patch that GitHub has been running for several years, but
rebased forward and with a new commit message for upstream]
[1] https://lore.kernel.org/git/20220208231911.725273-1-calvinwan@google.com/
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config/transfer.txt | 4 ++++
serve.c | 14 +++++++++++++-
t/t5555-http-smart-common.sh | 1 -
t/t5701-git-serve.sh | 24 +++++++++++++++++++++++-
4 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
index a9cbdb88a1..f1ce50f4a6 100644
--- a/Documentation/config/transfer.txt
+++ b/Documentation/config/transfer.txt
@@ -121,3 +121,7 @@ transfer.bundleURI::
information from the remote server (if advertised) and download
bundles before continuing the clone through the Git protocol.
Defaults to `false`.
+
+transfer.advertiseObjectInfo::
+ When `true`, the `object-info` capability is advertised by
+ servers. Defaults to false.
diff --git a/serve.c b/serve.c
index a1d71134d4..aa651b73e9 100644
--- a/serve.c
+++ b/serve.c
@@ -12,6 +12,7 @@
#include "trace2.h"
static int advertise_sid = -1;
+static int advertise_object_info = -1;
static int client_hash_algo = GIT_HASH_SHA1;
static int always_advertise(struct repository *r UNUSED,
@@ -67,6 +68,17 @@ static void session_id_receive(struct repository *r UNUSED,
trace2_data_string("transfer", NULL, "client-sid", client_sid);
}
+static int object_info_advertise(struct repository *r, struct strbuf *value UNUSED)
+{
+ if (advertise_object_info == -1 &&
+ repo_config_get_bool(r, "transfer.advertiseobjectinfo",
+ &advertise_object_info)) {
+ /* disabled by default */
+ advertise_object_info = 0;
+ }
+ return advertise_object_info;
+}
+
struct protocol_capability {
/*
* The name of the capability. The server uses this name when
@@ -135,7 +147,7 @@ static struct protocol_capability capabilities[] = {
},
{
.name = "object-info",
- .advertise = always_advertise,
+ .advertise = object_info_advertise,
.command = cap_object_info,
},
{
diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
index b1cfe8b7db..3dcb3340a3 100755
--- a/t/t5555-http-smart-common.sh
+++ b/t/t5555-http-smart-common.sh
@@ -131,7 +131,6 @@ test_expect_success 'git upload-pack --advertise-refs: v2' '
fetch=shallow wait-for-done
server-option
object-format=$(test_oid algo)
- object-info
0000
EOF
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index 3591bc2417..c48830de8f 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -20,7 +20,6 @@ test_expect_success 'test capability advertisement' '
fetch=shallow wait-for-done
server-option
object-format=$(test_oid algo)
- object-info
EOF
cat >expect.trailer <<-EOF &&
0000
@@ -323,6 +322,8 @@ test_expect_success 'unexpected lines are not allowed in fetch request' '
# Test the basics of object-info
#
test_expect_success 'basics of object-info' '
+ test_config transfer.advertiseObjectInfo true &&
+
test-tool pkt-line pack >in <<-EOF &&
command=object-info
object-format=$(test_oid algo)
@@ -380,4 +381,25 @@ test_expect_success 'basics of bundle-uri: dies if not enabled' '
test_must_be_empty out
'
+test_expect_success 'object-info missing from capabilities when disabled' '
+ test_config transfer.advertiseObjectInfo false &&
+
+ GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
+ --advertise-capabilities >out &&
+ test-tool pkt-line unpack <out >actual &&
+
+ ! grep object.info actual
+'
+
+test_expect_success 'object-info commands rejected when disabled' '
+ test_config transfer.advertiseObjectInfo false &&
+
+ test-tool pkt-line pack >in <<-EOF &&
+ command=object-info
+ EOF
+
+ test_must_fail test-tool serve-v2 --stateless-rpc <in 2>err &&
+ grep invalid.command err
+'
+
test_done
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 7/9] upload-pack: always turn off save_commit_buffer
From: Jeff King @ 2024-02-28 22:39 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When the client sends us "want $oid" lines, we call parse_object($oid)
to get an object struct. It's important to parse the commits because we
need to traverse them in the negotiation phase. But of course we don't
need to hold on to the commit messages for each one.
We've turned off the save_commit_buffer flag in get_common_commits() for
a long time, since f0243f26f6 (git-upload-pack: More efficient usage of
the has_sha1 array, 2005-10-28). That helps with the commits we see
while actually traversing. But:
1. That function is only used by the v0 protocol. I think the v2
protocol's code path leaves the flag on (and thus pays the extra
memory penalty), though I didn't measure it specifically.
2. If the client sends us a bunch of "want" lines, that happens before
the negotiation phase. So we'll hold on to all of those commit
messages. Generally the number of "want" lines scales with the
refs, not with the number of objects in the repo. But a malicious
client could send a lot in order to waste memory.
As an example of (2), if I generate a request to fetch all commits in
git.git like this:
pktline() {
local msg="$*"
printf "%04x%s\n" $((1+4+${#msg})) "$msg"
}
want_commits() {
pktline command=fetch
printf 0001
git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |
while read oid type; do
test "$type" = "commit" || continue
pktline want $oid
done
pktline done
printf 0000
}
want_commits | GIT_PROTOCOL=version=2 valgrind --tool=massif git-upload-pack . >/dev/null
before this patch upload-pack peaks at ~125MB, and after at ~35MB. The
difference is not coincidentally about the same as the sum of all commit
object sizes as computed by:
git cat-file --batch-all-objects --batch-check='%(objecttype) %(objectsize)' |
perl -alne '$v += $F[1] if $F[0] eq "commit"; END { print $v }'
In a larger repository like linux.git, that number is ~1GB.
In a repository with a full commit-graph file this will have no impact
(and the commit graph would save us from parsing at all, so is a much
better solution!). But it's easy to do, might help a little in
real-world cases (where even if you have a commit graph it might not be
fully up to date), and helps a lot for a worst-case malicious request.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/upload-pack.c | 2 ++
upload-pack.c | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 9b021ef026..15afb97260 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -8,6 +8,7 @@
#include "replace-object.h"
#include "upload-pack.h"
#include "serve.h"
+#include "commit.h"
static const char * const upload_pack_usage[] = {
N_("git-upload-pack [--[no-]strict] [--timeout=<n>] [--stateless-rpc]\n"
@@ -37,6 +38,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
packet_trace_identity("upload-pack");
disable_replace_refs();
+ save_commit_buffer = 0;
argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
diff --git a/upload-pack.c b/upload-pack.c
index 2a5c52666e..3970bb9b30 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -526,8 +526,6 @@ static int get_common_commits(struct upload_pack_data *data,
int got_other = 0;
int sent_ready = 0;
- save_commit_buffer = 0;
-
for (;;) {
const char *arg;
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 8/9] upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places
From: Jeff King @ 2024-02-28 22:39 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
In commit 0bc2557951 (upload-pack: skip parse-object re-hashing of
"want" objects, 2022-09-06), we optimized the parse_object() calls for
v2 "want" lines from the client so that they avoided parsing blobs, and
so that they used the commit-graph rather than parsing commit objects
from scratch.
We should extend that to two other spots:
1. We parse "have" objects in the got_oid() function. These won't
generally be non-commits (unlike "want" lines from a partial
clone). But we still benefit from the use of the commit-graph.
2. For v0, the "want" lines are parsed in receive_needs(). These are
also less likely to be non-commits because by default they have to
be ref tips. There are config options you might set to allow
non-tip objects, but you'd mostly do so to support partial clones,
and clients recent enough to support partial clone will generally
speak v2 anyway.
So I don't expect this change to improve performance much for day-to-day
operations. But both are possible denial-of-service vectors, where an
attacker can waste our time by sending over a large number of objects to
parse (of course we may waste even more time serving a pack to them, but
we try as much as possible to optimize that in pack-objects; we should
do what we can here in upload-pack, too).
With this patch, running p5600 with GIT_TEST_PROTOCOL_VERSION=0 shows
similar results to what we saw in 0bc2557951 (which ran with the v2
protocol by default). Here are the numbers for linux.git:
Test HEAD^ HEAD
-----------------------------------------------------------------------------
5600.3: checkout of result 50.91(87.95+2.93) 41.75(79.00+3.18) -18.0%
Or for a more extreme (and malicious) case, we can claim to "have" every
blob in git.git over the v0 protocol:
$ {
echo "0032want $(git rev-parse HEAD)"
printf 0000
git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |
perl -alne 'print "0032have $F[0]" if $F[1] eq "blob"'
} >input
$ time ./git.old upload-pack . <input >/dev/null
real 0m52.951s
user 0m51.633s
sys 0m1.304s
$ time ./git.new upload-pack . <input >/dev/null
real 0m0.261s
user 0m0.156s
sys 0m0.105s
(Note that these don't actually compute a pack because of the hacky
protocol usage, so those numbers are representing the raw blob-parsing
effort done by upload-pack).
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 3970bb9b30..b721155442 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -469,7 +469,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,
static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)
{
int we_knew_they_have = 0;
- struct object *o = parse_object(the_repository, oid);
+ struct object *o = parse_object_with_flags(the_repository, oid,
+ PARSE_OBJECT_SKIP_HASH_CHECK);
if (!o)
die("oops (%s)", oid_to_hex(oid));
@@ -1148,7 +1149,8 @@ static void receive_needs(struct upload_pack_data *data,
free(client_sid);
}
- o = parse_object(the_repository, &oid_buf);
+ o = parse_object_with_flags(the_repository, &oid_buf,
+ PARSE_OBJECT_SKIP_HASH_CHECK);
if (!o) {
packet_writer_error(&data->writer,
"upload-pack: not our ref %s",
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 9/9] upload-pack: free tree buffers after parsing
From: Jeff King @ 2024-02-28 22:39 UTC (permalink / raw)
To: git; +Cc: Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
When a client sends us a "want" or "have" line, we call parse_object()
to get an object struct. If the object is a tree, then the parsed state
means that tree->buffer points to the uncompressed contents of the tree.
But we don't really care about it. We only really need to parse commits
and tags; for trees and blobs, the important output is just a "struct
object" with the correct type.
But much worse, we do not ever free that tree buffer. It's not leaked in
the traditional sense, in that we still have a pointer to it from the
global object hash. But if the client requests many trees, we'll hold
all of their contents in memory at the same time.
Nobody really noticed because it's rare for clients to directly request
a tree. It might happen for a lightweight tag pointing straight at a
tree, or it might happen for a "tree:depth" partial clone filling in
missing trees.
But it's also possible for a malicious client to request a lot of trees,
causing upload-pack's memory to balloon. For example, without this
patch, requesting every tree in git.git like:
pktline() {
local msg="$*"
printf "%04x%s\n" $((1+4+${#msg})) "$msg"
}
want_trees() {
pktline command=fetch
printf 0001
git cat-file --batch-all-objects --batch-check='%(objectname) %(objecttype)' |
while read oid type; do
test "$type" = "tree" || continue
pktline want $oid
done
pktline done
printf 0000
}
want_trees | GIT_PROTOCOL=version=2 valgrind --tool=massif ./git upload-pack . >/dev/null
shows a peak heap usage of ~3.7GB. Which is just about the sum of the
sizes of all of the uncompressed trees. For linux.git, it's closer to
17GB.
So the obvious thing to do is to call free_tree_buffer() after we
realize that we've parsed a tree. We know that upload-pack won't need it
later. But let's push the logic into parse_object_with_flags(), telling
it to discard the tree buffer immediately. There are two reasons for
this. One, all of the relevant call-sites already call the with_options
variant to pass the SKIP_HASH flag. So it actually ends up as less code
than manually free-ing in each spot. And two, it enables an extra
optimization that I'll discuss below.
I've touched all of the sites that currently use SKIP_HASH in
upload-pack. That drops the peak heap of the upload-pack invocation
above from 3.7GB to ~24MB.
I've also modified the caller in get_reference(); a partial clone
benefits from its use in pack-objects for the reasons given in
0bc2557951 (upload-pack: skip parse-object re-hashing of "want" objects,
2022-09-06), where we were measuring blob requests. But note that the
results of get_reference() are used for traversing, as well; so we
really would _eventually_ use the tree contents. That makes this at
first glance a space/time tradeoff: we won't hold all of the trees in
memory at once, but we'll have to reload them each when it comes time to
traverse.
And here's where our extra optimization comes in. If the caller is not
going to immediately look at the tree contents, and it doesn't care
about checking the hash, then parse_object() can simply skip loading the
tree entirely, just like we do for blobs! And now it's not a space/time
tradeoff in get_reference() anymore. It's just a lazy-load: we're
delaying reading the tree contents until it's time to actually traverse
them one by one.
And of course for upload-pack, this optimization means we never load the
trees at all, saving lots of CPU time. Timing the "every tree from
git.git" request above shows upload-pack dropping from 32 seconds of CPU
to 19 (the remainder is mostly due to pack-objects actually sending the
pack; timing just the upload-pack portion shows we go from 13s to
~0.28s).
These are all highly gamed numbers, of course. For real-world
partial-clone requests we're saving only a small bit of time in
practice. But it does help harden upload-pack against malicious
denial-of-service attacks.
Signed-off-by: Jeff King <peff@peff.net>
---
object.c | 14 ++++++++++++++
object.h | 1 +
revision.c | 3 ++-
upload-pack.c | 9 ++++++---
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/object.c b/object.c
index e6a1c4d905..f11c59ac0c 100644
--- a/object.c
+++ b/object.c
@@ -271,6 +271,7 @@ struct object *parse_object_with_flags(struct repository *r,
enum parse_object_flags flags)
{
int skip_hash = !!(flags & PARSE_OBJECT_SKIP_HASH_CHECK);
+ int discard_tree = !!(flags & PARSE_OBJECT_DISCARD_TREE);
unsigned long size;
enum object_type type;
int eaten;
@@ -298,6 +299,17 @@ struct object *parse_object_with_flags(struct repository *r,
return lookup_object(r, oid);
}
+ /*
+ * If the caller does not care about the tree buffer and does not
+ * care about checking the hash, we can simply verify that we
+ * have the on-disk object with the correct type.
+ */
+ if (skip_hash && discard_tree &&
+ (!obj || obj->type == OBJ_TREE) &&
+ oid_object_info(r, oid, NULL) == OBJ_TREE) {
+ return &lookup_tree(r, oid)->object;
+ }
+
buffer = repo_read_object_file(r, oid, &type, &size);
if (buffer) {
if (!skip_hash &&
@@ -311,6 +323,8 @@ struct object *parse_object_with_flags(struct repository *r,
buffer, &eaten);
if (!eaten)
free(buffer);
+ if (discard_tree && type == OBJ_TREE)
+ free_tree_buffer((struct tree *)obj);
return obj;
}
return NULL;
diff --git a/object.h b/object.h
index 114d45954d..c7123cade6 100644
--- a/object.h
+++ b/object.h
@@ -197,6 +197,7 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet);
*/
enum parse_object_flags {
PARSE_OBJECT_SKIP_HASH_CHECK = 1 << 0,
+ PARSE_OBJECT_DISCARD_TREE = 1 << 1,
};
struct object *parse_object(struct repository *r, const struct object_id *oid);
struct object *parse_object_with_flags(struct repository *r,
diff --git a/revision.c b/revision.c
index 2424c9bd67..b10f63a607 100644
--- a/revision.c
+++ b/revision.c
@@ -381,7 +381,8 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
object = parse_object_with_flags(revs->repo, oid,
revs->verify_objects ? 0 :
- PARSE_OBJECT_SKIP_HASH_CHECK);
+ PARSE_OBJECT_SKIP_HASH_CHECK |
+ PARSE_OBJECT_DISCARD_TREE);
if (!object) {
if (revs->ignore_missing)
diff --git a/upload-pack.c b/upload-pack.c
index b721155442..761af4a532 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -470,7 +470,8 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid
{
int we_knew_they_have = 0;
struct object *o = parse_object_with_flags(the_repository, oid,
- PARSE_OBJECT_SKIP_HASH_CHECK);
+ PARSE_OBJECT_SKIP_HASH_CHECK |
+ PARSE_OBJECT_DISCARD_TREE);
if (!o)
die("oops (%s)", oid_to_hex(oid));
@@ -1150,7 +1151,8 @@ static void receive_needs(struct upload_pack_data *data,
}
o = parse_object_with_flags(the_repository, &oid_buf,
- PARSE_OBJECT_SKIP_HASH_CHECK);
+ PARSE_OBJECT_SKIP_HASH_CHECK |
+ PARSE_OBJECT_DISCARD_TREE);
if (!o) {
packet_writer_error(&data->writer,
"upload-pack: not our ref %s",
@@ -1467,7 +1469,8 @@ static int parse_want(struct packet_writer *writer, const char *line,
"expected to get oid, not '%s'", line);
o = parse_object_with_flags(the_repository, &oid,
- PARSE_OBJECT_SKIP_HASH_CHECK);
+ PARSE_OBJECT_SKIP_HASH_CHECK |
+ PARSE_OBJECT_DISCARD_TREE);
if (!o) {
packet_writer_error(writer,
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 0/4] some v2 capability advertisement cleanups
From: Jeff King @ 2024-02-28 22:46 UTC (permalink / raw)
To: git
While working on another series, I noticed that upload-pack will accept
the "packfile-uris" directive even if we didn't advertise it. That's not
a huge deal in practice, but the spec says we're not supposed to. And
while cleaning that up, I noticed a bit of duplication in the existing
advertisement/allow code.
So patches 1-3 clean up the situation a bit, and then patch 4 tightens
up the packfile-uris handling.
There are some small textual conflicts with the series I just posted in:
https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/
I'm happy to prepare this on top of that if it's easier.
[1/4]: upload-pack: use repository struct to get config
[2/4]: upload-pack: centralize setup of sideband-all config
[3/4]: upload-pack: use existing config mechanism for advertisement
[4/4]: upload-pack: only accept packfile-uris if we advertised it
t/t5702-protocol-v2.sh | 18 +++++++++++++
upload-pack.c | 58 +++++++++++++++++++-----------------------
2 files changed, 44 insertions(+), 32 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/4] upload-pack: use repository struct to get config
From: Jeff King @ 2024-02-28 22:46 UTC (permalink / raw)
To: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
Our upload_pack_v2() function gets a repository struct, but we ignore it
totally. In practice this doesn't cause any problems, as it will never
differ from the_repository. But in the spirit of taking a small step
towards getting rid of the_repository, let's at least starting using it
to grab config. There are probably other spots that could benefit, but
it's a start.
Note that we don't need to pass the repo for protected_config(); the
whole point there is that we are not looking at repo config, so there is
no repo-specific version of the function.
For the v0 version of the protocol, we're not passed a repository
struct, so we'll continue to use the_repository there.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 2537affa90..e156c1e472 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1385,9 +1385,10 @@ static int upload_pack_protected_config(const char *var, const char *value,
return 0;
}
-static void get_upload_pack_config(struct upload_pack_data *data)
+static void get_upload_pack_config(struct repository *r,
+ struct upload_pack_data *data)
{
- git_config(upload_pack_config, data);
+ repo_config(r, upload_pack_config, data);
git_protected_config(upload_pack_protected_config, data);
}
@@ -1398,7 +1399,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
struct upload_pack_data data;
upload_pack_data_init(&data);
- get_upload_pack_config(&data);
+ get_upload_pack_config(the_repository, &data);
data.stateless_rpc = stateless_rpc;
data.timeout = timeout;
@@ -1771,7 +1772,7 @@ enum fetch_state {
FETCH_DONE,
};
-int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)
+int upload_pack_v2(struct repository *r, struct packet_reader *request)
{
enum fetch_state state = FETCH_PROCESS_ARGS;
struct upload_pack_data data;
@@ -1780,7 +1781,7 @@ int upload_pack_v2(struct repository *r UNUSED, struct packet_reader *request)
upload_pack_data_init(&data);
data.use_sideband = LARGE_PACKET_MAX;
- get_upload_pack_config(&data);
+ get_upload_pack_config(r, &data);
while (state != FETCH_DONE) {
switch (state) {
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 2/4] upload-pack: centralize setup of sideband-all config
From: Jeff King @ 2024-02-28 22:47 UTC (permalink / raw)
To: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
We read uploadpack.allowsidebandall to set a matching flag in our
upload_pack_data struct. But for our tests, we also respect
GIT_TEST_SIDEBAND_ALL from the environment, and anybody looking at the
flag in the struct needs to remember to check both. There's only one
such piece of code now, but we're about to add another.
So let's have the config step actually fold the environment value into
the struct, letting the rest of the code use the flag in the obvious
way.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index e156c1e472..6bda20754d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1390,6 +1390,8 @@ static void get_upload_pack_config(struct repository *r,
{
repo_config(r, upload_pack_config, data);
git_protected_config(upload_pack_protected_config, data);
+
+ data->allow_sideband_all |= git_env_bool("GIT_TEST_SIDEBAND_ALL", 0);
}
void upload_pack(const int advertise_refs, const int stateless_rpc,
@@ -1639,8 +1641,7 @@ static void process_args(struct packet_reader *request,
continue;
}
- if ((git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
- data->allow_sideband_all) &&
+ if (data->allow_sideband_all &&
!strcmp(arg, "sideband-all")) {
data->writer.use_sideband = 1;
continue;
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* Re: [PATCH 0/9] bound upload-pack memory allocations
From: Junio C Hamano @ 2024-02-28 22:47 UTC (permalink / raw)
To: Jeff King; +Cc: git, Benjamin Flesch
In-Reply-To: <20240228223700.GA1157826@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We're not doing a coordinated disclosure or special release for this.
> Even after these patches, it's possible to get upload-pack to allocate
> quite a bit of memory, especially for a large repository. Not to mention
> that pack-objects may also allocate quite a bit to serve the pack
> itself. So while this is low-hanging fruit, a public-facing Git site
> probably still needs to have some kind of external tooling to kill
> hungry processes (even if it's just OOM-killing them so they don't hurt
> other clients).
>
> [1/9]: upload-pack: drop separate v2 "haves" array
> [2/9]: upload-pack: switch deepen-not list to an oid_array
> [3/9]: upload-pack: use oidset for deepen_not list
> [4/9]: upload-pack: use a strmap for want-ref lines
> [5/9]: upload-pack: accept only a single packfile-uri line
> [6/9]: upload-pack: disallow object-info capability by default
> [7/9]: upload-pack: always turn off save_commit_buffer
> [8/9]: upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places
> [9/9]: upload-pack: free tree buffers after parsing
I saw them when they were posted to the security list and they
looked good already. Will queue. Thanks.
^ permalink raw reply
* [PATCH 3/4] upload-pack: use existing config mechanism for advertisement
From: Jeff King @ 2024-02-28 22:48 UTC (permalink / raw)
To: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
When serving a v2 capabilities request, we call upload_pack_advertise()
to tell us the set of features we can advertise to the client. That
involves looking at various config options, all of which need to be kept
in sync with the rules we use in upload_pack_config to set flags like
allow_filter, allow_sideband_all, and so on. If these two pieces of code
get out of sync then we may refuse to respect a capability we
advertised, or vice versa accept one that we should not.
Instead, let's call the same config helper that we'll use for processing
the actual client request, and then just pick the values out of the
resulting struct. This is only a little bit shorter than the current
code, but we don't repeat any policy logic (e.g., we don't have to worry
about the magic sideband-all environment variable here anymore).
And this reveals a gap in the existing code: there is no struct flag for
the packfile-uris capability (we accept it even if it is not advertised,
which we should not). We'll leave the advertisement code for now and
deal with it in the next patch.
Signed-off-by: Jeff King <peff@peff.net>
---
upload-pack.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 6bda20754d..491ef51daa 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1841,41 +1841,35 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)
int upload_pack_advertise(struct repository *r,
struct strbuf *value)
{
+ struct upload_pack_data data;
+
+ upload_pack_data_init(&data);
+ get_upload_pack_config(r, &data);
+
if (value) {
- int allow_filter_value;
- int allow_ref_in_want;
- int allow_sideband_all_value;
char *str = NULL;
strbuf_addstr(value, "shallow wait-for-done");
- if (!repo_config_get_bool(r,
- "uploadpack.allowfilter",
- &allow_filter_value) &&
- allow_filter_value)
+ if (data.allow_filter)
strbuf_addstr(value, " filter");
- if (!repo_config_get_bool(r,
- "uploadpack.allowrefinwant",
- &allow_ref_in_want) &&
- allow_ref_in_want)
+ if (data.allow_ref_in_want)
strbuf_addstr(value, " ref-in-want");
- if (git_env_bool("GIT_TEST_SIDEBAND_ALL", 0) ||
- (!repo_config_get_bool(r,
- "uploadpack.allowsidebandall",
- &allow_sideband_all_value) &&
- allow_sideband_all_value))
+ if (data.allow_sideband_all)
strbuf_addstr(value, " sideband-all");
if (!repo_config_get_string(r,
"uploadpack.blobpackfileuri",
&str) &&
str) {
strbuf_addstr(value, " packfile-uris");
free(str);
}
}
+ upload_pack_data_clear(&data);
+
return 1;
}
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Jeff King @ 2024-02-28 22:50 UTC (permalink / raw)
To: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
Clients are only supposed to request particular capabilities or features
if the server advertised them. For the "packfile-uris" feature, we only
advertise it if uploadpack.blobpacfileuri is set, but we always accept a
request from the client regardless.
In practice this doesn't really hurt anything, as we'd pass the client's
protocol list on to pack-objects, which ends up ignoring it. But we
should try to follow the protocol spec, and tightening this up may catch
buggy or misbehaving clients more easily.
Thanks to recent refactoring, we can hoist the config check from
upload_pack_advertise() into upload_pack_config(). Note the subtle
handling of a value-less bool (which does not count for triggering an
advertisement).
Signed-off-by: Jeff King <peff@peff.net>
---
I suspect in the long term that we may have other ways to trigger this
feature than the static blobpackfileuri config (e.g., a hook that knows
about site-specific packfiles "somehow"). So we may need to update the
test later for that, but presumably in the vanilla config we'll continue
to skip advertising it.
t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
upload-pack.c | 16 +++++++---------
2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 6ef4971845..902e42c1c0 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '
! grep ^GIT_PROTOCOL env.trace
'
+test_expect_success 'reject client packfile-uris if not advertised' '
+ {
+ packetize command=fetch &&
+ printf 0001 &&
+ packetize packfile-uris https &&
+ packetize done &&
+ printf 0000
+ } >input &&
+ test_must_fail env GIT_PROTOCOL=version=2 \
+ git upload-pack client <input &&
+ test_must_fail env GIT_PROTOCOL=version=2 \
+ git -c uploadpack.blobpackfileuri \
+ upload-pack client <input &&
+ GIT_PROTOCOL=version=2 \
+ git -c uploadpack.blobpackfileuri=anything \
+ upload-pack client <input
+'
+
# Test protocol v2 with 'http://' transport
#
. "$TEST_DIRECTORY"/lib-httpd.sh
diff --git a/upload-pack.c b/upload-pack.c
index 491ef51daa..66f4de9d87 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -113,6 +113,7 @@ struct upload_pack_data {
unsigned done : 1; /* v2 only */
unsigned allow_ref_in_want : 1; /* v2 only */
unsigned allow_sideband_all : 1; /* v2 only */
+ unsigned allow_packfile_uris : 1; /* v2 only */
unsigned advertise_sid : 1;
unsigned sent_capabilities : 1;
};
@@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,
data->allow_ref_in_want = git_config_bool(var, value);
} else if (!strcmp("uploadpack.allowsidebandall", var)) {
data->allow_sideband_all = git_config_bool(var, value);
+ } else if (!strcmp("uploadpack.blobpackfileuri", var)) {
+ if (value)
+ data->allow_packfile_uris = 1;
} else if (!strcmp("core.precomposeunicode", var)) {
precomposed_unicode = git_config_bool(var, value);
} else if (!strcmp("transfer.advertisesid", var)) {
@@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,
continue;
}
- if (skip_prefix(arg, "packfile-uris ", &p)) {
+ if (data->allow_packfile_uris &&
+ skip_prefix(arg, "packfile-uris ", &p)) {
string_list_split(&data->uri_protocols, p, ',', -1);
continue;
}
@@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,
get_upload_pack_config(r, &data);
if (value) {
- char *str = NULL;
-
strbuf_addstr(value, "shallow wait-for-done");
if (data.allow_filter)
@@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,
if (data.allow_sideband_all)
strbuf_addstr(value, " sideband-all");
- if (!repo_config_get_string(r,
- "uploadpack.blobpackfileuri",
- &str) &&
- str) {
+ if (data.allow_packfile_uris)
strbuf_addstr(value, " packfile-uris");
- free(str);
- }
}
upload_pack_data_clear(&data);
--
2.44.0.rc2.424.gbdbf4d014b
^ permalink raw reply related
* Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Junio C Hamano @ 2024-02-28 23:43 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240228225050.GA1159078@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I suspect in the long term that we may have other ways to trigger this
> feature than the static blobpackfileuri config (e.g., a hook that knows
> about site-specific packfiles "somehow"). So we may need to update the
> test later for that, but presumably in the vanilla config we'll continue
> to skip advertising it.
Sounds quite sensible. Thanks for a series that is very pleasant to
read.
>
> t/t5702-protocol-v2.sh | 18 ++++++++++++++++++
> upload-pack.c | 16 +++++++---------
> 2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 6ef4971845..902e42c1c0 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '
> ! grep ^GIT_PROTOCOL env.trace
> '
>
> +test_expect_success 'reject client packfile-uris if not advertised' '
> + {
> + packetize command=fetch &&
> + printf 0001 &&
> + packetize packfile-uris https &&
> + packetize done &&
> + printf 0000
> + } >input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git upload-pack client <input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri \
> + upload-pack client <input &&
> + GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri=anything \
> + upload-pack client <input
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> diff --git a/upload-pack.c b/upload-pack.c
> index 491ef51daa..66f4de9d87 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -113,6 +113,7 @@ struct upload_pack_data {
> unsigned done : 1; /* v2 only */
> unsigned allow_ref_in_want : 1; /* v2 only */
> unsigned allow_sideband_all : 1; /* v2 only */
> + unsigned allow_packfile_uris : 1; /* v2 only */
> unsigned advertise_sid : 1;
> unsigned sent_capabilities : 1;
> };
> @@ -1362,6 +1363,9 @@ static int upload_pack_config(const char *var, const char *value,
> data->allow_ref_in_want = git_config_bool(var, value);
> } else if (!strcmp("uploadpack.allowsidebandall", var)) {
> data->allow_sideband_all = git_config_bool(var, value);
> + } else if (!strcmp("uploadpack.blobpackfileuri", var)) {
> + if (value)
> + data->allow_packfile_uris = 1;
> } else if (!strcmp("core.precomposeunicode", var)) {
> precomposed_unicode = git_config_bool(var, value);
> } else if (!strcmp("transfer.advertisesid", var)) {
> @@ -1647,7 +1651,8 @@ static void process_args(struct packet_reader *request,
> continue;
> }
>
> - if (skip_prefix(arg, "packfile-uris ", &p)) {
> + if (data->allow_packfile_uris &&
> + skip_prefix(arg, "packfile-uris ", &p)) {
> string_list_split(&data->uri_protocols, p, ',', -1);
> continue;
> }
> @@ -1847,8 +1852,6 @@ int upload_pack_advertise(struct repository *r,
> get_upload_pack_config(r, &data);
>
> if (value) {
> - char *str = NULL;
> -
> strbuf_addstr(value, "shallow wait-for-done");
>
> if (data.allow_filter)
> @@ -1860,13 +1863,8 @@ int upload_pack_advertise(struct repository *r,
> if (data.allow_sideband_all)
> strbuf_addstr(value, " sideband-all");
>
> - if (!repo_config_get_string(r,
> - "uploadpack.blobpackfileuri",
> - &str) &&
> - str) {
> + if (data.allow_packfile_uris)
> strbuf_addstr(value, " packfile-uris");
> - free(str);
> - }
> }
>
> upload_pack_data_clear(&data);
^ permalink raw reply
* Re: [PATCH 0/4] some v2 capability advertisement cleanups
From: Junio C Hamano @ 2024-02-28 23:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240228224625.GA1158651@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> There are some small textual conflicts with the series I just posted in:
>
> https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/
>
> I'm happy to prepare this on top of that if it's easier.
Thanks for a heads-up.
The other one merged first and then this one does give the
following, which does not look like a huge deal.
diff --cc upload-pack.c
index 281bdf85c9,66f4de9d87..0000000000
--- i/upload-pack.c
+++ w/upload-pack.c
@@@ -113,7 -113,7 +113,8 @@@ struct upload_pack_data
unsigned done : 1; /* v2 only */
unsigned allow_ref_in_want : 1; /* v2 only */
unsigned allow_sideband_all : 1; /* v2 only */
+ unsigned seen_haves : 1; /* v2 only */
+ unsigned allow_packfile_uris : 1; /* v2 only */
unsigned advertise_sid : 1;
unsigned sent_capabilities : 1;
};
@@@ -1648,10 -1651,8 +1654,11 @@@ static void process_args(struct packet_
continue;
}
- if (skip_prefix(arg, "packfile-uris ", &p)) {
+ if (data->allow_packfile_uris &&
+ skip_prefix(arg, "packfile-uris ", &p)) {
+ if (data->uri_protocols.nr)
+ send_err_and_die(data,
+ "multiple packfile-uris lines forbidden");
string_list_split(&data->uri_protocols, p, ',', -1);
continue;
}
^ permalink raw reply
* Re: [PATCH v2 1/1] completion: dir-type optargs for am, format-patch
From: Rubén Justo @ 2024-02-29 0:04 UTC (permalink / raw)
To: Britton Kerin, git
In-Reply-To: <CAC4O8c9z4s4fFU6_h6ZRBnDhZyiTp3XR8j0DrARj+1SauLbQEQ@mail.gmail.com>
On Mon, Feb 12, 2024 at 13:52:53 -0900, Britton Kerin wrote:
> __git_complete_dir ()
> {
> local cur_="$cur"
>
> while test $# != 0; do
> case "$1" in
> --cur=*) cur_="${1##--cur=}" ;;
> *) return 1 ;;
> esac
> shift
> done
>
> # This rev-parse invocation amounts to a pwd which respects -C options
> local context_dir=$(__git rev-parse --show-toplevel
> --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
> [ -d "$context_dir" ] || return 1
>
> compopt -o noquote
>
> local IFS=$'\n'
> local unescaped_candidates=($(cd "$context_dir" 2>/dev/null &&
> compgen -d -S / -- "$cur_"))
> for ii in "${!unescaped_candidates[@]}"; do
> COMPREPLY[$ii]=$(printf "%q" "${unescaped_candidates[$ii]}")
> done
> }
>
> This one works for all weird characters that I've tried in bash 5.2 at
> least, and in frameworks that do their own escaping also (e.g.
> ble.sh). Since your advice so far was so good I thought I'd ask if
> there is anything obvious to you that is still wrong here?
> If not I guess what's left is special code to make it work better with
> old versions of bash. I'm a little sceptical that this is worth it
> since bash 5 is already 5 years old and it's only completion code
> we're talking about but I guess it could be done.
I don't think you need to dig too much into old Bash versions. If it
works with a recent one, it's a good start.
Have you considered adding some tests to t/t9902-completion.sh?
It is desirable to see some tests at least for __git_complete_dir.
Perhaps it would also help you to polish the function.
Sorry for the late response. I just found your message while reviewing
the topics in the 'What's cooking'.
^ permalink raw reply
* Re: [PATCH 0/4] some v2 capability advertisement cleanups
From: Jeff King @ 2024-02-29 0:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1q8wjgft.fsf@gitster.g>
On Wed, Feb 28, 2024 at 03:51:34PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > There are some small textual conflicts with the series I just posted in:
> >
> > https://lore.kernel.org/git/20240228223700.GA1157826@coredump.intra.peff.net/
> >
> > I'm happy to prepare this on top of that if it's easier.
>
> Thanks for a heads-up.
>
> The other one merged first and then this one does give the
> following, which does not look like a huge deal.
Yep, that matches the resolution I did locally.
-Peff
^ permalink raw reply
* Re: [PATCH] branch: adjust documentation
From: Dragan Simic @ 2024-02-29 1:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Rubén Justo
In-Reply-To: <xmqqttlsld4t.fsf@gitster.g>
On 2024-02-28 18:20, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> Just checking, do you see the changes that Ruben proposed in this
>> patch
>> fit for our starting point in the git-branch documentation rewrite?
First of all, I _really_ appreciate your highly detailed feedback.
Thank you very much for taking the time to write it all down!
> The synopsis part may, but with reservations. I know you two are
> aiming to make this in many small incremental steps, but even then
> I'd have to say the way these placeholder words will be used in the
> text part (both DESCRIPTION and OPTIONS) will be so different from
> the final shape envisioned [*1*], the wording that may fit well within
> the current text might not be the best fit in the final text.
Please, let me remind you that I've already agreed upon doing it all
in one fell swoop, which may consist of multiple patches in a series,
but applied as a whole.
> The current description section talks about option and its arguments
> without showing the syntax, making it unnecessary to be extra
> verbose. For example, we see something like this:
>
> With a `-m` or `-M` option, <oldbranch> will be renamed to
> <newbranch>. If <oldbranch> had a corresponding reflog, it is
> renamed to match ...
>
> But in the final shape of the documentation, I would like to see the
> description section not talk about these arguments, and would read
> more like
>
> When given `-m` or `-M` options, the command renames an existing
> branch to a different name.
>
> among short descriptions made at the conceptual level on other modes
> like "listing" mode, "delete" mode, etc. [*3*]
I agree on that being the final outcome, because the descriptions
of arguments actually belong to the envisioned "OPTIONS" section,
as part of the command descriptions.
> And the option description would become something like [*]:
>
> -m [<one>] <two>::
> Renames the branch <one> (the current branch is used when
> not given) to a new name <two>, together with its reflog and
> configuration settings for the branch. ...
>
> Side note: <one> and <two> are meta-placeholders for the purpose
> of this document; Ruben's patch proposes to call them <branch>
> and <new-branch>.
>
> Now in such a context, <one> and <two> placeholders having actually
> the word "branch" in it would sound redundant and awkward to read,
> Even though the choice of words Ruben made in the patch under
> discussion may work well in the current document structure. I
> suspect these words will have to be chosen again when we start
> making the real organizational changes to the description and
> options sections.
Well, perhaps it's the best to revisit the argument naming later.
> In other words, I do not think that the patch makes an effective
> "one good step in the right direction". At least, I couldn't see
> how the wording for placeholders the patch proposes to use would be
> helpful in deciding the placeholders used in the final version.
I see, thanks for the clarification.
> [Footnotes]
>
> *1* Do we share the vision on how the final version should look
> like? Here I am assuming "the final shape envisioned" is along
> the lines of [*2*], i.e.
>
> (1) trim descriptions to just enumerate different modes of
> operations with explanation on what they do at the
> conceptual level;
Yes, that's also how I see it. The "DESCRIPTION" sections should
end up describing only the available different modes.
> (2) make each entry in the options section self contained, by
> showing the option with their <argument>s, referring to
> them in the explanation text;
Agreed once again. The "OPTIONS" section should end up describing
the available options, together with their respective arguments, in
a way that doesn't require the reader to jump to other places in the
document to fully understand each of the options.
> (3) remove non-option <argument> from the options list.
Yes, that goes along with the descriptions being self-contained.
> *2* https://lore.kernel.org/git/xmqqttmmlahf.fsf@gitster.g/
Sure, I already went through your message linked above, which already
described it quite well.
> *3* Because "git branch" does so many things, the DESCRIPTION
> section should first say the purpose of the section is to serve
> as brief catalog of features to help readers to find the entry
> in the option section to jump to quickly. Something like:
Yup, as I already explained it above. To reiterate, the purpose of
the "DESCRIPTION" section is to let the reader know what git-branch
can do, in form of easily readable prose.
> The "git branch" command works in many modes (see SYNOPSIS
> above). By default the command works in the "list" option (the
> `--list` option explicitly asks for this mode).
>
> will be at the beginning of the section. The first four
> paragraphs in the current DESCRIPTION section is about this
> list mode. The first three of them should probably be moved to
> the OPTIONS section under `--list`. The fourth paragraph
> should be split and described in the entries of individual
> options it talks about in the OPTIONS section.
Agreed, once again. :) Most of the prose currently found in the
"DESCRIPTION" section should actually be moved to the various command
descriptions in the envisioned future "OPTIONS" section.
^ permalink raw reply
* Re: [PATCH v2 1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Josh Triplett @ 2024-02-29 4:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzmpu7k6.fsf@gitster.g>
On Tue, Feb 27, 2024 at 09:43:21AM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index ea13f5d8db..2d576f7a44 100644
>
> I do not seem to have the preimage ea13f5d8db; as a bugfix patch, it
> would be preferrable to make the patches not to depend on anything
> in flight. If feasible, it may even be nicer to base them on one of
> the maintenance tracks.
>
> I managed to wiggle the patch in (somehow a context line was
> misindented), so there hopefully is no need to resend.
Sorry about that. I had these two patches in the same branch as my other recent patch
`advice: Add advice.scissors to suppress "do not modify or remove this line"`
but the two ended up independent so I didn't send them as a series. That
patch and this two-patch series should be applicable independently.
If you do end up needing a resend of any of them, I'm happy to do so.
^ permalink raw reply
* [Outreachy][PATCH v2 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string
From: Achu Luma @ 2024-02-29 5:40 UTC (permalink / raw)
To: git; +Cc: chriscool, christian.couder, gitster, Achu Luma
In-Reply-To: <20240226143350.3596-1-ach.lumap@gmail.com>
In a following commit we are going to port code from
"t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
a new "t/unit-tests/t-hash.c" file using the recently added unit test
framework.
To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
we are going to need a new strbuf_addstrings() function that repeatedly
adds the same string a number of times to a buffer.
Such a strbuf_addstrings() function would already be useful in
"json-writer.c" and "builtin/submodule-helper.c" as both of these files
already have code that repeatedly adds the same string. So let's
introduce such a strbuf_addstrings() function in "strbuf.{c,h}" and use
it in both "json-writer.c" and "builtin/submodule-helper.c".
We use the "strbuf_addstrings" name as this way strbuf_addstr() and
strbuf_addstrings() would be similar for strings as strbuf_addch() and
strbuf_addchars() for characters.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
The changes between version 1 and version 2 are the following:
- Remove memcpy() that involves manual offset computation
- Use strbuf_add() repeatedly after calling strbuf_grow() once to avoid
repeated allocation.
Thanks to Junio for pointing this out.
Here is a diff between v1 and v2:
- size_t len = strlen(s);
- if (unsigned_mult_overflows(len, n))
- die("you want to use way too much memory");
- strbuf_grow(sb, len * n);
- for (size_t i = 0; i < n; i++)
- memcpy(sb->buf + sb->len + len * i, s, len);
- strbuf_setlen(sb, sb->len + len * n);
+ size_t len = strlen(s);
+
+ if (unsigned_mult_overflows(len, n))
+ die("you want to use way too much memory");
+ strbuf_grow(sb, len * n);
+ for (size_t i = 0; i < n; i++)
+ strbuf_add(sb, s, len);
builtin/submodule--helper.c | 4 +---
json-writer.c | 5 +----
strbuf.c | 10 ++++++++++
strbuf.h | 5 +++++
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1..bed08af410 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -257,11 +257,9 @@ static void module_list_active(struct module_list *list)
static char *get_up_path(const char *path)
{
- int i;
struct strbuf sb = STRBUF_INIT;
- for (i = count_slashes(path); i; i--)
- strbuf_addstr(&sb, "../");
+ strbuf_addstrings(&sb, "../", count_slashes(path));
/*
* Check if 'path' ends with slash or not
diff --git a/json-writer.c b/json-writer.c
index 005c820aa4..25b9201f9c 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -46,10 +46,7 @@ static void append_quoted_string(struct strbuf *out, const char *in)
static void indent_pretty(struct json_writer *jw)
{
- int k;
-
- for (k = 0; k < jw->open_stack.len; k++)
- strbuf_addstr(&jw->json, " ");
+ strbuf_addstrings(&jw->json, " ", jw->open_stack.len);
}
/*
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..f4282b70ad 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -302,6 +302,16 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
strbuf_setlen(sb, sb->len + len);
}
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
+{
+ size_t len = strlen(s);
+
+ if (unsigned_mult_overflows(len, n))
+ die("you want to use way too much memory");
+ strbuf_grow(sb, len * n);
+ for (size_t i = 0; i < n; i++)
+ strbuf_add(sb, s, len);
+}
+
void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
{
strbuf_grow(sb, sb2->len);
diff --git a/strbuf.h b/strbuf.h
index e959caca87..0fb1b5e81e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
strbuf_add(sb, s, strlen(s));
}
+/**
+ * Add a NUL-terminated string the specified number of times to the buffer.
+ */
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
+
/**
* Copy the contents of another buffer at the end of the current one.
*/
--
2.43.0.windows.1
^ permalink raw reply related
* [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c
From: Achu Luma @ 2024-02-29 5:40 UTC (permalink / raw)
To: git; +Cc: chriscool, christian.couder, gitster, Achu Luma
In-Reply-To: <20240229054004.3807-1-ach.lumap@gmail.com>
In the recent codebase update (8bf6fbd (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for hash functionality from the
legacy approach using the test-tool command `test-tool sha1`and
`test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
new unit testing framework (t/unit-tests/test-lib.h). Porting
t0013-sha1dc.sh is left for later.
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
The change between version 1 and version 2 is:
- Deleted t/helper/test-sha256.c
Here is a diff between v1 and v2:
-#include "test-tool.h"
-#include "hash-ll.h"
-
-int cmd__sha256(int ac, const char **av)
-{
- return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
-}
Makefile | 2 +-
t/helper/test-sha256.c | 7 ----
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0015-hash.sh | 56 -------------------------------
t/unit-tests/t-hash.c | 75 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 76 insertions(+), 66 deletions(-)
delete mode 100644 t/helper/test-sha256.c
delete mode 100755 t/t0015-hash.sh
create mode 100644 t/unit-tests/t-hash.c
diff --git a/Makefile b/Makefile
index 4e255c81f2..c85f24f813 100644
--- a/Makefile
+++ b/Makefile
@@ -847,7 +847,6 @@ TEST_BUILTINS_OBJS += test-run-command.o
TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
TEST_BUILTINS_OBJS += test-serve-v2.o
TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
TEST_BUILTINS_OBJS += test-sigchain.o
TEST_BUILTINS_OBJS += test-simple-ipc.o
TEST_BUILTINS_OBJS += test-strcmp-offset.o
@@ -1347,6 +1346,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGRAMS += t-ctype
UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-hash
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-sha256.c b/t/helper/test-sha256.c
deleted file mode 100644
index 08cf149001..0000000000
--- a/t/helper/test-sha256.c
+++ /dev/null
@@ -1,7 +0,0 @@
-#include "test-tool.h"
-#include "hash-ll.h"
-
-int cmd__sha256(int ac, const char **av)
-{
- return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4..7bfbb75c9b 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
{ "serve-v2", cmd__serve_v2 },
{ "sha1", cmd__sha1 },
{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
- { "sha256", cmd__sha256 },
{ "sigchain", cmd__sigchain },
{ "simple-ipc", cmd__simple_ipc },
{ "strcmp-offset", cmd__strcmp_offset },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf5..8139c9664d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
int cmd__sha1(int argc, const char **argv);
int cmd__sha1_is_sha1dc(int argc, const char **argv);
int cmd__oid_array(int argc, const char **argv);
-int cmd__sha256(int argc, const char **argv);
int cmd__sigchain(int argc, const char **argv);
int cmd__simple_ipc(int argc, const char **argv);
int cmd__strcmp_offset(int argc, const char **argv);
diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 0a087a1983..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,56 +0,0 @@
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-1 hash values' '
- test-tool sha1 </dev/null >actual &&
- grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
- printf "a" | test-tool sha1 >actual &&
- grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
- printf "abc" | test-tool sha1 >actual &&
- grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
- printf "message digest" | test-tool sha1 >actual &&
- grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
- printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
- grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
- perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
- test-tool sha1 >actual &&
- grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
- printf "blob 0\0" | test-tool sha1 >actual &&
- grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
- printf "blob 3\0abc" | test-tool sha1 >actual &&
- grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
- printf "tree 0\0" | test-tool sha1 >actual &&
- grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
-'
-
-test_expect_success 'test basic SHA-256 hash values' '
- test-tool sha256 </dev/null >actual &&
- grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
- printf "a" | test-tool sha256 >actual &&
- grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
- printf "abc" | test-tool sha256 >actual &&
- grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
- printf "message digest" | test-tool sha256 >actual &&
- grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
- printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
- grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
- # Try to exercise the chunking code by turning autoflush on.
- perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
- test-tool sha256 >actual &&
- grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
- perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
- test-tool sha256 >actual &&
- grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
- printf "blob 0\0" | test-tool sha256 >actual &&
- grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
- printf "blob 3\0abc" | test-tool sha256 >actual &&
- grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
- printf "tree 0\0" | test-tool sha256 >actual &&
- grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
new file mode 100644
index 0000000000..b512e64bf1
--- /dev/null
+++ b/t/unit-tests/t-hash.c
@@ -0,0 +1,75 @@
+#include "test-lib.h"
+#include "hash-ll.h"
+#include "hex.h"
+#include "strbuf.h"
+
+static void check_hash_data(const void *data, size_t data_length, const char *expected, int algo) {
+ git_hash_ctx ctx;
+ unsigned char hash[GIT_MAX_HEXSZ];
+ const struct git_hash_algo *algop = &hash_algos[algo];
+
+ if (!check(!!data)) {
+ test_msg("Error: No data provided when expecting: %s", expected);
+ return;
+ }
+
+ algop->init_fn(&ctx);
+ algop->update_fn(&ctx, data, data_length);
+ algop->final_fn(hash, &ctx);
+
+ check_str(hash_to_hex_algop(hash, algop), expected);
+}
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA1_STR(data, expected) \
+ TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
+ "SHA1 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA1_LITERAL(literal, expected) \
+ TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
+ "SHA1 (%s) works", #literal)
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA256_STR(data, expected) \
+ TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+ "SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+ TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+ "SHA256 (%s) works", #literal)
+
+int cmd_main(int argc, const char **argv) {
+ struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+ struct strbuf alphabet_100000 = STRBUF_INIT;
+
+ strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+ strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
+
+ TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
+ TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
+ TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
+ TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
+ TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
+ TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
+ TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
+ TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
+ TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
+
+ TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+ TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+ TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+ TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+ TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+ TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+ TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+ TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+ TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+ TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
+ strbuf_release(&aaaaaaaaaa_100000);
+ strbuf_release(&alphabet_100000);
+
+ return test_done();
+}
--
2.43.0.windows.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Junio C Hamano @ 2024-02-29 5:41 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
In-Reply-To: <ZeAFutaddf4M2wjM@localhost>
Josh Triplett <josh@joshtriplett.org> writes:
> If you do end up needing a resend of any of them, I'm happy to do so.
I do not think there is need for resending, but I think you promised
to add some tests earlier, so an updated patch may be in order ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] upload-pack: only accept packfile-uris if we advertised it
From: Jeff King @ 2024-02-29 5:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20240228225050.GA1159078@coredump.intra.peff.net>
On Wed, Feb 28, 2024 at 05:50:50PM -0500, Jeff King wrote:
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 6ef4971845..902e42c1c0 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -778,6 +778,24 @@ test_expect_success 'archive with custom path does not request v2' '
> ! grep ^GIT_PROTOCOL env.trace
> '
>
> +test_expect_success 'reject client packfile-uris if not advertised' '
> + {
> + packetize command=fetch &&
> + printf 0001 &&
> + packetize packfile-uris https &&
> + packetize done &&
> + printf 0000
> + } >input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git upload-pack client <input &&
> + test_must_fail env GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri \
> + upload-pack client <input &&
> + GIT_PROTOCOL=version=2 \
> + git -c uploadpack.blobpackfileuri=anything \
> + upload-pack client <input
> +'
Sorry, this needs one tweak to pass under the sha256 CI job:
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 902e42c1c0..1ef540f73d 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -781,6 +781,7 @@ test_expect_success 'archive with custom path does not request v2' '
test_expect_success 'reject client packfile-uris if not advertised' '
{
packetize command=fetch &&
+ packetize object-format=$(test_oid algo) &&
printf 0001 &&
packetize packfile-uris https &&
packetize done &&
Otherwise the server complains that the other side did not respect its
advertised object-format (I sure am glad to have included the final
"hey, this input works, right?" test there, as that is what caught it).
-Peff
^ permalink raw reply related
* Re: [PATCH v2 1/2] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Josh Triplett @ 2024-02-29 6:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqttlrj08t.fsf@gitster.g>
On Wed, Feb 28, 2024 at 09:41:22PM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> > If you do end up needing a resend of any of them, I'm happy to do so.
>
> I do not think there is need for resending, but I think you promised
> to add some tests earlier, so an updated patch may be in order ;-)
I did add a test; v2 that you replied to has this:
> Add a test for this.
[...]
> t/t7502-commit-porcelain.sh | 5 +++++
[...]
> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index a87c211d0b..b37e2018a7 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -736,6 +736,11 @@ test_expect_success 'message shows date when it is explicitly set' '
> .git/COMMIT_EDITMSG
> '
>
> +test_expect_success 'message does not have multiple scissors lines' '
> + git commit --cleanup=scissors -v --allow-empty -e -m foo &&
> + test $(grep -c -e "--- >8 ---" .git/COMMIT_EDITMSG) -eq 1
> +'
> +
> test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
>
> echo >>negative &&
https://lore.kernel.org/git/xmqqedcxvnn8.fsf@gitster.g/T/#Z2e.:..:4f97933f173220544a5be2bf05c2bee2b044d2b1.1709024540.git.josh::40joshtriplett.org:1t:t7502-commit-porcelain.sh
^ permalink raw reply
* Re: [PATCH v4 04/11] commit-reach(paint_down_to_common): prepare for handling shallow commits
From: Johannes Schindelin @ 2024-02-29 9:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt,
Dirk Gouders
In-Reply-To: <xmqqedcwjq04.fsf@gitster.g>
Hi Junio,
On Wed, 28 Feb 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When `git fetch --update-shallow` needs to test for commit ancestry, it
> > can naturally run into a missing object (e.g. if it is a parent of a
> > shallow commit). For the purpose of `--update-shallow`, this needs to be
> > treated as if the child commit did not even have that parent, i.e. the
> > commit history needs to be clamped.
> >
> > For all other scenarios, clamping the commit history is actually a bug,
> > as it would hide repository corruption (for an analysis regarding
> > shallow and partial clones, see the analysis further down).
> >
> > Add a flag to optionally ask the function to ignore missing commits, as
> > `--update-shallow` needs it to, while detecting missing objects as a
> > repository corruption error by default.
> >
> > This flag is needed, and cannot replaced by `is_repository_shallow()` to
Hrmpf. I just spotted the missing "be" between "cannot" and "replaced".
Junio, would you kindly amend the commit message accordingly?
> > indicate that situation, because that function would return 0 in the
> > `--update-shallow` scenario: There is not actually a `shallow` file in
> > that scenario, as demonstrated e.g. by t5537.10 ("add new shallow root
> > with receive.updateshallow on") and t5538.4 ("add new shallow root with
> > receive.updateshallow on").
>
> Nicely written.
>
> The description above that has been totally revamped reads much much
> clearer, at least to me, compared to the previous round.
Thank you!
> Should we declare the topic done and mark it for 'next'?
After you looked over the correctness of the patches, I would be
comfortable with that.
Thanks!
Johannes
^ permalink raw reply
* Re: [PATCH v4 04/11] commit-reach(paint_down_to_common): prepare for handling shallow commits
From: Johannes Schindelin @ 2024-02-29 9:54 UTC (permalink / raw)
To: Dirk Gouders
Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git,
Patrick Steinhardt
In-Reply-To: <gh7cioqp8p.fsf@gouders.net>
Hi Dirk,
On Wed, 28 Feb 2024, Dirk Gouders wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >>
> >> When `git fetch --update-shallow` needs to test for commit ancestry, it
> >> can naturally run into a missing object (e.g. if it is a parent of a
> >> shallow commit). For the purpose of `--update-shallow`, this needs to be
> >> treated as if the child commit did not even have that parent, i.e. the
> >> commit history needs to be clamped.
> >>
> >> For all other scenarios, clamping the commit history is actually a bug,
> >> as it would hide repository corruption (for an analysis regarding
> >> shallow and partial clones, see the analysis further down).
> >>
> >> Add a flag to optionally ask the function to ignore missing commits, as
> >> `--update-shallow` needs it to, while detecting missing objects as a
> >> repository corruption error by default.
> >>
> >> This flag is needed, and cannot replaced by `is_repository_shallow()` to
> >> indicate that situation, because that function would return 0 in the
> >> `--update-shallow` scenario: There is not actually a `shallow` file in
> >> that scenario, as demonstrated e.g. by t5537.10 ("add new shallow root
> >> with receive.updateshallow on") and t5538.4 ("add new shallow root with
> >> receive.updateshallow on").
> >
> > Nicely written.
> >
> > The description above that has been totally revamped reads much much
> > clearer, at least to me, compared to the previous round.
> >
> > Should we declare the topic done and mark it for 'next'?
> >
> > Thanks.
>
> I agree that this text reads much clearer -- even to me with close to
> zero experience, here.
>
> Thank you for taking the time to rewrite the text, Johannes.
Thank _you_ for taking the time to review the patches and help me with
improving them!
Ciao,
Johannes
^ permalink raw reply
* Re: [RFD] should "git log --graph -g" work and if so how?
From: Oswald Buddenhagen @ 2024-02-29 10:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7c5n0ob.fsf@gitster.g>
On Sat, Feb 24, 2024 at 11:04:52AM -0800, Junio C Hamano wrote:
>Now, if I could run
>
> $ git log --oneline --graph -g --since=2024-02-20 --boundary
>
>on the result, such a history might look like this:
>
> * snapshot as of 2024-02-24 (HEAD)
> | * snapshot as of 2024-02-23 (HEAD@{1})
> |/
> | * snapshot as of 2024-02-22 (HEAD@{2})
> |/
> * add 'bar' (HEAD~1)
> o add 'foo' (HEAD~2)
>
>to show the same history.
>
>Unfortunately, "--graph" and "-g" does not mix X-<.
>
>So, the RFD is,
>
> (1) Should "git log" learn a trick to show a history like this in a
> readable way? Does it have utility outside this use case of
> mine? I am not interested in adding a new feature just for
> myself ;-)
>
i'm not sure i fully understand your use case; i failed to extract the
conceptual requirements from your description.
but as a "heavy revisionist", i would appreciate it very much if there
was a convenient way to list and diff revisions of the same logical
commit (ideally omitting empty rebases). sort of like a range-diff on
steroids.
this would certainly require correlating the reflog with some stable
commit ids, like gerrit and jj maintain.
^ permalink raw reply
* [GSOC][PATCH] userdiff: add builtin patterns for JavaScript.
From: Sergius Nyah @ 2024-02-29 10:11 UTC (permalink / raw)
To: git, christian.couder, gitster; +Cc: pk, shyamthakkar001, Sergius Nyah
In-Reply-To: <xmqqttlsjvsi.fsf@gitster.g>
This commit introduces builtin patterns for JavaScript in userdiff.
It adds a new test case in t4018-diff-funcname.sh to verify the enhanced
JavaScript function detection in Git diffs.
Signed-off-by: Sergius Justus Chesami Nyah <sergiusnyah@gmail.com>
---
userdiff.c | 17 +++++++++++++++--
t/t4018-diff-funcname.sh | 25 ++++++++-
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index e399543823..12e31ff14d 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -18,40 +16,19 @@
#include "git-compat-util.h"
#include "config.h"
#include "userdiff.h"
#include "strbuf.h"
PATTERNS("javascript",
/* Looks for lines that start with optional whitespace, followed
* by 'function'* and any characters (for function declarations),
* or valid JavaScript identifiers, equals sign '=', 'function' keyword
* and any characters (for function expressions).
* Also considers functions defined inside blocks with '{...}'.
*/
"^[ \t]*(function[ \t]*.*|[a-zA-Z_$][0-9a-zA-Z_$]*[ \t]*=[ \t]*function[ \t]*.*|(\\{[ \t]*)?)\n",
/* This pattern matches JavaScript identifiers */
"[a-zA-Z_$][0-9a-zA-Z_$]*"
"|[-+0-9.eE]+|0[xX][0-9a-fA-F]+"
"|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\|"),
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 43593866bc..9c3b80665e 100644
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -18,40 +16,19 @@
test_expect_success 'identify builtin patterns in Javascript' '
# setup
echo "function myFunction() { return true; }" > test.js &&
echo "var myVar = function() { return false; }" >> test.js &&
git add test.js &&
git commit -m "add test.js" &&
# modify the file
echo "function myFunction() { return false; }" > test.js &&
echo "var myVar = function() { return true; }" >> test.js &&
# command under test
git diff >output &&
# check results
test_i18ngrep "function myFunction() { return true; }" output &&
test_i18ngrep "function myFunction() { return false; }" output &&
test_i18ngrep "var myVar = function() { return false; }" output &&
test_i18ngrep "var myVar = function() { return true; }" output
'
test_done
--
2.43.2
^ 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