* [PATCH v10 3/9] environment: move editor_program into repo_config_values
From: Tian Yuchen @ 2026-07-12 11:17 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-1-cat@malon.dev>
The global variable 'editor_program' holds the path to the user's
preferred editor. Move 'editor_program' into
'struct repo_config_values' to continue the libification effort.
There have been discussions on whether external programs like
editors truly need to be configured on a per-repository basis within
the same process. While a single process might rarely invoke
different editors, this migration is necessary for two reasons:
1. Developers frequently use different toolchains for different
projects. Per-repo configuration respects this.
2. Moving this string into 'repo_config_values' eliminates mutable
global state. As the codebase moves toward becoming a long-running
processes, managing multiple repositories concurrently must
not overwrite each other's program configurations.
No standalone getter function is introduced. Callers directly access
the field via 'repo_config_values()'. Heap memory is safely reclaimed
in 'repo_config_values_clear()'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
editor.c | 4 ++--
environment.c | 7 ++++---
environment.h | 2 +-
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/editor.c b/editor.c
index fd174e6a03..0d1cb8768d 100644
--- a/editor.c
+++ b/editor.c
@@ -29,8 +29,8 @@ const char *git_editor(void)
const char *editor = getenv("GIT_EDITOR");
int terminal_is_dumb = is_terminal_dumb();
- if (!editor && editor_program)
- editor = editor_program;
+ if (!editor)
+ editor = repo_config_values(the_repository)->editor_program;
if (!editor && !terminal_is_dumb)
editor = getenv("VISUAL");
if (!editor)
diff --git a/environment.c b/environment.c
index 275931c213..a65d575af4 100644
--- a/environment.c
+++ b/environment.c
@@ -55,7 +55,6 @@ int fsync_object_files = -1;
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
-char *editor_program;
char *askpass_program;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
@@ -437,8 +436,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.editor")) {
- FREE_AND_NULL(editor_program);
- return git_config_string(&editor_program, var, value);
+ FREE_AND_NULL(cfg->editor_program);
+ return git_config_string(&cfg->editor_program, var, value);
}
if (!strcmp(var, "core.commentchar") ||
@@ -725,6 +724,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
cfg->excludes_file = NULL;
+ cfg->editor_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -741,4 +741,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
{
FREE_AND_NULL(cfg->attributes_file);
FREE_AND_NULL(cfg->excludes_file);
+ FREE_AND_NULL(cfg->editor_program);
}
diff --git a/environment.h b/environment.h
index 4776ccc657..8178ebab76 100644
--- a/environment.h
+++ b/environment.h
@@ -91,6 +91,7 @@ struct repo_config_values {
/* section "core" config values */
char *attributes_file;
char *excludes_file;
+ char *editor_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -218,7 +219,6 @@ const char *get_commit_output_encoding(void);
extern char *git_commit_encoding;
extern char *git_log_output_encoding;
-extern char *editor_program;
extern char *askpass_program;
/*
--
2.43.0
^ permalink raw reply related
* [PATCH v10 1/9] repository: introduce repo_config_values_clear()
From: Tian Yuchen @ 2026-07-12 11:17 UTC (permalink / raw)
To: git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen,
Christian Couder, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-1-cat@malon.dev>
As part of the ongoing libification effort, dynamically allocated
global configuration variables are being moved into
'struct repo_config_values'. To prevent memory leaks, we need a
destructor to free these heap-allocated variables when a repository
instance is torn down.
Introduce 'repo_config_values_clear()' in environment.c and invoke it
from 'repo_clear()' in repository.c. As a starting point, update this
new function to handle the cleanup of 'attributes_file'.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
environment.c | 5 +++++
environment.h | 9 +++++++++
repository.c | 1 +
3 files changed, 15 insertions(+)
diff --git a/environment.c b/environment.c
index ba2c60103f..ae05f16d04 100644
--- a/environment.c
+++ b/environment.c
@@ -726,3 +726,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->sparse_expect_files_outside_of_patterns = 0;
cfg->warn_on_object_refname_ambiguity = 1;
}
+
+void repo_config_values_clear(struct repo_config_values *cfg)
+{
+ FREE_AND_NULL(cfg->attributes_file);
+}
diff --git a/environment.h b/environment.h
index 6f18286955..9169d7f62d 100644
--- a/environment.h
+++ b/environment.h
@@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg);
+/*
+ * Frees memory allocated for dynamically loaded configuration values
+ * inside `repo_config_values`.
+ *
+ * As dynamically allocated variables are migrated into this struct,
+ * their FREE_AND_NULL() calls should be appended here.
+ */
+void repo_config_values_clear(struct repo_config_values *cfg);
+
/*
* TODO: All the below state either explicitly or implicitly relies on
* `the_repository`. We should eventually get rid of these and make the
diff --git a/repository.c b/repository.c
index 187dd471c4..669e2d1200 100644
--- a/repository.c
+++ b/repository.c
@@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
FREE_AND_NULL(repo->parsed_objects);
repo_settings_clear(repo);
+ repo_config_values_clear(&repo->config_values_private_);
if (repo->config) {
git_configset_clear(repo->config);
--
2.43.0
^ permalink raw reply related
* [PATCH v10 0/9] migrate more variables into repo_config_values
From: Tian Yuchen @ 2026-07-12 11:17 UTC (permalink / raw)
To: git; +Cc: pabloosabaterr, cirnovskyv, szeder.dev, Tian Yuchen
In-Reply-To: <20260709161145.13349-1-cat@malon.dev>
Hi everyone,
This patch series continues the ongoing libification effort by migrating
a batch of global configuration variables into struct repo_config_values.
What does this series do:
infrastructure & strings (commits 1-6):
Introduce 'repo_config_values_clear()' to manage the lifecycle
of heap-allocated configuration strings. This infrastructure is utilized
to migrate string variables, including 'excludes_file', 'apply' whitespace
configs, and external programs including 'editor', 'pager', 'askpass'.
enums (commits 7-9):
Migrate enumerations 'push_default', 'autorebase', and
'object_creation_mode'. Care was taken to make these types available
to the configuration structure without triggering circular header
dependencies.
RFC:
Commit 3~5. Is it really necessary to migrate _program variables?
https://lore.kernel.org/git/8e657184-ee0b-453a-9f2d-a98080d3582e@gmail.com/
Commit 6~9. Previous related discussions on 'git_branch_track'.
https://lore.kernel.org/git/CAD=f0L-mPX+KECUjXk-WBzEbTP7wCa8sB56GySQT0yh9mfUOWw@mail.gmail.com/
Note:
Since a new getter 'repo_excludes_file()' is introduced, as previously
promised, once it is finally merged into 'master', there will be a patch to
update and squash the comments.
Similarly, I've noticed that the classification and sorting of variables in
'repo_config_values' don't seem to be correct. There will also be a patch
to fix this, and I think it will form a commit series along with the comment
patch?
Changes since v9:
- Fixed a few typos.
- Drop an unnecessary branching in editor.c
- Use repo_config_values *cfg to avoid multiple calls to
repo_config_values() in repo_excludes_file().
- Let repo_config_values_clear() receive 'struct repo_config_values' and
do not check (repo != the_repository) anymore. In repo_clear(), pass in
config_values_private_ instead so that we don't need to filter calls with
uninitialized repos. Therefore, drop the NEEDSWORK comment.
Special thanks to Pablo!
Tian Yuchen (9):
repository: introduce repo_config_values_clear()
environment: move excludes_file into repo_config_values
environment: move editor_program into repo_config_values
environment: move pager_program into repo_config_values
environment: move askpass_program into repo_config_values
environment: migrate apply_default_whitespace and
apply_default_ignorewhitespace
environment: move push_default into repo_config_values
environment: move autorebase into repo_config_values
environment: move object_creation_mode into repo_config_values
apply.c | 20 ++++++++------
branch.c | 2 +-
builtin/push.c | 8 +++---
dir.c | 4 +--
editor.c | 4 +--
environment.c | 75 +++++++++++++++++++++++++++++++++-----------------
environment.h | 75 +++++++++++++++++++++++++++++++-------------------
object-file.c | 2 +-
pager.c | 26 +++++++++++------
prompt.c | 3 +-
remote.c | 2 +-
repository.c | 1 +
12 files changed, 140 insertions(+), 82 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v9 1/9] repository: introduce repo_config_values_clear()
From: Tian Yuchen @ 2026-07-12 10:44 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <DJWHIWVJ52UW.24D8DAKBNDMLB@gmail.com>
Hey Pablo,
On 7/12/26 17:28, Pablo Sabater wrote:
> On Sat Jul 11, 2026 at 8:35 PM CEST, Tian Yuchen wrote:
>> Hii Pablo,
>>
>> On 7/12/26 01:21, Pablo Sabater wrote:
>>> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>>>> As part of the ongoing libification effort, dynamically allocated
>>>> global configuration variables are being moved into
>>>> 'struct repo_config_values'. To prevent memory leaks, we need a
>>>> destructor to free these heap-allocated variables when a repository
>>>> instance is torn down.
>>>>
>>>> Introduce 'repo_config_values_clear()' in environment.c and invoke it
>>>> from 'repo_clear()' in repository.c. As a starting point, update this
>>>> new function to handle the cleanup of 'attributes_file'.
>>>
>>> Makes sense.
>>>
>>>>
>>>> Note:
>>>>
>>>> Submodules are currently not supported by repo_config_values(), which
>>>> explicitly BUG()s out if 'repo != the_repository'. Since repo_clear()
>>>> cleans up all repository instances, we must bypass them to prevent
>>>> crashing.
>>>>
>>>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>>>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>>>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>>>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>>>> ---
>>>> environment.c | 19 +++++++++++++++++++
>>>> environment.h | 9 +++++++++
>>>> repository.c | 1 +
>>>> 3 files changed, 29 insertions(+)
>>>>
>>>> diff --git a/environment.c b/environment.c
>>>> index ba2c60103f..13677484de 100644
>>>> --- a/environment.c
>>>> +++ b/environment.c
>>>> @@ -726,3 +726,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
>>>> cfg->sparse_expect_files_outside_of_patterns = 0;
>>>> cfg->warn_on_object_refname_ambiguity = 1;
>>>> }
>>>> +
>>>> +void repo_config_values_clear(struct repository *repo)
>>>> +{
>>>> + struct repo_config_values *cfg;
>>>> +
>>>> + /*
>>>> + * NEEDSWORK: Submodules are currently not supported by
>>>> + * repo_config_values(), which explicitly BUG()s out if
>>>> + * repo != the_repository. Since repo_clear() cleans up all
>>>> + * repository instances, we must bypass them here to prevent
>>>> + * crashing.
>>>> + */
>>>> + if (repo != the_repository)
>>>> + return;
>>>> +
>>>> + cfg = repo_config_values(repo);
>>>> +
>>>> + FREE_AND_NULL(cfg->attributes_file);
>>>> +}
>>>> diff --git a/environment.h b/environment.h
>>>> index 6f18286955..c4a6a45704 100644
>>>> --- a/environment.h
>>>> +++ b/environment.h
>>>> @@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
>>>>
>>>> void repo_config_values_init(struct repo_config_values *cfg);
>>>>
>>>> +/*
>>>> + * Frees memory allocated for dynamically loaded configuration values
>>>> + * inside `repo_config_values`.
>>>> + *
>>>> + * As dynamically allocated variables are migrated into this struct,
>>>> + * their FREE_AND_NULL() calls should be appended here.
>>>> + */
>>>> +void repo_config_values_clear(struct repository *repo);
>>>
>>> I think that I'm not comfortable having the _init() and the _clear()
>>> functions with different signatures.
>>>
>>> _clear() takes struct repository to dodge a BUG().
>>>
>>> I would like to have both signatures equal, why can't we just do directly:
>>>
>>> void repo_config_values_clear(struct repo_config_values *cfg)
>>> {
>>> FREE_AND_NULL(cfg->attributes_file);
>>> }
>>>
>>> and call from repo_clear():
>>>
>>> repo_config_values_clear(&repo->config_values_private_)
>>>
>>
>> I particularly agree with your point that the signatures of these two
>> functions should be consistent. I missed it tbh...I will change it in
>> the next reroll.
>>
>> However, I think it makes more sense to refactor to pass in 'struct
>> repository', which is consistent with repo-settings.
>
> Whatever the signature is as long as it's symmetrical, is fine by me. However
> having 'struct repository' on _init() just passes the problem that
> _clear() had.
>
> Let's look at initialize_repository():
>
> void initialize_repository(struct repository *repo)
> {
> if (repo->initialized)
> BUG("repository initialized already");
> repo->initialized = true;
> [snip]
> repo_config_values_init(&repo->config_values_private_);
> [snip]
>
> if we change the _init() signature to receive 'struct repository', how do
> we access our _private_ field? We cannot call repo_config_values() on
> repo_config_values_init() because initialize_repository() is also called for
> submodules (repo_init() -> initialize_repository()) so we would BUG() out.
> We would have to access the _private_ field directly at _init().
>
> With _clear() accessing directly the _private_ field we can forget about
> the NEEDSWORK, it works for every repository.
>
>>
>>> I get that the workaround might be to not access directly to
>>> &repo->config_values_private_ which repo_config_values() returns but for
>>> example initialize_repository() access this _private_ field directly as
>>> well.
>>
>> Now that we have used the _private_ suffix, if we can just define a
>> _clear() to bypass the assertion of repo_config_values(), wouldn't this
>> be self-deception? I'm not saying that the original lines are
>> necessarily correct... but I do think that semantically speaking, it is
>> inappropriate to pass in config_values_private_ to _clear().
>
> I don't think it would be "self-deception": _init() and _clear() are the
> start and end of this _private_ field's lifecycle so having them receive
> 'struct repo_config_values' directly sounds reasonable to me.
>
>>
>>> Also at repo_init(), initialize_repository() sets repo->initialized
>>> before anything can fail and call repo_clear() but
>>> repo_config_values_clear() should be able to free attributes_file even
>>> just after a memset() (which happens before initialize_repository()).
>>> But calling repo_config_values has a BUG() in case of
>>> !repo->initialized are we comfortable with this assert?
>>
>> This goes back to the previous topic: Who is responsible for the call to
>> _clear()? Who is responsible for filtering all those invalid usage of
>> repo instances? Faced with a repo instance that was not initialized but
>> was handed over to _clear() in some way, we have two concepts:
>>
>> - It doesn't matter. Since we always handle config_values_private_, it's
>> NULL at this point, so we don't BUG() and continue.
>>
>> - The very existence of such a repo is a mistake. It shouldn't have
>> appeared and shouldn't have been passed on to me. However, since this
>> situation is relatively common at this point, we choose to return
>> instead of BUG()ing it directly to temporarily avoid it. We will
>> gradually tighten the conditions. When the invalid calls are eliminated
>> in the end, such checks will no longer exist.
>>
>> Our consensus should at least be that this 'use of uninitialized repos'
>> is bad, so they are just two different ways to solve unexpected
>> situations. However, in my opinion, the difference between these two
>> concepts lies in whether we are consciously moving the assertion
>> downward. I think the latter line of thinking does this better.
>
> Sounds reasonable, but I think I would go with the first thought of "It
> doesn't matter...".
>
> Having _clear() access directly removes the need for tightening the
> conditions, because there would be none. The _clear() works for any repo
> as long as it has gone through a memset().
>
> I think that the ones that should ensure that we don't have a "bad
> repository" should be the rest of the code and leave _clear() to just
> clear.
>
>>
>>
>> >
>> > Even with the NEEDSWORK it is a silent return, what will happen when
>> > submodules are supported? If no one remembers to change it we will leak
>> > the submodules silently.
>> >
>>
>> I will remember to change it ;)
>>
>> Regards, yuchen
>>
>>>> +
>>>> /*
>>>> * TODO: All the below state either explicitly or implicitly relies on
>>>> * `the_repository`. We should eventually get rid of these and make the
>>>> diff --git a/repository.c b/repository.c
>>>> index 187dd471c4..b31f1b7852 100644
>>>> --- a/repository.c
>>>> +++ b/repository.c
>>>> @@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
>>>> FREE_AND_NULL(repo->parsed_objects);
>>>>
>>>> repo_settings_clear(repo);
>>>> + repo_config_values_clear(repo);
>>>>
>>>> if (repo->config) {
>>>> git_configset_clear(repo->config);
>>>
>>> Regards,
>>> Pablo
>
> Regards,
> Pablo
I thought about it again, and okay, I have to admit your approach is
indeed more reasonable. Maybe it was just my stubbornness stemming from
staying up late thinking last night. :D
Thanks! yuchen
^ permalink raw reply
* Re: [PATCH] Makefile: fix up lib directory move
From: Johannes Schindelin @ 2026-07-12 10:15 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Patrick Steinhardt, GIT Mailing-list, Junio C Hamano
In-Reply-To: <0c94331b-7eb1-4116-afa5-811082ad5854@ramsayjones.plus.com>
Hi Ramsay,
On Fri, 10 Jul 2026, Ramsay Jones wrote:
> Commit 9759608622 ("Move libgit.a sources into separate "lib/" directory",
It's not your fault, but this commit is no longer reachable from any
official branch.
Maybe a more stable way to refer to this right now would be to name the
topic: `ps/libgit-in-subdir`.
> 2026-06-22) moved some files into a lib directory, but forgot to update
> a sparse dependency in the Makefile, resulting in a sparse error:
>
> SP lib/pack-revindex.c
> lib/pack-revindex.c:78:17: error: memset with byte count of 262144
> make: *** [Makefile:3446: lib/pack-revindex.sp] Error 1
>
> Add the missing 'lib/' prefix to the pack-revindex.sp path.
That reasoning and that patch make sense to me. Thank you!
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Patrick,
>
> If you need to re-roll your 'ps/libgit-in-subdir' branch, could you please squash
> this into the relevant patch. (This patch was created directly on top of the 'seen'
> branch, rather than on top of your branch).
That would be 8da3a2c01822 (Move libgit.a sources into separate "lib/"
directory, 2026-07-01), at least at the time of writing (that commit is
still not merged into `next` and hence subject to be rewritten).
Ciao,
Johannes
>
> Thanks
>
> ATB,
> Ramsay Jones
>
>
> Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 703772ba4f..a36d2c1942 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2974,7 +2974,7 @@ lib/gettext.sp lib/gettext.s lib/gettext.o: EXTRA_CPPFLAGS = \
> http-push.sp lib/http.sp lib/http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \
> -DCURL_DISABLE_TYPECHECK
>
> -pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
> +lib/pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
>
> ifdef NO_EXPAT
> lib/http-walker.sp lib/http-walker.s lib/http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
> --
> 2.55.0
>
>
^ permalink raw reply
* Re: [PATCH v9 2/9] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-07-12 10:08 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <DJVY828NHN8O.22CRAQOF73S6D@gmail.com>
On 7/12/26 02:21, Pablo Sabater wrote:
> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>> The global variable 'excludes_file' is used to track the path to the
>> global ignore file. If this variable is NULL,
>> 'setup_standard_excludes()'
>
> Nit: Strange line break here.
>
>> in 'dir.c' forcefully evaluates and assigns the XDG default path to it.
>>
>> Continue the libification effort by encapsulating this lazy-loading
>> fallback logic into a proper getter and moving the variable into
>> 'struct repo_config_values'.
>>
>> Since 'excludes_file' is a dynamically allocated string, it requires
>> proper heap memory management. It is safely freed using the newly
>> introduced `repo_config_values_clear()` function when the repository
>> is torn down.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>> ---
>> dir.c | 4 ++--
>> environment.c | 15 ++++++++++++---
>> environment.h | 4 +++-
>> 3 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 7a73690fbc..4f87a52b3c 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -3481,11 +3481,11 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
>>
>> void setup_standard_excludes(struct dir_struct *dir)
>> {
>> + const char *excludes_file = repo_excludes_file(the_repository);
>> +
>> dir->exclude_per_dir = ".gitignore";
>>
>> /* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
>> - if (!excludes_file)
>> - excludes_file = xdg_config_home("ignore");
>> if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
>> add_patterns_from_file_1(dir, excludes_file,
>> dir->untracked ? &dir->internal.ss_excludes_file : NULL);
>> diff --git a/environment.c b/environment.c
>> index 13677484de..5950592d63 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -57,7 +57,6 @@ enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
>> enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
>> char *editor_program;
>> char *askpass_program;
>> -char *excludes_file;
>> enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
>> enum eol core_eol = EOL_UNSET;
>> int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
>> @@ -134,6 +133,14 @@ int is_bare_repository(void)
>> return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
>> }
>>
>> +const char *repo_excludes_file(struct repository *repo)
>> +{
>> + if (!repo_config_values(repo)->excludes_file)
>> + repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
>> +
>> + return repo_config_values(repo)->excludes_file;
>> +}
>
> repo_config_values() returns a pointer so there should be no need to
> call the function 3 times.
>
> We could have the function be called once and use it then:
>
> const char *repo_excludes_file(struct repository *repo)
> {
> struct repo_config_values *cfg = repo_config_values(repo);
>
> if (!cfg->excludes_file)
> cfg->excludes_file = xdg_config_home("ignore");
>
> return cfg->excludes_file;
> }
>
Makes sense, thanks.
>> +
>> int have_git_dir(void)
>> {
>> return startup_info->have_repository
>> @@ -461,8 +468,8 @@ int git_default_core_config(const char *var, const char *value,
>> }
>>
>> if (!strcmp(var, "core.excludesfile")) {
>> - FREE_AND_NULL(excludes_file);
>> - return git_config_pathname(&excludes_file, var, value);
>> + FREE_AND_NULL(cfg->excludes_file);
>> + return git_config_pathname(&cfg->excludes_file, var, value);
>> }
>>
>> if (!strcmp(var, "core.whitespace")) {
>> @@ -715,6 +722,7 @@ int git_default_config(const char *var, const char *value,
>> void repo_config_values_init(struct repo_config_values *cfg)
>> {
>> cfg->attributes_file = NULL;
>> + cfg->excludes_file = NULL;
>> cfg->apply_sparse_checkout = 0;
>> cfg->branch_track = BRANCH_TRACK_REMOTE;
>> cfg->trust_ctime = 1;
>> @@ -744,4 +752,5 @@ void repo_config_values_clear(struct repository *repo)
>> cfg = repo_config_values(repo);
>>
>> FREE_AND_NULL(cfg->attributes_file);
>> + FREE_AND_NULL(cfg->excludes_file);
>> }
>> diff --git a/environment.h b/environment.h
>> index c4a6a45704..2e8352de7f 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -90,6 +90,7 @@ struct repository;
>> struct repo_config_values {
>> /* section "core" config values */
>> char *attributes_file;
>> + char *excludes_file;
>> int apply_sparse_checkout;
>> int trust_ctime;
>> int check_stat;
>> @@ -133,6 +134,8 @@ int git_default_config(const char *, const char *,
>> int git_default_core_config(const char *var, const char *value,
>> const struct config_context *ctx, void *cb);
>>
>> +const char *repo_excludes_file(struct repository *repo);
>> +
>> void repo_config_values_init(struct repo_config_values *cfg);
>>
>> /*
>> @@ -217,7 +220,6 @@ extern char *git_log_output_encoding;
>>
>> extern char *editor_program;
>> extern char *askpass_program;
>> -extern char *excludes_file;
>>
>> /*
>> * The character that begins a commented line in user-editable file
>
> The rest looks fine.
>
> Regards,
> Pablo
Regards, yuchen
^ permalink raw reply
* Re: [PATCH v9 0/9] migrate more variables into repo_config_values
From: Pablo Sabater @ 2026-07-12 9:34 UTC (permalink / raw)
To: Junio C Hamano, Pablo Sabater; +Cc: Tian Yuchen, git, cirnovskyv, szeder.dev
In-Reply-To: <xmqqo6gd9qyr.fsf@gitster.g>
On Sat Jul 11, 2026 at 11:06 PM CEST, Junio C Hamano wrote:
> "Pablo Sabater" <pabloosabaterr@gmail.com> writes:
>
>> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>>> ...
>> Hi!
>>
>> I missed a base-commit to easily apply this locally, could we
>> add one?
>>
>> Thanks!
>> Pablo
>
> FYI, a topic that is in 'seen' can be extracted from my tree by
> inspecting "git log --oneline origin/master..origin/seen" and
> finding the commit that merges the series.
I hadn't thought of that.
Thanks, -Pablo
^ permalink raw reply
* Re: [PATCH v9 1/9] repository: introduce repo_config_values_clear()
From: Pablo Sabater @ 2026-07-12 9:28 UTC (permalink / raw)
To: Tian Yuchen, Pablo Sabater, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <95f46463-a6e7-4b35-8ff4-ac89cadd6437@malon.dev>
On Sat Jul 11, 2026 at 8:35 PM CEST, Tian Yuchen wrote:
> Hii Pablo,
>
> On 7/12/26 01:21, Pablo Sabater wrote:
>> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>>> As part of the ongoing libification effort, dynamically allocated
>>> global configuration variables are being moved into
>>> 'struct repo_config_values'. To prevent memory leaks, we need a
>>> destructor to free these heap-allocated variables when a repository
>>> instance is torn down.
>>>
>>> Introduce 'repo_config_values_clear()' in environment.c and invoke it
>>> from 'repo_clear()' in repository.c. As a starting point, update this
>>> new function to handle the cleanup of 'attributes_file'.
>>
>> Makes sense.
>>
>>>
>>> Note:
>>>
>>> Submodules are currently not supported by repo_config_values(), which
>>> explicitly BUG()s out if 'repo != the_repository'. Since repo_clear()
>>> cleans up all repository instances, we must bypass them to prevent
>>> crashing.
>>>
>>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>>> ---
>>> environment.c | 19 +++++++++++++++++++
>>> environment.h | 9 +++++++++
>>> repository.c | 1 +
>>> 3 files changed, 29 insertions(+)
>>>
>>> diff --git a/environment.c b/environment.c
>>> index ba2c60103f..13677484de 100644
>>> --- a/environment.c
>>> +++ b/environment.c
>>> @@ -726,3 +726,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
>>> cfg->sparse_expect_files_outside_of_patterns = 0;
>>> cfg->warn_on_object_refname_ambiguity = 1;
>>> }
>>> +
>>> +void repo_config_values_clear(struct repository *repo)
>>> +{
>>> + struct repo_config_values *cfg;
>>> +
>>> + /*
>>> + * NEEDSWORK: Submodules are currently not supported by
>>> + * repo_config_values(), which explicitly BUG()s out if
>>> + * repo != the_repository. Since repo_clear() cleans up all
>>> + * repository instances, we must bypass them here to prevent
>>> + * crashing.
>>> + */
>>> + if (repo != the_repository)
>>> + return;
>>> +
>>> + cfg = repo_config_values(repo);
>>> +
>>> + FREE_AND_NULL(cfg->attributes_file);
>>> +}
>>> diff --git a/environment.h b/environment.h
>>> index 6f18286955..c4a6a45704 100644
>>> --- a/environment.h
>>> +++ b/environment.h
>>> @@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
>>>
>>> void repo_config_values_init(struct repo_config_values *cfg);
>>>
>>> +/*
>>> + * Frees memory allocated for dynamically loaded configuration values
>>> + * inside `repo_config_values`.
>>> + *
>>> + * As dynamically allocated variables are migrated into this struct,
>>> + * their FREE_AND_NULL() calls should be appended here.
>>> + */
>>> +void repo_config_values_clear(struct repository *repo);
>>
>> I think that I'm not comfortable having the _init() and the _clear()
>> functions with different signatures.
>>
>> _clear() takes struct repository to dodge a BUG().
>>
>> I would like to have both signatures equal, why can't we just do directly:
>>
>> void repo_config_values_clear(struct repo_config_values *cfg)
>> {
>> FREE_AND_NULL(cfg->attributes_file);
>> }
>>
>> and call from repo_clear():
>>
>> repo_config_values_clear(&repo->config_values_private_)
>>
>
> I particularly agree with your point that the signatures of these two
> functions should be consistent. I missed it tbh...I will change it in
> the next reroll.
>
> However, I think it makes more sense to refactor to pass in 'struct
> repository', which is consistent with repo-settings.
Whatever the signature is as long as it's symmetrical, is fine by me. However
having 'struct repository' on _init() just passes the problem that
_clear() had.
Let's look at initialize_repository():
void initialize_repository(struct repository *repo)
{
if (repo->initialized)
BUG("repository initialized already");
repo->initialized = true;
[snip]
repo_config_values_init(&repo->config_values_private_);
[snip]
if we change the _init() signature to receive 'struct repository', how do
we access our _private_ field? We cannot call repo_config_values() on
repo_config_values_init() because initialize_repository() is also called for
submodules (repo_init() -> initialize_repository()) so we would BUG() out.
We would have to access the _private_ field directly at _init().
With _clear() accessing directly the _private_ field we can forget about
the NEEDSWORK, it works for every repository.
>
>> I get that the workaround might be to not access directly to
>> &repo->config_values_private_ which repo_config_values() returns but for
>> example initialize_repository() access this _private_ field directly as
>> well.
>
> Now that we have used the _private_ suffix, if we can just define a
> _clear() to bypass the assertion of repo_config_values(), wouldn't this
> be self-deception? I'm not saying that the original lines are
> necessarily correct... but I do think that semantically speaking, it is
> inappropriate to pass in config_values_private_ to _clear().
I don't think it would be "self-deception": _init() and _clear() are the
start and end of this _private_ field's lifecycle so having them receive
'struct repo_config_values' directly sounds reasonable to me.
>
>> Also at repo_init(), initialize_repository() sets repo->initialized
>> before anything can fail and call repo_clear() but
>> repo_config_values_clear() should be able to free attributes_file even
>> just after a memset() (which happens before initialize_repository()).
>> But calling repo_config_values has a BUG() in case of
>> !repo->initialized are we comfortable with this assert?
>
> This goes back to the previous topic: Who is responsible for the call to
> _clear()? Who is responsible for filtering all those invalid usage of
> repo instances? Faced with a repo instance that was not initialized but
> was handed over to _clear() in some way, we have two concepts:
>
> - It doesn't matter. Since we always handle config_values_private_, it's
> NULL at this point, so we don't BUG() and continue.
>
> - The very existence of such a repo is a mistake. It shouldn't have
> appeared and shouldn't have been passed on to me. However, since this
> situation is relatively common at this point, we choose to return
> instead of BUG()ing it directly to temporarily avoid it. We will
> gradually tighten the conditions. When the invalid calls are eliminated
> in the end, such checks will no longer exist.
>
> Our consensus should at least be that this 'use of uninitialized repos'
> is bad, so they are just two different ways to solve unexpected
> situations. However, in my opinion, the difference between these two
> concepts lies in whether we are consciously moving the assertion
> downward. I think the latter line of thinking does this better.
Sounds reasonable, but I think I would go with the first thought of "It
doesn't matter...".
Having _clear() access directly removes the need for tightening the
conditions, because there would be none. The _clear() works for any repo
as long as it has gone through a memset().
I think that the ones that should ensure that we don't have a "bad
repository" should be the rest of the code and leave _clear() to just
clear.
>
>
> >
> > Even with the NEEDSWORK it is a silent return, what will happen when
> > submodules are supported? If no one remembers to change it we will leak
> > the submodules silently.
> >
>
> I will remember to change it ;)
>
> Regards, yuchen
>
>>> +
>>> /*
>>> * TODO: All the below state either explicitly or implicitly relies on
>>> * `the_repository`. We should eventually get rid of these and make the
>>> diff --git a/repository.c b/repository.c
>>> index 187dd471c4..b31f1b7852 100644
>>> --- a/repository.c
>>> +++ b/repository.c
>>> @@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
>>> FREE_AND_NULL(repo->parsed_objects);
>>>
>>> repo_settings_clear(repo);
>>> + repo_config_values_clear(repo);
>>>
>>> if (repo->config) {
>>> git_configset_clear(repo->config);
>>
>> Regards,
>> Pablo
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH
From: Christian Couder @ 2026-07-12 9:06 UTC (permalink / raw)
To: brian m. carlson, Christian Couder, git, Junio C Hamano,
Patrick Steinhardt, Karthik Nayak, Jeff King, Elijah Newren
In-Reply-To: <alFM-4FJQfaEjyju@fruit.crustytoothpaste.net>
On Fri, Jul 10, 2026 at 9:50 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2026-07-10 at 08:51:34, Christian Couder wrote:
> > Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> > 2024-04-16), lazy fetching has been controlled by the
> > `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> > nothing" boolean that is set to 'true' by default when calling `git
> > upload-pack` for security reasons.
> >
> > Recently the "promisor-remote" capability was added to protocol v2,
> > allowing servers and clients to agree on the promisor remotes they
> > can safely use.
> >
> > This series leverages that capability to implement a pragmatic middle
> > ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> > fetching is allowed only when fetching from promisor remotes that are
> > both advertised by the server and accepted by the client.
> >
> > Note that using an environment variable for this is probably not the
> > best from a usability perspective. An `upload-pack.allowLazyFetch`
> > configuration variable would likely be better.
> >
> > Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> > things currently work. It would be a much bigger and more invasive
> > change to implement `upload-pack.allowLazyFetch` in a way that is
> > compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> > backward compatibility. Therefore, transitioning to a configuration
> > variable is left for future work.
>
> I don't think this is a good idea. We get a lot of reports on the
> security list involving various tooling that isn't within the scope of
> our threat model. This substantially increases the amount of code which
> is now subject to that threat model and therefore our security
> guarantees and I don't think we should do that as it stands, very
> especially while so much of our network-facing code is written in C.
This small series doesn't change any defaults, especially
GIT_NO_LAZY_FETCH is still set to 1 when calling `git upload-pack` by
default. And the new option is more restrictive than the
GIT_NO_LAZY_FETCH=0 option which already exists.
So I don't think it's fair to say that this _substantially increases_
the amount of code subject to some threat model.
I agree that client acceptance of some promisor remotes doesn't make
the served repo trusted. It's a real concern, but I think it's
addressable by different mechanisms. See below.
> The fetch code by default reads lots of configuration information from
> the repository, including remote settings and information and we really
> want absolutely none of that code running in the context of an untrusted
> repository.
When a promisor remote has been accepted, it means both the client and
the server trust it, so at least the promisor remote is not untrusted.
Now the main security issue on the server side is making sure the
served repo itself is also trusted. And I agree that the operator of
the server should decide and mark that trust, not the client.
I also agree that on GitLab/GitHub-style multi-tenant hosts most
repositories shouldn't be marked as trusted.
However note that:
- The operator of the server is the only actor which can set
GIT_NO_LAZY_FETCH on the server (where it matters).
- In the case of corporate/self-hosted repos, the operator also
controls the repos.
- Different features could be developed (in future work) to improve on
the current state:
- a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
- an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
- operator-defined allow/deny rules, or maybe
- some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
I would be happy to hear opinions about those potential features or
any other ways to address the issue.
So I agree that this series doesn't fix all the problems on the server
side, but I think it's still valuable to be able to restrict lazy
fetching to accepted promisor remotes.
Also I definitely agree that the current series should have better
documentation about this, and I plan to improve on that in the v2 of
this series.
Thanks for your insightful comments.
^ permalink raw reply
* Re: Understanding why Git defaults to show author date and not committer date
From: Oswald Buddenhagen @ 2026-07-12 8:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Omri Sarig, git
In-Reply-To: <xmqq4ii5b639.fsf@gitster.g>
On Sat, Jul 11, 2026 at 01:54:02PM -0700, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>> In a workflow based on mailing patches, the committer date is usually
>> much less interesting. It is "when the maintainer happened to pick up
>> your patch", as opposed to when you wrote it.
>
>True. In mailing list workflow, the author date recorded is usually
>the date that the patch was sent to the mailing list, which may be
>later than when you wrote it, but is much more relevant as that is
>closer to the time when anybody other than the author have seen the
>patch for the first time.
>
but why do you consider this more interesting than when the commit
actually hit the target branch? why would most people exploring branch
histories care more for the "meta" surrounding submissions rather than
when they actually started to matter to users?
>> Likewise, we show the author's name by default, not the committer's.
>
which should be kept, because the committer identity is much less
interesting in most workflows.
showing author name + committer date by default may seem unclean, but
it's what is most *useful*.
if the change was to be made, it would have to be visualized to avoid
confusion. i kinda like s/Date:/Stamp:/ because it's short and the
metaphor kinda makes sense. "Committed:" seems worse. format=fuller's
"CommitDate" seems even worse in this context.
the new default should get a new somewhat arbitrary name like "compact".
note that i'm not worried about backwards compatibility, as somebody
parsing the porcelain output without even specifying a format doesn't
deserve differently.
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Ian Jackson @ 2026-07-12 8:22 UTC (permalink / raw)
To: Colin Stagner; +Cc: Junio C Hamano, git, Johannes Schindelin
In-Reply-To: <a8c72dcd-f8d7-47ce-a4b2-ebcd4188875e@howdoi.land>
Colin Stagner writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]"):
> On 7/11/26 18:04, Junio C Hamano wrote:
> > So, is there a conclusion after reviewing this?
>
> I think we're expecting a reroll, but this looks like the way forward.
Yes. Please bear with me, I'm travelling for a few days.
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* "discard!" commit message for commits that should be removed while cleaning up the history
From: Simon Richter @ 2026-07-12 5:54 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 440 bytes --]
Hi,
I often add printf statements during debugging, which obviously should
not end up in the final submission. My usual approach is to commit these
immediately, into commits with a message of "DISCARD", so that when I do
a final rebase pass, I can remove the debug code easily.
Would it make sense to add a mechanism that autosquash understands
directly, and that could be checked for by a push hook or CI rule?
Simon
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v9 0/4] graph: indent visual roots in graph
From: Chandra Pratap @ 2026-07-12 5:56 UTC (permalink / raw)
To: Mirko Faina
Cc: Pablo Sabater, git, ayu.chandekar, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31
In-Reply-To: <alJpjTXfZmYQccwk@exploit>
On Sat, 11 Jul 2026 at 21:55, Mirko Faina <mroik@delayed.space> wrote:
>
> On Sat, Jul 11, 2026 at 05:41:58PM +0200, Pablo Sabater wrote:
> > I think that this solves an ambiguity so it should be the default option
> > and someone who doesn't want the indentation has to explicitly unset it
> > maybe with something like '--no-graph-indent'.
>
> The reason I prefer the current way of printing as the default is
> because the ambiguity arises only when each commit occupies exactly one
> line. In any other case we can clearly see the edges connecting the
> vertices. I'd rather have --oneline imply what would be --graph-indent
> instead of having to pass --no-graph-indent on any other format
> different from --oneline or --format=reference.
Tying graph-drawing logic to specific formatting flags could introduce
inconsistencies. For example, if a user relies on a custom format like
--format="%h %s", the output is functionally single-line and suffers
from the exact same ambiguity, but it would miss the fix.
Even in multi-line formats, relying on the absence of a '|' character to spot
unrelated commits requires active effort. Indentation provides an immediate
visual cue that breaks the vertical lineage, which is helpful regardless of the
commit message length.
I agree with Pablo: for users who strictly want the old behavior, an opt-out
flag keeps the graph logic decoupled from the formatting logic.
> > Apart from having an option to disable indentation.
> >
> > We could have the cascading to have a limit or make it zig-zag:
> >
> > instead of:
> >
> > A
> > B
> > C
> > D
> >
> > We could do:
> >
> > A
> > B
> > C
> > D
> >
> > This would have its own edge cases like:
> >
> > A
> > B
> > C <- if we zig-zag here C and D become ambiguous, currently we are
> > D indenting only the last commits (visual roots) here we would have
> > D to chose between continuing cascading or indenting the first of D.
> >
> > I'm not so sure if I like the zig-zag solution because we need to think again
> > if it causes an ambiguity, but I wanted to mention it.
> >
> > I think we need some more opinions about the design.
>
> I don't dislike the the current solution but I can see it degenerating
> if someone contributes a lot of one-patch series.
>
> Maybe you could indent commits that are both head and tail up to two
> levels and then on the third go back to the beginning of the line. That
> way you kind of have a zig-zag but without ambiguity. You'd only have to
> add a counter to keep track of the level of indentation.
Not sure about this. A zig-zag pattern visually mimics branching and
merging, which makes unrelated commits look like a complex merge topology.
I also have a feeling that this will end up recreating the exact ambiguity this
patch series is trying to fix.
^ permalink raw reply
* Re: [PATCH GSoC v16 00/13] cat-file: add remote-object-info to batch-command
From: Chandra Pratap @ 2026-07-12 5:26 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, chriscool, eric.peijian, gitster, jltobler, karthik.188,
peff, toon
In-Reply-To: <20260710-ps-eric-work-rebase-v16-0-66e07b58a8fe@gmail.com>
On Fri, 10 Jul 2026 at 22:11, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>
> This patch series is a continuation of Eric Ju's
> (eric.peijian@gmail.com) and Calvin Wan's (calvinwan@google.com) patch
> series [1] and [2] respectively.
>
> Sometimes it is beneficial to retrieve information about an object
> without having to download it completely. The server logic for
> retrieving size has already been implemented and merged in a2ba162cda
> (object-info: support for retrieving object info, 2021-04-20) [3].
> This patch series implement the client option for it.
>
> Eric's series adds the remote-object-info command to cat-file
> --batch-command. This command allows the client to make an object-info
> command request to a server that supports protocol v2.
>
> If the server uses protocol v2 but does not support the object-info
> capability, cat-file --batch-command will die.
>
> If a user attempts to use remote-object-info with protocol v1, cat-file
> --batch-command will die.
>
> Currently, only the size (%(objectsize)) is supported end to end in this
> implementation. The type (%(objecttype)) is known by the client's
> allow-list and request path but is not supported on the server side
> nor the response parsing. A follow up series will add full end-to-end
> support for %(objecttype).
>
> The default format for remote-object-info is set to "%(objectname)
> %(objectsize)". Once %(objecttype) is supported, the default format will
> be unified accordingly.
>
> If the batch command format includes unsupported fields such as
> %(objecttype), %(objectsize:disk), or %(deltabase), the command will
> return empty strings for each unsupported field.
>
> This series completes Eric's work mainly with the refactor of the
> validation of the placeholder with an allow-list that filters what the
> client asks with what the server is capable of provide following Jeff
> King's idea [4].
>
> GitHub CI: https://github.com/pabloosabaterr/git/actions/runs/29091116939
>
> [1]: https://lore.kernel.org/git/20250221190451.12536-1-eric.peijian@gmail.com/
> [2]: https://lore.kernel.org/git/20220728230210.2952731-1-calvinwan@google.com/#t
> [3]: https://git.kernel.org/pub/scm/git/git.git/commit/?id=a2ba162cda2acc171c3e36acbbc854792b093cb7
> [4]: https://lore.kernel.org/git/20250313060250.GH94015@coredump.intra.peff.net/
>
> Changes since v15:
> - Completely dropped the static advertise_sid variable at fetch-pack.c
> - Split the hash_algo type change into its own commit.
> - Removed strtoumax_szt() from git-compat-util.h (and its commit) into a
> static parse_object_size() helper.
> - Removed backquotes from commit message bodies and fixed typos.
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
> Calvin Wan (3):
> fetch-pack: move fetch initialization
> serve: advertise object-info feature
> transport: add client support for object-info
>
> Eric Ju (3):
> cat-file: declare loop counter inside for()
> t1006: split test utility functions into new 'lib-cat-file.sh'
> cat-file: add remote-object-info to batch-command
>
> Pablo Sabater (7):
> transport-helper: fix memory leak of helper on disconnect
> fetch-pack: fix hash_algo variable type
> fetch-pack: drop static advertise_sid variable
> fetch-pack: move write_fetch_command_and_capabilities() to connect.c
> connect: make write_fetch_command_and_capabilities() more generic
> cat-file: validate remote atoms with an allow-list
> cat-file: make remote-object-info allow-list dynamic
>
> Documentation/git-cat-file.adoc | 29 +-
> Documentation/gitprotocol-v2.adoc | 11 +-
> Makefile | 1 +
> builtin/cat-file.c | 221 ++++++++++-
> connect.c | 34 ++
> connect.h | 8 +
> fetch-object-info.c | 129 ++++++
> fetch-object-info.h | 22 ++
> fetch-pack.c | 58 +--
> fetch-pack.h | 1 +
> meson.build | 1 +
> object-file.c | 10 +
> odb.h | 3 +
> serve.c | 5 +-
> t/lib-cat-file.sh | 16 +
> t/meson.build | 1 +
> t/t1006-cat-file.sh | 13 +-
> t/t1017-cat-file-remote-object-info.sh | 699 +++++++++++++++++++++++++++++++++
> transport-helper.c | 15 +-
> transport-internal.h | 8 +
> transport.c | 46 +++
> transport.h | 10 +
> 22 files changed, 1255 insertions(+), 86 deletions(-)
>
> base-commit: f60db8d575adb79761d363e026fb49bddf330c73
This version looks fine to me.
Thanks,
Chandra.
^ permalink raw reply
* Re: [PATCH v3] sequencer: honor --empty when a fixup!/squash! empties its target
From: Junio C Hamano @ 2026-07-12 5:01 UTC (permalink / raw)
To: Farid Zakaria; +Cc: git, Phillip Wood, Elijah Newren, Patrick Steinhardt
In-Reply-To: <20260711-fz-autosquash-empty-v3-1-d227b63eb511@gmail.com>
Farid Zakaria <farid.m.zakaria@gmail.com> writes:
> 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.
> ...
> Changes in v3:
> * Switch the new tests' assertions from grep to test_grep for better
> diagnostics (per review).
> * Link to v2: https://lore.kernel.org/r/20260710-fz-autosquash-empty-v2-1-fa1e277e05f8@gmail.com
I see you are already working well with Phillip, which is great.
This topic, when merged to 'seen', seems to have quite a lot of
overlaps with his pw/rebase-drop-notes-with-commit topic. We are
expecting the topic to be rerolled, and I was under the impression
that the remaining issues in that topic were all minor (Phillip,
correct me if I am wrong) and hopefully we will see it in 'next'
not in so distant future.
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.
Thanks.
^ permalink raw reply
* Git 2.55.0/next/seen NO_RUST fsmonitor BUG_vfl crash
From: Đoàn Trần Công Danh @ 2026-07-12 4:04 UTC (permalink / raw)
To: git
Hello,
With Git 2.55.0 built without Rust (NO_RUST = Yes), also observed
with seen and next branches.
Running ./t7527-builtin-fsmonitor.sh will run into crash on:
not ok 43 - Matrix[uc:false][fsm:true] move_directory_up
#
# matrix_clean_up_repo &&
# $fn &&
# if test $uc = false && test $fsm = false
# then
# git status --porcelain=v1 >.git/expect.$fn
# else
# git status --porcelain=v1 >.git/actual.$fn &&
# test_cmp .git/expect.$fn .git/actual.$fn
# fi
#
With backtrace:
#0 __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1 0x00007fad081c724f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:89
#2 0x00007fad08177fe2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3 0x00007fad08160efc in __GI_abort () at ./stdlib/abort.c:73
#4 0x0000561637c48e71 in BUG_vfl (file=0x561637fc98b0 "compat/fsmonitor/fsm-listen-linux.c", line=144, fmt=0x561637fc9890 "double remove of watch for '%s'", params=0x7fad03929b38)
at usage.c:343
#5 BUG_fl (file=file@entry=0x561637fc98b0 "compat/fsmonitor/fsm-listen-linux.c", line=line@entry=144, fmt=fmt@entry=0x561637fc9890 "double remove of watch for '%s'") at usage.c:360
#6 0x0000561637f490bf in remove_watch (w=0x561675350240, data=data@entry=0x56167533b050) at compat/fsmonitor/fsm-listen-linux.c:144
#7 0x0000561637f49f22 in rename_dir (cookie=<optimized out>, path=0x561675352260 "/home/sgn/src/git/t/trash directory.t7527-builtin-fsmonitor/T1/T3", data=<optimized out>)
at compat/fsmonitor/fsm-listen-linux.c:246
#8 process_event (path=0x561675352260 "/home/sgn/src/git/t/trash directory.t7527-builtin-fsmonitor/T1/T3", event=0x7fad03929de0, batch=<synthetic pointer>, cookie_list=0x7fad03929d60,
state=0x7ffdb01840c0) at compat/fsmonitor/fsm-listen-linux.c:559
#9 handle_events (state=state@entry=0x7ffdb01840c0) at compat/fsmonitor/fsm-listen-linux.c:661
#10 0x0000561637f4a73f in fsm_listen__loop (state=state@entry=0x7ffdb01840c0) at compat/fsmonitor/fsm-listen-linux.c:728
#11 0x0000561637ca8e65 in fsm_listen__thread_proc (_state=0x7ffdb01840c0) at builtin/fsmonitor--daemon.c:1194
#12 0x00007fad081c5579 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:448
#13 0x00007fad0823f858 in __GI___clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
[System Info]
git version:
git version 2.55.0.551.g10ab9bd069f7d
cpu: x86_64
built from commit: 10ab9bd069f7d523b2392c7b471a6a7af88a5740
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
feature: fsmonitor--daemon
gettext: enabled
libcurl: 8.21.0
OpenSSL: OpenSSL 3.6.3 9 Jun 2026
zlib: 1.3.2
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 7.0.14_1 #1 SMP PREEMPT_DYNAMIC Sat Jun 27 23:57:22 UTC 2026 x86_64
compiler info: gnuc: 14.2
libc info: glibc: 2.41
$SHELL (typically, interactive shell): /bin/zsh
--
Danh
^ permalink raw reply
* Re: [GSoC Patch] repo: support category-based prefix querying for info keys
From: K Jayatheerth @ 2026-07-12 2:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jltobler, lucasseikioshiro
In-Reply-To: <xmqq7bnbheo1.fsf@gitster.g>
Hey Junio,
On Sat, Jul 4, 2026 at 4:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> You mean "repo info" takes layout.bare and layout.shallow (right
> now, later we may gain a lot more), so you want to say "everything
> under 'layout' to grab these two values?
>
> Why should we limit ourselves to "prefix match"? Would a glob like
> "layout.*", or "path.*.absolute", work better? Especially the
> latter, i.e., "I want the path variables, but am not interested in
> their .relative values, only the .absolute ones." It is especially
> puzzling as you are going to do a dumb linear search in this mode
> anyway.
>
> Perhaps during each iteration of the loop over argv[], you can first
> look for exact match using the existing bsearch() codepath. If that
> succeeds, you have a single key to return the value for. If it does
> not match exactly any key, use the new "prefix" (or "glob" which I
> think would make far more sense) match codepath to find which key(s)
> to return values for, so iterate over them (or say "Hey, that pattern
> does not match any key!" and fail).
Sorry this response took a long time but I have given a good thought about this
You are right that adding globs makes much more sense in this case.
I was initially skeptical about globs, but looking at the direction we took
in path.* keys it makes much more sense.
But I think adding a query system is not a good idea anymore.
I have discussed this with my mentors at length.
Since we are building a plumbing command, we couldn't think of a use-case where
people would need globs over hard-coded value in scripts.
Also globing might introduce uncertainties if the script doesn't have
an appropriate fall back.
I am also wondering if there will be any significant performance difference
with --all vs globs.
A query system in itself is meant to simplify commands for user usage,
but I don't think adding it makes sense "yet".
I also wanted your opinion on this
For my GSoC I can pick the histograms patch in the git repo structure
instead of this.
Regards,
- K Jayatheerth
^ permalink raw reply
* [PATCH v3] sequencer: honor --empty when a fixup!/squash! empties its target
From: Farid Zakaria @ 2026-07-12 0:38 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Elijah Newren, Patrick Steinhardt, Junio C Hamano,
Farid Zakaria
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.
When "--empty=drop" applies, the emptied commit has already been created
by the preceding "pick", so drop it by moving HEAD back to its parent.
The commit is dropped rather than rewritten, so discard the pending
rewrite records and do not record the fixup either, leaving nothing for
the post-rewrite machinery; a following "label" or "update-ref" then sees
HEAD at the parent.
Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
---
At Meta we maintain a fork of LLVM that we regularly rebase onto
upstream. A set of internal patches rides on top, and we keep each one
as a single commit by folding follow-up changes into it with autosquash
"fixup!" commits. That works well for evolving a patch, but not for
retiring one: to back an internal patch out today we hand-edit the
interactive rebase todo list to delete the commit and its scattered
fixups, which is fiddly and easy to get wrong. (The history is rewritten
either way, so a force-push is still needed; what this avoids is the
manual todo surgery.)
It would be nicer to retire a patch the same way we amend one: commit a
revert of it as a "fixup!" and let autosquash fold the two together.
The net change is empty, so the commit should just drop out of the
series. Today it does not -- the rebase stops instead.
For example, starting from a commit we want to retire:
$ git log --oneline
4d5e6f7 add feature patch
9a1b2c3 base
# revert the feature and mark the revert as a fixup of it
$ git revert --no-edit HEAD
$ git commit --amend -m "fixup! add feature patch"
$ git rebase -i --autosquash --empty=drop 9a1b2c3
Rebasing (2/2)
You asked to amend the most recent commit, but doing so would
make it empty. You can repeat your command with --allow-empty [...]
Could not apply 8e9f0a1... # fixup! add feature patch
The "--empty=drop" is ignored. "--empty" only governs commits that are
picked empty, whereas a "fixup!" is applied by amending, and the
emptiness of an amended commit is measured against the wrong parent. So
the rebase falls through to "git commit --amend", which refuses to
create an empty commit, and halts.
With this patch the emptied commit is recognized and handled according
to "--empty", the same as any other commit that becomes empty during a
rebase:
$ git rebase -i --autosquash --empty=drop 9a1b2c3
Rebasing (2/2)
dropping 8e9f0a1... fixup! add feature patch -- resulting commit is empty
Successfully rebased and updated refs/heads/main.
$ git log --oneline
9a1b2c3 base
"--empty=keep" retains it as an empty commit, and "--empty=stop" (the
default under "-i") halts so the user can decide -- matching how these
options already behave for commits that become empty when picked.
Changes in v3:
* Switch the new tests' assertions from grep to test_grep for better
diagnostics (per review).
* Link to v2: https://lore.kernel.org/r/20260710-fz-autosquash-empty-v2-1-fa1e277e05f8@gmail.com
Changes in v2 (thanks to Phillip Wood's review):
* An emptied fixup/squash now honors --empty in all cases, including
when the commit it was folded into started out empty; v1 kept that
case regardless of --empty.
* On drop, the dropped commit and its fixup are no longer recorded as
rewritten, so nothing spurious reaches the post-rewrite machinery.
* Added tests for the empty-placeholder + fixup cases and for the
not-recorded-as-rewritten behavior; adjusted t3415 "abort last squash".
* Link to v1: https://lore.kernel.org/r/20260709-fz-autosquash-empty-v1-1-84cb494c3613@gmail.com
---
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
---
Documentation/git-rebase.adoc | 12 ++++
sequencer.c | 148 +++++++++++++++++++++++++++++++++++-------
t/t3415-rebase-autosquash.sh | 140 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 276 insertions(+), 24 deletions(-)
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. Such a commit is treated the same way and is
+dropped, kept, or stopped at according to this option.
++
See also INCOMPATIBLE OPTIONS below.
--no-keep-empty::
@@ -591,6 +596,13 @@ changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
are moved right after the commit they modify. The `--interactive` option can
be used to review and edit the todo list before proceeding.
+
+If melding a `fixup!` or `squash!` commit cancels out all of the changes of
+the commit it is applied to, the result is an empty commit. The handling of
+these empty commits can be configured with the `--empty` option: the emptied
+commit is dropped, kept, or stopped at. This makes it possible to back a
+change out of a series by committing a revert of it as a `fixup!` and letting
+`--autosquash --empty=drop` remove both.
++
The recommended way to create commits with squash markers is by using the
`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
linkgit:git-commit[1], which take the target commit as an argument and
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);
+ 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,
return buf.buf;
}
+/*
+ * A "fixup!"/"squash!" that melds into HEAD may empty it out. In that case,
+ * with --empty=drop, we want to drop the commit entirely. Since the commit
+ * being amended has already been created (by the preceding "pick"), and the
+ * index and worktree already match the tree of its parent, dropping it is a
+ * matter of moving HEAD back to that parent.
+ */
+static int reset_head_to_parent(struct repository *r, struct replay_opts *opts,
+ struct object_id *head)
+{
+ struct commit *head_commit = lookup_commit_reference(r, head);
+
+ if (!head_commit || repo_parse_commit(r, head_commit))
+ return error(_("could not parse HEAD commit"));
+ if (!head_commit->parents)
+ return error(_("cannot drop the root commit"));
+
+ return refs_update_ref(get_main_ref_store(r),
+ reflog_message(opts, "fixup",
+ "dropping emptied commit"),
+ "HEAD", &head_commit->parents->item->object.oid,
+ head, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
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)
{
struct replay_ctx *ctx = opts->ctx;
unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2277,6 +2334,9 @@ static int do_pick_commit(struct repository *r,
enum todo_command command = item->command;
struct commit *commit = item->commit;
+ if (dropped)
+ *dropped = 0;
+
if (is_rebase_i(opts))
reflog_action = reflog_message(
opts, command_to_string(item->command), NULL);
@@ -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 */
+ if (flags & AMEND_MSG) {
+ /*
+ * A "fixup!"/"squash!" amends HEAD. Separately from the usual
+ * empty-commit handling, check whether applying it leaves the
+ * commit empty and, if so, honor --empty (keep, drop, or -- when
+ * neither is requested -- halt below in do_commit), just as for a
+ * commit that becomes empty when picked.
+ */
+ int melded_empty = amended_commit_is_empty(r);
+ if (melded_empty < 0) {
+ res = melded_empty;
+ goto leave;
+ } else if (melded_empty && opts->keep_redundant_commits) {
+ flags |= ALLOW_EMPTY;
+ } else if (melded_empty && opts->drop_redundant_commits) {
+ 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);
+ /*
+ * The commit the fixup was melded into was already
+ * created by the preceding "pick", so drop it by moving
+ * HEAD back to its parent. Since the commit is being
+ * dropped rather than rewritten, discard the pending
+ * rewrite records and tell our caller not to add one, so
+ * that neither the dropped commit nor the fixup is
+ * recorded as rewritten.
+ */
+ res = reset_head_to_parent(r, opts, &head);
+ if (res)
+ goto leave;
+ unlink(rebase_path_rewritten_pending());
+ if (dropped)
+ *dropped = 1;
+ fprintf(stderr,
+ _("dropping %s %s -- resulting commit is empty\n"),
+ oid_to_hex(&commit->object.oid), msg.subject);
+ }
+ /* else the meld is non-empty, or empty but neither kept nor
+ * dropped, in which case do_commit halts on the empty result. */
+ } else {
+ 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 */
+ }
if (!opts->no_commit && !drop_commit) {
if (author || command == TODO_REVERT || (flags & AMEND_MSG))
res = do_commit(r, msg_file, author, reflog_action,
@@ -4958,12 +5062,12 @@ static int pick_one_commit(struct repository *r,
struct replay_opts *opts,
int *check_todo, int* reschedule)
{
- int res;
+ int res, dropped = 0;
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);
+ check_todo, &dropped);
if (is_rebase_i(opts) && res < 0) {
/* Reschedule */
*reschedule = 1;
@@ -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));
if (res && is_fixup(item->command)) {
@@ -5545,7 +5649,7 @@ static int single_pick(struct repository *r,
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- return do_pick_commit(r, &item, opts, 0, &check_todo);
+ return do_pick_commit(r, &item, opts, 0, &check_todo, NULL);
}
int sequencer_pick_revisions(struct repository *r,
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 &&
: 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 a fixup' '
@@ -510,4 +512,138 @@ test_expect_success 'pick and fixup respect commit.cleanup' '
test_commit_message HEAD -m "something"
'
+test_expect_success 'fixup! that empties its target is dropped with --empty=drop' '
+ git reset --hard base &&
+ test_commit --no-tag addX fileX 1 &&
+ test_commit --no-tag changeX fileX 2 &&
+ test_commit --no-tag later fileW hello &&
+ echo 1 >fileX &&
+ git commit -m "fixup! changeX" fileX &&
+
+ git rebase -i --autosquash --empty=drop HEAD~4 &&
+
+ git log --format=%s >actual &&
+ test_grep ! changeX actual &&
+ test_grep addX actual &&
+ test_grep later actual &&
+ echo 1 >expect &&
+ test_cmp expect fileX &&
+ echo hello >expect &&
+ test_cmp expect fileW
+'
+
+test_expect_success 'fixup! that empties its target is kept with --empty=keep' '
+ git reset --hard base &&
+ test_commit --no-tag addY fileY 1 &&
+ test_commit --no-tag changeY fileY 2 &&
+ echo 1 >fileY &&
+ git commit -m "fixup! changeY" fileY &&
+
+ git rebase -i --autosquash --empty=keep HEAD~3 &&
+
+ git log --format=%s >actual &&
+ test_grep changeY actual &&
+ : "the retained commit is empty" &&
+ git diff --exit-code HEAD~1 HEAD &&
+ echo 1 >expect &&
+ test_cmp expect fileY
+'
+
+test_expect_success 'fixup! that empties its target stops with --empty=stop' '
+ git reset --hard base &&
+ test_commit --no-tag addZ fileZ 1 &&
+ test_commit --no-tag changeZ fileZ 2 &&
+ echo 1 >fileZ &&
+ git commit -m "fixup! changeZ" fileZ &&
+
+ test_when_finished "git rebase --abort" &&
+ test_must_fail git rebase -i --autosquash --empty=stop HEAD~3
+'
+
+test_expect_success 'squash! that empties its target is dropped with --empty=drop' '
+ git reset --hard base &&
+ test_commit --no-tag addS fileS 1 &&
+ test_commit --no-tag changeS fileS 2 &&
+ echo 1 >fileS &&
+ git commit -m "squash! changeS" fileS &&
+
+ git rebase -i --autosquash --empty=drop HEAD~3 &&
+
+ git log --format=%s >actual &&
+ test_grep ! changeS actual &&
+ test_grep addS actual &&
+ echo 1 >expect &&
+ test_cmp expect fileS
+'
+
+test_expect_success 'fixup! filling in an empty commit keeps a non-empty commit' '
+ git reset --hard base &&
+ git commit --allow-empty -m placeholder &&
+ test_commit --no-tag "fixup! placeholder" fileP content &&
+
+ git rebase -i --autosquash --empty=stop HEAD~2 &&
+
+ git log --format=%s >actual &&
+ test_grep placeholder actual &&
+ echo content >expect &&
+ test_cmp expect fileP &&
+ : "the once-empty placeholder is no longer empty" &&
+ test_must_fail git diff --exit-code HEAD~1 HEAD
+'
+
+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" &&
+
+ 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 related
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Colin Stagner @ 2026-07-11 23:37 UTC (permalink / raw)
To: Junio C Hamano, Ian Jackson; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqmrvx86wi.fsf@gitster.g>
On 7/11/26 18:04, Junio C Hamano wrote:
> So, is there a conclusion after reviewing this?
I think we're expecting a reroll, but this looks like the way forward.
Colin
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Junio C Hamano @ 2026-07-11 23:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: Colin Stagner, git, Johannes Schindelin
In-Reply-To: <27215.27575.968985.583226@chiark.greenend.org.uk>
Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
> Hi. Thanks for the review. I'll go through it point by point:
>
> Colin Stagner writes ("Re: [PATCH 2/2] git-subtree: Bail out if we find output from Rust rewrite (test)"):
>> It may be slightly faster to create only one repo and just make orphan
>> branches, like `test_create_subtree_add()` does.
> ...
>> `test_commit()` from test-lib-functions.sh may be superior to manually
>> writing and committing this file.
>
> Thanks for the suggestions. I'll take a look.
So, is there a conclusion after reviewing this?
I think this is the only thing outstanding item among the review
comments this thread received. Specifically, regarding the use of
'local' discussed in the thread, our coding guidelines explicitly
state:
- Even though "local" is not part of POSIX, we make heavy use of it
in our test suite. We do not use it in scripted Porcelains, and
hopefully nobody starts using "local" before all shells that matter
support it (notably, ksh from AT&T Research does not support it yet).
Thus, we are fine there.
Just responding belatedly as I was scanning topics that are marked
as "Expecting a reroll" in my draft copy of the "What's cooking"
report that I work from.
Thanks.
^ permalink raw reply
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Junio C Hamano @ 2026-07-11 22:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: D. Ben Knoble, Jeff King, git, brian m . carlson, Ramsay Jones
In-Reply-To: <aktOn-3K41Uhl9cr@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> I don't think we'd necessarily need a way to detect this. Our current
> build default is to have this disabled, so I'd keep it this way, but
> automatically compile nsec-support into Git if available. And then we
> provide a way for users to opt-in to the new behaviour via the config.
>
> An automated test would of course be nice to have so that we know to
> enable this in cases where we can determine that it works. But with the
> above we'd already make the feature more accessible than it currently
> is, because I'd expect that most distros simply don't enable the build
> toggle at all.
In any case, the discussion tells me that if we were to pursue this
topic further, it would not primarily be about adding the build knob
to meson.build file, but rather a bit more involved to affect the
product for everybody regardless of the build framework used.
So I think it is safe for me discard this topic from my tree for
now, with an invitation to resurrect it as a topic with shifted
focus.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 00/10] commit-reach: terminate merge-base walk when one side is exhausted
From: Kristofer Karlsson @ 2026-07-11 21:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Kristofer Karlsson via GitGitGadget, git, Derrick Stolee,
Elijah Newren, René Scharfe, SZEDER Gábor
In-Reply-To: <xmqqv7al9rbj.fsf@gitster.g>
On Sat, 11 Jul 2026 at 22:58, Junio C Hamano <gitster@pobox.com> wrote:
>
> As always, do *not* base your patches on 'next'. I cannot apply
> such a patch series to my tree, as merging the resulting topic down
> to 'master' will pull _all_ the other topics, including those that
> are not ready, plus commits that merge these topics into 'next',
> into 'master'.
>
> Instead, choose the topics that you do depend on, prepare a merge of
> these branches into a stable base (like v2.55.0 or master), and then
> build your series on top.
Ah I think I phrased it poorly in the cover letter.
When I said that it's based on next, I meant that it is
verified to work against next but I also confirmed it
works against what you suggested earlier, e.g. a synthetic base:
git checkout -b synthetic-base origin/master
git merge --no-ff kk/commit-reach-find-all-fix
There is one textual conflict in commit-reach.c;
the resolution combines both:
if (!min_generation && !corrected_commit_dates_enabled(r)) {
queue.pq.compare = compare_commits_by_commit_date;
gen_ordered = 0;
}
After that, all ten patches apply cleanly with git am -3.
I should have stated this more clearly in the cover letter
instead of mentioning next at all.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH 1/2] commit-graph: add trace2 instrumentation for generation DFS
From: Junio C Hamano @ 2026-07-11 21:18 UTC (permalink / raw)
To: Taylor Blau
Cc: Kristofer Karlsson, Taylor Blau,
Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <alF4rYSTxpQUC38K@com-79390>
Taylor Blau <ttaylorr@openai.com> writes:
>> If the test involved is longer than 3 lines, I would recommend
>> against it, as "git show" of such a patch will show the full code
>> change to implement a different behaviour plus "_failure" changing
>> to "_success" in the test, with the body of the test hidden outside
>> the context, which makes it hard to guess what the behaviour change
>> is really about.
>
> Hmm, I am not sure that I agree. Or, at the very least, that is now how
> I have written series in the past where I want to demonstrate and then
> subsequently fix an existing bug.
After applying and in viewing "git log -W -p", there is no such
difficulty like the one I described in the message you are
responding to, but it makes it harder on reviewers on the mailing
list, to make a quick pre-review based only on the material that
they can see in the e-mail.
It may be easier to write the commits, but given that we seem to
have more patches sent to the list than reviewers can review, it may
not be a good trade-off.
^ permalink raw reply
* Re: [PATCH v9 0/9] migrate more variables into repo_config_values
From: Junio C Hamano @ 2026-07-11 21:10 UTC (permalink / raw)
To: Tian Yuchen; +Cc: Pablo Sabater, git, cirnovskyv, szeder.dev
In-Reply-To: <a7aaa57d-5250-43a6-9646-d1aa29328964@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
>> I missed a base-commit to easily apply this locally, could we
>> add one?
>>
>> Thanks!
>> Pablo
>
> Thanks for pointing out.
>
> The base commit is 8d96f09e9245ddf80c1981476fcbac8c4bb4125f.
> I will put it on the cover letter in the next reroll (if any)!
>
> Regards, yuchen
FWIW, I think I queued it on top of ab776a62a7 (Git 2.55-rc2,
2026-06-22).
Others can find it out by looking at the output from
$ git log --oneline origin/master....origin/seen
and finding the commit that merges the topic.
^ permalink raw reply
* Re: [PATCH v9 0/9] migrate more variables into repo_config_values
From: Junio C Hamano @ 2026-07-11 21:06 UTC (permalink / raw)
To: Pablo Sabater; +Cc: Tian Yuchen, git, cirnovskyv, szeder.dev
In-Reply-To: <DJVUGL8XA0Y0.12LN2COXI5BIY@gmail.com>
"Pablo Sabater" <pabloosabaterr@gmail.com> writes:
> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>> ...
> Hi!
>
> I missed a base-commit to easily apply this locally, could we
> add one?
>
> Thanks!
> Pablo
FYI, a topic that is in 'seen' can be extracted from my tree by
inspecting "git log --oneline origin/master..origin/seen" and
finding the commit that merges the series.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox