* [PATCH v4 0/7] odb: add write operation to ODB transaction interface
From: Justin Tobler @ 2026-05-14 18:37 UTC (permalink / raw)
To: git; +Cc: ps, gitster, peff, Justin Tobler
In-Reply-To: <20260402213220.2651523-1-jltobler@gmail.com>
Greetings,
This series lays the groundwork for introducing write operations to the
ODB transaction interface. The eventual goal is for all object writes
performed within a transaction to go through this interface explicitly,
rather than implicitly relying on the transaction to reconfigure ODB
sources so that writes are redirected to a temporary location.
For now, only `odb_transaction_write_object_stream()` is implemented and
wires up the existing logic for streaming "large" blobs directly into a
packfile as part of the transaction.
Most of the patches are structural refactorings to enable this, but
patch 4 introduces a behavioral change in how packfiles that would
exceed "pack.packSizeLimit" are handled.
Changes since V3:
- Fixed leak due to an fd not being closed when exiting prior to
close_loose_object() being invoked.
Changes since V2:
- Renamed some variables to improve clarity
- Make `odb_write_stream_from_fd()` fully initialize the underlying
`struct odb_write_stream`
- Move `struct odb_write_stream` to "odb/streaming.h"
- Make the `hash_blob_stream()` helper more generic by operating on a
`struct odb_write_stream` instead of reading from an fd directly.
- Introduce an `odb_write_stream_release()` helper to free the
underlying stream data.
Changes since V1:
- Fixed some typos
- Improved error handling
- Removed unnecessary guard statement
- Documented in comments why inflated object size is used to approximate
if object write will exceed "pack.packSizeLimit".
- Updated `struct odb_write_stream` read() callback to support returning
errors and using caller provided buffer
- Updated the `hash_blob_stream()` function signature to operate on a
`struct odb_write_stream` instead of an fd directly
- Renamed some variables/functions for better clarity
Thanks,
-Justin
Justin Tobler (7):
odb: split `struct odb_transaction` into separate header
odb/transaction: use pluggable `begin_transaction()`
odb: update `struct odb_write_stream` read() callback
object-file: remove flags from transaction packfile writes
object-file: avoid fd seekback by checking object size upfront
object-file: generalize packfile writes to use odb_write_stream
odb/transaction: make `write_object_stream()` pluggable
Makefile | 1 +
builtin/add.c | 1 +
builtin/unpack-objects.c | 21 ++--
builtin/update-index.c | 1 +
cache-tree.c | 1 +
meson.build | 1 +
object-file.c | 238 ++++++++++++++++++++-------------------
odb.c | 25 ----
odb.h | 37 +-----
odb/streaming.c | 51 +++++++++
odb/streaming.h | 30 +++++
odb/transaction.c | 35 ++++++
odb/transaction.h | 57 ++++++++++
read-cache.c | 1 +
14 files changed, 312 insertions(+), 188 deletions(-)
create mode 100644 odb/transaction.c
create mode 100644 odb/transaction.h
Range-diff against v3:
1: eee372b426 = 1: eee372b426 odb: split `struct odb_transaction` into separate header
2: 57ac075560 = 2: 57ac075560 odb/transaction: use pluggable `begin_transaction()`
3: 11321ad607 ! 3: d53ad95712 odb: update `struct odb_write_stream` read() callback
@@ object-file.c: int odb_source_loose_write_stream(struct odb_source *source,
+ ssize_t read_len = odb_write_stream_read(in_stream, buf,
+ sizeof(buf));
+ if (read_len < 0) {
++ close(fd);
+ err = -1;
+ goto cleanup;
+ }
4: 72d4656eee = 4: fa7a3ad5dc object-file: remove flags from transaction packfile writes
5: e4896101ff = 5: 1ca08e0590 object-file: avoid fd seekback by checking object size upfront
6: b3cb0a707c = 6: a548401057 object-file: generalize packfile writes to use odb_write_stream
7: e1d292a7ed = 7: 4765b1024a odb/transaction: make `write_object_stream()` pluggable
base-commit: 5361983c075154725be47b65cca9a2421789e410
--
2.54.0.105.g59ff4886a5
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Pablo Sabater @ 2026-05-14 17:45 UTC (permalink / raw)
To: phillip.wood
Cc: git, gitster, christian.couder, karthik.188, jltobler,
ayu.chandekar, siddharthasthana31, chandrapratap3519
In-Reply-To: <26d887d2-6ec2-4af1-b0bd-8e9b017bb4dd@gmail.com>
El jue, 14 may 2026 a las 17:15, Phillip Wood
(<phillip.wood123@gmail.com>) escribió:
>
> Hi Pablo
>
> On 02/04/2026 22:17, Pablo Sabater wrote:
> > When having a history with multiple root commits and drawing the history
> > near the roots, the graphing engine renders the commit one below the other,
> > seeming that they are related, which makes the graph confusing.
> >
> > This issue was reported by Junio at:
> > https://lore.kernel.org/git/xmqqikaawrpx.fsf@gitster.g/
> >
> > e.g.:
> >
> > * root-B
> > * child-A2
> > * child-A1
> > * root-A
> >
> > [...]
> >
> > * root-B
> > * child-A2
> > /
> > * child-A1
> > * root-A
>
> I'm rather late to the party here, but personally I find the indentation
> a bit confusing, it would be clearer to me if we had a blank line after
> a root commit
Hi,
>
> * root-B
>
> * child-A2
> * child-A1
> * root-A
>
> It takes the same amount of vertical space but keeps the children of
> root-A together.
I have mixed feelings about which approach to choose.
The idea of a blank line was thought at
https://lore.kernel.org/git/xmqq8s8vvw9m.fsf@gitster.c.googlers.com/
but Junio argued against it for having an extra row because the
indentation he proposed didn't collapse, however I find indentation +
no collapse the most confusing one.
I'd say that I'm fine with both approaches, blank line or indentation
+ collapse.
> > without the patch:
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > * D root
> >
> > with the patch, the indentation cascades:
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > _ /
> > /
> > /
> > * D root
* A root
* B root
* C root
* D1 child
* D root
Here I think a blank line looks worse, too much space for just 5
commits and becomes one extra line which if this were like up to 7 or
more parentless commits one after the other would be more noticeable.
But there are cases that blank line might be better:
* 10_A2
* 10_A1
* 10_A
* 10_M
/|\
| | * 10_D
| * 10_C
* 10_B
Feels like a shower of commits instead of an indented merge.
Pro to the blank line, the parentless check is the same and it's just
printing a '\n' at the right spot, while indent i'm mimicking like if
there was a commit there.
Anyways, I think in the majority of the cases the indentation +
collapsing looks better.
Sorry for the brief reply, I'm busy today.
Regards,
--
Pablo
>
> Thanks
>
> Phillip
>
> > This is done by adding a is_placeholder flag to the columns, the root commit
> > is actually there but marked as a placeholder
> >
> > e.g.:
> >
> > * root-B
> > (B) * child-A2
> > /
> > * child-A1
> > * root-A
> >
> > (B) would be root-B column with the placeholder flag active.
> >
> > Then teaching the rendering function to print a padding ' ' when meeting a
> > placeholder column outputs the second example.
> >
> > There could also be the case where there are multiple roots
> >
> > without the patch:
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > * D root
> >
> > with the patch, the indentation cascades:
> >
> > * A root
> > * B root
> > * C root
> > * D1 child
> > _ /
> > /
> > /
> > * D root
> >
> > the _ / might look weird but that's how the collapsing rendering does it
> > for big gaps, this case being from the 4th column to the 0th column.
> > Another patch could change the collapsing rendering for placeholders ?
> > I haven't done it to keep it minimal, but a follow up could make it
> > to be straight '/'. This would make it bigger but easier for the eye to follow.
> > IMO is not worth it, but opinions are welcome.
> >
> > The patch also adds tests for different cases like a root preceding multiple
> > parents merges and the examples above.
> >
> > There could be some edge cases still so any testing is very welcome.
> >
> > Pablo Sabater (1):
> > graph: add indentation for commits preceded by a root
> >
> > graph.c | 68 ++++++++++++++++--
> > t/t4215-log-skewed-merges.sh | 136 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 198 insertions(+), 6 deletions(-)
> >
> >
> > base-commit: 256554692df0685b45e60778b08802b720880c50
>
^ permalink raw reply
* Re: What's cooking in git.git (May 2026, #03)
From: Pushkar Singh @ 2026-05-14 17:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmry3i9a4.fsf@gitster.g>
Hi Junio,
My thinking was mainly that git stash show normally omits untracked
changes, while --include-untracked consults the additional untracked
parent of the stash commit.
I did not see existing coverage specifically checking that behavior, so
I thought a small test for it could be useful. But I understand your
point that not every observable behavior necessarily needs explicit
coverage, and I'll keep that in mind for future patches.
Thanks,
Pushkar
^ permalink raw reply
* [PATCH] revision: use priority queue in limit_list()
From: Kristofer Karlsson via GitGitGadget @ 2026-05-14 16:51 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
From: Kristofer Karlsson <krka@spotify.com>
limit_list() maintains a date-sorted work queue of commits using a
linked list with commit_list_insert_by_date() for insertion. Each
insertion walks the list to find the right position — O(n) per insert.
In repositories with merge-heavy histories, the symmetric difference
can contain thousands of commits, making this O(n) insertion the
dominant cost.
Replace the sorted linked list with a prio_queue (binary heap). This
gives O(log n) insertion and O(log n) extraction instead of O(n)
insertion and O(1) extraction, which is a net win when the queue is
large.
The still_interesting() and everybody_uninteresting() helpers are
updated to scan the prio_queue's contiguous array instead of walking a
linked list. process_parents() already accepts both a commit_list and
a prio_queue parameter, so the change in limit_list() simply switches
which one is passed.
Benchmark: git rev-list --left-right --count HEAD~N...HEAD
Repository: 2.3M commits, merge-heavy DAG (monorepo)
Best of 5 runs, times in seconds:
commits in
symmetric diff baseline patched speedup
-------------- -------- ------- -------
10 0.01 0.01 1.0x
50 0.01 0.01 1.0x
3751 21.23 8.49 2.5x
4524 21.70 8.29 2.6x
10130 20.10 6.65 3.0x
No change for small traversals; 2.5-3.0x faster when the queue grows
to thousands of commits.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
revision: use priority queue in limit_list()
This patch speeds up limit_list() by 2.5–3x on large, merge-heavy
repositories by replacing a sorted linked list with a priority queue.
The sorted linked list used as a work queue in limit_list() has O(n)
insertion cost per commit, where n is the current queue length (the
"width" of the active walk frontier). In merge-heavy DAGs this frontier
grows wide — profiling on a 2.3M-commit monorepo showed 59% of total CPU
time in commit_list_insert_by_date(). Total cost is O(N·w) where N is
commits walked and w is peak queue width; in merge-heavy histories w
scales with N, approaching O(N²).
Switching to a prio_queue (binary heap) reduces insertion cost to O(log
w), bringing total cost to O(N·log w). The practical result on the same
repository:
commits in
symmetric diff before after speedup
-------------- -------- ------- -------
3751 21.2s 8.5s 2.5x
4524 21.7s 8.3s 2.6x
10130 20.1s 6.6s 3.0x
This affects any command that triggers limit_list() — i.e., when
revs->limited is set — including --left-right, --cherry-mark,
--cherry-pick, --ancestry-path, bisect, and rebase's fork-point
computation. The practical trigger is git status --ahead-behind on a
branch that has diverged from upstream in a merge-heavy repository.
The change is minimal (+21/−17 lines, single file) because
process_parents() already accepts both a commit_list and a prio_queue
parameter — limit_list() just switches which one it passes.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2114%2Fspkrka%2Flimit-list-prio-queue-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2114/spkrka/limit-list-prio-queue-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2114
revision.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/revision.c b/revision.c
index 599b3a66c3..2b1b3bb10e 100644
--- a/revision.c
+++ b/revision.c
@@ -473,10 +473,10 @@ static struct commit *handle_commit(struct rev_info *revs,
die("%s is unknown object", name);
}
-static int everybody_uninteresting(struct commit_list *orig,
+static int everybody_uninteresting(struct prio_queue *orig,
struct commit **interesting_cache)
{
- struct commit_list *list = orig;
+ size_t i;
if (*interesting_cache) {
struct commit *commit = *interesting_cache;
@@ -484,9 +484,8 @@ static int everybody_uninteresting(struct commit_list *orig,
return 0;
}
- while (list) {
- struct commit *commit = list->item;
- list = list->next;
+ for (i = 0; i < orig->nr; i++) {
+ struct commit *commit = orig->array[i].data;
if (commit->object.flags & UNINTERESTING)
continue;
@@ -1300,20 +1299,17 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
/* How many extra uninteresting commits we want to see.. */
#define SLOP 5
-static int still_interesting(struct commit_list *src, timestamp_t date, int slop,
+static int still_interesting(struct prio_queue *src, timestamp_t date, int slop,
struct commit **interesting_cache)
{
/*
- * No source list at all? We're definitely done..
+ * Since src is sorted by date, it is enough to peek at the
+ * first entry to compare dates. No entry at all means done.
*/
- if (!src)
+ struct commit *commit = prio_queue_peek(src);
+ if (!commit)
return 0;
-
- /*
- * Does the destination list contain entries with a date
- * before the source list? Definitely _not_ done.
- */
- if (date <= src->item->date)
+ if (date <= commit->date)
return SLOP;
/*
@@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
struct commit_list *newlist = NULL;
struct commit_list **p = &newlist;
struct commit *interesting_cache = NULL;
+ struct prio_queue queue = { .compare = compare_commits_by_commit_date };
if (revs->ancestry_path_implicit_bottoms) {
collect_bottom_commits(original_list,
@@ -1461,6 +1458,11 @@ static int limit_list(struct rev_info *revs)
while (original_list) {
struct commit *commit = pop_commit(&original_list);
+ prio_queue_put(&queue, commit);
+ }
+
+ while (queue.nr) {
+ struct commit *commit = prio_queue_get(&queue);
struct object *obj = &commit->object;
if (commit == interesting_cache)
@@ -1468,11 +1470,13 @@ static int limit_list(struct rev_info *revs)
if (revs->max_age != -1 && (commit->date < revs->max_age))
obj->flags |= UNINTERESTING;
- if (process_parents(revs, commit, &original_list, NULL) < 0)
+ if (process_parents(revs, commit, NULL, &queue) < 0) {
+ clear_prio_queue(&queue);
return -1;
+ }
if (obj->flags & UNINTERESTING) {
mark_parents_uninteresting(revs, commit);
- slop = still_interesting(original_list, date, slop, &interesting_cache);
+ slop = still_interesting(&queue, date, slop, &interesting_cache);
if (slop)
continue;
break;
@@ -1509,7 +1513,7 @@ static int limit_list(struct rev_info *revs)
}
}
- commit_list_free(original_list);
+ clear_prio_queue(&queue);
revs->commits = newlist;
return 0;
}
base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 4/4] grep: prefetch necessary blobs
From: Elijah Newren via GitGitGadget @ 2026-05-14 16:25 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Phillip Wood, Derrick Stolee, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.2089.v3.git.1778775928.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
In partial clones, `git grep` fetches necessary blobs on-demand one
at a time, which can be very slow. In partial clones, add an extra
preliminary walk over the tree similar to grep_tree() which collects
the blobs of interest, and then prefetches them.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++
t/t7810-grep.sh | 58 ++++++++++++++++++++
2 files changed, 201 insertions(+)
diff --git a/builtin/grep.c b/builtin/grep.c
index e33285e5e6..85656d8d3f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -28,9 +28,12 @@
#include "object-file.h"
#include "object-name.h"
#include "odb.h"
+#include "oid-array.h"
+#include "oidset.h"
#include "packfile.h"
#include "pager.h"
#include "path.h"
+#include "promisor-remote.h"
#include "read-cache-ll.h"
#include "write-or-die.h"
@@ -692,6 +695,144 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
return hit;
}
+static void collect_blob_oids_for_tree(struct repository *repo,
+ const struct pathspec *pathspec,
+ struct tree_desc *tree,
+ struct strbuf *base,
+ int tn_len,
+ struct oidset *blob_oids)
+{
+ struct name_entry entry;
+ int old_baselen = base->len;
+ struct strbuf name = STRBUF_INIT;
+ enum interesting match = entry_not_interesting;
+
+ while (tree_entry(tree, &entry)) {
+ if (match != all_entries_interesting) {
+ strbuf_addstr(&name, base->buf + tn_len);
+ match = tree_entry_interesting(repo->index,
+ &entry, &name,
+ pathspec);
+ strbuf_reset(&name);
+
+ if (match == all_entries_not_interesting)
+ break;
+ if (match == entry_not_interesting)
+ continue;
+ }
+
+ strbuf_add(base, entry.path, tree_entry_len(&entry));
+
+ if (S_ISREG(entry.mode)) {
+ if (!odb_has_object(repo->objects, &entry.oid, 0))
+ oidset_insert(blob_oids, &entry.oid);
+ } else if (S_ISDIR(entry.mode)) {
+ enum object_type type;
+ struct tree_desc sub_tree;
+ void *data;
+ unsigned long size;
+
+ data = odb_read_object(repo->objects, &entry.oid,
+ &type, &size);
+ if (!data)
+ die(_("unable to read tree (%s)"),
+ oid_to_hex(&entry.oid));
+
+ strbuf_addch(base, '/');
+ init_tree_desc(&sub_tree, &entry.oid, data, size);
+ collect_blob_oids_for_tree(repo, pathspec, &sub_tree,
+ base, tn_len, blob_oids);
+ free(data);
+ }
+ /*
+ * ...no else clause for S_ISGITLINK: submodules have their
+ * own promisor configuration and would need separate fetches
+ * anyway.
+ */
+
+ strbuf_setlen(base, old_baselen);
+ }
+
+ strbuf_release(&name);
+}
+
+static void collect_blob_oids_for_treeish(struct grep_opt *opt,
+ const struct pathspec *pathspec,
+ const struct object_id *tree_ish_oid,
+ const char *name,
+ struct oidset *blob_oids)
+{
+ struct tree_desc tree;
+ void *data;
+ unsigned long size;
+ struct strbuf base = STRBUF_INIT;
+ int len;
+
+ data = odb_read_object_peeled(opt->repo->objects, tree_ish_oid,
+ OBJ_TREE, &size, NULL);
+
+ if (!data)
+ return;
+
+ len = name ? strlen(name) : 0;
+ if (len) {
+ strbuf_add(&base, name, len);
+ strbuf_addch(&base, ':');
+ }
+ init_tree_desc(&tree, tree_ish_oid, data, size);
+
+ collect_blob_oids_for_tree(opt->repo, pathspec, &tree,
+ &base, base.len, blob_oids);
+
+ strbuf_release(&base);
+ free(data);
+}
+
+static void prefetch_grep_blobs(struct grep_opt *opt,
+ const struct pathspec *pathspec,
+ const struct object_array *list)
+{
+ struct oidset blob_oids = OIDSET_INIT;
+
+ /* Exit if we're not in a partial clone */
+ if (!repo_has_promisor_remote(opt->repo))
+ return;
+
+ /* For each tree, gather the blobs in it */
+ for (int i = 0; i < list->nr; i++) {
+ struct object *real_obj;
+
+ obj_read_lock();
+ real_obj = deref_tag(opt->repo, list->objects[i].item,
+ NULL, 0);
+ obj_read_unlock();
+
+ if (real_obj &&
+ (real_obj->type == OBJ_COMMIT ||
+ real_obj->type == OBJ_TREE))
+ collect_blob_oids_for_treeish(opt, pathspec,
+ &real_obj->oid,
+ list->objects[i].name,
+ &blob_oids);
+ }
+
+ /* Prefetch the blobs we found */
+ if (oidset_size(&blob_oids)) {
+ struct oid_array to_fetch = OID_ARRAY_INIT;
+ struct oidset_iter iter;
+ const struct object_id *oid;
+
+ oidset_iter_init(&blob_oids, &iter);
+ while ((oid = oidset_iter_next(&iter)))
+ oid_array_append(&to_fetch, oid);
+
+ promisor_remote_get_direct(opt->repo, to_fetch.oid, to_fetch.nr);
+
+ oid_array_clear(&to_fetch);
+ }
+ oidset_clear(&blob_oids);
+}
+
static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
struct object *obj, const char *name, const char *path)
{
@@ -732,6 +873,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
int hit = 0;
const unsigned int nr = list->nr;
+ prefetch_grep_blobs(opt, pathspec, list);
+
for (i = 0; i < nr; i++) {
struct object *real_obj;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 64ac4f04ee..3d08fd2a0c 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1929,4 +1929,62 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '
test_cmp expected actual
'
+test_expect_success 'grep of revision in partial clone batches prefetch and honors pathspec' '
+ test_when_finished "rm -rf grep-partial-src grep-partial" &&
+
+ git init grep-partial-src &&
+ (
+ cd grep-partial-src &&
+ git config uploadpack.allowfilter 1 &&
+ git config uploadpack.allowanysha1inwant 1 &&
+ mkdir a b &&
+ echo "needle in haystack" >a/matches.txt &&
+ echo "nothing to see here" >a/nomatch.txt &&
+ echo "needle again" >b/matches.md &&
+ git add . &&
+ git commit -m "initial"
+ ) &&
+
+ git clone --no-checkout --filter=blob:none \
+ "file://$(pwd)/grep-partial-src" grep-partial &&
+
+ # All three blobs are missing immediately after a blobless clone.
+ git -C grep-partial rev-list --quiet --objects \
+ --missing=print HEAD >missing &&
+ test_line_count = 3 missing &&
+
+ # A pathspec-limited grep should prefetch only the two blobs
+ # in a/. It should fetch both blobs in one batched request.
+ GIT_TRACE2_EVENT="$(pwd)/grep-trace-pathspec" \
+ git -C grep-partial grep -c "needle" HEAD -- "a/*.txt" >result &&
+
+ # Only a/matches.txt contains "needle" among the matched paths.
+ test_line_count = 1 result &&
+
+ # Exactly the two a/*.txt blobs should have been requested, and
+ # the server packed those two objects in the response.
+ test_trace2_data promisor fetch_count 2 <grep-trace-pathspec &&
+ test_trace2_data pack-objects written 2 <grep-trace-pathspec &&
+
+ # b/matches.md should still be missing locally.
+ git -C grep-partial rev-list --quiet --objects \
+ --missing=print HEAD >missing &&
+ test_line_count = 1 missing &&
+
+ # A second grep without a pathspec must recurse into both
+ # subdirectories, but should request only the still-missing blob
+ # from the promisor.
+ GIT_TRACE2_EVENT="$(pwd)/grep-trace-all" \
+ git -C grep-partial grep -c "needle" HEAD >result &&
+
+ test_line_count = 2 result &&
+ test_trace2_data promisor fetch_count 1 <grep-trace-all &&
+ test_trace2_data pack-objects written 1 <grep-trace-all &&
+
+ # Everything is local now.
+ git -C grep-partial rev-list --quiet --objects \
+ --missing=print HEAD >missing &&
+ test_line_count = 0 missing
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 3/4] builtin/log: prefetch necessary blobs for `git cherry`
From: Elijah Newren via GitGitGadget @ 2026-05-14 16:25 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Phillip Wood, Derrick Stolee, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.2089.v3.git.1778775928.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
In partial clones, `git cherry` fetches necessary blobs on-demand one
at a time, which can be very slow. We would like to prefetch all
necessary blobs upfront. To do so, we need to be able to first figure
out which blobs are needed.
`git cherry` does its work in a two-phase approach: first computing
header-only IDs (based on file paths and modes), then falling back to
full content-based IDs only when header-only IDs collide -- or, more
accurately, whenever the oidhash() of the header-only object_ids
collide.
patch-ids.c handles this by creating an ids->patches hashmap that has
all the data we need, but the problem is that any attempt to query the
hashmap will invoke the patch_id_neq() function on any colliding objects,
which causes the on-demand fetching.
Insert a new prefetch_cherry_blobs() function before checking for
collisions. Use a temporary replacement on the ids->patches.cmpfn
in order to enumerate the blobs that would be needed without yet
fetching them, and then fetch them all at once, then restore the old
ids->patches.cmpfn.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
builtin/log.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++
t/t3500-cherry.sh | 27 ++++++++++
2 files changed, 158 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index 8c0939dd42..e464b30af4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -21,10 +21,12 @@
#include "color.h"
#include "commit.h"
#include "diff.h"
+#include "diffcore.h"
#include "diff-merges.h"
#include "revision.h"
#include "log-tree.h"
#include "oid-array.h"
+#include "oidset.h"
#include "tag.h"
#include "reflog-walk.h"
#include "patch-ids.h"
@@ -43,9 +45,11 @@
#include "utf8.h"
#include "commit-reach.h"
+#include "promisor-remote.h"
#include "range-diff.h"
#include "tmp-objdir.h"
#include "tree.h"
+#include "userdiff.h"
#include "write-or-die.h"
#define MAIL_DEFAULT_WRAP 72
@@ -2602,6 +2606,131 @@ static void print_commit(char sign, struct commit *commit, int verbose,
}
}
+/*
+ * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.
+ * Skips files whose userdiff driver explicitly declares binary status
+ * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and
+ * never reads blob content. Use userdiff_find_by_path() since
+ * diff_filespec_load_driver() is static in diff.c.
+ *
+ * Clean up with diff_queue_clear() (from diffcore.h).
+ */
+static void collect_diff_blob_oids(struct commit *commit,
+ struct diff_options *opts,
+ struct oidset *blobs)
+{
+ struct diff_queue_struct *q;
+
+ /*
+ * Merge commits are filtered out by patch_id_defined() in patch-ids.c,
+ * so we'll never be called with one.
+ */
+ assert(!commit->parents || !commit->parents->next);
+
+ if (commit->parents)
+ diff_tree_oid(&commit->parents->item->object.oid,
+ &commit->object.oid, "", opts);
+ else
+ diff_root_tree_oid(&commit->object.oid, "", opts);
+ diffcore_std(opts);
+
+ q = &diff_queued_diff;
+ for (int i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ struct userdiff_driver *drv;
+
+ /* Skip binary files */
+ drv = userdiff_find_by_path(opts->repo->index, p->one->path);
+ if (drv && drv->binary > 0)
+ continue;
+
+ if (DIFF_FILE_VALID(p->one) &&
+ odb_read_object_info_extended(opts->repo->objects,
+ &p->one->oid, NULL,
+ OBJECT_INFO_FOR_PREFETCH))
+ oidset_insert(blobs, &p->one->oid);
+ if (DIFF_FILE_VALID(p->two) &&
+ odb_read_object_info_extended(opts->repo->objects,
+ &p->two->oid, NULL,
+ OBJECT_INFO_FOR_PREFETCH))
+ oidset_insert(blobs, &p->two->oid);
+ }
+ diff_queue_clear(q);
+}
+
+static int always_match(const void *cmp_data UNUSED,
+ const struct hashmap_entry *entry1 UNUSED,
+ const struct hashmap_entry *entry2 UNUSED,
+ const void *keydata UNUSED)
+{
+ return 0;
+}
+
+/*
+ * Prefetch blobs for git cherry in partial clones.
+ *
+ * Called between the revision walk (which builds the head-side
+ * commit list) and the has_commit_patch_id() comparison loop.
+ *
+ * Uses a cmpfn-swap trick to avoid reading blobs: temporarily
+ * replaces the hashmap's comparison function with a trivial
+ * always-match function, so hashmap_get()/hashmap_get_next() match
+ * any entry with the same oidhash bucket. These are the set of oids
+ * that would trigger patch_id_neq() during normal lookup and cause
+ * blobs to be read on demand, and we want to prefetch them all at
+ * once instead.
+ */
+static void prefetch_cherry_blobs(struct repository *repo,
+ struct commit_list *list,
+ struct patch_ids *ids)
+{
+ struct oidset blobs = OIDSET_INIT;
+ hashmap_cmp_fn original_cmpfn;
+
+ /* Exit if we're not in a partial clone */
+ if (!repo_has_promisor_remote(repo))
+ return;
+
+ /* Save original cmpfn, replace with always_match */
+ original_cmpfn = ids->patches.cmpfn;
+ ids->patches.cmpfn = always_match;
+
+ /* Find header-only collisions, gather blobs from those commits */
+ for (struct commit_list *l = list; l; l = l->next) {
+ struct commit *c = l->item;
+ bool match_found = false;
+ for (struct patch_id *cur = patch_id_iter_first(c, ids);
+ cur;
+ cur = patch_id_iter_next(cur, ids)) {
+ match_found = true;
+ collect_diff_blob_oids(cur->commit, &ids->diffopts,
+ &blobs);
+ }
+ if (match_found)
+ collect_diff_blob_oids(c, &ids->diffopts, &blobs);
+ }
+
+ /* Restore original cmpfn */
+ ids->patches.cmpfn = original_cmpfn;
+
+ /* If we have any blobs to fetch, fetch them */
+ if (oidset_size(&blobs)) {
+ struct oid_array to_fetch = OID_ARRAY_INIT;
+ struct oidset_iter iter;
+ const struct object_id *oid;
+
+ oidset_iter_init(&blobs, &iter);
+ while ((oid = oidset_iter_next(&iter)))
+ oid_array_append(&to_fetch, oid);
+
+ promisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);
+
+ oid_array_clear(&to_fetch);
+ }
+
+ oidset_clear(&blobs);
+}
+
int cmd_cherry(int argc,
const char **argv,
const char *prefix,
@@ -2673,6 +2802,8 @@ int cmd_cherry(int argc,
commit_list_insert(commit, &list);
}
+ prefetch_cherry_blobs(the_repository, list, &ids);
+
for (struct commit_list *l = list; l; l = l->next) {
char sign = '+';
diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
index 78c3eac54b..3e66827d76 100755
--- a/t/t3500-cherry.sh
+++ b/t/t3500-cherry.sh
@@ -78,4 +78,31 @@ test_expect_success 'cherry ignores whitespace' '
test_cmp expect actual
'
+# Reuse the expect file from the previous test, in a partial clone
+test_expect_success 'cherry in partial clone does bulk prefetch' '
+ test_config uploadpack.allowfilter 1 &&
+ test_config uploadpack.allowanysha1inwant 1 &&
+ test_when_finished "rm -rf copy" &&
+
+ git clone --bare --filter=blob:none file://"$(pwd)" copy &&
+ (
+ cd copy &&
+ GIT_TRACE2_EVENT="$(pwd)/trace.output" git cherry upstream-with-space feature-without-space >actual &&
+ test_cmp ../expect actual &&
+
+ grep "child_start.*fetch.negotiationAlgorithm" trace.output >fetches &&
+ test_line_count = 1 fetches &&
+ test_trace2_data promisor fetch_count 4 <trace.output &&
+
+ # A second invocation should not refetch any blobs, since
+ # the prefetch is expected to filter out OIDs that are
+ # already present locally.
+ GIT_TRACE2_EVENT="$(pwd)/trace2.output" git cherry upstream-with-space feature-without-space >actual &&
+ test_cmp ../expect actual &&
+
+ ! grep "child_start.*fetch.negotiationAlgorithm" trace2.output &&
+ ! grep "\"key\":\"fetch_count\"" trace2.output
+ )
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/4] patch-ids.h: add missing trailing parenthesis in documentation comment
From: Elijah Newren via GitGitGadget @ 2026-05-14 16:25 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Phillip Wood, Derrick Stolee, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.2089.v3.git.1778775928.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
patch-ids.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/patch-ids.h b/patch-ids.h
index 490d739371..57534ee722 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -37,7 +37,7 @@ int has_commit_patch_id(struct commit *commit, struct patch_ids *);
* struct patch_id *cur;
* for (cur = patch_id_iter_first(commit, ids);
* cur;
- * cur = patch_id_iter_next(cur, ids) {
+ * cur = patch_id_iter_next(cur, ids)) {
* ... look at cur->commit
* }
*/
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 1/4] promisor-remote: document caller filtering contract
From: Elijah Newren via GitGitGadget @ 2026-05-14 16:25 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Phillip Wood, Derrick Stolee, Elijah Newren,
Elijah Newren
In-Reply-To: <pull.2089.v3.git.1778775928.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
promisor_remote_get_direct() does not, on its happy path, filter out
OIDs that are already present in the local object store: every OID
the caller supplies is written to the fetch subprocess's stdin and
ends up in the response pack. The only filtering it performs is in
remove_fetched_oids(), and that only runs after a fetch failure when
falling back to a different configured promisor remote.
Almost every existing caller already filters locally-present OIDs out
itself (typically with odb_read_object_info_extended() and
OBJECT_INFO_FOR_PREFETCH, or odb_has_object() with no fetch flag). But
the existing API comment does not state this expectation, so a new
caller is easy to write incorrectly (I missed this originally and wrote
two problematic callers). Omitting the filter still "works" in the
sense that the desired objects end up local, but it silently makes the
fetch request -- and the response pack -- larger than necessary,
defeating part of the point of batching.
Spell the contract out so future callers know to filter (and
deduplicate) themselves, and point them at the helpers they should
use to check local presence without accidentally triggering a lazy
fetch.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
promisor-remote.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/promisor-remote.h b/promisor-remote.h
index 3d4d2de018..301f5ac5cb 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -29,6 +29,17 @@ int repo_has_promisor_remote(struct repository *r);
* Fetches all requested objects from all promisor remotes, trying them one at
* a time until all objects are fetched.
*
+ * Callers are responsible for filtering out OIDs that are already present
+ * locally before calling this function: every supplied OID is sent in the
+ * fetch request, even if the object already exists in the local object
+ * store. (Only after a fetch failure does this function fall back to
+ * stripping already-present OIDs from the list before trying the next
+ * configured promisor remote.) Callers should also deduplicate the OIDs.
+ *
+ * To test for local presence without triggering a lazy fetch (which would
+ * defeat the purpose of batching), use odb_has_object(..., 0) or
+ * odb_read_object_info_extended() with OBJECT_INFO_FOR_PREFETCH.
+ *
* If oid_nr is 0, this function returns immediately.
*/
void promisor_remote_get_direct(struct repository *repo,
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 0/4] Batch prefetching
From: Elijah Newren via GitGitGadget @ 2026-05-14 16:25 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Phillip Wood, Derrick Stolee, Elijah Newren
In-Reply-To: <pull.2089.v2.git.1776472347.gitgitgadget@gmail.com>
Changes since v2:
* Modified the final patch as suggested by Stolee to include pathspec usage
in the testcase
* Modified the last two patches to not re-download blobs we already have
locally, and adjusted the tests to verify
* Inserted a new first patch, containing a documentation addition that
would have helped me avoid making the above mistake in the first place.
Note: Stolee also suggest some code sharing or code movement in his review
of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch
isn't about a diff) and it's not clear to me what could be shared or moved,
so that's not part of this round.
Changes since v1:
* Remove stray file that should have never been added. So embarrassing that
I didn't catch that before submitting.
Original cover-letter:
======================
Partial clones provide a trade-off for users: avoid downloading blobs
upfront, at the expense of needing to download them later as they run other
commands. This tradeoff can sometimes incur a more severe cost than
expected, particularly if needed blobs are discovered as they are accessed,
resulting in downloading blobs one at a time. Some commands like checkout,
diff, and merge do batch prefetches of necessary blobs, since that can
dramatically reduce the pain of on-demand loading. Extend this ability to
two more commands: cherry and grep.
This series was spurred by a report where git cherry jobs were each doing
hundreds of single-blob fetches, at a cost of 3s each. Batching those
downloads should dramatically speed up their jobs. (And I decided to fix up
git grep similarly while at it.)
I'll also note that git backfill with revisions and/or pathspecs could also
improve things for these users, but since backfill is a manual command users
would have to run and requires users to try to figure out which data is
needed (a challenge in the case of cherry), it still makes sense to provide
smarter behavior for folks who don't choose to manually run backfill.
Also, correct a documentation typo I noticed in patch-ids.h (related to code
I was using for the git cherry fixes) as a preparatory fixup.
Elijah Newren (4):
promisor-remote: document caller filtering contract
patch-ids.h: add missing trailing parenthesis in documentation comment
builtin/log: prefetch necessary blobs for `git cherry`
grep: prefetch necessary blobs
builtin/grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++
builtin/log.c | 131 ++++++++++++++++++++++++++++++++++++++++++
patch-ids.h | 2 +-
promisor-remote.h | 11 ++++
t/t3500-cherry.sh | 27 +++++++++
t/t7810-grep.sh | 58 +++++++++++++++++++
6 files changed, 371 insertions(+), 1 deletion(-)
base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2089%2Fnewren%2Fbatch-prefetching-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2089/newren/batch-prefetching-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2089
Range-diff vs v2:
-: ---------- > 1: 6ad11e2c28 promisor-remote: document caller filtering contract
1: 663816a344 = 2: 08a2c6517b patch-ids.h: add missing trailing parenthesis in documentation comment
2: a705852723 ! 3: c0655e5d41 builtin/log: prefetch necessary blobs for `git cherry`
@@ builtin/log.c: static void print_commit(char sign, struct commit *commit, int ve
+ if (drv && drv->binary > 0)
+ continue;
+
-+ if (DIFF_FILE_VALID(p->one))
++ if (DIFF_FILE_VALID(p->one) &&
++ odb_read_object_info_extended(opts->repo->objects,
++ &p->one->oid, NULL,
++ OBJECT_INFO_FOR_PREFETCH))
+ oidset_insert(blobs, &p->one->oid);
-+ if (DIFF_FILE_VALID(p->two))
++ if (DIFF_FILE_VALID(p->two) &&
++ odb_read_object_info_extended(opts->repo->objects,
++ &p->two->oid, NULL,
++ OBJECT_INFO_FOR_PREFETCH))
+ oidset_insert(blobs, &p->two->oid);
+ }
+ diff_queue_clear(q);
@@ t/t3500-cherry.sh: test_expect_success 'cherry ignores whitespace' '
+
+ grep "child_start.*fetch.negotiationAlgorithm" trace.output >fetches &&
+ test_line_count = 1 fetches &&
-+ test_trace2_data promisor fetch_count 4 <trace.output
++ test_trace2_data promisor fetch_count 4 <trace.output &&
++
++ # A second invocation should not refetch any blobs, since
++ # the prefetch is expected to filter out OIDs that are
++ # already present locally.
++ GIT_TRACE2_EVENT="$(pwd)/trace2.output" git cherry upstream-with-space feature-without-space >actual &&
++ test_cmp ../expect actual &&
++
++ ! grep "child_start.*fetch.negotiationAlgorithm" trace2.output &&
++ ! grep "\"key\":\"fetch_count\"" trace2.output
+ )
+'
+
3: 8fbfe69bc4 ! 4: 75d4ca7cff grep: prefetch necessary blobs
@@ builtin/grep.c: static int grep_tree(struct grep_opt *opt, const struct pathspec
+ strbuf_add(base, entry.path, tree_entry_len(&entry));
+
+ if (S_ISREG(entry.mode)) {
-+ oidset_insert(blob_oids, &entry.oid);
++ if (!odb_has_object(repo->objects, &entry.oid, 0))
++ oidset_insert(blob_oids, &entry.oid);
+ } else if (S_ISDIR(entry.mode)) {
+ enum object_type type;
+ struct tree_desc sub_tree;
@@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch
test_cmp expected actual
'
-+test_expect_success 'grep of revision in partial clone does bulk prefetch' '
++test_expect_success 'grep of revision in partial clone batches prefetch and honors pathspec' '
+ test_when_finished "rm -rf grep-partial-src grep-partial" &&
+
+ git init grep-partial-src &&
@@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch
+ cd grep-partial-src &&
+ git config uploadpack.allowfilter 1 &&
+ git config uploadpack.allowanysha1inwant 1 &&
-+ echo "needle in haystack" >searchme &&
-+ echo "no match here" >other &&
-+ mkdir subdir &&
-+ echo "needle again" >subdir/deep &&
++ mkdir a b &&
++ echo "needle in haystack" >a/matches.txt &&
++ echo "nothing to see here" >a/nomatch.txt &&
++ echo "needle again" >b/matches.md &&
+ git add . &&
+ git commit -m "initial"
+ ) &&
@@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch
+ git clone --no-checkout --filter=blob:none \
+ "file://$(pwd)/grep-partial-src" grep-partial &&
+
-+ # All blobs should be missing after a blobless clone.
++ # All three blobs are missing immediately after a blobless clone.
+ git -C grep-partial rev-list --quiet --objects \
+ --missing=print HEAD >missing &&
+ test_line_count = 3 missing &&
+
-+ # grep HEAD should batch-prefetch all blobs in one request.
-+ GIT_TRACE2_EVENT="$(pwd)/grep-trace" \
++ # A pathspec-limited grep should prefetch only the two blobs
++ # in a/. It should fetch both blobs in one batched request.
++ GIT_TRACE2_EVENT="$(pwd)/grep-trace-pathspec" \
++ git -C grep-partial grep -c "needle" HEAD -- "a/*.txt" >result &&
++
++ # Only a/matches.txt contains "needle" among the matched paths.
++ test_line_count = 1 result &&
++
++ # Exactly the two a/*.txt blobs should have been requested, and
++ # the server packed those two objects in the response.
++ test_trace2_data promisor fetch_count 2 <grep-trace-pathspec &&
++ test_trace2_data pack-objects written 2 <grep-trace-pathspec &&
++
++ # b/matches.md should still be missing locally.
++ git -C grep-partial rev-list --quiet --objects \
++ --missing=print HEAD >missing &&
++ test_line_count = 1 missing &&
++
++ # A second grep without a pathspec must recurse into both
++ # subdirectories, but should request only the still-missing blob
++ # from the promisor.
++ GIT_TRACE2_EVENT="$(pwd)/grep-trace-all" \
+ git -C grep-partial grep -c "needle" HEAD >result &&
+
-+ # Should find matches in two files.
+ test_line_count = 2 result &&
++ test_trace2_data promisor fetch_count 1 <grep-trace-all &&
++ test_trace2_data pack-objects written 1 <grep-trace-all &&
+
-+ # Should have prefetched all 3 objects at once
-+ test_trace2_data promisor fetch_count 3 <grep-trace
++ # Everything is local now.
++ git -C grep-partial rev-list --quiet --objects \
++ --missing=print HEAD >missing &&
++ test_line_count = 0 missing
+'
+
test_done
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v3 3/4] approxidate: make "specials" respect fixed day-of-month
From: Junio C Hamano @ 2026-05-14 16:06 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <20260514115520.6660-4-taahol@utu.fi>
Tuomas Ahola <taahol@utu.fi> writes:
> The special approxidate time formats, "noon" and "tea" differ from
> "12pm" and "5pm" by having the feature of wrapping to the previous day
> if the current time is before those hours:
>
> now -> 2026-05-13 11:00:00 +0000
>
> 12pm -> 2026-05-13 12:00:00 +0000
> 5pm -> 2026-05-13 17:00:00 +0000
>
> noon -> 2026-05-12 12:00:00 +0000
> tea -> 2026-05-12 17:00:00 +0000
>
> However, that logic carries too far. Even when the date is specified,
> the behavior of the "specials" depends on the current time. Assuming
> the same time as above, we get:
>
> today at noon -> 2026-05-12 12:00:00 +0000 (should be 13 May)
> 13 May at tea -> 2026-05-12 17:00:00 +0000
>
> or, using an example mentioned in date-formats.adoc:
>
> last Friday at noon -> 2026-05-07 12:00:00 +0000 (should be 8 May)
>
> The quirk seems to be rather old. Already in 2006, Linus Torvalds
> remarked that the date yielded by "one year ago yesterday at tea-time"
> was "just silly and not even correct". Indeed, even today it gives:
>
> One year ago yesterday at tea-time -> 2025-05-11 17:00:00 +0000
> (should be 12 May)
>
> Let's fix all of those with a simple patch. Check whether we already
> have a specified day-of-month in `tm->tm_mday` and make `date_time()`
> stick to it. Ensure the correct behavior with relevant tests.
I find this vastly easier to follow the reasoning, compared to the
previous iteration. Very nicely done.
>
> Links:
> 1. https://lore.kernel.org/git/Pine.LNX.4.64.0610101102560.3952@g5.osdl.org/
>
> Signed-off-by: Tuomas Ahola <taahol@utu.fi>
> ---
>
> Notes:
> > Again, this may be just me, but I happen to find the version of
> > comment in Peff's review on the earlier iteration of this series
> > much easier to understand.
> >
>
> Thanks, applied.
>
> date.c | 6 +++++-
> t/t0006-date.sh | 4 ++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/date.c b/date.c
> index 412aca6dc4..73879d202c 100644
> --- a/date.c
> +++ b/date.c
> @@ -1132,7 +1132,11 @@ static void date_yesterday(struct tm *tm, struct tm *now, int *num)
>
> static void date_time(struct tm *tm, struct tm *now, int hour)
> {
> - if (tm->tm_hour < hour)
> + /*
> + * If we do not yet have a specified day, we'll use the most recent
> + * version of "hour" relative to now. But that may be yesterday.
> + */
> + if (tm->tm_mday < 0 && tm->tm_hour < hour)
> update_tm(tm, now, 24*60*60);
> tm->tm_hour = hour;
> tm->tm_min = 0;
> diff --git a/t/t0006-date.sh b/t/t0006-date.sh
> index c7667bade2..d800cb30fe 100755
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -208,8 +208,12 @@ check_approxidate '6pm yesterday' '2009-08-29 18:00:00'
> check_approxidate '3:00' '2009-08-30 03:00:00'
> check_approxidate '15:00' '2009-08-30 15:00:00'
> check_approxidate 'noon today' '2009-08-30 12:00:00'
> +check_approxidate 'today at noon' '2009-08-30 12:00:00' '-12 hours'
> check_approxidate 'noon yesterday' '2009-08-29 12:00:00'
> +check_approxidate 'last Friday at noon' '2009-08-28 12:00:00'
> +check_approxidate 'last Friday at noon' '2009-08-28 12:00:00' '-12 hours'
> check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00'
> +check_approxidate 'January 5th noon pm' '2009-01-05 12:00:00' '-12 hours'
> check_approxidate '10am noon' '2009-08-29 12:00:00'
> check_approxidate 'January 5th yesterday' '2009-01-29 19:20:00'
> check_approxidate 'January 5th yesterday' '2008-12-31 19:20:00' '+2 days'
^ permalink raw reply
* [PATCH 3/3] daemon: guard NULL REMOTE_PORT in execute() logging
From: Sebastien Tardif via GitGitGadget @ 2026-05-14 15:46 UTC (permalink / raw)
To: git; +Cc: Sebastien Tardif, Sebastien Tardif
In-Reply-To: <pull.2300.git.git.1778773592.gitgitgadget@gmail.com>
From: Sebastien Tardif <sebtardif@ncf.ca>
The REMOTE_PORT environment variable is used in a format string
without a NULL check, while REMOTE_ADDR is checked. If REMOTE_PORT
is unset, NULL is passed to printf's %s, which is undefined behavior.
Add a fallback string for the NULL case.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
daemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/daemon.c b/daemon.c
index 103c08d868..78cca8673f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -753,7 +753,7 @@ static int execute(void)
struct strvec env = STRVEC_INIT;
if (addr)
- loginfo("Connection from %s:%s", addr, port);
+ loginfo("Connection from %s:%s", addr, port ? port : "?");
set_keep_alive(0);
alarm(init_timeout ? init_timeout : timeout);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] daemon: fix IPv6 address truncation in ip2str()
From: Sebastien Tardif via GitGitGadget @ 2026-05-14 15:46 UTC (permalink / raw)
To: git; +Cc: Sebastien Tardif, Sebastien Tardif
In-Reply-To: <pull.2300.git.git.1778773592.gitgitgadget@gmail.com>
From: Sebastien Tardif <sebtardif@ncf.ca>
The sockaddr struct size (ai_addrlen) is passed as the output buffer
size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but
INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated.
Fix this by passing sizeof(ip) instead, which is the actual size of
the destination buffer. Drop the now-unused len parameter from
ip2str() and update all callers.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
daemon.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/daemon.c b/daemon.c
index 80fa0226d8..103c08d868 100644
--- a/daemon.c
+++ b/daemon.c
@@ -947,7 +947,7 @@ struct socketlist {
size_t alloc;
};
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+static const char *ip2str(int family, struct sockaddr *sin)
{
#ifdef NO_IPV6
static char ip[INET_ADDRSTRLEN];
@@ -958,11 +958,11 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
switch (family) {
#ifndef NO_IPV6
case AF_INET6:
- inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+ inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, sizeof(ip));
break;
#endif
case AF_INET:
- inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+ inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, sizeof(ip));
break;
default:
xsnprintf(ip, sizeof(ip), "<unknown>");
@@ -1019,14 +1019,14 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
logerror("Could not bind to %s: %s",
- ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+ ip2str(ai->ai_family, ai->ai_addr),
strerror(errno));
close(sockfd);
continue; /* not fatal */
}
if (listen(sockfd, 5) < 0) {
logerror("Could not listen to %s: %s",
- ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+ ip2str(ai->ai_family, ai->ai_addr),
strerror(errno));
close(sockfd);
continue; /* not fatal */
@@ -1080,7 +1080,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
logerror("Could not bind to %s: %s",
- ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+ ip2str(AF_INET, (struct sockaddr *)&sin),
strerror(errno));
close(sockfd);
return 0;
@@ -1088,7 +1088,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
if (listen(sockfd, 5) < 0) {
logerror("Could not listen to %s: %s",
- ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+ ip2str(AF_INET, (struct sockaddr *)&sin),
strerror(errno));
close(sockfd);
return 0;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/3] daemon: fix IPv6 address corruption in lookup_hostname()
From: Sebastien Tardif via GitGitGadget @ 2026-05-14 15:46 UTC (permalink / raw)
To: git; +Cc: Sebastien Tardif, Sebastien Tardif
In-Reply-To: <pull.2300.git.git.1778773592.gitgitgadget@gmail.com>
From: Sebastien Tardif <sebtardif@ncf.ca>
getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6
results. However, the code unconditionally casts ai_addr to
sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts,
this reads from the wrong struct offset, producing garbage IP
addresses.
Fix this by checking ai_family and extracting the address pointer
into a local variable before calling inet_ntop() once with the
correct family. Die on unexpected address families.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
daemon.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/daemon.c b/daemon.c
index 0a7b1aae44..80fa0226d8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -674,9 +674,20 @@ static void lookup_hostname(struct hostinfo *hi)
gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
if (!gai) {
- struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+ void *addr;
+
+ if (ai->ai_family == AF_INET) {
+ struct sockaddr_in *sa = (void *)ai->ai_addr;
+ addr = &sa->sin_addr;
+ } else if (ai->ai_family == AF_INET6) {
+ struct sockaddr_in6 *sa6 = (void *)ai->ai_addr;
+ addr = &sa6->sin6_addr;
+ } else {
+ die("unexpected address family: %d",
+ ai->ai_family);
+ }
- inet_ntop(AF_INET, &sin_addr->sin_addr,
+ inet_ntop(ai->ai_family, addr,
addrbuf, sizeof(addrbuf));
strbuf_addstr(&hi->ip_address, addrbuf);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/3] daemon: fix network address handling bugs
From: Sebastien Tardif via GitGitGadget @ 2026-05-14 15:46 UTC (permalink / raw)
To: git; +Cc: Sebastien Tardif
Fix three related issues in daemon.c's network address handling:
IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with
AF_UNSPEC hints, so it may return IPv6 results. However, the code
unconditionally casts ai_addr to sockaddr_in and passes AF_INET to
inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset,
producing garbage IP addresses. Fixed by checking ai_family and handling
both AF_INET and AF_INET6.
IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen)
is passed as the output buffer size to inet_ntop(). For IPv6,
sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6
addresses are silently truncated. Fixed by passing sizeof(ip) instead, and
dropping the now-unused len parameter.
NULL pointer in execute() logging: REMOTE_PORT environment variable is used
in a format string without a NULL check (only REMOTE_ADDR was checked). If
REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined
behavior. Fixed by using a fallback string.
Sebastien Tardif (3):
daemon: fix IPv6 address corruption in lookup_hostname()
daemon: fix IPv6 address truncation in ip2str()
daemon: guard NULL REMOTE_PORT in execute() logging
daemon.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2300%2FSebTardif%2Ffix%2Fdaemon-ipv6-and-null-port-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2300/SebTardif/fix/daemon-ipv6-and-null-port-v1
Pull-Request: https://github.com/git/git/pull/2300
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] git-jump: pick a mode automatically when invoked without arguments
From: Erik Cervin Edin @ 2026-05-14 15:40 UTC (permalink / raw)
To: git
In-Reply-To: <20260508175240.GA737125@coredump.intra.peff.net>
On 26/05/08 04:30PM, Greg Hurrell wrote:
> Hopefully, they at least read the README before installing it from contrib/
> (although Homebrew recently starting installing it for folks automatically,
> so may not remain true for much longer on macOS...)
Oh! I didn't know that but looking closer on my homebrew installed Git,
looks like you're right.
I would've assumed that if users had jumped through the hoops of
installing git-jump from contrib/ it stands to reason they would have
some sense of what it does. But if this is landing as a part of regular
brew install git, I'd wager there's a few unsuspecting people running
git-jump that don't know what it does.
> Would having "git jump auto" work for you?
Imo, this sounds ideal -- there's something odd about `git jump` picking
the subcommand heuristically. At least when I invoke git-jump I always
do so with a specific intent of _where_ I want to jump. Then again, I
never do a jump merge or a jump ws.
> Homebrew installing `git-jump` by default a few months ago[^2] broke
> this, because aliases can't shadow builtin commands.
But it looks like this doesn't work in this case. Even if you have
git-jump installed stand-alone (mine is in ~/bin/git-jump, in PATH,
before /opt/homebrew/bin.)
FWIW I alias jump to j, jump diff to jd and jump grep to jg. E.g.
git jd # git jump diff
On 26/05/08 09:07AM, Greg Hurrell via GitGitGadget wrote:
> -usage: git jump [--stdout] <mode> [<args>]
> +usage: git jump [--stdout] [<mode>] [<args>]
The usage message makes <mode> optional but doesn't explain what
happens when you omit it. Seems worth documenting the auto-detect behavior
there too.
> But there are two situations where we can usefully infer the most
> valuable and likely mode that a user would want to use, and select it
> automatically when they run `git jump` without arguments:
>
> 1. When there are unmerged paths in the index, the user likely
> wants `git jump merge`.
>
> 2. When the working tree has unstaged changes, the user likely
> wants `git jump diff`.
I can think of a third situation -- when there are staged changes flagged by
git diff --cached --check.
If we're going to teach git-jump how to be more clever about where to jump,
does it also make sense to bake `git jump ws` into this?
Also, if this is going to grow into a proper auto-detect heuristic, it
might be cleaner as a first-class mode rather than logic spliced into the
argument parser. Something like:
mode_auto() {
if test -n "$(git ls-files -u)"; then
mode_merge "$@"
elif ! git diff --quiet; then
mode_diff "$@"
elif ! git diff --cached --check >/dev/null 2>&1; then
mode_ws --cached "$@"
else
return 0
fi
}
That way `git jump auto` works explicitly, bare `git jump` defaults
to it (just `set -- auto` when $# -lt 1), and the usage text can
document the heuristic. It also keeps the detection and dispatch in
one place in case someone wants to tweak the priority later.
All in all, I think an auto jumping mode could be genuinely useful.
^ permalink raw reply
* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Junio C Hamano @ 2026-05-14 15:36 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <20260514115520.6660-3-taahol@utu.fi>
Tuomas Ahola <taahol@utu.fi> writes:
> Sorry, I don't know if I understood. Does the patch change the behavior of
> that command somehow? Is there some kind of edge case I missed?
No, I did not think it was a good idea to carve the behaviour in
stone that "git log --since=today" behaves as if it were given "git
log --since=now". My reaction would have been very different if we
were deliberatly and explicitly saying "today is synonym for now",
but the thing is, it is not a designed behaviour but what
approxidate does for anything it does not understand, e.g.
git log --since=decay
git log --since=bogus
all behave as if it were given --since=now.
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Phillip Wood @ 2026-05-14 15:15 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: gitster, christian.couder, karthik.188, jltobler, ayu.chandekar,
siddharthasthana31, chandrapratap3519
In-Reply-To: <20260402211717.3604688-1-pabloosabaterr@gmail.com>
Hi Pablo
On 02/04/2026 22:17, Pablo Sabater wrote:
> When having a history with multiple root commits and drawing the history
> near the roots, the graphing engine renders the commit one below the other,
> seeming that they are related, which makes the graph confusing.
>
> This issue was reported by Junio at:
> https://lore.kernel.org/git/xmqqikaawrpx.fsf@gitster.g/
>
> e.g.:
>
> * root-B
> * child-A2
> * child-A1
> * root-A
>
> [...]
>
> * root-B
> * child-A2
> /
> * child-A1
> * root-A
I'm rather late to the party here, but personally I find the indentation
a bit confusing, it would be clearer to me if we had a blank line after
a root commit
* root-B
* child-A2
* child-A1
* root-A
It takes the same amount of vertical space but keeps the children of
root-A together.
Thanks
Phillip
> This is done by adding a is_placeholder flag to the columns, the root commit
> is actually there but marked as a placeholder
>
> e.g.:
>
> * root-B
> (B) * child-A2
> /
> * child-A1
> * root-A
>
> (B) would be root-B column with the placeholder flag active.
>
> Then teaching the rendering function to print a padding ' ' when meeting a
> placeholder column outputs the second example.
>
> There could also be the case where there are multiple roots
>
> without the patch:
>
> * A root
> * B root
> * C root
> * D1 child
> * D root
>
> with the patch, the indentation cascades:
>
> * A root
> * B root
> * C root
> * D1 child
> _ /
> /
> /
> * D root
>
> the _ / might look weird but that's how the collapsing rendering does it
> for big gaps, this case being from the 4th column to the 0th column.
> Another patch could change the collapsing rendering for placeholders ?
> I haven't done it to keep it minimal, but a follow up could make it
> to be straight '/'. This would make it bigger but easier for the eye to follow.
> IMO is not worth it, but opinions are welcome.
>
> The patch also adds tests for different cases like a root preceding multiple
> parents merges and the examples above.
>
> There could be some edge cases still so any testing is very welcome.
>
> Pablo Sabater (1):
> graph: add indentation for commits preceded by a root
>
> graph.c | 68 ++++++++++++++++--
> t/t4215-log-skewed-merges.sh | 136 +++++++++++++++++++++++++++++++++++
> 2 files changed, 198 insertions(+), 6 deletions(-)
>
>
> base-commit: 256554692df0685b45e60778b08802b720880c50
^ permalink raw reply
* [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: René Scharfe @ 2026-05-14 15:13 UTC (permalink / raw)
To: Git List; +Cc: Jeff King
In-Reply-To: <c6e9b337-c4fc-4cbd-ac32-e8d3814749b0@web.de>
Clang and GCC optimize away comparisons of overflow checks by checking
the carry flag on x64. GCC does the same on ARM64, but Clang currently
(version 22.1) doesn't.
Provide a variant of st_add() that wraps __builtin_add_overflow() to
help Clang optimize it. Use it on all platforms for simplicity.
On an Apple M1 I get a nice speedup for a command that builds lots of
strings using a strbuf, which exercises the st_add3() in strbuf_grow()
for every line of output:
Benchmark 1: ./git_main cat-file --batch-all-objects --batch-check='%(objectname)'
Time (mean ± σ): 119.8 ms ± 0.2 ms [User: 113.0 ms, System: 5.8 ms]
Range (min … max): 119.6 ms … 120.4 ms 24 runs
Benchmark 2: ./git cat-file --batch-all-objects --batch-check='%(objectname)'
Time (mean ± σ): 114.6 ms ± 0.1 ms [User: 107.6 ms, System: 6.0 ms]
Range (min … max): 114.4 ms … 114.9 ms 25 runs
Summary
./git cat-file --batch-all-objects --batch-check='%(objectname)' ran
1.05 ± 0.00 times faster than ./git_main cat-file --batch-all-objects --batch-check='%(objectname)'
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git-compat-util.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index ae1bdc90a4..aa088d04bb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -614,6 +614,17 @@ static inline bool strip_suffix(const char *str, const char *suffix,
int git_open_cloexec(const char *name, int flags);
#define git_open(name) git_open_cloexec(name, O_RDONLY)
+/* Help Clang; GCC generates the same code for both variants. */
+#if defined(__clang__)
+static inline size_t st_add(size_t a, size_t b)
+{
+ size_t sum;
+ if (__builtin_add_overflow(a, b, &sum))
+ die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
+ (uintmax_t)a, (uintmax_t)b);
+ return sum;
+}
+#else
static inline size_t st_add(size_t a, size_t b)
{
if (unsigned_add_overflows(a, b))
@@ -621,6 +632,7 @@ static inline size_t st_add(size_t a, size_t b)
(uintmax_t)a, (uintmax_t)b);
return a + b;
}
+#endif
#define st_add3(a,b,c) st_add(st_add((a),(b)),(c))
#define st_add4(a,b,c,d) st_add(st_add3((a),(b),(c)),(d))
--
2.54.0
^ permalink raw reply related
* [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: René Scharfe @ 2026-05-14 15:11 UTC (permalink / raw)
To: Git List
Simplify the code by calling st_add3() to do overflow checks instead of
open-coding it. This changes the error message to include the offending
summands, which can be helpful when tracking down the cause.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
strbuf.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index 3e04addc22..bb04d3910e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -106,12 +106,9 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
void strbuf_grow(struct strbuf *sb, size_t extra)
{
int new_buf = !sb->alloc;
- if (unsigned_add_overflows(extra, 1) ||
- unsigned_add_overflows(sb->len, extra + 1))
- die("you want to use way too much memory");
if (new_buf)
sb->buf = NULL;
- ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
+ ALLOC_GROW(sb->buf, st_add3(sb->len, extra, 1), sb->alloc);
if (new_buf)
sb->buf[0] = '\0';
}
--
2.54.0
^ permalink raw reply related
* [PATCH v1 11/11] git-gui: add gui and pick as explicit subcommands
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git-gui accepts subcommands blame | browser | citool, and assumes the
subcommand is 'gui' if none is actually given, But, git gui also has a
repository picker (choose_repository::pick) that can create a new
repository + worktree, or choose an existing one, switch to that, and
the run the gui. The user has no direct control over invoking the
picker, instead the picker is triggered by failure in the repository /
worktree discover process: this includes being started in a directory
not controlled by git, which is probably the intended use case.
The picker can appear when the user has no intention of creating a new
worktree, and the user cannot use the picker to create a new worktree
inside another.
So, add two new explicit subcommands:
gui - Run the gui if repository/worktree discovery succeeds, or die
with an error message, but never run the picker.
pick - First run the picker, regardless, then start the gui in
the chosen worktree.
Nothing in this changes the prior behavior, the alternates above must be
explicitly selected to see any change.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 3a83dd5..c56aeef 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1021,6 +1021,7 @@ proc load_config {include_global} {
##
## feature option selection
+set run_picker_on_error 1
if {[regexp {^git-(.+)$} [file tail $argv0] _junk subcommand]} {
unset _junk
} else {
@@ -1030,6 +1031,7 @@ if {$subcommand eq {gui.sh}} {
set subcommand gui
}
if {$subcommand eq {gui} && [llength $argv] > 0} {
+ set run_picker_on_error 0
set subcommand [lindex $argv 0]
set argv [lrange $argv 1 end]
}
@@ -1047,6 +1049,7 @@ blame {
disable_option multicommit
disable_option branch
disable_option transport
+ set run_picker_on_error 0
}
citool {
enable_option singlecommit
@@ -1055,6 +1058,7 @@ citool {
disable_option multicommit
disable_option branch
disable_option transport
+ set run_picker_on_error 0
while {[llength $argv] > 0} {
set a [lindex $argv 0]
@@ -1162,14 +1166,28 @@ proc pick_repo {} {
set picked 1
}
+# run repository picker if explicitly requested
+switch -- $subcommand {
+ pick {
+ pick_repo
+ set subcommand gui
+ set run_picker_on_error 0
+ }
+}
+
# find repository.
if {[catch {
set _gitdir [git rev-parse --absolute-git-dir]
} err]} {
if {[is_gitvars_error $err]} {
exit 1
- } else {
+ }
+ if {$run_picker_on_error} {
pick_repo
+ } else {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "Git directory not found:"] "\n\n$err"]
+ exit 1
}
}
@@ -3051,7 +3069,7 @@ gui {
# fall through to setup UI for commits
}
default {
- set err "[mc usage:] $argv0 \[{blame|browser|citool}\]"
+ set err "[mc usage:] $argv0 \[{blame|browser|citool|gui|pick}\]"
if {[tk windowingsystem] eq "win32"} {
wm withdraw .
tk_messageBox -icon error -message $err \
--
2.54.0.99.14
^ permalink raw reply related
* [PATCH v1 10/11] git-gui: improve worktree discovery
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git gui's worktree discovery needs update based upon prior work in this
series. In the normal case, all information we need comes directly from
git rev-parse (--show-toplevel, and --show-prefix). Should this work, we
have a valid worktree and all git gui commands can run.
If not, we need to consider:
- if GIT_DIR or GIT_WORK_TREE are in the environment, just stop as we
the input configuration was wrong, the user must fix that.
- if we have a browser or blame subcommand, no worktree is needed so
git-gui can run without.
- using the git repository's parent is a valid worktree (if possible),
restoring prior behavior.
The current directory should be either the root of the worktree, if one
is found, or the top-level of the git repository.
Make it so. Also, make worktree discover directly follow repository
discovery, reducing the locations that might need error trapping to
catch configuration issues.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 56 ++++++++++++++++++++++--------------------------------
1 file changed, 23 insertions(+), 33 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index e326401..3a83dd5 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1173,6 +1173,28 @@ if {[catch {
}
}
+# find worktree, continue without if not required
+if {[catch {
+ set _gitworktree [git rev-parse --show-toplevel]
+ set _prefix [git rev-parse --show-prefix]
+ cd $_gitworktree
+} err]} {
+ if {[is_gitvars_error $err]} {
+ exit 1
+ }
+ set _gitworktree {}
+ set _prefix {}
+ if {[is_enabled bare]} {
+ cd $_gitdir
+ } elseif {![is_parent_worktree]} {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" $_gitdir]
+ exit 1
+ }
+}
+
+# repository and worktree config are complete, export them
+set_gitdir_vars
# Use object format as hash algorithm (either "sha1" or "sha256")
set hashalgorithm [git rev-parse --show-object-format]
@@ -1189,37 +1211,8 @@ if {$hashalgorithm eq "sha1"} {
load_config 0
apply_config
-set _gitworktree [git rev-parse --show-toplevel]
-if {$_prefix ne {}} {
- if {$_gitworktree eq {}} {
- regsub -all {[^/]+/} $_prefix ../ cdup
- } else {
- set cdup $_gitworktree
- }
- if {[catch {cd $cdup} err]} {
- catch {wm withdraw .}
- error_popup [strcat [mc "Cannot move to top of working directory:"] "\n\n$err"]
- exit 1
- }
- set _gitworktree [pwd]
- unset cdup
-} elseif {![is_enabled bare]} {
- if {[is_bare]} {
- catch {wm withdraw .}
- error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"]
- exit 1
- }
- if {$_gitworktree eq {}} {
- set _gitworktree [file dirname $_gitdir]
- }
- if {[catch {cd $_gitworktree} err]} {
- catch {wm withdraw .}
- error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
- exit 1
- }
- set _gitworktree [pwd]
-}
+# Derive a human-readable repository name
set _reponame [file split [file normalize $_gitdir]]
if {[lindex $_reponame end] eq {.git}} {
set _reponame [lindex $_reponame end-1]
@@ -1227,9 +1220,6 @@ if {[lindex $_reponame end] eq {.git}} {
set _reponame [lindex $_reponame end]
}
-# Export the final paths
-set_gitdir_vars
-
######################################################################
##
## global init
--
2.54.0.99.14
^ permalink raw reply related
* [PATCH v1 09/11] git-gui: support using repository parent dir as a worktree
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git-gui, since 87cd09f43e ("git-gui: work from the .git dir",
2010-01-23), has had the intent to allow starting from inside a
repository, then switching to the parent directory if that is a valid
worktree.
This certainly hasn't worked since 2d92ab32fd ("rev-parse: make
--show-toplevel without a worktree an error", 2019-11-19) in git, but
breaking this git-gui feature was unintentional.
Add a proc to test if the parent of the git repository is a valid
worktree, and set that directory as the worktree if so. Use invocations
of git rev-parse to assure all validity and safety checks included in
git-core are executed.
---
git-gui.sh | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/git-gui.sh b/git-gui.sh
index a03eaa7..e326401 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1100,6 +1100,23 @@ unset argv0dir
##
## repository setup
+proc is_parent_worktree {} {
+ # Directory 'parent' of a repository named 'parent/.git' might be the worktree
+ set ok 0
+ if {[file tail $::_gitdir] eq {.git}} {
+ set gitdir_parent [file join $::_gitdir {..}]
+ set expected_worktree [file normalize $gitdir_parent]
+ catch {set git_worktree [git -C $gitdir_parent rev-parse --show-toplevel]}
+ if {[string compare $expected_worktree $git_worktree] == 0} {
+ set ::_prefix {}
+ set ::_gitworktree $git_worktree
+ cd $git_worktree
+ set ok 1
+ }
+ }
+ return $ok
+}
+
proc is_gitvars_error {err} {
set havevars 0
set GIT_DIR {}
--
2.54.0.99.14
^ permalink raw reply related
* [PATCH v1 08/11] git-gui: simplify [is_bare] to report if a worktree is known
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git-gui includes proc is_bare, used in several places to make decisions
on whether a worktree exists, but also in discovery to tell if a
worktree can be supported.
But, is_bare is out of date with regard to multiple worktrees, safe
repository guards, and possibly other relevant features known to git
rev-parse. Also, is_bare caches its result on the first call, so is not
useful if a later step in the discovery process finds a worktree.
So, simplify is_bare to report whether git-gui has a worktree or is
working only from a repository.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 25 +------------------------
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 81789dd..a03eaa7 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -372,7 +372,6 @@ if {[tk windowingsystem] eq "aqua"} {
set _appname {Git Gui}
set _gitdir {}
set _gitworktree {}
-set _isbare {}
set _githtmldir {}
set _prefix {}
set _reponame {}
@@ -524,29 +523,7 @@ proc get_config {name} {
}
proc is_bare {} {
- global _isbare
- global _gitdir
- global _gitworktree
-
- if {$_isbare eq {}} {
- if {[catch {
- set _bare [git rev-parse --is-bare-repository]
- switch -- $_bare {
- true { set _isbare 1 }
- false { set _isbare 0}
- default { throw }
- }
- }]} {
- if {[is_config_true core.bare]
- || ($_gitworktree eq {}
- && [lindex [file split $_gitdir] end] ne {.git})} {
- set _isbare 1
- } else {
- set _isbare 0
- }
- }
- }
- return $_isbare
+ return [expr {$::_gitworktree eq {}}]
}
######################################################################
--
2.54.0.99.14
^ permalink raw reply related
* [PATCH v1 07/11] git-gui: use rev-parse exclusively to find a repository
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git-gui attempts to use env(GIT_DIR) directly as the git repository,
accepting GIT_DIR if it is a directory. Only if that fails is git
rev-parse used to discover the repository. But, this avoids all of
git-core's validity checking on a repository, thus possibly deferring an
error to a later step, possibly unexpected. Repository validation should
be part of initial setup so that later processing does not need error
trapping for configuration errors.
Let's just invoke rev-parse so all error checking is done. Stop here if
the user set GIT_DIR or GIT_WORK_TREE. Otherwise, continue the existing
behavior and show the repository picker.
Also, remove a later check on whether _gitdir is a directory: that code
cannot be reached without rev-parse having validating the repository.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 2e2ddc0..81789dd 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -374,6 +374,7 @@ set _gitdir {}
set _gitworktree {}
set _isbare {}
set _githtmldir {}
+set _prefix {}
set _reponame {}
set _shellpath {@@SHELL_PATH@@}
@@ -1167,19 +1168,18 @@ proc pick_repo {} {
set picked 1
}
+# find repository.
if {[catch {
- set _gitdir $env(GIT_DIR)
- set _prefix {}
- }]
- && [catch {
- # beware that from the .git dir this sets _gitdir to .
- # and _prefix to the empty string
- set _gitdir [git rev-parse --absolute-git-dir]
- set _prefix [git rev-parse --show-prefix]
- } err]} {
+ set _gitdir [git rev-parse --absolute-git-dir]
+} err]} {
+ if {[is_gitvars_error $err]} {
+ exit 1
+ } else {
pick_repo
+ }
}
+
# Use object format as hash algorithm (either "sha1" or "sha256")
set hashalgorithm [git rev-parse --show-object-format]
if {$hashalgorithm eq "sha1"} {
@@ -1191,12 +1191,6 @@ if {$hashalgorithm eq "sha1"} {
exit 1
}
-if {![file isdirectory $_gitdir]} {
- catch {wm withdraw .}
- error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
- exit 1
-}
-
# _gitdir exists, so try loading the config
load_config 0
apply_config
--
2.54.0.99.14
^ permalink raw reply related
* [PATCH v1 06/11] git gui: GIT_DIR / GIT_WORK_TREE make any discovery error fatal
From: Mark Levedahl @ 2026-05-14 14:33 UTC (permalink / raw)
To: git; +Cc: egg_mushroomcow, j6t, bootaina702, Mark Levedahl
In-Reply-To: <20260514143322.865587-1-mlevedahl@gmail.com>
git accepts any combination of GIT_DIR and GIT_WORK_TREE to override the
normal repository and worktree discovery process. git-gui should accept
any such valid configuration, but overriding the discovery process means
the user has assured that the combination of current directory, GIT_DIR,
and GIT_WORK_TREE will lead to the correct repository and worktree. As
such, an error found during discovery where either or both of GIT_DIR
and GIT_WORK_TREE are set is a fatal error, no further exploration
should be tried.
Provide a common proc to support displaying an error message and exiting
if GIT_DIR or GIT_WORK_TREE are in the environment.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
git-gui.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/git-gui.sh b/git-gui.sh
index c2cf5f1..2e2ddc0 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1122,6 +1122,24 @@ unset argv0dir
##
## repository setup
+proc is_gitvars_error {err} {
+ set havevars 0
+ set GIT_DIR {}
+ set GIT_WORK_TREE {}
+ catch {set GIT_DIR $::env(GIT_DIR); set havevars 1}
+ catch {set GIT_WORK_TREE $::env(GIT_WORK_TREE) ; set havevars 1}
+
+ if {$havevars} {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "Invalid configuration:"] \
+ "\n" "GIT_DIR: " $GIT_DIR \
+ "\n" "GIT_WORK_TREE: " $GIT_WORK_TREE \
+ "\n\n$err"]
+ return 1
+ }
+ return 0
+}
+
proc set_gitdir_vars {} {
global _gitdir _gitworktree env
if {$_gitdir ne {}} {
--
2.54.0.99.14
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox