* Re: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?
From: brian m. carlson @ 2023-10-13 21:05 UTC (permalink / raw)
To: Feiyang Xue; +Cc: git
In-Reply-To: <20231012200447.3553757-1-hi@fxue.dev>
[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]
On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
> Looking for comments on if this is a good idea to add SHA-NI option to git,
> And if so, what'd be a good implementation.
>
> The attached patch is more of a proof of concept, using a sha-ni example
> found on internet and have it tapped into git's "hash-ll.h" when it sees make
> flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for sha1/sha256 respectively.
>
> "git hash-object" is about 3-ish times faster for me
You almost certainly don't want to use SHA-1 using native instructions
because it doesn't detect collisions, unlike the default implementation
(SHA-1-DC). Since SHA-1 collisions are relatively cheap (less than USD
45,000) to make, not detecting collisions would be a security issue
worthy of a CVE.
It's fine for SHA-256, but see below.
> There are few topics that i'm uncertain about.
>
> 1. Is it good idea to have machine-specific asm codes in git. I read there was
> commit fd1ec82547 which removed assembly implementation for PPC_SHA1. Does that
> imply maintainers doesn't want machine-specific assembly in source?
I'd prefer we didn't.
Nettle has SHA-NI extensions on systems that support it, and so does
OpenSSL. OpenSSL can't be used by distros for licensing reasons, but
Nettle can, and both of those libraries automatically detect the best
code to use based on the chip. One of those libraries is almost always
going to be linked into Git at some point because of libcurl anyway.
If we ship the code, then someone has to maintain it, and I don't know
if we have any assembly experts. We might also have a bug that produced
incorrect results, which would be pretty disastrous for the affected
users. It's much better for maintainability if we push that off onto
the cryptographic library maintainers who are much more competent in
that regard than I am (I will not speak for others) and let distro
maintainers simply link the appropriate library, which, as I said above,
they're already effectively doing.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH 6/8] midx: read `OIDF` chunk with `pair_chunk_expect()`
From: Junio C Hamano @ 2023-10-13 21:04 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <b39203b32c24772d5d8a257c4f647b7d9bd92d53.1697225110.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> + if (pair_chunk_expect(cf, MIDX_CHUNKID_OIDFANOUT,
> + (const unsigned char **)&m->chunk_oid_fanout,
> + 256 * sizeof(uint32_t))) {
> + error(_("multi-pack-index OID fanout is of the wrong size"));
> die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
> + }
This is not a new problem, but when laid out this way, the doubled
messages look a bit suboptimal.
Together with reporting the actual and expected byte counts and
doing so consistenly I alluded to in a separate message, cleaning
these up should probably be left outside of the topic, I suspect.
> + m->num_objects = ntohl(m->chunk_oid_fanout[255]);
> if (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))
> die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
> if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
^ permalink raw reply
* Re: Regarding the depreciation of ssh+git/git+ssh protocols
From: David Rogers @ 2023-10-13 20:49 UTC (permalink / raw)
To: git
Git repositories have become indispensable resources for citing parts
of a development history with links. However, the format of git
remote entries is not always distinguishable from other types of
citation -- for example a git reference vs. a plain URL.
Rather than rely on context to tell me that `https://github.com/git/git`
refers to a git repository which I could clone with git over https, it
would be nice to use a url like `git+https://github.com/git/git`
or even
`git+https://github.com/git/git?commit=d0e8084c65cbf949038ae4cc344ac2c2efd77415`
to unambiguously specify that the type of data and its method of
access are native to git.
This issue is extremely important for version control systems which
build dependency lists from git, e.g.
https://pip.pypa.io/en/stable/topics/vcs-support/
That project lists several invented URL schemes (all beginning with
git+) and assigning special reserved characters
(https://datatracker.ietf.org/doc/html/rfc3986#section-2.2)
git+https://git.example.com/MyProject.git@master
git+https://git.example.com/MyProject.git@v1.0
git+https://git.example.com/MyProject.git@da39a3ee5e6b4b0d3255bfef95601890afd80709
git+https://git.example.com/MyProject.git@refs/pull/123/head
It would be helpful for the git project itself to define its own URL
scheme to codify these use cases and, possibly in addition, provide a
standard way to reference within git repositories.
For reference, some of the ways URLs are already used/defined within
git are documented here:
- https://github.com/git/git/blob/d0e8084c65cbf949038ae4cc344ac2c2efd77415/connect.c#L107
(alternately, using gitweb syntax not actually available on github,
https://github.com/git/git.git/blob/d0e8084c65cbf949038ae4cc344ac2c2efd77415:/git/connect.c)
- https://mirrors.edge.kernel.org/pub/software/scm/git/docs/gitremote-helpers.html
- https://git-scm.com/docs/git-http-backend
- https://git-scm.com/docs/gitweb
Currently, a comment in connect.c notes "git+" schemes were
deprecated. However, I would argue that at a minimum, these "git+"
schemes should be a supported and documented feature of git. Also,
something has to be fixed (or better communicated) about URLs of the
form "git@github.com:user/project.git" These are implicitly treated
as "git+ssh://git@github.com/user/project.git", but the use of ":" is
confusing from the perspective of translating between these two forms.
In addition, the use of paths, queries, and fragments should be
considered to allow (IMHO) at least 3 distinct uses:
1. naming commit-ish objects (and potentially metadata like author and
parents within the commit)
2. naming tree-ish objects and paths within them
3. naming blobs (and potentially fragment identifiers like lines or
HTML tags within those blobs)
These further refinements don't have to be supported by any special
functions within git. However, their existence may influence git data
structures and api-s in the future.
The last discussion I can find of this issue on the git mailing list
(https://lore.kernel.org/git/C9Y2DPYH4XO1.3KFD8LT770P2@taiga)
indicates that defining conventions like these within git's
documentation would be a good place to start. On a separate thread, I
will send a draft "git+" URI naming scheme for discussion and eventual
submission to IANA
(https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml).
~ David M. Rogers
^ permalink raw reply
* Re: [PATCH v5 0/2] attr: add attr.tree config
From: Junio C Hamano @ 2023-10-13 20:47 UTC (permalink / raw)
To: John Cai via GitGitGadget
Cc: git, Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <xmqqa5smz2lk.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> So in a sense, for !!ignore_bad_attr_tree case, the code ends up
> doing the right thing. But if !ignore_bad_attr_tree is true, i.e.,
> a blob object name is given via --attr-source or GIT_ATTR_SOURCE,
> then the bug will be uncovered.
Having said all that, I suspect that this problem is not new and
certainly not caused by this topic. We should have unconditionally
died when GIT_ATTR_SOURCE gave a blob object name, but pretended as
if an empty tree was given. There may even be existing users who
now assume that is working as intended and depend on this bug.
So, let's leave it as a "possible bug" that we might want to fix in
the future, outside the scope of this series.
Thanks.
> t/t0003-attributes.sh | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
> index ecf43ab545..0f02f22171 100755
> --- c/t/t0003-attributes.sh
> +++ w/t/t0003-attributes.sh
> @@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
> test_cmp expect actual
> '
>
> +test_expect_success '--attr-source that points at a non-treeish' '
> + test_when_finished rm -rf empty &&
> + git init empty &&
> + (
> + cd empty &&
> + echo "$bad_attr_source_err" >expect_err &&
> + H=$(git hash-object -t blob --stdin -w </dev/null) &&
> + test_must_fail git --attr-source=$H check-attr test -- f/path 2>err &&
> + test_cmp expect_err err
> + )
> +'
> +
> test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
> test_when_finished rm -rf empty &&
> git init empty &&
^ permalink raw reply
* Re: [PATCH v2 1/2] t: add a test helper to truncate files
From: Junio C Hamano @ 2023-10-13 20:32 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Taylor Blau, Jason Hatton
In-Reply-To: <ZSmmD5sz7TPWCGRd@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2023-10-12 at 22:52:59, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>> > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600);
>> > + if (fd < 0)
>> > + die_errno("failed to open file %s", argv[1]);
>>
>> contrib/coccinelle/xopen.cocci tells us to write this simply as
>>
>> fd = xopen(argv[1], O_WRONLY | O_CREAT, 0600);
>
> Sure, I can do that.
Unless there are other changes needed, I'll handle it on this end.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 0/2] attr: add attr.tree config
From: Junio C Hamano @ 2023-10-13 20:31 UTC (permalink / raw)
To: John Cai via GitGitGadget
Cc: git, Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <xmqqmswmz76w.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> if (repo_get_oid_treeish(the_repository,
> default_attr_source_tree_object_name,
> attr_source) && !ignore_bad_attr_tree)
> die(_("bad --attr-source or GIT_ATTR_SOURCE"));
>
> OOPS! Sorry for not noticing earlier, but repo_get_oid_treeish()
> does *NOT* error out when the discovered object is not a treeish, as
> the suggested object type is merely supplied for disambiguation
> purposes (e.g., with objects 012345 that is a tree and 012346 that
> is a blob, you can still ask for treeish "01234" but if you ask for
> an object "01234" it will fail).
>
> So, the alternative test would have caught this bug, no? Instead of
> silently treating the non-treeish as an empty tree, we would have
> died much later when the object supposedly a tree-ish turns out to
> be a blob, or something?
There indeed is a bug, but not really. If we add this test:
test_expect_success 'attr.tree that points at a non-treeish' '
test_when_finished rm -rf empty &&
git init empty &&
(
cd empty &&
echo "f/path: test: unspecified" >expect &&
H=$(git hash-object -t blob --stdin -w </dev/null) &&
git -c attr.tree=$H check-attr test -- f/path >actual 2>err &&
test_must_be_empty err &&
test_cmp expect actual
)
'
repo_get_oid_treeish() returns a blob object name and we end up
storing a blob object name in "attr_source" static variable of
default_attr_source() function.
Later this is fed to read_attr() by bootstrap_attr_stack() and then
to read_attr_from_blob() that uses it to call get_tree_entry(),
which fails for any path because it is a blob. We do not give any
errors or error messages during the whole process.
So in a sense, for !!ignore_bad_attr_tree case, the code ends up
doing the right thing. But if !ignore_bad_attr_tree is true, i.e.,
a blob object name is given via --attr-source or GIT_ATTR_SOURCE,
then the bug will be uncovered.
t/t0003-attributes.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
index ecf43ab545..0f02f22171 100755
--- c/t/t0003-attributes.sh
+++ w/t/t0003-attributes.sh
@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
test_cmp expect actual
'
+test_expect_success '--attr-source that points at a non-treeish' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
+ echo "$bad_attr_source_err" >expect_err &&
+ H=$(git hash-object -t blob --stdin -w </dev/null) &&
+ test_must_fail git --attr-source=$H check-attr test -- f/path 2>err &&
+ test_cmp expect_err err
+ )
+'
+
test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
test_when_finished rm -rf empty &&
git init empty &&
^ permalink raw reply related
* Re: [PATCH] send-email: move process_address_list earlier to avoid, uninitialized address error
From: Michael Strawbridge @ 2023-10-13 20:25 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: Bagas Sanjaya, Todd Zullinger, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List, Uwe Kleine-König
In-Reply-To: <20231011224753.GE518221@coredump.intra.peff.net>
On 10/11/23 18:47, Jeff King wrote:
> On Wed, Oct 11, 2023 at 03:37:39PM -0700, Junio C Hamano wrote:
>
>> On the other hand, I am not sure what is wrong with "after the user
>> typed", actually. As you said, anybody sane would be using --to (or
>> an equivalent configuration variable in the repository) to send
>> their patches to the project address instead of typing, and to them
>> it is not a problem. After getting the recipient address from the
>> end user, the validation may fail due to a wrong address, in which
>> case it is a good thing. If the validation failed due to wrong
>> contents of the patch (perhaps it included a change to the file with
>> trade secret that appeared in the context lines), as long as the
>> reason why the validation hook rejected the patches is clear enough
>> (e.g., "it's the patches, not the recipients"), such "a rejection
>> after typing" would be only once per a patch series, so it does not
>> sound too bad, either.
>>
>> But perhaps I am not seeing the reason why "fail after the user typed"
>> is so disliked and being unnecessarily unsympathetic. I dunno.
> I did not look carefully at the flow of send-email, so this may or may
> not be an issue. But what I think would be _really_ annoying is if you
> asked to write a cover letter, went through the trouble of writing it,
> and then send-email bailed due to some validation failure that could
> have been checked earlier.
>
> There is probably a way to recover your work (presumably we leave it in
> a temporary file somewhere), but it may not be entirely trivial,
> especially for users who are not comfortable with advanced usage of
> their editor. ;)
As I was looking at covering the case of interactive input (--compose) to the fix I noticed that this seems to be at least partly handled by the $compose_filename code. There is a nice output message telling you exactly where the intermediate version of the email you are composing is located if there are errors. I took a quick look inside and can verify that any lost work should be minimal as long as someone knows how to edit files with their editor of choice.
>
> I seem to remember we had one or two such problems in the early days
> with "git commit", where you would go to the trouble to type a commit
> message only to bail on some condition which _could_ have been checked
> earlier. You can recover the message from .git/COMMIT_EDITMSG, but you
> need to remember to do so before re-invoking "git commit", otherwise it
> gets obliterated.
>
> Now for send-email, if your flow is to generate the patches with
> "format-patch", then edit the cover letter separately, and then finally
> ship it all out with "send-email", that might not be an issue. But some
> workflows use the --compose option instead.
>
> -Peff
I have been looking into handling the interactive input cases while solving this issue, but have yet to make a breakthrough. Simply moving the validation code below the original process_address_list code results in a a scenario where I get the email address being seen as something like "ARRAY (0x55ddb951d768)" rather than the email address I wrote in the compose buffer.
^ permalink raw reply
* Re: [PATCH v2 1/2] t: add a test helper to truncate files
From: brian m. carlson @ 2023-10-13 20:23 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Taylor Blau, Jason Hatton
In-Reply-To: <CAPig+cSUvyCS-NOYOoJAmg7LGyU5Dqky5HyS-QTNW1QoHj-0bA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 912 bytes --]
On 2023-10-12 at 17:49:13, Eric Sunshine wrote:
> > + sz = strtoumax(argv[2], &p, 0);
> > + if (*p)
> > + die("invalid size");
>
> Do you want to check 'errno' here, as well (probably before the '*p' check)?
>
> Or is that being too defensive for a 'test-tool' command?
I don't believe that's necessary. The Linux manual page leads me to
believe that if *argv[2] is not 0 but *p is 0, then the entire string is
valid, which would imply that errno is not set. I'm happy to ignore for
the moment the case where the user specifies "" as the argument, because
it is a test helper and "don't do weird, unexpected things with the test
helper without looking at the source code first" is legitimate advice
for Git developers.
If this were user-facing, improved robustness would be warranted, I
agree.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] t: add a test helper to truncate files
From: brian m. carlson @ 2023-10-13 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Taylor Blau, Jason Hatton
In-Reply-To: <xmqqsf6f312s.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
On 2023-10-12 at 22:52:59, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600);
> > + if (fd < 0)
> > + die_errno("failed to open file %s", argv[1]);
>
> contrib/coccinelle/xopen.cocci tells us to write this simply as
>
> fd = xopen(argv[1], O_WRONLY | O_CREAT, 0600);
Sure, I can do that.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: why does git set X in LESS env var?
From: Dragan Simic @ 2023-10-13 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Christoph Anton Mitterer, git
In-Reply-To: <xmqqr0lzhkzk.fsf@gitster.g>
On 2023-10-12 18:19, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Please note that dropping "-X" and leaving "-F" would actually
>> introduce the inconsistency that I already mentioned. To reiterate,
>> short outputs would then remain displayed on screen, while long
>> outputs would disappear after exiting less(1).
>
> Good point.
I've been thinking about this, and a rather elegant, backward-compatible
solution is possible, but it requires some improvements to be made to
less(1) first. I'll reach out to the author of less(1) and propose that
new feature, and I'll let you know his opinion about it.
^ permalink raw reply
* Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:49 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
On Fri, Oct 13, 2023 at 03:25:30PM -0400, Taylor Blau wrote:
> Perform an identical conversion as in previous commits to read the BIDX
> chunk.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> commit-graph.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
Oops. This fails t4216 because it changes the warning() message, which I
thought I excluded from this patch :-<.
Here is a replacement that passes the test. I can reroll the entire
"series" if we decide that this is a worthwhile direction to go in:
--- 8< ---
Subject: [PATCH] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
Perform an identical conversion as in previous commits to read the BIDX
chunk.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 0fab99f5dd..66c2e628d8 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -330,18 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_bloom_index(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != g->num_commits * 4) {
- warning("commit-graph changed-path index chunk is too small");
- return -1;
- }
- g->chunk_bloom_indexes = chunk_start;
- return 0;
-}
-
static int graph_read_bloom_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
}
if (s->commit_graph_read_changed_paths) {
- read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
- graph_read_bloom_index, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,
+ &graph->chunk_bloom_indexes,
+ st_mult(graph->num_commits, 4)) == -1)
+ warning(_("commit-graph changed-path index chunk is too small"));
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
graph_read_bloom_data, graph);
}
--
2.38.0.16.g393fd4c6db
^ permalink raw reply related
* [PATCH 8/8] midx: read `OOFF` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform an identical conversion as in previous commits to read the
OOFF chunk in the MIDX machinery.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/midx.c b/midx.c
index 74167b8fdb..4aeadd5e00 100644
--- a/midx.c
+++ b/midx.c
@@ -61,19 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
strbuf_addf(out, "-%s.rev", hash_to_hex(get_midx_checksum(m)));
}
-static int midx_read_object_offsets(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct multi_pack_index *m = data;
- m->chunk_object_offsets = chunk_start;
-
- if (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {
- error(_("multi-pack-index object offset chunk is the wrong size"));
- return 1;
- }
- return 0;
-}
-
struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)
{
struct multi_pack_index *m = NULL;
@@ -158,8 +145,12 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
error(_("multi-pack-index OID lookup chunk is the wrong size"));
die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
}
- if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
+ if (pair_chunk_expect(cf, MIDX_CHUNKID_OBJECTOFFSETS,
+ &m->chunk_object_offsets,
+ st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH))) {
+ error(_("multi-pack-index object offset chunk is the wrong size"));
die(_("multi-pack-index required object offsets chunk missing or corrupted"));
+ }
pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,
&m->chunk_large_offsets_len);
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 7/8] midx: read `OIDL` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform an identical conversion as in previous commits to read the
OIDL chunk in the MIDX machinery.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/midx.c b/midx.c
index 38bf816cce..74167b8fdb 100644
--- a/midx.c
+++ b/midx.c
@@ -61,19 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
strbuf_addf(out, "-%s.rev", hash_to_hex(get_midx_checksum(m)));
}
-static int midx_read_oid_lookup(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct multi_pack_index *m = data;
- m->chunk_oid_lookup = chunk_start;
-
- if (chunk_size != st_mult(m->hash_len, m->num_objects)) {
- error(_("multi-pack-index OID lookup chunk is the wrong size"));
- return 1;
- }
- return 0;
-}
-
static int midx_read_object_offsets(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -166,8 +153,11 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
}
m->num_objects = ntohl(m->chunk_oid_fanout[255]);
- if (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))
+ if (pair_chunk_expect(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup,
+ st_mult(m->hash_len, m->num_objects))) {
+ error(_("multi-pack-index OID lookup chunk is the wrong size"));
die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
+ }
if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
die(_("multi-pack-index required object offsets chunk missing or corrupted"));
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 6/8] midx: read `OIDF` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform an identical conversion as in previous commits to read the
OIDF chunk in the MIDX machinery.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
midx.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/midx.c b/midx.c
index 2f3863c936..38bf816cce 100644
--- a/midx.c
+++ b/midx.c
@@ -61,20 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
strbuf_addf(out, "-%s.rev", hash_to_hex(get_midx_checksum(m)));
}
-static int midx_read_oid_fanout(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct multi_pack_index *m = data;
- m->chunk_oid_fanout = (uint32_t *)chunk_start;
-
- if (chunk_size != 4 * 256) {
- error(_("multi-pack-index OID fanout is of the wrong size"));
- return 1;
- }
- m->num_objects = ntohl(m->chunk_oid_fanout[255]);
- return 0;
-}
-
static int midx_read_oid_lookup(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -173,8 +159,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
if (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names, &m->chunk_pack_names_len))
die(_("multi-pack-index required pack-name chunk missing or corrupted"));
- if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))
+ if (pair_chunk_expect(cf, MIDX_CHUNKID_OIDFANOUT,
+ (const unsigned char **)&m->chunk_oid_fanout,
+ 256 * sizeof(uint32_t))) {
+ error(_("multi-pack-index OID fanout is of the wrong size"));
die(_("multi-pack-index required OID fanout chunk missing or corrupted"));
+ }
+ m->num_objects = ntohl(m->chunk_oid_fanout[255]);
if (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))
die(_("multi-pack-index required OID lookup chunk missing or corrupted"));
if (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform an identical conversion as in previous commits to read the BIDX
chunk.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 0fab99f5dd..ad98f6334d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -330,18 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_bloom_index(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != g->num_commits * 4) {
- warning("commit-graph changed-path index chunk is too small");
- return -1;
- }
- g->chunk_bloom_indexes = chunk_start;
- return 0;
-}
-
static int graph_read_bloom_data(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
}
if (s->commit_graph_read_changed_paths) {
- read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
- graph_read_bloom_index, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,
+ &graph->chunk_bloom_indexes,
+ st_mult(graph->num_commits, 4)) == -1)
+ warning(_("commit-graph changed-path index chunk is too small (%d)"), graph->num_commits * 4);
read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
graph_read_bloom_data, graph);
}
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 4/8] commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform an identical conversion as in previous commits to read the GDAT
chunk.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 97d4824673..0fab99f5dd 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -330,16 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,
return 0;
}
-static int graph_read_generation_data(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != g->num_commits * sizeof(uint32_t))
- return error("commit-graph generations chunk is wrong size");
- g->chunk_generation_data = chunk_start;
- return 0;
-}
-
static int graph_read_bloom_index(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -457,8 +447,11 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
&graph->chunk_base_graphs_size);
if (s->commit_graph_generation_version >= 2) {
- read_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,
- graph_read_generation_data, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_GENERATION_DATA,
+ &graph->chunk_generation_data,
+ st_mult(graph->num_commits,
+ sizeof(uint32_t))))
+ error(_("commit-graph generations chunk is wrong size"));
pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
&graph->chunk_generation_data_overflow,
&graph->chunk_generation_data_overflow_size);
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 3/8] commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
Perform a similar conversion as in the previous commit read the CDAT
bits.
While we're here, mark the error() string for translation, and guard
against overflow when computing the expected size by wrapping it in an
st_mult() call.
Note that the pre-image of this patch was already sufficiently guarded
against overflow, since GRAPH_DATA_WIDTH is defined as
(the_hash_algo->rawsz + 16), so the expression in the parenthesis would
get performed as a size_t, and then g->num_commits would be promoted to
the width of size_t for the purposes of evaluating this expression.
But let's make it explicitly clear that this computation is safe by
wrapping it in an st_mult() call.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index cdefd7f926..97d4824673 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -330,16 +330,6 @@ static int graph_read_oid_lookup(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 != g->num_commits * GRAPH_DATA_WIDTH)
- 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)
{
@@ -457,7 +447,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
256 * sizeof(uint32_t)))
error(_("commit-graph oid fanout chunk is wrong size"));
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
- read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_DATA,
+ &graph->chunk_commit_data,
+ st_mult(graph->num_commits, GRAPH_DATA_WIDTH)))
+ error(_("commit-graph commit data chunk is wrong size"));
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
&graph->chunk_extra_edges_size);
pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 2/8] commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
The `OIDF` chunk can benefit from the new chunk-format API function
described in the previous commit. Convert it to `pair_chunk_expect()`
accordingly.
While we're at it, let's mark the error() string for translation.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
commit-graph.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 1f334987b5..cdefd7f926 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -321,16 +321,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)
return 0;
}
-static int graph_read_oid_fanout(const unsigned char *chunk_start,
- size_t chunk_size, void *data)
-{
- struct commit_graph *g = data;
- if (chunk_size != 256 * sizeof(uint32_t))
- return error("commit-graph oid fanout chunk is wrong size");
- g->chunk_oid_fanout = (const uint32_t *)chunk_start;
- return 0;
-}
-
static int graph_read_oid_lookup(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
@@ -462,7 +452,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
GRAPH_HEADER_SIZE, graph->num_chunks, 1))
goto free_and_return;
- read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
+ if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,
+ (const unsigned char **)&graph->chunk_oid_fanout,
+ 256 * sizeof(uint32_t)))
+ error(_("commit-graph oid fanout chunk is wrong size"));
read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);
pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 1/8] chunk-format: introduce `pair_chunk_expect()` helper
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
In previous commits, 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_fanout(const unsigned char *chunk_start,
size_t chunk_size, void *data)
{
struct commit_graph *g = data;
if (chunk_size != 256 * sizeof(uint32_t))
return error("commit-graph oid fanout chunk is wrong size");
g->chunk_oid_fanout = (const uint32_t *)chunk_start;
return 0;
}
, leaving the caller to use read_chunk(), like so:
read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);
The callback to read_chunk() (in the above, `graph_read_oid_fanout()`)
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, 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 "size_t" 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_OIDFANOUT,
(const unsigned char **)&graph->chunk_oid_fanout,
256 * sizeof(uint32_t)))
error(_("commit-graph oid fanout chunk is wrong size"));
We will perform those transformations in the following commits.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
chunk-format.c | 22 ++++++++++++++++++++++
chunk-format.h | 12 +++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/chunk-format.c b/chunk-format.c
index cdc7f39b70..9550f15699 100644
--- a/chunk-format.c
+++ b/chunk-format.c
@@ -163,6 +163,8 @@ int read_table_of_contents(struct chunkfile *cf,
struct pair_chunk_data {
const unsigned char **p;
size_t *size;
+
+ size_t expected_size;
};
static int pair_chunk_fn(const unsigned char *chunk_start,
@@ -175,6 +177,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 (pcd->expected_size != chunk_size)
+ return -1;
+ *pcd->p = chunk_start;
+ return 0;
+}
+
int pair_chunk(struct chunkfile *cf,
uint32_t chunk_id,
const unsigned char **p,
@@ -184,6 +197,15 @@ 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 expected_size)
+{
+ struct pair_chunk_data pcd = { .p = p, .expected_size = expected_size };
+ 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..92c529d7ab 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,15 @@ int pair_chunk(struct chunkfile *cf,
const unsigned char **p,
size_t *size);
+/*
+ * Similar to 'pair_chunk', but used for callers who have an expected
+ * size for the given 'chunk_id' in advance.
+ */
+int pair_chunk_expect(struct chunkfile *cf,
+ uint32_t chunk_id,
+ const unsigned char **p,
+ size_t expected_size);
+
typedef int (*chunk_read_fn)(const unsigned char *chunk_start,
size_t chunk_size, void *data);
/*
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply related
* [PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API
From: Taylor Blau @ 2023-10-13 19:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20231009205544.GA3281950@coredump.intra.peff.net>
While reviewing this series, I noticed a couple of spots that would be
helped by having a convenience function that stores the start of a
chunk in a designated location *after* checking that the chunk has the
expected size.
I called this function `pair_chunk_expect()` and touched up seven spots
that I think could benefit from having a convenience function like this.
This applies directly on top of 'jk/chunk-bounds'. Thanks in advance for
your review!
Taylor Blau (8):
chunk-format: introduce `pair_chunk_expect()` helper
commit-graph: read `OIDF` 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 `OIDF` chunk with `pair_chunk_expect()`
midx: read `OIDL` chunk with `pair_chunk_expect()`
midx: read `OOFF` chunk with `pair_chunk_expect()`
chunk-format.c | 22 +++++++++++++++++
chunk-format.h | 12 +++++++++-
commit-graph.c | 65 +++++++++++++-------------------------------------
midx.c | 58 ++++++++++++--------------------------------
4 files changed, 65 insertions(+), 92 deletions(-)
--
2.42.0.352.gd9c5062ff7.dirty
^ permalink raw reply
* Re: [PATCH v5 0/2] attr: add attr.tree config
From: Junio C Hamano @ 2023-10-13 18:52 UTC (permalink / raw)
To: John Cai via GitGitGadget
Cc: git, Jeff King, Jonathan Tan, Eric Sunshine, John Cai
In-Reply-To: <pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v4:
>
> * removed superfluous test
An alternative would have been to point with the ref some non-tree
object like a blob, but as the outcome should be the same as missing
case (from the code --- which is not exactly kosher), it should be
OK.
if (repo_get_oid_treeish(the_repository,
default_attr_source_tree_object_name,
attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));
OOPS! Sorry for not noticing earlier, but repo_get_oid_treeish()
does *NOT* error out when the discovered object is not a treeish, as
the suggested object type is merely supplied for disambiguation
purposes (e.g., with objects 012345 that is a tree and 012346 that
is a blob, you can still ask for treeish "01234" but if you ask for
an object "01234" it will fail).
So, the alternative test would have caught this bug, no? Instead of
silently treating the non-treeish as an empty tree, we would have
died much later when the object supposedly a tree-ish turns out to
be a blob, or something?
^ permalink raw reply
* [Outreachy Applicant]
From: estherugbiedah @ 2023-10-13 18:44 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: christian.couder@gmail.com
In-Reply-To: <355182457.15373219.1697222676128.ref@mail.yahoo.com>
Hello Git Community,
I am an Outreachy applicant for the 2023 cohort and I am thrilled to contribute to the Git project.
Having reviewed the available projects, I am particularly interested in this project. I believe that contributing to this project will not only enhance my skills but also allow me to make a meaningful impact.
I am ready and excited to immerse myself in the project and make valuable contributions.
Thank you for your guidance, and I look forward to working together on this exciting journey.
^ permalink raw reply
* Re: [PATCH 0/2] [Outreachy][PATCH v2] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-13 18:30 UTC (permalink / raw)
To: Isoken June Ibizugbe via GitGitGadget, git; +Cc: Isoken June Ibizugbe
In-Reply-To: <pull.1590.git.git.1697211227.gitgitgadget@gmail.com>
On 13-oct-2023 15:33:45, Isoken June Ibizugbe via GitGitGadget wrote:
> As per the CodingGuidelines document, it is recommended that a single-line
> message provided to error messages such as die(), error() and warning(),
> should start with a lowercase letter and should not end with a period.
>
> Signed-off-by: Isoken June Ibizugbe isokenjune@gmail.com
>
> Isoken June Ibizugbe (2):
> branch.c: ammend error messages for die()
> branch.c: adjust error messages to coding guidelines
I don't understand why two commits. Maybe it was unintentional and you
want to merge all changes into one commit, using git rebase.
>
> builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 33 deletions(-)
You still need to adjust the tests. Take a look at:
https://github.com/git/git/actions/runs/6509642887/job/17681596358
^ permalink raw reply
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Junio C Hamano @ 2023-10-13 18:29 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git
In-Reply-To: <CAJHH8bE15VotFy7QQ-Npmfk9ePvf=-h5SP+Q8phrDBRj8Ti=hQ@mail.gmail.com>
Isoken Ibizugbe <isokenjune@gmail.com> writes:
> I am sorry if I made any mistakes, It's my first time using
> gitgitgadget. This commit was already sent before through git-send
> email. The patch was intended to submit the revisions.
I cannot help with GGG, but it looked strange for this change to be
in two patches, especially since what the rule of deciding which
message is to be touched in which one of the two patches were not
explained anywhere in the series.
I suspect what you want to do is to squash these two commits down
into a single commit, review the resulting code change and also the
commit log message to see if the latter still matches what the
combined patch does, update them as needed, run the test suite
again.
And then format-patch without the cover letter to send the resulting
patch out. Or force push to tell GGG to resend a new iteration
without any cover letter.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-13 18:25 UTC (permalink / raw)
To: Isoken June Ibizugbe via GitGitGadget, git; +Cc: Isoken June Ibizugbe
In-Reply-To: <91e4ad3984147fcc277254a3f6836bf79f5c9550.1697211227.git.gitgitgadget@gmail.com>
On 13-oct-2023 15:33:47, Isoken June Ibizugbe via GitGitGadget wrote:
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
Thanks. This change is not explicitly suggested in the guidelines, but I think
it fits well in the spirit of this series.
> @@ -965,7 +965,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
You have mistakenly deleted this full stop in the previous [1/2] patch.
Therefore, if you stop doing so, you do not need to add it here.
^ 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