[PATCH] read-cache: hide refresh_cache_entry()

[PATCH] 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    | 8 +++++---
 2 files changed, 5 insertions(+), 5 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 c4cf08a3a3..94dff890a7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -129,6 +129,8 @@ static struct mem_pool *find_mem_pool(struct index_state *istate)
 	return *pool_ptr;
 }
 
+static struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
+
 static const char *alternate_index_output;
 
 static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)
@@ -1624,9 +1626,9 @@ 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)
+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);
 }
-- 
2.56.0-433-ga9209adf11

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

[Patrick Steinhardt]

On Wed, Oct 28, 2026 at 01:34:42PM -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).  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.

Seeing all these patches dropping public declarations I wonder whether
they should have been part of the same patch series :)

> diff --git a/read-cache.c b/read-cache.c
> index c4cf08a3a3..94dff890a7 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -129,6 +129,8 @@ static struct mem_pool *find_mem_pool(struct index_state *istate)
>  	return *pool_ptr;
>  }
>  
> +static struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
> +
>  static const char *alternate_index_output;
>  
>  static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)

Could we avoid this forward declaration by moving the function around?

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> 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.
>
> Seeing all these patches dropping public declarations I wonder whether
> they should have been part of the same patch series :)

Perhaps ;-).

>> +static struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
>> +
>>  static const char *alternate_index_output;
>>  
>>  static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)
>
> Could we avoid this forward declaration by moving the function around?

As refresh_cache_entry() that has mere one line in its body calls
refresh_cache_ent() that is a screenful and defined nearby, both of
them need to be moved upward, so it becomes a rather unsightly
change, even though nobody would care when they only look at the end
result X-<.

By the way, I tried "history fixup" to make this change, and noticed
that the subcommand ignores "[notes] rewriteref = refs/notes/amlog",
just like "git history reword" does.  Again, I wonder if this is a
known limitation, or a deliberate design.

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

[Patrick Steinhardt]

On Fri, Oct 30, 2026 at 08:46:33AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> 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.
> >
> > Seeing all these patches dropping public declarations I wonder whether
> > they should have been part of the same patch series :)
> 
> Perhaps ;-).
> 
> >> +static struct cache_entry *refresh_cache_entry(struct index_state *, struct cache_entry *, unsigned int);
> >> +
> >>  static const char *alternate_index_output;
> >>  
> >>  static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)
> >
> > Could we avoid this forward declaration by moving the function around?
> 
> As refresh_cache_entry() that has mere one line in its body calls
> refresh_cache_ent() that is a screenful and defined nearby, both of
> them need to be moved upward, so it becomes a rather unsightly
> change, even though nobody would care when they only look at the end
> result X-<.

Well, people probably know by now that I myself tend to err more on the
side of fixing up stuff, even if it's a bit more on the painful side of
things. So I'm not going to surprise you by saying that I think that
would be worth it.

In any case, I don't feel strongly about it.

> By the way, I tried "history fixup" to make this change, and noticed
> that the subcommand ignores "[notes] rewriteref = refs/notes/amlog",
> just like "git history reword" does.  Again, I wonder if this is a
> known limitation, or a deliberate design.

Yeah, I already replied in the other series, this is a known limitation.
I've created an issue to track this that we may have capacity to work on
in the next release cycle. But I wouldn't mind anybody else beating us
to it.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> > Could we avoid this forward declaration by moving the function around?
>> 
>> As refresh_cache_entry() that has mere one line in its body calls
>> refresh_cache_ent() that is a screenful and defined nearby, both of
>> them need to be moved upward, so it becomes a rather unsightly
>> change, even though nobody would care when they only look at the end
>> result X-<.
>
> Well, people probably know by now that I myself tend to err more on the
> side of fixing up stuff, even if it's a bit more on the painful side of
> things. So I'm not going to surprise you by saying that I think that
> would be worth it.
>
> In any case, I don't feel strongly about it.

FWIW, the version with large code movement will be the final
version, as my philosophy is "once written, unless there is a
compelling reason, do not flip flop" and the last one I wrote was
the one you suggested, so that I can see how bad the result looks.

I think the patch itself may be ugly but the result is easier to
work on going forward.  Thanks for pushing.