* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Junio C Hamano @ 2023-11-09 23:33 UTC (permalink / raw)
To: Taylor Blau, Johannes Schindelin; +Cc: Jeff King, git
In-Reply-To: <ZUzcmsfJe6jk4fTk@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
>> * tb/merge-tree-write-pack (2023-10-23) 5 commits
> ...
> This series received a couple of LGTMs from you and Patrick:
>
> - https://lore.kernel.org/git/xmqqo7go7w63.fsf@gitster.g/#t
> - https://lore.kernel.org/git/ZTjKmcV5c_EFuoGo@tanuki/
Yup, I am aware of them.
> Johannes had posted some comments[1] about instead using a temporary
> object store where objects are written as loose that would extend to git
> replay....
I was hoping to hear from Johannes saying he agrees with the above.
It is not strictly required, but is much nice to have once we hear
"let's step back a bit---are we going in the right direction?" and
it has been responded.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash
From: Junio C Hamano @ 2023-11-09 23:27 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <xmqqsf5mgl0r.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Thanks. The compiled result from this version looks quite good. As
> you started a new round with typofix, let me start the final
> nitpicking.
> ...
> Here is what I ended up queuing.
>
> Thanks.
Any comments on the "counterproposal to fix nitpicks"? I'd like to
make sure the original author is either OK with the update (in which
case I can just move what I have in my tree forward) or unhappy with
it and plan to send in an update on their own (in which case I can
wait for the next iteration), in a case like this.
Thanks.
^ permalink raw reply
* Re: [PATCH v10 1/3] unit tests: Add a project plan document
From: Junio C Hamano @ 2023-11-09 23:15 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <f706ba9b682e7c4070d49086ad3af582bf269c79.1699555664.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions). Describe what we hope to accomplish by
> implementing unit tests, and explain some open questions and milestones.
> Discuss desired features for test frameworks/harnesses, and provide a
> comparison of several different frameworks. Finally, document our
> rationale for implementing a custom framework.
>
> Co-authored-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
> Documentation/Makefile | 1 +
> Documentation/technical/unit-tests.txt | 240 +++++++++++++++++++++++++
> 2 files changed, 241 insertions(+)
> create mode 100644 Documentation/technical/unit-tests.txt
Looks good. I'll downcase "Add" on the title to match what I have
in my tree, but otherwise it looks OK to me. Let's see if we can
mark this round ready for 'next' and check if we hear complaints.
I have to make sure I do not forget about the other topic that
builds on top of this one.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Junio C Hamano @ 2023-11-09 23:02 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
In-Reply-To: <20231109185515.GD2711684@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
>> process_subtree_split_trailer () {
>> - assert test $# = 2 -o $# = 3
>> + assert test $# -ge 2
>> + assert test $# -le 3
>
> It took me a minute to figure out why we were swapping "=" for "-ge". It
> is because we want to logical-OR the two conditions, but "assert"
> requires that we test one at a time. I think that is probably worth
> explaining in the commit message.
I wish we could write something like
assert test $# -ge 2 && test $# -le 3
(and I'd allow double quoting the whole thing after assert if
needed) but we cannot do so without tweaking the implementation of
assert.
>
>> @@ -916,7 +919,7 @@ cmd_split () {
>> if test $# -eq 0
>> then
>> rev=$(git rev-parse HEAD)
>> - elif test $# -eq 1 -o $# -eq 2
>> + elif test $# -eq 1 || test $# -eq 2
>
> OK, this one is a straight-forward use of "||".
Yes, but why not consistently use the range notation like the
earlier one here, or below?
elif test $# -ge 1 && test $# -le 2
>> cmd_merge () {
>> - test $# -eq 1 -o $# -eq 2 ||
>> + if test $# -lt 1 || test $# -gt 2
>> ...
> (I am OK with either, it just took me a minute to verify that your
> conversion was correct. But that is a one-time issue now while
> reviewing, and I think the code is readable going forward).
Yeah, the end result looks good.
Thanks, both.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-09 23:00 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109180308.GA2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]
Hi Jeff,
On Thu, Nov 09, 2023 at 01:03:08PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
>
> > I've tried something even simpler:
> >
> > ---8<---
> > #!/bin/sh
> >
> > mutt -H -;
> > --->8---
> >
> > I used it for sending a couple of patches to linux-man@, and it seems to
> > work. I don't have much experience with mutt, so maybe I'm missing some
> > corner cases. Do you expect it to not work for some case? Otherwise,
> > we might have a winner. :)
>
> Wow, I don't know how I missed that when I read the manual. That was
> exactly the feature I was thinking that mutt would need. ;)
>
> So yeah, that is obviously better than the "postponed" hackery I showed.
> I notice that "-H" even causes mutt to ignore "-i" (a sendmail flag that
> Git adds to sendemail.sendmailcmd). So you can just invoke it directly
> from your config like:
>
> git config sendemail.sendmailcmd "mutt -H -"
Hmm great then! Definitely a winner. :)
>
> Annoyingly, "-E" doesn't work when reading over stdin (I guess mutt
> isn't willing to re-open the tty itself). But if you're happy with not
> editing as they go through, then "-H" is then that's enough (in my
> workflow, I do the final proofread via mutt).
Since git-send-email allows editing, I usually edit with that. Having
-E would be redundant (and in fact it felt like that to me with your
suggested mutt-as-mta.sh) for my use case.
Cheers,
Alex
>
> -Peff
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-09 22:56 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
In-Reply-To: <20231109184843.GC2711684@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But IMHO it is worth getting rid of all -o/-a regardless. Even
> non-ambiguous cases make reasoning about the code harder, and we don't
> want to encourage people to think they're OK to use.
You're 100% correct. "We do not have to worry about it" is a very
strong incentive to go after.
>
>> I do not need a subshell for grouping, either. Plain {} should do
>> (but you may need a LF or semicolon after the statement)..
>
> This I definitely agree with. :)
;-).
^ permalink raw reply
* [PATCH 7/7] midx: read `OOFF` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `OOFF` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 17 +++--------------
t/t5319-multi-pack-index.sh | 1 -
2 files changed, 3 insertions(+), 15 deletions(-)
diff --git a/midx.c b/midx.c
index 66f34ed1a3..ca41748b74 100644
--- a/midx.c
+++ b/midx.c
@@ -86,19 +86,6 @@ static int midx_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int midx_read_object_offsets(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct multi_pack_index *m = data;
- m->chunk_object_offsets = chunk_start;
-
- if (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {
- error(_("multi-pack-index object offset chunk is the wrong size"));
- return 1;
- }
- return 0;
-}
-
struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)
{
struct multi_pack_index *m = NULL;
@@ -176,7 +163,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
if (pair_chunk_expect(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup,
m->hash_len, m->num_objects))
die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
- if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
+ if (pair_chunk_expect(cf, MIDX_CHUNKID_OBJECTOFFSETS,
+ &m->chunk_object_offsets, MIDX_CHUNK_OFFSET_WIDTH,
+ m->num_objects))
die(_("multi-pack-index required object offsets chunk missing or corrupted"));
pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 2d68616c59..f1f6764efe 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1111,7 +1111,6 @@ test_expect_success 'reader notices too-small object offset chunk' '
corrupt_chunk OOFF clear 00000000 &&
test_must_fail git log 2>err &&
cat >expect <<-\EOF &&
- error: multi-pack-index object offset chunk is the wrong size
fatal: multi-pack-index required object offsets chunk missing or corrupted
EOF
test_cmp expect err
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 6/7] midx: read `OIDL` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `OIDL` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 16 ++--------------
t/t5319-multi-pack-index.sh | 1 -
2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/midx.c b/midx.c
index 1d14661dad..66f34ed1a3 100644
--- a/midx.c
+++ b/midx.c
@@ -86,19 +86,6 @@ static int midx_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int midx_read_oid_lookup(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct multi_pack_index *m = data;
- m->chunk_oid_lookup = chunk_start;
-
- if (chunk_size != st_mult(m->hash_len, m->num_objects)) {
- error(_("multi-pack-index OID lookup chunk is the wrong size"));
- return 1;
- }
- return 0;
-}
-
static int midx_read_object_offsets(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -186,7 +173,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
die(_("multi-pack-index required pack-name chunk missing or corrupted"));
if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))
die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
- if (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))
+ if (pair_chunk_expect(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup,
+ m->hash_len, m->num_objects))
die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
die(_("multi-pack-index required object offsets chunk missing or corrupted"));
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 313496c0cf..2d68616c59 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -1077,7 +1077,6 @@ test_expect_success 'reader notices too-small oid lookup chunk' '
corrupt_chunk OIDL clear 00000000 &&
test_must_fail git log 2>err &&
cat >expect <<-\EOF &&
- error: multi-pack-index OID lookup chunk is the wrong size
fatal: multi-pack-index required OID lookup chunk missing or corrupted
EOF
test_cmp expect err
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 5/7] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `BIDX` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 3b456d199f..76d220508b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -299,18 +299,6 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_bloom_index(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size / 4 != g->num_commits) {
- warning(_("commit-graph changed-path index chunk is too small"));
- return -1;
- }
- g->chunk_bloom_indexes = chunk_start;
- return 0;
-}
-
static int graph_read_bloom_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -437,8 +425,11 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
}
if (s->commit_graph_read_changed_paths) {
- read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
- graph_read_bloom_index, graph);
+ int res = pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,
+ &graph->chunk_bloom_indexes,
+ sizeof(uint32_t), graph->num_commits);
+ if (res && res != CHUNK_NOT_FOUND)
+ warning(_("commit-graph changed-path index chunk is too small"));
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
graph_read_bloom_data, graph);
}
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 4/7] commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `GDAT` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 67ab0f2448..3b456d199f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_generation_data(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size / sizeof(uint32_t) != g->num_commits)
- return error(_("commit-graph generations chunk is wrong size"));
- g->chunk_generation_data = chunk_start;
- return 0;
-}
-
static int graph_read_bloom_index(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -433,8 +423,11 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
&graph->chunk_base_graphs_size);
if (s->commit_graph_generation_version >= 2) {
- read_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,
- graph_read_generation_data, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_GENERATION_DATA,
+ &graph->chunk_generation_data,
+ sizeof(uint32_t),
+ graph->num_commits))
+ error(_("commit-graph generations chunk is wrong size"));
pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
&graph->chunk_generation_data_overflow,
&graph->chunk_generation_data_overflow_size);
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 3/7] commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `CDAT` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 15 +++------------
t/t5318-commit-graph.sh | 3 +--
2 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 6072c2a17f..67ab0f2448 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_commit_data(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
- return error(_("commit-graph commit data chunk is wrong size"));
- g->chunk_commit_data = chunk_start;
- return 0;
-}
-
static int graph_read_generation_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -431,8 +421,9 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
error(_("commit-graph OID lookup chunk is the wrong size"));
goto free_and_return;
}
- if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {
- error(_("commit-graph required commit data chunk missing or corrupted"));
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data,
+ GRAPH_DATA_WIDTH, graph->num_commits)) {
+ error(_("commit-graph commit data chunk is wrong size"));
goto free_and_return;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index b3e8af018d..fd5165bd07 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -550,7 +550,7 @@ test_expect_success 'detect missing OID lookup chunk' '
test_expect_success 'detect missing commit data chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \
- "commit-graph required commit data chunk missing or corrupted"
+ "commit-graph commit data chunk is wrong size"
'
test_expect_success 'detect incorrect fanout' '
@@ -875,7 +875,6 @@ test_expect_success 'reader notices too-small commit data chunk' '
check_corrupt_chunk CDAT clear 00000000 &&
cat >expect.err <<-\EOF &&
error: commit-graph commit data chunk is wrong size
- error: commit-graph required commit data chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 2/7] commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
The `OIDL` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
While here, clean up some of the duplicate error messages to simplify
the output when we are missing or have a corrupt OIDL chunk.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 16 ++++------------
t/t5318-commit-graph.sh | 3 +--
2 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 93cf690565..6072c2a17f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_oid_lookup(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- g->chunk_oid_lookup = chunk_start;
- if (chunk_size / g->hash_len != g->num_commits)
- return error(_("commit-graph OID lookup chunk is the wrong size"));
- return 0;
-}
-
static int graph_read_commit_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -435,8 +425,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
error(_("commit-graph required OID fanout chunk missing or corrupted"));
goto free_and_return;
}
- if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {
- error(_("commit-graph required OID lookup chunk missing or corrupted"));
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDLOOKUP,
+ &graph->chunk_oid_lookup, graph->hash_len,
+ graph->num_commits)) {
+ error(_("commit-graph OID lookup chunk is the wrong size"));
goto free_and_return;
}
if (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index b0d436a6f0..b3e8af018d 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -545,7 +545,7 @@ test_expect_success 'detect missing OID fanout chunk' '
test_expect_success 'detect missing OID lookup chunk' '
corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \
- "commit-graph required OID lookup chunk missing or corrupted"
+ "commit-graph OID lookup chunk is the wrong size"
'
test_expect_success 'detect missing commit data chunk' '
@@ -851,7 +851,6 @@ test_expect_success 'reader notices fanout/lookup table mismatch' '
check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
cat >expect.err <<-\EOF &&
error: commit-graph OID lookup chunk is the wrong size
- error: commit-graph required OID lookup chunk missing or corrupted
EOF
test_cmp expect.err err
'
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699569246.git.me@ttaylorr.com>
In 570b8b8836 (chunk-format: note that pair_chunk() is unsafe,
2023-10-09), the pair_chunk() interface grew a required "size" pointer,
so that the caller is forced to at least have a handle on the actual
size of the given chunk.
Many callers were converted to the new interface. A handful were instead
converted from the unsafe version of pair_chunk() to read_chunk() so
that they could check their expected size.
This led to a lot of code like:
static int graph_read_oid_lookup(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
g->chunk_oid_lookup = chunk_start;
if (chunk_size / g->hash_len != g->num_commits)
return error(_("commit-graph OID lookup chunk is the wrong size"));
return 0;
}
, leaving the caller to use read_chunk(), like so:
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
The callback to read_chunk() (in the above, `graph_read_oid_lookup()`)
does nothing more than (a) assign a pointer to the location of the start
of the chunk in the mmap'd file, and (b) assert that it has the correct
size.
For callers that know the expected size of their chunk(s) up-front (most
often because they are made up of a known number of fixed-size records),
we can simplify this by teaching the chunk-format API itself to validate
the expected size for us.
This is wrapped in a new function, called `pair_chunk_expect()` which
takes a pair of "size_t"s (corresponding to the record size and count),
instead of a "size_t *", and validates that the given chunk matches the
expected size as given.
This will allow us to reduce the number of lines of code it takes to
perform these basic read_chunk() operations, by taking the above and
replacing it with something like:
if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDLOOKUP,
&graph->chunk_oid_lookup,
graph->hash_len, graph->num_commits))
error(_("commit-graph oid lookup chunk is wrong size"));
We will perform those transformations in the following commits.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
chunk-format.c | 29 +++++++++++++++++++++++++++++
chunk-format.h | 13 ++++++++++++-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/chunk-format.c b/chunk-format.c
index cdc7f39b70..be078dcca8 100644
--- a/chunk-format.c
+++ b/chunk-format.c
@@ -163,6 +163,10 @@ int read_table_of_contents(struct chunkfile *cf,
struct pair_chunk_data {
const unsigned char **p;
size_t *size;
+
+ /* for pair_chunk_expect() only */
+ size_t record_size;
+ size_t record_nr;
};
static int pair_chunk_fn(const unsigned char *chunk_start,
@@ -175,6 +179,17 @@ static int pair_chunk_fn(const unsigned char *chunk_start,
return 0;
}
+static int pair_chunk_expect_fn(const unsigned char *chunk_start,
+ size_t chunk_size,
+ void *data)
+{
+ struct pair_chunk_data *pcd = data;
+ if (chunk_size / pcd->record_size != pcd->record_nr)
+ return -1;
+ *pcd->p = chunk_start;
+ return 0;
+}
+
int pair_chunk(struct chunkfile *cf,
uint32_t chunk_id,
const unsigned char **p,
@@ -184,6 +199,20 @@ int pair_chunk(struct chunkfile *cf,
return read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);
}
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id,
+ const unsigned char **p,
+ size_t record_size,
+ size_t record_nr)
+{
+ struct pair_chunk_data pcd = {
+ .p = p,
+ .record_size = record_size,
+ .record_nr = record_nr,
+ };
+ return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);
+}
+
int read_chunk(struct chunkfile *cf,
uint32_t chunk_id,
chunk_read_fn fn,
diff --git a/chunk-format.h b/chunk-format.h
index 14b76180ef..10806d7a9a 100644
--- a/chunk-format.h
+++ b/chunk-format.h
@@ -17,7 +17,8 @@ struct chunkfile;
*
* If reading a file, use a NULL 'struct hashfile *' and then call
* read_table_of_contents(). Supply the memory-mapped data to the
- * pair_chunk() or read_chunk() methods, as appropriate.
+ * pair_chunk(), pair_chunk_expect(), or read_chunk() methods, as
+ * appropriate.
*
* DO NOT MIX THESE MODES. Use different 'struct chunkfile' instances
* for reading and writing.
@@ -54,6 +55,16 @@ int pair_chunk(struct chunkfile *cf,
const unsigned char **p,
size_t *size);
+/*
+ * Similar to 'pair_chunk', but used for callers who are reading a chunk
+ * with a known number of fixed-width records.
+ */
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id,
+ const unsigned char **p,
+ size_t record_size,
+ size_t record_nr);
+
typedef int (*chunk_read_fn)(const unsigned char *chunk_start,
size_t chunk_size, void *data);
/*
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply related
* [PATCH 0/7] chunk-format: introduce `pair_chunk_expect()`
From: Taylor Blau @ 2023-11-09 22:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
This series is based on the latest patches in what I am assuming will
become "jk/chunk-bounds", found here [1].
This is a rework of my earlier round of patches in [2], which introduces
a new `pair_chunk_expect()` to replace some of the trivial callbacks
given to `read_chunk()` that assert on the size of the expected chunk.
Let me know what you think, and thanks in advance for your review!
[1]: https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
[2]: https://lore.kernel.org/git/cover.1697225110.git.me@ttaylorr.com/
Taylor Blau (7):
chunk-format: introduce `pair_chunk_expect()` helper
commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
midx: read `OIDL` chunk with `pair_chunk_expect()`
midx: read `OOFF` chunk with `pair_chunk_expect()`
chunk-format.c | 29 ++++++++++++++++
chunk-format.h | 13 ++++++-
commit-graph.c | 67 ++++++++++---------------------------
midx.c | 33 +++---------------
t/t5318-commit-graph.sh | 6 ++--
t/t5319-multi-pack-index.sh | 2 --
6 files changed, 65 insertions(+), 85 deletions(-)
base-commit: 8be77c5de65442b331a28d63802c7a3b94a06c5a
prerequisite-patch-id: 507f1dfd74fae351883612048d334ed750db8b8c
prerequisite-patch-id: c5eef290abc1d28950b3ee8729ea86d2e1773027
prerequisite-patch-id: 0853baab4862833faee8ade3b1b63ee3aa406224
prerequisite-patch-id: 6dd32f90fd87aa92f7d0661414cdaab257bd14b7
prerequisite-patch-id: b0e1617c501a011c703605e59dd5eba89f8a3573
prerequisite-patch-id: 906426f565c470dc86d7e7379d25ecfbd2bc30ce
prerequisite-patch-id: 970cd79d1911bde36b88c340f001266d7e8d843b
prerequisite-patch-id: 083a24abc73a83345bd9e4ca714577990dd9b08b
prerequisite-patch-id: 210371a338dfe9b6f905d8821501aa9c235c8722
--
2.43.0.rc0.39.g44bd344727
^ permalink raw reply
* Re: [PATCH 6/9] commit-graph: use fanout value for graph size
From: Taylor Blau @ 2023-11-09 22:15 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109213813.GB2728242@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 04:38:13PM -0500, Jeff King wrote:
> Or by "duplicate" did you mean the two-line:
>
> > > + error: commit-graph OID lookup chunk is the wrong size
> > > + error: commit-graph required OID lookup chunk missing or corrupted
>
> output. That's because our chunk of the return value from read_chunk()
> is a bit lazy, and does not distinguish "missing chunk" (where we should
> write a string saying so) from errors produced by the callback (where a
> more specific error message has already been written). That's true of
> all of the read_chunk() callers, including the midx ones.
Yeah, I meant duplicate in the sense above.
> This is one of the ways that pair_chunk_expect() could do better without
> adding a lot of code, because it can check the return value from
> read_chunk(). It doesn't help the other cases (like OIDF) that still
> have to call read_chunk() themselves, though. Possibly read_chunk()
> should just take a flag to indicate that it should complain when the
> chunk is missing. And then callers could just do:
>
> if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)
> return -1; /* no need to complain; either our_callback() did, or
> read_chunk() itself */
We do return CHUNK_NOT_FOUND when we have a missing chunk, which we
could check for individually. But TBH, I don't find the first error all
that useful. In this instance, there's really only one way for the OIDL
chunk to be corrupt, which is that it has the wrong size.
And short of making the error much more robust, e.g.:
error: commit-graph OID lookup chunk is the wrong size (got: $X, wanted: $Y)
and dropping the generic "missing or corrupt" error, I think that just
the generic error is fine here.
> -Peff
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 6/9] commit-graph: use fanout value for graph size
From: Jeff King @ 2023-11-09 21:38 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZU1NNcP/ARxK6D89@nand.local>
On Thu, Nov 09, 2023 at 04:20:53PM -0500, Taylor Blau wrote:
> On Thu, Nov 09, 2023 at 02:24:35AM -0500, Jeff King wrote:
> > @@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
> > {
> > struct commit_graph *g = data;
> > g->chunk_oid_lookup = chunk_start;
> > - g->num_commits = chunk_size / g->hash_len;
> > + if (chunk_size / g->hash_len != g->num_commits)
> > + return error(_("commit-graph OID lookup chunk is the wrong size"));
> > return 0;
> > }
>
> My understanding is that you need this error message to come from
> graph_read_oid_lookup() in order to pass the "detect incorrect fanout
> final value" test, but I wish that we didn't have to, since having the
> more-or-less duplicate error messages in the latter "reader notices
> fanout/lookup table mismatch" is somewhat unfortunate.
I'm not sure what you mean here. We notice the problem in the reading
code, which is used in turn by the verify code. So both of these tests:
> > test_expect_success 'detect incorrect fanout final value' '
> > corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
> > - "oid table and fanout disagree on size"
> > + "OID lookup chunk is the wrong size"
> > '
> >
> > test_expect_success 'detect incorrect OID order' '
> > @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
> > test_expect_success 'reader notices fanout/lookup table mismatch' '
> > check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
> > cat >expect.err <<-\EOF &&
> > - error: commit-graph oid table and fanout disagree on size
> > + error: commit-graph OID lookup chunk is the wrong size
> > + error: commit-graph required OID lookup chunk missing or corrupted
> > EOF
> > test_cmp expect.err err
> > '
will see a message regardless of where we put it. Or by "duplicate" did
you mean the two-line:
> > + error: commit-graph OID lookup chunk is the wrong size
> > + error: commit-graph required OID lookup chunk missing or corrupted
output. That's because our chunk of the return value from read_chunk()
is a bit lazy, and does not distinguish "missing chunk" (where we should
write a string saying so) from errors produced by the callback (where a
more specific error message has already been written). That's true of
all of the read_chunk() callers, including the midx ones.
This is one of the ways that pair_chunk_expect() could do better without
adding a lot of code, because it can check the return value from
read_chunk(). It doesn't help the other cases (like OIDF) that still
have to call read_chunk() themselves, though. Possibly read_chunk()
should just take a flag to indicate that it should complain when the
chunk is missing. And then callers could just do:
if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)
return -1; /* no need to complain; either our_callback() did, or
read_chunk() itself */
-Peff
^ permalink raw reply
* Re: [PATCH 1/9] commit-graph: handle overflow in chunk_size checks
From: Jeff King @ 2023-11-09 21:27 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZU1LbV0/ciDdO1aD@nand.local>
On Thu, Nov 09, 2023 at 04:13:17PM -0500, Taylor Blau wrote:
> So everything in this patch makes sense and looks good to me. It does
> make me think about the pair_chunk_expect() function that I proposed
> elsewhere. I haven't yet read the rest of the series, so it may be a
> totally useless direction by the end of this series ;-).
Nope, it's not useless. But I do think it affects what we'd want the
interface to look like, and...
> But I wonder if the interface we want is something like:
>
> int pair_chunk_expect(struct chunkfile *cf, uint32_t chunk_id,
> const unsigned char **p,
> size_t record_size, size_t record_nr);
>
> So we can then grab the size of the chunk, divide it by "record_size"
> and ensure that end up with "record_nr" as a result.
...this is exactly the direction I was thinking it would go. So if you
got to the same place after reading my explanation, then hopefully
something went right. ;)
-Peff
^ permalink raw reply
* Re: [PATCH 0/9] some more chunk-file bounds-checks fixes
From: Taylor Blau @ 2023-11-09 21:22 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109070310.GA2697602@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:03:10AM -0500, Jeff King wrote:
> This is a follow-up to the series from:
>
> https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/
>
> which was merged to master as jk/chunk-bounds. There were a few issues
> left open by that series and its review:
>
> 1. the midx code didn't check fanout ordering
>
> 2. whether we needed to sprinkle some more st_mult() on it
>
> 3. improving some of the error messages (translations, some
> consistency, maybe more details)
>
> 4. possible refactoring with a pair_chunk_expect() API (Taylor posted
> a series in that direction, which is currently in limbo)
I read this series thoroughly and was very pleased with the result.
Thanks for patching these up.
I think that I am still of the mind that it would be useful to have some
kind of pair_chunk_expect() function, so I'll try and rebase/rewrite my
patches on top of your new ones here.
In the meantime, this series LGTM.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 6/9] commit-graph: use fanout value for graph size
From: Taylor Blau @ 2023-11-09 21:20 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109072435.GF2698043@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:24:35AM -0500, Jeff King wrote:
> @@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
> {
> struct commit_graph *g = data;
> g->chunk_oid_lookup = chunk_start;
> - g->num_commits = chunk_size / g->hash_len;
> + if (chunk_size / g->hash_len != g->num_commits)
> + return error(_("commit-graph OID lookup chunk is the wrong size"));
> return 0;
> }
My understanding is that you need this error message to come from
graph_read_oid_lookup() in order to pass the "detect incorrect fanout
final value" test, but I wish that we didn't have to, since having the
more-or-less duplicate error messages in the latter "reader notices
fanout/lookup table mismatch" is somewhat unfortunate.
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index affb959d64..8bd7fcefb5 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' '
>
> test_expect_success 'detect incorrect fanout final value' '
> corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
> - "oid table and fanout disagree on size"
> + "OID lookup chunk is the wrong size"
> '
>
> test_expect_success 'detect incorrect OID order' '
> @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '
> test_expect_success 'reader notices fanout/lookup table mismatch' '
> check_corrupt_chunk OIDF 1020 "FFFFFFFF" &&
> cat >expect.err <<-\EOF &&
> - error: commit-graph oid table and fanout disagree on size
> + error: commit-graph OID lookup chunk is the wrong size
> + error: commit-graph required OID lookup chunk missing or corrupted
> EOF
> test_cmp expect.err err
> '
> --
> 2.43.0.rc1.572.g273fc7bed6
>
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk
From: Taylor Blau @ 2023-11-09 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109071711.GE2698043@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:17:11AM -0500, Jeff King wrote:
> The code to read commit-graph files tries to read all of the required
> chunks, but doesn't abort if we can't find one (or if it's corrupted).
> It's only at the end of reading the file that we then do some sanity
> checks for NULL entries. But it's preferable to detect the errors and
> bail immediately, for a few reasons:
>
> 1. It's less error-prone. It's easy in the reader functions to flag an
> error but still end up setting some struct fields (an error I in
> fact made while working on this patch series).
>
> 2. It's safer. Since verifying some chunks depends on the values of
> other chunks, we may be depending on not-yet-verified data. I don't
> know offhand of any case where this can cause problems, but it's
> one less subtle thing to worry about in the reader code.
>
> 3. It prevents the user from seeing nonsense errors. If we're missing
> an OIDL chunk, then g->num_commits will be zero. And so we may
> complain that the size of our CDAT chunk (which should have a
> fixed-size record for each commit) is wrong unless it's also zero.
> But that's misleading; the problem is the missing OIDL chunk; the
> CDAT one might be fine!
>
> So let's just check the return value from read_chunk(). This is exactly
> how the midx chunk-reading code does it.
All very well explained. I hit that same snag as you did when I was
working on the few patches I proposed we put on top of your earlier
chunk-format hardening series.
I'm glad to see this getting cleaned up, and I'm very happy with the
post-image of this patch.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/9] commit-graph: handle overflow in chunk_size checks
From: Taylor Blau @ 2023-11-09 21:13 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231109070948.GA2698043@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:09:48AM -0500, Jeff King wrote:
> So instead, we can do a division like this:
>
> if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
>
> where there's no possibility of overflow. We do lose a little bit of
> precision; due to integer division truncation we'd allow up to an extra
> GRAPH_DATA_WIDTH-1 bytes of data in the chunk. That's OK. Our main goal
> here is making sure we don't have too _few_ bytes, which would cause an
> out-of-bounds read (we could actually replace our "!=" with "<", but I
> think it's worth being a little pedantic, as a large mismatch could be a
> sign of other problems).
This is wonderfully explained, and the patch below follows trivially
from what you wrote here.
So everything in this patch makes sense and looks good to me. It does
make me think about the pair_chunk_expect() function that I proposed
elsewhere. I haven't yet read the rest of the series, so it may be a
totally useless direction by the end of this series ;-).
But I wonder if the interface we want is something like:
int pair_chunk_expect(struct chunkfile *cf, uint32_t chunk_id,
const unsigned char **p,
size_t record_size, size_t record_nr);
So we can then grab the size of the chunk, divide it by "record_size"
and ensure that end up with "record_nr" as a result.
Again, this is perhaps totally useless by the end of your series, but
just having looked at the first patch I wonder if this is a productive
direction to consider...
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`
From: Taylor Blau @ 2023-11-09 19:26 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Elijah Newren, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <ZUszSs0CYoFV9YJ0@tanuki>
On Wed, Nov 08, 2023 at 08:05:46AM +0100, Patrick Steinhardt wrote:
> > @@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)
> > return ret;
> > }
> >
> > +int tmp_objdir_repack(struct tmp_objdir *t)
> > +{
> > + struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > + cmd.git_cmd = 1;
> > +
> > + strvec_pushl(&cmd.args, "repack", "-a", "-d", "-k", "-l", NULL);
> > + strvec_pushv(&cmd.env, tmp_objdir_env(t));
>
> I wonder what performance of this repack would be like in a large
> repository with many refs. Ideally, I would expect that the repacking
> performance should scale with the number of objects we have written into
> the temporary object directory. But in practice, the repack will need to
> compute reachability and thus also scales with the size of the repo
> itself, doesn't it?
Good question. We definitely do not want to be doing an all-into-one
repack as a consequence of running 'git replay' in a large repository
with lots of refs, objects, or both.
But since we push the result of calling `tmp_objdir_env(t)` into the
environment of the child process, we are only repacking the objects in
the temporary directory, not the main object store.
I have a test that verifies this is the case by making sure that in a
repository with some arbitrary set of pre-existing packs, that only one
pack is added to that set after running 'replay', and that the
pre-existing packs remain in place.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement
From: Jeff King @ 2023-11-09 18:56 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <7c54d9070fac15b8f0504251d920d0e1fc1fb1f4.1699526999.git.ps@pks.im>
On Thu, Nov 09, 2023 at 11:53:39AM +0100, Patrick Steinhardt wrote:
> The `subtree_for_commit ()` helper function asserts that the subtree
> identified by its parameters are either a commit or tree. This is done
> via the `-o` parameter of test, which is discouraged.
>
> Refactor the code to instead use a switch statement over the type.
> Despite being aligned with our coding guidelines, the resulting code is
> arguably also easier to read.
Yes, I'd agree that the result is much easier to follow.
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Jeff King @ 2023-11-09 18:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <b1ea45b8a8884d09ab070bb0f099834447d28938.1699526999.git.ps@pks.im>
On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
> Functions in git-subtree.sh all assert that they are being passed the
> correct number of arguments. In cases where we accept a variable number
> of arguments we assert this via a single call to `test` with `-o`, which
> is discouraged by our coding guidelines.
>
> Convert these cases to stop doing so.
OK. I think these ones really are safe, because they're only expanding
$#, but I agree with the principle to follow the guidelines.
> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> process_subtree_split_trailer () {
> - assert test $# = 2 -o $# = 3
> + assert test $# -ge 2
> + assert test $# -le 3
It took me a minute to figure out why we were swapping "=" for "-ge". It
is because we want to logical-OR the two conditions, but "assert"
requires that we test one at a time. I think that is probably worth
explaining in the commit message.
> @@ -916,7 +919,7 @@ cmd_split () {
> if test $# -eq 0
> then
> rev=$(git rev-parse HEAD)
> - elif test $# -eq 1 -o $# -eq 2
> + elif test $# -eq 1 || test $# -eq 2
OK, this one is a straight-forward use of "||".
> cmd_merge () {
> - test $# -eq 1 -o $# -eq 2 ||
> + if test $# -lt 1 || test $# -gt 2
> + then
> die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
> + fi
> +
But here we swap "-eq" for other operators. We have to because we went
from "||" to an "if". I think what you have here is correct, but you
could also write:
if ! { test $# -eq 1 || test $# -eq 2; }
(I am OK with either, it just took me a minute to verify that your
conversion was correct. But that is a one-time issue now while
reviewing, and I think the code is readable going forward).
-Peff
^ permalink raw reply
* [PATCH v10 3/3] ci: run unit tests in CI
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <cover.1699555664.git.steadmon@google.com>
Run unit tests in both Cirrus and GitHub CI. For sharded CI instances
(currently just Windows on GitHub), run only on the first shard. This is
OK while we have only a single unit test executable, but we may wish to
distribute tests more evenly when we add new unit tests in the future.
We may also want to add more status output in our unit test framework,
so that we can do similar post-processing as in
ci/lib.sh:handle_failed_tests().
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
.cirrus.yml | 2 +-
ci/run-build-and-tests.sh | 2 ++
ci/run-test-slice.sh | 5 +++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index 4860bebd32..b6280692d2 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -19,4 +19,4 @@ freebsd_12_task:
build_script:
- su git -c gmake
test_script:
- - su git -c 'gmake test'
+ - su git -c 'gmake DEFAULT_UNIT_TEST_TARGET=unit-tests-prove test unit-tests'
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2528f25e31..7a1466b868 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -50,6 +50,8 @@ if test -n "$run_tests"
then
group "Run tests" make test ||
handle_failed_tests
+ group "Run unit tests" \
+ make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
fi
check_unignored_build_artifacts
diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index a3c67956a8..ae8094382f 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -15,4 +15,9 @@ group "Run tests" make --quiet -C t T="$(cd t &&
tr '\n' ' ')" ||
handle_failed_tests
+# We only have one unit test at the moment, so run it in the first slice
+if [ "$1" == "0" ] ; then
+ group "Run unit tests" make --quiet -C t unit-tests-prove
+fi
+
check_unignored_build_artifacts
--
2.42.0.869.gea05f2083d-goog
^ 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