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

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

[Tao Klerks via GitGitGadget]

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

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.

Signed-off-by: Tao Klerks <tao@klerks.biz>
---
    replace-refs: fix support of qualified replace ref paths

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1903%2FTaoK%2Fstructured-replace-ref-decoration-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1903/TaoK/structured-replace-ref-decoration-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1903

 log-tree.c                       |  5 +++--
 t/t4207-log-decoration-colors.sh | 17 +++++++++++++++++
 t/t6050-replace.sh               | 10 ++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

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;
 		}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 2e83cc820a1..f80684efcff 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -131,4 +131,21 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
 	cmp_filtered_decorations
 '
 
+test_expect_success 'test replace decoration for nested replace path' '
+	test_when_finished remove_replace_refs &&
+
+	CURRENT_HASH=$(git rev-parse --verify HEAD) &&
+	git replace --graft HEAD HEAD~2 &&
+	git update-ref refs/tmp/tmpref refs/replace/$CURRENT_HASH &&
+	git update-ref -d refs/replace/$CURRENT_HASH &&
+	git update-ref refs/replace/nested-path/abc/$CURRENT_HASH refs/tmp/tmpref &&
+
+	git log --decorate -1 HEAD >actual &&
+	test_grep "replaced" actual &&
+
+	git --no-replace-objects log --decorate -1 HEAD >actual &&
+	test_grep ! "replaced" actual
+
+'
+
 test_done
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index aa1b5351873..9b06baa1439 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -546,4 +546,14 @@ test_expect_success '--convert-graft-file' '
 	test_grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
 '
 
+test_expect_success 'replace ref in a nested path' '
+	git cat-file commit $HASH2 >actual &&
+	R=$(sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
+	git update-ref refs/replace/abc1/$HASH2 $R &&
+	git show $HASH2 >actual &&
+	test_grep "O Thor" actual &&
+	git --no-replace-objects show $HASH2 >actual &&
+	test_grep "A U Thor" actual
+'
+
 test_done

base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
-- 
gitgitgadget

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

[Patrick Steinhardt]

On Sat, Apr 26, 2025 at 07:10:52AM +0000, Tao Klerks via GitGitGadget wrote:
> 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".

> 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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

Yup, I am reasonbly sure that this was not a designed behaviour.  If
this were discovered during the original review, I would probably
have suggested tightening the rule to ignore anything with extra
levels in the middle.  It can be argued that it is too late, but
with this ...

>> The only way this didn't "work" is in the commit decoration process,

... where the "funny" replace refs are honored in some but not all
situations, it also can be argued that it is highly unlikely anybody
sane is actually depending on this "feature", so such a tightening
may not hurt too much.

The reason why I would slightly prefer tightening the rule, rather
than making the loophole even larger by adjusting the code for "the
commit decoration process", is what it means to have two different
replacement objects for the same object, which would naturally be
prevented from happening if we did not allow extra levels in the
middle.  Would one always consistently trump the other?  When a
filesystem rebalances, would we just pick one at randomly based
purely on the first one readdir() happened to return?  It smells
to lead to nothing but a confused mess.

Maybe the answer is "don't do it, then".  The same answer, however,
can be given for creating any extra level between refs/replace and
the object name itself, so...  I dunno.

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

"What if refs/replace/{a,b,c}/$name point at different objects?"
would be a solid reason why we shouldn't do this, but I personally
do not feel too strongly about it, as "don't do it, then" may be a
reasonable answer for such an insane scenario.

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

[Tao Klerks]

On Tue, Apr 29, 2025 at 8:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > I wonder whether this really was an intentional choice or whether it is
> > simply a bug that led to useful behaviour.

I don't know that for sure either - although the fact that we *do*
already have sanity checks against multiple replace refs targeting the
same object id leads me to believe it was intentionally allowed.

I tried to explain why this is useful, and why it should be *better*
supported, in the change description: *without* this behavior, replace
refs cannot be "namespaced" - they are always inherently an
undifferentiated pile, and that in turn makes *sharing* of replace
refs completely impractical.

>
> ... where the "funny" replace refs are honored in some but not all
> situations, it also can be argued that it is highly unlikely anybody
> sane is actually depending on this "feature", so such a tightening
> may not hurt too much.

Well, I guess my sanity can be reasonably questioned, but it would
certainly hurt me and my thousand plus users - we use replace refs to
keep nice tidy squash-based history by default, but allow for
different types of "deep blame" by setting up one or more fetch
refspecs that map into "refs/replace/SOMETHING/*"

>
> The reason why I would slightly prefer tightening the rule, rather
> than making the loophole even larger by adjusting the code for "the
> commit decoration process", is what it means to have two different
> replacement objects for the same object, which would naturally be
> prevented from happening if we did not allow extra levels in the
> middle.

This is already accounted for with a hard error: If you end up with
duplicate replace ref targets, many operations fail with a clear
error. Clean up your replace refs and you're off to the races again.

> Would one always consistently trump the other?  When a
> filesystem rebalances, would we just pick one at randomly based
> purely on the first one readdir() happened to return?  It smells
> to lead to nothing but a confused mess.

Reasonable concern, but already addressed.

>
> Maybe the answer is "don't do it, then".  The same answer, however,
> can be given for creating any extra level between refs/replace and
> the object name itself, so...  I dunno.

The same answer should probably not be given: One behavior is
*useful*, and one is not.

>
> > If we can agree that this is something that we want we should definitely
> > amend git-replace(1) to document this new format.

I will (propose to) do so.

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

My point here is that it's *not* a loosening - this "loose" behavior
already exists, it works, it is useful, and there are people using it.
At least my thousand users. All I'm proposing here is to show them
less spurious warnings when they run a log with --decorate.

>
> "What if refs/replace/{a,b,c}/$name point at different objects?"
> would be a solid reason why we shouldn't do this, but I personally
> do not feel too strongly about it, as "don't do it, then" may be a
> reasonable answer for such an insane scenario.

That is indeed the current outcome: don't do it or you get a hard (but
nondestructive) error. You can then of course fix it and you're set.

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

[Junio C Hamano]

Tao Klerks <tao@klerks.biz> writes:

> This is already accounted for with a hard error: If you end up with
> duplicate replace ref targets, many operations fail with a clear
> error. Clean up your replace refs and you're off to the races again.

OK.  As long as that safety is there, I offhand see no more reason
to forbid it.  As you said, it can lead to a useful use case.