refs: return conflict error when checking packed refs #1716
Conversation
Welcome to GitGitGadgetHi @ivantsepp, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
ivantsepp
force-pushed
the
return_name_conflict_error_for_packed_refs
branch
3 times, most recently
from
186a589
to
a87ba26
Compare
|
/allow |
|
User ivantsepp is now allowed to use GitGitGadget. |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Apr 30, 2024 at 02:50:47PM +0000, Ivan Tse via GitGitGadget wrote:
> From: Ivan Tse <[email protected]>
>
> 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`.
The change itself makes sense to me. But I'd like to see a test that
demonstrates the new behaviour so that we don't regress this in the
future.
Thanks!
Patrick |
|
User |
ivantsepp
force-pushed
the
return_name_conflict_error_for_packed_refs
branch
from
a87ba26
to
58b2cda
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, May 03, 2024 at 04:50:29AM +0000, Ivan Tse via GitGitGadget wrote:
> From: Ivan Tse <[email protected]>
>
> 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 <[email protected]>
> ---
> refs: return conflict error when checking packed refs
>
> Changes against v1:
>
> * added test for the error message during git fetch
>
> Thanks for reviewing! I've gone ahead and attempted to add tests for
> this behavior. It tests that the error message is shown for both cases
> when the ref is stored as loose vs packed-refs. How does this test look?
> Also, should this test have a REFFILES prerequisite?
There is no need for the REFFILES prerequisite as you never access refs
on disk directly, but instead use our plumbing commands. Furthermore, we
do want to verify that both backends behave the same here. If the test
failed with the "reftable" backend I'd consider that to be a bug in the
backend.
[snip]
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 33d34d5ae9e..ae0828e26a1 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 &&
> + 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 &&
> + test_grep " '\''git remote prune origin'\'' to remove any old, conflicting branches" err &&
> + test_grep "error: some local refs could not be updated; try running" err-packed &&
> + test_grep " '\''git remote prune origin'\'' to remove any old, conflicting branches" err-packed
I would personally add those calls to `test_grep` right after the
respective fetches to make the test a bit easier to follow.
Also, instead of using '\''`, you can use the "${SQ}" variable to insert
single quotes. So, something like this:
test_grep " ${SQ}git remote prune origin${SQ} to remove any old, conflicting branches" err &&
Other than that this patch looks good to me, thanks!
Patrick
> +'
> +
> test_expect_success 'fetching a one-level ref works' '
> test_commit extra &&
> git reset --hard HEAD^ &&
>
> base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377
> --
> gitgitgadget
> |
|
User |
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 <[email protected]>
ivantsepp
force-pushed
the
return_name_conflict_error_for_packed_refs
branch
from
58b2cda
to
943a862
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Sat, May 04, 2024 at 03:04:08AM +0000, Ivan Tse via GitGitGadget wrote:
> From: Ivan Tse <[email protected]>
>
> 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 <[email protected]>
This version looks good to me, thanks!
Patrick |
|
On the Git mailing list, Karthik Nayak wrote (reply to this): Hello Ivan,
"Ivan Tse via GitGitGadget" <[email protected]> writes:
> From: Ivan Tse <[email protected]>
[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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Karthik Nayak <[email protected]> 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. |
|
This branch is now known as |
|
This patch series was integrated into seen via b16fe0b. |
|
There was a status update in the "New Topics" section about the branch Comments? cf. <CAOLa=ZSre3f+0SR-_migfkPONqhinobKjU=NnGOJ_sTNM_L5ug@mail.gmail.com> source: <[email protected]> |
|
This patch series was integrated into seen via bc113dd. |
|
On the Git mailing list, Ivan Tse wrote (reply to this): On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <[email protected]> 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? |
|
On the Git mailing list, Karthik Nayak wrote (reply to this): Hello Ivan,
Ivan Tse <[email protected]> writes:
> On Mon, May 6, 2024 at 3:01 PM Junio C Hamano <[email protected]> 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. |
|
This patch series was integrated into seen via 00a7969. |
|
This patch series was integrated into seen via f6a1496. |
|
On the Git mailing list, Ivan Tse wrote (reply to this): Hi Karthik!
On Tue, May 7, 2024 at 9:37 AM Karthik Nayak <[email protected]> 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! |
|
This patch series was integrated into seen via 53e0675. |
|
There was a status update in the "Cooking" section about the branch Comments? cf. <CAOLa=ZSre3f+0SR-_migfkPONqhinobKjU=NnGOJ_sTNM_L5ug@mail.gmail.com> source: <[email protected]> |
|
This patch series was integrated into seen via 575476a. |
|
This patch series was integrated into seen via d7507bc. |
|
There was a status update in the "Cooking" section about the branch Comments? cf. <CAOLa=ZSre3f+0SR-_migfkPONqhinobKjU=NnGOJ_sTNM_L5ug@mail.gmail.com> source: <[email protected]> |
|
This patch series was integrated into seen via 21497ca. |
|
This patch series was integrated into seen via 9e94e6a. |
|
This patch series was integrated into seen via 7a9b667. |
|
This patch series was integrated into seen via 43b6863. |
|
There was a status update in the "Cooking" section about the branch Expose "name conflict" error when a ref creation fails due to D/F conflict in the ref namespace, to improve an error message given by "git fetch". Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via f80a062. |
|
This patch series was integrated into next via 39ef3ec. |
|
This patch series was integrated into seen via 34b6441. |
|
This patch series was integrated into seen via a01dd0a. |
|
There was a status update in the "Cooking" section about the branch Expose "name conflict" error when a ref creation fails due to D/F conflict in the ref namespace, to improve an error message given by "git fetch". Will merge to 'master'. source: <[email protected]> |
Changes against v2:
test_grepto follow after their respectivegit fetchcommands"${SQ}"instead of'\''Thanks for the explanation and for the comments. I've updated the changes to incorporate your feedback!
cc: Patrick Steinhardt [email protected]
cc: Karthik Nayak [email protected]