From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 11 Oct 2016 19:36:38 GMT
Subject: Re: [PATCH] contrib: add credential helper for libsecret
Message-ID: <xmqqoa2q8ypl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1476198080.3876.8.camel@kaarsemaker.net>

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
>
>> +		s = g_hash_table_lookup(attributes, "user");
>> +		if (s) {
>> +			g_free(c->username);
>> +			c->username = g_strdup(s);
>> +		}
>
> This always overwrites c->username, the original gnome-keyring version
> only does that when the username isn't set. Other than that it looks
> good to me.
>
> Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> Tested-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>

Thanks for a review.  I'll wait until one of (1) a squashable patch
to address the "we do not want unconditional overwrite" issue, (2) a
reroll from Mantas to do the same, or (3) a counter-argument from
somebody to explain why unconditional overwrite is a good idea here
(but not in the original) appears.