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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
