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

[Jeff King <peff@peff.net>]

On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote:

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

Yeah, I see your line of reasoning. I think this is probably a case of
YAGNI, but it is really a matter of personal preference. It's not a big
deal either way.

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

Yeah, I had the same thought, but I noticed that you don't call
string_list_clear. Nor is it really necessary to do so. Since git
programs are generally short-lived, we often let the OS take care of
deallocating variables like this (it's not appropriate for all
variables, of course, but quite frequently there are variables that are
effectively allocated at program startup and used until program end).

Again, I think this is a matter of taste. I don't mind if you want to
string_list_clear at the end, and I agree that using nodup() is the
right thing in that case.

-Peff