| Message ID | 20220930140948.80367-3-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase --update-refs: smooth out some rough edges | expand |
On Fri, Sep 30 2022, SZEDER Gábor wrote: > The test '--update-refs: check failed ref update' added in b3b1a21d1a > (sequencer: rewrite update-refs as user edits todo list, 2022-07-19) > directly modifies the contents of a ref file, so mark this test with > the REFFILES prereq. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > t/t3404-rebase-interactive.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 688b01e3eb..7f0df58628 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' ' > test_cmp_rev HEAD refs/heads/no-conflict-branch > ' > > -test_expect_success '--update-refs: check failed ref update' ' > +test_expect_success REFFILES '--update-refs: check failed ref update' ' > git checkout -B update-refs-error no-conflict-branch && > git branch -f base HEAD~4 && > git branch -f first HEAD~3 && We had various tests that depended on .git/refs/* for a good reason, and some that didn't. I may be missing something, but in this case this seems to be a "no good reason" case. I.e. the fix seems to just be: diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 688b01e3eb6..f1c021a7f7b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1994,8 +1994,9 @@ test_expect_success '--update-refs: check failed ref update' ' # At this point, the values of first, second, and third are # recorded in the update-refs file. We will force-update the # "second" ref, but "git branch -f" will not work because of - # the lock in the update-refs file. - git rev-parse third >.git/refs/heads/second && + # the lock in the update-refs file, so we need to use + # "update-ref". + git update-ref refs/heads/second third && test_must_fail git rebase --continue 2>err && grep "update_ref failed for ref '\''refs/heads/second'\''" err && As the comment notes if you try that with "git branch" you'll get an error, even with --force, but update-ref works just fine...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Fri, Sep 30 2022, SZEDER Gábor wrote: > >> The test '--update-refs: check failed ref update' added in b3b1a21d1a >> (sequencer: rewrite update-refs as user edits todo list, 2022-07-19) >> directly modifies the contents of a ref file, so mark this test with >> the REFFILES prereq. >> ... > # At this point, the values of first, second, and third are > # recorded in the update-refs file. We will force-update the > # "second" ref, but "git branch -f" will not work because of > - # the lock in the update-refs file. > - git rev-parse third >.git/refs/heads/second && > + # the lock in the update-refs file, so we need to use > + # "update-ref". > + git update-ref refs/heads/second third && > > test_must_fail git rebase --continue 2>err && > grep "update_ref failed for ref '\''refs/heads/second'\''" err && > > As the comment notes if you try that with "git branch" you'll get an > error, even with --force, but update-ref works just fine... Ah, I had exactly the same thought but was stopped from sending the suggestion to use "git update-ref" right away by "because of the lock in the update-refs file" that did not immediately click. What is happening is "git branch" and any Porcelain commands that call the now slightly misnamed branch_checked_out() function to see if an operation is allowed to touch a branch would refrain from touching this ref, but "update-ref" as a plumbing is deliberately designed to be usable to bypass the check [*]. The --update-refs series added code to branch_checked_out() to consider any refs being rewritten as "checked out hence no touch". Even though being listed in the update-refs file may count as a moral equivalent to having a "lock in the update-refs file", because write_update_refs_state() mentions no "lock", the proposed log message was confusing and I think that was why it did not "immediately click" at least for me. If this works with the update-ref plumbing, we should do so instead of adding REFFILES prerequisite. [Footnote] * Allowing more flexibility to the plumbing is OK, but those scripting around plumbing commands should get the same support as those writing built-in and making internal calls to functions like branch_checked_out(), in order for them to be able to write their Porcelain and offer the same safety to their users. The function certainly should be exposed to plumbing users, and possibly many other internal-only features. The trend in the past ten or so years has been "Porcelain first" and then "Porcelain only", which sadly made the composability of Git plumbing commands much less useful than pre-made Porcelain commands.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 688b01e3eb..7f0df58628 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1964,7 +1964,7 @@ test_expect_success 'respect user edits to update-ref steps' ' test_cmp_rev HEAD refs/heads/no-conflict-branch ' -test_expect_success '--update-refs: check failed ref update' ' +test_expect_success REFFILES '--update-refs: check failed ref update' ' git checkout -B update-refs-error no-conflict-branch && git branch -f base HEAD~4 && git branch -f first HEAD~3 &&
The test '--update-refs: check failed ref update' added in b3b1a21d1a (sequencer: rewrite update-refs as user edits todo list, 2022-07-19) directly modifies the contents of a ref file, so mark this test with the REFFILES prereq. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/t3404-rebase-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)