Skip to content

ldap: drop duplicate ldap_set_option() on Windows#20930

Closed
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:ldap-win-dedupe-set-option
Closed

ldap: drop duplicate ldap_set_option() on Windows#20930
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:ldap-win-dedupe-set-option

Conversation

@vszakats
Copy link
Copy Markdown
Member

@vszakats vszakats commented Mar 15, 2026

Already set after ldap_sslinit()/ldap_init() and before
ldap_ssl-specific initialization.

Follow-up to 39d1976 #19830
Follow-up to b41e65a
Follow-up to b91421b


/cc @gknauf

@vszakats vszakats added LDAP Windows Windows-specific tidy-up labels Mar 15, 2026
@vszakats
Copy link
Copy Markdown
Member Author

I found no sign that protocol version could be reset by other
set options, but also can't test this, without an LDAPS server.
If someone can, please do.

@vszakats vszakats requested a review from Copilot March 16, 2026 10:34
@vszakats
Copy link
Copy Markdown
Member Author

augment review

@vszakats
Copy link
Copy Markdown
Member Author

@aisle-analyzer

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 16, 2026

🤖 Augment PR Summary

Summary: Removes a redundant Windows-specific call to set the LDAP protocol version during bind setup.

Why: The protocol version is already configured earlier after ldap_sslinit()/ldap_init(), so the extra ldap_set_option(LDAP_OPT_PROTOCOL_VERSION, ...) before ldap_win_bind() was duplicate.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

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 removes a redundant Windows-only ldap_set_option(LDAP_OPT_PROTOCOL_VERSION, ...) call in the LDAP transfer path, relying on the existing option setup that already happens immediately after ldap_sslinit()/ldap_init().

Changes:

  • Drop duplicate ldap_set_option(... LDAP_OPT_PROTOCOL_VERSION ...) just before ldap_win_bind() on Windows builds.

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

You can also share your feedback on Copilot code review. Take the survey.

@vszakats vszakats requested a review from MarcelRaad March 16, 2026 10:37
@vszakats vszakats closed this in 9d104f5 Mar 16, 2026
@vszakats vszakats deleted the ldap-win-dedupe-set-option branch March 16, 2026 12:19
@vszakats
Copy link
Copy Markdown
Member Author

Thanks for verifying, @MarcelRaad!

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

Labels

Development

Successfully merging this pull request may close these issues.

3 participants