* [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 2/9] environment: move excludes_file 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 'excludes_file' is used to track the path to the
global ignore file. If this variable is NULL,
'setup_standard_excludes()' 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 | 17 ++++++++++++++---
environment.h | 4 +++-
3 files changed, 19 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 ae05f16d04..275931c213 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,16 @@ int is_bare_repository(void)
return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
}
+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;
+}
+
int have_git_dir(void)
{
return startup_info->have_repository
@@ -461,8 +470,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 +724,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;
@@ -730,4 +740,5 @@ void repo_config_values_init(struct repo_config_values *cfg)
void repo_config_values_clear(struct repo_config_values *cfg)
{
FREE_AND_NULL(cfg->attributes_file);
+ FREE_AND_NULL(cfg->excludes_file);
}
diff --git a/environment.h b/environment.h
index 9169d7f62d..4776ccc657 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
--
2.43.0
^ permalink raw reply related
* [PATCH v10 4/9] environment: move pager_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 'pager_program' variable is currently defined as a file-scoped
static string in pager.c. Move it into 'struct repo_config_values'.
The configuration parsing logic remains strictly within pager.c to
respect subsystem boundaries. The read/write operations are simply
redirected to the repository-specific structure using
'repo_config_values()'.
Similar to the recent editor_program migration, no standalone getter
is introduced to keep the code minimal. The dynamically allocated
memory is now managed by 'repo_config_values_clear()'.
On top of that, fix a memory leak in pager.c while we are at it.
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 | 2 ++
environment.h | 1 +
| 26 +++++++++++++++++---------
3 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/environment.c b/environment.c
index a65d575af4..975c9cb9eb 100644
--- a/environment.c
+++ b/environment.c
@@ -725,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->attributes_file = NULL;
cfg->excludes_file = NULL;
cfg->editor_program = NULL;
+ cfg->pager_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -742,4 +743,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);
+ FREE_AND_NULL(cfg->pager_program);
}
diff --git a/environment.h b/environment.h
index 8178ebab76..39b6691b47 100644
--- a/environment.h
+++ b/environment.h
@@ -92,6 +92,7 @@ struct repo_config_values {
char *attributes_file;
char *excludes_file;
char *editor_program;
+ char *pager_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
--git a/pager.c b/pager.c
index 35b210e048..bc55546670 100644
--- a/pager.c
+++ b/pager.c
@@ -5,6 +5,8 @@
#include "run-command.h"
#include "sigchain.h"
#include "alias.h"
+#include "repository.h"
+#include "environment.h"
int pager_use_color = 1;
@@ -13,7 +15,6 @@ int pager_use_color = 1;
#endif
static struct child_process pager_process;
-static char *pager_program;
static int old_fd1 = -1, old_fd2 = -1;
/* Is the value coming back from term_columns() just a guess? */
@@ -75,10 +76,15 @@ static void wait_for_pager_signal(int signo)
static int core_pager_config(const char *var, const char *value,
const struct config_context *ctx UNUSED,
- void *data UNUSED)
+ void *data)
{
- if (!strcmp(var, "core.pager"))
- return git_config_string(&pager_program, var, value);
+ struct repository *r = data;
+
+ if (!strcmp(var, "core.pager")) {
+ FREE_AND_NULL(repo_config_values(r)->pager_program);
+ return git_config_string(&repo_config_values(r)->pager_program, var, value);
+ }
+
return 0;
}
@@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
pager = getenv("GIT_PAGER");
if (!pager) {
- if (!pager_program)
+ if (!repo_config_values(r)->pager_program)
read_early_config(r,
- core_pager_config, NULL);
- pager = pager_program;
+ core_pager_config, r);
+ pager = repo_config_values(r)->pager_program;
}
if (!pager)
pager = getenv("PAGER");
@@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
read_early_config(r, pager_command_config, &data);
- if (data.value)
- pager_program = data.value;
+ if (data.value) {
+ free(repo_config_values(r)->pager_program);
+ repo_config_values(r)->pager_program = data.value;
+ }
return data.want;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v10 5/9] environment: move askpass_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 'askpass_program' stores the path to the program
used to prompt the user for credentials. Move it into repo_config_values
to continue the libification effort.
While it is uncommon for a single process to require different askpass
programs for different repositories, maintaining this value as a mutable
global string is a blocker for libification. Global heap-allocated
strings introduce thread-safety issues in a multi-repo environment.
Move 'askpass_program' into 'struct repo_config_values' to eliminate
this global state. The memory is now safely managed and freed via
'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>
---
environment.c | 6 ++++--
environment.h | 1 +
prompt.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/environment.c b/environment.c
index 975c9cb9eb..1a26c9c6d6 100644
--- a/environment.c
+++ b/environment.c
@@ -464,8 +464,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.askpass")) {
- FREE_AND_NULL(askpass_program);
- return git_config_string(&askpass_program, var, value);
+ FREE_AND_NULL(cfg->askpass_program);
+ return git_config_string(&cfg->askpass_program, var, value);
}
if (!strcmp(var, "core.excludesfile")) {
@@ -726,6 +726,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->excludes_file = NULL;
cfg->editor_program = NULL;
cfg->pager_program = NULL;
+ cfg->askpass_program = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -744,4 +745,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
FREE_AND_NULL(cfg->excludes_file);
FREE_AND_NULL(cfg->editor_program);
FREE_AND_NULL(cfg->pager_program);
+ FREE_AND_NULL(cfg->askpass_program);
}
diff --git a/environment.h b/environment.h
index 39b6691b47..a2e9def89d 100644
--- a/environment.h
+++ b/environment.h
@@ -93,6 +93,7 @@ struct repo_config_values {
char *excludes_file;
char *editor_program;
char *pager_program;
+ char *askpass_program;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
diff --git a/prompt.c b/prompt.c
index 706fba2a50..d8d74c7e37 100644
--- a/prompt.c
+++ b/prompt.c
@@ -3,6 +3,7 @@
#include "git-compat-util.h"
#include "parse.h"
#include "environment.h"
+#include "repository.h"
#include "run-command.h"
#include "strbuf.h"
#include "prompt.h"
@@ -51,7 +52,7 @@ char *git_prompt(const char *prompt, int flags)
askpass = getenv("GIT_ASKPASS");
if (!askpass)
- askpass = askpass_program;
+ askpass = repo_config_values(the_repository)->askpass_program;
if (!askpass)
askpass = getenv("SSH_ASKPASS");
if (askpass && *askpass)
--
2.43.0
^ permalink raw reply related
* [PATCH v10 6/9] environment: migrate apply_default_whitespace and apply_default_ignorewhitespace
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 variables 'apply_default_whitespace' and
'apply_default_ignorewhitespace' are used to store the default
whitespace configuration for 'git apply'. Move these variables
into 'struct repo_config_values' to continue the libification
effort.
Dynamically allocated strings fetched via 'repo_config_get_string()'
are now tracked per-repository and safely freed in
'repo_config_values_clear()'.
As part of this transition, update 'git_apply_config()' to accept a
'struct repository *' argument rather than relying on the
'the_repository' global.
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>
---
apply.c | 20 ++++++++++++--------
environment.c | 6 ++++--
environment.h | 4 ++--
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/apply.c b/apply.c
index 249248d4f2..66db9b7678 100644
--- a/apply.c
+++ b/apply.c
@@ -47,11 +47,13 @@ struct gitdiff_data {
int p_value;
};
-static void git_apply_config(void)
+static void git_apply_config(struct repository *repo)
{
- repo_config_get_string(the_repository, "apply.whitespace", &apply_default_whitespace);
- repo_config_get_string(the_repository, "apply.ignorewhitespace", &apply_default_ignorewhitespace);
- repo_config(the_repository, git_xmerge_config, NULL);
+ repo_config_get_string(repo, "apply.whitespace",
+ &repo_config_values(repo)->apply_default_whitespace);
+ repo_config_get_string(repo, "apply.ignorewhitespace",
+ &repo_config_values(repo)->apply_default_ignorewhitespace);
+ repo_config(repo, git_xmerge_config, NULL);
}
static int parse_whitespace_option(struct apply_state *state, const char *option)
@@ -126,10 +128,12 @@ int init_apply_state(struct apply_state *state,
strset_init(&state->kept_symlinks);
strbuf_init(&state->root, 0);
- git_apply_config();
- if (apply_default_whitespace && parse_whitespace_option(state, apply_default_whitespace))
+ git_apply_config(repo);
+ if (repo_config_values(repo)->apply_default_whitespace &&
+ parse_whitespace_option(state, repo_config_values(repo)->apply_default_whitespace))
return -1;
- if (apply_default_ignorewhitespace && parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+ if (repo_config_values(repo)->apply_default_ignorewhitespace &&
+ parse_ignorewhitespace_option(state, repo_config_values(repo)->apply_default_ignorewhitespace))
return -1;
return 0;
}
@@ -192,7 +196,7 @@ int check_apply_state(struct apply_state *state, int force_apply)
static void set_default_whitespace_mode(struct apply_state *state)
{
- if (!state->whitespace_option && !apply_default_whitespace)
+ if (!state->whitespace_option && !repo_config_values(state->repo)->apply_default_whitespace)
state->ws_error_action = (state->apply ? warn_on_ws_error : nowarn_ws_error);
}
diff --git a/environment.c b/environment.c
index 1a26c9c6d6..41ba013c86 100644
--- a/environment.c
+++ b/environment.c
@@ -49,8 +49,6 @@ int assume_unchanged;
int is_bare_repository_cfg = -1; /* unspecified */
char *git_commit_encoding;
char *git_log_output_encoding;
-char *apply_default_whitespace;
-char *apply_default_ignorewhitespace;
int fsync_object_files = -1;
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
@@ -727,6 +725,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->editor_program = NULL;
cfg->pager_program = NULL;
cfg->askpass_program = NULL;
+ cfg->apply_default_whitespace = NULL;
+ cfg->apply_default_ignorewhitespace = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -746,4 +746,6 @@ void repo_config_values_clear(struct repo_config_values *cfg)
FREE_AND_NULL(cfg->editor_program);
FREE_AND_NULL(cfg->pager_program);
FREE_AND_NULL(cfg->askpass_program);
+ FREE_AND_NULL(cfg->apply_default_whitespace);
+ FREE_AND_NULL(cfg->apply_default_ignorewhitespace);
}
diff --git a/environment.h b/environment.h
index a2e9def89d..553f87adee 100644
--- a/environment.h
+++ b/environment.h
@@ -94,6 +94,8 @@ struct repo_config_values {
char *editor_program;
char *pager_program;
char *askpass_program;
+ char *apply_default_whitespace;
+ char *apply_default_ignorewhitespace;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -182,8 +184,6 @@ extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
extern int ignore_case;
extern int assume_unchanged;
-extern char *apply_default_whitespace;
-extern char *apply_default_ignorewhitespace;
extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
--
2.43.0
^ permalink raw reply related
* [PATCH v10 7/9] environment: move push_default 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 'push_default' specifies the default behavior of
'git push' when no explicit refspec is provided. Move 'push_default'
into 'struct repo_config_values' to continue the libification effort.
While 'enum push_default_type' ideally belongs in 'remote.h', moving it
there introduces a circular dependency chain:
remote.h -> hash.h -> repository.h -> environment.h.
Therefore, the enum definition is kept in 'environment.h' just above
'struct repo_config_values' with a NEEDSWORK comment for future cleanup.
Modify the configuration parsing in environment.c to update the
per-repository structure directly, and update caller across the
codebase to access the value via 'repo_config_values()'.
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>
---
builtin/push.c | 8 ++++----
environment.c | 16 +++++++++-------
environment.h | 26 ++++++++++++++++----------
remote.c | 2 +-
4 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 6021b71d66..6dc3224b60 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -88,7 +88,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
}
}
- if (push_default == PUSH_DEFAULT_UPSTREAM &&
+ if (repo_config_values(the_repository)->push_default == PUSH_DEFAULT_UPSTREAM &&
skip_prefix(matched->name, "refs/heads/", &branch_name)) {
struct branch *branch = branch_get(branch_name);
if (branch->merge_nr == 1 && branch->merge[0]->src) {
@@ -160,7 +160,7 @@ static NORETURN void die_push_simple(struct branch *branch,
* Don't show advice for people who explicitly set
* push.default.
*/
- if (push_default == PUSH_DEFAULT_UNSPECIFIED)
+ if (cfg->push_default == PUSH_DEFAULT_UNSPECIFIED)
advice_pushdefault_maybe = _("\n"
"To choose either option permanently, "
"see push.default in 'git help config'.\n");
@@ -232,7 +232,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
const char *dst;
int same_remote;
- switch (push_default) {
+ switch (repo_config_values(the_repository)->push_default) {
case PUSH_DEFAULT_MATCHING:
refspec_append(&rs, ":");
return;
@@ -252,7 +252,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
dst = branch->refname;
same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
- switch (push_default) {
+ switch (repo_config_values(the_repository)->push_default) {
default:
case PUSH_DEFAULT_UNSPECIFIED:
case PUSH_DEFAULT_SIMPLE:
diff --git a/environment.c b/environment.c
index 41ba013c86..0080012f31 100644
--- a/environment.c
+++ b/environment.c
@@ -59,7 +59,6 @@ enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
char *check_roundtrip_encoding;
enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
-enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
@@ -621,21 +620,23 @@ static int git_default_branch_config(const char *var, const char *value)
static int git_default_push_config(const char *var, const char *value)
{
+ struct repo_config_values *cfg = repo_config_values(the_repository);
+
if (!strcmp(var, "push.default")) {
if (!value)
return config_error_nonbool(var);
else if (!strcmp(value, "nothing"))
- push_default = PUSH_DEFAULT_NOTHING;
+ cfg->push_default = PUSH_DEFAULT_NOTHING;
else if (!strcmp(value, "matching"))
- push_default = PUSH_DEFAULT_MATCHING;
+ cfg->push_default = PUSH_DEFAULT_MATCHING;
else if (!strcmp(value, "simple"))
- push_default = PUSH_DEFAULT_SIMPLE;
+ cfg->push_default = PUSH_DEFAULT_SIMPLE;
else if (!strcmp(value, "upstream"))
- push_default = PUSH_DEFAULT_UPSTREAM;
+ cfg->push_default = PUSH_DEFAULT_UPSTREAM;
else if (!strcmp(value, "tracking")) /* deprecated */
- push_default = PUSH_DEFAULT_UPSTREAM;
+ cfg->push_default = PUSH_DEFAULT_UPSTREAM;
else if (!strcmp(value, "current"))
- push_default = PUSH_DEFAULT_CURRENT;
+ cfg->push_default = PUSH_DEFAULT_CURRENT;
else {
error(_("malformed value for %s: %s"), var, value);
return error(_("must be one of nothing, matching, simple, "
@@ -727,6 +728,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->askpass_program = NULL;
cfg->apply_default_whitespace = NULL;
cfg->apply_default_ignorewhitespace = NULL;
+ cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 553f87adee..6a5c8bd06f 100644
--- a/environment.h
+++ b/environment.h
@@ -87,6 +87,21 @@ extern const char * const local_repo_env[];
struct strvec;
struct repository;
+
+/*
+ * NEEDSWORK: It would be better if these definitions could be moved to
+ * other more specific files, but care is needed to avoid circular
+ * inclusion issues.
+ */
+enum push_default_type {
+ PUSH_DEFAULT_NOTHING = 0,
+ PUSH_DEFAULT_MATCHING,
+ PUSH_DEFAULT_SIMPLE,
+ PUSH_DEFAULT_UPSTREAM,
+ PUSH_DEFAULT_CURRENT,
+ PUSH_DEFAULT_UNSPECIFIED
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -96,6 +111,7 @@ struct repo_config_values {
char *askpass_program;
char *apply_default_whitespace;
char *apply_default_ignorewhitespace;
+ enum push_default_type push_default;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -197,16 +213,6 @@ enum rebase_setup_type {
};
extern enum rebase_setup_type autorebase;
-enum push_default_type {
- PUSH_DEFAULT_NOTHING = 0,
- PUSH_DEFAULT_MATCHING,
- PUSH_DEFAULT_SIMPLE,
- PUSH_DEFAULT_UPSTREAM,
- PUSH_DEFAULT_CURRENT,
- PUSH_DEFAULT_UNSPECIFIED
-};
-extern enum push_default_type push_default;
-
enum object_creation_mode {
OBJECT_CREATION_USES_HARDLINKS = 0,
OBJECT_CREATION_USES_RENAMES = 1
diff --git a/remote.c b/remote.c
index 00723b385e..d48c01d375 100644
--- a/remote.c
+++ b/remote.c
@@ -1933,7 +1933,7 @@ static char *branch_get_push_1(struct repository *repo,
if (remote->mirror)
return tracking_for_push_dest(remote, branch->refname, err);
- switch (push_default) {
+ switch (repo_config_values(repo)->push_default) {
case PUSH_DEFAULT_NOTHING:
return error_buf(err, _("push has no destination (push.default is 'nothing')"));
--
2.43.0
^ permalink raw reply related
* [PATCH v10 8/9] environment: move autorebase 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 'autorebase' dictates whether a newly created
branch should be configured to automatically rebase by default.
Move it into 'struct repo_config_values' to continue the
libification effort.
The 'enum rebase_setup_type' definition is moved higher up in
'environment.h' so that it is visible to the repository-specific
structure. The default state AUTOREBASE_NEVER is now correctly
initialized in 'repo_config_values_init()'.
Configuration parsing in 'git_default_branch_config()' is updated to
write directly to the repository's configuration instance.
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>
---
branch.c | 2 +-
environment.c | 10 +++++-----
environment.h | 16 ++++++++--------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 243db7d0fc..e1c1f8c89d 100644
--- a/branch.c
+++ b/branch.c
@@ -61,7 +61,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
static int should_setup_rebase(const char *origin)
{
- switch (autorebase) {
+ switch (repo_config_values(the_repository)->autorebase) {
case AUTOREBASE_NEVER:
return 0;
case AUTOREBASE_LOCAL:
diff --git a/environment.c b/environment.c
index 0080012f31..42829a9c7a 100644
--- a/environment.c
+++ b/environment.c
@@ -58,7 +58,6 @@ enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
char *check_roundtrip_encoding;
-enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
@@ -602,13 +601,13 @@ static int git_default_branch_config(const char *var, const char *value)
if (!value)
return config_error_nonbool(var);
else if (!strcmp(value, "never"))
- autorebase = AUTOREBASE_NEVER;
+ cfg->autorebase = AUTOREBASE_NEVER;
else if (!strcmp(value, "local"))
- autorebase = AUTOREBASE_LOCAL;
+ cfg->autorebase = AUTOREBASE_LOCAL;
else if (!strcmp(value, "remote"))
- autorebase = AUTOREBASE_REMOTE;
+ cfg->autorebase = AUTOREBASE_REMOTE;
else if (!strcmp(value, "always"))
- autorebase = AUTOREBASE_ALWAYS;
+ cfg->autorebase = AUTOREBASE_ALWAYS;
else
return error(_("malformed value for %s"), var);
return 0;
@@ -729,6 +728,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_whitespace = NULL;
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
+ cfg->autorebase = AUTOREBASE_NEVER;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index 6a5c8bd06f..deabc5ef30 100644
--- a/environment.h
+++ b/environment.h
@@ -102,6 +102,13 @@ enum push_default_type {
PUSH_DEFAULT_UNSPECIFIED
};
+enum rebase_setup_type {
+ AUTOREBASE_NEVER = 0,
+ AUTOREBASE_LOCAL,
+ AUTOREBASE_REMOTE,
+ AUTOREBASE_ALWAYS
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -112,6 +119,7 @@ struct repo_config_values {
char *apply_default_whitespace;
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
+ enum rebase_setup_type autorebase;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -205,14 +213,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum rebase_setup_type {
- AUTOREBASE_NEVER = 0,
- AUTOREBASE_LOCAL,
- AUTOREBASE_REMOTE,
- AUTOREBASE_ALWAYS
-};
-extern enum rebase_setup_type autorebase;
-
enum object_creation_mode {
OBJECT_CREATION_USES_HARDLINKS = 0,
OBJECT_CREATION_USES_RENAMES = 1
--
2.43.0
^ permalink raw reply related
* [PATCH v10 9/9] environment: move object_creation_mode 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 'object_creation_mode' controls how Git creates
object files, specifically determining whether to use hardlinks or
renames when moving temporary files into the object database. Move
it into 'struct repo_config_values' to continue the libification
effort.
Move the 'enum object_creation_mode' definition higher up in
'environment.h' to ensure it is visible to the structure. Initialize
the per-repository value to its default macro value
OBJECT_CREATION_MODE inside 'repo_config_values_init()'.
Update configuration parsing in 'git_default_core_config()' to write
directly to the repository-specific configuration structure.
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 | 6 +++---
environment.h | 12 ++++++------
object-file.c | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/environment.c b/environment.c
index 42829a9c7a..e882e4ada2 100644
--- a/environment.c
+++ b/environment.c
@@ -61,7 +61,6 @@ char *check_roundtrip_encoding;
#ifndef OBJECT_CREATION_MODE
#define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
#endif
-enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
int grafts_keep_true_parents;
unsigned long pack_size_limit_cfg;
@@ -513,9 +512,9 @@ int git_default_core_config(const char *var, const char *value,
if (!value)
return config_error_nonbool(var);
if (!strcmp(value, "rename"))
- object_creation_mode = OBJECT_CREATION_USES_RENAMES;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_RENAMES;
else if (!strcmp(value, "link"))
- object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
+ cfg->object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
die(_("invalid mode for object creation: %s"), value);
return 0;
@@ -729,6 +728,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->apply_default_ignorewhitespace = NULL;
cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
cfg->autorebase = AUTOREBASE_NEVER;
+ cfg->object_creation_mode = OBJECT_CREATION_MODE;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
diff --git a/environment.h b/environment.h
index deabc5ef30..de6e80cce2 100644
--- a/environment.h
+++ b/environment.h
@@ -109,6 +109,11 @@ enum rebase_setup_type {
AUTOREBASE_ALWAYS
};
+enum object_creation_mode {
+ OBJECT_CREATION_USES_HARDLINKS = 0,
+ OBJECT_CREATION_USES_RENAMES = 1
+};
+
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
@@ -120,6 +125,7 @@ struct repo_config_values {
char *apply_default_ignorewhitespace;
enum push_default_type push_default;
enum rebase_setup_type autorebase;
+ enum object_creation_mode object_creation_mode;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -213,12 +219,6 @@ extern unsigned long pack_size_limit_cfg;
extern int protect_hfs;
extern int protect_ntfs;
-enum object_creation_mode {
- OBJECT_CREATION_USES_HARDLINKS = 0,
- OBJECT_CREATION_USES_RENAMES = 1
-};
-extern enum object_creation_mode object_creation_mode;
-
extern int grafts_keep_true_parents;
const char *get_log_output_encoding(void);
diff --git a/object-file.c b/object-file.c
index 9afa842da2..cbbfc8f1dc 100644
--- a/object-file.c
+++ b/object-file.c
@@ -415,7 +415,7 @@ int finalize_object_file_flags(struct repository *repo,
retry:
ret = 0;
- if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
+ if (repo_config_values(repo)->object_creation_mode == OBJECT_CREATION_USES_RENAMES)
goto try_rename;
else if (link(tmpfile, filename))
ret = errno;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v9 0/4] graph: indent visual roots in graph
From: Mirko Faina @ 2026-07-12 13:10 UTC (permalink / raw)
To: Chandra Pratap
Cc: Pablo Sabater, git, ayu.chandekar, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31, Mirko Faina
In-Reply-To: <CA+J6zkQcHu-LVKE-1ypfT=59gEzo4qBzi-pmhSJNC_udCDCJZg@mail.gmail.com>
On Sun, Jul 12, 2026 at 11:26:27AM +0530, Chandra Pratap wrote:
> 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.
In that case, together with --[no]-graph-indent, a configuration
variable like "graph.indent" could be introduced to reduce the usage of
--[no]-graph-indent for those that would like to retain the old
behaviour for most formats.
> > > 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.
While a zig-zag pattern might be ambiguous, what I proposed is a little
different.
What I proposed is effectively a wrapping for anything that goes beyond
two levels of indentation. I don't think it would look anything like a
fork/merge pattern.
* A
* B
* C
* D
* E
* F
The difference between two indentation levels and no indentation is very
noticeble, I don't think anyone confused this. This would fix the
staircase pattern on adjacent one-patch series.
^ permalink raw reply
* Re: [PATCH v9 0/4] graph: indent visual roots in graph
From: Mirko Faina @ 2026-07-12 13:12 UTC (permalink / raw)
To: Chandra Pratap
Cc: Pablo Sabater, git, ayu.chandekar, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31, Mirko Faina
In-Reply-To: <alOOXKGIB8BqACxR@exploit>
On Sun, Jul 12, 2026 at 03:10:49PM +0200, Mirko Faina wrote:
> The difference between two indentation levels and no indentation is very
> noticeble, I don't think anyone confused this. This would fix the
> staircase pattern on adjacent one-patch series.
Sorry, meant to write, "I don't think anyone would be confused by this".
^ permalink raw reply
* Re: "discard!" commit message for commits that should be removed while cleaning up the history
From: Junio C Hamano @ 2026-07-12 13:28 UTC (permalink / raw)
To: Simon Richter; +Cc: git
In-Reply-To: <07c9811e-41db-473e-ba0a-cdcbf8187be7@hogyros.de>
Simon Richter <Simon.Richter@hogyros.de> writes:
> 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?
The sequencer machinery used by "git rebase [-i]" already knows how
to react to commits with certain subjects. For example, a commit
with the subject "fixup! <title>" causes that commit to be moved
next to the target commit, and its "pick" insn is turned into a
"fixup" insn. The "git commit" command itself helps you prepare
such a specially formatted commit title with options like "--fixup".
So, it is not totally out of the question to add support for
noticing a subject that begins with "drop!" (rather than your proposed
"DISCARD", purely so that it matches the existing "fixup!" convention
that instructs the sequencer to use the "fixup" insn). This would
automatically turn the "pick" insn into a "drop" insn when "git
rebase -i" works on a history segment that includes such a commit.
On the "git commit" side, we likely do not want to add any support
similar to the "--fixup" option (for example, "--fixup=drop"), as
you can run "git commit -m 'drop!'" just as easily.
Having said all that, you can use a custom GIT_SEQUENCE_EDITOR that
notices commits you titled "DISCARD" and rewrites the "pick" insn for
these commits into a "drop" insn in the todo list, without making
any changes to Git.
^ permalink raw reply
* Re: [PATCH] Makefile: fix up lib directory move
From: Ramsay Jones @ 2026-07-12 13:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Patrick Steinhardt, GIT Mailing-list, Junio C Hamano
In-Reply-To: <ef422523-1c50-ec79-e89a-f43f02ba1ca7@gmx.de>
On 12/07/2026 11:15 am, Johannes Schindelin wrote:
> 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.
Ah, yes, sorry about that! :(
I wrote this patch first, on Linux, when 'easing' myself back after the v2.55.0
release. Then I found the cygwin failures ... :) When I eventually sent the cygwin
email, some days later, I just tacked this on without thinking.
[Usually I send these kind of 'patches' within hours of the branch being published, so
that commit IDs are still 'fresh'. However, even such a small window is no guarantee,
of course! :) ]
>
> Maybe a more stable way to refer to this right now would be to name the
> topic: `ps/libgit-in-subdir`.
Indeed. ;)
>> 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!
>
Thanks!
ATB,
Ramsay Jones
^ 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-12 13:42 UTC (permalink / raw)
To: Ian Jackson; +Cc: Colin Stagner, git, Johannes Schindelin
In-Reply-To: <27219.20156.438730.881821@chiark.greenend.org.uk>
Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
> 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.
No worries, and take your time. I was just updating the status of
the various topics in the "What's cooking" draft.
Thanks.
^ permalink raw reply
* Re: [PATCH] Makefile: fix up lib directory move
From: Junio C Hamano @ 2026-07-12 14:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Ramsay Jones, Patrick Steinhardt, GIT Mailing-list
In-Reply-To: <ef422523-1c50-ec79-e89a-f43f02ba1ca7@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 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`.
Yes, citing the object name alone would have been useless.
On the other hand, it is good to have it as a sanity-checking
clue. If or when the patch does not apply cleanly, we can tell
that the fix-up was written based on an older iteration. The
"reference" format used here carries more information than the
topic branch name alone, and with the commit title, we can run
$ git show 'origin/seen^{/^Move libgit\.a sources into}'
when 9759608622 cannot be found.
>> 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!
Yes, indeed. It is queued on top of the topic.
Thanks.
^ permalink raw reply
* Re: [PATCH v10 4/9] environment: move pager_program into repo_config_values
From: Pablo Sabater @ 2026-07-12 14:12 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-5-cat@malon.dev>
On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
> The 'pager_program' variable is currently defined as a file-scoped
> static string in pager.c. Move it into 'struct repo_config_values'.
>
> The configuration parsing logic remains strictly within pager.c to
> respect subsystem boundaries. The read/write operations are simply
> redirected to the repository-specific structure using
> 'repo_config_values()'.
>
> Similar to the recent editor_program migration, no standalone getter
> is introduced to keep the code minimal. The dynamically allocated
> memory is now managed by 'repo_config_values_clear()'.
>
> On top of that, fix a memory leak in pager.c while we are at it.
>
> 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 | 2 ++
> environment.h | 1 +
> pager.c | 26 +++++++++++++++++---------
> 3 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index a65d575af4..975c9cb9eb 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -725,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->attributes_file = NULL;
> cfg->excludes_file = NULL;
> cfg->editor_program = NULL;
> + cfg->pager_program = NULL;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> @@ -742,4 +743,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);
> + FREE_AND_NULL(cfg->pager_program);
> }
> diff --git a/environment.h b/environment.h
> index 8178ebab76..39b6691b47 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -92,6 +92,7 @@ struct repo_config_values {
> char *attributes_file;
> char *excludes_file;
> char *editor_program;
> + char *pager_program;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
> diff --git a/pager.c b/pager.c
> index 35b210e048..bc55546670 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -5,6 +5,8 @@
> #include "run-command.h"
> #include "sigchain.h"
> #include "alias.h"
> +#include "repository.h"
> +#include "environment.h"
>
> int pager_use_color = 1;
>
> @@ -13,7 +15,6 @@ int pager_use_color = 1;
> #endif
>
> static struct child_process pager_process;
> -static char *pager_program;
> static int old_fd1 = -1, old_fd2 = -1;
>
> /* Is the value coming back from term_columns() just a guess? */
> @@ -75,10 +76,15 @@ static void wait_for_pager_signal(int signo)
>
> static int core_pager_config(const char *var, const char *value,
> const struct config_context *ctx UNUSED,
> - void *data UNUSED)
> + void *data)
Could this change behaviour that a caller expects?
(looking at the hunk below) we are now using repo_config_values() which
contains the condition 'repo != the_repository'. This means that if there
is a caller that sends anything but the_repository, it will BUG() out.
Before this patch it would have worked, it worked because callers were
sending the correct repository. Now we enforce it.
If we check the callers we can see that everyone sends the_repository,
so this new assert is fine and prevents sending submodules by mistake.
Makes sense.
I think it's worth mentioning that on the commit body/function.
> {
> - if (!strcmp(var, "core.pager"))
> - return git_config_string(&pager_program, var, value);
> + struct repository *r = data;
> +
> + if (!strcmp(var, "core.pager")) {
> + FREE_AND_NULL(repo_config_values(r)->pager_program);
> + return git_config_string(&repo_config_values(r)->pager_program, var, value);
> + }
Ok. Now that pager_program is not file-scoped we drop the UNUSED and
pager_program now lives in the per-repo field.
Then we change the address where ->pager_program (which we access through
repo_config_values()) points to.
FREE_AND_NULL() is new, before this patch it must have been leaking,
good job.
Similar to previous patches, let's change the pattern to only call
repo_config_values() once and use the pointer it returns.
> +
> return 0;
> }
>
> @@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
>
> pager = getenv("GIT_PAGER");
> if (!pager) {
> - if (!pager_program)
> + if (!repo_config_values(r)->pager_program)
> read_early_config(r,
> - core_pager_config, NULL);
> - pager = pager_program;
> + core_pager_config, r);
> + pager = repo_config_values(r)->pager_program;
Same as above, let's call repo_config_values() once.
> }
> if (!pager)
> pager = getenv("PAGER");
> @@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
>
> read_early_config(r, pager_command_config, &data);
>
> - if (data.value)
> - pager_program = data.value;
> + if (data.value) {
> + free(repo_config_values(r)->pager_program);
> + repo_config_values(r)->pager_program = data.value;
Same pattern. This also frees, but the log says "a" memory leak is fixed
in this patch, should we change it to two?
> + }
> return data.want;
> }
I peeked at later patches of this series and the multiple calls of
repo_config_values() pattern keeps appearing.
I haven't finished reviewing 5-9. If you reroll before I get to those
patches, it may be worth fixing this pattern across the whole series.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v10 5/9] environment: move askpass_program into repo_config_values
From: Pablo Sabater @ 2026-07-12 14:31 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-6-cat@malon.dev>
On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
> The global variable 'askpass_program' stores the path to the program
> used to prompt the user for credentials. Move it into repo_config_values
> to continue the libification effort.
>
> While it is uncommon for a single process to require different askpass
> programs for different repositories, maintaining this value as a mutable
> global string is a blocker for libification. Global heap-allocated
> strings introduce thread-safety issues in a multi-repo environment.
>
> Move 'askpass_program' into 'struct repo_config_values' to eliminate
> this global state. The memory is now safely managed and freed via
> '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>
> ---
> environment.c | 6 ++++--
> environment.h | 1 +
> prompt.c | 3 ++-
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 975c9cb9eb..1a26c9c6d6 100644
> --- a/environment.c
> +++ b/environment.c
I think that the drop of the global variable is missing.
> @@ -464,8 +464,8 @@ int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.askpass")) {
> - FREE_AND_NULL(askpass_program);
> - return git_config_string(&askpass_program, var, value);
> + FREE_AND_NULL(cfg->askpass_program);
> + return git_config_string(&cfg->askpass_program, var, value);
> }
>
> if (!strcmp(var, "core.excludesfile")) {
> @@ -726,6 +726,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->excludes_file = NULL;
> cfg->editor_program = NULL;
> cfg->pager_program = NULL;
> + cfg->askpass_program = NULL;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> @@ -744,4 +745,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
> FREE_AND_NULL(cfg->excludes_file);
> FREE_AND_NULL(cfg->editor_program);
> FREE_AND_NULL(cfg->pager_program);
> + FREE_AND_NULL(cfg->askpass_program);
> }
> diff --git a/environment.h b/environment.h
> index 39b6691b47..a2e9def89d 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -93,6 +93,7 @@ struct repo_config_values {
> char *excludes_file;
> char *editor_program;
> char *pager_program;
> + char *askpass_program;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
> diff --git a/prompt.c b/prompt.c
> index 706fba2a50..d8d74c7e37 100644
> --- a/prompt.c
> +++ b/prompt.c
> @@ -3,6 +3,7 @@
> #include "git-compat-util.h"
> #include "parse.h"
> #include "environment.h"
> +#include "repository.h"
> #include "run-command.h"
> #include "strbuf.h"
> #include "prompt.h"
> @@ -51,7 +52,7 @@ char *git_prompt(const char *prompt, int flags)
>
> askpass = getenv("GIT_ASKPASS");
> if (!askpass)
> - askpass = askpass_program;
> + askpass = repo_config_values(the_repository)->askpass_program;
> if (!askpass)
> askpass = getenv("SSH_ASKPASS");
> if (askpass && *askpass)
The rest LGTM.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH 1/6] SubmittingPatches: clarify expected structure of commit log message
From: Weijie Yuan @ 2026-07-12 14:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20260711192650.2417665-2-gitster@pobox.com>
On Sat, Jul 11, 2026 at 12:26:45PM -0700, Junio C Hamano wrote:
> The current text on log message has lots of justification and
> rationale before telling contributors what exactly is expected of
> them.
Nit: s/message/messages/ ?
> Simplify the rationale section and jump straight to what to write
> and how.
> [...]
> +Reviewers will evaluate your commit message for clarity and structure.
> +A well-structured commit message typically follows a three-part flow:
> +**Observation**, **Solution**, and **Command**.
>
> -. justifies the way the change solves the problem, i.e. why the
> - result with the change is better.
> -
> -. alternate solutions considered but discarded, if any.
> +[[meaningful-message]]
> +==== Structure of a Commit Message
>
> -. records the resolution of design or viability concerns raised by the
> - community during the review, if any, ensuring the historical record
> - explains why the chosen approach was accepted over alternatives.
> +0. **Title**:
> + The first line of the commit log message is the title that lets
> + readers of `git log --oneline` quickly understand what area the
> + commit touches and what problem it addresses.
>
> +1. **Observation (The Status Quo)**:
> + Explain the problem you are trying to solve. Describe what is
> + wrong with the current code *without* your change.
> ++
> [[present-tense]]
> -The problem statement that describes the status quo is written in the
> -present tense. Write "The code does X when it is given input Y",
> -instead of "The code used to do Y when given input X". You do not
> -have to say "Currently"---the status quo in the problem statement is
> -about the code _without_ your change, by project convention.
> -
> -[[imperative-mood]]
> -Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> -instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> -to do frotz", as if you are giving orders to the codebase to change
> -its behavior. Try to make sure your explanation can be understood
> -without external resources. Instead of giving a URL to a mailing list
> -archive, summarize the relevant points of the discussion.
> +Write this problem statement in the **present tense** (e.g., "The
> +code does X when given input Y", not "The code used to do Y"). The
> +status quo in the problem statement is always about the code without
> +your change, by project convention. Do not use words like
> +"Currently" to describe this state.
> +
> +2. **Solution (The Approach)**:
> + Justify the way your change solves the problem. Explain why the
> + proposed approach is better and mention any alternate solutions
> + considered and discarded.
> ++
> +If your change only addresses a subset of a larger problem (e.g.,
> +handles directories but not files because of characteristic Y),
> +explain this limitation. This helps future developers understand the
> +boundaries of your work and whether it can be safely extended.
> ++
> +If the change resolves design or viability concerns raised by the
> +community during prior review rounds, ensure the message records the
> +resolution, explaining why the chosen approach was accepted over
> +alternatives.
> +
> +3. **Command (The Instruction)**:
> + [[imperative-mood]]
> + Command the codebase to change. Write this in the **imperative
> + mood** (e.g., "make xyzzy do frotz" instead of "This patch makes
> + xyzzy do..." or "I changed xyzzy..."), as if you are giving orders
> + to the codebase to change its behavior.
Stopped and confused for a moment. I am not sure that "Command" belongs
alongside "Observation" and "Solution" as a third part of the message.
Sometimes the command still describes the solution. In other words,
Solution and Command seem not to be logically completely separable.
> +#### Formatting and Style Guidelines
Perhaps using "====" here would be in harmony with the existing content.
> +* **The Subject Line (First Line)**:
> + * Keep it short (50 characters is the soft limit).
> + * Skip the full stop at the end.
> + * Prefix the subject with the modified area followed by a colon
> + and a space (e.g., "area: subject"). The area is typically a
> + filename or identifier (e.g., `doc:`, `transport:`, `t5601:`).
> + Run `git log --no-merges` on target files to see conventions.
> + * [[summary-section]]
> + Do not capitalize the first word after the "area:" prefix unless
> + there is a specific reason (e.g., `HEAD` is always in caps).
> + E.g., use "doc: clarify...", not "doc: Clarify...".
> +
> +* **The Body**:
> + * Explain the *why* rather than repeating the *what* of the diff.
> + * Try to make the explanation self-contained. Avoid relying on
> + external URLs (like mailing list archives) as the sole
> + explanation; summarize the relevant points of the discussion
> + instead.
> + * Wrap lines to 68-72 columns.
MyFirstContribution:
This commit message is intentionally formatted to 72 columns per line
Should we update both?
btw I don't know which editors/projects have the default setting of 68.
Is it Emacs?
Thanks.
^ permalink raw reply
* Re: [PATCH v10 6/9] environment: migrate apply_default_whitespace and apply_default_ignorewhitespace
From: Pablo Sabater @ 2026-07-12 15:04 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-7-cat@malon.dev>
On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
> The global variables 'apply_default_whitespace' and
> 'apply_default_ignorewhitespace' are used to store the default
> whitespace configuration for 'git apply'. Move these variables
> into 'struct repo_config_values' to continue the libification
> effort.
>
> Dynamically allocated strings fetched via 'repo_config_get_string()'
> are now tracked per-repository and safely freed in
> 'repo_config_values_clear()'.
>
> As part of this transition, update 'git_apply_config()' to accept a
> 'struct repository *' argument rather than relying on the
> 'the_repository' global.
>
> 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>
> ---
> apply.c | 20 ++++++++++++--------
> environment.c | 6 ++++--
> environment.h | 4 ++--
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 249248d4f2..66db9b7678 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -47,11 +47,13 @@ struct gitdiff_data {
> int p_value;
> };
>
> -static void git_apply_config(void)
> +static void git_apply_config(struct repository *repo)
> {
> - repo_config_get_string(the_repository, "apply.whitespace", &apply_default_whitespace);
> - repo_config_get_string(the_repository, "apply.ignorewhitespace", &apply_default_ignorewhitespace);
> - repo_config(the_repository, git_xmerge_config, NULL);
> + repo_config_get_string(repo, "apply.whitespace",
> + &repo_config_values(repo)->apply_default_whitespace);
> + repo_config_get_string(repo, "apply.ignorewhitespace",
> + &repo_config_values(repo)->apply_default_ignorewhitespace);
Same pattern, let's call repo_config_values() once.
Also, similar to the previous patches, shouldn't be here a
FREE_AND_NULL() before each repo_config_get_string() call?
> + repo_config(repo, git_xmerge_config, NULL);
> }
>
> static int parse_whitespace_option(struct apply_state *state, const char *option)
> @@ -126,10 +128,12 @@ int init_apply_state(struct apply_state *state,
> strset_init(&state->kept_symlinks);
> strbuf_init(&state->root, 0);
>
> - git_apply_config();
> - if (apply_default_whitespace && parse_whitespace_option(state, apply_default_whitespace))
> + git_apply_config(repo);
> + if (repo_config_values(repo)->apply_default_whitespace &&
> + parse_whitespace_option(state, repo_config_values(repo)->apply_default_whitespace))
> return -1;
> - if (apply_default_ignorewhitespace && parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
> + if (repo_config_values(repo)->apply_default_ignorewhitespace &&
> + parse_ignorewhitespace_option(state, repo_config_values(repo)->apply_default_ignorewhitespace))
> return -1;
> return 0;
> }
> @@ -192,7 +196,7 @@ int check_apply_state(struct apply_state *state, int force_apply)
>
> static void set_default_whitespace_mode(struct apply_state *state)
> {
> - if (!state->whitespace_option && !apply_default_whitespace)
> + if (!state->whitespace_option && !repo_config_values(state->repo)->apply_default_whitespace)
We should extract cfg from repo_config_values() to avoid this overly
long line.
> state->ws_error_action = (state->apply ? warn_on_ws_error : nowarn_ws_error);
> }
>
> diff --git a/environment.c b/environment.c
> index 1a26c9c6d6..41ba013c86 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -49,8 +49,6 @@ int assume_unchanged;
> int is_bare_repository_cfg = -1; /* unspecified */
> char *git_commit_encoding;
> char *git_log_output_encoding;
> -char *apply_default_whitespace;
> -char *apply_default_ignorewhitespace;
> int fsync_object_files = -1;
> int use_fsync = -1;
> enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
> @@ -727,6 +725,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->editor_program = NULL;
> cfg->pager_program = NULL;
> cfg->askpass_program = NULL;
> + cfg->apply_default_whitespace = NULL;
> + cfg->apply_default_ignorewhitespace = NULL;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> @@ -746,4 +746,6 @@ void repo_config_values_clear(struct repo_config_values *cfg)
> FREE_AND_NULL(cfg->editor_program);
> FREE_AND_NULL(cfg->pager_program);
> FREE_AND_NULL(cfg->askpass_program);
> + FREE_AND_NULL(cfg->apply_default_whitespace);
> + FREE_AND_NULL(cfg->apply_default_ignorewhitespace);
> }
> diff --git a/environment.h b/environment.h
> index a2e9def89d..553f87adee 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -94,6 +94,8 @@ struct repo_config_values {
> char *editor_program;
> char *pager_program;
> char *askpass_program;
> + char *apply_default_whitespace;
> + char *apply_default_ignorewhitespace;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
> @@ -182,8 +184,6 @@ extern int has_symlinks;
> extern int minimum_abbrev, default_abbrev;
> extern int ignore_case;
> extern int assume_unchanged;
> -extern char *apply_default_whitespace;
> -extern char *apply_default_ignorewhitespace;
> extern unsigned long pack_size_limit_cfg;
>
> extern int protect_hfs;
The rest LGTM.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v10 4/9] environment: move pager_program into repo_config_values
From: Junio C Hamano @ 2026-07-12 15:36 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-5-cat@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
> The 'pager_program' variable is currently defined as a file-scoped
> static string in pager.c. Move it into 'struct repo_config_values'.
>
> The configuration parsing logic remains strictly within pager.c to
> respect subsystem boundaries. The read/write operations are simply
> redirected to the repository-specific structure using
> 'repo_config_values()'.
By redirecting to repo_config_values(r), we now enforce that the
passed repository must be 'the_repository' (due to the assertion in
repo_config_values()). All current callers of git_pager() and
check_pager_config() indeed pass 'the_repository', so this new
enforcement does not harm them. However, it paves the way to later
lift the assertion and allow us to configure different pagers for
different repositories, which is a welcome improvement.
> static int core_pager_config(const char *var, const char *value,
> const struct config_context *ctx UNUSED,
> - void *data UNUSED)
> + void *data)
> {
> - if (!strcmp(var, "core.pager"))
> - return git_config_string(&pager_program, var, value);
> + struct repository *r = data;
> +
> + if (!strcmp(var, "core.pager")) {
> + FREE_AND_NULL(repo_config_values(r)->pager_program);
> + return git_config_string(&repo_config_values(r)->pager_program, var, value);
> + }
It may be just me, but I would have preferred to see this written
more like
if (!strcmp(var, "core.pager")) {
struct repo_config_values *values = repo_config_values(r);
FREE_AND_NULL(values->pager_program);
return git_config_string(&values->pager_program, var, value);
}
which will make it easier to see that we are freeing the same thing
immediately before we overwrite it. It also shortens the lines.
For a temporary variable with a very short scope like this one that
is introduced solely for readability, it is OK to use even shorter
name like 'v' if you want to ('r' certainly has a much longer
lifespan that it, and I would probably have preferred to see it
called 'repo').
Side note: we might want to give a hint in the coding guidelines
document that a variable with larger lifespan should get longer
names, or something.
> return 0;
> }
>
> @@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
>
> pager = getenv("GIT_PAGER");
> if (!pager) {
> - if (!pager_program)
> + if (!repo_config_values(r)->pager_program)
> read_early_config(r,
> - core_pager_config, NULL);
> - pager = pager_program;
> + core_pager_config, r);
> + pager = repo_config_values(r)->pager_program;
> }
Same here.
> if (!pager)
> pager = getenv("PAGER");
> @@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
>
> read_early_config(r, pager_command_config, &data);
>
> - if (data.value)
> - pager_program = data.value;
> + if (data.value) {
> + free(repo_config_values(r)->pager_program);
> + repo_config_values(r)->pager_program = data.value;
> + }
Same here.
^ permalink raw reply
* Re: [PATCH v10 5/9] environment: move askpass_program into repo_config_values
From: Junio C Hamano @ 2026-07-12 15:47 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-6-cat@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
> environment.c | 6 ++++--
> environment.h | 1 +
> prompt.c | 3 ++-
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 975c9cb9eb..1a26c9c6d6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -464,8 +464,8 @@ int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.askpass")) {
> - FREE_AND_NULL(askpass_program);
> - return git_config_string(&askpass_program, var, value);
> + FREE_AND_NULL(cfg->askpass_program);
> + return git_config_string(&cfg->askpass_program, var, value);
> }
> @@ -726,6 +726,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->excludes_file = NULL;
> cfg->editor_program = NULL;
> cfg->pager_program = NULL;
> + cfg->askpass_program = NULL;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> @@ -744,4 +745,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
> FREE_AND_NULL(cfg->excludes_file);
> FREE_AND_NULL(cfg->editor_program);
> FREE_AND_NULL(cfg->pager_program);
> + FREE_AND_NULL(cfg->askpass_program);
> }
The 'askpass_program' global variable has been removed. Instead,
the member in repo_config_values is correctly initialized to NULL
and properly cleaned up when finished.
> diff --git a/environment.h b/environment.h
> index 39b6691b47..a2e9def89d 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -93,6 +93,7 @@ struct repo_config_values {
> char *excludes_file;
> char *editor_program;
> char *pager_program;
> + char *askpass_program;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
Wait, wasn't there an extern declaration for that global variable in
a header file somewhere? There must also be an actual definition
for it. Both should be removed to ensure no one accesses a stale
variable; doing so allows the compiler to help you spot any
leftover users.
Thanks.
^ permalink raw reply
* Re: [PATCH v10 7/9] environment: move push_default into repo_config_values
From: Pablo Sabater @ 2026-07-12 15:49 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260712111734.1073514-8-cat@malon.dev>
On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
> The global variable 'push_default' specifies the default behavior of
> 'git push' when no explicit refspec is provided. Move 'push_default'
> into 'struct repo_config_values' to continue the libification effort.
>
> While 'enum push_default_type' ideally belongs in 'remote.h', moving it
> there introduces a circular dependency chain:
>
> remote.h -> hash.h -> repository.h -> environment.h.
>
> Therefore, the enum definition is kept in 'environment.h' just above
> 'struct repo_config_values' with a NEEDSWORK comment for future cleanup.
>
> Modify the configuration parsing in environment.c to update the
> per-repository structure directly, and update caller across the
> codebase to access the value via 'repo_config_values()'.
>
> 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>
> ---
> builtin/push.c | 8 ++++----
> environment.c | 16 +++++++++-------
> environment.h | 26 ++++++++++++++++----------
> remote.c | 2 +-
> 4 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 6021b71d66..6dc3224b60 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -88,7 +88,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
> }
> }
>
> - if (push_default == PUSH_DEFAULT_UPSTREAM &&
> + if (repo_config_values(the_repository)->push_default == PUSH_DEFAULT_UPSTREAM &&
Can we extract cfg from repo_config_values() to shorten this line?
If we look at the hunk below, we can see this pattern. let's do the
same.
> skip_prefix(matched->name, "refs/heads/", &branch_name)) {
> struct branch *branch = branch_get(branch_name);
> if (branch->merge_nr == 1 && branch->merge[0]->src) {
> @@ -160,7 +160,7 @@ static NORETURN void die_push_simple(struct branch *branch,
> * Don't show advice for people who explicitly set
> * push.default.
> */
> - if (push_default == PUSH_DEFAULT_UNSPECIFIED)
> + if (cfg->push_default == PUSH_DEFAULT_UNSPECIFIED)
> advice_pushdefault_maybe = _("\n"
> "To choose either option permanently, "
> "see push.default in 'git help config'.\n");
> @@ -232,7 +232,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
> const char *dst;
> int same_remote;
>
> - switch (push_default) {
> + switch (repo_config_values(the_repository)->push_default) {
> case PUSH_DEFAULT_MATCHING:
> refspec_append(&rs, ":");
> return;
> @@ -252,7 +252,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
> dst = branch->refname;
> same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
>
> - switch (push_default) {
> + switch (repo_config_values(the_repository)->push_default) {
> default:
> case PUSH_DEFAULT_UNSPECIFIED:
> case PUSH_DEFAULT_SIMPLE:
> diff --git a/environment.c b/environment.c
> index 41ba013c86..0080012f31 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -59,7 +59,6 @@ enum eol core_eol = EOL_UNSET;
> int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
> char *check_roundtrip_encoding;
> enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
> -enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
> #ifndef OBJECT_CREATION_MODE
> #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
> #endif
> @@ -621,21 +620,23 @@ static int git_default_branch_config(const char *var, const char *value)
>
> static int git_default_push_config(const char *var, const char *value)
> {
> + struct repo_config_values *cfg = repo_config_values(the_repository);
> +
> if (!strcmp(var, "push.default")) {
> if (!value)
> return config_error_nonbool(var);
> else if (!strcmp(value, "nothing"))
> - push_default = PUSH_DEFAULT_NOTHING;
> + cfg->push_default = PUSH_DEFAULT_NOTHING;
> else if (!strcmp(value, "matching"))
> - push_default = PUSH_DEFAULT_MATCHING;
> + cfg->push_default = PUSH_DEFAULT_MATCHING;
> else if (!strcmp(value, "simple"))
> - push_default = PUSH_DEFAULT_SIMPLE;
> + cfg->push_default = PUSH_DEFAULT_SIMPLE;
> else if (!strcmp(value, "upstream"))
> - push_default = PUSH_DEFAULT_UPSTREAM;
> + cfg->push_default = PUSH_DEFAULT_UPSTREAM;
> else if (!strcmp(value, "tracking")) /* deprecated */
> - push_default = PUSH_DEFAULT_UPSTREAM;
> + cfg->push_default = PUSH_DEFAULT_UPSTREAM;
> else if (!strcmp(value, "current"))
> - push_default = PUSH_DEFAULT_CURRENT;
> + cfg->push_default = PUSH_DEFAULT_CURRENT;
> else {
> error(_("malformed value for %s: %s"), var, value);
> return error(_("must be one of nothing, matching, simple, "
> @@ -727,6 +728,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->askpass_program = NULL;
> cfg->apply_default_whitespace = NULL;
> cfg->apply_default_ignorewhitespace = NULL;
> + cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> diff --git a/environment.h b/environment.h
> index 553f87adee..6a5c8bd06f 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -87,6 +87,21 @@ extern const char * const local_repo_env[];
> struct strvec;
>
> struct repository;
> +
> +/*
> + * NEEDSWORK: It would be better if these definitions could be moved to
> + * other more specific files, but care is needed to avoid circular
> + * inclusion issues.
> + */
> +enum push_default_type {
> + PUSH_DEFAULT_NOTHING = 0,
> + PUSH_DEFAULT_MATCHING,
> + PUSH_DEFAULT_SIMPLE,
> + PUSH_DEFAULT_UPSTREAM,
> + PUSH_DEFAULT_CURRENT,
> + PUSH_DEFAULT_UNSPECIFIED
> +};
> +
> struct repo_config_values {
> /* section "core" config values */
> char *attributes_file;
> @@ -96,6 +111,7 @@ struct repo_config_values {
> char *askpass_program;
> char *apply_default_whitespace;
> char *apply_default_ignorewhitespace;
> + enum push_default_type push_default;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
> @@ -197,16 +213,6 @@ enum rebase_setup_type {
> };
> extern enum rebase_setup_type autorebase;
>
> -enum push_default_type {
> - PUSH_DEFAULT_NOTHING = 0,
> - PUSH_DEFAULT_MATCHING,
> - PUSH_DEFAULT_SIMPLE,
> - PUSH_DEFAULT_UPSTREAM,
> - PUSH_DEFAULT_CURRENT,
> - PUSH_DEFAULT_UNSPECIFIED
> -};
> -extern enum push_default_type push_default;
> -
> enum object_creation_mode {
> OBJECT_CREATION_USES_HARDLINKS = 0,
> OBJECT_CREATION_USES_RENAMES = 1
> diff --git a/remote.c b/remote.c
> index 00723b385e..d48c01d375 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1933,7 +1933,7 @@ static char *branch_get_push_1(struct repository *repo,
> if (remote->mirror)
> return tracking_for_push_dest(remote, branch->refname, err);
>
> - switch (push_default) {
> + switch (repo_config_values(repo)->push_default) {
> case PUSH_DEFAULT_NOTHING:
> return error_buf(err, _("push has no destination (push.default is 'nothing')"));
>
I haven't checked how doable fixing the dependency cycle of the
NEEDSWORK is.
Similar to other patches where we add repo_config_values() to functions
that didn't have it before. We need to check that there's no caller that
could pass a repository different from the_repository. I haven't checked
it this time.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH 1/6] SubmittingPatches: clarify expected structure of commit log message
From: Junio C Hamano @ 2026-07-12 16:07 UTC (permalink / raw)
To: Weijie Yuan; +Cc: git
In-Reply-To: <alOplirhJxIkpDYh@wyuan.org>
Weijie Yuan <wy@wyuan.org> writes:
>> +2. **Solution (The Approach)**:
>> +3. **Command (The Instruction)**:
>> + [[imperative-mood]]
>> + Command the codebase to change. Write this in the **imperative
>> + mood** (e.g., "make xyzzy do frotz" instead of "This patch makes
>> + xyzzy do..." or "I changed xyzzy..."), as if you are giving orders
>> + to the codebase to change its behavior.
>
> Stopped and confused for a moment. I am not sure that "Command" belongs
> alongside "Observation" and "Solution" as a third part of the message.
> Sometimes the command still describes the solution. In other words,
> Solution and Command seem not to be logically completely separable.
I do not think "Command the codebase to change" is a good phrasing.
It would have been better to highlight the distinction between the
design of the solution (approach) and the implementation. Perhaps
2. Design (The Approach)
3. Implementation (The Changes)
[[imperative-mood]]
Describe how the change is implemented. Write this in the
imperative mood. ...
or something?
>> +#### Formatting and Style Guidelines
>
> Perhaps using "====" here would be in harmony with the existing content.
Indeed.
>> +* **The Body**:
>> + * Explain the *why* rather than repeating the *what* of the diff.
>> + * Try to make the explanation self-contained. Avoid relying on
>> + external URLs (like mailing list archives) as the sole
>> + explanation; summarize the relevant points of the discussion
>> + instead.
>> + * Wrap lines to 68-72 columns.
>
> MyFirstContribution:
> This commit message is intentionally formatted to 72 columns per line
>
> Should we update both?
Perhaps just to stick to "around 70".
I do not think the defaults in various editors matter.
The "wrap around 70 columns" rule exists so that in a text based
email exchange, where you lose two columns to leading "> " when
quoted, and an additional column with each subsequent reply, the
lines will still fit on standard 80-column terminals.
Thanks.
^ permalink raw reply
* Re: [PATCH v10 4/9] environment: move pager_program into repo_config_values
From: Tian Yuchen @ 2026-07-12 16:54 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <DJWNK8BOFIAW.3VVAROHMKVDWE@gmail.com>
On 7/12/26 22:12, Pablo Sabater wrote:
> On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
>> The 'pager_program' variable is currently defined as a file-scoped
>> static string in pager.c. Move it into 'struct repo_config_values'.
>>
>> The configuration parsing logic remains strictly within pager.c to
>> respect subsystem boundaries. The read/write operations are simply
>> redirected to the repository-specific structure using
>> 'repo_config_values()'.
>>
>> Similar to the recent editor_program migration, no standalone getter
>> is introduced to keep the code minimal. The dynamically allocated
>> memory is now managed by 'repo_config_values_clear()'.
>>
>> On top of that, fix a memory leak in pager.c while we are at it.
>>
>> 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 | 2 ++
>> environment.h | 1 +
>> pager.c | 26 +++++++++++++++++---------
>> 3 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index a65d575af4..975c9cb9eb 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -725,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
>> cfg->attributes_file = NULL;
>> cfg->excludes_file = NULL;
>> cfg->editor_program = NULL;
>> + cfg->pager_program = NULL;
>> cfg->apply_sparse_checkout = 0;
>> cfg->branch_track = BRANCH_TRACK_REMOTE;
>> cfg->trust_ctime = 1;
>> @@ -742,4 +743,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);
>> + FREE_AND_NULL(cfg->pager_program);
>> }
>> diff --git a/environment.h b/environment.h
>> index 8178ebab76..39b6691b47 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -92,6 +92,7 @@ struct repo_config_values {
>> char *attributes_file;
>> char *excludes_file;
>> char *editor_program;
>> + char *pager_program;
>> int apply_sparse_checkout;
>> int trust_ctime;
>> int check_stat;
>> diff --git a/pager.c b/pager.c
>> index 35b210e048..bc55546670 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -5,6 +5,8 @@
>> #include "run-command.h"
>> #include "sigchain.h"
>> #include "alias.h"
>> +#include "repository.h"
>> +#include "environment.h"
>>
>> int pager_use_color = 1;
>>
>> @@ -13,7 +15,6 @@ int pager_use_color = 1;
>> #endif
>>
>> static struct child_process pager_process;
>> -static char *pager_program;
>> static int old_fd1 = -1, old_fd2 = -1;
>>
>> /* Is the value coming back from term_columns() just a guess? */
>> @@ -75,10 +76,15 @@ static void wait_for_pager_signal(int signo)
>>
>> static int core_pager_config(const char *var, const char *value,
>> const struct config_context *ctx UNUSED,
>> - void *data UNUSED)
>> + void *data)
>
> Could this change behaviour that a caller expects?
>
> (looking at the hunk below) we are now using repo_config_values() which
> contains the condition 'repo != the_repository'. This means that if there
> is a caller that sends anything but the_repository, it will BUG() out.
>
> Before this patch it would have worked, it worked because callers were
> sending the correct repository. Now we enforce it.
>
> If we check the callers we can see that everyone sends the_repository,
> so this new assert is fine and prevents sending submodules by mistake.
>
> Makes sense.
>
;)
> I think it's worth mentioning that on the commit body/function.
>
Okay, will add a line to briefly explain this. Something like what Junio
said:
All current callers of git_pager() and
check_pager_config() indeed pass 'the_repository', so this new
enforcement does not harm them.
>> {
>> - if (!strcmp(var, "core.pager"))
>> - return git_config_string(&pager_program, var, value);
>> + struct repository *r = data;
>> +
>> + if (!strcmp(var, "core.pager")) {
>> + FREE_AND_NULL(repo_config_values(r)->pager_program);
>> + return git_config_string(&repo_config_values(r)->pager_program, var, value);
>> + }
>
> Ok. Now that pager_program is not file-scoped we drop the UNUSED and
> pager_program now lives in the per-repo field.
> Then we change the address where ->pager_program (which we access through
> repo_config_values()) points to.
>
> FREE_AND_NULL() is new, before this patch it must have been leaking,
> good job.
>
> Similar to previous patches, let's change the pattern to only call
> repo_config_values() once and use the pointer it returns.
>
>> +
>> return 0;
>> }
>>
>> @@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
>>
>> pager = getenv("GIT_PAGER");
>> if (!pager) {
>> - if (!pager_program)
>> + if (!repo_config_values(r)->pager_program)
>> read_early_config(r,
>> - core_pager_config, NULL);
>> - pager = pager_program;
>> + core_pager_config, r);
>> + pager = repo_config_values(r)->pager_program;
>
> Same as above, let's call repo_config_values() once.
>
>> }
>> if (!pager)
>> pager = getenv("PAGER");
>> @@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
>>
>> read_early_config(r, pager_command_config, &data);
>>
>> - if (data.value)
>> - pager_program = data.value;
>> + if (data.value) {
>> + free(repo_config_values(r)->pager_program);
>> + repo_config_values(r)->pager_program = data.value;
>
> Same pattern. This also frees, but the log says "a" memory leak is fixed
> in this patch, should we change it to two?
Nice catch.
>
>> + }
>> return data.want;
>> }
>
> I peeked at later patches of this series and the multiple calls of
> repo_config_values() pattern keeps appearing.
> I haven't finished reviewing 5-9. If you reroll before I get to those
> patches, it may be worth fixing this pattern across the whole series.
>
> Regards,
> Pablo
For those cases where multiple calls to repo_config_values() appear in a
single function body, I will fix such pattern by using cfg.
Thanks, yuchen
^ permalink raw reply
* Re: [PATCH v10 4/9] environment: move pager_program into repo_config_values
From: Tian Yuchen @ 2026-07-12 16:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, pabloosabaterr, cirnovskyv, szeder.dev, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <xmqqy0fg43vf.fsf@gitster.g>
On 7/12/26 23:36, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
>
>> The 'pager_program' variable is currently defined as a file-scoped
>> static string in pager.c. Move it into 'struct repo_config_values'.
>>
>> The configuration parsing logic remains strictly within pager.c to
>> respect subsystem boundaries. The read/write operations are simply
>> redirected to the repository-specific structure using
>> 'repo_config_values()'.
>
> By redirecting to repo_config_values(r), we now enforce that the
> passed repository must be 'the_repository' (due to the assertion in
> repo_config_values()). All current callers of git_pager() and
> check_pager_config() indeed pass 'the_repository', so this new
> enforcement does not harm them. However, it paves the way to later
> lift the assertion and allow us to configure different pagers for
> different repositories, which is a welcome improvement.
>
Exactly.
>> static int core_pager_config(const char *var, const char *value,
>> const struct config_context *ctx UNUSED,
>> - void *data UNUSED)
>> + void *data)
>> {
>> - if (!strcmp(var, "core.pager"))
>> - return git_config_string(&pager_program, var, value);
>> + struct repository *r = data;
>> +
>> + if (!strcmp(var, "core.pager")) {
>> + FREE_AND_NULL(repo_config_values(r)->pager_program);
>> + return git_config_string(&repo_config_values(r)->pager_program, var, value);
>> + }
>
> It may be just me, but I would have preferred to see this written
> more like
>
>
> if (!strcmp(var, "core.pager")) {
> struct repo_config_values *values = repo_config_values(r);
>
> FREE_AND_NULL(values->pager_program);
> return git_config_string(&values->pager_program, var, value);
> }
>
I see. I think it's better to name the struct 'cfg' so that it is
consistent with what we did before.
> which will make it easier to see that we are freeing the same thing
> immediately before we overwrite it. It also shortens the lines.
> For a temporary variable with a very short scope like this one that
> is introduced solely for readability, it is OK to use even shorter
> name like 'v' if you want to ('r' certainly has a much longer
> lifespan that it, and I would probably have preferred to see it
> called 'repo').
>
> Side note: we might want to give a hint in the coding guidelines
> document that a variable with larger lifespan should get longer
> names, or something.
>
>> return 0;
>> }
>>
>> @@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
>>
>> pager = getenv("GIT_PAGER");
>> if (!pager) {
>> - if (!pager_program)
>> + if (!repo_config_values(r)->pager_program)
>> read_early_config(r,
>> - core_pager_config, NULL);
>> - pager = pager_program;
>> + core_pager_config, r);
>> + pager = repo_config_values(r)->pager_program;
>> }
>
> Same here.
>
>> if (!pager)
>> pager = getenv("PAGER");
>> @@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
>>
>> read_early_config(r, pager_command_config, &data);
>>
>> - if (data.value)
>> - pager_program = data.value;
>> + if (data.value) {
>> + free(repo_config_values(r)->pager_program);
>> + repo_config_values(r)->pager_program = data.value;
>> + }
>
> Same here.
Thanks, will change all these parts.
Regard, yuchen
^ permalink raw reply
* Re: [PATCH v9 0/4] graph: indent visual roots in graph
From: Pablo Sabater @ 2026-07-12 16:59 UTC (permalink / raw)
To: Mirko Faina, Chandra Pratap
Cc: Pablo Sabater, git, ayu.chandekar, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31
In-Reply-To: <alOOXKGIB8BqACxR@exploit>
On Sun Jul 12, 2026 at 3:10 PM CEST, Mirko Faina wrote:
> On Sun, Jul 12, 2026 at 11:26:27AM +0530, Chandra Pratap wrote:
>> 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.
>
> In that case, together with --[no]-graph-indent, a configuration
> variable like "graph.indent" could be introduced to reduce the usage of
> --[no]-graph-indent for those that would like to retain the old
> behaviour for most formats.
>
>> > > 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.
>
> While a zig-zag pattern might be ambiguous, what I proposed is a little
> different.
>
> What I proposed is effectively a wrapping for anything that goes beyond
> two levels of indentation. I don't think it would look anything like a
> fork/merge pattern.
>
> * A
> * B
> * C
> * D
> * E
> * F
>
> The difference between two indentation levels and no indentation is very
> noticeble, I don't think anyone confused this. This would fix the
> staircase pattern on adjacent one-patch series.
I agree that having an infinite stair is not a good solution. the 3
column wrap looks reasonable.
I see two cases with this wrap:
1. No conflict case:
A
B
C
D
E
F
No ambiguity, this would be the ideal case.
2. Ambiguity:
If it happens that the visual number on visual roots meet the condition
(number_of_visual_roots % 3 == 0) and the next commit is NOT a visual
root this would happen:
A
B
C
D
E
E
Which would be ambiguous. The solution is to check with the lookahead
buffer that we have since patch 3 if the next is a visual root, if it's
not we indent D anyway:
A
B
C
D
E
E
Which I find the pyramid effect uncomfortable.
What about capping at 4 columns?
1.
A
B
C
D
E
F
G
H
2.
A
B
C
D
E
F
F
I prefer the 4 column wrap because it looks more abrupt and IMO shows
better that the commits are unrelated.
What do you think?
Also, about the no-opt option "--no-graph-indent" is still wanted
regardless of the final design that we choose?
Thanks,
Pablo
^ 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