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 |
|
Venkat Ragavan wrote on the Git mailing list (how to reply to this email): [Resending as plain text after the mailing list rejected the HTML version.]
> 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.
Thanks for following up, and thanks Junio for explaining the setup. Since
the loop intentionally exercises loose, packed, and mixed ref layouts, I'll
leave the test unchanged.
On Fri, Oct 16, 2026 at 8:47 AM Venkat Ragavan <mail2venkat21@gmail.com> wrote:
>
> > 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.
>
> Thanks for following up, and thanks Junio for explaining the setup. Since
> the loop intentionally exercises loose, packed, and mixed ref layouts, I’ll
> leave the test unchanged.
>
> On Fri, Oct 16, 2026 at 2:47 AM Harald Nordgren <haraldnordgren@gmail.com> wrote:
>>
>> > > 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 |
|
This branch is now known as |
|
This patch series was integrated into seen via git@9e6062a. |
|
@gitster are we good to close this? I see that the patch is already integrated. |
@venkatr21 please note that the fearless Git maintainer does not frequent GitHub, but almost exclusively works within the bounds of the Git mailing list. That's the entire point of GitGitGadget, to let contributors enjoy at least some of the convenience of GitHub Pull Requests yet still be able to contribute to the Git project. As to closing this PR, that should be GitGitGadget's job. And it typically does that (using heuristics, because your commits will never end up in Git as-are, if they are integrated into Git, they will first get projected from the code space into the email dimensions, re-expanded into commits on the other side that are quite potentially based on totally different base commits, and hence losing at least some of the confidence that resulted from your own testing of your commits). Most importantly, the commits on the other side will (if they are deemed worthy, which is not the case with all of them, see e.g. https://lore.kernel.org/git/20261002081846.25144-1-scott@gitbutler.net/, which was seen and discussed) first go into a branch in the https://github.com/gitster/git repository (mirrored into gitgitgadget/git by a scheduled workflow), and then they will get merged into the Note that The next step, which will be a lot closer to acceptance, will be for that branch to be merged into Before that, however, the patch needs to see some reviews. That is where your patch is currently at. In this patch's instance, I would not be surprised at all if a reviewer pointed out that the test case adds quite some computation to the already very, very expensive CI runs: Your branch alone cost 8h 41m 49s total. Actually, I take that back, I would be surprised if any reviewer was mindful of the test suite's runtime, I mean, there is only one logical explanation how we got to that huge run time in the first place. Once reviews have been performed, resulted in recommendations to merge, and there were no opposing views, and at least after cooking for a couple of days or more often weeks, the branch will advance to the That is the time when GitGitGadget will close the PR (and for technical reasons, it will not be marked as 
@venkatr21 if you have any splendid idea how all of this information (or at least the parts relevant to GitGitGadget users) could be surfaced more efficiently, I'd be really interested in your suggestions (preferably as a PR at https://github.com/gitgitgadget/gitgitgadget). |
|
There was a status update in the "Cooking" section about the branch The logic to seek through loose references across directory boundaries has been corrected by including the trailing slash when comparing directory components, and by properly adjusting the iterator position when leaf entries sort either strictly before or after the seek point. Waiting for review. cf. <CAFuK-oarU1rZOcDzYr=YA1SNvfAzyB6LaXXCjtZV6WWGswOnyw@mail.gmail.com> source: <pull.2258.git.1792067455341.gitgitgadget@gmail.com> |
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