* [PATCH v2 0/2] t9811: reformat and modernize tests
From: Marcelo Machado Lage @ 2026-07-11 16:04 UTC (permalink / raw)
To: git; +Cc: Marcelo Machado Lage
In-Reply-To: <20260702140704.65805-1-marcelomlage@usp.br>
This patch series reformats and modernizes the t9811 tests.
Changes since v1:
- Break long && chains into multiple lines according to how git tests are
written nowadays. This was suggested by Patrick Steinhardt.
- Replace 'test -f' calls by more useful 'test_path_*' helpers as the
second commit in the series.
Marcelo Machado Lage (2):
t9811: break long && chains into multiple lines
t9811: replace 'test -f' and '! test -f' with 'test_path_*'
t/t9811-git-p4-label-import.sh | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
Range-diff against v1:
-: ---------- > 1: 0f03c913eb t9811: break long && chains into multiple lines
1: f319f2e6e7 ! 2: 3e590881c3 t9811: replace 'test -f' and '! test -f' with 'test_path_*'
@@ Commit message
Replace the basic shell commands 'test -f', with more modern test
helpers 'test_path_is_file' and 'test_path_is_missing'.
+ These modern helpers emit useful information when the corresponding
+ tests fail, unlike 'test -f' and '! test -f'.
+
+ The occurrences of '! test -f filename' were replaced by
+ 'file_path_is_missing filename', a stronger guarantee equivalent to
+ '! test -e filename'.
+
+ Co-authored-by: Vinicius Lira de Freitas <vinilira@usp.br>
+ Signed-off-by: Vinicius Lira de Freitas <vinilira@usp.br>
+ Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
## t/t9811-git-p4-label-import.sh ##
@@ t/t9811-git-p4-label-import.sh: test_expect_success 'basic p4 labels' '
@@ t/t9811-git-p4-label-import.sh: test_expect_success 'basic p4 labels' '
- ! test -f f2 &&
+ test_path_is_missing f2 &&
git checkout TAG_WITH\$_SHELL_CHAR &&
-- test -f f1 && test -f f2 && test -f file_with_\$metachar &&
-+ test_path_is_file f1 && test_path_is_file f2 && test_path_is_file file_with_\$metachar &&
+- test -f f1 &&
+- test -f f2 &&
+- test -f file_with_\$metachar &&
++ test_path_is_file f1 &&
++ test_path_is_file f2 &&
++ test_path_is_file file_with_\$metachar &&
git show TAG_LONG_LABEL | grep -q "A Label second line"
)
--
2.34.1
^ permalink raw reply
* [PATCH v2 1/2] t9811: break long && chains into multiple lines
From: Marcelo Machado Lage @ 2026-07-11 16:04 UTC (permalink / raw)
To: git; +Cc: Marcelo Machado Lage, Vinicius Lira de Freitas, Junio C Hamano
In-Reply-To: <20260711160447.99708-1-marcelomlage@usp.br>
Rewrite single-line && chains by breaking them into multiple lines.
Co-authored-by: Vinicius Lira de Freitas <vinilira@usp.br>
Signed-off-by: Vinicius Lira de Freitas <vinilira@usp.br>
Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
---
t/t9811-git-p4-label-import.sh | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 7614dfbd95..072bc88210 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -64,7 +64,9 @@ test_expect_success 'basic p4 labels' '
git checkout TAG_F1_ONLY &&
! test -f f2 &&
git checkout TAG_WITH\$_SHELL_CHAR &&
- test -f f1 && test -f f2 && test -f file_with_\$metachar &&
+ test -f f1 &&
+ test -f f2 &&
+ test -f file_with_\$metachar &&
git show TAG_LONG_LABEL | grep -q "A Label second line"
)
@@ -231,17 +233,25 @@ test_expect_success 'importing labels with missing revisions' '
P4CLIENT=missing-revision &&
client_view "//depot/missing-revision/... //missing-revision/..." &&
cd "$cli" &&
- >f1 && p4 add f1 && p4 submit -d "start" &&
+ >f1 &&
+ p4 add f1 &&
+ p4 submit -d "start" &&
p4 tag -l TAG_S0 ... &&
- >f2 && p4 add f2 && p4 submit -d "second" &&
+ >f2 &&
+ p4 add f2 &&
+ p4 submit -d "second" &&
startrev=$(p4_head_revision //depot/missing-revision/...) &&
- >f3 && p4 add f3 && p4 submit -d "third" &&
+ >f3 &&
+ p4 add f3 &&
+ p4 submit -d "third" &&
- p4 edit f2 && date >f2 && p4 submit -d "change" f2 &&
+ p4 edit f2 &&
+ date >f2 &&
+ p4 submit -d "change" f2 &&
endrev=$(p4_head_revision //depot/missing-revision/...) &&
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/2] t9811: replace 'test -f' and '! test -f' with 'test_path_*'
From: Marcelo Machado Lage @ 2026-07-11 16:04 UTC (permalink / raw)
To: git; +Cc: Marcelo Machado Lage, Vinicius Lira de Freitas, Junio C Hamano
In-Reply-To: <20260711160447.99708-1-marcelomlage@usp.br>
Replace the basic shell commands 'test -f', with more modern test
helpers 'test_path_is_file' and 'test_path_is_missing'.
These modern helpers emit useful information when the corresponding
tests fail, unlike 'test -f' and '! test -f'.
The occurrences of '! test -f filename' were replaced by
'file_path_is_missing filename', a stronger guarantee equivalent to
'! test -e filename'.
Co-authored-by: Vinicius Lira de Freitas <vinilira@usp.br>
Signed-off-by: Vinicius Lira de Freitas <vinilira@usp.br>
Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
---
t/t9811-git-p4-label-import.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh
index 072bc88210..866d7b597b 100755
--- a/t/t9811-git-p4-label-import.sh
+++ b/t/t9811-git-p4-label-import.sh
@@ -62,11 +62,11 @@ test_expect_success 'basic p4 labels' '
cd main &&
git checkout TAG_F1_ONLY &&
- ! test -f f2 &&
+ test_path_is_missing f2 &&
git checkout TAG_WITH\$_SHELL_CHAR &&
- test -f f1 &&
- test -f f2 &&
- test -f file_with_\$metachar &&
+ test_path_is_file f1 &&
+ test_path_is_file f2 &&
+ test_path_is_file file_with_\$metachar &&
git show TAG_LONG_LABEL | grep -q "A Label second line"
)
@@ -104,11 +104,11 @@ test_expect_success 'two labels on the same changelist' '
git checkout TAG_F1_1 &&
ls &&
- test -f f1 &&
+ test_path_is_file f1 &&
git checkout TAG_F1_2 &&
ls &&
- test -f f1
+ test_path_is_file f1
)
'
@@ -137,9 +137,9 @@ test_expect_success 'export git tags to p4' '
p4 labels ... | grep LIGHTWEIGHT_TAG &&
p4 label -o GIT_TAG_1 | grep "tag created in git:xyzzy" &&
p4 sync ...@GIT_TAG_1 &&
- ! test -f main/f10 &&
+ test_path_is_missing main/f10 &&
p4 sync ...@GIT_TAG_2 &&
- test -f main/f10
+ test_path_is_file main/f10
)
'
@@ -170,9 +170,9 @@ test_expect_success 'export git tags to p4 with deletion' '
cd "$cli" &&
p4 sync ... &&
p4 sync ...@GIT_TAG_ON_DELETED &&
- test -f main/deleted_file &&
+ test_path_is_file main/deleted_file &&
p4 sync ...@GIT_TAG_AFTER_DELETION &&
- ! test -f main/deleted_file &&
+ test_path_is_missing main/deleted_file &&
echo "checking label contents" &&
p4 label -o GIT_TAG_ON_DELETED | grep "tag on deleted file"
)
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v9 0/9] migrate more variables into repo_config_values
From: Tian Yuchen @ 2026-07-11 16:11 UTC (permalink / raw)
To: Pablo Sabater, git; +Cc: cirnovskyv, szeder.dev
In-Reply-To: <DJVUGL8XA0Y0.12LN2COXI5BIY@gmail.com>
Hi Pablo,
On 7/11/26 23:24, Pablo Sabater wrote:
> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>> Hi everyone,
>>
>> This patch series continues the ongoing libification effort by migrating
>> a batch of global configuration variables into struct repo_config_values.
>>
>> What does this series do:
>>
>> infrastructure & strings (commits 1-6):
>> Introduce 'repo_config_values_clear()' to manage the lifecycle
>> of heap-allocated configuration strings. This infrastructure is utilized
>> to migrate string variables, including 'excludes_file', 'apply' whitespace
>> configs, and external programs including 'editor', 'pager', 'askpass'.
>>
>> enums (commits 7-9):
>> Migrate enumerations 'push_default', 'autorebase', and
>> 'object_creation_mode'. Care was taken to make these types available
>> to the configuration structure without triggering circular header
>> dependencies.
>>
>> RFC:
>>
>> Commit 3~5. Is it really necessary to migrate _program variables?
>> https://lore.kernel.org/git/8e657184-ee0b-453a-9f2d-a98080d3582e@gmail.com/
>>
>> Commit 6~9. Previous related discussions on 'git_branch_track'.
>> https://lore.kernel.org/git/CAD=f0L-mPX+KECUjXk-WBzEbTP7wCa8sB56GySQT0yh9mfUOWw@mail.gmail.com/
>>
>> Note:
>>
>> Since a new getter 'repo_excludes_file()' is introduced, as previously
>> promised, once it is finally merged into 'master', there will be a patch to
>> update and squash the comments.
>>
>> Similarly, I've noticed that the classification and sorting of variables in
>> 'repo_config_values' don't seem to be correct. There will also be a patch
>> to fix this, and I think it will form a commit series along with the comment
>> patch?
>>
>> Change since v8:
>>
>> Fixed a memory leak in pager.c.
>>
>> Thanks!
>>
>> Tian Yuchen (9):
>> repository: introduce repo_config_values_clear()
>> environment: move excludes_file into repo_config_values
>> environment: move editor_program into repo_config_values
>> environment: move pager_program into repo_config_values
>> environment: move askpass_program into repo_config_values
>> environment: migrate apply_default_whitespace and
>> apply_default_ignorewhitespace
>> environment: move push_default into repo_config_values
>> environment: move autorebase into repo_config_values
>> environment: move object_creation_mode into repo_config_values
>>
>> apply.c | 20 +++++++-----
>> branch.c | 2 +-
>> builtin/push.c | 8 ++---
>> dir.c | 4 +--
>> editor.c | 4 +--
>> environment.c | 87 +++++++++++++++++++++++++++++++++++---------------
>> environment.h | 75 +++++++++++++++++++++++++++----------------
>> object-file.c | 2 +-
>> pager.c | 26 +++++++++------
>> prompt.c | 3 +-
>> remote.c | 2 +-
>> repository.c | 1 +
>> 12 files changed, 152 insertions(+), 82 deletions(-)
>
> Hi!
>
> I missed a base-commit to easily apply this locally, could we
> add one?
>
> Thanks!
> Pablo
Thanks for pointing out.
The base commit is 8d96f09e9245ddf80c1981476fcbac8c4bb4125f.
I will put it on the cover letter in the next reroll (if any)!
Regards, yuchen
^ permalink raw reply
* Re: [PATCH v9 0/4] graph: indent visual roots in graph
From: Mirko Faina @ 2026-07-11 16:25 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, krka, peff, phillip.wood,
siddharthasthana31, Mirko Faina
In-Reply-To: <DJVUU76PUXR4.2BYRTA8SEEBVC@gmail.com>
On Sat, Jul 11, 2026 at 05:41:58PM +0200, Pablo Sabater wrote:
> I think that this solves an ambiguity so it should be the default option
> and someone who doesn't want the indentation has to explicitly unset it
> maybe with something like '--no-graph-indent'.
The reason I prefer the current way of printing as the default is
because the ambiguity arises only when each commit occupies exactly one
line. In any other case we can clearly see the edges connecting the
vertices. I'd rather have --oneline imply what would be --graph-indent
instead of having to pass --no-graph-indent on any other format
different from --oneline or --format=reference.
> 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.
^ permalink raw reply
* Re: [PATCH v2 9/8?] pack-objects: drop unused return value from add_object_entry()
From: Junio C Hamano @ 2026-07-11 16:42 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git, Justin Tobler
In-Reply-To: <20260711075811.GC1457061@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jul 10, 2026 at 10:48:52AM +0200, Patrick Steinhardt wrote:
>
>> The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
>> 2026-07-06) with ps/odb-drop-whence at 8a7ad23e11 (odb: document object
>> info fields, 2026-07-02) merged into it.
>
> Here's a patch doing the cleanup I proposed upthread.
>
> -- >8 --
> Subject: pack-objects: drop unused return value from add_object_entry()
>
> This function returns 0/1 to its caller to tell them whether we actually
> added a new entry (or if we considered it redundant). But nobody has
> relied on that behavior since 5379a5c5ee (Thin pack generation:
> optimization., 2006-04-05).
>
> The extra return does not hurt much, but it recently became a bit more
> confusing. We have a sister function, add_object_entry_from_bitmap(),
> which had the same return value semantics. That function recently
> changed to always return 0 (not void, because it must conform to a
> callback function interface). So now we have two related functions which
> both return an "int" but with different semantics.
>
> Let's drop the unused "int" return from add_object_entry() entirely,
> which makes it more clear that the two functions have diverged.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I couldn't reference the commit by its id, since Junio has not yet
> picked up the v2 sent a few hours ago. ;)
Heh, if you do intend to make this a part of the series as 9/8, you
can just say "earlier in the series" with its title, and that should
be sufficient to identify which patch, as I never make a
fast-forward merge when merging topics into integration branches
(which means that Michael's "git when-merged" works well).
And if we ever see v3 of this series, you and Patrick can work
together to see if it makes sense to squash it in, or move it
earlier in a series to as preliminary clean-up, etc.
Thanks. I agree with the reasoning upthread that led to this
change.
^ permalink raw reply
* Re: [PATCH v9 1/9] repository: introduce repo_config_values_clear()
From: Pablo Sabater @ 2026-07-11 17:21 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260709161145.13349-2-cat@malon.dev>
On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
> As part of the ongoing libification effort, dynamically allocated
> global configuration variables are being moved into
> 'struct repo_config_values'. To prevent memory leaks, we need a
> destructor to free these heap-allocated variables when a repository
> instance is torn down.
>
> Introduce 'repo_config_values_clear()' in environment.c and invoke it
> from 'repo_clear()' in repository.c. As a starting point, update this
> new function to handle the cleanup of 'attributes_file'.
Makes sense.
>
> Note:
>
> Submodules are currently not supported by repo_config_values(), which
> explicitly BUG()s out if 'repo != the_repository'. Since repo_clear()
> cleans up all repository instances, we must bypass them to prevent
> crashing.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
> environment.c | 19 +++++++++++++++++++
> environment.h | 9 +++++++++
> repository.c | 1 +
> 3 files changed, 29 insertions(+)
>
> diff --git a/environment.c b/environment.c
> index ba2c60103f..13677484de 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -726,3 +726,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->sparse_expect_files_outside_of_patterns = 0;
> cfg->warn_on_object_refname_ambiguity = 1;
> }
> +
> +void repo_config_values_clear(struct repository *repo)
> +{
> + struct repo_config_values *cfg;
> +
> + /*
> + * NEEDSWORK: Submodules are currently not supported by
> + * repo_config_values(), which explicitly BUG()s out if
> + * repo != the_repository. Since repo_clear() cleans up all
> + * repository instances, we must bypass them here to prevent
> + * crashing.
> + */
> + if (repo != the_repository)
> + return;
> +
> + cfg = repo_config_values(repo);
> +
> + FREE_AND_NULL(cfg->attributes_file);
> +}
> diff --git a/environment.h b/environment.h
> index 6f18286955..c4a6a45704 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
>
> void repo_config_values_init(struct repo_config_values *cfg);
>
> +/*
> + * Frees memory allocated for dynamically loaded configuration values
> + * inside `repo_config_values`.
> + *
> + * As dynamically allocated variables are migrated into this struct,
> + * their FREE_AND_NULL() calls should be appended here.
> + */
> +void repo_config_values_clear(struct repository *repo);
I think that I'm not comfortable having the _init() and the _clear()
functions with different signatures.
_clear() takes struct repository to dodge a BUG().
I would like to have both signatures equal, why can't we just do directly:
void repo_config_values_clear(struct repo_config_values *cfg)
{
FREE_AND_NULL(cfg->attributes_file);
}
and call from repo_clear():
repo_config_values_clear(&repo->config_values_private_)
I get that the workaround might be to not access directly to
&repo->config_values_private_ which repo_config_values() returns but for
example initialize_repository() access this _private_ field directly as
well.
Even with the NEEDSWORK it is a silent return, what will happen when
submodules are supported? If no one remembers to change it we will leak
the submodules silently.
Also at repo_init(), initialize_repository() sets repo->initialized
before anything can fail and call repo_clear() but
repo_config_values_clear() should be able to free attributes_file even
just after a memset() (which happens before initialize_repository()).
But calling repo_config_values has a BUG() in case of
!repo->initialized are we comfortable with this assert?
> +
> /*
> * TODO: All the below state either explicitly or implicitly relies on
> * `the_repository`. We should eventually get rid of these and make the
> diff --git a/repository.c b/repository.c
> index 187dd471c4..b31f1b7852 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
> FREE_AND_NULL(repo->parsed_objects);
>
> repo_settings_clear(repo);
> + repo_config_values_clear(repo);
>
> if (repo->config) {
> git_configset_clear(repo->config);
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v9 2/9] environment: move excludes_file into repo_config_values
From: Pablo Sabater @ 2026-07-11 18:21 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260709161145.13349-3-cat@malon.dev>
On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
> The global variable 'excludes_file' is used to track the path to the
> global ignore file. If this variable is NULL,
> 'setup_standard_excludes()'
Nit: Strange line break here.
> in 'dir.c' forcefully evaluates and assigns the XDG default path to it.
>
> Continue the libification effort by encapsulating this lazy-loading
> fallback logic into a proper getter and moving the variable into
> 'struct repo_config_values'.
>
> Since 'excludes_file' is a dynamically allocated string, it requires
> proper heap memory management. It is safely freed using the newly
> introduced `repo_config_values_clear()` function when the repository
> is torn down.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
> dir.c | 4 ++--
> environment.c | 15 ++++++++++++---
> environment.h | 4 +++-
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 7a73690fbc..4f87a52b3c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -3481,11 +3481,11 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
>
> void setup_standard_excludes(struct dir_struct *dir)
> {
> + const char *excludes_file = repo_excludes_file(the_repository);
> +
> dir->exclude_per_dir = ".gitignore";
>
> /* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
> - if (!excludes_file)
> - excludes_file = xdg_config_home("ignore");
> if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
> add_patterns_from_file_1(dir, excludes_file,
> dir->untracked ? &dir->internal.ss_excludes_file : NULL);
> diff --git a/environment.c b/environment.c
> index 13677484de..5950592d63 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -57,7 +57,6 @@ enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
> enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
> char *editor_program;
> char *askpass_program;
> -char *excludes_file;
> enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
> enum eol core_eol = EOL_UNSET;
> int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
> @@ -134,6 +133,14 @@ int is_bare_repository(void)
> return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
> }
>
> +const char *repo_excludes_file(struct repository *repo)
> +{
> + if (!repo_config_values(repo)->excludes_file)
> + repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
> +
> + return repo_config_values(repo)->excludes_file;
> +}
repo_config_values() returns a pointer so there should be no need to
call the function 3 times.
We could have the function be called once and use it then:
const char *repo_excludes_file(struct repository *repo)
{
struct repo_config_values *cfg = repo_config_values(repo);
if (!cfg->excludes_file)
cfg->excludes_file = xdg_config_home("ignore");
return cfg->excludes_file;
}
> +
> int have_git_dir(void)
> {
> return startup_info->have_repository
> @@ -461,8 +468,8 @@ int git_default_core_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "core.excludesfile")) {
> - FREE_AND_NULL(excludes_file);
> - return git_config_pathname(&excludes_file, var, value);
> + FREE_AND_NULL(cfg->excludes_file);
> + return git_config_pathname(&cfg->excludes_file, var, value);
> }
>
> if (!strcmp(var, "core.whitespace")) {
> @@ -715,6 +722,7 @@ int git_default_config(const char *var, const char *value,
> void repo_config_values_init(struct repo_config_values *cfg)
> {
> cfg->attributes_file = NULL;
> + cfg->excludes_file = NULL;
> cfg->apply_sparse_checkout = 0;
> cfg->branch_track = BRANCH_TRACK_REMOTE;
> cfg->trust_ctime = 1;
> @@ -744,4 +752,5 @@ void repo_config_values_clear(struct repository *repo)
> cfg = repo_config_values(repo);
>
> FREE_AND_NULL(cfg->attributes_file);
> + FREE_AND_NULL(cfg->excludes_file);
> }
> diff --git a/environment.h b/environment.h
> index c4a6a45704..2e8352de7f 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -90,6 +90,7 @@ struct repository;
> struct repo_config_values {
> /* section "core" config values */
> char *attributes_file;
> + char *excludes_file;
> int apply_sparse_checkout;
> int trust_ctime;
> int check_stat;
> @@ -133,6 +134,8 @@ int git_default_config(const char *, const char *,
> int git_default_core_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
>
> +const char *repo_excludes_file(struct repository *repo);
> +
> void repo_config_values_init(struct repo_config_values *cfg);
>
> /*
> @@ -217,7 +220,6 @@ extern char *git_log_output_encoding;
>
> extern char *editor_program;
> extern char *askpass_program;
> -extern char *excludes_file;
>
> /*
> * The character that begins a commented line in user-editable file
The rest looks fine.
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v9 1/9] repository: introduce repo_config_values_clear()
From: Tian Yuchen @ 2026-07-11 18:35 UTC (permalink / raw)
To: Pablo Sabater, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <DJVWYOJNU0IW.1M107L4ABH54V@gmail.com>
Hii Pablo,
On 7/12/26 01:21, Pablo Sabater wrote:
> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>> As part of the ongoing libification effort, dynamically allocated
>> global configuration variables are being moved into
>> 'struct repo_config_values'. To prevent memory leaks, we need a
>> destructor to free these heap-allocated variables when a repository
>> instance is torn down.
>>
>> Introduce 'repo_config_values_clear()' in environment.c and invoke it
>> from 'repo_clear()' in repository.c. As a starting point, update this
>> new function to handle the cleanup of 'attributes_file'.
>
> Makes sense.
>
>>
>> Note:
>>
>> Submodules are currently not supported by repo_config_values(), which
>> explicitly BUG()s out if 'repo != the_repository'. Since repo_clear()
>> cleans up all repository instances, we must bypass them to prevent
>> crashing.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>> ---
>> environment.c | 19 +++++++++++++++++++
>> environment.h | 9 +++++++++
>> repository.c | 1 +
>> 3 files changed, 29 insertions(+)
>>
>> diff --git a/environment.c b/environment.c
>> index ba2c60103f..13677484de 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -726,3 +726,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
>> cfg->sparse_expect_files_outside_of_patterns = 0;
>> cfg->warn_on_object_refname_ambiguity = 1;
>> }
>> +
>> +void repo_config_values_clear(struct repository *repo)
>> +{
>> + struct repo_config_values *cfg;
>> +
>> + /*
>> + * NEEDSWORK: Submodules are currently not supported by
>> + * repo_config_values(), which explicitly BUG()s out if
>> + * repo != the_repository. Since repo_clear() cleans up all
>> + * repository instances, we must bypass them here to prevent
>> + * crashing.
>> + */
>> + if (repo != the_repository)
>> + return;
>> +
>> + cfg = repo_config_values(repo);
>> +
>> + FREE_AND_NULL(cfg->attributes_file);
>> +}
>> diff --git a/environment.h b/environment.h
>> index 6f18286955..c4a6a45704 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -135,6 +135,15 @@ int git_default_core_config(const char *var, const char *value,
>>
>> void repo_config_values_init(struct repo_config_values *cfg);
>>
>> +/*
>> + * Frees memory allocated for dynamically loaded configuration values
>> + * inside `repo_config_values`.
>> + *
>> + * As dynamically allocated variables are migrated into this struct,
>> + * their FREE_AND_NULL() calls should be appended here.
>> + */
>> +void repo_config_values_clear(struct repository *repo);
>
> I think that I'm not comfortable having the _init() and the _clear()
> functions with different signatures.
>
> _clear() takes struct repository to dodge a BUG().
>
> I would like to have both signatures equal, why can't we just do directly:
>
> void repo_config_values_clear(struct repo_config_values *cfg)
> {
> FREE_AND_NULL(cfg->attributes_file);
> }
>
> and call from repo_clear():
>
> repo_config_values_clear(&repo->config_values_private_)
>
I particularly agree with your point that the signatures of these two
functions should be consistent. I missed it tbh...I will change it in
the next reroll.
However, I think it makes more sense to refactor to pass in 'struct
repository', which is consistent with repo-settings.
> I get that the workaround might be to not access directly to
> &repo->config_values_private_ which repo_config_values() returns but for
> example initialize_repository() access this _private_ field directly as
> well.
Now that we have used the _private_ suffix, if we can just define a
_clear() to bypass the assertion of repo_config_values(), wouldn't this
be self-deception? I'm not saying that the original lines are
necessarily correct... but I do think that semantically speaking, it is
inappropriate to pass in config_values_private_ to _clear().
> Also at repo_init(), initialize_repository() sets repo->initialized
> before anything can fail and call repo_clear() but
> repo_config_values_clear() should be able to free attributes_file even
> just after a memset() (which happens before initialize_repository()).
> But calling repo_config_values has a BUG() in case of
> !repo->initialized are we comfortable with this assert?
This goes back to the previous topic: Who is responsible for the call to
_clear()? Who is responsible for filtering all those invalid usage of
repo instances? Faced with a repo instance that was not initialized but
was handed over to _clear() in some way, we have two concepts:
- It doesn't matter. Since we always handle config_values_private_, it's
NULL at this point, so we don't BUG() and continue.
- The very existence of such a repo is a mistake. It shouldn't have
appeared and shouldn't have been passed on to me. However, since this
situation is relatively common at this point, we choose to return
instead of BUG()ing it directly to temporarily avoid it. We will
gradually tighten the conditions. When the invalid calls are eliminated
in the end, such checks will no longer exist.
Our consensus should at least be that this 'use of uninitialized repos'
is bad, so they are just two different ways to solve unexpected
situations. However, in my opinion, the difference between these two
concepts lies in whether we are consciously moving the assertion
downward. I think the latter line of thinking does this better.
>
> Even with the NEEDSWORK it is a silent return, what will happen when
> submodules are supported? If no one remembers to change it we will leak
> the submodules silently.
>
I will remember to change it ;)
Regards, yuchen
>> +
>> /*
>> * TODO: All the below state either explicitly or implicitly relies on
>> * `the_repository`. We should eventually get rid of these and make the
>> diff --git a/repository.c b/repository.c
>> index 187dd471c4..b31f1b7852 100644
>> --- a/repository.c
>> +++ b/repository.c
>> @@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
>> FREE_AND_NULL(repo->parsed_objects);
>>
>> repo_settings_clear(repo);
>> + repo_config_values_clear(repo);
>>
>> if (repo->config) {
>> git_configset_clear(repo->config);
>
> Regards,
> Pablo
^ permalink raw reply
* Re: [PATCH v7 3/9] environment: move editor_program into repo_config_values
From: Pablo Sabater @ 2026-07-11 19:05 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260706142530.3681520-4-cat@malon.dev>
On Mon Jul 6, 2026 at 4:25 PM CEST, Tian Yuchen wrote:
> 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.
Nit:
"As the codebase moves toward becoming 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()'.
Why no getter function here? in patch 2/9 repo_excludes_file() is
introduced and also access the field via repo_config_values().
Super nit: on a previous commit you used backquotes for
repo_config_values_clear() but now you're using single quotes.
>
> 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..07d264cba0 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 && repo_config_values(the_repository)->editor_program)
> + editor = repo_config_values(the_repository)->editor_program;
Same as the previous patch, we can store repo_config_values() and avoid
re-calling the function.
Also, do we need the right-side condition?
> if (!editor && !terminal_is_dumb)
> editor = getenv("VISUAL");
> if (!editor)
> diff --git a/environment.c b/environment.c
> index 5950592d63..0a01f4761a 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;
> @@ -435,8 +434,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") ||
> @@ -723,6 +722,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;
> @@ -753,4 +753,5 @@ void repo_config_values_clear(struct repository *repo)
>
> 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 2e8352de7f..1ec19149cb 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;
>
> /*
Regards,
Pablo
^ permalink raw reply
* [PATCH 0/6] Update Contributor Guides
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
I have been tracking the rules I follow while updating the "What's
cooking" draft, which guides my daily work, and noticed a few gaps
in our contributor documentation.
* We often tell contributors how commit log messages should look on
the mailing list, but the language in `SubmittingPatches` is too
wordy. The first patch in this series shortens it to get to the
point earlier.
* We recently updated `MyFirstContribution` to advise contributors
to pace themselves when they find mistakes or receive feedback.
However, we lack instructions for when a patch receives no
reaction. The second patch addresses this gap.
* There seems to be some confusion regarding when contributors should
add `Reviewed-by:` and `Acked-by:` trailers. The third patch
clarifies this process.
* We want to ensure contributors don't walk away once their patch lands
in `seen`, as that is merely the beginning of the story. The fourth
and fifth patches clarify this point.
* An experimental feature in `SubmittingPatches` invites contributors
to draft the description for their topic in the "What's cooking"
report. However, instead of outlining the expected tone, we simply
told them to emulate existing entries. The final patch remedies this.
1/6: SubmittingPatches: clarify expected structure of commit log message
2/6: MyFirstContribution: what if I don't get a reply?
3/6: MyFirstContribution: carrying over trailers
4/6: MyFirstContribution: clarify that 'seen' does not mean acceptance
5/6: SubmittingPatches: clarify the meaning of "Will queue"
6/6: SubmittingPatches: clarify the writing style of whats-cooking
Documentation/MyFirstContribution.adoc | 53 +++++++-
Documentation/SubmittingPatches | 171 +++++++++++++------------
2 files changed, 135 insertions(+), 89 deletions(-)
--
2.55.0-391-gdf86bf5712
^ permalink raw reply
* [PATCH 1/6] SubmittingPatches: clarify expected structure of commit log message
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
The current text on log message has lots of justification and
rationale before telling contributors what exactly is expected of
them.
Simplify the rationale section and jump straight to what to write
and how.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 140 +++++++++++++++-----------------
1 file changed, 65 insertions(+), 75 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index d2d82eb543..12f9660cef 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -289,86 +289,76 @@ run `git diff --check` on your changes before you commit.
The log message that explains your changes is just as important as the
changes themselves. Your code may be clearly written with in-code
-comment to sufficiently explain how it works with the surrounding
-code, but those who need to fix or enhance your code in the future
-will need to know _why_ your code does what it does, for a few
-reasons:
-
-. Your code may be doing something differently from what you wanted it
- to do. Writing down what you actually wanted to achieve will help
- them fix your code and make it do what it should have been doing
- (also, you often discover your own bugs yourself, while writing the
- log message to summarize the thought behind it).
-
-. Your code may be doing things that were only necessary for your
- immediate needs (e.g. "do X to directories" without implementing or
- even designing what is to be done on files). Writing down why you
- excluded what the code does not do will help guide future developers.
- Writing down "we do X to directories, because directories have
- characteristic Y" would help them infer "oh, files also have the same
- characteristic Y, so perhaps doing X to them would also make sense?".
- Saying "we don't do the same X to files, because ..." will help them
- decide if the reasoning is sound (in which case they do not waste
- time extending your code to cover files), or reason differently (in
- which case, they can explain why they extend your code to cover
- files, too).
-
-The goal of your log message is to convey the _why_ behind your change
-to help future developers. The reviewers will also make sure that
-your proposed log message will serve this purpose well.
-
-The first line of the commit message should be a short description (50
-characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
-and should skip the full stop. It is also conventional in most cases to
-prefix the first line with "area: " where the area is a filename or
-identifier for the general area of the code being modified, e.g.
-
-* doc: clarify distinction between sign-off and pgp-signing
-* githooks.txt: improve the intro section
-
-If in doubt which identifier to use, run `git log --no-merges` on the
-files you are modifying to see the current conventions.
-
-[[summary-section]]
-The title sentence after the "area:" prefix omits the full stop at the
-end, and its first word is not capitalized (the omission
-of capitalization applies only to the word after the "area:"
-prefix of the title) unless there is a reason to
-capitalize it other than because it is the first word in the sentence.
-E.g. "doc: clarify...", not "doc: Clarify...", or "githooks.txt:
-improve...", not "githooks.txt: Improve...". But "refs: HEAD is also
-treated as a ref" is correct, as we spell `HEAD` in all caps even when
-it appears in the middle of a sentence.
+comments, but future developers need to know *why* your code does what
+it does. The goal of your log message is to convey the intent and
+rationales behind your changes.
-[[meaningful-message]]
-The body should provide a meaningful commit message, which:
-
-. explains the problem the change tries to solve, i.e. what is wrong
- with the current code without the change.
+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.
+
+#### Formatting and Style Guidelines
+
+* **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.
[[commit-reference]]
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* [PATCH 2/6] MyFirstContribution: what if I don't get a reply?
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
Tell readers that pinging is a perfectly sensible thing to do when
they do not see a response.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/MyFirstContribution.adoc | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
index 4832e5bad5..fc2ce2e785 100644
--- a/Documentation/MyFirstContribution.adoc
+++ b/Documentation/MyFirstContribution.adoc
@@ -1438,6 +1438,19 @@ substantial rework, and mention which parts of the current series will become
obsolete so reviewers can avoid spending time on them until the updated series
is ready.
+=== What if I don't get a reply?
+
+If you don't receive any review comments after a week or two, do not
+assume your patch has been accepted or merged. In the Git project,
+silence does not equal approval. It usually means reviewers are busy
+or haven't noticed your contribution.
+
+If your patch is overlooked, it is perfectly acceptable to send a
+polite ping to the thread. You can do this by replying to your own
+cover letter (or patch) to ask if anyone has had a chance to look at
+it. You can also CC additional people who might be interested; use
+the `git-contacts` script (mentioned earlier) to find relevant contributors.
+
[[reviewing]]
=== Responding to Reviews
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* [PATCH 3/6] MyFirstContribution: carrying over trailers
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
The maintainer will usually collect and add Reviewed-by and Acked-by
trailers on the receiving end, but there are occasions when
contributors can carry them over from previous iterations to the new
iteration they are sending out.
Document how this procedure works and how it helps the maintainer.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/MyFirstContribution.adoc | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
index fc2ce2e785..988f0d4fba 100644
--- a/Documentation/MyFirstContribution.adoc
+++ b/Documentation/MyFirstContribution.adoc
@@ -1509,6 +1509,28 @@ changing history, but since it's local history which you haven't shared with
anyone, that is okay for now! (Later, it may not make sense to do this; take a
look at the section below this one for some context.)
+=== Handling trailers in subsequent versions
+
+If a reviewer replies with an `Acked-by: Real Name <email>` trailer,
+carry it forward when preparing v2:
+
+- If your v2 changes are minor (e.g., fixing typos or making small
+ style tweaks) and do not affect the reviewed logic, add their
+ trailer to the commit message of the updated patch. This lets the
+ maintainer know that the patch has received favorable review.
+
+- If your v2 contains significant logic changes or rewrites to address
+ feedback, do *not* carry over the trailer, as the reviewer has not
+ seen the new logic yet. Mention in your cover letter that you made
+ changes that require re-review.
+
+The rule for the `Reviewed-by:` trailer is more strict: you generally
+should not carry it over to a new iteration unless you are resending
+the patch without any change. For example, a new iteration of a patch
+series might update other patches while leaving the reviewed patch
+that received the `Reviewed-by:` trailer untouched.
+
+
[[after-approval]]
=== After Review Approval
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* [PATCH 4/6] MyFirstContribution: clarify that 'seen' does not mean acceptance
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
Document that getting a patch picked up into 'seen' is not the end
of the story for contributors; it is merely the beginning.
This is also described in SubmittingPatches:[[patch-flow]] section,
but beneficial to make new contributors aware of it early.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/MyFirstContribution.adoc | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
index 988f0d4fba..5acc265589 100644
--- a/Documentation/MyFirstContribution.adoc
+++ b/Documentation/MyFirstContribution.adoc
@@ -1534,10 +1534,20 @@ that received the `Reviewed-by:` trailer untouched.
[[after-approval]]
=== After Review Approval
-The Git project has four integration branches: `seen`, `next`, `master`, and
-`maint`. Your change will be placed into `seen` fairly early on by the maintainer
-while it is still in the review process; from there, when it is ready for wider
-testing, it will be merged into `next`. Plenty of early testers use `next` and
+The Git project maintains four integration branches: `seen`, `next`,
+`master`, and `maint`. The maintainer will often place your change
+into `seen` fairly early in the review process; sometimes even before
+it receives its first comments.
+
+However, being queued in `seen` does not mean your patch has been
+accepted. It is only there for integration testing, CI, and giving
+wider exposure and ready access to reviewers. To advance from `seen`
+to `next`, your topic needs positive reviews and community consensus
+on the mailing list. If reviews are favorable, the maintainer will
+mark the topic as "Will merge to `next`" in the "What's cooking"
+report before actually merging it.
+
+Plenty of early testers use `next` and
may report issues. Eventually, changes in `next` will make it to `master`,
which is typically considered stable. Finally, when a new release is cut,
`maint` is used to base bugfixes onto. As mentioned at the beginning of this
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* [PATCH 5/6] SubmittingPatches: clarify the meaning of "Will queue"
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
Document that "Will queue" contributors get is merely a promise to
put the topic in 'seen' and has no other meaning.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 12f9660cef..0a80358703 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -104,7 +104,9 @@ of review.
branch, in order to make it easier for people to play with it
without having to pick up and apply the patches to their trees
themselves. Being in `seen` has no other meaning. Specifically, it
- does not mean the patch was "accepted" in any way.
+ does not mean the patch was "accepted" in any way. The maintainer
+ may reply with "Will queue" when choosing to add the patches to
+ `seen`, but it does not mean the patch has been "accepted", either.
. When the discussion reaches a consensus that the latest iteration of
the patches are in good enough shape, the maintainer includes the
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* [PATCH 6/6] SubmittingPatches: clarify the writing style of whats-cooking
From: Junio C Hamano @ 2026-07-11 19:26 UTC (permalink / raw)
To: git
In-Reply-To: <20260711192650.2417665-1-gitster@pobox.com>
Unlike commit log messages, that use present tense to make
observations of the current code, and imperative mood to describe
what changes the commit makes, entries in the whats-cooking report
are written mostly in past or present perfect tense to report what
has been done.
Spell it out for contributors.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0a80358703..8917cc3805 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -714,17 +714,26 @@ line via `git format-patch --notes`.
When sending a topic, you can optionally propose a topic name and/or a
one-paragraph summary that should appear in the "What's cooking"
-report when it is picked up to explain the topic. If you choose to do
-so, please write a 2-5 line paragraph that will fit well in our
-release notes (see many bulleted entries in the
+report when it is picked up to explain the topic.
+
+If you choose to do so, please write a 2-5 line paragraph that will
+fit well in our release notes (see many bulleted entries in the
Documentation/RelNotes/* files for examples), and make it the first
(or second, if including a suggested topic name) paragraph of the
-cover letter. If suggesting a topic name, use the format
-"XX/your-topic-name", where "XX" is a stand-in for the primary
-author's initials, and "your-topic-name" is a brief, dash-delimited
-description of what your topic does. For a single-patch series, use
-the space between the three-dash line and the diffstat, as described
-earlier.
+cover letter.
+
+If suggesting a topic name, use the format "XX/your-topic-name", where
+"XX" is a stand-in for the primary author's initials, and
+"your-topic-name" is a brief, dash-delimited description of what your
+topic does. For a single-patch series, use the space between the
+three-dash line and the diffstat, as described earlier.
+
+TIP: When proposing a topic summary in your cover letter, write it in
+the reporting style (passive voice, past or present perfect tense
+describing the change as completed, e.g., "The XYZ subsystem has
+been updated to...") rather than the imperative mood, like you do
+in the proposed commit log messages. This matches the format
+used in the "What's cooking" report and release notes.
[[multi-series-efforts]]
If your patch series is part of a larger effort spanning multiple
--
2.55.0-391-gdf86bf5712
^ permalink raw reply related
* Re: [PATCH v9 3/9] environment: move editor_program into repo_config_values
From: Pablo Sabater @ 2026-07-11 19:30 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260709161145.13349-4-cat@malon.dev>
[Resending review against v9, I accidentally sent the review in-reply-to
v7. The patch seems to not have changed and it applies to v9 as well.]
On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
> 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.
Nit:
"As the codebase moves toward becoming 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()'.
Why no getter function here? in patch 2/9 repo_excludes_file() is
introduced and also access the field via repo_config_values().
Super nit: on a previous commit you used backquotes for
repo_config_values_clear() but now you're using single quotes.
>
> 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..07d264cba0 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 && repo_config_values(the_repository)->editor_program)
> + editor = repo_config_values(the_repository)->editor_program;
Same as the previous patch, we can store repo_config_values() and avoid
re-calling the function.
Also, do we need the right-side condition?
> if (!editor && !terminal_is_dumb)
> editor = getenv("VISUAL");
> if (!editor)
> diff --git a/environment.c b/environment.c
> index 5950592d63..0a01f4761a 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;
> @@ -435,8 +434,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") ||
> @@ -723,6 +722,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;
> @@ -753,4 +753,5 @@ void repo_config_values_clear(struct repository *repo)
>
> 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 2e8352de7f..1ec19149cb 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;
>
> /*
Regards,
Pablo
^ permalink raw reply
* Re: [PATCH v18 5/7] branch: add --delete-merged <branch>
From: Harald Nordgren @ 2026-07-11 19:36 UTC (permalink / raw)
To: phillip.wood
Cc: Harald Nordgren via GitGitGadget, git, Kristoffer Haugsbakk,
Johannes Sixt, Phillip Wood
In-Reply-To: <CAHwyqnU0ifHu0+GfMR9GqWKgFrTOyQn-FbUH0wTm_07nCa26tA@mail.gmail.com>
> > > + (
> > > + cd repo &&
> > > + git checkout -b mainline main &&
> > > + git checkout -b on-local mainline &&
> > > + git branch --set-upstream-to=mainline on-local &&
> >
> > Why do we need on-local to track mainline rather than main? I'm a bit
> > confused what the point of mainline is.
>
> It's to have an indirection of a branch that is the same as main but
> will be protected. I tried to delete it now and replace it with just
> main, but then main was deleted and subsequent tests failed.
Digging more into this, probably the most elegant solution is to
replace mainline with main, but then also do this:
git config branch.main.pushRemote origin
This exposes something that I don't love about this feature, which is
that when using a pushDefault (like we do in the tests with 'git
config remote.pushDefault fork') if not adding a special case for the
main/master branch (like 'git config branch.main.pushRemote origin'),
then it will get cleaned up as a forked branch.
But there was a lot of discussion about this already, so I won't get
into this again.
Harald
^ permalink raw reply
* Re: [PATCH v7 3/9] environment: move editor_program into repo_config_values
From: Pablo Sabater @ 2026-07-11 19:38 UTC (permalink / raw)
To: Tian Yuchen, git
Cc: cirnovskyv, szeder.dev, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <DJVZ5QPXBFY8.VBJN0TB3WROC@gmail.com>
Please ignore this. I replied to v7 by mistake. I've resent it correctly at:
https://lore.kernel.org/git/DJVZP8E2GS7C.1X325XFFFZ6WR@gmail.com/#t
Pablo
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Ian Jackson @ 2026-07-11 19:58 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Colin Stagner, git, Johannes Schindelin
In-Reply-To: <CALnO6CAPMEjVsj-5X9VyUtGM1JipXj6g_0JrC5gk37s178G02A@mail.gmail.com>
D. Ben Knoble writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]"):
> Just to make sure I understand you (I regularly use -X subtree with
> one project): the Rust rewrite won't support -X subtree merges, but we
> don't intend to discourage folks from using -X subtree merges in toto,
> right? Merely not support a mix of the 2?
Precisely so.
I think you may find my git-subtree rewrite superior in ergonomics to
git merge -X subtree so you might want to switch to it, when it exists
and supports that transition.
I haven't yet thought about how that transition ought to go but I
think it might look like what I'm calling an "unmarked subtree
merge~. I've made a TODO note in my working branch to remind myself
to consider this situation.
Regards,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: Understanding why Git defaults to show author date and not committer date
From: Junio C Hamano @ 2026-07-11 20:54 UTC (permalink / raw)
To: Jeff King; +Cc: Omri Sarig, git
In-Reply-To: <20260711080331.GB1470749@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jul 10, 2026 at 05:08:11PM +0200, Omri Sarig wrote:
>
>> I understand the distinction between the 2 formats, and I can see the utility of
>> both. I'm curious about the decision to show the author date and not the
>> committer date as default one in Git commands.
>> Are there some workflows where the author date is more relevant, or is that
>> mostly a legacy decision?
>>
>> I'd be interested in hearing about workflows where the author date is the more
>> useful one, as I use the committer date almost always.
>
> In a workflow based on mailing patches, the committer date is usually
> much less interesting. It is "when the maintainer happened to pick up
> your patch", as opposed to when you wrote it. Likewise, we show the
> author's name by default, not the committer's.
True. In mailing list workflow, the author date recorded is usually
the date that the patch was sent to the mailing list, which may be
later than when you wrote it, but is much more relevant as that is
closer to the time when anybody other than the author have seen the
patch for the first time.
^ permalink raw reply
* Re: [PATCH] fixup! fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Junio C Hamano @ 2026-07-11 20:55 UTC (permalink / raw)
To: Ramsay Jones; +Cc: GIT Mailing-list
In-Reply-To: <387a34d5-fdf5-4513-9aaf-4e73d9304c1d@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Junio,
>
> You have probably already noticed, but just in case, I had to fixup commit df9481e6d6
> ("fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal", 2026-07-10)
> in order to get a clean test-suite run on the 'seen' branch (@a77a48500d) this afternoon.
>
> [this is part of the 'pz/fetch-submodule-errors-config' branch].
Thanks. I am being lazy and expecting the author to send in a fix
before my next integration cycle ;-)
> - grep "Errors during submodule fetch" err
> + test_grep "Errors during submodule fetch" err
^ permalink raw reply
* Re: [PATCH v6 00/10] commit-reach: terminate merge-base walk when one side is exhausted
From: Junio C Hamano @ 2026-07-11 20:58 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Elijah Newren, Kristofer Karlsson,
René Scharfe, SZEDER Gábor
In-Reply-To: <pull.2149.v6.git.1783776466.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Changes since v5:
>
> * Rebased on next, which now contains kk/commit-reach-find-all-fix. The
> gen_ordered guard from that topic is carried through patches 7-9 via
> state.gen_ordered, then removed in patch 10 along with the date-ordering
> fallback.
As always, do *not* base your patches on 'next'. I cannot apply
such a patch series to my tree, as merging the resulting topic down
to 'master' will pull _all_ the other topics, including those that
are not ready, plus commits that merge these topics into 'next',
into 'master'.
Instead, choose the topics that you do depend on, prepare a merge of
these branches into a stable base (like v2.55.0 or master), and then
build your series on top.
Thanks.
^ permalink raw reply
* Re: [PATCH v9 0/9] migrate more variables into repo_config_values
From: Junio C Hamano @ 2026-07-11 21:06 UTC (permalink / raw)
To: Pablo Sabater; +Cc: Tian Yuchen, git, cirnovskyv, szeder.dev
In-Reply-To: <DJVUGL8XA0Y0.12LN2COXI5BIY@gmail.com>
"Pablo Sabater" <pabloosabaterr@gmail.com> writes:
> On Thu Jul 9, 2026 at 6:11 PM CEST, Tian Yuchen wrote:
>> ...
> Hi!
>
> I missed a base-commit to easily apply this locally, could we
> add one?
>
> Thanks!
> Pablo
FYI, a topic that is in 'seen' can be extracted from my tree by
inspecting "git log --oneline origin/master..origin/seen" and
finding the commit that merges the series.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox