Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2012, 18:15 UTC
Message-ID: <7vfw7cyx4n.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1345741068-11004-1-git-send-email-pah@qo.cx>

"Philipp A. Hartmann" <pah@qo.cx> writes:

Show 21 quoted lines

> All,
>
> the following patch series proposes enhancements to the credential helper
> implementations in the contrib section.  The detailed development history
> can be found at GitHub [1].
>
>   The first patch adds a GnomeKeyring credential backend.  The GnomeKeyring
> specific parts are based on the work by John Szakmeister, who wrote the
> helper originally for the initial, unpublished version of the credential
> helper protocol.
>
>   The second patch adds a generic implementation of a credential helper
> based on a simplified credential API and common helper functions.  Helpers
> based on this implementation do not need to worry about the credential
> protocol and only need to implement callback functions for the different
> credential operations.
>
>   The third and fourth patches port the existing helpers to this generic
> implementation.
>
> Looking forward to your thoughts.

It struck me somewhat odd to see a new one added as the first step in the series, and then "the generic", the third patch only to lose code from the first one, and then use the same code reduction of existing one with the last one in the series. A natural progression would have been the introduction of generic infrastructure (including the tiny bit this series had to add as part of 4/4) as the first patch, update existing OSX one to it as the second, and then build a new Gnome one on the infrastructure as the third and final patch; that way we have to review less code, too ;-).

I gave it a cursory look; other than getting distracted by inconsistent coding styles here and there, the patches seemed reasonably clean and well thought out.

Thanks.