* [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
* [PATCH v10 2/3] unit tests: add TAP unit test framework
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>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
This patch contains an implementation for writing unit tests with TAP
output. Each test is a function that contains one or more checks. The
test is run with the TEST() macro and if any of the checks fail then the
test will fail. A complete program that tests STRBUF_INIT would look
like
#include "test-lib.h"
#include "strbuf.h"
static void t_static_init(void)
{
struct strbuf buf = STRBUF_INIT;
check_uint(buf.len, ==, 0);
check_uint(buf.alloc, ==, 0);
check_char(buf.buf[0], ==, '\0');
}
int main(void)
{
TEST(t_static_init(), "static initialization works);
return test_done();
}
The output of this program would be
ok 1 - static initialization works
1..1
If any of the checks in a test fail then they print a diagnostic message
to aid debugging and the test will be reported as failing. For example a
failing integer check would look like
# check "x >= 3" failed at my-test.c:102
# left: 2
# right: 3
not ok 1 - x is greater than or equal to three
There are a number of check functions implemented so far. check() checks
a boolean condition, check_int(), check_uint() and check_char() take two
values to compare and a comparison operator. check_str() will check if
two strings are equal. Custom checks are simple to implement as shown in
the comments above test_assert() in test-lib.h.
Tests can be skipped with test_skip() which can be supplied with a
reason for skipping which it will print. Tests can print diagnostic
messages with test_msg(). Checks that are known to fail can be wrapped
in TEST_TODO().
There are a couple of example test programs included in this
patch. t-basic.c implements some self-tests and demonstrates the
diagnostic output for failing test. The output of this program is
checked by t0080-unit-test-output.sh. t-strbuf.c shows some example
unit tests for strbuf.c
The unit tests will be built as part of the default "make all" target,
to avoid bitrot. If you wish to build just the unit tests, you can run
"make build-unit-tests". To run the tests, you can use "make unit-tests"
or run the test binaries directly, as in "./t/unit-tests/bin/t-strbuf".
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 28 ++-
t/Makefile | 15 +-
t/t0080-unit-test-output.sh | 58 +++++++
t/unit-tests/.gitignore | 1 +
t/unit-tests/t-basic.c | 95 +++++++++++
t/unit-tests/t-strbuf.c | 120 +++++++++++++
t/unit-tests/test-lib.c | 330 ++++++++++++++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 ++++++++++++++++
8 files changed, 792 insertions(+), 4 deletions(-)
create mode 100755 t/t0080-unit-test-output.sh
create mode 100644 t/unit-tests/.gitignore
create mode 100644 t/unit-tests/t-basic.c
create mode 100644 t/unit-tests/t-strbuf.c
create mode 100644 t/unit-tests/test-lib.c
create mode 100644 t/unit-tests/test-lib.h
diff --git a/Makefile b/Makefile
index e440728c24..18c13f06c0 100644
--- a/Makefile
+++ b/Makefile
@@ -682,6 +682,9 @@ TEST_BUILTINS_OBJS =
TEST_OBJS =
TEST_PROGRAMS_NEED_X =
THIRD_PARTY_SOURCES =
+UNIT_TEST_PROGRAMS =
+UNIT_TEST_DIR = t/unit-tests
+UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
# Having this variable in your environment would break pipelines because
# you cause "cd" to echo its destination to stdout. It can also take
@@ -1331,6 +1334,12 @@ THIRD_PARTY_SOURCES += compat/regex/%
THIRD_PARTY_SOURCES += sha1collisiondetection/%
THIRD_PARTY_SOURCES += sha1dc/%
+UNIT_TEST_PROGRAMS += t-basic
+UNIT_TEST_PROGRAMS += t-strbuf
+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
+
# xdiff and reftable libs may in turn depend on what is in libgit.a
GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
EXTLIBS =
@@ -2672,6 +2681,7 @@ OBJECTS += $(TEST_OBJS)
OBJECTS += $(XDIFF_OBJS)
OBJECTS += $(FUZZ_OBJS)
OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
+OBJECTS += $(UNIT_TEST_OBJS)
ifndef NO_CURL
OBJECTS += http.o http-walker.o remote-curl.o
@@ -3167,7 +3177,7 @@ endif
test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
bin-wrappers/%: wrap-for-bin.sh
$(call mkdir_p_parent_template)
@@ -3592,7 +3602,7 @@ endif
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
- $(MOFILES)
+ $(UNIT_TEST_PROGS) $(MOFILES)
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3653,7 +3663,7 @@ clean: profile-clean coverage-clean cocciclean
$(RM) $(OBJECTS)
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
- $(RM) $(TEST_PROGRAMS)
+ $(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
$(RM) $(FUZZ_PROGRAMS)
$(RM) $(SP_OBJ)
$(RM) $(HCC)
@@ -3831,3 +3841,15 @@ $(FUZZ_PROGRAMS): all
$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
fuzz-all: $(FUZZ_PROGRAMS)
+
+$(UNIT_TEST_BIN):
+ @mkdir -p $(UNIT_TEST_BIN)
+
+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
+.PHONY: build-unit-tests unit-tests
+build-unit-tests: $(UNIT_TEST_PROGS)
+unit-tests: $(UNIT_TEST_PROGS)
+ $(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 3e00cdd801..75d9330437 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -17,6 +17,7 @@ TAR ?= $(TAR)
RM ?= rm -f
PROVE ?= prove
DEFAULT_TEST_TARGET ?= test
+DEFAULT_UNIT_TEST_TARGET ?= unit-tests-raw
TEST_LINT ?= test-lint
ifdef TEST_OUTPUT_DIRECTORY
@@ -41,6 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
# checks all tests in all scripts via a single invocation, so tell individual
@@ -65,6 +67,17 @@ prove: pre-clean check-chainlint $(TEST_LINT)
$(T):
@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+$(UNIT_TESTS):
+ @echo "*** $@ ***"; $@
+
+.PHONY: unit-tests unit-tests-raw unit-tests-prove
+unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
+
+unit-tests-raw: $(UNIT_TESTS)
+
+unit-tests-prove:
+ @echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
+
pre-clean:
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
@@ -149,4 +162,4 @@ perf:
$(MAKE) -C perf/ all
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
- check-chainlint clean-chainlint test-chainlint
+ check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
new file mode 100755
index 0000000000..961b54b06c
--- /dev/null
+++ b/t/t0080-unit-test-output.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='Test the output of the unit test framework'
+
+. ./test-lib.sh
+
+test_expect_success 'TAP output from unit tests' '
+ cat >expect <<-EOF &&
+ ok 1 - passing test
+ ok 2 - passing test and assertion return 1
+ # check "1 == 2" failed at t/unit-tests/t-basic.c:76
+ # left: 1
+ # right: 2
+ not ok 3 - failing test
+ ok 4 - failing test and assertion return 0
+ not ok 5 - passing TEST_TODO() # TODO
+ ok 6 - passing TEST_TODO() returns 1
+ # todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+ not ok 7 - failing TEST_TODO()
+ ok 8 - failing TEST_TODO() returns 0
+ # check "0" failed at t/unit-tests/t-basic.c:30
+ # skipping test - missing prerequisite
+ # skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+ ok 9 - test_skip() # SKIP
+ ok 10 - skipped test returns 1
+ # skipping test - missing prerequisite
+ ok 11 - test_skip() inside TEST_TODO() # SKIP
+ ok 12 - test_skip() inside TEST_TODO() returns 1
+ # check "0" failed at t/unit-tests/t-basic.c:48
+ not ok 13 - TEST_TODO() after failing check
+ ok 14 - TEST_TODO() after failing check returns 0
+ # check "0" failed at t/unit-tests/t-basic.c:56
+ not ok 15 - failing check after TEST_TODO()
+ ok 16 - failing check after TEST_TODO() returns 0
+ # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+ # left: "\011hello\\\\"
+ # right: "there\"\012"
+ # check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+ # left: "NULL"
+ # right: NULL
+ # check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+ # left: ${SQ}a${SQ}
+ # right: ${SQ}\012${SQ}
+ # check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+ # left: ${SQ}\\\\${SQ}
+ # right: ${SQ}\\${SQ}${SQ}
+ not ok 17 - messages from failing string and char comparison
+ # BUG: test has no checks at t/unit-tests/t-basic.c:91
+ not ok 18 - test with no checks
+ ok 19 - test with no checks returns 0
+ 1..19
+ EOF
+
+ ! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+ test_cmp expect actual
+'
+
+test_done
diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
new file mode 100644
index 0000000000..5e56e040ec
--- /dev/null
+++ b/t/unit-tests/.gitignore
@@ -0,0 +1 @@
+/bin
diff --git a/t/unit-tests/t-basic.c b/t/unit-tests/t-basic.c
new file mode 100644
index 0000000000..fda1ae59a6
--- /dev/null
+++ b/t/unit-tests/t-basic.c
@@ -0,0 +1,95 @@
+#include "test-lib.h"
+
+/*
+ * The purpose of this "unit test" is to verify a few invariants of the unit
+ * test framework itself, as well as to provide examples of output from actually
+ * failing tests. As such, it is intended that this test fails, and thus it
+ * should not be run as part of `make unit-tests`. Instead, we verify it behaves
+ * as expected in the integration test t0080-unit-test-output.sh
+ */
+
+/* Used to store the return value of check_int(). */
+static int check_res;
+
+/* Used to store the return value of TEST(). */
+static int test_res;
+
+static void t_res(int expect)
+{
+ check_int(check_res, ==, expect);
+ check_int(test_res, ==, expect);
+}
+
+static void t_todo(int x)
+{
+ check_res = TEST_TODO(check(x));
+}
+
+static void t_skip(void)
+{
+ check(0);
+ test_skip("missing prerequisite");
+ check(1);
+}
+
+static int do_skip(void)
+{
+ test_skip("missing prerequisite");
+ return 1;
+}
+
+static void t_skip_todo(void)
+{
+ check_res = TEST_TODO(do_skip());
+}
+
+static void t_todo_after_fail(void)
+{
+ check(0);
+ TEST_TODO(check(0));
+}
+
+static void t_fail_after_todo(void)
+{
+ check(1);
+ TEST_TODO(check(0));
+ check(0);
+}
+
+static void t_messages(void)
+{
+ check_str("\thello\\", "there\"\n");
+ check_str("NULL", NULL);
+ check_char('a', ==, '\n');
+ check_char('\\', ==, '\'');
+}
+
+static void t_empty(void)
+{
+ ; /* empty */
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ test_res = TEST(check_res = check_int(1, ==, 1), "passing test");
+ TEST(t_res(1), "passing test and assertion return 1");
+ test_res = TEST(check_res = check_int(1, ==, 2), "failing test");
+ TEST(t_res(0), "failing test and assertion return 0");
+ test_res = TEST(t_todo(0), "passing TEST_TODO()");
+ TEST(t_res(1), "passing TEST_TODO() returns 1");
+ test_res = TEST(t_todo(1), "failing TEST_TODO()");
+ TEST(t_res(0), "failing TEST_TODO() returns 0");
+ test_res = TEST(t_skip(), "test_skip()");
+ TEST(check_int(test_res, ==, 1), "skipped test returns 1");
+ test_res = TEST(t_skip_todo(), "test_skip() inside TEST_TODO()");
+ TEST(t_res(1), "test_skip() inside TEST_TODO() returns 1");
+ test_res = TEST(t_todo_after_fail(), "TEST_TODO() after failing check");
+ TEST(check_int(test_res, ==, 0), "TEST_TODO() after failing check returns 0");
+ test_res = TEST(t_fail_after_todo(), "failing check after TEST_TODO()");
+ TEST(check_int(test_res, ==, 0), "failing check after TEST_TODO() returns 0");
+ TEST(t_messages(), "messages from failing string and char comparison");
+ test_res = TEST(t_empty(), "test with no checks");
+ TEST(check_int(test_res, ==, 0), "test with no checks returns 0");
+
+ return test_done();
+}
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
new file mode 100644
index 0000000000..de434a4441
--- /dev/null
+++ b/t/unit-tests/t-strbuf.c
@@ -0,0 +1,120 @@
+#include "test-lib.h"
+#include "strbuf.h"
+
+/* wrapper that supplies tests with an empty, initialized strbuf */
+static void setup(void (*f)(struct strbuf*, void*), void *data)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ f(&buf, data);
+ strbuf_release(&buf);
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+}
+
+/* wrapper that supplies tests with a populated, initialized strbuf */
+static void setup_populated(void (*f)(struct strbuf*, void*), char *init_str, void *data)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, init_str);
+ check_uint(buf.len, ==, strlen(init_str));
+ f(&buf, data);
+ strbuf_release(&buf);
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+}
+
+static int assert_sane_strbuf(struct strbuf *buf)
+{
+ /* Initialized strbufs should always have a non-NULL buffer */
+ if (!check(!!buf->buf))
+ return 0;
+ /* Buffers should always be NUL-terminated */
+ if (!check_char(buf->buf[buf->len], ==, '\0'))
+ return 0;
+ /*
+ * Freshly-initialized strbufs may not have a dynamically allocated
+ * buffer
+ */
+ if (buf->len == 0 && buf->alloc == 0)
+ return 1;
+ /* alloc must be at least one byte larger than len */
+ return check_uint(buf->len, <, buf->alloc);
+}
+
+static void t_static_init(void)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, ==, 0);
+ check_char(buf.buf[0], ==, '\0');
+}
+
+static void t_dynamic_init(void)
+{
+ struct strbuf buf;
+
+ strbuf_init(&buf, 1024);
+ check(assert_sane_strbuf(&buf));
+ check_uint(buf.len, ==, 0);
+ check_uint(buf.alloc, >=, 1024);
+ check_char(buf.buf[0], ==, '\0');
+ strbuf_release(&buf);
+}
+
+static void t_addch(struct strbuf *buf, void *data)
+{
+ const char *p_ch = data;
+ const char ch = *p_ch;
+ size_t orig_alloc = buf->alloc;
+ size_t orig_len = buf->len;
+
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ strbuf_addch(buf, ch);
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ if (!(check_uint(buf->len, ==, orig_len + 1) &&
+ check_uint(buf->alloc, >=, orig_alloc)))
+ return; /* avoid de-referencing buf->buf */
+ check_char(buf->buf[buf->len - 1], ==, ch);
+ check_char(buf->buf[buf->len], ==, '\0');
+}
+
+static void t_addstr(struct strbuf *buf, void *data)
+{
+ const char *text = data;
+ size_t len = strlen(text);
+ size_t orig_alloc = buf->alloc;
+ size_t orig_len = buf->len;
+
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ strbuf_addstr(buf, text);
+ if (!check(assert_sane_strbuf(buf)))
+ return;
+ if (!(check_uint(buf->len, ==, orig_len + len) &&
+ check_uint(buf->alloc, >=, orig_alloc) &&
+ check_uint(buf->alloc, >, orig_len + len) &&
+ check_char(buf->buf[orig_len + len], ==, '\0')))
+ return;
+ check_str(buf->buf + orig_len, text);
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ if (!TEST(t_static_init(), "static initialization works"))
+ test_skip_all("STRBUF_INIT is broken");
+ TEST(t_dynamic_init(), "dynamic initialization works");
+ TEST(setup(t_addch, "a"), "strbuf_addch adds char");
+ TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
+ TEST(setup_populated(t_addch, "initial value", "a"),
+ "strbuf_addch appends to initial value");
+ TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
+ TEST(setup_populated(t_addstr, "initial value", "hello there"),
+ "strbuf_addstr appends string to initial value");
+
+ return test_done();
+}
diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
new file mode 100644
index 0000000000..a2cc21c706
--- /dev/null
+++ b/t/unit-tests/test-lib.c
@@ -0,0 +1,330 @@
+#include "test-lib.h"
+
+enum result {
+ RESULT_NONE,
+ RESULT_FAILURE,
+ RESULT_SKIP,
+ RESULT_SUCCESS,
+ RESULT_TODO
+};
+
+static struct {
+ enum result result;
+ int count;
+ unsigned failed :1;
+ unsigned lazy_plan :1;
+ unsigned running :1;
+ unsigned skip_all :1;
+ unsigned todo :1;
+} ctx = {
+ .lazy_plan = 1,
+ .result = RESULT_NONE,
+};
+
+static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
+{
+ fflush(stderr);
+ if (prefix)
+ fprintf(stdout, "%s", prefix);
+ vprintf(format, ap); /* TODO: handle newlines */
+ putc('\n', stdout);
+ fflush(stdout);
+}
+
+void test_msg(const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+ msg_with_prefix("# ", format, ap);
+ va_end(ap);
+}
+
+void test_plan(int count)
+{
+ assert(!ctx.running);
+
+ fflush(stderr);
+ printf("1..%d\n", count);
+ fflush(stdout);
+ ctx.lazy_plan = 0;
+}
+
+int test_done(void)
+{
+ assert(!ctx.running);
+
+ if (ctx.lazy_plan)
+ test_plan(ctx.count);
+
+ return ctx.failed;
+}
+
+void test_skip(const char *format, ...)
+{
+ va_list ap;
+
+ assert(ctx.running);
+
+ ctx.result = RESULT_SKIP;
+ va_start(ap, format);
+ if (format)
+ msg_with_prefix("# skipping test - ", format, ap);
+ va_end(ap);
+}
+
+void test_skip_all(const char *format, ...)
+{
+ va_list ap;
+ const char *prefix;
+
+ if (!ctx.count && ctx.lazy_plan) {
+ /* We have not printed a test plan yet */
+ prefix = "1..0 # SKIP ";
+ ctx.lazy_plan = 0;
+ } else {
+ /* We have already printed a test plan */
+ prefix = "Bail out! # ";
+ ctx.failed = 1;
+ }
+ ctx.skip_all = 1;
+ ctx.result = RESULT_SKIP;
+ va_start(ap, format);
+ msg_with_prefix(prefix, format, ap);
+ va_end(ap);
+}
+
+int test__run_begin(void)
+{
+ assert(!ctx.running);
+
+ ctx.count++;
+ ctx.result = RESULT_NONE;
+ ctx.running = 1;
+
+ return ctx.skip_all;
+}
+
+static void print_description(const char *format, va_list ap)
+{
+ if (format) {
+ fputs(" - ", stdout);
+ vprintf(format, ap);
+ }
+}
+
+int test__run_end(int was_run UNUSED, const char *location, const char *format, ...)
+{
+ va_list ap;
+
+ assert(ctx.running);
+ assert(!ctx.todo);
+
+ fflush(stderr);
+ va_start(ap, format);
+ if (!ctx.skip_all) {
+ switch (ctx.result) {
+ case RESULT_SUCCESS:
+ printf("ok %d", ctx.count);
+ print_description(format, ap);
+ break;
+
+ case RESULT_FAILURE:
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ break;
+
+ case RESULT_TODO:
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ printf(" # TODO");
+ break;
+
+ case RESULT_SKIP:
+ printf("ok %d", ctx.count);
+ print_description(format, ap);
+ printf(" # SKIP");
+ break;
+
+ case RESULT_NONE:
+ test_msg("BUG: test has no checks at %s", location);
+ printf("not ok %d", ctx.count);
+ print_description(format, ap);
+ ctx.result = RESULT_FAILURE;
+ break;
+ }
+ }
+ va_end(ap);
+ ctx.running = 0;
+ if (ctx.skip_all)
+ return 1;
+ putc('\n', stdout);
+ fflush(stdout);
+ ctx.failed |= ctx.result == RESULT_FAILURE;
+
+ return ctx.result != RESULT_FAILURE;
+}
+
+static void test_fail(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ ctx.result = RESULT_FAILURE;
+}
+
+static void test_pass(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ if (ctx.result == RESULT_NONE)
+ ctx.result = RESULT_SUCCESS;
+}
+
+static void test_todo(void)
+{
+ assert(ctx.result != RESULT_SKIP);
+
+ if (ctx.result != RESULT_FAILURE)
+ ctx.result = RESULT_TODO;
+}
+
+int test_assert(const char *location, const char *check, int ok)
+{
+ assert(ctx.running);
+
+ if (ctx.result == RESULT_SKIP) {
+ test_msg("skipping check '%s' at %s", check, location);
+ return 1;
+ }
+ if (!ctx.todo) {
+ if (ok) {
+ test_pass();
+ } else {
+ test_msg("check \"%s\" failed at %s", check, location);
+ test_fail();
+ }
+ }
+
+ return !!ok;
+}
+
+void test__todo_begin(void)
+{
+ assert(ctx.running);
+ assert(!ctx.todo);
+
+ ctx.todo = 1;
+}
+
+int test__todo_end(const char *location, const char *check, int res)
+{
+ assert(ctx.running);
+ assert(ctx.todo);
+
+ ctx.todo = 0;
+ if (ctx.result == RESULT_SKIP)
+ return 1;
+ if (res) {
+ test_msg("todo check '%s' succeeded at %s", check, location);
+ test_fail();
+ } else {
+ test_todo();
+ }
+
+ return !res;
+}
+
+int check_bool_loc(const char *loc, const char *check, int ok)
+{
+ return test_assert(loc, check, ok);
+}
+
+union test__tmp test__tmp[2];
+
+int check_int_loc(const char *loc, const char *check, int ok,
+ intmax_t a, intmax_t b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ test_msg(" left: %"PRIdMAX, a);
+ test_msg(" right: %"PRIdMAX, b);
+ }
+
+ return ret;
+}
+
+int check_uint_loc(const char *loc, const char *check, int ok,
+ uintmax_t a, uintmax_t b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ test_msg(" left: %"PRIuMAX, a);
+ test_msg(" right: %"PRIuMAX, b);
+ }
+
+ return ret;
+}
+
+static void print_one_char(char ch, char quote)
+{
+ if ((unsigned char)ch < 0x20u || ch == 0x7f) {
+ /* TODO: improve handling of \a, \b, \f ... */
+ printf("\\%03o", (unsigned char)ch);
+ } else {
+ if (ch == '\\' || ch == quote)
+ putc('\\', stdout);
+ putc(ch, stdout);
+ }
+}
+
+static void print_char(const char *prefix, char ch)
+{
+ printf("# %s: '", prefix);
+ print_one_char(ch, '\'');
+ fputs("'\n", stdout);
+}
+
+int check_char_loc(const char *loc, const char *check, int ok, char a, char b)
+{
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ fflush(stderr);
+ print_char(" left", a);
+ print_char(" right", b);
+ fflush(stdout);
+ }
+
+ return ret;
+}
+
+static void print_str(const char *prefix, const char *str)
+{
+ printf("# %s: ", prefix);
+ if (!str) {
+ fputs("NULL\n", stdout);
+ } else {
+ putc('"', stdout);
+ while (*str)
+ print_one_char(*str++, '"');
+ fputs("\"\n", stdout);
+ }
+}
+
+int check_str_loc(const char *loc, const char *check,
+ const char *a, const char *b)
+{
+ int ok = (!a && !b) || (a && b && !strcmp(a, b));
+ int ret = test_assert(loc, check, ok);
+
+ if (!ret) {
+ fflush(stderr);
+ print_str(" left", a);
+ print_str(" right", b);
+ fflush(stdout);
+ }
+
+ return ret;
+}
diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h
new file mode 100644
index 0000000000..a8f07ae0b7
--- /dev/null
+++ b/t/unit-tests/test-lib.h
@@ -0,0 +1,149 @@
+#ifndef TEST_LIB_H
+#define TEST_LIB_H
+
+#include "git-compat-util.h"
+
+/*
+ * Run a test function, returns 1 if the test succeeds, 0 if it
+ * fails. If test_skip_all() has been called then the test will not be
+ * run. The description for each test should be unique. For example:
+ *
+ * TEST(test_something(arg1, arg2), "something %d %d", arg1, arg2)
+ */
+#define TEST(t, ...) \
+ test__run_end(test__run_begin() ? 0 : (t, 1), \
+ TEST_LOCATION(), __VA_ARGS__)
+
+/*
+ * Print a test plan, should be called before any tests. If the number
+ * of tests is not known in advance test_done() will automatically
+ * print a plan at the end of the test program.
+ */
+void test_plan(int count);
+
+/*
+ * test_done() must be called at the end of main(). It will print the
+ * plan if plan() was not called at the beginning of the test program
+ * and returns the exit code for the test program.
+ */
+int test_done(void);
+
+/* Skip the current test. */
+__attribute__((format (printf, 1, 2)))
+void test_skip(const char *format, ...);
+
+/* Skip all remaining tests. */
+__attribute__((format (printf, 1, 2)))
+void test_skip_all(const char *format, ...);
+
+/* Print a diagnostic message to stdout. */
+__attribute__((format (printf, 1, 2)))
+void test_msg(const char *format, ...);
+
+/*
+ * Test checks are built around test_assert(). checks return 1 on
+ * success, 0 on failure. If any check fails then the test will fail. To
+ * create a custom check define a function that wraps test_assert() and
+ * a macro to wrap that function to provide a source location and
+ * stringified arguments. Custom checks that take pointer arguments
+ * should be careful to check that they are non-NULL before
+ * dereferencing them. For example:
+ *
+ * static int check_oid_loc(const char *loc, const char *check,
+ * struct object_id *a, struct object_id *b)
+ * {
+ * int res = test_assert(loc, check, a && b && oideq(a, b));
+ *
+ * if (!res) {
+ * test_msg(" left: %s", a ? oid_to_hex(a) : "NULL";
+ * test_msg(" right: %s", b ? oid_to_hex(a) : "NULL";
+ *
+ * }
+ * return res;
+ * }
+ *
+ * #define check_oid(a, b) \
+ * check_oid_loc(TEST_LOCATION(), "oideq("#a", "#b")", a, b)
+ */
+int test_assert(const char *location, const char *check, int ok);
+
+/* Helper macro to pass the location to checks */
+#define TEST_LOCATION() TEST__MAKE_LOCATION(__LINE__)
+
+/* Check a boolean condition. */
+#define check(x) \
+ check_bool_loc(TEST_LOCATION(), #x, x)
+int check_bool_loc(const char *loc, const char *check, int ok);
+
+/*
+ * Compare two integers. Prints a message with the two values if the
+ * comparison fails. NB this is not thread safe.
+ */
+#define check_int(a, op, b) \
+ (test__tmp[0].i = (a), test__tmp[1].i = (b), \
+ check_int_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].i op test__tmp[1].i, \
+ test__tmp[0].i, test__tmp[1].i))
+int check_int_loc(const char *loc, const char *check, int ok,
+ intmax_t a, intmax_t b);
+
+/*
+ * Compare two unsigned integers. Prints a message with the two values
+ * if the comparison fails. NB this is not thread safe.
+ */
+#define check_uint(a, op, b) \
+ (test__tmp[0].u = (a), test__tmp[1].u = (b), \
+ check_uint_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].u op test__tmp[1].u, \
+ test__tmp[0].u, test__tmp[1].u))
+int check_uint_loc(const char *loc, const char *check, int ok,
+ uintmax_t a, uintmax_t b);
+
+/*
+ * Compare two chars. Prints a message with the two values if the
+ * comparison fails. NB this is not thread safe.
+ */
+#define check_char(a, op, b) \
+ (test__tmp[0].c = (a), test__tmp[1].c = (b), \
+ check_char_loc(TEST_LOCATION(), #a" "#op" "#b, \
+ test__tmp[0].c op test__tmp[1].c, \
+ test__tmp[0].c, test__tmp[1].c))
+int check_char_loc(const char *loc, const char *check, int ok,
+ char a, char b);
+
+/* Check whether two strings are equal. */
+#define check_str(a, b) \
+ check_str_loc(TEST_LOCATION(), "!strcmp("#a", "#b")", a, b)
+int check_str_loc(const char *loc, const char *check,
+ const char *a, const char *b);
+
+/*
+ * Wrap a check that is known to fail. If the check succeeds then the
+ * test will fail. Returns 1 if the check fails, 0 if it
+ * succeeds. For example:
+ *
+ * TEST_TODO(check(0));
+ */
+#define TEST_TODO(check) \
+ (test__todo_begin(), test__todo_end(TEST_LOCATION(), #check, check))
+
+/* Private helpers */
+
+#define TEST__STR(x) #x
+#define TEST__MAKE_LOCATION(line) __FILE__ ":" TEST__STR(line)
+
+union test__tmp {
+ intmax_t i;
+ uintmax_t u;
+ char c;
+};
+
+extern union test__tmp test__tmp[2];
+
+int test__run_begin(void);
+__attribute__((format (printf, 3, 4)))
+int test__run_end(int, const char *, const char *, ...);
+void test__todo_begin(void);
+int test__todo_end(const char *, const char *, int);
+
+#endif /* TEST_LIB_H */
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [PATCH v10 1/3] unit tests: Add a project plan document
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>
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
diff --git a/Documentation/Makefile b/Documentation/Makefile
index b629176d7d..3f2383a12c 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -122,6 +122,7 @@ TECH_DOCS += technical/scalar
TECH_DOCS += technical/send-pack-pipeline
TECH_DOCS += technical/shallow
TECH_DOCS += technical/trivial-merge
+TECH_DOCS += technical/unit-tests
SP_ARTICLES += $(TECH_DOCS)
SP_ARTICLES += technical/api-index
diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
new file mode 100644
index 0000000000..206037ffb1
--- /dev/null
+++ b/Documentation/technical/unit-tests.txt
@@ -0,0 +1,240 @@
+= Unit Testing
+
+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).
+Unit tests additionally provide stability to the codebase and can simplify
+debugging through isolation. Writing unit tests in pure C, rather than with our
+current shell/test-tool helper setup, simplifies test setup, simplifies passing
+data around (no shell-isms required), and reduces testing runtime by not
+spawning a separate process for every test invocation.
+
+We believe that a large body of unit tests, living alongside the existing test
+suite, will improve code quality for the Git project.
+
+== Definitions
+
+For the purposes of this document, we'll use *test framework* to refer to
+projects that support writing test cases and running tests within the context
+of a single executable. *Test harness* will refer to projects that manage
+running multiple executables (each of which may contain multiple test cases) and
+aggregating their results.
+
+In reality, these terms are not strictly defined, and many of the projects
+discussed below contain features from both categories.
+
+For now, we will evaluate projects solely on their framework features. Since we
+are relying on having TAP output (see below), we can assume that any framework
+can be made to work with a harness that we can choose later.
+
+
+== Summary
+
+We believe the best way forward is to implement a custom TAP framework for the
+Git project. We use a version of the framework originally proposed in
+https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
+
+See the <<framework-selection,Framework Selection>> section below for the
+rationale behind this decision.
+
+
+== Choosing a test harness
+
+During upstream discussion, it was occasionally noted that `prove` provides many
+convenient features, such as scheduling slower tests first, or re-running
+previously failed tests.
+
+While we already support the use of `prove` as a test harness for the shell
+tests, it is not strictly required. The t/Makefile allows running shell tests
+directly (though with interleaved output if parallelism is enabled). Git
+developers who wish to use `prove` as a more advanced harness can do so by
+setting DEFAULT_TEST_TARGET=prove in their config.mak.
+
+We will follow a similar approach for unit tests: by default the test
+executables will be run directly from the t/Makefile, but `prove` can be
+configured with DEFAULT_UNIT_TEST_TARGET=prove.
+
+
+[[framework-selection]]
+== Framework selection
+
+There are a variety of features we can use to rank the candidate frameworks, and
+those features have different priorities:
+
+* Critical features: we probably won't consider a framework without these
+** Can we legally / easily use the project?
+*** <<license,License>>
+*** <<vendorable-or-ubiquitous,Vendorable or ubiquitous>>
+*** <<maintainable-extensible,Maintainable / extensible>>
+*** <<major-platform-support,Major platform support>>
+** Does the project support our bare-minimum needs?
+*** <<tap-support,TAP support>>
+*** <<diagnostic-output,Diagnostic output>>
+*** <<runtime-skippable-tests,Runtime-skippable tests>>
+* Nice-to-have features:
+** <<parallel-execution,Parallel execution>>
+** <<mock-support,Mock support>>
+** <<signal-error-handling,Signal & error-handling>>
+* Tie-breaker stats
+** <<project-kloc,Project KLOC>>
+** <<adoption,Adoption>>
+
+[[license]]
+=== License
+
+We must be able to legally use the framework in connection with Git. As Git is
+licensed only under GPLv2, we must eliminate any LGPLv3, GPLv3, or Apache 2.0
+projects.
+
+[[vendorable-or-ubiquitous]]
+=== Vendorable or ubiquitous
+
+We want to avoid forcing Git developers to install new tools just to run unit
+tests. Any prospective frameworks and harnesses must either be vendorable
+(meaning, we can copy their source directly into Git's repository), or so
+ubiquitous that it is reasonable to expect that most developers will have the
+tools installed already.
+
+[[maintainable-extensible]]
+=== Maintainable / extensible
+
+It is unlikely that any pre-existing project perfectly fits our needs, so any
+project we select will need to be actively maintained and open to accepting
+changes. Alternatively, assuming we are vendoring the source into our repo, it
+must be simple enough that Git developers can feel comfortable making changes as
+needed to our version.
+
+In the comparison table below, "True" means that the framework seems to have
+active developers, that it is simple enough that Git developers can make changes
+to it, and that the project seems open to accepting external contributions (or
+that it is vendorable). "Partial" means that at least one of the above
+conditions holds.
+
+[[major-platform-support]]
+=== Major platform support
+
+At a bare minimum, unit-testing must work on Linux, MacOS, and Windows.
+
+In the comparison table below, "True" means that it works on all three major
+platforms with no issues. "Partial" means that there may be annoyances on one or
+more platforms, but it is still usable in principle.
+
+[[tap-support]]
+=== TAP support
+
+The https://testanything.org/[Test Anything Protocol] is a text-based interface
+that allows tests to communicate with a test harness. It is already used by
+Git's integration test suite. Supporting TAP output is a mandatory feature for
+any prospective test framework.
+
+In the comparison table below, "True" means this is natively supported.
+"Partial" means TAP output must be generated by post-processing the native
+output.
+
+Frameworks that do not have at least Partial support will not be evaluated
+further.
+
+[[diagnostic-output]]
+=== Diagnostic output
+
+When a test case fails, the framework must generate enough diagnostic output to
+help developers find the appropriate test case in source code in order to debug
+the failure.
+
+[[runtime-skippable-tests]]
+=== Runtime-skippable tests
+
+Test authors may wish to skip certain test cases based on runtime circumstances,
+so the framework should support this.
+
+[[parallel-execution]]
+=== Parallel execution
+
+Ideally, we will build up a significant collection of unit test cases, most
+likely split across multiple executables. It will be necessary to run these
+tests in parallel to enable fast develop-test-debug cycles.
+
+In the comparison table below, "True" means that individual test cases within a
+single test executable can be run in parallel. We assume that executable-level
+parallelism can be handled by the test harness.
+
+[[mock-support]]
+=== Mock support
+
+Unit test authors may wish to test code that interacts with objects that may be
+inconvenient to handle in a test (e.g. interacting with a network service).
+Mocking allows test authors to provide a fake implementation of these objects
+for more convenient tests.
+
+[[signal-error-handling]]
+=== Signal & error handling
+
+The test framework should fail gracefully when test cases are themselves buggy
+or when they are interrupted by signals during runtime.
+
+[[project-kloc]]
+=== Project KLOC
+
+The size of the project, in thousands of lines of code as measured by
+https://dwheeler.com/sloccount/[sloccount] (rounded up to the next multiple of
+1,000). As a tie-breaker, we probably prefer a project with fewer LOC.
+
+[[adoption]]
+=== Adoption
+
+As a tie-breaker, we prefer a more widely-used project. We use the number of
+GitHub / GitLab stars to estimate this.
+
+
+=== Comparison
+
+:true: [lime-background]#True#
+:false: [red-background]#False#
+:partial: [yellow-background]#Partial#
+
+:gpl: [lime-background]#GPL v2#
+:isc: [lime-background]#ISC#
+:mit: [lime-background]#MIT#
+:expat: [lime-background]#Expat#
+:lgpl: [lime-background]#LGPL v2.1#
+
+:custom-impl: https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[Custom Git impl.]
+:greatest: https://github.com/silentbicycle/greatest[Greatest]
+:criterion: https://github.com/Snaipe/Criterion[Criterion]
+:c-tap: https://github.com/rra/c-tap-harness/[C TAP]
+:check: https://libcheck.github.io/check/[Check]
+
+[format="csv",options="header",width="33%",subs="specialcharacters,attributes,quotes,macros"]
+|=====
+Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
+{custom-impl},{gpl},{true},{true},{true},{true},{true},{true},{false},{false},{false},1,0
+{greatest},{isc},{true},{partial},{true},{partial},{true},{true},{false},{false},{false},3,1400
+{criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800
+{c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33
+{check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973
+|=====
+
+=== Additional framework candidates
+
+Several suggested frameworks have been eliminated from consideration:
+
+* Incompatible licenses:
+** https://github.com/zorgnax/libtap[libtap] (LGPL v3)
+** https://cmocka.org/[cmocka] (Apache 2.0)
+* Missing source: https://www.kindahl.net/mytap/doc/index.html[MyTap]
+* No TAP support:
+** https://nemequ.github.io/munit/[µnit]
+** https://github.com/google/cmockery[cmockery]
+** https://github.com/lpabon/cmockery2[cmockery2]
+** https://github.com/ThrowTheSwitch/Unity[Unity]
+** https://github.com/siu/minunit[minunit]
+** https://cunit.sourceforge.net/[CUnit]
+
+
+== Milestones
+
+* Add useful tests of library-like code
+* Integrate with
+ https://lore.kernel.org/git/20230502211454.1673000-1-calvinwan@google.com/[stdlib
+ work]
+* Run alongside regular `make test` target
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply related
* [PATCH v10 0/3] Add unit test framework and project plan
From: Josh Steadmon @ 2023-11-09 18:50 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood123, oswald.buddenhagen, christian.couder
In-Reply-To: <0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com>
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). Unit tests additionally provide stability to the
codebase and can simplify debugging through isolation. Turning parts of
Git into libraries[1] gives us the ability to run unit tests on the
libraries and to write unit tests in C. Writing unit tests in pure C,
rather than with our current shell/test-tool helper setup, simplifies
test setup, simplifies passing data around (no shell-isms required), and
reduces testing runtime by not spawning a separate process for every
test invocation.
This series begins with a project document covering our goals for adding
unit tests and a discussion of alternative frameworks considered, as
well as the features used to evaluate them. A rendered preview of this
doc can be found at [2]. It also adds Phillip Wood's TAP implemenation
(with some slightly re-worked Makefile rules) and a sample strbuf unit
test. Finally, we modify the configs for GitHub and Cirrus CI to run the
unit tests. Sample runs showing successful CI runs can be found at [3],
[4], and [5].
[1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/
[2] https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
[3] https://github.com/steadmon/git/actions/runs/5884659246/job/15959781385#step:4:1803
[4] https://github.com/steadmon/git/actions/runs/5884659246/job/15959938401#step:5:186
[5] https://cirrus-ci.com/task/6126304366428160 (unrelated tests failed,
but note that t-strbuf ran successfully)
Changes in v10:
- Included a promised style cleanup in test-lib.c that was accidentally
dropped in v9.
Changes in v9:
- Included some asciidoc cleanups suggested by Oswald Buddenhagen.
- Applied a style fixup that Coccinelle complained about.
- Applied some NULL-safety fixups.
- Used check_*() more widely in t-strbuf helper functions
Changes in v8:
- Flipped return values for TEST, TEST_TODO, and check_* macros &
functions. This makes it easier to reason about control flow for
patterns like:
if (check(some_condition)) { ... }
- Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
patterns.
- Removed testing of some strbuf implementation details in t-strbuf.c
Changes in v7:
- Fix corrupt diff in patch #2, sorry for the noise.
Changes in v6:
- Officially recommend using Phillip Wood's TAP framework
- Add an example strbuf unit test using the TAP framework as well as
Makefile integration
- Run unit tests in CI
Changes in v5:
- Add comparison point "License".
- Discuss feature priorities
- Drop frameworks:
- Incompatible licenses: libtap, cmocka
- Missing source: MyTAP
- No TAP support: µnit, cmockery, cmockery2, Unity, minunit, CUnit
- Drop comparison point "Coverage reports": this can generally be
handled by tools such as `gcov` regardless of the framework used.
- Drop comparison point "Inline tests": there didn't seem to be
strong interest from reviewers for this feature.
- Drop comparison point "Scheduling / re-running": this was not
supported by any of the main contenders, and is generally better
handled by the harness rather than framework.
- Drop comparison point "Lazy test planning": this was supported by
all frameworks that provide TAP output.
Changes in v4:
- Add link anchors for the framework comparison dimensions
- Explain "Partial" results for each dimension
- Use consistent dimension names in the section headers and comparison
tables
- Add "Project KLOC", "Adoption", and "Inline tests" dimensions
- Fill in a few of the missing entries in the comparison table
Changes in v3:
- Expand the doc with discussion of desired features and a WIP
comparison.
- Drop all implementation patches until a framework is selected.
- Link to v2: https://lore.kernel.org/r/20230517-unit-tests-v2-v2-0-21b5b60f4b32@google.com
Josh Steadmon (2):
unit tests: Add a project plan document
ci: run unit tests in CI
Phillip Wood (1):
unit tests: add TAP unit test framework
.cirrus.yml | 2 +-
Documentation/Makefile | 1 +
Documentation/technical/unit-tests.txt | 240 ++++++++++++++++++
Makefile | 28 ++-
ci/run-build-and-tests.sh | 2 +
ci/run-test-slice.sh | 5 +
t/Makefile | 15 +-
t/t0080-unit-test-output.sh | 58 +++++
t/unit-tests/.gitignore | 1 +
t/unit-tests/t-basic.c | 95 +++++++
t/unit-tests/t-strbuf.c | 120 +++++++++
t/unit-tests/test-lib.c | 330 +++++++++++++++++++++++++
t/unit-tests/test-lib.h | 149 +++++++++++
13 files changed, 1041 insertions(+), 5 deletions(-)
create mode 100644 Documentation/technical/unit-tests.txt
create mode 100755 t/t0080-unit-test-output.sh
create mode 100644 t/unit-tests/.gitignore
create mode 100644 t/unit-tests/t-basic.c
create mode 100644 t/unit-tests/t-strbuf.c
create mode 100644 t/unit-tests/test-lib.c
create mode 100644 t/unit-tests/test-lib.h
Range-diff against v9:
-: ---------- > 1: f706ba9b68 unit tests: Add a project plan document
1: 8b831f4937 ! 2: 7a5e21bcff unit tests: add TAP unit test framework
@@ t/unit-tests/test-lib.c (new)
+ if (ctx.result == RESULT_SKIP) {
+ test_msg("skipping check '%s' at %s", check, location);
+ return 1;
-+ } else if (!ctx.todo) {
++ }
++ if (!ctx.todo) {
+ if (ok) {
+ test_pass();
+ } else {
2: 08d27bb5f9 = 3: 0129ec062c ci: run unit tests in CI
base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3
--
2.42.0.869.gea05f2083d-goog
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Jeff King @ 2023-11-09 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqqpm0jyx02.fsf@gitster.g>
On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
> > -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
>
> I do not think this is strictly necessary.
>
> Because the command line parser of "test" comes from left, notices
> "-d" and takes the next one to check if it is a directory. There is
> no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
> same way as the problem Peff pointed out during the discussion.
I think this is one of the ambiguous cases. If $GIT_DIR is "=", then
"test" cannot tell if you meant:
var1=-d
var2=-o
test "$var1" = "$var2" ...
or:
var1="="
test -d "$var1" -o ...
With bash, for example:
$ test -d /tmp -o -f .git; echo $?
0
$ test -d = -o -f .git; echo $?
bash: test: syntax error: `-f' unexpected
2
Without "-o", it uses the number of arguments to disambiguate (though of
course the lack of quotes around $GIT_DIR is another potential problem
here).
And I think the same is true of the other cases below using "-z", "-n",
and so on.
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.
> 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. :)
-Peff
^ permalink raw reply
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Jeff King @ 2023-11-09 18:35 UTC (permalink / raw)
To: Simon Ser; +Cc: René Scharfe, git, Junio C Hamano
In-Reply-To: <20231109111950.387219-1-contact@emersion.fr>
On Thu, Nov 09, 2023 at 11:19:56AM +0000, Simon Ser wrote:
> When writing the cover letter, the encode_email_headers option was
> ignored. That is, UTF-8 subject lines and email addresses were
> written out as-is, without any Q-encoding, even if
> --encode-email-headers was passed on the command line.
>
> This is due to encode_email_headers not being copied over from
> struct rev_info to struct pretty_print_context. Fix that and add
> a test.
That makes sense, and your patch looks the right thing to do as an
immediate fix. But I have to wonder:
1. Are there other bits that need to be copied? Grepping for other
code that does the same thing, I see that show_log() and
cmd_format_patch() copy a lot more. (For that matter, why doesn't
make_cover_letter() just use the context set up by
cmd_format_patch()?).
2. Why are we copying this stuff at all? When we introduced the
pretty-print context back in 6bf139440c (clean up calling
conventions for pretty.c functions, 2011-05-26), the idea was just
to keep all of the format options together. But later, 6d167fd7cc
(pretty: use fmt_output_email_subject(), 2017-03-01) added a
pointer to the rev_info directly. So could/should we just be using
pp->rev->encode_email_headers here?
Or if that field is not always set (or we want to override some
elements), should there be a single helper function to initialize
the pretty_print_context from a rev_info, that could be shared
between spots like show_log() and make_cover_letter()?
I don't think that answering those questions needs to hold up your
patch. We can take it as a quick fix for a real bug, and then anybody
interested can dig further as a separate topic on top.
> diff --git a/builtin/log.c b/builtin/log.c
> index ba775d7b5cf8..87fd1c8560de 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> pp.date_mode.type = DATE_RFC2822;
> pp.rev = rev;
> pp.print_email_subject = 1;
> + pp.encode_email_headers = rev->encode_email_headers;
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> prepare_cover_text(&pp, description_file, branch_name, &sb,
> encoding, need_8bit_cte);
This part looks obviously good.
> +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
> + test_config branch.rebuild-1.description "Café?
> +
> +body" &&
> + git checkout rebuild-1 &&
> + git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
> + grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
> + ! grep "Café" actual
> +'
The test looks correct to me.
Some of these long lines (and the in-string newlines!) make this ugly
and hard to read. But it is also just copying the already-ugly style of
nearby tests. So I'm OK with that. But a prettier version might be:
test_expect_success 'cover letter respects --encode-email-headers' '
test_config branch.rebuild-1.description "Café?" &&
git checkout rebuild-1 &&
git format-patch --stdout --encode-email-headers \
--cover-letter --cover-from-description=subject \
main >actual &&
...
'
I also wondered if we could be just be testing this much more easily
with another header like "--to". But I guess that would be found in both
the cover letter and the actual patches (we also don't seem to encode
it even in the regular patches; is that a bug?).
-Peff
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Jeff King @ 2023-11-09 18:03 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: git
In-Reply-To: <ZUz6H3IqRc1YGPZM@debian>
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 -"
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).
-Peff
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Konstantin Ryabitsev @ 2023-11-09 17:59 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZU0aAQhVj7BQwr0q@debian>
On Thu, Nov 09, 2023 at 06:42:19PM +0100, Alejandro Colomar wrote:
> I haven't yet tried b4(1), and considered trying it some time ago, but
> really didn't find git-send-email(1) and mutt(1) so difficult to use
> that b4(1) would simplify much.
Well, sending is only a small part of what b4 will do for you -- the core
benefits are really cover letter management, automatic versioning and
simplified list trailer collection. It's all tailored to kernel needs, but it
will work for any project that depends on mailed patches.
> But I have tried patatt(1) before, which is what I think b4(1) uses for
> signing. Here are some of my concerns about patatt(4):
>
> It doesn't sign the mail, but just the patch.
Well, no, it signs the entire thing, not just the patch, but it's true that
it's specifically targeted at patches (hence the name).
> There's not much
> difference, if any, in authenticability terms, but there's a big
> difference in usability terms:
>
> To authenticate a given patch submitted to a mailing list, the receiver
> needs to also have patatt(1) configured. Otherwise, it looks like a
> random message.
Yes, but that's a feature.
> MUAs normally don't show random headers (patatt(1)
> signs by adding the signature header), so unless one is searching for
> that header, it will be ignored. This means, if I contribute to other
> projects, I need to tell them my patch is signed via patatt(1) in
> order for them to verify. If instead, I sign the email as usual with my
> MUA, every MUA will recognize the signature by default and show it to
> the reader.
I go into this in the FAQ for patatt:
https://github.com/mricon/patatt#why-not-simply-pgp-sign-all-patches
Basically, developers really hated getting patches signed with PGP, either
inline or MIME, which is why it never took off. Putting it into the header
where it's not seen except by specialized tooling was a design choice.
> It also doesn't allow encrypting mail, so let's say I send some patch
> fixing a security vulnerability, I'll need a custom tool to send it. If
> instead, I use mutt(1) to send it signed+encrypted to a mailing list
> that provides a PGP public key, I can reuse my usual tools.
Right, the goal was really *just* attestation. For encrypted patch exchange we
have remail (https://korg.docs.kernel.org/remail.html), which worked
significantly better than any other alternative we've considered.
> Also, and I don't know if b4(1) handles this automagically, but AFAIR,
> patatt(1) didn't: fo signing a patch, I had to configure per-directory
> with `patatt install-hook`. I have more than a hundred git worktrees
> (think of dozens of git repositories, and half a dozen worktrees --see
> git-worktree(1)-- per repository). If I need to configure every one of
> those worktrees to sign all of my patches, that's going to be
> cumbersome. Also, I scrape and re-create worktrees for new branches
> all the time, so I'd need to be installing hooks for patatt(1) all the
> time. Compare that to having mutt(1) configured once. It doesn't
> scale that well.
Also true -- patatt was really envisioned as a library for b4, where you can
configure patch signing in your ~/.gitconfig for all projects.
-K
^ permalink raw reply
* Re: [PATCH v9 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-09 17:51 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Phillip Wood, Randall S. Becker,
Oswald Buddenhagen
In-Reply-To: <CAP8UFD3SVnu+HFQhFpsF4PA6pK5B5L+aP-jxRX=Ro3EYekS0kg@mail.gmail.com>
On 2023.11.03 22:54, Christian Couder wrote:
> On Thu, Nov 2, 2023 at 12:31 AM Josh Steadmon <steadmon@google.com> wrote:
> >
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> > +int test_assert(const char *location, const char *check, int ok)
> > +{
> > + assert(ctx.running);
> > +
> > + if (ctx.result == RESULT_SKIP) {
> > + test_msg("skipping check '%s' at %s", check, location);
> > + return 1;
> > + } else if (!ctx.todo) {
>
> I suggested removing the "else" and moving the "if (!ctx.todo) {" to
> its own line in the previous round and thought you agreed with that,
> but maybe it fell through the cracks somehow.
>
> Anyway I think this is a minor nit, and the series looks good to me.
Ahh, sorry about that, I must have accidentally dropped a fixup patch at
some point. I'll correct that and send v10 soon.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-09 17:42 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Jeff King, git
In-Reply-To: <vooebygemslmvmi4fzxtcl474wefcvxnigqeestmruzrsj5zsu@5kkq3pveol6c>
[-- Attachment #1: Type: text/plain, Size: 3697 bytes --]
Hi Konstantin,
On Thu, Nov 09, 2023 at 11:08:58AM -0500, Konstantin Ryabitsev wrote:
> On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> > 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. :)
>
> Since it's a Linux project, I suggest also checking out b4, which will do the
> mail sending for you as part of the contributor-oriented features:
>
> https://b4.docs.kernel.org/en/latest/contributor/overview.html
>
> We also provide a web relay for people who can't configure or use SMTP due to
> their company policies.
>
> > > > Would you mind adding this as part of git? Or should we suggest the
> > > > mutt project adding this script?
> > >
> > > IMHO it is a little too weird and user-specific to really make sense in
> > > either project. It's really glue-ing together two systems. And as it's
> > > not something I use myself, I don't plan it moving it further along. But
> > > you are welcome to take what I wrote and do what you will with it,
> > > including submitting it to mutt.
> >
> > I'll start by creating a git repository in my own server, and will write
> > something about it to let the public know about it. I'll also start
> > requiring contributors to linux-man@ to sign their patches, and
> > recommend them using this if they use mutt(1).
>
> B4 will also sign your patches for you. ;)
I haven't yet tried b4(1), and considered trying it some time ago, but
really didn't find git-send-email(1) and mutt(1) so difficult to use
that b4(1) would simplify much. But still, I'll give it a chance.
Maybe I see why afterwards.
But I have tried patatt(1) before, which is what I think b4(1) uses for
signing. Here are some of my concerns about patatt(4):
It doesn't sign the mail, but just the patch. There's not much
difference, if any, in authenticability terms, but there's a big
difference in usability terms:
To authenticate a given patch submitted to a mailing list, the receiver
needs to also have patatt(1) configured. Otherwise, it looks like a
random message. MUAs normally don't show random headers (patatt(1)
signs by adding the signature header), so unless one is searching for
that header, it will be ignored. This means, if I contribute to other
projects, I need to tell them my patch is signed via patatt(1) in
order for them to verify. If instead, I sign the email as usual with my
MUA, every MUA will recognize the signature by default and show it to
the reader.
It also doesn't allow encrypting mail, so let's say I send some patch
fixing a security vulnerability, I'll need a custom tool to send it. If
instead, I use mutt(1) to send it signed+encrypted to a mailing list
that provides a PGP public key, I can reuse my usual tools.
Also, and I don't know if b4(1) handles this automagically, but AFAIR,
patatt(1) didn't: fo signing a patch, I had to configure per-directory
with `patatt install-hook`. I have more than a hundred git worktrees
(think of dozens of git repositories, and half a dozen worktrees --see
git-worktree(1)-- per repository). If I need to configure every one of
those worktrees to sign all of my patches, that's going to be
cumbersome. Also, I scrape and re-create worktrees for new branches
all the time, so I'd need to be installing hooks for patatt(1) all the
time. Compare that to having mutt(1) configured once. It doesn't
scale that well.
Cheers,
Alex
>
> -K
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Konstantin Ryabitsev @ 2023-11-09 16:08 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZUz6H3IqRc1YGPZM@debian>
On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> 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. :)
Since it's a Linux project, I suggest also checking out b4, which will do the
mail sending for you as part of the contributor-oriented features:
https://b4.docs.kernel.org/en/latest/contributor/overview.html
We also provide a web relay for people who can't configure or use SMTP due to
their company policies.
> > > Would you mind adding this as part of git? Or should we suggest the
> > > mutt project adding this script?
> >
> > IMHO it is a little too weird and user-specific to really make sense in
> > either project. It's really glue-ing together two systems. And as it's
> > not something I use myself, I don't plan it moving it further along. But
> > you are welcome to take what I wrote and do what you will with it,
> > including submitting it to mutt.
>
> I'll start by creating a git repository in my own server, and will write
> something about it to let the public know about it. I'll also start
> requiring contributors to linux-man@ to sign their patches, and
> recommend them using this if they use mutt(1).
B4 will also sign your patches for you. ;)
-K
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox