* [PATCH v11 5/7] graph: wrap cascading commits after 4 columns
From: Pablo Sabater @ 2026-07-13 16:44 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, krka, mroik,
peff, phillip.wood, siddharthasthana31
In-Reply-To: <20260713-ps-pre-commit-indent-v11-0-dcb65bc4ba99@gmail.com>
Currently the visual root commits in a graph cascade indefinitely until
a commit which is not a visual root or the last commit appears.
On filters like --author where one author might contribute mostly on
single patches this can become a visual issue.
Make the cascading wrap after 4 columns.
There are two possible cases of the wrap:
1. No ambiguity:
* A
* B
* C
* D
* E
* F
2. Ambiguous conflict:
If F happens to not be a visual root and E gets wrapped back to the
initial column then E and F would be vertically adjacent. The solution
is to forcefully indent E one level:
* A
* B
* C
* D
* E
* F
* F
The magic number 4 comes as the minimum number of columns to wrap where
the output shows clearly the commits are unrelated and doesn't cause too
much "pyramid" effects
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
graph.c | 22 +++++++++++++++++++++-
t/t4218-log-graph-indentation.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/graph.c b/graph.c
index 087094189f..e3e206170c 100644
--- a/graph.c
+++ b/graph.c
@@ -1042,6 +1042,23 @@ void graph_update(struct git_graph *graph, struct commit *commit)
*/
if (!graph->visual_root_depth && flags.is_next_visual_root)
graph->visual_root_cascade = 1;
+
+ /*
+ * We wrap the cascading at a max of four columns at most, after
+ * that we wrap it back to the initial column.
+ *
+ * This could cause ambiguity in case of the next commit not
+ * being a visual root and be at the initial column after the
+ * first wrap.
+ *
+ * In case of being a non-visual-root the next, stop the
+ * cascading to get the commit indented.
+ */
+ if (!flags.is_next_visual_root &&
+ graph->visual_root_depth &&
+ !(graph->visual_root_depth % 4))
+ graph->visual_root_cascade = 0;
+
graph->visual_root_depth++;
} else {
graph->visual_root_depth = 0;
@@ -1328,8 +1345,11 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
* Each visual column is 2 characters wide.
* Omit the indentation for the first visual
* root in cascade mode.
+ *
+ * Have a max of 4 columns when cascading, after
+ * that wrap it and repeat.
*/
- int padding = (depth - graph->visual_root_cascade) * 2;
+ int padding = ((depth - graph->visual_root_cascade) % 4) * 2;
graph_line_addchars(line, ' ', padding);
graph->width += padding;
}
diff --git a/t/t4218-log-graph-indentation.sh b/t/t4218-log-graph-indentation.sh
index 60c7d84af7..d4c850c0d4 100755
--- a/t/t4218-log-graph-indentation.sh
+++ b/t/t4218-log-graph-indentation.sh
@@ -511,4 +511,33 @@ test_expect_success '--grep skipped parent makes a visual root' '
EOF
'
+# The cascading wraps after 4 columns and when wraping (column % 4 == 0) if the
+# next is a non visual-root, force indentation to avoid an ambiguous graph
+# (commit 59_A is forcefully indented)
+test_expect_success 'visual root cascading gets wrapped after 4 columns' '
+ create_orphan _58 && test_commit 58_A && test_commit 58_B &&
+ create_orphan _59 && test_commit 59_A &&
+ create_orphan _60 && test_commit 60_A &&
+ create_orphan _61 && test_commit 61_A &&
+ create_orphan _62 && test_commit 62_A &&
+ create_orphan _63 && test_commit 63_A &&
+ create_orphan _64 && test_commit 64_A &&
+ create_orphan _65 && test_commit 65_A &&
+ create_orphan _66 && test_commit 66_A &&
+ create_orphan _67 && test_commit 67_A &&
+ lib_test_check_graph _58 _59 _60 _61 _62 _63 _64 _65 _66 _67 <<-\EOF
+ * 67_A
+ * 66_A
+ * 65_A
+ * 64_A
+ * 63_A
+ * 62_A
+ * 61_A
+ * 60_A
+ * 59_A
+ * 58_B
+ * 58_A
+ EOF
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 00/10] sequencer: do not record dropped commits as rewritten
From: Junio C Hamano @ 2026-07-13 17:00 UTC (permalink / raw)
To: Phillip Wood
Cc: git, Uwe Kleine-König, Oswald Buddenhagen, Farid Zakaria
In-Reply-To: <cover.1783948637.git.phillip.wood@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Thanks to everyone who commented on v1. I've squashed the fixups that
> Junio had in "seen", squashed patches 8 & 9 together as suggested by
> Oswald and expanded the commit message, and added Uwe's Tested-by:
> trailer to the final patch. Oswald suggested extended the use of the
> enum which I think is a good idea in the long-term but I punted on
> that for now because I think it would be fairly invasive and this
> series has enough refactoring in it already.
Thanks for a concise yet very informative summary of the changes
upfront. This may be a format we want to encourage to contributors.
> If a commit gets dropped because its changes are already upstream
> then we should not record it as rewritten. As well as confusing any
> post-rewrite hooks this means we end up copying the notes from the
> dropped commit to the commit that was picked immediately before the
> one that was dropped.
Very well. I did not see anything questionable in this edition.
The contents of the tree at the end of the series is unchanged since
the previous iteration.
Shall we mark the topic ready for 'next' now?
Thanks.
> This series is structured as follows:
>
> Patch 1 restores some test coverage that was lost when the default
> rebase backend was changed.
>
> Patch 2 moves a function so it can be called without a forward
> declaration in Patch 11.
>
> Patches 3 & 4 fix the return value of do_pick_commit() when an external
> command fails (this is in preparation for patch 9).
>
> Patches 5-8 try and simplify the control flow in pick_one_commit()
> in preparation for patch 9.
>
> Patch 9 changes the return type of do_pick_commit() to an enum.
>
> Patch 10 adds a new member to the enum from patch 9 for commits that
> are dropped when they become empty and uses that to stop them from
> being recorded as rewritten.
>
> base-commit: 6c3d7b73556db708feb3b16232fab1efc4353428
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv2
> View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...c89234dd9
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v2
>
>
> Phillip Wood (10):
> t3400: restore coverage for note copying with apply backend
> sequencer: move definition of is_final_fixup()
> sequencer: be more careful with external merge
> sequencer: never reschedule on failed commit
> sequencer: remove unnecessary "or" in pick_one_commit()
> sequencer: simplify handing of fixup with conflicts
> sequencer: remove unnecessary condition in pick_one_commit()
> sequencer: simplify pick_one_commit()
> sequencer: use an enum to represent result of picking a commit
> sequencer: do not record dropped commits as rewritten
>
> sequencer.c | 154 +++++++++++++++++++++++-----------
> t/t3400-rebase.sh | 16 +++-
> t/t3404-rebase-interactive.sh | 11 +++
> t/t5407-post-rewrite-hook.sh | 23 +++++
> 4 files changed, 155 insertions(+), 49 deletions(-)
>
> Range-diff against v1:
> 1: 65af2ac07a2 = 1: 65af2ac07a2 t3400: restore coverage for note copying with apply backend
> 2: 02670f57e7d = 2: 02670f57e7d sequencer: move definition of is_final_fixup()
> 3: 16fba1e823b ! 3: 3d79362332c sequencer: be more careful with external merge
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> + opts->xopts.nr, opts->xopts.v,
> common, oid_to_hex(&head), remotes);
> + /*
> -+ * If the there were conflicts, try_merge_command() returns 1,
> ++ * If there were conflicts, try_merge_command() returns 1,
> + * any other no-zero return code means that either the merge
> + * command could not be run, or it failed to merge.
> + */
> 4: 3ffd06d6509 ! 4: fc89e77c6e8 sequencer: never reschedule on failed commit
> @@ sequencer.c: static int do_pick_commit(struct repository *r,
> *check_todo = 1;
> }
> + /*
> -+ * If "git commit" failed to run than res == -1 but we dont
> ++ * If "git commit" failed to run then res == -1, but we don't
> + * want reschedule the last command because the picking the
> + * commit was successful.
> + */
> 5: cb286ac70d7 ! 5: 26eef6c0958 sequencer: remove unnecessary "or" in pick_one_commit()
> @@ Commit message
>
> If error_with_patch(..., res, ...) succeeds then it returns "res", if
> it fails then it returns -1. This means that or-ing the return value
> - with "res" is pointless the result is the same as the return value.
> + with "res" is pointless as the result is the same as the return value.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 6: 1585d47e2ea = 6: 26dc48951ce sequencer: simplify handing of fixup with conflicts
> 7: 4386ca67d10 = 7: 71ed717d322 sequencer: remove unnecessary condition in pick_one_commit()
> 8: f51751fa3ec ! 8: e8b7fa4c59e sequencer: simplify pick_one_commit()
> @@ Commit message
> sequencer: simplify pick_one_commit()
>
> Unless we're rebasing all we do in pick_one_commit() is call
> - do_pick_commit() and return its result. Simplify the code by returing
> + do_pick_commit() and return its result. Simplify the code by returning
> early if we're not rebasing so that we don't have to continually call
> is_rebase_i() in the rest of the function. Note that there are a couple
> of conditions that do not call is_rebase_i() but they check for either
> an "edit" or a "fixup" command, both of which imply we're rebasing.
> +
> + The only block that does not return early is the one guarded by
> + "!res". Move the return into that block to make it clear that after
> + recording the commit as rewritten all we do is return from the function.
>
> As the conditional blocks are all mutually exclusive (either the
> conditions are mutually exclusive, or an earlier conditional block
> that would match a later one contains a "return" statement) chain
> them together with "else if" to make that clear.
> +
> + While we could remove "res" from the conditions below "if (!res)"
> + they are left alone because, when we start using an enum in the next
> + commit, it makes it clear that these clauses are handling cases where
> + there are conflicts.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> @@ sequencer.c: static int pick_one_commit(struct repository *r,
> record_in_rewritten(&item->commit->object.oid,
> peek_command(todo_list, 1));
> - if (res && is_fixup(item->command)) {
> ++ return 0;
> + } else if (res && is_fixup(item->command)) {
> return error_failed_squash(r, item->commit, opts,
> item->arg_len, arg);
> @@ sequencer.c: static int pick_one_commit(struct repository *r,
> int to_amend = 0;
> struct object_id oid;
>
> +@@ sequencer.c: static int pick_one_commit(struct repository *r,
> + return error_with_patch(r, item->commit, arg, item->arg_len,
> + opts, res, to_amend);
> + }
> +- return res;
> ++
> ++ BUG("Unhandled return value from do_pick_commit()");
> + }
> +
> + static int pick_commits(struct repository *r,
> 9: 2541a4d6e3d < -: ----------- sequencer: return early from pick_one_commit() on success
> 10: e4050ead27f = 9: 4fb641afb3c sequencer: use an enum to represent result of picking a commit
> 11: 26551f2687b ! 10: c89234dd949 sequencer: do not record dropped commits as rewritten
> @@ Commit message
> when rewording a fast-forwarded commit.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> + Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> ## sequencer.c ##
^ permalink raw reply
* Re: [PATCH v18 5/7] branch: add --delete-merged <branch>
From: Harald Nordgren @ 2026-07-13 18:17 UTC (permalink / raw)
To: phillip.wood
Cc: Harald Nordgren via GitGitGadget, git, Kristoffer Haugsbakk,
Johannes Sixt
In-Reply-To: <279e6d69-191b-437a-b1b1-ecd879343f3d@gmail.com>
> >> This exposes something that I don't love about this feature,
> >
> > by "this feature" do you mean "git branch --delete-merged"?
> >
> >> which is
> >> that when using a pushDefault (like we do in the tests with 'git
> >> config remote.pushDefault fork') if not adding a special case for the
> >> main/master branch (like 'git config branch.main.pushRemote origin'),
> >> then it will get cleaned up as a forked branch.
> >
> > Oh, so because the default push remote is not "origin" we need to
> > override that for the branches that we do push to "origin". That's a
> > pain, but even if we did add a special case for the default branch, it
> > would not protect other branches like "next" and "seen".
>
> Thinking about this a bit more, rather than protecting branches where
> $branch@{push} == $branch@{upstream}, perhaps we should be protecting
> branches that are merged into their upstream but
>
> git push branch.$branch.remote $branch
>
> would update $branch@{upstream}. So we'd apply the push refspec to the
> branch name, then apply the fetch refspec to that and check the result
> did not match the name of the upstream branch.
>
> Does that make sense?
This makes a lot of sense and fixes my major gripe. Seems very
possible to implement as well, I'll give it a shot.
Are you done with the rest of your review so I can push out the next version?
Harald
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Kristofer Karlsson @ 2026-07-13 19:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, Taylor Blau, Kristofer Karlsson via GitGitGadget,
git
In-Reply-To: <xmqqech99qe3.fsf@gitster.g>
On Sat, 11 Jul 2026 at 23:18, Junio C Hamano <gitster@pobox.com> wrote:
>
> Taylor Blau <ttaylorr@openai.com> writes:
>
> >> If the test involved is longer than 3 lines, I would recommend
> >> against it, as "git show" of such a patch will show the full code
> >> change to implement a different behaviour plus "_failure" changing
> >> to "_success" in the test, with the body of the test hidden outside
> >> the context, which makes it hard to guess what the behaviour change
> >> is really about.
> >
> > Hmm, I am not sure that I agree. Or, at the very least, that is now how
> > I have written series in the past where I want to demonstrate and then
> > subsequently fix an existing bug.
>
> After applying and in viewing "git log -W -p", there is no such
> difficulty like the one I described in the message you are
> responding to, but it makes it harder on reviewers on the mailing
> list, to make a quick pre-review based only on the material that
> they can see in the e-mail.
>
> It may be easier to write the commits, but given that we seem to
> have more patches sent to the list than reviewers can review, it may
> not be a good trade-off.
I've been pondering this dilemma for a bit. I agree with Taylor
that atomic commits are valuable and I quite like proving the bug
exists before fixing it. It's not black and white though,
for race conditions or hard to reproduce cases I tend to fold the
test into the fix commit directly instead.
But the review process is also critical and its overhead should be
minimized.
Could tooling help here? The submitter should know which parts
of the patch need more context for review. If they could selectively
expand context before sending, reviewers would see the full picture
in the email without sacrificing having atomic commits.
git apply already handles patches with extra context lines just
fine, so we just need something to assist in producing that extra
context -- either some configurability in git format-patch itself
(like -W, but more fine-grained control over _where_ that gets
applied) or some post-processing tool to expand context in patches
before sending.
Too late for this round, but I might give that a try in the future
if I run into a similar scenario again.
Thanks,
Kristofer
^ permalink raw reply
* Re: cygwin v2.55.0 test failures
From: Ramsay Jones @ 2026-07-13 20:15 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: GIT Mailing-list, Johannes Schindelin, Patrick Steinhardt,
Junio C Hamano, Johannes Sixt, Adam Dinwoodie
In-Reply-To: <20260712200426.GA11328@tb-raspi4>
On 12/07/2026 9:04 pm, Torsten Bögershausen wrote:
> On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:
[snip]
>> [I also had a note-to-self about 'mixed / and \ urls' in the config file
>> which is exposed by these same tests. So, another patch may be needed?]
> Not sure if I follow. cygwin allows mixed / and \ . What should be patched ?
Yes, maybe nothing needs patching - it was a note-to-self to check that the
mixed urls don't cause any issues and, maybe, normalize the urls before
writing them to the config.
>>
>> Anyway, something to think about. Hmm, I suspect it would be best to just
>> tidy up this patch first. ;)
>>
>> Just FYI. Thanks!
>>
>> ATB,
>> Ramsay Jones
>> diff --git a/connect.c b/connect.c
>> index 47e39d2a73..6f5715e938 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -1088,10 +1088,12 @@ static enum url_scheme parse_connect_url(const char *url_orig, char **ret_host,
>>
>> if (scheme == URL_SCHEME_LOCAL)
>> path = end;
>> +#ifdef DUMMY
>> else if (scheme == URL_SCHEME_FILE && *host != '/' &&
>> !has_dos_drive_prefix(host) &&
>> offset_1st_component(host - 2) > 1)
>> path = host - 2; /* include the leading "//" */
>> +#endif
>
> This very lines come from
>
> commit ebb8d2c90fb0840a0803935804e37e2205505f23
> mingw: support UNC in git clone file://server/share/repo
>
> ...and I can not see a reason to remove it.
Heh, I just read a few references [1][2][3] about file URIs to refresh my
memory (I read the RFCs many many moons ago ... and they seem to have
changed in the meantime? At least I don't remember it said that! :) ).
I seem to have misremembered the 'number of slashes' after the 'file:'
prefix as three or four, not two (specifically with a windows UNC or
absolute path). However, I was clearly wrong!
[The 'non-standard' rules on win32 are wild - git clearly doesn't support
all the edge cases].
OK, so I probably need to look at the two failing tests again - maybe I
need to mark them with !CYGWIN.
Anyway, more work to do! ;)
ATB,
Ramsay Jones
[1] https://en.wikipedia.org/wiki/File_URI_scheme
[2] https://datatracker.ietf.org/doc/rfc8089/
[3] https://learn.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows
^ permalink raw reply
* Re: [PATCH v11 0/7] graph: indent visual roots in graph
From: Junio C Hamano @ 2026-07-13 20:28 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, jltobler,
karthik.188, krka, mroik, peff, phillip.wood, siddharthasthana31
In-Reply-To: <20260713-ps-pre-commit-indent-v11-0-dcb65bc4ba99@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> V9 DIFF:
>
> - Changed boolean variables to be bit fields.
v11???
>
> 7: 737331b68d ! 7: c1fa81022e graph: add --[no-]graph-indent and log.graphIndent
> @@ revision.h: struct rev_info {
> /* Display history graph */
> struct git_graph *graph;
> int graph_max_lanes;
> -+ int no_graph_indent;
> -+ unsigned int graph_indent_set;
> ++ unsigned int no_graph_indent:1;
> ++ unsigned int graph_indent_set:1;
OK. References to these occur primarily in a boolean context, and
all assignments to them are either 0 or 1.
graph.c:442: revs->no_graph_indent = !val;
graph.c:1008: !graph->revs->no_graph_indent;
graph.c:1353: if (graph->is_visual_root && !graph->revs->no_graph_indent) {
revision.c:2630: revs->no_graph_indent = 0;
revision.c:2631: revs->graph_indent_set = 1;
revision.c:2633: revs->no_graph_indent = 1;
revision.c:2634: revs->graph_indent_set = 1;
revision.c:3209: if (revs->graph_indent_set > 0 && !revs->graph)
You may want to rewrite the last conditional check to:
if (revs->graph_indent_set && !revs->graph)
This avoids confusing readers into thinking the member can be set
to 2 or greater.
Thanks.
^ permalink raw reply
* Re: cygwin v2.55.0 test failures
From: Ramsay Jones @ 2026-07-13 20:28 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: GIT Mailing-list, Johannes Schindelin, Junio C Hamano,
Johannes Sixt, Adam Dinwoodie, Torsten Bögershausen
In-Reply-To: <alTGqS2_RmfGHvfV@pks.im>
On 13/07/2026 12:06 pm, Patrick Steinhardt wrote:
> On Fri, Jul 10, 2026 at 07:32:23PM +0100, Ramsay Jones wrote:
> [snip]
[snip]
>
> By the way: I was pondering multiple times over whether or not we should
> add Cygwin to our CI matrix. It seems to be sufficiently different from
> both MSYS2 and native Win32 to have its own set of compatibility issues,
> so that could be worth it?
Hmm, I don't know. It is a distinct platform with its own set of peculiar
issues. So, it may be worth it. However, I wouldn't want to expend CI
resources on a platform which has an unknown user-base. How many cygwin
users are there? (it can sometimes feel like there are very many, sometimes
maybe just a handful!). ;)
> For the record: I don't really have much of an opinion on this given
> that I tend to not use Windows, except when I (once again) break some
> tests there. Especially the path handling si something that tends to
> cause lots of confusion on my side.
You are not alone!
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Junio C Hamano @ 2026-07-13 20:42 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Taylor Blau, Taylor Blau, Kristofer Karlsson via GitGitGadget,
git
In-Reply-To: <CAL71e4M8-KtnkC5qQP2iuhON=ROoOTVZfbZB8UhJ-+3KgEP9=g@mail.gmail.com>
Kristofer Karlsson <krka@spotify.com> writes:
> I've been pondering this dilemma for a bit. I agree with Taylor
> that atomic commits are valuable and I quite like proving the bug
> exists before fixing it.
I do not quite understand. Even if you fix the code and add a
passing test, the commit remains atomic. With an artificial
split, you only increase your commit count while making the changes
harder to review. When grouping a code fix with a newly passing
test:
* "git show" displays both the implementation changes and the
test. You can review both, and if you agree with the behavior
expected by the test, the change is complete.
* If the pre-fix behavior is unclear, it is easy to check by
running:
$ git show ':!t/' | git apply -R && make test
This demonstrates exactly how the unfixed code breaks on the
new test.
> Too late for this round, but I might give that a try in the future
> if I run into a similar scenario again.
The existing tooling already supports this workflow (as demonstrated
by the command above). Please avoid artificially making the context
larger, as doing so increases the likelihood of merge conflicts with
other changes.
^ permalink raw reply
* Re: [PATCH v11 0/7] graph: indent visual roots in graph
From: Pablo Sabater @ 2026-07-13 20:51 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, jltobler,
karthik.188, krka, mroik, peff, phillip.wood, siddharthasthana31
In-Reply-To: <xmqqy0fews69.fsf@gitster.g>
On Mon Jul 13, 2026 at 10:28 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> V9 DIFF:
>>
>> - Changed boolean variables to be bit fields.
>
> v11???
>
My bad, I updated it manually and I forgot to.
>>
>> 7: 737331b68d ! 7: c1fa81022e graph: add --[no-]graph-indent and log.graphIndent
>> @@ revision.h: struct rev_info {
>> /* Display history graph */
>> struct git_graph *graph;
>> int graph_max_lanes;
>> -+ int no_graph_indent;
>> -+ unsigned int graph_indent_set;
>> ++ unsigned int no_graph_indent:1;
>> ++ unsigned int graph_indent_set:1;
>
> OK. References to these occur primarily in a boolean context, and
> all assignments to them are either 0 or 1.
>
> graph.c:442: revs->no_graph_indent = !val;
> graph.c:1008: !graph->revs->no_graph_indent;
> graph.c:1353: if (graph->is_visual_root && !graph->revs->no_graph_indent) {
> revision.c:2630: revs->no_graph_indent = 0;
> revision.c:2631: revs->graph_indent_set = 1;
> revision.c:2633: revs->no_graph_indent = 1;
> revision.c:2634: revs->graph_indent_set = 1;
> revision.c:3209: if (revs->graph_indent_set > 0 && !revs->graph)
>
> You may want to rewrite the last conditional check to:
>
> if (revs->graph_indent_set && !revs->graph)
>
> This avoids confusing readers into thinking the member can be set
> to 2 or greater.
I'll do that. Thanks.
>
> Thanks.
Regards,
Pablo.
^ permalink raw reply
* tc/replay-linearize [Was: Re: What's cooking in git.git (Jul 2026, #05)]
From: Elijah Newren @ 2026-07-13 21:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqik6j1m7u.fsf@gitster.g>
On Sun, Jul 12, 2026 at 10:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> * tc/replay-linearize (2026-07-07) 3 commits
> (merged to 'next' on 2026-07-09 at 371c2e9c3b)
> + replay: offer an option to linearize the commit topology
> + replay: resolve the replay base outside pick_regular_commit()
> + replay: add helper to put entry into replayed_commits
>
> The 'git replay' command has been taught the '--linearize' option to
> drop merge commits and linearize the replayed history, mimicking 'git
> rebase --no-rebase-merges'.
>
> Will merge to 'master'.
Could we hold off on that until there's a response to
<CABPp-BGzU9KHGF1nipi2HZaa1AiikMKGGaapQzHVH06wO4V1ww@mail.gmail.com> ?
I think the third patch has a pretty serious principle of least
astonishment violation, and there's two alternatives that weren't
previously considered -- one of which would be simple to implement.
^ permalink raw reply
* Re: [PATCH v7 3/3] replay: offer an option to linearize the commit topology
From: Junio C Hamano @ 2026-07-13 22:09 UTC (permalink / raw)
To: Elijah Newren; +Cc: Toon Claes, git, Johannes Schindelin
In-Reply-To: <CABPp-BGzU9KHGF1nipi2HZaa1AiikMKGGaapQzHVH06wO4V1ww@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> For what it's worth, looking back at the v5 thread, it seems the `base
> = last_commit` rule came in to fix the real bug Junio and Phillip
> pointed out there -- that without it, only one side of a linearized
> merge survived. That fix is clearly correct for the single-branch
> case. My worry is only that applying it unconditionally reintroduces
> the multiple-positive-refs ordering problem we deliberately avoid
> elsewhere. Making `--linearize` reject multiple positive refs would
> keep the merge-flattening fix while sidestepping this entirely.
>
>> A user
>> who wants to linearize ranges independently is advised to use separate
>> git-replay(1) invocations.
>
> Which, to me, is another argument for just disallowing multiple
> positive refs under `--linearize`: if the recommended way to do it is
> separate invocations anyway, we may as well require them.
Hmph. To me, this is slightly different. It acts more like an
escape hatch: "if you really do not want to mix unrelated things
into a single linear history, you can do this other thing."
Stepping back, the unpredictable order of multiple merged lines of
history exists even without multiple positive refs. If you have
independent lines of development that were merged and you linearize
them, someone must choose which line comes first. If you let the
machinery make that decision, the resulting commit order may not
reflect your preferences.
While I rarely perform octopus merges anymore, in situations where an
octopus merge is appropriate (e.g., when you have N independent
branches and their merge order does not matter), linearizing such
a history into a random sequence of N segments, built on top of
one another in an unspecified order, could actually be considered a
feature. You do not have to make a decision about something that is
inconsequential.
So, I am not convinced we should forbid this behavior to avoid
dealing with a history containing merges or multiple positive tips.
When achieving a strictly linear history is the user's goal under
the "--linearize" option, is it not inherent that there is no single
"correct" order for these independent segments of history to appear
in the final linear result?
Perhaps I am not reading you correctly, but that is how I read that
escape hatch explanation.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Kristofer Karlsson @ 2026-07-13 22:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, Taylor Blau, Kristofer Karlsson via GitGitGadget,
git
In-Reply-To: <xmqqldbewriu.fsf@gitster.g>
On Mon, 13 Jul 2026 at 22:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> I do not quite understand. Even if you fix the code and add a
> passing test, the commit remains atomic. With an artificial
> split, you only increase your commit count while making the changes
> harder to review. When grouping a code fix with a newly passing
> test:
>
> * "git show" displays both the implementation changes and the
> test. You can review both, and if you agree with the behavior
> expected by the test, the change is complete.
>
> * If the pre-fix behavior is unclear, it is easy to check by
> running:
>
> $ git show ':!t/' | git apply -R && make test
That's quite neat, and it matches the local
development flow if you write the failing test first.
I can see the advantages of grouping the test and bugfix in the
same commit, and I'm happy to follow that convention going forward.
> > Too late for this round, but I might give that a try in the future
> > if I run into a similar scenario again.
>
> The existing tooling already supports this workflow (as demonstrated
> by the command above). Please avoid artificially making the context
> larger, as doing so increases the likelihood of merge conflicts with
> other changes.
Thanks, that makes sense. It was an interesting thought experiment,
but I'll leave it there.
- Kristofer
^ permalink raw reply
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Ben Knoble @ 2026-07-13 22:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, D. Ben Knoble, Jeff King, git,
brian m carlson, Ramsay Jones
In-Reply-To: <xmqqa4rx9mb5.fsf@gitster.g>
> Le 11 juil. 2026 à 18:46, Junio C Hamano <gitster@pobox.com> a écrit :
>
> Patrick Steinhardt <ps@pks.im> writes:
>
>> I don't think we'd necessarily need a way to detect this. Our current
>> build default is to have this disabled, so I'd keep it this way, but
>> automatically compile nsec-support into Git if available. And then we
>> provide a way for users to opt-in to the new behaviour via the config.
>>
>> An automated test would of course be nice to have so that we know to
>> enable this in cases where we can determine that it works. But with the
>> above we'd already make the feature more accessible than it currently
>> is, because I'd expect that most distros simply don't enable the build
>> toggle at all.
>
> In any case, the discussion tells me that if we were to pursue this
> topic further, it would not primarily be about adding the build knob
> to meson.build file, but rather a bit more involved to affect the
> product for everybody regardless of the build framework used.
>
> So I think it is safe for me discard this topic from my tree for
> now, with an invitation to resurrect it as a topic with shifted
> focus.
>
> Thanks.
Yep, I’d been meaning to send a « please discard » message per the new guidelines ;) been on vacation.
^ permalink raw reply
* [PATCH 0/2] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-13 22:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
Packfile URI downloads currently stage a pack at
objects/pack/pack-<hash>.pack.temp. Two Git processes fetching the same
pack into one object database can append to that file concurrently,
which can corrupt the temporary pack or cause a resume request at EOF.
The first patch gives each direct packfile URI download a private
temporary file. Ordinary dumb HTTP pack requests retain their existing
resumable staging behavior. A later packfile URI retry starts a new
download.
The second patch handles the related .keep race. When another process
has already created the keep file, index-pack reports "pack<TAB><hash>"
instead of "keep<TAB><hash>". Accept both successful forms and remove
only keep files created by the current process.
Each patch adds a regression test for its respective race.
Ted Nyman (2):
http: use unique tempfiles for packfile URI downloads
fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 5 +-
fetch-pack.c | 36 ++++++++-------
http.c | 77 +++++++++++++++++++++----------
http.h | 1 +
t/t5550-http-fetch-dumb.sh | 72 ++++++++++++++++++++++++++++-
t/t5702-protocol-v2.sh | 31 +++++++++++++
6 files changed, 177 insertions(+), 45 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
2.55.0
^ permalink raw reply
* [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Ted Nyman @ 2026-07-13 22:34 UTC (permalink / raw)
To: git
Cc: 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>
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;
+ } else if (preq->packfile) {
fclose(preq->packfile);
preq->packfile = NULL;
}
@@ -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;
tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
@@ -2711,7 +2717,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
cleanup:
close(tmpfile_fd);
- unlink(preq->tmpfile.buf);
+ if (preq->tempfile)
+ delete_tempfile(&preq->tempfile);
+ else
+ unlink(preq->tmpfile.buf);
return ret;
}
@@ -2723,20 +2732,8 @@ void http_install_packfile(struct packed_git *p,
packfile_store_add_pack(files->packed, p);
}
-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));
-}
-
-struct http_pack_request *new_direct_http_pack_request(
- const unsigned char *packed_git_hash, char *url)
+static struct http_pack_request *new_http_pack_request_for_url(
+ const unsigned char *packed_git_hash, char *url, int resumable)
{
off_t prev_posn = 0;
struct http_pack_request *preq;
@@ -2746,9 +2743,22 @@ struct http_pack_request *new_direct_http_pack_request(
preq->url = url;
- odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
- strbuf_addstr(&preq->tmpfile, ".temp");
- preq->packfile = fopen(preq->tmpfile.buf, "a");
+ 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);
@@ -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) {
if (http_is_verbose)
fprintf(stderr,
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
@@ -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;
}
+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_http_pack_request_for_url(packed_git_hash,
+ strbuf_detach(&buf, NULL), 1);
+}
+
+struct http_pack_request *new_direct_http_pack_request(
+ const unsigned char *packed_git_hash, char *url)
+{
+ return new_http_pack_request_for_url(packed_git_hash, url, 0);
+}
+
/* Helpers for fetching objects (loose) */
static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
void *data)
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' '
+ git init packfileclient-concurrent &&
+ HASH=$(git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git rev-parse HEAD) &&
+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+ ls objects/pack/pack-*.pack) &&
+ packhash=$(basename "$p" .pack) &&
+ packhash=${packhash#pack-} &&
+
+ mkfifo first-ready first-continue &&
+ exec 8<>first-ready &&
+ exec 9<>first-continue &&
+ write_script git-wait-index-pack <<-\EOF &&
+ echo ready >"$GIT_TEST_WAIT_READY" &&
+ read continue <"$GIT_TEST_WAIT_CONTINUE" &&
+ exec git index-pack "$@"
+ EOF
+
+ # 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=$!
+ } &&
+ 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
+ " &&
+
+ read ready <&8 &&
+ test "$ready" = ready &&
+ git -C packfileclient-concurrent http-fetch \
+ --packfile="$packhash" \
+ --index-pack-arg=index-pack \
+ --index-pack-arg=--stdin \
+ --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >second.out &&
+ echo continue >&9 &&
+ wait "$first_pid" &&
+
+ printf "pack\t%s\n" "$packhash" >expect &&
+ test_cmp expect first.out &&
+ printf "keep\t%s\n" "$packhash" >expect &&
+ test_cmp expect second.out &&
+ test_path_is_missing \
+ "packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
+ find packfileclient-concurrent/.git/objects/pack \
+ -name "tmp_pack_*" -print >tmpfiles &&
+ test_must_be_empty tmpfiles &&
+ git -C packfileclient-concurrent cat-file -e "$HASH"
+'
+
test_expect_success 'fetch notices corrupt pack' '
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
@@ -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
'
test_expect_success 'fetch notices corrupt idx' '
--
2.55.0
^ permalink raw reply related
* [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs
From: Ted Nyman @ 2026-07-13 22:34 UTC (permalink / raw)
To: git
Cc: 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>
When "index-pack --keep" creates a .keep file, it reports
"keep<TAB><hash>". If the file already exists, index-pack leaves it
untouched and reports "pack<TAB><hash>" instead.
Since dd4b732df7 (upload-pack: send part of packfile response as uri,
2020-06-10), fetch-pack has accepted only the "keep" form for packs
downloaded through packfile URIs. A concurrent fetch can install the
same pack and create its .keep file before another process reaches
index-pack. The latter process then fails even though index-pack
completed successfully.
Accept both successful forms. Add a path to pack_lockfiles only for the
"keep" form, so cleanup removes only a keep file created by the current
process and preserves a pre-existing one.
Add a regression test which pre-creates a keep file and verifies that a
fetch succeeds without changing it.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
fetch-pack.c | 36 ++++++++++++++++++++----------------
t/t5702-protocol-v2.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 16 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..a16b80177a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1887,9 +1887,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
for (i = 0; i < packfile_uris.nr; i++) {
+ int created_keep = 0;
int j;
struct child_process cmd = CHILD_PROCESS_INIT;
- char packname[GIT_MAX_HEXSZ + 1];
+ char packname[GIT_MAX_HEXSZ + 6];
+ const char *packhash;
+ const int packname_len = the_hash_algo->hexsz + 6;
const char *uri = packfile_uris.items[i].string +
the_hash_algo->hexsz + 1;
@@ -1907,16 +1910,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
if (start_command(&cmd))
die("fetch-pack: unable to spawn http-fetch");
- if (read_in_full(cmd.out, packname, 5) < 0 ||
- memcmp(packname, "keep\t", 5))
- die("fetch-pack: expected keep then TAB at start of http-fetch output");
-
- if (read_in_full(cmd.out, packname,
- the_hash_algo->hexsz + 1) < 0 ||
- packname[the_hash_algo->hexsz] != '\n')
- die("fetch-pack: expected hash then LF at end of http-fetch output");
-
- packname[the_hash_algo->hexsz] = '\0';
+ if (read_in_full(cmd.out, packname, packname_len) != packname_len ||
+ packname[packname_len - 1] != '\n')
+ die("fetch-pack: expected pack or keep, TAB, hash, "
+ "then LF in http-fetch output");
+ packname[packname_len - 1] = '\0';
+ if (skip_prefix(packname, "keep\t", &packhash))
+ created_keep = 1;
+ else if (!skip_prefix(packname, "pack\t", &packhash))
+ die("fetch-pack: expected pack or keep, TAB, hash, "
+ "then LF in http-fetch output");
parse_gitmodules_oids(cmd.out, &fsck_options.gitmodules_found);
@@ -1925,16 +1928,17 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
if (finish_command(&cmd))
die("fetch-pack: unable to finish http-fetch");
- if (memcmp(packfile_uris.items[i].string, packname,
+ if (memcmp(packfile_uris.items[i].string, packhash,
the_hash_algo->hexsz))
die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
uri, (int) the_hash_algo->hexsz,
packfile_uris.items[i].string);
- string_list_append_nodup(pack_lockfiles,
- xstrfmt("%s/pack/pack-%s.keep",
- repo_get_object_directory(the_repository),
- packname));
+ if (created_keep)
+ string_list_append_nodup(pack_lockfiles,
+ xstrfmt("%s/pack/pack-%s.keep",
+ repo_get_object_directory(the_repository),
+ packhash));
}
string_list_clear(&packfile_uris, 0);
strvec_clear(&index_pack_args);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 9f6cf4142d..1861eb7d7c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -1291,6 +1291,37 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
fetch "$HTTPD_URL/smart/http_parent"
'
+test_expect_success 'packfile URI preserves an existing keep file' '
+ P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+ rm -rf "$P" http_child keep.expect &&
+
+ git init "$P" &&
+ git -C "$P" config uploadpack.allowsidebandall true &&
+
+ echo my-blob >"$P/my-blob" &&
+ git -C "$P" add my-blob &&
+ git -C "$P" commit -m x &&
+ configure_exclusion "$P" my-blob >h &&
+
+ git init http_child &&
+ packhash=$(cat packh) &&
+ keep="http_child/.git/objects/pack/pack-$packhash.keep" &&
+ echo pre-existing >"$keep" &&
+ cp "$keep" keep.expect &&
+
+ GIT_TEST_SIDEBAND_ALL=1 \
+ git -C http_child -c protocol.version=2 \
+ -c fetch.uriprotocols=http,https \
+ fetch "$HTTPD_URL/smart/http_parent" &&
+
+ test_path_is_file \
+ "http_child/.git/objects/pack/pack-$packhash.pack" &&
+ test_path_is_file \
+ "http_child/.git/objects/pack/pack-$packhash.idx" &&
+ test_cmp keep.expect "$keep" &&
+ git -C http_child cat-file -e "$(cat h)"
+'
+
test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
rm -rf "$P" http_child log &&
--
2.55.0
^ permalink raw reply related
* [PATCH 0/2] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-13 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
Packfile URI downloads currently stage a pack at
objects/pack/pack-<hash>.pack.temp. Two Git processes fetching the same
pack into one object database can append to that file concurrently,
which can corrupt the temporary pack or cause a resume request at EOF.
The first patch gives each direct packfile URI download a private
temporary file. Ordinary dumb HTTP pack requests retain their existing
resumable staging behavior. A later packfile URI retry starts a new
download.
The second patch handles the related .keep race. When another process
has already created the keep file, index-pack reports "pack<TAB><hash>"
instead of "keep<TAB><hash>". Accept both successful forms and remove
only keep files created by the current process.
Each patch adds a regression test for its respective race.
Ted Nyman (2):
http: use unique tempfiles for packfile URI downloads
fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 5 +-
fetch-pack.c | 36 ++++++++-------
http.c | 77 +++++++++++++++++++++----------
http.h | 1 +
t/t5550-http-fetch-dumb.sh | 72 ++++++++++++++++++++++++++++-
t/t5702-protocol-v2.sh | 31 +++++++++++++
6 files changed, 177 insertions(+), 45 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
2.55.0
^ permalink raw reply
* [PATCH 0/2] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-13 22:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Taylor Blau, Jeff King, Patrick Steinhardt,
Karthik Nayak, brian m. carlson,
Ævar Arnfjörð Bjarmason
Packfile URI downloads currently stage a pack at
objects/pack/pack-<hash>.pack.temp. Two Git processes fetching the same
pack into one object database can append to that file concurrently,
which can corrupt the temporary pack or cause a resume request at EOF.
The first patch gives each direct packfile URI download a private
temporary file. Ordinary dumb HTTP pack requests retain their existing
resumable staging behavior. A later packfile URI retry starts a new
download.
The second patch handles the related .keep race. When another process
has already created the keep file, index-pack reports "pack<TAB><hash>"
instead of "keep<TAB><hash>". Accept both successful forms and remove
only keep files created by the current process.
Each patch adds a regression test for its respective race.
Ted Nyman (2):
http: use unique tempfiles for packfile URI downloads
fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 5 +-
fetch-pack.c | 36 ++++++++-------
http.c | 77 +++++++++++++++++++++----------
http.h | 1 +
t/t5550-http-fetch-dumb.sh | 72 ++++++++++++++++++++++++++++-
t/t5702-protocol-v2.sh | 31 +++++++++++++
6 files changed, 177 insertions(+), 45 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
2.55.0
^ permalink raw reply
* Re: [PATCH 0/2] packfile URIs: support concurrent downloads
From: Junio C Hamano @ 2026-07-13 22:48 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Taylor Blau, Jeff King, Patrick Steinhardt, Karthik Nayak,
brian m. carlson, Ævar Arnfjörð Bjarmason
In-Reply-To: <alVn7UWvdWRAG-Vv@com-76773>
Ted Nyman <tnyman@openai.com> writes:
> Packfile URI downloads currently stage a pack at
> objects/pack/pack-<hash>.pack.temp. Two Git processes fetching the same
> pack into one object database can append to that file concurrently,
> which can corrupt the temporary pack or cause a resume request at EOF.
>
> The first patch gives each direct packfile URI download a private
> temporary file. Ordinary dumb HTTP pack requests retain their existing
> resumable staging behavior. A later packfile URI retry starts a new
> download.
>
> The second patch handles the related .keep race. When another process
> has already created the keep file, index-pack reports "pack<TAB><hash>"
> instead of "keep<TAB><hash>". Accept both successful forms and remove
> only keep files created by the current process.
>
> Each patch adds a regression test for its respective race.
>
> Ted Nyman (2):
> http: use unique tempfiles for packfile URI downloads
> fetch-pack: accept "pack" output for packfile URIs
This cover letter has
Message-ID: <alVn7UWvdWRAG-Vv@com-76773>
but in the header of [PATCH 1/2] has
Message-ID: <alVn-QmK3K91_tkH@com-76773>
References: <cover.1783982021.git.tnyman@openai.com>
In-Reply-To: <cover.1783982021.git.tnyman@openai.com>
Similarly, [PATCH 2/2] has
Message-ID: <alVoA5-fDDPwKPZZ@com-76773>
References: <cover.1783982021.git.tnyman@openai.com>
In-Reply-To: <cover.1783982021.git.tnyman@openai.com>
And "b4 am" seems to be having problem grabbing the patchset X-<.
^ permalink raw reply
* Re: [PATCH 0/2] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-13 22:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Taylor Blau, Jeff King, Patrick Steinhardt, Karthik Nayak,
brian m. carlson, Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqq4ii2wlo1.fsf@gitster.g>
> And "b4 am" seems to be having problem grabbing the patchset X-<.
Sorry for the noise -- Mutt rewrote the original cover-letter
Message-ID. The patches reference the corrected cover:
https://lore.kernel.org/git/cover.1783982021.git.tnyman@openai.com/
I confirmed that this retrieves both patches:
b4 am cover.1783982021.git.tnyman@openai.com
Thanks,
Ted
^ permalink raw reply
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Junio C Hamano @ 2026-07-14 1:00 UTC (permalink / raw)
To: Ted Nyman
Cc: git, Taylor Blau, Jeff King, Patrick Steinhardt, Karthik Nayak,
brian m. carlson, Ævar Arnfjörð Bjarmason
In-Reply-To: <alVn-QmK3K91_tkH@com-76773>
Ted Nyman <tnyman@openai.com> writes:
> 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.
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?
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? If the
open call fails because the file already exists, the second process
can detect that another process is active and wait for it to finish
rather than initiating its own network request.
Doing so might require setting up a trigger or polling mechanism to
wait for the first process's download to complete (and detecting if
the other process dies without cleaning up), though that may open a
can of worms.
^ permalink raw reply
* Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
From: Ted Nyman @ 2026-07-14 1:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Taylor Blau, Jeff King, Patrick Steinhardt, Karthik Nayak,
brian m. carlson, Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqse5mv10a.fsf@gitster.g>
> 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.
The waiting case would also need a complete handoff. If the first
process finishes, the second would need to notice the installed pack
and account for the expected index-pack result and keep state. If the
first process fails and removes its temporary file, the second would
need to retry as the downloader. That is possible, but introduces
cross-process coordination and a timeout policy in http-fetch.
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.
Thanks,
Ted
^ permalink raw reply
* Re: [PATCH 0/2] packfile URIs: support concurrent downloads
From: Taylor Blau @ 2026-07-14 2:42 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: <alVs4JO9BNQrXsnO@com-76773>
On Mon, Jul 13, 2026 at 03:55:28PM -0700, Ted Nyman wrote:
> > And "b4 am" seems to be having problem grabbing the patchset X-<.
>
> Sorry for the noise -- Mutt rewrote the original cover-letter
> Message-ID. The patches reference the corrected cover:
>
> https://lore.kernel.org/git/cover.1783982021.git.tnyman@openai.com/
>
> I confirmed that this retrieves both patches:
>
> b4 am cover.1783982021.git.tnyman@openai.com
This is a mistake on my end as I was porting over some of the scripts
for sending patches to the mailing list to OpenAI's infrastructure.
The short version of this e-mail is that the issue is fixed. But the
longer version is funny (at least to me), so I figured I would share.
As some background, my workflow for sending patches to the mailing list
is to use a script called 'git mail' that effectively runs format-patch
to build an *.mbox and then opens Mutt in that directory. I then review
the patches one last time before sending, and then run a macro I have
bound to 'B', which (effectively) runs <resend-message>.
For reasons that I cannot quite recall, I chose this workflow many years
ago when it would likely have been more appropriate to use `mutt -H`,
which does *not* rewrite Message-ID headers when resending.
To work around this, I wrote a patch that I applied to the version of
Mutt I used both on my old work laptop as well as the Linux workstation
where I did the majority of my work. The patch is fairly small, and is
effectively:
--- 8< ---
diff --git a/postpone.c b/postpone.c
index f557976d..accbb4f6 100644
--- a/postpone.c
+++ b/postpone.c
@@ -607,13 +607,9 @@ int mutt_prepare_template (FILE *fp, CONTEXT *ctx, HEADER *newhdr, HEADER *hdr,
newhdr->content->length = hdr->content->length;
mutt_parse_part (fp, newhdr->content);
- /* If resending a message, don't keep message_id or mail_followup_to.
- * Otherwise, we are resuming a postponed message, and want to keep those
- * headers if they exist.
- */
+ /* If resending a message, don't keep mail_followup_to. */
if (resend)
{
- FREE (&newhdr->env->message_id);
FREE (&newhdr->env->mail_followup_to);
}
--
2.26.0.106.g9fadedd637
--- >8 ---
(The Git version this patch was prepared with should give you some sense
of how ancient this part of my workflow is ;-).)
When looking at this yesterday after sending the 'no-ref-delta' patches
to the list, I could not figure out quite why my Mutt client was
rewriting Message-ID headers until I remembered the aforementioned
patch.
The fix is somewhat OpenAI-specific, and so not interesting to share
with the list, but effectively relies on piping messages to 'mutt -H -'
to send the message without dropping (and thus rewriting) the Message-ID
header.
(As an alternative, I could have continued to carry that patch to
'postpone.c', but in retrospect it seems gross^W unnecessary, so I
ditched it.)
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v11 06/10] environment: migrate apply_default_whitespace and apply_default_ignorewhitespace
From: Tian Yuchen @ 2026-07-14 3:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <xmqqa4ruyhbh.fsf@gitster.g>
On 7/14/26 00:39, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
>
>> Subject: Re: [PATCH v11 06/10] environment: migrate apply_default_whitespace and apply_default_ignorewhitespace
>
> Are there patches 7..10/10 posted somewhere else? I didn't see them
> in the thread (neither did "b4").
>
Oh, I didn't notice that:
Died at /usr/lib/git-core/git-send-email line 1665.
Will resend very soon.
>>
>> -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);
>> }
>
> OK.
>
>> static int parse_whitespace_option(struct apply_state *state, const char *option)
>> @@ -126,10 +132,15 @@ 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);
>> +
>> + struct repo_config_values *cfg = repo_config_values(repo);
>
> Doesn't "-Wdeclaration-after-statement" complain on this, declaring cfg
> after calling "git_apply_config(repo)" on the line before?
>
Nice catch, thanks!
Regards, yuchen
^ permalink raw reply
* [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