{"thread":{"id":"44261","subject":"[PATCH] contrib: add credential helper for libsecret","startedAt":"2016-10-09T12:34:54Z","lastAt":"2016-10-11T20:32:21Z","messageCount":13,"participants":["Mantas Mikulėnas","Jeff King","Dennis Kaarsemaker","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"303673","messageId":"20161009123417.147239-1-grawity@gmail.com","threadId":"44261","inReplyTo":null,"subject":"[PATCH] contrib: add credential helper for libsecret","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2016-10-09T12:34:17Z","receivedAt":"2016-10-09T12:34:54Z","isPatch":true,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"This is based on the existing gnome-keyring helper, but instead of\nlibgnome-keyring (which was specific to GNOME and is deprecated), it\nuses libsecret which can support other implementations of XDG Secret\nService API.\n\nPasses t0303-credential-external.sh.\n\nSigned-off-by: Mantas Mikulėnas \n---\n contrib/credential/libsecret/Makefile | 25 ++\n .../libsecret/git-credential-libsecret.c | 370 +++++++++++++++++++++\n 2 files changed, 395 insertions(+)\n create mode 100644 contrib/credential/libsecret/Makefile\n create mode 100644 contrib/credential/libsecret/git-credential-libsecret.c\n\ndiff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile\nnew file mode 100644\nindex 000000000000..3e67552cc5b5\n--- /dev/null\n+++ b/contrib/credential/libsecret/Makefile\n@@ -0,0 +1,25 @@\n+MAIN:=git-credential-libsecret\n+all:: $(MAIN)\n+\n+CC = gcc\n+RM = rm -f\n+CFLAGS = -g -O2 -Wall\n+PKG_CONFIG = pkg-config\n+\n+-include ../../../config.mak.autogen\n+-include ../../../config.mak\n+\n+INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)\n+LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)\n+\n+SRCS:=$(MAIN).c\n+OBJS:=$(SRCS:.c=.o)\n+\n+%.o: %.c\n+\t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n+\n+$(MAIN): $(OBJS)\n+\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n+\n+clean:\n+\t@$(RM) $(MAIN) $(OBJS)\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nnew file mode 100644\nindex 000000000000..4c56979d8a08\n--- /dev/null\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -0,0 +1,370 @@\n+/*\n+ * Copyright (C) 2011 John Szakmeister \n+ * 2012 Philipp A. Hartmann \n+ * 2016 Mantas Mikulėnas \n+ *\n+ * This program is free software; you can redistribute it and/or modify\n+ * it under the terms of the GNU General Public License as published by\n+ * the Free Software Foundation; either version 2 of the License, or\n+ * (at your option) any later version.\n+ *\n+ * This program is distributed in the hope that it will be useful,\n+ * but WITHOUT ANY WARRANTY; without even the implied warranty of\n+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n+ * GNU General Public License for more details.\n+ *\n+ * You should have received a copy of the GNU General Public License\n+ * along with this program; if not, write to the Free Software\n+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n+ */\n+\n+/*\n+ * Credits:\n+ * - GNOME Keyring API handling originally written by John Szakmeister\n+ * - ported to credential helper API by Philipp A. Hartmann\n+ */\n+\n+#include \n+#include \n+#include \n+#include \n+#include \n+\n+/*\n+ * This credential struct and API is simplified from git's credential.{h,c}\n+ */\n+struct credential {\n+\tchar *protocol;\n+\tchar *host;\n+\tunsigned short port;\n+\tchar *path;\n+\tchar *username;\n+\tchar *password;\n+};\n+\n+#define CREDENTIAL_INIT { NULL, NULL, 0, NULL, NULL, NULL }\n+\n+typedef int (*credential_op_cb)(struct credential *);\n+\n+struct credential_operation {\n+\tchar *name;\n+\tcredential_op_cb op;\n+};\n+\n+#define CREDENTIAL_OP_END { NULL, NULL }\n+\n+/* ----------------- Secret Service functions ----------------- */\n+\n+static char *make_label(struct credential *c)\n+{\n+\tif (c->port)\n+\t\treturn g_strdup_printf(\"Git: %s://%s:%hu/%s\",\n+\t\t\t\t\tc->protocol, c->host, c->port, c->path ? c->path : \"\");\n+\telse\n+\t\treturn g_strdup_printf(\"Git: %s://%s/%s\",\n+\t\t\t\t\tc->protocol, c->host, c->path ? c->path : \"\");\n+}\n+\n+static GHashTable *make_attr_list(struct credential *c)\n+{\n+\tGHashTable *al = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_free);\n+\n+\tif (c->username)\n+\t\tg_hash_table_insert(al, \"user\", g_strdup(c->username));\n+\tif (c->protocol)\n+\t\tg_hash_table_insert(al, \"protocol\", g_strdup(c->protocol));\n+\tif (c->host)\n+\t\tg_hash_table_insert(al, \"server\", g_strdup(c->host));\n+\tif (c->port)\n+\t\tg_hash_table_insert(al, \"port\", g_strdup_printf(\"%hu\", c->port));\n+\tif (c->path)\n+\t\tg_hash_table_insert(al, \"object\", g_strdup(c->path));\n+\n+\treturn al;\n+}\n+\n+static int keyring_get(struct credential *c)\n+{\n+\tSecretService *service = NULL;\n+\tGHashTable *attributes = NULL;\n+\tGError *error = NULL;\n+\tGList *items = NULL;\n+\n+\tif (!c->protocol || !(c->host || c->path))\n+\t\treturn EXIT_FAILURE;\n+\n+\tservice = secret_service_get_sync(0, NULL, &error);\n+\tif (error != NULL) {\n+\t\tg_critical(\"could not connect to Secret Service: %s\", error->message);\n+\t\tg_error_free(error);\n+\t\treturn EXIT_FAILURE;\n+\t}\n+\n+\tattributes = make_attr_list(c);\n+\titems = secret_service_search_sync(service,\n+\t\t\t\t\t SECRET_SCHEMA_COMPAT_NETWORK,\n+\t\t\t\t\t attributes,\n+\t\t\t\t\t SECRET_SEARCH_LOAD_SECRETS,\n+\t\t\t\t\t NULL,\n+\t\t\t\t\t &error);\n+\tg_hash_table_unref(attributes);\n+\tif (error != NULL) {\n+\t\tg_critical(\"lookup failed: %s\", error->message);\n+\t\tg_error_free(error);\n+\t\treturn EXIT_FAILURE;\n+\t}\n+\n+\tif (items != NULL) {\n+\t\tSecretItem *item;\n+\t\tSecretValue *secret;\n+\t\tconst char *s;\n+\n+\t\titem = items->data;\n+\t\tsecret = secret_item_get_secret(item);\n+\t\tattributes = secret_item_get_attributes(item);\n+\n+\t\ts = g_hash_table_lookup(attributes, \"user\");\n+\t\tif (s) {\n+\t\t\tg_free(c->username);\n+\t\t\tc->username = g_strdup(s);\n+\t\t}\n+\n+\t\ts = secret_value_get_text(secret);\n+\t\tif (s) {\n+\t\t\tg_free(c->password);\n+\t\t\tc->password = g_strdup(s);\n+\t\t}\n+\n+\t\tg_hash_table_unref(attributes);\n+\t\tsecret_value_unref(secret);\n+\t\tg_list_free_full(items, g_object_unref);\n+\t}\n+\n+\treturn EXIT_SUCCESS;\n+}\n+\n+\n+static int keyring_store(struct credential *c)\n+{\n+\tchar *label = NULL;\n+\tGHashTable *attributes = NULL;\n+\tGError *error = NULL;\n+\n+\t/*\n+\t * Sanity check that what we are storing is actually sensible.\n+\t * In particular, we can't make a URL without a protocol field.\n+\t * Without either a host or pathname (depending on the scheme),\n+\t * we have no primary key. And without a username and password,\n+\t * we are not actually storing a credential.\n+\t */\n+\tif (!c->protocol || !(c->host || c->path) ||\n+\t !c->username || !c->password)\n+\t\treturn EXIT_FAILURE;\n+\n+\tlabel = make_label(c);\n+\tattributes = make_attr_list(c);\n+\tsecret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,\n+\t\t\t\t attributes,\n+\t\t\t\t NULL,\n+\t\t\t\t label,\n+\t\t\t\t c->password,\n+\t\t\t\t NULL,\n+\t\t\t\t &error);\n+\tg_free(label);\n+\tg_hash_table_unref(attributes);\n+\n+\tif (error != NULL) {\n+\t\tg_critical(\"store failed: %s\", error->message);\n+\t\tg_error_free(error);\n+\t\treturn EXIT_FAILURE;\n+\t}\n+\n+\treturn EXIT_SUCCESS;\n+}\n+\n+static int keyring_erase(struct credential *c)\n+{\n+\tGHashTable *attributes = NULL;\n+\tGError *error = NULL;\n+\n+\t/*\n+\t * Sanity check that we actually have something to match\n+\t * against. The input we get is a restrictive pattern,\n+\t * so technically a blank credential means \"erase everything\".\n+\t * But it is too easy to accidentally send this, since it is equivalent\n+\t * to empty input. So explicitly disallow it, and require that the\n+\t * pattern have some actual content to match.\n+\t */\n+\tif (!c->protocol && !c->host && !c->path && !c->username)\n+\t\treturn EXIT_FAILURE;\n+\n+\tattributes = make_attr_list(c);\n+\tsecret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,\n+\t\t\t\t attributes,\n+\t\t\t\t NULL,\n+\t\t\t\t &error);\n+\tg_hash_table_unref(attributes);\n+\n+\tif (error != NULL) {\n+\t\tg_critical(\"erase failed: %s\", error->message);\n+\t\tg_error_free(error);\n+\t\treturn EXIT_FAILURE;\n+\t}\n+\n+\treturn EXIT_SUCCESS;\n+}\n+\n+/*\n+ * Table with helper operation callbacks, used by generic\n+ * credential helper main function.\n+ */\n+static struct credential_operation const credential_helper_ops[] = {\n+\t{ \"get\", keyring_get },\n+\t{ \"store\", keyring_store },\n+\t{ \"erase\", keyring_erase },\n+\tCREDENTIAL_OP_END\n+};\n+\n+/* ------------------ credential functions ------------------ */\n+\n+static void credential_init(struct credential *c)\n+{\n+\tmemset(c, 0, sizeof(*c));\n+}\n+\n+static void credential_clear(struct credential *c)\n+{\n+\tg_free(c->protocol);\n+\tg_free(c->host);\n+\tg_free(c->path);\n+\tg_free(c->username);\n+\tg_free(c->password);\n+\n+\tcredential_init(c);\n+}\n+\n+static int credential_read(struct credential *c)\n+{\n+\tchar *buf;\n+\tsize_t line_len;\n+\tchar *key;\n+\tchar *value;\n+\n+\tkey = buf = g_malloc(1024);\n+\n+\twhile (fgets(buf, 1024, stdin)) {\n+\t\tline_len = strlen(buf);\n+\n+\t\tif (line_len && buf[line_len-1] == '\\n')\n+\t\t\tbuf[--line_len] = '\\0';\n+\n+\t\tif (!line_len)\n+\t\t\tbreak;\n+\n+\t\tvalue = strchr(buf, '=');\n+\t\tif (!value) {\n+\t\t\tg_warning(\"invalid credential line: %s\", key);\n+\t\t\tg_free(buf);\n+\t\t\treturn -1;\n+\t\t}\n+\t\t*value++ = '\\0';\n+\n+\t\tif (!strcmp(key, \"protocol\")) {\n+\t\t\tg_free(c->protocol);\n+\t\t\tc->protocol = g_strdup(value);\n+\t\t} else if (!strcmp(key, \"host\")) {\n+\t\t\tg_free(c->host);\n+\t\t\tc->host = g_strdup(value);\n+\t\t\tvalue = strrchr(c->host, ':');\n+\t\t\tif (value) {\n+\t\t\t\t*value++ = '\\0';\n+\t\t\t\tc->port = atoi(value);\n+\t\t\t}\n+\t\t} else if (!strcmp(key, \"path\")) {\n+\t\t\tg_free(c->path);\n+\t\t\tc->path = g_strdup(value);\n+\t\t} else if (!strcmp(key, \"username\")) {\n+\t\t\tg_free(c->username);\n+\t\t\tc->username = g_strdup(value);\n+\t\t} else if (!strcmp(key, \"password\")) {\n+\t\t\tg_free(c->password);\n+\t\t\tc->password = g_strdup(value);\n+\t\t\twhile (*value)\n+\t\t\t\t*value++ = '\\0';\n+\t\t}\n+\t\t/*\n+\t\t * Ignore other lines; we don't know what they mean, but\n+\t\t * this future-proofs us when later versions of git do\n+\t\t * learn new lines, and the helpers are updated to match.\n+\t\t */\n+\t}\n+\n+\tg_free(buf);\n+\n+\treturn 0;\n+}\n+\n+static void credential_write_item(FILE *fp, const char *key, const char *value)\n+{\n+\tif (!value)\n+\t\treturn;\n+\tfprintf(fp, \"%s=%s\\n\", key, value);\n+}\n+\n+static void credential_write(const struct credential *c)\n+{\n+\t/* only write username/password, if set */\n+\tcredential_write_item(stdout, \"username\", c->username);\n+\tcredential_write_item(stdout, \"password\", c->password);\n+}\n+\n+static void usage(const char *name)\n+{\n+\tstruct credential_operation const *try_op = credential_helper_ops;\n+\tconst char *basename = strrchr(name, '/');\n+\n+\tbasename = (basename) ? basename + 1 : name;\n+\tfprintf(stderr, \"usage: %s <\", basename);\n+\twhile (try_op->name) {\n+\t\tfprintf(stderr, \"%s\", (try_op++)->name);\n+\t\tif (try_op->name)\n+\t\t\tfprintf(stderr, \"%s\", \"|\");\n+\t}\n+\tfprintf(stderr, \"%s\", \">\\n\");\n+}\n+\n+int main(int argc, char *argv[])\n+{\n+\tint ret = EXIT_SUCCESS;\n+\n+\tstruct credential_operation const *try_op = credential_helper_ops;\n+\tstruct credential cred = CREDENTIAL_INIT;\n+\n+\tif (!argv[1]) {\n+\t\tusage(argv[0]);\n+\t\texit(EXIT_FAILURE);\n+\t}\n+\n+\tg_set_application_name(\"Git Credential Helper\");\n+\n+\t/* lookup operation callback */\n+\twhile (try_op->name && strcmp(argv[1], try_op->name))\n+\t\ttry_op++;\n+\n+\t/* unsupported operation given -- ignore silently */\n+\tif (!try_op->name || !try_op->op)\n+\t\tgoto out;\n+\n+\tret = credential_read(&cred);\n+\tif (ret)\n+\t\tgoto out;\n+\n+\t/* perform credential operation */\n+\tret = (*try_op->op)(&cred);\n+\n+\tcredential_write(&cred);\n+\n+out:\n+\tcredential_clear(&cred);\n+\treturn ret;\n+}\n-- \n2.10.0\n\n"},{"id":"303780","messageId":"20161010183410.dzuqxtms636euxbk@sigill.intra.peff.net","threadId":"44261","inReplyTo":"20161009123417.147239-1-grawity@gmail.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T18:34:10Z","receivedAt":"2016-10-10T18:34:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 09, 2016 at 03:34:17PM +0300, Mantas Mikulėnas wrote:\n\n> This is based on the existing gnome-keyring helper, but instead of\n> libgnome-keyring (which was specific to GNOME and is deprecated), it\n> uses libsecret which can support other implementations of XDG Secret\n> Service API.\n\nThanks for working on this; I'm happy to see credential helpers for as\nmany storage APIs as possible.\n\n> Passes t0303-credential-external.sh.\n\nThank you for running that, as my first question would have been about\nits results. :)\n\nI don't know much about the Secret Service API, nor do I run a desktop\nenvironment which provides the server side of it. But the code looks\nstraightforward and reasonable, it passes t0303, and presumably it is\nworking for you. So this seems worth merging to me.\n\n-Peff\n"},{"id":"303800","messageId":"1476130850.7457.8.camel@kaarsemaker.net","threadId":"44261","inReplyTo":"20161009123417.147239-1-grawity@gmail.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-10T20:20:50Z","receivedAt":"2016-10-10T20:20:58Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:\n> This is based on the existing gnome-keyring helper, but instead of\n> libgnome-keyring (which was specific to GNOME and is deprecated), it\n> uses libsecret which can support other implementations of XDG Secret\n> Service API.\n> \n> Passes t0303-credential-external.sh.\n\nWhen setting credential.helper to this helper, I get the following output:\n\n$ git clone https://private-repo-url-removed private\nCloning into 'private'...\n/home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied\n\nLooks suboptimal. Am I holding it wrong?\n\nD.\n"},{"id":"303802","messageId":"20161010204654.krj44nk6xbjh4t2v@sigill.intra.peff.net","threadId":"44261","inReplyTo":"1476130850.7457.8.camel@kaarsemaker.net","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-10T20:46:54Z","receivedAt":"2016-10-10T20:47:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:\n\n> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:\n> > This is based on the existing gnome-keyring helper, but instead of\n> > libgnome-keyring (which was specific to GNOME and is deprecated), it\n> > uses libsecret which can support other implementations of XDG Secret\n> > Service API.\n> > \n> > Passes t0303-credential-external.sh.\n> \n> When setting credential.helper to this helper, I get the following output:\n> \n> $ git clone https://private-repo-url-removed private\n> Cloning into 'private'...\n> /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied\n> \n> Looks suboptimal. Am I holding it wrong?\n\nThat looks like a directory name in your error message. How did you set\nup credential.helper? I'd expect normal usage to be something like this:\n\n # do this once, or cp the binary into your $PATH\n PATH=$PATH:/home/dennis/code/git/contrib/credential/libsecret\n git config --global credential.helper libsecret\n\nBut if you don't want to put it in your PATH, then I think:\n\n git config --global credential.helper \\\n '!/home/dennis/code/git/contrib/credential/git-credential-libsecret'\n\nwould work.\n\n-Peff\n"},{"id":"303872","messageId":"1476198080.3876.8.camel@kaarsemaker.net","threadId":"44261","inReplyTo":"20161009123417.147239-1-grawity@gmail.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-11T15:01:20Z","receivedAt":"2016-10-11T15:07:32Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:\n\n> +\t\ts = g_hash_table_lookup(attributes, \"user\");\n> +\t\tif (s) {\n> +\t\t\tg_free(c->username);\n> +\t\t\tc->username = g_strdup(s);\n> +\t\t}\n\nThis always overwrites c->username, the original gnome-keyring version\nonly does that when the username isn't set. Other than that it looks\ngood to me.\n\nReviewed-by: Dennis Kaarsemaker \nTested-by: Dennis Kaarsemaker \n\nD.\n"},{"id":"303874","messageId":"1476198071.3876.7.camel@kaarsemaker.net","threadId":"44261","inReplyTo":"20161010204654.krj44nk6xbjh4t2v@sigill.intra.peff.net","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-11T15:01:11Z","receivedAt":"2016-10-11T15:20:07Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Mon, 2016-10-10 at 16:46 -0400, Jeff King wrote:\n> On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:\n> \n> > On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:\n> > > This is based on the existing gnome-keyring helper, but instead of\n> > > libgnome-keyring (which was specific to GNOME and is deprecated), it\n> > > uses libsecret which can support other implementations of XDG Secret\n> > > Service API.\n> > > \n> > > Passes t0303-credential-external.sh.\n> > \n> > When setting credential.helper to this helper, I get the following output:\n> > \n> > $ git clone https://private-repo-url-removed private\n> > Cloning into 'private'...\n> > /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied\n> > \n> > Looks suboptimal. Am I holding it wrong?\n> \n> That looks like a directory name in your error message. How did you set\n> up credential.helper?\n\nDoh.\n\nI had done git config --global credential.helper ~/code/git/contrib/credential/libsecret/\n\nAfter doing it properly (actual path to the binary), it works just\nfine. The error message above is slightly suboptimal, but there's no\nsolution for pebkac.\n\nD.\n\n\n\n\n"},{"id":"303914","messageId":"xmqqoa2q8ypl.fsf@gitster.mtv.corp.google.com","threadId":"44261","inReplyTo":"1476198080.3876.8.camel@kaarsemaker.net","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-11T19:36:38Z","receivedAt":"2016-10-11T19:36:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:\n>\n>> +\t\ts = g_hash_table_lookup(attributes, \"user\");\n>> +\t\tif (s) {\n>> +\t\t\tg_free(c->username);\n>> +\t\t\tc->username = g_strdup(s);\n>> +\t\t}\n>\n> This always overwrites c->username, the original gnome-keyring version\n> only does that when the username isn't set. Other than that it looks\n> good to me.\n>\n> Reviewed-by: Dennis Kaarsemaker \n> Tested-by: Dennis Kaarsemaker \n\nThanks for a review. I'll wait until one of (1) a squashable patch\nto address the \"we do not want unconditional overwrite\" issue, (2) a\nreroll from Mantas to do the same, or (3) a counter-argument from\nsomebody to explain why unconditional overwrite is a good idea here\n(but not in the original) appears.\n\n\n"},{"id":"303917","messageId":"c87e4dd4-7253-d7c2-010b-6d8c7f587093@gmail.com","threadId":"44261","inReplyTo":"xmqqoa2q8ypl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2016-10-11T19:48:29Z","receivedAt":"2016-10-11T19:54:28Z","isPatch":true,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"On 2016-10-11 22:36, Junio C Hamano wrote:\n> Thanks for a review. I'll wait until one of (1) a squashable patch\n> to address the \"we do not want unconditional overwrite\" issue, (2) a\n> reroll from Mantas to do the same, or (3) a counter-argument from\n> somebody to explain why unconditional overwrite is a good idea here\n> (but not in the original) appears.\n\nI overlooked that. I can write a patch, but it shouldn't make any\ndifference in practice – if c->username *was* set, then it would also\nget added to the search attribute list, therefore the search couldn't\npossibly return any results with a different username anyway.\n\n-- \nMantas Mikulėnas \n"},{"id":"303918","messageId":"61eb8570-52bc-0059-9c81-ad1d66da12ac@gmail.com","threadId":"44261","inReplyTo":"c87e4dd4-7253-d7c2-010b-6d8c7f587093@gmail.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Mantas Mikulėnas","fromEmail":"grawity@gmail.com","sentAt":"2016-10-11T19:59:17Z","receivedAt":"2016-10-11T19:59:27Z","isPatch":true,"sender":{"key":"grawity@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31021?v=4"},"body":"On 2016-10-11 22:48, Mantas Mikulėnas wrote:\n> On 2016-10-11 22:36, Junio C Hamano wrote:\n>> Thanks for a review. I'll wait until one of (1) a squashable patch\n>> to address the \"we do not want unconditional overwrite\" issue, (2) a\n>> reroll from Mantas to do the same, or (3) a counter-argument from\n>> somebody to explain why unconditional overwrite is a good idea here\n>> (but not in the original) appears.\n> \n> I overlooked that. I can write a patch, but it shouldn't make any\n> difference in practice – if c->username *was* set, then it would also\n> get added to the search attribute list, therefore the search couldn't\n> possibly return any results with a different username anyway.\n\nOn a second thought, it doesn't actually make sense _not_ to override\nthe username. Let's say the search query *somehow* returned a different\naccount than requested. With the original (gnome-keyring helper's)\nbehavior, the final output would have the old account's username, but\nthe new account's password – which has very little chance of working.\n\n-- \nMantas Mikulėnas \n"},{"id":"303920","messageId":"1476216585.3876.10.camel@kaarsemaker.net","threadId":"44261","inReplyTo":"c87e4dd4-7253-d7c2-010b-6d8c7f587093@gmail.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-11T20:09:45Z","receivedAt":"2016-10-11T20:09:54Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:\n> On 2016-10-11 22:36, Junio C Hamano wrote:\n> > Thanks for a review. I'll wait until one of (1) a squashable patch\n> > to address the \"we do not want unconditional overwrite\" issue, (2) a\n> > reroll from Mantas to do the same, or (3) a counter-argument from\n> > somebody to explain why unconditional overwrite is a good idea here\n> > (but not in the original) appears.\n> \n> \n> I overlooked that. I can write a patch, but it shouldn't make any\n> difference in practice – if c->username *was* set, then it would also\n> get added to the search attribute list, therefore the search couldn't\n> possibly return any results with a different username anyway.\n\nMakes sense, so a (3) it is.\n\nD.\n"},{"id":"303922","messageId":"xmqqa8ea8wzy.fsf@gitster.mtv.corp.google.com","threadId":"44261","inReplyTo":"1476216585.3876.10.camel@kaarsemaker.net","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-11T20:13:37Z","receivedAt":"2016-10-11T20:14:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker writes:\n\n> On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:\n>> On 2016-10-11 22:36, Junio C Hamano wrote:\n>> > Thanks for a review. I'll wait until one of (1) a squashable patch\n>> > to address the \"we do not want unconditional overwrite\" issue, (2) a\n>> > reroll from Mantas to do the same, or (3) a counter-argument from\n>> > somebody to explain why unconditional overwrite is a good idea here\n>> > (but not in the original) appears.\n>> \n>> \n>> I overlooked that. I can write a patch, but it shouldn't make any\n>> difference in practice – if c->username *was* set, then it would also\n>> get added to the search attribute list, therefore the search couldn't\n>> possibly return any results with a different username anyway.\n>\n> Makes sense, so a (3) it is.\n\nSo... does it mean the gnome-keyring one needs a bugfix?\n"},{"id":"303923","messageId":"xmqq60oy8wq7.fsf@gitster.mtv.corp.google.com","threadId":"44261","inReplyTo":"xmqqa8ea8wzy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-11T20:19:28Z","receivedAt":"2016-10-11T20:19:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Dennis Kaarsemaker writes:\n>\n>> On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:\n>>> On 2016-10-11 22:36, Junio C Hamano wrote:\n>>> > Thanks for a review. I'll wait until one of (1) a squashable patch\n>>> > to address the \"we do not want unconditional overwrite\" issue, (2) a\n>>> > reroll from Mantas to do the same, or (3) a counter-argument from\n>>> > somebody to explain why unconditional overwrite is a good idea here\n>>> > (but not in the original) appears.\n>>> \n>>> \n>>> I overlooked that. I can write a patch, but it shouldn't make any\n>>> difference in practice – if c->username *was* set, then it would also\n>>> get added to the search attribute list, therefore the search couldn't\n>>> possibly return any results with a different username anyway.\n>>\n>> Makes sense, so a (3) it is.\n>\n> So... does it mean the gnome-keyring one needs a bugfix?\n\nJust so there is no misunderstanding, updating (or not)\ngnome-keyring code is an unrelated issue. \n\nI'll queue the patch under discussion in this thread, and if an\nupdate to gnome-keyring appears, that will be queued separately.\n\nThanks again, both of you.\n"},{"id":"303924","messageId":"1476217934.3876.12.camel@kaarsemaker.net","threadId":"44261","inReplyTo":"xmqqa8ea8wzy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] contrib: add credential helper for libsecret","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2016-10-11T20:32:14Z","receivedAt":"2016-10-11T20:32:21Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On Tue, 2016-10-11 at 13:13 -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker writes:\n> \n> > On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:\n> > > On 2016-10-11 22:36, Junio C Hamano wrote:\n> > > > Thanks for a review. I'll wait until one of (1) a squashable patch\n> > > > to address the \"we do not want unconditional overwrite\" issue, (2) a\n> > > > reroll from Mantas to do the same, or (3) a counter-argument from\n> > > > somebody to explain why unconditional overwrite is a good idea here\n> > > > (but not in the original) appears.\n> > > \n> > > \n> > > \n> > > I overlooked that. I can write a patch, but it shouldn't make any\n> > > difference in practice – if c->username *was* set, then it would also\n> > > get added to the search attribute list, therefore the search couldn't\n> > > possibly return any results with a different username anyway.\n> > \n> > \n> > Makes sense, so a (3) it is.\n> \n> \n> So... does it mean the gnome-keyring one needs a bugfix?\n\nI'd say both behaviours are correct.\n\nWhen you do a search without a username, both helpers fill in the\nusername returned by the actual credential storage. When you do a\nsearch with a username, the gnome-keyring helper won't overwrite the\nvalue passed in and the libsecret helper overwrites it with the same\nvalue, as the search can only return matches that have the same value.\n\nD.\n"}]}