Repository navigation
Conversation
When seeking through loose refs, cache_ref_iterator_seek() compares
each directory component without its trailing slash. This can confuse
a directory with a similarly named ref that sorts before it.
For example, with these refs:
refs/tags/releases-z
refs/tags/releases/v1
refs/tags/releases/v2
Starting after refs/tags/releases/v1 can position the iterator at
releases-z instead of descending into releases/. Iteration then
returns v1 again, even though it should return only v2.
Include the trailing slash when comparing directory components so
that the seek follows the correct directory.
Also handle two related positioning errors. If every entry in a
directory precedes the cursor, stop searching that level rather than
descending into its last entry. If a leaf sorts after the cursor,
leave the iterator positioned to return it, even when the cursor has
remaining path components.
Add regression tests for shared-prefix names, missing nested markers,
and markers beyond the end of the ref listing, using loose, packed,
and mixed refs.
Signed-off-by: Venkat Ragavan S <mail2venkat21@gmail.com>
Welcome to GitGitGadgetHi @venkatr21, 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: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, 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 git-mentoring@googlegroups.com, 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, |
|
/allow |
|
Error: User larsxschneider is not yet permitted to use GitGitGadget |
|
/allow |
|
User venkatr21 is now allowed to use GitGitGadget. WARNING: venkatr21 has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
|
/allow larsxschneider |
|
User larsxschneider is now allowed to use GitGitGadget. WARNING: larsxschneider has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
|
@venkatr21 don't worry about those three CI failures. I hit them, too. This is fixed in |
@dscho Thanks for mentioning that, I was trying to debug what went wrong there. |
|
/preview |
|
Preview email sent as pull.2258.git.1792067169794.gitgitgadget@gmail.com |
|
Preview email sent as pull.2258.git.1792067204341.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.2258.git.1792067455341.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): The general idea looks sound.
> + if test "$storage" = mixed
> + then
> + git pack-refs --all || return 1
> + fi &&
I don't see any reason why a test would need an if block, we know what data we
expect, no?
Harald |
|
User |
|
"D. Ben Knoble" wrote on the Git mailing list (how to reply to this email): On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
<haraldnordgren@gmail.com> wrote:
[snip]
> > + if test "$storage" = mixed
> > + then
> > + git pack-refs --all || return 1
> > + fi &&
>
> I don't see any reason why a test would need an if block, we know what data we
> expect, no?
I didn't examine too closely, but this test is wrapped in a loop over
the storage variable that appears to determine whether and when we
pack-refs, so I think the conditional makes sense for that?
--
D. Ben Knoble |
|
User |
|
"D. Ben Knoble" wrote on the Git mailing list (how to reply to this email): [resending with corrected plain-text ':D]
> On Thu, 15 Oct 2026 at 10:38 PM, D. Ben Knoble <ben.knoble@gmail.com> wrote:
>>
>> On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
>> <haraldnordgren@gmail.com> wrote:
>> [snip]
>> > > + if test "$storage" = mixed
>> > > + then
>> > > + git pack-refs --all || return 1
>> > > + fi &&
>> >
>> > I don't see any reason why a test would need an if block, we know what data we
>> > expect, no?
>>
>> I didn't examine too closely, but this test is wrapped in a loop over
>> the storage variable that appears to determine whether and when we
>> pack-refs, so I think the conditional makes sense for that?
On Thu, Oct 15, 2026 at 1:13 PM Venkat Ragavan <mail2venkat21@gmail.com> wrote:
>
> Yes it does, I interpreted Harald’s suggestion was to write separate tests instead of looping over a variable outside the test block, either ways the logic of the test is going to remain the same.
[we bottom-post here]
I don’t mind the loop, personally; it might be worth looking around
for other tests and see how they approach it.
You can also extract shared sections into functions if that makes
writing separate tests easier.
--
D. Ben Knoble |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Harald Nordgren <haraldnordgren@gmail.com> writes:
> The general idea looks sound.
>
>> + if test "$storage" = mixed
>> + then
>> + git pack-refs --all || return 1
>> + fi &&
>
> I don't see any reason why a test would need an if block, we know what data we
> expect, no?
This is three tests implemented as a single test with a for-loop
around it. The variable instructs the single test_expect_success
to behave slightly differently depending on its setting.
+for storage in loose packed mixed
+do
+ test_expect_success "start after respects directory boundaries ($storage)" '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit --no-tag base &&
+ git update-ref refs/tags/packed HEAD &&
+ if test "$storage" = mixed
+ then
+ git pack-refs --all || return 1
+ fi &&
+ git update-ref --stdin <<-\EOF &&
+ create refs/tags/a-leaf HEAD
+ ...
+ EOF
+ if test "$storage" = packed
+ then
+ git pack-refs --all || return 1
+ fi &&
+ ... real test in the repository begins ...
+ )
+ '
+done
It creates a tag "packed", and packs when storage is "mixed". Then
we create many other refs, and packs the whole refs when storage is
"packed". So during these three runs of the for-loop, the preparation
part quoted above behaves this way:
storage=loose no 'git pack-refs' run and every ref is loose
storage=packed the later 'git pack-refs' packs everything
storage=mixed 'packed' tag is packed, everything else is loose
and the test checks what happens in this repository with refs packed
in three different ways. |
|
Harald Nordgren wrote on the Git mailing list (how to reply to this email): > > The general idea looks sound.
> >
> >> + if test "$storage" = mixed
> >> + then
> >> + git pack-refs --all || return 1
> >> + fi &&
> >
> > I don't see any reason why a test would need an if block, we know what data we
> > expect, no?
>
> This is three tests implemented as a single test with a for-loop
> around it. The variable instructs the single test_expect_success
> to behave slightly differently depending on its setting.
I realize now that I didn't read the code properly. I didn't expect to
see a loop encapsulating a call to test_expect_success. That explains
the if case as well.
Harald |
3 participants
CC: Karthik Nayak karthik.188@gmail.com, Patrick Steinhardt ps@pks.im
cc: Harald Nordgren haraldnordgren@gmail.com
cc: "D. Ben Knoble" ben.knoble@gmail.com