[PATCH 0/4] mark symbols that are unnecessarily public static

[PATCH 0/4] mark symbols that are unnecessarily public static

[Junio C Hamano]

Over the years, refactoring various subsystems, adding new callers,
and rewriting them from scratch, made some symbols that started as
static into global, and some other symbols that started as global,
no longer need to be visible outside a file that use them.  Here is
a small patch series that turns them back into file-scope static.

Honestly, I suspect that [3/4] was probably better with a forward
declaration in the preview I sent earlier, but in the longer term,
readers would not care if this conversion ended up in a large code
movement, as they do not have to even know it.

 1/4: ident: remove unused author_ident_sufficiently_given()
 2/4: add -i: hide init/clear_add_i_state() functions
 3/4: read-cache: hide refresh_cache_entry()
 4/4: unpack-trees: hide verify_uptodate()

 add-interactive.c |  11 +-
 add-interactive.h |   9 --
 ident.c           |   5 -
 ident.h           |   1 -
 read-cache-ll.h   |   2 -
 read-cache.c      | 258 +++++++++++++++++++++++-----------------------
 unpack-trees.c    |   4 +-
 unpack-trees.h    |   3 -
 8 files changed, 139 insertions(+), 154 deletions(-)

-- 
2.56.0-486-gd49ba3b80a

[PATCH 1/4] ident: remove unused author_ident_sufficiently_given()

[Junio C Hamano]

A pair of functions {author,committer}_ident_sufficiently_given()
were created to replace user_ident_explicitly_given() in d6991ceedc
(ident: keep separate "explicit" flags for author and committer,
2012-11-14).

But author_ident_sufficiently_given() is never used in our history.
We only check if committer ident is sufficiently given and use it to
nudge the user to set user.{name,email} configuration variables.

Remove the unused author_ident_sufficiently_given() function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ident.c | 5 -----
 ident.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/ident.c b/ident.c
index 0b7aacecd7..eb73d073d8 100644
--- a/ident.c
+++ b/ident.c
@@ -602,11 +602,6 @@ int committer_ident_sufficiently_given(void)
 	return ident_is_sufficient(committer_ident_explicitly_given);
 }
 
-int author_ident_sufficiently_given(void)
-{
-	return ident_is_sufficient(author_ident_explicitly_given);
-}
-
 static int set_ident(const char *var, const char *value)
 {
 	if (!strcmp(var, "author.name")) {
diff --git a/ident.h b/ident.h
index 3c03403879..7e60e36415 100644
--- a/ident.h
+++ b/ident.h
@@ -60,7 +60,6 @@ const char *fmt_ident(const char *name, const char *email,
 const char *fmt_name(enum want_ident);
 
 int committer_ident_sufficiently_given(void);
-int author_ident_sufficiently_given(void);
 
 struct config_context;
 int git_ident_config(const char *, const char *, const struct config_context *,
-- 
2.56.0-486-gd49ba3b80a

Re: [PATCH 1/4] ident: remove unused author_ident_sufficiently_given()

[Jeff King]

On Fri, Oct 30, 2026 at 10:39:53AM -0700, Junio C Hamano wrote:

> A pair of functions {author,committer}_ident_sufficiently_given()
> were created to replace user_ident_explicitly_given() in d6991ceedc
> (ident: keep separate "explicit" flags for author and committer,
> 2012-11-14).
> 
> But author_ident_sufficiently_given() is never used in our history.
> We only check if committer ident is sufficiently given and use it to
> nudge the user to set user.{name,email} configuration variables.
> 
> Remove the unused author_ident_sufficiently_given() function.

Makes sense. I think I must have just added it for symmetry with the
committer one, but I agree it was never used. It almost looks like we
could drop author_ident_explicitly_given, too, which appears to be
write-only after this patch. But it is somewhat subtly used via
set_env_if() and prepare_fallback_ident(). So this seems like an obvious
cleanup and a good stopping point.

-Peff

Re: [PATCH 1/4] ident: remove unused author_ident_sufficiently_given()

[Patrick Steinhardt]

On Fri, Oct 30, 2026 at 10:39:53AM -0700, Junio C Hamano wrote:
> A pair of functions {author,committer}_ident_sufficiently_given()
> were created to replace user_ident_explicitly_given() in d6991ceedc

Nit, not worth a reroll, but isn't it "was created" as it is a singular
pair? Might just be me not being a native speaker though.

Patrick

Re: [PATCH 1/4] ident: remove unused author_ident_sufficiently_given()

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Oct 30, 2026 at 10:39:53AM -0700, Junio C Hamano wrote:
>> A pair of functions {author,committer}_ident_sufficiently_given()
>> were created to replace user_ident_explicitly_given() in d6991ceedc
>
> Nit, not worth a reroll, but isn't it "was created" as it is a singular
> pair? Might just be me not being a native speaker though.

Neither am I a native speaker, but "Syntactic vs Notional Agreement"
is what makes both valid.

Treating the "pair" as a singular subject is the strictly formal
choice preferred by most academic and traditional style guides.

Even though "pair" is grammatically singular, it represents two
distinct things, and in this particular case, we are retiring one of
them while keeping the other, treating the "pair" as a separable
two.

[PATCH 2/4] add -i: hide init/clear_add_i_state() functions

[Junio C Hamano]

The "add -i" machinery is encapsulated in run_add_i() function,
which uses init_add_i_state() to initialize the machinery, does its
thing, and clear_add_i_state() to tear it down.  External callers
have no need to call anything other than run_add_i(), starting at
d51b61f5da (add-patch: remove dependency on "add-interactive"
subsystem, 2026-03-02) refactored the code paths involved.

Make init/clear_add_i_state() file-scope static functions, and
"struct add_i_state" type as well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 add-interactive.c | 11 ++++++++---
 add-interactive.h |  9 ---------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 3cf8a1dbf8..9a00daf9f9 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -19,14 +19,19 @@
 #include "prompt.h"
 #include "tree.h"
 
-void init_add_i_state(struct add_i_state *s, struct repository *r,
-		      struct interactive_options *opts)
+struct add_i_state {
+	struct repository *r;
+	struct interactive_config cfg;
+};
+
+static void init_add_i_state(struct add_i_state *s, struct repository *r,
+			     struct interactive_options *opts)
 {
 	s->r = r;
 	interactive_config_init(&s->cfg, r, opts);
 }
 
-void clear_add_i_state(struct add_i_state *s)
+static void clear_add_i_state(struct add_i_state *s)
 {
 	interactive_config_clear(&s->cfg);
 	memset(s, 0, sizeof(*s));
diff --git a/add-interactive.h b/add-interactive.h
index eefa2edc7c..47f67ede82 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -6,15 +6,6 @@
 struct pathspec;
 struct repository;
 
-struct add_i_state {
-	struct repository *r;
-	struct interactive_config cfg;
-};
-
-void init_add_i_state(struct add_i_state *s, struct repository *r,
-		      struct interactive_options *opts);
-void clear_add_i_state(struct add_i_state *s);
-
 int run_add_i(struct repository *r, const struct pathspec *ps,
 	      struct interactive_options *opts);
 
-- 
2.56.0-486-gd49ba3b80a

Re: [PATCH 2/4] add -i: hide init/clear_add_i_state() functions

[Patrick Steinhardt]

On Fri, Oct 30, 2026 at 10:39:54AM -0700, Junio C Hamano wrote:
> The "add -i" machinery is encapsulated in run_add_i() function,
> which uses init_add_i_state() to initialize the machinery, does its
> thing, and clear_add_i_state() to tear it down.  External callers
> have no need to call anything other than run_add_i(), starting at
> d51b61f5da (add-patch: remove dependency on "add-interactive"
> subsystem, 2026-03-02) refactored the code paths involved.

This sentence reads a bit odd. Probably missing a "which"?

Patrick

Re: [PATCH 2/4] add -i: hide init/clear_add_i_state() functions

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Oct 30, 2026 at 10:39:54AM -0700, Junio C Hamano wrote:
>> The "add -i" machinery is encapsulated in run_add_i() function,
>> which uses init_add_i_state() to initialize the machinery, does its
>> thing, and clear_add_i_state() to tear it down.  External callers
>> have no need to call anything other than run_add_i(), starting at
>> d51b61f5da (add-patch: remove dependency on "add-interactive"
>> subsystem, 2026-03-02) refactored the code paths involved.
>
> This sentence reads a bit odd. Probably missing a "which"?
>
> Patrick

Thanks.  ", which refactored the code paths..." is what it should
have said.

[PATCH 3/4] read-cache: hide refresh_cache_entry()

[Junio C Hamano]

Once upon a time, there was a merge strategy called "recursive",
which was added in 6d297f8137 (Status update on merge-recursive in
C, 2006-07-08).  This implementation came with its own machinery
that creates an in-core index entry, and then refreshed the index
entry it created by calling the refresh_cache_entry() function
borrowed from the read-cache.c API.  refresh_cache_entry() used to
be file-scope static, but was made public with this change.

When ad45b327c0 (merge-recursive.[ch]: thoroughly debug these,
2025-04-08) finally removed the entire code for the "recursive"
merge strategy, the last external caller of refresh_cache_entry()
went with it.

Hide the low-level implementation detail again by making it a
file-scope static function.  The external callers would indirectly
call it when they call make_cache_entry() or rename_index_entry_at()
external interface, which "ort" (the successor of "recursive") does.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache-ll.h |   2 -
 read-cache.c    | 258 ++++++++++++++++++++++++------------------------
 2 files changed, 129 insertions(+), 131 deletions(-)

diff --git a/read-cache-ll.h b/read-cache-ll.h
index 066dd8bc3b..dd30befbb3 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -475,8 +475,6 @@ int refresh_index(struct index_state *, unsigned int flags, const struct pathspe
  */
 int repo_refresh_and_write_index(struct repository*, unsigned int refresh_flags, unsigned int write_flags, int gentle, const struct pathspec *, char *seen, const char *header_msg);
 
-struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
-
 void set_alternate_index_output(const char *);
 
 extern int verify_index_checksum;
diff --git a/read-cache.c b/read-cache.c
index e40f290bb3..555bf55132 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -129,6 +129,135 @@ static struct mem_pool *find_mem_pool(struct index_state *istate)
 	return *pool_ptr;
 }
 
+/*
+ * "refresh" does not calculate a new sha1 file or bring the
+ * cache up-to-date for mode/content changes. But what it
+ * _does_ do is to "re-match" the stat information of a file
+ * with the cache, so that you can refresh the cache for a
+ * file that hasn't been changed but where the stat entry is
+ * out of date.
+ *
+ * For example, you'd want to do this after doing a "git-read-tree",
+ * to link up the stat cache details with the proper files.
+ */
+static struct cache_entry *refresh_cache_ent(struct index_state *istate,
+					     struct cache_entry *ce,
+					     unsigned int options, int *err,
+					     int *changed_ret,
+					     int *t2_did_lstat,
+					     int *t2_did_scan)
+{
+	struct stat st;
+	struct cache_entry *updated;
+	int changed;
+	int refresh = options & CE_MATCH_REFRESH;
+	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
+	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
+	int ignore_missing = options & CE_MATCH_IGNORE_MISSING;
+	int ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;
+
+	if (!refresh || ce_uptodate(ce))
+		return ce;
+
+	if (!ignore_fsmonitor)
+		refresh_fsmonitor(istate);
+	/*
+	 * CE_VALID or CE_SKIP_WORKTREE means the user promised us
+	 * that the change to the work tree does not matter and told
+	 * us not to worry.
+	 */
+	if (!ignore_skip_worktree && ce_skip_worktree(ce)) {
+		ce_mark_uptodate(ce);
+		return ce;
+	}
+	if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
+		ce_mark_uptodate(ce);
+		return ce;
+	}
+	if (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID)) {
+		ce_mark_uptodate(ce);
+		return ce;
+	}
+
+	if (has_symlink_leading_path(ce->name, ce_namelen(ce))) {
+		if (ignore_missing)
+			return ce;
+		if (err)
+			*err = ENOENT;
+		return NULL;
+	}
+
+	if (t2_did_lstat)
+		*t2_did_lstat = 1;
+	if (lstat(ce->name, &st) < 0) {
+		if (ignore_missing && errno == ENOENT)
+			return ce;
+		if (err)
+			*err = errno;
+		return NULL;
+	}
+
+	changed = ie_match_stat(istate, ce, &st, options);
+	if (changed_ret)
+		*changed_ret = changed;
+	if (!changed) {
+		/*
+		 * The path is unchanged.  If we were told to ignore
+		 * valid bit, then we did the actual stat check and
+		 * found that the entry is unmodified.  If the entry
+		 * is not marked VALID, this is the place to mark it
+		 * valid again, under "assume unchanged" mode.
+		 */
+		if (ignore_valid && assume_unchanged &&
+		    !(ce->ce_flags & CE_VALID))
+			; /* mark this one VALID again */
+		else {
+			/*
+			 * We do not mark the index itself "modified"
+			 * because CE_UPTODATE flag is in-core only;
+			 * we are not going to write this change out.
+			 */
+			if (!S_ISGITLINK(ce->ce_mode)) {
+				ce_mark_uptodate(ce);
+				mark_fsmonitor_valid(istate, ce);
+			}
+			return ce;
+		}
+	}
+
+	if (t2_did_scan)
+		*t2_did_scan = 1;
+	if (ie_modified(istate, ce, &st, options)) {
+		if (err)
+			*err = EINVAL;
+		return NULL;
+	}
+
+	updated = make_empty_cache_entry(istate, ce_namelen(ce));
+	copy_cache_entry(updated, ce);
+	memcpy(updated->name, ce->name, ce->ce_namelen + 1);
+	fill_stat_cache_info(istate, updated, &st);
+	/*
+	 * If ignore_valid is not set, we should leave CE_VALID bit
+	 * alone.  Otherwise, paths marked with --no-assume-unchanged
+	 * (i.e. things to be edited) will reacquire CE_VALID bit
+	 * automatically, which is not really what we want.
+	 */
+	if (!ignore_valid && assume_unchanged &&
+	    !(ce->ce_flags & CE_VALID))
+		updated->ce_flags &= ~CE_VALID;
+
+	/* istate->cache_changed is updated in the caller */
+	return updated;
+}
+
+static struct cache_entry *refresh_cache_entry(struct index_state *istate,
+					       struct cache_entry *ce,
+					       unsigned int options)
+{
+	return refresh_cache_ent(istate, ce, options, NULL, NULL, NULL, NULL);
+}
+
 static const char *alternate_index_output;
 
 static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)
@@ -1339,128 +1468,6 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
 	return 0;
 }
 
-/*
- * "refresh" does not calculate a new sha1 file or bring the
- * cache up-to-date for mode/content changes. But what it
- * _does_ do is to "re-match" the stat information of a file
- * with the cache, so that you can refresh the cache for a
- * file that hasn't been changed but where the stat entry is
- * out of date.
- *
- * For example, you'd want to do this after doing a "git-read-tree",
- * to link up the stat cache details with the proper files.
- */
-static struct cache_entry *refresh_cache_ent(struct index_state *istate,
-					     struct cache_entry *ce,
-					     unsigned int options, int *err,
-					     int *changed_ret,
-					     int *t2_did_lstat,
-					     int *t2_did_scan)
-{
-	struct stat st;
-	struct cache_entry *updated;
-	int changed;
-	int refresh = options & CE_MATCH_REFRESH;
-	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
-	int ignore_skip_worktree = options & CE_MATCH_IGNORE_SKIP_WORKTREE;
-	int ignore_missing = options & CE_MATCH_IGNORE_MISSING;
-	int ignore_fsmonitor = options & CE_MATCH_IGNORE_FSMONITOR;
-
-	if (!refresh || ce_uptodate(ce))
-		return ce;
-
-	if (!ignore_fsmonitor)
-		refresh_fsmonitor(istate);
-	/*
-	 * CE_VALID or CE_SKIP_WORKTREE means the user promised us
-	 * that the change to the work tree does not matter and told
-	 * us not to worry.
-	 */
-	if (!ignore_skip_worktree && ce_skip_worktree(ce)) {
-		ce_mark_uptodate(ce);
-		return ce;
-	}
-	if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
-		ce_mark_uptodate(ce);
-		return ce;
-	}
-	if (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID)) {
-		ce_mark_uptodate(ce);
-		return ce;
-	}
-
-	if (has_symlink_leading_path(ce->name, ce_namelen(ce))) {
-		if (ignore_missing)
-			return ce;
-		if (err)
-			*err = ENOENT;
-		return NULL;
-	}
-
-	if (t2_did_lstat)
-		*t2_did_lstat = 1;
-	if (lstat(ce->name, &st) < 0) {
-		if (ignore_missing && errno == ENOENT)
-			return ce;
-		if (err)
-			*err = errno;
-		return NULL;
-	}
-
-	changed = ie_match_stat(istate, ce, &st, options);
-	if (changed_ret)
-		*changed_ret = changed;
-	if (!changed) {
-		/*
-		 * The path is unchanged.  If we were told to ignore
-		 * valid bit, then we did the actual stat check and
-		 * found that the entry is unmodified.  If the entry
-		 * is not marked VALID, this is the place to mark it
-		 * valid again, under "assume unchanged" mode.
-		 */
-		if (ignore_valid && assume_unchanged &&
-		    !(ce->ce_flags & CE_VALID))
-			; /* mark this one VALID again */
-		else {
-			/*
-			 * We do not mark the index itself "modified"
-			 * because CE_UPTODATE flag is in-core only;
-			 * we are not going to write this change out.
-			 */
-			if (!S_ISGITLINK(ce->ce_mode)) {
-				ce_mark_uptodate(ce);
-				mark_fsmonitor_valid(istate, ce);
-			}
-			return ce;
-		}
-	}
-
-	if (t2_did_scan)
-		*t2_did_scan = 1;
-	if (ie_modified(istate, ce, &st, options)) {
-		if (err)
-			*err = EINVAL;
-		return NULL;
-	}
-
-	updated = make_empty_cache_entry(istate, ce_namelen(ce));
-	copy_cache_entry(updated, ce);
-	memcpy(updated->name, ce->name, ce->ce_namelen + 1);
-	fill_stat_cache_info(istate, updated, &st);
-	/*
-	 * If ignore_valid is not set, we should leave CE_VALID bit
-	 * alone.  Otherwise, paths marked with --no-assume-unchanged
-	 * (i.e. things to be edited) will reacquire CE_VALID bit
-	 * automatically, which is not really what we want.
-	 */
-	if (!ignore_valid && assume_unchanged &&
-	    !(ce->ce_flags & CE_VALID))
-		updated->ce_flags &= ~CE_VALID;
-
-	/* istate->cache_changed is updated in the caller */
-	return updated;
-}
-
 static void show_file(const char * fmt, const char * name, int in_porcelain,
 		      int * first, const char *header_msg)
 {
@@ -1627,13 +1634,6 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	return has_errors;
 }
 
-struct cache_entry *refresh_cache_entry(struct index_state *istate,
-					struct cache_entry *ce,
-					unsigned int options)
-{
-	return refresh_cache_ent(istate, ce, options, NULL, NULL, NULL, NULL);
-}
-
 
 /*****************************************************************
  * Index File I/O
-- 
2.56.0-486-gd49ba3b80a

Re: [PATCH 3/4] read-cache: hide refresh_cache_entry()

[Jeff King]

On Fri, Oct 30, 2026 at 10:39:55AM -0700, Junio C Hamano wrote:

> Once upon a time, there was a merge strategy called "recursive",
> which was added in 6d297f8137 (Status update on merge-recursive in
> C, 2006-07-08).

  $ git log --grep='Once upon' --format=%an  | sort | uniq -c | sort -rn
       22 Jeff King
        4 SZEDER Gábor
        2 Johannes Schindelin
        1 Jonas Fonseca
        1 Elijah Newren

Welcome to the club. :)

The patch itself seems obviously correct, and the commit message is a
nice overview of how we got here.

-Peff

[PATCH 4/4] unpack-trees: hide verify_uptodate()

[Junio C Hamano]

64b1abe962 (merge-recursive: fix overwriting dirty files involved in
renames, 2018-04-19) made this function available to external callers.

With the removal of the "recursive" strategy in ad45b327c0
(merge-recursive.[ch]: thoroughly debug these, 2025-04-08), the last
external caller of this function want away.

Make it file-scope static again.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 unpack-trees.c | 4 ++--
 unpack-trees.h | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 1802809ad3..3298773490 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2269,8 +2269,8 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 	return add_rejected_path(o, error_type, ce->name);
 }
 
-int verify_uptodate(const struct cache_entry *ce,
-		    struct unpack_trees_options *o)
+static int verify_uptodate(const struct cache_entry *ce,
+			   struct unpack_trees_options *o)
 {
 	if (!o->skip_sparse_checkout &&
 	    (ce->ce_flags & CE_SKIP_WORKTREE) &&
diff --git a/unpack-trees.h b/unpack-trees.h
index 5867e26e17..638a6b076d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -123,9 +123,6 @@ enum update_sparsity_result {
 enum update_sparsity_result update_sparsity(struct unpack_trees_options *options,
 					    struct pattern_list *pl);
 
-int verify_uptodate(const struct cache_entry *ce,
-		    struct unpack_trees_options *o);
-
 int threeway_merge(const struct cache_entry * const *stages,
 		   struct unpack_trees_options *o);
 int twoway_merge(const struct cache_entry * const *src,
-- 
2.56.0-486-gd49ba3b80a

Re: [PATCH 0/4] mark symbols that are unnecessarily public static

[Jeff King]

On Fri, Oct 30, 2026 at 10:39:52AM -0700, Junio C Hamano wrote:

> Over the years, refactoring various subsystems, adding new callers,
> and rewriting them from scratch, made some symbols that started as
> static into global, and some other symbols that started as global,
> no longer need to be visible outside a file that use them.  Here is
> a small patch series that turns them back into file-scope static.
> 
> Honestly, I suspect that [3/4] was probably better with a forward
> declaration in the preview I sent earlier, but in the longer term,
> readers would not care if this conversion ended up in a large code
> movement, as they do not have to even know it.

The subject for patch 1 caught my eye, but I eyeballed the whole series
and they all look reasonable to me. I agree that big code movement is
sometimes annoying in a diff, but the flip side is that the resulting
file is nicer to read and maintain.

-Peff

Re: [PATCH 0/4] mark symbols that are unnecessarily public static

[Patrick Steinhardt]

On Fri, Oct 30, 2026 at 02:26:48PM -0400, Jeff King wrote:
> On Fri, Oct 30, 2026 at 10:39:52AM -0700, Junio C Hamano wrote:
> 
> > Over the years, refactoring various subsystems, adding new callers,
> > and rewriting them from scratch, made some symbols that started as
> > static into global, and some other symbols that started as global,
> > no longer need to be visible outside a file that use them.  Here is
> > a small patch series that turns them back into file-scope static.
> > 
> > Honestly, I suspect that [3/4] was probably better with a forward
> > declaration in the preview I sent earlier, but in the longer term,
> > readers would not care if this conversion ended up in a large code
> > movement, as they do not have to even know it.
> 
> The subject for patch 1 caught my eye, but I eyeballed the whole series
> and they all look reasonable to me. I agree that big code movement is
> sometimes annoying in a diff, but the flip side is that the resulting
> file is nicer to read and maintain.

Yup, I'm happy, as well. There's two tiny nits on the commit messages,
but nothing that'd really need a reroll.

Thanks!

Patrick