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

From: Paul Tan <pyokagan@gmail.com>
Date: Mar 14, 2015, 08:15 UTC
Message-ID: <CACRoPnR9pTc2LC87Vf0bMAgTj-FnbsRBpjn+3RCxCP6yrzsCkw@mail.gmail.com>
In-Reply-To: <20150313061530.GA24588@peff.net>

On Fri, Mar 13, 2015 at 2:15 PM, Jeff King <peff@peff.net> wrote:

Show 7 quoted lines

>> +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.

Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of "the first file in the list is the default" in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. It's a balance of flexibility, but in this case I think putting the default filename in a separate argument is a good thing.

Show 11 quoted lines

>> +     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).

Actually, thinking about it again from a memory management perspective, STRING_LIST_INIT_DUP should be used instead as the string_list `fns` should own the memory of the strings inside it. Thus, in this case since `file` is pulled from the argv array, string_list_append() should be used to duplicate the memory, and for expand_user_path(), string_list_append_nodup() should be used because the returned path is newly-allocated memory. Finally, string_list_clear() should be called at the end to release memory.

Thanks for taking the time to review the patches, I will work on v4 now. (Really keen on getting this to pu)

Regards, Paul