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

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.

quoted

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