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

From: Jeff King <peff@peff.net>
Date: Mar 14, 2015, 17:42 UTC
Message-ID: <20150314174204.GA32706@peff.net>
In-Reply-To: <20150314173328.GA32599@peff.net>

On Sat, Mar 14, 2015 at 01:33:28PM -0400, Jeff King wrote:

Show 14 quoted lines

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

By the way, I hope this (and the other comment) do not come off as "you are wrong, but I do not feel like arguing with you". I really do think these are a matter of taste, and while we often express issues of taste in reviews, it is ultimately up to the patch submitter (who is, after all, doing most of the work) to have the final say on minor issues like this. Sometimes the response to taste issue is "oh, I didn't think to do that, thanks for the suggestion" and sometimes it is "nah, I like it better my way". And both are OK.

Of course there are also taste issues where we insist (e.g., consistent whitespace), but I do not think this is one of them. :)

Maybe that was all obvious, but since you are new to the list, I wanted to make sure I was clear.

-Peff