Re: [PATCH] replace-refs: fix support of qualified replace ref paths

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 29, 2025, 10:46 UTC
Message-ID: <aBCt8YrqJ7IM0ld6@pks.im>
In-Reply-To: <pull.1903.git.1745651452869.gitgitgadget@gmail.com>

On Sat, Apr 26, 2025 at 07:10:52AM +0000, Tao Klerks via GitGitGadget wrote:

Show 12 quoted lines

> From: Tao Klerks <tao@klerks.biz>
> 
> 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".

Show 11 quoted lines

> 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.

Show 18 quoted lines

> 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