Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 18, 2024, 06:38 UTC
Message-ID: <CAPig+cQ_SCBNc7-x=eTz+kMg0_bg4uc2SubbJpq=i7Ok_Y719Q@mail.gmail.com>
In-Reply-To: <pull.1667.git.1708212896.gitgitgadget@gmail.com>

On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 8 quoted lines

> git-credential-osxkeychain has largely fallen behind other external
> credential helpers in the features it supports, and hasn't received any
> functional changes since 2013. [...]
>
> osxkeychain also made use of macOS APIs that had been deprecated since 2014.
> Replacement API was able to be used without regressing the minimum supported
> macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer
> in osxkeychain, 2023-05-01).

Although I'm not familiar with the SecItem API nor with the Git keychain API, I gave this series a readthrough and left a few minor comments. Aside from a few very minor style nits, perhaps the only substantive comment was that patch [1/4] could do a slightly better job of protecting against future programmer error, but even that is minor in that it doesn't impact the functionality actually implemented by the patch, thus may not be worth a reroll.

Overall, despite not being familiar with the APIs in question, everything I read in the patches made sense and was cleanly implemented. Nicely done.