Matthieu Moy <matthieu....@grenoble-inp.fr> writes: > Junio C Hamano <gits...@pobox.com> writes: > >> Paul Tan <pyoka...@gmail.com> writes: >> >>>> 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. >> >> I am not sure if this is not a premature over-engineering > > I would say so if having this default_fn made the code more complex,
True, or if it made caller(s) be redundant or repeat themselves without a good reason. Otherwise we would end up having to drop the redundant and/or unnecessary arguments in future clean-up patches; I had to de-conflict a series with 7ce7c760 (convert: drop arguments other than 'path' from would_convert_to_git(), 2014-08-21) recently, which reminded me of this point ;-). > but > here the code is basically > > + if (default_fn) > + store_credential_file(default_fn, c); > > and > > - store_credential(file, &c); > + store_credential(&fns, &c, fns.items[0].string); > > Taking the first element in the list wouldn't change much. > > I'm personally fine with both versions. Turning the current code to drop the extra parameter and to use the first element instead wouldn't be a big change, but these things tend to add up, so unless this discussion immediately lead to a future enhancement plan to make use of the flexibility the parameter gives us, I'd rather see things kept simpler. I do not terribly mind either way, either, though. -- 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
