Re: [PATCH v3 1/4] git-credential-store: support multiple credential files

From: Jeff King <peff@peff.net>
Date: Mar 13, 2015, 06:15 UTC
Message-ID: <20150313061530.GA24588@peff.net>
In-Reply-To: <1426056553-9364-2-git-send-email-pyokagan@gmail.com>

On Wed, Mar 11, 2015 at 02:49:10PM +0800, Paul Tan wrote:

Show 6 quoted lines

> Previously, git-credential-store only supported storing credentials in a
> single file: ~/.git-credentials. In order to support the XDG base
> directory specification[1], git-credential-store needs to be able to
> lookup and erase credentials from multiple files, as well as to pick the
> appropriate file to write to so that the credentials can be found on
> subsequent lookups.

I looked over the code here and in the second patch, and I didn't see any real problems. Thanks for iterating on this.

Two minor nits, that might not even be worth addressing:

> +static void store_credential(const struct string_list *fns, struct credential *c,
> +			     const char *default_fn)

I think you could even get away without passing default_fn here, and just use the rule "the first file in the list is the default". Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful.

> +	struct string_list fns = STRING_LIST_INIT_NODUP;

This is declared NODUP...

> -	if (!file)
> +	if (file)
> +		string_list_append_nodup(&fns, file);

So you can just call the regular string_list_append here (the idea of declaring the list as DUP or NODUP is that the callers do not have to care; string_list_append does the right thing).

-Peff