On Sat, Apr 26, 2025 at 07:10:52AM +0000, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <tao@xxxxxxxxxx>
>
> The enactment of replace refs in
> replace-object.c#register_replace_ref() explicitly uses the last
> portion of the ref "path", the substring after the last slash, as the
> object ID to be replaced. This means that replace refs can be
> organized into different paths; you can separate replace refs created
> for different purposes, like "refs/replace/2012-migration/*". This in
> turn makes it practical to store prepared replace refs in different ref
> paths on a git server, and have users "map" them, via specific
> refspecs, into their local repos; different types of replacements can
> be mapped into different sub-paths of refs/replace/.
I wonder whether this really was an intentional choice or whether it is
simply a bug that led to useful behaviour. In any case, git-replace(1)
itself does not mention your behaviour at all -- it simply talks about
the "name of the replace reference".
> The only way this didn't "work" is in the commit decoration process,
> in log-tree.c#add_ref_decoration(), where different logic was used to
> obtain the replaced object ID, removing the "refs/replace/" prefix
> only. This inconsistent logic meant that more structured replace ref
> paths caused a warning to be printed, and the "replaced" decoration to
> be omitted.
>
> Fix this decoration logic to use the same "last part of ref path"
> logic, fixing spurious warnings (and missing decorations) for users
> of more structured replace ref paths. Also add tests for qualified
> replace ref paths.
If we can agree that this is something that we want we should definitely
amend git-replace(1) to document this new format.
Which raises the question: is this something that we want? Are there any
arguments that would speak against loosening the format of replace refs
now?
The change itself would be backwards compatible, so any old ref that
parsed as replace ref would continue to parse as one. But the bigger
question is what happens when using an old Git version or an alternative
implementation of Git to parse such replace refs. How do those handle
such refs? For old Git versions we know, for alternative implementations
like JGit or libgit2 we don't. My assumption would be that those simply
ignore the new format, which raises the question whether that is okay.
In any case, we should provide good reasoning why this change is okay to
make.
> diff --git a/log-tree.c b/log-tree.c
> index a4d4ab59ca0..6a87724527b 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -163,10 +163,11 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,
>
> if (starts_with(refname, git_replace_ref_base)) {
> struct object_id original_oid;
> + const char *slash = strrchr(refname, '/');
> + const char *hash = slash ? slash + 1 : refname;
> if (!replace_refs_enabled(the_repository))
> return 0;
> - if (get_oid_hex(refname + strlen(git_replace_ref_base),
> - &original_oid)) {
> + if (get_oid_hex(hash, &original_oid)) {
> warning("invalid replace ref %s", refname);
> return 0;
> }
It would probably make sense to pull out a common function that parses
replace refs for us so that we can deduplicate callsites and show that
all of them behave the same now.
Thanks!
Patrick
