* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Jeff King @ 2026-07-14 6:46 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alVn-QmK3K91_tkH@com-76773>
On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
Yuck. In theory they're writing the same thing, but I think the source
of the corruption is append mode. Two concurrent writers will keep
auto-seeking to the end of the file, rather than keeping their own file
pointers. There's no way to ask for O_APPEND without O_TRUNC via stdio,
but we can drop down a level like this:
diff --git a/http.c b/http.c
index b4e7b8d00b..d7362c99a2 100644
--- a/http.c
+++ b/http.c
@@ -2740,6 +2740,7 @@ struct http_pack_request *new_direct_http_pack_request(
{
off_t prev_posn = 0;
struct http_pack_request *preq;
+ int fd;
CALLOC_ARRAY(preq, 1);
strbuf_init(&preq->tmpfile, 0);
@@ -2748,12 +2749,13 @@ struct http_pack_request *new_direct_http_pack_request(
odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
strbuf_addstr(&preq->tmpfile, ".temp");
- preq->packfile = fopen(preq->tmpfile.buf, "a");
- if (!preq->packfile) {
+ fd = open(preq->tmpfile.buf, O_WRONLY|O_CREAT, 0666);
+ if (fd < 0) {
error("Unable to open local file %s for pack",
preq->tmpfile.buf);
goto abort;
}
+ preq->packfile = xfdopen(fd, "w");
preq->slot = get_active_slot();
preq->headers = object_request_headers();
That patch (with no other code changes) passes your test.
I suspect it could cause us to racily send an http range of "N-" to the
server, where N is the total number of bytes in the file (because we
don't know how many bytes there are supposed to be). I don't know if
that would cause an HTTP 416 or not. I think possibly not, and the 416
you saw (and that I see when running the test without any code changes)
might be from sending a range that starts _past_ N. We end up with a
too-long when both processes are appending.
I can't say I love the overall notion of "two processes are writing the
same data, it will probably be fine!". There might be portability
issues, and I'm not sure what would happen if we ever did get
conflicting data. If we're just feeding this to "index-pack --stdin"
we'd at least notice the problem (rather than quietly corrupting the
indexed file!).
So I'm offering this as a point for further discussion, and not
necessarily a counter-proposal. ;)
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.
Arguably losing the ability to retry is a regression. In general, I
think we should prefer correctness to efficiency. But I wonder if this
is a case where the user might want to make the choice to say "I am not
going to fetch two packfiles at once; please enable resumable fetches".
Especially because one of the selling points of packfile URIs is that
they are resumable.
One other thought on resumable transfers: if we are not going to resume
the transfer, then why spool the pack to disk at all? In other words,
why not just send it straight to "index-pack --stdin". That fixes your
concurrency issue (because it uses its own tempfiles behind the scene),
but has two other big advantages:
1. It halves the number of disk writes, and lowers the peak disk usage
(with the current code, there is a moment where both the tempfile
and the indexed pack are present on disk).
2. It pipelines the data processing. The current code bottlenecks on
the network while the CPU sits idle, and then bottlenecks on the
CPU once we have the whole file. We could be doing useful CPU work
during the network transfer, just like a regular pack code does.
So I'm not quite sold on losing the ability to resume entirely. And in
cases where we do lose it, I think it opens up other improvements.
But I'll reader over the rest of the patch with the notion that this is
the direction we want to go in.
> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
> line (which is not expected in
> this case), 'git http-fetch' fetches the packfile directly at the given
> URL and uses index-pack to generate corresponding .idx and .keep files.
> - The hash is used to determine the name of the temporary file and is
> - arbitrary. The output of index-pack is printed to stdout. Requires
> - --index-pack-args.
> + The hash is arbitrary. The output of index-pack is printed to stdout.
> + Requires --index-pack-args.
Do we even need to provide a hash anymore? After your patch I don't
think we even use it. It might be worth keeping around, though, as it
would be a unique key for de-duping or resuming, if we ever did
implement those on top.
> void release_http_pack_request(struct http_pack_request *preq)
> {
> - if (preq->packfile) {
> + if (preq->tempfile) {
> + delete_tempfile(&preq->tempfile);
> + preq->packfile = NULL;
> + } else if (preq->packfile) {
> fclose(preq->packfile);
> preq->packfile = NULL;
> }
OK. I think this is correct, though see my comments elsewhere in the
thread.
> @@ -2688,7 +2691,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
> int tmpfile_fd;
> int ret = 0;
>
> - fclose(preq->packfile);
> + if (preq->tempfile)
> + close_tempfile_gently(preq->tempfile);
> + else
> + fclose(preq->packfile);
> preq->packfile = NULL;
OK, and this is correct because preq->packfile is just an alias for
preq->tempfile.fp when the tempfile is valid. The NULL assignment is
important here so that the release() function doesn't double-free.
> -struct http_pack_request *new_http_pack_request(
> - const unsigned char *packed_git_hash, const char *base_url) {
> -
> - struct strbuf buf = STRBUF_INIT;
> -
> - end_url_with_slash(&buf, base_url);
> - strbuf_addf(&buf, "objects/pack/pack-%s.pack",
> - hash_to_hex(packed_git_hash));
> - return new_direct_http_pack_request(packed_git_hash,
> - strbuf_detach(&buf, NULL));
> -}
This hunk puzzled me at first, but it's because we used to just be a
wrapper for the "direct" variant, and now the two will share a single
static helper. That might have been a little more clear as a preparatory
patch, but OK.
> + if (resumable) {
> + odb_pack_name(the_repository, &preq->tmpfile,
> + packed_git_hash, "pack");
> + strbuf_addstr(&preq->tmpfile, ".temp");
> + preq->packfile = fopen(preq->tmpfile.buf, "a");
> + } else {
> + strbuf_addf(&preq->tmpfile, "%s/pack/tmp_pack_XXXXXX",
> + repo_get_object_directory(the_repository));
> + preq->tempfile = mks_tempfile_m(preq->tmpfile.buf, 0444);
> + if (preq->tempfile) {
> + strbuf_reset(&preq->tmpfile);
> + strbuf_addstr(&preq->tmpfile,
> + get_tempfile_path(preq->tempfile));
> + preq->packfile = fdopen_tempfile(preq->tempfile, "w");
> + }
> + }
> if (!preq->packfile) {
> error("Unable to open local file %s for pack",
> preq->tmpfile.buf);
OK, and this is the meat of the change. We usually use odb_mkstemp() for
tmp_pack_* files, but that annoyingly doesn't give you a tempfile
struct. So setting up your own filename and using mks_tempfile_m() makes
sense here.
The error path is a little funny, but we catch it in the context when
preq->packfile is NULL. Good.
> @@ -2766,8 +2776,9 @@ struct http_pack_request *new_direct_http_pack_request(
> * If there is data present from a previous transfer attempt,
> * resume where it left off
> */
> - prev_posn = ftello(preq->packfile);
> - if (prev_posn>0) {
> + if (resumable)
> + prev_posn = ftello(preq->packfile);
> + if (prev_posn > 0) {
I think this is not technically necessary, as ftello() would just return
"0" for our newly-created file. But it does make the intent clear.
> @@ -2779,12 +2790,28 @@ struct http_pack_request *new_direct_http_pack_request(
> return preq;
>
> abort:
> - strbuf_release(&preq->tmpfile);
> - free(preq->url);
> - free(preq);
> + release_http_pack_request(preq);
> return NULL;
> }
OK, now we have potentially more to free, so we rely on the release
function. That could cause problems if we jump to this abort label when
the struct isn't fully initialized. I think it is OK, though. We zero
the whole thing, so the extra fields that the release() function
considers will just be ignored.
> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>
> FILE *packfile;
> struct strbuf tmpfile;
> + struct tempfile *tempfile;
> struct active_request_slot *slot;
> struct curl_slist *headers;
Yuck, now we have "tempfile" and "tmpfile" with two different types and
totally different semantics (and even when "tempfile" is in use,
"tmpfile" is still meaningful!).
Can we even just call the second one non_resumable_tempfile or
something? It's a mouthful, but it makes it less likely to confuse the
two.
> + # Hold the first download before it is indexed, so that the second
> + # download installs the pack first.
> + {
> + (
> + if ! PATH="$TRASH_DIRECTORY:$PATH" \
> + GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
> + GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
> + git -C packfileclient-concurrent http-fetch \
> + --packfile="$packhash" \
> + --index-pack-arg=wait-index-pack \
> + --index-pack-arg=--stdin \
> + --index-pack-arg=--keep \
> + "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
> + then
> + echo failed >"$TRASH_DIRECTORY/first-ready" &&
> + exit 1
> + fi
> + ) &
> + first_pid=$!
> + } &&
OK. I wonder if it would be simpler and a more robust test if rather
than writing the correct bytes (and then waiting), the first process
just wrote total garbage. Then we'd be sure the other process is not
reading it, because it would definitely corrupt their input.
I dunno. This is a more realistic scenario, so in that sense maybe it is
more interesting.
> + test_when_finished "
> + echo continue >&9
> + wait $first_pid 2>/dev/null || :
> + exec 8>&-
> + exec 9>&-
> + rm -f first-ready first-continue git-wait-index-pack
> + " &&
> [...]
The rest of the fifo handling looks plausibly correct. This is a tricky
area and it's common to introduce funky races, but I didn't see anything
wrong, and it passed a few dozen rounds of --stress.
> @@ -313,7 +381,9 @@ test_expect_success 'http-fetch --packfile with corrupt pack' '
> git init packfileclient &&
> p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && ls objects/pack/pack-*.pack) &&
> test_must_fail git -C packfileclient http-fetch --packfile \
> - "$HTTPD_URL"/dumb/repo_bad1.git/$p
> + "$HTTPD_URL"/dumb/repo_bad1.git/$p &&
> + find packfileclient/.git/objects/pack -name "tmp_pack_*" -print >tmpfiles &&
> + test_must_be_empty tmpfiles
> '
OK, so here we just detect that we cleaned up after ourselves. Makes
sense.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Jeff King @ 2026-07-14 5:44 UTC (permalink / raw)
To: Taylor Blau
Cc: Ted Nyman, git, Junio C Hamano, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alW1tAnMtOznxrhK@com-79390>
On Mon, Jul 13, 2026 at 09:06:12PM -0700, Taylor Blau wrote:
> > void release_http_pack_request(struct http_pack_request *preq)
> > {
> > - if (preq->packfile) {
> > + if (preq->tempfile) {
> > + delete_tempfile(&preq->tempfile);
> > + preq->packfile = NULL;
>
> We should be able to drop the assignment to NULL on the second line,
> since `delete_tempfile()` takes a double pointer to the 'struct
> packfile' and NULL's it out for us.
>
> (The other callers appear to avoid explicitly setting `preq->tempfile`
> to NULL.)
It takes a double-pointer to the "struct tempfile"; the NULL assignment
is to the "packfile" member, which is the FILE handle.
I thought at first this was buggy; we still call fdopen() on the
tempfile and assign the result to preq->packfile, even in the new
non-resumable case. Don't we need to fclose() it? But the answer is no:
fdopen_tempfile() retains ownership of the result, storing it in
tempfile.fp. So it will be correctly closed during delete_tempfile(),
and in fact we must _not_ fclose it again.
But assigning NULL can happen with either style. So doing it
unconditionally like:
if (preq->tempfile)
delete_tempfile(&preq->tempfile);
else if (preq->packfile)
fclose(preq->packfile);
preq->packfile = NULL;
makes more sense, as it is done in finish_http_pack_request(). It might
even make sense to add a comment explaining why we don't need to
fclose() in the first part of the conditional.
All that said, I do not think setting it to NULL matters at all here,
since the function ends with free(preq). So just dropping the NULL would
perhaps be more clear.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`
From: Patrick Steinhardt @ 2026-07-14 5:36 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Justin Tobler, Junio C Hamano, Jeff King
In-Reply-To: <alW0KzSZuZnHmOZD@com-79390>
On Mon, Jul 13, 2026 at 08:59:39PM -0700, Taylor Blau wrote:
> On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:
> > Range-diff versus v2:
> >
> > 1: baf2adb012 = 1: 7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
> > 2: 57eecf3031 = 2: 2e5908c9c3 pack-bitmap: mark object filter as `const`
> > -: ---------- > 3: f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
> > 3: 92dd6a6f6e = 4: af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
> > 4: 92fe41577d = 5: 6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
> > 5: e5d59959e3 = 6: f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
> > 6: ab3547ac2b = 7: b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
> > 7: 026f21f522 = 8: a5bf309bec odb: introduce object filters to `odb_for_each_object()`
> > 8: 534b25c817 = 9: 600b15a907 builtin/cat-file: filter objects via object database
>
> Thanks, this version looks good to me.
Thanks for your review!
Patrick
^ permalink raw reply
* Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects
From: Patrick Steinhardt @ 2026-07-14 5:35 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <alWx1wj1bc48g11X@com-79390>
On Mon, Jul 13, 2026 at 08:49:43PM -0700, Taylor Blau wrote:
> On Mon, Jul 13, 2026 at 11:54:43AM +0200, Patrick Steinhardt wrote:
> > On Fri, Jul 10, 2026 at 03:25:10PM -0700, Taylor Blau wrote:
> > > On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> > > > Fix the issue by using `packed_object_info()` directly.
> > >
> > > What you wrote here makes sense to me insofar as I understand the
> > > pluggable ODB code.
> > >
> > > However, I am confused by the way this function is written in general.
> > > We use `bsearch_one_midx()` to locate the first possible MIDX position
> > > in which an object matching the given prefix may exist, which is
> > > sensible. However, we go from that position up to "num", where "num" is
> > > the total number of objects in the MIDX!
> > >
> > > Functionally this is not incorrect as we will happily discard objects
> > > that do not match the prefix. But it causes us to waste CPU cycles
> > > repeatedly calling `match_hash()` (at least for the first byte of the
> > > prefix) for objects that we know will match.
> >
> > That's not quite true though, as we abort iteration as soon as
> > `match_hash()` tells us that the prefix doesn't match anymore.
>
> Right, we neither iterate through more objects than necessary once we
> know that `match_hash()` will stop returning true, nor do we emit
> objects that don't actually match the prefix.
>
> What I was trying to say above is that in the special case where our
> prefix is a single byte long, we don't have to call `match_hash()` at
> *all*, since we can enumerate just the portion of the fanout for that
> specific byte, and we know that all such entries will match.
Oh, now that's what you're getting at. I don't think that this case ever
happens at all right now. I think the shortest prefix that we're ever
using should be at least 2 bytes, as we don't treat anything shorter
than 4 hex characters as an abbreviated object ID.
Thanks for clarifying!
Patrick
^ permalink raw reply
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Jeff King @ 2026-07-14 5:28 UTC (permalink / raw)
To: Ted Nyman
Cc: Junio C Hamano, git, Taylor Blau, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alWXwAGWgXSXoRJv@com-76773>
On Mon, Jul 13, 2026 at 06:58:24PM -0700, Ted Nyman wrote:
> > Are there better ways for these processes to coordinate with each
> > other? Instead of appending to the file, what if the second process
> > uses a predictable temporary name (which we already use) to open a
> > new file with O_CREAT | O_EXCL to avoid this redundant work?
>
> Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
> would prevent concurrent writes, but EEXIST alone would not
> distinguish an in-progress download from one left by an earlier
> failed or interrupted invocation. The existing .pack.temp name is not
> covered by the tmp_* pruning path, so simply waiting for it to
> disappear could leave a fetch stuck after a crash.
A few thoughts:
- Using O_EXCL makes this essentially a lockfile. So we could apply
the logic used elsewhere for lockfiles, like auto-removing files
with ancient mtimes. Or we could even go all-in with a pid check for
liveness; most of Git's lockfiles don't do that, but at least one
does (the background auto-gc lock).
- If we're not already using a name which is auto-cleaned during
maintenance, we probably ought to be. Leaving aside concurrency
issues, nobody would ever clean up the on-disk cruft.
But of course the original code here is intentionally _not_ using a
name we'd clean up, because it wants to be able to resume an
interrupted transfer. And you're explicitly breaking that for the
packfile URI case.
Is that a cost we're OK with paying? Fixing it opens up that same
coordination can of worms. You have to tell the difference a
concurrent writer and a previous dead one (whose work you can
resume).
It does feel weird that we'd do one thing for dumb-http and another
for packfile URIs. Wouldn't they suffer from the same concurrency
and resumption problems?
> The unique tempfile preserves the existing "download, index, then
> install" behavior for each invocation and fixes both the
> concurrent-append and EOF-resume failures. Avoiding the duplicate
> transfer would be useful for large packs, but I would prefer to keep
> that as a follow-up unless you think it is necessary for this
> correctness fix.
If we're OK with killing the ability to resume, then yeah, I think it
would make sense to start simple and un-break things. And then put a
coordination layer on top later (or never if nobody cares enough).
-Peff
^ permalink raw reply
* Re: [PATCH v8 0/5] history: add squash subcommand to fold a range
From: Matt Hunter @ 2026-07-14 4:44 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Harald Nordgren
In-Reply-To: <pull.2337.v8.git.git.1783674396.gitgitgadget@gmail.com>
On Fri Jul 10, 2026 at 5:06 AM EDT, Harald Nordgren via GitGitGadget wrote:
> Adds git history squash <revision-range> to fold a range of commits.
>
> Changes in v8:
>
> * --reedit-message now builds the same editor template as git rebase -i
> --autosquash: fixup!, squash! and amend! commits are grouped under the
> commit they target instead of shown in commit order, and an amend!
> replaces its target's message.
> * A fixup!, squash! or amend! is refused only when its target is outside
> the range, so several fixups for an in-range commit fold together. A
> range that is entirely markers for one below-range target is combined
> into a single commit, keeping the last amend! message.
> * Merges inside the range are folded when the range has a single base, with
> no dedicated opt-in flag, --ancestry-path ensures only commits descended
> from the base are folded, and a range reaching more than one base is
> rejected.
> * Rev-list options are accepted and sanitized the way git replay does,
> forcing the walk order back with a warning, which also fixes git history
> squash -- --reverse slipping past the previous option check.
> * Kept this as an explicit squash subcommand rather than making
> --reedit-message the default or renaming the command.
This feature looks like it's coming together pretty well imo. I just have
one observation I want to comment on:
I noticed that 'git history squash <range>', when --reedit-message is
omitted, will ignore any amend! message in the range that targets the
first folded commit.
On the surface, this makes sense. The feature is pretty explicit that
it will faithfully stick with the first commit's message, unless
modified by use of --reedit-message.
However, this edge case is a little surprising, given that
'git history squash' seems to be aware of the semantics of fixup!, amend!,
and squash! messages whether --reedit-message was given or not. For instance,
the default command notices when the range contains a squash! commit whose
target is elsewhere (a useful feature). It seems consistent then, that the
default command would incorporate an amend! it is aware of when placing the
"first commit's" message in the resulting squash. This seems useful to me
as well.
At the same time, I can understand why the current implementation does
what it does. So I'm not entirely sure what the correct answer is here.
I'll mention as well that I really like the decisions made for how this
command handles squashing a bunch of related fixups. This "fixup
consolidation" is a use-case that this command may steal away from rebase
for me. And the way a final amend! is handled in this case is what got me
thinking about it in the general case.
Thanks for the work on this topic!
^ permalink raw reply
* Re: [PATCH 0/2] packfile URIs: support concurrent downloads
From: Taylor Blau @ 2026-07-14 4:13 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover.1783982021.git.tnyman@openai.com>
On Mon, Jul 13, 2026 at 03:37:58PM -0700, Ted Nyman wrote:
> Ted Nyman (2):
> http: use unique tempfiles for packfile URI downloads
> fetch-pack: accept "pack" output for packfile URIs
I left one pretty minor style-nit on the first patch, but otherwise this
looks good to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Taylor Blau @ 2026-07-14 4:07 UTC (permalink / raw)
To: Ted Nyman
Cc: Junio C Hamano, git, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alWXwAGWgXSXoRJv@com-76773>
On Mon, Jul 13, 2026 at 06:58:24PM -0700, Ted Nyman wrote:
> > While that does sound like a safe and correct approach, stepping
> > back briefly, would it not be wasteful for the second process to
> > download the same packfile that the first has already started
> > downloading?
>
> Yes. If two fetches overlap, the second download is redundant.
>
> > Are there better ways for these processes to coordinate with each
> > other? Instead of appending to the file, what if the second process
> > uses a predictable temporary name (which we already use) to open a
> > new file with O_CREAT | O_EXCL to avoid this redundant work?
>
> Using the existing pack-<hash>.pack.temp name with O_CREAT | O_EXCL
> would prevent concurrent writes, but EEXIST alone would not
> distinguish an in-progress download from one left by an earlier
> failed or interrupted invocation. The existing .pack.temp name is not
> covered by the tmp_* pruning path, so simply waiting for it to
> disappear could leave a fetch stuck after a crash.
Exactly. If two processes are downloading the same pack at the same time
to different locations, the effort is of course redundant. But I don't
think we can reliably distinguish between that case and one where an
earlier process died in the middle of downloading a pack but was unable
to clean up after itself.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Taylor Blau @ 2026-07-14 4:06 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Junio C Hamano, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <alVn-QmK3K91_tkH@com-76773>
On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:
> Since 8d5d2a34df (http-fetch: support fetching packfiles by URL,
> 2020-06-10), packfile URI downloads have been staged at
> objects/pack/pack-<hash>.pack.temp.
>
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
>
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.
>
> Add a test which pauses one process after downloading the pack and
> starts another process using the same object database.
>
> Signed-off-by: Ted Nyman <tnyman@openai.com>
> ---
> Documentation/git-http-fetch.adoc | 5 +-
> http.c | 77 +++++++++++++++++++++----------
> http.h | 1 +
> t/t5550-http-fetch-dumb.sh | 72 ++++++++++++++++++++++++++++-
> 4 files changed, 126 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
> line (which is not expected in
> this case), 'git http-fetch' fetches the packfile directly at the given
> URL and uses index-pack to generate corresponding .idx and .keep files.
> - The hash is used to determine the name of the temporary file and is
> - arbitrary. The output of index-pack is printed to stdout. Requires
> - --index-pack-args.
> + The hash is arbitrary. The output of index-pack is printed to stdout.
> + Requires --index-pack-args.
>
> --index-pack-args=<args>::
> For internal use only. The command to run on the contents of the
> diff --git a/http.c b/http.c
> index b4e7b8d00b..5a46e7c65c 100644
> --- a/http.c
> +++ b/http.c
> @@ -2668,7 +2668,10 @@ int http_get_info_packs(const char *base_url, struct packfile_list *packs)
>
> void release_http_pack_request(struct http_pack_request *preq)
> {
> - if (preq->packfile) {
> + if (preq->tempfile) {
> + delete_tempfile(&preq->tempfile);
> + preq->packfile = NULL;
We should be able to drop the assignment to NULL on the second line,
since `delete_tempfile()` takes a double pointer to the 'struct
packfile' and NULL's it out for us.
(The other callers appear to avoid explicitly setting `preq->tempfile`
to NULL.)
The rest of the patch looks good to me.
> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>
> FILE *packfile;
> struct strbuf tmpfile;
> + struct tempfile *tempfile;
> struct active_request_slot *slot;
> struct curl_slist *headers;
> };
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index b0080bf204..314a74c433 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -293,6 +293,74 @@ test_expect_success 'http-fetch --packfile' '
> git -C packfileclient cat-file -e "$HASH"
> '
>
> +test_expect_success PIPE 'concurrent http-fetch --packfile' '
Phew ;-).
This is definitely tricky to test, but what you wrote here looks
plausibly correct to me.
Thanks,
Taylor
^ permalink raw reply
* local mistake - need help in recover
From: N.S Kishore @ 2026-07-14 4:04 UTC (permalink / raw)
To: git@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 86 bytes --]
Hi Team,
Need help to recover files from local mistake.
Regards,
Kishore N S.
[-- Attachment #1.2: Type: text/html, Size: 1163 bytes --]
[-- Attachment #2: git-bugreport-2026-07-14-0922.txt --]
[-- Type: text/plain, Size: 3839 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
1. Working on a feature branch with several documentation files.
2. HEAD contained only a subset of those files (already committed earlier).
3. Created additional files locally and edited existing ones.
4. Staged the new and modified files with `git add` from the IDE (VS Code / Cursor Git extension), over a period of roughly 20–30 minutes.
5. Did NOT commit before doing other git operations.
6. Later ran `git merge <remote-branch>` into the feature branch. Reflog also shows a `reset: moving to HEAD` shortly before merge / branch activity.
7. Noticed that previously staged files were no longer in the index and were not in any commit.
What did you expect to happen? (Expected behavior)
- Staged changes should remain in the index until explicitly unstaged, committed, or discarded.
- `git merge` should not silently drop unrelated staged-but-uncommitted work.
- Staged files should still appear under "Changes to be committed" after the merge.
- The IDE should continue to show staged files and allow commit.
What happened instead? (Actual behavior)
- After merge (and/or related operations in the same session), staged files disappeared from the index.
- `git status` no longer listed the files as staged.
- New files that had been `git add`ed were untracked again or missing from staging.
- The IDE logged warnings like "File not found" when comparing against HEAD, because those paths only existed in the index, not in any commit.
- At one point, index entries for staged files suddenly disappeared (visible in IDE Git logs).
- File content was not fully lost: blobs were recoverable via `git fsck --lost-found` and `.git/lost-found/other/`, but manual recovery was required (copy blobs, re-apply edits, recommit).
What's different between what you expected and what actually happened?
Expected: staging is a safe holding area until commit; merge should not wipe it.
Actual: staging was cleared without commit; IDE showed confusing errors; work had to be reconstructed from lost-found blobs.
The merge only touched unrelated paths (no content conflict on the staged files), yet staged work still vanished from the index.
Anything else you want to add:
Editor: Cursor (VS Code-based), Git extension used for staging.
Rough timeline:
- Staged multiple files over ~20–30 minutes
- Index entries disappeared shortly after last `git add`
- `git reset` and `git merge` occurred in the same session
- Recovery required copying from lost-found blobs and recommitting
Suggested reproduction:
1. Stage several NEW untracked files (not in HEAD) with `git add`.
2. Do NOT commit.
3. Run `git merge <other-branch>` or `git reset` / branch checkout in the same repo.
4. Check whether the index still contains the staged new files.
Impact: time lost reconstructing work; risk of data loss if blobs had been garbage-collected.
Workaround: `git fsck --lost-found`, `git show <blob>`, copy from `.git/lost-found/other/`, recommit.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.54.0
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
feature: fsmonitor--daemon
gettext: enabled
libcurl: 8.7.1
zlib: 1.2.12
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Darwin 25.5.0 Darwin Kernel Version 25.5.0: Tue Jun 9 22:28:34 PDT 2026; root:xnu-12377.121.10~1/RELEASE_ARM64_T6041 arm64
compiler info: clang: 21.0.0 (clang-2100.0.123.102)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh
[Enabled Hooks]
^ permalink raw reply
* Re: [PATCH v3 0/9] odb: introduce object filters to `odb_for_each_object()`
From: Taylor Blau @ 2026-07-14 3:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano, Jeff King
In-Reply-To: <20260713-pks-odb-for-each-object-filter-v3-0-b3c65c641073@pks.im>
On Mon, Jul 13, 2026 at 04:41:24PM +0200, Patrick Steinhardt wrote:
> Range-diff versus v2:
>
> 1: baf2adb012 = 1: 7c0dc1be0d odb/source-packed: improve lookup when enumerating objects
> 2: 57eecf3031 = 2: 2e5908c9c3 pack-bitmap: mark object filter as `const`
> -: ---------- > 3: f4d66ccfc6 pack-objects: drop unused return value from add_object_entry()
> 3: 92dd6a6f6e = 4: af475654b8 pack-bitmap: allow aborting iteration of bitmapped objects
> 4: 92fe41577d = 5: 6ca42587c9 pack-bitmap: iterate object sources when opening bitmaps
> 5: e5d59959e3 = 6: f62c3bbc81 pack-bitmap: drop `_1` suffix from functions that open bitmaps
> 6: ab3547ac2b = 7: b2d25b6e9b pack-bitmap: introduce function to open bitmap for a single source
> 7: 026f21f522 = 8: a5bf309bec odb: introduce object filters to `odb_for_each_object()`
> 8: 534b25c817 = 9: 600b15a907 builtin/cat-file: filter objects via object database
Thanks, this version looks good to me.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 3/8] pack-bitmap: allow aborting iteration of bitmapped objects
From: Taylor Blau @ 2026-07-14 3:58 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git, Justin Tobler, Junio C Hamano
In-Reply-To: <alS1rus9thie7NiE@pks.im>
On Mon, Jul 13, 2026 at 11:53:50AM +0200, Patrick Steinhardt wrote:
> On Sat, Jul 11, 2026 at 04:01:14AM -0400, Jeff King wrote:
> > On Fri, Jul 10, 2026 at 03:34:53PM -0700, Taylor Blau wrote:
> >
> > > However, the remaining `show_objects_for_type()` callers from within
> > > `traverse_bitmap_commit_list()` do *not* bother to inspect the return
> > > value, despite taking in an arbitrary 'show_reachable_fn', which itself
> > > may return a non-zero value.
> > >
> > > I guess this must be effectively OK in practice with respect to the
> > > existing code for the same reason you indicate in the commit message
> > > above, but we should change this function to *also* propagate non-zero
> > > return values to eliminate the foot-gun completely.
> >
> > The matching non-bitmap traverse_commit_list() does not allow aborting
> > based on callback returns, either. In fact, its callbacks return void!
> >
> > Whichever direction we go, those two should probably stay in sync (so
> > either both should allow aborting early with a non-zero return, or both
> > should return void).
>
> That's fair. But adapting `traverse_commit_list()` requires tons of
> changes all over the tree, so I'm inclined to rather leave both
> `traverse_bitmap_commit_list()` and `traverse_commit_list()` as-is.
> Does that work for both of you?
I think that it's fine to leave it as-is for the purpose of this series,
though I would like to address it.
I don't think we need to adapt `traverse_commit_list()`, though. We can
go in the other direction Peff suggested, which would be to split the
callback type used by `for_each_bitmapped_object()` from
`show_reachable_fn`, keep the former abortable, and make the latter
return void.
That keeps `traverse_bitmap_commit_list()` in sync with
`traverse_commit_list()` without changing the non-bitmap traversal
machinery. I have a small two-patch follow-up on top of v3 that does
this, which I'll send separately.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2 1/8] odb/source-packed: improve lookup when enumerating objects
From: Taylor Blau @ 2026-07-14 3:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Justin Tobler, Junio C Hamano
In-Reply-To: <alS1440iifvTvGKP@pks.im>
On Mon, Jul 13, 2026 at 11:54:43AM +0200, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 03:25:10PM -0700, Taylor Blau wrote:
> > On Fri, Jul 10, 2026 at 10:48:53AM +0200, Patrick Steinhardt wrote:
> > > Fix the issue by using `packed_object_info()` directly.
> >
> > What you wrote here makes sense to me insofar as I understand the
> > pluggable ODB code.
> >
> > However, I am confused by the way this function is written in general.
> > We use `bsearch_one_midx()` to locate the first possible MIDX position
> > in which an object matching the given prefix may exist, which is
> > sensible. However, we go from that position up to "num", where "num" is
> > the total number of objects in the MIDX!
> >
> > Functionally this is not incorrect as we will happily discard objects
> > that do not match the prefix. But it causes us to waste CPU cycles
> > repeatedly calling `match_hash()` (at least for the first byte of the
> > prefix) for objects that we know will match.
>
> That's not quite true though, as we abort iteration as soon as
> `match_hash()` tells us that the prefix doesn't match anymore.
Right, we neither iterate through more objects than necessary once we
know that `match_hash()` will stop returning true, nor do we emit
objects that don't actually match the prefix.
What I was trying to say above is that in the special case where our
prefix is a single byte long, we don't have to call `match_hash()` at
*all*, since we can enumerate just the portion of the fanout for that
specific byte, and we know that all such entries will match.
> Or do you mean that `num` should only be `m->num_objects` instead of
> also iterating through `num_objects_in_base`? I have to admit that I'm
> alwas struggling with the chained MIDX. It's never quite clear to me
> whether a given function cares about the complete chain or whether it
> really only cares about a single MIDX.
If the goal is to yield all such objects that match the prefix, then we
need to enumerate each layer. The analogy that I have had in my head
while working on these is that they are the same conceptually as the
incremental commit-graph format.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers
From: Taylor Blau @ 2026-07-14 3:31 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Kristofer Karlsson, ', Taylor Blau,
Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <alSCv5I94qjbSucQ@pks.im>
On Mon, Jul 13, 2026 at 08:16:31AM +0200, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 03:14:28PM -0700, Taylor Blau wrote:
> > On Tue, Jul 07, 2026 at 04:57:13PM +0200, Kristofer Karlsson wrote:
> > > (b) Move topo_levels to struct object_database. Since
> > > fill_commit_graph_info() can already reach the odb via
> > > g->odb_source->odb, no signature changes are needed.
> > > The write side becomes a single assignment:
> > >
> > > ctx.r->objects->topo_levels = &topo_levels;
> > >
> > > and cleanup becomes:
> > >
> > > ctx.r->objects->topo_levels = NULL;
> > >
> > > No chain walk needed and the diff is fairly small.
> > > I am not sure about the semantics of it though -- should the odb
> > > have a reference to topo_levels?
> >
> > This seems to be the most promising approach, though I'd be curious what
> > Patrick's thoughts are. The commit-slab API is really a property of the
> > object database, but we treat these as a global as I do not recall them
> > yet being touched by the ODB refactoring effort.
>
> I was investigating several times whether we can remove them from global
> scope and move them into the object database indeed. The answer is that
> it's somewhat complicated because we reuse the slab for multiple
> different things, and detangling that has proven to be a bit of a mess.
It's an interesting question, and I think worth discussing, though note
that I would also like to ensure that we resolve this in the short-term
to prevent any future regression while the pluggable ODB refactor
continues on.
> The other question here is whether commit graphs really are a property
> of the object database itself, or whether they are rather a property of
> a given backend. Sure, we can only have a single commit graph at any
> point in time, so they feel like they are at the object database level.
> But is the current implementation of a commit graph really the best for
> all potential backends out there?
>
> If you take for example a distributed backend to store objects, then you
> probably don't want to have a single local commit graph that is stored
> in ".git/objects/info". Furthermore, the current format may not even be
> the best one to store the cached information, either.
I think I agree here in part, though I think there is some subtlety that
is specific to commit-graphs.
If I understand your argument correctly, I think that I am on-board with
it if you substitute "commit-graph" with "MIDX" or "reachability
bitmaps", as those are optimizations over a specific representation of
the object store.
The commit-graph is somewhat of an oddity in that regard. While it is
partially an optimization in the representation format, it is also a
data-structure which is useful independent of the underlying storage. On
the former, I absolutely agree with what you're saying: having a
row-oriented layout to optimize commit traversals may not be necessary
in a different implementation of the object store which has efficient
enough access to the commit objects so as to make the row-oriented
layout unnecessary.
However, it is a useful question to ask "what is the generation number
of this commit?" independently of whether we store the commit objects
themselves in the existing ODB, in a generic blob storage system, or
something else entirely.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v12 10/10] repository: adjust the comment of config_values_private_
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The configurations in 'struct config_values_private_' are not all
parsed in 'git_default_config()'. For example, 'pager_program' is
now parsed in 'pager.c'. Therefore, update the comment.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
repository.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/repository.h b/repository.h
index 36e2db2633..9093e6af93 100644
--- a/repository.h
+++ b/repository.h
@@ -152,7 +152,7 @@ struct repository {
/* Repository's compatibility hash algorithm. */
const struct git_hash_algo *compat_hash_algo;
- /* Repository's config values parsed by git_default_config() */
+ /* Repository-specific configuration values. */
struct repo_config_values config_values_private_;
/* Repository's reference storage format, as serialized on disk. */
--
2.43.0
^ permalink raw reply related
* [PATCH v12 09/10] environment: move object_creation_mode into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'object_creation_mode' controls how Git creates
object files, specifically determining whether to use hardlinks or
renames when moving temporary files into the object database. Move
it into 'struct repo_config_values' to continue the libification
effort.
Move the 'enum object_creation_mode' definition higher up in
'environment.h' to ensure it is visible to the structure. Initialize
the per-repository value to its default macro value
OBJECT_CREATION_MODE inside 'repo_config_values_init()'.
Update configuration parsing in 'git_default_core_config()' to write
directly to the repository-specific configuration structure.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 6 +++---
environment.h | 12 ++++++------
object-file.c | 3 ++-
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/environment.c b/environment.c
index c0bf7577b7..ef3c032e0c 100644
--- a/environment.c
+++ b/environment.c
@@ -60,7 +60,6 @@ char *check_roundtrip_encoding;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
-enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
unsigned long pack_size_limit_cfg;
@@ -512,9 +511,9 @@ int git_default_core_config(const char *var, const char *value,
if (!value)
return config_error_nonbool(var);
if (!strcmp(value, "rename"))
- object_creation_mode = OBJECT_CREATION_USES_RENAMES;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_RENAMES;
else if (!strcmp(value, "link"))
- object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
die(_("invalid mode for object creation: %s"), value);
return 0;
@@ -728,6 +727,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
cfg->autorebase = AUTOREBASE_NEVER;
+ cfg->object_creation_mode = OBJECT_CREATION_MODE;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 46b2f0d861..a47a5c83db 100644
--- a/environment.h
+++ b/environment.h
@@ -109,6 +109,11 @@ enum rebase_setup_type {
AUTOREBASE_ALWAYS
};
+enum object_creation_mode {
+ OBJECT_CREATION_USES_HARDLINKS = 0,
+ OBJECT_CREATION_USES_RENAMES = 1
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -120,6 +125,7 @@ struct repo_config_values {
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
enum rebase_setup_type autorebase;
+ enum object_creation_mode object_creation_mode;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -213,12 +219,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum object_creation_mode {
- OBJECT_CREATION_USES_HARDLINKS = 0,
- OBJECT_CREATION_USES_RENAMES = 1
-};
-extern enum object_creation_mode object_creation_mode;
-
extern int grafts_keep_true_parents;
const char *get_log_output_encoding(void);
diff --git a/object-file.c b/object-file.c
index 9afa842da2..c00dd3afca 100644
--- a/object-file.c
+++ b/object-file.c
@@ -411,11 +411,12 @@ int finalize_object_file_flags(struct repository *repo,
{
unsigned retries = 0;
int ret;
+ struct repo_config_values *cfg = repo_config_values(repo);
retry:
ret = 0;
- if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
+ if (cfg->object_creation_mode == OBJECT_CREATION_USES_RENAMES)
goto try_rename;
else if (link(tmpfile, filename))
ret = errno;
--
2.43.0
^ permalink raw reply related
* [PATCH v12 08/10] environment: move autorebase into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'autorebase' dictates whether a newly created
branch should be configured to automatically rebase by default.
Move it into 'struct repo_config_values' to continue the
libification effort.
The 'enum rebase_setup_type' definition is moved higher up in
'environment.h' so that it is visible to the repository-specific
structure. The default state AUTOREBASE_NEVER is now correctly
initialized in 'repo_config_values_init()'.
Configuration parsing in 'git_default_branch_config()' is updated to
write directly to the repository's configuration instance.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
branch.c | 2 +-
environment.c | 10 +++++-----
environment.h | 16 ++++++++--------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 243db7d0fc..e1c1f8c89d 100644
--- a/branch.c
+++ b/branch.c
@@ -61,7 +61,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
static int should_setup_rebase(const char *origin)
{
- switch (autorebase) {
+ switch (repo_config_values(the_repository)->autorebase) {
case AUTOREBASE_NEVER:
return 0;
case AUTOREBASE_LOCAL:
diff --git a/environment.c b/environment.c
index 66c1ac1ab8..c0bf7577b7 100644
--- a/environment.c
+++ b/environment.c
@@ -57,7 +57,6 @@ enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
char *check_roundtrip_encoding;
-enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
@@ -601,13 +600,13 @@ static int git_default_branch_config(const char *var, const char *value)
if (!value)
return config_error_nonbool(var);
else if (!strcmp(value, "never"))
- autorebase = AUTOREBASE_NEVER;
+ cfg->autorebase = AUTOREBASE_NEVER;
else if (!strcmp(value, "local"))
- autorebase = AUTOREBASE_LOCAL;
+ cfg->autorebase = AUTOREBASE_LOCAL;
else if (!strcmp(value, "remote"))
- autorebase = AUTOREBASE_REMOTE;
+ cfg->autorebase = AUTOREBASE_REMOTE;
else if (!strcmp(value, "always"))
- autorebase = AUTOREBASE_ALWAYS;
+ cfg->autorebase = AUTOREBASE_ALWAYS;
else
return error(_("malformed value for %s"), var);
return 0;
@@ -728,6 +727,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_whitespace = NULL;
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
+ cfg->autorebase = AUTOREBASE_NEVER;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 17a3a628d2..46b2f0d861 100644
--- a/environment.h
+++ b/environment.h
@@ -102,6 +102,13 @@ enum push_default_type {
PUSH_DEFAULT_UNSPECIFIED
};
+enum rebase_setup_type {
+ AUTOREBASE_NEVER = 0,
+ AUTOREBASE_LOCAL,
+ AUTOREBASE_REMOTE,
+ AUTOREBASE_ALWAYS
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -112,6 +119,7 @@ struct repo_config_values {
char *apply_default_whitespace;
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
+ enum rebase_setup_type autorebase;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -205,14 +213,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum rebase_setup_type {
- AUTOREBASE_NEVER = 0,
- AUTOREBASE_LOCAL,
- AUTOREBASE_REMOTE,
- AUTOREBASE_ALWAYS
-};
-extern enum rebase_setup_type autorebase;
-
enum object_creation_mode {
OBJECT_CREATION_USES_HARDLINKS = 0,
OBJECT_CREATION_USES_RENAMES = 1
--
2.43.0
^ permalink raw reply related
* [PATCH v12 07/10] environment: move push_default into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'push_default' specifies the default behavior of
'git push' when no explicit refspec is provided. Move 'push_default'
into 'struct repo_config_values' to continue the libification effort.
While 'enum push_default_type' ideally belongs in 'remote.h', moving it
there introduces a circular dependency chain:
remote.h -> hash.h -> repository.h -> environment.h.
Therefore, the enum definition is kept in 'environment.h' just above
'struct repo_config_values' with a NEEDSWORK comment for future cleanup.
Modify the configuration parsing in environment.c to update the
per-repository structure directly, and update caller across the
codebase to access the value via 'repo_config_values()'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
builtin/push.c | 10 ++++++----
environment.c | 16 +++++++++-------
environment.h | 26 ++++++++++++++++----------
remote.c | 2 +-
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 6021b71d66..7578ff38c4 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -73,6 +73,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
struct remote *remote, struct ref *matched)
{
const char *branch_name;
+ struct repo_config_values *cfg = repo_config_values(the_repository);
if (remote->push.nr) {
struct refspec_item query = {
@@ -88,7 +89,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
}
}
- if (push_default == PUSH_DEFAULT_UPSTREAM &&
+ if (cfg->push_default == PUSH_DEFAULT_UPSTREAM &&
skip_prefix(matched->name, "refs/heads/", &branch_name)) {
struct branch *branch = branch_get(branch_name);
if (branch->merge_nr == 1 && branch->merge[0]->src) {
@@ -160,7 +161,7 @@ static NORETURN void die_push_simple(struct branch *branch,
* Don't show advice for people who explicitly set
* push.default.
*/
- if (push_default == PUSH_DEFAULT_UNSPECIFIED)
+ if (cfg->push_default == PUSH_DEFAULT_UNSPECIFIED)
advice_pushdefault_maybe = _("\n"
"To choose either option permanently, "
"see push.default in 'git help config'.\n");
@@ -231,8 +232,9 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
struct branch *branch;
const char *dst;
int same_remote;
+ struct repo_config_values *cfg = repo_config_values(the_repository);
- switch (push_default) {
+ switch (cfg->push_default) {
case PUSH_DEFAULT_MATCHING:
refspec_append(&rs, ":");
return;
@@ -252,7 +254,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
dst = branch->refname;
same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
- switch (push_default) {
+ switch (cfg->push_default) {
default:
case PUSH_DEFAULT_UNSPECIFIED:
case PUSH_DEFAULT_SIMPLE:
diff --git a/environment.c b/environment.c
index 20500658a2..66c1ac1ab8 100644
--- a/environment.c
+++ b/environment.c
@@ -58,7 +58,6 @@ enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
char *check_roundtrip_encoding;
enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
-enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
@@ -620,21 +619,23 @@ static int git_default_branch_config(const char *var, const char *value)
static int git_default_push_config(const char *var, const char *value)
{
+ struct repo_config_values *cfg = repo_config_values(the_repository);
+
if (!strcmp(var, "push.default")) {
if (!value)
return config_error_nonbool(var);
else if (!strcmp(value, "nothing"))
- push_default = PUSH_DEFAULT_NOTHING;
+ cfg->push_default = PUSH_DEFAULT_NOTHING;
else if (!strcmp(value, "matching"))
- push_default = PUSH_DEFAULT_MATCHING;
+ cfg->push_default = PUSH_DEFAULT_MATCHING;
else if (!strcmp(value, "simple"))
- push_default = PUSH_DEFAULT_SIMPLE;
+ cfg->push_default = PUSH_DEFAULT_SIMPLE;
else if (!strcmp(value, "upstream"))
- push_default = PUSH_DEFAULT_UPSTREAM;
+ cfg->push_default = PUSH_DEFAULT_UPSTREAM;
else if (!strcmp(value, "tracking")) /* deprecated */
- push_default = PUSH_DEFAULT_UPSTREAM;
+ cfg->push_default = PUSH_DEFAULT_UPSTREAM;
else if (!strcmp(value, "current"))
- push_default = PUSH_DEFAULT_CURRENT;
+ cfg->push_default = PUSH_DEFAULT_CURRENT;
else {
error(_("malformed value for %s: %s"), var, value);
return error(_("must be one of nothing, matching, simple, "
@@ -726,6 +727,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->askpass_program = NULL;
cfg->apply_default_whitespace = NULL;
cfg->apply_default_ignorewhitespace = NULL;
+ cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index f450242ac0..17a3a628d2 100644
--- a/environment.h
+++ b/environment.h
@@ -87,6 +87,21 @@ extern const char * const local_repo_env[];
struct strvec;
struct repository;
+
+/*
+ * NEEDSWORK: It would be better if these definitions could be moved to
+ * other more specific files, but care is needed to avoid circular
+ * inclusion issues.
+ */
+enum push_default_type {
+ PUSH_DEFAULT_NOTHING = 0,
+ PUSH_DEFAULT_MATCHING,
+ PUSH_DEFAULT_SIMPLE,
+ PUSH_DEFAULT_UPSTREAM,
+ PUSH_DEFAULT_CURRENT,
+ PUSH_DEFAULT_UNSPECIFIED
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -96,6 +111,7 @@ struct repo_config_values {
char *askpass_program;
char *apply_default_whitespace;
char *apply_default_ignorewhitespace;
+ enum push_default_type push_default;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -197,16 +213,6 @@ enum rebase_setup_type {
};
extern enum rebase_setup_type autorebase;
-enum push_default_type {
- PUSH_DEFAULT_NOTHING = 0,
- PUSH_DEFAULT_MATCHING,
- PUSH_DEFAULT_SIMPLE,
- PUSH_DEFAULT_UPSTREAM,
- PUSH_DEFAULT_CURRENT,
- PUSH_DEFAULT_UNSPECIFIED
-};
-extern enum push_default_type push_default;
-
enum object_creation_mode {
OBJECT_CREATION_USES_HARDLINKS = 0,
OBJECT_CREATION_USES_RENAMES = 1
diff --git a/remote.c b/remote.c
index 00723b385e..d48c01d375 100644
--- a/remote.c
+++ b/remote.c
@@ -1933,7 +1933,7 @@ static char *branch_get_push_1(struct repository *repo,
if (remote->mirror)
return tracking_for_push_dest(remote, branch->refname, err);
- switch (push_default) {
+ switch (repo_config_values(repo)->push_default) {
case PUSH_DEFAULT_NOTHING:
return error_buf(err, _("push has no destination (push.default is 'nothing')"));
--
2.43.0
^ permalink raw reply related
* [PATCH v12 06/10] environment: migrate apply_default_whitespace and apply_default_ignorewhitespace
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variables 'apply_default_whitespace' and
'apply_default_ignorewhitespace' are used to store the default
whitespace configuration for 'git apply'. Move these variables
into 'struct repo_config_values' to continue the libification
effort.
Dynamically allocated strings fetched via 'repo_config_get_string()'
are now tracked per-repository and safely freed in
'repo_config_values_clear()'.
As part of this transition, update 'git_apply_config()' to accept a
'struct repository *' argument rather than relying on the
'the_repository' global.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
apply.c | 28 ++++++++++++++++++++--------
environment.c | 6 ++++--
environment.h | 4 ++--
3 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/apply.c b/apply.c
index 249248d4f2..b1db1fe495 100644
--- a/apply.c
+++ b/apply.c
@@ -47,11 +47,17 @@ struct gitdiff_data {
int p_value;
};
-static void git_apply_config(void)
+static void git_apply_config(struct repository *repo)
{
- repo_config_get_string(the_repository, "apply.whitespace", &apply_default_whitespace);
- repo_config_get_string(the_repository, "apply.ignorewhitespace", &apply_default_ignorewhitespace);
- repo_config(the_repository, git_xmerge_config, NULL);
+ struct repo_config_values *cfg = repo_config_values(repo);
+
+ FREE_AND_NULL(cfg->apply_default_whitespace);
+ repo_config_get_string(repo, "apply.whitespace",
+ &cfg->apply_default_whitespace);
+ FREE_AND_NULL(cfg->apply_default_ignorewhitespace);
+ repo_config_get_string(repo, "apply.ignorewhitespace",
+ &cfg->apply_default_ignorewhitespace);
+ repo_config(repo, git_xmerge_config, NULL);
}
static int parse_whitespace_option(struct apply_state *state, const char *option)
@@ -109,6 +115,8 @@ int init_apply_state(struct apply_state *state,
struct repository *repo,
const char *prefix)
{
+ struct repo_config_values *cfg = repo_config_values(repo);
+
memset(state, 0, sizeof(*state));
state->prefix = prefix;
state->repo = repo;
@@ -126,10 +134,13 @@ int init_apply_state(struct apply_state *state,
strset_init(&state->kept_symlinks);
strbuf_init(&state->root, 0);
- git_apply_config();
- if (apply_default_whitespace && parse_whitespace_option(state, apply_default_whitespace))
+ git_apply_config(repo);
+
+ if (cfg->apply_default_whitespace &&
+ parse_whitespace_option(state, cfg->apply_default_whitespace))
return -1;
- if (apply_default_ignorewhitespace && parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+ if (cfg->apply_default_ignorewhitespace &&
+ parse_ignorewhitespace_option(state, cfg->apply_default_ignorewhitespace))
return -1;
return 0;
}
@@ -192,7 +203,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
static void set_default_whitespace_mode(struct apply_state *state)
{
- if (!state->whitespace_option && !apply_default_whitespace)
+ if (!state->whitespace_option &&
+ !repo_config_values(state->repo)->apply_default_whitespace)
state->ws_error_action = (state->apply ? warn_on_ws_error : nowarn_ws_error);
}
diff --git a/environment.c b/environment.c
index 3857818da3..20500658a2 100644
--- a/environment.c
+++ b/environment.c
@@ -49,8 +49,6 @@ int assume_unchanged;
int is_bare_repository_cfg = -1; /* unspecified */
char *git_commit_encoding;
char *git_log_output_encoding;
-char *apply_default_whitespace;
-char *apply_default_ignorewhitespace;
int fsync_object_files = -1;
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
@@ -726,6 +724,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->editor_program = NULL;
cfg->pager_program = NULL;
cfg->askpass_program = NULL;
+ cfg->apply_default_whitespace = NULL;
+ cfg->apply_default_ignorewhitespace = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -745,4 +745,6 @@ void repo_config_values_clear(struct repo_config_values *cfg)
FREE_AND_NULL(cfg->editor_program);
FREE_AND_NULL(cfg->pager_program);
FREE_AND_NULL(cfg->askpass_program);
+ FREE_AND_NULL(cfg->apply_default_whitespace);
+ FREE_AND_NULL(cfg->apply_default_ignorewhitespace);
}
diff --git a/environment.h b/environment.h
index 856dc70cc4..f450242ac0 100644
--- a/environment.h
+++ b/environment.h
@@ -94,6 +94,8 @@ struct repo_config_values {
char *editor_program;
char *pager_program;
char *askpass_program;
+ char *apply_default_whitespace;
+ char *apply_default_ignorewhitespace;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -182,8 +184,6 @@ extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
extern int ignore_case;
extern int assume_unchanged;
-extern char *apply_default_whitespace;
-extern char *apply_default_ignorewhitespace;
extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
--
2.43.0
^ permalink raw reply related
* [PATCH v12 05/10] environment: move askpass_program into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'askpass_program' stores the path to the program
used to prompt the user for credentials. Move it into repo_config_values
to continue the libification effort.
While it is uncommon for a single process to require different askpass
programs for different repositories, maintaining this value as a mutable
global string is a blocker for libification. Global heap-allocated
strings introduce thread-safety issues in a multi-repo environment.
Move 'askpass_program' into 'struct repo_config_values' to eliminate
this global state. The memory is now safely managed and freed via
'repo_config_values_clear()'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 7 ++++---
environment.h | 3 +--
prompt.c | 3 ++-
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/environment.c b/environment.c
index 975c9cb9eb..3857818da3 100644
--- a/environment.c
+++ b/environment.c
@@ -55,7 +55,6 @@ int fsync_object_files = -1;
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
-char *askpass_program;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
@@ -464,8 +463,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.askpass")) {
- FREE_AND_NULL(askpass_program);
- return git_config_string(&askpass_program, var, value);
+ FREE_AND_NULL(cfg->askpass_program);
+ return git_config_string(&cfg->askpass_program, var, value);
}
if (!strcmp(var, "core.excludesfile")) {
@@ -726,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->excludes_file = NULL;
cfg->editor_program = NULL;
cfg->pager_program = NULL;
+ cfg->askpass_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -744,4 +744,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
FREE_AND_NULL(cfg->excludes_file);
FREE_AND_NULL(cfg->editor_program);
FREE_AND_NULL(cfg->pager_program);
+ FREE_AND_NULL(cfg->askpass_program);
}
diff --git a/environment.h b/environment.h
index 39b6691b47..856dc70cc4 100644
--- a/environment.h
+++ b/environment.h
@@ -93,6 +93,7 @@ struct repo_config_values {
char *excludes_file;
char *editor_program;
char *pager_program;
+ char *askpass_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -220,8 +221,6 @@ const char *get_commit_output_encoding(void);
extern char *git_commit_encoding;
extern char *git_log_output_encoding;
-extern char *askpass_program;
-
/*
* The character that begins a commented line in user-editable file
* that is subject to stripspace.
diff --git a/prompt.c b/prompt.c
index 706fba2a50..d8d74c7e37 100644
--- a/prompt.c
+++ b/prompt.c
@@ -3,6 +3,7 @@
#include "git-compat-util.h"
#include "parse.h"
#include "environment.h"
+#include "repository.h"
#include "run-command.h"
#include "strbuf.h"
#include "prompt.h"
@@ -51,7 +52,7 @@ char *git_prompt(const char *prompt, int flags)
askpass = getenv("GIT_ASKPASS");
if (!askpass)
- askpass = askpass_program;
+ askpass = repo_config_values(the_repository)->askpass_program;
if (!askpass)
askpass = getenv("SSH_ASKPASS");
if (askpass && *askpass)
--
2.43.0
^ permalink raw reply related
* [PATCH v12 04/10] environment: move pager_program into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The 'pager_program' variable is currently defined as a file-scoped
static string in pager.c. Move it into 'struct repo_config_values'.
The configuration parsing logic remains strictly within pager.c to
respect subsystem boundaries. The read/write operations are simply
redirected to the repository-specific structure using
'repo_config_values()'. All current callers indeed pass
'the_repository', so this new enforcement does not harm them.
Similar to the recent editor_program migration, no standalone getter
is introduced to keep the code minimal. The dynamically allocated
memory is now managed by 'repo_config_values_clear()'.
On top of that, fix memory leaks in pager.c while we are at it.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 2 ++
environment.h | 1 +
| 32 +++++++++++++++++++++++---------
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/environment.c b/environment.c
index a65d575af4..975c9cb9eb 100644
--- a/environment.c
+++ b/environment.c
@@ -725,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->attributes_file = NULL;
cfg->excludes_file = NULL;
cfg->editor_program = NULL;
+ cfg->pager_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -742,4 +743,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
FREE_AND_NULL(cfg->attributes_file);
FREE_AND_NULL(cfg->excludes_file);
FREE_AND_NULL(cfg->editor_program);
+ FREE_AND_NULL(cfg->pager_program);
}
diff --git a/environment.h b/environment.h
index 8178ebab76..39b6691b47 100644
--- a/environment.h
+++ b/environment.h
@@ -92,6 +92,7 @@ struct repo_config_values {
char *attributes_file;
char *excludes_file;
char *editor_program;
+ char *pager_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
--git a/pager.c b/pager.c
index 35b210e048..543ef12936 100644
--- a/pager.c
+++ b/pager.c
@@ -5,6 +5,8 @@
#include "run-command.h"
#include "sigchain.h"
#include "alias.h"
+#include "repository.h"
+#include "environment.h"
int pager_use_color = 1;
@@ -13,7 +15,6 @@ int pager_use_color = 1;
#endif
static struct child_process pager_process;
-static char *pager_program;
static int old_fd1 = -1, old_fd2 = -1;
/* Is the value coming back from term_columns() just a guess? */
@@ -75,10 +76,17 @@ static void wait_for_pager_signal(int signo)
static int core_pager_config(const char *var, const char *value,
const struct config_context *ctx UNUSED,
- void *data UNUSED)
+ void *data)
{
- if (!strcmp(var, "core.pager"))
- return git_config_string(&pager_program, var, value);
+ struct repository *r = data;
+
+ if (!strcmp(var, "core.pager")) {
+ struct repo_config_values *cfg = repo_config_values(r);
+
+ FREE_AND_NULL(cfg->pager_program);
+ return git_config_string(&cfg->pager_program, var, value);
+ }
+
return 0;
}
@@ -91,10 +99,12 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
pager = getenv("GIT_PAGER");
if (!pager) {
- if (!pager_program)
+ struct repo_config_values *cfg = repo_config_values(r);
+
+ if (!cfg->pager_program)
read_early_config(r,
- core_pager_config, NULL);
- pager = pager_program;
+ core_pager_config, r);
+ pager = cfg->pager_program;
}
if (!pager)
pager = getenv("PAGER");
@@ -302,7 +312,11 @@ int check_pager_config(struct repository *r, const char *cmd)
read_early_config(r, pager_command_config, &data);
- if (data.value)
- pager_program = data.value;
+ if (data.value) {
+ struct repo_config_values *cfg = repo_config_values(r);
+
+ free(cfg->pager_program);
+ cfg->pager_program = data.value;
+ }
return data.want;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v12 03/10] environment: move editor_program into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'editor_program' holds the path to the user's
preferred editor. Move 'editor_program' into
'struct repo_config_values' to continue the libification effort.
There have been discussions on whether external programs like
editors truly need to be configured on a per-repository basis within
the same process. While a single process might rarely invoke
different editors, this migration is necessary for two reasons:
1. Developers frequently use different toolchains for different
projects. Per-repo configuration respects this.
2. Moving this string into 'repo_config_values' eliminates mutable
global state. As the codebase moves toward becoming a long-running
processes, managing multiple repositories concurrently must
not overwrite each other's program configurations.
No standalone getter function is introduced. Callers directly access
the field via 'repo_config_values()'. Heap memory is safely reclaimed
in 'repo_config_values_clear()'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
editor.c | 4 ++--
environment.c | 7 ++++---
environment.h | 2 +-
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/editor.c b/editor.c
index fd174e6a03..0d1cb8768d 100644
--- a/editor.c
+++ b/editor.c
@@ -29,8 +29,8 @@ const char *git_editor(void)
const char *editor = getenv("GIT_EDITOR");
int terminal_is_dumb = is_terminal_dumb();
- if (!editor && editor_program)
- editor = editor_program;
+ if (!editor)
+ editor = repo_config_values(the_repository)->editor_program;
if (!editor && !terminal_is_dumb)
editor = getenv("VISUAL");
if (!editor)
diff --git a/environment.c b/environment.c
index 275931c213..a65d575af4 100644
--- a/environment.c
+++ b/environment.c
@@ -55,7 +55,6 @@ int fsync_object_files = -1;
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
-char *editor_program;
char *askpass_program;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
@@ -437,8 +436,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.editor")) {
- FREE_AND_NULL(editor_program);
- return git_config_string(&editor_program, var, value);
+ FREE_AND_NULL(cfg->editor_program);
+ return git_config_string(&cfg->editor_program, var, value);
}
if (!strcmp(var, "core.commentchar") ||
@@ -725,6 +724,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
cfg->excludes_file = NULL;
+ cfg->editor_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -741,4 +741,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
{
FREE_AND_NULL(cfg->attributes_file);
FREE_AND_NULL(cfg->excludes_file);
+ FREE_AND_NULL(cfg->editor_program);
}
diff --git a/environment.h b/environment.h
index 4776ccc657..8178ebab76 100644
--- a/environment.h
+++ b/environment.h
@@ -91,6 +91,7 @@ struct repo_config_values {
/* section "core" config values */
char *attributes_file;
char *excludes_file;
+ char *editor_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -218,7 +219,6 @@ const char *get_commit_output_encoding(void);
extern char *git_commit_encoding;
extern char *git_log_output_encoding;
-extern char *editor_program;
extern char *askpass_program;
/*
--
2.43.0
^ permalink raw reply related
* [PATCH v12 02/10] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
The global variable 'excludes_file' is used to track the path to the
global ignore file. If this variable is NULL,
'setup_standard_excludes()' in 'dir.c' forcefully evaluates and assigns
the XDG default path to it.
Continue the libification effort by encapsulating this lazy-loading
fallback logic into a proper getter and moving the variable into
'struct repo_config_values'.
Since 'excludes_file' is a dynamically allocated string, it requires
proper heap memory management. It is safely freed using the newly
introduced 'repo_config_values_clear()' function when the repository
is torn down.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
dir.c | 4 ++--
environment.c | 17 ++++++++++++++---
environment.h | 4 +++-
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/dir.c b/dir.c
index 7a73690fbc..4f87a52b3c 100644
--- a/dir.c
+++ b/dir.c
@@ -3481,11 +3481,11 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
void setup_standard_excludes(struct dir_struct *dir)
{
+ const char *excludes_file = repo_excludes_file(the_repository);
+
dir->exclude_per_dir = ".gitignore";
/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
- if (!excludes_file)
- excludes_file = xdg_config_home("ignore");
if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
add_patterns_from_file_1(dir, excludes_file,
dir->untracked ? &dir->internal.ss_excludes_file : NULL);
diff --git a/environment.c b/environment.c
index ae05f16d04..275931c213 100644
--- a/environment.c
+++ b/environment.c
@@ -57,7 +57,6 @@ enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
char *editor_program;
char *askpass_program;
-char *excludes_file;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
@@ -134,6 +133,16 @@ int is_bare_repository(void)
return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
}
+const char *repo_excludes_file(struct repository *repo)
+{
+ struct repo_config_values *cfg = repo_config_values(repo);
+
+ if (!cfg->excludes_file)
+ cfg->excludes_file = xdg_config_home("ignore");
+
+ return cfg->excludes_file;
+}
+
int have_git_dir(void)
{
return startup_info->have_repository
@@ -461,8 +470,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.excludesfile")) {
- FREE_AND_NULL(excludes_file);
- return git_config_pathname(&excludes_file, var, value);
+ FREE_AND_NULL(cfg->excludes_file);
+ return git_config_pathname(&cfg->excludes_file, var, value);
}
if (!strcmp(var, "core.whitespace")) {
@@ -715,6 +724,7 @@ int git_default_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
+ cfg->excludes_file = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -730,4 +740,5 @@ void repo_config_values_init(struct repo_config_values *cfg)
void repo_config_values_clear(struct repo_config_values *cfg)
{
FREE_AND_NULL(cfg->attributes_file);
+ FREE_AND_NULL(cfg->excludes_file);
}
diff --git a/environment.h b/environment.h
index 9169d7f62d..4776ccc657 100644
--- a/environment.h
+++ b/environment.h
@@ -90,6 +90,7 @@ struct repository;
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
+ char *excludes_file;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -133,6 +134,8 @@ int git_default_config(const char *, const char *,
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb);
+const char *repo_excludes_file(struct repository *repo);
+
void repo_config_values_init(struct repo_config_values *cfg);
/*
@@ -217,7 +220,6 @@ extern char *git_log_output_encoding;
extern char *editor_program;
extern char *askpass_program;
-extern char *excludes_file;
/*
* The character that begins a commented line in user-editable file
--
2.43.0
^ permalink raw reply related
* [PATCH v12 01/10] repository: introduce repo_config_values_clear()
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260714032525.1611141-1-cat@malon.dev>
As part of the ongoing libification effort, dynamically allocated
global configuration variables are being moved into
'struct repo_config_values'. To prevent memory leaks, we need a
destructor to free these heap-allocated variables when a repository
instance is torn down.
Introduce 'repo_config_values_clear()' in environment.c and invoke it
from 'repo_clear()' in repository.c. As a starting point, update this
new function to handle the cleanup of 'attributes_file'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 5 +++++
environment.h | 9 +++++++++
repository.c | 1 +
3 files changed, 15 insertions(+)
diff --git a/environment.c b/environment.c
index ba2c60103f..ae05f16d04 100644
--- a/environment.c
+++ b/environment.c
@@ -726,3 +726,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->sparse_expect_files_outside_of_patterns = 0;
cfg->warn_on_object_refname_ambiguity = 1;
}
+
+void repo_config_values_clear(struct repo_config_values *cfg)
+{
+ FREE_AND_NULL(cfg->attributes_file);
+}
diff --git a/environment.h b/environment.h
index 6f18286955..9169d7f62d 100644
--- a/environment.h
+++ b/environment.h
@@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg);
+/*
+ * Frees memory allocated for dynamically loaded configuration values
+ * inside `repo_config_values`.
+ *
+ * As dynamically allocated variables are migrated into this struct,
+ * their FREE_AND_NULL() calls should be appended here.
+ */
+void repo_config_values_clear(struct repo_config_values *cfg);
+
/*
* TODO: All the below state either explicitly or implicitly relies on
* `the_repository`. We should eventually get rid of these and make the
diff --git a/repository.c b/repository.c
index 187dd471c4..669e2d1200 100644
--- a/repository.c
+++ b/repository.c
@@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
FREE_AND_NULL(repo->parsed_objects);
repo_settings_clear(repo);
+ repo_config_values_clear(&repo->config_values_private_);
if (repo->config) {
git_configset_clear(repo->config);
--
2.43.0
^ permalink raw reply related
* [PATCH v12 00/10] migrate more variables into repo_config_values
From: Tian Yuchen @ 2026-07-14 3:25 UTC (permalink / raw)
To: git; +Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen
In-Reply-To: <20260712111734.1073514-1-cat@malon.dev>
Hi everyone,
This patch series continues the ongoing libification effort by migrating
a batch of global configuration variables into struct repo_config_values.
What does this series do:
infrastructure & strings (commits 1-6):
Introduce 'repo_config_values_clear()' to manage the lifecycle
of heap-allocated configuration strings. This infrastructure is utilized
to migrate string variables, including 'excludes_file', 'apply' whitespace
configs, and external programs including 'editor', 'pager', 'askpass'.
enums (commits 7-9):
Migrate enumerations 'push_default', 'autorebase', and
'object_creation_mode'. Care was taken to make these types available
to the configuration structure without triggering circular header
dependencies.
edit comment (commit 10):
Adjust the comment for config_values_private_ in repository.h.
RFC:
Commit 3~5. Is it really necessary to migrate _program variables?
https://lore.kernel.org/git/8e657184-ee0b-453a-9f2d-a98080d3582e@gmail.com/
Commit 6~9. Previous related discussions on 'git_branch_track'.
https://lore.kernel.org/git/CAD=f0L-mPX+KECUjXk-WBzEbTP7wCa8sB56GySQT0yh9mfUOWw@mail.gmail.com/
Note:
Since a new getter 'repo_excludes_file()' is introduced, as previously
promised, once it is finally merged into 'master', there will be a patch to
update and squash the comments.
Similarly, I've noticed that the classification and sorting of variables in
'repo_config_values' don't seem to be correct. There will also be a patch
to fix this, and I think it will form a commit series along with the comment
patch?
Changes since v11:
- Resending commit 7~10/10, which were not sent in V11 due to network
issue.
- In commit 6/10, fix a declaration-after-statement error in apply.c
Special thanks to Pablo and Junio!
Tian Yuchen (10):
repository: introduce repo_config_values_clear()
environment: move excludes_file into repo_config_values
environment: move editor_program into repo_config_values
environment: move pager_program into repo_config_values
environment: move askpass_program into repo_config_values
environment: migrate apply_default_whitespace and
apply_default_ignorewhitespace
environment: move push_default into repo_config_values
environment: move autorebase into repo_config_values
environment: move object_creation_mode into repo_config_values
repository: adjust the comment of config_values_private_
apply.c | 28 ++++++++++++------
branch.c | 2 +-
builtin/push.c | 10 ++++---
dir.c | 4 +--
editor.c | 4 +--
environment.c | 76 ++++++++++++++++++++++++++++++++-----------------
environment.h | 77 ++++++++++++++++++++++++++++++--------------------
object-file.c | 3 +-
pager.c | 32 +++++++++++++++------
prompt.c | 3 +-
remote.c | 2 +-
repository.c | 1 +
repository.h | 2 +-
13 files changed, 158 insertions(+), 86 deletions(-)
--
2.43.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox