| Message ID | pull.1716.v3.git.git.1714791848557.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9339fca23efbe1a2f9dc916356be9d0a22d233c2 |
| Headers | show |
| Series | [v3] refs: return conflict error when checking packed refs | expand |
On Sat, May 04, 2024 at 03:04:08AM +0000, Ivan Tse via GitGitGadget wrote: > From: Ivan Tse <ivan.tse1@gmail.com> > > The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a > ref due to a name conflict with another ref. An example of this is a > directory/file conflict such as ref names A/B and A. > > "git fetch" uses this error code to more accurately describe the error > by recommending to the user that they try running "git remote prune" to > remove any old refs that are deleted by the remote which would clear up > any directory/file conflicts. > > This helpful error message is not displayed when the conflicted ref is > stored in packed refs. This change fixes this by ensuring error return > code consistency in `lock_raw_ref`. > > Signed-off-by: Ivan Tse <ivan.tse1@gmail.com> This version looks good to me, thanks! Patrick
Hello Ivan, "Ivan Tse via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Ivan Tse <ivan.tse1@gmail.com> [snip] > diff --git a/refs/files-backend.c b/refs/files-backend.c > index a098d14ea00..97473f377d1 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs, > */ > if (refs_verify_refname_available( > refs->packed_ref_store, refname, > - extras, NULL, err)) > + extras, NULL, err)) { > + ret = TRANSACTION_NAME_CONFLICT; > goto error_return; > + } > } > > ret = 0; > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather the same lock again for creating the reflog entry? > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 33d34d5ae9e..530369266fd 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' ' > test_cmp expect actual > ' > > +test_expect_success 'branchname D/F conflict rejected with targeted error message' ' > + git clone . df-conflict-error && > + git branch dir_conflict && > + ( > + cd df-conflict-error && > + git update-ref refs/remotes/origin/dir_conflict/file HEAD && > + test_must_fail git fetch 2>err && > + test_grep "error: some local refs could not be updated; try running" err && > + test_grep " ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches" err && > + git pack-refs --all && > + test_must_fail git fetch 2>err-packed && > + test_grep "error: some local refs could not be updated; try running" err-packed && > + test_grep " ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches" err-packed > + ) > +' > + > test_expect_success 'fetching a one-level ref works' ' > test_commit extra && > git reset --hard HEAD^ && > > base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377 > -- > gitgitgadget
Karthik Nayak <karthik.188@gmail.com> writes: >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index a098d14ea00..97473f377d1 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs, >> */ >> if (refs_verify_refname_available( >> refs->packed_ref_store, refname, >> - extras, NULL, err)) >> + extras, NULL, err)) { >> + ret = TRANSACTION_NAME_CONFLICT; >> goto error_return; >> + } >> } >> >> ret = 0; >> > > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather > the same lock again for creating the reflog entry? An interesting question.
On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote: > >> diff --git a/refs/files-backend.c b/refs/files-backend.c > >> index a098d14ea00..97473f377d1 100644 > >> --- a/refs/files-backend.c > >> +++ b/refs/files-backend.c > >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs, > >> */ > >> if (refs_verify_refname_available( > >> refs->packed_ref_store, refname, > >> - extras, NULL, err)) > >> + extras, NULL, err)) { > >> + ret = TRANSACTION_NAME_CONFLICT; > >> goto error_return; > >> + } > >> } > >> > >> ret = 0; > >> > > > > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather > > the same lock again for creating the reflog entry? > > An interesting question. Hi! Apologies but I'm not sure I follow - could you elaborate? I am not too familiar with the Git source code (or C language) but from looking at `lock_ref_oid_basic`, it looks like that function returns a lock struct and not an integer success/error code. I'm not sure how we would apply this change in that function as well?
Hello Ivan, Ivan Tse <ivan.tse1@gmail.com> writes: > On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> >> index a098d14ea00..97473f377d1 100644 >> >> --- a/refs/files-backend.c >> >> +++ b/refs/files-backend.c >> >> @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs, >> >> */ >> >> if (refs_verify_refname_available( >> >> refs->packed_ref_store, refname, >> >> - extras, NULL, err)) >> >> + extras, NULL, err)) { >> >> + ret = TRANSACTION_NAME_CONFLICT; >> >> goto error_return; >> >> + } >> >> } >> >> >> >> ret = 0; >> >> >> > >> > Shouldn't we also do this change in `lock_ref_oid_basic` where we gather >> > the same lock again for creating the reflog entry? >> >> An interesting question. > > Hi! > > Apologies but I'm not sure I follow - could you elaborate? I am not > too familiar with the Git source code (or C language) but from looking > at `lock_ref_oid_basic`, it looks like that function returns a lock > struct and not an integer success/error code. I'm not sure how we > would apply this change in that function as well? That's correct, my question was merely that we also have a `refs_verify_refname_available` function call there and was wondering if we somehow need to catch and propagate the same error from there. Like you mentioned the way it is currently structured doesn't make that possible.
Hi Karthik! On Tue, May 7, 2024 at 9:37 AM Karthik Nayak <karthik.188@gmail.com> wrote: > > Hello Ivan, > [snip] > > That's correct, my question was merely that we also have a > `refs_verify_refname_available` function call there and was wondering if > we somehow need to catch and propagate the same error from there. Like > you mentioned the way it is currently structured doesn't make that > possible. Ah I see, that makes sense. Thanks for the clarification and for the initial question!
diff --git a/refs/files-backend.c b/refs/files-backend.c index a098d14ea00..97473f377d1 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -794,8 +794,10 @@ static int lock_raw_ref(struct files_ref_store *refs, */ if (refs_verify_refname_available( refs->packed_ref_store, refname, - extras, NULL, err)) + extras, NULL, err)) { + ret = TRANSACTION_NAME_CONFLICT; goto error_return; + } } ret = 0; diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 33d34d5ae9e..530369266fd 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1091,6 +1091,22 @@ test_expect_success 'branchname D/F conflict resolved by --prune' ' test_cmp expect actual ' +test_expect_success 'branchname D/F conflict rejected with targeted error message' ' + git clone . df-conflict-error && + git branch dir_conflict && + ( + cd df-conflict-error && + git update-ref refs/remotes/origin/dir_conflict/file HEAD && + test_must_fail git fetch 2>err && + test_grep "error: some local refs could not be updated; try running" err && + test_grep " ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches" err && + git pack-refs --all && + test_must_fail git fetch 2>err-packed && + test_grep "error: some local refs could not be updated; try running" err-packed && + test_grep " ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches" err-packed + ) +' + test_expect_success 'fetching a one-level ref works' ' test_commit extra && git reset --hard HEAD^ &&