* [PATCH] revision: make get_commit_action() a pure predicate
From: Michael Montalbo via GitGitGadget @ 2026-07-15 19:29 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor, Michael Montalbo, Michael Montalbo
From: Michael Montalbo <mmontalbo@gmail.com>
get_commit_action() reads as a predicate that decides whether a commit
is shown or ignored, but for a line-level log without parent rewriting
it also calls line_log_process_ranges_arbitrary_commit(), which
mutates the tracked line ranges. That hidden side effect makes it unsafe
to evaluate ahead of the walk, the way a lookahead would.
get_commit_action() was split out of simplify_commit() in beb5af43a6
(graph API: fix bug in graph_is_interesting(), 2009-08-18) as the
show/ignore decision minus the parent rewriting, so the graph renderer
could reuse it; line-level log later routed its filtering through it as
well, in 3cb9d2b6 (line-log: more responsive, incremental 'git log -L',
2020-05-11). Besides simplify_commit(), the walk driver,
graph_is_interesting() is its only other caller, and it runs only under
--graph, which sets rewrite_parents and therefore want_ancestry(); the
"-L without ancestry" branch that holds the side effect never fires
there, so it is dormant today.
The line-level processing folds a commit's tracked ranges onto its
parents, which must happen even for a commit that get_commit_action()
filters from the output, or the ranges never reach the parents. Move it
to simplify_commit() and run it before get_commit_action(), gated by
get_commit_action()'s leading checks (already shown, uninteresting, and
the like) so a commit ignored by those is not folded, as before; factor
those checks out as commit_early_ignore(). get_commit_action() is then
side-effect free.
commit_early_ignore() runs twice on the -L path, once for that gate and
once inside get_commit_action(), but it reads only object flags and pack
membership, disjoint from the TREESAME flag the fold sets, so the repeat
is harmless.
Add a "line-log-peek" subcommand to the revision-walking test helper
that evaluates get_commit_action() on a commit the walk has not reached
yet, plus a t4211 check that the call leaves the commit's flags
unchanged. The flags are compared rather than the commit list because
add_line_range() merges ranges by union, which is idempotent, so the
side effect never changed which commits a linear -L history shows.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
---
revision: make get_commit_action() a pure predicate
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2169%2Fmmontalbo%2Fmm%2Fline-log-tidy-proto-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2169/mmontalbo/mm/line-log-tidy-proto-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2169
revision.c | 70 ++++++++++++++++++++------------
t/helper/test-revision-walking.c | 63 ++++++++++++++++++++++++++++
t/t4211-line-log.sh | 20 +++++++++
3 files changed, 127 insertions(+), 26 deletions(-)
diff --git a/revision.c b/revision.c
index 0c95edef59..5d650affc0 100644
--- a/revision.c
+++ b/revision.c
@@ -4175,37 +4175,39 @@ static timestamp_t comparison_date(const struct rev_info *revs,
commit->date;
}
-enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
+/*
+ * Whether the commit is ignored by the cheap checks that read only its
+ * traversal flags and pack membership (e.g. already shown, or marked
+ * uninteresting), before any check that examines the commit's date,
+ * parents, message, or diff.
+ */
+static int commit_early_ignore(struct rev_info *revs, struct commit *commit)
{
if (commit->object.flags & SHOWN)
- return commit_ignore;
+ return 1;
if (revs->maximal_only && (commit->object.flags & CHILD_VISITED))
- return commit_ignore;
+ return 1;
if (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))
- return commit_ignore;
- if (revs->no_kept_objects) {
- if (has_object_kept_pack(revs->repo, &commit->object.oid,
- revs->keep_pack_cache_flags))
- return commit_ignore;
- }
+ return 1;
+ if (revs->no_kept_objects &&
+ has_object_kept_pack(revs->repo, &commit->object.oid,
+ revs->keep_pack_cache_flags))
+ return 1;
if (commit->object.flags & UNINTERESTING)
+ return 1;
+ return 0;
+}
+
+/*
+ * Decide whether this commit is shown or ignored. Keep it a pure
+ * predicate: callers such as the commit graph depend on it having no
+ * side effects, so per-commit mutations (such as -L range tracking)
+ * belong in the caller, simplify_commit(), not here.
+ */
+enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
+{
+ if (commit_early_ignore(revs, commit))
return commit_ignore;
- if (revs->line_level_traverse && !want_ancestry(revs)) {
- /*
- * In case of line-level log with parent rewriting
- * prepare_revision_walk() already took care of all line-level
- * log filtering, and there is nothing left to do here.
- *
- * If parent rewriting was not requested, then this is the
- * place to perform the line-level log filtering. Notably,
- * this check, though expensive, must come before the other,
- * cheaper filtering conditions, because the tracked line
- * ranges must be adjusted even when the commit will end up
- * being ignored based on other conditions.
- */
- if (!line_log_process_ranges_arbitrary_commit(revs, commit))
- return commit_ignore;
- }
if (revs->min_age != -1 &&
comparison_date(revs, commit) > revs->min_age)
return commit_ignore;
@@ -4314,7 +4316,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit
enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
{
- enum commit_action action = get_commit_action(revs, commit);
+ enum commit_action action;
+
+ /*
+ * For a line-level log without parent rewriting, fold each commit's
+ * ranges as the walk reaches it (parent rewriting does this eagerly in
+ * prepare_revision_walk()). Fold before get_commit_action() so the
+ * ranges carry across a commit that a later, cheaper check ignores;
+ * the commit_early_ignore() guard skips a commit get_commit_action()
+ * would ignore outright.
+ */
+ if (revs->line_level_traverse && !want_ancestry(revs) &&
+ !commit_early_ignore(revs, commit)) {
+ if (!line_log_process_ranges_arbitrary_commit(revs, commit))
+ return commit_ignore;
+ }
+
+ action = get_commit_action(revs, commit);
if (action == commit_show &&
revs->prune && revs->dense && want_ancestry(revs)) {
diff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c
index 70051eeaf8..24d7f29417 100644
--- a/t/helper/test-revision-walking.c
+++ b/t/helper/test-revision-walking.c
@@ -13,9 +13,12 @@
#include "test-tool.h"
#include "commit.h"
#include "diff.h"
+#include "line-log.h"
+#include "object-name.h"
#include "repository.h"
#include "revision.h"
#include "setup.h"
+#include "string-list.h"
static void print_commit(struct commit *commit)
{
@@ -51,6 +54,60 @@ static int run_revision_walk(void)
return got_revision;
}
+/*
+ * Check that get_commit_action() is a pure predicate by evaluating it on a
+ * commit the walk has not reached yet. No git command makes that out-of-order
+ * call, so this probe does it deliberately, and reports whether the call
+ * mutated the peeked commit: a pure get_commit_action() leaves it untouched.
+ * We compare the commit's flags rather than the emitted commit list because
+ * range merges are idempotent, so a side effect would not change which commits
+ * are shown. Only meaningful for a plain "-L" walk with no parent rewriting.
+ */
+static int line_log_peek(const char **argv)
+{
+ struct repository *repo = the_repository;
+ struct rev_info rev;
+ struct string_list range_args = STRING_LIST_INIT_DUP;
+ struct object_id oid;
+ struct commit *peek;
+ const char *rev_argv[3];
+ unsigned before, after;
+
+ if (repo_get_oid(repo, argv[0], &oid))
+ die("bad peek commit: %s", argv[0]);
+ peek = lookup_commit_reference(repo, &oid);
+ if (!peek || repo_parse_commit(repo, peek))
+ die("cannot parse peek commit: %s", argv[0]);
+
+ repo_init_revisions(repo, &rev, NULL);
+ rev.diffopt.flags.recursive = 1;
+ rev.line_level_traverse = 1;
+ string_list_append(&range_args, argv[1]);
+
+ rev_argv[0] = "line-log-peek";
+ rev_argv[1] = argv[2];
+ rev_argv[2] = NULL;
+ setup_revisions(2, rev_argv, &rev, NULL);
+
+ line_log_init(&rev, NULL, &range_args);
+
+ if (rev.rewrite_parents || rev.children.name)
+ die("line-log-peek requires a non-ancestry (-L, no --graph) walk");
+
+ if (prepare_revision_walk(&rev))
+ die("prepare_revision_walk failed");
+
+ before = peek->object.flags;
+ get_commit_action(&rev, peek);
+ after = peek->object.flags;
+
+ printf("mutated %d\n", before != after);
+
+ release_revisions(&rev);
+ string_list_clear(&range_args, 0);
+ return 0;
+}
+
int cmd__revision_walking(int argc, const char **argv)
{
if (argc < 2)
@@ -69,6 +126,12 @@ int cmd__revision_walking(int argc, const char **argv)
return 0;
}
+ if (!strcmp(argv[1], "line-log-peek")) {
+ if (argc != 5)
+ die("usage: test-tool revision-walking line-log-peek <peek-commit> <start,end:file> <rev>");
+ return line_log_peek(argv + 2);
+ }
+
fprintf(stderr, "check usage\n");
return 1;
}
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index ca4eb7bbc7..f4a7d8ab61 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -781,4 +781,24 @@ test_expect_success '--summary shows new file on root commit' '
test_grep "create mode 100644 file.c" actual
'
+test_expect_success 'get_commit_action() does not mutate a not-yet-walked commit' '
+ git init peek &&
+ (
+ cd peek &&
+ test_write_lines 1 2 3 4 5 >f.c &&
+ git add f.c && test_tick && git commit -m base &&
+ test_write_lines 1 two 3 4 5 >f.c &&
+ test_tick && git commit -am change &&
+
+ # Peek HEAD^, which the walk has not reached (the out-of-order
+ # call a lookahead makes), and confirm get_commit_action() leaves
+ # it untouched. A side effect is invisible in the commit list
+ # (range merges are idempotent), so the helper reports whether the
+ # call mutated the peeked commit at all.
+ echo "mutated 0" >expect &&
+ test-tool revision-walking line-log-peek HEAD^ 1,3:f.c HEAD >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v5 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Junio C Hamano @ 2026-07-15 19:13 UTC (permalink / raw)
To: Paulius Zaleckas
Cc: git, Ramsay Jones, Jean-Noël Avila, Glen Choo,
Patrick Steinhardt, Ævar Arnfjörð Bjarmason
In-Reply-To: <20260715103518.526326-3-paulius.zaleckas@gmail.com>
Paulius Zaleckas <paulius.zaleckas@gmail.com> writes:
> +/* really private - use accessors below to parse and format */
> +static const char *submodule_errors_names[] = {
> + [SUBMODULE_ERRORS_FAIL] = "fail",
> + [SUBMODULE_ERRORS_WARN] = "warn",
> +};
> +
> +static const char *submodule_errors_to_string(int mode)
> +{
> + if (mode < 0 || (size_t)mode >= ARRAY_SIZE(submodule_errors_names))
> + BUG("invalid submodule errors mode %d", mode);
> + return submodule_errors_names[mode];
> +}
> +
I am ranting here, and it is not entirely your fault, but I
have to mention that this is the kind of bad code that
"-Wsign-compare" forces on us. We know that 'mode' is a small
integer used to index into the submodule_errors_names[] array.
Theoretically, an array might contain as many elements as
(size_t)(-1), but we know nobody needs to feed us a number
that does not fit in a platform-natural "int".
Side note: submodule_errors_names[] is a horrible name.
It should be submodule_error_name[]. Look for "Array names"
in the CodingGuidelines document.
Working around "-Wsign-compare" has forced an unnecessary cast on
us here. If anything, we could have just done:
static const char *submodule_errors_to_string(unsigned mode)
and
if (ARRAY_SIZE(submodule_error_names) <= mode)
BUG(...);
which would have been vastly more readable. To me, a plain "int"
is also fine, but if we must squelch "-Wsign-compare", using
"unsigned" is much saner than turning everything into "size_t".
> +static int parse_submodule_errors(const char *name)
> +{
> + size_t i;
> +
> + for (i = 0; i < ARRAY_SIZE(submodule_errors_names); i++)
> + if (!strcmp(submodule_errors_names[i], name))
> + return i;
> + return -1;
> +}
And there is no sensible way to justify "size_t i" here. Using
a platform-natural "unsigned" would have been much easier to
understand.
It is a disease to bend our code only to appease the compiler's
warnings; we should resist such temptation.
Also worth reading:
https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
Thanks.
^ permalink raw reply
* Re: [PATCH v2 03/10] sequencer: be more careful with external merge
From: Junio C Hamano @ 2026-07-15 18:53 UTC (permalink / raw)
To: Phillip Wood
Cc: Oswald Buddenhagen, Phillip Wood, git, Uwe Kleine-König,
Farid Zakaria
In-Reply-To: <6cdccc2b-c0b4-497f-8408-a18bd0981505@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 15/07/2026 10:35, Phillip Wood wrote:
>> Hi Oswald
>>
>> On 13/07/2026 15:01, Oswald Buddenhagen wrote:
>>> On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:
>>>> If an external merge strategy cannot merge (for example because it
>>>> would overwrite an untracked file) it exits with a non-zero exit
>>>> code other than 1. This should be treated differently to a merge
>>>>
>>> s/to/from/, i think?
>>
>> Both are valid - the internet tells be "different to" is more common it
>
> sigh s/be/me/
>
> Phillip
sigh s/it/in/ ;-)
>
>> British English, whereas "different from" is more common in American
>> English. I guess for an international audience "from" would be the
>> better choice.
^ permalink raw reply
* Re: [PATCH GSoC v18 13/13] cat-file: make remote-object-info allow-list dynamic
From: Pablo Sabater @ 2026-07-15 18:52 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon
In-Reply-To: <xmqqcxwonnkx.fsf@gitster.g>
On Wed Jul 15, 2026 at 7:56 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> The static allow-list in expand_atom() is hardcoded to only allow
>> "objectname" and "objectsize" for remote queries. This works because
>> up to this point all servers will either support object-info with name
>> and size or they do not support them at all, but we cannot expect that
>> in a future different servers with different git versions to have the
>> same object-info capabilities. Therefore, the allow_list needs to be
>> dynamic depending on what the server advertises.
>>
>> The client will now:
>>
>> 1. Request the protocol option that the placeholder refers to (i.e.
>> "size" when "%(objectsize)").
>
> "when" -> "for"?
Will change.
>
>>
>> 2. Filters the request in fetch_object_info() dropping any option that
>> the server does not advertise.
>>
>> 3. After the fetching, the options that haven't been dropped are the ones
>> fetched and supported by the server, these supported options are
>> mapped and remote_allowed_atoms is populated with the placeholders.
>>
>> 4. expand_atom() checks remote_allowed_atoms with the same behaviour as
>> the static allow_list had.
>
> I am not sure I follow the above entirely. Could you add a
> concrete example to the commit message?
>
> For instance, if the client wants "%(objectsize) %(objectcolor)" and
> the server only supports 'size' but not 'color', the filtering in
> step (2) prevents the client from asking about the color, requesting
> only the size instead. When the server says the size is 42, step (3)
> uses that to substitute '%(objectsize)'. Would the end result then
> be "42 %(objectcolor)"?
You've gotten everything right until the last step, because we have only
size from the server there is no data to match %(objectcolor) and the
end result is an empty string for %(objeccolor):
"42 "
Note that %(objectcolor) doesn't exists and it would have die(), the
empty string is only for known but unsupported placeholders.
This is what for-each-ref does for known but unaplicable placeholders (atoms).
I'll add an example to the commmit log so it is clearer.
>
>> -static const char *remote_object_info_atoms[] = {
>> - "objectname",
>> - "objectsize",
>> + struct string_list remote_allowed_atoms;
>> };
>> +#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD, \
>> + .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
>
> Hmph, is this list expected to change over time? One-line-per-item
> format would be more suited for updates if it is the case.
Will format like so.
>
>> @@ -683,12 +675,12 @@ static int get_remote_info(struct batch_options *opt,
>> int argc,
>> const char **argv,
>> struct object_info **remote_object_info,
>> - struct oid_array *object_info_oids)
>> + struct oid_array *object_info_oids,
>> + struct string_list *object_info_options)
>> {
>> int retval = 0;
>> struct remote *remote = NULL;
>> struct object_id oid;
>> - struct string_list object_info_options = STRING_LIST_INIT_NODUP;
>> struct transport *gtransport;
>>
>> /*
>> @@ -736,15 +728,12 @@ static int get_remote_info(struct batch_options *opt,
>> CALLOC_ARRAY(*remote_object_info, object_info_oids->nr);
>> gtransport->smart_options->object_info_oids = object_info_oids;
>>
>> - string_list_append(&object_info_options, "size");
>> -
>> - if (object_info_options.nr > 0) {
>> - gtransport->smart_options->object_info_options = &object_info_options;
>> + if (object_info_options->nr > 0) {
>> + gtransport->smart_options->object_info_options = object_info_options;
>> gtransport->smart_options->object_info_data = *remote_object_info;
>> retval = transport_fetch_object_info(gtransport);
>> }
>
> This is not a new issue, but if the caller does not ask for
> anything in object_info_options, no call to
> transport_fetch_object_info() is made here. This is so even
> though we went through quite a lot of work, including the
> connection establishment and teardown below.
>
> By failing to contact the remote side, we wouldn't even know if
> the objects being queried actually exist there, which is
> probably even worse.
[Answered below]
>
>> static void parse_cmd_remote_object_info(struct batch_options *opt,
>> const char *line, struct strbuf *output,
>> struct expand_data *data)
>> @@ -839,6 +843,7 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
>> char *line_to_split;
>> struct object_info *remote_object_info = NULL;
>> struct oid_array object_info_oids = OID_ARRAY_INIT;
>> + struct string_list object_info_options = STRING_LIST_INIT_NODUP;
>>
>> if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
>> die(_("remote-object-info command too long"));
>> @@ -851,32 +856,57 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
>> die(_("remote-object-info supports at most %d objects"),
>> MAX_ALLOWED_OBJ_LIMIT);
>>
>> + if (data->info.sizep)
>> + string_list_append(&object_info_options, "size");
>> + if (data->info.typep)
>> + string_list_append(&object_info_options, "type");
>
> And if the request is only for "%(objectname)", an empty
> object_info_options is given to get_remote_info().
Right now 'name' is not part of the protocol as 'type' or 'size' are,
'objectname' is always allowed but only shown if it's present on the
format.
If the format is only "%(objectname)" then there's nothing to ask the
server for.
The current code avoids making the request if there's only objectname or
nothing supported, but still goes through the connection work. I will
add an early return to just output the oid back without any connection.
Returning the oid is what the client expects for which we don't need a
connection, but this means that we skip the existence check that
we would get if we asked for size.
The server is not ready to support a bare oid for existence check, it
could be done but that work belongs to a future series.
>
>> if (get_remote_info(opt, count, argv, &remote_object_info,
>> - &object_info_oids))
>> + &object_info_oids, &object_info_options))
>> goto cleanup;
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v7 3/3] replay: offer an option to linearize the commit topology
From: Junio C Hamano @ 2026-07-15 18:49 UTC (permalink / raw)
To: Elijah Newren; +Cc: Toon Claes, git, Johannes Schindelin
In-Reply-To: <CABPp-BGxO0bd3UzDYNnhNUgDSKYwcFVCFsJ9rCzmNX7Q0xBrow@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> You're right that when flattening merges within a single branch, the
> machinery must pick an order, and that's fine — unavoidable, even. My
> objection isn't that; it's primarily the concatenation of distinct
> branches named on the command line into one chain, and, as a secondary
> point, the ignoring of the order of branches explicitly specified by
> the user on the command line.
That is true, but a user who wishes to avoid flattening in
an unspecified order can always choose to supply only one
branch at a time on the command line.
> Concretely: I have three branches to rebase onto master; one of them
> happens to contain a merge I'd like flattened. I add --linearize for
> that one merge — and now all three branches are silently concatenated
> into a single chain. That makes no sense to me, and I think won't to
> most users.
But if that is not the outcome they wanted, I fail to see why they
would feed all three branches to a single invocation of --linearize
in the first place. After all, the command is only doing what it
was asked to do.
> Consider the following history
>
> M1 M2 M3 M4 M5
> *---*---*---*---* <- master
> \ \
> \ \ A1 A2 A3 A4
> \ \-*---*---*---* <- branchA
> \ \
> \ -*---* <- branchC
> \ C1 C2
> \
> \-*---*---* <- branchB
> B1 B2 B3
>
> git replay was designed to allow you to update all your branches at once.
> For example, with this above history, running
> git replay --onto master branchA branchB branchC
> will rebase all three branches onto master (and handles the shared portion
> of history between branchA and branchC in the obvious way):
> ...
> M1 M2 M3 M4 M5 B1 B2 B3 A1 A2 C1 C2 A3 A4
> *---*---*---*---*---*---*---*---*---*---*---*---*---*
> ^ ^ ^ ^
> | | | |
> master branchB branchC branchA
If that is not what you want, why did you give all three to the
single invocation? If you want A's and B's all consecutive, linearlize
branchA on top of 'master', and brnachB on top of it, and branch C
on top, perhaps?
If that breaks because by the time you feed branchC to the machinery
nobody remembers that A1 and A2 were already handled, _that_ is the
problem the command needs to solve, no? I am confused.
Or do you want to be able to tell "linearlize B, A, and C in this
turn on top of 'master'" and M1..M5..B1'..B3'..A1'..A4'..C1'..C2' as
the result? That would mean the command line syntax cannot be an
arbitrary rev list range, but limited to a single negative plus one
or more positive revision, which may be very limited but is much
less error prone for casual users.
^ permalink raw reply
* Git 2.55.0 breaks revision path filtering with --no-walk
From: Peter Colberg @ 2026-07-15 18:48 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson
Hi,
Since commit dd4bc01c0a8f ("revision: use priority queue for
non-limited streaming walks") in Git 2.55.0, git rev-list
--no-walk no longer considers optional <path>... arguments.
https://lore.kernel.org/git/pull.2127.git.1779897003.gitgitgadget@gmail.com/
The following example lists all commits between two Linux kernel
releases that modify paths within a given directory and further
modified paths outside of that directory, too.
With Git 2.54.0, the second rev-list correctly filters by paths:
% git rev-list --topo-order v7.0..v7.1 -- drivers/gpu/drm/ | wc -l
2026
% git rev-list --topo-order v7.0..v7.1 -- drivers/gpu/drm/ | git rev-list --stdin --no-walk=unsorted -- ':!drivers/gpu/drm/' | wc -l
146
With Git 2.55.0, the second rev-list passes through all commits:
% git rev-list --topo-order v7.0..v7.1 -- drivers/gpu/drm/ | wc -l
2026
% git rev-list --topo-order v7.0..v7.1 -- drivers/gpu/drm/ | git rev-list --stdin --no-walk=unsorted -- ':!drivers/gpu/drm/' | wc -l
2026
Reverting commit dd4bc01c0a8f ("revision: use priority queue for
non-limited streaming walks") on top of Git 2.55.0 restores the
previous behaviour. Specifically, the following hunk that no longer
invokes process_parents() in the no_walk case causes the regression.
@@ -4390,12 +4394,13 @@ static struct commit *get_revision_1(struct rev_info *revs)
break;
case REV_WALK_STREAMING:
if (process_parents(revs, commit,
- &revs->commits, NULL) < 0) {
+ &revs->commit_queue) < 0) {
if (!revs->ignore_missing_links)
die("Failed to traverse parents of commit %s",
oid_to_hex(&commit->object.oid));
}
break;
+ case REV_WALK_NO_WALK:
case REV_WALK_LIMITED:
break;
}
Is the behaviour in Git 2.55.0 intentional, i.e., was --no-walk never
intended to support path filtering, or is this indeed a regression?
Thanks,
Peter
^ permalink raw reply
* [PATCH v6] show-branch: convert per-branch flags to commit-slab
From: Gatla Vishweshwar Reddy @ 2026-07-15 18:42 UTC (permalink / raw)
To: gitster; +Cc: git, Gatla Vishweshwar Reddy
In-Reply-To: <xmqqy0fcnpee.fsf@gitster.g>
show-branch uses commit->object.flags to store per-branch
reachability bits, one bit per branch starting at REV_SHIFT.
The flags word has only a fixed number of available bits, limiting
the number of branches that can be shown simultaneously to MAX_REVS.
Convert the per-branch bits to a dedicated commit-slab using uint64_t
as the element type, initialized with a stride via
init_commit_rev_flags_with_stride(). Keep the UNINTERESTING bit in
object.flags where it belongs, as it is used for revision walking and
does not need to be in the per-branch slab. With UNINTERESTING removed
from the slab, REV_SHIFT becomes 0 and all 64 bits of uint64_t are
available for branch tracking, lifting MAX_REVS from 27 to 64 branches.
Add helper functions get_rev_flags_ptr(), peek_rev_flags_ptr(),
has_any_rev_flags(), or_rev_flag_bit(), test_rev_flag_bit(),
has_all_rev_flags(), has_only_rev_flag_bit() to encapsulate per-bit
slab access cleanly. Use has_only_rev_flag_bit() in show_independent()
to preserve the original semantics: a commit is independent only if
reachable from exactly one tip. Update all bit operations to use
UINT64_C(1) for correct 64-bit shifts.
Fix join_revs() to correctly propagate UNINTERESTING to parents: use
a local commit_is_merge_base variable to track whether the current
commit is a merge base, and propagate UNINTERESTING to its parents
without smudging the commit itself, matching the original behavior.
Update format strings from %d to %lu with unsigned long cast since
MAX_REVS is now size_t-based. Update documentation to reflect the
new limit of 64 branches. Add tests to verify show-branch works
correctly with more than 27 branches. Include revision.h for the
shared UNINTERESTING definition.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
Thank you for the thorough review. Replies inline:
> In the original, a local 'flags' variable is made UNINTERESTING,
> which is then used in the 'while' loop below to inspect and
> propagate the UNINTERESTING (and other) bits to the parents,
> without smudging the current commit itself.
Correct. Fixed in v6 using a local commit_is_merge_base variable.
When the commit has all rev bits set, we propagate UNINTERESTING
to its parents via p->object.flags |= UNINTERESTING, but do not
touch the commit itself. The postprocess loop then handles marking
merge-base commits UNINTERESTING for display purposes.
> In the updated code, you do not paint these parents UNINTERESTING
> at all.
Fixed. Parents are now painted UNINTERESTING when commit_is_merge_base
is true, matching the original p->object.flags |= flags behavior.
> What is this change about? (blank line removal)
An accidental whitespace change with no semantic meaning. Restored
in v6.
> our CodingGuidelines document says we cannot portably use "%zu"
> yet. Can't we use an unsigned long or something more established?
Changed to %lu with explicit (unsigned long) cast throughout.
> "It cannot show more than 26 branches and commits", which needs
> updating. We should check if any existing tests need updating,
> and write a few new ones.
Updated documentation to say 64. Added three new tests in
t3202-show-branch.sh verifying show-branch works correctly with
30 branches, including --independent and --merge-base modes.
---
Changes in v6:
- Fix join_revs() UNINTERESTING propagation (Junio)
- Fix parent skip condition (Junio)
- Restore blank line before postprocess comment (Junio)
- %zu -> %lu with (unsigned long) cast (Junio)
- Update docs from 26 to 64 branches (Junio)
- Add tests for 30+ branches (Junio)
Documentation/git-show-branch.adoc | 2 +-
builtin/show-branch.c | 199 ++++++++++++++++++-----------
t/t3202-show-branch.sh | 32 +++++
3 files changed, 157 insertions(+), 76 deletions(-)
diff --git a/Documentation/git-show-branch.adoc b/Documentation/git-show-branch.adoc
index 7e86d54a24..fe65c0a95a 100644
--- a/Documentation/git-show-branch.adoc
+++ b/Documentation/git-show-branch.adoc
@@ -22,7 +22,7 @@ Shows the commit ancestry graph starting from the commits named
with <rev>s or <glob>s (or all refs under refs/heads
and/or refs/tags) semi-visually.
-It cannot show more than 26 branches and commits at a time.
+It cannot show more than 64 branches and commits at a time.
It uses `showbranch.default` multi-valued configuration items if
no <rev> or <glob> is given on the command line.
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f02831b085..f7b52d6cb1 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -9,6 +9,7 @@
#include "hex.h"
#include "pretty.h"
#include "refs.h"
+#include "revision.h"
#include "color.h"
#include "strvec.h"
#include "object-name.h"
@@ -35,15 +36,12 @@ static enum git_colorbool showbranch_use_color = GIT_COLOR_UNKNOWN;
static struct strvec default_args = STRVEC_INIT;
/*
- * TODO: convert this use of commit->object.flags to commit-slab
- * instead to store a pointer to ref name directly. Then use the same
- * UNINTERESTING definition from revision.h here.
+ * TODO: store a pointer to ref name directly in the commit-slab
+ * instead, and use the UNINTERESTING definition from revision.h
+ * here once that is done.
*/
-#define UNINTERESTING 01
-
-#define REV_SHIFT 2
-#define MAX_REVS (FLAG_BITS - REV_SHIFT) /* should not exceed bits_per_int - REV_SHIFT */
-
+#define REV_SHIFT 0
+#define MAX_REVS (sizeof(uint64_t) * 8)
#define DEFAULT_REFLOG 4
static const char *get_color_code(int idx)
@@ -79,11 +77,72 @@ struct commit_name {
define_commit_slab(commit_name_slab, struct commit_name *);
static struct commit_name_slab name_slab;
+define_commit_slab(commit_rev_flags, uint64_t);
+static struct commit_rev_flags rev_flags_slab;
+static int flags_stride; /* number of uint64_t words per commit */
+
static struct commit_name *commit_to_name(struct commit *commit)
{
return *commit_name_slab_at(&name_slab, commit);
}
+static uint64_t *get_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_at(&rev_flags_slab, commit);
+}
+
+static uint64_t *peek_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_peek(&rev_flags_slab, commit);
+}
+
+static int has_any_rev_flags(struct commit *commit)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++)
+ if (f[i])
+ return 1;
+ return 0;
+}
+
+static void or_rev_flag_bit(struct commit *commit, int branch)
+{
+ get_rev_flags_ptr(commit)[branch / 64] |= UINT64_C(1) << (branch % 64);
+}
+
+static int test_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ return f && !!(f[branch / 64] & (UINT64_C(1) << (branch % 64)));
+}
+
+static int has_all_rev_flags(struct commit *commit, int num_rev)
+{
+ int i;
+ for (i = 0; i < num_rev; i++)
+ if (!test_rev_flag_bit(commit, i))
+ return 0;
+ return 1;
+}
+
+static int has_only_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++) {
+ uint64_t expected = (i == branch / 64)
+ ? (UINT64_C(1) << (branch % 64))
+ : 0;
+ if (f[i] != expected)
+ return 0;
+ }
+ return 1;
+}
/* Name the commit as nth generation ancestor of head_name;
* we count only the first-parent relationship for naming purposes.
@@ -215,7 +274,7 @@ static void name_commits(struct commit_list *list,
static int mark_seen(struct commit *commit, struct commit_list **seen_p)
{
- if (!commit->object.flags) {
+ if (!has_any_rev_flags(commit)) {
commit_list_insert(commit, seen_p);
return 1;
}
@@ -226,39 +285,43 @@ static void join_revs(struct prio_queue *queue,
struct commit_list **seen_p,
int num_rev, int extra)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
while (queue->nr) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
struct commit *commit = prio_queue_peek(queue);
bool get_pending = true;
- int flags = commit->object.flags & all_mask;
if (!still_interesting && extra <= 0)
break;
mark_seen(commit, seen_p);
- if ((flags & all_revs) == all_revs)
- flags |= UNINTERESTING;
- parents = commit->parents;
-
- while (parents) {
- struct commit *p = parents->item;
- int this_flag = p->object.flags;
- parents = parents->next;
- if ((this_flag & flags) == flags)
- continue;
- repo_parse_commit(the_repository, p);
- if (mark_seen(p, seen_p) && !still_interesting)
- extra--;
- p->object.flags |= flags;
- if (get_pending)
- prio_queue_replace(queue, p);
- else
- prio_queue_put(queue, p);
- get_pending = false;
+ {
+ int commit_is_merge_base = has_all_rev_flags(commit, num_rev);
+ parents = commit->parents;
+
+ while (parents) {
+ struct commit *p = parents->item;
+ parents = parents->next;
+ if (has_all_rev_flags(p, num_rev) &&
+ (!commit_is_merge_base || (p->object.flags & UNINTERESTING)))
+ continue;
+ repo_parse_commit(the_repository, p);
+ if (mark_seen(p, seen_p) && !still_interesting)
+ extra--;
+ {
+ int _b;
+ for (_b = 0; _b < num_rev; _b++)
+ if (test_rev_flag_bit(commit, _b))
+ or_rev_flag_bit(p, _b);
+ }
+ if (commit_is_merge_base)
+ p->object.flags |= UNINTERESTING;
+ if (get_pending)
+ prio_queue_replace(queue, p);
+ else
+ prio_queue_put(queue, p);
+ get_pending = false;
+ }
}
if (get_pending)
prio_queue_get(queue);
@@ -278,7 +341,7 @@ static void join_revs(struct prio_queue *queue,
struct commit *c = s->item;
struct commit_list *parents;
- if (((c->object.flags & all_revs) != all_revs) &&
+ if (!has_all_rev_flags(c, num_rev) &&
!(c->object.flags & UNINTERESTING))
continue;
@@ -410,9 +473,9 @@ static int append_ref(const char *refname, const struct object_id *oid,
return 0;
}
if (MAX_REVS <= ref_name_cnt) {
- warning(Q_("ignoring %s; cannot handle more than %d ref",
- "ignoring %s; cannot handle more than %d refs",
- MAX_REVS), refname, MAX_REVS);
+ warning(Q_("ignoring %s; cannot handle more than %lu ref",
+ "ignoring %s; cannot handle more than %lu refs",
+ MAX_REVS), refname, (unsigned long)MAX_REVS);
return 0;
}
ref_name[ref_name_cnt++] = xstrdup(refname);
@@ -511,15 +574,12 @@ static int rev_is_head(const char *head, const char *name)
static int show_merge_base(const struct commit_list *seen, int num_rev)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
int exit_status = 1;
for (const struct commit_list *s = seen; s; s = s->next) {
struct commit *commit = s->item;
- int flags = commit->object.flags & all_mask;
- if (!(flags & UNINTERESTING) &&
- ((flags & all_revs) == all_revs)) {
+ if (!(commit->object.flags & UNINTERESTING) &&
+ has_all_rev_flags(commit, num_rev)) {
puts(oid_to_hex(&commit->object.oid));
exit_status = 0;
commit->object.flags |= UNINTERESTING;
@@ -528,17 +588,13 @@ static int show_merge_base(const struct commit_list *seen, int num_rev)
return exit_status;
}
-static int show_independent(struct commit **rev,
- int num_rev,
- unsigned int *rev_mask)
+static int show_independent(struct commit **rev, int num_rev)
{
int i;
for (i = 0; i < num_rev; i++) {
struct commit *commit = rev[i];
- unsigned int flag = rev_mask[i];
-
- if (commit->object.flags == flag)
+ if (has_only_rev_flag_bit(commit, i))
puts(oid_to_hex(&commit->object.oid));
commit->object.flags |= UNINTERESTING;
}
@@ -603,13 +659,12 @@ static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
* Otherwise, if it is a merge that is reachable from only one
* tip, it is not that interesting.
*/
- int i, flag, count;
+ int i, count;
for (i = 0; i < n; i++)
if (rev[i] == commit)
return 0;
- flag = commit->object.flags;
for (i = count = 0; i < n; i++) {
- if (flag & (1u << (i + REV_SHIFT)))
+ if (test_rev_flag_bit(commit, i))
count++;
}
if (count == 1)
@@ -648,10 +703,8 @@ int cmd_show_branch(int ac,
char *reflog_msg[MAX_REVS] = {0};
struct commit_list *seen = NULL;
struct prio_queue queue = { compare_commits_by_commit_date };
- unsigned int rev_mask[MAX_REVS];
int num_rev, i, extra = 0;
int all_heads = 0, all_remotes = 0;
- int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
char *head;
struct object_id head_oid;
@@ -713,7 +766,8 @@ int cmd_show_branch(int ac,
const char **args_copy = NULL;
int ret;
- init_commit_name_slab(&name_slab);
+ flags_stride = (MAX_REVS + 63) / 64;
+ init_commit_rev_flags_with_stride(&rev_flags_slab, flags_stride);
repo_config(the_repository, git_show_branch_config, NULL);
@@ -779,9 +833,9 @@ int cmd_show_branch(int ac,
die(_("--reflog option needs one branch name"));
if (MAX_REVS < reflog)
- die(Q_("only %d entry can be shown at one time.",
- "only %d entries can be shown at one time.",
- MAX_REVS), MAX_REVS);
+ die(Q_("only %lu entry can be shown at one time.",
+ "only %lu entries can be shown at one time.",
+ MAX_REVS), (unsigned long)MAX_REVS);
if (!repo_dwim_ref(the_repository, *av, strlen(*av), &oid,
&ref, 0))
die(_("no such ref %s"), *av);
@@ -870,12 +924,12 @@ int cmd_show_branch(int ac,
for (num_rev = 0; ref_name[num_rev]; num_rev++) {
struct object_id revkey;
- unsigned int flag = 1u << (num_rev + REV_SHIFT);
+ int first_seen;
if (MAX_REVS <= num_rev)
- die(Q_("cannot handle more than %d rev.",
- "cannot handle more than %d revs.",
- MAX_REVS), MAX_REVS);
+ die(Q_("cannot handle more than %lu rev.",
+ "cannot handle more than %lu revs.",
+ MAX_REVS), (unsigned long)MAX_REVS);
if (repo_get_oid(the_repository, ref_name[num_rev], &revkey))
die(_("'%s' is not a valid ref."), ref_name[num_rev]);
commit = lookup_commit_reference(the_repository, &revkey);
@@ -885,17 +939,15 @@ int cmd_show_branch(int ac,
repo_parse_commit(the_repository, commit);
mark_seen(commit, &seen);
- /* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
- * and so on. REV_SHIFT bits from bit 0 are used for
- * internal bookkeeping.
+ /* rev#0 uses bit 0, rev#1 uses bit 1,
+ * and so on. All bits are available for branch tracking.
*/
- commit->object.flags |= flag;
- if (commit->object.flags == flag)
+ first_seen = !has_any_rev_flags(commit);
+ or_rev_flag_bit(commit, num_rev);
+ if (first_seen)
prio_queue_put(&queue, commit);
rev[num_rev] = commit;
}
- for (i = 0; i < num_rev; i++)
- rev_mask[i] = rev[i]->object.flags;
if (0 <= extra)
join_revs(&queue, &seen, num_rev, extra);
@@ -908,7 +960,7 @@ int cmd_show_branch(int ac,
}
if (independent) {
- ret = show_independent(rev, num_rev, rev_mask);
+ ret = show_independent(rev, num_rev);
goto out;
}
@@ -958,13 +1010,9 @@ int cmd_show_branch(int ac,
if (!sha1_name && !no_name)
name_commits(seen, rev, ref_name, num_rev);
- all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
for (struct commit_list *l = seen; l; l = l->next) {
struct commit *commit = l->item;
- int this_flag = commit->object.flags;
- int is_merge_point = ((this_flag & all_revs) == all_revs);
+ int is_merge_point = has_all_rev_flags(commit, num_rev);
shown_merge_point |= is_merge_point;
@@ -973,14 +1021,14 @@ int cmd_show_branch(int ac,
commit->parents->next);
if (topics &&
!is_merge_point &&
- (this_flag & (1u << REV_SHIFT)))
+ test_rev_flag_bit(commit, 0))
continue;
if (!sparse && is_merge &&
omit_in_dense(commit, rev, num_rev))
continue;
for (i = 0; i < num_rev; i++) {
int mark;
- if (!(this_flag & (1u << (i + REV_SHIFT))))
+ if (!test_rev_flag_bit(commit, i))
mark = ' ';
else if (is_merge)
mark = '-';
@@ -1010,6 +1058,7 @@ int cmd_show_branch(int ac,
free(reflog_msg[i]);
commit_list_free(seen);
clear_prio_queue(&queue);
+ clear_commit_rev_flags(&rev_flags_slab);
free(args_copy);
free(head);
return ret;
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index a1139f79e2..d04f642998 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -283,4 +283,36 @@ test_expect_success '--reflog handles missing reflog' '
test_must_be_empty actual
'
+test_expect_success 'show-branch with 30 branches succeeds' '
+ git checkout initial &&
+ for i in $(test_seq 11 30)
+ do
+ git checkout -b branch$i initial &&
+ test_commit --no-tag branch$i || return 1
+ done &&
+ git show-branch $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --independent with 30 branches' '
+ git show-branch --independent $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --merge-base with 30 branches' '
+ git rev-parse initial >expect &&
+ git show-branch --merge-base $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_cmp expect actual
+'
+
+
test_done
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v5 4/4] environment: move has_symlinks into repo_config_values
From: Junio C Hamano @ 2026-07-15 18:23 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, ps, cirnovskyv, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260715035501.48271-5-cat@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
> Note:
> To support early platform-specific (MinGW) overrides
> before repository initialization, a global variable
> 'default_has_symlinks' fallback is introduced as a fallback
> in environment.h. The *writer* in compat/mingw.c can only
> access this variable.
This may invite people to abuse the global variable. I wonder if we
want to do something similar to how we handle is_dir_sep() and
friends instead.
The idea is to have something like this in the generic header:
#ifndef platform_has_symlinks
#define platform_has_symlinks() 1
#endif
And then allow selected platforms override it:
/* in compat/mingw.h */
#define platform_has_symlinks() mingw_platform_has_symlinks()
extern int mingw_platform_has_symlinks(void);
/* in compat/mingw.c */
int mingw_platform_has_symlinks(void)
{
if (!(tmp = getenv("MSYS")) || !strstr(tmp, "winsymlinks:nativestrict"))
return 0;
else
return 1;
}
This keeps the namespace clean and avoids exposing a mutable state
variable that others might be tempted to meddle with.
^ permalink raw reply
* Re: [PATCH GSoC v18 10/13] transport: add client support for object-info
From: Pablo Sabater @ 2026-07-15 18:08 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <xmqqpl0onp5d.fsf@gitster.g>
On Wed Jul 15, 2026 at 7:22 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> +static size_t parse_object_size(const char *s, size_t *res)
>> +{
>> + uintmax_t uim;
>> +
>> + if (!s[0] || s[strspn(s, "0123456789")])
>> + return -1;
>> + errno = 0;
>> + uim = strtoumax(s, NULL, 10);
>> + if (errno || uim > SIZE_MAX)
>> + return -1;
>> + *res = uim;
>> + return 0;
>> +}
>
> Since size_t is unsigned, returning -1 is a bit problematic,
> isn't it? Perhaps this should return a plain 'int' instead.
>
> The sole caller only cares about a boolean "did we succeed or
> fail?" result, and more importantly, the actual size parsed
> is already returned via the out-parameter.
>
> Thanks.
Completly true, when I changed this from beign strtoumax_szt() I must have
been thinking too much about size_t.
I will change it to return int. Thanks.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH GSoC v18 11/13] cat-file: add remote-object-info to batch-command
From: Pablo Sabater @ 2026-07-15 18:06 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <xmqqjyqwnoqf.fsf@gitster.g>
>
>> +static void parse_cmd_remote_object_info(struct batch_options *opt,
>> + const char *line, struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + int count;
>> + const char **argv;
>> + char *line_to_split;
>> + struct object_info *remote_object_info = NULL;
>> + struct oid_array object_info_oids = OID_ARRAY_INIT;
>> +
>> + if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
>> + die(_("remote-object-info command too long"));
>> +
>> + line_to_split = xstrdup(line);
>> + count = split_cmdline(line_to_split, &argv);
>> + if (count < 0)
>> + die(_("remote-object-info: %s"), split_cmdline_strerror(count));
>> + if (count - 1 > MAX_ALLOWED_OBJ_LIMIT)
>> + die(_("remote-object-info supports at most %d objects"),
>> + MAX_ALLOWED_OBJ_LIMIT);
>> +
>> + if (get_remote_info(opt, count, argv, &remote_object_info,
>> + &object_info_oids))
>> + goto cleanup;
>
> Since this function does not return a value, the caller cannot
> even tell if there was an error if we just silently return like
> this. Is it really OK to silently ignore such a failure? Should
> we not die() loudly to report it instead?
True, this comes from the v11 before I got into, I think Eric tried to do
something like what 'info' does (it doesn't die) prints "<oid> missing"
but this comes from failing fetching.
'remote-object-info' prints "<oid> missing" when the fetching works but
the oid is unrecognized.
I will add a die instead of the goto. Thanks.
>
> Thanks.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH GSoC v18 13/13] cat-file: make remote-object-info allow-list dynamic
From: Junio C Hamano @ 2026-07-15 17:56 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon
In-Reply-To: <20260715-ps-eric-work-rebase-v18-13-34d7adb051bb@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> The static allow-list in expand_atom() is hardcoded to only allow
> "objectname" and "objectsize" for remote queries. This works because
> up to this point all servers will either support object-info with name
> and size or they do not support them at all, but we cannot expect that
> in a future different servers with different git versions to have the
> same object-info capabilities. Therefore, the allow_list needs to be
> dynamic depending on what the server advertises.
>
> The client will now:
>
> 1. Request the protocol option that the placeholder refers to (i.e.
> "size" when "%(objectsize)").
"when" -> "for"?
>
> 2. Filters the request in fetch_object_info() dropping any option that
> the server does not advertise.
>
> 3. After the fetching, the options that haven't been dropped are the ones
> fetched and supported by the server, these supported options are
> mapped and remote_allowed_atoms is populated with the placeholders.
>
> 4. expand_atom() checks remote_allowed_atoms with the same behaviour as
> the static allow_list had.
I am not sure I follow the above entirely. Could you add a
concrete example to the commit message?
For instance, if the client wants "%(objectsize) %(objectcolor)" and
the server only supports 'size' but not 'color', the filtering in
step (2) prevents the client from asking about the color, requesting
only the size instead. When the server says the size is 42, step (3)
uses that to substitute '%(objectsize)'. Would the end result then
be "42 %(objectcolor)"?
> -static const char *remote_object_info_atoms[] = {
> - "objectname",
> - "objectsize",
> + struct string_list remote_allowed_atoms;
> };
> +#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD, \
> + .remote_allowed_atoms = STRING_LIST_INIT_NODUP }
Hmph, is this list expected to change over time? One-line-per-item
format would be more suited for updates if it is the case.
> @@ -683,12 +675,12 @@ static int get_remote_info(struct batch_options *opt,
> int argc,
> const char **argv,
> struct object_info **remote_object_info,
> - struct oid_array *object_info_oids)
> + struct oid_array *object_info_oids,
> + struct string_list *object_info_options)
> {
> int retval = 0;
> struct remote *remote = NULL;
> struct object_id oid;
> - struct string_list object_info_options = STRING_LIST_INIT_NODUP;
> struct transport *gtransport;
>
> /*
> @@ -736,15 +728,12 @@ static int get_remote_info(struct batch_options *opt,
> CALLOC_ARRAY(*remote_object_info, object_info_oids->nr);
> gtransport->smart_options->object_info_oids = object_info_oids;
>
> - string_list_append(&object_info_options, "size");
> -
> - if (object_info_options.nr > 0) {
> - gtransport->smart_options->object_info_options = &object_info_options;
> + if (object_info_options->nr > 0) {
> + gtransport->smart_options->object_info_options = object_info_options;
> gtransport->smart_options->object_info_data = *remote_object_info;
> retval = transport_fetch_object_info(gtransport);
> }
This is not a new issue, but if the caller does not ask for
anything in object_info_options, no call to
transport_fetch_object_info() is made here. This is so even
though we went through quite a lot of work, including the
connection establishment and teardown below.
By failing to contact the remote side, we wouldn't even know if
the objects being queried actually exist there, which is
probably even worse.
> static void parse_cmd_remote_object_info(struct batch_options *opt,
> const char *line, struct strbuf *output,
> struct expand_data *data)
> @@ -839,6 +843,7 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
> char *line_to_split;
> struct object_info *remote_object_info = NULL;
> struct oid_array object_info_oids = OID_ARRAY_INIT;
> + struct string_list object_info_options = STRING_LIST_INIT_NODUP;
>
> if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
> die(_("remote-object-info command too long"));
> @@ -851,32 +856,57 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
> die(_("remote-object-info supports at most %d objects"),
> MAX_ALLOWED_OBJ_LIMIT);
>
> + if (data->info.sizep)
> + string_list_append(&object_info_options, "size");
> + if (data->info.typep)
> + string_list_append(&object_info_options, "type");
And if the request is only for "%(objectname)", an empty
object_info_options is given to get_remote_info().
> if (get_remote_info(opt, count, argv, &remote_object_info,
> - &object_info_oids))
> + &object_info_oids, &object_info_options))
> goto cleanup;
^ permalink raw reply
* Re: [PATCH GSoC v18 11/13] cat-file: add remote-object-info to batch-command
From: Junio C Hamano @ 2026-07-15 17:31 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <20260715-ps-eric-work-rebase-v18-11-34d7adb051bb@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> +static void parse_cmd_remote_object_info(struct batch_options *opt,
> + const char *line, struct strbuf *output,
> + struct expand_data *data)
> +{
> + int count;
> + const char **argv;
> + char *line_to_split;
> + struct object_info *remote_object_info = NULL;
> + struct oid_array object_info_oids = OID_ARRAY_INIT;
> +
> + if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE)
> + die(_("remote-object-info command too long"));
> +
> + line_to_split = xstrdup(line);
> + count = split_cmdline(line_to_split, &argv);
> + if (count < 0)
> + die(_("remote-object-info: %s"), split_cmdline_strerror(count));
> + if (count - 1 > MAX_ALLOWED_OBJ_LIMIT)
> + die(_("remote-object-info supports at most %d objects"),
> + MAX_ALLOWED_OBJ_LIMIT);
> +
> + if (get_remote_info(opt, count, argv, &remote_object_info,
> + &object_info_oids))
> + goto cleanup;
Since this function does not return a value, the caller cannot
even tell if there was an error if we just silently return like
this. Is it really OK to silently ignore such a failure? Should
we not die() loudly to report it instead?
Thanks.
^ permalink raw reply
* Re: [PATCH GSoC v18 10/13] transport: add client support for object-info
From: Junio C Hamano @ 2026-07-15 17:22 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon, Calvin Wan, Jonathan Tan
In-Reply-To: <20260715-ps-eric-work-rebase-v18-10-34d7adb051bb@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
> +static size_t parse_object_size(const char *s, size_t *res)
> +{
> + uintmax_t uim;
> +
> + if (!s[0] || s[strspn(s, "0123456789")])
> + return -1;
> + errno = 0;
> + uim = strtoumax(s, NULL, 10);
> + if (errno || uim > SIZE_MAX)
> + return -1;
> + *res = uim;
> + return 0;
> +}
Since size_t is unsigned, returning -1 is a bit problematic,
isn't it? Perhaps this should return a plain 'int' instead.
The sole caller only cares about a boolean "did we succeed or
fail?" result, and more importantly, the actual size parsed
is already returned via the out-parameter.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 4/4] environment: move has_symlinks into repo_config_values
From: Junio C Hamano @ 2026-07-15 17:18 UTC (permalink / raw)
To: Christian Couder
Cc: Tian Yuchen, git, ps, cirnovskyv, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <CAP8UFD2=FbbnCqWkTLEGBpz=90sh=j_70h2UJR=p4uj6u3tqMQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Wed, Jul 15, 2026 at 5:55 AM Tian Yuchen <cat@malon.dev> wrote:
>>
>> Move the global 'has_symlinks' configuration into the
>> repository-specific 'repo_config_values' struct.
>>
>> To ensure code readability, the getter function
>> 'repo_has_symlinks()' has been introduced. Callers access
>> this configuration by passing in 'repo' when possible,
>> and explicitly fall back to 'the_repository' the rest
>> of the time.
>>
>> Note:
>> To support early platform-specific (MinGW) overrides
>> before repository initialization, a global variable
>> 'default_has_symlinks' fallback is introduced as a fallback
>
> It seems a bit redundant to use "fallback" twice in the above sentence.
>
>> in environment.h. The *writer* in compat/mingw.c can only
>> access this variable.
>
> Otherwise this series looks good to me.
Thanks for helping, Christian, and thanks, Tian, for working on this
topic.
^ permalink raw reply
* Re: [PATCH v5] show-branch: convert per-branch flags to commit-slab
From: Junio C Hamano @ 2026-07-15 17:17 UTC (permalink / raw)
To: Gatla Vishweshwar Reddy; +Cc: git
In-Reply-To: <20260715120156.53025-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
> +static struct commit_rev_flags rev_flags_slab;
> +static int flags_stride; /* number of uint64_t words per commit */
> +
> static struct commit_name *commit_to_name(struct commit *commit)
> {
> return *commit_name_slab_at(&name_slab, commit);
> }
>
> +static uint64_t *get_rev_flags_ptr(struct commit *commit)
> +{
> + return commit_rev_flags_at(&rev_flags_slab, commit);
> +}
> +
> +static uint64_t *peek_rev_flags_ptr(struct commit *commit)
> +{
> + return commit_rev_flags_peek(&rev_flags_slab, commit);
> +}
> +
> +static int has_any_rev_flags(struct commit *commit)
> +{
> + uint64_t *f = peek_rev_flags_ptr(commit);
> + int i;
> + if (!f)
> + return 0;
> + for (i = 0; i < flags_stride; i++)
> + if (f[i])
> + return 1;
> + return 0;
> +}
We are no longer limited to 26 or 64, which is excellent. Early
in "git show-branch --help", we prominently say "It cannot show
more than 26 branches and commits", which needs updating.
I wonder if we have enough test coverage for this command. If we
were paranoid, we might have had a test that feeds 30 revs to make
sure the command fails, which would now fail with this change.
We should check if any existing tests need updating, and write a
few new ones to ensure proper coverage of the expanded limits.
> @@ -226,34 +285,34 @@ static void join_revs(struct prio_queue *queue,
> struct commit_list **seen_p,
> int num_rev, int extra)
> {
> - int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
> - int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
> -
> while (queue->nr) {
> struct commit_list *parents;
> int still_interesting = !!interesting(queue);
> struct commit *commit = prio_queue_peek(queue);
> bool get_pending = true;
> - int flags = commit->object.flags & all_mask;
>
> if (!still_interesting && extra <= 0)
> break;
>
> mark_seen(commit, seen_p);
> - if ((flags & all_revs) == all_revs)
> - flags |= UNINTERESTING;
> + if (has_all_rev_flags(commit, num_rev))
> + commit->object.flags |= UNINTERESTING;
I am afraid these two do quite different things.
In the original, a local 'flags' variable is made UNINTERESTING,
which is then used in the 'while' loop below to inspect and
propagate the UNINTERESTING (and other) bits to the parents,
without smudging the current commit itself.
In the updated code, you smudge the commit in question itself with
the UNINTERESTING bit. Won't that prevent this commit, which is a
merge-base, from being shown?
> parents = commit->parents;
>
> while (parents) {
> struct commit *p = parents->item;
> - int this_flag = p->object.flags;
> parents = parents->next;
> - if ((this_flag & flags) == flags)
> + if (has_all_rev_flags(p, num_rev))
> continue;
> repo_parse_commit(the_repository, p);
> if (mark_seen(p, seen_p) && !still_interesting)
> extra--;
> - p->object.flags |= flags;
> + {
> + int _b;
> + for (_b = 0; _b < num_rev; _b++)
> + if (test_rev_flag_bit(commit, _b))
> + or_rev_flag_bit(p, _b);
> + }
This part also behaves quite differently. The original checks if
the parent already has all the bits in 'flags' (including the
UNINTERESTING bit) and avoids traversing further if so. If the
parent is missing any of those bits, however, they are
propagated down to it.
In the updated code, you do not paint these parents
UNINTERESTING at all.
> @@ -263,7 +322,6 @@ static void join_revs(struct prio_queue *queue,
> if (get_pending)
> prio_queue_get(queue);
> }
> -
> /*
> * Postprocess to complete well-poisoning.
> *
What is this change about?
> - warning(Q_("ignoring %s; cannot handle more than %d ref",
> - "ignoring %s; cannot handle more than %d refs",
> + warning(Q_("ignoring %s; cannot handle more than %zu ref",
> + "ignoring %s; cannot handle more than %zu refs",
> MAX_REVS), refname, MAX_REVS);
Indeed. Since you are no longer limited to 27 or 64 bits, it is
certainly nice to see that the code is prepared to bust the %d
limit. ;-)
However, our CodingGuidelines document says we cannot portably use
"%zu" yet. Can't we use an unsigned long or something more
established here? We surely do not expect to ever fill the full
range expressible by size_t.
Thanks.
^ permalink raw reply
* Re: [PATCH] mv: report missing destination leading directory
From: Ben Knoble @ 2026-07-15 16:46 UTC (permalink / raw)
To: Lucas Zamboni Orioli via GitGitGadget; +Cc: git, Lucas Zamboni Orioli
In-Reply-To: <pull.2356.git.git.1784125963694.gitgitgadget@gmail.com>
> Le 15 juil. 2026 à 10:51, Lucas Zamboni Orioli via GitGitGadget <gitgitgadget@gmail.com> a écrit :
>
> From: Lucas Zamboni Orioli <lucaszam0@gmail.com>
>
> When moving a file to a destination whose leading directory does not
> exist, "git mv" fails at the rename(2) syscall with ENOENT. Because
> the error is reported via die_errno() using only the source path:
>
> fatal: renaming 'src' failed: No such file or directory
>
> the message misleadingly blames the source, even though it is the
> destination's parent directory that is missing. A user who runs
>
> git mv a/file b/does-not-exist/file
>
> is told the problem is with 'a/file', which exists, giving no hint
> that 'b/does-not-exist/' needs to be created first.
>
> The checking phase already rejects a missing destination directory
> when the destination ends in a slash, but a destination that names a
> file inside a non-existent directory is not caught and only fails
> later at rename(2). As a result "git mv -n" also fails to detect the
> problem, since the dry run never reaches the syscall and reports a
> move that would not actually succeed.
>
> Detect this during the checking phase instead: for entries that will
> be renamed on disk, stat the destination's leading directory and, if
> it is missing, fail with the existing "destination directory does not
> exist" message. Guard the check with the same condition under which
> rename(2) is invoked so that directory moves, whose child entries are
> expanded to paths under a not-yet-created directory, and sparse or
> out-of-cone destinations, which are not written to the worktree, are
> not flagged incorrectly.
I suppose this still allows a TOCTOU issue where the check succeeds and (with lucky timing) the destination then disappears?
In that case, I think a worthwhile additional change would also be for the error message to diagnose which file is missing (or at least include both source and destination).
Now, without checking I somehow doubt whether rename(2) tells us which entry is missing. Worse, if we check afterwards, we could have a « TOUTOC » :p where the entry reappears to confuse the error diagnosis.
So perhaps
fatal: renaming A -> B failed: no such file or directory
taking some inspiration from the -i modes of cp, mv?
> This gives a clear message and lets "git mv -n" report the failure.
>
> Signed-off-by: Lucas Zamboni Orioli <lucaszam0@gmail.com>
> ---
> mv: report missing destination leading directory
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2356%2FZamboniL%2Fmv-detect-non-existing-target-folder-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2356/ZamboniL/mv-detect-non-existing-target-folder-v1
> Pull-Request: https://github.com/git/git/pull/2356
>
> builtin/mv.c | 21 +++++++++++++++++++++
> t/t7001-mv.sh | 14 ++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index e03823370c..a95531f0b2 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -444,6 +444,27 @@ dir_check:
> goto act_on_entry;
> }
>
> + /*
> + * If we are going to move SRC to DST on disk, DST's leading
> + * directories must already exist.
> + */
> + if (!(modes[i] & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> + !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE))) {
> + char *dst_dir = xstrdup(dst);
> + char *slash = strrchr(dst_dir, '/');
> +
> + if (slash) {
> + struct stat dir_st;
> + *slash = '\0';
> + if (lstat(dst_dir, &dir_st) < 0 && errno == ENOENT) {
> + free(dst_dir);
> + bad = _("destination directory does not exist");
> + goto act_on_entry;
> + }
> + }
> + free(dst_dir);
> + }
> +
> if (ignore_sparse &&
> (dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
> index_entry_exists(the_repository->index, dst, strlen(dst))) {
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 920479e925..8a45997b33 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -114,6 +114,20 @@ test_expect_success 'clean up' '
> git reset --hard
> '
>
> +test_expect_success 'moving to non-existent destination parent directory' '
> + git reset --hard &&
> + mkdir -p from &&
> + echo content >from/file &&
> + git add from/file &&
> + test_must_fail git mv from/file no-such-dir/file 2>actual &&
> + test_grep "destination directory does not exist" actual
> +'
> +
> +test_expect_success 'mv --dry-run detects non-existent destination parent directory' '
> + test_must_fail git mv -n from/file no-such-dir/file 2>actual &&
> + test_grep "destination directory does not exist" actual
> +'
> +
> test_expect_success 'moving to existing untracked target with trailing slash' '
> mkdir path1 &&
> git mv path0/ path1/ &&
>
> base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
> --
> gitgitgadget
>
^ permalink raw reply
* [PATCH] trace2: tolerate failed timestamp formatting
From: Derrick Stolee via GitGitGadget @ 2026-07-15 16:12 UTC (permalink / raw)
To: git; +Cc: gitster, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <stolee@gmail.com>
Some users reported issues of repeated messages:
fatal: recursion detected in die handler
This wasn't happening every time, but we eventually captured a
GIT_TRACE2_PERF log file with this issue and revealed an interesting
internal detail, failing with this message:
unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ
This specific format string tracks to tr2_tbuf_utc_datetime_extended()
in trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in
ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) but
was later split in bad229aef23 (trace2: clarify UTC datetime formatting,
2019-04-15).
This use of xsnprintf() is writing a very specific datetime format into a
32-character buffer. The format requires that the input data will not
overflow the format digits or the buffer will not hold the result. Since
we are using xsnprintf() here, those failures turn into die() events.
This method and its siblings, tr2_tbuf_local_time() and
tr2_tbuf_utc_datetime(), are used in the tracing library. The extended
form is used only for the 'event' format, which these users were using
via a config setting for use in client-side telemetry. The non-extended
form is used to help generate the 'SID' that defines the process in the
traces.
Not only are these inappropriate times for a failure, but the extended
method is called specifially during the 'atexit' event, which was
triggering this problem in a loop as the 'atexit' event would be
retriggered by the die().
I could not determine the exact cause of why these errors started
occuring in a bunch. My best guess is that these users are dogfooding an
early operating system version that is more likely to fail in the
gettimeofday() function and thus leaves the structures uninitialized and
potentially violating the expected values.
However, for full defense-in-depth I made several modifications:
1. Both 'tv' and 'tm' structs are initialized with zero values, allowing
an erroring gettimeofday() or gmtime_r() method to leave them
zero-valued. A zero-valued date is better than a die() here.
2. Replace the use of xsnprintf() with snprintf() to avoid the
possibility of calling die() here. Instead, check the response to see
if there was a failure. On failure, put a blank value into the buffer
instead of possibly allowing a value that would not format correctly
for a trace2 consumer. This value should be seen as obviously wrong
and therefore signals a problem.
As the core issue in this code seems to require a system method
returning an error, no test accompanies this change.
This change removes all uses of xsnprintf() from the trace2/ directory.
There are two uses of xstrdup() that could be considered for removal,
but they only die() on out-of-memory errors instead of formatting
issues. I chose to leave those in place for now.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
trace2: tolerate failed timestamp formatting
As mentioned, this is based on real trace logs of failed commands users
are seeing.
I wish I had a better way to test this or to be 100% sure that the
system call was failing. But users were seeing failures and these seemed
like appropriate changes.
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2178%2Fderrickstolee%2Ftrace2-dont-die-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2178/derrickstolee/trace2-dont-die-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2178
trace2/tr2_tbuf.c | 49 ++++++++++++++++++++++++++++++++---------------
1 file changed, 34 insertions(+), 15 deletions(-)
diff --git a/trace2/tr2_tbuf.c b/trace2/tr2_tbuf.c
index c3b3822ed7..ef57376f3c 100644
--- a/trace2/tr2_tbuf.c
+++ b/trace2/tr2_tbuf.c
@@ -3,45 +3,64 @@
void tr2_tbuf_local_time(struct tr2_tbuf *tb)
{
- struct timeval tv;
- struct tm tm;
+ struct timeval tv = { 0 };
+ struct tm tm = { 0 };
time_t secs;
+ int len;
gettimeofday(&tv, NULL);
secs = tv.tv_sec;
localtime_r(&secs, &tm);
- xsnprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld", tm.tm_hour,
- tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+ len = snprintf(tb->buf, sizeof(tb->buf), "%02d:%02d:%02d.%06ld",
+ tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+ if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+ const char *blank = "00:00:00.000000";
+ strlcpy(tb->buf, blank, sizeof(tb->buf));
+ }
}
void tr2_tbuf_utc_datetime_extended(struct tr2_tbuf *tb)
{
- struct timeval tv;
- struct tm tm;
+ struct timeval tv = { 0 };
+ struct tm tm = { 0 };
time_t secs;
+ int len;
gettimeofday(&tv, NULL);
secs = tv.tv_sec;
gmtime_r(&secs, &tm);
- xsnprintf(tb->buf, sizeof(tb->buf),
- "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ", tm.tm_year + 1900,
- tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec,
- (long)tv.tv_usec);
+ len = snprintf(tb->buf, sizeof(tb->buf),
+ "%4d-%02d-%02dT%02d:%02d:%02d.%06ldZ",
+ tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+ tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+ if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+ const char *blank = "1900-00-00T00:00:00.000000Z";
+ strlcpy(tb->buf, blank, sizeof(tb->buf));
+ }
}
void tr2_tbuf_utc_datetime(struct tr2_tbuf *tb)
{
- struct timeval tv;
- struct tm tm;
+ struct timeval tv = { 0 };
+ struct tm tm = { 0 };
time_t secs;
+ int len;
gettimeofday(&tv, NULL);
secs = tv.tv_sec;
gmtime_r(&secs, &tm);
- xsnprintf(tb->buf, sizeof(tb->buf), "%4d%02d%02dT%02d%02d%02d.%06ldZ",
- tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
- tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+ len = snprintf(tb->buf, sizeof(tb->buf),
+ "%4d%02d%02dT%02d%02d%02d.%06ldZ",
+ tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+ tm.tm_hour, tm.tm_min, tm.tm_sec, (long)tv.tv_usec);
+
+ if (len < 0 || (size_t)len >= sizeof(tb->buf)) {
+ const char *blank = "19000000T000000.000000Z";
+ strlcpy(tb->buf, blank, sizeof(tb->buf));
+ }
}
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3] sequencer: honor --empty when a fixup!/squash! empties its target
From: Phillip Wood @ 2026-07-15 15:44 UTC (permalink / raw)
To: Farid Zakaria, Junio C Hamano
Cc: git, Phillip Wood, Elijah Newren, Patrick Steinhardt
In-Reply-To: <DJXL4KSUEAD4.1EE4ERHJZ00TR@gmail.com>
Hi Farid
On 13/07/2026 17:30, Farid Zakaria wrote:
> On Mon Jul 13, 2026 at 6:18 AM PDT, Phillip Wood wrote:
>> On 12/07/2026 06:01, Junio C Hamano wrote:
>
> Thanks for cc'd. I'm not familiar with the workflow (I read the docs)
> but is there an email reply when it's accepted into 'next' that I will
> just look-out for ? I'm not subscribed to the mailing list in general
> otherwise.
There isn't a specific notification for each topic, but the status of
all topics is in the regular "what's cooking in git.git" email on the list.
>>> So it might make sense for you to coordinate with Phillip, and wait
>>> for his topic to be merged to 'next'. After that happens, you would
>>> prepare a merge commit of the other branch into f85a7e6620 (Start
>>> Git 2.56 cycle, 2026-07-06) or some other stable point, and rebuild
>>> this patch on top of it. That way, it will be much less likely that
>>> I'd make stupid and unnecessary mismerges when attempting to
>>> integrate this topic into my tree.
>>
>> That makes sense, assuming no-one has any more comments on
>> 'pw/rebase-drop-notes-with-commit' it should in be 'next' fairly soon.
>>
>> Thanks
>>
>> Phillip
>
> Phillip,
>
> Let me know if you have any more comments. I suspect not much will
> changes logic-wise once I rebase it onto 'next'.
I've left some comments on the patch in a separate mail.
> For clarity, is the f85a7e6620 commit the 'next' branch ? I would have
> thought to just rebase ontop of 'next' and I'm a bit confused with this
> commit hash.
In general it is better to base patches directly on top of the topic
they build on rather than on top of next. Once a topic is merged to next
it should be stable, whereas the tip of next is periodically rebuilt and
force-pushed. The tip of pw/rebase-drop-notes-with-commit is currently
7e70d12417d (sequencer: do not record dropped commits as rewritten,
2026-07-13) but that will change when Junio picks up v3. I find the
branch tips in seen and next with
git show $(git log --merges --format=%H --grep 'pw/.*drop-notes/' \
-1 origin/seen)^2
> If there is anything else I should be aware of, I would appreciate a CC
> if you can remember :)
Elsewhere you asked about using AI. There are some notes about that in
Documentation/SubmittingPatches. TLDR it is fine so long as it does not
conflict with your obligations under the Developer Certificate of Origin.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH] remote-curl: simplify passing of push specs
From: René Scharfe @ 2026-07-15 15:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Git List
In-Reply-To: <alcrhGUCVMCnm2-i@pks.im>
On 7/15/26 8:41 AM, Patrick Steinhardt wrote:
> On Wed, Jul 15, 2026 at 06:41:17AM +0200, René Scharfe wrote:
>> diff --git a/remote-curl.c b/remote-curl.c
>> index 9e614c5567..2c35dd5240 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -1340,10 +1340,9 @@ static void parse_get(const char *arg)
>> fflush(stdout);
>> }
>>
>> -static int push_dav(int nr_spec, const char **specs)
>> +static int push_dav(const char **specs)
>> {
>> struct child_process child = CHILD_PROCESS_INIT;
>> - size_t i;
>>
>> child.git_cmd = 1;
>> strvec_push(&child.args, "http-push");
>
> I wonder whether the interface would be even better if we simply passed
> around a `const struct strvec *` directly. That makes it explicit what
> kind of guarantees we have, and all transitive callers already have one
> available anyway.
You mean that passing a managed array instead of a plain NULL-terminated
one would make more places visibly safer at almost no cost?
>> @@ -1353,15 +1352,14 @@ static int push_dav(int nr_spec, const char **specs)
>> if (options.verbosity > 1)
>> strvec_push(&child.args, "--verbose");
>> strvec_push(&child.args, url.buf);
>> - for (i = 0; i < nr_spec; i++)
>> - strvec_push(&child.args, specs[i]);
>> + strvec_pushv(&child.args, specs);
>
> I thought that we had something like `strvec_pushvec()` that knew to
> also optimize for this case so that we don't have to reallocate the
> vector multiple times. And if we had that function it would even be more
> efficient to pass it down the stack. But we seemingly don't have it, so
> that argument is kind of moot.
We could add one. Not sure it would make a measurable difference; if
the number of specs is huge there are probably other costs that dwarf
pushing them to a strvec.
I have to admit that the simplicity of strvec_pushv() nudged me towards
using a NULL-terminated array here, though. So just having a
strvec_pushvec() available could guide towards using the length-limited
strvec instead of a simpler NULL-terminated array (which explodes if
left unterminated).
René
^ permalink raw reply
* Re: [PATCH GSoC v17 00/13] cat-file: add remote-object-info to batch-command
From: Pablo Sabater @ 2026-07-15 15:36 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater
Cc: chandrapratap3519, chriscool, eric.peijian, git, jltobler,
karthik.188, peff, toon
In-Reply-To: <xmqqfr1kp98u.fsf@gitster.g>
On Wed Jul 15, 2026 at 5:23 PM CEST, Junio C Hamano wrote:
> "Pablo Sabater" <pabloosabaterr@gmail.com> writes:
>
>>> Thanks. How close are we to the finish line, by the way?
>>
>> There's one month left. Final evaluation ends on 17th August (more weeks
>> can be asked, if it seems too rushed, ...
>
> That is the deadline to wrap up your work, which is not quite what
> I was asking. I meant to ask how close you assess this topic is
> to completion at iteration #18. Are all remaining issues just
> minor nits? Are there still large gaps between the desired and
> actual behavior of the new feature? That sort of thing.
Oh, sorry.
It already does what it's supposed to do, the last rerolls have been
mostly cleanups.
If there's anything more to do it should only be minor nits.
>
> Thanks.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v3] sequencer: honor --empty when a fixup!/squash! empties its target
From: Phillip Wood @ 2026-07-15 15:30 UTC (permalink / raw)
To: Farid Zakaria, git
Cc: Phillip Wood, Elijah Newren, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20260711-fz-autosquash-empty-v3-1-d227b63eb511@gmail.com>
Hi Farid
On 12/07/2026 01:38, Farid Zakaria wrote:
> When "git rebase --autosquash" melds a "fixup!" or "squash!" commit into
> its target, the result can be a commit that no longer changes anything
> relative to its parent, for example when the melded change reverts the
> target. Rather than dropping or keeping this empty commit, the rebase
> stops with
>
> You asked to amend the most recent commit, but doing so would
> make it empty. ...
>
> and the "--empty" option has no effect on it. This makes backing a
> change out of a series awkward: reverting a commit as a "fixup!" and
> running "git rebase --autosquash --empty=drop" ought to remove both the
> commit and its revert, but it halts instead.
>
> A "fixup!" is applied by amending HEAD, so the melded commit has HEAD's
> parent as its parent and is empty when the index matches the tree of that
> parent, not of HEAD. do_pick_commit() only compares against HEAD, so it
> never notices that the meld cancelled the commit out and falls through to
> "git commit --amend", which refuses to create an empty commit.
>
> After melding a fixup or squash, check whether the amended commit is
> empty -- its index matches the tree of HEAD's parent -- and, if so, honor
> "--empty" just as for a commit that becomes empty when picked: keep it,
> drop it, or halt.
To honor --empty we need to know if the commit that is being fixed up
was originally empty or not, as we should only drop commits that become
empty. That means we cannot just check if the commit has become empty
after applying the fixup - we somehow need to remember whether the
original commit was empty as well.
Having thought about it a little more, there are a quite a few corner
cases which we need to think about. If there are conflicts when applying
the revert the user might run "git reset HEAD^" to drop the commit
themselves which makes our life easy because we don't need to do
anything special when they continue the rebase. However, they could run
"git checkout HEAD^ :/" to reset all the files in the worktree without
dropping the commit, in which case we need to update
commit_staged_changes() to drop HEAD if it wasn't originally empty.
If HEAD becomes empty in the middle of a sequence of fixups, for example
pick C
fixup revert-C
fixup D
we don't want to squash D into the previous commit, so I think we should
only drop commits that become empty after applying the all the fixups
targeting it. do_pick_commit() has a final_fixup function argument so
that should not be a problem.
If the original commit is empty then
pick empty
fixup commit-that-becomes-empty
or
pick empty
fixup empty-fixup
should not drop the fixed up commit. In the first example we should
continue to respect --empty=stop for the fixup becoming empty. The
latter only really makes sense with "fixup -C", or "fixup -c".
There isn't necessarily a pick command before a fixup for example
reset C
fixup revert-C
or
exec some command
fixup revert-HEAD
or
break
fixup revert-HEAD
are all possible if the user edits the todo list. For these three cases
one option is to say that because there is not a "pick" command before
the "fixup" command we don't drop the commit. I think that probably
makes it easier to determine if the original commit was empty because we
can record that when we see the "pick" command. That does feels a bit
inconsistent though. It is possible that a commit can become empty after
the user has reworded or edited it
reword C # or edit C
fixup revert-C
but it is a bit strange for the user to ask to edit a commit if they
really want to drop it, so maybe requiring a "pick" command in order for
the commit to be dropped is a good idea.
I think we can record whether a pick is empty at the beginning of
do_pick_commit() and store that in a new member of struct replay_ctx.
We'll need to save and restore that new member when we stop for the user
to resolve conflicts. The state reading is done in read_populate_opts().
To save it we'll need to create a file when we stop for conflicts.
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index f6c22d1598..7eb8bbe95f 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -282,6 +282,11 @@ by `git log --cherry-mark ...`) are detected and dropped as a
> preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
> passed).
> +
> +A commit can also become empty as a result of `--autosquash`, when a
> +`fixup!` or `squash!` commit cancels out all of the changes of the
> +commit it is melded into.
The rebase man page does not currently use "melded", it talks about
squashing commits together - we should probably make the new text
consistent with that.
> diff --git a/sequencer.c b/sequencer.c
> index 0fe8fed6c3..bc24132c7c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1817,6 +1817,39 @@ static int allow_empty(struct repository *r,
> return 0;
> }
>
> +/*
> + * Melding a "fixup!"/"squash!" amends HEAD, so the resulting commit is empty
> + * when the index matches the tree of HEAD's parent (rather than of HEAD, as a
> + * plain pick would). Returns 1 if the amended commit would be empty, 0 if not,
> + * and negative on error.
> + */
> +static int amended_commit_is_empty(struct repository *r)
> +{
> + struct object_id head_oid, *cache_tree_oid;
> + const struct object_id *parent_tree_oid;
> + struct commit *head_commit;
> +
> + if (repo_get_oid(r, "HEAD", &head_oid))
> + return error(_("could not resolve HEAD commit"));
> + head_commit = lookup_commit_reference(r, &head_oid);
You can simplify this slightly with
head = lookup_commit_reference_by_name(r, "HEAD");
> + if (!head_commit || repo_parse_commit(r, head_commit))
> + return -1;
> +
> + if (head_commit->parents) {
> + struct commit *parent = head_commit->parents->item;
> + if (repo_parse_commit(r, parent))
> + return -1;
> + parent_tree_oid = get_commit_tree_oid(parent);
> + } else {
> + parent_tree_oid = the_hash_algo->empty_tree;
> + }
> +
> + if (!(cache_tree_oid = get_cache_tree_oid(r->index)))
> + return -1;
> +
> + return oideq(cache_tree_oid, parent_tree_oid);
> +}
> +
> static struct {
> char c;
> const char *str;
> @@ -2260,10 +2293,34 @@ static const char *reflog_message(struct replay_opts *opts,
> [...]
> static int do_pick_commit(struct repository *r,
> struct todo_item *item,
> struct replay_opts *opts,
> - int final_fixup, int *check_todo)
> + int final_fixup, int *check_todo, int *dropped)
Rather than adding a new parameter, I think we should extend the return
enum added in pw/rebase-drop-notes-with-commit with a new member to
indicate that we dropped HEAD.
> @@ -2493,23 +2553,67 @@ static int do_pick_commit(struct repository *r,
> }
>
> drop_commit = 0;
> - allow = allow_empty(r, opts, commit);
> - if (allow < 0) {
> - res = allow;
> - goto leave;
> - } else if (allow == 1) {
> - flags |= ALLOW_EMPTY;
> - } else if (allow == 2) {
> - drop_commit = 1;
> - refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
> - NULL, REF_NO_DEREF);
> - unlink(git_path_merge_msg(r));
> - refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
> - NULL, REF_NO_DEREF);
> - fprintf(stderr,
> - _("dropping %s %s -- patch contents already upstream\n"),
> - oid_to_hex(&commit->object.oid), msg.subject);
> - } /* else allow == 0 and there's nothing special to do */
I don't think we want to delete this - we still want to tell the user if
a fixup became empty, but we want an additional check along the lines of
if (final_fixup) {
/*
* If the original commit was not empty and HEAD is now
* empty then drop HEAD.
*/
}
> @@ -4980,7 +5084,7 @@ static int pick_one_commit(struct repository *r,
> return error_with_patch(r, commit,
> arg, item->arg_len, opts, res, !res);
> }
> - if (is_rebase_i(opts) && !res)
> + if (is_rebase_i(opts) && !res && !dropped)
> record_in_rewritten(&item->commit->object.oid,
> peek_command(todo_list, 1));
Don't we need to clear the pending list of rewritten commits from the
original pick and any intermediate fixups, rather than just to skipping
recording the final fixup as rewritten? It is probably worth adding a
test to 5407 to check that (there is an example in
pw/rebase-drop-notes-with-commit).
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 5033411a43..d8085abf1d 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -461,13 +461,15 @@ test_expect_success 'abort last squash' '
> git commit --allow-empty -m second &&
> git commit --allow-empty --squash HEAD &&
>
> + : "squashing empty onto empty leaves an empty commit; --empty=keep" &&
> + : "keeps it so the squash still reaches the editor, which aborts" &&
> test_must_fail git -c core.editor="grep -q ^pick" \
> - rebase -ki --autosquash HEAD~4 &&
> + rebase -ki --autosquash --empty=keep HEAD~4 &&
Are we adding --empty=keep for clarity here? I wonder if the original
was deliberately testing the default.
> : do not finish the squash, but resolve it manually &&
> git commit --allow-empty --amend -m edited-first &&
> git rebase --skip &&
> git show >actual &&
> - ! grep first actual
> + test_grep ! first actual
> '
> +test_expect_success 'fixup! leaving an empty commit empty stops with --empty=stop' '
> + git reset --hard base &&
> + git commit --allow-empty -m placeholder &&
> + git commit --allow-empty -m "fixup! placeholder" &&
As both commits start off empty we shouldn't stop. --empty only applies
to commits that become empty when they are rebased. The same applies to
the next couple of tests.
Thanks
Phillip
> + test_when_finished "git rebase --abort" &&
> + test_must_fail git rebase -i --autosquash --empty=stop HEAD~2
> +'
> +
> +test_expect_success 'fixup! leaving an empty commit empty is dropped with --empty=drop' '
> + git reset --hard base &&
> + git commit --allow-empty -m placeholder &&
> + git commit --allow-empty -m "fixup! placeholder" &&
> +
> + git rebase -i --autosquash --empty=drop HEAD~2 &&
> +
> + git log --format=%s >actual &&
> + test_grep ! placeholder actual
> +'
> +
> +test_expect_success 'fixup! leaving an empty commit empty is kept with --empty=keep' '
> + git reset --hard base &&
> + git commit --allow-empty -m placeholder &&
> + git commit --allow-empty -m "fixup! placeholder" &&
> +
> + git rebase -i --autosquash --empty=keep HEAD~2 &&
> +
> + git log --format=%s >actual &&
> + test_grep placeholder actual &&
> + git diff --exit-code HEAD~1 HEAD
> +'
> +
> +test_expect_success 'a dropped emptied fixup is not recorded as rewritten' '
> + git reset --hard base &&
> + test_commit --no-tag preR fileR 1 &&
> + test_commit --no-tag changeR fileR 2 &&
> + R=$(git rev-parse HEAD) &&
> + echo 1 >fileR &&
> + git commit -m "fixup! changeR" fileR &&
> + F=$(git rev-parse HEAD) &&
> + test_commit --no-tag keepR fileK keep &&
> +
> + test_when_finished "rm -f .git/hooks/post-rewrite actual.rewrites" &&
> + write_script .git/hooks/post-rewrite <<-\EOF &&
> + cat >actual.rewrites
> + EOF
> +
> + git rebase -i --autosquash --empty=drop HEAD~4 &&
> +
> + : "changeR and its fixup were dropped, so must not be reported as" &&
> + : "rewritten, but the surviving keepR must be" &&
> + test_grep ! -e "$R" -e "$F" actual.rewrites &&
> + test_grep "$(git rev-parse HEAD)" actual.rewrites
> +'
> +
> test_done
>
>
>
>
^ permalink raw reply
* Re: [PATCH GSoC v17 00/13] cat-file: add remote-object-info to batch-command
From: Junio C Hamano @ 2026-07-15 15:23 UTC (permalink / raw)
To: Pablo Sabater
Cc: chandrapratap3519, chriscool, eric.peijian, git, jltobler,
karthik.188, peff, toon
In-Reply-To: <DJZ0JXIP13UO.DH1ONBDEVF3N@gmail.com>
"Pablo Sabater" <pabloosabaterr@gmail.com> writes:
>> Thanks. How close are we to the finish line, by the way?
>
> There's one month left. Final evaluation ends on 17th August (more weeks
> can be asked, if it seems too rushed, ...
That is the deadline to wrap up your work, which is not quite what
I was asking. I meant to ask how close you assess this topic is
to completion at iteration #18. Are all remaining issues just
minor nits? Are there still large gaps between the desired and
actual behavior of the new feature? That sort of thing.
Thanks.
^ permalink raw reply
* [PATCH v3 9/9] sequencer: do not record dropped commits as rewritten
From: Phillip Wood @ 2026-07-15 15:22 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Andrei Rybak, Phillip Wood
In-Reply-To: <cover.1784128921.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks, it means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.
While we do not want to record the dropped commit as rewritten, if
it is the final commit in a chain of fixups then we need to flush
the list of rewritten commits. The behavior of an "edit" command
where the commit is dropped is changed so that "rebase --continue"
will not amend the previous pick. However, as the code comment notes
it will still be erroneously recorded as rewritten when the rebase
continues. That will need to be addressed separately along with not
recording skipped commits as rewritten.
The initialization of "drop_commit" is moved to ensure it is initialized
when rewording a fast-forwarded commit.
Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 24 +++++++++++++++++++-----
t/t3400-rebase.sh | 12 ++++++++++++
t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 4b3092dc9bb..7a5898b215d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2264,6 +2264,7 @@ enum pick_result {
PICK_RESULT_ERROR = -1,
PICK_RESULT_OK,
PICK_RESULT_CONFLICTS,
+ PICK_RESULT_DROPPED,
};
static enum pick_result do_pick_commit(struct repository *r,
@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,
const char *base_label, *next_label, *reflog_action;
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
- int res, unborn = 0, reword = 0, allow, drop_commit;
+ int res, unborn = 0, reword = 0, allow, drop_commit = 0;
enum todo_command command = item->command;
struct commit *commit = item->commit;
@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,
goto leave;
}
- drop_commit = 0;
allow = allow_empty(r, opts, commit);
if (allow < 0) {
res = allow;
@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,
return PICK_RESULT_ERROR;
else if (res > 0)
return PICK_RESULT_CONFLICTS;
+ else if (drop_commit)
+ return PICK_RESULT_DROPPED;
else
return PICK_RESULT_OK;
}
@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
int res = pick_res == PICK_RESULT_CONFLICTS;
+ int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
+ pick_res != PICK_RESULT_DROPPED;
- if (pick_res == PICK_RESULT_OK) {
+ /*
+ * NEEDSWORK: Do not record the commit as rewritten when
+ * continuing if it was dropped. Does it even make sense
+ * to stop if the commit was dropped?
+ */
+ if (pick_res == PICK_RESULT_OK ||
+ pick_res == PICK_RESULT_DROPPED) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
- return error_with_patch(r, commit,
- arg, item->arg_len, opts, res, !res);
+ return error_with_patch(r, commit, arg, item->arg_len, opts,
+ res, to_amend);
} else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
+ return 0;
+ } else if (pick_res == PICK_RESULT_DROPPED) {
+ if (is_final_fixup(todo_list))
+ flush_rewritten_pending();
return 0;
} else if (pick_res == PICK_RESULT_CONFLICTS &&
is_fixup(item->command)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f0e7fcf649a..1d09886ea35 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '
git reset --hard n3 &&
git rebase --apply --onto n1 n2 &&
test "a note" = "$(git notes show HEAD)"
+'
+
+test_expect_success 'rebase drops notes of dropped commits' '
+ git checkout n1 &&
+ echo n3 >n3.t &&
+ echo n4 >n4.t &&
+ git add n3.t n4.t &&
+ git commit -m n34 &&
+ git rebase HEAD n3 &&
+ test_commit_message HEAD -m n2 &&
+ test_must_fail git notes list HEAD >actual &&
+ test_must_be_empty actual
'
test_expect_success 'rebase commit with an ancient timestamp' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f002..51991956d1d 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '
cat >expected.data <<-EOF &&
$(git rev-parse C) $(git rev-parse HEAD^)
$(git rev-parse D) $(git rev-parse HEAD)
+ EOF
+ verify_hook_input
+'
+
+test_expect_success 'rebase with commits that become empty' '
+ cat >todo <<-\EOF &&
+ pick H
+ pick E
+ fixup I
+ fixup H
+ pick G
+ pick I
+ EOF
+ (
+ set_replace_editor todo &&
+ git rebase -i --empty=drop A A
+ ) &&
+ echo rebase >expected.args &&
+ cat >expected.data <<-EOF &&
+ $(git rev-parse H) $(git rev-parse HEAD~2)
+ $(git rev-parse E) $(git rev-parse HEAD~1)
+ $(git rev-parse I) $(git rev-parse HEAD~1)
+ $(git rev-parse G) $(git rev-parse HEAD)
EOF
verify_hook_input
'
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v3 8/9] sequencer: use an enum to represent result of picking a commit
From: Phillip Wood @ 2026-07-15 15:22 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Andrei Rybak, Phillip Wood
In-Reply-To: <cover.1784128921.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Rather than using an integer where -1 is an error, 0 is success and 1
indicates there were conflicts, use an enum. This is clearer and lets
us add a separate return value for commits that are dropped because
they become empty in the next commit.
Note we continue to use "return error(...)" to return errors and
take advantage of C's lax typing of enums
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 16 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9016af9b5d7..4b3092dc9bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
-static int do_pick_commit(struct repository *r,
- struct todo_item *item,
- struct replay_opts *opts,
- int final_fixup, int *check_todo)
+enum pick_result {
+ PICK_RESULT_ERROR = -1,
+ PICK_RESULT_OK,
+ PICK_RESULT_CONFLICTS,
+};
+
+static enum pick_result do_pick_commit(struct repository *r,
+ struct todo_item *item,
+ struct replay_opts *opts,
+ int final_fixup, int *check_todo)
{
struct replay_ctx *ctx = opts->ctx;
unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,
free(author);
update_abort_safety_file();
- return res;
+ if (res < 0)
+ return PICK_RESULT_ERROR;
+ else if (res > 0)
+ return PICK_RESULT_CONFLICTS;
+ else
+ return PICK_RESULT_OK;
}
static int prepare_revs(struct replay_opts *opts)
@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,
struct replay_opts *opts,
int *check_todo, int* reschedule)
{
- int res;
+ enum pick_result pick_res;
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
- res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
- check_todo);
+ pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
+ check_todo);
if (!is_rebase_i(opts))
- return res;
+ switch (pick_res) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
- if (res < 0) {
+ if (pick_res == PICK_RESULT_ERROR) {
/* Reschedule */
*reschedule = 1;
return -1;
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
- if (!res) {
+ int res = pick_res == PICK_RESULT_CONFLICTS;
+
+ if (pick_res == PICK_RESULT_OK) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- } else if (!res) {
+ } else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
return 0;
- } else if (res && is_fixup(item->command)) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS &&
+ is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS) {
int to_amend = 0;
struct object_id oid;
@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,
to_amend = 1;
return error_with_patch(r, item->commit, arg, item->arg_len,
- opts, res, to_amend);
+ opts, 1, to_amend);
}
BUG("Unhandled return value from do_pick_commit()");
@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- return do_pick_commit(r, &item, opts, 0, &check_todo);
+ switch (do_pick_commit(r, &item, opts, 0, &check_todo)) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
+
}
int sequencer_pick_revisions(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH v3 7/9] sequencer: simplify pick_one_commit()
From: Phillip Wood @ 2026-07-15 15:22 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Junio C Hamano, Oswald Buddenhagen,
Farid Zakaria, Andrei Rybak, Phillip Wood
In-Reply-To: <cover.1784128921.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Unless we're rebasing, all we do in pick_one_commit() is call
do_pick_commit() and return its result. Simplify the code by returning
early if we're not rebasing so that we don't have to repeatedly call
is_rebase_i() in the rest of the function. Note that there are a couple
of conditions that do not call is_rebase_i() but they check for either
an "edit" or a "fixup" command, both of which imply we're rebasing.
The only block that does not return early is the one guarded by
"!res". Move the return into that block to make it clear that after
recording the commit as rewritten, all we do is return from the
function.
As the conditional blocks are all mutually exclusive (either the
conditions are mutually exclusive, or an earlier conditional block
that would match a later one contains a "return" statement) chain
them together with "else if" to make that clear.
While we could remove "res" from the conditions below "if (!res)"
they are left alone because, when we start using an enum in the next
commit, it makes it clear that these clauses are handling cases where
there are conflicts.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 8f3eed205e7..9016af9b5d7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,
res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
check_todo);
- if (is_rebase_i(opts) && res < 0) {
+ if (!is_rebase_i(opts))
+ return res;
+
+ if (res < 0) {
/* Reschedule */
*reschedule = 1;
return -1;
- }
- if (item->command == TODO_EDIT) {
+ } else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res) {
if (!opts->verbose)
@@ -4981,14 +4983,14 @@ static int pick_one_commit(struct repository *r,
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- }
- if (is_rebase_i(opts) && !res)
+ } else if (!res) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
- if (res && is_fixup(item->command)) {
+ return 0;
+ } else if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res && is_rebase_i(opts)) {
+ } else if (res) {
int to_amend = 0;
struct object_id oid;
@@ -5008,7 +5010,8 @@ static int pick_one_commit(struct repository *r,
return error_with_patch(r, item->commit, arg, item->arg_len,
opts, res, to_amend);
}
- return res;
+
+ BUG("Unhandled return value from do_pick_commit()");
}
static int pick_commits(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ 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