{"thread":{"id":"61397","subject":"[PATCH] refs: return conflict error when checking packed refs","startedAt":"2024-04-30T14:50:52Z","lastAt":"2024-05-08T01:39:51Z","messageCount":11,"participants":["Ivan Tse via GitGitGadget","Patrick Steinhardt","Karthik Nayak","Junio C Hamano","Ivan Tse"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"493787","messageId":"pull.1716.git.git.1714488648294.gitgitgadget@gmail.com","threadId":"61397","inReplyTo":null,"subject":"[PATCH] refs: return conflict error when checking packed refs","fromName":"Ivan Tse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-30T14:50:47Z","receivedAt":"2024-04-30T14:50:52Z","isPatch":true,"sender":{"key":"ivan.tse1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4662860?v=4"},"body":"From: Ivan Tse <ivan.tse1@gmail.com>\n\nThe TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\nref due to a name conflict with another ref. An example of this is a\ndirectory/file conflict such as ref names A/B and A.\n\n\"git fetch\" uses this error code to more accurately describe the error\nby recommending to the user that they try running \"git remote prune\" to\nremove any old refs that are deleted by the remote which would clear up\nany directory/file conflicts.\n\nThis helpful error message is not displayed when the conflicted ref is\nstored in packed refs. This change fixes this by ensuring error return\ncode consistency in `lock_raw_ref`.\n\nSigned-off-by: Ivan Tse <ivan.tse1@gmail.com>\n---\n    refs: return conflict error when checking packed refs\n    \n    I wanted to provide extra context around the error message I'm referring\n    to so it's clear what I'm trying to achieve.\n    \n    First, let's get into a directory/file conflict situation:\n    \n    git branch dir_file_conflict\n    git push origin dir_file_conflict\n    git update-ref -d refs/remotes/origin/dir_file_conflict\n    git update-ref -d refs/heads/dir_file_conflict\n    git update-ref refs/remotes/origin/dir_file_conflict/file master\n    \n    \n    From above, the remote origin has a dir_file_conflict ref name and the\n    local repository has a dir_file_conflict/file ref name for the remote.\n    This situation can occur if dir_file_conflict/file used to exist and but\n    was later deleted in the remote.\n    \n    Then when we git fetch:\n    \n    yolo10[master] $ git fetch\n    error: cannot lock ref 'refs/remotes/origin/dir_file_conflict': 'refs/remotes/origin/dir_file_conflict/file' exists; cannot create 'refs/remotes/origin/dir_file_conflict'\n    From github.com:ivantsepp/yolo10\n     ! [new branch]      dir_file_conflict -> origin/dir_file_conflict  (unable to update local ref)\n    error: some local refs could not be updated; try running\n     'git remote prune origin' to remove any old, conflicting branches\n    \n    \n    We get the helpful error message to run git remote prune origin to\n    remove old, conflicting branches.\n    \n    However, when the ref is stored in packed refs:\n    \n    yolo10[master] $ git pack-refs --all\n    yolo10[master] $ git fetch\n    error: cannot lock ref 'refs/remotes/origin/dir_file_conflict': 'refs/remotes/origin/dir_file_conflict/file' exists; cannot create 'refs/remotes/origin/dir_file_conflict'\n    From github.com:ivantsepp/yolo10\n     ! [new branch]      dir_file_conflict -> origin/dir_file_conflict  (unable to update local ref)\n    \n    \n    The helpful message is not there! I believe this error message should\n    show up regardless of how the ref is stored (loose vs packed-refs). I\n    attempted to track down the necessary change to make this happen and it\n    seems like a straightforward change. I hope I didn't overlook anything!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1716%2Fivantsepp%2Freturn_name_conflict_error_for_packed_refs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1716/ivantsepp/return_name_conflict_error_for_packed_refs-v1\nPull-Request: https://github.com/git/git/pull/1716\n\n refs/files-backend.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a098d14ea00..97473f377d1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t */\n \t\tif (refs_verify_refname_available(\n \t\t\t\t    refs->packed_ref_store, refname,\n-\t\t\t\t    extras, NULL, err))\n+\t\t\t\t    extras, NULL, err)) {\n+\t\t\tret = TRANSACTION_NAME_CONFLICT;\n \t\t\tgoto error_return;\n+\t\t}\n \t}\n \n \tret = 0;\n\nbase-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8\n-- \ngitgitgadget\n"},{"id":"493917","messageId":"ZjOJONdlaINfN_i_@tanuki","threadId":"61397","inReplyTo":"pull.1716.git.git.1714488648294.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: return conflict error when checking packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-02T12:38:16Z","receivedAt":"2024-05-02T12:38:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 02:50:47PM +0000, Ivan Tse via GitGitGadget wrote:\n> From: Ivan Tse <ivan.tse1@gmail.com>\n> \n> The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\n> ref due to a name conflict with another ref. An example of this is a\n> directory/file conflict such as ref names A/B and A.\n> \n> \"git fetch\" uses this error code to more accurately describe the error\n> by recommending to the user that they try running \"git remote prune\" to\n> remove any old refs that are deleted by the remote which would clear up\n> any directory/file conflicts.\n> \n> This helpful error message is not displayed when the conflicted ref is\n> stored in packed refs. This change fixes this by ensuring error return\n> code consistency in `lock_raw_ref`.\n\nThe change itself makes sense to me. But I'd like to see a test that\ndemonstrates the new behaviour so that we don't regress this in the\nfuture.\n\nThanks!\n\nPatrick\n"},{"id":"493955","messageId":"pull.1716.v2.git.git.1714711829698.gitgitgadget@gmail.com","threadId":"61397","inReplyTo":"pull.1716.git.git.1714488648294.gitgitgadget@gmail.com","subject":"[PATCH v2] refs: return conflict error when checking packed refs","fromName":"Ivan Tse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-03T04:50:29Z","receivedAt":"2024-05-03T04:50:34Z","isPatch":true,"sender":{"key":"ivan.tse1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4662860?v=4"},"body":"From: Ivan Tse <ivan.tse1@gmail.com>\n\nThe TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\nref due to a name conflict with another ref. An example of this is a\ndirectory/file conflict such as ref names A/B and A.\n\n\"git fetch\" uses this error code to more accurately describe the error\nby recommending to the user that they try running \"git remote prune\" to\nremove any old refs that are deleted by the remote which would clear up\nany directory/file conflicts.\n\nThis helpful error message is not displayed when the conflicted ref is\nstored in packed refs. This change fixes this by ensuring error return\ncode consistency in `lock_raw_ref`.\n\nSigned-off-by: Ivan Tse <ivan.tse1@gmail.com>\n---\n    refs: return conflict error when checking packed refs\n    \n    Changes against v1:\n    \n     * added test for the error message during git fetch\n    \n    Thanks for reviewing! I've gone ahead and attempted to add tests for\n    this behavior. It tests that the error message is shown for both cases\n    when the ref is stored as loose vs packed-refs. How does this test look?\n    Also, should this test have a REFFILES prerequisite?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1716%2Fivantsepp%2Freturn_name_conflict_error_for_packed_refs-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1716/ivantsepp/return_name_conflict_error_for_packed_refs-v2\nPull-Request: https://github.com/git/git/pull/1716\n\nRange-diff vs v1:\n\n 1:  a87ba267e44 ! 1:  58b2cda5c18 refs: return conflict error when checking packed refs\n     @@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,\n       \t}\n       \n       \tret = 0;\n     +\n     + ## t/t5510-fetch.sh ##\n     +@@ t/t5510-fetch.sh: test_expect_success 'branchname D/F conflict resolved by --prune' '\n     + \ttest_cmp expect actual\n     + '\n     + \n     ++test_expect_success 'branchname D/F conflict rejected with targeted error message' '\n     ++\tgit clone . df-conflict-error &&\n     ++\tgit branch dir_conflict &&\n     ++\t(\n     ++\t\tcd df-conflict-error &&\n     ++\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n     ++\t\ttest_must_fail git fetch 2>../err &&\n     ++\t\tgit pack-refs --all &&\n     ++\t\ttest_must_fail git fetch 2>../err-packed\n     ++\t) &&\n     ++\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n     ++\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err &&\n     ++\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n     ++\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err-packed\n     ++'\n     ++\n     + test_expect_success 'fetching a one-level ref works' '\n     + \ttest_commit extra &&\n     + \tgit reset --hard HEAD^ &&\n\n\n refs/files-backend.c |  4 +++-\n t/t5510-fetch.sh     | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a098d14ea00..97473f377d1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t */\n \t\tif (refs_verify_refname_available(\n \t\t\t\t    refs->packed_ref_store, refname,\n-\t\t\t\t    extras, NULL, err))\n+\t\t\t\t    extras, NULL, err)) {\n+\t\t\tret = TRANSACTION_NAME_CONFLICT;\n \t\t\tgoto error_return;\n+\t\t}\n \t}\n \n \tret = 0;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 33d34d5ae9e..ae0828e26a1 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'branchname D/F conflict rejected with targeted error message' '\n+\tgit clone . df-conflict-error &&\n+\tgit branch dir_conflict &&\n+\t(\n+\t\tcd df-conflict-error &&\n+\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n+\t\ttest_must_fail git fetch 2>../err &&\n+\t\tgit pack-refs --all &&\n+\t\ttest_must_fail git fetch 2>../err-packed\n+\t) &&\n+\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n+\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err &&\n+\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n+\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err-packed\n+'\n+\n test_expect_success 'fetching a one-level ref works' '\n \ttest_commit extra &&\n \tgit reset --hard HEAD^ &&\n\nbase-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377\n-- \ngitgitgadget\n"},{"id":"493963","messageId":"ZjSGX1eK-kSFKK1u@tanuki","threadId":"61397","inReplyTo":"pull.1716.v2.git.git.1714711829698.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] refs: return conflict error when checking packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-03T06:38:23Z","receivedAt":"2024-05-03T06:38:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 03, 2024 at 04:50:29AM +0000, Ivan Tse via GitGitGadget wrote:\n> From: Ivan Tse <ivan.tse1@gmail.com>\n> \n> The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\n> ref due to a name conflict with another ref. An example of this is a\n> directory/file conflict such as ref names A/B and A.\n> \n> \"git fetch\" uses this error code to more accurately describe the error\n> by recommending to the user that they try running \"git remote prune\" to\n> remove any old refs that are deleted by the remote which would clear up\n> any directory/file conflicts.\n> \n> This helpful error message is not displayed when the conflicted ref is\n> stored in packed refs. This change fixes this by ensuring error return\n> code consistency in `lock_raw_ref`.\n> \n> Signed-off-by: Ivan Tse <ivan.tse1@gmail.com>\n> ---\n>     refs: return conflict error when checking packed refs\n>     \n>     Changes against v1:\n>     \n>      * added test for the error message during git fetch\n>     \n>     Thanks for reviewing! I've gone ahead and attempted to add tests for\n>     this behavior. It tests that the error message is shown for both cases\n>     when the ref is stored as loose vs packed-refs. How does this test look?\n>     Also, should this test have a REFFILES prerequisite?\n\nThere is no need for the REFFILES prerequisite as you never access refs\non disk directly, but instead use our plumbing commands. Furthermore, we\ndo want to verify that both backends behave the same here. If the test\nfailed with the \"reftable\" backend I'd consider that to be a bug in the\nbackend.\n\n[snip]\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 33d34d5ae9e..ae0828e26a1 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'branchname D/F conflict rejected with targeted error message' '\n> +\tgit clone . df-conflict-error &&\n> +\tgit branch dir_conflict &&\n> +\t(\n> +\t\tcd df-conflict-error &&\n> +\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n> +\t\ttest_must_fail git fetch 2>../err &&\n> +\t\tgit pack-refs --all &&\n> +\t\ttest_must_fail git fetch 2>../err-packed\n> +\t) &&\n> +\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n> +\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err &&\n> +\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n> +\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err-packed\n\nI would personally add those calls to `test_grep` right after the\nrespective fetches to make the test a bit easier to follow.\n\nAlso, instead of using '\\''`, you can use the \"${SQ}\" variable to insert\nsingle quotes. So, something like this:\n\n    test_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err &&\n\nOther than that this patch looks good to me, thanks!\n\nPatrick\n\n> +'\n> +\n>  test_expect_success 'fetching a one-level ref works' '\n>  \ttest_commit extra &&\n>  \tgit reset --hard HEAD^ &&\n> \n> base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377\n> -- \n> gitgitgadget\n> \n"},{"id":"494073","messageId":"pull.1716.v3.git.git.1714791848557.gitgitgadget@gmail.com","threadId":"61397","inReplyTo":"pull.1716.v2.git.git.1714711829698.gitgitgadget@gmail.com","subject":"[PATCH v3] refs: return conflict error when checking packed refs","fromName":"Ivan Tse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-04T03:04:08Z","receivedAt":"2024-05-04T03:04:12Z","isPatch":true,"sender":{"key":"ivan.tse1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4662860?v=4"},"body":"From: Ivan Tse <ivan.tse1@gmail.com>\n\nThe TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\nref due to a name conflict with another ref. An example of this is a\ndirectory/file conflict such as ref names A/B and A.\n\n\"git fetch\" uses this error code to more accurately describe the error\nby recommending to the user that they try running \"git remote prune\" to\nremove any old refs that are deleted by the remote which would clear up\nany directory/file conflicts.\n\nThis helpful error message is not displayed when the conflicted ref is\nstored in packed refs. This change fixes this by ensuring error return\ncode consistency in `lock_raw_ref`.\n\nSigned-off-by: Ivan Tse <ivan.tse1@gmail.com>\n---\n    refs: return conflict error when checking packed refs\n    \n    Changes against v2:\n    \n     * move test_grep to follow after their respective git fetch commands\n     * Use \"${SQ}\" instead of '\\''\n    \n    Thanks for the explanation and for the comments. I've updated the\n    changes to incorporate your feedback!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1716%2Fivantsepp%2Freturn_name_conflict_error_for_packed_refs-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1716/ivantsepp/return_name_conflict_error_for_packed_refs-v3\nPull-Request: https://github.com/git/git/pull/1716\n\nRange-diff vs v2:\n\n 1:  58b2cda5c18 ! 1:  943a8629b5f refs: return conflict error when checking packed refs\n     @@ t/t5510-fetch.sh: test_expect_success 'branchname D/F conflict resolved by --pru\n      +\t(\n      +\t\tcd df-conflict-error &&\n      +\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n     -+\t\ttest_must_fail git fetch 2>../err &&\n     ++\t\ttest_must_fail git fetch 2>err &&\n     ++\t\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n     ++\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err &&\n      +\t\tgit pack-refs --all &&\n     -+\t\ttest_must_fail git fetch 2>../err-packed\n     -+\t) &&\n     -+\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n     -+\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err &&\n     -+\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n     -+\ttest_grep \" '\\''git remote prune origin'\\'' to remove any old, conflicting branches\" err-packed\n     ++\t\ttest_must_fail git fetch 2>err-packed &&\n     ++\t\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n     ++\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err-packed\n     ++\t)\n      +'\n      +\n       test_expect_success 'fetching a one-level ref works' '\n\n\n refs/files-backend.c |  4 +++-\n t/t5510-fetch.sh     | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a098d14ea00..97473f377d1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t */\n \t\tif (refs_verify_refname_available(\n \t\t\t\t    refs->packed_ref_store, refname,\n-\t\t\t\t    extras, NULL, err))\n+\t\t\t\t    extras, NULL, err)) {\n+\t\t\tret = TRANSACTION_NAME_CONFLICT;\n \t\t\tgoto error_return;\n+\t\t}\n \t}\n \n \tret = 0;\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 33d34d5ae9e..530369266fd 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'branchname D/F conflict rejected with targeted error message' '\n+\tgit clone . df-conflict-error &&\n+\tgit branch dir_conflict &&\n+\t(\n+\t\tcd df-conflict-error &&\n+\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n+\t\ttest_must_fail git fetch 2>err &&\n+\t\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n+\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err &&\n+\t\tgit pack-refs --all &&\n+\t\ttest_must_fail git fetch 2>err-packed &&\n+\t\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n+\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err-packed\n+\t)\n+'\n+\n test_expect_success 'fetching a one-level ref works' '\n \ttest_commit extra &&\n \tgit reset --hard HEAD^ &&\n\nbase-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377\n-- \ngitgitgadget\n"},{"id":"494116","messageId":"Zjh9GlzhJRcS1ZGG@tanuki","threadId":"61397","inReplyTo":"pull.1716.v3.git.git.1714791848557.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-06T06:47:54Z","receivedAt":"2024-05-06T06:47:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, May 04, 2024 at 03:04:08AM +0000, Ivan Tse via GitGitGadget wrote:\n> From: Ivan Tse <ivan.tse1@gmail.com>\n> \n> The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a\n> ref due to a name conflict with another ref. An example of this is a\n> directory/file conflict such as ref names A/B and A.\n> \n> \"git fetch\" uses this error code to more accurately describe the error\n> by recommending to the user that they try running \"git remote prune\" to\n> remove any old refs that are deleted by the remote which would clear up\n> any directory/file conflicts.\n> \n> This helpful error message is not displayed when the conflicted ref is\n> stored in packed refs. This change fixes this by ensuring error return\n> code consistency in `lock_raw_ref`.\n> \n> Signed-off-by: Ivan Tse <ivan.tse1@gmail.com>\n\nThis version looks good to me, thanks!\n\nPatrick\n"},{"id":"494148","messageId":"CAOLa=ZSre3f+0SR-_migfkPONqhinobKjU=NnGOJ_sTNM_L5ug@mail.gmail.com","threadId":"61397","inReplyTo":"pull.1716.v3.git.git.1714791848557.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-06T11:40:46Z","receivedAt":"2024-05-06T11:40:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello Ivan,\n\n\"Ivan Tse via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Ivan Tse <ivan.tse1@gmail.com>\n\n[snip]\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index a098d14ea00..97473f377d1 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n>  \t\t */\n>  \t\tif (refs_verify_refname_available(\n>  \t\t\t\t    refs->packed_ref_store, refname,\n> -\t\t\t\t    extras, NULL, err))\n> +\t\t\t\t    extras, NULL, err)) {\n> +\t\t\tret = TRANSACTION_NAME_CONFLICT;\n>  \t\t\tgoto error_return;\n> +\t\t}\n>  \t}\n>\n>  \tret = 0;\n>\n\nShouldn't we also do this change in `lock_ref_oid_basic` where we gather\nthe same lock again for creating the reflog entry?\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 33d34d5ae9e..530369266fd 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'branchname D/F conflict rejected with targeted error message' '\n> +\tgit clone . df-conflict-error &&\n> +\tgit branch dir_conflict &&\n> +\t(\n> +\t\tcd df-conflict-error &&\n> +\t\tgit update-ref refs/remotes/origin/dir_conflict/file HEAD &&\n> +\t\ttest_must_fail git fetch 2>err &&\n> +\t\ttest_grep \"error: some local refs could not be updated; try running\" err &&\n> +\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err &&\n> +\t\tgit pack-refs --all &&\n> +\t\ttest_must_fail git fetch 2>err-packed &&\n> +\t\ttest_grep \"error: some local refs could not be updated; try running\" err-packed &&\n> +\t\ttest_grep \" ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches\" err-packed\n> +\t)\n> +'\n> +\n>  test_expect_success 'fetching a one-level ref works' '\n>  \ttest_commit extra &&\n>  \tgit reset --hard HEAD^ &&\n>\n> base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377\n> --\n> gitgitgadget\n"},{"id":"494180","messageId":"xmqqttjaydbw.fsf@gitster.g","threadId":"61397","inReplyTo":"CAOLa=ZSre3f+0SR-_migfkPONqhinobKjU=NnGOJ_sTNM_L5ug@mail.gmail.com","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T19:01:23Z","receivedAt":"2024-05-06T19:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index a098d14ea00..97473f377d1 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n>>  \t\t */\n>>  \t\tif (refs_verify_refname_available(\n>>  \t\t\t\t    refs->packed_ref_store, refname,\n>> -\t\t\t\t    extras, NULL, err))\n>> +\t\t\t\t    extras, NULL, err)) {\n>> +\t\t\tret = TRANSACTION_NAME_CONFLICT;\n>>  \t\t\tgoto error_return;\n>> +\t\t}\n>>  \t}\n>>\n>>  \tret = 0;\n>>\n>\n> Shouldn't we also do this change in `lock_ref_oid_basic` where we gather\n> the same lock again for creating the reflog entry?\n\nAn interesting question.\n"},{"id":"494237","messageId":"CA+WCWMyxBA90QRsxn-i0H1TRi7WrhRcD=Tgj07f_Y_=UVNkhbw@mail.gmail.com","threadId":"61397","inReplyTo":"xmqqttjaydbw.fsf@gitster.g","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Ivan Tse","fromEmail":"ivan.tse1@gmail.com","sentAt":"2024-05-07T05:51:40Z","receivedAt":"2024-05-07T05:51:52Z","isPatch":true,"sender":{"key":"ivan.tse1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4662860?v=4"},"body":"On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> >> index a098d14ea00..97473f377d1 100644\n> >> --- a/refs/files-backend.c\n> >> +++ b/refs/files-backend.c\n> >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n> >>               */\n> >>              if (refs_verify_refname_available(\n> >>                                  refs->packed_ref_store, refname,\n> >> -                                extras, NULL, err))\n> >> +                                extras, NULL, err)) {\n> >> +                    ret = TRANSACTION_NAME_CONFLICT;\n> >>                      goto error_return;\n> >> +            }\n> >>      }\n> >>\n> >>      ret = 0;\n> >>\n> >\n> > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather\n> > the same lock again for creating the reflog entry?\n>\n> An interesting question.\n\nHi!\n\nApologies but I'm not sure I follow - could you elaborate? I am not\ntoo familiar with the Git source code (or C language) but from looking\nat `lock_ref_oid_basic`, it looks like that function returns a lock\nstruct and not an integer success/error code. I'm not sure how we\nwould apply this change in that function as well?\n"},{"id":"494276","messageId":"CAOLa=ZTu1to4HRc8sD2BcA+1ZzgcnXW9nHb1gg_VEG=nLOjHvg@mail.gmail.com","threadId":"61397","inReplyTo":"CA+WCWMyxBA90QRsxn-i0H1TRi7WrhRcD=Tgj07f_Y_=UVNkhbw@mail.gmail.com","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-07T13:37:08Z","receivedAt":"2024-05-07T13:37:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello Ivan,\n\nIvan Tse <ivan.tse1@gmail.com> writes:\n\n> On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> >> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> >> index a098d14ea00..97473f377d1 100644\n>> >> --- a/refs/files-backend.c\n>> >> +++ b/refs/files-backend.c\n>> >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs,\n>> >>               */\n>> >>              if (refs_verify_refname_available(\n>> >>                                  refs->packed_ref_store, refname,\n>> >> -                                extras, NULL, err))\n>> >> +                                extras, NULL, err)) {\n>> >> +                    ret = TRANSACTION_NAME_CONFLICT;\n>> >>                      goto error_return;\n>> >> +            }\n>> >>      }\n>> >>\n>> >>      ret = 0;\n>> >>\n>> >\n>> > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather\n>> > the same lock again for creating the reflog entry?\n>>\n>> An interesting question.\n>\n> Hi!\n>\n> Apologies but I'm not sure I follow - could you elaborate? I am not\n> too familiar with the Git source code (or C language) but from looking\n> at `lock_ref_oid_basic`, it looks like that function returns a lock\n> struct and not an integer success/error code. I'm not sure how we\n> would apply this change in that function as well?\n\nThat's correct, my question was merely that we also have a\n`refs_verify_refname_available` function call there and was wondering if\nwe somehow need to catch and propagate the same error from there. Like\nyou mentioned the way it is currently structured doesn't make that\npossible.\n"},{"id":"494294","messageId":"CA+WCWMxmpjeYfUGb=eymFF-xD7mopP0nVMSh+KHhOtrYU_euFA@mail.gmail.com","threadId":"61397","inReplyTo":"CAOLa=ZTu1to4HRc8sD2BcA+1ZzgcnXW9nHb1gg_VEG=nLOjHvg@mail.gmail.com","subject":"Re: [PATCH v3] refs: return conflict error when checking packed refs","fromName":"Ivan Tse","fromEmail":"ivan.tse1@gmail.com","sentAt":"2024-05-08T01:39:39Z","receivedAt":"2024-05-08T01:39:51Z","isPatch":true,"sender":{"key":"ivan.tse1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4662860?v=4"},"body":"Hi Karthik!\n\nOn Tue, May 7, 2024 at 9:37 AM Karthik Nayak <karthik.188@gmail.com> wrote:\n>\n> Hello Ivan,\n>\n[snip]\n>\n> That's correct, my question was merely that we also have a\n> `refs_verify_refname_available` function call there and was wondering if\n> we somehow need to catch and propagate the same error from there. Like\n> you mentioned the way it is currently structured doesn't make that\n> possible.\n\nAh I see, that makes sense. Thanks for the clarification and for the\ninitial question!\n"}]}