replace-refs: fix support of qualified replace ref paths
PATCH 1/1 replace-refs: fix support of qualified replace ref paths
From: Tao Klerks <[email protected]>
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.
---
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(-)
log-tree.c +3 -2
@@ -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 ; }
t/t4207-log-decoration-colors.sh +17 -0
@@ -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
t/t6050-replace.sh +10 -0
@@ -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
Signature · 1 line
gitgitgadget
Quoted 12 lines · Tao Klerks via GitGitGadget
From: Tao Klerks <[email protected]>
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".
Quoted 11 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.
Quoted 8 lines
log-tree.c +0
-0@@ -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;
log-tree.c static int add_ref_decoration(const char *refname, const char *referent UNUSED, · line 173
Quoted 10 lines
log-tree.c +3
-2+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
Quoted 4 lines · Patrick Steinhardt
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 ...
Quoted 1 line
Quoted 1 line
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.
Quoted 6 lines
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.
Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Patrick Steinhardt
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.
Quoted 5 lines
... 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/*"
Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 4 lines
Quoted 4 lines
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.
Quoted 5 lines
"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.
Quoted 3 lines · Tao Klerks
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.