Skip to content

Fixing cmpfunc: fix sort to distinguish prefix-overlapping query keys#438

Open
arpitguptagithub wants to merge 2 commits intocurl:masterfrom
arpitguptagithub:fix/cmpfunc-prefix-sort
Open

Fixing cmpfunc: fix sort to distinguish prefix-overlapping query keys#438
arpitguptagithub wants to merge 2 commits intocurl:masterfrom
arpitguptagithub:fix/cmpfunc-prefix-sort

Conversation

@arpitguptagithub
Copy link
Copy Markdown

When one query key is a prefix of another (e.g. 'a' vs 'ab'), the comparison function returned 0 (equal), making --sort-query produce non-deterministic order for such pairs.

I fix it by returning the length difference when the common prefix matches, so shorter keys sort before longer ones.

Adding a test with prefix-overlapping query keys to verify deterministic sort order.

@arpitguptagithub arpitguptagithub force-pushed the fix/cmpfunc-prefix-sort branch 2 times, most recently from 296d7f5 to 7d9f271 Compare April 11, 2026 09:16
@bagder bagder requested a review from Copilot April 12, 2026 20:56
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes query sorting determinism in trurl by making the query-pair comparator distinguish strings where one is a prefix of the other, and adds a regression test intended to cover prefix-overlapping query keys.

Changes:

  • Update cmpfunc to break ties when one query pair is a strict prefix of another (shorter sorts first).
  • Add a new --sort-query test case intended to verify deterministic ordering with prefix-overlapping keys.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
trurl.c Adjusts the qsort comparator so prefix-overlapping strings no longer compare equal.
tests.json Adds a regression test for --sort-query ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests.json
Comment thread trurl.c
@arpitguptagithub
Copy link
Copy Markdown
Author

@bagder ig now this will be okay, have a look? please

@bagder
Copy link
Copy Markdown
Member

bagder commented Apr 23, 2026

This branch cannot be rebased due to conflicts

Please rebase your branch ontop of master and force-push

When one query key is a prefix of another (e.g. 'a' vs 'ab'), the
comparison function returned 0 (equal), making --sort-query produce
non-deterministic order for such pairs.

Fix by returning the length difference when the common prefix matches,
so shorter keys sort before longer ones.

Add a test with prefix-overlapping query keys to verify deterministic
sort order.
@arpitguptagithub arpitguptagithub force-pushed the fix/cmpfunc-prefix-sort branch 6 times, most recently from 5acbd43 to 61e8698 Compare April 25, 2026 23:15
@arpitguptagithub arpitguptagithub force-pushed the fix/cmpfunc-prefix-sort branch from 61e8698 to 9546979 Compare April 25, 2026 23:18
@arpitguptagithub
Copy link
Copy Markdown
Author

arpitguptagithub commented Apr 25, 2026

FYI

I have added a regresiion test---sort-query with prefix-overlapping keys (ab=2&a=1&abc=3) verifying correct sort output (a=1&ab=2&abc=3).
and along with that also added a "excludes": ["uppercase-hex"] to 3 existing upstream tests (tests 52, 132, 172): These tests have lowercase hex in their expected output (%3d, %5c, %2f) but fail on CI builds linked against libcurl 8.20.0-DEV which produces uppercase hex (%3D, %5C, %2F) The output on the github Actions UI for test output made it very difficult to spot the actual difference.

anyhow adding the excludes annotation skips them on uppercase-hex builds, matching the pattern already used by other tests in the repo (e.g. tests around line 717).

These are pre-existing failures unrelated to the cmpfunc fix.

Hope thats fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants