Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: However, the shared bits are simple enough that maybe that is not a concern. An interesting test would be to add a 5/4 porting Erik's win32 credential helper, since that is the platform least like our other ones. Very true. So I am OK with this series, but I am also OK with leaving it at patch 1, and just keeping the implementations separate. Amen. Just to make sure we do not leave loose ends, could somebody try to see if the new generic helper infrastructure is useful to shrink Erik's win32 credential helper implementation? If we see much code reduction and improved clarity, this refactoring may worth keeping. Otherwise it may be sufficient to drop the later ones in the series. Without knowing which, it is hard to decide. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
n 26/08/12 19:46, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: However, the shared bits are simple enough that maybe that is not a concern. An interesting test would be to add a 5/4 porting Erik's win32 credential helper, since that is the platform least like our other ones. Very true. In principle, I agree that with the current set of helper implementations, the generic infrastructure may not yet pay off. So I am OK with this series, but I am also OK with leaving it at patch 1, and just keeping the implementations separate. Amen. Just to make sure we do not leave loose ends, could somebody try to see if the new generic helper infrastructure is useful to shrink Erik's win32 credential helper implementation? I'll try to give it a shot now, although I won't be able to test it. I'll send the results as a single 5/4 addition to the previous series. If we see much code reduction and improved clarity, this refactoring may worth keeping. Otherwise it may be sufficient to drop the later ones in the series. Without knowing which, it is hard to decide. If we decide that the generic implementation is useful, I'll then resend the reordered series adding the generic helper first, then refactoring the existing ones and adding the new one for GnomeKeyring last. Thanks, Philipp -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
Philipp A. Hartmann p...@qo.cx writes: 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib: GnomeKeyring support + generic helper implementation
On Fri, Aug 24, 2012 at 11:15:36AM -0700, Junio C Hamano wrote: The third and fourth patches port the existing helpers to this generic implementation. 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 think this is partially because I talked with Phillipp off-list and was somewhat unsure how much we wanted to factor out of the helpers. My initial thought was that the protocol would be sufficiently simple that helpers would just be stand-alone and not need to share code with each other. Then the generic bits would not have to worry about being cross-platform compatible. However, the shared bits are simple enough that maybe that is not a concern. An interesting test would be to add a 5/4 porting Erik's win32 credential helper, since that is the platform least like our other ones. So I am OK with this series, but I am also OK with leaving it at patch 1, and just keeping the implementations separate. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
