{"thread":{"id":"63354","subject":"[PATCH] replace-refs: fix support of qualified replace ref paths","startedAt":"2025-04-26T07:10:56Z","lastAt":"2025-04-30T00:11:28Z","messageCount":5,"participants":["Tao Klerks via GitGitGadget","Patrick Steinhardt","Junio C Hamano","Tao Klerks"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"516846","messageId":"pull.1903.git.1745651452869.gitgitgadget@gmail.com","threadId":"63354","inReplyTo":null,"subject":"[PATCH] replace-refs: fix support of qualified replace ref paths","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-26T07:10:52Z","receivedAt":"2025-04-26T07:10:56Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe enactment of replace refs in\nreplace-object.c#register_replace_ref() explicitly uses the last\nportion of the ref \"path\", the substring after the last slash, as the\nobject ID to be replaced. This means that replace refs can be\norganized into different paths; you can separate replace refs created\nfor different purposes, like \"refs/replace/2012-migration/*\". This in\nturn makes it practical to store prepared replace refs in different ref\npaths on a git server, and have users \"map\" them, via specific\nrefspecs, into their local repos; different types of replacements can\nbe mapped into different sub-paths of refs/replace/.\n\nThe only way this didn't \"work\" is in the commit decoration process,\nin log-tree.c#add_ref_decoration(), where different logic was used to\nobtain the replaced object ID, removing the \"refs/replace/\" prefix\nonly. This inconsistent logic meant that more structured replace ref\npaths caused a warning to be printed, and the \"replaced\" decoration to\nbe omitted.\n\nFix this decoration logic to use the same \"last part of ref path\"\nlogic, fixing spurious warnings (and missing decorations) for users\nof more structured replace ref paths. Also add tests for qualified\nreplace ref paths.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    replace-refs: fix support of qualified replace ref paths\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1903%2FTaoK%2Fstructured-replace-ref-decoration-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1903/TaoK/structured-replace-ref-decoration-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1903\n\n log-tree.c                       |  5 +++--\n t/t4207-log-decoration-colors.sh | 17 +++++++++++++++++\n t/t6050-replace.sh               | 10 ++++++++++\n 3 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex a4d4ab59ca0..6a87724527b 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -163,10 +163,11 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,\n \n \tif (starts_with(refname, git_replace_ref_base)) {\n \t\tstruct object_id original_oid;\n+\t\tconst char *slash = strrchr(refname, '/');\n+\t\tconst char *hash = slash ? slash + 1 : refname;\n \t\tif (!replace_refs_enabled(the_repository))\n \t\t\treturn 0;\n-\t\tif (get_oid_hex(refname + strlen(git_replace_ref_base),\n-\t\t\t\t&original_oid)) {\n+\t\tif (get_oid_hex(hash, &original_oid)) {\n \t\t\twarning(\"invalid replace ref %s\", refname);\n \t\t\treturn 0;\n \t\t}\ndiff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh\nindex 2e83cc820a1..f80684efcff 100755\n--- a/t/t4207-log-decoration-colors.sh\n+++ b/t/t4207-log-decoration-colors.sh\n@@ -131,4 +131,21 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A\n \tcmp_filtered_decorations\n '\n \n+test_expect_success 'test replace decoration for nested replace path' '\n+\ttest_when_finished remove_replace_refs &&\n+\n+\tCURRENT_HASH=$(git rev-parse --verify HEAD) &&\n+\tgit replace --graft HEAD HEAD~2 &&\n+\tgit update-ref refs/tmp/tmpref refs/replace/$CURRENT_HASH &&\n+\tgit update-ref -d refs/replace/$CURRENT_HASH &&\n+\tgit update-ref refs/replace/nested-path/abc/$CURRENT_HASH refs/tmp/tmpref &&\n+\n+\tgit log --decorate -1 HEAD >actual &&\n+\ttest_grep \"replaced\" actual &&\n+\n+\tgit --no-replace-objects log --decorate -1 HEAD >actual &&\n+\ttest_grep ! \"replaced\" actual\n+\n+'\n+\n test_done\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex aa1b5351873..9b06baa1439 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -546,4 +546,14 @@ test_expect_success '--convert-graft-file' '\n \ttest_grep \"$EMPTY_BLOB $EMPTY_TREE\" .git/info/grafts\n '\n \n+test_expect_success 'replace ref in a nested path' '\n+\tgit cat-file commit $HASH2 >actual &&\n+\tR=$(sed -e \"s/A U/O/\" actual | git hash-object -t commit --stdin -w) &&\n+\tgit update-ref refs/replace/abc1/$HASH2 $R &&\n+\tgit show $HASH2 >actual &&\n+\ttest_grep \"O Thor\" actual &&\n+\tgit --no-replace-objects show $HASH2 >actual &&\n+\ttest_grep \"A U Thor\" actual\n+'\n+\n test_done\n\nbase-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3\n-- \ngitgitgadget\n"},{"id":"516945","messageId":"aBCt8YrqJ7IM0ld6@pks.im","threadId":"63354","inReplyTo":"pull.1903.git.1745651452869.gitgitgadget@gmail.com","subject":"Re: [PATCH] replace-refs: fix support of qualified replace ref paths","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-29T10:46:09Z","receivedAt":"2025-04-29T10:46:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Apr 26, 2025 at 07:10:52AM +0000, Tao Klerks via GitGitGadget wrote:\n> From: Tao Klerks <tao@klerks.biz>\n> \n> The enactment of replace refs in\n> replace-object.c#register_replace_ref() explicitly uses the last\n> portion of the ref \"path\", the substring after the last slash, as the\n> object ID to be replaced. This means that replace refs can be\n> organized into different paths; you can separate replace refs created\n> for different purposes, like \"refs/replace/2012-migration/*\". This in\n> turn makes it practical to store prepared replace refs in different ref\n> paths on a git server, and have users \"map\" them, via specific\n> refspecs, into their local repos; different types of replacements can\n> be mapped into different sub-paths of refs/replace/.\n\nI wonder whether this really was an intentional choice or whether it is\nsimply a bug that led to useful behaviour. In any case, git-replace(1)\nitself does not mention your behaviour at all -- it simply talks about\nthe \"name of the replace reference\".\n\n> The only way this didn't \"work\" is in the commit decoration process,\n> in log-tree.c#add_ref_decoration(), where different logic was used to\n> obtain the replaced object ID, removing the \"refs/replace/\" prefix\n> only. This inconsistent logic meant that more structured replace ref\n> paths caused a warning to be printed, and the \"replaced\" decoration to\n> be omitted.\n> \n> Fix this decoration logic to use the same \"last part of ref path\"\n> logic, fixing spurious warnings (and missing decorations) for users\n> of more structured replace ref paths. Also add tests for qualified\n> replace ref paths.\n\nIf we can agree that this is something that we want we should definitely\namend git-replace(1) to document this new format.\n\nWhich raises the question: is this something that we want? Are there any\narguments that would speak against loosening the format of replace refs\nnow?\n\nThe change itself would be backwards compatible, so any old ref that\nparsed as replace ref would continue to parse as one. But the bigger\nquestion is what happens when using an old Git version or an alternative\nimplementation of Git to parse such replace refs. How do those handle\nsuch refs? For old Git versions we know, for alternative implementations\nlike JGit or libgit2 we don't. My assumption would be that those simply\nignore the new format, which raises the question whether that is okay.\n\nIn any case, we should provide good reasoning why this change is okay to\nmake.\n\n> diff --git a/log-tree.c b/log-tree.c\n> index a4d4ab59ca0..6a87724527b 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -163,10 +163,11 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,\n>  \n>  \tif (starts_with(refname, git_replace_ref_base)) {\n>  \t\tstruct object_id original_oid;\n> +\t\tconst char *slash = strrchr(refname, '/');\n> +\t\tconst char *hash = slash ? slash + 1 : refname;\n>  \t\tif (!replace_refs_enabled(the_repository))\n>  \t\t\treturn 0;\n> -\t\tif (get_oid_hex(refname + strlen(git_replace_ref_base),\n> -\t\t\t\t&original_oid)) {\n> +\t\tif (get_oid_hex(hash, &original_oid)) {\n>  \t\t\twarning(\"invalid replace ref %s\", refname);\n>  \t\t\treturn 0;\n>  \t\t}\n\nIt would probably make sense to pull out a common function that parses\nreplace refs for us so that we can deduplicate callsites and show that\nall of them behave the same now.\n\nThanks!\n\nPatrick\n"},{"id":"516980","messageId":"xmqqh6266c1c.fsf@gitster.g","threadId":"63354","inReplyTo":"aBCt8YrqJ7IM0ld6@pks.im","subject":"Re: [PATCH] replace-refs: fix support of qualified replace ref paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-29T18:46:39Z","receivedAt":"2025-04-29T18:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I wonder whether this really was an intentional choice or whether it is\n> simply a bug that led to useful behaviour. In any case, git-replace(1)\n> itself does not mention your behaviour at all -- it simply talks about\n> the \"name of the replace reference\".\n\nYup, I am reasonbly sure that this was not a designed behaviour.  If\nthis were discovered during the original review, I would probably\nhave suggested tightening the rule to ignore anything with extra\nlevels in the middle.  It can be argued that it is too late, but\nwith this ...\n\n>> The only way this didn't \"work\" is in the commit decoration process,\n\n... where the \"funny\" replace refs are honored in some but not all\nsituations, it also can be argued that it is highly unlikely anybody\nsane is actually depending on this \"feature\", so such a tightening\nmay not hurt too much.\n\nThe reason why I would slightly prefer tightening the rule, rather\nthan making the loophole even larger by adjusting the code for \"the\ncommit decoration process\", is what it means to have two different\nreplacement objects for the same object, which would naturally be\nprevented from happening if we did not allow extra levels in the\nmiddle.  Would one always consistently trump the other?  When a\nfilesystem rebalances, would we just pick one at randomly based\npurely on the first one readdir() happened to return?  It smells\nto lead to nothing but a confused mess.\n\nMaybe the answer is \"don't do it, then\".  The same answer, however,\ncan be given for creating any extra level between refs/replace and\nthe object name itself, so...  I dunno.\n\n> If we can agree that this is something that we want we should definitely\n> amend git-replace(1) to document this new format.\n>\n> Which raises the question: is this something that we want? Are there any\n> arguments that would speak against loosening the format of replace refs\n> now?\n\n\"What if refs/replace/{a,b,c}/$name point at different objects?\"\nwould be a solid reason why we shouldn't do this, but I personally\ndo not feel too strongly about it, as \"don't do it, then\" may be a\nreasonable answer for such an insane scenario.\n"},{"id":"516981","messageId":"CAPMMpohgEXVPHKCQtvc-zLC35qtY+qJ9WgQO_quOgUG01eyTOw@mail.gmail.com","threadId":"63354","inReplyTo":"xmqqh6266c1c.fsf@gitster.g","subject":"Re: [PATCH] replace-refs: fix support of qualified replace ref paths","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2025-04-29T19:35:52Z","receivedAt":"2025-04-29T19:36:05Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Apr 29, 2025 at 8:46 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> > I wonder whether this really was an intentional choice or whether it is\n> > simply a bug that led to useful behaviour.\n\nI don't know that for sure either - although the fact that we *do*\nalready have sanity checks against multiple replace refs targeting the\nsame object id leads me to believe it was intentionally allowed.\n\nI tried to explain why this is useful, and why it should be *better*\nsupported, in the change description: *without* this behavior, replace\nrefs cannot be \"namespaced\" - they are always inherently an\nundifferentiated pile, and that in turn makes *sharing* of replace\nrefs completely impractical.\n\n>\n> ... where the \"funny\" replace refs are honored in some but not all\n> situations, it also can be argued that it is highly unlikely anybody\n> sane is actually depending on this \"feature\", so such a tightening\n> may not hurt too much.\n\nWell, I guess my sanity can be reasonably questioned, but it would\ncertainly hurt me and my thousand plus users - we use replace refs to\nkeep nice tidy squash-based history by default, but allow for\ndifferent types of \"deep blame\" by setting up one or more fetch\nrefspecs that map into \"refs/replace/SOMETHING/*\"\n\n>\n> The reason why I would slightly prefer tightening the rule, rather\n> than making the loophole even larger by adjusting the code for \"the\n> commit decoration process\", is what it means to have two different\n> replacement objects for the same object, which would naturally be\n> prevented from happening if we did not allow extra levels in the\n> middle.\n\nThis is already accounted for with a hard error: If you end up with\nduplicate replace ref targets, many operations fail with a clear\nerror. Clean up your replace refs and you're off to the races again.\n\n> Would one always consistently trump the other?  When a\n> filesystem rebalances, would we just pick one at randomly based\n> purely on the first one readdir() happened to return?  It smells\n> to lead to nothing but a confused mess.\n\nReasonable concern, but already addressed.\n\n>\n> Maybe the answer is \"don't do it, then\".  The same answer, however,\n> can be given for creating any extra level between refs/replace and\n> the object name itself, so...  I dunno.\n\nThe same answer should probably not be given: One behavior is\n*useful*, and one is not.\n\n>\n> > If we can agree that this is something that we want we should definitely\n> > amend git-replace(1) to document this new format.\n\nI will (propose to) do so.\n\n> >\n> > Which raises the question: is this something that we want? Are there any\n> > arguments that would speak against loosening the format of replace refs\n> > now?\n\nMy point here is that it's *not* a loosening - this \"loose\" behavior\nalready exists, it works, it is useful, and there are people using it.\nAt least my thousand users. All I'm proposing here is to show them\nless spurious warnings when they run a log with --decorate.\n\n>\n> \"What if refs/replace/{a,b,c}/$name point at different objects?\"\n> would be a solid reason why we shouldn't do this, but I personally\n> do not feel too strongly about it, as \"don't do it, then\" may be a\n> reasonable answer for such an insane scenario.\n\nThat is indeed the current outcome: don't do it or you get a hard (but\nnondestructive) error. You can then of course fix it and you're set.\n"},{"id":"516997","messageId":"xmqqselq33v6.fsf@gitster.g","threadId":"63354","inReplyTo":"CAPMMpohgEXVPHKCQtvc-zLC35qtY+qJ9WgQO_quOgUG01eyTOw@mail.gmail.com","subject":"Re: [PATCH] replace-refs: fix support of qualified replace ref paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-30T00:11:25Z","receivedAt":"2025-04-30T00:11:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> This is already accounted for with a hard error: If you end up with\n> duplicate replace ref targets, many operations fail with a clear\n> error. Clean up your replace refs and you're off to the races again.\n\nOK.  As long as that safety is there, I offhand see no more reason\nto forbid it.  As you said, it can lead to a useful use case.\n"}]}