Git development
 help / color / mirror / Atom feed
* Re: [PATCH 1/3] t/lib-httpd: bump apache timeout
From: Michael Montalbo @ 2026-07-02  3:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Patrick Steinhardt, git, Junio C Hamano
In-Reply-To: <20260628080009.GA107826@coredump.intra.peff.net>

On Sun, Jun 28, 2026 at 1:00 AM Jeff King <peff@peff.net> wrote:
>
> I didn't reference Michael's bugzilla report directly, because you can't
> read it without a login. :(
>
> Maybe it's worth doing anyway?
>

I also thought the report being behind a login was unfortunate. For the
historical record, I ended up submitting a patch[1] to their public GitHub
mirror that describes the issue in more detail.

[1] https://github.com/apache/httpd/pull/676

^ permalink raw reply

* Bug report - git rev-list --exclude-first-parent-only [SEC=UNOFFICIAL]
From: Michael Hore @ 2026-07-02  3:59 UTC (permalink / raw)
  To: git@vger.kernel.org

I believe I have found a bug -

My repo has a commit structure like

R2
|\
| F
|/
R1

i.e.
 - there is a merge commit R2 with parents R1 and F
 - the parent of F is R1

I ran "git rev-list --exclude-first-parent-only F ^R2"

it gave the expected result: "F"

I ran "git rev-list --exclude-first-parent-only F R1 ^R2"

I expected the same result, but I got an unexpected result - nothing at all

Suspected cause - I had a look at the code, and it looks like process_parents() in revision.c, when processing uninteresting flags, will skip the 1st parent and mark the 2nd parent as uninteresting if the 1st parent is already SEEN, even with the flag exclude-first-parent-only. I think maybe explicitly selecting R1 on the command line causes it to be marked SEEN before ^R2 is processed, thus resulting in F being marked uninteresting.

[System Info]
git version:
git version 2.54.0.windows.1
cpu: x86_64
built from commit: 2b8a3ab140826ac423c2845ef81d4c6ac4f7bf3c
sizeof-long: 4
sizeof-size_t: 8
shell-path: D:/git-sdk-64-build-installers/usr/bin/sh
rust: disabled
feature: fsmonitor--daemon
gettext: enabled
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1


[Enabled Hooks]

Regards,
Michael

Please consider the environment before printing this document.

Information collected by ASIC may contain personal information. Please refer to our Privacy Policy<https://asic.gov.au/privacy/> for information about how we handle your personal information, your rights to seek access to and correct your personal information, and how to complain about breaches of your privacy by ASIC.

This e-mail and any attachments are intended for the addressee(s) only and may be confidential. They may contain legally privileged, copyright material or personal and /or confidential information. You should not read, copy, use or disclose the content without authorisation. If you have received this email in error, please notify the sender as soon as possible, delete the email and destroy any copies. This notice should not be removed.

^ permalink raw reply

* Re: [PATCH v2 5/6] t: convert grep assertions to test_grep
From: Michael Montalbo @ 2026-07-02  4:14 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Michael Montalbo via GitGitGadget, SZEDER Gábor, git,
	D. Ben Knoble, Eric Sunshine
In-Reply-To: <xmqqqzlpt543.fsf@gitster.g>

On Mon, Jun 29, 2026 at 2:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > It may not be a bad idea to go through "# lint-ok:" introduced by
> > Michael's series with finer toothed comb (there are only a handful
> > of them) and see if there are similar "look, the file we are
> > grepping in never exists with correctly running Git" gotchas.
>
> In any case, I think SZEDER's fix to stop grepping in the file but
> instead insisting on its absense does make sense and it is now in
> 'next'.  So perhaps this topic can have a small and final reroll v3
> that omits change to this particular line (and possibly fix other
> lines that punts with "# lint-ok" if needed) and we can declare
> victory after that?
>
> Thanks, all.

Thank you, SZEDER, for the nice catch.

I will apply the suggested fix to the series locally, and go through
the other #lint-ok's with a fine toothed comb as Junio suggests.

Appreciate the eyes on the series, will send a reroll soon.

^ permalink raw reply

* [PATCH] apply: avoid leaking abandoned git-header state
From: Zephyr Yao @ 2026-07-02  4:17 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Zephyr Yao, Mahya SamDaliri, Haotian Zhang,
	Martin Kellogg

When find_header() sees a "diff --git" line, it calls
parse_git_diff_header() to parse the git-style extended header. That parser
updates the caller's struct patch as it goes, filling in the default name,
old/new names, and new/delete state.

But not every "diff --git" line found while scanning is ultimately accepted
as the patch header. If parse_git_diff_header() returns a length that covers
only the "diff --git" line, find_header() continues scanning for another
header. In that case the partially parsed git-header state must not interfere
with the later traditional "---" / "+++" header.

Leaving that state behind can combine incompatible metadata from the
abandoned git header and the later traditional header. For example, after:

	diff --git a/foo b/foo

	--- /dev/null
	+++ b/foo
	@@ -0,0 +1 @@
	+x

the abandoned git header can leave an old name in the patch, while the
traditional header marks the patch as creating a new file. That impossible
state later trips the check_preimage() assertion that a creation patch should
not have a preimage.

Parse a candidate git header into a temporary patch and line number. Commit
that temporary state to the real patch only when the git header is actually
accepted; otherwise release it and keep scanning with the original patch
state unchanged.

Also reject an empty parsed default name from the "diff --git" line.
An empty patch->def_name is not a valid pathname, and should not be
used later as a fallback when old_name and new_name are missing.

Add regression tests for both the empty default-name case and the non-empty
abandoned-header case above.

Co-authored-by: Mahya SamDaliri <ms3539@njit.edu>
Signed-off-by: Mahya SamDaliri <ms3539@njit.edu>
Co-authored-by: Haotian Zhang <haotian.zhang@njit.edu>
Signed-off-by: Haotian Zhang <haotian.zhang@njit.edu>
Co-authored-by: Martin Kellogg <martin.kellogg@njit.edu>
Signed-off-by: Martin Kellogg <martin.kellogg@njit.edu>
Signed-off-by: Zephyr Yao <zhihao.yao@njit.edu>
---
 apply.c               | 29 ++++++++++++++++++++++-------
 t/t4100-apply-stat.sh | 25 +++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/apply.c b/apply.c
index 5e54453..2ce9b6a 100644
--- a/apply.c
+++ b/apply.c
@@ -1362,6 +1362,9 @@ int parse_git_diff_header(struct strbuf *root,
 	 * the default name from the header.
 	 */
 	patch->def_name = git_header_name(p_value, line, len);
+	if (patch->def_name && !*patch->def_name)
+		FREE_AND_NULL(patch->def_name);
+
 	if (patch->def_name && root->len) {
 		char *s = xstrfmt("%s%s", root->buf, patch->def_name);
 		free(patch->def_name);
@@ -1632,15 +1635,27 @@ static int find_header(struct apply_state *state,
 		 * or mode change, so we handle that specially
 		 */
 		if (!memcmp("diff --git ", line, 11)) {
-			int git_hdr_len = parse_git_diff_header(&state->root,
-								state->patch_input_file,
-								&state->linenr,
-								state->p_value, line, len,
-								size, patch);
-			if (git_hdr_len < 0)
+			struct patch git_patch = { 0 };
+			int git_linenr = state->linenr;
+			int git_hdr_len;
+
+			git_patch.inaccurate_eof = patch->inaccurate_eof;
+			git_patch.recount = patch->recount;
+			git_hdr_len = parse_git_diff_header(&state->root,
+							    state->patch_input_file,
+							    &git_linenr,
+							    state->p_value, line, len,
+							    size, &git_patch);
+			if (git_hdr_len < 0) {
+				release_patch(&git_patch);
 				return -128;
-			if (git_hdr_len <= len)
+			}
+			if (git_hdr_len <= len) {
+				release_patch(&git_patch);
 				continue;
+			}
+			*patch = git_patch;
+			state->linenr = git_linenr;
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
index 8393076..d3406ed 100755
--- a/t/t4100-apply-stat.sh
+++ b/t/t4100-apply-stat.sh
@@ -113,6 +113,31 @@ test_expect_success 'applying a patch with a missing filename reports the input'
 	test_cmp expect err
 '
 
+test_expect_success 'empty default filename reports the input' '
+	cat >empty-name.patch <<-\EOF &&
+	diff --git "a/""b/"
+
+	--- /dev/null
+	+++ "
+	@@ -0,0 +1 @@
+	+
+	EOF
+	test_must_fail git apply empty-name.patch 2>err &&
+	test_grep "git diff header lacks filename information" err
+'
+
+test_expect_success 'abandoned git header does not reuse names' '
+	cat >abandoned-git-header.patch <<-\EOF &&
+	diff --git a/foo b/foo
+
+	--- /dev/null
+	+++ b/foo
+	@@ -0,0 +1 @@
+	+x
+	EOF
+	git apply --check abandoned-git-header.patch
+'
+
 test_expect_success 'applying a patch with an invalid mode reports the input' '
 	cat >mode.patch <<-\EOF &&
 	diff --git a/f b/f
-- 
2.47.0

^ permalink raw reply related

* Re: [PATCH RFC v2 2/2] Move libgit.a sources into separate "lib/" directory
From: Patrick Steinhardt @ 2026-07-02  5:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Phillip Wood, SZEDER Gábor, git, brian m. carlson,
	Elijah Newren, Derrick Stolee, Phillip Wood
In-Reply-To: <xmqq1pdmrcp6.fsf@gitster.g>

On Wed, Jul 01, 2026 at 07:45:09AM -0700, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > As I said last time this came up, I don't really buy the discoverability 
> > argument because there are just as many files to trawl through to find 
> > what you're looking through and now there is an extra directory to 
> > check. I think the solution to that is to recommend folks use "git grep" 
> > or ctags etc. not moving code to a new directory.
> 
> Hear, hear.  Also it would be great if we can trick some talented
> technical writer into writing the "map" of the source so that by
> reading this one or two pager, any new person with reasonable
> competence will know how things are partitioned into pieces and how
> these pieces fit together.  I wonder how good LLMs are these days?
> ;-)

This isn't about discoverability of the library files though, I
specifically want to improve discoverability of all the other files that
we have in our root directory. So yes, I fully agree that this change
does not help to make that one file that is part of our library easier
to find.

> > I do however think putting all the library code in a subdirectory makes 
> > it easier to say things like "please try to avoid new uses of 
> > 'the_repository' and prefer 'error()' over 'die()' in library code" 
> > because all the library code is in the same directory. I think that is a 
> > much stronger selling point.
> 
> Yes.  "library code (things outside the subdirectories) should not
> use X" would work just fine, though.

That rule doesn't quite work:

  - We have several C files that are not library files and that are in
    the top-level directory. For example "scalar.c" or "shell.c".

  - We have several C files that are part of the library and that are in
    a subdirectory. For example "compat/", "refs/", "odb/".

So having this properly cleaned up would help to have clear indicators
what component a given file belongs to.

> > Another cost is remembering things have moved - the other day I spent 
> > too long wondering why "git show origin/seen:wt-status.c" wasn't working 
> > until I ran "git log origin/seen" and realized it had move to 
> > lib/wt-status.c.
> 
> Yes, this has bit me multiple dozen times, as the tip of 'seen' is
> contaminated with this rename, already.  It is a huge pain.

Yeah, this one I don't have any arguments against besides a very
hand-wavy "it'll get better over time" :)

Thanks!

Patrick

^ permalink raw reply

* Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed
From: Jeff King @ 2026-07-02  5:37 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Junio C Hamano
In-Reply-To: <20260701212442.1430084-2-sandals@crustytoothpaste.net>

On Wed, Jul 01, 2026 at 09:24:39PM +0000, brian m. carlson wrote:

> +test_lazy_prereq SVN '
> +	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
> +		use SVN::Core;
> +		use SVN::Repos;
> +		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
> +	"
> +'

The single-quotes in your inline perl will be interpreted as ending (and
restarting) the lazy-prereq snippet. So you actually get a bare:

  $SVN::Core::VERSION gt 1.1.0 or exit(42);

fed to perl (no quotes around 1.1.0). We sometimes catch these cases
automatically it results in an extra argument to test_expect_success,
etc. But here you are unlucky enough that it does not (and anyway, we do
not seem to have the same safety check for test_lazy_prereq; we'd just
ignore the extra arguments).

And of course being perl, it doesn't complain. I'm not sure how it is
interpreted, but I doubt the use of "gt" is right. My version of
SVN::Core is 1.14.5, which is (correctly) more than "1.1.0", but is
(incorrect) not more than "1.2.0".

I think the "gt" bug is inherited from lib-git-svn.sh (unless I'm just
holding it wrong), but the single-quote one is new (it happens at the
top-level in the original).

-Peff

^ permalink raw reply

* Unexpected recursion in 'git rm'
From: Евгений Плискин @ 2026-07-02  7:49 UTC (permalink / raw)
  To: git

Hello.

The following git command does recurse directories as contrary to the reference (https://git-scm.com/docs/git-rm):

    git rm -n *.json

Without directory specification before '*.json' this command is not expected to recurse directories, but it really does.

git version 2.55.0.windows.1

-- 
Regards,
Eugene Pliskin                          mailto:eugene.pliskin@gmail.com


^ permalink raw reply

* [PATCH 0/9] hash algorithm leak fixes
From: Jeff King @ 2026-07-02  7:52 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt

This series fixes some leaks you can find by running:

  make SANITIZE=leak \
       OPENSSL_SHA256=1 \
       GIT_TEST_DEFAULT_HASH=sha256 \
       test

The crux of the issue is that we depend on calling git_hash_final() to
clean up any git_hash_ctx we've initialized. But we don't always call
that function (we may return early due to an error, etc).

We don't see these in our regular leak-test builds because the default
hash implementations we use treat the hash_ctx as a sequence of bytes.
So there's no cleanup needed, and just letting the context go out of
scope is fine. But other implementations do allocate on initialization,
and need to have some kind of free/discard function. So building with
OPENSSL_SHA256 above is what lets us see the leaks.

You can see the same thing with OPENSSL_SHA1, but of course we don't
recommend that. Using OPENSSL_SHA1_UNSAFE likewise, but it sees only a
subset of the leaks since it is only used in a few code paths. Those
leaks would be found if we turned on leak-checking in the
linux-TEST-vars job, but the rest of them would require leak-checking
the linux-sha256 job.

And as a special bonus, patch 8 is a semi-related leak that only affects
libgcrypt. I don't think we build against that in CI at all. :-/

  [1/9]: csum-file: drop discard_hashfile()
  [2/9]: hash: add discard primitive
  [3/9]: csum-file: always finalize or discard hash
  [4/9]: csum-file: provide a function to release checkpoints
  [5/9]: patch-id: discard hash when done
  [6/9]: check_stream_oid(): discard hash on read error
  [7/9]: http: discard hash in dumb-http http_object_request
  [8/9]: hash: fix memory leak copying sha256 gcrypt handles
  [9/9]: hash: add platform-specific discard functions

 builtin/fast-import.c |  1 +
 builtin/patch-id.c    |  1 +
 csum-file.c           | 30 +++++++++++++++++-------------
 csum-file.h           |  2 +-
 diff.c                |  1 +
 hash.c                | 29 +++++++++++++++++++++++++++++
 hash.h                | 22 ++++++++++++++++++++++
 http.c                |  4 ++++
 http.h                |  1 +
 object-file.c         |  4 ++++
 sha1/openssl.h        |  6 ++++++
 sha256/gcrypt.h       |  7 +++++++
 sha256/openssl.h      |  6 ++++++
 13 files changed, 100 insertions(+), 14 deletions(-)

-Peff

^ permalink raw reply

* [PATCH 1/9] csum-file: drop discard_hashfile()
From: Jeff King @ 2026-07-02  7:57 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

Commit c3d034df16 (csum-file: introduce discard_hashfile(), 2024-07-25)
added a cleanup function that no longer has any callers. In that commit
we adjusted do_write_index() to use the new function. But a similar fix
occurred on a parallel branch, making free_hashfile() public, and the
merge resolution in 1b6b2bfae5 (Merge branch 'ps/leakfixes-part-4',
2024-08-23) took the free_hashfile() version.

So now we have two functions, discard_hashfile() and free_hashfile(),
and we only need one. Which one do we want to keep?

The only difference between them is that the discard variant also closes
the descriptors held in the struct. Let's look at the three callers:

  1. In finalize_hashfile() we've either already closed the descriptors
     (if the CSUM_CLOSE flag is passed) or the caller didn't want them
     closed (if it didn't pass that flag). So we want the more limited
     free_hashfile().

  2. In object-file.c:flush_packfile_transaction() we close the
     descriptor ourselves. So discard_hashfile() could save us a line of
     code.

  3. In do_write_index() we don't close the descriptor. This was the spot
     for which c3d034df16 added the discard function in the first place,
     but I'm skeptical that closing the descriptor here is the right
     thing. It is true that we are done with the descriptor at this
     point and closing it would be ideal. But we don't really own it!

     The descriptor comes from a tempfile struct (as part of a lock) and
     that tempfile will hold on to the descriptor and try to close it
     when it is deleted. This might happen at the end of the program, in
     which case the double-close is mostly harmless (we might
     accidentally close some other open descriptor, but at that point
     we're just closing and unlinking everything we can).

     But in theory it could also cause subtle bugs. If do_write_index()
     fails, we return the error up the stack and would eventually end up
     in write_locked_index(). There we roll back the lock file on error,
     which will close the descriptor. So now we get our double close,
     and we might actually close something else that was opened in the
     interim.

     This is probably unlikely in practice (as soon as we see the error
     we'd mostly be unwinding the stack, not opening new files). But it
     highlights a potential problem with the discard_hashfile()
     interface: the hashfile doesn't necessarily own that descriptor.

Note that I said "descriptors" plural above. Those callers all care
about the "fd" member of the struct. But discard_hashfile() also closes
check_fd. That is only used if the struct is initialized with
hashfd_check(), and neither of its two callers call either discard or
free (they always "finalize" instead). So closing it is irrelevant for
the current callers.

I think we're better off sticking with the simpler free_hashfile()
interface, and the handful of callers can decide how to handle the
descriptors themselves.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a semi-related cleanup that is in this series because we'll be
touching the free function in a bit. And at first I thought we'd
want the discard() variant, but after poking around a bit I'm pretty
sure we don't.

I do like the name discard() better, as it makes it more clear that it
is an alternative to finalize(). Since they have the same signature,
swapping the names/implementations _could_ confuse long-running branches
or topics in flight, but I kind of doubt there are any, given the
history.

 csum-file.c | 9 ---------
 csum-file.h | 1 -
 2 files changed, 10 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index d7a682c2b6..8ca9246a80 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -101,15 +101,6 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,
 	return fd;
 }
 
-void discard_hashfile(struct hashfile *f)
-{
-	if (0 <= f->check_fd)
-		close(f->check_fd);
-	if (0 <= f->fd)
-		close(f->fd);
-	free_hashfile(f);
-}
-
 void hashwrite(struct hashfile *f, const void *buf, uint32_t count)
 {
 	while (count) {
diff --git a/csum-file.h b/csum-file.h
index a270738a7a..d1a0ff29cd 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -74,7 +74,6 @@ void free_hashfile(struct hashfile *f);
  * Finalize the hashfile by flushing data to disk and free'ing it.
  */
 int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);
-void discard_hashfile(struct hashfile *);
 void hashwrite(struct hashfile *, const void *, uint32_t);
 void hashflush(struct hashfile *f);
 void crc32_begin(struct hashfile *);
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 2/9] hash: add discard primitive
From: Jeff King @ 2026-07-02  7:59 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

The usual life-cycle for a git_hash_ctx is calling git_hash_init(),
adding some data, and then using git_hash_final() to get the output
digest and free any resources.

Sometimes we decide to abort the operation without the final() call
(e.g., due to errors or other reasons). In that case we just abandon the
hash_ctx completely and let it go out of scope. For most hash
implementations this is fine; they were just holding values directly in
the struct.

But some implementations do allocate memory, and in these cases we leak
the memory. Notably OpenSSL >= 3.0 requires us to allocate the digest
context on the heap with EVP_MD_CTX_new().

Let's provide a git_hash_discard() function that can be used in these
code paths to free any resources. For now we'll implement it by just
calling git_hash_final() into a dummy output, relying on its side effect
of freeing the resources. Our view of the underlying hash implementation
is abstracted behind the platform_SHA_* macros, so that's the best we
can do without widening that interface.

It's a little inefficient, but probably not noticeably so in practice,
especially as we'd usually hit this on an error code path. And by
abstracting it in this function, we can later swap it out when the
platform_SHA interface lets us do so.

Signed-off-by: Jeff King <peff@peff.net>
---
In case you're on the edge of your seat, that widening happens in patch
9. It was helpful to make sure the simple-and-stupid thing actually
fixed the leaks first, and then do the convoluted platform-macro magic
later.

 hash.c | 12 ++++++++++++
 hash.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/hash.c b/hash.c
index e925b9754e..63672a3d22 100644
--- a/hash.c
+++ b/hash.c
@@ -283,6 +283,18 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
 	ctx->algop->final_oid_fn(oid, ctx);
 }
 
+void git_hash_discard(struct git_hash_ctx *ctx)
+{
+	/*
+	 * XXX Many implementations do not need to do anything here,
+	 * and a dummy final() call is wasteful. But we can't fix
+	 * that unless our implementation API exposes a discard
+	 * primitive.
+	 */
+	unsigned char dummy[GIT_MAX_RAWSZ];
+	git_hash_final(dummy, ctx);
+}
+
 uint32_t hash_algo_by_name(const char *name)
 {
 	if (!name)
diff --git a/hash.h b/hash.h
index c082a53c9a..6b2f04e2a4 100644
--- a/hash.h
+++ b/hash.h
@@ -325,6 +325,7 @@ void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src);
 void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len);
 void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx);
 void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx);
+void git_hash_discard(struct git_hash_ctx *ctx);
 const struct git_hash_algo *hash_algo_ptr_by_number(uint32_t algo);
 struct git_hash_ctx *git_hash_alloc(void);
 void git_hash_free(struct git_hash_ctx *ctx);
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 3/9] csum-file: always finalize or discard hash
From: Jeff King @ 2026-07-02  8:01 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

When a hashfile struct is created, we always initialize the git_hash_ctx
inside it. We usually end up in hashfile_finalize(), which passes that
ctx to git_hash_final(), cleaning it up.

But a few code paths don't do so:

  1. If we bail on the hashfile and call free_hashfile() directly rather
     than finalizing.

  2. If the skip_hash flag is set, the hashfile_finalize() call will
     never call git_hash_final(). (You might think that we should just
     avoid git_hash_init() entirely in this case, but the skip_hash flag
     is set by the caller after the hashfile is initialized).

For most hash implementations this is OK, but for ones that allocate on
initialization it causes a memory leak. You can see many failures by
running:

  make SANITIZE=leak OPENSSL_SHA1_UNSAFE=1 test

since OpenSSL >= 3.0 is such an allocating hash implementation (and
csum-file uses the "unsafe" algorithm variant).

We can solve this by calling git_hash_discard() as appropriate.

Note that free_hashfile() is used both directly by callers to abort
without finalizing, and by hashfile_finalize() to free memory. In the
latter case we _don't_ want to call git_hash_discard(), because we'll
already have either finalized or discarded it. So we'll push that to an
internal "free_memory" function, and keep free_hashfile() as the public
interface to abort a hashfile without finalizing.

This fix makes several scripts leak-free with the command above: t1600,
t1601, t2107, t7008, t9210, t9211.

Signed-off-by: Jeff King <peff@peff.net>
---
 csum-file.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index 8ca9246a80..44ff460692 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -55,24 +55,32 @@ void hashflush(struct hashfile *f)
 	}
 }
 
-void free_hashfile(struct hashfile *f)
+static void free_hashfile_memory(struct hashfile *f)
 {
 	free(f->buffer);
 	free(f->check_buffer);
 	free(f);
 }
 
+void free_hashfile(struct hashfile *f)
+{
+	git_hash_discard(&f->ctx);
+	free_hashfile_memory(f);
+}
+
 int finalize_hashfile(struct hashfile *f, unsigned char *result,
 		      enum fsync_component component, unsigned int flags)
 {
 	int fd;
 
 	hashflush(f);
 
-	if (f->skip_hash)
+	if (f->skip_hash) {
+		git_hash_discard(&f->ctx);
 		hashclr(f->buffer, f->algop);
-	else
+	} else {
 		git_hash_final(f->buffer, &f->ctx);
+	}
 
 	if (result)
 		hashcpy(result, f->buffer, f->algop);
@@ -97,7 +105,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,
 		if (close(f->check_fd))
 			die_errno("%s: sha1 file error on close", f->name);
 	}
-	free_hashfile(f);
+	free_hashfile_memory(f);
 	return fd;
 }
 
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 4/9] csum-file: provide a function to release checkpoints
From: Jeff King @ 2026-07-02  8:03 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

A hashfile_checkpoint struct is basically just a copy of the hash_ctx
state at a given point in the file. As such, it contains its own
git_hash_ctx which may (depending on the underlying hash implementation)
need to be discarded when we're done with it.

Let's add a "release" function which cleans up the hash context it
holds. I chose "release" here and not "discard" because you'd use this
to clean up every checkpoint, whether you used it or not. As opposed to
git_hash_discard(), which is needed only if you didn't call
git_hash_final().

There are only two callers which use hashfile_checkpoints, and we can
add release calls to both. When built with "SANITIZE=leak
OPENSSL_SHA1_UNSAFE=1", this makes both t1050 and t9300 leak-free.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fast-import.c | 1 +
 csum-file.c           | 5 +++++
 csum-file.h           | 1 +
 object-file.c         | 2 ++
 4 files changed, 9 insertions(+)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index aa656c5195..f6473dcc8e 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1216,6 +1216,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
 out:
 	free(in_buf);
 	free(out_buf);
+	hashfile_checkpoint_release(&checkpoint);
 }
 
 /* All calls must be guarded by find_object() or find_mark() to
diff --git a/csum-file.c b/csum-file.c
index 44ff460692..b166f89624 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -223,6 +223,11 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint
 	return 0;
 }
 
+void hashfile_checkpoint_release(struct hashfile_checkpoint *checkpoint)
+{
+	git_hash_discard(&checkpoint->ctx);
+}
+
 void crc32_begin(struct hashfile *f)
 {
 	f->crc32 = crc32(0, NULL, 0);
diff --git a/csum-file.h b/csum-file.h
index d1a0ff29cd..6ed74d1637 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -39,6 +39,7 @@ struct hashfile_checkpoint {
 void hashfile_checkpoint_init(struct hashfile *, struct hashfile_checkpoint *);
 void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint *);
 int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);
+void hashfile_checkpoint_release(struct hashfile_checkpoint *);
 
 /* finalize_hashfile flags */
 #define CSUM_CLOSE		1
diff --git a/object-file.c b/object-file.c
index e3d92bbda2..32a0d6d237 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1352,6 +1352,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 			   state->alloc_written);
 		state->written[state->nr_written++] = idx;
 	}
+
+	hashfile_checkpoint_release(&checkpoint);
 	return 0;
 }
 
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 5/9] patch-id: discard hash when done
From: Jeff King @ 2026-07-02  8:04 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

When computing a patch-id, we have a flush_one_hunk() helper that calls
git_hash_final() on our running hunk git_hash_ctx, and then
reinitializes that context for the next hunk.

When we run out of hunks to look at, we return, discarding the
git_hash_ctx. This can cause a leak if the hash implementation we are
using allocates any memory during its initialization. This includes
OpenSSL >= 3.0, for both SHA-1 and SHA-256. Normally we would not use
SHA-1 here at all, as we only recommend using non-DC implementations for
the "unsafe" variant (and patch-id, though they probably _could_ use the
unsafe variant, were never taught to do so).

But it is certainly a problem for SHA-256, which you can see with:

  make SANITIZE=leak \
       OPENSSL_SHA256=1 \
       GIT_TEST_DEFAULT_HASH=sha256 \
       test

That results in leak failures of 60 scripts, 57 of which are fixed by
this patch (basically anything which runs rebase will hit this case).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/patch-id.c | 1 +
 diff.c             | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 2781598ede..57d9bd4a65 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -173,6 +173,7 @@ static size_t get_one_patchid(struct object_id *next_oid, struct object_id *resu
 		oidclr(next_oid, the_repository->hash_algo);
 
 	flush_one_hunk(result, &ctx);
+	git_hash_discard(&ctx);
 
 	return patchlen;
 }
diff --git a/diff.c b/diff.c
index 2a9d0d8687..1568f0ed9c 100644
--- a/diff.c
+++ b/diff.c
@@ -6987,6 +6987,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
 		flush_one_hunk(oid, &ctx);
 	}
 
+	git_hash_discard(&ctx);
 	return 0;
 }
 
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 6/9] check_stream_oid(): discard hash on read error
From: Jeff King @ 2026-07-02  8:05 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

The happy path of check_stream_oid() is to initialize a hash, feed the
loose object zlib stream into it, and then get the final result. But if
we hit a zlib error or see extra cruft we'll bail early with an error.

Since we never call git_hash_final() in this cases, any resources held
by the git_hash_ctx may be leaked. Our default hash algorithms don't
allocate anything in the hash_ctx, but some implementations do. For
example, running:

  make SANITIZE=leak \
       OPENSSL_SHA256=1 \
       GIT_TEST_DEFAULT_HASH=sha256 \
       test

will fail t1450, since it feeds corrupted objects that cause us to bail
from check_stream_oid(). This patch fixes it by discarding the hash in
those early return paths. Trying to jump to a common "out:" label is not
worth it here, as we must _not_ discard a hash that was already fed to
git_hash_final(). And the hash_ctx itself does not carry any information
(so we cannot check for a NULL pointer, etc).

Signed-off-by: Jeff King <peff@peff.net>
---
 object-file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/object-file.c b/object-file.c
index 32a0d6d237..035d005279 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1587,11 +1587,13 @@ static int check_stream_oid(git_zstream *stream,
 
 	if (status != Z_STREAM_END) {
 		error(_("corrupt loose object '%s'"), oid_to_hex(expected_oid));
+		git_hash_discard(&c);
 		return -1;
 	}
 	if (stream->avail_in) {
 		error(_("garbage at end of loose object '%s'"),
 		      oid_to_hex(expected_oid));
+		git_hash_discard(&c);
 		return -1;
 	}
 
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 7/9] http: discard hash in dumb-http http_object_request
From: Jeff King @ 2026-07-02  8:07 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

Usually an object request results in finish_http_object_request()
calling git_hash_final_oid(), after we've received all of the data. But
if we hit an error, we'll bail early and free the http_object_request,
dropping the git_hash_ctx entirely.  This can cause a leak for hash
implementations that allocate memory in their context, like OpenSSL >=
3.0.

The obvious fix is for abort_http_object_request() to call
git_hash_discard(), under the assumption that every request is either
finished or aborted. But that's not quite true:

  1. Not everybody calls the abort function. Sometimes they jump
     straight to release_http_object_request(). So we'd have to put it
     there.

  2. After the finish function finalizes the hash, we can still
     encounter errors! In that case we end up aborting or releasing,
     and they must not discard that hash (since that would be a
     double-free).

So we'll keep a flag marking the validity of the hash_ctx field of the
request. The lifetime is simple: it is valid immediately after creation,
up until we call finalize. And then our release function can just
conditionally discard the hash based on that flag.

This fixes test failures in t5550 and t5619 when run with:

  make SANITIZE=leak \
       OPENSSL_SHA256=1 \
       GIT_TEST_DEFAULT_HASH=sha256 \
       test

The flag handling could be removed if the hash-discard function were
idempotent. This could be done easily-ish by having the underlying
hash functions (like the ones in sha256/openssl.h) set the context
pointer to NULL after free-ing. But it's something that every platform
implementation would have to remember to do, and the benefit for the
callers is not that huge (it would let us shave a few lines here and
probably in a few other spots).

Signed-off-by: Jeff King <peff@peff.net>
---
I think the "set to NULL" thing gets weird with gcrypt, too, which does
not even use a pointer (we typedef their libgcrypt handle into our own
context struct).

 http.c | 4 ++++
 http.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/http.c b/http.c
index b4e7b8d00b..63abbaae8a 100644
--- a/http.c
+++ b/http.c
@@ -2880,6 +2880,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	git_inflate_init(&freq->stream);
 
 	the_hash_algo->init_fn(&freq->c);
+	freq->hash_ctx_valid = 1;
 
 	freq->url = get_remote_object_url(base_url, hex, 0);
 
@@ -2988,6 +2989,7 @@ int finish_http_object_request(struct http_object_request *freq)
 	}
 
 	git_hash_final_oid(&freq->real_oid, &freq->c);
+	freq->hash_ctx_valid = 0;
 	if (freq->zret != Z_STREAM_END) {
 		unlink_or_warn(freq->tmpfile.buf);
 		return -1;
@@ -3028,6 +3030,8 @@ void release_http_object_request(struct http_object_request **freq_p)
 	curl_slist_free_all(freq->headers);
 	strbuf_release(&freq->tmpfile);
 	git_inflate_end(&freq->stream);
+	if (freq->hash_ctx_valid)
+		git_hash_discard(&freq->c);
 
 	free(freq);
 	*freq_p = NULL;
diff --git a/http.h b/http.h
index 729c51904d..6b0639150f 100644
--- a/http.h
+++ b/http.h
@@ -255,6 +255,7 @@ struct http_object_request {
 	struct object_id oid;
 	struct object_id real_oid;
 	struct git_hash_ctx c;
+	int hash_ctx_valid;
 	git_zstream stream;
 	int zret;
 	int rename;
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 8/9] hash: fix memory leak copying sha256 gcrypt handles
From: Jeff King @ 2026-07-02  8:09 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

Our abstracted hash-algorithm API allows for cloning a hash context. By
default this just memcpy()s the bytes, but specific implementations can
provide a custom clone function.

Our API is based around the way that OpenSSL works, which is that you
first initialize the destination context, then copy into it. In our code
that is this:

  algo->init_fn(&dst);
  git_hash_clone(&dst, src);

and that translates into OpenSSL calls like:

  /* init_fn */
  dst->ectx = EVP_MD_CTX_new();
  EVP_DigestInit_ex(dst->ectx, EVP_sha256());
  /* clone */
  EVP_MD_CTX_copy_ex(dst->ectx, src->ectx);

So the allocation happens in the first step, and then the clone is just
copying values (the DigestInit is initializing values that just get
overwritten, but that's not wrong, just a little inefficient).

But libgcrypt doesn't work like that! Its copy function initializes dst
from scratch. So when using the sha256 gcrypt backend, that becomes:

  /* init_fn; this allocates */
  gcry_md_open(&dst, GCRY_MD_SHA256);
  /* clone; this also allocates, leaking the previous value! */
  gcry_md_copy(&dst, src);

You can see the leaks in the test suite by running:

  make \
    SANITIZE=leak \
    GCRYPT_SHA256=1 \
    GIT_TEST_DEFAULT_SHA=256 \
    test

which has many failures, as opposed to building with OPENSSL_SHA256,
which is leak-free.

The easy fix here is for the clone function to close the open context
we're about to overwrite. It's a little inefficient (we did a pointless
open in the init function), but probably not a big deal in practice.

If our API went the other way, assuming that we're always cloning into
garbage bytes, then we could be more efficient. We'd teach OpenSSL's
clone function to do its own new(), skip the DigestInit, and then copy
into it. And gcrypt could stick with just the copy() call.

But look again at the asymmetry in the very first code example. We call
the init function straight from the git_hash_algo struct, and then
subsequent calls are dispatched through our git_hash_* wrappers. If you
wanted to clone into an uninitialized destination, you'd do something
like:

  algo->clone_fn(&dst, src);

instead. That would require changing all of the callers. There's not
that many of them, but I don't know that it's worth changing our calling
conventions to try to reclaim this tiny bit of efficiency.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha256/gcrypt.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sha256/gcrypt.h b/sha256/gcrypt.h
index 17a90f1052..694a2b70a1 100644
--- a/sha256/gcrypt.h
+++ b/sha256/gcrypt.h
@@ -27,6 +27,7 @@ static inline void gcrypt_SHA256_Final(unsigned char *digest, gcrypt_SHA256_CTX
 
 static inline void gcrypt_SHA256_Clone(gcrypt_SHA256_CTX *dst, const gcrypt_SHA256_CTX *src)
 {
+	gcry_md_close(*dst);
 	gcry_md_copy(dst, *src);
 }
 
-- 
2.55.0.418.g37da59dd42


^ permalink raw reply related

* [PATCH 9/9] hash: add platform-specific discard functions
From: Jeff King @ 2026-07-02  8:13 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt
In-Reply-To: <20260702075234.GA1548258@coredump.intra.peff.net>

Our git_hash_discard() is a bit hacky: it just calls git_hash_final()
into a dummy result buffer, using the side effect that each
implementation's Final() function will also free any resources.

This is probably not too terrible, since generating the final hash is
not that expensive and we'd mostly call discard on unusual or error code
paths. But we can do better by widening the platform API a bit to add an
explicit discard function.

This requires an annoying amount of boilerplate:

  - Each algorithm needs a git_$ALGO_discard() wrapper that dereferences
    the union'd git_hash_ctx into the type-safe field. So sha1 + sha256
    + sha1-unsafe, plus a BUG() for the unknown algo. And then these all
    need to be referenced in the git_hash_algo structs.

  - Platforms which don't do anything special to discard now need a
    fallback function which does nothing. And we need this for each algo
    (sha1, sha256, and sha1-unsafe).

  - Platforms which do need to discard must define their discard
    functions. This includes sha1/openssl, sha256/openssl, and
    sha256/gcrypt (no sha1-unsafe here as it sits atop the sha1/openssl
    functions).

  - Algo selection needs to point platform_*_Discard to the appropriate
    underlying macro, or indicate that the fallback should be used. We
    have a similar situation for the Clone function (where a straight
    memcpy() of the context struct is not enough for some platforms).
    I've tied Discard to the same flag used by Clone here, since they
    are basically the same problem: is the hash context a sequence of
    bytes, or does it need smart copying/discarding?

It's easy to miss a case here since we don't even compile the
implementations we aren't using. I've tested with each of:

  - no flags, which uses our internal sha1/sha256 implementations, both
    of which exercise the noop fallback function

  - OPENSSL_SHA1_UNSAFE=1, which checks that our unsafe macro
    redirections work

  - OPENSSL_SHA1=1, though you should not do that in real life!

  - OPENSSL_SHA256=1, passes tests with GIT_TEST_DEFAULT_HASH=sha256

  - GCRYPT_SHA256=1, which likewise passes

The other implementations do not set the CLONE_HELPER flag, so they
treat the context as bytes and should be fine with the fallback.

Signed-off-by: Jeff King <peff@peff.net>
---
One of the reasons I left this to the end is that I wasn't sure it would
be worth it. I think it probably is (hence posting it), but we could
live with the hacky implementation forever if we wanted. :)

It also prompted me to test with all of the backends I could build,
which is how I found the unrelated gcrypt leak fixed in patch 8.

 hash.c           | 33 +++++++++++++++++++++++++--------
 hash.h           | 21 +++++++++++++++++++++
 sha1/openssl.h   |  6 ++++++
 sha256/gcrypt.h  |  6 ++++++
 sha256/openssl.h |  6 ++++++
 5 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/hash.c b/hash.c
index 63672a3d22..55d1d41770 100644
--- a/hash.c
+++ b/hash.c
@@ -72,6 +72,11 @@ static void git_hash_sha1_final_oid(struct object_id *oid, struct git_hash_ctx *
 	oid->algo = GIT_HASH_SHA1;
 }
 
+static void git_hash_sha1_discard(struct git_hash_ctx *ctx)
+{
+	git_SHA1_Discard(&ctx->state.sha1);
+}
+
 static void git_hash_sha1_init_unsafe(struct git_hash_ctx *ctx)
 {
 	ctx->algop = unsafe_hash_algo(&hash_algos[GIT_HASH_SHA1]);
@@ -102,6 +107,11 @@ static void git_hash_sha1_final_oid_unsafe(struct object_id *oid, struct git_has
 	oid->algo = GIT_HASH_SHA1;
 }
 
+static void git_hash_sha1_discard_unsafe(struct git_hash_ctx *ctx)
+{
+	git_SHA1_Discard_unsafe(&ctx->state.sha1_unsafe);
+}
+
 static void git_hash_sha256_init(struct git_hash_ctx *ctx)
 {
 	ctx->algop = unsafe_hash_algo(&hash_algos[GIT_HASH_SHA256]);
@@ -135,6 +145,11 @@ static void git_hash_sha256_final_oid(struct object_id *oid, struct git_hash_ctx
 	oid->algo = GIT_HASH_SHA256;
 }
 
+static void git_hash_sha256_discard(struct git_hash_ctx *ctx)
+{
+	git_SHA256_Discard(&ctx->state.sha256);
+}
+
 static void git_hash_unknown_init(struct git_hash_ctx *ctx UNUSED)
 {
 	BUG("trying to init unknown hash");
@@ -165,6 +180,11 @@ static void git_hash_unknown_final_oid(struct object_id *oid UNUSED,
 	BUG("trying to finalize unknown hash");
 }
 
+static void git_hash_unknown_discard(struct git_hash_ctx *ctx UNUSED)
+{
+	BUG("trying to discard unknown hash");
+}
+
 static const struct git_hash_algo sha1_unsafe_algo = {
 	.name = "sha1",
 	.format_id = GIT_SHA1_FORMAT_ID,
@@ -176,6 +196,7 @@ static const struct git_hash_algo sha1_unsafe_algo = {
 	.update_fn = git_hash_sha1_update_unsafe,
 	.final_fn = git_hash_sha1_final_unsafe,
 	.final_oid_fn = git_hash_sha1_final_oid_unsafe,
+	.discard_fn = git_hash_sha1_discard_unsafe,
 	.empty_tree = &empty_tree_oid,
 	.empty_blob = &empty_blob_oid,
 	.null_oid = &null_oid_sha1,
@@ -193,6 +214,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
 		.update_fn = git_hash_unknown_update,
 		.final_fn = git_hash_unknown_final,
 		.final_oid_fn = git_hash_unknown_final_oid,
+		.discard_fn = git_hash_unknown_discard,
 		.empty_tree = NULL,
 		.empty_blob = NULL,
 		.null_oid = NULL,
@@ -208,6 +230,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
 		.update_fn = git_hash_sha1_update,
 		.final_fn = git_hash_sha1_final,
 		.final_oid_fn = git_hash_sha1_final_oid,
+		.discard_fn = git_hash_sha1_discard,
 		.unsafe = &sha1_unsafe_algo,
 		.empty_tree = &empty_tree_oid,
 		.empty_blob = &empty_blob_oid,
@@ -224,6 +247,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
 		.update_fn = git_hash_sha256_update,
 		.final_fn = git_hash_sha256_final,
 		.final_oid_fn = git_hash_sha256_final_oid,
+		.discard_fn = git_hash_sha256_discard,
 		.empty_tree = &empty_tree_oid_sha256,
 		.empty_blob = &empty_blob_oid_sha256,
 		.null_oid = &null_oid_sha256,
@@ -285,14 +309,7 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
 
 void git_hash_discard(struct git_hash_ctx *ctx)
 {
-	/*
-	 * XXX Many implementations do not need to do anything here,
-	 * and a dummy final() call is wasteful. But we can't fix
-	 * that unless our implementation API exposes a discard
-	 * primitive.
-	 */
-	unsigned char dummy[GIT_MAX_RAWSZ];
-	git_hash_final(dummy, ctx);
+	ctx->algop->discard_fn(ctx);
 }
 
 uint32_t hash_algo_by_name(const char *name)
diff --git a/hash.h b/hash.h
index 6b2f04e2a4..0a23ef4dfd 100644
--- a/hash.h
+++ b/hash.h
@@ -37,6 +37,7 @@
 #    define platform_SHA1_Clone_unsafe openssl_SHA1_Clone
 #    define platform_SHA1_Update_unsafe openssl_SHA1_Update
 #    define platform_SHA1_Final_unsafe openssl_SHA1_Final
+#    define platform_SHA1_Discard_unsafe openssl_SHA1_Discard
 #  else
 #    define platform_SHA_CTX_unsafe SHA_CTX
 #    define platform_SHA1_Init_unsafe SHA1_Init
@@ -92,6 +93,7 @@
 #  define platform_SHA1_Final_unsafe   platform_SHA1_Final
 #  ifdef platform_SHA1_Clone
 #    define platform_SHA1_Clone_unsafe platform_SHA1_Clone
+#    define platform_SHA1_Discard_unsafe platform_SHA1_Discard
 #  endif
 #  ifdef SHA1_NEEDS_CLONE_HELPER
 #    define SHA1_NEEDS_CLONE_HELPER_UNSAFE
@@ -110,9 +112,11 @@
 
 #ifdef platform_SHA1_Clone
 #define git_SHA1_Clone	platform_SHA1_Clone
+#define git_SHA1_Discard platform_SHA1_Discard
 #endif
 #ifdef platform_SHA1_Clone_unsafe
 #  define git_SHA1_Clone_unsafe platform_SHA1_Clone_unsafe
+#  define git_SHA1_Discard_unsafe platform_SHA1_Discard_unsafe
 #endif
 
 #ifndef platform_SHA256_CTX
@@ -129,6 +133,7 @@
 
 #ifdef platform_SHA256_Clone
 #define git_SHA256_Clone	platform_SHA256_Clone
+#define git_SHA256_Discard	platform_SHA256_Discard
 #endif
 
 #ifdef SHA1_MAX_BLOCK_SIZE
@@ -142,20 +147,32 @@ static inline void git_SHA1_Clone(git_SHA_CTX *dst, const git_SHA_CTX *src)
 {
 	memcpy(dst, src, sizeof(*dst));
 }
+static inline void git_SHA1_Discard(git_SHA_CTX *ctx UNUSED)
+{
+	/* noop */
+}
 #endif
 #ifndef SHA1_NEEDS_CLONE_HELPER_UNSAFE
 static inline void git_SHA1_Clone_unsafe(git_SHA_CTX_unsafe *dst,
 				       const git_SHA_CTX_unsafe *src)
 {
 	memcpy(dst, src, sizeof(*dst));
 }
+static inline void git_SHA1_Discard_unsafe(git_SHA_CTX_unsafe *ctx UNUSED)
+{
+	/* noop */
+}
 #endif
 
 #ifndef SHA256_NEEDS_CLONE_HELPER
 static inline void git_SHA256_Clone(git_SHA256_CTX *dst, const git_SHA256_CTX *src)
 {
 	memcpy(dst, src, sizeof(*dst));
 }
+static inline void git_SHA256_Discard(git_SHA256_CTX *ctx UNUSED)
+{
+	/* noop */
+}
 #endif
 
 /*
@@ -271,6 +288,7 @@ typedef void (*git_hash_clone_fn)(struct git_hash_ctx *dst, const struct git_has
 typedef void (*git_hash_update_fn)(struct git_hash_ctx *ctx, const void *in, size_t len);
 typedef void (*git_hash_final_fn)(unsigned char *hash, struct git_hash_ctx *ctx);
 typedef void (*git_hash_final_oid_fn)(struct object_id *oid, struct git_hash_ctx *ctx);
+typedef void (*git_hash_discard_fn)(struct git_hash_ctx *ctx);
 
 struct git_hash_algo {
 	/*
@@ -306,6 +324,9 @@ struct git_hash_algo {
 	/* The hash finalization function for object IDs. */
 	git_hash_final_oid_fn final_oid_fn;
 
+	/* Discard an initialized hash without finalizing. */
+	git_hash_discard_fn discard_fn;
+
 	/* The OID of the empty tree. */
 	const struct object_id *empty_tree;
 
diff --git a/sha1/openssl.h b/sha1/openssl.h
index 1038af47da..48deeb724a 100644
--- a/sha1/openssl.h
+++ b/sha1/openssl.h
@@ -40,12 +40,18 @@ static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst,
 	EVP_MD_CTX_copy_ex(dst->ectx, src->ectx);
 }
 
+static inline void openssl_SHA1_Discard(struct openssl_SHA1_CTX *ctx)
+{
+	EVP_MD_CTX_free(ctx->ectx);
+}
+
 #ifndef platform_SHA_CTX
 #define platform_SHA_CTX openssl_SHA1_CTX
 #define platform_SHA1_Init openssl_SHA1_Init
 #define platform_SHA1_Clone openssl_SHA1_Clone
 #define platform_SHA1_Update openssl_SHA1_Update
 #define platform_SHA1_Final openssl_SHA1_Final
+#define platform_SHA1_Discard openssl_SHA1_Discard
 #endif
 
 #endif /* SHA1_OPENSSL_H */
diff --git a/sha256/gcrypt.h b/sha256/gcrypt.h
index 694a2b70a1..d91ffe73d3 100644
--- a/sha256/gcrypt.h
+++ b/sha256/gcrypt.h
@@ -31,10 +31,16 @@ static inline void gcrypt_SHA256_Clone(gcrypt_SHA256_CTX *dst, const gcrypt_SHA2
 	gcry_md_copy(dst, *src);
 }
 
+static inline void gcrypt_SHA256_Discard(gcrypt_SHA256_CTX *ctx)
+{
+	gcry_md_close(*ctx);
+}
+
 #define platform_SHA256_CTX gcrypt_SHA256_CTX
 #define platform_SHA256_Init gcrypt_SHA256_Init
 #define platform_SHA256_Clone gcrypt_SHA256_Clone
 #define platform_SHA256_Update gcrypt_SHA256_Update
 #define platform_SHA256_Final gcrypt_SHA256_Final
+#define platform_SHA256_Discard gcrypt_SHA256_Discard
 
 #endif
diff --git a/sha256/openssl.h b/sha256/openssl.h
index c1083d9491..3d457ca99d 100644
--- a/sha256/openssl.h
+++ b/sha256/openssl.h
@@ -40,10 +40,16 @@ static inline void openssl_SHA256_Clone(struct openssl_SHA256_CTX *dst,
 	EVP_MD_CTX_copy_ex(dst->ectx, src->ectx);
 }
 
+static inline void openssl_SHA256_Discard(struct openssl_SHA256_CTX *ctx)
+{
+	EVP_MD_CTX_free(ctx->ectx);
+}
+
 #define platform_SHA256_CTX openssl_SHA256_CTX
 #define platform_SHA256_Init openssl_SHA256_Init
 #define platform_SHA256_Clone openssl_SHA256_Clone
 #define platform_SHA256_Update openssl_SHA256_Update
 #define platform_SHA256_Final openssl_SHA256_Final
+#define platform_SHA256_Discard openssl_SHA256_Discard
 
 #endif /* SHA256_OPENSSL_H */
-- 
2.55.0.418.g37da59dd42

^ permalink raw reply related

* Re: [PATCH v2 2/4] parse-options: add a separate case for help output on error
From: Jeff King @ 2026-07-02  8:38 UTC (permalink / raw)
  To: brian m. carlson; +Cc: git, Junio C Hamano
In-Reply-To: <20260701212442.1430084-3-sandals@crustytoothpaste.net>

On Wed, Jul 01, 2026 at 09:24:40PM +0000, brian m. carlson wrote:

> However, there are some cases where we print help output because the
> user has provided ambiguous or invalid input, such as an ambiguous
> option, and we'll want to exit unsuccessfully there.  Make this easier
> by defining a new return code, PARSE_OPT_HELP_ERROR, that can be used in
> this case, while reserving PARSE_OPT_HELP for those cases where the user
> has requested help directly.

Makes sense. We'd want to audit every spot that generates PARSE_OPT_HELP
and see if it should be PARSE_OPT_HELP_ERROR. I only see one spot
touched here:

> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -583,7 +583,7 @@ static enum parse_opt_result parse_long_opt(
>  			ambiguous.option->long_name,
>  			(abbrev.flags & OPT_UNSET) ?  "no-" : "",
>  			abbrev.option->long_name);
> -		return PARSE_OPT_HELP;
> +		return PARSE_OPT_HELP_ERROR;
>  	}

That one makes sense. The other site that generates it is within
usage_with_options_internal(), which handles both asked-for "-h" and
unexpected errors, but still always returns PARSE_OPT_HELP.

Ah...it looks like you _do_ switch it in patch 4 (when the distinction
between the two starts to make a difference). I think it should be done
in this patch, though, since the point is generating the correct
HELP/HELP_ERROR here (even though it does not yet matter).

I wonder if we'd also want:

diff --git a/parse-options.c b/parse-options.c
index 742444eead..08c21d9fc0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1373,7 +1373,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	parse_options_check_harder(opts);
 
 	if (!usagestr)
-		return PARSE_OPT_HELP;
+		return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");

I can't figure out when we wouldn't have a usagestr, though. Perhaps not
ever from parse-options itself, but only when called via
usage_with_options() or something? That function does not look at our
return value so it would not matter, but it feels like we should keep
things consistent.

-Peff

^ permalink raw reply related

* Re: [PATCH v2 0/4] rev-parse: exit 0 on --help
From: Jeff King @ 2026-07-02  8:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <xmqqcxx6mkjn.fsf@gitster.g>

On Wed, Jul 01, 2026 at 03:06:52PM -0700, Junio C Hamano wrote:

> > One of the patches is rather long because we have many cases in which
> > we've hard-coded exit code 129 into our tests.  However, the changes
> > there should not be complex, only somewhat tedious to review.
> 
> It is borderline for "yes, we all know it is obvious that things
> should have worked this way from day one, we regret that it is not
> the case, but it has been working differently and users' scripts all
> have been working with the current behaviour, and it is likely that
> they will all break".
> 
> Two big things that make it much less likely, saving grace, are that
> this is only about "--help" (which is unlikely to be a part of
> end-user script), and this makes the invocation succeed (if we were
> changing from exit 0 to exit 129, we would be breaking tons more).

My big concern is a script accidentally continuing when fed "--help" and
generating nonsense. But I think the eval magic explained in patch 3
makes that unlikely (any such caller was already kind-of broken).

The other issue I raised in the earlier round is that a script like:

  cat >git-foo <<\EOF
  #!/bin/sh
  git log --my-options "$@" >output || exit 1
  do_something <output
  EOF

when invoked as "git foo --help" will now call do_something with
nonsense input, rather than exiting from the "error" returned by
git-log. This only affects hacky little scripts like this that are not
otherwise parsing their own options, but sometimes those are the most
common. ;)

I'm not convinced there will be much fallout, but it is possible.

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Jeff King @ 2026-07-02  8:58 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <akTXYoY7mSQUM33P@pks.im>

On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:

> > > linux-reftable or linux-reftable-leaks? I think it would certainly make
> > > sense to drop one of these and merge it into linux-TEST-vars. The
> > > linux-reftable job doesn't provide any benefit over its -leak variant,
> > > so that would be the candidate I'd personally merge.
> > 
> > Both. Fold linux-reftable into linux-TEST-vars, and then drop
> > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
> 
> Hm, okay. I guess that should be fine. Do we also want to do a similar
> thing for macOS and create a macos-TEST-vars job that exercises all of
> this?

It could be helpful if we expect the interaction of macOS and those
test-vars to be interesting, but I'm a bit skeptical. Most of them are
about feature selection. So I'm doubtful it would turn up anything
useful. But who knows.

Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
both is useful, as they have different warnings. But have we ever seen a
case where running the tests showed a different result with different
compilers?

I dunno. I guess there is an argument for CI-maximalism; as long as the
jobs run in parallel and they're "just" CPU-minutes. But those minutes
eventually have a cost, and I'm not sure I've gotten useful data from
most of the jobs (i.e., failures that didn't also just happen somewhere
else).

Anyway, that is all a big tangent/rant. Mostly I think it would be fine
to cannibalize linux-reftable into linux-TEST-vars if we want to get
more coverage without increasing the CI cost.

Note that I did find some leaks that would only be hit running
linux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But
that is getting super specific now (even if we ran linux-sha256 with
leak detection, would we want to do it with openssl and not the default
backend)?

> Also, while at it... I really think that job name is just plain awful.
> While at it, we might rename it to something more sensible like
> "linux-changed-defaults".

Yes please. Every time I see the all-caps TEST in the middle I think I'm
having a stroke.

change-defaults is OK but not super descriptive. I might call it
linux-exotic-flags or something. That's not descriptive either, but is a
little more fun.

-Peff

^ permalink raw reply

* Re: [PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
From: Christian Couder @ 2026-07-02  9:31 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-5-b0228e7d908d@pks.im>

On Mon, Jun 29, 2026 at 11:02 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> Introduce a new test helper that allows us to write reftable blocks.
> This helper will be used by subsequent commits.
>
> Suggested-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> index f4bded7d26..f4e926ce3a 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -14,6 +14,31 @@ license that can be found in the LICENSE file or at
>  #include "reftable/reftable-error.h"
>  #include "strbuf.h"
>
> +static int cl_reftable_write_block(struct reftable_buf *buf,
> +                                  uint8_t block_type,
> +                                  struct reftable_record *recs,
> +                                  size_t nrecs)

Yeah, I suggested:

int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
                           size_t block_size, uint32_t header_off,
                           struct reftable_record *recs, size_t nrecs)

which accepts `size_t block_size` and `uint32_t header_off` as
arguments, so that more existing tests could be refactored using
cl_reftable_write_block().

Your choice to not have these extra arguments is reasonable though, as
they are not needed for the code that your series adds, and they make
the implementation of cl_reftable_write_block() a bit more complex.

Also they can still be added in the future if we really want to clean
up more existing tests.

This version of your series looks good to me now.

Thanks.

^ permalink raw reply

* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Patrick Steinhardt @ 2026-07-02 10:08 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Karthik Nayak
In-Reply-To: <20260702085821.GC481298@coredump.intra.peff.net>

On Thu, Jul 02, 2026 at 04:58:21AM -0400, Jeff King wrote:
> On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:
> 
> > > > linux-reftable or linux-reftable-leaks? I think it would certainly make
> > > > sense to drop one of these and merge it into linux-TEST-vars. The
> > > > linux-reftable job doesn't provide any benefit over its -leak variant,
> > > > so that would be the candidate I'd personally merge.
> > > 
> > > Both. Fold linux-reftable into linux-TEST-vars, and then drop
> > > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
> > 
> > Hm, okay. I guess that should be fine. Do we also want to do a similar
> > thing for macOS and create a macos-TEST-vars job that exercises all of
> > this?
> 
> It could be helpful if we expect the interaction of macOS and those
> test-vars to be interesting, but I'm a bit skeptical. Most of them are
> about feature selection. So I'm doubtful it would turn up anything
> useful. But who knows.
> 
> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
> both is useful, as they have different warnings. But have we ever seen a
> case where running the tests showed a different result with different
> compilers?

Not that I'd know of. As you say, I think it makes sense to use
different compilers in general. But I don't really think we need to have
this as a full "compiler x tests" matrix.

> I dunno. I guess there is an argument for CI-maximalism; as long as the
> jobs run in parallel and they're "just" CPU-minutes. But those minutes
> eventually have a cost, and I'm not sure I've gotten useful data from
> most of the jobs (i.e., failures that didn't also just happen somewhere
> else).

I'm certainly on board with reducing the test matrix a bit. I'm sure
that we can have a cleverer selection of jobs where we both have the
same test coverage as we have right now while running less jobs overall.

> Anyway, that is all a big tangent/rant. Mostly I think it would be fine
> to cannibalize linux-reftable into linux-TEST-vars if we want to get
> more coverage without increasing the CI cost.

You got to start somewhere :)

> Note that I did find some leaks that would only be hit running
> linux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But
> that is getting super specific now (even if we ran linux-sha256 with
> leak detection, would we want to do it with openssl and not the default
> backend)?
> 
> > Also, while at it... I really think that job name is just plain awful.
> > While at it, we might rename it to something more sensible like
> > "linux-changed-defaults".
> 
> Yes please. Every time I see the all-caps TEST in the middle I think I'm
> having a stroke.

Heh :P

> change-defaults is OK but not super descriptive. I might call it
> linux-exotic-flags or something. That's not descriptive either, but is a
> little more fun.

I certainly like it more than my suggestion.

Thanks!

Patrick

^ permalink raw reply

* Re: [PATCH] meson: restore hook-list.h to builtin_sources
From: Adrian Ratiu @ 2026-07-02 10:34 UTC (permalink / raw)
  To: Mike Gilbert, git; +Cc: Mike Gilbert
In-Reply-To: <20260701193928.358825-1-floppym@gentoo.org>

On Wed, 01 Jul 2026, Mike Gilbert <floppym@gentoo.org> wrote:
> This fixes a racy build failure.
>
> ```
> builtin/bugreport.c:12:10: fatal error: hook-list.h: No such file or directory
>    12 | #include "hook-list.h"
>       |          ^~~~~~~~~~~~~
>
> ```
>
> hook-list.h must be generated before builtin/bugreport.c is compiled.
>
> Bug: https://bugs.gentoo.org/978326
> Fixes: 2eb541e8f2a9 (hook: move is_known_hook() to hook.c for wider use, 2026-04-10)
> Signed-off-by: Mike Gilbert <floppym@gentoo.org>
> ---
>  meson.build | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 3247697f74aa..bdc83843e8e0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -278,7 +278,20 @@ compat_sources = [
>    'compat/terminal.c',
>  ]
>  
> +hook_list = custom_target(
> +  input: 'Documentation/githooks.adoc',
> +  output: 'hook-list.h',
> +  command: [
> +    shell,
> +    meson.current_source_dir() + '/tools/generate-hooklist.sh',
> +    meson.current_source_dir(),
> +    '@OUTPUT@',
> +  ],
> +  env: script_environment,
> +)
> +
>  libgit_sources = [
> +  hook_list,
>    'abspath.c',
>    'add-interactive.c',
>    'add-patch.c',
> @@ -566,19 +579,8 @@ libgit_sources += custom_target(
>    env: script_environment,
>  )
>  
> -libgit_sources += custom_target(
> -  input: 'Documentation/githooks.adoc',
> -  output: 'hook-list.h',
> -  command: [
> -    shell,
> -    meson.current_source_dir() + '/tools/generate-hooklist.sh',
> -    meson.current_source_dir(),
> -    '@OUTPUT@',
> -  ],
> -  env: script_environment,
> -)
> -
>  builtin_sources = [
> +  hook_list,
>    'builtin/add.c',
>    'builtin/am.c',
>    'builtin/annotate.c',
> -- 
> 2.54.0

LGTM, thanks and nice find!

Sorry for the build regression, at the time I did the builtin->libgit
move IIRC only libgit was using the generated file, but it's clearly
safer to have it for both precisely to avoid these kinds of build races.

^ permalink raw reply

* Re: What's cooking in git.git (Jul 2026, #01)
From: Patrick Steinhardt @ 2026-07-02 10:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv7ayl1nj.fsf@gitster.g>

On Wed, Jul 01, 2026 at 04:40:16PM -0700, Junio C Hamano wrote:
> * ps/odb-drop-whence (2026-06-24) 7 commits
>  - odb: document object info fields
>  - odb: drop `whence` field from object info
>  - treewide: convert users of `whence` to the new source field
>  - odb: add `source` field to struct object_info_source
>  - odb: make backend-specific fields optional
>  - packfile: thread odb_source_packed through packed_object_info()
>  - Merge branch 'ps/odb-source-packed' into ps/odb-drop-whence
>  (this branch uses ps/odb-source-packed.)
> 
>  The whence field in struct object_info has been removed,
>  refactoring backend-specific object information retrieval into an
>  opt-in struct object_info_source structure.
> 
>  Will merge to 'next'?
>  cf. <akOod6X1a2axIXKZ@pks.im>
>  cf. <xmqqv7b0rmt6.fsf@gitster.g>
>  source: <20260624-b4-pks-odb-drop-whence-v1-0-8d1877b790ac@pks.im>

I'll send a small reroll to rename `sourcep` to `source_infop` based on
Justin's feedback.

> * ps/odb-generalize-prepare (2026-06-22) 3 commits
>  - odb: introduce `odb_prepare()`
>  - odb/source: generalize `reprepare()` callback
>  - Merge branch 'ps/odb-source-packed' into ps/odb-generalize-prepare
>  (this branch uses ps/odb-source-packed.)
> 
>  The `reprepare()` callback for object database sources has been
>  generalized into a `prepare()` callback with an optional flush cache
>  flag, and a new `odb_prepare()` wrapper has been introduced to
>  allow pre-opening object database sources.
> 
>  Will merge to 'next'?
>  cf. <87ik704f1j.fsf@emacs.iotcl.com>
>  source: <20260622-b4-pks-odb-generalize-prepare-v1-0-d2a5c5d13144@pks.im>

This one should be ready.

> * ps/refs-writing-subcommands (2026-06-30) 5 commits
>  - builtin/refs: add "rename" subcommand
>  - builtin/refs: add "create" subcommand
>  - builtin/refs: add "update" subcommand
>  - builtin/refs: add "delete" subcommand
>  - builtin/refs: drop `the_repository`
> 
>  The "git refs" toolbox has been extended with new "create", "delete",
>  "update", and "rename" subcommands to create, delete, update, and
>  rename references, respectively.
> 
>  Will merge to 'next'?
>  cf. <xmqqcxx7susi.fsf@gitster.g>
>  source: <20260630-pks-refs-writing-subcommands-v3-0-deb04de1ecef@pks.im>

Likewise.

Thanks!

Patrick

^ permalink raw reply

* Re: [PATCH] meson: restore hook-list.h to builtin_sources
From: Patrick Steinhardt @ 2026-07-02 11:06 UTC (permalink / raw)
  To: Mike Gilbert; +Cc: git, adrian.ratiu
In-Reply-To: <20260701193928.358825-1-floppym@gentoo.org>

On Wed, Jul 01, 2026 at 03:39:28PM -0400, Mike Gilbert wrote:
> This fixes a racy build failure.
> 
> ```
> builtin/bugreport.c:12:10: fatal error: hook-list.h: No such file or directory
>    12 | #include "hook-list.h"
>       |          ^~~~~~~~~~~~~
> 
> ```
> 
> hook-list.h must be generated before builtin/bugreport.c is compiled.

"hook-list.h" is required by both "hook.c" and by "builtin/bugreport.c".
So you would expect that we indeed need the header generated for both of
these, but right now we only explicitly list the dependency for our
libgit sources, not to our builtin sources. And consequently the header
may not be generated:

    $ meson setup build
    ...
    $ ninja -C build git.p/builtin_bugreport.c.o
    ...
    ../builtin/bugreport.c:12:10: fatal error: 'hook-list.h' file not found
   12 | #include "hook-list.h"
      |          ^~~~~~~~~~~~~
   1 error generated.

The fix is of course to explicitly list the header for both targets.
And...

> diff --git a/meson.build b/meson.build
> index 3247697f74aa..bdc83843e8e0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -278,7 +278,20 @@ compat_sources = [
>    'compat/terminal.c',
>  ]
>  
> +hook_list = custom_target(
> +  input: 'Documentation/githooks.adoc',
> +  output: 'hook-list.h',
> +  command: [
> +    shell,
> +    meson.current_source_dir() + '/tools/generate-hooklist.sh',
> +    meson.current_source_dir(),
> +    '@OUTPUT@',
> +  ],
> +  env: script_environment,
> +)
> +
>  libgit_sources = [
> +  hook_list,
>    'abspath.c',
>    'add-interactive.c',
>    'add-patch.c',
> @@ -566,19 +579,8 @@ libgit_sources += custom_target(
>    env: script_environment,
>  )
>  
> -libgit_sources += custom_target(
> -  input: 'Documentation/githooks.adoc',
> -  output: 'hook-list.h',
> -  command: [
> -    shell,
> -    meson.current_source_dir() + '/tools/generate-hooklist.sh',
> -    meson.current_source_dir(),
> -    '@OUTPUT@',
> -  ],
> -  env: script_environment,
> -)
> -
>  builtin_sources = [
> +  hook_list,
>    'builtin/add.c',
>    'builtin/am.c',
>    'builtin/annotate.c',

... that's exactly what you do. So this fix looks good to me, thanks!

Patrick

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox