* Re: [PATCH v2 3/4] diff: add long-running diff process via diff.<driver>.process
From: Junio C Hamano @ 2026-05-26 1:56 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <c25647c6e571e293fc994e0620ca37709f680f8a.1779733799.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +struct diff_subprocess {
> + struct subprocess_entry subprocess;
> + unsigned int supported_capabilities;
> +};
> +
> +static int subprocess_map_initialized;
> +static struct hashmap subprocess_map;
Can we avoid introducing new global variables like these? Would
"struct userdiff_driver" or "struct diff_options" be a good place to
hang this hashmap, perhaps?
> +static int send_file_content(int fd, const char *buf, long size)
> +{
> + int ret;
> +
> + if (size > 0)
> + ret = write_packetized_from_buf_no_flush(buf, size, fd);
> + else
> + ret = 0;
Shouldn't "size == -24" be flagged as an invalid input?
> + if (ret)
> + return ret;
> + return packet_flush_gently(fd);
> +}
> +static int parse_hunk_line(const char *line, struct xdl_hunk *hunk)
> +{
> +...
> +}
This gives a silent error diagnosis, which is good for a lower level
helper.
> +int diff_process_get_hunks(struct userdiff_driver *drv,
> + const char *path,
> + const char *old_buf, long old_size,
> + const char *new_buf, long new_size,
> + struct xdl_hunk **hunks_out,
> + size_t *nr_hunks_out)
> +{
> + struct diff_subprocess *backend;
> + struct child_process *process;
> + int fd_in, fd_out;
> + struct strbuf status = STRBUF_INIT;
> + struct xdl_hunk *hunks = NULL;
> + struct xdl_hunk hunk;
> + size_t nr_hunks = 0, alloc_hunks = 0;
> + int len;
> + char *line;
> +
> + if (!drv || !drv->process)
> + return -1;
A driver that does not define process is not an error; it is
perfectly normal in the current world order where nobody has such an
external process and even fi this patch lands, external processes
are optional. So here "return -1" does not mean an error, and
silent return is perfectly fine.
> + backend = find_or_start_process(drv->process);
> + if (!backend)
> + return -1;
This is probably an error; the user specified drv->process, we
either tried to find or start the process and failed. Isn't it an
event that deserves to be reported in an error message?
> + if (!(backend->supported_capabilities & CAP_HUNKS))
> + return -1;
Backend started, but the "hunks" feature is not supported. Perhaps
in a year or two, this external process protocol may have become so
popular that it gained more capabilities, possibly making get_hunks
obsolete. We may be looking at such an external process that uses
other capabilities but not this one. This is not an error, so
silent return is perfectly fine.
> + process = subprocess_get_child_process(&backend->subprocess);
> + fd_in = process->in;
> + fd_out = process->out;
> +
> + /* Send request */
> + if (packet_write_fmt_gently(fd_in, "command=hunks\n") ||
> + packet_write_fmt_gently(fd_in, "pathname=%s\n", path) ||
> + packet_flush_gently(fd_in))
> + goto error;
> +
> + /* Send old file content */
> + if (send_file_content(fd_in, old_buf, old_size))
> + goto error;
> +
> + /* Send new file content */
> + if (send_file_content(fd_in, new_buf, new_size))
> + goto error;
> +
> + /* Read hunks until flush packet */
> + while ((len = packet_read_line_gently(fd_out, NULL, &line)) >= 0 &&
> + line) {
> + if (parse_hunk_line(line, &hunk) < 0)
> + goto error;
> + ALLOC_GROW(hunks, nr_hunks + 1, alloc_hunks);
> + hunks[nr_hunks++] = hunk;
> + }
> + if (len < 0)
> + goto error;
> +
> + /* Read status */
> + if (subprocess_read_status(fd_out, &status))
> + goto error;
> +
> + if (strcmp(status.buf, "success")) {
> + if (!strcmp(status.buf, "abort"))
> + backend->supported_capabilities &= ~CAP_HUNKS;
> + goto error;
> + }
> +
> + *hunks_out = hunks;
> + *nr_hunks_out = nr_hunks;
> + strbuf_release(&status);
> + return 0;
> +
> +error:
All exceptions that lead here look like events that should be
reported to the end-user.
> + free(hunks);
> + strbuf_release(&status);
> + return -1;
> +}
> +/*
> + * Query a diff process for hunks describing the changes
> + * between old_buf and new_buf.
> + *
> + * The backend is a long-running subprocess configured via
> + * diff.<driver>.process. It receives file content via
> + * pkt-line and returns hunks with 1-based line numbers.
> + *
> + * On success, sets *hunks_out and *nr_hunks_out to a newly allocated
> + * array (caller must free) and returns 0.
> + *
> + * On failure, returns -1. The caller should fall back to the
> + * builtin diff algorithm.
> + */
I do not agree with this. If it is a failure, the user should fix
the external process (or disable). It shouldn't be hidden behind a
fallback. As I left comments, in this round of implementation,
there are conditions that returns -1 for soemthing that is not an
error (i.e., not configured, or process not supporting the
particular capability) *and* in those cases the caller should fall
back as if nothing happened. But some error cases, the caller
should't hide them.
^ permalink raw reply
* Re: Expected test suite behavior
From: Junio C Hamano @ 2026-05-26 1:11 UTC (permalink / raw)
To: brian m. carlson; +Cc: Amogh Dambal, Jeff King, Michael Montalbo, git
In-Reply-To: <ahTsTDhVPkHTEbB_@fruit.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I think I know what the problem is: you're running as root. I suspect
> `test -x` in the test says that you have permission to execute it
> because you're root and root always ignores permissions. My guess is
> that most of the tests you're failing have to do with permissions of
> some sort that are being ignored because you're privileged.
I know !SANITY defeats a-rw and lets the tester read or write to the
path, but this is the first time I heard that !SANITY defeats a-x
and lets the tester _execute_ it.
I do not think we drop POSIXPERM prereq when !SANITY automatically,
and I do not think we should, but we should probably have an
automated check to drop POSIXPERM?
(
# is an unexecutable look to the user as executable?
umask 0; >testfile; chmod a-w testfile; test -x testfile;
status=$?;
rm -f testfile; exit $status
)
> I'll just note that if you just want to do Git development, macOS is a
> fully supported platform on which to do that. I will admit most of the
> major contributors (with the notable exception of the Git for Windows
> maintainer) do use Linux and of course I like and endorse Debian, but
> macOS should build and run just fine if you prefer that.
Hear hear.
We want to encourage developers to do more _native_ development on
their own system, and this is not limited to macOS. As long as
users on a particular platform rely on working Git there, we are
better off if we have more people actively using that platform to
build, test, develop on, and debug Git for their own use.
Thanks.
^ permalink raw reply
* Re: Expected test suite behavior
From: brian m. carlson @ 2026-05-26 0:41 UTC (permalink / raw)
To: Amogh Dambal; +Cc: Jeff King, Michael Montalbo, git
In-Reply-To: <4649049a-ded5-4cc6-bc2b-d5f543e6df99@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]
On 2026-05-25 at 22:25:11, Amogh Dambal wrote:
> > What are the OS and file system on the host? We tend to see
> > executable bits set when NTFS, FAT, or other Windows-adjacent file
> > systems are used on Linux and you're mounting `$(PWD)` into the
> > container as a volume.
>
> Ah, this is a smoking gun. I'm not on a Windows-adjacent file system; I'm
> running macOS Sequoia 15.5 on the host. Specifically:
>
> $ uname -msprsv
> Darwin 24.5.0 Darwin Kernel Version 24.5.0: Tue Apr 22 19:54:26 PDT 2025;
> root:xnu-11417.121.6~2/RELEASE_ARM64_T8112 arm64 arm
>
> But I am mounting $(PWD) into the container as a volume.
I wouldn't expect that to be a problem, then. macOS uses Unix-style
permissions and I've never seen odd permissions behaviour mounting a
macOS APFS file system into a container. I will, however, note that I
am using a case-sensitive APFS volume, but I cannot imagine how this
would occur with _any_ macOS APFS volume mounted into a Linux container.
> > Git doesn't use `/tmp` for most files in the tests. Those are stored
> > under `t/`, so you'd want to create your test directory there.
>
> ACK, good to know, thanks! I am still seeing the same behavior with a
> `debug` directory under `t/`:
>
> root@ec94ab1b260e:~/git/t/debug# /root/git/git init plain
> root@ec94ab1b260e:~/git/t/debug# ls -alhrt
> /root/git/t/debug/plain/.git/config
> -rw-r--r-- 1 root root 111 May 25 22:24 /root/git/t/debug/plain/.git/config
I think I know what the problem is: you're running as root. I suspect
`test -x` in the test says that you have permission to execute it
because you're root and root always ignores permissions. My guess is
that most of the tests you're failing have to do with permissions of
some sort that are being ignored because you're privileged.
In general, you would not want to run this as root. Use `adduser` to
create yourself a regular user and then use the `USER` directive in the
Dockerfile to change users. I don't run the tests as root and I'm sure
none of the other regular contributors do, either. Running unprivileged
in a container is a best practice anyway.
I'll just note that if you just want to do Git development, macOS is a
fully supported platform on which to do that. I will admit most of the
major contributors (with the notable exception of the Git for Windows
maintainer) do use Linux and of course I like and endorse Debian, but
macOS should build and run just fine if you prefer that.
But if you do want to use a container, I'd try unprivileged and see if
substantially more of the tests pass.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: How does git track history overwrites?
From: Jens Tröger @ 2026-05-25 23:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chris Torek, git
In-Reply-To: <xmqqfr3fnl2n.fsf@gitster.g>
Hello Junio,
> Sorry, I may have been unclear. I specifically meant the "grafted,
> " part in the message. I know how "git log" output looks like ;-)
The “grafted” too was part of the git log output; here is the complete
cmd line output:
/tmp/bla > git log
commit fda77690955e9b63c6687d8806bafd56a526e45f (grafted, HEAD)
Author: Adam Johnson <me@adamj.eu>
Date: Mon Sep 8 16:31:35 2025 +0100
Version 1.20.0
On *why* the “grafted” is there in the first place, I suspect that’s got
to do with the fetch --depth=1 and that previous history isn’t available
in the shallow repo clone.
Cheers,
Jens
^ permalink raw reply
* Re: How does git track history overwrites?
From: Junio C Hamano @ 2026-05-25 23:09 UTC (permalink / raw)
To: Jens Tröger; +Cc: Chris Torek, git
In-Reply-To: <074E783A-027D-4C5B-BC44-CC38C53735D7@light-speed.de>
Jens Tröger <jens.troeger@light-speed.de> writes:
> Thank you Chris and Junio!
>
>
>> [Junio] Where does this line in your discussion page at GitHub (which is
>> omitted from the post to this list) come from?
>>
>> commit fda77690955e9b63c6687d8806bafd56a526e45f (grafted, HEAD)
>>
>> Are you doing anything funky with .git/info/grafts by any chance?
>
> That line is the result of a `git log` after the `git fetch` I mentioned in my initial email.
Sorry, I may have been unclear. I specifically meant the "grafted,
" part in the message. I know how "git log" output looks like ;-)
^ permalink raw reply
* Re: [PATCH v2] receive-pack: fix updateInstead with core.worktree
From: Junio C Hamano @ 2026-05-25 22:54 UTC (permalink / raw)
To: Alyssa Ross
Cc: git, Ævar Arnfjörð Bjarmason, Kristoffer Haugsbakk
In-Reply-To: <20260525162311.66240-2-hi@alyssa.is>
Alyssa Ross <hi@alyssa.is> writes:
> Previously, only one of push_to_checkout() or push_to_deploy() was
> called. In a8cc594333 (hooks: fix an obscure TOCTOU "did we just run a
> hook?" race, 2022-03-07), this was changed to always call
> push_to_checkout(), and then to call push_to_deploy() if
> push_to_checkout() didn't run anything. This change didn't take into
> account that push_to_checkout() had a side effect of modifying env, and
> that modified env broke updating the worktree in push_to_deploy() if
> core.worktree was configured. To fix this, only mutate the environment
> used inside push_to_commit(), rather than the environment that might
> later be passed to push_to_deploy().
>
> Signed-off-by: Alyssa Ross <hi@alyssa.is>
> ---
> v2: reword commit message in response to feedback
You also fixed an incorrectly indentated line in the new test, which
is very much appreciated.
Will queue. Thanks.
>
> builtin/receive-pack.c | 2 +-
> t/t5516-fetch-push.sh | 11 +++++++++++
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index c7b2818f20..7ee157532d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1460,8 +1460,8 @@ static const char *push_to_checkout(unsigned char *hash,
>
> opt.invoked_hook = invoked_hook;
>
> - strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
> strvec_pushv(&opt.env, env->v);
> + strvec_pushf(&opt.env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
> strvec_push(&opt.args, hash_to_hex(hash));
> if (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))
> return "push-to-checkout hook declined";
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 117cfa051f..db6cc18673 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
> )
> '
>
> +test_expect_success 'denyCurrentBranch and core.worktree' '
> + test_when_finished "rm -fr cloned cloned.git" &&
> + git clone --separate-git-dir cloned.git . cloned &&
> + git --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&
> + git --git-dir cloned.git config core.worktree "$PWD/cloned" &&
> + test_commit raspberry &&
> + git push cloned.git HEAD:main &&
> + test_path_exists cloned/raspberry.t &&
> + test_must_fail git push --delete cloned.git main
> +'
> +
> test_expect_success 'denyCurrentBranch and worktrees' '
> test_when_finished "rm -fr cloned && git worktree remove --force new-wt" &&
> git worktree add new-wt &&
>
> base-commit: aec3f587505a472db67e9462d0702e7d463a449d
^ permalink raw reply
* Re: [PATCH v2 2/3] commit-reach: deduplicate queue entries in paint_down_to_common
From: Junio C Hamano @ 2026-05-25 22:50 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Jeff King, Kristofer Karlsson
In-Reply-To: <fc38c0f856e93b80073ec3f1b9f641b9ab187e4e.1779719286.git.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Kristofer Karlsson <krka@spotify.com>
>
> paint_down_to_common() can enqueue the same commit multiple times
> when it is reached through different parents with different flag
> combinations. Add an ENQUEUED flag to track whether a commit is
> currently in the priority queue, and skip it if already present.
>
> Introduce prio_queue_put_dedup() and prio_queue_get_dedup()
> wrappers that manage the ENQUEUED flag on enqueue and dequeue.
OK. I guess an obvious alternative design would be to have an
associated hashtable for deduping, or tweak prio_queue_get() so
that it notices duplicated entry just before it returns (i.e.,
peek and discard until queue->array[0].data is different from
what you are going to return). Both would not beat the cheap cost
of using a single bit per object, I guess ;-)
> This change is performance-neutral on its own: the O(n)
> queue_has_nonstale() scan still dominates the per-iteration cost.
> However, the deduplication guarantee (each commit appears in the
> queue at most once) is a prerequisite for the next commit, which
> replaces that scan with O(1) tracking.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> commit-reach.c | 27 ++++++++++++++++++++++-----
> object.h | 2 +-
> 2 files changed, 23 insertions(+), 6 deletions(-)
Thanks for the clean-up in the previous step, by the way.
> diff --git a/commit-reach.c b/commit-reach.c
> index 5a52be90a6..85583ae359 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -17,8 +17,9 @@
> #define PARENT2 (1u<<17)
> #define STALE (1u<<18)
> #define RESULT (1u<<19)
> +#define ENQUEUED (1u<<20)
>
> -static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
> +static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
>
> static int compare_commits_by_gen(const void *_a, const void *_b)
> {
> @@ -39,6 +40,22 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
> return 0;
> }
>
> +static void prio_queue_put_dedup(struct prio_queue *queue, struct commit *c)
> +{
> + if (c->object.flags & ENQUEUED)
> + return;
> + c->object.flags |= ENQUEUED;
> + prio_queue_put(queue, c);
> +}
> +
> +static struct commit *prio_queue_get_dedup(struct prio_queue *queue)
> +{
> + struct commit *commit = prio_queue_get(queue);
> + if (commit)
> + commit->object.flags &= ~ENQUEUED;
> + return commit;
> +}
> +
> static int queue_has_nonstale(struct prio_queue *queue)
> {
> for (size_t i = 0; i < queue->nr; i++) {
> @@ -70,15 +87,15 @@ static int paint_down_to_common(struct repository *r,
> commit_list_append(one, result);
> return 0;
> }
> - prio_queue_put(&queue, one);
> + prio_queue_put_dedup(&queue, one);
>
> for (i = 0; i < n; i++) {
> twos[i]->object.flags |= PARENT2;
> - prio_queue_put(&queue, twos[i]);
> + prio_queue_put_dedup(&queue, twos[i]);
> }
>
> while (queue_has_nonstale(&queue)) {
> - struct commit *commit = prio_queue_get(&queue);
> + struct commit *commit = prio_queue_get_dedup(&queue);
> struct commit_list *parents;
> int flags;
> timestamp_t generation = commit_graph_generation(commit);
> @@ -132,7 +149,7 @@ static int paint_down_to_common(struct repository *r,
> oid_to_hex(&p->object.oid));
> }
> p->object.flags |= flags;
> - prio_queue_put(&queue, p);
> + prio_queue_put_dedup(&queue, p);
> }
> }
>
> diff --git a/object.h b/object.h
> index 2b26de3044..8fb03ff90a 100644
> --- a/object.h
> +++ b/object.h
> @@ -75,7 +75,7 @@ void object_array_init(struct object_array *array);
> * bundle.c: 16
> * http-push.c: 11-----14
> * commit-graph.c: 15
> - * commit-reach.c: 16-----19
> + * commit-reach.c: 16-------20
> * builtin/last-modified.c: 1617
> * object-name.c: 20
> * list-objects-filter.c: 21
^ permalink raw reply
* Re: How does git track history overwrites?
From: Jens Tröger @ 2026-05-25 22:47 UTC (permalink / raw)
To: Junio C Hamano, Chris Torek; +Cc: git
In-Reply-To: <87se7gasn8.fsf@gitster.g>
Thank you Chris and Junio!
> [Junio] Where does this line in your discussion page at GitHub (which is
> omitted from the post to this list) come from?
>
> commit fda77690955e9b63c6687d8806bafd56a526e45f (grafted, HEAD)
>
> Are you doing anything funky with .git/info/grafts by any chance?
That line is the result of a `git log` after the `git fetch` I mentioned in my initial email.
> [Chris] To really understand this properly, we need to understand
> the root of a seeming contradiction:
>
> [...]
Thank you for your elaborate explanation, Chris, that all makes a lot of sense. A few follow-up questions:
• Is all the object information stored with a repo clone locally as well, or does some/most/all of it stay on the remote server repo?
• How exactly does git connect the dots between commit dda8db1 and fda7769, how does it “know” the former was superseded by the latter (i.e. I fetch the former and Git uses the latter for head)?
• Based on the previous question, can I manually find such a connection between two commit objects too?
It sounds like reading some internals would be helpful. I’m noodling through https://git-scm.com/book/en/v2/Git-Internals-Git-Objects but perhaps you have some more recommendations?
With many greetings,
Jens
^ permalink raw reply
* Re: Expected test suite behavior
From: Amogh Dambal @ 2026-05-25 22:25 UTC (permalink / raw)
To: brian m. carlson, Jeff King, Michael Montalbo, git
In-Reply-To: <ahTKq_zCmEDJpoN5@fruit.crustytoothpaste.net>
> What are the OS and file system on the host? We tend to see
> executable bits set when NTFS, FAT, or other Windows-adjacent file
> systems are used on Linux and you're mounting `$(PWD)` into the
> container as a volume.
Ah, this is a smoking gun. I'm not on a Windows-adjacent file system;
I'm running macOS Sequoia 15.5 on the host. Specifically:
$ uname -msprsv
Darwin 24.5.0 Darwin Kernel Version 24.5.0: Tue Apr 22 19:54:26 PDT
2025; root:xnu-11417.121.6~2/RELEASE_ARM64_T8112 arm64 arm
But I am mounting $(PWD) into the container as a volume.
> Git doesn't use `/tmp` for most files in the tests. Those are stored
> under `t/`, so you'd want to create your test directory there.
ACK, good to know, thanks! I am still seeing the same behavior with a
`debug` directory under `t/`:
root@ec94ab1b260e:~/git/t/debug# /root/git/git init plain
root@ec94ab1b260e:~/git/t/debug# ls -alhrt
/root/git/t/debug/plain/.git/config
-rw-r--r-- 1 root root 111 May 25 22:24 /root/git/t/debug/plain/.git/config
^ permalink raw reply
* Re: Expected test suite behavior
From: brian m. carlson @ 2026-05-25 22:18 UTC (permalink / raw)
To: Amogh Dambal; +Cc: Jeff King, Michael Montalbo, git
In-Reply-To: <23221493-ea81-47c3-9647-6c6ac8d03360@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]
On 2026-05-25 at 22:01:23, Amogh Dambal wrote:
> `GIT_TEST_OPTS=--verbose` was very illuminating. I captured STDOUT/STDERR:
> `make test GIT_TEST_OPTS=--verbose &> git-test-verbose-fail.tmp`, which
> shows that almost every test fails `check_config` because a `git init` is
> creating a `.git/config` file whose executable bit is set:
What are the OS and file system on the host? We tend to see
executable bits set when NTFS, FAT, or other Windows-adjacent file
systems are used on Linux and you're mounting `$(PWD)` into the
container as a volume.
> --
> Initialized empty Git repository in /root/git/t/trash
> directory.t0001-init/plain/.git/
> plain/.git/config is executable?
> not ok 1 - plain
>
> [...]
>
>
> However, I'm not able to reproduce this, e.g. directly using the local built
> binary seems to work fine:
>
> mkdir -p /tmp/debug && cd /tmp/debug
> /root/git/git init plain
> ls -alhrt /tmp/debug/plain/.git
> root@ec94ab1b260e:/tmp/debug# ls -alhrt /tmp/debug/plain/.git
> total 24K
> -rw-r--r-- 1 root root 92 May 25 21:26 config
> drwxr-xr-x 3 root root 4.0K May 25 21:26 ..
> drwxr-xr-x 4 root root 4.0K May 25 21:26 refs
> drwxr-xr-x 4 root root 4.0K May 25 21:26 objects
> -rw-r--r-- 1 root root 23 May 25 21:26 HEAD
> drwxr-xr-x 4 root root 4.0K May 25 21:26 .
Git doesn't use `/tmp` for most files in the tests. Those are stored
under `t/`, so you'd want to create your test directory there.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: Expected test suite behavior
From: Amogh Dambal @ 2026-05-25 22:01 UTC (permalink / raw)
To: Jeff King, Michael Montalbo; +Cc: git
In-Reply-To: <20260525072711.GE2737798@coredump.intra.peff.net>
Thanks for the replies, and the pointers!
>> Hello. If you run `make test GIT_TEST_OPTS=--verbose` or uncomment
>> L16 of t/Makefile is there more information describing the issue?
> You can use --verbose-log instead, and
> then output is in t/test-results/t1234-whatever.out.
`GIT_TEST_OPTS=--verbose` was very illuminating. I captured
STDOUT/STDERR: `make test GIT_TEST_OPTS=--verbose &>
git-test-verbose-fail.tmp`, which shows that almost every test fails
`check_config` because a `git init` is creating a `.git/config` file
whose executable bit is set:
--
Initialized empty Git repository in /root/git/t/trash
directory.t0001-init/plain/.git/
plain/.git/config is executable?
not ok 1 - plain
[...]
However, I'm not able to reproduce this, e.g. directly using the local
built binary seems to work fine:
mkdir -p /tmp/debug && cd /tmp/debug
/root/git/git init plain
ls -alhrt /tmp/debug/plain/.git
root@ec94ab1b260e:/tmp/debug# ls -alhrt /tmp/debug/plain/.git
total 24K
-rw-r--r-- 1 root root 92 May 25 21:26 config
drwxr-xr-x 3 root root 4.0K May 25 21:26 ..
drwxr-xr-x 4 root root 4.0K May 25 21:26 refs
drwxr-xr-x 4 root root 4.0K May 25 21:26 objects
-rw-r--r-- 1 root root 23 May 25 21:26 HEAD
drwxr-xr-x 4 root root 4.0K May 25 21:26 .
In fact, even the specific test directory that I assume is being checked
reports properly set bits on the `config` file:
root@f695346357b9:~/git# ls -alhrt t/trash\ directory.t0001-init/plain/.git/
total 12K
drwxr-xr-x 3 root root 96 May 25 21:08 ..
drwxr-xr-x 3 root root 96 May 25 21:08 info
-rwxr-xr-x 1 root root 73 May 25 21:08 description
drwxr-xr-x 16 root root 512 May 25 21:08 hooks
-rw-r--r-- 1 root root 111 May 25 21:08 config
drwxr-xr-x 4 root root 128 May 25 21:08 refs
-rw-r--r-- 1 root root 23 May 25 21:08 HEAD
drwxr-xr-x 9 root root 288 May 25 21:08 .
drwxr-xr-x 4 root root 128 May 25 21:08 objects
I've further checked that the POSIXPERM requirement is being set and
verified that there are no shell differences (I'm using /bin/bash as my
main shell in the Docker container, while the test scripts use /bin/sh
(which on this Docker container is symlinked to `dash`):
root@ec94ab1b260e:~/git/t# ls -alhrt /bin/sh
lrwxrwxrwx 1 root root 4 Feb 4 2025 /bin/sh -> dash
I'm sure that I'm missing something blindingly obvious with respect to
my setup, but I have not yet been able to figure out what that might be.
^ permalink raw reply
* [PATCH v2 3/3] line-log: allow non-patch diff formats with -L
From: Michael Montalbo via GitGitGadget @ 2026-05-25 19:40 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2094.v2.git.1779738059.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
Now that -L flows through log_tree_diff_flush() and diff_flush(),
metadata-only diff formats work because they only read filepair
fields (status, mode, path, oid) already set on the pre-computed
pairs.
Expand the allowlist in setup_revisions() to also accept --raw,
--name-only, --name-status, and --summary. Diff stat formats
(--stat, --numstat, --shortstat, --dirstat) remain blocked because
they call compute_diffstat() on full blob content and would show
whole-file statistics rather than range-scoped ones.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
Documentation/line-range-options.adoc | 10 +++---
revision.c | 4 ++-
t/t4211-line-log.sh | 47 +++++++++++++++++++++++++--
3 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/Documentation/line-range-options.adoc b/Documentation/line-range-options.adoc
index ecb2c79fb9..72f639b5e7 100644
--- a/Documentation/line-range-options.adoc
+++ b/Documentation/line-range-options.adoc
@@ -8,12 +8,14 @@
give zero or one positive revision arguments, and
_<start>_ and _<end>_ (or _<funcname>_) must exist in the starting revision.
You can specify this option more than once. Implies `--patch`.
- Patch output can be suppressed using `--no-patch`, but other diff formats
- (namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
- `--name-only`, `--name-status`, `--check`) are not currently implemented.
+ Patch output can be suppressed using `--no-patch`.
+ Non-patch diff formats `--raw`, `--name-only`, `--name-status`,
+ and `--summary` are supported. Diff stat formats
+ (`--stat`, `--numstat`, `--shortstat`, `--dirstat`) are not
+ currently implemented.
+
Patch formatting options such as `--word-diff`, `--color-moved`,
`--no-prefix`, and whitespace options (`-w`, `-b`) are supported,
-as are pickaxe options (`-S`, `-G`).
+as are pickaxe options (`-S`, `-G`) and `--diff-filter`.
+
include::line-range-format.adoc[]
diff --git a/revision.c b/revision.c
index c903f7a1b4..f26fc1f4d5 100644
--- a/revision.c
+++ b/revision.c
@@ -3181,7 +3181,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->line_level_traverse &&
(revs->full_diff ||
(revs->diffopt.output_format &
- ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
+ DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
+ DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
die(_("-L does not yet support the requested diff format"));
if (revs->expand_tabs_in_log < 0)
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index e3937138a9..4722ec3e29 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -155,8 +155,45 @@ test_expect_success '-p shows the default patch output' '
test_cmp expect actual
'
-test_expect_success '--raw is forbidden' '
- test_must_fail git log -L1,24:b.c --raw
+test_expect_success '--raw shows mode, oid, status and path' '
+ git log -L1,24:b.c --raw --format= >actual &&
+ test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M b.c$" actual &&
+ ! test_grep "^diff --git" actual &&
+ ! test_grep "^@@" actual
+'
+
+test_expect_success '--name-only shows path' '
+ git log -L1,24:b.c --name-only --format= >actual &&
+ test_grep "^b.c$" actual &&
+ ! test_grep "^diff --git" actual &&
+ ! test_grep "^@@" actual
+'
+
+test_expect_success '--name-status shows status and path' '
+ git log -L1,24:b.c --name-status --format= >actual &&
+ test_grep "^M b.c$" actual &&
+ ! test_grep "^diff --git" actual &&
+ ! test_grep "^@@" actual
+'
+
+test_expect_success '--stat is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --stat 2>err &&
+ test_grep "does not yet support" err
+'
+
+test_expect_success '--numstat is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --numstat 2>err &&
+ test_grep "does not yet support" err
+'
+
+test_expect_success '--shortstat is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --shortstat 2>err &&
+ test_grep "does not yet support" err
+'
+
+test_expect_success '--dirstat is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --dirstat 2>err &&
+ test_grep "does not yet support" err
'
test_expect_success 'setup for checking fancy rename following' '
@@ -738,4 +775,10 @@ test_expect_success '-L --oneline has no extra blank line before diff' '
test_grep "^diff --git" line2
'
+test_expect_success '--summary shows new file on root commit' '
+ git checkout parent-oids &&
+ git log -L:func2:file.c --summary --format= >actual &&
+ test_grep "create mode 100644 file.c" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/3] line-log: integrate -L output with the standard log-tree pipeline
From: Michael Montalbo via GitGitGadget @ 2026-05-25 19:40 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2094.v2.git.1779738059.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
`git log -L` has bypassed log_tree_diff() and log_tree_diff_flush()
since the feature was introduced, short-circuiting from
log_tree_commit() directly into line_log_print(). This skips the
no_free save/restore (noted in a NEEDSWORK comment added by
f8781bfda3), the always_show_header fallback, show_diff_of_diff(),
and diff_free() cleanup.
Restructure so that -L flows through log_tree_diff() ->
log_tree_diff_flush(), the same path used by the normal
single-parent and merge diff codepaths:
- Rename line_log_print() to line_log_queue_pairs() and strip it
down to just queuing pre-computed filepairs. The show_log(),
separator, diffcore_std(), and diff_flush() calls are removed
since log_tree_diff_flush() handles all of those.
- In log_tree_diff(), call line_log_queue_pairs() then
log_tree_diff_flush(), mirroring the diff_tree_oid() + flush
pattern used by the single-parent and merge codepaths.
- Remove the early return in log_tree_commit() that is no longer
needed now that -L output flows through log_tree_diff() and
log_tree_diff_flush(); this restores no_free save/restore,
always_show_header, and diff_free() cleanup.
Because show_log() is now deferred until after diffcore_std() inside
log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and
--diff-filter now properly suppress commits when all pairs are
filtered out.
The blank-line separator between commit header and diff changes
slightly: the old code printed one unconditionally, while
log_tree_diff_flush() only emits one for verbose headers. This
matches the rest of log output.
Also reject --full-diff, which is not yet supported with -L: the
filepairs are pre-computed during the history walk and scoped to
tracked line ranges, so there is currently no full-tree diff to
fall back to for display.
Update tests accordingly.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
line-log.c | 30 ++++-------
line-log.h | 2 +-
log-tree.c | 10 ++--
revision.c | 6 ++-
t/t4211-line-log.sh | 53 ++++++++++++++-----
t/t4211/sha1/expect.parallel-change-f-to-main | 1 -
.../sha256/expect.parallel-change-f-to-main | 1 -
7 files changed, 60 insertions(+), 43 deletions(-)
diff --git a/line-log.c b/line-log.c
index 858a899cd2..7ee55b05cc 100644
--- a/line-log.c
+++ b/line-log.c
@@ -13,7 +13,6 @@
#include "revision.h"
#include "xdiff-interface.h"
#include "strbuf.h"
-#include "log-tree.h"
#include "line-log.h"
#include "setup.h"
#include "strvec.h"
@@ -1004,29 +1003,18 @@ static int process_all_files(struct line_log_data **range_out,
return changed;
}
-int line_log_print(struct rev_info *rev, struct commit *commit)
+void line_log_queue_pairs(struct rev_info *rev, struct commit *commit)
{
- show_log(rev);
- if (!(rev->diffopt.output_format & DIFF_FORMAT_NO_OUTPUT)) {
- struct line_log_data *range = lookup_line_range(rev, commit);
- struct line_log_data *r;
- const char *prefix = diff_line_prefix(&rev->diffopt);
-
- fprintf(rev->diffopt.file, "%s\n", prefix);
-
- for (r = range; r; r = r->next) {
- if (r->pair) {
- struct diff_filepair *p =
- diff_filepair_dup(r->pair);
- p->line_ranges = &r->ranges;
- diff_q(&diff_queued_diff, p);
- }
- }
+ struct line_log_data *range = lookup_line_range(rev, commit);
+ struct line_log_data *r;
- diffcore_std(&rev->diffopt);
- diff_flush(&rev->diffopt);
+ for (r = range; r; r = r->next) {
+ if (r->pair) {
+ struct diff_filepair *p = diff_filepair_dup(r->pair);
+ p->line_ranges = &r->ranges;
+ diff_q(&diff_queued_diff, p);
+ }
}
- return 1;
}
static int bloom_filter_check(struct rev_info *rev,
diff --git a/line-log.h b/line-log.h
index 04a6ea64d3..99e1755ce3 100644
--- a/line-log.h
+++ b/line-log.h
@@ -46,7 +46,7 @@ int line_log_filter(struct rev_info *rev);
int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,
struct commit *commit);
-int line_log_print(struct rev_info *rev, struct commit *commit);
+void line_log_queue_pairs(struct rev_info *rev, struct commit *commit);
void line_log_free(struct rev_info *rev);
diff --git a/log-tree.c b/log-tree.c
index 7e048701d0..88b3019293 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -1105,6 +1105,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
if (!all_need_diff && !opt->merges_need_diff)
return 0;
+ if (opt->line_level_traverse) {
+ line_log_queue_pairs(opt, commit);
+ log_tree_diff_flush(opt);
+ return !opt->loginfo;
+ }
+
parse_commit_or_die(commit);
oid = get_commit_tree_oid(commit);
@@ -1179,10 +1185,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
opt->loginfo = &log;
opt->diffopt.no_free = 1;
- /* NEEDSWORK: no restoring of no_free? Why? */
- if (opt->line_level_traverse)
- return line_log_print(opt, commit);
-
if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
shown = log_tree_diff(opt, commit, &log);
diff --git a/revision.c b/revision.c
index 4a8e24bc38..c903f7a1b4 100644
--- a/revision.c
+++ b/revision.c
@@ -3179,8 +3179,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
if (revs->line_level_traverse &&
- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
- die(_("-L does not yet support diff formats besides -p and -s"));
+ (revs->full_diff ||
+ (revs->diffopt.output_format &
+ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
+ die(_("-L does not yet support the requested diff format"));
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index aaf197d2ed..e3937138a9 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -368,7 +368,6 @@ test_expect_success '-L diff output includes index and new file mode' '
test_expect_success '-L with --word-diff' '
cat >expect <<-\EOF &&
-
diff --git a/file.c b/file.c
--- a/file.c
+++ b/file.c
@@ -377,7 +376,6 @@ test_expect_success '-L with --word-diff' '
{
return [-F2;-]{+F2 + 2;+}
}
-
diff --git a/file.c b/file.c
new file mode 100644
--- /dev/null
@@ -433,7 +431,6 @@ test_expect_success 'show line-log with graph' '
null_blob=$(test_oid zero | cut -c1-7) &&
qz_to_tab_space >expect <<-EOF &&
* $head_oid Modify func2() in file.c
- |Z
| diff --git a/file.c b/file.c
| index $head_blob_old..$head_blob_new 100644
| --- a/file.c
@@ -445,7 +442,6 @@ test_expect_success 'show line-log with graph' '
| + return F2 + 2;
| }
* $root_oid Add func1() and func2() in file.c
- ZZ
diff --git a/file.c b/file.c
new file mode 100644
index $null_blob..$root_blob
@@ -494,23 +490,17 @@ test_expect_success '-L --find-object does not crash with merge and rename' '
--find-object=$(git rev-parse HEAD:file) >actual
'
-# Commit-level filtering with pickaxe does not yet work for -L.
-# show_log() prints the commit header before diffcore_std() runs
-# pickaxe, so commits cannot be suppressed even when no diff pairs
-# survive filtering. Fixing this would require deferring show_log()
-# until after diffcore_std(), which is a larger restructuring of the
-# log-tree output pipeline.
-test_expect_failure '-L -G should filter commits by pattern' '
+test_expect_success '-L -G should filter commits by pattern' '
git log --format="%s" --no-patch -L 1,1:file -G "nomatch" >actual &&
test_must_be_empty actual
'
-test_expect_failure '-L -S should filter commits by pattern' '
+test_expect_success '-L -S should filter commits by pattern' '
git log --format="%s" --no-patch -L 1,1:file -S "nomatch" >actual &&
test_must_be_empty actual
'
-test_expect_failure '-L --find-object should filter commits by object' '
+test_expect_success '-L --find-object should filter commits by object' '
git log --format="%s" --no-patch -L 1,1:file \
--find-object=$ZERO_OID >actual &&
test_must_be_empty actual
@@ -711,4 +701,41 @@ test_expect_success '-L with -G filters to diff-text matches' '
grep "F2 + 2" actual
'
+test_expect_success '-L with --diff-filter=M excludes root commit' '
+ git checkout parent-oids &&
+ git log -L:func2:file.c --diff-filter=M --format=%s --no-patch >actual &&
+ # Root commit is an Add (A), not a Modify (M), so it should
+ # be excluded; only the modification commit remains.
+ echo "Modify func2() in file.c" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '-L with --diff-filter=A shows only root commit' '
+ git checkout parent-oids &&
+ git log -L:func2:file.c --diff-filter=A --format=%s --no-patch >actual &&
+ echo "Add func1() and func2() in file.c" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '-L with -S suppresses non-matching commits' '
+ git checkout parent-oids &&
+ git log -L:func2:file.c -S "F2 + 2" --format=%s --no-patch >actual &&
+ # Only the commit that changes the count of "F2 + 2" should appear.
+ echo "Modify func2() in file.c" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '--full-diff is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --full-diff 2>err &&
+ test_grep "does not yet support" err
+'
+
+test_expect_success '-L --oneline has no extra blank line before diff' '
+ git checkout parent-oids &&
+ git log --oneline -L:func2:file.c -1 >actual &&
+ # Oneline header on line 1, diff starts immediately on line 2
+ sed -n 2p actual >line2 &&
+ test_grep "^diff --git" line2
+'
+
test_done
diff --git a/t/t4211/sha1/expect.parallel-change-f-to-main b/t/t4211/sha1/expect.parallel-change-f-to-main
index 65a8cc673a..6d7a201036 100644
--- a/t/t4211/sha1/expect.parallel-change-f-to-main
+++ b/t/t4211/sha1/expect.parallel-change-f-to-main
@@ -5,7 +5,6 @@ Date: Fri Apr 12 16:16:24 2013 +0200
Merge across the rename
-
commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
Author: Thomas Rast <trast@student.ethz.ch>
Date: Thu Feb 28 10:49:50 2013 +0100
diff --git a/t/t4211/sha256/expect.parallel-change-f-to-main b/t/t4211/sha256/expect.parallel-change-f-to-main
index 3178989253..c93e03bef4 100644
--- a/t/t4211/sha256/expect.parallel-change-f-to-main
+++ b/t/t4211/sha256/expect.parallel-change-f-to-main
@@ -5,7 +5,6 @@ Date: Fri Apr 12 16:16:24 2013 +0200
Merge across the rename
-
commit 4f7a58195a92c400e28a2354328587f1ff14fb77f5cf894536f17ccbc72931b9
Author: Thomas Rast <trast@student.ethz.ch>
Date: Thu Feb 28 10:49:50 2013 +0100
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/3] revision: move -L setup before output_format-to-diff derivation
From: Michael Montalbo via GitGitGadget @ 2026-05-25 19:40 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2094.v2.git.1779738059.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
The line_level_traverse block sets a default DIFF_FORMAT_PATCH when
no output format has been explicitly requested. This default must
be visible to the "Did the user ask for any diff output?" check
that derives revs->diff from revs->diffopt.output_format.
Currently the -L block runs after that derivation, so revs->diff
stays 0 when no explicit format is given. This does not matter yet
because log_tree_commit() short-circuits into line_log_print()
before consulting revs->diff, but the next commit will route -L
through the normal log_tree_diff() path, which checks revs->diff.
Move the block above the derivation so the default DIFF_FORMAT_PATCH
is in place when revs->diff is computed. No behavior change on its
own.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
revision.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/revision.c b/revision.c
index 599b3a66c3..4a8e24bc38 100644
--- a/revision.c
+++ b/revision.c
@@ -3112,6 +3112,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
object_context_release(&oc);
}
+ if (revs->line_level_traverse) {
+ if (want_ancestry(revs))
+ revs->limited = 1;
+ revs->topo_order = 1;
+ if (!revs->diffopt.output_format)
+ revs->diffopt.output_format = DIFF_FORMAT_PATCH;
+ }
+
/* Did the user ask for any diff output? Run the diff! */
if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
revs->diff = 1;
@@ -3125,14 +3133,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->diffopt.objfind)
revs->simplify_history = 0;
- if (revs->line_level_traverse) {
- if (want_ancestry(revs))
- revs->limited = 1;
- revs->topo_order = 1;
- if (!revs->diffopt.output_format)
- revs->diffopt.output_format = DIFF_FORMAT_PATCH;
- }
-
if (revs->topo_order && !generation_numbers_enabled(the_repository))
revs->limited = 1;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/3] line-log: integrate -L with the standard log output pipeline
From: Michael Montalbo via GitGitGadget @ 2026-05-25 19:40 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Michael Montalbo
In-Reply-To: <pull.2094.git.1777349126.gitgitgadget@gmail.com>
Since its introduction, git log -L has short-circuited from
log_tree_commit() into its own output function, bypassing log_tree_diff()
and log_tree_diff_flush(). This skips no_free save/restore,
always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G,
--find-object) and --diff-filter cannot suppress commits whose pairs are all
filtered out, because show_log() runs before diffcore_std().
This series restructures the flow so that -L goes through the same
log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and
merge diffs, then uses that to enable several non-patch diff formats.
Patch 1: revision: move -L setup before output_format-to-diff derivation
Preparatory reorder in setup_revisions(). The -L block sets a default
DIFF_FORMAT_PATCH when no format is requested; move it before the derivation
of revs->diff from output_format so the default is visible to that check. No
behavior change on its own.
Patch 2: line-log: integrate -L output with the standard log-tree pipeline
Rename line_log_print() to line_log_queue_pairs(), stripping it down to only
queue pre-computed filepairs. log_tree_diff_flush() handles show_log(),
diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter
suppression, and aligns the commit/diff separator with the rest of log
output. Rejects --full-diff, which is not yet supported when filepairs are
pre-computed.
Patch 3: line-log: allow non-patch diff formats with -L
Expand the allowlist to accept --raw, --name-only, --name-status, and
--summary. These only read filepair metadata already set by the line-log
machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat)
remain blocked because they call compute_diffstat() on full blob content and
would show whole-file statistics rather than range-scoped ones.
Changes since v1:
* Patch 2: use !opt->loginfo return convention in log_tree_diff() to match
the existing single-parent and merge codepaths, instead of returning
log_tree_diff_flush() directly.
* Patch 2: reword the early-return removal to explicitly tie it to the
pipeline change.
* Patch 2: soften --full-diff rejection to "not yet supported".
* Patches 2-3: use test_grep consistently in new tests.
* Patch 2: replace sed | grep pipe with sed > file && test_grep for proper
exit status handling.
Michael Montalbo (3):
revision: move -L setup before output_format-to-diff derivation
line-log: integrate -L output with the standard log-tree pipeline
line-log: allow non-patch diff formats with -L
Documentation/line-range-options.adoc | 10 +-
line-log.c | 30 ++----
line-log.h | 2 +-
log-tree.c | 10 +-
revision.c | 24 +++--
t/t4211-line-log.sh | 100 +++++++++++++++---
t/t4211/sha1/expect.parallel-change-f-to-main | 1 -
.../sha256/expect.parallel-change-f-to-main | 1 -
8 files changed, 121 insertions(+), 57 deletions(-)
base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2094
Range-diff vs v1:
1: 9633eb62c6 = 1: 9633eb62c6 revision: move -L setup before output_format-to-diff derivation
2: 2d9e0ca015 ! 2: 7acfc5376e line-log: integrate -L output with the standard log-tree pipeline
@@ Commit message
log_tree_diff_flush(), mirroring the diff_tree_oid() + flush
pattern used by the single-parent and merge codepaths.
- - Remove the early return in log_tree_commit() that bypassed
- no_free save/restore, always_show_header, and diff_free().
+ - Remove the early return in log_tree_commit() that is no longer
+ needed now that -L output flows through log_tree_diff() and
+ log_tree_diff_flush(); this restores no_free save/restore,
+ always_show_header, and diff_free() cleanup.
Because show_log() is now deferred until after diffcore_std() inside
log_tree_diff_flush(), pickaxe (-S, -G, --find-object) and
@@ Commit message
log_tree_diff_flush() only emits one for verbose headers. This
matches the rest of log output.
- Also reject --full-diff, which is meaningless with -L: the filepairs
- are pre-computed during the history walk and scoped to tracked paths,
- so there is no tree diff to widen.
+ Also reject --full-diff, which is not yet supported with -L: the
+ filepairs are pre-computed during the history walk and scoped to
+ tracked line ranges, so there is currently no full-tree diff to
+ fall back to for display.
Update tests accordingly.
@@ log-tree.c: static int log_tree_diff(struct rev_info *opt, struct commit *commit
+ if (opt->line_level_traverse) {
+ line_log_queue_pairs(opt, commit);
-+ return log_tree_diff_flush(opt);
++ log_tree_diff_flush(opt);
++ return !opt->loginfo;
+ }
+
parse_commit_or_die(commit);
@@ log-tree.c: int log_tree_commit(struct rev_info *opt, struct commit *commit)
## revision.c ##
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
+ die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
+
if (revs->line_level_traverse &&
- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
- die(_("-L does not yet support diff formats besides -p and -s"));
-+ if (revs->line_level_traverse && revs->full_diff)
-+ die(_("-L is not compatible with --full-diff"));
+- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
+- die(_("-L does not yet support diff formats besides -p and -s"));
++ (revs->full_diff ||
++ (revs->diffopt.output_format &
++ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
++ die(_("-L does not yet support the requested diff format"));
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
@@ t/t4211-line-log.sh: test_expect_success '-L with -G filters to diff-text matche
+ test_cmp expect actual
+'
+
-+test_expect_success '--full-diff is not supported with -L' '
++test_expect_success '--full-diff is not yet supported with -L' '
+ test_must_fail git log -L1,24:b.c --full-diff 2>err &&
-+ test_grep "not compatible with --full-diff" err
++ test_grep "does not yet support" err
+'
+
+test_expect_success '-L --oneline has no extra blank line before diff' '
+ git checkout parent-oids &&
+ git log --oneline -L:func2:file.c -1 >actual &&
+ # Oneline header on line 1, diff starts immediately on line 2
-+ sed -n 2p actual | grep "^diff --git"
++ sed -n 2p actual >line2 &&
++ test_grep "^diff --git" line2
+'
+
test_done
3: 06c24b416f ! 3: 10a3d8dde2 line-log: allow non-patch diff formats with -L
@@ Documentation/line-range-options.adoc
## revision.c ##
@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
- die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
-
if (revs->line_level_traverse &&
-- (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
-- die(_("-L does not yet support diff formats besides -p and -s"));
-+ (revs->diffopt.output_format &
-+ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
-+ DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
-+ DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY)))
-+ die(_("-L does not yet support the requested diff format"));
- if (revs->line_level_traverse && revs->full_diff)
- die(_("-L is not compatible with --full-diff"));
+ (revs->full_diff ||
+ (revs->diffopt.output_format &
+- ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT))))
++ ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT |
++ DIFF_FORMAT_RAW | DIFF_FORMAT_NAME |
++ DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_SUMMARY))))
+ die(_("-L does not yet support the requested diff format"));
+ if (revs->expand_tabs_in_log < 0)
## t/t4211-line-log.sh ##
@@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
@@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
- test_must_fail git log -L1,24:b.c --raw
+test_expect_success '--raw shows mode, oid, status and path' '
+ git log -L1,24:b.c --raw --format= >actual &&
-+ grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M b.c$" actual &&
-+ ! grep "^diff --git" actual &&
-+ ! grep "^@@" actual
++ test_grep "^:100644 100644 [0-9a-f]\{7\} [0-9a-f]\{7\} M b.c$" actual &&
++ ! test_grep "^diff --git" actual &&
++ ! test_grep "^@@" actual
+'
+
+test_expect_success '--name-only shows path' '
+ git log -L1,24:b.c --name-only --format= >actual &&
-+ grep "^b.c$" actual &&
-+ ! grep "^diff --git" actual &&
-+ ! grep "^@@" actual
++ test_grep "^b.c$" actual &&
++ ! test_grep "^diff --git" actual &&
++ ! test_grep "^@@" actual
+'
+
+test_expect_success '--name-status shows status and path' '
+ git log -L1,24:b.c --name-status --format= >actual &&
-+ grep "^M b.c$" actual &&
-+ ! grep "^diff --git" actual &&
-+ ! grep "^@@" actual
++ test_grep "^M b.c$" actual &&
++ ! test_grep "^diff --git" actual &&
++ ! test_grep "^@@" actual
+'
+
+test_expect_success '--stat is not yet supported with -L' '
@@ t/t4211-line-log.sh: test_expect_success '-p shows the default patch output' '
test_expect_success 'setup for checking fancy rename following' '
@@ t/t4211-line-log.sh: test_expect_success '-L --oneline has no extra blank line before diff' '
- sed -n 2p actual | grep "^diff --git"
+ test_grep "^diff --git" line2
'
+test_expect_success '--summary shows new file on root commit' '
+ git checkout parent-oids &&
+ git log -L:func2:file.c --summary --format= >actual &&
-+ grep "create mode 100644 file.c" actual
++ test_grep "create mode 100644 file.c" actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 4/4] blame: consult diff process for zero-hunk detection
From: Michael Montalbo via GitGitGadget @ 2026-05-25 18:29 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2120.v2.git.1779733799.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
When a diff process is configured via diff.<driver>.process,
consult it during blame's per-commit diffing. If the process
returns zero hunks for a commit's changes to a file, treat the
commit as having no changes, causing blame to attribute lines
to earlier commits.
The subprocess is long-running (one startup cost amortized
across the blame traversal), but each commit in the file's
history incurs a round-trip to the tool.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
Documentation/gitattributes.adoc | 3 +++
blame.c | 43 +++++++++++++++++++++++++++++---
t/t4080-diff-process.sh | 32 ++++++++++++++++++++++++
3 files changed, 74 insertions(+), 4 deletions(-)
diff --git a/Documentation/gitattributes.adoc b/Documentation/gitattributes.adoc
index 962896a0b4..c087b4b265 100644
--- a/Documentation/gitattributes.adoc
+++ b/Documentation/gitattributes.adoc
@@ -857,6 +857,9 @@ The tool responds with lines of the form
If the tool returns zero hunks with `status=success`, Git treats
the file as having no changes and produces no diff output.
+`git blame` also consults the diff process and skips commits
+where it reports zero hunks, attributing lines to earlier commits
+instead.
Tools should ignore unknown keys in the per-file request to
remain forward-compatible.
diff --git a/blame.c b/blame.c
index a3c49d132e..8a5f14db7a 100644
--- a/blame.c
+++ b/blame.c
@@ -19,6 +19,8 @@
#include "tag.h"
#include "trace2.h"
#include "blame.h"
+#include "diff-process.h"
+#include "userdiff.h"
#include "alloc.h"
#include "commit-slab.h"
#include "bloom.h"
@@ -315,16 +317,47 @@ static struct commit *fake_working_tree_commit(struct repository *r,
static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
- xdl_emit_hunk_consume_func_t hunk_func, void *cb_data, int xdl_opts)
+ xdl_emit_hunk_consume_func_t hunk_func, void *cb_data,
+ int xdl_opts, struct index_state *istate,
+ const char *path)
{
xpparam_t xpp = {0};
xdemitconf_t xecfg = {0};
xdemitcb_t ecb = {NULL};
+ struct xdl_hunk *ext_hunks = NULL;
+ int ret;
xpp.flags = xdl_opts;
xecfg.hunk_func = hunk_func;
ecb.priv = cb_data;
- return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);
+
+ if (path && istate) {
+ struct userdiff_driver *drv;
+ drv = userdiff_find_by_path(istate, path);
+ if (drv && drv->process) {
+ size_t nr = 0;
+ if (!diff_process_get_hunks(drv, path,
+ file_a->ptr, file_a->size,
+ file_b->ptr, file_b->size,
+ &ext_hunks, &nr)) {
+ if (!nr) {
+ /*
+ * Zero hunks: the diff process
+ * considers these files equivalent.
+ * Skip so blame looks past this
+ * commit.
+ */
+ return 0;
+ }
+ xpp.external_hunks = ext_hunks;
+ xpp.external_hunks_nr = nr;
+ }
+ }
+ }
+
+ ret = xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);
+ free(ext_hunks);
+ return ret;
}
static const char *get_next_line(const char *start, const char *end)
@@ -1961,7 +1994,8 @@ static void pass_blame_to_parent(struct blame_scoreboard *sb,
&sb->num_read_blob, ignore_diffs);
sb->num_get_patch++;
- if (diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, sb->xdl_opts))
+ if (diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, sb->xdl_opts,
+ sb->revs->diffopt.repo->index, target->path))
die("unable to generate diff (%s -> %s)",
oid_to_hex(&parent->commit->object.oid),
oid_to_hex(&target->commit->object.oid));
@@ -2114,7 +2148,8 @@ static void find_copy_in_blob(struct blame_scoreboard *sb,
* file_p partially may match that image.
*/
memset(split, 0, sizeof(struct blame_entry [3]));
- if (diff_hunks(file_p, &file_o, handle_split_cb, &d, sb->xdl_opts))
+ if (diff_hunks(file_p, &file_o, handle_split_cb, &d, sb->xdl_opts,
+ NULL, NULL))
die("unable to generate diff (%s)",
oid_to_hex(&parent->commit->object.oid));
/* remainder, if any, all match the preimage */
diff --git a/t/t4080-diff-process.sh b/t/t4080-diff-process.sh
index 083e48e872..50f49a9b02 100755
--- a/t/t4080-diff-process.sh
+++ b/t/t4080-diff-process.sh
@@ -335,4 +335,36 @@ test_expect_success PYTHON 'diff process zero hunks suppresses diff output' '
test_must_be_empty actual
'
+test_expect_success PYTHON 'blame skips commits with zero hunks from diff process' '
+ cat >blame.c <<-\EOF &&
+ int main(void)
+ {
+ return 0;
+ }
+ EOF
+ git add blame.c &&
+ git commit -m "add blame.c" &&
+
+ cat >blame.c <<-\EOF &&
+ int main(void)
+ {
+ return 0;
+ }
+ EOF
+ git add blame.c &&
+ git commit -m "reformat blame.c" &&
+ BLAME_COMMIT=$(git rev-parse --short HEAD) &&
+
+ # Without zero-hunk mode, blame attributes the change.
+ git blame blame.c >without &&
+ test_grep "$BLAME_COMMIT" without &&
+
+ # With zero-hunk mode, the process considers the files equivalent
+ # and blame skips the reformat commit.
+ git -c diff.cdiff.process="$BACKEND --mode=zero-hunk" \
+ blame blame.c >with &&
+ ! test_grep "$BLAME_COMMIT" with
+'
+
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/4] diff: add long-running diff process via diff.<driver>.process
From: Michael Montalbo via GitGitGadget @ 2026-05-25 18:29 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2120.v2.git.1779733799.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
Add support for external diff processes that communicate via the
long-running process protocol (pkt-line over stdin/stdout).
A diff process is configured per userdiff driver:
[diff "cdiff"]
process = /path/to/diff-tool
The tool provides custom line-matching: it receives file pairs
and returns hunks that reference original line numbers. Unlike
textconv, which transforms the displayed content, the diff
output shows the actual file while the tool controls which
lines are marked as changed.
The handshake negotiates version=1 and capability=hunks. Per-file
requests send command=hunks, pathname, and both file contents as
packetized data. The tool responds with hunk lines and a status
packet. On error, git falls back to the builtin diff algorithm
with a warning.
Zero hunks with status=success means the tool considers the
files equivalent. Git skips diff output for that file.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
Documentation/config/diff.adoc | 8 +
Documentation/gitattributes.adoc | 40 ++++
Makefile | 1 +
diff-process.c | 206 +++++++++++++++++++
diff-process.h | 28 +++
diff.c | 23 +++
t/.gitattributes | 1 +
t/t4080-diff-process.sh | 338 +++++++++++++++++++++++++++++++
8 files changed, 645 insertions(+)
create mode 100644 diff-process.c
create mode 100644 diff-process.h
create mode 100755 t/t4080-diff-process.sh
diff --git a/Documentation/config/diff.adoc b/Documentation/config/diff.adoc
index 1135a62a0a..4ab5f60df6 100644
--- a/Documentation/config/diff.adoc
+++ b/Documentation/config/diff.adoc
@@ -218,6 +218,14 @@ endif::git-diff[]
Set this option to `true` to make the diff driver cache the text
conversion outputs. See linkgit:gitattributes[5] for details.
+`diff.<driver>.process`::
+ The command to run as a long-running diff process.
+ The tool communicates via the pkt-line protocol and returns
+ hunks that are fed into Git's diff and blame pipelines.
+ If the tool returns zero hunks, the file is treated as
+ unchanged for both diff output and blame attribution.
+ See linkgit:gitattributes[5] for details.
+
`diff.indentHeuristic`::
Set this option to `false` to disable the default heuristics
that shift diff hunk boundaries to make patches easier to read.
diff --git a/Documentation/gitattributes.adoc b/Documentation/gitattributes.adoc
index f20041a323..962896a0b4 100644
--- a/Documentation/gitattributes.adoc
+++ b/Documentation/gitattributes.adoc
@@ -821,6 +821,46 @@ NOTE: If `diff.<name>.command` is defined for path with the
(see above), and adding `diff.<name>.algorithm` has no effect, as the
algorithm is not passed to the external diff driver.
+Using an external diff process
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+An external tool can provide content-aware line matching by
+setting `diff.<name>.process` to the command that runs
+the tool. The tool is a long-running process that communicates via
+the pkt-line protocol (described in
+Documentation/technical/long-running-process-protocol.adoc).
+
+------------------------
+*.c diff=cdiff
+------------------------
+
+----------------------------------------------------------------
+[diff "cdiff"]
+ process = /path/to/diff-process-tool
+----------------------------------------------------------------
+
+The tool receives file pairs and returns hunk descriptors indicating
+which lines changed. Git feeds these hunks into its standard diff
+pipeline, so all output features (word diff, function context,
+color) work normally.
+
+If the tool fails or returns an error, Git silently falls back to
+the builtin diff algorithm. If the tool returns invalid hunks
+(out of bounds, overlapping), Git also falls back silently.
+
+The handshake negotiates `version=1` and `capability=hunks`.
+Per-file requests send `command=hunks` and `pathname=<path>`,
+followed by the old and new file content as packetized data.
+The tool responds with lines of the form
+`hunk <old_start> <old_count> <new_start> <new_count>`
+(1-based line numbers), a flush packet, and `status=success`.
+
+If the tool returns zero hunks with `status=success`, Git treats
+the file as having no changes and produces no diff output.
+
+Tools should ignore unknown keys in the per-file request to
+remain forward-compatible.
+
Defining a custom hunk-header
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/Makefile b/Makefile
index cedc234173..22900368dd 100644
--- a/Makefile
+++ b/Makefile
@@ -1142,6 +1142,7 @@ LIB_OBJS += diff-delta.o
LIB_OBJS += diff-merges.o
LIB_OBJS += diff-lib.o
LIB_OBJS += diff-no-index.o
+LIB_OBJS += diff-process.o
LIB_OBJS += diff.o
LIB_OBJS += diffcore-break.o
LIB_OBJS += diffcore-delta.o
diff --git a/diff-process.c b/diff-process.c
new file mode 100644
index 0000000000..801ac9e22e
--- /dev/null
+++ b/diff-process.c
@@ -0,0 +1,206 @@
+/*
+ * Diff process backend: communicates with a long-running external
+ * tool via the pkt-line protocol to obtain custom line-matching
+ * results. Unlike textconv, which transforms the displayed content,
+ * hunks from a diff process reference original line numbers and
+ * the display shows the actual file content.
+ *
+ * Protocol: pkt-line over stdin/stdout, following the pattern of
+ * the long-running filter process protocol (see convert.c).
+ *
+ * Handshake:
+ * git> git-diff-client / version=1 / flush
+ * tool< git-diff-server / version=1 / flush
+ * git> capability=hunks / flush
+ * tool< capability=hunks / flush
+ *
+ * Per-file:
+ * git> command=hunks / pathname=<path> / flush
+ * git> <old content packetized> / flush
+ * git> <new content packetized> / flush
+ * tool< hunk <old_start> <old_count> <new_start> <new_count>
+ * tool< ... / flush
+ * tool< status=success / flush
+ *
+ * Zero hunks with status=success means the tool considers the
+ * files equivalent. Git will skip the diff for that file.
+ */
+
+#include "git-compat-util.h"
+#include "diff-process.h"
+#include "userdiff.h"
+#include "sub-process.h"
+#include "pkt-line.h"
+#include "strbuf.h"
+#include "xdiff/xdiff.h"
+
+#define CAP_HUNKS (1u << 0)
+
+struct diff_subprocess {
+ struct subprocess_entry subprocess;
+ unsigned int supported_capabilities;
+};
+
+static int subprocess_map_initialized;
+static struct hashmap subprocess_map;
+
+static int start_diff_process_fn(struct subprocess_entry *subprocess)
+{
+ static int versions[] = { 1, 0 };
+ static struct subprocess_capability capabilities[] = {
+ { "hunks", CAP_HUNKS },
+ { NULL, 0 }
+ };
+ struct diff_subprocess *entry =
+ (struct diff_subprocess *)subprocess;
+
+ /* Uses dying pkt-line variant, same as convert.c filters. */
+ return subprocess_handshake(subprocess, "git-diff",
+ versions, NULL,
+ capabilities,
+ &entry->supported_capabilities);
+}
+
+static struct diff_subprocess *find_or_start_process(const char *cmd)
+{
+ struct diff_subprocess *entry;
+
+ if (!subprocess_map_initialized) {
+ subprocess_map_initialized = 1;
+ hashmap_init(&subprocess_map, cmd2process_cmp, NULL, 0);
+ }
+
+ entry = (struct diff_subprocess *)
+ subprocess_find_entry(&subprocess_map, cmd);
+ if (entry)
+ return entry;
+
+ entry = xcalloc(1, sizeof(*entry));
+ if (subprocess_start(&subprocess_map, &entry->subprocess,
+ cmd, start_diff_process_fn)) {
+ free(entry);
+ return NULL;
+ }
+
+ return entry;
+}
+
+static int send_file_content(int fd, const char *buf, long size)
+{
+ int ret;
+
+ if (size > 0)
+ ret = write_packetized_from_buf_no_flush(buf, size, fd);
+ else
+ ret = 0;
+ if (ret)
+ return ret;
+ return packet_flush_gently(fd);
+}
+
+static int parse_hunk_line(const char *line, struct xdl_hunk *hunk)
+{
+ char *end;
+
+ /* Format: "hunk <old_start> <old_count> <new_start> <new_count>" */
+ if (!skip_prefix(line, "hunk ", &line))
+ return -1;
+
+ hunk->old_start = strtol(line, &end, 10);
+ if (end == line || *end != ' ')
+ return -1;
+ line = end;
+
+ hunk->old_count = strtol(line, &end, 10);
+ if (end == line || *end != ' ')
+ return -1;
+ line = end;
+
+ hunk->new_start = strtol(line, &end, 10);
+ if (end == line || *end != ' ')
+ return -1;
+ line = end;
+
+ hunk->new_count = strtol(line, &end, 10);
+ if (end == line || *end != '\0')
+ return -1;
+
+ return 0;
+}
+
+int diff_process_get_hunks(struct userdiff_driver *drv,
+ const char *path,
+ const char *old_buf, long old_size,
+ const char *new_buf, long new_size,
+ struct xdl_hunk **hunks_out,
+ size_t *nr_hunks_out)
+{
+ struct diff_subprocess *backend;
+ struct child_process *process;
+ int fd_in, fd_out;
+ struct strbuf status = STRBUF_INIT;
+ struct xdl_hunk *hunks = NULL;
+ struct xdl_hunk hunk;
+ size_t nr_hunks = 0, alloc_hunks = 0;
+ int len;
+ char *line;
+
+ if (!drv || !drv->process)
+ return -1;
+
+ backend = find_or_start_process(drv->process);
+ if (!backend)
+ return -1;
+
+ if (!(backend->supported_capabilities & CAP_HUNKS))
+ return -1;
+
+ process = subprocess_get_child_process(&backend->subprocess);
+ fd_in = process->in;
+ fd_out = process->out;
+
+ /* Send request */
+ if (packet_write_fmt_gently(fd_in, "command=hunks\n") ||
+ packet_write_fmt_gently(fd_in, "pathname=%s\n", path) ||
+ packet_flush_gently(fd_in))
+ goto error;
+
+ /* Send old file content */
+ if (send_file_content(fd_in, old_buf, old_size))
+ goto error;
+
+ /* Send new file content */
+ if (send_file_content(fd_in, new_buf, new_size))
+ goto error;
+
+ /* Read hunks until flush packet */
+ while ((len = packet_read_line_gently(fd_out, NULL, &line)) >= 0 &&
+ line) {
+ if (parse_hunk_line(line, &hunk) < 0)
+ goto error;
+ ALLOC_GROW(hunks, nr_hunks + 1, alloc_hunks);
+ hunks[nr_hunks++] = hunk;
+ }
+ if (len < 0)
+ goto error;
+
+ /* Read status */
+ if (subprocess_read_status(fd_out, &status))
+ goto error;
+
+ if (strcmp(status.buf, "success")) {
+ if (!strcmp(status.buf, "abort"))
+ backend->supported_capabilities &= ~CAP_HUNKS;
+ goto error;
+ }
+
+ *hunks_out = hunks;
+ *nr_hunks_out = nr_hunks;
+ strbuf_release(&status);
+ return 0;
+
+error:
+ free(hunks);
+ strbuf_release(&status);
+ return -1;
+}
diff --git a/diff-process.h b/diff-process.h
new file mode 100644
index 0000000000..4c84951e02
--- /dev/null
+++ b/diff-process.h
@@ -0,0 +1,28 @@
+#ifndef DIFF_PROCESS_H
+#define DIFF_PROCESS_H
+
+struct userdiff_driver;
+struct xdl_hunk;
+
+/*
+ * Query a diff process for hunks describing the changes
+ * between old_buf and new_buf.
+ *
+ * The backend is a long-running subprocess configured via
+ * diff.<driver>.process. It receives file content via
+ * pkt-line and returns hunks with 1-based line numbers.
+ *
+ * On success, sets *hunks_out and *nr_hunks_out to a newly allocated
+ * array (caller must free) and returns 0.
+ *
+ * On failure, returns -1. The caller should fall back to the
+ * builtin diff algorithm.
+ */
+int diff_process_get_hunks(struct userdiff_driver *drv,
+ const char *path,
+ const char *old_buf, long old_size,
+ const char *new_buf, long new_size,
+ struct xdl_hunk **hunks_out,
+ size_t *nr_hunks_out);
+
+#endif /* DIFF_PROCESS_H */
diff --git a/diff.c b/diff.c
index 397e38b41c..1aeb0f319e 100644
--- a/diff.c
+++ b/diff.c
@@ -25,6 +25,7 @@
#include "utf8.h"
#include "odb.h"
#include "userdiff.h"
+#include "diff-process.h"
#include "submodule.h"
#include "hashmap.h"
#include "mem-pool.h"
@@ -3991,6 +3992,7 @@ static void builtin_diff(const char *name_a,
xpparam_t xpp;
xdemitconf_t xecfg;
struct emit_callback ecbdata;
+ struct xdl_hunk *ext_hunks = NULL;
unsigned ws_rule;
const struct userdiff_funcname *pe;
@@ -4031,6 +4033,26 @@ static void builtin_diff(const char *name_a,
xpp.ignore_regex_nr = o->ignore_regex_nr;
xpp.anchors = o->anchors;
xpp.anchors_nr = o->anchors_nr;
+
+ if (!o->ignore_driver_algorithm &&
+ one->driver && one->driver->process) {
+ size_t ext_hunks_nr = 0;
+ if (!diff_process_get_hunks(
+ one->driver, name_a,
+ mf1.ptr, mf1.size,
+ mf2.ptr, mf2.size,
+ &ext_hunks, &ext_hunks_nr)) {
+ if (!ext_hunks_nr)
+ goto free_ab_and_return;
+ xpp.external_hunks = ext_hunks;
+ xpp.external_hunks_nr = ext_hunks_nr;
+ } else {
+ warning(_("diff process failed for '%s',"
+ " falling back to builtin diff"),
+ name_a);
+ }
+ }
+
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xecfg.flags = XDL_EMIT_FUNCNAMES;
@@ -4111,6 +4133,7 @@ static void builtin_diff(const char *name_a,
} else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume,
&ecbdata, &xpp, &xecfg))
die("unable to generate diff for %s", one->path);
+ free(ext_hunks);
if (o->word_diff)
free_diff_words_data(&ecbdata);
if (textconv_one)
diff --git a/t/.gitattributes b/t/.gitattributes
index 7664c6e027..de97920cab 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -23,3 +23,4 @@ t[0-9][0-9][0-9][0-9]/* -whitespace
/t8005/*.txt eol=lf
/t9*/*.dump eol=lf
/t0040*.sh whitespace=-indent-with-non-tab
+/t4080-diff-process.sh whitespace=-indent-with-non-tab
diff --git a/t/t4080-diff-process.sh b/t/t4080-diff-process.sh
new file mode 100755
index 0000000000..083e48e872
--- /dev/null
+++ b/t/t4080-diff-process.sh
@@ -0,0 +1,338 @@
+#!/bin/sh
+
+test_description='diff process via long-running process'
+
+. ./test-lib.sh
+
+if test_have_prereq PYTHON
+then
+ PYTHON_PATH=$(command -v python3) || PYTHON_PATH=$(command -v python)
+fi
+
+#
+# A single parametric diff process.
+# Usage: diff-process-backend --mode=<mode> [--log=<path>]
+#
+# Modes:
+# whole-file - report all lines as changed (default)
+# fixed-hunk - always report hunk 5 2 5 2
+# bad-hunk - report out-of-bounds hunk 999 1 999 1
+# zero-hunk - return zero hunks (files considered equivalent)
+# error - return status=error for every request
+# abort - return status=abort for every request
+# crash - read one request then exit without responding
+#
+setup_backend () {
+ cat >"$TRASH_DIRECTORY/diff-process-backend.py" <<-\PYEOF
+ import sys, os
+
+ def read_pkt():
+ hdr = sys.stdin.buffer.read(4)
+ if len(hdr) < 4: return None
+ length = int(hdr, 16)
+ if length == 0: return ""
+ data = sys.stdin.buffer.read(length - 4)
+ return data.decode().rstrip("\n")
+
+ def write_pkt(line):
+ data = (line + "\n").encode()
+ sys.stdout.buffer.write(f"{len(data)+4:04x}".encode() + data)
+ sys.stdout.buffer.flush()
+
+ def write_flush():
+ sys.stdout.buffer.write(b"0000")
+ sys.stdout.buffer.flush()
+
+ def read_content():
+ chunks = []
+ while True:
+ hdr = sys.stdin.buffer.read(4)
+ if len(hdr) < 4: break
+ length = int(hdr, 16)
+ if length == 0: break
+ chunks.append(sys.stdin.buffer.read(length - 4))
+ return b"".join(chunks)
+
+ mode = "whole-file"
+ logfile = None
+ for arg in sys.argv[1:]:
+ if arg.startswith("--mode="):
+ mode = arg[7:]
+ elif arg.startswith("--log="):
+ logfile = open(arg[6:], "a")
+
+ def log(msg):
+ if logfile:
+ logfile.write(msg + "\n")
+ logfile.flush()
+
+ # Handshake
+ assert read_pkt() == "git-diff-client"
+ assert read_pkt() == "version=1"
+ read_pkt()
+ write_pkt("git-diff-server")
+ write_pkt("version=1")
+ write_flush()
+ while True:
+ p = read_pkt()
+ if p == "": break
+ write_pkt("capability=hunks")
+ write_flush()
+
+ log("ready")
+
+ while True:
+ cmd = None
+ pathname = None
+ while True:
+ p = read_pkt()
+ if p is None: sys.exit(0)
+ if p == "": break
+ if p.startswith("command="): cmd = p.split("=",1)[1]
+ if p.startswith("pathname="): pathname = p.split("=",1)[1]
+ if cmd is None: sys.exit(0)
+ old = read_content()
+ new = read_content()
+ log(f"command={cmd} pathname={pathname}")
+
+ if mode == "error":
+ write_flush()
+ write_pkt("status=error")
+ write_flush()
+ continue
+
+ if mode == "abort":
+ write_flush()
+ write_pkt("status=abort")
+ write_flush()
+ continue
+
+ if mode == "crash":
+ sys.exit(1)
+
+ if cmd == "hunks":
+ if mode == "fixed-hunk":
+ write_pkt("hunk 5 2 5 2")
+ elif mode == "bad-hunk":
+ write_pkt("hunk 999 1 999 1")
+ elif mode == "zero-hunk":
+ pass
+ else:
+ ol = len(old.split(b"\n"))
+ nl = len(new.split(b"\n"))
+ write_pkt(f"hunk 1 {ol} 1 {nl}")
+ write_flush()
+ write_pkt("status=success")
+ write_flush()
+ else:
+ write_flush()
+ write_pkt("status=error")
+ write_flush()
+ PYEOF
+ write_script diff-process-backend <<-SHEOF
+ exec "$PYTHON_PATH" "$TRASH_DIRECTORY/diff-process-backend.py" "\$@"
+ SHEOF
+}
+
+BACKEND="./diff-process-backend"
+
+test_expect_success PYTHON 'setup' '
+ setup_backend &&
+ echo "*.c diff=cdiff" >.gitattributes &&
+ git add .gitattributes &&
+ git commit -m "initial"
+'
+
+test_expect_success PYTHON 'diff process hunk boundaries affect output' '
+ cat >boundary.c <<-\EOF &&
+ line1
+ line2
+ line3
+ line4
+ OLD5
+ OLD6
+ line7
+ line8
+ OLD9
+ OLD10
+ EOF
+ git add boundary.c &&
+ git commit -m "add boundary.c" &&
+
+ cat >boundary.c <<-\EOF &&
+ line1
+ line2
+ line3
+ line4
+ NEW5
+ NEW6
+ line7
+ line8
+ NEW9
+ NEW10
+ EOF
+
+ # The file has changes at lines 5-6 and 9-10, but fixed-hunk
+ # only reports lines 5-6 as changed. Lines 9-10 should not
+ # appear as changed in the output.
+ git -c diff.cdiff.process="$BACKEND --mode=fixed-hunk" \
+ diff boundary.c >actual &&
+ test_grep "^-OLD5" actual &&
+ test_grep "^-OLD6" actual &&
+ test_grep "^+NEW5" actual &&
+ test_grep "^+NEW6" actual &&
+ ! test_grep "^-OLD9" actual &&
+ ! test_grep "^-OLD10" actual &&
+ ! test_grep "^+NEW9" actual &&
+ ! test_grep "^+NEW10" actual
+'
+
+test_expect_success PYTHON 'diff process fallback on tool error status' '
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --mode=error --log=backend.log" \
+ diff boundary.c >actual &&
+ # Fallback produces the full builtin diff (both change regions).
+ test_grep "^-OLD5" actual &&
+ test_grep "^+NEW5" actual &&
+ test_grep "^-OLD9" actual &&
+ test_grep "^+NEW9" actual &&
+ # Tool was contacted (it replied with error, not crash).
+ test_grep "command=hunks pathname=boundary.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process fallback on bad hunks' '
+ git -c diff.cdiff.process="$BACKEND --mode=bad-hunk" \
+ diff boundary.c >actual &&
+ test_grep "^-OLD5" actual &&
+ test_grep "^+NEW5" actual &&
+ test_grep "^-OLD9" actual &&
+ test_grep "^+NEW9" actual
+'
+
+test_expect_success PYTHON 'diff process fallback on tool crash' '
+ git -c diff.cdiff.process="$BACKEND --mode=crash" \
+ diff boundary.c >actual &&
+ test_grep "^-OLD5" actual &&
+ test_grep "^+NEW5" actual &&
+ test_grep "^-OLD9" actual &&
+ test_grep "^+NEW9" actual
+'
+
+test_expect_success PYTHON 'diff process abort disables for session' '
+ cat >abort1.c <<-\EOF &&
+ int first(void) { return 1; }
+ EOF
+ cat >abort2.c <<-\EOF &&
+ int second(void) { return 2; }
+ EOF
+ git add abort1.c abort2.c &&
+ git commit -m "add abort files" &&
+
+ cat >abort1.c <<-\EOF &&
+ int first(void) { return 10; }
+ EOF
+ cat >abort2.c <<-\EOF &&
+ int second(void) { return 20; }
+ EOF
+
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --mode=abort --log=backend.log" \
+ diff -- abort1.c abort2.c >actual &&
+ # Both files should still produce diff output via fallback.
+ test_grep "return 10" actual &&
+ test_grep "return 20" actual &&
+ # The tool aborts on the first file and git clears its
+ # capability. The second file never contacts the tool,
+ # so the log should have exactly one entry, not two.
+ test_grep "command=hunks" backend.log >matches &&
+ test_line_count = 1 matches
+'
+
+test_expect_success PYTHON 'diff process handles multiple files' '
+ cat >multi1.c <<-\EOF &&
+ int one(void) { return 1; }
+ EOF
+ cat >multi2.c <<-\EOF &&
+ int two(void) { return 2; }
+ EOF
+ git add multi1.c multi2.c &&
+ git commit -m "add multi files" &&
+
+ cat >multi1.c <<-\EOF &&
+ int one(void) { return 10; }
+ EOF
+ cat >multi2.c <<-\EOF &&
+ int two(void) { return 20; }
+ EOF
+
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ diff -- multi1.c multi2.c >actual &&
+ test_grep "return 10" actual &&
+ test_grep "return 20" actual &&
+ test_grep "pathname=multi1.c" backend.log &&
+ test_grep "pathname=multi2.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process with --word-diff' '
+ cat >worddiff.c <<-\EOF &&
+ int value(void) { return 1; }
+ EOF
+ git add worddiff.c &&
+ git commit -m "add worddiff.c" &&
+
+ cat >worddiff.c <<-\EOF &&
+ int value(void) { return 999; }
+ EOF
+
+ git -c diff.cdiff.process="$BACKEND" \
+ diff --word-diff worddiff.c >actual &&
+ test_grep "\[-1;-\]" actual &&
+ test_grep "{+999;+}" actual
+'
+
+test_expect_success PYTHON 'diff process bypassed by --diff-algorithm' '
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ diff --diff-algorithm=patience worddiff.c >actual &&
+ test_grep "return 999" actual &&
+ test_path_is_missing backend.log
+'
+
+test_expect_success PYTHON 'diff process works with git log -p' '
+ cat >logtest.c <<-\EOF &&
+ int logfunc(void) { return 1; }
+ EOF
+ git add logtest.c &&
+ git commit -m "add logtest.c" &&
+
+ cat >logtest.c <<-\EOF &&
+ int logfunc(void) { return 2; }
+ EOF
+ git add logtest.c &&
+ git commit -m "change logtest.c" &&
+
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ log -1 -p -- logtest.c >actual &&
+ test_grep "return 2" actual &&
+ test_grep "command=hunks pathname=logtest.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process zero hunks suppresses diff output' '
+ cat >zerohunk.c <<-\EOF &&
+ int zero(void) { return 0; }
+ EOF
+ git add zerohunk.c &&
+ git commit -m "add zerohunk.c" &&
+
+ cat >zerohunk.c <<-\EOF &&
+ int zero(void) { return 999; }
+ EOF
+
+ git -c diff.cdiff.process="$BACKEND --mode=zero-hunk" \
+ diff zerohunk.c >actual &&
+ test_must_be_empty actual
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/4] userdiff: add diff.<driver>.process config
From: Michael Montalbo via GitGitGadget @ 2026-05-25 18:29 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2120.v2.git.1779733799.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
Add a new per-driver configuration key that specifies the command
for a long-running diff process.
The name follows filter.<driver>.process: a long-running subprocess
that stays alive across files within a single git invocation.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
userdiff.c | 7 +++++++
userdiff.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/userdiff.c b/userdiff.c
index fe710a68bf..81c0bebcce 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -499,6 +499,13 @@ int userdiff_config(const char *k, const char *v)
drv->algorithm = drv->algorithm_owned;
return ret;
}
+ if (!strcmp(type, "process")) {
+ int ret;
+ FREE_AND_NULL(drv->process_owned);
+ ret = git_config_string(&drv->process_owned, k, v);
+ drv->process = drv->process_owned;
+ return ret;
+ }
return 0;
}
diff --git a/userdiff.h b/userdiff.h
index 827361b0bc..51c26e0d41 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -31,6 +31,8 @@ struct userdiff_driver {
char *textconv_owned;
struct notes_cache *textconv_cache;
int textconv_want_cache;
+ const char *process;
+ char *process_owned;
};
enum userdiff_driver_type {
USERDIFF_DRIVER_TYPE_BUILTIN = 1<<0,
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/4] xdiff: support external hunks via xpparam_t
From: Michael Montalbo via GitGitGadget @ 2026-05-25 18:29 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo, Michael Montalbo
In-Reply-To: <pull.2120.v2.git.1779733799.gitgitgadget@gmail.com>
From: Michael Montalbo <mmontalbo@gmail.com>
Add two new xpparam_t fields (external_hunks, external_hunks_nr)
that let callers supply pre-computed hunks. When set, xdl_diff()
populates the changed[] arrays from these hunks instead of running
the diff algorithm, then continues through compaction and emission
as usual.
Validate supplied hunks before use: reject out-of-bounds line
numbers, overlapping or out-of-order hunks, negative counts, and
violations of the synchronization invariant (unchanged line counts
must match between files). On validation failure, fall back to
the builtin diff algorithm.
Skip trim_common_tail() in xdi_diff() when external hunks are
present, since external hunks reference line numbers in the
original content.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
xdiff-interface.c | 7 +++-
xdiff/xdiff.h | 13 ++++++++
xdiff/xdiffi.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
xdiff/xprepare.c | 10 ++++++
xdiff/xprepare.h | 1 +
5 files changed, 112 insertions(+), 3 deletions(-)
diff --git a/xdiff-interface.c b/xdiff-interface.c
index f043330f2a..9542c0bcc2 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -124,7 +124,12 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t co
if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
return -1;
- if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
+ /*
+ * External hunks reference line numbers in the original content;
+ * trimming the tail would change line counts and invalidate them.
+ */
+ if (!xpp->external_hunks &&
+ !xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
trim_common_tail(&a, &b);
return xdl_diff(&a, &b, xpp, xecfg, xecb);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index dc370712e9..2ee6f1aae3 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -78,6 +78,15 @@ typedef struct s_mmbuffer {
long size;
} mmbuffer_t;
+/*
+ * Hunk descriptor for externally computed diffs.
+ * Line numbers are 1-based, matching unified diff convention.
+ */
+struct xdl_hunk {
+ long old_start, old_count;
+ long new_start, new_count;
+};
+
typedef struct s_xpparam {
unsigned long flags;
@@ -88,6 +97,10 @@ typedef struct s_xpparam {
/* See Documentation/diff-options.adoc. */
char **anchors;
size_t anchors_nr;
+
+ /* Externally computed hunks: bypass the diff algorithm. */
+ const struct xdl_hunk *external_hunks;
+ size_t external_hunks_nr;
} xpparam_t;
typedef struct s_xdemitcb {
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 5455b4690d..e7d6190d37 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -1085,16 +1085,96 @@ static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,
}
}
+/*
+ * Populate the changed[] arrays from externally supplied hunks,
+ * bypassing the diff algorithm. Validates that hunks are in order,
+ * non-overlapping, and within bounds.
+ *
+ * Returns 0 on success, -1 on validation failure.
+ */
+static int xdl_populate_hunks_from_external(xdfenv_t *xe,
+ const struct xdl_hunk *hunks,
+ size_t nr_hunks)
+{
+ size_t i;
+ long j, prev_old_end = 0, prev_new_end = 0;
+ long total_old = 0, total_new = 0;
+
+ xdl_clear_changed(&xe->xdf1);
+ xdl_clear_changed(&xe->xdf2);
+
+ for (i = 0; i < nr_hunks; i++) {
+ const struct xdl_hunk *h = &hunks[i];
+
+ if (h->old_count < 0 || h->new_count < 0)
+ return -1;
+
+ /* Bounds check (1-based line numbers) */
+ if (h->old_count > 0 &&
+ (h->old_start < 1 ||
+ h->old_start + h->old_count - 1 > (long)xe->xdf1.nrec))
+ return -1;
+ if (h->new_count > 0 &&
+ (h->new_start < 1 ||
+ h->new_start + h->new_count - 1 > (long)xe->xdf2.nrec))
+ return -1;
+
+ /* Zero-count hunks: start must still be in [1, nrec+1] */
+ if (h->old_count == 0 &&
+ (h->old_start < 1 || h->old_start > (long)xe->xdf1.nrec + 1))
+ return -1;
+ if (h->new_count == 0 &&
+ (h->new_start < 1 || h->new_start > (long)xe->xdf2.nrec + 1))
+ return -1;
+
+ /* Ordering: no overlap with previous hunk */
+ if (h->old_start < prev_old_end ||
+ h->new_start < prev_new_end)
+ return -1;
+
+ for (j = 0; j < h->old_count; j++)
+ xe->xdf1.changed[h->old_start - 1 + j] = true;
+ for (j = 0; j < h->new_count; j++)
+ xe->xdf2.changed[h->new_start - 1 + j] = true;
+
+ prev_old_end = h->old_start + h->old_count;
+ prev_new_end = h->new_start + h->new_count;
+ total_old += h->old_count;
+ total_new += h->new_count;
+ }
+
+ /*
+ * Synchronization invariant: unchanged line counts must match.
+ * Otherwise xdl_build_script() would walk off one array.
+ */
+ if ((long)xe->xdf1.nrec - total_old !=
+ (long)xe->xdf2.nrec - total_new)
+ return -1;
+
+ return 0;
+}
+
int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
xdchange_t *xscr;
xdfenv_t xe;
emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
- if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
+ if (xpp->external_hunks) {
+ if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
+ return -1;
+ if (xdl_populate_hunks_from_external(&xe,
+ xpp->external_hunks,
+ xpp->external_hunks_nr) == 0)
+ goto diff_done;
+ xdl_free_env(&xe);
+ }
+ if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
return -1;
- }
+
+diff_done:
+
if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
xdl_build_script(&xe, &xscr) < 0) {
diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index cd4fc405eb..4645a9a746 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -432,3 +432,13 @@ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
return 0;
}
+
+/*
+ * Reset the changed[] array so that no lines are marked as changed.
+ * Also clears the sentinel slots at changed[-1] and changed[nrec]
+ * that xdl_change_compact() relies on during backward scans.
+ */
+void xdl_clear_changed(xdfile_t *xdf)
+{
+ memset(xdf->changed - 1, 0, (xdf->nrec + 2) * sizeof(bool));
+}
diff --git a/xdiff/xprepare.h b/xdiff/xprepare.h
index 947d9fc1bb..0413baf07b 100644
--- a/xdiff/xprepare.h
+++ b/xdiff/xprepare.h
@@ -28,6 +28,7 @@
int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
xdfenv_t *xe);
void xdl_free_env(xdfenv_t *xe);
+void xdl_clear_changed(xdfile_t *xdf);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/4] [RFC] diff: add diff.<driver>.process for external hunk providers
From: Michael Montalbo via GitGitGadget @ 2026-05-25 18:29 UTC (permalink / raw)
To: git; +Cc: Michael Montalbo
In-Reply-To: <pull.2120.git.1779415884.gitgitgadget@gmail.com>
This series adds diff.<driver>.process, a long-running subprocess protocol
that lets external tools provide hunks into git's diff and blame pipelines.
diff.<driver>.process opens the door to integrating content-aware logic that
don't exist inside git: structural diff tools, format-aware analyzers, and
other tools that understand the semantics of the content being tracked.
Where diff.<driver>.command replaces the diff pipeline entirely,
diff.<driver>.process feeds hunks into it, so all downstream features (word
diff, function context, color-moved, stat, blame) work normally.
The protocol follows filter.<driver>.process: pkt-line over stdin/stdout,
capability negotiation, one tool invocation per git command. The tool
receives file pairs and returns hunk descriptors that git feeds into the
standard xdiff pipeline.
Zero hunks with status=success means the tool considers the files
equivalent. git diff shows no output for the file, and git blame skips the
commit, attributing lines to earlier commits.
On error or tool crash, git falls back silently to the builtin diff
algorithm. The feature is opt-in via diff.<driver>.process and
.gitattributes; unconfigured files are unaffected.
The blame integration calls the diff process for each commit in the file's
history. The subprocess is long-running (one startup amortized across the
traversal), but per-commit round-trips add latency. A natural follow-up
would be a capability that sends blob OIDs instead of content, allowing
tools that can read the object store directly to avoid the cost of
serializing and deserializing blob content over the pipe for each file pair.
Changes since v1:
* Dropped the built-in diff-process-normalize tool since it obscured the
main use case for the RFC and update series messaging accordingly.
* Encapsulated changed[] memory layout behind xdl_clear_changed() helper in
xprepare.c.
* Restructured the external hunks path in xdl_diff() to fall through to the
regular diff algorithm on validation failure instead of duplicating diff
logic on fallback then returning.
* Changed subprocess error reporting from trace to warning.
* Fixed whitespace issues in the test's embedded Python.
* Changed instances of grep to test_grep in tests.
Michael Montalbo (4):
xdiff: support external hunks via xpparam_t
userdiff: add diff.<driver>.process config
diff: add long-running diff process via diff.<driver>.process
blame: consult diff process for zero-hunk detection
Documentation/config/diff.adoc | 8 +
Documentation/gitattributes.adoc | 43 ++++
Makefile | 1 +
blame.c | 43 +++-
diff-process.c | 206 +++++++++++++++++
diff-process.h | 28 +++
diff.c | 23 ++
t/.gitattributes | 1 +
t/t4080-diff-process.sh | 370 +++++++++++++++++++++++++++++++
userdiff.c | 7 +
userdiff.h | 2 +
xdiff-interface.c | 7 +-
xdiff/xdiff.h | 13 ++
xdiff/xdiffi.c | 84 ++++++-
xdiff/xprepare.c | 10 +
xdiff/xprepare.h | 1 +
16 files changed, 840 insertions(+), 7 deletions(-)
create mode 100644 diff-process.c
create mode 100644 diff-process.h
create mode 100755 t/t4080-diff-process.sh
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2120%2Fmmontalbo%2Fmm%2Fstructural-diff-backend-clean-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2120/mmontalbo/mm/structural-diff-backend-clean-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2120
Range-diff vs v1:
1: 8c0ea0bc07 ! 1: f887a7e2ba xdiff: support external hunks via xpparam_t
@@ xdiff/xdiffi.c: static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdf
+ long j, prev_old_end = 0, prev_new_end = 0;
+ long total_old = 0, total_new = 0;
+
-+ /*
-+ * Clear changed[] arrays. xdl_prepare_env() may have dirtied
-+ * them via xdl_cleanup_records(). The allocation is nrec + 2
-+ * elements; changed points one past the start (see xprepare.c).
-+ */
-+ memset(xe->xdf1.changed - 1, 0,
-+ (xe->xdf1.nrec + 2) * sizeof(bool));
-+ memset(xe->xdf2.changed - 1, 0,
-+ (xe->xdf2.nrec + 2) * sizeof(bool));
++ xdl_clear_changed(&xe->xdf1);
++ xdl_clear_changed(&xe->xdf2);
+
+ for (i = 0; i < nr_hunks; i++) {
+ const struct xdl_hunk *h = &hunks[i];
@@ xdiff/xdiffi.c: static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdf
+ /* Bounds check (1-based line numbers) */
+ if (h->old_count > 0 &&
+ (h->old_start < 1 ||
-+ h->old_start + h->old_count - 1 > xe->xdf1.nrec))
++ h->old_start + h->old_count - 1 > (long)xe->xdf1.nrec))
+ return -1;
+ if (h->new_count > 0 &&
+ (h->new_start < 1 ||
-+ h->new_start + h->new_count - 1 > xe->xdf2.nrec))
++ h->new_start + h->new_count - 1 > (long)xe->xdf2.nrec))
+ return -1;
+
+ /* Zero-count hunks: start must still be in [1, nrec+1] */
+ if (h->old_count == 0 &&
-+ (h->old_start < 1 || h->old_start > xe->xdf1.nrec + 1))
++ (h->old_start < 1 || h->old_start > (long)xe->xdf1.nrec + 1))
+ return -1;
+ if (h->new_count == 0 &&
-+ (h->new_start < 1 || h->new_start > xe->xdf2.nrec + 1))
++ (h->new_start < 1 || h->new_start > (long)xe->xdf2.nrec + 1))
+ return -1;
+
+ /* Ordering: no overlap with previous hunk */
@@ xdiff/xdiffi.c: static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdf
emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
- if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
--
-- return -1;
+ if (xpp->external_hunks) {
+ if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
+ return -1;
+ if (xdl_populate_hunks_from_external(&xe,
+ xpp->external_hunks,
-+ xpp->external_hunks_nr) < 0) {
-+ /*
-+ * Invalid external hunks; fall back to the
-+ * builtin diff algorithm. Re-runs
-+ * xdl_prepare_env() via xdl_do_diff().
-+ */
-+ xdl_free_env(&xe);
-+ if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
-+ return -1;
-+ }
-+ } else {
-+ if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
-+ return -1;
- }
++ xpp->external_hunks_nr) == 0)
++ goto diff_done;
++ xdl_free_env(&xe);
++ }
+
++ if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
+ return -1;
+- }
++
++diff_done:
+
if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
xdl_build_script(&xe, &xscr) < 0) {
+
+ ## xdiff/xprepare.c ##
+@@ xdiff/xprepare.c: int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
+
+ return 0;
+ }
++
++/*
++ * Reset the changed[] array so that no lines are marked as changed.
++ * Also clears the sentinel slots at changed[-1] and changed[nrec]
++ * that xdl_change_compact() relies on during backward scans.
++ */
++void xdl_clear_changed(xdfile_t *xdf)
++{
++ memset(xdf->changed - 1, 0, (xdf->nrec + 2) * sizeof(bool));
++}
+
+ ## xdiff/xprepare.h ##
+@@
+ int xdl_prepare_env(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
+ xdfenv_t *xe);
+ void xdl_free_env(xdfenv_t *xe);
++void xdl_clear_changed(xdfile_t *xdf);
+
+
+
2: 3bc127c800 = 2: de6d85f9d7 userdiff: add diff.<driver>.process config
3: f9976fc6aa ! 3: c25647c6e5 diff: add long-running diff process via diff.<driver>.process
@@ Commit message
[diff "cdiff"]
process = /path/to/diff-tool
- The tool receives file pairs and returns hunks describing which
- lines changed. Git feeds these hunks into the standard xdiff
- pipeline, so all output features (word diff, function context,
- color) work normally.
+ The tool provides custom line-matching: it receives file pairs
+ and returns hunks that reference original line numbers. Unlike
+ textconv, which transforms the displayed content, the diff
+ output shows the actual file while the tool controls which
+ lines are marked as changed.
The handshake negotiates version=1 and capability=hunks. Per-file
requests send command=hunks, pathname, and both file contents as
packetized data. The tool responds with hunk lines and a status
- packet. On error, git falls back to the builtin diff algorithm.
+ packet. On error, git falls back to the builtin diff algorithm
+ with a warning.
- Zero hunks with status=success means the tool considers the files
- equivalent. Git skips diff output for that file entirely.
+ Zero hunks with status=success means the tool considers the
+ files equivalent. Git skips diff output for that file.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
@@ Documentation/gitattributes.adoc: NOTE: If `diff.<name>.command` is defined for
+An external tool can provide content-aware line matching by
+setting `diff.<name>.process` to the command that runs
+the tool. The tool is a long-running process that communicates via
-+the pkt-line protocol (see
-+linkgit:gitprotocol-long-running-process[5]).
++the pkt-line protocol (described in
++Documentation/technical/long-running-process-protocol.adoc).
+
+------------------------
+*.c diff=cdiff
@@ diff-process.c (new)
@@
+/*
+ * Diff process backend: communicates with a long-running external
-+ * tool via the pkt-line protocol to obtain content-aware hunks.
++ * tool via the pkt-line protocol to obtain custom line-matching
++ * results. Unlike textconv, which transforms the displayed content,
++ * hunks from a diff process reference original line numbers and
++ * the display shows the actual file content.
+ *
+ * Protocol: pkt-line over stdin/stdout, following the pattern of
+ * the long-running filter process protocol (see convert.c).
@@ diff.c
#include "userdiff.h"
+#include "diff-process.h"
#include "submodule.h"
-+#include "trace2.h"
#include "hashmap.h"
#include "mem-pool.h"
- #include "merge-ll.h"
@@ diff.c: static void builtin_diff(const char *name_a,
xpparam_t xpp;
xdemitconf_t xecfg;
@@ diff.c: static void builtin_diff(const char *name_a,
+ xpp.external_hunks = ext_hunks;
+ xpp.external_hunks_nr = ext_hunks_nr;
+ } else {
-+ trace2_data_string("diff",
-+ o->repo,
-+ "diff-process-fallback",
-+ name_a);
++ warning(_("diff process failed for '%s',"
++ " falling back to builtin diff"),
++ name_a);
+ }
+ }
+
@@ diff.c: static void builtin_diff(const char *name_a,
free_diff_words_data(&ecbdata);
if (textconv_one)
+ ## t/.gitattributes ##
+@@ t/.gitattributes: t[0-9][0-9][0-9][0-9]/* -whitespace
+ /t8005/*.txt eol=lf
+ /t9*/*.dump eol=lf
+ /t0040*.sh whitespace=-indent-with-non-tab
++/t4080-diff-process.sh whitespace=-indent-with-non-tab
+
## t/t4080-diff-process.sh (new) ##
@@
+#!/bin/sh
@@ t/t4080-diff-process.sh (new)
+ # appear as changed in the output.
+ git -c diff.cdiff.process="$BACKEND --mode=fixed-hunk" \
+ diff boundary.c >actual &&
-+ grep "^-OLD5" actual &&
-+ grep "^-OLD6" actual &&
-+ grep "^+NEW5" actual &&
-+ grep "^+NEW6" actual &&
-+ ! grep "^-OLD9" actual &&
-+ ! grep "^-OLD10" actual &&
-+ ! grep "^+NEW9" actual &&
-+ ! grep "^+NEW10" actual
++ test_grep "^-OLD5" actual &&
++ test_grep "^-OLD6" actual &&
++ test_grep "^+NEW5" actual &&
++ test_grep "^+NEW6" actual &&
++ ! test_grep "^-OLD9" actual &&
++ ! test_grep "^-OLD10" actual &&
++ ! test_grep "^+NEW9" actual &&
++ ! test_grep "^+NEW10" actual
+'
+
+test_expect_success PYTHON 'diff process fallback on tool error status' '
@@ t/t4080-diff-process.sh (new)
+ git -c diff.cdiff.process="$BACKEND --mode=error --log=backend.log" \
+ diff boundary.c >actual &&
+ # Fallback produces the full builtin diff (both change regions).
-+ grep "^-OLD5" actual &&
-+ grep "^+NEW5" actual &&
-+ grep "^-OLD9" actual &&
-+ grep "^+NEW9" actual &&
++ test_grep "^-OLD5" actual &&
++ test_grep "^+NEW5" actual &&
++ test_grep "^-OLD9" actual &&
++ test_grep "^+NEW9" actual &&
+ # Tool was contacted (it replied with error, not crash).
-+ grep "command=hunks pathname=boundary.c" backend.log
++ test_grep "command=hunks pathname=boundary.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process fallback on bad hunks' '
+ git -c diff.cdiff.process="$BACKEND --mode=bad-hunk" \
+ diff boundary.c >actual &&
-+ grep "^-OLD5" actual &&
-+ grep "^+NEW5" actual &&
-+ grep "^-OLD9" actual &&
-+ grep "^+NEW9" actual
++ test_grep "^-OLD5" actual &&
++ test_grep "^+NEW5" actual &&
++ test_grep "^-OLD9" actual &&
++ test_grep "^+NEW9" actual
+'
+
+test_expect_success PYTHON 'diff process fallback on tool crash' '
+ git -c diff.cdiff.process="$BACKEND --mode=crash" \
+ diff boundary.c >actual &&
-+ grep "^-OLD5" actual &&
-+ grep "^+NEW5" actual &&
-+ grep "^-OLD9" actual &&
-+ grep "^+NEW9" actual
++ test_grep "^-OLD5" actual &&
++ test_grep "^+NEW5" actual &&
++ test_grep "^-OLD9" actual &&
++ test_grep "^+NEW9" actual
+'
+
+test_expect_success PYTHON 'diff process abort disables for session' '
@@ t/t4080-diff-process.sh (new)
+ git -c diff.cdiff.process="$BACKEND --mode=abort --log=backend.log" \
+ diff -- abort1.c abort2.c >actual &&
+ # Both files should still produce diff output via fallback.
-+ grep "return 10" actual &&
-+ grep "return 20" actual &&
++ test_grep "return 10" actual &&
++ test_grep "return 20" actual &&
+ # The tool aborts on the first file and git clears its
+ # capability. The second file never contacts the tool,
+ # so the log should have exactly one entry, not two.
-+ grep "command=hunks" backend.log >matches &&
++ test_grep "command=hunks" backend.log >matches &&
+ test_line_count = 1 matches
+'
+
@@ t/t4080-diff-process.sh (new)
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ diff -- multi1.c multi2.c >actual &&
-+ grep "return 10" actual &&
-+ grep "return 20" actual &&
-+ grep "pathname=multi1.c" backend.log &&
-+ grep "pathname=multi2.c" backend.log
++ test_grep "return 10" actual &&
++ test_grep "return 20" actual &&
++ test_grep "pathname=multi1.c" backend.log &&
++ test_grep "pathname=multi2.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process with --word-diff' '
@@ t/t4080-diff-process.sh (new)
+
+ git -c diff.cdiff.process="$BACKEND" \
+ diff --word-diff worddiff.c >actual &&
-+ grep "\[-1;-\]" actual &&
-+ grep "{+999;+}" actual
++ test_grep "\[-1;-\]" actual &&
++ test_grep "{+999;+}" actual
+'
+
+test_expect_success PYTHON 'diff process bypassed by --diff-algorithm' '
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ diff --diff-algorithm=patience worddiff.c >actual &&
-+ grep "return 999" actual &&
++ test_grep "return 999" actual &&
+ test_path_is_missing backend.log
+'
+
@@ t/t4080-diff-process.sh (new)
+ rm -f backend.log &&
+ git -c diff.cdiff.process="$BACKEND --log=backend.log" \
+ log -1 -p -- logtest.c >actual &&
-+ grep "return 2" actual &&
-+ grep "command=hunks pathname=logtest.c" backend.log
++ test_grep "return 2" actual &&
++ test_grep "command=hunks pathname=logtest.c" backend.log
+'
+
+test_expect_success PYTHON 'diff process zero hunks suppresses diff output' '
4: 4e6ea6d518 ! 4: 39ff53acef blame: consult diff process for zero-hunk detection
@@ t/t4080-diff-process.sh: test_expect_success PYTHON 'diff process zero hunks sup
+
+ # Without zero-hunk mode, blame attributes the change.
+ git blame blame.c >without &&
-+ grep "$BLAME_COMMIT" without &&
++ test_grep "$BLAME_COMMIT" without &&
+
+ # With zero-hunk mode, the process considers the files equivalent
+ # and blame skips the reformat commit.
+ git -c diff.cdiff.process="$BACKEND --mode=zero-hunk" \
+ blame blame.c >with &&
-+ ! grep "$BLAME_COMMIT" with
++ ! test_grep "$BLAME_COMMIT" with
+'
+
+
5: 8c7359b8a1 < -: ---------- diff-process-normalize: add built-in whitespace normalizer
--
gitgitgadget
^ permalink raw reply
* [PATCH v2] receive-pack: fix updateInstead with core.worktree
From: Alyssa Ross @ 2026-05-25 16:23 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Kristoffer Haugsbakk
In-Reply-To: <20260522154418.5883-1-hi@alyssa.is>
Previously, only one of push_to_checkout() or push_to_deploy() was
called. In a8cc594333 (hooks: fix an obscure TOCTOU "did we just run a
hook?" race, 2022-03-07), this was changed to always call
push_to_checkout(), and then to call push_to_deploy() if
push_to_checkout() didn't run anything. This change didn't take into
account that push_to_checkout() had a side effect of modifying env, and
that modified env broke updating the worktree in push_to_deploy() if
core.worktree was configured. To fix this, only mutate the environment
used inside push_to_commit(), rather than the environment that might
later be passed to push_to_deploy().
Signed-off-by: Alyssa Ross <hi@alyssa.is>
---
v2: reword commit message in response to feedback
builtin/receive-pack.c | 2 +-
t/t5516-fetch-push.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c7b2818f20..7ee157532d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1460,8 +1460,8 @@ static const char *push_to_checkout(unsigned char *hash,
opt.invoked_hook = invoked_hook;
- strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
strvec_pushv(&opt.env, env->v);
+ strvec_pushf(&opt.env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
strvec_push(&opt.args, hash_to_hex(hash));
if (run_hooks_opt(the_repository, push_to_checkout_hook, &opt))
return "push-to-checkout hook declined";
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 117cfa051f..db6cc18673 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1791,6 +1791,17 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
)
'
+test_expect_success 'denyCurrentBranch and core.worktree' '
+ test_when_finished "rm -fr cloned cloned.git" &&
+ git clone --separate-git-dir cloned.git . cloned &&
+ git --git-dir cloned.git config receive.denyCurrentBranch updateInstead &&
+ git --git-dir cloned.git config core.worktree "$PWD/cloned" &&
+ test_commit raspberry &&
+ git push cloned.git HEAD:main &&
+ test_path_exists cloned/raspberry.t &&
+ test_must_fail git push --delete cloned.git main
+'
+
test_expect_success 'denyCurrentBranch and worktrees' '
test_when_finished "rm -fr cloned && git worktree remove --force new-wt" &&
git worktree add new-wt &&
base-commit: aec3f587505a472db67e9462d0702e7d463a449d
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v2 00/11] Improve git gui operation without a worktree
From: Mark Levedahl @ 2026-05-25 16:02 UTC (permalink / raw)
To: Johannes Sixt; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <43f070e4-e624-4a33-8c24-294520fb503a@kdbg.org>
On 5/24/26 3:16 AM, Johannes Sixt wrote:
> Am 20.05.26 um 22:23 schrieb Mark Levedahl:
> I've completed my review of this iteration.
>
> Repository and working tree discovery is already converging fast.
> However, I have issues with the proposed argument parsing of the browser
> and blame modes, in particular, I don't think that we need to
> accommodate the uncanny file-before-rev argument order and that it
> disregards the worktree completely. Maybe we should postpone any changes
> in this area, if possible?
I'm not willing to give up on browser/blame yet: making these work without a worktree was
my motivator to start this series. Ignoring implementation details, etc., the issues I see
here are:
The undocumented feature to accept rev / path or path / rev, as does git blame. The latter
at least has a comment on what is expected in the code, but no mention of this exists in
blames man-page, nor that of any other git command I've examined. I'd prefer to just
remove it, I'll take your comment above as agreement in principal.
browser and blame are both fundamentally about git history. Considering browser.tcl and
blame.tcl, which produce the displays:
browser shows only content from a tree in a commit. It never uses any information from a
worktree. Given a non-existent path (or a non-existent rev),
browser displays an empty window, not an error message.
blame can take content from a file in a commit, or from the worktree as long as the file
is in a commit. A modified file in the worktree has changed / added lines annotated as
uncommitted work. But, given a file not in rev, blame displays the file with no
annotations at all, not as uncommitted work, and no error message.
So. both blame and browser require that $path is contained in $rev, even if $rev defaults
to HEAD.
The parser never checks this, though.
These commands, that *should* work fine without a worktree do not. and display confusing
information (e.g., a blank browser window, or unannotated file) when a simple error
message from the parser would convey more information.
>
> Throughout, we use a strange indentation style of 'if {[catch ...' that
> is violated in new code, but I left uncommented. It should indent the
> catch body one additional level like so:
>
> if {catch {
> commands that can fail
> } err]} {
> error handling here
> }
Yes, just count the number of { - number of }. Vim's indent mode for tcl gets this very
wrong. All fixed, I hope.
>
> Thank you very much for working on this topic.
and thank you for the very thorough review.
>
> -- Hannes
>
^ permalink raw reply
* [PATCH v2 0/3] commit-reach: replace queue_has_nonstale() scan with O(1) tracking
From: Kristofer Karlsson via GitGitGadget @ 2026-05-25 14:28 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Jeff King, Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2124.git.1779644541.gitgitgadget@gmail.com>
This is v2 of the series to replace the O(n) queue_has_nonstale() scan with
O(1) tracking.
Changes since v1:
* Replaced the nonstale counter with a max_nonstale pointer approach, as
suggested by Jeff King. Instead of counting non-stale entries, we track
the lowest-priority non-stale commit; when it is popped, all remaining
entries must be stale.
* Restructured from 5 patches to 3:
1. object.h: fix stale entries in object flag allocation table
2. commit-reach: deduplicate queue entries in paint_down_to_common
3. commit-reach: replace queue_has_nonstale() scan with O(1) tracking
* Separated concerns: ENQUEUED dedup is a paint_down_to_common concern
(commit 2), while nonstale_queue is a general wrapper usable by both
paint_down_to_common and ahead_behind (commit 3). ahead_behind uses its
own PARENT2-based dedup via insert_no_dup.
* The nonstale_queue struct is intentionally kept thin (no ENQUEUED
handling). Dedup variants (nonstale_queue_put_dedup /
nonstale_queue_get_dedup) are layered on top for paint_down_to_common.
Performance on a large monorepo (3.7M commits), merge-base --all on deep
import branches:
Baseline Patched
component import, wide frontier (1): 8536ms 3956ms
component import, wide frontier (2): 5757ms 4383ms
component import, wide frontier (3): 4743ms 1927ms
Profiling shows paint_down_to_common() drops from 50% to 4% of total runtime
(~27x faster). The remaining time is in commit graph lookups, heap
operations, and object management — per-commit costs that are not addressed
by this series.
Simple/linear cases show no regression (sub-15ms regardless).
Kristofer Karlsson (3):
object.h: fix stale entries in object flag allocation table
commit-reach: deduplicate queue entries in paint_down_to_common
commit-reach: replace queue_has_nonstale() scan with O(1) tracking
commit-reach.c | 101 +++++++++++++++++++++++++++++++++++++------------
object.h | 7 ++--
2 files changed, 80 insertions(+), 28 deletions(-)
base-commit: 56a4f3c3a221adf1df9b39da69b8a6890f803157
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2124%2Fspkrka%2Fqueue-has-nonstale-v3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2124/spkrka/queue-has-nonstale-v3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2124
Range-diff vs v1:
-: ---------- > 1: 105f4646c2 object.h: fix stale entries in object flag allocation table
1: 1d3751569b ! 2: fc38c0f856 commit-reach: deduplicate queue entries in paint_down_to_common
@@ Commit message
combinations. Add an ENQUEUED flag to track whether a commit is
currently in the priority queue, and skip it if already present.
+ Introduce prio_queue_put_dedup() and prio_queue_get_dedup()
+ wrappers that manage the ENQUEUED flag on enqueue and dequeue.
+
This change is performance-neutral on its own: the O(n)
queue_has_nonstale() scan still dominates the per-iteration cost.
However, the deduplication guarantee (each commit appears in the
queue at most once) is a prerequisite for the next commit, which
- replaces that scan with an O(1) nonstale counter.
+ replaces that scan with O(1) tracking.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
@@ commit-reach.c: static int compare_commits_by_gen(const void *_a, const void *_b
return 0;
}
-+static void maybe_enqueue(struct prio_queue *queue, struct commit *c)
++static void prio_queue_put_dedup(struct prio_queue *queue, struct commit *c)
+{
+ if (c->object.flags & ENQUEUED)
+ return;
+ c->object.flags |= ENQUEUED;
+ prio_queue_put(queue, c);
+}
++
++static struct commit *prio_queue_get_dedup(struct prio_queue *queue)
++{
++ struct commit *commit = prio_queue_get(queue);
++ if (commit)
++ commit->object.flags &= ~ENQUEUED;
++ return commit;
++}
+
static int queue_has_nonstale(struct prio_queue *queue)
{
@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
return 0;
}
- prio_queue_put(&queue, one);
-+ maybe_enqueue(&queue, one);
++ prio_queue_put_dedup(&queue, one);
for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
- prio_queue_put(&queue, twos[i]);
-+ maybe_enqueue(&queue, twos[i]);
++ prio_queue_put_dedup(&queue, twos[i]);
}
while (queue_has_nonstale(&queue)) {
-@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
+- struct commit *commit = prio_queue_get(&queue);
++ struct commit *commit = prio_queue_get_dedup(&queue);
+ struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
-
-+ commit->object.flags &= ~ENQUEUED;
-+
- if (min_generation && generation > last_gen)
- BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
- generation, last_gen,
@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
oid_to_hex(&p->object.oid));
}
p->object.flags |= flags;
- prio_queue_put(&queue, p);
-+ maybe_enqueue(&queue, p);
++ prio_queue_put_dedup(&queue, p);
}
}
@@ object.h: void object_array_init(struct object_array *array);
- * commit-reach.c: 16-----19
+ * commit-reach.c: 16-------20
* builtin/last-modified.c: 1617
- * sha1-name.c: 20
+ * object-name.c: 20
* list-objects-filter.c: 21
2: 4742f5e634 < -: ---------- commit-reach: optimize queue scan in paint_down_to_common
3: 711a0e2235 ! 3: 03771eb34c commit-reach: optimize queue scan in ahead_behind
@@ Metadata
Author: Kristofer Karlsson <krka@spotify.com>
## Commit message ##
- commit-reach: optimize queue scan in ahead_behind
+ commit-reach: replace queue_has_nonstale() scan with O(1) tracking
- Apply the same nonstale_count optimization from the previous commit
- to ahead_behind(). This replaces the remaining caller of the O(n)
- queue_has_nonstale() scan with an O(1) counter check, allowing
- queue_has_nonstale() to be removed.
+ paint_down_to_common() and ahead_behind() call queue_has_nonstale()
+ on every iteration to decide whether to continue the walk.
+ queue_has_nonstale() performs a linear scan of the priority queue,
+ making the overall walk O(n*m) where n is the number of commits
+ walked and m is the queue size.
- ahead_behind() already deduplicates queue entries using the PARENT2
- flag (via insert_no_dup), so the counter is maintained through
- insert_no_dup() and mark_stale() using PARENT2 as the queued_flag.
+ Introduce 'struct nonstale_queue', a thin wrapper around prio_queue
+ that maintains a 'max_nonstale' pointer — the lowest-priority
+ (oldest) non-stale commit seen so far. When this commit is popped,
+ every remaining queue entry is known to be stale, so the walk can
+ stop. This reduces the per-iteration termination check from O(m)
+ to O(1).
+ Uses <= 0 (not < 0) when comparing priorities so that among distinct
+ commits with equal priority (same generation and timestamp) the
+ last-enqueued one is tracked. Since prio_queue breaks ties by
+ insertion order, this ensures max_nonstale is always the last in its
+ priority class to be popped, making pointer equality on pop
+ sufficient for correctness.
+
+ The previous commit's ENQUEUED deduplication guarantees each commit
+ appears at most once in the queue, which is required for the pointer
+ equality check to be unambiguous.
+
+ On a large monorepo (3.7M commits), this yields ~2x end-to-end
+ speedup for merge-base calculations on deep import branches.
+ Profiling shows paint_down_to_common() drops from 50% to 4% of
+ total runtime (~27x faster), with the remaining time in commit
+ graph lookups and heap operations:
+
+ Before: 8536ms / 5757ms / 4743ms (three test cases)
+ After: 3956ms / 4383ms / 1927ms
+
+ Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
## commit-reach.c ##
-@@ commit-reach.c: static void mark_stale(struct commit *c, unsigned queued_flag,
- }
+@@ commit-reach.c: static int compare_commits_by_gen(const void *_a, const void *_b)
+ return 0;
+ }
+
+-static void prio_queue_put_dedup(struct prio_queue *queue, struct commit *c)
++/*
++ * A prio_queue with O(1) termination check. 'max_nonstale' tracks
++ * the lowest-priority non-stale commit enqueued so far; once it is
++ * popped, every remaining entry is known to be STALE.
++ */
++struct nonstale_queue {
++ struct prio_queue pq;
++ struct commit *max_nonstale;
++};
++
++static void nonstale_queue_put(struct nonstale_queue *queue,
++ struct commit *c)
++{
++ struct commit *old = queue->max_nonstale;
++
++ prio_queue_put(&queue->pq, c);
++ if (c->object.flags & STALE)
++ return;
++ if (!old || queue->pq.compare(old, c, queue->pq.cb_data) <= 0)
++ queue->max_nonstale = c;
++}
++
++static struct commit *nonstale_queue_get(struct nonstale_queue *queue)
++{
++ struct commit *commit = prio_queue_get(&queue->pq);
++
++ if (commit == queue->max_nonstale)
++ queue->max_nonstale = NULL;
++
++ return commit;
++}
++
++static void clear_nonstale_queue(struct nonstale_queue *queue)
++{
++ clear_prio_queue(&queue->pq);
++ queue->max_nonstale = NULL;
++}
++
++static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
++ struct commit *c)
+ {
+ if (c->object.flags & ENQUEUED)
+ return;
+ c->object.flags |= ENQUEUED;
+- prio_queue_put(queue, c);
++ nonstale_queue_put(queue, c);
+ }
+
+-static struct commit *prio_queue_get_dedup(struct prio_queue *queue)
++static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
+ {
+- struct commit *commit = prio_queue_get(queue);
++ struct commit *commit = nonstale_queue_get(queue);
++
+ if (commit)
+ commit->object.flags &= ~ENQUEUED;
+ return commit;
}
-static int queue_has_nonstale(struct prio_queue *queue)
@@ commit-reach.c: static void mark_stale(struct commit *c, unsigned queued_flag,
/* all input commits in one and twos[] must have been parsed! */
static int paint_down_to_common(struct repository *r,
struct commit *one, int n,
+@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
+ enum merge_base_flags mb_flags,
+ struct commit_list **result)
+ {
+- struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
++ struct nonstale_queue queue = {
++ { compare_commits_by_gen_then_commit_date }
++ };
+ int i;
+ timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
+ struct commit_list **tail = result;
+
+ if (!min_generation && !corrected_commit_dates_enabled(r))
+- queue.compare = compare_commits_by_commit_date;
++ queue.pq.compare = compare_commits_by_commit_date;
+
+ one->object.flags |= PARENT1;
+ if (!n) {
+ commit_list_append(one, result);
+ return 0;
+ }
+- prio_queue_put_dedup(&queue, one);
++ nonstale_queue_put_dedup(&queue, one);
+
+ for (i = 0; i < n; i++) {
+ twos[i]->object.flags |= PARENT2;
+- prio_queue_put_dedup(&queue, twos[i]);
++ nonstale_queue_put_dedup(&queue, twos[i]);
+ }
+
+- while (queue_has_nonstale(&queue)) {
+- struct commit *commit = prio_queue_get_dedup(&queue);
++ while (queue.max_nonstale) {
++ struct commit *commit = nonstale_queue_get_dedup(&queue);
+ struct commit_list *parents;
+ int flags;
+ timestamp_t generation = commit_graph_generation(commit);
+@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
+ if ((p->object.flags & flags) == flags)
+ continue;
+ if (repo_parse_commit(r, p)) {
+- clear_prio_queue(&queue);
++ clear_nonstale_queue(&queue);
+ commit_list_free(*result);
+ *result = NULL;
+ /*
+@@ commit-reach.c: static int paint_down_to_common(struct repository *r,
+ oid_to_hex(&p->object.oid));
+ }
+ p->object.flags |= flags;
+- prio_queue_put_dedup(&queue, p);
++ nonstale_queue_put_dedup(&queue, p);
+ }
+ }
+
+- clear_prio_queue(&queue);
++ clear_nonstale_queue(&queue);
+ commit_list_sort_by_date(result);
+ return 0;
+ }
@@ commit-reach.c: struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
define_commit_slab(bit_arrays, struct bitmap *);
static struct bit_arrays bit_arrays;
-static void insert_no_dup(struct prio_queue *queue, struct commit *c)
-+static void insert_no_dup(struct prio_queue *queue, struct commit *c,
-+ int *nonstale_count)
++static void insert_no_dup(struct nonstale_queue *queue, struct commit *c)
{
if (c->object.flags & PARENT2)
return;
- prio_queue_put(queue, c);
+- prio_queue_put(queue, c);
++ nonstale_queue_put(queue, c);
c->object.flags |= PARENT2;
-+ if (!(c->object.flags & STALE))
-+ (*nonstale_count)++;
}
- static struct bitmap *get_bit_array(struct commit *c, int width)
@@ commit-reach.c: void ahead_behind(struct repository *r,
+ struct commit **commits, size_t commits_nr,
+ struct ahead_behind_count *counts, size_t counts_nr)
{
- struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
+- struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
++ struct nonstale_queue queue = {
++ { .compare = compare_commits_by_gen_then_commit_date }
++ };
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
-+ int nonstale_count = 0;
if (!commits_nr || !counts_nr)
- return;
@@ commit-reach.c: void ahead_behind(struct repository *r,
- struct bitmap *bitmap = get_bit_array(c, width);
-
- bitmap_set(bitmap, i);
-- insert_no_dup(&queue, c);
-+ insert_no_dup(&queue, c, &nonstale_count);
+ insert_no_dup(&queue, c);
}
- while (queue_has_nonstale(&queue)) {
-+ while (nonstale_count > 0) {
- struct commit *c = prio_queue_get(&queue);
+- struct commit *c = prio_queue_get(&queue);
++ while (queue.max_nonstale) {
++ struct commit *c = nonstale_queue_get(&queue);
struct commit_list *p;
struct bitmap *bitmap_c = get_bit_array(c, width);
-+ if (!(c->object.flags & STALE))
-+ nonstale_count--;
-+
- for (size_t i = 0; i < counts_nr; i++) {
- int reach_from_tip = !!bitmap_get(bitmap_c, counts[i].tip_index);
- int reach_from_base = !!bitmap_get(bitmap_c, counts[i].base_index);
@@ commit-reach.c: void ahead_behind(struct repository *r,
- * queue is STALE.
- */
- if (bitmap_popcount(bitmap_p) == commits_nr)
-- p->item->object.flags |= STALE;
-+ mark_stale(p->item, PARENT2, &nonstale_count);
-- insert_no_dup(&queue, p->item);
-+ insert_no_dup(&queue, p->item, &nonstale_count);
- }
+ /* STALE is used here, PARENT2 is used by insert_no_dup(). */
+ repo_clear_commit_marks(r, PARENT2 | STALE);
+- for (size_t i = 0; i < queue.nr; i++)
+- free_bit_array(queue.array[i].data);
++ for (size_t i = 0; i < queue.pq.nr; i++)
++ free_bit_array(queue.pq.array[i].data);
+ clear_bit_arrays(&bit_arrays);
+- clear_prio_queue(&queue);
++ clear_nonstale_queue(&queue);
+ }
- free_bit_array(c);
+ struct commit_and_index {
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 3/3] commit-reach: replace queue_has_nonstale() scan with O(1) tracking
From: Kristofer Karlsson via GitGitGadget @ 2026-05-25 14:28 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Jeff King, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2124.v2.git.1779719286.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
paint_down_to_common() and ahead_behind() call queue_has_nonstale()
on every iteration to decide whether to continue the walk.
queue_has_nonstale() performs a linear scan of the priority queue,
making the overall walk O(n*m) where n is the number of commits
walked and m is the queue size.
Introduce 'struct nonstale_queue', a thin wrapper around prio_queue
that maintains a 'max_nonstale' pointer — the lowest-priority
(oldest) non-stale commit seen so far. When this commit is popped,
every remaining queue entry is known to be stale, so the walk can
stop. This reduces the per-iteration termination check from O(m)
to O(1).
Uses <= 0 (not < 0) when comparing priorities so that among distinct
commits with equal priority (same generation and timestamp) the
last-enqueued one is tracked. Since prio_queue breaks ties by
insertion order, this ensures max_nonstale is always the last in its
priority class to be popped, making pointer equality on pop
sufficient for correctness.
The previous commit's ENQUEUED deduplication guarantees each commit
appears at most once in the queue, which is required for the pointer
equality check to be unambiguous.
On a large monorepo (3.7M commits), this yields ~2x end-to-end
speedup for merge-base calculations on deep import branches.
Profiling shows paint_down_to_common() drops from 50% to 4% of
total runtime (~27x faster), with the remaining time in commit
graph lookups and heap operations:
Before: 8536ms / 5757ms / 4743ms (three test cases)
After: 3956ms / 4383ms / 1927ms
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 96 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 65 insertions(+), 31 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 85583ae359..b5328a804c 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -40,32 +40,62 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
return 0;
}
-static void prio_queue_put_dedup(struct prio_queue *queue, struct commit *c)
+/*
+ * A prio_queue with O(1) termination check. 'max_nonstale' tracks
+ * the lowest-priority non-stale commit enqueued so far; once it is
+ * popped, every remaining entry is known to be STALE.
+ */
+struct nonstale_queue {
+ struct prio_queue pq;
+ struct commit *max_nonstale;
+};
+
+static void nonstale_queue_put(struct nonstale_queue *queue,
+ struct commit *c)
+{
+ struct commit *old = queue->max_nonstale;
+
+ prio_queue_put(&queue->pq, c);
+ if (c->object.flags & STALE)
+ return;
+ if (!old || queue->pq.compare(old, c, queue->pq.cb_data) <= 0)
+ queue->max_nonstale = c;
+}
+
+static struct commit *nonstale_queue_get(struct nonstale_queue *queue)
+{
+ struct commit *commit = prio_queue_get(&queue->pq);
+
+ if (commit == queue->max_nonstale)
+ queue->max_nonstale = NULL;
+
+ return commit;
+}
+
+static void clear_nonstale_queue(struct nonstale_queue *queue)
+{
+ clear_prio_queue(&queue->pq);
+ queue->max_nonstale = NULL;
+}
+
+static void nonstale_queue_put_dedup(struct nonstale_queue *queue,
+ struct commit *c)
{
if (c->object.flags & ENQUEUED)
return;
c->object.flags |= ENQUEUED;
- prio_queue_put(queue, c);
+ nonstale_queue_put(queue, c);
}
-static struct commit *prio_queue_get_dedup(struct prio_queue *queue)
+static struct commit *nonstale_queue_get_dedup(struct nonstale_queue *queue)
{
- struct commit *commit = prio_queue_get(queue);
+ struct commit *commit = nonstale_queue_get(queue);
+
if (commit)
commit->object.flags &= ~ENQUEUED;
return commit;
}
-static int queue_has_nonstale(struct prio_queue *queue)
-{
- for (size_t i = 0; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
- if (!(commit->object.flags & STALE))
- return 1;
- }
- return 0;
-}
-
/* all input commits in one and twos[] must have been parsed! */
static int paint_down_to_common(struct repository *r,
struct commit *one, int n,
@@ -74,28 +104,30 @@ static int paint_down_to_common(struct repository *r,
enum merge_base_flags mb_flags,
struct commit_list **result)
{
- struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
+ struct nonstale_queue queue = {
+ { compare_commits_by_gen_then_commit_date }
+ };
int i;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;
if (!min_generation && !corrected_commit_dates_enabled(r))
- queue.compare = compare_commits_by_commit_date;
+ queue.pq.compare = compare_commits_by_commit_date;
one->object.flags |= PARENT1;
if (!n) {
commit_list_append(one, result);
return 0;
}
- prio_queue_put_dedup(&queue, one);
+ nonstale_queue_put_dedup(&queue, one);
for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
- prio_queue_put_dedup(&queue, twos[i]);
+ nonstale_queue_put_dedup(&queue, twos[i]);
}
- while (queue_has_nonstale(&queue)) {
- struct commit *commit = prio_queue_get_dedup(&queue);
+ while (queue.max_nonstale) {
+ struct commit *commit = nonstale_queue_get_dedup(&queue);
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
@@ -133,7 +165,7 @@ static int paint_down_to_common(struct repository *r,
if ((p->object.flags & flags) == flags)
continue;
if (repo_parse_commit(r, p)) {
- clear_prio_queue(&queue);
+ clear_nonstale_queue(&queue);
commit_list_free(*result);
*result = NULL;
/*
@@ -149,11 +181,11 @@ static int paint_down_to_common(struct repository *r,
oid_to_hex(&p->object.oid));
}
p->object.flags |= flags;
- prio_queue_put_dedup(&queue, p);
+ nonstale_queue_put_dedup(&queue, p);
}
}
- clear_prio_queue(&queue);
+ clear_nonstale_queue(&queue);
commit_list_sort_by_date(result);
return 0;
}
@@ -1057,11 +1089,11 @@ struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
define_commit_slab(bit_arrays, struct bitmap *);
static struct bit_arrays bit_arrays;
-static void insert_no_dup(struct prio_queue *queue, struct commit *c)
+static void insert_no_dup(struct nonstale_queue *queue, struct commit *c)
{
if (c->object.flags & PARENT2)
return;
- prio_queue_put(queue, c);
+ nonstale_queue_put(queue, c);
c->object.flags |= PARENT2;
}
@@ -1086,7 +1118,9 @@ void ahead_behind(struct repository *r,
struct commit **commits, size_t commits_nr,
struct ahead_behind_count *counts, size_t counts_nr)
{
- struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
+ struct nonstale_queue queue = {
+ { .compare = compare_commits_by_gen_then_commit_date }
+ };
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
if (!commits_nr || !counts_nr)
@@ -1109,8 +1143,8 @@ void ahead_behind(struct repository *r,
insert_no_dup(&queue, c);
}
- while (queue_has_nonstale(&queue)) {
- struct commit *c = prio_queue_get(&queue);
+ while (queue.max_nonstale) {
+ struct commit *c = nonstale_queue_get(&queue);
struct commit_list *p;
struct bitmap *bitmap_c = get_bit_array(c, width);
@@ -1152,10 +1186,10 @@ void ahead_behind(struct repository *r,
/* STALE is used here, PARENT2 is used by insert_no_dup(). */
repo_clear_commit_marks(r, PARENT2 | STALE);
- for (size_t i = 0; i < queue.nr; i++)
- free_bit_array(queue.array[i].data);
+ for (size_t i = 0; i < queue.pq.nr; i++)
+ free_bit_array(queue.pq.array[i].data);
clear_bit_arrays(&bit_arrays);
- clear_prio_queue(&queue);
+ clear_nonstale_queue(&queue);
}
struct commit_and_index {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/3] commit-reach: deduplicate queue entries in paint_down_to_common
From: Kristofer Karlsson via GitGitGadget @ 2026-05-25 14:28 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Jeff King, Kristofer Karlsson, Kristofer Karlsson,
Kristofer Karlsson
In-Reply-To: <pull.2124.v2.git.1779719286.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
paint_down_to_common() can enqueue the same commit multiple times
when it is reached through different parents with different flag
combinations. Add an ENQUEUED flag to track whether a commit is
currently in the priority queue, and skip it if already present.
Introduce prio_queue_put_dedup() and prio_queue_get_dedup()
wrappers that manage the ENQUEUED flag on enqueue and dequeue.
This change is performance-neutral on its own: the O(n)
queue_has_nonstale() scan still dominates the per-iteration cost.
However, the deduplication guarantee (each commit appears in the
queue at most once) is a prerequisite for the next commit, which
replaces that scan with O(1) tracking.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 27 ++++++++++++++++++++++-----
object.h | 2 +-
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 5a52be90a6..85583ae359 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -17,8 +17,9 @@
#define PARENT2 (1u<<17)
#define STALE (1u<<18)
#define RESULT (1u<<19)
+#define ENQUEUED (1u<<20)
-static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
+static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
static int compare_commits_by_gen(const void *_a, const void *_b)
{
@@ -39,6 +40,22 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
return 0;
}
+static void prio_queue_put_dedup(struct prio_queue *queue, struct commit *c)
+{
+ if (c->object.flags & ENQUEUED)
+ return;
+ c->object.flags |= ENQUEUED;
+ prio_queue_put(queue, c);
+}
+
+static struct commit *prio_queue_get_dedup(struct prio_queue *queue)
+{
+ struct commit *commit = prio_queue_get(queue);
+ if (commit)
+ commit->object.flags &= ~ENQUEUED;
+ return commit;
+}
+
static int queue_has_nonstale(struct prio_queue *queue)
{
for (size_t i = 0; i < queue->nr; i++) {
@@ -70,15 +87,15 @@ static int paint_down_to_common(struct repository *r,
commit_list_append(one, result);
return 0;
}
- prio_queue_put(&queue, one);
+ prio_queue_put_dedup(&queue, one);
for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
- prio_queue_put(&queue, twos[i]);
+ prio_queue_put_dedup(&queue, twos[i]);
}
while (queue_has_nonstale(&queue)) {
- struct commit *commit = prio_queue_get(&queue);
+ struct commit *commit = prio_queue_get_dedup(&queue);
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
@@ -132,7 +149,7 @@ static int paint_down_to_common(struct repository *r,
oid_to_hex(&p->object.oid));
}
p->object.flags |= flags;
- prio_queue_put(&queue, p);
+ prio_queue_put_dedup(&queue, p);
}
}
diff --git a/object.h b/object.h
index 2b26de3044..8fb03ff90a 100644
--- a/object.h
+++ b/object.h
@@ -75,7 +75,7 @@ void object_array_init(struct object_array *array);
* bundle.c: 16
* http-push.c: 11-----14
* commit-graph.c: 15
- * commit-reach.c: 16-----19
+ * commit-reach.c: 16-------20
* builtin/last-modified.c: 1617
* object-name.c: 20
* list-objects-filter.c: 21
--
gitgitgadget
^ 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