Re: [PATCH v2 00/16] Make Gnome Credential helper more Gnome-y and support ancient distros

From: Jeff King <peff@peff.net>
Date: Sep 24, 2013, 03:49 UTC
Message-ID: <20130924034932.GB2766@sigill.intra.peff.net>
In-Reply-To: <1379962157-1338-1-git-send-email-bcasey@nvidia.com>

On Mon, Sep 23, 2013 at 11:49:01AM -0700, Brandon Casey wrote:

> Brandon Casey (16):
>   contrib/git-credential-gnome-keyring.c: remove unnecessary
>     pre-declarations
> [...]

Thanks, I think this is a good change. There were patches[1] from Philipp about a year ago that went in the opposite direction, factoring out common credential functions to be used by several helpers. We ended up not merging them, because porting the wincred helper to the generic implementation introduced complications.

And that was part of the reason for splitting out the helpers in the first place: to let them worry about their own individual system-specific quirks (and take advantage of system-specific helpers). IOW, to make the gnome helper more gnome-y than git-y. And your patches go in that direction, which I think is sane.

[1] http://thread.gmane.org/gmane.comp.version-control.git/204154/focus=204157

>   contrib/git-credential-gnome-keyring.c: report failure to store
>     password

Your subject lines are rather on the long side. I wonder if they would be more readable as just:

  gnome-keyring: report failure to store password

and so forth. Anyone familiar with git.git knows that the "contrib" and "git-credential-" bits are implied by "gnome-keyring". But it's not a huge deal either way.

> Inserts a patch to fix the style issues for block statements.
> i.e. use "if ()" instead of "if()"

There are a ton of other style issues in the existing code (lack of whitespace around operators and function arguments, and "char* foo" instead of "char *foo" were the two I noticed). As a separate contrib/ project, I'm not that concerned, and I would say it is up to whoever is nominally in charge of maintaining that contrib directory (Philipp or John, in this case).

-Peff