* git-retry tool or git.retry config (built-in implementation)?
From: Yaroslav Halchenko @ 2023-09-26 16:47 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Isaac To
Dear Git Gurus,
In DataLad (https://datalad.org) we are doing lots of automated cloning,
fetching etc as part of our CI etc jobs. Once in a while git operations
fail [see e.g. 1], and beg us to retry but we need to know when to
do so, and not do it upon every failed git invocation since some
failures could be legit (repository is gone). While looking how others
solve it we found
https://stackoverflow.com/questions/35014012/git-retry-if-http-request-failed
which pointed to tools like git-retry and later part of
https://chromium.googlesource.com/infra/infra/+/HEAD/go/src/infra/tools/git/retry_regexp.go
which serve as a collection of regexes to be on lookout for to retry.
Would that be the "best" strategy currently?
As regex matching might eventually break whenever `git` changes
anything in the output messages, I wondered if there could be a more
robust internal implementation in git itself? Similarly git-annex has
annex.retry config setting which sets the count of retries for
"retriable" operations.
Do you think something like that could be implemented in git in some
foreseable future?
[1] https://github.com/datalad/datalad/issues/7485
Thank you in advance!
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Junio C Hamano @ 2023-09-26 16:50 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230926143933.GA1897653@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Sep 26, 2023 at 07:19:46AM -0700, Junio C Hamano wrote:
>
>> At the same time, it would be one less thing they need to tweak
>> before starting to use it, and if there are two or more users to do
>> so, it would already have paid off. Developers typically outnumber
>> projects they work on.
>>
>> I also have to wonder if it might make it more obvious what is going
>> on if you made the default to $user/$fork and have the project
>> override it, which hopefully may make it easier to find out what
>> they need to do for those who want to override it to a different
>> value to suit their need?
>
> Yeah, that was my thinking (and what I had been proposing).
>
> But I really think it probably doesn't matter that much either way. I
> would not be surprised if there are zero developers who use this,
> because of the setup on the coverity side, and the fact that the results
> are not always immediately actionable.
>
> Even I, who has been running coverity on my local fork for a few years,
> will probably just switch to using the git.git run and occasionally
> looking at the results (that creates an extra headache because somebody
> has to grant acess to the git.git run results to interested parties, but
> it's also a one-time setup thing).
Sure.
I do not care too much either way, and in a situation like this
where the design decision does not have a crucial longer-term impact
either way exactly because it is a one-time thing for any user,
whoever has invested their work on should have the final say.
Thanks.
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Junio C Hamano @ 2023-09-26 16:42 UTC (permalink / raw)
To: Josh Soref; +Cc: Kristoffer Haugsbakk, Josh Soref via GitGitGadget, git
In-Reply-To: <CACZqfqCfvWASMPMxRpRuAUQPcprcB4RHxxke_qTZ3bFGLf3T8Q@mail.gmail.com>
Josh Soref <jsoref@gmail.com> writes:
> As it (IMO) takes effort (admittedly trivial, and quite likely
> habitual) to insert a second space, I assume that this was a desired
> thing and the one space was the aberration, hence the patch went in
> favor of two spaces.
Back in typewriter days, people were taught to leave two spaces
after a full stop, because on monospace output it was easier to read
that way. Then the desktop publishing software came and people were
encouraged to leave the appearance up to the software.
So to old timers who have already been trained to type two spaces
after a sentence, it takes more effort not to type the space bar
twice.
But as I said already, I personally feel that the consistency
guidelines around this area should be:
(1) it would be nice if a new document does not use mixed style, but
(2) it is not worth the patch noise only to "fix" them one way or
the other.
Here "patch noise" does not primarily mean the effort a producer of
such a patch spends making sure what they did is correct. The cost
of making sure unrelated mistakes do not slip into the codebase and
documentation without being noticed is much higher for such a patch
that ought to be a mechanical conversion.
Thanks.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Kousik Sanagavarapu @ 2023-09-26 16:39 UTC (permalink / raw)
To: Christian Couder
Cc: git, Shuqi Liang, Victoria Dye, Kaartic Sivaraam, Hariom verma
In-Reply-To: <CAP8UFD1bsez-eMis5yH7Esds+LkhMnj0qTUMFPL1tRuDv2fiPw@mail.gmail.com>
Hi,
On Tue, Sep 26, 2023 at 04:48:12PM +0200, Christian Couder wrote:
> By the way, congrats to Shuqi and
> Kousik for successfully completing their projects!
Thanks.
> So Shuqi and Kousik, please tell us if you would like to continue
> working on your projects or if it's Ok if we propose them for
> Outreachy.
I would like to continue working on the project and hopefully complete
the duplication of the remaining pretty formats into ref-fitler.
^ permalink raw reply
* [PATCH] setup: Only allow extenions.objectFormat to be specified once
From: Eric W. Biederman @ 2023-09-26 16:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
Today there is no sanity checking of what happens when
extensions.objectFormat is specified multiple times. Catch confused git
configurations by only allowing this option to be specified once.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
setup.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/setup.c b/setup.c
index 18927a847b86..ef9f79b8885e 100644
--- a/setup.c
+++ b/setup.c
@@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,
if (!strcmp(ext, "noop-v1")) {
return EXTENSION_OK;
} else if (!strcmp(ext, "objectformat")) {
+ struct string_list_item *item;
int format;
if (!value)
@@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,
if (format == GIT_HASH_UNKNOWN)
return error(_("invalid value for '%s': '%s'"),
"extensions.objectformat", value);
+ /* Only support objectFormat being specified once. */
+ for_each_string_list_item(item, &data->v1_only_extensions) {
+ if (!strcmp(item->string, "objectformat"))
+ return error(_("'%s' already specified as '%s'"),
+ "extensions.objectformat",
+ hash_algos[data->hash_algo].name);
+ }
data->hash_algo = format;
return EXTENSION_OK;
}
--
2.41.0
^ permalink raw reply related
* [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin
From: Eric W. Biederman @ 2023-09-26 15:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <87zg1h58xa.fsf@gmail.froward.int.ebiederm.org>
As the code is written today index_bulk_checkin only accepts blobs.
Remove the enum object_type parameter and rename index_bulk_checkin to
index_blob_bulk_checkin, index_stream to index_blob_stream,
deflate_to_pack to deflate_blob_to_pack, stream_to_pack to
stream_blob_to_pack, to make this explicit.
Not supporting commits, tags, or trees has no downside as it is not
currently supported now, and commits, tags, and trees being smaller by
design do not have the problem that the problem that index_bulk_checkin
was built to solve.
Before we start adding code to support the hash function transition
supporting additional objects types in index_bulk_checkin has no real
additional cost, just an extra function parameter to know what the
object type is. Once we begin the hash function transition this is not
the case.
The hash function transition document specifies that a repository with
compatObjectFormat enabled will compute and store both the SHA-1 and
SHA-256 hash of every object in the repository.
What makes this a challenge is that it is not just an additional hash
over the same object. Instead the hash function transition document
specifies that the compatibility hash (specified with
compatObjectFormat) be computed over the equivalent object that another
git repository whose storage hash (specified with objectFormat) would
store. When comparing equivalent repositories built with different
storage hash functions, the oids embedded in objects used to refer to
other objects differ and the location of signatures within objects
differ.
As blob objects have neither oids referring to other objects nor stored
signatures their storage hash and their compatibility hash are computed
over the same object.
The other kinds of objects: trees, commits, and tags, all store oids
referring to other objects. Signatures are stored in commit and tag
objects. As oids and the tags to store signatures are not the same size
in repositories built with different storage hashes the size of the
equivalent objects are also different.
A version of index_bulk_checkin that supports more than just blobs when
computing both the SHA-1 and the SHA-256 of every object added would
need a different, and more expensive structure. The structure is more
expensive because it would be required to temporarily buffering the
equivalent object the compatibility hash needs to be computed over.
A temporary object is needed, because before a hash over an object can
computed it's object header needs to be computed. One of the members of
the object header is the entire size of the object. To know the size of
an equivalent object an entire pass over the original object needs to be
made, as trees, commits, and tags are composed of a variable number of
variable sized pieces. Unfortunately there is no formula to compute the
size of an equivalent object from just the size of the original object.
Avoid all of those future complications by limiting index_bulk_checkin
to only work on blobs.
Inspired-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
bulk-checkin.c | 35 +++++++++++++++++------------------
bulk-checkin.h | 6 +++---
object-file.c | 12 ++++++------
3 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 73bff3a23d27..223562b4e748 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -155,10 +155,10 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
* status before calling us just in case we ask it to call us again
* with a new pack.
*/
-static int stream_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags)
+static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
+ int fd, size_t size, const char *path,
+ unsigned flags)
{
git_zstream s;
unsigned char ibuf[16384];
@@ -170,7 +170,7 @@ static int stream_to_pack(struct bulk_checkin_packfile *state,
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
@@ -247,11 +247,10 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
-static int deflate_to_pack(struct bulk_checkin_packfile *state,
- struct object_id *result_oid,
- int fd, size_t size,
- enum object_type type, const char *path,
- unsigned flags)
+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
{
off_t seekback, already_hashed_to;
git_hash_ctx ctx;
@@ -265,7 +264,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
return error("cannot find the current offset");
header_len = format_object_header((char *)obuf, sizeof(obuf),
- type, size);
+ OBJ_BLOB, size);
the_hash_algo->init_fn(&ctx);
the_hash_algo->update_fn(&ctx, obuf, header_len);
@@ -282,8 +281,8 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,
idx->offset = state->offset;
crc32_begin(state->f);
}
- if (!stream_to_pack(state, &ctx, &already_hashed_to,
- fd, size, type, path, flags))
+ if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
+ fd, size, path, flags))
break;
/*
* Writing this object to the current pack will make
@@ -350,12 +349,12 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)
}
}
-int index_bulk_checkin(struct object_id *oid,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags)
+int index_blob_bulk_checkin(struct object_id *oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
{
- int status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,
- path, flags);
+ int status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,
+ path, flags);
if (!odb_transaction_nesting)
flush_bulk_checkin_packfile(&bulk_checkin_packfile);
return status;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 48fe9a6e9171..aa7286a7b3e1 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -9,9 +9,9 @@
void prepare_loose_object_bulk_checkin(void);
void fsync_loose_object_bulk_checkin(int fd, const char *filename);
-int index_bulk_checkin(struct object_id *oid,
- int fd, size_t size, enum object_type type,
- const char *path, unsigned flags);
+int index_blob_bulk_checkin(struct object_id *oid,
+ int fd, size_t size,
+ const char *path, unsigned flags);
/*
* Tell the object database to optimize for adding
diff --git a/object-file.c b/object-file.c
index 7dc0c4bfbba8..7c7afe579364 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2446,11 +2446,11 @@ static int index_core(struct index_state *istate,
* binary blobs, they generally do not want to get any conversion, and
* callers should avoid this code path when filters are requested.
*/
-static int index_stream(struct object_id *oid, int fd, size_t size,
- enum object_type type, const char *path,
- unsigned flags)
+static int index_blob_stream(struct object_id *oid, int fd, size_t size,
+ const char *path,
+ unsigned flags)
{
- return index_bulk_checkin(oid, fd, size, type, path, flags);
+ return index_blob_bulk_checkin(oid, fd, size, path, flags);
}
int index_fd(struct index_state *istate, struct object_id *oid,
@@ -2472,8 +2472,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,
ret = index_core(istate, oid, fd, xsize_t(st->st_size),
type, path, flags);
else
- ret = index_stream(oid, fd, xsize_t(st->st_size), type, path,
- flags);
+ ret = index_blob_stream(oid, fd, xsize_t(st->st_size), path,
+ flags);
close(fd);
return ret;
}
--
2.41.0
^ permalink raw reply related
* Projects for the next Outreachy round
From: Christian Couder @ 2023-09-26 14:48 UTC (permalink / raw)
To: git, Kousik Sanagavarapu, Shuqi Liang
Cc: Victoria Dye, Kaartic Sivaraam, Hariom verma
Hi everyone, especially Shuqi and Kousik,
September 29 is the deadline for proposing Outreachy projects. As we
have at least one mentor for now (me), we should propose at least one
project though.
All ideas for projects are welcome, don't hesitate to suggest one! But
the simplest would be to continue one of the projects we proposed for
this year's GSoC which is just over. By the way, congrats to Shuqi and
Kousik for successfully completing their projects!
So Shuqi and Kousik, please tell us if you would like to continue
working on your projects or if it's Ok if we propose them for
Outreachy.
About "Unifying the pretty and ref-filter formats", I don't think
there are a lot more atoms to work on, but maybe that would be enough
work if Kousik stopped working on it after his current work on mailmap
options for some atom is merged (which might happen soon).
About the "More Sparse Index Integrations" Shuqi worked on, mentored
by Victoria, I am likely not the best person to mentor it, but I think
I could manage. It would be nice though if I got an idea about what
should be done next and how much work is left in general in this area.
(Shuqi's GSoC final report at
https://cheskaqiqi.github.io/2023/08/22/Final/ doesn't talk much about
this.) Perhaps even if Shuqi is continuing to work on the project,
there is still work that could be done in parallel on other commands
than the ones he is working on.
Sorry for the late request, but please let me know soon about this.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Jeff King @ 2023-09-26 14:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <4e0f404e-0c49-b085-c35d-be53b0381402@gmx.de>
On Tue, Sep 26, 2023 at 04:02:27PM +0200, Johannes Schindelin wrote:
> > > > This may not matter all that much, because I don't expect most
> > > > people to set this up for their forks
> > >
> > > Except, of course, Git for Windows. And that has been the entire
> > > motivation for me to work on this here patch series in the first
> > > place, so it would be rather pointless if I could not override this in
> > > the git-for-windows/git fork.
> >
> > I didn't at all mean that it should not be possible to override it.
>
> Aha! The "This" in "This may not matter all that much" was referring to
> the `git` instead of the `COVERITY_PROJECT` part. That had not been clear
> to me.
I think we are on the same page now, but I wanted to elaborate here
because this miscommunication interested me. The "this" in what I wrote
was actually the "X" in:
This may not matter because of $FOO, but X.
The X got snipped in what you quoted, but was "But I just wondered if a
better default would would be...". I certainly did not help with
readability by inserting a huge parenthetical phrase and putting in a
full-stop and starting the new sentence on "But" (my ninth grade English
teacher would be horrified).
I mention it mostly as a note to myself about trying to make sure I keep
my writing clear and readable (and a cautionary tale for others!).
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Jeff King @ 2023-09-26 14:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqq4jjhow3h.fsf@gitster.g>
On Tue, Sep 26, 2023 at 07:19:46AM -0700, Junio C Hamano wrote:
> At the same time, it would be one less thing they need to tweak
> before starting to use it, and if there are two or more users to do
> so, it would already have paid off. Developers typically outnumber
> projects they work on.
>
> I also have to wonder if it might make it more obvious what is going
> on if you made the default to $user/$fork and have the project
> override it, which hopefully may make it easier to find out what
> they need to do for those who want to override it to a different
> value to suit their need?
Yeah, that was my thinking (and what I had been proposing).
But I really think it probably doesn't matter that much either way. I
would not be surprised if there are zero developers who use this,
because of the setup on the coverity side, and the fact that the results
are not always immediately actionable.
Even I, who has been running coverity on my local fork for a few years,
will probably just switch to using the git.git run and occasionally
looking at the results (that creates an extra headache because somebody
has to grant acess to the git.git run results to interested parties, but
it's also a one-time setup thing).
-Peff
^ permalink raw reply
* Re: [PATCH v3 5/6] git-std-lib: introduce git standard library
From: phillip.wood123 @ 2023-09-26 14:23 UTC (permalink / raw)
To: Calvin Wan, git; +Cc: nasamuffin, jonathantanmy, linusa, vdye
In-Reply-To: <20230908174443.1027716-5-calvinwan@google.com>
Hi Calvin
On 08/09/2023 18:44, Calvin Wan wrote:
> The Git Standard Library intends to serve as the foundational library
> and root dependency that other libraries in Git will be built off of.
> That is to say, suppose we have libraries X and Y; a user that wants to
> use X and Y would need to include X, Y, and this Git Standard Library.
>
> Add Documentation/technical/git-std-lib.txt to further explain the
> design and rationale.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> ---
> Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++++++
I need the following diff to build the html documentation.
Best Wishes
Phillip
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3f2383a12c..f1dc673838 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -110,6 +110,7 @@ TECH_DOCS += SubmittingPatches
TECH_DOCS += ToolsForGit
TECH_DOCS += technical/bitmap-format
TECH_DOCS += technical/bundle-uri
+TECH_DOCS += technical/git-std-lib
TECH_DOCS += technical/hash-function-transition
TECH_DOCS += technical/long-running-process-protocol
TECH_DOCS += technical/multi-pack-index
diff --git a/Documentation/technical/git-std-lib.txt b/Documentation/technical/git-std-lib.txt
index d03b8565b4..28e6cdac2a 100644
--- a/Documentation/technical/git-std-lib.txt
+++ b/Documentation/technical/git-std-lib.txt
@@ -1,5 +1,4 @@
-Git Standard Library
-================
+= Git Standard Library
The Git Standard Library intends to serve as the foundational library
and root dependency that other libraries in Git will be built off of.
@@ -9,8 +8,7 @@ This does not mean that the Git Standard Library will be the only
possible root dependency in the future, but rather the most significant
and widely used one.
-Dependency graph in libified Git
-================
+== Dependency graph in libified Git
If you look in the Git Makefile, all of the objects defined in the Git
library are compiled and archived into a singular file, libgit.a, which
@@ -57,8 +55,7 @@ if someone wanted their own custom build of Git with their own custom
implementation of the object store, they would only have to swap out
object-store.a rather than do a hard fork of Git.
-Rationale behind Git Standard Library
-================
+== Rationale behind Git Standard Library
The rationale behind what's in and what's not in the Git Standard
Library essentially is the result of two observations within the Git
@@ -67,8 +64,7 @@ in a couple of different files, and wrapper.c + usage.c have
difficult-to-separate circular dependencies with each other and other
files.
-Ubiquity of git-compat-util.h and circular dependencies
-========
+=== Ubiquity of git-compat-util.h and circular dependencies
Every file in the Git codebase includes git-compat-util.h. It serves as
"a compatibility aid that isolates the knowledge of platform specific
@@ -79,10 +75,9 @@ for wrapper.c and usage.c to be a part of the root library. They have
difficult to separate circular dependencies with each other so they
can't be independent libraries. Wrapper.c has dependencies on parse.c,
abspath.c, strbuf.c, which in turn also have dependencies on usage.c and
-wrapper.c -- more circular dependencies.
+wrapper.c - more circular dependencies.
-Tradeoff between swappability and refactoring
-========
+=== Tradeoff between swappability and refactoring
From the above dependency graph, we can see that git-std-lib.a could be
many smaller libraries rather than a singular library. So why choose a
@@ -99,8 +94,7 @@ and change the API for the library if there becomes enough of a reason
to do so (remember we are avoiding promising stability of the interfaces
of those libraries).
-Reuse of compatibility functions in git-compat-util.h
-========
+=== Reuse of compatibility functions in git-compat-util.h
Most functions defined in git-compat-util.h are implemented in compat/
and have dependencies limited to strbuf.h and wrapper.h so they can be
@@ -110,8 +104,7 @@ compat/. The rest of the functions defined in git-compat-util.h are
implemented in top level files and are hidden behind
an #ifdef if their implementation is not in git-std-lib.a.
-Rationale summary
-========
+=== Rationale summary
The Git Standard Library allows us to get the libification ball rolling
with other libraries in Git. By not spending many
@@ -123,8 +116,7 @@ the code cleanups that have happened so far have been minor and
beneficial for the codebase. It is probable that making large movements
would negatively affect code clarity.
-Git Standard Library boundary
-================
+== Git Standard Library boundary
While I have described above some useful heuristics for identifying
potential candidates for git-std-lib.a, a standard library should not
@@ -150,8 +142,7 @@ to be able to trace functions in those files and other files in git-std-lib.a.
In order for git-std-lib.a to compile with those dependencies, stubbed out
versions of those files are implemented and swapped in during compilation time.
-Files inside of Git Standard Library
-================
+== Files inside of Git Standard Library
The initial set of files in git-std-lib.a are:
abspath.c
@@ -171,21 +162,19 @@ complete library:
stubs/pager.c
stubs/trace2.c
-Pitfalls
-================
+== Pitfalls
There are a small amount of files under compat/* that have dependencies
not inside of git-std-lib.a. While those functions are not called on
Linux, other OSes might call those problematic functions. I don't see
this as a major problem, just moreso an observation that libification in
general may also require some minor compatibility work in the future.
-Testing
-================
+== Testing
Unit tests should catch any breakages caused by changes to files in
git-std-lib.a (i.e. introduction of a out of scope dependency) and new
functions introduced to git-std-lib.a will require unit tests written
for them.
-[1] https://lore.kernel.org/git/xmqqwn17sydw.fsf@gitster.g/
\ No newline at end of file
+[1] https://lore.kernel.org/git/xmqqwn17sydw.fsf@gitster.g/
^ permalink raw reply related
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Junio C Hamano @ 2023-09-26 14:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <4e0f404e-0c49-b085-c35d-be53b0381402@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I meant that the default should be $user/$fork from the Actions
>> environment,
>
> I'd much rather default to the value needed by the Git project than a
> value that may be needed by any other user. I do aim to support the Git
> project with this patch series, first and foremost.
I appreciate the sentiment.
At the same time, it would be one less thing they need to tweak
before starting to use it, and if there are two or more users to do
so, it would already have paid off. Developers typically outnumber
projects they work on.
I also have to wonder if it might make it more obvious what is going
on if you made the default to $user/$fork and have the project
override it, which hopefully may make it easier to find out what
they need to do for those who want to override it to a different
value to suit their need?
Thanks.
^ permalink raw reply
* Re: [PATCH 3/6] coverity: allow overriding the Coverity project
From: Johannes Schindelin @ 2023-09-26 14:02 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <20230925121157.GB1623701@coredump.intra.peff.net>
Hi Jeff,
On Mon, 25 Sep 2023, Jeff King wrote:
> On Mon, Sep 25, 2023 at 01:52:47PM +0200, Johannes Schindelin wrote:
>
> > On Sat, 23 Sep 2023, Jeff King wrote:
> >
> > > On Fri, Sep 22, 2023 at 10:42:00AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > +# By default, the builds are submitted to the Coverity project
> > > > +# `git`. To override this, set the repository variable
> > > > +# `COVERITY_PROJECT`.
> > >
> > > This may not matter all that much, because I don't expect most
> > > people to set this up for their forks
> >
> > Except, of course, Git for Windows. And that has been the entire
> > motivation for me to work on this here patch series in the first
> > place, so it would be rather pointless if I could not override this in
> > the git-for-windows/git fork.
>
> I didn't at all mean that it should not be possible to override it.
Aha! The "This" in "This may not matter all that much" was referring to
the `git` instead of the `COVERITY_PROJECT` part. That had not been clear
to me.
> I meant that the default should be $user/$fork from the Actions
> environment,
I'd much rather default to the value needed by the Git project than a
value that may be needed by any other user. I do aim to support the Git
project with this patch series, first and foremost.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 0/6] Add a GitHub workflow to submit builds to Coverity Scan
From: Johannes Schindelin @ 2023-09-26 13:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Jeff King
In-Reply-To: <xmqqy1guuq3w.fsf@gitster.g>
Hi Junio,
On Mon, 25 Sep 2023, Junio C Hamano wrote:
> One thing that caught my eye was the asterisks around "22" that look
> as if they were designed to confuse readers [...]
>
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > [...]
> > ++ case $? in
> > ++ 0) ;; # okay
> > ++ *22*) # 40x, i.e. access denied
> > ++ echo "::error::incorrect token or project?" >&2
> > + exit 1
> > + ;;
>
> [...]
>
> Will replace but I may be tempted to edit those asterisks out myself
> while queueing.
D'oh, of course. Thank you,
Johannes
^ permalink raw reply
* Re: Request for Curl.exe update included in Git binaries
From: Johannes Schindelin @ 2023-09-26 13:53 UTC (permalink / raw)
To: brian m. carlson; +Cc: Robert Smith, git@vger.kernel.org
In-Reply-To: <ZRIBmxT4mh5Rlt3-@tapette.crustytoothpaste.net>
Hi brian and Robert,
On Mon, 25 Sep 2023, brian m. carlson wrote:
> On 2023-09-25 at 15:37:46, Robert Smith wrote:
>
> > Regarding this CVE:
> >
> > https://curl.se/docs/CVE-2023-38039.html
In the future, please consider sending security-relevant enquiries to
git-security@googlegroups.com instead of the regular Git mailing list.
In this case, not much harm was done, but let's not risk anything. I say
not much harm was done because that CVE would appear to be very low risk.
The cURL project itself says this:
When curl retrieves an HTTP response, it stores the incoming
headers so that they can be accessed later via the libcurl headers
API.
However, curl did not have a limit on the size or quantity of
headers it would accept in a response, allowing a malicious server
to stream an endless series of headers to a client and eventually
cause curl to run out of heap memory.
So the most damage that can be done by exploiting this vulnerability is to
host a Git server from which a user targeted by the attack simply cannot
clone because the process will fail with an out-of-memory condition.
The Git for Windows project carefully vets any security updates of any of
the components distributed with Git for Windows, and if it is determined
that they constitute a vulnerability that can be exploited via regular Git
usage, we aim to release a new version as swiftly as possible.
In this instance, it was determined that the severity is low (deviating
from cURL's assessment because Git's usage of libcurl has a narrower focus
than general cURL usage), and no new Git for Windows version was deemed
necessary.
> > Is there any plan to update Git for Windows to include the updated
> > 8.3.0 Curl binaries?
Ever since https://github.com/git-for-windows/git/issues/4605 was
addressed, the v8.3.0 cURL binaries have been ready to go for the next Git
for Windows version.
> The Git project doesn't ship any binaries at all, and we don't ship
> curl. Git for Windows does ship a substantial amount of other software,
> including curl. You can find their issue tracker at
> https://github.com/git-for-windows/git/issues, but I believe this has
> already been fixed in https://github.com/git-for-windows/git/issues/4605
> and will be included in the next version.
Precisely.
> I'm not certain about their release policy,
Git for Windows' release policy is documented at
https://github.com/git-for-windows/git/security/policy.
> but I seem to recall that they don't issue updates for dependent
> packages until a new release would normally be done. To be certain,
> you'd have to inquire with them.
Git for Windows does follow "upstream" Git releases. That is, every
official Git version on the latest major version release train is followed
shortly thereafter with a corresponding Git for Windows version.
As documented at
https://github.com/git-for-windows/git/security/policy#version-number-scheme
sometimes Git for Windows releases versions that do _not_ correspond to
upstream Git versions. Reasons for that include security bug fixes in
dependencies that affect Git usage, and bug fixes that are specific to Git
for Windows which are considered important enough to deliver to Git for
Windows users as quickly as possible.
In this instance, I do not see any reason to risk upgrade fatigue and
expect to publish the first Git for Windows version that includes cURL
v8.3.0 in the wake of Git v2.43.0 (slated for November 20th, 2023, see
https://gh.io/gitCal).
Robert, if you still feel very strongly that you need to have a Git for
Windows that includes an updated `curl.exe`, I invite you to install the
latest snapshot at https://wingit.blob.core.windows.net/files/index.html.
These snapshots are designed to be as robust and dependable as full Git
for Windows releases, the only difference being that snapshots are
released with every update to Git for Windows' `main` branch, i.e. at a
much faster cadence than official Git for Windows versions.
Ciao,
Johannes
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas
From: Bagas Sanjaya @ 2023-09-26 11:33 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Taylor Blau,
Git Mailing List
In-Reply-To: <20230925080010.GA1534025@coredump.intra.peff.net>
On 25/09/2023 15:00, Jeff King wrote:
> Do you have any other send-email related config? Can you show us the
> output of "git config --list"?
>
Hi Jeff, sure here is the output from my git.git clone:
```
$ git config --list
core.abbrev=10
alias.am-failed=am --show-current-patch=diff
alias.fixwhat=show -s --pretty=fixes
user.name=Bagas Sanjaya
user.email=bagasdotme@gmail.com
user.signingkey=D0A10F5A447A37A42A63419FD7B55A665D5C863D
format.signature=An old man doll... just what I always wanted! - Clara
diff.algorithm=histogram
sendemail.smtpencryption=tls
sendemail.smtpserver=smtp.gmail.com
sendemail.smtpserverport=587
sendemail.smtpuser=bagasdotme@gmail.com
sendemail.smtppass=<app password>
pack.deltacachesize=225M
pack.window=13
pack.windowmemory=460M
pack.threads=2
pretty.fixes=Fixes: %h ("%s")
pretty.fixup=fixup for "%s"
pretty.kreference=%h ("%s")
pretty.upstream=commit %H upstream.
pretty.upstreamsasha=[ Upstream commit %H ]
merge.conflictstyle=diff3
tar.xz.command=xz -c
tar.zst.command=zstd -c
diff.algorithm=histogram
filter.lfs.clean=git-lfs clean -- %f
filter.lfs.smudge=git-lfs smudge -- %f
filter.lfs.process=git-lfs filter-process
filter.lfs.required=true
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
tag.sort=creatordate
remote.origin.url=https://git.kernel.org/pub/scm/git/git.git
remote.origin.fetch=+refs/heads/master:refs/remotes/origin/master
remote.origin.fetch=+refs/heads/main:refs/remotes/origin/main
remote.origin.fetch=+refs/heads/next:refs/remotes/origin/next
remote.origin.fetch=+refs/heads/seen:refs/remotes/origin/seen
remote.origin.fetch=+refs/heads/maint:refs/remotes/origin/maint
remote.origin.fetch=+refs/heads/todo:refs/remotes/origin/todo
sendemail.sendmailcmd=/usr/sbin/sendmail
sendemail.envelopesender=auto
branch.master.remote=origin
branch.master.merge=refs/heads/master
branch.next.remote=origin
branch.next.merge=refs/heads/next
branch.seen.remote=origin
branch.seen.merge=refs/heads/seen
branch.maint.remote=origin
branch.maint.merge=refs/heads/maint
branch.todo.remote=origin
branch.todo.merge=refs/heads/todo
sendemail.sendmailcmd=/usr/sbin/sendmail
sendemail.envelopesender=auto
branch.main.remote=origin
branch.main.merge=refs/heads/main
remote.gitnode.url=git@gitnode.io:bagas/git.git
remote.gitnode.fetch=+refs/heads/*:refs/remotes/gitnode/*
```
Thanks.
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Sergey Organov @ 2023-09-26 9:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqjzsdps0h.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> P.S. I also figure that maybe our divergence comes from the fact that I
>> consider merge commits to be primarily commits (introducing particular
>> set of changes, and then having reference to the source of the changes),
>> whereas you consider them primarily merges (joining two histories, and
>> then maybe some artificial changes that make merges "evil"). That's why
>> we often end up agreeing to disagree, as both these points of view seem
>> pretty valid.
>
> It rarely is the case that two opposing world views are equally
> valid, though.
Yes. In this particular case the two are not opposing though, rather
orthogonal, as they reflect the intrinsic dualism of the concept of
"merge commit". Merge commit is both a new state, and history junction,
neither of which is more or less valid or essential, and I use both
views myself, depending on situation.
An electron is both a particle and a wave, and one just uses its side
that is more convenient for explanation of the case in hand.
I promote features that I routinely need in my workflows, yet respecting
the other side of the coin as well, even though I may rarely find this
other side useful. I mean, for me, this -c/--cc (let alone -m) output is
only confusing, yet I won't be saying that it's somehow less valid than
proposed -d.
> If there were an option that forbids any comparison output from a
> single parent commit (say --ndfnm "no-diff-for-non-merge"),
> then those with "merges are the primary thing, single-parent commits
> on the merged branches are implementation details" worldview would be
> commonly using "--diff-merges=first-parent --patch --ndfnm" and (1)
> viewing only the combined effect of merging side branches without
> seeing noise from individual commits whose effects are already shown
> in these merges, and (2) traversing the side branches as well, so that
> merges from side-side branches into the side branches are viewable the
> same way as merges into the mainline.
No need to ask for a new option, as the behavior you describe is already
there, and is spelled "git log --diff-merges=first-parent"
(--diff-merges=1 for short).
> But because no such option exists and nobody asked for such an
> option during the whole lifetime of the project, I highly doubt
> that it is a valid world view with wide backing from the users.
Your concern above seems to be void, so this doesn't hold either.
As a side-note though, something like this has been asked recently, as
what you describe by --ndfnm should in fact have been what --no-patch
does, but surprisingly does not (please recall recent discussion of this
issue).
> Even if it were a valid world view with wide backing,
Apparently it is valid, otherwise there would be no need for diff to
first parent at all, let alone "git log --first-parent -p" have used it
by default.
> the combination "--diff-merges=first-parent --patch" would be less
> than ideal presentation for them (due to lack of "--ndfnm").
First, as we figured above, --ndfnm is not needed, and second, me, being
one of "them", tries hard to convince you it is the best presentation
"them" can get, while "ideal" simply never exists.
> And as I already said, it would not be useful without --first-parent
> traversal for the worldview git has supported.
Yes, you said it earlier in this thread, and as well I already explained
how it is useful without --first-parent.
> That is why I find it of dubious value to let short-and-sweet '-d'
> be squatted by less than ideal "--diff-merges=first-parent --patch"
> combination.
Hopefully I do understand your concerns, yet I believe
"--diff-merges=first-parent --patch" is way better for "-d" shortcut
than "--first-parent --patch", for the reasons I already explained
earlier in this thread.
> Shorthands are scarse resources, and we want to be careful before
> handing them out.
Yep, agreed.
I believe I carefully thought it over though, weighing all pros and
cons, and thus -d fits nicely among -c and --cc, being yet another
frequently desired format for merges, plays nice with -p as well, and is
very mnemonic, giving us convenient, user-friendly, and consistent user
interface overall.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2 3/3] pkt-line: do not chomp newlines for sideband messages
From: Oswald Buddenhagen @ 2023-09-26 8:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jiang Xin, Git List, Jonathan Tan, Jiang Xin
In-Reply-To: <xmqqa5t9rkft.fsf@gitster.g>
>Jiang Xin <worldhello.net@gmail.com> writes:
>
>> +++ b/pkt-line.c
>> @@ -462,8 +462,33 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>> }
>> + case 2:
>> + /* fallthrough */
>> + case 3:
>
while not entirely unprecedented, it's unnecessary and even
counter-productive to annotate directly adjacent cases with fallthrough.
regards
^ permalink raw reply
* [PATCH v4 4/4] trailer: only use trailer_block_* variables if trailers were found
From: Linus Arver via GitGitGadget @ 2023-09-26 6:22 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously, these variables were overloaded to act as the end of the log
message even if no trailers were found.
Remove the overloaded meaning by adding a new end_of_log_message field
to the trailer_info struct. The trailer_info struct consumers now only
refer to the trailer_block_start and trailer_block_end fields if
trailers were found (trailer_nr > 0), and otherwise refer to the
end_of_log_message.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 31 +++++++++++++++++++++++--------
trailer.h | 12 +++++++-----
2 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/trailer.c b/trailer.c
index 3dc2faa969c..c11839ae365 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1083,9 +1083,14 @@ void process_trailers(const char *file,
parse_trailers(&info, sb.buf, &head, opts);
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ /* Print the lines before the trailers (if any) as is. */
+ if (!opts->only_trailers) {
+ if (info.trailer_nr) {
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ } else {
+ fwrite(sb.buf, 1, info.end_of_log_message, outfile);
+ }
+ }
if (!opts->only_trailers && !info.blank_line_before_trailer)
fprintf(outfile, "\n");
@@ -1105,9 +1110,14 @@ void process_trailers(const char *file,
free_all(&head);
trailer_info_release(&info);
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ /* Print the lines after the trailers (if any) as is. */
+ if (!opts->only_trailers) {
+ if (info.trailer_nr) {
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ } else {
+ fwrite(sb.buf + info.end_of_log_message, 1, sb.len - info.end_of_log_message, outfile);
+ }
+ }
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
@@ -1153,8 +1163,13 @@ void trailer_info_get(struct trailer_info *info, const char *str,
info->blank_line_before_trailer = ends_with_blank_line(str,
trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
+ info->trailer_block_start = 0;
+ info->trailer_block_end = 0;
+ if (nr) {
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
+ }
+ info->end_of_log_message = end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
diff --git a/trailer.h b/trailer.h
index 70d7b8bf1d8..d1e8751952b 100644
--- a/trailer.h
+++ b/trailer.h
@@ -38,14 +38,16 @@ struct trailer_info {
/*
* Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input, per find_true_end_of_input().
- *
- * NOTE: This will be changed so that these point to 0 in the next
- * patch if no trailers are found.
+ * string. If no trailer block is found, these are set to 0.
*/
size_t trailer_block_start, trailer_block_end;
+ /*
+ * Offset to the end of the log message in the input (may not be the
+ * same as the end of the input).
+ */
+ size_t end_of_log_message;
+
/*
* Array of trailers found.
*/
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 3/4] trailer: use offsets for trailer_start/trailer_end
From: Linus Arver via GitGitGadget @ 2023-09-26 6:22 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously these fields in the trailer_info struct were of type "const
char *" and pointed to positions in the input string directly (to the
start and end positions of the trailer block).
Use offsets to make the intended usage less ambiguous. We only need to
reference the input string in format_trailer_info(), so update that
function to take a pointer to the input.
While we're at it, rename trailer_start to trailer_block_start to be
more explicit about these offsets (that they are for the entire trailer
block including other trailers). Ditto for trailer_end.
Signed-off-by: Linus Arver <linusa@google.com>
---
sequencer.c | 2 +-
trailer.c | 29 ++++++++++++++---------------
trailer.h | 13 ++++++++-----
3 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed9..8707a92204f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -345,7 +345,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
- if (info.trailer_start == info.trailer_end)
+ if (info.trailer_block_start == info.trailer_block_end)
return 0;
for (i = 0; i < info.trailer_nr; i++)
diff --git a/trailer.c b/trailer.c
index 96cb285a4ea..3dc2faa969c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -856,7 +856,7 @@ static size_t find_end_of_log_message(const char *input, int no_divider)
* Return the position of the first trailer line or len if there are no
* trailers.
*/
-static size_t find_trailer_start(const char *buf, size_t len)
+static size_t find_trailer_block_start(const char *buf, size_t len)
{
const char *s;
ssize_t end_of_title, l;
@@ -1072,7 +1072,6 @@ void process_trailers(const char *file,
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
struct trailer_info info;
- size_t trailer_end;
FILE *outfile = stdout;
ensure_configured();
@@ -1083,11 +1082,10 @@ void process_trailers(const char *file,
outfile = create_in_place_tempfile(file);
parse_trailers(&info, sb.buf, &head, opts);
- trailer_end = info.trailer_end - sb.buf;
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile);
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
if (!opts->only_trailers && !info.blank_line_before_trailer)
fprintf(outfile, "\n");
@@ -1109,7 +1107,7 @@ void process_trailers(const char *file,
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
@@ -1121,7 +1119,7 @@ void process_trailers(const char *file,
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
- int end_of_log_message, trailer_start;
+ size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
size_t nr = 0, alloc = 0;
@@ -1130,10 +1128,10 @@ void trailer_info_get(struct trailer_info *info, const char *str,
ensure_configured();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
- trailer_start = find_trailer_start(str, end_of_log_message);
+ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
- trailer_lines = strbuf_split_buf(str + trailer_start,
- end_of_log_message - trailer_start,
+ trailer_lines = strbuf_split_buf(str + trailer_block_start,
+ end_of_log_message - trailer_block_start,
'\n',
0);
for (ptr = trailer_lines; *ptr; ptr++) {
@@ -1154,9 +1152,9 @@ void trailer_info_get(struct trailer_info *info, const char *str,
strbuf_list_free(trailer_lines);
info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_start);
- info->trailer_start = str + trailer_start;
- info->trailer_end = str + end_of_log_message;
+ trailer_block_start);
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
@@ -1171,6 +1169,7 @@ void trailer_info_release(struct trailer_info *info)
static void format_trailer_info(struct strbuf *out,
const struct trailer_info *info,
+ const char *msg,
const struct process_trailer_options *opts)
{
size_t origlen = out->len;
@@ -1180,8 +1179,8 @@ static void format_trailer_info(struct strbuf *out,
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, info->trailer_start,
- info->trailer_end - info->trailer_start);
+ strbuf_add(out, msg + info->trailer_block_start,
+ info->trailer_block_end - info->trailer_block_start);
return;
}
@@ -1235,7 +1234,7 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg,
struct trailer_info info;
trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, opts);
+ format_trailer_info(out, &info, msg, opts);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index ab2cd017567..70d7b8bf1d8 100644
--- a/trailer.h
+++ b/trailer.h
@@ -32,16 +32,19 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
struct trailer_info {
/*
* True if there is a blank line before the location pointed to by
- * trailer_start.
+ * trailer_block_start.
*/
int blank_line_before_trailer;
/*
- * Pointers to the start and end of the trailer block found. If there
- * is no trailer block found, these 2 pointers point to the end of the
- * input string.
+ * Offsets to the trailer block start and end positions in the input
+ * string. If no trailer block is found, these are both set to the
+ * "true" end of the input, per find_true_end_of_input().
+ *
+ * NOTE: This will be changed so that these point to 0 in the next
+ * patch if no trailers are found.
*/
- const char *trailer_start, *trailer_end;
+ size_t trailer_block_start, trailer_block_end;
/*
* Array of trailers found.
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 2/4] trailer: find the end of the log message
From: Linus Arver via GitGitGadget @ 2023-09-26 6:22 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Previously, trailer_info_get() computed the trailer block end position
by
(1) checking for the opts->no_divider flag and optionally calling
find_patch_start() to find the "patch start" location (patch_start), and
(2) calling find_trailer_end() to find the end of the trailer block
using patch_start as a guide, saving the return value into
"trailer_end".
The logic in (1) was awkward because the variable "patch_start" is
misleading if there is no patch in the input. The logic in (2) was
misleading because it could be the case that no trailers are in the
input (yet we are setting a "trailer_end" variable before even searching
for trailers, which happens later in find_trailer_start()). The name
"find_trailer_end" was misleading because that function did not look for
any trailer block itself --- instead it just computed the end position
of the log message in the input where the end of the trailer block (if
it exists) would be (because trailer blocks must always come after the
end of the log message).
Combine the logic in (1) and (2) together into find_patch_start() by
renaming it to find_end_of_log_message(). The end of the log message is
the starting point which find_trailer_start() needs to start searching
backward to parse individual trailers (if any).
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 61 ++++++++++++++++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/trailer.c b/trailer.c
index 3c54b38a85a..96cb285a4ea 100644
--- a/trailer.c
+++ b/trailer.c
@@ -809,21 +809,47 @@ static ssize_t last_line(const char *buf, size_t len)
}
/*
- * Return the position of the start of the patch or the length of str if there
- * is no patch in the message.
+ * Find the end of the log message as an offset from the start of the input
+ * (where callers of this function are interested in looking for a trailers
+ * block in the same input). We have to consider two categories of content that
+ * can come at the end of the input which we want to ignore (because they don't
+ * belong in the log message):
+ *
+ * (1) the "patch part" which begins with a "---" divider and has patch
+ * information (like the output of git-format-patch), and
+ *
+ * (2) any trailing comment lines, blank lines like in the output of "git
+ * commit -v", or stuff below the "cut" (scissor) line.
+ *
+ * As a formula, the situation looks like this:
+ *
+ * INPUT = LOG MESSAGE + IGNORED
+ *
+ * where IGNORED can be either of the two categories described above. It may be
+ * that there is nothing to ignore. Now it may be the case that the LOG MESSAGE
+ * contains a trailer block, but that's not the concern of this function.
*/
-static size_t find_patch_start(const char *str)
+static size_t find_end_of_log_message(const char *input, int no_divider)
{
+ size_t end;
+
const char *s;
- for (s = str; *s; s = next_line(s)) {
+ /* Assume the naive end of the input is already what we want. */
+ end = strlen(input);
+
+ /* Optionally skip over any patch part ("---" line and below). */
+ for (s = input; *s; s = next_line(s)) {
const char *v;
- if (skip_prefix(s, "---", &v) && isspace(*v))
- return s - str;
+ if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v)) {
+ end = s - input;
+ break;
+ }
}
- return s - str;
+ /* Skip over other ignorable bits. */
+ return end - ignored_log_message_bytes(input, end);
}
/*
@@ -925,12 +951,6 @@ continue_outer_loop:
return len;
}
-/* Return the position of the end of the trailers. */
-static size_t find_trailer_end(const char *buf, size_t len)
-{
- return len - ignored_log_message_bytes(buf, len);
-}
-
static int ends_with_blank_line(const char *buf, size_t len)
{
ssize_t ll = last_line(buf, len);
@@ -1101,7 +1121,7 @@ void process_trailers(const char *file,
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
- int patch_start, trailer_end, trailer_start;
+ int end_of_log_message, trailer_start;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
size_t nr = 0, alloc = 0;
@@ -1109,16 +1129,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,
ensure_configured();
- if (opts->no_divider)
- patch_start = strlen(str);
- else
- patch_start = find_patch_start(str);
-
- trailer_end = find_trailer_end(str, patch_start);
- trailer_start = find_trailer_start(str, trailer_end);
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+ trailer_start = find_trailer_start(str, end_of_log_message);
trailer_lines = strbuf_split_buf(str + trailer_start,
- trailer_end - trailer_start,
+ end_of_log_message - trailer_start,
'\n',
0);
for (ptr = trailer_lines; *ptr; ptr++) {
@@ -1141,7 +1156,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
info->blank_line_before_trailer = ends_with_blank_line(str,
trailer_start);
info->trailer_start = str + trailer_start;
- info->trailer_end = str + trailer_end;
+ info->trailer_end = str + end_of_log_message;
info->trailers = trailer_strings;
info->trailer_nr = nr;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 0/4] Trailer readability cleanups
From: Linus Arver via GitGitGadget @ 2023-09-26 6:22 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <pull.1563.v3.git.1695412245.gitgitgadget@gmail.com>
These patches were created while digging into the trailer code to better
understand how it works, in preparation for making the trailer.{c,h} files
as small as possible to make them available as a library for external users.
This series was originally created as part of [1], but are sent here
separately because the changes here are arguably more subjective in nature.
These patches do not add or change any features. Instead, their goal is to
make the code easier to understand for new contributors (like myself), by
making various cleanups and improvements. Ultimately, my hope is that with
such cleanups, we are better positioned to make larger changes (especially
the broader libification effort, as in "Introduce Git Standard Library"
[2]).
Updates in v4
=============
* The first 3 patches in v3 were merged into 'master'. Necessarily, those 3
patches have been dropped.
* Patch 4 in v3 ("trailer: rename *_DEFAULT enums to *_UNSPECIFIED") has
been dropped, as well as Patch 9 in v3 ("trailer: make stack variable
names match field names"). These were dropped to simplify this series for
what I think is the more immediate, important change (see next bullet
point).
* Patches 5-8 in v3 are the only ones remaining in this series. They still
solely deal with --no-divider and trailer block start/end cleanups.
Updates in v3
=============
* Patches 4 and 6 (--no-divider and trailer block start/end cleanups) have
been reorganized to Patches 5-8. This ended up touching commit.c in a
minor way, but otherwise all of the changes here are cleanups and do not
change any behavior.
Updates in v2
=============
* Patch 1: Drop the use of a #define. Instead just use an anonymous struct
named internal.
* Patch 2: Don't free info out parameter inside parse_trailers(). Instead
free it from the caller, process_trailers(). Update comment in
parse_trailers().
* Patch 3: Reword commit message.
* Patch 4: Mention be3d654343 (commit: pass --no-divider to
interpret-trailers, 2023-06-17) in commit message.
* Added Patch 6 to make trailer_info use offsets for trailer_start and
trailer_end (thanks to Glen Choo for the suggestion).
[1]
https://lore.kernel.org/git/pull.1564.git.1691210737.gitgitgadget@gmail.com/T/#mb044012670663d8eb7a548924bbcc933bef116de
[2]
https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
[3]
https://lore.kernel.org/git/pull.1149.git.1677143700.gitgitgadget@gmail.com/
[4]
https://lore.kernel.org/git/6b4cb31b17077181a311ca87e82464a1e2ad67dd.1686797630.git.gitgitgadget@gmail.com/
[5]
https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#m0131f9829c35d8e0103ffa88f07d8e0e43dd732c
Linus Arver (4):
commit: ignore_non_trailer computes number of bytes to ignore
trailer: find the end of the log message
trailer: use offsets for trailer_start/trailer_end
trailer: only use trailer_block_* variables if trailers were found
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
commit.c | 2 +-
commit.h | 4 +-
sequencer.c | 2 +-
trailer.c | 105 ++++++++++++++++++++++++++++++-----------------
trailer.h | 15 ++++---
7 files changed, 83 insertions(+), 49 deletions(-)
base-commit: bcb6cae2966cc407ca1afc77413b3ef11103c175
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1563%2Flistx%2Ftrailer-libification-prep-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1563/listx/trailer-libification-prep-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1563
Range-diff vs v3:
1: 4f116d2550f < -: ----------- trailer: separate public from internal portion of trailer_iterator
2: c00f4623d0b < -: ----------- trailer: split process_input_file into separate pieces
3: f78c2345fad < -: ----------- trailer: split process_command_line_args into separate functions
4: 47186a09b24 < -: ----------- trailer: rename *_DEFAULT enums to *_UNSPECIFIED
5: da52cec42e1 = 1: 4ce5cf77005 commit: ignore_non_trailer computes number of bytes to ignore
6: ab8a6ced143 = 2: c904caba7e1 trailer: find the end of the log message
7: 091805eb7d9 = 3: 796e47c1e5f trailer: use offsets for trailer_start/trailer_end
8: 1762f78a613 = 4: 64e1bd4e4be trailer: only use trailer_block_* variables if trailers were found
9: a784c45ed71 < -: ----------- trailer: make stack variable names match field names
--
gitgitgadget
^ permalink raw reply
* [PATCH v4 1/4] commit: ignore_non_trailer computes number of bytes to ignore
From: Linus Arver via GitGitGadget @ 2023-09-26 6:22 UTC (permalink / raw)
To: git; +Cc: Glen Choo, Christian Couder, Phillip Wood, Linus Arver,
Linus Arver
In-Reply-To: <pull.1563.v4.git.1695709372.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
ignore_non_trailer() returns the _number of bytes_ that should be
ignored from the end of the log message. It does not by itself "ignore"
anything.
Rename this function to remove the leading "ignore" verb, to sound more
like a quantity than an action.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
commit.c | 2 +-
commit.h | 4 ++--
trailer.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7da5f924484..d1785d32db1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -900,7 +900,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
strbuf_stripspace(&sb, '\0');
if (signoff)
- append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
+ append_signoff(&sb, ignored_log_message_bytes(sb.buf, sb.len), 0);
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
die_errno(_("could not write commit template"));
diff --git a/builtin/merge.c b/builtin/merge.c
index 545da0c8a11..c654a29fe85 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -870,7 +870,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
_(no_scissors_editor_comment), comment_line_char);
}
if (signoff)
- append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
+ append_signoff(&msg, ignored_log_message_bytes(msg.buf, msg.len), 0);
write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), NULL,
diff --git a/commit.c b/commit.c
index b3223478bc2..4440fbabb83 100644
--- a/commit.c
+++ b/commit.c
@@ -1769,7 +1769,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
* Returns the number of bytes from the tail to ignore, to be fed as
* the second parameter to append_signoff().
*/
-size_t ignore_non_trailer(const char *buf, size_t len)
+size_t ignored_log_message_bytes(const char *buf, size_t len)
{
size_t boc = 0;
size_t bol = 0;
diff --git a/commit.h b/commit.h
index 28928833c54..1cc872f225f 100644
--- a/commit.h
+++ b/commit.h
@@ -294,8 +294,8 @@ const char *find_header_mem(const char *msg, size_t len,
const char *find_commit_header(const char *msg, const char *key,
size_t *out_len);
-/* Find the end of the log message, the right place for a new trailer. */
-size_t ignore_non_trailer(const char *buf, size_t len);
+/* Find the number of bytes to ignore from the end of a log message. */
+size_t ignored_log_message_bytes(const char *buf, size_t len);
typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
void *cb_data);
diff --git a/trailer.c b/trailer.c
index b6de5d9cb2d..3c54b38a85a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -928,7 +928,7 @@ continue_outer_loop:
/* Return the position of the end of the trailers. */
static size_t find_trailer_end(const char *buf, size_t len)
{
- return len - ignore_non_trailer(buf, len);
+ return len - ignored_log_message_bytes(buf, len);
}
static int ends_with_blank_line(const char *buf, size_t len)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 6/6] commit-graph: report incomplete chains during verification
From: Jeff King @ 2023-09-26 6:04 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@coredump.intra.peff.net>
The load_commit_graph_chain_fd_st() function will stop loading chains
when it sees an error. But if it has loaded any graph slice at all, it
will return it. This is a good thing for normal use (we use what data we
can, and this is just an optimization). But it's a bad thing for
"commit-graph verify", which should be careful about finding any
irregularities. We do complain to stderr with a warning(), but the
verify command still exits with a successful return code.
The new tests here cover corruption of both the base and tip slices of
the chain. The corruption of the base file already works (it is the
first file we look at, so when we see the error we return NULL). The
"tip" case is what is fixed by this patch (it complains to stderr but
still returns the base slice).
Note that this also causes us to adjust a test later in the file that
similarly corrupts a tip (though confusingly the test script calls this
"base"). It checks stderr but erroneously expects the whole "verify"
command to exit with a successful code.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit-graph.c | 10 +++++++++-
commit-graph.c | 9 +++++++--
commit-graph.h | 3 ++-
t/t5324-split-commit-graph.sh | 28 +++++++++++++++++++++++++++-
4 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 50c15d9bfe..f5e66e9969 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -74,6 +74,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
int fd;
struct stat st;
int flags = 0;
+ int incomplete_chain = 0;
int ret;
static struct option builtin_commit_graph_verify_options[] = {
@@ -122,13 +123,20 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
else if (opened == OPENED_GRAPH)
graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
else
- graph = load_commit_graph_chain_fd_st(the_repository, fd, &st);
+ graph = load_commit_graph_chain_fd_st(the_repository, fd, &st,
+ &incomplete_chain);
if (!graph)
return 1;
ret = verify_commit_graph(the_repository, graph, flags);
free_commit_graph(graph);
+
+ if (incomplete_chain) {
+ error("one or more commit-graph chain files could not be loaded");
+ ret |= 1;
+ }
+
return ret;
}
diff --git a/commit-graph.c b/commit-graph.c
index b1d3e5bf94..148af50058 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -563,14 +563,17 @@ int open_commit_graph_chain(const char *chain_file,
}
struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
- int fd, struct stat *st)
+ int fd, struct stat *st,
+ int *incomplete_chain)
{
struct commit_graph *graph_chain = NULL;
struct strbuf line = STRBUF_INIT;
struct object_id *oids;
int i = 0, valid = 1, count;
FILE *fp = xfdopen(fd, "r");
+ *incomplete_chain = 0;
+
count = st->st_size / (the_hash_algo->hexsz + 1);
CALLOC_ARRAY(oids, count);
@@ -608,6 +611,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
if (!valid) {
warning(_("unable to find all commit-graph files"));
+ *incomplete_chain = 1;
break;
}
}
@@ -630,8 +634,9 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
struct commit_graph *g = NULL;
if (open_commit_graph_chain(chain_file, &fd, &st)) {
+ int incomplete;
/* ownership of fd is taken over by load function */
- g = load_commit_graph_chain_fd_st(r, fd, &st);
+ g = load_commit_graph_chain_fd_st(r, fd, &st, &incomplete);
}
free(chain_file);
diff --git a/commit-graph.h b/commit-graph.h
index 3b662fd2b5..20ada7e891 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -107,7 +107,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
int fd, struct stat *st,
struct object_directory *odb);
struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,
- int fd, struct stat *st);
+ int fd, struct stat *st,
+ int *incomplete_chain);
struct commit_graph *read_commit_graph_one(struct repository *r,
struct object_directory *odb);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 86d56debf6..598660557a 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -285,6 +285,32 @@ test_expect_success 'verify hashes along chain, even in shallow' '
)
'
+test_expect_success 'verify notices chain slice which is bogus (base)' '
+ git clone --no-hardlinks . verify-chain-bogus-base &&
+ (
+ cd verify-chain-bogus-base &&
+ git commit-graph verify &&
+ base_file=$graphdir/graph-$(sed -n 1p $graphdir/commit-graph-chain).graph &&
+ echo "garbage" >$base_file &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph file is too small" err
+ )
+'
+
+test_expect_success 'verify notices chain slice which is bogus (tip)' '
+ git clone --no-hardlinks . verify-chain-bogus-tip &&
+ (
+ cd verify-chain-bogus-tip &&
+ git commit-graph verify &&
+ tip_file=$graphdir/graph-$(sed -n 2p $graphdir/commit-graph-chain).graph &&
+ echo "garbage" >$tip_file &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph file is too small" err
+ )
+'
+
test_expect_success 'verify --shallow does not check base contents' '
git clone --no-hardlinks . verify-shallow &&
(
@@ -306,7 +332,7 @@ test_expect_success 'warn on base graph chunk incorrect' '
git commit-graph verify &&
base_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&
corrupt_file "$base_file" $(test_oid base) "\01" &&
- git commit-graph verify --shallow 2>test_err &&
+ test_must_fail git commit-graph verify --shallow 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "commit-graph chain does not match" err
)
--
2.42.0.758.gd56856b565
^ permalink raw reply related
* [PATCH 5/6] commit-graph: tighten chain size check
From: Jeff King @ 2023-09-26 6:03 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@coredump.intra.peff.net>
When we open a commit-graph-chain file, if it's smaller than a single
entry, we just quietly treat that as ENOENT. That make some sense if the
file is truly zero bytes, but it means that "commit-graph verify" will
quietly ignore a file that contains garbage if that garbage happens to
be short.
Instead, let's only simulate ENOENT when the file is truly empty, and
otherwise return EINVAL. The normal graph-loading routines don't care,
but "commit-graph verify" will notice and complain about the difference.
It's not entirely clear to me that the 0-is-ENOENT case actually happens
in real life, so we could perhaps just eliminate this special-case
altogether. But this is how we've always behaved, so I'm preserving it
in the name of backwards compatibility (though again, it really only
matters for "verify", as the regular routines are happy to load what
they can).
Signed-off-by: Jeff King <peff@peff.net>
---
There's a related issue which I didn't fix in this series, which is that
the "load" function does a truncating division of (size / hash_size + 1)
to find the number of entries (the +1 is for the newline). So a
less-than-hash-sized chunk of garbage at the end of the file will be
ignored.
The simplest way to fix that is that we can insist that the size mod
hash_size+1 is 0. But I was wary of being too picky here (e.g., what
about a file that doesn't end with newline) especially in a way that
affected more than just "verify".
commit-graph.c | 10 ++++++++--
t/t5324-split-commit-graph.sh | 12 ++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 8b29c6de24..b1d3e5bf94 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -548,9 +548,15 @@ int open_commit_graph_chain(const char *chain_file,
close(*fd);
return 0;
}
- if (st->st_size <= the_hash_algo->hexsz) {
+ if (st->st_size < the_hash_algo->hexsz) {
close(*fd);
- errno = ENOENT;
+ if (!st->st_size) {
+ /* treat empty files the same as missing */
+ errno = ENOENT;
+ } else {
+ warning("commit-graph chain file too small");
+ errno = EINVAL;
+ }
return 0;
}
return 1;
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 0ac7bbd1dc..86d56debf6 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -327,6 +327,18 @@ test_expect_success 'verify after commit-graph-chain corruption' '
)
'
+test_expect_success 'verify notices too-short chain file' '
+ git clone --no-hardlinks . verify-chain-garbage &&
+ (
+ cd verify-chain-garbage &&
+ git commit-graph verify &&
+ echo "garbage" >$graphdir/commit-graph-chain &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err &&
+ grep "commit-graph chain file too small" err
+ )
+'
+
test_expect_success 'verify across alternates' '
git clone --no-hardlinks . verify-alt &&
(
--
2.42.0.758.gd56856b565
^ permalink raw reply related
* [PATCH 4/6] commit-graph: detect read errors when verifying graph chain
From: Jeff King @ 2023-09-26 6:00 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Derrick Stolee
In-Reply-To: <20230926055452.GA1341109@coredump.intra.peff.net>
Because it's OK to not have a graph file at all, the graph_verify()
function needs to tell the difference between a missing file and a real
error. So when loading a traditional graph file, we call
open_commit_graph() separately from load_commit_graph_chain_fd_st(), and
don't complain if the first one fails with ENOENT.
When the function learned about chain files in 3da4b609bb (commit-graph:
verify chains with --shallow mode, 2019-06-18), we couldn't be as
careful, since the only way to load a chain was with
read_commit_graph_one(), which did both the open/load as a single unit.
So we'll miss errors in chain files we load, thinking instead that there
was just no chain file at all.
Note that we do still report some of these problems to stderr, as the
loading function calls error() and warning(). But we'd exit with a
successful exit code, which is wrong.
We can fix that by using the recently split open/load functions for
chains. That lets us treat the chain file just like a single file with
respect to error handling here.
An existing test (from 3da4b609bb) shows off the problem; we were
expecting "commit-graph verify" to report success, but that makes no
sense. We did not even verify the contents of the graph data, because we
couldn't load it! I don't think this was an intentional exception, but
rather just the test covering what happened to occur.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit-graph.c | 23 ++++++++++++++++-------
t/t5324-split-commit-graph.sh | 4 ++--
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24..50c15d9bfe 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -69,7 +69,8 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
struct commit_graph *graph = NULL;
struct object_directory *odb = NULL;
char *graph_name;
- int open_ok;
+ char *chain_name;
+ enum { OPENED_NONE, OPENED_GRAPH, OPENED_CHAIN } opened = OPENED_NONE;
int fd;
struct stat st;
int flags = 0;
@@ -102,21 +103,29 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
odb = find_odb(the_repository, opts.obj_dir);
graph_name = get_commit_graph_filename(odb);
- open_ok = open_commit_graph(graph_name, &fd, &st);
- if (!open_ok && errno != ENOENT)
+ chain_name = get_commit_graph_chain_filename(odb);
+ if (open_commit_graph(graph_name, &fd, &st))
+ opened = OPENED_GRAPH;
+ else if (errno != ENOENT)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
+ else if (open_commit_graph_chain(chain_name, &fd, &st))
+ opened = OPENED_CHAIN;
+ else if (errno != ENOENT)
+ die_errno(_("could not open commit-graph chain '%s'"), chain_name);
FREE_AND_NULL(graph_name);
+ FREE_AND_NULL(chain_name);
FREE_AND_NULL(options);
- if (open_ok)
+ if (opened == OPENED_NONE)
+ return 0;
+ else if (opened == OPENED_GRAPH)
graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
else
- graph = read_commit_graph_one(the_repository, odb);
+ graph = load_commit_graph_chain_fd_st(the_repository, fd, &st);
- /* Return failure if open_ok predicted success */
if (!graph)
- return !!open_ok;
+ return 1;
ret = verify_commit_graph(the_repository, graph, flags);
free_commit_graph(graph);
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index e335ef87a6..0ac7bbd1dc 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -317,11 +317,11 @@ test_expect_success 'verify after commit-graph-chain corruption' '
(
cd verify-chain &&
corrupt_file "$graphdir/commit-graph-chain" 30 "G" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "invalid commit-graph chain" err &&
corrupt_file "$graphdir/commit-graph-chain" 30 "A" &&
- git commit-graph verify 2>test_err &&
+ test_must_fail git commit-graph verify 2>test_err &&
grep -v "^+" test_err >err &&
test_i18ngrep "unable to find all commit-graph files" err
)
--
2.42.0.758.gd56856b565
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox